object_changes inserted by #touch_with_version is not correct value after v9.0.0

[fukayatsu]

v8.1.6 (expected)

bug_report_pt_8.rb

# frozen_string_literal: true

# Use this template to report PaperTrail bugs.
# Please include only the minimum code necessary to reproduce your issue.
require "bundler/inline"

# STEP ONE: What versions are you using?
gemfile(true) do
  ruby "2.5.1"
  source "https://rubygems.org"
  gem "activerecord", "5.1.0"
  gem "minitest", "5.11.3"
  gem "paper_trail", "8.1.2", require: false
  gem "sqlite3", "1.3.13"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Please use sqlite for your bug reports, if possible.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :users, force: true do |t|
    t.text :first_name, null: false
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_id, null: false
    t.string :event, null: false
    t.string :whodunnit
    t.text :object, limit: 1_073_741_823
    t.text :object_changes, limit: 1_073_741_823
    t.datetime :created_at
  end
  add_index :versions, %i[item_type item_id]
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_touch_with_version_on_pt8
    user = User.create(first_name: "Jane")
    user.paper_trail.touch_with_version

    assert_equal 2, user.versions.size
    assert_equal ['updated_at'], YAML.load(user.versions.last.object_changes).keys
  end
end

# STEP SIX: Run this script using `ruby my_bug_report.rb`

result

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

v9.2.0 (not expected)

bug_report_pt_9.rb

# frozen_string_literal: true

# Use this template to report PaperTrail bugs.
# Please include only the minimum code necessary to reproduce your issue.
require "bundler/inline"

# STEP ONE: What versions are you using?
gemfile(true) do
  ruby "2.5.1"
  source "https://rubygems.org"
  gem "activerecord", "5.1.0"
  gem "minitest", "5.11.3"
  # gem "paper_trail", "8.1.2", require: false
  gem "paper_trail", "9.2.0", require: false
  gem "sqlite3", "1.3.13"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Please use sqlite for your bug reports, if possible.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = nil
ActiveRecord::Schema.define do
  # STEP TWO: Define your tables here.
  create_table :users, force: true do |t|
    t.text :first_name, null: false
    t.timestamps null: false
  end

  create_table :versions do |t|
    t.string :item_type, null: false
    t.integer :item_id, null: false
    t.string :event, null: false
    t.string :whodunnit
    t.text :object, limit: 1_073_741_823
    t.text :object_changes, limit: 1_073_741_823
    t.datetime :created_at
  end
  add_index :versions, %i[item_type item_id]
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_touch_with_version_on_pt9
    user = User.create(first_name: "Jane")
    user.paper_trail.touch_with_version # (deprecated)

    assert_equal 2, user.versions.size
    assert_equal ['updated_at'], YAML.load(user.versions.last.object_changes).keys
  end
end

# STEP SIX: Run this script using `ruby my_bug_report.rb`

result

  1) Failure:
BugTest#test_touch_with_version_on_pt9 [bug_report_pt_9.rb:58]:
Expected: ["updated_at"]
  Actual: []

1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

[jaredbeck]

Hello Fukaya-san, thanks for the bug report.

Can we split this into two issues?

Issue A - touch_with_version

  def test_touch_with_version_on_pt9
    user = User.create(first_name: "Jane")
    user.paper_trail.touch_with_version # (deprecated)

    assert_equal 2, user.versions.size
    assert_equal ['updated_at'], YAML.load(user.versions.last.object_changes).keys
  end

Expected: ["updated_at"]
  Actual: []

This is a regression, and I'd like to fix it. Perhaps:

diff --git a/lib/paper_trail/record_trail.rb b/lib/paper_trail/record_trail.rb
index 316ee15..4f5594e 100644
--- a/lib/paper_trail/record_trail.rb
+++ b/lib/paper_trail/record_trail.rb
@@ -449,7 +449,7 @@ module PaperTrail
       ::PaperTrail.request(enabled: false) do
         @record.save!(validate: false)
       end
-      record_update(force: true, in_after_callback: false, is_touch: false)
+      record_update(force: true, in_after_callback: true, is_touch: false)
     end

and we may want to rename in_after_callback to something like use_saved_changes.

What do you think?

Issue B - touch

  def test_touch_on_pt9
    user = User.create(first_name: "Jane")
    user.touch # (create a version after 9.0.0)

    assert_equal 2, user.versions.size
    assert_equal ['updated_at'], YAML.load(user.versions.last.object_changes).keys
  end

  1) Failure:
BugTest#test_touch_on_pt9 [bug_report_pt_9.rb:66]:
--- expected
+++ actual
@@ -1 +1 @@
-["updated_at"]
+["id", "first_name", "created_at", "updated_at"]

I actually like this behavior. We've been talking in #1099 about storing object_changes on destroy. Maybe we should also store object_changes on create. If we did, your test (test_touch_on_pt9) would pass. It's a little wasteful of disk space, but it makes the code a lot simpler.

Let's focus on issue A here, and if you'd like to also work on issue B, please open a separate issue.

[fukayatsu]

@jaredbeck Thank you for your response.

I edited this issue and its title to focus on issue A.

and we may want to rename in_after_callback to something like use_saved_changes.

use_saved_changes Sounds good 👍

Issue B - touch

I actually like this behavior. We've been talking in #1099 about storing object_changes on destroy. Maybe we should also store object_changes on create. If we did, your test (test_touch_on_pt9) would pass. It's a little wasteful of disk space, but it makes the code a lot simpler.

I like your thoughts :)
I'd like to open a new issue about this. => #1121

[jaredbeck]

and we may want to rename in_after_callback to something like use_saved_changes.

use_saved_changes Sounds good 👍

Great! Let me know if you have any trouble with your PR.

[jaredbeck]

Hello Fukaya-san, are you still working on this? Want me to keep it open?

[jaredbeck]

Closing due to inactivity.