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

patch to load_changeset to also load has_many -> through changes #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

punkisdead
Copy link

I had a need to try to track changes of a has many through relationship using paper trail and have leveraged some of the work in this project to do so. I'd like to start a conversation about how to best accomplish this, and also add the functionality to the other association types has_many, has_one, etc.

Below is the start of this work, and it is working for what our case needs, but still needs better testing and some refactoring yet.

@punkisdead punkisdead force-pushed the update_changesets_to_include_associated_changes branch 2 times, most recently from e6aaa17 to 3d2014d Compare December 29, 2020 14:48
@westonganger
Copy link
Owner

I think it is likely desirable that we would sometimes want to call the default load_changeset method without loading the associations every time. Instead of monkey patching the original method, could we maybe instead provide a new method?

@punkisdead
Copy link
Author

What about doing something similar to how reify works in this gem, where you pass in an options hash and if nothing is passed, just call the original method?

@westonganger
Copy link
Owner

Sure

@westonganger
Copy link
Owner

@punkisdead could you please add some documentation for this as well?

@punkisdead
Copy link
Author

@punkisdead could you please add some documentation for this as well?

Yes, absolutely. I've got a few other tasks that are taking priority right now, but will get back to it in a day or two.

@punkisdead punkisdead force-pushed the update_changesets_to_include_associated_changes branch from e58fba3 to aa1e4e4 Compare January 14, 2021 15:42
@westonganger
Copy link
Owner

@punkisdead I saw you made some changes and updated the documentation Please advise once you are done. Do you still intend to add the other relationship functionality?

@punkisdead
Copy link
Author

@punkisdead I saw you made some changes and updated the documentation Please advise once you are done. Do you still intend to add the other relationship functionality?

I hadn't intended to add the other relationship functionality in this PR, selfishly our needs were only for this. I can possibly look into adding them later in a separate PR if you'd like.

@westonganger
Copy link
Owner

It would be sweet if we could add them within this PR. If you don't get around to it, I will try and take a look at some point.

@punkisdead
Copy link
Author

Ok, let me see what I can do.

@GMolini
Copy link

GMolini commented Mar 25, 2021

I would love this feature. I dont really need to reify anything but would love to be able to diff including the has_many associations

@dingyu-tourcandy
Copy link

+1 for this feature.

Just tried it on paper_trail 15.1 and got the following error:

/app/.bundle/bundler/gems/paper_trail-association_tracking-aa1e4e462936/lib/paper_trail_association_tracking/version_concern.rb:19:in `block in <module:VersionConcern>': undefined method `scope' for PaperTrail::VersionConcern:Module (NoMethodError)
scope(:within_transaction, ->(id) { where transaction_id: id })
^^^^^

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.

4 participants