feat: latest attempt endpoint fixes (#158)

* feat: latest attempt fixes for edx-proctoring

* test: move factories to core

* style: code review

edx_exams/apps/api/test_utils/__init__.py

@@ -4,9 +4,9 @@
 
 from rest_framework.test import APIClient, APITestCase
 
-from edx_exams.apps.api.test_utils.factories import UserFactory
 from edx_exams.apps.api.test_utils.mixins import JwtMixin
 from edx_exams.apps.core.models import ProctoringProvider
+from edx_exams.apps.core.test_utils.factories import UserFactory
 
 TEST_USERNAME = 'api_worker'
 TEST_EMAIL = '[email protected]'

edx_exams/apps/api/v1/tests/test_views.py

@@ -15,17 +15,17 @@
 
 from edx_exams.apps.api.serializers import ExamSerializer, StudentAttemptSerializer
 from edx_exams.apps.api.test_utils import ExamsAPITestCase
-from edx_exams.apps.api.test_utils.factories import (
+from edx_exams.apps.core.exam_types import get_exam_type
+from edx_exams.apps.core.exceptions import ExamAttemptOnPastDueExam, ExamIllegalStatusTransition
+from edx_exams.apps.core.models import CourseExamConfiguration, Exam, ExamAttempt, ProctoringProvider
+from edx_exams.apps.core.statuses import ExamAttemptStatus
+from edx_exams.apps.core.test_utils.factories import (
     AssessmentControlResultFactory,
     CourseExamConfigurationFactory,
     ExamAttemptFactory,
     ExamFactory,
     UserFactory
 )
-from edx_exams.apps.core.exam_types import get_exam_type
-from edx_exams.apps.core.exceptions import ExamAttemptOnPastDueExam, ExamIllegalStatusTransition
-from edx_exams.apps.core.models import CourseExamConfiguration, Exam, ExamAttempt, ProctoringProvider
-from edx_exams.apps.core.statuses import ExamAttemptStatus
 
 
 @ddt.ddt
@@ -899,13 +899,15 @@ def setUp(self):
         self.other_user = UserFactory()
         self.now = timezone.now()
 
-    def get_api(self, user):
+    def get_api(self, user, content_id=None):
         """
         Helper function to make get request to the API
         """
 
         headers = self.build_jwt_headers(user)
         url = reverse('api:v1:exams-attempt-latest')
+        if content_id:
+            url += f'?content_id={content_id}'
         return self.client.get(url, **headers, content_type='application/json')
 
     def create_mock_attempt(self, user, status, start_time, allowed_time_limit_mins):
@@ -922,16 +924,16 @@ def create_mock_attempt(self, user, status, start_time, allowed_time_limit_mins)
         )
 
     @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
-    @patch('edx_exams.apps.api.v1.views.get_latest_attempt_for_user')
-    def test_get_latest_exam_attempt_for_user(self, mock_get_latest_attempt_for_user, mock_check_if_exam_timed_out):
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
+    def test_get_without_content_id(self, mock_get_active_attempt_for_user, mock_check_if_exam_timed_out):
         """
         Test that the GET function in the ExamAttempt view returns
         the expected exam attempt for a user with status 200
         """
 
         mock_attempt = self.create_mock_attempt(self.user, ExamAttemptStatus.started, self.now, 60)
 
-        mock_get_latest_attempt_for_user.return_value = mock_attempt
+        mock_get_active_attempt_for_user.return_value = mock_attempt
         mock_check_if_exam_timed_out.return_value = mock_attempt
 
         response = self.get_api(self.user)
@@ -940,27 +942,51 @@ def test_get_latest_exam_attempt_for_user(self, mock_get_latest_attempt_for_user
 
         # Assert attempt id's are the same, as time_remaining_seconds will differ
         self.assertEqual(response.data.get('attempt_id'), mock_attempt.id)
-        mock_get_latest_attempt_for_user.assert_called_once_with(mock_attempt.user.id)
+        mock_get_active_attempt_for_user.assert_called_once_with(mock_attempt.user.id)
+        mock_check_if_exam_timed_out.assert_called_once_with(mock_attempt)
+
+    @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
+    @patch('edx_exams.apps.api.v1.views.get_exam_by_content_id')
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
+    @patch('edx_exams.apps.api.v1.views.get_current_exam_attempt')
+    def test_get_with_content_id(self, mock_get_current_exam_attempt, mock_get_active_attempt_for_user,
+                                 mock_get_exam_by_content_id, mock_check_if_exam_timed_out):
+        """
+        GET with a content_id query parameter should return the latest attempt for that exam
+        """
+        mock_attempt = self.create_mock_attempt(self.user, ExamAttemptStatus.started, self.now, 60)
+        mock_get_current_exam_attempt.return_value = mock_attempt
+        mock_check_if_exam_timed_out.return_value = mock_attempt
+        mock_get_exam_by_content_id.return_value = self.exam
+
+        response = self.get_api(self.user, mock_attempt.exam.content_id)
+
+        self.assertEqual(response.status_code, 200)
+
+        # Assert attempt id's are the same, as time_remaining_seconds will differ
+        self.assertEqual(response.data.get('attempt_id'), mock_attempt.id)
+        mock_get_current_exam_attempt.assert_called_once_with(self.user.id, self.exam.id)
+        mock_get_active_attempt_for_user.assert_not_called()
         mock_check_if_exam_timed_out.assert_called_once_with(mock_attempt)
 
     @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
-    @patch('edx_exams.apps.api.v1.views.get_latest_attempt_for_user')
-    def test_cannot_get_attempts_for_other_user(self, mock_get_latest_attempt_for_user, mock_check_if_exam_timed_out):
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
+    def test_cannot_get_attempts_for_other_user(self, mock_get_active_attempt_for_user, mock_check_if_exam_timed_out):
         """
         Test that a user cannot view the exam attempts of another user
         """
 
         current_users_attempt = self.create_mock_attempt(self.user, ExamAttemptStatus.started, self.now, 60)
         other_users_attempt = self.create_mock_attempt(self.other_user, ExamAttemptStatus.submitted, self.now, 60)
 
-        mock_get_latest_attempt_for_user.return_value = current_users_attempt
+        mock_get_active_attempt_for_user.return_value = current_users_attempt
         mock_check_if_exam_timed_out.return_value = current_users_attempt
         response = self.get_api(self.user)
 
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data.get('attempt_id'), current_users_attempt.id)
         self.assertNotEqual(response.data.get('attempt_id'), other_users_attempt.id)
-        mock_get_latest_attempt_for_user.assert_called_once_with(self.user.id)
+        mock_get_active_attempt_for_user.assert_called_once_with(self.user.id)
 
     @patch('edx_exams.apps.api.v1.views.get_active_exam_attempt')
     def test_no_attempts_for_user(self, mock_get_legacy_active_exam_attempt):
@@ -974,8 +1000,8 @@ def test_no_attempts_for_user(self, mock_get_legacy_active_exam_attempt):
         self.assertEqual(response.data, {})
 
     @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
-    @patch('edx_exams.apps.api.v1.views.get_latest_attempt_for_user')
-    def test_submit_on_timeout(self, mock_get_latest_attempt_for_user, mock_check_if_exam_timed_out):
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
+    def test_submit_on_timeout(self, mock_get_active_attempt_for_user, mock_check_if_exam_timed_out):
         """
         Test if the view returns an exam whose status is changed to be submitted
         """
@@ -990,7 +1016,7 @@ def test_submit_on_timeout(self, mock_get_latest_attempt_for_user, mock_check_if
         self.create_mock_attempt(self.user, ExamAttemptStatus.started, three_days_ago, None)
 
         # Mock return of attempt data before it is updated
-        mock_get_latest_attempt_for_user.return_value = current_attempt
+        mock_get_active_attempt_for_user.return_value = current_attempt
         # Update attempt to 'submitted'
         ExamAttempt.objects.update_or_create(start_time=one_hour_ago, defaults={'status': ExamAttemptStatus.submitted})
         # Mock return attempt data after it is updated
@@ -999,20 +1025,20 @@ def test_submit_on_timeout(self, mock_get_latest_attempt_for_user, mock_check_if
         response = self.get_api(self.user)
 
         self.assertEqual(response.data, StudentAttemptSerializer(current_attempt).data)
-        mock_get_latest_attempt_for_user.assert_called_once_with(self.user.id)
+        mock_get_active_attempt_for_user.assert_called_once_with(self.user.id)
         mock_check_if_exam_timed_out.assert_called_once_with(current_attempt)
 
     @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
-    @patch('edx_exams.apps.api.v1.views.get_latest_attempt_for_user')
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
     @patch('edx_exams.apps.api.v1.views.get_active_exam_attempt')
     def test_active_legacy_service_attempt(self, mock_get_legacy_active_exam_attempt,
-                                           mock_get_latest_attempt_for_user, mock_check_if_exam_timed_out):
+                                           mock_get_active_attempt_for_user, mock_check_if_exam_timed_out):
         """
         Test that if the user has an active edx-proctoring attempt, the endpoint returns it
         """
 
         mock_attempt = self.create_mock_attempt(self.user, ExamAttemptStatus.created, None, 60)
-        mock_get_latest_attempt_for_user.return_value = mock_attempt
+        mock_get_active_attempt_for_user.return_value = mock_attempt
         mock_check_if_exam_timed_out.return_value = mock_attempt
 
         mock_get_legacy_active_exam_attempt.return_value = ({'attempt_id': 123}, 200)
@@ -1023,16 +1049,16 @@ def test_active_legacy_service_attempt(self, mock_get_legacy_active_exam_attempt
         self.assertEqual(response.data.get('attempt_id'), 123)
 
     @patch('edx_exams.apps.api.v1.views.check_if_exam_timed_out')
-    @patch('edx_exams.apps.api.v1.views.get_latest_attempt_for_user')
+    @patch('edx_exams.apps.api.v1.views.get_active_attempt_for_user')
     @patch('edx_exams.apps.api.v1.views.get_active_exam_attempt')
     def test_ignore_legacy_service_errors(self, mock_get_legacy_active_exam_attempt,
-                                          mock_get_latest_attempt_for_user, mock_check_if_exam_timed_out):
+                                          mock_get_active_attempt_for_user, mock_check_if_exam_timed_out):
         """
         Test that if edx-proctoring returns an error we ignore the response and return latest attempt
         """
 
         mock_attempt = self.create_mock_attempt(self.user, ExamAttemptStatus.submitted, None, 60)
-        mock_get_latest_attempt_for_user.return_value = mock_attempt
+        mock_get_active_attempt_for_user.return_value = mock_attempt
         mock_check_if_exam_timed_out.return_value = mock_attempt
 
         mock_get_legacy_active_exam_attempt.return_value = ('bad things', 500)

edx_exams/apps/api/v1/views.py

@@ -26,13 +26,13 @@
 from edx_exams.apps.core.api import (
     check_if_exam_timed_out,
     create_exam_attempt,
+    get_active_attempt_for_user,
     get_attempt_by_id,
     get_course_exams,
     get_current_exam_attempt,
     get_exam_attempt_time_remaining,
     get_exam_attempts,
     get_exam_by_content_id,
-    get_latest_attempt_for_user,
     get_provider_by_exam_id,
     is_exam_passed_due,
     update_attempt_status
@@ -412,11 +412,15 @@ class LatestExamAttemptView(ExamsAPIView):
         HTTP GET: Get the data for a user's latest exam attempt.
 
     HTTP GET
-    Fetches a user's latest exam attempt.
+    Fetches a user's latest exam attempt for a given exam.
+    If no exam is provided, the actively running attempt is returned.
     Status changes to 'submitted' if time remaining is zero.
 
     **GET data Parameters**
         'user': The data of the user whose latest attempt we want to fetch.
+        'exam_id': The id of the exam whose latest attempt we want to fetch.
+        'content_id: (optional) If provided, return the state of the user's attempt
+            for this exam regardless of state.
 
     **Returns**
     {
@@ -440,36 +444,48 @@ def get(self, request):
         """
         HTTP GET handler to fetch all exam attempt data
 
+        TODO: The endpoint should be refactored on deprecation of edx-proctoring.
+        Most of the complexity here has to do with the fact that we are supporting
+        slightly different API behaviors from that service.
+
         Parameters:
             None
 
         Returns:
             A Response object containing all `ExamAttempt` data.
         """
         user = request.user
-        latest_attempt = get_latest_attempt_for_user(user.id)
-        serialized_attempt = StudentAttemptSerializer(latest_attempt)
-
-        # if there is an active attempt in this service, return it
-        if latest_attempt is not None:
-            latest_attempt = check_if_exam_timed_out(latest_attempt)
-            serialized_attempt = StudentAttemptSerializer(latest_attempt)
-            if latest_attempt.status in (ExamAttemptStatus.started, ExamAttemptStatus.ready_to_submit):
+        exam_content_id = request.GET.get('content_id', None)
+        exam = get_exam_by_content_id(exam_content_id)
+
+        # if a specific exam is requested always return that exam
+        if exam is not None:
+            attempt = get_current_exam_attempt(user.id, exam.id)
+        else:
+            attempt = get_active_attempt_for_user(user.id)
+
+        # if there is an active attempt in this service, return it.
+        if attempt is not None:
+            # An in progress attempt may be moved to 'submitted' if check_if_exam_timed_out
+            # determines that the attempt has timed out.
+            attempt = check_if_exam_timed_out(attempt)
+            serialized_attempt = StudentAttemptSerializer(attempt)
+            if attempt.status in (ExamAttemptStatus.started, ExamAttemptStatus.ready_to_submit):
                 return Response(status=status.HTTP_200_OK, data=serialized_attempt.data)
 
         # if edx-proctoring has an active attempt, return it
-        latest_attempt_legacy, response_status = get_active_exam_attempt(user.lms_user_id)
+        legacy_attempt, response_status = get_active_exam_attempt(user.lms_user_id)
         if (
-            latest_attempt_legacy is not None
-            and latest_attempt_legacy != {}
+            legacy_attempt is not None
+            and legacy_attempt != {}
             and response_status == status.HTTP_200_OK
         ):
-            return Response(status=status.HTTP_200_OK, data=latest_attempt_legacy)
+            return Response(status=status.HTTP_200_OK, data=legacy_attempt)
 
-        # otherwise return the latest attempt here regardless of status
+        # otherwise return the attempt from edx-exams regardless of status
         return Response(
             status=status.HTTP_200_OK,
-            data=serialized_attempt.data if latest_attempt is not None else {}
+            data=StudentAttemptSerializer(attempt).data if attempt else {}
         )
 
 
@@ -669,11 +685,11 @@ class CourseExamAttemptView(ExamsAPIView):
     authentication_classes = (JwtAuthentication,)
     permission_classes = (IsAuthenticated,)
 
-    def get(self, request, course_id, content_id):
+    def get(self, request, course_id, content_id):  # pylint: disable=unused-argument
         """
         HTTP GET handler. Returns exam and an attempt, if one exists for the exam
         """
-        exam = get_exam_by_content_id(course_id, content_id)
+        exam = get_exam_by_content_id(content_id)
 
         if exam is None:
             data = {'exam': {}}

edx_exams/apps/core/api.py

@@ -38,14 +38,11 @@ def get_attempt_by_id(attempt_id):
     return attempt
 
 
-def get_latest_attempt_for_user(user_id):
+def get_active_attempt_for_user(user_id):
     """
-    Function to fetch a user's latest exam attempt
+    Find the currently running attempt for a user if it exists.
     """
-
-    latest_attempt = ExamAttempt.get_latest_attempt_for_user(user_id)
-
-    return latest_attempt
+    return ExamAttempt.get_active_attempt_for_user(user_id)
 
 
 def get_attempt_for_user_with_attempt_number_and_resource_id(user_id, attempt_number, resource_id):
@@ -288,12 +285,12 @@ def create_exam_attempt(exam_id, user_id):
     return attempt.id
 
 
-def get_exam_by_content_id(course_id, content_id):
+def get_exam_by_content_id(content_id):
     """
-    Retrieve an exam filtered by a course_id and content_id
+    Retrieve an exam filtered by content_id
     """
     try:
-        exam = Exam.objects.get(course_id=course_id, content_id=content_id, is_active=True)
+        exam = Exam.objects.get(content_id=content_id, is_active=True)
         return exam
     except Exam.DoesNotExist:
         return None

edx_exams/apps/core/models.py

@@ -210,17 +210,25 @@ def get_attempt_by_id(cls, attempt_id):
         return attempt
 
     @classmethod
-    def get_latest_attempt_for_user(cls, user_id):
+    def get_active_attempt_for_user(cls, user_id):
         """
-        Return latest ExamAttempt (based on start time) associated with a given user_id
+        Return currently running attempt associated with a given user_id
 
         If start_time does not exist for any attempt, return None
         """
         try:
-            attempt = cls.objects.filter(user_id=user_id).latest('start_time', 'created')
-        except cls.DoesNotExist:
-            attempt = None
-        return attempt
+            return cls.objects.get(
+                user_id=user_id,
+                status__in=(ExamAttemptStatus.started, ExamAttemptStatus.ready_to_submit),
+            )
+        except ExamAttempt.DoesNotExist:
+            return None
+        except ExamAttempt.MultipleObjectsReturned:
+            log.error(
+                'Multiple attempts found for user_id=%(user_id)s with status IN_PROGRESS or READY_TO_SUBMIT',
+                {'user_id': user_id},
+            )
+            return None
 
     @classmethod
     def check_no_other_active_attempts_for_user(cls, user_id, attempt_id):