Store the actual changes made by a touch

[TylerRick]

(A follow-up to #1121)

It bugs me that I have update events that get created that don't have any changes listed in changeset (object_changes). Wat, nothing changed?? Then why was this useless update event recorded?

Looking into it, of course, I discovered that the version came from a touch.

But this isn't useful. You might even call it a bug (though I'm calling it a feature suggestion): it's a flat-out lie to claim there were no changes made by this update (a whiter lie than pre- b9c9e79, perhaps, but a lie nonetheless 😄). Why do we even bother recording an update event?

If a change was made, then we really should record what was changed. But how?

Record is_touch in the version record

Currently we pass is_touch: true in to the Events::Update object but we don't persist it anywhere.

One small improvement would be to at least pass it along as metadata so that if a column exists for it in the versions table, users will at least be able to see that the version was a touch and excuse the lack of a changeset with a "oh, it was from a touch, so at least we know it was one of the 7 timestamp attributes in this record that was updated". An improvement, but still not good enough...

Override touch

Pretty straightforward: just do a diff of attributes before and after calling super.

Unfortunately, it means overriding touch on all models that have PaperTrail, since we can't do it with callbacks alone currently. But I'm okay with that if you are...

Add paper_trail.touch() like we have paper_trail.update_columns()

Same as above but make it opt-in only so all call sites would have to be changed to paper_trail.touch() (ugh).

Personally, I'd rather have it just automatically record changesets from touches without having to opt in with paper_trail.touch(). That seems more consistent with how it automatically records changesets from all saves. (And I don't think comparing touch to update_columns is fair, since the docs for update_columns even claim, "... it goes straight to the database, but take into account that in consequence the regular update procedures are totally bypassed." whereas for touch, "the after_touch, after_commit and after_rollback callbacks are executed" so it's fair to have the expectation that callbacky things still work pretty much the same as they do for saves.)

Do our own mutation tracking

We could add an instance variable to each model for our own use that we update from each of PaperTrail's after_save/after_create/after_touch callbacks.

If we had a copy of attributes from before the touch (there's no before_touch but maybe it would be close enough if we reset our cache on every after_save?), then we could simply diff against that in our after_touch to get a hash of changes...

Wait for Rails to add a better way?

Maybe a hash of changes could be passed to after_touch or a touched_changes method could be added to Rails? See issue at rails/rails#33429.

Other options?

Please share...

[jaredbeck]

Good analysis, thanks Tyler. I think rails sees touch and update as two very different things. But, I agree that PT should try to populate object_changes, especially when you touch custom timestamp columns.

# updates started_at, ended_at and updated_at/on attributes
product.touch(:started_at, :ended_at) 
# `versions.object` is correct (right?), `object_changes` is null

As a workaround, I suppose you could do:

product.assign_attributes(started_at: Time.current, ended_at: Time.current)
product.save(validate: false)
# Both `versions.object` and `object_changes` are correct

Pretty awkward, but PT will track that change correctly.

.. overriding touch ..

We don't currently override any rails methods and I don't want to start.

Wait for Rails to add a better way?

Ideally rails would "simply" enable dirty-tracking for touch.

I also liked your suggestion about passing a hash of changes as the callback argument. Do AR callbacks support arguments?

Finally, it'd also be nice if rails added a before_touch callback.

Record is_touch in the version record
.. at least we know it was one of the 7 timestamp attributes in this record that was updated". An improvement, but still not good enough...

I agree, it's not great, and it'd be a pain to introduce a new column.

Add paper_trail.touch() like we have paper_trail.update_columns()

The reason we have a special paper_trail.update_columns method is because the rails update_columns method skips callbacks, so we can't hook into it. Conversely, touch has callbacks. So, I don't want to add a paper_trail.touch method.

My preferences, so far:

1. rails "simply" enables dirty-tracking for touch
2. rails adds either of the other two features we've suggested above
3. some idea we haven't had yet
4. adding a paper_trail.touch method

[hashwin]

@jaredbeck is there any way to skip adding a record completely when touching (given that updated_at is a skipped column)?

[feliperaul]

@jaredbeck Jared, can you remove the not a bug label considering that PaperTrail is creating this empty versions even when :updated_at is an ignored attribute?

Today I stumbled on this bug again. I have a Father model that has_many Child models, and when Childs are created they update a ignored attribute on the father, but it ends up creating tens of PaperTrail::Versions with empty change sets.

[jaredbeck]

Jared, can you remove the not a bug label ..

Sure thing, removed. 👍 Actually, I've decided to stop using labels for a while. A "bug" label gives the impression that someone else is going to fix the problem. We don't have enough volunteers for that (new contributors welcome!). I try to help people working on issues, though. I've spent many hours on this one. I've even helped make improvements to the rails docs as a result of this issue.

Anyone interested in taking this issue further should read: Tyler's and my analysis above, and definitely read all of rails/rails#33429. Whoever picks up this torch, I'll help as much as I can. ❤️

[xtr3me]

@feliperaul I use the following monkey patch to ignore creating a version for a model that is touched. It's not looking at what was updated, so the ignore: and skip: option is not used with this monkey patch.

Add this snippet to an initializer:

module RecordTrailTouchExtension
  def record_update(force:, in_after_callback:, is_touch:)
    return if is_touch
    super
  end
end

module PaperTrail
  class RecordTrail
    prepend RecordTrailTouchExtension
  end
end

[quainjn]

@jaredbeck looks like this was fixed in Rails 6 with rails/rails@d1107f4.

Started on the bug fix around ignored/skipped attributes in #1285 and could extend to support object_changes with touch, but would result in different behavior for Rails 5 vs 6.

[github-actions]

This issue has been automatically marked as stale due to inactivity.
The resources of our volunteers are limited.
Bug reports must provide a script that reproduces the bug, using our template. Feature suggestions must include a promise to build the feature yourself.
Thank you for all your contributions.

[jaredbeck]

Closed by #1285