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.

Sign up for GitHub

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: Handle empty course_overviews in get_courses_order_by function #35912

Closed
wants to merge 1 commit into from
Closed

fix: Handle empty course_overviews in get_courses_order_by function #35912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 25 additions & 0 deletions cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,31 @@ def setUp(self):
end=(datetime.now() - timedelta(days=365)).replace(tzinfo=pytz.UTC),
)

def test_no_courses_non_staff_user(self):
"""Get list of courses when there are no courses available.

Expected result:
- An empty list of courses available to the logged in user.
"""
client, _ = self.create_non_staff_authed_user_client()
CourseOverviewFactory._meta.model.objects.all().delete()
response = client.get(self.api_v2_url, {"order": "display_name"})

expected_data = {
"courses": [],
"in_process_course_actions": [],
}
expected_response = OrderedDict([
('count', 0),
('num_pages', 1),
('next', None),
('previous', None),
('results', expected_data),
])

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertDictEqual(expected_response, response.data)

def test_home_page_response(self):
"""Get list of courses available to the logged in user.

Expand Down
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/views/course.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

@mariajgrimaldi mariajgrimaldi Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mariajgrimaldi mariajgrimaldi Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mariajgrimaldi mariajgrimaldi Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

return course_overviews
try:
return course_overviews.order_by(order_query)
Expand Down
Loading