-
Notifications
You must be signed in to change notification settings - Fork 408
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
allow requesting a page that is out of range to return empty #68
Comments
Hi Michael,
From the Pagy class point of view, that is an error that cannot and should
not be handled by the class itself: the exception is the way to delegate it
to some other application layer, typically the UI.
I agree that an exception for the final user may be confusing, but catching
it and handling it in some appropriate way for the app is up to the
developers. In the doc there is an example about serving the last page, but
it is really simple to do anything else.
It's not clear to me what you are suggesting it should happen instead.
Could you provide some more details? For example what do you mean with
"make empty the default" ?
Thanks
…On Mon, Jul 9, 2018, 8:32 PM Michael Grosser ***@***.***> wrote:
atm it raises OutOfRangeError which is confusing for users when they click
on the last page, but some item got deleted or apis consumers that iterate
until they find an empty result
so either of these:
- make empty the default since that's what most expect
- add optional empty_when_out_of_range: true or so
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#68>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGJcVB3JixqMQW8cFWtCUDP-C4OVxboks5uE6GegaJpZM4VILxc>
.
|
Using this atm: # https://github.com/ddnexus/pagy/issues/68
Pagy::Backend.prepend(Module.new do
private
def pagy(*args)
super
rescue Pagy::OutOfRangeError => e
e.pagy.instance_variable_set(:@page, e.pagy.last)
[e.pagy, []]
end
end) which makes it behave like it's on the actual last page but has no items |
I understand it now :). However your solution looks wrong to me: you are serving the last page that has items at the time of the request (or it wouldn't be the last page), but you are hiding them, hence providing a wrong inconsistent response. The right and consistent thing to do is redirecting to the actual last page, eventually providing a feedback to the user, but showing the items as they are. The last page is the last valid page at the time of the request/error. I don't see any problem here. |
And if you're paginating with an API, you should return the error and the last page number so giving a feedback usable for the next request. |
- serving anything higher than the last page results in rendering errors,
so rendering last page which looks ok in the UI
- redirect might also work, but it means an extra request and potentially an
endless loop if things go wrong
- our api consumers expect 200 + empty reply ... fairly common afaik,
never saw anyone return 404
…On Mon, Jul 9, 2018 at 12:33 PM Domizio Demichelis ***@***.***> wrote:
And if you're paginating with an API, you should return the error and the
last page number so giving feedback usable for the next request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#68 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZz9XiiRjOBjHm9oZ2ayebPQuUtieks5uE6_-gaJpZM4VILxc>
.
|
Rendering an empty last page while it has items seems the worse solution to me. An API rendering an exception with the reason and feedback to get the next request in the correct range seems pretty correct to me. I didn't get the example of the 200 empty reply thought... |
lots of clients do pagination by requesting next page until it is empty, returning a 404 would break that behavior |
it's not telling the user what page it is, just showing empty content |
BTW, a default empty last page is not a possible solution, not only because it would be wrong as explained above, but it would obliterate the freedom to rescue and do something else. It is so simple to rescue - even to a wrong last empty page if you really want that - that adding an option doesn't seem so useful to me at this point. Easier than that... I only see an extra that could be explicitly required... but only if it would provide a solution consistent with the data, avoiding empty pages that are not really empty. |
Which is a quite questionable behavior :)... but I understand that the problem is that you have to create a pagy instance that works, hence you add the last page... Which is inconsistent but works because stops API clients with bad behaviors :) I understand it and it may be the best way to do it in your context, but don't feel like that should be in the pagy code. Any alternative solution? |
I guess a `Pagy.empty` would be nice but I can create that myself too ...
some docs on how to do the `return empty` would be nice
since I assume it's a common usecase especially when trying to migrate to
pagy without changing user experience anything
…On Mon, Jul 9, 2018 at 1:31 PM Domizio Demichelis ***@***.***> wrote:
returning a 404 would break that behavior
Which is a quite questionable behavior :)... but I understand that the
problem is that you have to create a pagy instance that works, hence you
add the last page... Which is inconsistent but works because stops API
clients with bad behaviors :)
I understand it and it may be the best way to do it in your context, but
don't feel like that should be in the pagy code.
Any alternative solution?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#68 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ0AhpZUGdhi9pJsUDO8DQVG3fxtaks5uE72ugaJpZM4VILxc>
.
|
Both good suggestions. Thanks! How |
just a pagy for the last page I guess ... otherwise it might to weird
pagination displays and index errors inside of pagy
…On Mon, Jul 9, 2018 at 1:44 PM Domizio Demichelis ***@***.***> wrote:
Both good suggestions. Thanks! What <Pagy.empty should look like in order
to work?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#68 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ_d8aTOntUgExFfbDAT4lpn7kpIJks5uE8COgaJpZM4VILxc>
.
|
I think we are mixing up 2 different cases: UI and API. With an UI you actually have to show something to the user, and an empty page would also likely have the page number in the With an API you may avoid the problem by not even showing the page number, but what abut the prev and next headers? You should have a pretty lame API with no pagination headers nor page number, nor feedback in the response in order to avoid the inconsistency problem. I don't think that neither cases should be officially supported by Pagy. I am trying to find a Pagy instance that could solve both problems but I cannot. |
yeah it's a weird area, I don't have a good universal solution :)
…On Tue, Jul 10, 2018 at 10:47 AM Domizio Demichelis < ***@***.***> wrote:
it's not telling the user what page it is, just showing empty content
last page I guess ...otherwise it might to weird pagination displays
I think we are mixing up 2 different cases: UI and API.
With an UI you actually have to show something to the user, and an empty
page would also likely have the page number in the pagy_info, pagy_nav
and URL: that raises the consistency problem with the last page wrongly
empty.
With an API you may avoid the problem by not even showing the page number,
but what abut the prev and next headers? You should have a pretty lame API
with no pagination headers nor page number, nor feedback in the response in
order to avoid the inconsistency problem.
I don't think that neither cases should be officially supported by Pagy. I
am trying to find a Pagy instance that could solve both problems but I
cannot.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#68 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ-kz-0xZ8xeMn0nS29xDmXWOqGxAks5uFOi0gaJpZM4VILxc>
.
|
Maybe you could write a brief paragraph in the how to about how to handle it? |
Thanks for the PR... |
@grosser |
Co-authored-by: Michael Grosser <michael@grosser.it>
@grosser now it should be less hacky :). |
atm it raises OutOfRangeError which is confusing for users when they click on the last page, but some item got deleted or apis consumers that iterate until they find an empty result
also impractical since now I cannot render the page but need to make up a fake pagy or redirect to a page that I don't know wether it exists (how do I get the last page valid ?)
so either of these:
empty_when_out_of_range: true
or soThe text was updated successfully, but these errors were encountered: