paper-trail-gem / paper_trail

Track changes to your rails models
MIT License
6.78k stars 897 forks source link

How to implement paper_trail on has_many associations #148

Closed blawzoo closed 9 years ago

blawzoo commented 12 years ago

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?

hrasguido commented 12 years ago

+1

garrettlancaster commented 11 years ago

+1 Any word on this?

batter commented 11 years ago

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

damien-roche commented 11 years ago

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?

batter commented 11 years ago

@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 11 years ago

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

batter commented 11 years ago

@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 11 years ago

@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 11 years ago

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 commented 11 years ago

@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 10 years ago

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?

batter commented 10 years ago

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 10 years ago

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 10 years ago

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

gdurelle commented 10 years ago

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.

DeepAnchor commented 10 years ago

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

NullVoxPopuli commented 10 years ago

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

dmitry commented 10 years ago

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

NullVoxPopuli commented 10 years ago

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.

batter commented 10 years ago

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

NullVoxPopuli commented 10 years ago

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

dmitry commented 10 years ago

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

batter commented 9 years ago

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