Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests and updates for addition/deletion of metadata labels #3354

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

rtibbles
Copy link
Member

Summary

Description of the change(s) you made

  • Updates the metadata labels to not squash previous edits with new changes
  • Updates handling of JSONFieldDictSerializer to remove keys when they are set as None to match the expected behaviour from Dexie generated update events.
  • Adds regression tests for this behaviour

Manual verification steps performed

Ran the new tests!

@rtibbles rtibbles requested a review from bjester March 28, 2022 21:34
else:
raise ValidationError("Tried to access a dot path in an invalid type")
# Finally, delete the final key
obj.pop(key.split(".")[-1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an intense chunk of code. The key splitting, object resolution piece seems like it could be a utility function? Is it something we do elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have something not dissimilar on the client side, I think we could turn it into a utility function, but wanted to keep it constrained for now until we work out exactly how we'd want it to be generalizable.

@rtibbles rtibbles merged commit f78ab9a into learningequality:unstable Apr 13, 2022
@rtibbles rtibbles deleted the dotpath_deletion branch April 13, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants