MST-675: QuerySet Performance for StudentOnboardingStatusByCourse Endpoint (#26789)

* The original use of user.has_perm('can_take_proctored_exam') in the get_active_enrollments_for_course method had very poor performance when used for multiple learners. The permission is not designed for use in bulk operations. It was being called for each user in a loop by edx-proctoring, resulting in many queries to the database. This lead to timeouts on the client. This change exposes a new service endpoint that performs this permission checking in the database, resulting in one single query to the necessary LMS SQL tables and many fewer queries to the modulestore.

* bump version of edx-proctoring to 3.7.3

openedx/core/djangoapps/enrollments/services.py

 """
 Enrollments Service
 """
+from functools import reduce
+from operator import or_
 
+from django.conf import settings
+from django.db.models import Q
 
+from opaque_keys.edx.keys import CourseKey
+from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.student.models import CourseEnrollment
+from xmodule.modulestore.django import modulestore
 
 
 class EnrollmentsService(object):
@@ -12,9 +19,67 @@ class EnrollmentsService(object):
 
     Provides functions related to course enrollments
     """
-
     def get_active_enrollments_by_course(self, course_id):
         """
         Returns a list of active enrollments for a course
         """
-        return list(CourseEnrollment.objects.filter(course_id=course_id, is_active=True))
+        return CourseEnrollment.objects.filter(course_id=course_id, is_active=True)
+
+    def _get_enrollments_for_course_proctoring_eligible_modes(self, course_id, allow_honor_mode=False):
+        """
+        Return all enrollments for a course that are in a mode that makes the corresponding user
+        eligible to take proctored exams.
+
+        Parameters:
+        * course_id: course ID for the course
+        * allow_honor_mode: represents whether the course allows users with enrollments
+        in the honor mode are eligible to take proctored exams
+        """
+        enrollments = CourseEnrollment.objects.filter(course_id=course_id, is_active=True)
+
+        # We only want to get enrollments in paid modes.
+        appropriate_modes = [
+            CourseMode.VERIFIED,
+            CourseMode.MASTERS,
+            CourseMode.PROFESSIONAL,
+            CourseMode.EXECUTIVE_EDUCATION,
+        ]
+
+        # If the proctoring provider allows learners in honor mode to take exams, include it in the filter.
+        if allow_honor_mode:
+            appropriate_modes.append(CourseMode.HONOR)
+
+        modes_filters = reduce(or_, [Q(mode=mode) for mode in appropriate_modes])
+
+        enrollments = enrollments.filter(modes_filters)
+        return enrollments
+
+    def get_enrollments_can_take_proctored_exams(self, course_id, text_search=None):
+        """
+        Return all enrollments for a course that are in a mode that makes the corresponding user
+        eligible to take proctored exams.
+
+        NOTE: Due to performance concerns, this method returns a QuerySet. Ordinarily, model implementations
+        should not be exposed to clients in this way. However, the clients may need to do additional computation
+        in the database to avoid performance penalties.
+
+        Parameters:
+        * course_id: course ID for the course
+        * text_search: the string against which to do a match on users' username or email; optional
+        """
+        course_id_coursekey = CourseKey.from_string(course_id)
+        course_module = modulestore().get_course(course_id_coursekey)
+        if not course_module or not course_module.enable_proctored_exams:
+            return None
+
+        allow_honor_mode = settings.PROCTORING_BACKENDS.get(
+            course_module.proctoring_provider, {}
+        ).get('allow_honor_mode', False)
+        enrollments = self._get_enrollments_for_course_proctoring_eligible_modes(course_id, allow_honor_mode)
+
+        enrollments = enrollments.select_related('user')
+        if text_search:
+            user_filters = Q(user__username__icontains=text_search) | Q(user__email__icontains=text_search)
+            enrollments = enrollments.filter(user_filters)
+
+        return enrollments

openedx/core/djangoapps/enrollments/tests/test_services.py

 """
 Enrollments Service Tests
 """
-
+import ddt
 
 from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.course_modes.tests.factories import CourseModeFactory
@@ -12,6 +12,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
 
+@ddt.ddt
 class EnrollmentsServiceTests(ModuleStoreTestCase):
     """
     Tests for Enrollments Service
@@ -19,38 +20,189 @@ class EnrollmentsServiceTests(ModuleStoreTestCase):
     def setUp(self):
         super().setUp()
         self.service = EnrollmentsService()
-        self.course = CourseFactory.create()
-        self.course_modes = [CourseMode.HONOR, CourseMode.VERIFIED, CourseMode.AUDIT]
-        for x in range(3):
-            CourseModeFactory.create(mode_slug=self.course_modes[x], course_id=self.course.id)
-            user = UserFactory(username='user{}'.format(x))
-            CourseEnrollment.enroll(user, self.course.id, mode=self.course_modes[x])
-
-    def test_get_active_enrollments_by_course(self):
+        self.course_modes = [
+            CourseMode.AUDIT,
+            CourseMode.EXECUTIVE_EDUCATION,
+            CourseMode.HONOR,
+            CourseMode.MASTERS,
+            CourseMode.PROFESSIONAL,
+            CourseMode.VERIFIED
+        ]
+        self.course = CourseFactory.create(enable_proctored_exams=True)
+
+        for index in range(len(self.course_modes)):
+            course_mode = self.course_modes[index]
+            course_id = self.course.id
+
+            CourseModeFactory.create(mode_slug=course_mode, course_id=course_id)
+            user = UserFactory(
+                username='user{}'.format(index),
+                email='LEARNER{}@example.com'.format(index)
+            )
+            CourseEnrollment.enroll(user, course_id, mode=course_mode)
+
+    def enrollment_to_dict(self, enrollment):
+        return {'username': enrollment.username, 'mode': enrollment.mode}
+
+    def test_get_enrollments_can_take_proctored_exams_by_course(self):
         """
         Test that it returns a list of active enrollments
         """
-        enrollments = self.service.get_active_enrollments_by_course(self.course.id)
-        assert len(enrollments) == 3
-        # At minimum, the function should return the user and mode tied to each enrollment
-        for x in range(3):
-            assert enrollments[x].user.username == 'user{}'.format(x)
-            assert enrollments[x].mode == self.course_modes[x]
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id))
 
-    def test_get_active_enrollments_by_course_ignore_inactive(self):
+        expected_values = [
+            {'username': 'user1', 'mode': 'executive-education'},
+            {'username': 'user3', 'mode': 'masters'},
+            {'username': 'user4', 'mode': 'professional'},
+            {'username': 'user5', 'mode': 'verified'}
+        ]
+        self.assertQuerysetEqual(enrollments, expected_values, self.enrollment_to_dict)
+
+    def test_get_enrollments_can_take_proctored_exams_by_course_ignore_inactive(self):
         """
         Test that inactive enrollments are ignored
         """
-        inactive_enrollment = CourseEnrollment.objects.get(course_id=self.course.id, user__username='user0')
+        inactive_enrollment = CourseEnrollment.objects.get(course_id=self.course.id, user__username='user1')
         inactive_enrollment.is_active = False
         inactive_enrollment.save()
-        enrollments = self.service.get_active_enrollments_by_course(self.course.id)
-        assert len(enrollments) == 2
 
-    def test_get_active_enrollments_no_enrollments(self):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id))
+
+        assert len(enrollments) == 3
+
+    def test_get_enrollments_can_take_proctored_exams_no_enrollments(self):
         """
         Test that an empty list is returned if a course has no enrollments
         """
-        new_course = CourseFactory()
-        enrollments = self.service.get_active_enrollments_by_course(new_course.id)  # pylint: disable=no-member
-        assert len(enrollments) == 0
+        course = CourseFactory.create(enable_proctored_exams=True)
+
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(str(course.id))  # pylint: disable=no-member
+
+        assert not enrollments.exists()
+
+    def test_get_enrollments_can_take_proctored_exams_allow_honor(self):
+        self.course.proctoring_provider = 'test'
+        self.store.update_item(self.course, self.user.id)
+
+        mock_proctoring_backend = {
+            'test': {
+                'allow_honor_mode': True
+            }
+        }
+        with self.settings(PROCTORING_BACKENDS=mock_proctoring_backend):
+            enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id))
+
+        expected_values = [
+            {'username': 'user1', 'mode': 'executive-education'},
+            {'username': 'user2', 'mode': 'honor'},
+            {'username': 'user3', 'mode': 'masters'},
+            {'username': 'user4', 'mode': 'professional'},
+            {'username': 'user5', 'mode': 'verified'}
+
+        ]
+        self.assertQuerysetEqual(enrollments, expected_values, self.enrollment_to_dict)
+
+    def test_get_enrollments_can_take_proctored_exams_not_enable_proctored_exams(self):
+        self.course.enable_proctored_exams = False
+        self.store.update_item(self.course, self.user.id)
+
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id))
+
+        assert enrollments is None
+
+    def test_get_enrollments_can_take_proctored_exams_no_course(self):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams('org.0/course_0/Run_100')
+
+        assert enrollments is None
+
+    @ddt.data('ser', 'uSeR', 'leaRNer', 'LEARNER', '@example.com')
+    def test_text_search_partial_return_all(self, text_search):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(
+            str(self.course.id),
+            text_search=text_search
+        )
+
+        expected_values = [
+            {'username': 'user1', 'mode': 'executive-education'},
+            {'username': 'user3', 'mode': 'masters'},
+            {'username': 'user4', 'mode': 'professional'},
+            {'username': 'user5', 'mode': 'verified'}
+        ]
+        self.assertQuerysetEqual(enrollments, expected_values, self.enrollment_to_dict)
+
+    def test_text_search_partial_return_some(self):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(
+            str(self.course.id),
+            text_search='3'
+        )
+
+        expected_values = [
+            {'username': 'user3', 'mode': 'masters'}
+        ]
+        self.assertQuerysetEqual(enrollments, expected_values, self.enrollment_to_dict)
+
+    @ddt.data('user1', 'USER1', 'LEARNER1@example.com', 'lEarNer1@eXAMPLE.com')
+    def test_text_search_exact_return_one(self, text_search):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(
+            str(self.course.id),
+            text_search=text_search
+        )
+
+        expected_values = [
+            {'username': 'user1', 'mode': 'executive-education'}
+        ]
+        self.assertQuerysetEqual(enrollments, expected_values, self.enrollment_to_dict)
+
+    def test_text_search_return_none(self):
+        enrollments = self.service.get_enrollments_can_take_proctored_exams(
+            str(self.course.id),
+            text_search='abc'
+        )
+
+        assert not enrollments.exists()
+
+
+@ddt.ddt
+class EnrollmentsServicePerformanceTests(ModuleStoreTestCase):
+    """
+    Tests for Enrollments Service performance
+    """
+    def setUp(self):
+        super().setUp()
+        self.service = EnrollmentsService()
+        self.course = CourseFactory.create(enable_proctored_exams=True)
+        self.course_modes = [
+            CourseMode.AUDIT,
+            CourseMode.EXECUTIVE_EDUCATION,
+            CourseMode.HONOR,
+            CourseMode.MASTERS,
+            CourseMode.PROFESSIONAL,
+            CourseMode.VERIFIED,
+        ]
+
+        for index in range(len(self.course_modes)):
+            CourseModeFactory.create(mode_slug=self.course_modes[index], course_id=self.course.id)
+
+    def create_and_enroll_users(self, num_users):
+        num_course_modes = len(self.course_modes)
+        for index in range(num_users):
+            user = UserFactory(username='user{}'.format(index))
+            CourseEnrollment.enroll(user, self.course.id, mode=self.course_modes[index % num_course_modes])
+
+    @ddt.data(10, 25, 50)
+    def test_get_enrollments_can_take_proctored_exams_num_queries(self, num_users):
+        self.create_and_enroll_users(num_users)
+
+        with self.assertNumQueries(1):
+            enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id))
+            # force execution of the QuerySet so that queries are exectued
+            repr(enrollments)
+
+    @ddt.data(10, 25, 50)
+    def test_get_enrollments_can_take_proctored_exams_num_queries_text_search(self, num_users):
+        self.create_and_enroll_users(num_users)
+
+        with self.assertNumQueries(1):
+            enrollments = self.service.get_enrollments_can_take_proctored_exams(str(self.course.id), text_search='edX')
+            # force execution of the QuerySet so that queries are exectued
+            repr(enrollments)

requirements/edx/base.txt

@@ -106,7 +106,7 @@ edx-milestones==0.3.0     # via -r requirements/edx/base.in
 edx-opaque-keys[django]==2.2.0  # via -r requirements/edx/paver.txt, edx-bulk-grades, edx-ccx-keys, edx-completion, edx-drf-extensions, edx-enterprise, edx-milestones, edx-organizations, edx-proctoring, edx-user-state-client, edx-when, lti-consumer-xblock, xmodule
 edx-organizations==6.9.0  # via -r requirements/edx/base.in
 edx-proctoring-proctortrack==1.0.5  # via -r requirements/edx/base.in
-edx-proctoring==3.7.1     # via -r requirements/edx/base.in, edx-proctoring-proctortrack
+edx-proctoring==3.7.3     # via -r requirements/edx/base.in, edx-proctoring-proctortrack
 edx-rbac==1.4.1           # via edx-enterprise
 edx-rest-api-client==5.3.0  # via -r requirements/edx/base.in, edx-enterprise, edx-proctoring
 edx-search==3.0.0         # via -r requirements/edx/base.in

requirements/edx/development.txt

@@ -118,7 +118,7 @@ edx-milestones==0.3.0     # via -r requirements/edx/testing.txt
 edx-opaque-keys[django]==2.2.0  # via -r requirements/edx/testing.txt, edx-bulk-grades, edx-ccx-keys, edx-completion, edx-drf-extensions, edx-enterprise, edx-milestones, edx-organizations, edx-proctoring, edx-user-state-client, edx-when, lti-consumer-xblock, xmodule
 edx-organizations==6.9.0  # via -r requirements/edx/testing.txt
 edx-proctoring-proctortrack==1.0.5  # via -r requirements/edx/testing.txt
-edx-proctoring==3.7.1     # via -r requirements/edx/testing.txt, edx-proctoring-proctortrack
+edx-proctoring==3.7.3     # via -r requirements/edx/testing.txt, edx-proctoring-proctortrack
 edx-rbac==1.4.1           # via -r requirements/edx/testing.txt, edx-enterprise
 edx-rest-api-client==5.3.0  # via -r requirements/edx/testing.txt, edx-enterprise, edx-proctoring
 edx-search==3.0.0         # via -r requirements/edx/testing.txt

requirements/edx/testing.txt

@@ -115,7 +115,7 @@ edx-milestones==0.3.0     # via -r requirements/edx/base.txt
 edx-opaque-keys[django]==2.2.0  # via -r requirements/edx/base.txt, edx-bulk-grades, edx-ccx-keys, edx-completion, edx-drf-extensions, edx-enterprise, edx-milestones, edx-organizations, edx-proctoring, edx-user-state-client, edx-when, lti-consumer-xblock, xmodule
 edx-organizations==6.9.0  # via -r requirements/edx/base.txt
 edx-proctoring-proctortrack==1.0.5  # via -r requirements/edx/base.txt
-edx-proctoring==3.7.1     # via -r requirements/edx/base.txt, edx-proctoring-proctortrack
+edx-proctoring==3.7.3     # via -r requirements/edx/base.txt, edx-proctoring-proctortrack
 edx-rbac==1.4.1           # via -r requirements/edx/base.txt, edx-enterprise
 edx-rest-api-client==5.3.0  # via -r requirements/edx/base.txt, edx-enterprise, edx-proctoring
 edx-search==3.0.0         # via -r requirements/edx/base.txt