fix: Handle empty course_overviews in get_courses_order_by function - #35912
fix: Handle empty course_overviews in get_courses_order_by function#35912Ali-Salman29 wants to merge 1 commit into
Conversation
45fb387 to
0b61077
Compare
|
Hello @ayesha-waris @AhtishamShahid I noticed your recent contributions to the edx-platform repository, and I truly appreciate the expertise you bring to the project. I was hoping you could review my PR whenever you have a moment. Thank You, |
0b61077 to
5b85fd0
Compare
3036670 to
dd5c7b8
Compare
|
@AhtishamShahid |
|
@mariajgrimaldi has already created the PR for the same issue. @AhtishamShahid, Should I close this PR now? |
|
@Ali-Salman29 @AhtishamShahid: thanks for tagging me! I completely missed this PR while researching the issue with the empty list. I went with another approach, but this also looks good! I'd also try handling the empty list here if that's possible: get_courses_by_search_query I like this approach since it's more centralized, I just have one concern so I left a review. Also, I'd suggest grabbing the test cases I added in #35942, so we know we're well covered. Thank you again! |
| course_overviews (Course Overview objects): queryset to be ordered. | ||
| """ | ||
| if not order_query: | ||
| if not (order_query and course_overviews): |
There was a problem hiding this comment.
What if course_overviews is actually a queryset, would this statement evaluate the queryset before the filtering/ordering? Would that affect performance?
Also, do you think adding typing annotations to these functions would be a good idea?
There was a problem hiding this comment.
Evaluating course_overviews would incur additional costs. I think it’s better to annotate these functions so they don’t accept empty lists and only accept querysets.
There was a problem hiding this comment.
Do you think there's an in-between solution that wouldn't incur additional costs but would catch this type of inconsistency with lists vs querysets?
While implementing the other approach, I tried evaluating courses_list, which increased the hits to the database. That's why I went with checking if course_keys was empty and then immediately returning here, but I wonder if there are other cases I'm not considering where lists could be used instead of querysets so a more centralized approach like this should be favored.
Let me know what you think!
There was a problem hiding this comment.
To make it more centralized and minimize costs, we can add a condition for lists. For example:
def get_courses_order_by(order_query, course_overviews):
"""
Return course overviews based on a base queryset ordered by a query.
Args:
order_query (str): Any string used to order Course Overviews.
course_overviews (QuerySet or list): Queryset to be ordered or a list of objects.
Returns:
QuerySet or list: Ordered queryset or the original list if ordering is not applicable.
"""
if isinstance(course_overviews, list):
log.warning("Expected a queryset but received a list object. Skipping ordering.")
return course_overviews
if not order_query:
return course_overviews
try:
return course_overviews.order_by(order_query)
except FieldError as e:
log.exception(f"Error ordering courses by {order_query}: {e}")
return course_overviewsHowever, your solution is much better because the function expects a queryset, not a list. I’ve reviewed the code, and this function is only used in get_filtered_and_ordered_courses, which is being used in _accessible_courses_summary_iter and _accessible_courses_list_from_groups. The case won’t occur in _accessible_courses_summary_iter, and your changes also validate this case in _accessible_courses_list_from_groups.
There was a problem hiding this comment.
Thank you for the thorough research on where this is currently being used. Although I'd prefer a more centralized solution, I agree that leaving the responsibility to the caller is the easiest and least impactful route. A good in-between is adding type-hints as well, so I'm adding them.
Can you leave me a review in the PR? Thank you!
|
Closing the PR. The issue has been fixed in #35942. |
3 participants
Description
This pull request fixes an issue in the
get_courses_order_byfunction where an empty list is passed as a course_overviews throws an error. Previously, the function attempted to callorder_by()on an empty list, leading to unexpected crashes. This change ensures that the function explicitly checks ifcourse_overviewsis empty and returns an empty list in such cases.Changes Made
Supporting information
This fixes the following traceback error:
The error occurred during course filtering and sorting operations in the CMS, where an invalid or empty course_overviews queryset caused the failure.
Testing instructions
Steps to Reproduce the Error:
api/contentstore/v2/home/courses?page=1&order=display_name, an error message will appear stating: "Failed to fetch courses. Please try again later."After Incorporating PR Changes:
Deadline
None
Other information