randym / activeadmin-axlsx

ActiveAdmin plugin using Axlsx for adding Excel (xlsx) download links for your resources
MIT License
82 stars 134 forks source link

Repeated exports contain duplicate data #3

Closed crishoj closed 11 years ago

crishoj commented 11 years ago

I'm seeing an issue where repeated exports results in XLSX sheets with all rows and headers repeated several times (the number of repetitions being equal to the number of times the export is run).

The cause of the problem seems to be that the ActiveAdmin::Axlsx::Builder provided by ActiveAdmin::Axlsx::ResourceExtension is shared across requests. For each request, the serialize method of the builder simply adds more rows to the existing sheet and returns it.

First of all, I would like to provide a test case for the bug. It wasn't obvious to be where to put it. What would you advice? My first hunch was to add a test case to resource_controller_spec.rb, and simply parse the response, but perhaps you have a better idea.

Secondly, I have a crude fix:

--- a/lib/active_admin/axlsx/resource_extension.rb
+++ b/lib/active_admin/axlsx/resource_extension.rb
@@ -2,11 +2,11 @@ module ActiveAdmin
   module Axlsx
     module ResourceExtension
       def xlsx_builder=(builder)
-        @xlsx_builder = builder
+        @configured_xlsx_builder = builder
       end

       def xlsx_builder
-        @xlsx_builder ||= ActiveAdmin::Axlsx::Builder.new(resource_class)
+        @configured_xlsx_builder || ActiveAdmin::Axlsx::Builder.new(resource_class)
       end

This makes sure that every request gets a fresh builder, but it doesn't do so for a custom builder. What are your thoughts on this?

I'd be happy to submit a pull request pending your feedback.

randym commented 11 years ago

@crishoj Great Stuff Mate!

I think testing at that level is the best thing to do with this kind of issue, but the idea that the @builder is cached between requests seems odd to me.....

I expected that this:

https://github.com/randym/activeadmin-axlsx/blob/master/lib/active_admin/axlsx/builder.rb#L167

Would cache the axlsx package for the request, and allow you to add charts, and to other cool design/image/pivot table/chart stuff before sending it off to the client.

But if subsequent requests are adding rows... hmmm....

crishoj commented 11 years ago

crishoj/activeadmin-axlsx@a17aaaef4fe45a83f7e4ab26db6bc9fd836e815b provides a test case for this.

I'm not completely satisfied by my proprosed fix, though, as it makes it rather clumsy to reconfigure the default builder. What are your toughts on this?

matteofuzz commented 11 years ago

Any news?

follmann commented 11 years ago

Great gem! But same problem here. Any development?

randym commented 11 years ago

On it!

Sorry for being so freaking slow!

randym commented 11 years ago

This is fixed in master as of 7154782aee1b6666765e697b422afca7ca3b3632

Can I ask you to pull from github and confirm? If everything looks good, I'll release a new version.

matteofuzz commented 11 years ago

I tested it on Rails 3.2.12 and ActiveAdmin 0.6.0 and seem all OK.

Thanks a lot, great gem, we AA users need it.

follmann commented 11 years ago

@randym No need for apologizing! Anyone maintaining an open source project understands the effort behind it. Thanks for the update! I can also confirm the fix works for AA 0.6 and Rails 3.2.13. Are you gonna release it?

randym commented 11 years ago

Hi all,

I'll be releasing this weekend. Thanks for everyone's patience!

randym commented 11 years ago

released