Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

How to implement paper_trail on has_many associations #148

Closed
blawzoo opened this Issue Apr 1, 2012 · 23 comments

Comments

Projects
None yet

blawzoo commented Apr 1, 2012

Actually I use paper_trail in rails to track my models versions.But the documentation on the github repo indicates that the gem doesn't support has_many, belongs_to associations.

Let's say I've an app that records the ceos names of some comapnies:

class Company < ActiveRecord::Base
  has_many :ceos
  has_paper_trail
end

class Ceo < ActiveRecord::Base
  belongs_to :companies
  has_paper_trail
end

The above example represent the informations of ABC inc

company.name => "ABC"
company.ceo.past => "John Henry"
company.ceo.present =>  "Amy Warren"

How can I implement the following operation so It will reset the company and the company's ceos names to the last version?

+1

+1 Any word on this?

Collaborator

batter commented May 14, 2013

Not yet, but suggestions are welcome. I'm going to visit this soon when I started tackling the 3.1.0 release.

I was just thinking about this.

How about a VersionAssociation and an accompanying table? Either that, or a reference to associations within the row on the version table.

table.versions.associations = [123, 234]`
Version.associations =>  [Version#123, Version#234]

I'm sure a join table would keep things cleaner, and I don't think you can extend this functionality properly without introducing join tables. This would be much cleaner:

Version.associations << Version.association1
Version.associations => [Version.association1]

Then, when calling Version.association1, you would reference the Version.associations array to see if it exists, if not, return the live version of that association. I imagine you could, with some ingenuity, provide facility to manage any number of associations:

assoc = Version.association1
assoc.associations << assoc.deep_association
Version.associations << assoc
Version.association1.associations => [Version.deep_association]

Version above is actually model.previous_version, which would be wrapped in an object to proxy the associations.

This would work for has_one relationships quite simply, but has_many would require a different strategy to address append/delete.

Thoughts? Or am I perhaps missing some subtle complexity?

Collaborator

batter commented Jul 31, 2013

@damien-roche - Thanks for the suggestion, I believe that the concept of a VersionAssociation was actually introduced by @tderks in his pull request in #44. Admittedly I have been meaning to try to take a look at and tackle this issue for a while but still haven't gotten around to it. I'm going to try to start looking at it this month in the hopes of releasing 3.0.0 with this feature (or some portion of it) and some tests for it, but this seems like it will likely be the most challenging issue to tackle. If you get a chance to experiment with this and find anything promising then please make a pull request and I'll review it as soon as I get a chance.

myitcv commented Aug 13, 2013

@fullbridge-batkins - I'm still getting up to speed on this issue, so apologies in advance if these comments cover old ground.

Building slightly on the comment above from @damien-roche (which according to your comment builds on a PR from @tderks)

Is the problem simplified by creating a new version of the parent each time a child object (either has_one or has_many) ticks a version? And then using something like a VersionAssociation to track the associations as described?

In pseudo code:

p = Person.create
# p.versions.count == 1

h = p.homes.create
# p.versions.count == 2
# h.versions.count == 1

h.update_attribute :name, 'test'
# p.versions.count == 3
# h.versions.count == 2

I realise this increases the number of versions considerably but it does ensure the reify process is guaranteed.

Collaborator

batter commented Aug 13, 2013

@myitcv - It seems like this should probably work along these lines. Again, I haven't had a chance to take a proper crack at this but I'm planning on doing so soon.

myitcv commented Aug 15, 2013

@fullbridge-batkins - great to hear.

Is it worth pulling together a wiki page where people can capture requirements/edge cases? Would that be useful?

tderks commented Aug 15, 2013

Hey all,

It's been some time since i worked on it, focused on finishing my study.
I also noticed there were many difficult problems arising when trying to restore associations correctly.

  1. Restoring all the associations correctly in memory (the reify command) was hard. This was a long time ago, so probably alot of things changes in rails. But it used to be very hard to construct the whole object correclt in memory. (without modifying objects)
  2. The version_association table makes it easy to find related changes. However there are MANY different changing possibilities, which i think could lead to unexpected behaviours. Alot of unit tests will be needed and probably some decisions that need to be made when restoring. (see 3.)
  3. When restoring has_one and has_many associations you may get unexpected behaviours when you have assigned child objects to a different parent. When you restore a parent object that has a child assigned to another parent object, you will probably overwrite the object and remove it from the new parent.

To summarize it is a complex feature, which brings me to an easy but less complete solution which could be usefull in alot of cases. In my previous patch i also introduced transaction support. For every model change it also stores the transaction id. With a new function called 'rollback' you can restore all the objects changed in the same transaction. This does not allow you to restore objects including relations to a specific time. However it does allow you to rollback child objects that are created/deleted when the parent is created/deleted very easily!

I could probably wrap up this feature including unit tests in one evening!

@batter batter was assigned Oct 2, 2013

Collaborator

batter commented Oct 2, 2013

@tderks - If you have the time / bandwidth to take another crack at it, that would be awesome. I was going to go back and take a look at your work and try to merge it in later this week or early next week but I'm a bit concerned I'll be lost. I'd say that the feature doesn't have to be amazing off the bat, I just wanted to include some sort of functionality that provides at least a partial solution for this type of thing in the final 3.0.0 release.

niuage commented Nov 20, 2013

Hey, I read a lot of threads about this issue and I'm a bit confused right now. I have models similar in principle to the Person, Book, Authorship example from the doc, and I want to know if the following should work:

book = Book.create
author_a = Person.create
author_b = Person.create

book.authors << author_a

book.update_attributes(title: "new title")

book.authors << author_b

book.authors.count
# => 2

book.previous_version.authors.count
# => 1

Should this work? Basically, when I come back to a previous version, only the associated models at the time should be returned, right?

Collaborator

batter commented Nov 20, 2013

Theoretically thats what the code in pull request #90 would do, but it hasn't been merged in. I'm putting it off until 3.1.0 because I don't want to delay 3.0.0 any longer (I just released 3.0.0.rc2 and think that will be the final). I'm pushing this off until the next final release (and have been procrastinating working with this haha) because of the complexity involved and I'm anticipating there being some issues a high degree of difficulty to get this working AND make sure it is well tested. This is the first thing on my TODO list after the 3.0.0 release is finalized.

niuage commented Nov 21, 2013

Ok, thanks a lot for confirming this. I'll try to work with #90, see if it works for me, for now, and I'll upgrade to 3.1.0 when it's released!

pakgva commented Jan 20, 2014

Any update for this issue? I really appreciate the update soon. Thanks.

Just an idean : why not using the metadatas ?
You can already add metas so why not using it with some naming conventions like :

class Chapter < ActiveRecord::Base
  belongs_to :book
  has_paper_trail meta: {restor_books :books_count}
end

And yet if you restore the chapter just go find the n books having this chapter id in there chapter_id column.

@batter batter added the Not a bug label Apr 17, 2014

@gdurelle its not a very scalable solution being an n+1 query

Contributor

NullVoxPopuli commented Oct 15, 2014

I think I have this implemented. I'll try to get a PR together in a couple weeks.

dmitry commented Oct 15, 2014

@NullVoxPopuli please let me know, if you will require some help.

Contributor

NullVoxPopuli commented Oct 15, 2014

How much do we want to depend on active record?
I can put my stuff in a gist until I have time to work on a PR, after asking my manager if it can be open sourced (should be ok -- pretty requested feature for paper_trail).

I also ran in to a snafu with soft deleted records, which has been addressed, but I know a couple of the soft delete gems allow customization of the column that tracks the deleted_at date, so, I don't know how to best handle that (generically) either.

Collaborator

batter commented Oct 16, 2014

Hey all, there is an open PR in #345 which I think may be the solution we're looking for, at least the author (@lyfeyaj) claimed he had some success with it, I am trying to give it attention soon. Just been real busy with new stuff at work and what not recently, but I am making progress towards 3.1.0. If anybody wants to put together a PR with tests, that would be the best case scenario, otherwise I need to take time and write bunch of tests to make sure everything is working properly, which will take some time..

Contributor

NullVoxPopuli commented Oct 16, 2014

that looks kinda similar, but simpler than what I did. I wonder if all my specs will pass under that code

dmitry commented Oct 16, 2014

@NullVoxPopuli at least you have specs, it's a half of work. ;)

Collaborator

batter commented Dec 11, 2014

Closed in favor of #439, thanks to everyone who contributed to the dialog here.

@batter batter closed this Dec 11, 2014

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