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

Pagination unit tests #742

Open
1 task
AetherUnbound opened this issue Dec 14, 2021 · 3 comments
Open
1 task

Pagination unit tests #742

AetherUnbound opened this issue Dec 14, 2021 · 3 comments
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API 🔧 tech: django Involves Django 🐍 tech: python Involves Python

Comments

@AetherUnbound
Copy link
Collaborator

Current Situation

A bug that was recently fixed in WordPress/openverse-api#398 involved the pagination.py utilities, which currently don't appear to have tests.

Suggested Improvement

It would be great to add unit tests for this file!

Benefit

Additional context

Implementation

  • 🙋 I would be interested in implementing this feature.
@AetherUnbound AetherUnbound transferred this issue from WordPress/openverse-catalog Dec 14, 2021
@AetherUnbound AetherUnbound added good first issue New-contributor friendly help wanted Open to participation from the community 🐍 tech: python Involves Python 💻 aspect: code Concerns the software code in the repository 🔧 tech: django Involves Django 🟩 priority: low Low priority and doesn't need to be rushed 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Feb 2, 2022
@prikshit-1103
Copy link

prikshit-1103 commented Sep 10, 2022

hi @dhruvkb @AetherUnbound can i work on this issue, i am comfortable in python and Django and i think I will be able to handle this, can you please assign this to me.

@dhruvkb
Copy link
Member

dhruvkb commented Sep 10, 2022

Thanks @prikshit-1103, feel free to take this up! Assigning to you.

@sarayourfriend
Copy link
Collaborator

Rather than unit tests, I'd suggest adding pagination to the integration test suite. It's more important to test the pagination utilities as they are used by the paginated views, and testing it in isolation could easily miss some of the finer points (like the fact that page_count and results count are technically initialised late in the request cycle by the view, just before the response is made, rather than by the actual pagination utility.

Writing this, it occurs to me that we're not using the pagination at all the way its intended. It's meant to automatically handle pagination of query sets, but we don't use a query set in these routes. We might be better off writing an extention to serializers.ListSerializer that encodes pagination metadata into the response (and nests the results into the results key), rather than hacking around the DRF pagination utilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users good first issue New-contributor friendly help wanted Open to participation from the community 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API 🔧 tech: django Involves Django 🐍 tech: python Involves Python
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

5 participants