Merge pull request #245 from edx/cdodge/add-second-review-required-status

PHX-226 Define an internal status of 'second_review_required' when a failing …

edx_proctoring/admin.py

@@ -298,7 +298,7 @@ class ProctoredExamSoftwareSecureReviewAdmin(admin.ModelAdmin):
         review.save()
         # call the review saved and since it's coming from
         # the Django admin will we accept failures
-        get_backend_provider().on_review_saved(review, allow_status_update_on_fail=True)
+        get_backend_provider().on_review_saved(review, allow_rejects=True)
 
     def get_form(self, request, obj=None, **kwargs):
         form = super(ProctoredExamSoftwareSecureReviewAdmin, self).get_form(request, obj, **kwargs)

edx_proctoring/api.py

@@ -1687,6 +1687,13 @@ def _get_proctored_exam_view(exam, context, exam_id, user_id, course_id):
             ) else 'proctored_exam/submitted.html'
         else:
             student_view_template = 'proctored_exam/waiting_for_app_shutdown.html'
+    elif attempt_status == ProctoredExamStudentAttemptStatus.second_review_required:
+        # the student should still see a 'submitted'
+        # rendering even if the review needs a 2nd review
+        student_view_template = None if _was_review_status_acknowledged(
+            attempt['is_status_acknowledged'],
+            exam['due_date']
+        ) else 'proctored_exam/submitted.html'
     elif attempt_status == ProctoredExamStudentAttemptStatus.verified:
         student_view_template = None if _was_review_status_acknowledged(
             attempt['is_status_acknowledged'],

edx_proctoring/backends/software_secure.py

@@ -247,9 +247,9 @@ class SoftwareSecureBackendProvider(ProctoringBackendProvider):
             # update our attempt status, note we have to import api.py here because
             # api.py imports software_secure.py, so we'll get an import circular reference
 
-            allow_status_update_on_fail = not constants.REQUIRE_FAILURE_SECOND_REVIEWS
+            allow_rejects = not constants.REQUIRE_FAILURE_SECOND_REVIEWS
 
-            self.on_review_saved(review, allow_status_update_on_fail=allow_status_update_on_fail)
+            self.on_review_saved(review, allow_rejects=allow_rejects)
 
         # emit an event for 'review-received'
         data = {
@@ -265,7 +265,7 @@ class SoftwareSecureBackendProvider(ProctoringBackendProvider):
         exam = serialized_exam_object.data
         emit_event(exam, 'review-received', attempt=attempt, override_data=data)
 
-    def on_review_saved(self, review, allow_status_update_on_fail=False):  # pylint: disable=arguments-differ
+    def on_review_saved(self, review, allow_rejects=False):  # pylint: disable=arguments-differ
         """
         called when a review has been save - either through API (on_review_callback) or via Django Admin panel
         in order to trigger any workflow associated with proctoring review results
@@ -293,21 +293,23 @@ class SoftwareSecureBackendProvider(ProctoringBackendProvider):
         status = (
             ProctoredExamStudentAttemptStatus.verified
             if review.review_status in self.passing_review_status
-            else ProctoredExamStudentAttemptStatus.rejected
+            else (
+                # if we are not allowed to store 'rejected' on this
+                # code path, then put status into 'second_review_required'
+                ProctoredExamStudentAttemptStatus.rejected if allow_rejects else
+                ProctoredExamStudentAttemptStatus.second_review_required
+            )
         )
 
-        # are we allowed to update the status if we have a failure status
-        # i.e. do we need a review to come in from Django Admin panel?
-        if status == ProctoredExamStudentAttemptStatus.verified or allow_status_update_on_fail:
-            # updating attempt status will trigger workflow
-            # (i.e. updating credit eligibility table)
-            from edx_proctoring.api import update_attempt_status
-
-            update_attempt_status(
-                attempt_obj.proctored_exam_id,
-                attempt_obj.user_id,
-                status
-            )
+        # updating attempt status will trigger workflow
+        # (i.e. updating credit eligibility table)
+        from edx_proctoring.api import update_attempt_status
+
+        update_attempt_status(
+            attempt_obj.proctored_exam_id,
+            attempt_obj.user_id,
+            status
+        )
 
     def _save_review_comment(self, review, comment):
         """

edx_proctoring/backends/tests/test_software_secure.py

@@ -778,7 +778,8 @@ class SoftwareSecureTests(TestCase):
         self.assertEqual(records[0].review_status, 'Clean')
         self.assertEqual(records[1].review_status, 'Suspicious')
 
-    def test_failure_submission(self):
+    @ddt.data(False, True)
+    def test_failure_submission(self, allow_rejects):
         """
         Tests that a submission of a failed test and make sure that we
         don't automatically update the status to failure
@@ -824,10 +825,17 @@ class SoftwareSecureTests(TestCase):
         # now simulate a update via Django Admin table which will actually
         # push through the failure into our attempt status (as well as trigger)
         # other workflow
-        provider.on_review_saved(review, allow_status_update_on_fail=True)
+        provider.on_review_saved(review, allow_rejects=allow_rejects)
 
         attempt = get_exam_attempt_by_id(attempt_id)
-        self.assertEqual(attempt['status'], ProctoredExamStudentAttemptStatus.rejected)
+
+        # if we don't allow rejects to be stored in attempt status
+        # then we should expect a 'second_review_required' status
+        expected_status = (
+            ProctoredExamStudentAttemptStatus.rejected if allow_rejects else
+            ProctoredExamStudentAttemptStatus.second_review_required
+        )
+        self.assertEqual(attempt['status'], expected_status)
 
     def test_update_archived_attempt(self):
         """
@@ -877,7 +885,7 @@ class SoftwareSecureTests(TestCase):
         # now simulate a update via Django Admin table which will actually
         # push through the failure into our attempt status but
         # as this is an archived attempt, we don't do anything
-        provider.on_review_saved(review, allow_status_update_on_fail=True)
+        provider.on_review_saved(review, allow_rejects=True)
 
         # look at the attempt again, since it moved into Archived state
         # then it should still remain unchanged
@@ -897,7 +905,7 @@ class SoftwareSecureTests(TestCase):
         review = ProctoredExamSoftwareSecureReview()
         review.attempt_code = 'foo'
 
-        self.assertIsNone(provider.on_review_saved(review, allow_status_update_on_fail=True))
+        self.assertIsNone(provider.on_review_saved(review, allow_rejects=True))
 
     def test_split_fullname(self):
         """

edx_proctoring/models.py

@@ -150,6 +150,9 @@ class ProctoredExamStudentAttemptStatus(object):
     # the student has submitted the exam for proctoring review
     submitted = 'submitted'
 
+    # the student has submitted the exam for proctoring review
+    second_review_required = 'second_review_required'
+
     # the exam has been verified and approved
     verified = 'verified'
 
@@ -176,8 +179,8 @@ class ProctoredExamStudentAttemptStatus(object):
         that it cannot go backwards in state
         """
         return status in [
-            cls.declined, cls.timed_out, cls.submitted, cls.verified, cls.rejected,
-            cls.not_reviewed, cls.error
+            cls.declined, cls.timed_out, cls.submitted, cls.second_review_required,
+            cls.verified, cls.rejected, cls.not_reviewed, cls.error
         ]
 
     @classmethod
@@ -196,8 +199,8 @@ class ProctoredExamStudentAttemptStatus(object):
         Returns a boolean if the passed in to_status calls for an update to the credit requirement status.
         """
         return to_status in [
-            cls.verified, cls.rejected, cls.declined, cls.not_reviewed, cls.submitted,
-            cls.error
+            cls.verified, cls.rejected, cls.declined, cls.not_reviewed,
+            cls.submitted, cls.error
         ]
 
     @classmethod

edx_proctoring/static/proctoring/js/views/proctored_exam_attempt_view.js

@@ -14,6 +14,7 @@ var edx = edx || {};
         ready_to_submit: gettext('Ready to submit'),
         declined: gettext('Declined'),
         timed_out: gettext('Timed out'),
+        second_review_required: gettext('Second Review Required'),
         submitted: gettext('Submitted'),
         verified: gettext('Verified'),
         rejected: gettext('Rejected'),

edx_proctoring/tests/test_api.py

@@ -824,6 +824,7 @@ class ProctoredExamApiTests(LoggedInTestCase):
         (ProctoredExamStudentAttemptStatus.submitted, 'submitted'),
         (ProctoredExamStudentAttemptStatus.declined, 'declined'),
         (ProctoredExamStudentAttemptStatus.error, 'failed'),
+        (ProctoredExamStudentAttemptStatus.second_review_required, None),
     )
     @ddt.unpack
     def test_remove_exam_attempt_with_status(self, to_status, requirement_status):
@@ -843,19 +844,24 @@ class ProctoredExamApiTests(LoggedInTestCase):
         credit_service = get_runtime_service('credit')
         credit_status = credit_service.get_credit_state(self.user.id, exam_attempt.proctored_exam.course_id)
 
-        self.assertEqual(len(credit_status['credit_requirement_status']), 1)
-        self.assertEqual(
-            credit_status['credit_requirement_status'][0]['status'],
-            requirement_status
-        )
+        if requirement_status:
+            self.assertEqual(len(credit_status['credit_requirement_status']), 1)
+            self.assertEqual(
+                credit_status['credit_requirement_status'][0]['status'],
+                requirement_status
+            )
 
-        # now remove exam attempt which calls the credit service method 'remove_credit_requirement_status'
-        remove_exam_attempt(exam_attempt.proctored_exam_id)
+            # now remove exam attempt which calls the credit service method 'remove_credit_requirement_status'
+            remove_exam_attempt(exam_attempt.proctored_exam_id)
 
-        # make sure the credit requirement status is no longer there
-        credit_status = credit_service.get_credit_state(self.user.id, exam_attempt.proctored_exam.course_id)
+            # make sure the credit requirement status is no longer there
+            credit_status = credit_service.get_credit_state(self.user.id, exam_attempt.proctored_exam.course_id)
 
-        self.assertEqual(len(credit_status['credit_requirement_status']), 0)
+            self.assertEqual(len(credit_status['credit_requirement_status']), 0)
+        else:
+            # There is not an expected changed to the credit requirement table
+            # given the attempt status
+            self.assertEqual(len(credit_status['credit_requirement_status']), 0)
 
     def test_stop_a_non_started_exam(self):
         """
@@ -1548,6 +1554,25 @@ class ProctoredExamApiTests(LoggedInTestCase):
             )
             self.assertIn(self.proctored_exam_submitted_msg, rendered_response)
 
+            # now make sure if this status transitions to 'second_review_required'
+            # the student will still see a 'submitted' message
+            update_attempt_status(
+                exam_attempt.proctored_exam_id,
+                exam_attempt.user_id,
+                ProctoredExamStudentAttemptStatus.second_review_required
+            )
+            rendered_response = get_student_view(
+                user_id=self.user_id,
+                course_id=self.course_id,
+                content_id=self.content_id,
+                context={
+                    'is_proctored': True,
+                    'display_name': self.exam_name,
+                    'default_time_limit_mins': 90
+                }
+            )
+            self.assertIn(self.proctored_exam_submitted_msg, rendered_response)
+
     def test_get_studentview_submitted_status_with_duedate(self):
         """
         Test for get_student_view proctored exam which has been submitted
@@ -2471,6 +2496,23 @@ class ProctoredExamApiTests(LoggedInTestCase):
         self.assertIn(ProctoredExamStudentAttemptStatus.get_status_alias(status), mail.outbox[0].body)
         self.assertIn(credit_state['course_name'], mail.outbox[0].body)
 
+    @ddt.data(
+        ProctoredExamStudentAttemptStatus.second_review_required,
+        ProctoredExamStudentAttemptStatus.error
+    )
+    def test_email_not_sent(self, status):
+        """
+        Assert than email is not sent on the following statuses of proctoring attempt
+        """
+
+        exam_attempt = self._create_started_exam_attempt()
+        update_attempt_status(
+            exam_attempt.proctored_exam_id,
+            self.user.id,
+            status
+        )
+        self.assertEquals(len(mail.outbox), 0)
+
     def test_send_email_unicode(self):
         """
         Assert that email can be sent with a unicode course name.