Merge pull request #278 from edx/sanchez/TIM-473-Get-Latest-PWI

Fixing TIM-473 to allow re-leasing a submission

apps/openassessment/assessment/peer_api.py

@@ -741,11 +741,14 @@ def _create_peer_workflow_item(workflow, submission_uuid):
     """
     try:
         peer_workflow = PeerWorkflow.objects.get(submission_uuid=submission_uuid)
+
         workflow_item, __ = PeerWorkflowItem.objects.get_or_create(
             scorer=workflow,
             author=peer_workflow,
             submission_uuid=submission_uuid
         )
+        workflow_item.started_at = timezone.now()
+        workflow_item.save()
         return workflow_item
     except DatabaseError:
         error_message = _(
@@ -845,6 +848,7 @@ def _get_submission_for_review(workflow, graded_by, over_grading=False):
             "   select pwi.author_id "
             "   from assessment_peerworkflowitem pwi "
             "   where pwi.scorer_id=%s "
+            "   and pwi.assessment_id is not NULL "
             ") "
             "and ("
             "   select count(pwi.id) as c "
@@ -972,7 +976,14 @@ def _close_active_assessment(
 
     """
     try:
-        item = workflow.graded.get(submission_uuid=submission_uuid)
+        item_query = workflow.graded.filter(submission_uuid=submission_uuid).order_by("-started_at", "-id")
+        items = list(item_query[:1])
+        if not items:
+            raise PeerAssessmentWorkflowError(_(
+                u"No open assessment was found for student {} while assessing "
+                u"submission UUID {}.".format(workflow.student_id, submission_uuid)
+            ))
+        item = items[0]
         item.assessment = assessment
         if (not item.author.grading_completed_at
                 and item.author.graded_by.all().count() >= num_required_grades):

apps/openassessment/assessment/test/test_peer.py

 # coding=utf-8
 import datetime
-
-from django.db import DatabaseError
 import pytz
 
+from django.db import DatabaseError
+from django.utils import timezone
 from ddt import ddt, file_data
 from mock import patch
 from nose.tools import raises
@@ -13,7 +13,6 @@ from openassessment.assessment import peer_api
 from openassessment.assessment.models import Assessment, PeerWorkflow, PeerWorkflowItem, AssessmentFeedback
 from openassessment.workflow import api as workflow_api
 from submissions import api as sub_api
-from submissions.models import Submission
 from submissions.tests.test_api import STUDENT_ITEM, ANSWER_ONE
 
 # Possible points: 14
@@ -202,6 +201,55 @@ class TestPeerApi(CacheResetTest):
         self.assertTrue(finished)
         self.assertEqual(count, 1)
 
+    def test_peer_leases_same_submission(self):
+        """
+        Tests the scenario where a student pulls a peer's submission for
+        assessment, lets the lease expire, then pulls the same peer's submission
+        a second time.
+
+        This creates two similar PeerWorkflowItems in the database, and when
+        completing the assessment, the latest PeerWorkflowItem should be
+        updated.
+        """
+        yesterday = timezone.now() - datetime.timedelta(days=1)
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer")
+        self._create_student_and_submission("Bob", "Bob's answer")
+        self._create_student_and_submission("Sally", "Sally's answer")
+        sub = peer_api.get_submission_to_assess(tim_sub['uuid'], REQUIRED_GRADED)
+        self.assertEqual(u"Bob's answer", sub['answer'])
+
+        # And now we cheat; we want to set the clock back such that the lease
+        # on this PeerWorkflowItem has expired.
+        pwis = PeerWorkflowItem.objects.filter(submission_uuid=sub['uuid'])
+        self.assertEqual(len(pwis), 1)
+        pwis[0].started_at = yesterday
+        pwis[0].save()
+
+        sub = peer_api.get_submission_to_assess(tim_sub['uuid'], REQUIRED_GRADED)
+        self.assertEqual(u"Bob's answer", sub['answer'])
+
+        peer_api.create_assessment(
+            tim_sub["uuid"], tim["student_id"], ASSESSMENT_DICT, RUBRIC_DICT,
+            REQUIRED_GRADED_BY,
+        )
+
+        pwis = PeerWorkflowItem.objects.filter(submission_uuid=sub['uuid'])
+        self.assertEqual(len(pwis), 1)
+        self.assertNotEqual(pwis[0].started_at, yesterday)
+
+    @raises(peer_api.PeerAssessmentWorkflowError)
+    def test_no_submission_found_closing_assessment(self):
+        """
+        Confirm the appropriate error is raised when no submission is found
+        open for assessment, when submitting an assessment.
+        """
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", MONDAY)
+        peer_api.create_assessment(
+            tim_sub["uuid"], tim["student_id"], ASSESSMENT_DICT, RUBRIC_DICT,
+            REQUIRED_GRADED_BY,
+        )
+
+
     def test_peer_assessment_workflow(self):
         tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer")
         bob_sub, bob = self._create_student_and_submission("Bob", "Bob's answer")
@@ -603,7 +651,7 @@ class TestPeerApi(CacheResetTest):
             scored_at=assessment_dict["scored_at"])[0]
         peer_api._close_active_assessment(buffy_workflow, xander_answer["uuid"], assessment, REQUIRED_GRADED_BY)
 
-        item = peer_api._create_peer_workflow_item(buffy_workflow, xander_answer["uuid"])
+        item = PeerWorkflowItem.objects.get(submission_uuid=xander_answer['uuid'])
         self.assertEqual(xander_answer["uuid"], submission["uuid"])
         self.assertIsNotNone(item.assessment)
 
@@ -665,7 +713,7 @@ class TestPeerApi(CacheResetTest):
         mock_filter.side_effect = DatabaseError("Oh no.")
         self._create_student_and_submission("Tim", "Tim's answer", MONDAY)
 
-    @patch.object(PeerWorkflowItem.objects, 'get_or_create')
+    @patch.object(PeerWorkflow.objects, 'get')
     @raises(peer_api.PeerAssessmentInternalError)
     def test_create_workflow_item_error(self, mock_filter):
         mock_filter.side_effect = DatabaseError("Oh no.")