Skip to content

Touch callback does not always create version#1285

Merged
jaredbeck merged 1 commit intopaper-trail-gem:masterfrom
parachutehealth:fix-touch-with-skip-ignore
Apr 5, 2021
Merged

Touch callback does not always create version#1285
jaredbeck merged 1 commit intopaper-trail-gem:masterfrom
parachutehealth:fix-touch-with-skip-ignore

Conversation

@quainjn
Copy link
Copy Markdown
Contributor

@quainjn quainjn commented Jan 4, 2021

Currently touch with an ignored or skipped attribute will still create a new version. This changes the behavior to make it consistent with update callback.

@tlynam
Copy link
Copy Markdown
Contributor

tlynam commented Jan 18, 2021

Hi @jaredbeck, I hope you're well. This looks good to me. Does this look good to you?

@tlynam
Copy link
Copy Markdown
Contributor

tlynam commented Jan 18, 2021

Do you know why the tests are failing? Looks like the tests need to be updated?

@quainjn
Copy link
Copy Markdown
Contributor Author

quainjn commented Jan 21, 2021

Do you know why the tests are failing? Looks like the tests need to be updated?

I think it's due to a difference in behavior in ActiveRecord's handling of touch and dirty attributes from Rails 5 to Rails 6. I don't think it's possible to support this new functionality with Rails 5 which is why the tests are failing (and possibly why the previous implementation set force: true for touch). @jaredbeck is it reasonable to have the behavior of touch depend on the version of Rails between 5 and 6? If so, can update the code/specs to support both.

@jaredbeck
Copy link
Copy Markdown
Member

Currently touch with an ignored or skipped attribute will still create a new version.

I wonder why. Touch-tracking was implemented by @westonganger in 02b6de2. Weston, do you remember why on_touch uses force: true? Is it OK to change that?

.. is it reasonable to have the behavior of touch depend on the version of Rails between 5 and 6?

Yes, if the differences are thoroughly documented.

@westonganger
Copy link
Copy Markdown
Contributor

Well I see that in 02b6de2 there was no option for is_touch so I assume that force: true was required to cause the update event for touch when no attributes had changed. With the is_touch option its possible we no longer need to use the force: trueto achieve this.

02b6de2#diff-1f8592ba4c3060dd4ee3248e4104efc57d55d350f3234cf8562a55825b949dc0R123

@quainjn quainjn force-pushed the fix-touch-with-skip-ignore branch 2 times, most recently from 1a0c6d3 to 435421d Compare March 9, 2021 01:13
@quainjn quainjn force-pushed the fix-touch-with-skip-ignore branch from 435421d to 16ad921 Compare March 9, 2021 01:16
@jaredbeck jaredbeck merged commit 584e2f7 into paper-trail-gem:master Apr 5, 2021
jaredbeck added a commit that referenced this pull request Apr 5, 2021
@jaredbeck
Copy link
Copy Markdown
Member

Released in 12.1.0

y-yagi added a commit to y-yagi/paper_trail that referenced this pull request Mar 1, 2022
Before paper-trail-gem#1285, a version record was created when associated object is touched.
But because of Rails don't track implicit touch mutation[1], now PaperTail
don't create a version record was created when associated object is touched.

This patch try to keep creating a version record in that case by doing checking an
event and changed value.

Fixes paper-trail-gem#1339.

[1]: rails/rails@dcb8259
y-yagi added a commit to y-yagi/paper_trail that referenced this pull request Mar 1, 2022
Before paper-trail-gem#1285, a version record was created when associated object is touched.
But because of Rails don't track implicit touch mutation[1], now PaperTail
don't create a version record was created when associated object is touched.

This patch try to keep creating a version record in that case by doing checking an
event and changed value.

Fixes paper-trail-gem#1339.

[1]: rails/rails@dcb8259
y-yagi added a commit to y-yagi/paper_trail that referenced this pull request Mar 1, 2022
Before paper-trail-gem#1285, a version record was created when associated object is touched.
But because of Rails don't track implicit touch mutation[1], now PaperTail
don't create a version record was created when associated object is touched.

This patch try to keep creating a version record in that case by doing checking an
event and changed value.

Fixes paper-trail-gem#1339.

[1]: rails/rails@dcb8259
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