fix: Handle empty course_overviews in get_courses_order_by function #35912

Ali-Salman29 · 2024-11-21T15:07:13Z

Description

This pull request fixes an issue in the get_courses_order_by function where an empty list is passed as a course_overviews throws an error. Previously, the function attempted to call order_by() on an empty list, leading to unexpected crashes. This change ensures that the function explicitly checks if course_overviews is empty and returns an empty list in such cases.

Changes Made

Added a condition to return an empty list if course_overviews is None or empty.
Improved the robustness of the function to handle edge cases gracefully.

2024-12-02 13:35:13,362 ERROR 378 [django.request] [user None] [ip None] log.py:241 - Internal Server Error: /api/contentstore/v2/home/courses
Traceback (most recent call last):
  File "/openedx/venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/pyenv/versions/3.11.8/lib/python3.11/contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "/openedx/venv/lib/python3.11/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view
    return view_func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/venv/lib/python3.11/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/venv/lib/python3.11/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/venv/lib/python3.11/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/openedx/venv/lib/python3.11/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/openedx/venv/lib/python3.11/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/rest_api/v2/views/home.py", line 136, in get
    courses, in_process_course_actions = get_course_context_v2(request)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/utils.py", line 1666, in get_course_context_v2
    courses_iter, in_process_course_actions = get_courses_accessible_to_user(request, org)
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/pyenv/versions/3.11.8/lib/python3.11/contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 770, in get_courses_accessible_to_user
    courses, in_process_course_actions = _accessible_courses_list_from_groups(request)
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 580, in _accessible_courses_list_from_groups
    courses_list = get_filtered_and_ordered_courses(
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 485, in get_filtered_and_ordered_courses
    course_overviews = get_courses_order_by(order, course_overviews)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 627, in get_courses_order_by
    return course_overviews.order_by(order_query)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'list' object has no attribute 'order_by'
[02/Dec/2024 13:35:13] "GET /api/contentstore/v2/home/courses?page=1&order=display_name HTTP/1.1" 500 304112

Supporting information

This fixes the following traceback error:

return course_overviews.order_by(order_query)
AttributeError: 'list' object has no attribute 'order_by'

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:

Create a non-staff user through the registration page.
Access Studio; you will be redirected to course-authoring/home.
During an API call to 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:

Repeat the same steps after applying the PR changes.
Verify that the page loads correctly without errors and displays a component for adding a course or becoming a course instructor.

Deadline

None

Other information

This change does not introduce migrations, security issues, or deprecations.
No configuration changes are required.

Ali-Salman29 · 2024-12-02T14:06:56Z

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,
Ali Salman

Ali-Salman29 · 2024-12-04T09:08:32Z

@AhtishamShahid
If all looks good to you. Could you please merge this to the master?

Ali-Salman29 · 2024-12-04T09:13:39Z

@mariajgrimaldi has already created the PR for the same issue.
#35942

@AhtishamShahid, Should I close this PR now?

mariajgrimaldi · 2024-12-04T09:45:57Z

@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
get_courses_by_status

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!

mariajgrimaldi · 2024-12-04T09:58:55Z

cms/djangoapps/contentstore/views/course.py

@@ -621,7 +621,7 @@ def get_courses_order_by(order_query, course_overviews):
        order_query (str): any string used to order Course Overviews.
        course_overviews (Course Overview objects): queryset to be ordered.
    """
-    if not order_query:
+    if not (order_query and course_overviews):


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?

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.

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!

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_overviews

However, 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.

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!

Ali-Salman29 · 2024-12-04T15:15:47Z

Closing the PR. The issue has been fixed in #35942.

Ali-Salman29 force-pushed the alisalman/fix-studio-courses-list branch 2 times, most recently from 45fb387 to 0b61077 Compare December 2, 2024 13:30

AhtishamShahid approved these changes Dec 3, 2024

View reviewed changes

Ali-Salman29 force-pushed the alisalman/fix-studio-courses-list branch from 0b61077 to 5b85fd0 Compare December 3, 2024 09:14

fix: Handle empty course_overviews in get_courses_order_by function

dd5c7b8

Ali-Salman29 force-pushed the alisalman/fix-studio-courses-list branch from 3036670 to dd5c7b8 Compare December 4, 2024 08:43

mariajgrimaldi mentioned this pull request Dec 4, 2024

fix: return empty list when no courses are found for request #35942

Merged

mariajgrimaldi reviewed Dec 4, 2024

View reviewed changes

Ali-Salman29 closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Handle empty course_overviews in get_courses_order_by function #35912

fix: Handle empty course_overviews in get_courses_order_by function #35912

Ali-Salman29 commented Nov 21, 2024 •

edited

Loading

Ali-Salman29 commented Dec 2, 2024 •

edited

Loading

Ali-Salman29 commented Dec 4, 2024

Ali-Salman29 commented Dec 4, 2024

mariajgrimaldi commented Dec 4, 2024 •

edited

Loading

mariajgrimaldi Dec 4, 2024 •

edited

Loading

Ali-Salman29 Dec 4, 2024

mariajgrimaldi Dec 4, 2024 •

edited

Loading

Ali-Salman29 Dec 4, 2024

mariajgrimaldi Dec 4, 2024 •

edited

Loading

Ali-Salman29 commented Dec 4, 2024

fix: Handle empty course_overviews in get_courses_order_by function #35912

fix: Handle empty course_overviews in get_courses_order_by function #35912

Conversation

Ali-Salman29 commented Nov 21, 2024 • edited Loading

Description

Supporting information

Testing instructions

Steps to Reproduce the Error:

After Incorporating PR Changes:

Deadline

Other information

Ali-Salman29 commented Dec 2, 2024 • edited Loading

Ali-Salman29 commented Dec 4, 2024

Ali-Salman29 commented Dec 4, 2024

mariajgrimaldi commented Dec 4, 2024 • edited Loading

mariajgrimaldi Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Ali-Salman29 Dec 4, 2024

Choose a reason for hiding this comment

mariajgrimaldi Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Ali-Salman29 Dec 4, 2024

Choose a reason for hiding this comment

mariajgrimaldi Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Ali-Salman29 commented Dec 4, 2024

Ali-Salman29 commented Nov 21, 2024 •

edited

Loading

Ali-Salman29 commented Dec 2, 2024 •

edited

Loading

mariajgrimaldi commented Dec 4, 2024 •

edited

Loading

mariajgrimaldi Dec 4, 2024 •

edited

Loading

mariajgrimaldi Dec 4, 2024 •

edited

Loading

mariajgrimaldi Dec 4, 2024 •

edited

Loading