Skip to content

Conversation

@mariajgrimaldi
Copy link
Member

@mariajgrimaldi mariajgrimaldi commented Feb 2, 2024

Description

This PR builds pagination capabilities on top of the new HomePageCourses API version. These changes are intended to be used by the new Course Home MFE. The new API is based on HomePageCourses with tweaks, so it gets courses with Course Overview helpers that allow filtering, ordering, and pagination with Django/DRF methods and classes.

API changes compared to V1

Besides the changes mentioned here that add filtering/ordering capabilities, we:

  1. Used a simple paginator object before returning the courses list; this paginator only considers the list of CourseOverviews returned by the contentstore utilities.
  2. Since iterators don't have length, they can't be paginated. So we had to convert the filter(course_filter, courses_summary) iterator into a list to paginate later.
  3. We added a setting configuration for the API page size.

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

Login as an administrator or as course staff (they should both work), then:
You can use your browser to get your list of courses from Studio HTTP(s)://<CMS_HOST>/api/contentstore/v2/home/courses where the list of courses will appear paginated:
image

Deadline

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

Other information

There are still some architectural decisions to be made around how to present data to the Studio Home MFE, ie. how to split active/archived courses. So, we'll be updating this PR accordingly.

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

openedx-webhooks commented Feb 2, 2024

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.

@mariajgrimaldi mariajgrimaldi changed the title Mjg/course home api v2 pagination feat: add pagination to the HomePageCoursesV2 API Feb 2, 2024
@mariajgrimaldi mariajgrimaldi changed the title feat: add pagination to the HomePageCoursesV2 API [WIP] feat: add pagination to the HomePageCoursesV2 API Feb 2, 2024
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch 2 times, most recently from b76a4e9 to 4f15af1 Compare February 5, 2024 21:01
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from 4f15af1 to b9c7322 Compare February 5, 2024 21:20
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from e18468d to 113f4a1 Compare February 6, 2024 13:04
Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Feb 6, 2024

Choose a reason for hiding this comment

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

Note to reviewer: I'm a bit worried about the performance impact of this change, but this was one of the alternatives which included fewer changes to the current source code.

@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from 113f4a1 to 99fd94e Compare February 6, 2024 13:11
Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Feb 6, 2024

Choose a reason for hiding this comment

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

Note to reviewer: I'm unsure how to handle this case. In this view, not only courses are returned, but also returns in_process_course_actions which is a list of CourseRerunState objects not necessarily related to the courses list so I'm not sure why they're currently returned together.

So I didn't add pagination to in_process_course_actions since I believe that that list should be returned by another different view dedicated only to course reruns state objects.

What do you think? That new view is out of the scope of this PR, so this new view returns the list of course reruns objects without pagination.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we currently need this paginated list, I don't see a problem to leave it like that for the moment, and work on it later in a dedicated PR if necessary.

@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from da58387 to 13bc1d5 Compare February 6, 2024 14:05
@mariajgrimaldi mariajgrimaldi changed the title [WIP] feat: add pagination to the HomePageCoursesV2 API feat: add pagination to the HomePageCoursesV2 API Feb 6, 2024
@mariajgrimaldi mariajgrimaldi marked this pull request as ready for review February 6, 2024 14:27
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from e7aab6f to 58725d3 Compare February 6, 2024 14:27
@johnvente
Copy link

Hi there @mariajgrimaldi! Could you please add num_pages that will indicate the number of pages available? This means, for instance, if I had 20 courses and the items per page is 10, then the num_pages would be 2

@johnvente
Copy link

Hi there @mariajgrimaldi! This is working as expected. My only concern is about setting the number of items peer page but is not a blocking is a nice to have. LGTM

})
return Response(OrderedDict([
('count', paginator.page.paginator.count),
('num_pages', paginator.page.paginator.num_pages),
Copy link
Member Author

@mariajgrimaldi mariajgrimaldi Feb 15, 2024

Choose a reason for hiding this comment

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

Note to reviewer
I'm currently studying which parts of the implementation are affecting performance. Based on those results, this or the count might have to go.

@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from 581b0d8 to 9bdf9d1 Compare February 16, 2024 14:25
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/course-home-api-v2-pagination branch from 1f5c5fd to e59900e Compare March 1, 2024 19:56
@mariajgrimaldi
Copy link
Member Author

I'll merge this PR to the V2 API implementation.

@mariajgrimaldi mariajgrimaldi merged commit 8697e15 into MJG/course-home-api-v2 Mar 1, 2024
@mariajgrimaldi mariajgrimaldi deleted the MJG/course-home-api-v2-pagination branch March 1, 2024 19:57
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants