From aed10d579a17c74074f390d8303436336b857621 Mon Sep 17 00:00:00 2001 From: Matt Hughes Date: Wed, 25 Sep 2019 13:42:56 -0400 Subject: [PATCH] Add exception and logging when backends return 200 but don't send id on exam attempt creation We have seen ProctorTrack (third party service) would responde with 200 saying the exam attempt is successful, but then the attempt ID is not sent to us in response. See https://openedx.atlassian.net/browse/CR-1277 --- edx_proctoring/__init__.py | 2 +- edx_proctoring/api.py | 55 ++++++++++++++++--- edx_proctoring/backends/rest.py | 9 ++- edx_proctoring/backends/software_secure.py | 2 +- edx_proctoring/backends/tests/test_backend.py | 9 ++- edx_proctoring/backends/tests/test_rest.py | 45 +++++++-------- edx_proctoring/exceptions.py | 13 ++++- edx_proctoring/tests/test_api.py | 12 ++++ package.json | 2 +- 9 files changed, 111 insertions(+), 38 deletions(-) diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py index 8de5a36be38..baf5ab86e70 100644 --- 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__ = '2.1.0' +__version__ = '2.1.1' default_app_config = 'edx_proctoring.apps.EdxProctoringConfig' # pylint: disable=invalid-name diff --git a/edx_proctoring/api.py b/edx_proctoring/api.py index 03473ab6c59..306c7125827 100644 --- a/edx_proctoring/api.py +++ b/edx_proctoring/api.py @@ -25,13 +25,21 @@ from edx_proctoring import constants from edx_proctoring.backends import get_backend_provider -from edx_proctoring.exceptions import (BackendProviderOnboardingException, ProctoredExamAlreadyExists, - ProctoredExamIllegalStatusTransition, ProctoredExamNotActiveException, - ProctoredExamNotFoundException, ProctoredExamPermissionDenied, - ProctoredExamReviewPolicyAlreadyExists, - ProctoredExamReviewPolicyNotFoundException, - StudentExamAttemptAlreadyExistsException, - StudentExamAttemptDoesNotExistsException, StudentExamAttemptedAlreadyStarted) +from edx_proctoring.exceptions import ( + BackendProviderCannotRegisterAttempt, + BackendProviderOnboardingException, + BackendProviderSentNoAttemptID, + ProctoredExamAlreadyExists, + ProctoredExamIllegalStatusTransition, + ProctoredExamNotActiveException, + ProctoredExamNotFoundException, + ProctoredExamPermissionDenied, + ProctoredExamReviewPolicyAlreadyExists, + ProctoredExamReviewPolicyNotFoundException, + StudentExamAttemptAlreadyExistsException, + StudentExamAttemptDoesNotExistsException, + StudentExamAttemptedAlreadyStarted, +) from edx_proctoring.models import (ProctoredExam, ProctoredExamReviewPolicy, ProctoredExamSoftwareSecureReview, ProctoredExamStudentAllowance, ProctoredExamStudentAttempt) from edx_proctoring.runtime import get_runtime_service @@ -629,8 +637,37 @@ def create_exam_attempt(exam_id, user_id, taking_as_proctored=False): exam, context=context, ) - except BackendProviderOnboardingException as exc: - force_status = exc.status + except BackendProviderSentNoAttemptID as ex: + log_message = ( + 'Failed to get the attempt ID for {user_id}' + 'in {exam_id} from the backend because the backend' + 'did not provide the id in API response, even when the' + 'HTTP response status is {status}, ' + 'Response: {response}'.format( + user_id=user_id, + exam_id=exam_id, + response=six.text_type(ex), + status=ex.http_status + ) + ) + log.error(log_message) + raise ex + except BackendProviderCannotRegisterAttempt as ex: + log_message = ( + 'Failed to create attempt for {user_id} ' + 'in {exam_id} because backend was unable ' + 'to register the attempt. Status: {status}, ' + 'Reponse: {response}'.format( + user_id=user_id, + exam_id=exam_id, + response=six.text_type(ex), + status=ex.http_status, + ) + ) + log.error(log_message) + raise ex + except BackendProviderOnboardingException as ex: + force_status = ex.status log_msg = ( 'Failed to create attempt for {user_id} ' 'in {exam_id} because of onboarding failure: ' diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index ca45cf15e36..189dc11d4bb 100644 --- a/edx_proctoring/backends/rest.py +++ b/edx_proctoring/backends/rest.py @@ -21,6 +21,7 @@ BackendProviderCannotRegisterAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, + BackendProviderSentNoAttemptID, ) from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus from edx_rest_api_client.client import OAuthAPIClient @@ -187,13 +188,17 @@ def register_exam_attempt(self, exam, context): log.debug('Creating exam attempt for %r at %r', exam['external_id'], url) response = self.session.post(url, json=payload) if response.status_code != 200: - raise BackendProviderCannotRegisterAttempt(response.content) + raise BackendProviderCannotRegisterAttempt(response.content, response.status_code) + status_code = response.status_code response = response.json() log.debug(response) onboarding_status = response.get('status', None) if onboarding_status in ProctoredExamStudentAttemptStatus.onboarding_errors: raise BackendProviderOnboardingException(onboarding_status) - return response['id'] + attempt_id = response.get('id', None) + if not attempt_id: + raise BackendProviderSentNoAttemptID(response, status_code) + return attempt_id def start_exam_attempt(self, exam, attempt): """ diff --git a/edx_proctoring/backends/software_secure.py b/edx_proctoring/backends/software_secure.py index 25c17581e12..7ac5a697179 100644 --- a/edx_proctoring/backends/software_secure.py +++ b/edx_proctoring/backends/software_secure.py @@ -94,7 +94,7 @@ def register_exam_attempt(self, exam, context): ) ) log.error(err_msg) - raise BackendProviderCannotRegisterAttempt(err_msg) + raise BackendProviderCannotRegisterAttempt(err_msg, status) # get the external ID that Software Secure has defined # for this attempt diff --git a/edx_proctoring/backends/tests/test_backend.py b/edx_proctoring/backends/tests/test_backend.py index e586c807abd..ba944100218 100644 --- a/edx_proctoring/backends/tests/test_backend.py +++ b/edx_proctoring/backends/tests/test_backend.py @@ -14,7 +14,11 @@ from edx_proctoring.backends.backend import ProctoringBackendProvider from edx_proctoring.backends.null import NullBackendProvider from edx_proctoring.backends.mock import MockProctoringBackendProvider -from edx_proctoring.exceptions import BackendProviderCannotRetireUser, BackendProviderOnboardingException +from edx_proctoring.exceptions import ( + BackendProviderCannotRetireUser, + BackendProviderOnboardingException, + BackendProviderSentNoAttemptID, +) # pragma pylint: disable=useless-super-delegation @@ -30,6 +34,7 @@ class TestBackendProvider(ProctoringBackendProvider): last_retire_user = None attempt_error = None last_attempt_remove = None + no_attempt_id_error = None def register_exam_attempt(self, exam, context): """ @@ -37,6 +42,8 @@ def register_exam_attempt(self, exam, context): """ if self.attempt_error: raise BackendProviderOnboardingException(self.attempt_error) + elif self.no_attempt_id_error: + raise BackendProviderSentNoAttemptID(self.no_attempt_id_error, http_status=200) return 'testexternalid' def start_exam_attempt(self, exam, attempt): diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index a628ed8d9dd..eadcd01b629 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/edx_proctoring/backends/tests/test_rest.py @@ -17,6 +17,7 @@ BackendProviderCannotRegisterAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, + BackendProviderSentNoAttemptID, ) from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus @@ -45,6 +46,12 @@ def setUp(self): 'is_proctored': True, 'is_practice': False, } + self.register_exam_context = { + 'attempt_code': '2', + 'obs_user_id': 'abcdefghij', + 'full_name': 'user name', + 'lms_host': 'http://lms.com' + } @responses.activate def test_get_software_download_url(self): @@ -174,18 +181,13 @@ def test_bad_exam_save(self): @responses.activate def test_register_exam_attempt(self): - context = { - 'attempt_code': '2', - 'obs_user_id': 'abcdefghij', - 'full_name': 'user name', - 'lms_host': 'http://lms.com' - } responses.add( responses.POST, url=self.provider.create_exam_attempt_url.format(exam_id=self.backend_exam['external_id']), - json={'id': 2} + json={'id': 2}, + status=200 ) - attempt_external_id = self.provider.register_exam_attempt(self.backend_exam, context) + attempt_external_id = self.provider.register_exam_attempt(self.backend_exam, self.register_exam_context) request = json.loads(responses.calls[1].request.body) self.assertEqual(attempt_external_id, 2) self.assertEqual(request['status'], 'created') @@ -194,12 +196,6 @@ def test_register_exam_attempt(self): @responses.activate def test_register_exam_attempt_failure(self): - context = { - 'attempt_code': '2', - 'obs_user_id': 'abcdefghij', - 'full_name': 'user name', - 'lms_host': 'http://lms.com' - } responses.add( responses.POST, url=self.provider.create_exam_attempt_url.format(exam_id=self.backend_exam['external_id']), @@ -207,19 +203,13 @@ def test_register_exam_attempt_failure(self): status=400 ) with self.assertRaises(BackendProviderCannotRegisterAttempt): - self.provider.register_exam_attempt(self.backend_exam, context) + self.provider.register_exam_attempt(self.backend_exam, self.register_exam_context) @ddt.data( *ProctoredExamStudentAttemptStatus.onboarding_errors ) @responses.activate def test_attempt_failure_onboarding(self, failure_status): - context = { - 'attempt_code': '2', - 'obs_user_id': 'abcdefghij', - 'full_name': 'user name', - 'lms_host': 'http://lms.com' - } responses.add( responses.POST, url=self.provider.create_exam_attempt_url.format(exam_id=self.backend_exam['external_id']), @@ -227,10 +217,21 @@ def test_attempt_failure_onboarding(self, failure_status): status=200 ) with self.assertRaises(BackendProviderOnboardingException) as exc_manager: - self.provider.register_exam_attempt(self.backend_exam, context) + self.provider.register_exam_attempt(self.backend_exam, self.register_exam_context) exception = exc_manager.exception assert exception.status == failure_status + @responses.activate + def test_attempt_no_id_returned(self): + responses.add( + responses.POST, + url=self.provider.create_exam_attempt_url.format(exam_id=self.backend_exam['external_id']), + json={'error': 'something'}, + status=200 + ) + with self.assertRaises(BackendProviderSentNoAttemptID): + self.provider.register_exam_attempt(self.backend_exam, self.register_exam_context) + @ddt.data( ['start_exam_attempt', 'start'], ['stop_exam_attempt', 'stop'], diff --git a/edx_proctoring/exceptions.py b/edx_proctoring/exceptions.py index c767194b176..06c8240def0 100644 --- a/edx_proctoring/exceptions.py +++ b/edx_proctoring/exceptions.py @@ -76,6 +76,17 @@ class BackendProviderCannotRegisterAttempt(ProctoredBaseException): Raised when a back-end provider cannot register an attempt """ + def __init__(self, content, http_status): + super(BackendProviderCannotRegisterAttempt, self).__init__(self, content) + self.http_status = http_status + + +class BackendProviderSentNoAttemptID(BackendProviderCannotRegisterAttempt): + """ + Raised when a back-end provider returns a JSON without exam ID + in response to new exam attempt registration + """ + class BackendProviderOnboardingException(ProctoredBaseException): """ @@ -83,7 +94,7 @@ class BackendProviderOnboardingException(ProctoredBaseException): because of missing/failed onboarding requirements """ def __init__(self, status): - ProctoredBaseException.__init__(self, status) + super(BackendProviderOnboardingException, self).__init__(self, status) self.status = status diff --git a/edx_proctoring/tests/test_api.py b/edx_proctoring/tests/test_api.py index c83f77d67f6..b6308633602 100644 --- a/edx_proctoring/tests/test_api.py +++ b/edx_proctoring/tests/test_api.py @@ -57,6 +57,7 @@ ) from edx_proctoring.constants import DEFAULT_CONTACT_EMAIL from edx_proctoring.exceptions import ( + BackendProviderSentNoAttemptID, ProctoredExamAlreadyExists, ProctoredExamNotFoundException, StudentExamAttemptAlreadyExistsException, @@ -560,6 +561,17 @@ def test_attempt_onboarding_error(self, onboarding_error): assert attempt['status'] == onboarding_error test_backend.attempt_error = None + def test_attempt_without_attempt_id(self): + """ + Test that the exam attempt is throwing exceptions when the attempt id is not included in the + API response from proctoring backend + """ + test_backend = get_backend_provider(name='test') + test_backend.no_attempt_id_error = 'No id returned' + with self.assertRaises(BackendProviderSentNoAttemptID): + create_exam_attempt(self.proctored_exam_id, self.user_id, taking_as_proctored=True) + test_backend.no_attempt_id_error = None + def test_no_existing_attempt(self): """ Make sure we get back a None when calling get_exam_attempt_by_id() with a non existing attempt diff --git a/package.json b/package.json index 6959422de82..3ec15ca0a2e 100644 --- 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": "2.1.0", + "version": "2.1.1", "main": "edx_proctoring/static/index.js", "repository": { "type": "git",