-
Notifications
You must be signed in to change notification settings - Fork 716
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
Handle mismatching datasets for exams and lessons to prevent syncing issues #8513
Handle mismatching datasets for exams and lessons to prevent syncing issues #8513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to read a bit harder to parse what was happening in the upgrade steps - but nothing blocking. Code makes sense and tests cover it!
], | ||
output_field=UUIDField(), | ||
) | ||
Exam.objects.exclude(collection__dataset_id=F("dataset_id")).update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took me a couple of reads to get my head around. I think I understand what is going on though.
The dataset_id of the Exam has previously been set from the creator
, and so if there is a mismatch between the dataset_id of the Exam and the dataset_id of the collection it belongs to, then we have discovered our problem exams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the dataset inference was changed in #7887 so this migrates that change, like you said where the collection dataset doesn't match the exam's. This wasn't as simple as I wanted, and as simple as the query could have been, but Django constrained me a little bit.
) | ||
|
||
# un-set assigned_by for all exam assignments assigned by a user in a different dataset | ||
assignment_sub_query = Subquery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this is now relying on the migration above having happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good note, it does depend on that to achieve the end result we want
Background
Lesson and exam models have foreign references to the user model, and if those models are created by a user who was in a different facility (like a superuser), then the models would fail to deserialize when synced
Summary
creator
,created_by
, andassigned_by
nullableReferences
Resolves: #7453
Builds off: #7887
Ref: #7859
Reviewer guidance
…
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)