From a07ef124fc7d153138a2508c7f25a736079a3f33 Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Tue, 24 Dec 2024 01:20:07 +1300 Subject: [PATCH 1/2] fix: enrollment api to remove exam scores for multiple courses --- .../dashboard/serializers.py | 58 ++++++++--------- futurex_openedx_extensions/dashboard/views.py | 9 ++- tests/test_dashboard/test_serializers.py | 64 ++++++++++++++++++- 3 files changed, 96 insertions(+), 35 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/serializers.py b/futurex_openedx_extensions/dashboard/serializers.py index ff9bd80b..a39a9c95 100644 --- a/futurex_openedx_extensions/dashboard/serializers.py +++ b/futurex_openedx_extensions/dashboard/serializers.py @@ -241,6 +241,9 @@ def get_year_of_birth(self, obj: get_user_model) -> Any: class CourseScoreAndCertificateSerializer(ModelSerializerOptionalFields): """ Course Score and Certificate Details Serializer + + Handles data for multiple courses, but exam_scores is included only for a single course when course_id is + provided in the context. Otherwise, exam_scores is excluded, even if requested in requested_optional_field_tags. """ exam_scores = SerializerOptionalMethodField(field_tags=['exam_scores', 'csv_export']) certificate_available = serializers.BooleanField() @@ -266,27 +269,34 @@ def __init__(self, *args: Any, **kwargs: Any): self._grading_info: Dict[str, Any] = {} self._subsection_locations: Dict[str, Any] = {} - def collect_grading_info(self, course_ids: list) -> None: - """Collect the grading info.""" + if self.context.get('course_id'): + self.collect_grading_info() + + def collect_grading_info(self) -> None: + """ + Collect the grading info. + """ + course_id = CourseLocator.from_string(self.context.get('course_id')) self._grading_info = {} self._subsection_locations = {} - index = 0 if not self.is_optional_field_requested('exam_scores'): return - for course_id in course_ids: - grading_context = grading_context_for_course(get_course_by_id(course_id)) - for assignment_type_name, subsection_infos in grading_context['all_graded_subsections_by_type'].items(): - for subsection_index, subsection_info in enumerate(subsection_infos, start=1): - header_enum = f' {subsection_index}' if len(subsection_infos) > 1 else '' - header_name = f'{assignment_type_name}{header_enum}' - if self.is_exam_name_in_header: - header_name += f': {subsection_info["subsection_block"].display_name}' - self._grading_info[str(index)] = { - 'header_name': header_name, - 'location': str(subsection_info['subsection_block'].location), - } - self._subsection_locations[str(subsection_info['subsection_block'].location)] = str(index) - index += 1 + + grading_context = grading_context_for_course(get_course_by_id(course_id)) + index = 0 + for assignment_type_name, subsection_infos in grading_context['all_graded_subsections_by_type'].items(): + for subsection_index, subsection_info in enumerate(subsection_infos, start=1): + header_enum = f' {subsection_index}' if len(subsection_infos) > 1 else '' + header_name = f'{assignment_type_name}{header_enum}' + if self.is_exam_name_in_header: + header_name += f': {subsection_info["subsection_block"].display_name}' + + self._grading_info[str(index)] = { + 'header_name': header_name, + 'location': str(subsection_info['subsection_block'].location), + } + self._subsection_locations[str(subsection_info['subsection_block'].location)] = str(index) + index += 1 @property def is_exam_name_in_header(self) -> bool: @@ -387,15 +397,9 @@ class Meta: model = get_user_model() fields = LearnerBasicDetailsSerializer.Meta.fields + CourseScoreAndCertificateSerializer.Meta.fields - def __init__(self, *args: Any, **kwargs: Any): - """Initialize the serializer.""" - super().__init__(*args, **kwargs) - self._course_id = CourseLocator.from_string(self.context.get('course_id')) - self.collect_grading_info([self._course_id]) - def _get_course_id(self, obj: Any = None) -> CourseLocator: """Get the course ID. Its helper method required for CourseScoreAndCertificateSerializer""" - return self._course_id + return CourseLocator.from_string(self.context.get('course_id')) def _get_user(self, obj: Any = None) -> get_user_model: """Get the User. Its helper method required for CourseScoreAndCertificateSerializer""" @@ -416,12 +420,6 @@ class Meta: ['course_id'] ) - def __init__(self, *args: Any, **kwargs: Any): - """Initialize the serializer.""" - super().__init__(*args, **kwargs) - course_ids = self.context.get('course_ids') - self.collect_grading_info(course_ids) - def _get_course_id(self, obj: Any = None) -> CourseLocator | None: """Get the course ID. Its helper method required for CourseScoreAndCertificateSerializer""" return obj.course_id if obj else None diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index bef20d55..760d10bd 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -490,6 +490,7 @@ class LearnersEnrollmentView(ExportCSVMixin, FXViewRoleInfoMixin, ListAPIView): fx_view_name = 'learners_enrollment_details' fx_default_read_only_roles = ['staff', 'instructor', 'data_researcher', 'org_course_creator_group'] fx_view_description = 'api/fx/learners/v1/enrollments: Get the list of enrollments' + is_single_course_requested = False def get_queryset(self, *args: Any, **kwargs: Any) -> QuerySet: """Get the list of learners for a course""" @@ -506,6 +507,9 @@ def get_queryset(self, *args: Any, **kwargs: Any) -> QuerySet: username.strip() for username in usernames.split(',') ] if usernames else None + if course_ids_list and len(course_ids_list) == 1: + self.is_single_course_requested = True + return get_learners_enrollments_queryset( fx_permission_info=self.request.fx_permission_info, user_ids=user_ids_list, @@ -518,8 +522,9 @@ def get_queryset(self, *args: Any, **kwargs: Any) -> QuerySet: def get_serializer_context(self) -> Dict[str, Any]: """Get the serializer context""" context = super().get_serializer_context() - context['course_ids'] = [course_enrollment.course_id for course_enrollment in self.get_queryset()] - context['omit_subsection_name'] = self.request.query_params.get('omit_subsection_name', '0') + if self.is_single_course_requested and self.get_queryset().exists(): + context['course_id'] = str(self.get_queryset().first().course_id) + context['omit_subsection_name'] = self.request.query_params.get('omit_subsection_name', '0') return context diff --git a/tests/test_dashboard/test_serializers.py b/tests/test_dashboard/test_serializers.py index 1301d27a..d616513c 100644 --- a/tests/test_dashboard/test_serializers.py +++ b/tests/test_dashboard/test_serializers.py @@ -284,14 +284,14 @@ class Meta: @pytest.mark.django_db @patch('futurex_openedx_extensions.dashboard.serializers.CourseScoreAndCertificateSerializer.collect_grading_info') def test_learner_enrollments_serializer(mock_collect, base_data,): # pylint: disable=unused-argument - """Verify that the LearnerDetailsForCourseSerializer returns the needed fields.""" + """Verify that the LearnerEnrollmentSerializer returns the needed fields.""" queryset = CourseEnrollment.objects.filter(user_id=10, course_id='course-v1:ORG3+1+1').annotate( certificate_available=Value(True), course_score=Value(0.67), active_in_course=Value(True), ) serializer = LearnerEnrollmentSerializer(queryset, context={ - 'course_ids': ['course-v1:ORG3+1+1'] + 'course_id': 'course-v1:ORG3+1+1' }, many=True) mock_collect.assert_called_once() data = serializer.data @@ -301,6 +301,64 @@ def test_learner_enrollments_serializer(mock_collect, base_data,): # pylint: di assert data[0]['active_in_course'] is True +@pytest.mark.django_db +@patch('futurex_openedx_extensions.dashboard.serializers.get_course_blocks_completion_summary') +@pytest.mark.parametrize('is_course_id_in_context, optional_field_tags', [ + (True, ['exam_scores']), + (True, ['__all__']), + (True, ['csv_export']), + (False, ['exam_scores']), + (False, ['__all__']), + (False, ['csv_export']), +]) +def test_learner_enrollment_serializer_exam_scores( + mock_get_completion, is_course_id_in_context, optional_field_tags, grading_context, base_data, +): # pylint: disable=unused-argument, redefined-outer-name + """ + Verify that the LearnerEnrollmentSerializer returns exam_scores + only when course_id is set in serializer_context + """ + queryset = CourseEnrollment.objects.filter(course_id='course-v1:ORG2+1+1').annotate( + certificate_available=Value(True), + course_score=Value(0.67), + active_in_course=Value(True), + ) + PersistentSubsectionGrade.objects.create( + user_id=queryset[0].user.id, + course_id='course-v1:ORG2+1+1', + usage_key='block-v1:ORG2+1+1+type@homework+block@1', + earned_graded=0, + possible_graded=0, + earned_all=77.0, + possible_all=88.0, + ) + PersistentSubsectionGrade.objects.create( + user_id=queryset[0].user.id, + course_id='course-v1:ORG2+1+1', + usage_key='block-v1:ORG2+1+1+type@homework+block@2', + earned_graded=0, + possible_graded=0, + earned_all=9.0, + possible_all=10.0, + first_attempted=now() - timedelta(days=1), + ) + mock_get_completion.return_value = {'complete_count': 2, 'incomplete_count': 1, 'locked_count': 1} + context = {'requested_optional_field_tags': optional_field_tags} + + if is_course_id_in_context: + context.update({'course_id': 'course-v1:ORG2+1+1'}) + + serializer = LearnerEnrollmentSerializer(queryset[0], context=context) + data = serializer.data + + if is_course_id_in_context: + assert 'earned - Homework 1: First Homework' in data.keys() + assert 'earned - Homework 2: Second Homework' in data.keys() + else: + assert 'earned - Homework 1: First Homework' not in data.keys() + assert 'earned - Homework 2: Second Homework' not in data.keys() + + def test_learner_details_for_course_serializer_optional_fields(): """Verify that the LearnerDetailsForCourseSerializer returns the correct optional fields.""" serializer = LearnerDetailsForCourseSerializer(context={'course_id': 'course-v1:ORG2+1+1'}) @@ -367,7 +425,7 @@ def test_learner_details_for_course_serializer_collect_grading_info_not_used( assert not serializer.grading_info assert not serializer.subsection_locations - serializer.collect_grading_info(['course-v1:ORG2+1+1']) + serializer.collect_grading_info() assert not serializer.grading_info assert not serializer.subsection_locations From e651b2bed9fe23a95d943d67edfd567e56805740 Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Wed, 25 Dec 2024 01:43:12 +1300 Subject: [PATCH 2/2] add swagger docs and course_search --- .../dashboard/details/learners.py | 15 ++++- .../dashboard/docs_src.py | 55 +++++++++++++++++++ futurex_openedx_extensions/dashboard/views.py | 5 +- .../test_details/test_details_learners.py | 23 ++++++++ 4 files changed, 93 insertions(+), 5 deletions(-) diff --git a/futurex_openedx_extensions/dashboard/details/learners.py b/futurex_openedx_extensions/dashboard/details/learners.py index f3db3492..e7ff8e19 100644 --- a/futurex_openedx_extensions/dashboard/details/learners.py +++ b/futurex_openedx_extensions/dashboard/details/learners.py @@ -270,7 +270,8 @@ def get_learners_enrollments_queryset( # pylint: disable=too-many-arguments user_ids: list = None, course_ids: list = None, usernames: list = None, - search_text: str | None = None, + learner_search: str | None = None, + course_search: str | None = None, include_staff: bool = False, ) -> QuerySet: """ @@ -279,12 +280,13 @@ def get_learners_enrollments_queryset( # pylint: disable=too-many-arguments :param course_ids: List of course IDs to filter by (optional). :param user_ids: List of user IDs to filter by (optional). - :param search_text: Text to filter users by (optional). + :param learner_search: Text to search enrollments by user (username, email or national_id) (optional). + :param course_search: Text to search enrollments by course (dispaly name, id) (optional). :param include_staff: Flag to include staff users (default: False). :return: List of dictionaries containing user and course details. """ accessible_users = get_permitted_learners_queryset( - queryset=get_learners_search_queryset(search_text), + queryset=get_learners_search_queryset(search_text=learner_search), fx_permission_info=fx_permission_info, include_staff=include_staff, ) @@ -315,6 +317,13 @@ def get_learners_enrollments_queryset( # pylint: disable=too-many-arguments verify_course_ids(course_ids) accessible_courses = accessible_courses.filter(id__in=course_ids) + course_search = (course_search or '').strip() + if course_search: + accessible_courses = accessible_courses.filter( + Q(display_name__icontains=course_search) | + Q(id__icontains=course_search), + ) + queryset = CourseEnrollment.objects.filter( is_active=True, course__in=Subquery(accessible_courses.values('id')), diff --git a/futurex_openedx_extensions/dashboard/docs_src.py b/futurex_openedx_extensions/dashboard/docs_src.py index e9a382b0..330f13cb 100644 --- a/futurex_openedx_extensions/dashboard/docs_src.py +++ b/futurex_openedx_extensions/dashboard/docs_src.py @@ -390,6 +390,61 @@ def responses( 'responses': responses(), }, + 'LearnersEnrollmentView.get': { + 'summary': 'Get the list of enrollments', + 'description': 'Get the list of enrollments in the tenants, which is the list of all learners having at ' + ' least one enrollment in any course.', + 'parameters': [ + common_parameters['tenant_ids'], + query_parameter( + 'course_ids', + str, + 'a comma separated list of course ids to filter the results by. If not provided, the system will ' + 'assume all courses that are accessible to the caller.', + ), + query_parameter( + 'user_ids', + str, + 'a comma separated list of learner user ids to filter the results by. If not provided, the system ' + 'will assume all users that are accessible to the caller.', + ), + query_parameter( + 'usernames', + str, + 'a comma separated list of learner usernames to filter the results by. If not provided, the system ' + ' will assume all users that are accessible to the caller.', + ), + query_parameter( + 'learner_search', + str, + 'A search text to filter results, matched against the user\'s full name, username, national ID, and ' + ' email address.', + ), + query_parameter( + 'course_search', + str, + 'A search text to filter results, matched against the course\'s ID and display name.', + ), + query_parameter( + 'optional_field_tags', + str, + 'a comma seperated list of optional_fields. The data `exam_scores`, `certificate_url` and `progress` ' + 'in result are optional and can only be added if requested exclusively. Caller can also use `__all__` ' + ' to include all optional fields in result.', + ), + common_parameters['include_staff'], + common_parameters['download'], + query_parameter( + 'omit_subsection_name', + int, + 'Omit the subsection name from the response. Can be `0` or `1`. This is useful when `exam_scores`' + ' optional fields are requested; it\'ll omit the subsection names for cleaner representation of the' + ' data. Default is `0`. Any value other than `1` is considered as `0`.', + ), + ], + 'responses': responses(), + }, + 'LearnerInfoView.get': { 'summary': 'Get learner\'s information', 'description': 'Get full information for a specific learner using the `username`. The caller must have access' diff --git a/futurex_openedx_extensions/dashboard/views.py b/futurex_openedx_extensions/dashboard/views.py index 760d10bd..1a178338 100644 --- a/futurex_openedx_extensions/dashboard/views.py +++ b/futurex_openedx_extensions/dashboard/views.py @@ -481,7 +481,7 @@ def get_serializer_context(self) -> Dict[str, Any]: return context -@exclude_schema_for('get') +@docs('LearnersEnrollmentView.get') class LearnersEnrollmentView(ExportCSVMixin, FXViewRoleInfoMixin, ListAPIView): """View to get the list of learners for a course""" serializer_class = serializers.LearnerEnrollmentSerializer @@ -515,7 +515,8 @@ def get_queryset(self, *args: Any, **kwargs: Any) -> QuerySet: user_ids=user_ids_list, course_ids=course_ids_list, usernames=usernames_list, - search_text=self.request.query_params.get('search_text'), + learner_search=self.request.query_params.get('learner_search'), + course_search=self.request.query_params.get('course_search'), include_staff=self.request.query_params.get('include_staff', '0') == '1', ) diff --git a/tests/test_dashboard/test_details/test_details_learners.py b/tests/test_dashboard/test_details/test_details_learners.py index c11eff20..ef65e258 100644 --- a/tests/test_dashboard/test_details/test_details_learners.py +++ b/tests/test_dashboard/test_details/test_details_learners.py @@ -334,3 +334,26 @@ def test_get_learners_enrollments_queryset_for_accessible_courses_and_users( assert not ( {str(enrollment.course.id) for enrollment in queryset} - set(expected_course_ids) ), f'unexpected enrollment course ids for case: {usecase}' + + +@pytest.mark.django_db +@pytest.mark.parametrize( + 'course_search, learner_search, expected_count, usecase', + [ + ('', '', 25, 'no search'), + ('Course 5', '', 8, 'only course name search'), + ('', 'user15', 2, 'only user search'), + ('Course 5', 'user15', 1, 'only user search'), + ], +) +def test_get_learners_enrollments_queryset_for_course_and_learner_search( + course_search, learner_search, expected_count, usecase, fx_permission_info, +): + """Test get_learners_by_course_queryset result for accessible users and courses.""" + fx_permission_info['view_allowed_full_access_orgs'] = ['org1', 'org2'] + queryset = get_learners_enrollments_queryset( + fx_permission_info=fx_permission_info, + course_search=course_search, + learner_search=learner_search + ) + assert queryset.count() == expected_count, f'unexpected enrollment queryset count for case: {usecase}'