feat: add paginated HomePageCoursesV2 view with filtering & ordering #34173

mariajgrimaldi · 2024-02-02T14:47:04Z

Description

This PR adds a new version of the HomePageCourses API with pagination, filtering & ordering, intending to be used by the new Course Home MFE. The new API is based on HomePageCourses API with tweaks, so it gets courses with Course Overview helpers that allow filtering and ordering with query set API methods.

The scope of this implementation only considers the API endpoint used to list courses in the Course Home MFE which is the HomePageCoursesViewV2 that serves api/contentstore/v2/home/courses. Other API endpoints in V1 will not be implemented as part of this initiative.

API changes compared to V1

As I mentioned before, the HomePage Courses API version 2 is implemented based on the API version 1 with a few changes:

We're using a new get_course_context_v2 function to get the list of active and archived courses, and the in process course actions. That function is based on get_course_context but with pagination configurations.
Also, courses are not split into active/inactive but filtered using a status query parameter.

Other changes

When using the API, the ENABLE_HOME_PAGE_COURSE_API_V2 feature toggle should be enabled, so we're not hitting Mongo to get all course summaries. Instead, we're listing them using the Django ORM for SQL db engines: https://github.com/openedx/edx-platform/blob/908753b895f41d1f8c5dc3b6c34c26744dd50fa2/cms/djangoapps/contentstore/views/course.py#L445. These changes make querying courses easier using Q objects instead of list filtering.
Helpers modifications to support query params to ordering or filtering courses when needed: https://github.com/openedx/edx-platform/pull/34173/files#diff-937adee5129f53ead6b3c7c988d70db9e9e95862f17a6f85a1eea2388d12b461R418-R428

Supporting information

For more information on this enhancement, please check:
https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4032856101/Studio+Home+incremental+upgrades+-+product+approach

Testing instructions

1st, turn on the Django ORM only reads by setting: FEATURES["ENABLE_HOME_PAGE_COURSE_API_V2"] = True in your environment. Then, login as an administrator or as course staff (they should both work):

You can use your browser to get your list of courses from Studio HTTP(s)://<CMS_HOST>/api/contentstore/v2/home/courses. Check that the output is the same as: HTTP(s)://<CMS_HOST>/api/contentstore/v1/home/courses
Now, try filtering courses by their course ID, org or display name: /api/contentstore/v2/home/courses?search=demo
Then, try ordering courses by their course ID, org or display name /api/contentstore/v2/home/courses?order=display_name , /api/contentstore/v2/home/courses?order=-display_name for alphabetic order, order=created or order=-created for creation date
Or both 3. and 4.

Deadline

This effort is part of the Spanish consortium project, so it'd be ideal to merge this before the end of the project.

openedx-webhooks · 2024-02-02T14:47:09Z

Thanks for the pull request, @mariajgrimaldi! 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:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

cms/djangoapps/contentstore/rest_api/v2/views/home.py

cms/djangoapps/contentstore/views/course.py

mariajgrimaldi · 2024-02-06T13:02:30Z

I've addressed your comments. Could you check again? Thanks!

mariajgrimaldi · 2024-02-06T13:59:49Z

Notes to reviewer

As I mentioned in the cover letter, this PR is heavily inspired by the HomePageCourses V1. We're using the same structure as the previous version implementation but with helpers that allow filtering and ordering without refactoring them to make them more suitable for this API-MFE architecture. So, there's room for improvement in these implementations to modernize them and make them more maintainable.

mariajgrimaldi · 2024-02-06T14:10:23Z

cms/djangoapps/contentstore/views/course.py

+def _accessible_courses_summary_iter_v2(request, org=None):
+    """
+    List all courses available to the logged in user by iterating through all the courses.
+
+    Args:
+        request: the request object
+        org (string): if not None, this value will limit the courses returned. An empty
+            string will result in no courses, and otherwise only courses with the
+            specified org will be returned. The default value is None.
+    """
+    def course_filter(course_summary):
+        """
+        Filter out inaccessible courses for the current logged in user.
+
+        Args:
+            course_summary (CourseOverview): the course overview object.
+        """
+        return has_studio_read_access(request.user, course_summary.id)
+
+    if org is not None:
+        courses_summary = [] if org == '' else CourseOverview.get_all_courses(orgs=[org])
+    else:
+        courses_summary = CourseOverview.get_all_courses()
+
+    search_query = request.GET.get('search')
+    order = request.GET.get('order')
+    active_only = get_bool_param(request, 'active_only', None)
+    archived_only = get_bool_param(request, 'archived_only', None)
+    courses_summary = get_courses_by_status(active_only, archived_only, courses_summary)
+    courses_summary = get_courses_by_search_query(search_query, courses_summary)
+    courses_summary = get_courses_order_by(order, courses_summary)
+    courses_summary = list(filter(course_filter, courses_summary))
+    in_process_course_actions = get_in_process_course_actions(request)
+    return courses_summary, in_process_course_actions


Notes to reviewer

As I mentioned here, there's still room for improvement here. Here's a list of things I'd like to do but don't have the time to do so:

Use Custom Django filters instead of those get_courses_by... in the helpers _accessible_courses_summary_iter_v2 and _accessible_courses_list_from_groups_v2

Try refactoring get_courses_accessible_to_user_v2 so a single course getter could be used. Or at least smaller/simpler ones

Moving out this implementation to its own helper so the parent function it's easier to read

I'll still open this PR for review and hope to have more time to make these changes.

I agree, the idea was to replicate API v1 including some changes, it was done and it works. Later, we can make appropriate improvements to the code.

cms/djangoapps/contentstore/rest_api/v2/serializers/home.py

johnvente

Hi, I've tested this and is working for me as expected

mariajgrimaldi · 2024-02-13T19:07:39Z

Hi there, folks @felipemontoya @feanil! This is our approach for the API that was in conversations for the Studio Home incremental upgrades. As the cover letter says, it's heavily inspired in Studio HomePage V1 included in these PRs:
#33204
#33909
But with the necessary changes for this feature to work. Let me know what you think about the approach!

cms/djangoapps/contentstore/views/course.py

feanil

The URL signature of v2 and the output format look like it's a superset of v1, does it make sense to make a whole new version instead of just enhancing v1? What's preventing us from updating v2 to do more work when more query params are provided? Is there something I'm missing about the signature or the output format?

feanil · 2024-02-15T20:28:20Z

cms/djangoapps/contentstore/utils.py

+def get_course_context_v2(request):
+    """Get context of the homepage course tab from the Studio Home."""
+
+    from cms.djangoapps.contentstore.views.course import (


Why is this import inside this function? Can we add a comment to explain?

I moved it outside the function along with the other imports and got:

cms-1 | Traceback (most recent call last): cms-1 | File "./manage.py", line 103, in <module> cms-1 | startup.run() cms-1 | File "/openedx/edx-platform/cms/startup.py", line 20, in run cms-1 | django.setup() cms-1 | File "/openedx/venv/lib/python3.8/site-packages/django/__init__.py", line 24, in setup cms-1 | apps.populate(settings.INSTALLED_APPS) cms-1 | File "/openedx/venv/lib/python3.8/site-packages/django/apps/registry.py", line 124, in populate cms-1 | app_config.ready() cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/apps.py", line 23, in ready cms-1 | from .signals import handlers # pylint: disable=unused-import cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/signals/handlers.py", line 19, in <module> cms-1 | from cms.djangoapps.contentstore.courseware_index import ( cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/courseware_index.py", line 15, in <module> cms-1 | from cms.djangoapps.contentstore.course_group_config import GroupConfiguration cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/course_group_config.py", line 12, in <module> cms-1 | from cms.djangoapps.contentstore.utils import reverse_usage_url cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/utils.py", line 88, in <module> cms-1 | from cms.djangoapps.contentstore.views.course import ( cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/views/__init__.py", line 3, in <module> cms-1 | from .assets import * cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/views/assets.py", line 6, in <module> cms-1 | from cms.djangoapps.contentstore.asset_storage_handlers import ( cms-1 | File "/openedx/edx-platform/cms/djangoapps/contentstore/asset_storage_handlers.py", line 35, in <module> cms-1 | from .utils import reverse_course_url, get_files_uploads_url, get_response_format, request_response_format_is_json cms-1 | ImportError: cannot import name 'reverse_course_url' from partially initialized module 'cms.djangoapps.contentstore.utils' (most likely due to a circular import) (/openedx/edx-platform/cms/djangoapps/contentstore/utils.py)

I have already added the comment explaining why it needs to be done.

feanil · 2024-02-15T20:38:25Z

cms/djangoapps/contentstore/utils.py

+
+    optimization_enabled = GlobalStaff().has_user(request.user) and ENABLE_GLOBAL_STAFF_OPTIMIZATION.is_enabled()
+
+    org = request.GET.get('org', '') if optimization_enabled else None


It seems like blank vs empty string will result in different behavior here? Can you add a comment explaining what the expected difference is between those two? Or maybe that explaination needs to go into the docstring of get_course_accessible_to_user_v2?

This is precisely how V1 behaves. So, I'll have to test out the behavior with those specifications. I'll make sure to answer once I know more.

So I researched the behavior: org= and org='' which correspond to '' and "''" respectively. So for the blank string "''" this line would be executed: https://github.com/openedx/edx-platform/pull/34173/files#diff-937adee5129f53ead6b3c7c988d70db9e9e95862f17a6f85a1eea2388d12b461R443, but for the empty string '' it will not. Unless there are organizations named '' or any other variation of the blank string -from what I see, you can actually insert rows into the course overview table with the org '' since there are no restrictions from the course table- then the result would be empty for the "''" case.
I'm not sure why this is a case considered, so I can't explain it.

By the way, these helpers (get_course_context_v2, etc) are based on the ones used by the v1 API with some modifications:

get_course_context_v2 -> based on get_course_context

get_courses_accessible_to_user_v2 -> based on get_courses_accessible_to_user

_accessible_courses_summary_iter_v2 -> based on _accessible_courses_summary_iter

And so on. I thought it would be easier instead of modifying the behavior of the existing ones in case other structures depended on them. Now I think it's even more confusing having it this way, duplicating code in a V2 version helper.

So I'll be modifying the v1 helpers instead where I see fit.

Done. But I broke some tests in the process. I'll fix them if this approach is approved.

mariajgrimaldi · 2024-02-15T21:22:10Z

The URL signature of v2 and the output format look like it's a superset of v1, does it make sense to make a whole new version instead of just enhancing v1? What's preventing us from updating v2 to do more work when more query params are provided? Is there something I'm missing about the signature or the output format?

@feanil: thanks for the reply. To clarify: this is our first approach, it's not a definitive solution to have a V2 API. We're open to this being a V1 enhancement if that's better. Here are some reasons why we think it's a breaking change, which means migrating to V2 and then deprecating V1. In V1, the output is:

        {
            "archived_courses": [...],
            "courses": [...],
            "in_process_course_actions": [],
        }

There's a differentiation between active/archived courses. In V2, the client should pass query params (active_only/archived_only or none to list both) to specify which courses to list, so it'd look like this:

        {
            "courses": [...], # list of active courses, archived or both
            "in_process_course_actions": [],
        }

Also, we're adding pagination in a different PR, which changes how to handle the data returned by the API. And the course serializer has more information, specifically the is_active and cms_link fields. Another change that would make sense is what I described here about in_process_course_actions, which would also change the API output, but it wasn't implemented.

As I mentioned, we're open to accommodating these changes as V1 improvements if that's better.

mariajgrimaldi · 2024-02-20T01:34:29Z

Some notes about performance

We ran simple performance tests to study the API's behavior with different stress loads and user roles. To do so, we used a tutor local nightly environment in an EC2 t2.large instance machine and Postman as the test runner. We started with a database equipped with ~42k course overviews:

There are 10 items per page
There are 41748 course overview records
There are 4175 pages
There are 1197 course overviews matching demo (either the org / display name)
There are 22067 active course overviews

Iterations: 1 per test
Consecutively

ID	Request path	Response time	User role
Test 1	/api/contentstore/v2/home/courses?page=1	10.12s	Global staff
Test 3	/api/contentstore/v2/home/courses?search=demo	637ms	Global staff
Test 4	/api/contentstore/v2/home/courses?active_only=true	5.42s	Global staff

Iterations: 1 per test
Consecutively

ID	Request path	Response time	User role
Test 5	/api/contentstore/v2/home/courses?page=1	213ms	Instructor
Test 7	/api/contentstore/v2/home/courses?search=demo	215ms	Instructor

Iterations: 100 per test
Simultaneous

ID	Request path	Duration	Avg. Resp. Time	User role
Test 1	/api/contentstore/v2/home/courses?page=1	22m 10s	13250ms	Global staff
Test 2	/api/contentstore/v2/home/courses?page=100...200	22m 6s	13211ms	Global staff
Test 3	/api/contentstore/v2/home/courses?search=demo	11m 59s	7144ms	Global staff
Test 4	/api/contentstore/v2/home/courses?active_only=true	13m 12s	7872ms	Global staff

Iterations: 100 per test
Simultaneous

ID	Request path	Duration	Avg. Resp. Time	User role
Test 5	/api/contentstore/v2/home/courses?page=1	23s 831ms	194ms	Instructor
Test 6	/api/contentstore/v2/home/courses?page=1...2	22s 801ms	181ms	Instructor
Test 7	/api/contentstore/v2/home/courses?search=demo	23s 326ms	185ms	Instructor

So, the global staff has access to the ~42k courses; meanwhile, the instructor can access 12. A staff user's average of ~13s is not a bad result given the test circumstances (like infrastructure details, amount of courses, simultaneous test runs, etc). We also ran a cprofile report in our environment, I'll attach some screenshots here for you to see:

Let me know if you want another type of information from this report.

This reverts commit 5d53a1e.

This reverts commit 5ff30de.

This reverts commit 0638311.

felipemontoya

Tested it again, it works nicely. Thanks for updating the default and making sure the tests cover this case thoroughly.

felipemontoya · 2024-03-20T01:38:17Z

btw I should add that this time I also tested the course-authoring master to make sure that with the feature on, the old course-authoring home still works as usual.

mariajgrimaldi · 2024-03-20T15:31:48Z

I'll merge this now since all comments were addressed, and the PR has engineering approval. Thank you.

openedx-webhooks · 2024-03-20T15:32:03Z

@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-03-20T21:24:30Z

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

edx-pipeline-bot · 2024-03-20T21:44:26Z

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

Turn on getting courses from the HomePageCourses API which allows pagination, filtering and ordering. See openedx/edx-platform#34173 for more details on the API implementation.

* refactor!: turn on homepage course API V2 consumption by default Turn on getting courses from the HomePageCourses API which allows pagination, filtering and ordering. See openedx/edx-platform#34173 for more details on the API implementation. * fix: home page initial a-z course sort --------- Co-authored-by: Diana Olarte <[email protected]>

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

mariajgrimaldi force-pushed the MJG/course-home-api-v2 branch 2 times, most recently from 064e3a6 to 150297d Compare February 2, 2024 17:43

mariajgrimaldi changed the title ~~Mjg/course home api v2~~ feat: add CourseHomeCoursesV2 with filtering & ordering capabilities Feb 2, 2024

mariajgrimaldi changed the title ~~feat: add CourseHomeCoursesV2 with filtering & ordering capabilities~~ [WIP] feat: add CourseHomeCoursesV2 with filtering & ordering capabilities Feb 2, 2024

mariajgrimaldi mentioned this pull request Feb 5, 2024

feat: add pagination to the HomePageCoursesV2 API #34175

Merged

mariajgrimaldi force-pushed the MJG/course-home-api-v2 branch from f829a42 to 516dc90 Compare February 5, 2024 21:17

mariajgrimaldi requested review from BryanttV and johnvente February 5, 2024 21:21

mariajgrimaldi changed the title ~~[WIP] feat: add CourseHomeCoursesV2 with filtering & ordering capabilities~~ [WIP] feat: add HomePageCoursesV2 with filtering & ordering capabilities Feb 5, 2024

BryanttV reviewed Feb 6, 2024

View reviewed changes

mariajgrimaldi requested a review from BryanttV February 6, 2024 13:30

mariajgrimaldi changed the title ~~[WIP] feat: add HomePageCoursesV2 with filtering & ordering capabilities~~ feat: add HomePageCoursesV2 with filtering & ordering capabilities Feb 6, 2024

mariajgrimaldi marked this pull request as ready for review February 6, 2024 13:54

mariajgrimaldi commented Feb 6, 2024

View reviewed changes

mariajgrimaldi force-pushed the MJG/course-home-api-v2 branch from b310100 to 8f44261 Compare February 6, 2024 14:27

BryanttV reviewed Feb 6, 2024

View reviewed changes

cms/djangoapps/contentstore/rest_api/v2/serializers/home.py Outdated Show resolved Hide resolved

johnvente mentioned this pull request Feb 6, 2024

feat: pagination studio home for courses openedx/frontend-app-authoring#825

Merged

mariajgrimaldi requested a review from BryanttV February 7, 2024 12:45

BryanttV approved these changes Feb 7, 2024

View reviewed changes

johnvente approved these changes Feb 7, 2024

View reviewed changes

mariajgrimaldi commented Feb 15, 2024

View reviewed changes

cms/djangoapps/contentstore/views/course.py Outdated Show resolved Hide resolved

feanil reviewed Feb 15, 2024

View reviewed changes

mariajgrimaldi force-pushed the MJG/course-home-api-v2 branch from 9528663 to fa66d6c Compare February 19, 2024 14:35

mariajgrimaldi added 16 commits March 19, 2024 17:31

Revert "fix: address test failures"

8b306da

This reverts commit 5d53a1e.

fix: address test failures

b5a5f36

fix: import missing check_mongo_calls

4aeee4e

refactor: modify tests to match latest changes in API helpers

46d9feb

docs: add in-line comment explaining why read from mysql

8fbd800

refactor: register URL if and only if homepage course API v2 is enabled

a9d13d5

fix: correct variable name according feature toggle

73a9586

refactor: return not found when feature not enabled

8e6b1f5

docs: update method docstrings

fb848f9

refactor: address PR reviews

d1e5b99

Revert "Revert "fix: address test failures""

f808777

This reverts commit 5ff30de.

refactor: turn on feature as default

084adfa

Revert "Revert "fix: address test failures""

9666b1d

This reverts commit 0638311.

fix: address test failures

3fb4b67

fix: address test (quality) failures

7923987

fix: address test failures

93e55c8

mariajgrimaldi force-pushed the MJG/course-home-api-v2 branch from 08cd471 to 93e55c8 Compare March 19, 2024 21:31

felipemontoya approved these changes Mar 20, 2024

View reviewed changes

arbrandes removed their request for review March 20, 2024 15:06

mariajgrimaldi merged commit 45178e0 into master Mar 20, 2024
67 checks passed

mariajgrimaldi deleted the MJG/course-home-api-v2 branch March 20, 2024 15:32

mariajgrimaldi mentioned this pull request Sep 19, 2024

refactor!: turn on homepage course API V2 consumption by default openedx/frontend-app-authoring#1307

Merged

mariajgrimaldi mentioned this pull request Dec 2, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add paginated HomePageCoursesV2 view with filtering & ordering #34173

feat: add paginated HomePageCoursesV2 view with filtering & ordering #34173

mariajgrimaldi commented Feb 2, 2024 •

edited by felipemontoya

Loading

openedx-webhooks commented Feb 2, 2024 •

edited

Loading

mariajgrimaldi commented Feb 6, 2024

mariajgrimaldi commented Feb 6, 2024 •

edited

Loading

mariajgrimaldi Feb 6, 2024 •

edited

Loading

BryanttV Feb 6, 2024

johnvente left a comment

mariajgrimaldi commented Feb 13, 2024

feanil left a comment

feanil Feb 15, 2024

mariajgrimaldi Feb 19, 2024

feanil Feb 15, 2024

mariajgrimaldi Feb 15, 2024

mariajgrimaldi Feb 19, 2024 •

edited

Loading

mariajgrimaldi Feb 19, 2024 •

edited

Loading

mariajgrimaldi Feb 19, 2024

mariajgrimaldi commented Feb 15, 2024 •

edited

Loading

mariajgrimaldi commented Feb 20, 2024 •

edited

Loading

felipemontoya left a comment

felipemontoya commented Mar 20, 2024

mariajgrimaldi commented Mar 20, 2024

openedx-webhooks commented Mar 20, 2024

edx-pipeline-bot commented Mar 20, 2024

edx-pipeline-bot commented Mar 20, 2024


		optimization_enabled = GlobalStaff().has_user(request.user) and ENABLE_GLOBAL_STAFF_OPTIMIZATION.is_enabled()

		org = request.GET.get('org', '') if optimization_enabled else None

feat: add paginated HomePageCoursesV2 view with filtering & ordering #34173

feat: add paginated HomePageCoursesV2 view with filtering & ordering #34173

Conversation

mariajgrimaldi commented Feb 2, 2024 • edited by felipemontoya Loading

Description

API changes compared to V1

Other changes

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Feb 2, 2024 • edited Loading

mariajgrimaldi commented Feb 6, 2024

mariajgrimaldi commented Feb 6, 2024 • edited Loading

Notes to reviewer

mariajgrimaldi Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Notes to reviewer

BryanttV Feb 6, 2024

Choose a reason for hiding this comment

johnvente left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Feb 13, 2024

feanil left a comment

Choose a reason for hiding this comment

feanil Feb 15, 2024

Choose a reason for hiding this comment

mariajgrimaldi Feb 19, 2024

Choose a reason for hiding this comment

feanil Feb 15, 2024

Choose a reason for hiding this comment

mariajgrimaldi Feb 15, 2024

Choose a reason for hiding this comment

mariajgrimaldi Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Feb 19, 2024

Choose a reason for hiding this comment

mariajgrimaldi commented Feb 15, 2024 • edited Loading

mariajgrimaldi commented Feb 20, 2024 • edited Loading

Some notes about performance

felipemontoya left a comment

Choose a reason for hiding this comment

felipemontoya commented Mar 20, 2024

mariajgrimaldi commented Mar 20, 2024

openedx-webhooks commented Mar 20, 2024

edx-pipeline-bot commented Mar 20, 2024

edx-pipeline-bot commented Mar 20, 2024

mariajgrimaldi commented Feb 2, 2024 •

edited by felipemontoya

Loading

openedx-webhooks commented Feb 2, 2024 •

edited

Loading

mariajgrimaldi commented Feb 6, 2024 •

edited

Loading

mariajgrimaldi Feb 6, 2024 •

edited

Loading

mariajgrimaldi Feb 19, 2024 •

edited

Loading

mariajgrimaldi Feb 19, 2024 •

edited

Loading

mariajgrimaldi commented Feb 15, 2024 •

edited

Loading

mariajgrimaldi commented Feb 20, 2024 •

edited

Loading