-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
make CSV downloads respect pagination
- Loading branch information
1 parent
eba8176
commit a8ad4f9
Showing
1 changed file
with
3 additions
and
8 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this happen? Is there an alternate way to exclude CSVs from pagination?
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2449
Basically, I removed the 10k limit and instead moved it back to using pagination because I didn't like that AA had such non-obvious behavior. I naively thought that the added pain would force us to build a much more robust export system, but that hasn't yet happened.
For now, a monkeypatch:
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining the reasoning and for the monkey patch. Rich
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanlinsley I'm quite surprise by this change. AA always returned the 10000 first records when exporting to CSV. I understand that it's not "obvious" but changing this behavior introduced "pain" to all the people who relies on ActiveAdmin.
I can see
max_csv_records
defined as a configuration option.While I test the export to CSV in my apps, I don't test that all the records are returned... and my client found out about this regression. No cool. 😕
Anyway, in the meantime:
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcreux sorry, this is the first OSS project I've tried to manage. As I said above, I naively thought that the added pain would expedite the creation of a more robust system, but creating undue pain for AA users was the wrong approach.
0.6.1 already broke that API. Do you think I should un-break it and release 0.6.3?
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcreux I know the holidays have effectively already begun, but I'd still appreciate your opinion on this.
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: #2847
a8ad4f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit has now been reverted on 0-6-stable and on master.