Merge pull request #592 from edx/sanchez/fix-duplicate-assessments

Sanchez/fix duplicate assessments

openassessment/assessment/api/peer.py

@@ -249,7 +249,7 @@ def create_assessment(
     try:
         # Retrieve workflow information
         scorer_workflow = PeerWorkflow.objects.get(submission_uuid=scorer_submission_uuid)
-        peer_workflow_item = scorer_workflow.get_latest_open_workflow_item()
+        peer_workflow_item = scorer_workflow.find_active_assessments()
         if peer_workflow_item is None:
             message = (
                 u"There are no open assessments associated with the scorer's "
@@ -257,7 +257,7 @@ def create_assessment(
             ).format(scorer_submission_uuid)
             logger.warning(message)
             raise PeerAssessmentWorkflowError(message)
-        peer_submission_uuid = peer_workflow_item.author.submission_uuid
+        peer_submission_uuid = peer_workflow_item.submission_uuid
 
         assessment = _complete_assessment(
             rubric_dict,
@@ -666,7 +666,8 @@ def get_submission_to_assess(submission_uuid, graded_by):
             u"A Peer Assessment Workflow does not exist for the student "
             u"with submission UUID {}".format(submission_uuid)
         )
-    peer_submission_uuid = workflow.find_active_assessments()
+    open_item = workflow.find_active_assessments()
+    peer_submission_uuid = open_item.submission_uuid if open_item else None
     # If there is an active assessment for this user, get that submission,
     # otherwise, get the first assessment for review, otherwise,
     # get the first submission available for over grading ("over-grading").

openassessment/assessment/models/peer.py

@@ -216,13 +216,29 @@ class PeerWorkflow(models.Model):
                 for this PeerWorkflow.
 
         Returns:
-            submission_uuid (str): The submission_uuid for the submission that the
+            (PeerWorkflowItem) The PeerWorkflowItem for the submission that the
                 student has open for active assessment.
 
         """
         oldest_acceptable = now() - self.TIME_LIMIT
-        workflows = self.graded.filter(assessment__isnull=True, started_at__gt=oldest_acceptable)   # pylint:disable=E1101
-        return workflows[0].submission_uuid if workflows else None
+        items = list(self.graded.all().order_by("-started_at", "-id"))
+        valid_open_items = []
+        completed_sub_uuids = []
+
+        # First, remove all completed items.
+        for item in items:
+            if item.assessment is not None:
+                completed_sub_uuids.append(item.submission_uuid)
+            else:
+                valid_open_items.append(item)
+
+        # Remove any open items which have a submission which has been completed.
+        for item in valid_open_items:
+            if (item.started_at < oldest_acceptable or
+                    item.submission_uuid in completed_sub_uuids):
+                valid_open_items.remove(item)
+
+        return valid_open_items[0] if valid_open_items else None
 
     def get_submission_for_review(self, graded_by):
         """
@@ -331,19 +347,6 @@ class PeerWorkflow(models.Model):
             logger.exception(error_message)
             raise PeerAssessmentInternalError(error_message)
 
-    def get_latest_open_workflow_item(self):
-        """
-        Return the latest open workflow item for this workflow.
-
-        Returns:
-            A PeerWorkflowItem that is open for assessment.
-            None if no item is found.
-
-        """
-        workflow_query = self.graded.filter(assessment__isnull=True).order_by("-started_at", "-id")  # pylint:disable=E1101
-        items = list(workflow_query[:1])
-        return items[0] if items else None
-
     def close_active_assessment(self, submission_uuid, assessment, num_required_grades):
         """
         Updates a workflow item on the student's workflow with the associated

openassessment/assessment/test/test_peer.py

@@ -151,7 +151,7 @@ class TestPeerApi(CacheResetTest):
     Tests for the peer assessment API functions.
     """
 
-    CREATE_ASSESSMENT_NUM_QUERIES = 59
+    CREATE_ASSESSMENT_NUM_QUERIES = 58
 
     def test_create_assessment_points(self):
         self._create_student_and_submission("Tim", "Tim's answer")
@@ -879,8 +879,8 @@ class TestPeerApi(CacheResetTest):
         PeerWorkflow.create_item(buffy_workflow, xander_answer["uuid"])
 
         # Check to see if Buffy is still actively reviewing Xander's submission.
-        submission_uuid = buffy_workflow.find_active_assessments()
-        self.assertEqual(xander_answer["uuid"], submission_uuid)
+        item = buffy_workflow.find_active_assessments()
+        self.assertEqual(xander_answer["uuid"], item.submission_uuid)
 
     def test_get_workflow_by_uuid(self):
         buffy_answer, _ = self._create_student_and_submission("Buffy", "Buffy's answer")
@@ -1212,6 +1212,70 @@ class TestPeerApi(CacheResetTest):
             MONDAY,
         )
 
+    def test_ignore_duplicate_workflow_items(self):
+        """
+        A race condition may cause two workflow items to be opened for a single
+        submission. In this case, we want to be defensive in the API, such that
+        no open workflow item is acknowledged if an assessment has already been
+        made against the associated submission.
+
+        """
+        bob_sub, bob = self._create_student_and_submission('Bob', 'Bob submission')
+        tim_sub, tim = self._create_student_and_submission('Tim', 'Tim submission')
+        sally_sub, sally = self._create_student_and_submission('Sally', 'Sally submission')
+        jane_sub, jane = self._create_student_and_submission('Jane', 'Jane submission')
+
+        # Create two workflow items.
+        peer_api.create_peer_workflow_item(bob_sub['uuid'], tim_sub['uuid'])
+        peer_api.create_peer_workflow_item(bob_sub['uuid'], tim_sub['uuid'])
+
+        # Assess the submission, then get the next submission.
+        peer_api.create_assessment(
+            bob_sub['uuid'],
+            bob['student_id'],
+            ASSESSMENT_DICT['options_selected'],
+            ASSESSMENT_DICT['criterion_feedback'],
+            ASSESSMENT_DICT['overall_feedback'],
+            RUBRIC_DICT,
+            REQUIRED_GRADED_BY,
+            MONDAY
+        )
+
+        # Verify the next submission is not Tim again, but Sally.
+        next_sub = peer_api.get_submission_to_assess(bob_sub['uuid'], REQUIRED_GRADED_BY)
+        self.assertEqual(next_sub['uuid'], sally_sub['uuid'])
+
+        # Request another peer submission. Should pick up Sally again.
+        next_sub = peer_api.get_submission_to_assess(bob_sub['uuid'], REQUIRED_GRADED_BY)
+        self.assertEqual(next_sub['uuid'], sally_sub['uuid'])
+
+        # Ensure that the next assessment made is against Sally, not Tim.
+        # Assess the submission, then get the next submission.
+        peer_api.create_assessment(
+            bob_sub['uuid'],
+            bob['student_id'],
+            ASSESSMENT_DICT['options_selected'],
+            ASSESSMENT_DICT['criterion_feedback'],
+            ASSESSMENT_DICT['overall_feedback'],
+            RUBRIC_DICT,
+            REQUIRED_GRADED_BY,
+            MONDAY
+        )
+
+        # Make sure Tim has one assessment.
+        tim_assessments = peer_api.get_assessments(tim_sub['uuid'], scored_only=False)
+        self.assertEqual(1, len(tim_assessments))
+
+        # Make sure Sally has one assessment.
+        sally_assessments = peer_api.get_assessments(sally_sub['uuid'], scored_only=False)
+        self.assertEqual(1, len(sally_assessments))
+
+        # Make sure Jane has no assessment.
+        jane_assessments = peer_api.get_assessments(jane_sub['uuid'], scored_only=False)
+        self.assertEqual(0, len(jane_assessments))
+
+
+
     def test_get_submission_to_assess_no_workflow(self):
         # Try to retrieve a submission to assess when the student
         # doing the assessment hasn't yet submitted.

openassessment/xblock/static/js/spec/oa_base.js

@@ -26,6 +26,16 @@ describe("OpenAssessment.BaseView", function() {
                 defer.resolveWith(this, [server.fragments[component]]);
             }).promise();
         };
+
+        var successPromise = $.Deferred(
+            function(defer) {
+                defer.resolve();
+            }
+        ).promise();
+
+        this.peerAssess = function(optionsSelected, feedback) {
+            return successPromise;
+        };
     };
 
     // Stub runtime
@@ -76,4 +86,20 @@ describe("OpenAssessment.BaseView", function() {
             expect(server.fragmentsLoaded).toContain("grade");
         });
     });
+
+    it("Only load the peer section once on submit", function() {
+        loadSubviews(function() {
+            // Simulate a server error
+            view.peerView.peerAssess();
+            var numPeerLoads = 0;
+            for (var i = 0; i < server.fragmentsLoaded.length; i++) {
+                if (server.fragmentsLoaded[i] == 'peer_assessment') {
+                    numPeerLoads++;
+                }
+            }
+            // Peer should be called twice, once when loading the views,
+            // and again after the peer has been assessed.
+            expect(numPeerLoads).toBe(2);
+        });
+    });
 });

openassessment/xblock/static/js/src/oa_peer.js

@@ -158,7 +158,6 @@ OpenAssessment.PeerView.prototype = {
         var view = this;
         var baseView = view.baseView;
         this.peerAssessRequest(function() {
-            view.load();
             baseView.loadAssessmentModules();
             baseView.scrollToTop();
         });