Change use of get_active_enrollments_by_course method of the LMS Enro…

…llments service to get_enrollments_can_take_proctored_exams, which is more performant. This shifts the responsibility of checking learners' ability to access proctored exams to the LMS, allowing the LMS to construst a bulk query for all learners in a course with active enrollments instead of needing to execute multiple queries on a per learner basis.
openedx · Mar 2, 2021 · 8d8f920 · 8d8f920
1 parent 34f03ad
commit 8d8f920
Show file tree

Hide file tree

Showing 8 changed files with 31 additions and 221 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -14,6 +14,14 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[3.7.3] - 2021-03-02
+~~~~~~~~~~~~~~~~~~~~
+* Change use of get_active_enrollments_by_course method of the LMS Enrollments service to
+  get_enrollments_can_take_proctored_exams, which is more performant. This shifts the responsibility
+  of checking learners' ability to access proctored exams to the LMS, allowing the LMS to construst a
+  bulk query for all learners in a course with active enrollments instead of needing to execute multiple
+  queries on a per learner basis.
+
 [3.7.2] - 2021-03-02
 ~~~~~~~~~~~~~~~~~~~~
 * Refactor the proctoring API function to get all verified onboarding attempts of a group of learners.

diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py
@@ -3,6 +3,6 @@
 """
 
 # Be sure to update the version number in edx_proctoring/package.json
-__version__ = '3.7.2'
+__version__ = '3.7.3'
 
 default_app_config = 'edx_proctoring.apps.EdxProctoringConfig'  # pylint: disable=invalid-name
diff --git a/edx_proctoring/api.py b/edx_proctoring/api.py
@@ -2521,12 +2521,13 @@ def get_integration_specific_email(provider):
     return getattr(provider, 'integration_specific_email', None) or constants.DEFAULT_CONTACT_EMAIL
 
 
-def get_enrollments_for_course(course_id):
+def get_enrollments_can_take_proctored_exams(course_id, text_search=None):
     """
     Return all enrollments for a course from the LMS Enrollments runtime API.
 
     Parameters:
     * course_id: course ID for the course
+    * text_search: the string against which to do a match on users' username or email; optional
     """
-    enrollments_sevice = get_runtime_service('enrollments')
-    return enrollments_sevice.get_active_enrollments_by_course(course_id)
+    enrollments_service = get_runtime_service('enrollments')
+    return enrollments_service.get_enrollments_can_take_proctored_exams(course_id, text_search)
diff --git a/edx_proctoring/tests/test_api.py b/edx_proctoring/tests/test_api.py
@@ -32,7 +32,7 @@
     get_attempt_status_summary,
     get_backend_provider,
     get_current_exam_attempt,
-    get_enrollments_for_course,
+    get_enrollments_can_take_proctored_exams,
     get_exam_attempt_by_id,
     get_exam_by_content_id,
     get_exam_by_id,
@@ -2454,7 +2454,7 @@ def test_get_integration_specific_email(self):
         del test_backend.integration_specific_email
         assert get_integration_specific_email(test_backend) == DEFAULT_CONTACT_EMAIL
 
-    def test_get_enrollments(self):
+    def test_get_enrollments_can_take_proctored_exams(self):
         enrollments = [
             {
                 'user': 'user_1',
@@ -2469,11 +2469,11 @@ def test_get_enrollments(self):
         expected_enrollments = [enrollment['user'] for enrollment in enrollments]
 
         with patch(
-                'edx_proctoring.tests.test_services.MockEnrollmentsService.get_active_enrollments_by_course',
+                'edx_proctoring.tests.test_services.MockEnrollmentsService.get_enrollments_can_take_proctored_exams',
                 return_value=expected_enrollments
         ):
             set_runtime_service('enrollments', MockEnrollmentsService(enrollments))
-            self.assertEqual(expected_enrollments, get_enrollments_for_course('course_id'))
+            self.assertEqual(expected_enrollments, get_enrollments_can_take_proctored_exams('course_id'))
 
     @ddt.data(
         (ProctoredExamStudentAttemptStatus.verified, ProctoredExamStudentAttemptStatus.declined, True),

diff --git a/edx_proctoring/tests/test_services.py b/edx_proctoring/tests/test_services.py
@@ -326,3 +326,7 @@ def __init__(self, enrollments):
     def get_active_enrollments_by_course(self, course_id):
         """Returns mock enrollments"""
         return self.enrollments
+
+    def get_enrollments_can_take_proctored_exams(self, course_id, text_search=None):
+        """ Return mock enrollments"""
+        return self.enrollments
diff --git a/edx_proctoring/tests/test_views.py b/edx_proctoring/tests/test_views.py
@@ -855,182 +855,6 @@ def test_pagination_maintains_query_params(self):
         self.assertIn(text_search_string, next_url)
         self.assertIn(statuses_filter_string, next_url)
 
-    def test_partial_text_search_username(self):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': 'use',
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [
-                {
-                    'username': self.learner_1.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                },
-                {
-                    'username': self.learner_2.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                }
-            ],
-            'count': 2,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
-    def test_partial_text_search_email(self):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': 'learner',
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [
-                {
-                    'username': self.learner_1.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                },
-                {
-                    'username': self.learner_2.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                }
-            ],
-            'count': 2,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
-    def test_full_text_search_email(self):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': '[email protected]',
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [
-                {
-                    'username': self.learner_1.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                },
-            ],
-            'count': 1,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
-    def test_full_text_search_username(self):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': 'user1',
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [
-                {
-                    'username': self.learner_1.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                },
-            ],
-            'count': 1,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
-    @ddt.data(
-        'USER1',
-        'LeaRner_1',
-    )
-    def test_text_search_case_insensitivity(self, text_search):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': text_search,
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [
-                {
-                    'username': self.learner_1.username,
-                    'status': InstructorDashboardOnboardingAttemptStatus.not_started,
-                    'modified': None,
-                },
-            ],
-            'count': 1,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
-    def test_text_search_no_results(self):
-        response = self.client.get(reverse(
-                'edx_proctoring:user_onboarding.status.course',
-                kwargs={'course_id': self.onboarding_exam.course_id}
-            ),
-            {
-                'text_search': 'abc',
-            }
-        )
-        response_data = json.loads(response.content.decode('utf-8'))
-
-        expected_data = {
-            'results': [],
-            'count': 0,
-            'previous': None,
-            'next': None,
-            'num_pages': 1,
-        }
-
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response_data, expected_data)
-
     def test_one_status_filter(self):
         first_attempt_id = create_exam_attempt(self.onboarding_exam.id, self.user.id, True)
         second_attempt_id = create_exam_attempt(self.onboarding_exam.id, self.learner_1.id, True)
@@ -1262,9 +1086,10 @@ def test_multiple_exam_attempts(self):
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response_data, expected_data)
 
-    def test_ineligible_for_exam(self):
-        with mock_perm('edx_proctoring.can_take_proctored_exam'):
-            response = self.client.get(reverse(
+    def test_no_enrollments(self):
+        set_runtime_service('enrollments', MockEnrollmentsService([]))
+
+        response = self.client.get(reverse(
                     'edx_proctoring:user_onboarding.status.course',
                     kwargs={'course_id': self.onboarding_exam.course_id}
                 )

diff --git a/edx_proctoring/views.py b/edx_proctoring/views.py
@@ -32,7 +32,7 @@
     get_all_exams_for_course,
     get_allowances_for_course,
     get_backend_provider,
-    get_enrollments_for_course,
+    get_enrollments_can_take_proctored_exams,
     get_exam_attempt_by_external_id,
     get_exam_attempt_by_id,
     get_exam_by_content_id,
@@ -445,35 +445,25 @@ def get(self, request, course_id):
                 status=404,
                 data={'detail': _('There is no onboarding exam related to this course id.')}
             )
-        serialized_onboarding_exam = ProctoredExamSerializer(onboarding_exam).data
 
         data_page = request.GET.get('page', 1)
         text_search = request.GET.get('text_search')
         statuses_filter = request.GET.get('statuses')
 
-        enrollments = get_enrollments_for_course(course_id)
-        # filter down enrollments for users that can take proctored exams
-        allowed_enrollments_users = [
-            enrollment.user
-            for enrollment
-            in enrollments
-            if enrollment.user.has_perm('edx_proctoring.can_take_proctored_exam', serialized_onboarding_exam)
-        ]
-
-        # filter allowed_enrollments_users by text_search
-        allowed_enrollments_users = self._filter_users_by_username_or_email(allowed_enrollments_users, text_search)
+        enrollments = get_enrollments_can_take_proctored_exams(course_id, text_search)
 
+        users = [enrollment.user for enrollment in enrollments]
         # get onboarding attempts for users for the course
         onboarding_attempts = ProctoredExamStudentAttempt.objects.get_proctored_practice_attempts_by_course_id(
             course_id,
-            allowed_enrollments_users
+            users
         ).values('user_id', 'status', 'modified')
 
         # select a verified, or most recent, exam attempt per user
         onboarding_attempts_per_user = self._get_relevant_attempt_per_user(onboarding_attempts)
 
         onboarding_data = []
-        for user in allowed_enrollments_users:
+        for user in users:
             user_attempt = onboarding_attempts_per_user.get(user.id, {})
 
             data = {}
@@ -538,24 +528,6 @@ def _get_relevant_attempt_per_user(self, attempts):
 
         return onboarding_attempts_per_user
 
-    def _filter_users_by_username_or_email(self, users, text_search):
-        """
-        Given users, return users for whom there is insensitive partial or full match of
-        either their username or email.
-
-        Parameters:
-        * users: users against which to do the search
-        * text_search: the string aginst which to do a match
-        """
-        if text_search:
-            text_search = text_search.lower()
-            return [
-                user
-                for user in users
-                if text_search in user.username.lower() or text_search in user.email.lower()
-            ]
-        return users
-
     def _paginate_data(self, data, page_number, course_id, query_params):
         """
         Given data and a page number, return the page of data requested by the page number,

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": "3.7.2",
+  "version": "3.7.3",
   "main": "edx_proctoring/static/index.js",
   "repository": {
     "type": "git",