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.

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

feat: Use default pagination class for v2 lib views #34879

Conversation

yusuf-musleh
Copy link
Member

@yusuf-musleh yusuf-musleh commented May 30, 2024

Description

This PR removes the custom pagination added to the V2 Library views and uses the default one, as it already contains num_pages field. Which is needed for pagination of v2 Libraries in the course authoring MFE.

Supporting information

Related Tickets:

Testing instructions

Follow the instructions in openedx/frontend-app-authoring#1050


Private-ref: FAL-3751

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

openedx-webhooks commented May 30, 2024

Thanks for the pull request, @yusuf-musleh! 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.

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/add-num-pages-to-v2lib-paginated-resp branch 2 times, most recently from cad4475 to b7aed5c Compare June 2, 2024 20:32
@yusuf-musleh yusuf-musleh changed the title feat: Add num_pages to v2 lib paginated response feat: Use default pagination class for v2 lib views Jun 2, 2024
@yusuf-musleh yusuf-musleh marked this pull request as ready for review June 3, 2024 07:09
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 Works great @yusuf-musleh .

I have one question about whether or not we need to set a default page size value, but everything is working as described.

  • I tested this on my tutor dev stack
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation N/A
  • User-facing strings are extracted for translation N/A

openedx/core/djangoapps/content_libraries/views.py Outdated Show resolved Hide resolved
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/add-num-pages-to-v2lib-paginated-resp branch from b7aed5c to 53c8a39 Compare June 6, 2024 14:16
@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Jun 11, 2024

@yusuf-musleh @pomegranited Are we waiting for anything before we merge this? Is it backwards compatible with the code in the Library Authoring MFE?

@yusuf-musleh
Copy link
Member Author

@bradenmacdonald Its was just awaiting CC review, it should be good to go. It is backwards compatible with the library authoring mfe, I tested it out locally, the libraries show up as expected. This technically just added an extra field to the response num_pages which isn't used in the library authoring mfe when listing the libraries.

@bradenmacdonald
Copy link
Contributor

It has CC review already though :) Going to merge soon.

@yusuf-musleh
Copy link
Member Author

Oh didn't realize @pomegranited was a CC on this repo 😅

@bradenmacdonald bradenmacdonald merged commit 221e333 into openedx:master Jun 11, 2024
45 checks passed
@bradenmacdonald bradenmacdonald deleted the yusuf-musleh/add-num-pages-to-v2lib-paginated-resp branch June 11, 2024 18:15
@openedx-webhooks
Copy link

@yusuf-musleh 🎉 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
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

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