Reset state fix #11422

efischer19 · 2016-02-04T14:25:27Z

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 Submission edx-ora2#863
~~Clear state option edx-submissions#34~~ ~~Now looking at Move "deleted" submissions to separate table edx-submissions#36~~ Soft-delete Submissions to reset student state edx-submissions#33

efischer19 · 2016-02-04T15:43:57Z

requirements/edx/github.txt

@@ -79,7 +79,7 @@ git+https://github.com/edx/XBlock.git@xblock-0.4.4#egg=XBlock==0.4.4
 -e git+https://github.com/edx/django-splash.git@v0.2#egg=django-splash==0.2
 -e git+https://github.com/edx/acid-block.git@e46f9cda8a03e121a00c7e347084d142d22ebfb7#egg=acid-xblock
 -e git+https://github.com/edx/edx-ora2.git@0.2.6#egg=ora2==0.2.6
-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-submissions.git@efischer/clear_state#egg=edx-submissions


This branch label is just for sandbox testing; I'll update it with a properly-released new edx-submissions version before merging to master.

efischer19 · 2016-02-08T18:11:30Z

lms/djangoapps/instructor/tests/test_enrollment.py

-        # Verify that the student's scores have been reset in the submissions API
-        score = sub_api.get_score(student_item)
-        self.assertIs(score, None)
-


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,

ormsbee · 2016-02-09T14:41:57Z

lms/djangoapps/instructor/enrollment.py

-            anonymous_id_for_user(student, course_id),
-            course_id.to_deprecated_string(),
-            module_state_key.to_deprecated_string(),
-        )


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.

efischer19 · 2016-02-10T17:21:25Z

jenkins run bokchoy

efischer19 · 2016-02-10T17:21:30Z

jenkins run python

efischer19 · 2016-02-10T19:08:25Z

requirements/edx/github.txt

-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


These will be updated to proper release versions before merging this PR, this was just to get my sandbox set up.

efischer19 · 2016-02-10T20:42:21Z

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/

cahrens · 2016-02-10T20:50:24Z

lms/djangoapps/instructor/enrollment.py

+        if callable(clear_submission):
+            clear_submission(
+                user_id=user_id,
+                course_id=course_id.to_deprecated_string(),


Don't use to_deprecated_string. You should be able to do unicode(course_id).

Same thing below.

efischer19 · 2016-02-12T18:12:21Z

@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.

efischer19 · 2016-02-12T18:36:15Z

https://efischer19.sandbox.edx.org/ is updated with the latest version of this PR and the code specified in the branches listed in github.txt.

robrap · 2016-02-12T21:02:52Z

lms/djangoapps/instructor/enrollment.py

@@ -229,16 +231,25 @@ def reset_student_attempts(course_id, student, module_state_key, delete_module=F
                except StudentModule.DoesNotExist:
                    # If a particular child doesn't have any state, no big deal, as long as the parent does.
                    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)


As discussed, change this to "clear_student_state".

And this needs to be wrapped with an if delete_module:.

efischer19 · 2016-02-17T14:35:12Z

@robrap I've addressed your comments and squashed, would you mind re-reviewing?

robrap · 2016-02-17T15:52:27Z

lms/djangoapps/instructor/enrollment.py

-    # Reset the student's score in the submissions API
-    # Currently this is used only by open assessment (ORA 2)
+    # Reset the student's score in the submissions API, if xblock.clear_student_state has not done so already
+    # We want to remove this, once we've finalized and communicated how xblocks should handle clear_student_state


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.

robrap · 2016-02-17T15:53:50Z

@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!

efischer19 · 2016-02-17T15:56:45Z

Thanks for the suggestion @robrap, I've added that TODO as a comment.

dianakhuang · 2016-02-17T16:36:43Z

👍 from me as well.

The openassessment xblock stores some information that assumes a given student module is never cleared, it needs to be informed of this operation.

efischer19 · 2016-02-18T21:04:54Z

jenkins run all

Reset state fix

efischer19 force-pushed the efischer/reset_state_fix branch from c5a68c3 to bea33d1 Compare February 4, 2016 14:39

efischer19 mentioned this pull request Feb 4, 2016

Clear state option openedx/edx-submissions#34

Closed

efischer19 reviewed Feb 4, 2016
View reviewed changes

efischer19 mentioned this pull request Feb 4, 2016

Clear Submission openedx/edx-ora2#863

Merged

efischer19 force-pushed the efischer/reset_state_fix branch 2 times, most recently from bc138d5 to 02e4263 Compare February 5, 2016 20:43

efischer19 mentioned this pull request Feb 8, 2016

Mangle key to orphan data openedx/edx-submissions#35

Closed

efischer19 reviewed Feb 8, 2016
View reviewed changes

efischer19 force-pushed the efischer/reset_state_fix branch from fbaf324 to 74adfb4 Compare February 8, 2016 20:35

ormsbee reviewed Feb 9, 2016
View reviewed changes

efischer19 force-pushed the efischer/reset_state_fix branch 3 times, most recently from d01212b to 695c383 Compare February 10, 2016 14:39

efischer19 reviewed Feb 10, 2016
View reviewed changes

cahrens reviewed Feb 10, 2016
View reviewed changes

robrap reviewed Feb 12, 2016
View reviewed changes

efischer19 force-pushed the efischer/reset_state_fix branch 2 times, most recently from a6ae0d1 to 23b7b1b Compare February 17, 2016 14:34

robrap reviewed Feb 17, 2016
View reviewed changes

efischer19 force-pushed the efischer/reset_state_fix branch from 23b7b1b to 9a9acb5 Compare February 17, 2016 15:55

efischer19 mentioned this pull request Feb 18, 2016

0.2.8 Version Bump openedx/edx-ora2#876

Merged

Inform openassessment to clear submission

920cc3d

The openassessment xblock stores some information that assumes a given student module is never cleared, it needs to be informed of this operation.

efischer19 force-pushed the efischer/reset_state_fix branch from 9a9acb5 to 920cc3d Compare February 18, 2016 20:41

efischer19 pushed a commit that referenced this pull request Feb 18, 2016

Merge pull request #11422 from edx/efischer/reset_state_fix

09d0f43

Reset state fix

efischer19 merged commit 09d0f43 into master Feb 18, 2016

efischer19 deleted the efischer/reset_state_fix branch February 25, 2016 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset state fix #11422

Reset state fix #11422

efischer19 commented Feb 4, 2016

efischer19 Feb 4, 2016

efischer19 Feb 8, 2016

ormsbee Feb 9, 2016

efischer19 commented Feb 10, 2016

efischer19 commented Feb 10, 2016

efischer19 Feb 10, 2016

efischer19 commented Feb 10, 2016

cahrens Feb 10, 2016

efischer19 commented Feb 12, 2016

efischer19 commented Feb 12, 2016

robrap Feb 12, 2016

robrap Feb 12, 2016

efischer19 commented Feb 17, 2016

robrap Feb 17, 2016

robrap commented Feb 17, 2016

efischer19 commented Feb 17, 2016

dianakhuang commented Feb 17, 2016

efischer19 commented Feb 18, 2016

Reset state fix #11422

Reset state fix #11422

Conversation

efischer19 commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efischer19 commented Feb 10, 2016

efischer19 commented Feb 10, 2016

Choose a reason for hiding this comment

efischer19 commented Feb 10, 2016

Choose a reason for hiding this comment

efischer19 commented Feb 12, 2016

efischer19 commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efischer19 commented Feb 17, 2016

Choose a reason for hiding this comment

robrap commented Feb 17, 2016

efischer19 commented Feb 17, 2016

dianakhuang commented Feb 17, 2016

efischer19 commented Feb 18, 2016