paper-trail-gem / paper_trail

Track changes to your rails models
MIT License
6.8k stars 898 forks source link

When using papertrail with mock models papertrail takes the wrong `item_type` #1474

Open domingo2000 opened 6 months ago

domingo2000 commented 6 months ago

Check the following boxes:

This issue is about using paper_trail with rails and good migrations, so the template does not apply for this case, but i will put as much information as possible to reproduce the issue.

The bug is the following. When running a migration with good migrations the application does not autoload all the models. So a mock class needs to be created if it's necessary to use the model in the migration.

The problem is that when given a mock class in the migration, PaperTrail instead of taking class Foo it takes the class BackfillFooNames::Foo and the item_type takes the value item_type: BackfillFooNames::Foo instead of item_type: Foo. This is problematic because then the versions created by papertrail during the migrations are inconsistent with the ones in the application and cannot be used.

Here is the information and code to reproduce:

Migration code:

class BackfillFooNames < ActiveRecord::Migration[6.1]

  class Foo < ApplicationRecord
    self.table_name = "foos"
    has_paper_trail meta: { item_type: 'Foo' }
  end

  def up
    foo = Foo.first
    foo.bar = 'baz'
    foo.save!
  end

  def down
    foo = Foo.first
    foo.bar = nil
    foo.save!
  end
end

Our solution in papertrail < 14.0.0 is to also mock the item_type using the meta option as shown in the following code:

class BackfillFooNames < ActiveRecord::Migration[6.1]

  class Foo < ApplicationRecord
    self.table_name = "foos"
    has_paper_trail meta: { item_type: 'Foo' }
  end

  def up
    foo = Foo.first
    foo.bar = 'baz'
    foo.save!
  end

  def down
    foo = Foo.first
    foo.bar = nil
    foo.save!
  end
end

The problem is that in PaperTrail 14 introduced a breaking change that makes that meta attribute forbidden, so now the code above can not run in versions >= 14.0.0

Here is more data to reproduce the problem

Papertrail version: 15.1.0 Rails version: 6.1.7.7

Gemfile:

source 'https://rubygems.org'
git_source(:github) { |repo| "https://github.com/#{repo}.git" }

ruby '3.2.2'

gem 'rails', '~> 6.1.7', '>= 6.1.7.7'
gem 'sqlite3', '~> 1.4'
gem 'puma', '~> 5.0'
gem 'sass-rails', '>= 6'
gem 'webpacker', '~> 5.0'
gem 'turbolinks', '~> 5'
gem 'jbuilder', '~> 2.7'
gem 'bootsnap', '>= 1.4.4', require: false

group :development, :test do
  gem 'byebug', platforms: [:mri, :mingw, :x64_mingw]
end

group :development do
  gem 'web-console', '>= 4.1.0'
  gem 'rack-mini-profiler', '~> 2.0'
  gem 'listen', '~> 3.3'
  gem 'spring'
end

group :test do
  gem 'capybara', '>= 3.26'
  gem 'selenium-webdriver', '>= 4.0.0.rc1'
  gem 'webdrivers'
end

gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]

gem 'paper_trail', '~> 15.1.0'

Gemfile.lock dependencies:

DEPENDENCIES
  bootsnap (>= 1.4.4)
  byebug
  capybara (>= 3.26)
  jbuilder (~> 2.7)
  listen (~> 3.3)
  paper_trail (~> 15.1.0)
  puma (~> 5.0)
  rack-mini-profiler (~> 2.0)
  rails (~> 6.1.7, >= 6.1.7.7)
  sass-rails (>= 6)
  selenium-webdriver (>= 4.0.0.rc1)
  spring
  sqlite3 (~> 1.4)
  turbolinks (~> 5)
  tzinfo-data
  web-console (>= 4.1.0)
  webdrivers
  webpacker (~> 5.0)

As i see maybe you could rollback the forbid of has_paper_trail meta: { item_type: 'Foo' } at the own user risk and document that is risky to change that parameter as was before. Or maybe you have a better solution passing anothe parameter to the has_papertrail that could be used in the mock classes.

Im willing to open a PR to fix this issue.

MarcelEeken commented 5 months ago

We are running into a similar situation where we have a weird setup with a shared module that defines logic between two ActiveRecord classes that write to the same database table and are treated as the same entity.

We want to be able to track the history as a single entity, doesn't matter which of the two classes triggered the change. This looks like:

class Shared
  extend ActiveSupport::Concern

  included do
    self.table_name "shared_model"

    has_paper_trail(
      meta: {
        item_type: "SharedModel"
      }
    )
  end
end

class Foo
  include Shared
end

class Bar
  include Shared
end

After the upgrade to PaperTrail 14 this is now also throwing an error. And if we would remove the item_type setting it would result in the versions table having entries for Foo and Bar instead of SharedModel.

If possible we would also like to have an option to configure this somewhere.

github-actions[bot] commented 2 months ago

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.

domingo2000 commented 2 months ago

@jaredbeck is it possible to open again the has_paper_trail meta: { item_type: 'Foo' } for this use cases?. We still need this in the new Paper Trail versions.