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: add taxonomy api pagination #69

Merged

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Aug 2, 2023

This PR add pagination to the Taxonomy API

@rpenido rpenido marked this pull request as ready for review August 2, 2023 14:17
@rpenido rpenido force-pushed the rpenido/taxonomy-api-add-pagination branch from df959bf to 1d3d0e6 Compare August 2, 2023 14:27
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.

👍 Thank you for this quick fix @rpenido !

Couple of small nits, but this should be good to merge when they're addressed.

  • I tested this by running the dev server
    ❯ python3 manage.py migrate
    ❯ python3 manage.py createsuperuser
    ❯ python3 manage.py runserver
    
    Logged in via admin, created a taxonomy, and visited the taxonomies list API to check that pagination works.
  • I read through the code and checked the tests.
  • I checked for accessibility issues N/A
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding
    client's configuration-secure repository.
    N/A

tests/openedx_tagging/core/tagging/test_views.py Outdated Show resolved Hide resolved
openedx_tagging/core/tagging/rest_api/v1/views.py Outdated Show resolved Hide resolved
@rpenido rpenido force-pushed the rpenido/taxonomy-api-add-pagination branch from 3b089f5 to 7e5d7f4 Compare August 3, 2023 12:05
@rpenido rpenido force-pushed the rpenido/taxonomy-api-add-pagination branch from 7e5d7f4 to dd1b2de Compare August 3, 2023 12:07
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 3, 2023
@openedx-webhooks
Copy link

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

@pomegranited
Copy link
Contributor

pomegranited commented Aug 6, 2023

@bradenmacdonald or @ormsbee Could you give this a look when you get a chance? It should be a small change, which I'm hoping isn't controversial. (cf internal OpenCraft link: FAL-3450).

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

LGTM, just one change please.

class TaggingPagination(PageNumberPagination):
page_size_query_param = "page_size"
page_size = 100
max_page_size = 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you should just use edx_rest_framework_extensions.paginators.DefaultPagination instead.

The platform sets it as a global default for all APIs - maybe we can do so here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bradenmacdonald!

I believe you should just use edx_rest_framework_extensions.paginators.DefaultPagination instead.

In a discussion with @pomegranited open-craft#4 (comment), we talked that we shouldn't import edx-platform here. I think I misunderstood it and avoided importing other apps from edx in general. If this is not a problem, we should use this default pagination class.

The platform sets it as a global default for all APIs - maybe we can do so here too?

I think we can. Note that we have another rest_api in this app that will also be affected (not implemented, thou):

path("rest_api/", include("openedx_learning.rest_api.urls")),

If this is fine, we can set a default for all APIs.

Copy link
Contributor

@pomegranited pomegranited Aug 9, 2023

Choose a reason for hiding this comment

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

Awesome, thanks for that recommendation @bradenmacdonald , I didn't know that existed.

@rpenido Yes please add this dependency and use edx_rest_framework_extensions.paginators.DefaultPagination as our default for this library instead of creating our own class here.

It's ok to just use it directly, and keep its page_size and max_page_size too -- we can adjust these later if we need them in the Tagging UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pomegranited I think you pinged the wrong person ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! Sorry for the noise @robrap!

@rpenido
Copy link
Contributor Author

rpenido commented Aug 9, 2023

LGTM, just one change please.

Thanks! Should I revert the style commit?

@pomegranited
Copy link
Contributor

@rpenido

LGTM, just one change please.
Thanks! Should I revert the style commit?

I don't think that's what @bradenmacdonald meant by this.. it's ok to keep your style change.

@bradenmacdonald
Copy link
Contributor

Yup, I just meant "please just make the one change about using the existing pagination class, otherwise I think this looks great"

@pomegranited pomegranited merged commit 1992850 into openedx:main Aug 14, 2023
6 checks passed
@openedx-webhooks
Copy link

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

@pomegranited pomegranited deleted the rpenido/taxonomy-api-add-pagination branch August 14, 2023 00:19
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.

None yet

4 participants