Always include self, first, last pagination links #2149
Conversation
mecampbellsoup
force-pushed
the
more-pagination-links-by-default
branch
from
1316d69 to
7387266
Compare
|
No CI showing up here, except for appveyor (which stands as |
|
@kurko Travis showing up now. |
|
So, this looks good to me and I intend to merge it because it makes sense. The spec doesn't really say we should hide all links when there's only one page, but only the ones that are unavailable. |
| pages[:next] = collection.current_page + FIRST_PAGE | ||
| end | ||
| else | ||
| pages[:last] = FIRST_PAGE | ||
| end | ||
| end |
There was a problem hiding this comment.
sure, though it might be nice to refactor to something like
def as_json
{
"self": location_url,
"first": first_page_url,
"prev": prev_page_url,
"next": next_page_url,
"last": last_page_url
}
end
def location_url
url_for_page(collection.current_page)
end
def first_page_url
url_for_page(1)
end
def prev_page_url
return nil if collection.first_page?
url_for_page(collection.prev_page)
end
def next_page_url
return nil if collection.last_page? || collection.out_of_range?
url_for_page(collection.next_page)
end
def last_page_url
url_for_page(collection.total_pages)
end
def url_for_page(number)
params = query_parameters.dup
params[:page] = { page: per_page, number: number }
context.url_for(action: :index, params: params)
end
def per_page
@per_page ||= collection.try(:per_page) || collection.try(:limit_value) || collection.size
end
There was a problem hiding this comment.
It's not really a refactor here because there's change in behavior, (we are omitting next prev when it's not available vs just returning null), but I agree that we can improve this a bit.
There was a problem hiding this comment.
ah
Keys MUST either be omitted or have a null value to indicate that a particular link is unavailable.
I like the 'always be there' ¯\_(ツ)_/¯
There was a problem hiding this comment.
Seems like a matter of preference since missing key vs. present key with null value should produce same behavior for clients. We do need to pick one or the other, though... 😦
There was a problem hiding this comment.
@bf4 in your code snippet, why do you think context responds to url_for?
context.url_for(action: :index, params: params)
[11] pry(#<ActiveModelSerializers::Adapter::JsonApi::PaginationLinks>)> context.url_for(action: :index, params: params)
NoMethodError: undefined method `url_for' for #<ActiveModelSerializers::SerializationContext:0x007ff3af0e2338>
from (pry):11:in `url_for_page'
I did notice that url_helpers seem to be available as an instance variable, however:
[7] pry(#<ActiveModelSerializers::Adapter::JsonApi::PaginationLinks>)> context.url_helpers
NoMethodError: undefined method `url_helpers' for #<ActiveModelSerializers::SerializationContext:0x007ff3af0e2338>
from (pry):7:in `url_for_page'
[8] pry(#<ActiveModelSerializers::Adapter::JsonApi::PaginationLinks>)> context.instance_variable_get :@url_helpers
=> #<Module:0x007ff3aedee488>
[9] pry(#<ActiveModelSerializers::Adapter::JsonApi::PaginationLinks>)> _.lm
=> [:url_options, :url_for, :_routes, :optimize_routes_generation?, :included, :append_features, :class_methods]
| "prev": prev_page_url, | ||
| "next": next_page_url, | ||
| "last": last_page_url | ||
| } |
There was a problem hiding this comment.
I think this looks much nicer :) probably should be self: vs. "self":.. not sure why I used quotes... not asking for a change.. just thinking aloud
There was a problem hiding this comment.
actually, looks like this is causing failures... I hadn't check CI yet.
|
warnings via CI |
They will be converted to strings when rendered as JSON.
Found a bug in jsonapi adapter. Basically, when there’s one page, all links are omitted. The specs says
It doesn’t say “remove pagination entirely”. A book with one page still has a first/current/last page. It doesn’t contain
nextorprev.