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

touch_with_version does not work with conditional options #1051

Closed
westonganger opened this issue Feb 20, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented Feb 20, 2018

touch_with_version is not working correctly with the on and only options combined. However touch_with_version works fine with both options seperately. For example:

has_paper_trail on: [:update], only: [:some_attr]

With this in the model, after running touch_with_version runs and doesn't error but no version is created.

# 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.4.1"
  source "https://rubygems.org"
  gem "activerecord", "5.1.4"
  gem "minitest", "5.10.3"
  gem "paper_trail", require: false
  gem "sqlite3"
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.integer :transaction_id
    t.datetime :created_at
  end
  add_index :versions, %i[item_type item_id]
  add_index :versions, [:transaction_id]

  create_table :version_associations do |t|
    t.integer  :version_id
    t.string   :foreign_key_name, null: false
    t.integer  :foreign_key_id
  end
  add_index :version_associations, [:version_id]
  add_index :version_associations, %i[foreign_key_name foreign_key_id],
    name: "index_version_associations_on_foreign_key"
end
ActiveRecord::Base.logger = Logger.new(STDOUT)
require "paper_trail/config"

# STEP THREE: Configure PaperTrail as you would in your initializer
PaperTrail::Config.instance.track_associations = true

require "paper_trail"

# STEP FOUR: Define your AR models here.
class User < ActiveRecord::Base
  has_paper_trail on: [:update], only: [:first_name]
end

# STEP FIVE: Please write a test that demonstrates your issue.
class BugTest < ActiveSupport::TestCase
  def test_1
    user = User.new(first_name: "Jane")
    user.save!
    
    assert_difference(-> { PaperTrail::Version.count }, +1) {
      user.paper_trail.touch_with_version
    }
  end
end

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Turns out this is also true with the on & if option (and probably more).

Currently my workaround involves creating new classes that inherit from the parent class and simply have has_paper_trail without any options. I then use this new class whenever I want to call touch_with_version. However I think this is a very ugly solution.

How can I create a new version regardless of any of the options when using touch_with_version. This must be possible?

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

Hi Weston,

The docs say for touch_with_version say:

# Mimics the `touch` method from `ActiveRecord::Persistence`, but also
# creates a version. A version is created regardless of options such as
# `:on`, `:if`, or `:unless`.

The :only option isn't specifically mentioned, but I think it's clear that the intention is to always create a version.

Here's what I'm seeing when I run your script:

has_paper_trail                                     # pass
has_paper_trail on: [:update]                       # pass
has_paper_trail only: [:first_name]                 # fail
has_paper_trail on: [:update], only: [:first_name]  # fail

It seems :only is the problem and :on is not relevant.

Please take it from here and prepare a PR with a fix and new tests, thanks.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

After investigating, the cause of this problem is in lib/paper_trail/record_trail.rb

This bug would effect all of the conditional options only, if, unless, skip, ignore

I tried skipping the unless will_record_on_update? but some tests failed because 2 version entries were being created when only 1 should be created. Looks like I would need to re-create the logic for all these options within will_record_on_update? otherwise it can possibly create 2 version entries.

I will continue to work on adding that logic within that method. However, please let me know if theres a better way I can go about this.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

https://github.com/airblade/paper_trail/blob/3960d7ff75aa3000cf55a42e4fcc39af5e073987/lib/paper_trail/record_trail.rb#L433-L434

Instead of recreating all the logic I thought it might be easier to just wrap the save call in a without_versioning block. See the following code.

record_update(true)
@record.paper_trail.without_versioning do
  @record.save!(validate: false)
end

This fixed everything and all tests passed except for one test regarding enum'sin Rails 5.1. Related to this PR https://github.com/airblade/paper_trail/pull/846/files#diff-b231cea2817ec877c717184b869a5f74
Would love it if anyone has some feedback fixing the enum bug using this new implementation.

@westonganger westonganger changed the title touch_with_version does not work with on & only option combined touch_with_version does not work with conditional options Mar 9, 2018

@jaredbeck jaredbeck closed this in bd52be1 Mar 11, 2018

@jaredbeck

This comment has been minimized.

Copy link
Member

commented Mar 11, 2018

Fixed by #1053, will release in 9.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.