Skip to content

Commit

Permalink
Merge pull request #801 from edx/mroytman/MST-675-queryset-performanc…
Browse files Browse the repository at this point in the history 
…e-StudentOnboardingStatusByCourse

MST-675: QuerySet Performance for  StudentOnboardingStatusByCourse Endpoint
  • Loading branch information
@MichaelRoytman
MichaelRoytman authored Mar 2, 2021
2 parents 34f03ad + 8d8f920 commit 3fe9eba
Show file tree
Hide file tree
Showing 8 changed files with 31 additions and 221 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion edx_proctoring/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
7 changes: 4 additions & 3 deletions edx_proctoring/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
8 changes: 4 additions & 4 deletions edx_proctoring/tests/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand All @@ -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),
Expand Down
4 changes: 4 additions & 0 deletions edx_proctoring/tests/test_services.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
183 changes: 4 additions & 179 deletions edx_proctoring/tests/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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}
)
Expand Down
38 changes: 5 additions & 33 deletions edx_proctoring/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 3fe9eba

Please sign in to comment.