Reset state fix - #11422
Conversation
c5a68c3 to
bea33d1
Compare
There was a problem hiding this comment.
This branch label is just for sandbox testing; I'll update it with a properly-released new edx-submissions version before merging to master.
bc138d5 to
02e4263
Compare
There was a problem hiding this comment.
Since platform no longer calls into submissions directly, this test no longer belongs here. I'll add it to edx-ora2 as part of the sibling change,
fbaf324 to
74adfb4
Compare
| anonymous_id_for_user(student, course_id), | ||
| course_id.to_deprecated_string(), | ||
| module_state_key.to_deprecated_string(), | ||
| ) |
There was a problem hiding this comment.
ORA2 is not the only thing that uses submissions, though the number of third party blocks that do is limited. Submissions was meant to be an API usable by other XBlocks, for storing more easily audited submission and scoring information, especially across users.
d01212b to
695c383
Compare
|
jenkins run bokchoy |
|
jenkins run python |
| -e git+https://github.com/edx/edx-ora2.git@0.2.7#egg=ora2==0.2.7 | ||
| -e git+https://github.com/edx/edx-submissions.git@0.1.3#egg=edx-submissions==0.1.3 | ||
| -e git+https://github.com/edx/edx-ora2.git@efischer/clear_submission#egg=ora2 | ||
| -e git+https://github.com/edx/edx-submissions.git@efischer/soft_delete_sub#egg=edx-submissions |
There was a problem hiding this comment.
These will be updated to proper release versions before merging this PR, this was just to get my sandbox set up.
|
This PR is ready for review. @cahrens and @dianakhuang, could you look it over when you get a chance? I've run this LMS->xblock communication approach by the Arch Council, they are on board with using it here but have some further discussion regarding documentation that don't need to block this. The sibling change for ORA is also ready for review, but the edx-submissions one is still undergoing devops vetting. Regardless of what we do there, the API connection point to ORA will remain the same. The behavior can be tested at https://efischer19.sandbox.edx.org/ |
| if callable(clear_submission): | ||
| clear_submission( | ||
| user_id=user_id, | ||
| course_id=course_id.to_deprecated_string(), |
There was a problem hiding this comment.
Don't use to_deprecated_string. You should be able to do unicode(course_id).
Same thing below.
|
@robrap and @dianakhuang This final piece of the puzzle is also ready to be reviewed at your convenience. I'm working to get all 3 in-flight PRs onto my sandbox for testing. |
|
https://efischer19.sandbox.edx.org/ is updated with the latest version of this PR and the code specified in the branches listed in |
| pass | ||
| # Some blocks (openassessment) use StudentModule data as a key for internal submission data. | ||
| # Inform these blocks of the reset and allow them to handle their data. | ||
| clear_submission = getattr(block, "clear_submission", None) |
There was a problem hiding this comment.
As discussed, change this to "clear_student_state".
There was a problem hiding this comment.
And this needs to be wrapped with an if delete_module:.
a6ae0d1 to
23b7b1b
Compare
|
@robrap I've addressed your comments and squashed, would you mind re-reviewing? |
There was a problem hiding this comment.
Can you change this to something like:
# TODO: Remove this once we've finalized and communicated how xblocks should handle clear_student_state
# and made sure that other xblocks relying on the submission api understand this is going away.
|
@efischer19 Can you make the one comment change as part of your squash? I don't need to review again. 👍 once this is done. Thanks! |
23b7b1b to
9a9acb5
Compare
|
Thanks for the suggestion @robrap, I've added that |
5 participants
The fix here will be resolved by a change to the submissions API,
and this code will need to call the new method.
TNL-3880
Sibling changes:
Clear state option edx-submissions#34Now looking at Move "deleted" submissions to separate table edx-submissions#36Soft-delete Submissions to reset student state edx-submissions#33