Prevent out of bounds pages from returning a next link. #2171
Conversation
bcaplan
force-pushed
the
fix-next-links-for-out-of-bounds-pages
branch
from
87e47c7
to
de6c30c
Compare
bcaplan
force-pushed
the
fix-next-links-for-out-of-bounds-pages
branch
from
a748e56
to
36b7960
Compare
|
@bf4 Is there anything more I can do to help get this merged? I think tests were failing on the original branch I forked which may have prevented this from getting looked at. This is now fixed since I just pulled the upstream changes. |
|
@bf4 Would you be willing to provide any info on why this was closed and not merged? This was my first attempt at contributing to open source software, so the information would be much appreciated. |
|
@bcaplan Not sure how I might have closed it |
|
@bf4 Haha, no worries. Thanks for reopening. |
|
This looks fine to me. Does the spec comment on this scenario? I don't recall. hashtag 'lazy' |
|
@bf4 I named them |
Sorry, I meant http://jsonapi.org/format/#fetching-pagination
So, nothing about out of bounds. |
| url_for_page(collection.total_pages) | ||
| else | ||
| url_for_page(collection.current_page - FIRST_PAGE) | ||
| end |
There was a problem hiding this comment.
or
- url_for_page(collection.current_page - FIRST_PAGE)
+ page_number = collection.current_page > collection.total_pages ? collection.total_pages : collection.current_page - FIRST_PAGE
+ url_for_page(page_number)So, prev is the last page if current page is out of bounds,
| end | ||
|
|
||
| def next_page_url | ||
| return nil if collection.total_pages == 0 || collection.current_page == collection.total_pages | ||
| return nil if collection.total_pages == 0 || collection.current_page >= collection.total_pages | ||
| url_for_page(collection.next_page) |
There was a problem hiding this comment.
So no next page if the current page is the last page or out of bounds.
There was a problem hiding this comment.
That's correct. This way if you request out of bounds and rely on the next link, you don't paginate forever.
| @@ -174,6 +193,18 @@ def test_last_page_pagination_links_using_will_paginate | |||
| assert_equal expected_response_with_last_page_pagination_links, adapter.serializable_hash | |||
| end | |||
|
|
|||
| def test_out_of_bounds_page_pagination_links_using_kaminari | |||
| adapter = load_adapter(using_kaminari(4), mock_request) | |||
There was a problem hiding this comment.
might be good to replace 4 with out_of_bounds_page_number = 4 here and below
|
Ah, I see what you meant now. Yeah, it doesn't mention this specific circumstance. I decided when out of bounds: omitting the next link, providing the last page as the previous link, and providing a current link even though you are out of bounds seemed to best conform to my reading of the spirit of the spec. |
bcaplan
force-pushed
the
fix-next-links-for-out-of-bounds-pages
branch
from
36b7960
to
c951acc
Compare
Also makes sure the prev link is a real page. Self link is unmodified, it will return a link to the current page even if it doesn't exist. Seemed to best conform to JSON:API spec.
bcaplan
force-pushed
the
fix-next-links-for-out-of-bounds-pages
branch
from
c951acc
to
b30704e
Compare
|
It'd be good to see this merged. Is there anything I can do to help move things along? |
Purpose
In the JSON:API adapter, this will prevent
nextlink URLs from being generated for nonexistent pages if the page requested is out of bounds.Changes
Now checking
current_pageagainsttotal_pagesusing>=instead of==.This also makes sure the
prevlink is a real page.The
selflink is unmodified, it will return a link to the current page even if it doesn't exist. Seemed to best conform to the JSON:API spec.Caveats
Related GitHub issues
Additional helpful information