Merge pull request #502 from edx/dcs/fix-admin

Fixes django admin saving of reviews
openedx · Jan 7, 2019 · ceea22e · ceea22e
2 parents cdab034 + d993204
commit ceea22e
Show file tree

Hide file tree

Showing 13 changed files with 104 additions and 81 deletions.
diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py
@@ -5,6 +5,6 @@
 from __future__ import absolute_import
 
 # Be sure to update the version number in edx_proctoring/package.json
-__version__ = '1.5.0'
+__version__ = '1.5.1'
 
 default_app_config = 'edx_proctoring.apps.EdxProctoringConfig'  # pylint: disable=invalid-name
diff --git a/edx_proctoring/admin.py b/edx_proctoring/admin.py
@@ -21,7 +21,6 @@
     ProctoredExamStudentAttempt,
 )
 from edx_proctoring.api import update_attempt_status
-from edx_proctoring.backends import get_backend_provider
 from edx_proctoring.utils import locate_attempt_by_attempt_code
 from edx_proctoring.exceptions import (
     ProctoredExamIllegalStatusTransition,
@@ -292,9 +291,6 @@ def save_model(self, request, review, form, change):  # pylint: disable=argument
         """
         review.reviewed_by = request.user
         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_rejects=True)
 
     def get_form(self, request, obj=None, **kwargs):
         form = super(ProctoredExamSoftwareSecureReviewAdmin, self).get_form(request, obj, **kwargs)

diff --git a/edx_proctoring/signals.py b/edx_proctoring/signals.py
@@ -1,7 +1,15 @@
 "edx-proctoring signals"
+from crum import get_current_request
+
 from django.db.models.signals import pre_save, post_save, pre_delete
 from django.dispatch import receiver
+from django.urls import reverse
+
+from edx_proctoring import api
+from edx_proctoring import constants
 from edx_proctoring import models
+from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus
+from edx_proctoring.utils import emit_event, locate_attempt_by_attempt_code
 from edx_proctoring.backends import get_backend_provider
 
 
@@ -120,3 +128,65 @@ def on_review_changed(sender, instance, signal, **kwargs):  # pylint: disable=un
             # don't archive on create
             return
     models.archive_model(models.ProctoredExamSoftwareSecureReviewHistory, instance, id='review_id')
+
+
+@receiver(post_save, sender=models.ProctoredExamSoftwareSecureReview)
+def finish_review_workflow(sender, instance, signal, **kwargs):  # pylint: disable=unused-argument
+    """
+    Updates the attempt status based on the review status
+    Also notifies support about suspicious reviews.
+    """
+    review = instance
+    attempt_obj, is_archived = locate_attempt_by_attempt_code(review.attempt_code)
+    attempt = api.ProctoredExamStudentAttemptSerializer(attempt_obj).data
+
+    # we could have gotten a review for an archived attempt
+    # this should *not* cause an update in our credit
+    # eligibility table
+    if review.review_status in SoftwareSecureReviewStatus.passing_statuses:
+        attempt_status = ProctoredExamStudentAttemptStatus.verified
+    elif review.reviewed_by or not constants.REQUIRE_FAILURE_SECOND_REVIEWS:
+        # reviews from the django admin have a reviewer set. They should be allowed to
+        # reject an attempt
+        attempt_status = ProctoredExamStudentAttemptStatus.rejected
+    else:
+        # if we are not allowed to store 'rejected' on this
+        # code path, then put status into 'second_review_required'
+        attempt_status = ProctoredExamStudentAttemptStatus.second_review_required
+
+    if review.review_status in SoftwareSecureReviewStatus.notify_support_for_status:
+        instructor_service = api.get_runtime_service('instructor')
+        request = get_current_request()
+        if instructor_service and request:
+            course_id = attempt['proctored_exam']['course_id']
+            exam_id = attempt['proctored_exam']['id']
+            review_url = request.build_absolute_uri(
+                u'{}?attempt={}'.format(
+                    reverse('edx_proctoring:instructor_dashboard_exam', args=[course_id, exam_id]),
+                    attempt['external_id']
+                ))
+            instructor_service.send_support_notification(
+                course_id=attempt['proctored_exam']['course_id'],
+                exam_name=attempt['proctored_exam']['exam_name'],
+                student_username=attempt['user']['username'],
+                review_status=review.review_status,
+                review_url=review_url,
+            )
+
+    if not is_archived:
+        # updating attempt status will trigger workflow
+        # (i.e. updating credit eligibility table)
+        # archived attempts should not trigger the workflow
+        api.update_attempt_status(
+            attempt['proctored_exam']['id'],
+            attempt['user']['id'],
+            attempt_status,
+            raise_if_not_found=False
+        )
+
+    # emit an event for 'review_received'
+    data = {
+        'review_attempt_code': review.attempt_code,
+        'review_status': review.review_status,
+    }
+    emit_event(attempt['proctored_exam'], 'review_received', attempt=attempt, override_data=data)
diff --git a/edx_proctoring/statuses.py b/edx_proctoring/statuses.py
@@ -135,7 +135,7 @@ def validate(cls, status):
         """
         Validate review status
         """
-        if status not in [cls.passed, cls.violation, cls.suspicious, cls.not_reviewed]:
+        if status not in (cls.passed, cls.violation, cls.suspicious, cls.not_reviewed):
             raise ProctoredExamBadReviewStatus(status)
         return True
 

diff --git a/edx_proctoring/tests/test_reviews.py b/edx_proctoring/tests/test_reviews.py
@@ -5,6 +5,7 @@
 
 import json
 
+from crum import set_current_request
 import ddt
 from mock import patch
 
@@ -55,6 +56,7 @@ def setUp(self):
         set_runtime_service('instructor', MockInstructorService())
         set_runtime_service('grades', MockGradesService())
         set_runtime_service('certificates', MockCertificateService())
+        set_current_request(self.dummy_request)
 
     def tearDown(self):
         super(ReviewTests, self).tearDown()
@@ -119,7 +121,7 @@ def test_psi_review_callback(self, psi_review_status, review_status, credit_requ
             with self.assertRaises(ProctoredExamBadReviewStatus):
                 ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
         else:
-            ProctoredExamReviewCallback().make_review(self.attempt, test_payload, request=self.dummy_request)
+            ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
             # make sure that what we have in the Database matches what we expect
             review = ProctoredExamSoftwareSecureReview.get_review_by_attempt_code(self.attempt['attempt_code'])
 
@@ -245,7 +247,7 @@ def test_allow_review_resubmission(self):
 
         # now call again, this will not throw exception
         test_payload['status'] = ReviewStatus.suspicious
-        ProctoredExamReviewCallback().make_review(self.attempt, test_payload, request=self.dummy_request)
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
 
         # make sure that what we have in the Database matches what we expect
         review = ProctoredExamSoftwareSecureReview.get_review_by_attempt_code(self.attempt['attempt_code'])
@@ -280,7 +282,7 @@ def test_failure_submission(self):
 
         allow_rejects = not constants.REQUIRE_FAILURE_SECOND_REVIEWS
         # submit a Suspicious review payload
-        ProctoredExamReviewCallback().make_review(self.attempt, test_payload, request=self.dummy_request)
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
 
         # now look at the attempt and make sure it did not
         # transition to failure on the callback,
@@ -310,7 +312,7 @@ def test_failure_submission_rejected(self):
         test_payload = self.get_review_payload(ReviewStatus.suspicious)
         allow_rejects = not constants.REQUIRE_FAILURE_SECOND_REVIEWS
         # submit a Suspicious review payload
-        ProctoredExamReviewCallback().make_review(self.attempt, test_payload, request=self.dummy_request)
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
 
         # now look at the attempt and make sure it did not
         # transition to failure on the callback,
@@ -363,7 +365,7 @@ def test_update_archived_attempt(self):
         # now we'll make another review for the archived attempt. It should NOT update the status
         test_payload = self.get_review_payload(ReviewStatus.suspicious)
         self.attempt['is_archived'] = True
-        ProctoredExamReviewCallback().make_review(self.attempt, test_payload, request=self.dummy_request)
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
         attempt, is_archived = locate_attempt_by_attempt_code(self.attempt['attempt_code'])
         self.assertTrue(is_archived)
         self.assertEqual(attempt.status, 'verified')
diff --git a/edx_proctoring/views.py b/edx_proctoring/views.py
@@ -71,7 +71,6 @@
     get_time_remaining_for_attempt,
     locate_attempt_by_attempt_code,
     humanized_time,
-    emit_event,
 )
 
 ATTEMPTS_PER_PAGE = 25
@@ -794,7 +793,7 @@ class BaseReviewCallback(object):
     Base class for review callbacks.
     make_review handles saving reviews and review comments.
     """
-    def make_review(self, attempt, data, request=None, backend=None):
+    def make_review(self, attempt, data, backend=None):
         """
         Save the review and review comments
         """
@@ -861,54 +860,6 @@ def make_review(self, attempt, data, request=None, backend=None):
             )
             comment.save()
 
-        # we could have gotten a review for an archived attempt
-        # this should *not* cause an update in our credit
-        # eligibility table
-        if review.review_status in SoftwareSecureReviewStatus.passing_statuses:
-            attempt_status = ProctoredExamStudentAttemptStatus.verified
-        elif not constants.REQUIRE_FAILURE_SECOND_REVIEWS:
-            attempt_status = ProctoredExamStudentAttemptStatus.rejected
-        else:
-            # if we are not allowed to store 'rejected' on this
-            # code path, then put status into 'second_review_required'
-            attempt_status = ProctoredExamStudentAttemptStatus.second_review_required
-
-        if review.review_status in SoftwareSecureReviewStatus.notify_support_for_status:
-            instructor_service = get_runtime_service('instructor')
-            if instructor_service:
-                course_id = attempt['proctored_exam']['course_id']
-                exam_id = attempt['proctored_exam']['id']
-                review_url = request.build_absolute_uri(
-                    u'{}?attempt={}'.format(
-                        reverse('edx_proctoring:instructor_dashboard_exam', args=[course_id, exam_id]),
-                        attempt['external_id']
-                    ))
-                instructor_service.send_support_notification(
-                    course_id=attempt['proctored_exam']['course_id'],
-                    exam_name=attempt['proctored_exam']['exam_name'],
-                    student_username=attempt['user']['username'],
-                    review_status=review.review_status,
-                    review_url=review_url,
-                )
-
-        if not attempt.get('is_archived', False):
-            # updating attempt status will trigger workflow
-            # (i.e. updating credit eligibility table)
-            # archived attempts should not trigger the workflow
-            update_attempt_status(
-                attempt['proctored_exam']['id'],
-                attempt['user']['id'],
-                attempt_status,
-                raise_if_not_found=False
-            )
-
-        # emit an event for 'review_received'
-        data = {
-            'review_attempt_code': review.attempt_code,
-            'review_status': review.review_status,
-        }
-        emit_event(attempt['proctored_exam'], 'review_received', attempt=attempt, override_data=data)
-
 
 class ProctoredExamReviewCallback(ProctoredAPIView, BaseReviewCallback):
     """
@@ -922,7 +873,7 @@ def post(self, request, external_id):
         attempt = get_exam_attempt_by_external_id(external_id)
         if attempt is None:
             raise StudentExamAttemptDoesNotExistsException('not found')
-        self.make_review(attempt, request.data, request)
+        self.make_review(attempt, request.data)
         return Response(data='OK')
 
 
@@ -983,7 +934,6 @@ def post(self, request):
         serialized['is_archived'] = is_archived
         self.make_review(serialized,
                          request.data,
-                         request,
                          backend=provider)
         return Response('OK')
 

diff --git a/package.json b/package.json
@@ -2,7 +2,7 @@
   "name": "@edx/edx-proctoring",
   "//": "Be sure to update the version number in edx_proctoring/__init__.py",
   "//": "Note that the version format is slightly different than that of the Python version when using prereleases.",
-  "version": "1.5.0",
+  "version": "1.5.1",
   "main": "edx_proctoring/static/index.js",
   "repository": {
     "type": "git",

diff --git a/requirements/base.in b/requirements/base.in
@@ -8,6 +8,7 @@ django-ipware>=1.1.0
 jsonfield>=2.0.2
 pytz>=2018
 pycryptodomex>=3.4.7
+django-crum
 python-dateutil>=2.1
 django-webpack-loader>=0.6.0
 django-waffle

diff --git a/requirements/base.txt b/requirements/base.txt
@@ -6,11 +6,12 @@
 #
 certifi==2018.11.29       # via requests
 chardet==3.0.4            # via requests
+django-crum==0.7.3
 django-ipware==2.1.0
 django-model-utils==3.1.2
 django-waffle==0.15.0
 django-webpack-loader==0.6.0
-django==1.11.17
+django==1.11.18
 djangorestframework-jwt==1.11.0  # via edx-drf-extensions
 djangorestframework==3.6.4
 edx-django-utils==1.0.3   # via edx-drf-extensions
@@ -29,7 +30,7 @@ pyjwkest==1.3.2           # via edx-drf-extensions
 pyjwt==1.7.1              # via djangorestframework-jwt, edx-rest-api-client
 pymongo==3.7.2            # via edx-opaque-keys, event-tracking
 python-dateutil==2.7.5
-pytz==2018.7
+pytz==2018.9
 requests==2.21.0          # via edx-drf-extensions, edx-rest-api-client, pyjwkest, slumber
 rest-condition==1.0.3     # via edx-drf-extensions
 semantic-version==2.6.0   # via edx-drf-extensions

diff --git a/requirements/dev.txt b/requirements/dev.txt
@@ -13,19 +13,19 @@ certifi==2018.11.29       # via requests
 chardet==3.0.4            # via requests
 click-log==0.1.8          # via edx-lint
 click==7.0                # via click-log, edx-lint, pip-tools
-configparser==3.5.0       # via pydocstyle, pylint
+configparser==3.5.0       # via importlib-metadata, pydocstyle, pylint
 contextlib2==0.5.5        # via importlib-metadata
 diff-cover==1.0.6
 distlib==0.2.8            # via caniusepython3
-django==1.11.17
+django==1.11.18
 docutils==0.14            # via readme-renderer
 edx-i18n-tools==0.4.8
 edx_lint==0.5.5
 enum34==1.1.6             # via astroid
 filelock==3.0.10          # via tox
 futures==3.2.0            # via caniusepython3, isort
 idna==2.8                 # via requests
-importlib-metadata==0.7   # via path.py
+importlib-metadata==0.8   # via path.py
 inflect==2.1.0            # via jinja2-pluralize
 isort==4.3.4
 jinja2-pluralize==0.3.0   # via diff-cover
@@ -49,7 +49,7 @@ pylint-django==0.7.2      # via edx-lint
 pylint-plugin-utils==0.4  # via pylint-celery, pylint-django
 pylint==1.7.1             # via edx-lint, pylint-celery, pylint-django, pylint-plugin-utils
 pyparsing==2.3.0          # via packaging
-pytz==2018.7              # via django
+pytz==2018.9              # via django
 pyyaml==3.13              # via edx-i18n-tools
 readme-renderer==24.0     # via twine
 requests-toolbelt==0.8.0  # via twine
@@ -61,10 +61,11 @@ snowballstemmer==1.2.1    # via pydocstyle
 toml==0.10.0              # via tox
 tox-battery==0.5.1
 tox==3.6.1
-tqdm==4.28.1              # via twine
+tqdm==4.29.0              # via twine
 twine==1.12.1
 urllib3==1.24.1           # via requests
-virtualenv==16.1.0        # via tox
+virtualenv==16.2.0        # via tox
 webencodings==0.5.1       # via bleach
 wheel==0.32.3
 wrapt==1.10.11            # via astroid
+zipp==0.3.3               # via importlib-metadata
diff --git a/requirements/doc.txt b/requirements/doc.txt
@@ -9,11 +9,12 @@ babel==2.6.0              # via sphinx
 bleach==3.0.2             # via readme-renderer
 certifi==2018.11.29       # via requests
 chardet==3.0.4            # via doc8, requests
+django-crum==0.7.3
 django-ipware==2.1.0
 django-model-utils==3.1.2
 django-waffle==0.15.0
 django-webpack-loader==0.6.0
-django==1.11.17
+django==1.11.18
 djangorestframework-jwt==1.11.0  # via edx-drf-extensions
 djangorestframework==3.6.4
 doc8==0.8.0
@@ -42,11 +43,11 @@ pyjwt==1.7.1              # via djangorestframework-jwt, edx-rest-api-client
 pymongo==3.7.2            # via edx-opaque-keys, event-tracking
 pyparsing==2.3.0          # via packaging
 python-dateutil==2.7.5
-pytz==2018.7
+pytz==2018.9
 readme-renderer==24.0
 requests==2.21.0          # via edx-drf-extensions, edx-rest-api-client, pyjwkest, slumber, sphinx
 rest-condition==1.0.3     # via edx-drf-extensions
-restructuredtext-lint==1.2.1  # via doc8
+restructuredtext-lint==1.2.2  # via doc8
 semantic-version==2.6.0   # via edx-drf-extensions
 six==1.12.0               # via bleach, doc8, edx-drf-extensions, edx-opaque-keys, edx-sphinx-theme, packaging, pockets, pyjwkest, python-dateutil, readme-renderer, sphinx, sphinxcontrib-napoleon, stevedore
 slumber==0.7.1            # via edx-rest-api-client

diff --git a/requirements/quality.txt b/requirements/quality.txt
@@ -14,7 +14,7 @@ click-log==0.1.8          # via edx-lint
 click==7.0                # via click-log, edx-lint
 configparser==3.5.0       # via pydocstyle, pylint
 distlib==0.2.8            # via caniusepython3
-django==1.11.17
+django==1.11.18
 edx_lint==0.5.5
 enum34==1.1.6             # via astroid
 futures==3.2.0            # via caniusepython3, isort
@@ -30,7 +30,7 @@ pylint-django==0.7.2      # via edx-lint
 pylint-plugin-utils==0.4  # via pylint-celery, pylint-django
 pylint==1.7.1             # via edx-lint, pylint-celery, pylint-django, pylint-plugin-utils
 pyparsing==2.3.0          # via packaging
-pytz==2018.7              # via django
+pytz==2018.9              # via django
 requests==2.21.0          # via caniusepython3
 singledispatch==3.4.0.3   # via astroid, pylint
 six==1.12.0               # via astroid, edx-lint, packaging, pydocstyle, pylint, singledispatch