Skip to content

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

Open
punkisdead wants to merge 1 commit into
westonganger:masterfrom
BoldPenguin:update_changesets_to_include_associated_changes
Open

patch to load_changeset to also load has_many -> through changes#22
punkisdead wants to merge 1 commit into
westonganger:masterfrom
BoldPenguin:update_changesets_to_include_associated_changes

Conversation

@punkisdead

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Owner

Sure

@westonganger

Copy link
Copy Markdown
Owner

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

@punkisdead

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

westonganger commented Jan 27, 2021

Copy link
Copy Markdown
Owner

It would be sweet if we could add them within this PR.

@punkisdead

Copy link
Copy Markdown
Author

Ok, let me see what I can do.

@GMolini

GMolini commented Mar 25, 2021

Copy link
Copy Markdown

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
Copy Markdown

+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