Fixed history query for model subclasses #1567

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

Tuckie commented Mar 15, 2013

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

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 Apr 24, 2013

...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

bbenezech Apr 24, 2013

Collaborator

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

Tuckie commented Apr 24, 2013

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?

Collaborator

mshibuya commented Oct 4, 2013

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

mshibuya closed this Oct 4, 2013

Tuckie commented Oct 7, 2013

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