Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed history query for model subclasses #1567

Closed
wants to merge 1 commit into from

4 participants

@Tuckie

Fixes issue #1524 (thank you github edit button!)

@coveralls

Coverage increased (+1.2%) when pulling b9c8a97 on Tuckie:papertrail-with-inheritance into 51f2c8b on sferik:master.

View Details

@bbenezech bbenezech commented on the diff
...ails_admin/extensions/paper_trail/auditing_adapter.rb
@@ -67,7 +67,11 @@ def listing_for_model(model, query, sort, sort_reverse, all, page, per_page = (R
sort = :created_at
sort_reverse = "true"
end
- versions = ::Version.where :item_type => model.model.name
+ if model.model.base_class.name == model.model.name
+ versions = ::Version.where :item_type => model.model.name
+ else
+ versions = ::Version.where :item_id => model.model.all
@bbenezech Collaborator

Are you sure? This line looks really awkward to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Tuckie

The original query, versions = ::Version.where :item_type => model.model.name is more efficient (one simple db query), but only the base class name is stored in the versions table.

When dealing with subclasses, we need to determine what specific items are part of that subclass, so a separate query on the original table is needed:
versions = ::Version.where :it em_id => model.model.all
In hindsight, it would be slightly more efficient to do:
versions = ::Version.where :item_id => model.model.select(:id).all (or a join)

Your thoughts?

@mshibuya
Collaborator

We can't merge this, because it affects performance massively when the model has lots of records.
Please look for alternatives.

@mshibuya mshibuya closed this
@Tuckie

I realize that the first half of this isn't a true fix for the listing_for_model, as (among performance issues) it doesn't take into account deleted items. We can save this discussion for pull 1749.

Could we perhaps just merge the change from line 88? This would at least solve the polymorphic / STI listing_for_object history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 15, 2013
  1. @Tuckie
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 2 deletions.
  1. +6 −2 lib/rails_admin/extensions/paper_trail/auditing_adapter.rb
View
8 lib/rails_admin/extensions/paper_trail/auditing_adapter.rb
@@ -67,7 +67,11 @@ def listing_for_model(model, query, sort, sort_reverse, all, page, per_page = (R
sort = :created_at
sort_reverse = "true"
end
- versions = ::Version.where :item_type => model.model.name
+ if model.model.base_class.name == model.model.name
+ versions = ::Version.where :item_type => model.model.name
+ else
+ versions = ::Version.where :item_id => model.model.all
@bbenezech Collaborator

Are you sure? This line looks really awkward to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
versions = versions.where("event LIKE ?", "%#{query}%") if query.present?
versions = versions.order(sort_reverse == "true" ? "#{sort} DESC" : sort)
versions = all ? versions : versions.send(Kaminari.config.page_method_name, page.presence || "1").per(per_page)
@@ -81,7 +85,7 @@ def listing_for_object(model, object, query, sort, sort_reverse, all, page, per_
sort = :created_at
sort_reverse = "true"
end
- versions = ::Version.where :item_type => model.model.name, :item_id => object.id
+ versions = ::Version.where :item_type => model.model.base_class.name, :item_id => object.id
versions = versions.where("event LIKE ?", "%#{query}%") if query.present?
versions = versions.order(sort_reverse == "true" ? "#{sort} DESC" : sort)
versions = all ? versions : versions.send(Kaminari.config.page_method_name, page.presence || "1").per(per_page)
Something went wrong with that request. Please try again.