Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [FC-0031] Fix count items in pagination for api courses list. #33293

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 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 @@ -101,6 +101,13 @@ def _filter_by_search(course_queryset, search_term):

search_courses_ids = {course['data']['id'] for course in search_courses['results']}

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(
(
course for course in course_queryset
Expand All @@ -117,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 @@ -150,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 @@ -158,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
40 changes: 40 additions & 0 deletions lms/djangoapps/course_api/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,46 @@ def test_too_many_courses(self):
assert len(response.data['results']) == (30 if (page < 11) else 3)
assert [c['id'] for c in response.data['results']] == ordered_course_ids[((page - 1) * 30):(page * 30)]

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)] # pylint: disable=expression-not-assigned
response = self.verify_response(params={"search_term": "new"})
self.assertEqual(response.status_code, 200)
# 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):
"""
Test count items in pagination for api courses list
with search_term and filter by organisation -
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)] # 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 @@ -4428,6 +4428,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
Loading