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

Causes PaperTrail "destroy" versions to not be found #37

Open
TylerRick opened this issue Aug 23, 2018 · 4 comments
Open

Causes PaperTrail "destroy" versions to not be found #37

TylerRick opened this issue Aug 23, 2018 · 4 comments

Comments

@TylerRick
Copy link

I ran into a problem while trying out this branch of paper_trail (and a couple related ones): paper-trail-gem/paper_trail#1143. (It's not in master yet, but it could end up there soon)

When using store_base_sti_class, you can't find PaperTrail "destroy" versions through the versions association because store_base_sti_class causes the has_many :versions association to look for version records where(item_type: subclass) instead of the Rails default of where(item_type: base_class).

Non-destroy versions still work as expected with store_base_sti_class, since those get created through a standard has_many, which means item_type gets set to the subclass due to store_base_sti_class:

lib/paper_trail/record_trail.rb
=> 129:       versions_assoc = @record.send(@record.class.versions_association_name)
   130:       version = versions_assoc.create(data)

The problem is that when paper_trail creates "destroy" versions,,it doesn't go through the versions association — it hard-codes it to always store the base_class in item_type — such that the ActiveRecord hacks that store_base_sti_class provides have no effect.

So if you do this:

record.destroy
record.versions.last.event

you'll find that the destroy event is not there!

Here's the code used for creating destroy versions:

lib/paper_trail/events/destroy.rb
   12:       # Return attributes of nascent `Version` record.
   13:       #
   14:       # @api private
   15:       def data
=> 16:         data = {
   17:           item_id: @record.id,
   18:           item_type: @record.class.base_class.name,
lib/paper_trail/record_trail.rb

    99:       data = event.data.merge(data_for_destroy)
   100: 
=> 101:       version = @record.class.paper_trail.version_class.create(data)

You can see it doesn't go through versions_assoc like it does for update events. It just calls Version.create directly.

@TylerRick
Copy link
Author

Actually, it looks like the hard-coded use of base_class for destroy versions is not a new thing. That behavior has apparently been around since 2011 (paper-trail-gem/paper_trail@f51eb08).

So I'm not sure why I didn't notice this before...

@lorint
Copy link

lorint commented Aug 26, 2018

We have an app that needs to be absolutely accurate in describing past audit history, and while we have no intent to use store_base_sti_class, this kind of issue was a primary motivating factor when we had put together PR#1108. While that code works out great for our code base, it compromised the ability to use .joins() along with the :versions association, so certainly many other projects were adversely affected. As well, some projects rely on having item_type end up storing the base_class for an object as opposed to the real class. (Not that useful for us since we must accurately reflect exactly what type each object really is.)

After working through various scenarios, the cleanest option we had finally arrived upon for ModelConfig were these changes to make the :versions association a little smarter for STI classes.

The most unique part, which left a bad taste in the mouth of the maintainers of the PaperTrail gem, is this part which directly attaches a method to the Reflection object for :versions which attach to a subclassed model.

Our team is in the process of determining what final approach we might end up with to have our stored audit history be absolutely precise, and the secondary goal to reify objects across has_many associations with perfect fidelity, in keeping with what the Association Tracking extension to PaperTrail hopes to achieve.

I realise this might not solve the issue here, but perhaps it gives a little more food for thought in the quest of having destroy events to be accurately recorded. Since our code base seems to work with the issue you describe, I added an assertion in pet_spec.rb that mentions it.

@TylerRick
Copy link
Author

TylerRick commented Aug 31, 2018

Thanks for chiming in, @lorint. I've been watching your PR(s) with great interest to see how it all shakes out. I even tried out your branch for a while. (Ended up running into a really obscure AR behavior that I don't have time to describe (I hope it was just a dream) so switched over to item_subtype branch when it appeared, but overall your branch was working great and I really like what you did there!)

Those sound like great goals that you have, goals which many paper_trail users probably share. Keep up the good work!

(By the way, my name is Tyler Rick, not Rick Tyler as many people seem to like to transpose it to be. :) )

@armellarcier
Copy link

I'm pinging this issue as paper_trails has evolved. Is this still an issue ?

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