Clear state option - #34
Conversation
To fix the buggy behavior reported in TNL-3880, we need to ensure that the submission we're dropping is unlinked from the student_item used to find it. This prevents code (say, an ORA staff tool) from constructing a student item with an id, a course, and a problem, and using that to find a "cleared" submission.
PR ChecklistSandbox
Testing
ReviewersIf you've been tagged for review, please check your corresponding box once you've given the 👍.
Post-review
|
Turns out you have to let the database know that a value can be None, who knew? Migration included.
|
@dianakhuang @cahrens This simpler fix is ready for an initial review (not final, still some open questions to answer). What I misunderstood originally was that we aren't actually deleting anything, we're just orphaning submissions by having a student's module state forget the submission_uuid. However, staff can construct a perfectly valid lookup triplet of (student, course, problem), use that to query the submissions API, and get a (supposedly "orphaned") submission_uuid back. This fix will now eliminate the key that made such lookups possible, which prevents staff from seeing the individual submission even if they do construct the lookup triplet again. This solves the problem of staff being able to see deleted items with the What this does not address is the aggregate data in the |
|
@maxrothman what's the devops impact of an AlterField migration? We're just removing a |
|
To go along with the So it looks like we will need to inform ORA of the removal somehow, so it can clear out any items that refer to the cleared submission. I'll keep working on that. |
| name='student_item', | ||
| field=models.ForeignKey(to='submissions.StudentItem', null=True), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
One potential issue I've discovered during dev testing - trying to reverse this migration after NULL values have been added results in an error, so I'll have to figure that out, at least.
django.db.utils.OperationalError: (1138, 'Invalid use of NULL value').
|
I've updated this PR and it's siblings to inform openassessment of the clear operation, and it looks like it's working. @dianakhuang and/or @cahrens, could we start going over this in a bit more detail when we get a chance? I've installed all of this code on https://efischer19.sandbox.edx.org/ if you want to test it out. |
| field=models.ForeignKey(to='submissions.StudentItem', null=True, default=1), | ||
| preserve_default=False | ||
| ), | ||
| ] |
There was a problem hiding this comment.
@maxrothman Here's some data from running this migration on my devstack with ~1400 rows of data, the read-replica indicates ~670,000 on production
https://gist.github.com/efischer19/b56b9d14768de28cf612
There was a problem hiding this comment.
@efischer19 Do you feel those results are representative enough to indicate lack of an issue with ~100x rows?
There was a problem hiding this comment.
I think so. The two longest lines seem to be these, what else should we be considering when extrapolating?
DEBUG:django.db.backends:(0.114) ALTER TABLE `submissions_submission` MODIFY `student_item_id` integer NULL; args=[]
DEBUG:django.db.backends:(0.130) ALTER TABLE `submissions_submission` ADD CONSTRAINT `su_student_item_id_d3801ff833d05b1_fk_submissions_studentitem_id` FOREIGN KEY (`student_item_id`) REFERENCES `submissions_studentitem` (`id`); args=[]
There's also now a possibility of avoiding migrations entirely if the approach in #35 wins out as our best option.
4 participants
To fix the buggy behavior reported in TNL-3880, we need to ensure
that the submission we're dropping is unlinked from the student_item
used to find it. This prevents code (say, an ORA staff tool) from
constructing a student item with an id, a course, and a problem, and
using that to find a "cleared" submission.
TNL-3880
Sibling changes: