From 664af3d7ac5df0f530d6d74969e545f0877bf307 Mon Sep 17 00:00:00 2001 From: Michael Roytman Date: Thu, 24 Jan 2019 15:55:10 -0500 Subject: [PATCH] update backend.on_review_callback to not modify payload parameter --- edx_proctoring/backends/rest.py | 7 ---- edx_proctoring/backends/tests/test_rest.py | 39 +--------------------- edx_proctoring/tests/test_reviews.py | 32 ++++++++++++++++++ edx_proctoring/views.py | 7 +++- 4 files changed, 39 insertions(+), 46 deletions(-) diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index abb11473cba..f9bf9f22e33 100644 --- a/edx_proctoring/backends/rest.py +++ b/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): diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index 9afa36de1af..94f6240d5a4 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/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='someone@example.com' - ) - 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('someone@example.com') - new_payload = self.provider.on_review_callback(attempt, payload) - self.assertEqual(new_payload['reviewed_by'], person) - - payload = payload_with_email('nonexistent+person@example.com') - 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(), '') diff --git a/edx_proctoring/tests/test_reviews.py b/edx_proctoring/tests/test_reviews.py index f3f331a1606..364bd5d7d58 100644 --- a/edx_proctoring/tests/test_reviews.py +++ b/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 = 'testy@example.com' + 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) diff --git a/edx_proctoring/views.py b/edx_proctoring/views.py index 7d66f5714e1..1864558d423 100644 --- a/edx_proctoring/views.py +++ b/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()