Merge pull request #518 from edx/mroytman/update-on-review-callback

update backend.on_review_callback to not modify payload parameter

edx_proctoring/backends/rest.py

@@ -11,8 +11,6 @@
 from webpack_loader.utils import get_files
 from webpack_loader.exceptions import BaseWebpackLoaderException, WebpackBundleLookupError
 
-from django.contrib.auth.models import User
-
 from edx_proctoring.backends.backend import ProctoringBackendProvider
 from edx_proctoring.exceptions import BackendProviderCannotRegisterAttempt, BackendProviderCannotRetireUser
 from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus
@@ -218,11 +216,6 @@ def on_review_callback(self, attempt, payload):
         Called when the reviewing 3rd party service posts back the results
         """
         # REST backends should convert the payload into the expected data structure
-        if payload.get('reviewed_by', False):
-            try:
-                payload['reviewed_by'] = User.objects.get(email=payload['reviewed_by'])
-            except User.DoesNotExist:
-                payload['reviewed_by'] = None
         return payload
 
     def on_exam_saved(self, exam):

edx_proctoring/backends/tests/test_rest.py

@@ -11,7 +11,6 @@
 
 from django.test import TestCase
 from django.utils import translation
-from django.contrib.auth.models import User
 
 from edx_proctoring.backends.rest import BaseRestProctoringProvider
 from edx_proctoring.exceptions import BackendProviderCannotRegisterAttempt, BackendProviderCannotRetireUser
@@ -218,7 +217,7 @@ def test_update_exam_attempt_status(self, provider_method_name, corresponding_st
 
     def test_on_review_callback(self):
         """
-        on_review_callback should just return the payload when called without review author
+        on_review_callback should just return the payload
         """
         attempt = {
             'id': 1,
@@ -234,42 +233,6 @@ def test_on_review_callback(self):
         new_payload = self.provider.on_review_callback(attempt, payload)
         self.assertEqual(payload, new_payload)
 
-    def test_on_review_callback_with_reviewer(self):
-        """
-        on_review_callback should find a user if an email is provided
-        """
-        person = User(
-            username='tester',
-            email='[email protected]'
-        )
-        person.save()
-        attempt = {
-            'id': 1,
-            'external_id': 'abcd',
-            'user': 1
-        }
-
-        def payload_with_email(email):
-            """
-            generic payload with variable email
-            """
-            return {
-                'status': 'verified',
-                'comments': [
-                    {'comment': 'something happened', 'status': 'ok'},
-                ],
-                'reviewed_by': email,
-            }
-
-        payload = payload_with_email('[email protected]')
-        new_payload = self.provider.on_review_callback(attempt, payload)
-        self.assertEqual(new_payload['reviewed_by'], person)
-
-        payload = payload_with_email('[email protected]')
-        new_payload = self.provider.on_review_callback(attempt, payload)
-
-        self.assertEqual(new_payload['reviewed_by'], None)
-
     def test_get_javascript(self):
         self.assertEqual(self.provider.get_javascript(), '')
 

edx_proctoring/tests/test_reviews.py

@@ -9,6 +9,7 @@
 import ddt
 from mock import patch
 
+from django.contrib.auth.models import User
 from django.test import RequestFactory
 from django.urls import reverse
 
@@ -382,3 +383,34 @@ def test_clean_status(self):
 
             attempt = get_exam_attempt_by_id(self.attempt_id)
             self.assertEqual(attempt['status'], ProctoredExamStudentAttemptStatus.second_review_required)
+
+    def test_status_reviewed_by_field(self):
+        """
+        Test that `reviewed_by` field of Review model is correctly assigned to None or to a User object.
+        """
+        # no reviewed_by field
+        test_payload = self.get_review_payload(ReviewStatus.suspicious)
+        # submit a Suspicious review payload
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
+
+        review = ProctoredExamSoftwareSecureReview.objects.get(attempt_code=self.attempt['attempt_code'])
+        self.assertIsNone(review.reviewed_by)
+
+        # reviewed_by field with no corresponding User object
+        reviewed_by_email = '[email protected]'
+        test_payload['reviewed_by'] = reviewed_by_email
+
+        # submit a Suspicious review payload
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
+        review = ProctoredExamSoftwareSecureReview.objects.get(attempt_code=self.attempt['attempt_code'])
+        self.assertIsNone(review.reviewed_by)
+
+        # reviewed_by field with corresponding User object
+        user = User.objects.create(
+            email=reviewed_by_email,
+            username='TestyMcTesterson'
+        )
+        # submit a Suspicious review payload
+        ProctoredExamReviewCallback().make_review(self.attempt, test_payload)
+        review = ProctoredExamSoftwareSecureReview.objects.get(attempt_code=self.attempt['attempt_code'])
+        self.assertEqual(review.reviewed_by, user)

edx_proctoring/views.py

@@ -11,6 +11,7 @@
 
 from crum import get_current_request
 from django.conf import settings
+from django.contrib.auth.models import User
 from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger
 from django.urls import reverse, NoReverseMatch
 from django.shortcuts import redirect
@@ -844,7 +845,11 @@ def make_review(self, attempt, data, backend=None):
         review.raw_data = json.dumps(data)
         review.student_id = attempt['user']['id']
         review.exam_id = attempt['proctored_exam']['id']
-        review.reviewed_by = backend_review.get('reviewed_by', None)
+
+        try:
+            review.reviewed_by = User.objects.get(email=data['reviewed_by'])
+        except (User.DoesNotExist, KeyError):
+            review.reviewed_by = None
 
         review.save()