From 35d090564064266ccd26b7cdc2ed21e88cf8db64 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. (cherry picked from commit b14cf0cc76928d3903eafcfda47dbefb44044a7a) --- docs/swagger.yaml | 10 +++++ 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 | 3 +- .../content/course_overviews/models.py | 7 +++- 9 files changed, 102 insertions(+), 10 deletions(-) diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 48ca6e9ac63b..010cdc5045c1 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -2039,6 +2039,16 @@ paths: provided org code (e.g., "HarvardX") are returned. Case-insensitive. + permissions (optional): + If specified, it filters visible `CourseOverview` objects by + checking if each permission specified is granted for the username. + Notice that Staff users are always granted permission to list any + course. + + 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 b7ce3e1df522..62d2fac1ddf0 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): +def get_visible_courses(org=None, filter_=None, course_keys=None): """ Yield the CourseOverviews that should be visible in this branded instance. @@ -24,6 +24,8 @@ def get_visible_courses(org=None, filter_=None): filtering by organization. filter_ (dict): Optional parameter that allows custom filtering by fields on the course. + 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 @@ -35,12 +37,16 @@ def get_visible_courses(org=None, filter_=None): 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_) + courses = CourseOverview.get_all_courses( + orgs=[org], filter_=filter_, 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_) + courses = CourseOverview.get_all_courses( + orgs=current_site_orgs, filter_=filter_, course_keys=course_keys + ) else: - courses = CourseOverview.get_all_courses(filter_=filter_) + courses = CourseOverview.get_all_courses(filter_=filter_, 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 4b5eeb942fed..b5671ef173cb 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -113,7 +113,8 @@ def list_courses(request, org=None, filter_=None, search_term=None, - permissions=None): + permissions=None, + course_keys=None): """ Yield all available courses. @@ -142,12 +143,17 @@ def list_courses(request, permissions (list[str]): If specified, it filters visible `CourseOverview` objects by checking if each permission specified is granted for the username. + 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) + course_qs = get_courses( + user, org=org, filter_=filter_, permissions=permissions, 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 94fb34fde673..1b0a173ad828 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -63,6 +63,7 @@ class CourseListGetForm(UsernameValidatorMixin, Form): ] mobile = ExtendedNullBooleanField(required=False) permissions = MultiValueField(required=False) + course_keys = MultiValueField(required=False) def clean(self): """ @@ -79,6 +80,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 8de1b7875537..8c22482864a1 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -105,7 +105,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`. @@ -119,6 +120,7 @@ def _make_api_call(self, org=org, filter_=filter_, permissions=permissions, + course_keys=course_keys, ) def verify_courses(self, courses): @@ -242,6 +244,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 76c2fd081202..92a67541f03a 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -70,6 +70,7 @@ def set_up_data(self, user): 'search_term': '', 'filter_': None, 'permissions': set(), + 'course_keys': set(), } def test_basic(self): @@ -99,6 +100,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 5e4239e6f1f4..a8fabbfa0229 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -277,6 +277,10 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): Notice that Staff users are always granted permission to list any course. + 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 @@ -333,7 +337,8 @@ def get_queryset(self): org=form.cleaned_data['org'], filter_=form.cleaned_data['filter_'], search_term=form.cleaned_data['search_term'], - permissions=form.cleaned_data['permissions'] + permissions=form.cleaned_data['permissions'], + course_keys=form.cleaned_data['course_keys'], ) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index cd755a8d476f..3d7aab0ad19c 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -714,7 +714,7 @@ def get_course_syllabus_section(course, section_key): @function_trace('get_courses') -def get_courses(user, org=None, filter_=None, permissions=None): +def get_courses(user, org=None, filter_=None, permissions=None, 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 @@ -724,6 +724,7 @@ def get_courses(user, org=None, filter_=None, permissions=None): courses = branding.get_visible_courses( org=org, filter_=filter_, + 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 1b0b5bc3aae0..9776b4412d9c 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -653,7 +653,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): + def get_all_courses(cls, orgs=None, filter_=None, course_keys=None): """ Return a queryset containing all CourseOverview objects in the database. @@ -661,12 +661,17 @@ def get_all_courses(cls, orgs=None, filter_=None): orgs (list[string]): Optional parameter that allows case-insensitive filtering by organization. filter_ (dict): Optional parameter that allows custom filtering. + 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).