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

Add an option to track changes to dynamic/virtual attributes (ActiveRecord 5.1+ behaviour) #1162

Closed
cblunt opened this issue Oct 25, 2018 · 3 comments

Comments

@cblunt
Copy link

cblunt commented Oct 25, 2018

With the changes introduced in ActiveRecord 5.1, PaperTrail isn't able to track the changes of dynamic (virtual) attributes. After some investigation, I eventually ran into this issue on Rails - tracking the previous value of dynamic attributes is no longer supported.

This is breaking behaviour for an app I am upgrading from Rails 4 to Rails 5.2. In this particular case, the app is using ActsAsTaggableOn - changes to the tag_list attribute are no longer tracked. Instead, the previous value is always nil.

This will apply to any dynamic attribute that we try to track through PaperTrail.

Article.create(title: "Hello World", tag_list: "hello")

article = Article.first
article.update(tag_list: "world")

article.versions.last.changeset
=> {"tag_list" => [nil, ["world"]]   # Prior to AR5.1, {"tag_list" => [["hello"], ["world"]]

Describe the solution you'd like to build

My idea to workaround this is to cache the value of tag_list in a real column (cached_tag_list) and then restore its value into tag_list if and when the version is reified. This would require the addition of a after_reify callback, and ideally, a new dynamic[_attributes] option on the has_paper_trail method to easily declare which attributes should be cached.

The default would be to cache values into a column named cached_{attribute_name} on the model.

Example Usage

class Article < ApplicationRecord
  acts_as_taggable_on # for demonstration purposes. AATO adds a dynamic `tag_list` attribute
  
  has_paper_trail dynamic: [:tag_list]
end

Article.create(title: "Hello World", tag_list: "hello")
article = Article.first
article.update(tag_list: "world")

article.versions.last.changeset
=> { cached_tag_list: [["hello"], ["world"]], tag_list: [nil, ["world"]] }

article.versions.last.reify
=> #<Article id: 1, title: "My Article", tag_list: ["hello"], cached_tag_list: "hello">

Describe alternatives you've considered

I considered using the existing meta data feature to cache the value of the dynamic attributes, but this seems to be populated during the after_save callback, meaning that the previous value of the attribute is not available (attribute_before_last_save(:tag_list) is nil)

Edit: I'd also considered capturing the value of the attributes during the before_save callback and storing them to be written to the PaperTrail::Version record. If PaperTrail could do this, it would be a more elegant solution than adding additional fields to the database, but I haven't dug into PaperTrail enough to know if it's viable.

Resources

@jaredbeck
Copy link
Member

Hi Chris,

.. I am upgrading from Rails 4 to Rails 5.2 ..

So, I expected the provided script to pass against rails 4, but it fails with

-{"tag_list"=>[["hello"], ["world"]]}
+{"title"=>[nil, "My Article"], "tag_list"=>["", ["hello"]], "id"=>[nil, 1]}

I used AR 4.2 and AATO 5.

gemfile(true) do
  ruby '2.5.1'
  source 'https://rubygems.org'
  gem 'activerecord', '~> 4.2'
  gem 'minitest', '5.11.3'
  gem 'paper_trail', '10.0.1', require: false
  gem 'sqlite3', '1.3.13'
  gem 'acts-as-taggable-on', '5.0.0'
end

What am I missing? Thanks.

@jaredbeck
Copy link
Member

Closing due to inactivity. To reopen, please provide the requested information, thanks!

@cblunt
Copy link
Author

cblunt commented Nov 3, 2018

Hey @jaredbeck

The upgrade is coming from Rails 4.2 and old versions of PaperTrail (4.0.0) and ActsAsTaggableOn (3.5.0). There seems to be a change in AATO 4 which shows the same behaviour.

I'll do a bit more digging into this to see what changed in AATO as well. I spent a while trying to decide if this was a PT or AATO issue, but another test using my own dynamic attribute (without AATO) did the same thing. I'll try recreating that as well to see if I can isolate the issue.

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

No branches or pull requests

2 participants