Skip to content
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: allow page length 2500 #25062

Merged
merged 1 commit into from Mar 15, 2024
Merged

fix: allow page length 2500 #25062

merged 1 commit into from Mar 15, 2024

Conversation

barredterra
Copy link
Collaborator

@barredterra barredterra commented Feb 26, 2024

Multiple of our customers have recently expressed their struggle with loading many records: select page length 500, then klick load more, scroll down, load more, scroll down, ...

A long page length is useful for reports, when your filters result in a couple thousand records and you need to load and check (or print) all rows.

Why 2500? We always have 5x increments: 20, 100, 500, 2500

On my development site, the request take 115 ms for the server to process and about 4 seconds for the browser to render. This seems acceptable.

@barredterra barredterra requested a review from a team as a code owner February 26, 2024 12:58
@barredterra barredterra requested review from surajshetty3416 and removed request for a team February 26, 2024 12:58
@barredterra barredterra added backport version-14-hotfix backport to version 14 backport version-15-hotfix Backport the PR to v15 labels Feb 26, 2024
@ankush
Copy link
Member

ankush commented Feb 26, 2024

what do people do with 2500 records? 😳

Isn't filtering/searching far more useful for narrowing down? 2500 honestly is absurdly high page length 😅

@ankush
Copy link
Member

ankush commented Feb 26, 2024

A better option would be editable page length or 1000 maybe. 2500 is odd and a huge number 😅 People will load that many docs and wonder why everything is so slow. List view isn't quite designed to handle such high amount of elements.

@barredterra
Copy link
Collaborator Author

barredterra commented Feb 26, 2024

We got this request independently from two customers today. I guess clicking "show all" is just easier than coming up with adequate filtering or grouping strategies. Also, how to print a report with 5000 rows?

editable page length

people will just enter 9999

1000

Breaks the pattern of 5x and isn't much better than 500

List view isn't quite designed to handle such high amount of elements.

We can work on that :D

BTW, the original request was a "load all" button, which we objected. But I think 2500 is justifiable.

@ankush
Copy link
Member

ankush commented Feb 26, 2024

A better pattern for doing operations on everything matching filters is this:

image

Load All coupled with realtime refresh can be dangerous on DB with large tables 😅

@barredterra
Copy link
Collaborator Author

barredterra commented Feb 26, 2024

A better pattern for doing operations on everything matching filters is this

I don't see how this helps for use cases like:

  • make report builder show all, say, 4200 rows with a correct totals row
  • print a report (from report builder) with all, say, 4200 rows

Load All coupled with realtime refresh can be dangerous on DB with large tables

I pushed some fixes and UX improvements today that should avoid reloading all data or rebuilding the entire list HTML in some cases (just low hanging fruit for now. We can probably do more, if required):

Edit: Ankush also did some performance improvements:

@barredterra
Copy link
Collaborator Author

@ankush do you see any more bottlenecks we should tackle before this is good to go?

@ankush
Copy link
Member

ankush commented Mar 15, 2024

The only blocker with "apply action to all matching doc" is printing right? because prints are generated using client side HTML?

I really think we should attempt doing that server side 😅

Printing large reports also just doesn't work from UI. 😬 You'll hit max request size limit soon enough.

@ankush
Copy link
Member

ankush commented Mar 15, 2024

Merging for now.

@ankush ankush merged commit 21cc09e into frappe:develop Mar 15, 2024
25 checks passed
@ankush
Copy link
Member

ankush commented Mar 15, 2024

Can you list all operations that aren't possible right now? Apart from printing.

mergify bot pushed a commit that referenced this pull request Mar 15, 2024
mergify bot pushed a commit that referenced this pull request Mar 15, 2024
@barredterra barredterra deleted the page-length branch March 15, 2024 15:44
@barredterra
Copy link
Collaborator Author

barredterra commented Mar 15, 2024

Features that currently require loading all rows:

  • Showing correct values in the Total row of report builder

    Sums up the loaded data only. The difference is not obvious to users. Could also be solved by computing the total values in the backend.

  • Using the columns' search fields in the report builder

    Searches through loaded data only. The difference is not obvious to users. Could also be solved by disabling this feature / allowing query filters only.

  • Printing a report with all rows (via report builder)

    Could also be solved by printing in the backend. This would depend on the above point (disabling the columns' search fields, since they wouldn't get applied in the backend).

  • Bulk-editing all rows

    Could better be solved via a "select all n rows" button

ankush added a commit that referenced this pull request Mar 19, 2024
…-25062

fix: allow page length 2500 (backport #25062)
ankush added a commit that referenced this pull request Mar 19, 2024
…-25062

fix: allow page length 2500 (backport #25062)
frappe-pr-bot pushed a commit that referenced this pull request Mar 27, 2024
# [14.69.0](v14.68.2...v14.69.0) (2024-03-27)

### Bug Fixes

* allow page length 2500 ([#25062](#25062)) ([d40a169](d40a169))
* **Contact form:** make email translatable ([1a794df](1a794df))
* **Contact form:** make title and options translatable ([6d731fa](6d731fa))
* **Contact form:** translate internal notification to system language ([2b2cf83](2b2cf83))
* DB Query distinct handling with full table name ([#25594](#25594)) ([#25596](#25596)) ([d201326](d201326))
* diff after converting to html to text ([#25582](#25582)) ([#25583](#25583)) ([68d6947](68d6947))
* don't parse CSV params for Excel ([87c6fc6](87c6fc6))
* duplicate translation of field labels ([471ad26](471ad26))
* fieldname referenced before assignment ([95a0db8](95a0db8))
* **lint:** v14 doesn't have typed doctype classes, so this was never needed ([#25578](#25578)) ([e343a32](e343a32))
* Mark totals row correctly for print ([#25629](#25629)) ([#25630](#25630)) ([a902f6d](a902f6d))
* only add title field in search if it exists ([#25634](#25634)) ([#25635](#25635)) ([446c4d6](446c4d6))
* pop from form_params ([377b35c](377b35c))
* ruff fixes ([8ecc7d9](8ecc7d9))
* test reportview ([bb5c3a1](bb5c3a1))

### Features

* add csv preview ([d89a126](d89a126))
* add test for exporting reportview as CSV ([4eb31e0](4eb31e0))
* add translation context ([88cfe4b](88cfe4b))
* allow setting a custom rate limit for `login via email link` feature ([b7a1da5](b7a1da5))
* connect to redis sentinel for redis queue (backport [#25506](#25506)) ([#25556](#25556)) ([a9ee773](a9ee773))
* CSV params for query report ([7493c4e](7493c4e))
* CSV params for report view ([9f14d65](9f14d65))
* german translations for export dialog ([eab535d](eab535d))
* hide csv settings in collapsible section ([efb70be](efb70be))
* preview real data ([91b20b8](91b20b8))

### Performance Improvements

* Faster "show title in link field" on list view ([#25597](#25597)) ([#25621](#25621)) ([7df36f6](7df36f6))
* remove useless sorting on docstatus ([#25571](#25571)) ([#25590](#25590)) ([5e65cc9](5e65cc9))

### Reverts

* Revert "fix: escape text types before setting disp area (#25520) (#25522)" (#25604) ([0bf0cb8](0bf0cb8)), closes [#25520](#25520) [#25522](#25522) [#25604](#25604)
frappe-pr-bot pushed a commit that referenced this pull request Mar 27, 2024
# [15.19.0](v15.18.2...v15.19.0) (2024-03-27)

### Bug Fixes

* add missing arg while invoking _download_multi_pdf ([f82ed9f](f82ed9f))
* allow page length 2500 ([#25062](#25062)) ([75b1f6c](75b1f6c))
* **Contact form:** make email translatable ([800cfee](800cfee))
* **Contact form:** make title and options translatable ([b1dbd79](b1dbd79))
* **Contact form:** translate internal notification to system language ([f67e5f1](f67e5f1))
* DB Query distinct handling with full table name ([#25594](#25594)) ([4fe0498](4fe0498))
* diff after converting to html to text ([#25582](#25582)) ([#25584](#25584)) ([bd7372d](bd7372d))
* Mark totals row correctly for print ([#25629](#25629)) ([#25631](#25631)) ([725018f](725018f))
* only add title field in search if it exists ([#25634](#25634)) ([#25636](#25636)) ([b2260c1](b2260c1))
* redirect after login, todo filters (backport [#25521](#25521)) ([#25561](#25561)) ([d9f9aa4](d9f9aa4)), closes [#25455](#25455) [#25455](#25455)
* respect `null` as number value ([#25639](#25639)) ([5b64ac9](5b64ac9))
* set list row height to 40px ([#25619](#25619)) ([#25625](#25625)) ([af406f9](af406f9))
* **test:** update RQ serialization test ([c764282](c764282))
* Translate form and workflow builder (backport [#25482](#25482)) ([#25547](#25547)) ([01f469f](01f469f))
* **webhook:** `r` is referenced here before its initialized ([a60bb4e](a60bb4e)), closes [#21064](#21064)
* **workspace:** Don't allow Welcome as default workspace ([819a24c](819a24c))

### Features

* allow setting a custom rate limit for `login via email link` feature ([c7af627](c7af627))
* connect to redis sentinel for redis queue ([#25557](#25557)) ([82a33c3](82a33c3))
* drop sentry's RQ integration ([1eab176](1eab176))
* **workspace:** Allow user to choose a default workspace ([bfa1a83](bfa1a83))

### Performance Improvements

* Faster "show title in link field" on list view ([#25597](#25597)) ([#25622](#25622)) ([f62a30c](f62a30c))
* remove useless sorting on docstatus ([#25571](#25571)) ([#25591](#25591)) ([5e5853e](5e5853e))

### Reverts

* Revert "fix: escape text types before setting disp area (#25520) (#25523)" (#25603) ([bbf55df](bbf55df)), closes [#25520](#25520) [#25523](#25523) [#25603](#25603)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 backport version-15-hotfix Backport the PR to v15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants