Skip to content

Commit

Permalink
make CSV downloads respect pagination
Browse files Browse the repository at this point in the history
  • Loading branch information
@seanlinsley
seanlinsley committed Aug 18, 2013
1 parent eba8176 commit a8ad4f9
Showing 1 changed file with 3 additions and 8 deletions.
11 changes: 3 additions & 8 deletions lib/active_admin/resource_controller/data_access.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,23 +267,18 @@ def current_scope
end

def apply_pagination(chain)
page_method_name = Kaminari.config.page_method_name
page = params[Kaminari.config.param_name]
page_method = Kaminari.config.page_method_name
page_param = params[Kaminari.config.param_name]

chain.send(page_method_name, page).per(per_page)
chain.send(page_method, page_param).per(per_page)
end

def per_page
return max_csv_records if request.format == 'text/csv'
return max_per_page if active_admin_config.paginate == false

@per_page || active_admin_config.per_page
end

def max_csv_records
10_000
end

def max_per_page
10_000
end
Expand Down

8 comments on commit a8ad4f9

@kim3er
Copy link

@kim3er kim3er commented on a8ad4f9 Nov 6, 2013

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?

@seanlinsley
Copy link
Contributor Author

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:

# Workaround for until https://github.com/gregbell/active_admin/issues/2449 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

@kim3er
Copy link

@kim3er kim3er commented on a8ad4f9 Nov 6, 2013

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

@pcreux
Copy link
Contributor

@pcreux pcreux commented on a8ad4f9 Dec 13, 2013

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:

before_filter only: :index do
  @per_page = 10_000 if request.format == 'text/csv'
end

@seanlinsley
Copy link
Contributor Author

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?

@seanlinsley
Copy link
Contributor Author

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.

@seanlinsley
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref: #2847

@seanlinsley
Copy link
Contributor Author

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.

Please sign in to comment.