Skip to content

Allow default model manager to be overridden without affecting syncing behaviour - #269

Merged
rtibbles merged 3 commits into
learningequality:release-v0.8.xfrom
rtibbles:syncable_model_manager
Sep 23, 2025
Merged

Allow default model manager to be overridden without affecting syncing behaviour#269
rtibbles merged 3 commits into
learningequality:release-v0.8.xfrom
rtibbles:syncable_model_manager

Conversation

@rtibbles

@rtibbles rtibbles commented Sep 23, 2025

Copy link
Copy Markdown
Member

Summary

Adds a new syncable_objects manager that is used during sync operations
Updates all serialization and deserialization to use the syncable_objects managers instead of objects
Adds tests to assert the behaviour of the syncable_objects manager, serialization, and deserialization with a model with a modified default manager
Bumps Morango version for release.
Adds additional conditional statements to the SQLite migration action, because we weren't skipping all the steps, which caused them to error if skipped.

TODO

  • Have tests been written for the new code?

Reviewer guidance

This is being done to allow us to override objects on the FacilityUser model in Kolibri, and for any other models we might need it for in future!

I am also realizing, that overriding syncable objects could be an easy way to handle draft objects, like draft quizzes in Kolibri - where we only let objects be made available for syncing once a flag has been set.

All library code changes were done by hand. Additional tests were generated by Claude Code, then I updated the assertions it was making (including deleting superfluous ones).

@bjester bjester left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there's one more place we should update https://github.com/learningequality/morango/blob/release-v0.8.x/morango/models/core.py#L476

My other thought was whether having a separate manager instance was valuable if we only use it twice. My initial thought, without knowing exactly how Kolibri's override filtering works, was towards whether simply using .all() to reset the filters in important use cases would work? That said, I'm fine with this approach, because it's abundantly clear in the code the reason for it, when something like .all() could get erroneously deleted.

@rtibbles

Copy link
Copy Markdown
Member Author

I don't think .all actually clears any existing filters - it just returns a copy of the current queryset, filters and all: https://docs.djangoproject.com/en/3.2/ref/models/querysets/#all

Did you mean this: https://github.com/learningequality/morango/blob/release-v0.8.x/morango/models/core.py#L476, because I already updated that line - or is there somewhere else that I have missed and this got pasted instead?

@bjester

bjester commented Sep 23, 2025

Copy link
Copy Markdown
Member

I don't think .all actually clears any existing filters - it just returns a copy of the current queryset, filters and all: https://docs.djangoproject.com/en/3.2/ref/models/querysets/#all

Well that's a big misnomer 🤦

@rtibbles
rtibbles merged commit 0e29835 into learningequality:release-v0.8.x Sep 23, 2025
22 checks passed
@rtibbles
rtibbles deleted the syncable_model_manager branch September 23, 2025 16:29
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