From b7f49239771a84e31e7c97f6c02a44757a5aa9dc Mon Sep 17 00:00:00 2001 From: KyryloKireiev Date: Wed, 1 Nov 2023 15:20:00 +0200 Subject: [PATCH] fix: [FC-0031] Add limit the number of returned results for mobile_search --- lms/djangoapps/course_api/api.py | 26 ++++++++++++++----- lms/djangoapps/course_api/forms.py | 1 + lms/djangoapps/course_api/tests/test_forms.py | 1 + lms/djangoapps/course_api/tests/test_views.py | 25 +++++++++++++++--- lms/djangoapps/course_api/views.py | 5 ++++ lms/envs/common.py | 3 +++ 6 files changed, 51 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 39c34ec6da95..8210396441c7 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -82,7 +82,7 @@ def course_detail(request, username, course_key): return overview -def _filter_by_search(course_queryset, search_term): +def _filter_by_search(course_queryset, search_term, mobile_search=False): """ Filters a course queryset by the specified search term. """ @@ -100,10 +100,20 @@ def _filter_by_search(course_queryset, search_term): ) search_courses_ids = {course['data']['id'] for course in search_courses['results']} - courses = [course for course in course_queryset if str(course.id) in search_courses_ids] + + if mobile_search is True: + course_limit = getattr(settings, 'MOBILE_SEARCH_COURSE_LIMIT', 100) + courses = [course for course in course_queryset[:course_limit] if str(course.id) in search_courses_ids] + return LazySequence( + iter(courses), + est_len=len(courses) + ) return LazySequence( - iter(courses), - est_len=len(courses) + ( + course for course in course_queryset + if str(course.id) in search_courses_ids + ), + est_len=len(course_queryset) ) @@ -114,7 +124,8 @@ def list_courses(request, search_term=None, permissions=None, active_only=False, - course_keys=None): + course_keys=None, + mobile_search=False): """ Yield all available courses. @@ -147,6 +158,9 @@ def list_courses(request, course_keys (list[str]): If specified, it filters visible `CourseOverview` objects by the course keys (ids) provided + mobile_search (bool): + Optional parameter that limits the number of returned courses + to MOBILE_SEARCH_COURSE_LIMIT. Return value: Yield `CourseOverview` objects representing the collection of courses. @@ -155,7 +169,7 @@ def list_courses(request, 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) + course_qs = _filter_by_search(course_qs, search_term, mobile_search) return course_qs diff --git a/lms/djangoapps/course_api/forms.py b/lms/djangoapps/course_api/forms.py index ff6cee84aea6..79dba03f45e8 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -65,6 +65,7 @@ class CourseListGetForm(UsernameValidatorMixin, Form): active_only = ExtendedNullBooleanField(required=False) permissions = MultiValueField(required=False) course_keys = MultiValueField(required=False) + mobile_search = ExtendedNullBooleanField(required=False) def clean(self): """ diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index 3b5744402e79..43a4aa104bde 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -72,6 +72,7 @@ def set_up_data(self, user): 'permissions': set(), 'active_only': None, 'course_keys': set(), + 'mobile_search': None, } def test_basic(self): diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index bd0c10aff6dd..c39f808478ef 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -454,10 +454,12 @@ def test_count_item_pagination_with_search_term(self): Test count items in pagination for api courses list - class CourseListView """ # Create 15 new courses, courses have the word "new" in the title - _ = [self.create_and_index_course(f"numb_{number}", f"new_{number}") for number in range(15)] + [self.create_and_index_course(f"numb_{number}", f"new_{number}") for number in range(15)] # pylint: disable=expression-not-assigned response = self.verify_response(params={"search_term": "new"}) self.assertEqual(response.status_code, 200) - self.assertEqual(response.json()["pagination"]["count"], 15) + # We don't have 'count' 15 because 'mobile_search' param is None + # And LazySequence contains all courses + self.assertEqual(response.json()["pagination"]["count"], 18) def test_count_item_pagination_with_search_term_and_filter(self): """ @@ -466,12 +468,27 @@ def test_count_item_pagination_with_search_term_and_filter(self): class CourseListView """ # Create 25 new courses with two different organisations - _ = [self.create_and_index_course("Org_N", f"new_{number}") for number in range(10)] - _ = [self.create_and_index_course("Org_X", f"new_{number}") for number in range(15)] + [self.create_and_index_course("Org_N", f"new_{number}") for number in range(10)] # pylint: disable=expression-not-assigned + [self.create_and_index_course("Org_X", f"new_{number}") for number in range(15)] # pylint: disable=expression-not-assigned response = self.verify_response(params={"org": "Org_X", "search_term": "new"}) self.assertEqual(response.status_code, 200) self.assertEqual(response.json()["pagination"]["count"], 15) + def test_count_item_pagination_with_search_term_and_mobile_search(self): + """ + Test count items in pagination for api courses list + with search_term and 'mobile_search' is True + """ + # Create 25 new courses with two different words in titles + [self.create_and_index_course("Org_N", f"old_{number}") for number in range(10)] # pylint: disable=expression-not-assigned + [self.create_and_index_course("Org_N", f"new_{number}") for number in range(15)] # pylint: disable=expression-not-assigned + response = self.verify_response( + params={"search_term": "new", "mobile_search": True} + ) + self.assertEqual(response.status_code, 200) + # We have 'count' 15 because 'mobile_search' param is true + self.assertEqual(response.json()["pagination"]["count"], 15) + class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 98b01232a1be..b2019c793fec 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -290,6 +290,10 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): If specified, it fetches the `CourseOverview` objects for the the specified course keys + mobile_search (bool): + Optional parameter that limits the number of returned courses + to MOBILE_SEARCH_COURSE_LIMIT. + **Returns** * 200 on success, with a list of course discovery objects as returned @@ -349,6 +353,7 @@ def get_queryset(self): permissions=form.cleaned_data['permissions'], active_only=form.cleaned_data.get('active_only', False), course_keys=form.cleaned_data['course_keys'], + mobile_search=form.cleaned_data.get('mobile_search', False), ) diff --git a/lms/envs/common.py b/lms/envs/common.py index f0e8bc9383bb..57ce369542db 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4446,6 +4446,9 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring r'edX/org.edx.mobile', ] +# set course limit for mobile search +MOBILE_SEARCH_COURSE_LIMIT = 100 + # cache timeout in seconds for Mobile App Version Upgrade APP_UPGRADE_CACHE_TIMEOUT = 3600