Should 0.6.3 revert CSV pagination, going back to the 10k limit?

[seanlinsley]

a8ad4f9 was introduced in 0.6.1 as a naive attempt to make AA better by intentionally causing everyone pain under the assumption that pain motivates. In reality, everyone who relied on the old behavior just monkey-patched around it.

This was a mistake on my part, and I sincerely apologize to every developer who had to figure out for themselves why CSV export was no longer working as expected, and also to every user who had a core part of their workflow broken for no good reason.

We haven't really been following Semantic Versioning, but I think we should start. Master has a considerable amount of breaking changes in it, so it deserves to be 1.0.0. 0.6.2 was a bugfix release, and I want 0.6.3 to be as well (#2742).

The question here is: how should we fix this in 0.6.3? Simply revert a8ad4f9? @Fivell's #2810 / #2821 certainly makes pagination more easily customizable, but it doesn't actually solve the problem at hand. I'd appreciate everyone's thoughts on this.

[seanlinsley]

CC-ing everyone from #2449: @joallard, @gsilis, @bkno, @lauren, @stereoscott, @ball-hayden, @mattolson, @abhimediratta, @pcreux

As well as @kim3er from a8ad4f9#commitcomment-4524305

[joallard]

Props on trying to adopt Semver, and yes, I think in the spirit of it, you should revert it. As a user, I wouldn't expect 0.6.1->0.6.3 to introduce that change, but 0.7.0/1.0.0, yes.

[kim3er]

I'd prefer to simply revert.

[abhimediratta]

+1 for reverting the feature
On 06-Jan-2014 9:09 PM, "Sean Linsley" notifications@github.com wrote:

a8ad4f9a8ad4f989caec3a9ffbc820a949f309d2ce1a832was introduced in 0.6.1 as a naive attempt to make AA better by
intentionally causing everyone pain under the assumption that pain
motivates. In reality, everyone who relied on the old behavior just
monkey-patched around it.

This was a mistake on my part, and I sincerely apologize to every
developer who had to figure out for themselves why CSV export was no longer
working as expected, and also to every user who had a core part of their
workflow broken for no good reason.

We haven't really been following Semantic Versioning, but I think we
should start. Master has a considerable amount of breaking changes in it,
so it deserves to be 1.0.0. 0.6.2 was a bugfix release, and I want 0.6.3 to
be as well (#2742 #2742).

The question here is: how should we fix this in 0.6.3? Simply revert
a8ad4f9a8ad4f9?
@Fivell https://github.com/Fivell's #2810#2810
#2821 #2821 certainly
makes pagination more easily customizable, but it doesn't actually solve
the problem at hand. I'd appreciate everyone's thoughts on this.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2847
.

[Fivell]

+1 for reverting (#2821 is not about fixing CSV pagination, just allows to change max_per_page dynamically )

[lauren]

I'd also recommend just reverting the feature and perhaps noting next to the download link that only 10K records will be available for those with very large data sets.

[kim3er]

I like the idea of stating the limit in the UI, promoting it as a feature perhaps.

[gja]

A reference for the aforementioned monkey patch

Add the following to app/admin/fix_csv_pagination.rb

# Workaround for until https://github.com/gregbell/active_admin/issues/2847 is resolved
module ActiveAdmin
  class ResourceController < BaseController
    module DataAccess
      alias original_apply_pagination apply_pagination
      def apply_pagination(chain)
        request.format == 'text/csv' ? chain.limit(10000) : original_apply_pagination(chain)
      end
    end
  end
end

[seanlinsley]

This has now been reverted on 0-6-stable and on master.

[rdlugosz]

Hey @seanlinsley thanks for acknowledging the problems with the approach to this change; it caused some minor headaches but at the end of the day we all found reasonable workarounds. Semver is a welcomed change for sure; looking forward to updating to 0.6.3 & pulling out my patches!

I'm going to mention #1924 here since that was a related issue & there are some good ideas there about a proper export screen. Maybe a future release can incorporate something along those lines; for now the return of 10k CSVs is most excellent!