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

supporting has_many reification based on @lyfeyaj and @NullVoxPopuli 's works #439

Merged
merged 30 commits into from
Dec 22, 2014

Conversation

bli
Copy link
Contributor

@bli bli commented Nov 3, 2014

This is based on @lyfeyaj and @NullVoxPopuli 's works.
What I changed are mainly:

  1. Reification for has_many through
  2. Add tests and make sure all of them pass
  3. Add option mark_for_destruction to mark the has_one/has_many associations that did not exist in the
    reified version for destruction, instead of remove them.
  4. Disable recursive reification of has_one/has_many associations. See the scenario below for more.
  5. Some small tunings like using a hash to reduce the number of loop from n*m to n

Some notes:

There is a limitation that the Reification will not work properly if the parent of a child can be changed.
Consider this scenario:

  1. Create a customer A.
  2. Create a customer B
  3. Create an order for B.
  4. Update A.
  5. Change the customer of the order to A
  6. Now reify the last version of A. Ideally we would expect no orders in the reified A.

Recursive reification is not supported. Take the customer, order, and line item model for example, when a customer is reified, the line item is not reified.
Consider this scenario:

  1. Create a customer
  2. Create an order for the customer with a line item
  3. Update the customer
  4. Update the line item
  5. Now reify the last version of the customer. If we support recursive reification, we would expect to see the line item before the updates in step 4.

@bli
Copy link
Contributor Author

bli commented Nov 3, 2014

OK, I am done. :( The rest of the CI errors are due to Timecop's lack of support for Ruby 1.8.x and I can't find an easy replacement.

@NullVoxPopuli
Copy link
Contributor

maybe you could wrap some of this functionality in ruby version > 1.9?

@NullVoxPopuli
Copy link
Contributor

@bli there is also this https://github.com/bebanjo/delorean, which has similar functionality

@bli
Copy link
Contributor Author

bli commented Nov 4, 2014

Thanks @NullVoxPopuli . It looks like the good old DeLorean is working well in the good old 1.8.7. I have to use the more updated Timecop in 1.9.2 and above though as it does a better cleanup when 'return', otherwise a few tests would have been broken.

@batter batter added this to the 3.1.0 milestone Nov 4, 2014
@batter batter self-assigned this Nov 4, 2014
@batter
Copy link
Collaborator

batter commented Nov 4, 2014

Wow this looks very promising and like a ton of effort has gone into this. Looking forward to reviewing this. I'm at a conference this week but will make time to comb through this soon.

@NullVoxPopuli
Copy link
Contributor

Is there a test showing that the has_manies are only tracked if there are changes?

@bli
Copy link
Contributor Author

bli commented Nov 5, 2014

@NullVoxPopuli would you show a specific scenario for such a test? Currently there are tests to show that has_manies are tracked if they are changed.

Btw, current the transaction id is derived from the version id and the version-association class is hard-coded to PaperTrail::VersionAssociation. This would become a problem for people using multiple version tables. I was thinking maybe we can add an option (e.g. :association_class_name) to customize the version-association class name, like the existing :class_name option. What do you guys think?

Also, currently the version-association records are created as long as paper_trail is turned on and there are belongs_to associations. This might not be necessary or desired for some models from performance's perspective. What do you guys think about making it optional to capture these version-association records? Also, if we make the option to be false by default, we can reduce the impact to existing paper_trail users.

Ben Li added 2 commits December 11, 2014 14:24
… merge-with-lyfeyaj

Conflicts:
	lib/paper_trail/has_paper_trail.rb
… merge-with-lyfeyaj

Conflicts:
	lib/paper_trail/has_paper_trail.rb
@batter
Copy link
Collaborator

batter commented Dec 11, 2014

@bli - Awesome work! And thanks for the quick feedback loop, apologies for being so slow on my end, been super busy at work lately.

I think there are only a few loose ends to tie up before we can merge this. The install generator should have an option as to whether the association pieces are added (similar to what we do with the object_changes), and the README needs an update to talk about the functionality. I'm happy to try to do either but I figured I would give you an opportunity to take a crack at updating the README since you probably know the ins and outs of the feature(s) much better than I at this point, as well as any potential pitfalls. Are you up to that?

@bli
Copy link
Contributor Author

bli commented Dec 11, 2014

I am happy to try updating the README. should be able to find some time either today or tomorrow.

@batter
Copy link
Collaborator

batter commented Dec 16, 2014

@bli - Do you think you'll still have time to take a crack at the README or do you want me to try to tackle it?

@bli
Copy link
Contributor Author

bli commented Dec 16, 2014

I have done most of it. I have been busy these few days, but should be able to submit it later today.

batter added a commit that referenced this pull request Dec 22, 2014
supporting has_many reification based on @lyfeyaj and @NullVoxPopuli 's works
@batter batter merged commit 3175193 into paper-trail-gem:master Dec 22, 2014
batter added a commit that referenced this pull request Dec 22, 2014
@bli
Copy link
Contributor Author

bli commented Dec 22, 2014

@batter, just a reminder that the new option for the install generator has not been supported yet, not sure if this is in your agenda?

@batter
Copy link
Collaborator

batter commented Dec 22, 2014

Yes, that is on my todo list, it's been a busy day. Thanks again for all the work you did on this, super excited we got to merge this!

Now I have to take care of that and a bunch of failing tests and deprecation warnings that just started showing up in the test suite since the release of ActiveRecord 4.2.0 😦

@reggieb
Copy link

reggieb commented Dec 23, 2014

@bli and @batter really looking forward to refactoring this into my current project in the new year. It should be a great way to start 2015. Well done!

@mbajur
Copy link

mbajur commented Dec 24, 2014

First, i need to say that i'm EXTREMELY excited about that PR. I was waiting for that feature since my first days with ruby.

However, i'm not able to use it in a dull app. The app is here: https://github.com/mbajur/paper_trail_associations

Steps to reproduce:

  • clone app
  • run rake db:create db:migrate db:seed
  • start server
  • go to /posts/1/edit and try to edit a post by adding a tag to it.
  • observe how the versions list in /posts/1 is not beeing populated with the new version (with the tag added)

I'm not sure if i'm doing something wrong (it's really possible, maybe i'm too excited to stay focused and sweat is flooding my eyes!) or it's some kind of a hidden bug but it looks like paper_trail can't see my post edits (when the only modification is add/remove a tag).

Thanks in advance for any clues

@batter
Copy link
Collaborator

batter commented Dec 24, 2014

@mbajur - If you look a little more closely, you should see that there has been a new join table added called version_associations, which stores all the data about how to handle association restoration.

There was also an update to the README with details on how to use the association tracking feature.

@mbajur
Copy link

mbajur commented Dec 24, 2014

Thanks for your reply batter. I've seen these things and readed them carefully but still one thing is unclear to me - are the association changes automatically included in post.versions collection ? Or i need to handle that manually? Reifying is one thing but listing all the versions (with just associations changes) and their changes is something different.

@batter
Copy link
Collaborator

batter commented Dec 24, 2014

I don't think association changes would automatically be included in the singular association since they are stored in a one-off association. It wouldn't make sense for post.versions to contain a version that didn't correspond to that model instance in it IMO. The functionality probably can't be visualized in a perfect way since the structure is fairly complex, but you'd basically need to do something to mimic the join being done when reification happens. @bli can probably explain better than I could how you might be able to visualize that type of stuff, not sure it's feasible.

@bli
Copy link
Contributor Author

bli commented Dec 24, 2014

@batter has explained better than I could have. What happened here in our project is we make sure one version of post is always created whenever the associations are changed even if the post itself is not changed. With this, we can then just use post.versions. Otherwise, as @batter explained, you will then have to resort to a hand crafted query to get the versions for both the post and the tag.

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

Successfully merging this pull request may close these issues.

6 participants