Pagy::OverflowError when resetting filters, returning to a default which reduces the number of pages

brandondrew commented 9 months ago

Steps to Reproduce

I have an Avo page displaying employees at /tables/resources/employees. By default, I only show employees relevant to the current supervisor viewing the table. But the supervisor can loosen the filters to see all employees. If they go (for example) to the final page of all employees, and then they reset the filter, the page is not changed, and Pagy returns an error.

Here's an example URL:

http://localhost:3007/tables/resources/employees?page=1846&reset_filter=true

Expected behavior & Actual behavior

I would expect Avo to gracefully handle this. It can't display page 1846 of the employees when there are only 7 employees to show, but it can definitely show the last available page of employees (even if the last page is also the first and only page).

Instead, here's the error I see in development:

Pagy::OverflowError at /resources/employees expected :page in 1..1; got 1846

System configuration

Avo version: 2.46.0 & 2.47.0 (I upgraded to see if it fixed anything)

Rails version: 7.0.8

Ruby version: 3.3.0

License type:

[x] Community
[ ] Pro
[ ] Advanced

Are you using Avo monkey patches, overriding views or view components?

[ ] Yes. If so, please post code samples.
[x] No, none that would effect this

Screenshots or screen recordings

Impact

[ ] High impact (It makes my app un-usable.)
[x] Medium impact (Users won't notice immediately, but it will make my choice of Avo look bad to management when they do.)
[ ] Low impact (It's really a tiny thing that I could live with.)

Urgency

[ ] High urgency (I can't continue development without it.)
[x] Medium urgency (I have no current workaround, but it won't be immediately discovered.)
[ ] Low urgency (It can wait. I just wanted you to know about it.)

adrianthedev commented 9 months ago

I believe Avo 3 fixes this and a few other issues around pagination and filters. I'll let @Paul-Bob confirm it.

Are you able to upgrade to Avo 3?

Paul-Bob commented 8 months ago

Most of pagination and filters issues are fixed on Avo 3 yes.

Resetting the filters actually keep the page because when filters are loosen you get more pages that with applied filters so we kept the page setting on this situation.

But the supervisor can loosen the filters to see all employees. If they go (for example) to the final page of all employees, and then they reset the filter, the page is not changed, and Pagy returns an error.

@brandondrew Are you getting less pages when loosen the filters than with filters applied? Never thought about this scenario

brandondrew commented 8 months ago

Are you able to upgrade to Avo 3?

I was going to wait until everything was complete and code was cleaned up before trying to upgrade, so I haven't tried it yet, since it looks like there will be a lot of changes.

brandondrew commented 8 months ago

@brandondrew Are you getting less pages when loosen the filters than with filters applied? Never thought about this scenario

Yeah, this is for a major university, where some of the employees are grad-student researchers. So a professor might supervise a few people, or in extreme cases more than a hundred, but either they they normally only want to view the list of their own direct reports, so that's the default filter setting. If they remove the filters to show everyone, then that can be many hundreds of pages. If they then advance to a page that wouldn't exist for their default filters, and then reset filters, there's an error.

It's not likely to happen often but inevitably it will happen.

I think the most graceful way to handle it would be to go to the last available page.

Let's take a simple example. Suppose that there are only 1000 employees in the system, and Johan Wang supervises 20 people, and he is viewing 10 per page, so he has 2 pages of results. If he sets the filters to show everyone (which is not the default, but is available to him) then suddenly there are 100 pages. He can advance all the way to page 100 and everything works until he resets the filters back to his default. That's when there's an error. I think the best solution would be to show the last available page, so even if the query string says to show page 100, if there are only 2 pages, then showing the 2nd page makes the most sense in my opinion.

Paul-Bob commented 8 months ago

In order to keep it simple we opted for returning to page 1 when applying filters.

The described use case is not covered on avo 2 neither on avo 3.

Let's keep the same behavior, reset the pagination when reset the filters.

Paul-Bob commented 8 months ago

Thank you @brandondrew for the detailed explanation of your use case.

Paul-Bob commented 2 days ago

Approach

Reset pagination while resetting the filters.

We usually do that by including page=1 on the params

brandondrew commented 2 days ago

@Paul-Bob I don't think that's a very user-friendly approach.

Let's suppose you are looking through thousands of products trying to find a specific product—let's say it's a battery—and while you're on page 77 of 1000 pages you remember that the company that made it is Chinese, so you add a filter to show only products from China. You're definitely NOT going to want to start over from the beginning. The most logical behavior would be for the first remaining product from that page to be the first product on the new page. So it's not the earlier page number that would determine what is shown, it's the records visible before the filter change that would determine which records are visible after the filter change.

When there's a conflict between optimizing for ease of engineering and optimizing for serving the user's needs, I think serving the user's needs should come first.

avo-hq / avo