From 98de523b57066d2becb77faa040e5738519a6166 Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Mon, 2 Dec 2024 22:46:22 +1300 Subject: [PATCH] fix performance issue --- .../dashboard/details/learners.py | 31 ++++--------------- futurex_openedx_extensions/dashboard/views.py | 2 +- .../helpers/extractors.py | 16 ++++++++-- .../test_details/test_details_learners.py | 4 +-- tests/test_helpers/test_extractors.py | 3 +- tests/test_helpers/test_roles.py | 2 +- 6 files changed, 24 insertions(+), 34 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/details/learners.py b/futurex_openedx_extensions/dashboard/details/learners.py index 940e00d8..6ba9946f 100644 --- a/futurex_openedx_extensions/dashboard/details/learners.py +++ b/futurex_openedx_extensions/dashboard/details/learners.py @@ -1,7 +1,6 @@ """Learners details collectors""" from __future__ import annotations -import re from datetime import timedelta from common.djangoapps.student.models import CourseEnrollment @@ -15,9 +14,8 @@ from lms.djangoapps.grades.models import PersistentCourseGrade from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from futurex_openedx_extensions.helpers import constants as cs from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes -from futurex_openedx_extensions.helpers.extractors import get_partial_access_course_ids +from futurex_openedx_extensions.helpers.extractors import get_partial_access_course_ids, verify_course_ids from futurex_openedx_extensions.helpers.querysets import ( check_staff_exist_queryset, get_base_queryset_courses, @@ -295,37 +293,20 @@ def get_learners_enrollments_queryset( message=f'Invalid user ids: {invalid_user_ids}', ) accessible_users = accessible_users.filter(id__in=user_ids) - invalid_user_ids = set(user_ids) - set(accessible_users.values_list('id', flat=True)) # type: ignore - if invalid_user_ids: - raise FXCodedException( - code=FXExceptionCodes.INVALID_INPUT, - message=f'Inaccessible user ids: {invalid_user_ids}', - ) accessible_courses = CourseOverview.objects.filter( Q(id__in=get_partial_access_course_ids(fx_permission_info)) | Q(org__in=fx_permission_info['view_allowed_full_access_orgs']) ) if course_ids: - invalid_course_ids = [course_id for course_id in course_ids if not re.match(cs.COURSE_ID_REGX, course_id)] - if invalid_course_ids: - raise FXCodedException( - code=FXExceptionCodes.INVALID_INPUT, - message=f'Invalid course ids: {invalid_course_ids}', - ) + verify_course_ids(course_ids) accessible_courses = accessible_courses.filter(id__in=course_ids) - invalid_course_ids = set(course_ids) - {str(course.id) for course in accessible_courses} # type: ignore - if invalid_course_ids: - raise FXCodedException( - code=FXExceptionCodes.INVALID_INPUT, - message=f'Inaccessible course ids: {invalid_course_ids}', - ) queryset = CourseEnrollment.objects.filter( - Q(is_active=True) & Q(course__in=accessible_courses) & Q(user__in=accessible_users) - ) - - queryset = queryset.annotate( + is_active=True, + course__in=Subquery(accessible_courses.values('id')), + user__in=Subquery(accessible_users.values('id')) + ).annotate( certificate_available=Exists( GeneratedCertificate.objects.filter( user_id=OuterRef('user_id'), diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 159d0b30..4c9a6c57 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -540,7 +540,7 @@ def get_queryset(self) -> QuerySet: excluded_role_types=dummy_serializers.query_params['excluded_role_types'], ).values('user_id').distinct().order_by() ).select_related('profile').order_by('id') - except ValueError as exc: + except (ValueError, FXCodedException) as exc: raise ParseError(f'Invalid parameter: {exc}') from exc return q_set diff --git a/futurex_openedx_extensions/helpers/extractors.py b/futurex_openedx_extensions/helpers/extractors.py index 329fe96e..64cabe17 100644 --- a/futurex_openedx_extensions/helpers/extractors.py +++ b/futurex_openedx_extensions/helpers/extractors.py @@ -10,6 +10,7 @@ from xmodule.modulestore.django import modulestore from futurex_openedx_extensions.helpers import constants as cs +from futurex_openedx_extensions.helpers.exceptions import FXCodedException, FXExceptionCodes @dataclass @@ -118,13 +119,22 @@ def verify_course_ids(course_ids: List[str]) -> None: :type course_ids: List[str] """ if course_ids is None: - raise ValueError('course_ids must be a list of course IDs, but got None') + raise FXCodedException( + code=FXExceptionCodes.INVALID_INPUT, + message='course_ids must be a list of course IDs, but got None', + ) for course_id in course_ids: if not isinstance(course_id, str): - raise ValueError(f'course_id must be a string, but got {type(course_id).__name__}') + raise FXCodedException( + code=FXExceptionCodes.INVALID_INPUT, + message=f'course_id must be a string, but got {type(course_id).__name__}', + ) if not re.search(cs.COURSE_ID_REGX_EXACT, course_id) and not re.search(cs.LIBRARY_ID_REGX_EXACT, course_id): - raise ValueError(f'Invalid course ID format: {course_id}') + raise FXCodedException( + code=FXExceptionCodes.INVALID_INPUT, + message=f'Invalid course ID format: {course_id}', + ) def get_orgs_of_courses(course_ids: List[str]) -> Dict[str, Any]: diff --git a/tests/test_dashboard/test_details/test_details_learners.py b/tests/test_dashboard/test_details/test_details_learners.py index 541a9459..d920bdf0 100644 --- a/tests/test_dashboard/test_details/test_details_learners.py +++ b/tests/test_dashboard/test_details/test_details_learners.py @@ -231,10 +231,8 @@ def test_get_learners_enrollments_queryset_annotations( ([21], None, '', 3, 'only user_ids filter'), ([], ['course-v1:ORG1+5+5'], '', 2, 'only course_ids filter'), ([15, 29], ['course-v1:ORG1+5+5'], '', 1, 'both course_ids and user_ids filter'), - ([15, 29, 2], [], 'Inaccessible user ids: {2}', 0, 'inaccessible user ids.'), ([15, 29, 'not-int', 0.2], [], 'Invalid user ids: [\'not-int\', 0.2]', 0, 'invalid user ids.'), - (None, ['course-v1:ORG3+1+1'], 'Inaccessible course ids: {\'course-v1:ORG3+1+1\'}', 0, ''), - ([15, 29], ['course-v1:ORG1+5+5', 'invalid'], 'Invalid course ids: [\'invalid\']', 0, ''), + ([15, 29], ['course-v1:ORG1+5+5', 'invalid'], 'Invalid course ID format: invalid', 0, ''), ] ) @patch('futurex_openedx_extensions.dashboard.details.learners.get_permitted_learners_queryset') diff --git a/tests/test_helpers/test_extractors.py b/tests/test_helpers/test_extractors.py index bd5cb7d0..f4b8f801 100644 --- a/tests/test_helpers/test_extractors.py +++ b/tests/test_helpers/test_extractors.py @@ -5,6 +5,7 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from futurex_openedx_extensions.helpers import constants as cs +from futurex_openedx_extensions.helpers.exceptions import FXCodedException from futurex_openedx_extensions.helpers.extractors import ( DictHashcode, DictHashcodeSet, @@ -68,7 +69,7 @@ def test_verify_course_ids_success(course_ids): ]) def test_verify_course_ids_fail(course_ids, error_message): """Verify that verify_course_ids raises an error for invalid course IDs.""" - with pytest.raises(ValueError) as exc: + with pytest.raises(FXCodedException) as exc: verify_course_ids(course_ids) assert str(exc.value) == error_message diff --git a/tests/test_helpers/test_roles.py b/tests/test_helpers/test_roles.py index c18c8f89..e9518c97 100644 --- a/tests/test_helpers/test_roles.py +++ b/tests/test_helpers/test_roles.py @@ -858,7 +858,7 @@ def test_get_roles_for_users_queryset_bad_exclude(bad_role_type, expected_error_ def test_get_roles_for_users_queryset_bad_course_id(): """Verify that get_roles_for_users_queryset raises an error if the course_id parameter is invalid.""" - with pytest.raises(ValueError) as exc_info: + with pytest.raises(FXCodedException) as exc_info: get_course_access_roles_queryset( orgs_filter=['org1', 'org2'], remove_redundant=False,