-
Notifications
You must be signed in to change notification settings - Fork 299
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
Fix for pagination ‘last’ link to give the real last page #353
Conversation
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!
Hey @mblayman
You're right I forgot to run the tests, fixed and also added myself to AUTHORS
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
👍 Good stuff. Thank you!
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.