-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add taxonomy api pagination #69
Conversation
df959bf
to
1d3d0e6
Compare
There was a problem hiding this 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
Logged in via admin, created a taxonomy, and visited the taxonomies list API to check that pagination works.
❯ python3 manage.py migrate ❯ python3 manage.py createsuperuser ❯ python3 manage.py runserver
- I read through the code and checked the tests.
-
I checked for accessibility issuesN/A - Includes documentation
-
I made sure any change in configuration variables is reflected in the correspondingN/A
client'sconfiguration-secure
repository.
3b089f5
to
7e5d7f4
Compare
7e5d7f4
to
dd1b2de
Compare
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:
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. |
@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). |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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):
openedx-learning/projects/urls.py
Line 11 in fb1fd08
path("rest_api/", include("openedx_learning.rest_api.urls")), |
If this is fine, we can set a default for all APIs.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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!
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. |
Yup, I just meant "please just make the one change about using the existing pagination class, otherwise I think this looks great" |
@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. |
This PR add pagination to the Taxonomy API