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

mariajgrimaldi · 2024-12-02T19:08:56Z

Description

This PR addresses an issue reported while testing Sumac, where the API V2 is on by default in the authoring MFE: openedx/wg-build-test-release#428. It fails when retrieving an empty list of courses with the queryparams api/contentstore/v2/home/courses?page=1&order=display_name. When this was implemented, the course authoring MFE rendered the empty lists only with page=1 query param (didn't do any filtering/ordering by default), which was later changed to page=1&order=display_name which now ordered by default.

This issue occurs because all the filtering and ordering are done under the assumption that course_overviews is always a query set. However, that's only true when there are courses available and CourseOverview.get_all_courses is used. When not, an empty list is returned instead, raising a 500 error in Studio.

cms-1  |   File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 485, in get_filtered_and_ordered_courses
cms-1  |     course_overviews = get_courses_order_by(order, course_overviews)
cms-1  |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  |   File "/openedx/edx-platform/cms/djangoapps/contentstore/views/course.py", line 627, in get_courses_order_by
cms-1  |     return course_overviews.order_by(order_query)
cms-1  |            ^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1  | AttributeError: 'list' object has no attribute 'order_by'

Supporting information

openedx/wg-build-test-release#428
This feature was initially introduced here: #34173

Testing instructions

Sign up to studio as a new user
Try entering the Studio Home - no error should be raised
Ask for course creator permissions
Create a few courses
Filter them using the search bar or the filters available

Deadline

Before Thursday Dec 5.

The next step for this feature is the deprecation of Course Home API V1.

openedx-webhooks · 2024-12-02T19:09:01Z

Thanks for the pull request, @mariajgrimaldi!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mariajgrimaldi · 2024-12-03T10:34:37Z

cms/djangoapps/contentstore/views/course.py

+    else:
+        # If no course keys are found for the current user, then return without filtering
+        # or ordering the courses list.
+        return courses_list, []


This is the behavior this function had before #34173, you can review it here:
https://github.com/openedx/edx-platform/pull/34173/files#diff-937adee5129f53ead6b3c7c988d70db9e9e95862f17a6f85a1eea2388d12b461R575-L512

mariajgrimaldi · 2024-12-03T11:52:59Z

Hey @felipemontoya, just giving you a heads-up that this issue related to #34173 came up during Sumac testing.

felipemontoya

The changes looks completely reasonable. It is now complaining about the branch not being up to date, but other than that it looks good to merge

mariajgrimaldi · 2024-12-04T09:52:46Z

See the discussion here: #35912 (comment)

Ali-Salman29 · 2024-12-04T13:20:04Z

The changes look good to me.

mariajgrimaldi · 2024-12-04T13:36:10Z

@felipemontoya: could you re-review the PR? As a result of this discussion, I added type hints to the ordering/filtering function to be more explicit about the types they handle.

felipemontoya

lgtm

mariajgrimaldi · 2024-12-05T14:53:38Z

@Ali-Salman29 @felipemontoya: merged! Thank you for all the help.

edx-pipeline-bot · 2024-12-05T16:27:07Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

This change addresses an issue reported while testing Sumac, where the API V2 is on by default in the authoring MFE: openedx/wg-build-test-release#428. It fails when retrieving an empty list of courses with the queryparams api/contentstore/v2/home/courses?page=1&order=display_name. When this was implemented, the course authoring MFE rendered the empty lists only with page=1 query param (didn't do any filtering/ordering by default), which was later changed to page=1&order=display_name which now ordered by default. This issue occurs because all the filtering and ordering are done under the assumption that course_overviews is always a query set. However, that's only true when there are courses available and CourseOverview.get_all_courses is used. When not, an empty list is returned instead, raising a 500 error in Studio.

edx-pipeline-bot · 2024-12-06T15:34:08Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T15:34:08Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T15:46:27Z

2U Release Notice: This PR has been rolled back from the edX production environment.

edx-pipeline-bot · 2024-12-06T15:46:27Z

2U Release Notice: This PR has been rolled back from the edX production environment.

edx-pipeline-bot · 2024-12-06T17:59:39Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T17:59:39Z

2U Release Notice: This PR has been deployed to the edX production environment.

This change addresses an issue reported while testing Sumac, where the API V2 is on by default in the authoring MFE: openedx/wg-build-test-release#428. It fails when retrieving an empty list of courses with the queryparams api/contentstore/v2/home/courses?page=1&order=display_name. When this was implemented, the course authoring MFE rendered the empty lists only with page=1 query param (didn't do any filtering/ordering by default), which was later changed to page=1&order=display_name which now ordered by default. This issue occurs because all the filtering and ordering are done under the assumption that course_overviews is always a query set. However, that's only true when there are courses available and CourseOverview.get_all_courses is used. When not, an empty list is returned instead, raising a 500 error in Studio.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 2, 2024

mariajgrimaldi force-pushed the MJG/return-empty-course-list branch from 6b687c6 to 6cccb83 Compare December 2, 2024 19:35

mariajgrimaldi marked this pull request as ready for review December 2, 2024 19:36

mariajgrimaldi marked this pull request as draft December 2, 2024 20:45

mariajgrimaldi commented Dec 3, 2024

View reviewed changes

mariajgrimaldi force-pushed the MJG/return-empty-course-list branch from f58fd2f to faf99a5 Compare December 3, 2024 10:35

mariajgrimaldi marked this pull request as ready for review December 3, 2024 11:49

mariajgrimaldi force-pushed the MJG/return-empty-course-list branch from faf99a5 to 1a903cb Compare December 3, 2024 11:53

mariajgrimaldi mentioned this pull request Dec 3, 2024

[Test failure] < TC_AUTHOR_146 >: Cannot request course creator permission openedx/wg-build-test-release#428

Closed

mariajgrimaldi linked an issue Dec 3, 2024 that may be closed by this pull request

[Test failure] < TC_AUTHOR_146 >: Cannot request course creator permission openedx/wg-build-test-release#428

Closed

felipemontoya approved these changes Dec 3, 2024

View reviewed changes

Ali-Salman29 mentioned this pull request Dec 4, 2024

fix: Handle empty course_overviews in get_courses_order_by function #35912

Closed

mariajgrimaldi added 3 commits December 4, 2024 13:27

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

218f898

refactor: don't evaluate queryset before filtering/ordering

88f8ec6

test: add test cases for non-staff users

aba2b9f

mariajgrimaldi force-pushed the MJG/return-empty-course-list branch from 1a903cb to 5461fa1 Compare December 4, 2024 12:27

refactor: add type hints to filter/order functions

cf265ed

mariajgrimaldi force-pushed the MJG/return-empty-course-list branch from 5461fa1 to cf265ed Compare December 4, 2024 12:54

Ali-Salman29 approved these changes Dec 4, 2024

View reviewed changes

refactor: fix quality issues with type hints

4c6c4ab

felipemontoya approved these changes Dec 4, 2024

View reviewed changes

Merge branch 'master' into MJG/return-empty-course-list

ea5880d

mariajgrimaldi merged commit 1c835eb into master Dec 5, 2024
51 checks passed

mariajgrimaldi deleted the MJG/return-empty-course-list branch December 5, 2024 14:53

mariajgrimaldi mentioned this pull request Dec 5, 2024

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

Merged

feanil mentioned this pull request Dec 10, 2024

feanil/fix docs build #36004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mariajgrimaldi commented Dec 2, 2024 •

edited

Loading

openedx-webhooks commented Dec 2, 2024 •

edited

Loading

mariajgrimaldi Dec 3, 2024

mariajgrimaldi commented Dec 3, 2024 •

edited

Loading

felipemontoya left a comment

mariajgrimaldi commented Dec 4, 2024

Ali-Salman29 commented Dec 4, 2024

mariajgrimaldi commented Dec 4, 2024

felipemontoya left a comment

mariajgrimaldi commented Dec 5, 2024

edx-pipeline-bot commented Dec 5, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

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

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

Conversation

mariajgrimaldi commented Dec 2, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Dec 2, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

mariajgrimaldi Dec 3, 2024

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 3, 2024 • edited Loading

felipemontoya left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 4, 2024

Ali-Salman29 commented Dec 4, 2024

mariajgrimaldi commented Dec 4, 2024

felipemontoya left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 5, 2024

edx-pipeline-bot commented Dec 5, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

mariajgrimaldi commented Dec 2, 2024 •

edited

Loading

openedx-webhooks commented Dec 2, 2024 •

edited

Loading

mariajgrimaldi commented Dec 3, 2024 •

edited

Loading