Skip to content

Fix for pagination ‘last’ link to give the real last page #353

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

Merged
merged 3 commits into from
May 28, 2017
Merged

Fix for pagination ‘last’ link to give the real last page #353

merged 3 commits into from
May 28, 2017

Conversation

yanivp
Copy link
Contributor

@yanivp yanivp commented May 24, 2017

We're working on a project using django-json-api and came across unintuitive behavior for the pagination. The current pagination implementation for the 'last' link is basically a 'total count' - 'limit'. Consider the following example:

Objects = [0-21]
Limit = 10
(first page - offset 0)
0,1,2,3,4,5,6,7,8,9

(clicking 'next' - offset 10)
10,11,12,13,14,15,16,17,18,19

(clicking 'next' - offset 20)
20,21

(clicking 'last' - offset 12)
12,13,14,15,16,17,18,19,20,21

This behavior would make the user completely disoriented in the paging. It makes more sense that 'last' will offset to 20 just the same as clicking 'next'->'next'->'next' and simply return two objects instead of 10.

@mblayman
Copy link
Collaborator

Hi, @yanivp. Thanks for your contribution! It looks like you're a first time contributor so I'm excited that you took the time to help.

The PR, in its current state, can't be merged because the test suite is reporting failure (you can use the error at https://travis-ci.org/django-json-api/django-rest-framework-json-api/builds/235614695?utm_source=github_status&utm_medium=notification). Either the fix is not quite correct and the tests are saving us from making a mistake, or the test is not correct. Could you please look into what the issue is?

Also, please add your name to the AUTHORS file to give you some credit for your contribution. The listing is alphabetical by first name.

Let me know if you have any questions. Thanks again!

@yanivp
Copy link
Contributor Author

yanivp commented May 28, 2017

Hey @mblayman
You're right I forgot to run the tests, fixed and also added myself to AUTHORS

@codecov-io
Copy link

codecov-io commented May 28, 2017

Codecov Report

Merging #353 into develop will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #353      +/-   ##
===========================================
- Coverage    77.09%   77.06%   -0.03%     
===========================================
  Files           50       50              
  Lines         6026     6027       +1     
===========================================
- Hits          4646     4645       -1     
- Misses        1380     1382       +2
Impacted Files Coverage Δ
rest_framework_json_api/pagination.py 54.81% <100%> (ø) ⬆️
example/tests/unit/test_pagination.py 100% <100%> (ø) ⬆️
example/models.py 66.95% <0%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a12d98d...67f5ec3. Read the comment docs.

@mblayman
Copy link
Collaborator

👍 Good stuff. Thank you!

@mblayman mblayman merged commit ab4e1bd into django-json-api:develop May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants