Skip to content

Commit

Permalink
fix: [FC-0031] Add limit the number of returned results for mobile_se…
Browse files Browse the repository at this point in the history
…arch
  • Loading branch information
KyryloKireiev authored and OmarIthawi committed May 31, 2024
1 parent 9060663 commit b7f4923
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 10 deletions.
26 changes: 20 additions & 6 deletions lms/djangoapps/course_api/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
"""
Expand All @@ -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)
)


Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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


Expand Down
1 change: 1 addition & 0 deletions lms/djangoapps/course_api/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
1 change: 1 addition & 0 deletions lms/djangoapps/course_api/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
25 changes: 21 additions & 4 deletions lms/djangoapps/course_api/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand All @@ -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):
"""
Expand Down
5 changes: 5 additions & 0 deletions lms/djangoapps/course_api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
)


Expand Down
3 changes: 3 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit b7f4923

Please sign in to comment.