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

undefined method `total_count' for #<PaperTrail::Version::ActiveRecord_Relation> #2503

Closed
minyoung opened this issue Dec 9, 2015 · 7 comments

Comments

@minyoung
Copy link

minyoung commented Dec 9, 2015

Going to the history page (using PaperTrail for the auditing) and showing all history throws an error:

undefined method `total_count' for #PaperTrail::Version::ActiveRecord_Relation

(example url: http://localhost:3000/admin/model/history?all=true)

From what I can tell, this seems to only affect when showing all history. What I've dug up so far:

Where is this missing total_count defined? Kaminari::ActiveRecordRelationMethods
Where is this module included? Kaminari::ActiveModelExtension
It seems like this module is only included when Kaminari.config.page_method_name on the ActiveRecord::Model is called though?
Well, when is Kaminari.config.page_method_name called? RailsAdmin::Extensions::PaperTrail::AuditingAdapter.listing_for_model_or_object (which corresponds to the stack trace)
And it seems like Kaminari.config.page_method_name is only called when all is falsey.

So, if I'm understanding this correctly, then when all is truthy, Kaminari.config.page_method_name is not called, which means that the inclusion of Kaminari::ActiveRecordRelationMethods is not triggered, hence the missing method total_count.

What now? A couple ideas (in no particular order) comes to mind

  • just call Kaminari.config.page_method_name? But then the default limit (25) takes place. So then use a high limit? e.g. versions.page.limit(100000)
  • if total_count is missing, then fall back to count? e.g. versions.try(:total_count) || versions.count
  • remove Show all and instead make the items per page configurable?

I've tested the first 2 ideas, and both of them work.

Thoughts? Opinions? Other suggestions?

Top few lines of the stack trace:

activerecord (4.2.0) lib/active_record/relation/delegation.rb:136:in `method_missing'
activerecord (4.2.0) lib/active_record/relation/delegation.rb:99:in `method_missing'
rails_admin (0.8.1) lib/rails_admin/extensions/paper_trail/auditing_adapter.rb:101:in `listing_for_model_or_object'
rails_admin (0.8.1) lib/rails_admin/extensions/paper_trail/auditing_adapter.rb:74:in `listing_for_model'
rails_admin_history_rollback (0.0.3) lib/rails_admin_history_rollback/config/actions/history_index.rb:26:in `block (2 levels) in <class:HistoryIndex>'
rails_admin (0.8.1) app/controllers/rails_admin/main_controller.rb:22:in `instance_eval'
rails_admin (0.8.1) app/controllers/rails_admin/main_controller.rb:22:in `history_index'
@vborshchov
Copy link

Hello, @minyoung. I want to solve this issue using the second method:

  • if total_count is missing, then fall back to count? e.g. versions.try(:total_count) || versions.count

Can you tell me there exactly I have to use this codeversions.try(:total_count) || versions.count. Thanks

vborshchov added a commit to vborshchov/ports_manager_api that referenced this issue Jan 4, 2016
railsadminteam/rails_admin#2503

Going to the history page (using PaperTrail for the auditing) and showing all history throws an error:

undefined method `total_count' for #PaperTrail::Version::ActiveRecord_Relation
(example url: http://localhost:3000/admin/port/history?all=true)
@minyoung
Copy link
Author

minyoung commented Jan 6, 2016

Hi @vborshchov, you'd want to modify this line: https://github.com/sferik/rails_admin/blob/817e34bc6a653e3d4f0948dabeb63317be954ad8/lib/rails_admin/extensions/paper_trail/auditing_adapter.rb#L100 and replace versions.total_count with versions.try(:total_count) || versions.count

Your monkey patch also works. If you don't want to modify upstream library code, then going with the monkey patch might be better since it's clearer and should still work next version. We opted to go with the monkey patch until this is fixed in rails_admin itself.

@minyoung
Copy link
Author

This issue can also manifest as undefined method total_pages' for #PaperTrail::Version::ActiveRecord_Relation` when using "Show All" on the model listing page.

Again, it's Kaminari.config.page_method_name being selectively called on the relation. This time in RailsAdmin::Adapters::ActiveRecord#all.

Those pagination options are populated in RailsAdmin::MainController#get_collection. Which is called by RailsAdmin::MainController#list_entries, and that's where it looks at the all parameter to decide whether to include the pagination options or not.

list_entries is called in RailsAdmin::Config::Actions.Index, which is where the @objects is populated for view rendering (which is where the exception is ultimately thrown from).

In this case, maybe just not render the pagination controls if we're showing all? i.e. In app/views/rails_admin/main/index.html.haml

.col-md-6= paginate(@objects, theme: 'twitter-bootstrap', remote: true) unless params[:all]

@bbenezech
Copy link
Collaborator

@minyoung @vborshchov What is your opinion on where this should be fixed? Kaminari, PaperTrail or RailsAdmin?
If RailsAdmin, can you provide a PR?

@minyoung
Copy link
Author

minyoung commented Feb 5, 2016

I think that this should be fixed in RailsAdmin. The crux of the problem is that Kaminari selectively adds extension methods to ActiveRecord_Relation's (they're added when the page method is called). In the case of showing all objects, we don't call the page method (we're showing all the objects after all).

I've created a pull request with the simplest fixes. I haven't had the time to look into adding tests (yet).

@minyoung
Copy link
Author

minyoung commented Feb 5, 2016

Ah, the second exception I came across was caused by my monkey patch to work around the first exception. My bad about that.

@minyoung
Copy link
Author

Fix got merged in :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants