fix: [FC-0031] Fix count items in pagination for api courses list. #33293
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @oksana-slu! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
KyryloKireiev
force-pushed
the
rg/fix/FC0031/fix_count_items_in_pagination_for_api_courses_list
branch
from
bfb7bfc to
4b229ca
Compare
oksana-slu
force-pushed
the
rg/fix/FC0031/fix_count_items_in_pagination_for_api_courses_list
branch
from
4b229ca to
3bb2d49
Compare
|
Looks good to me. Can you please ask for a review in relevant squad i.e. team-aurora in this case. |
lms/djangoapps/course_api/api.py
Outdated
| @@ -100,13 +100,10 @@ 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] | |||
There was a problem hiding this comment.
@ormsbee is there gonna be a performance impact for changing the lazy sequence this way? I feel like getting an inaccurate count here is expected
It is immutable, and accepts an estimated length in order to support
__len__without exhausting the underlying sequence.
There was a problem hiding this comment.
@feanil: Almost certainly, yes. The whole point of LazySequence is to not have to iterate the entire queryset, which could potentially be 10K courses.
If this is related to the mobile effort, would it be possible to cap the number of results (e.g. pass in a parameter that will tell it to return no more than X results)? Doing something like the above with a cap of 100 courses or something should be fine.
There was a problem hiding this comment.
@oksana-slu let me know your thoughts but given the potential performance problem, I'm reluctant to land the change as is,.
There was a problem hiding this comment.
Wow, that's definitely a miss.
We discussed the possible solution today with @volodymyr-chekyrta, and it's a fair tradeoff to limit the number of returned results for mobile to 100 or even to 50 for now.
KyryloKireiev
force-pushed
the
rg/fix/FC0031/fix_count_items_in_pagination_for_api_courses_list
branch
from
24fc358 to
e120526
Compare
GlugovGrGlib
force-pushed
the
rg/fix/FC0031/fix_count_items_in_pagination_for_api_courses_list
branch
from
e120526 to
258f3fc
Compare
|
Hi @feanil @jawad-khan, we've updated the solution, kindly ask you to review the new implementation. We have introduced |
|
@feanil Yes, please merge it |
|
@oksana-slu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
This PR is resolving an issue with incorrect count returned from
paginationobject for all pages except for the last page while searching for courses using APIapi/courses/v1/courses/and providingsearch_termquery parameter.Supporting information
This contribution is done as a part of the project FC-0031
Testing instructions
GET
api/courses/v1/courses/specifying query_paramsearch_term.Check that
countparameter in a response has a correct value.