From 4ad8ba1b41d22a8709dc8990f0e4bece64b7ac98 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Jun 2023 13:22:36 +0300 Subject: [PATCH] feat: list courses details by keys This adds the ability to get a list of detailed courses based on their keys provided in the newly added `keys` query param in the `GET /courses/v1/courses/` endpoint. --- docs/lms-openapi.yaml | 4 ++ lms/djangoapps/branding/__init__.py | 14 +++++-- lms/djangoapps/course_api/api.py | 10 ++++- lms/djangoapps/course_api/forms.py | 15 ++++++++ lms/djangoapps/course_api/tests/test_api.py | 37 ++++++++++++++++++- lms/djangoapps/course_api/tests/test_forms.py | 9 +++++ lms/djangoapps/course_api/views.py | 7 +++- lms/djangoapps/courseware/courses.py | 5 ++- .../content/course_overviews/models.py | 7 +++- 9 files changed, 97 insertions(+), 11 deletions(-) diff --git a/docs/lms-openapi.yaml b/docs/lms-openapi.yaml index 98a9e49df70..202b983212b 100644 --- a/docs/lms-openapi.yaml +++ b/docs/lms-openapi.yaml @@ -2633,6 +2633,10 @@ paths: date are returned. This is different from search_term because this filtering is done on CourseOverview and not ElasticSearch. + course_keys (optional): + If specified, it filters visible `CourseOverview` objects by + the course keys (ids) provided + **Returns** * 200 on success, with a list of course discovery objects as returned diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index 0847f631cb6..954e5da9b1b 100644 --- a/lms/djangoapps/branding/__init__.py +++ b/lms/djangoapps/branding/__init__.py @@ -14,7 +14,7 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -def get_visible_courses(org=None, filter_=None, active_only=False): +def get_visible_courses(org=None, filter_=None, active_only=False, course_keys=None): """ Yield the CourseOverviews that should be visible in this branded instance. @@ -25,6 +25,8 @@ def get_visible_courses(org=None, filter_=None, active_only=False): filter_ (dict): Optional parameter that allows custom filtering by fields on the course. active_only (bool): Optional parameter that enables fetching active courses only. + course_keys (list[str]): Optional parameter that allows for selecting which + courses to fetch the `CourseOverviews` for """ # Import is placed here to avoid model import at project startup. from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -36,12 +38,16 @@ def get_visible_courses(org=None, filter_=None, active_only=False): if org: # Check the current site's orgs to make sure the org's courses should be displayed if not current_site_orgs or org in current_site_orgs: - courses = CourseOverview.get_all_courses(orgs=[org], filter_=filter_, active_only=active_only) + courses = CourseOverview.get_all_courses( + orgs=[org], filter_=filter_, active_only=active_only, course_keys=course_keys + ) elif current_site_orgs: # Only display courses that should be displayed on this site - courses = CourseOverview.get_all_courses(orgs=current_site_orgs, filter_=filter_, active_only=active_only) + courses = CourseOverview.get_all_courses( + orgs=current_site_orgs, filter_=filter_, active_only=active_only, course_keys=course_keys + ) else: - courses = CourseOverview.get_all_courses(filter_=filter_, active_only=active_only) + courses = CourseOverview.get_all_courses(filter_=filter_, active_only=active_only, course_keys=course_keys) courses = courses.order_by('id') diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 721c213992a..01b5e5bfee0 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -116,7 +116,8 @@ def list_courses(request, filter_=None, search_term=None, permissions=None, - active_only=False): + active_only=False, + course_keys=None): """ Yield all available courses. @@ -146,12 +147,17 @@ def list_courses(request, If specified, it filters visible `CourseOverview` objects by checking if each permission specified is granted for the username. active_only (bool): Optional parameter that enables fetching active courses only. + course_keys (list[str]): + If specified, it filters visible `CourseOverview` objects by + the course keys (ids) provided Return value: Yield `CourseOverview` objects representing the collection of courses. """ user = get_effective_user(request.user, username) - course_qs = get_courses(user, org=org, filter_=filter_, permissions=permissions, active_only=active_only) + course_qs = get_courses( + user, org=org, filter_=filter_, permissions=permissions, active_only=active_only, course_keys=course_keys + ) course_qs = _filter_by_search(course_qs, search_term) return course_qs diff --git a/lms/djangoapps/course_api/forms.py b/lms/djangoapps/course_api/forms.py index 055692da712..ff6cee84aea 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -64,6 +64,7 @@ class CourseListGetForm(UsernameValidatorMixin, Form): mobile = ExtendedNullBooleanField(required=False) active_only = ExtendedNullBooleanField(required=False) permissions = MultiValueField(required=False) + course_keys = MultiValueField(required=False) def clean(self): """ @@ -80,6 +81,20 @@ def clean(self): return cleaned_data + def clean_course_keys(self): + """ + Ensure valid course_keys were provided. + """ + course_keys = self.cleaned_data['course_keys'] + if course_keys: + for course_key in course_keys: + try: + CourseKey.from_string(course_key) + except InvalidKeyError: + raise ValidationError(f"'{str(course_key)}' is not a valid course key.") # lint-amnesty, pylint: disable=raise-missing-from + + return course_keys + class CourseIdListGetForm(UsernameValidatorMixin, Form): """ diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index 8d91b1ee97e..8e4d1253f8f 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -107,7 +107,8 @@ def _make_api_call(self, specified_user, org=None, filter_=None, - permissions=None): + permissions=None, + course_keys=None): """ Call the list_courses api endpoint to get information about `specified_user` on behalf of `requesting_user`. @@ -121,6 +122,7 @@ def _make_api_call(self, org=org, filter_=filter_, permissions=permissions, + course_keys=course_keys, ) def verify_courses(self, courses): @@ -244,6 +246,39 @@ def test_permissions(self): self.assertEqual({c.id for c in filtered_courses}, {self.course.id}) + def test_filter_by_keys(self): + """ + Verify that courses are filtered by the provided course keys. + """ + + # Create alternative courses to be included in the `course_keys` filter. + alternative_course_1 = self.create_course(course='alternative-course-1') + alternative_course_2 = self.create_course(course='alternative-course-2') + + # No filtering. + unfiltered_expected_courses = [ + self.course, + alternative_course_1, + alternative_course_2, + ] + unfiltered_courses = self._make_api_call(self.honor_user, self.honor_user) + assert {course.id for course in unfiltered_courses} == {course.id for course in unfiltered_expected_courses} + + # With filtering. + filtered_expected_courses = [ + alternative_course_1, + alternative_course_2, + ] + filtered_courses = self._make_api_call( + self.honor_user, + self.honor_user, + course_keys={ + alternative_course_1.id, + alternative_course_2.id + } + ) + assert {course.id for course in filtered_courses} == {course.id for course in filtered_expected_courses} + class TestGetCourseListExtras(CourseListTestMixin, ModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index 9d14e5a9989..3b5744402e7 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -71,6 +71,7 @@ def set_up_data(self, user): 'filter_': None, 'permissions': set(), 'active_only': None, + 'course_keys': set(), } def test_basic(self): @@ -100,6 +101,14 @@ def test_filter(self, param_field_name, param_field_value): self.assert_valid(self.cleaned_data) + def test_invalid_course_keys(self): + """ + Verify form checks for validity of course keys provided + """ + + self.form_data['course_keys'] = 'course-v1:edX+DemoX+Demo_Course,invalid_course_key' + self.assert_error('course_keys', "'invalid_course_key' is not a valid course key.") + class TestCourseIdListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreTestCase): # lint-amnesty, pylint: disable=missing-class-docstring FORM_CLASS = CourseIdListGetForm diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index fbb8517cf00..98b01232a1b 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -286,6 +286,10 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): date are returned. This is different from search_term because this filtering is done on CourseOverview and not ElasticSearch. + course_keys (optional): + If specified, it fetches the `CourseOverview` objects for the + the specified course keys + **Returns** * 200 on success, with a list of course discovery objects as returned @@ -343,7 +347,8 @@ def get_queryset(self): filter_=form.cleaned_data['filter_'], search_term=form.cleaned_data['search_term'], permissions=form.cleaned_data['permissions'], - active_only=form.cleaned_data.get('active_only', False) + active_only=form.cleaned_data.get('active_only', False), + course_keys=form.cleaned_data['course_keys'], ) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 009ebf6fdb5..09e9aa7cecf 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -753,7 +753,7 @@ def get_course_syllabus_section(course, section_key): @function_trace('get_courses') -def get_courses(user, org=None, filter_=None, permissions=None, active_only=False): +def get_courses(user, org=None, filter_=None, permissions=None, active_only=False, course_keys=None): """ Return a LazySequence of courses available, optionally filtered by org code (case-insensitive) or a set of permissions to be satisfied for the specified @@ -763,7 +763,8 @@ def get_courses(user, org=None, filter_=None, permissions=None, active_only=Fals courses = branding.get_visible_courses( org=org, filter_=filter_, - active_only=active_only + active_only=active_only, + course_keys=course_keys ).prefetch_related( 'modes', ).select_related( diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index d3101b60546..9f2a0ef43da 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -662,7 +662,7 @@ def update_select_courses(cls, course_keys, force_update=False): log.info('Finished generating course overviews.') @classmethod - def get_all_courses(cls, orgs=None, filter_=None, active_only=False): + def get_all_courses(cls, orgs=None, filter_=None, active_only=False, course_keys=None): """ Return a queryset containing all CourseOverview objects in the database. @@ -671,12 +671,17 @@ def get_all_courses(cls, orgs=None, filter_=None, active_only=False): filtering by organization. filter_ (dict): Optional parameter that allows custom filtering. active_only (bool): If provided, only the courses that have not ended will be returned. + course_keys (list[string]): Optional parameter that allows case-insensitive + filter by course ids """ # Note: If a newly created course is not returned in this QueryList, # make sure the "publish" signal was emitted when the course was # created. For tests using CourseFactory, use emit_signals=True. course_overviews = CourseOverview.objects.all() + if course_keys: + course_overviews = course_overviews.filter(id__in=course_keys) + if orgs: # In rare cases, courses belonging to the same org may be accidentally assigned # an org code with a different casing (e.g., Harvardx as opposed to HarvardX).