Skip to content

Stabilize the build 💚 #37

Closed
chubchenko wants to merge 11 commits intowestonganger:masterfrom
chubchenko:bugfix/stabilize-build
Closed

Stabilize the build 💚 #37
chubchenko wants to merge 11 commits intowestonganger:masterfrom
chubchenko:bugfix/stabilize-build

Conversation

@chubchenko
Copy link
Copy Markdown

  • I have removed Ruby 2.5 from the build matrix due to min the Ruby version for paper_trail is 2.6
  • Use paper_trail >= 12.2 due to this fix Rails 7.0 Compatibility paper-trail-gem/paper_trail#1365
  • Use #to_prepare (of PaperTrailAssociationTracking::VersionConcern) instead of include
  • Initialize paper_trail_association_tracking after paper_trail
  • Re-write setup_habtm_change_callbacks method to make it compatible with all versions of rails (due to this fix rails/rails@bfcac13)

Sorry I haven't tested this in my application and probably and I won't be able to do it in the near future due to the war in Ukraine 🇺🇦

Feel free to add all needed changes

@chubchenko chubchenko marked this pull request as ready for review March 8, 2022 21:14
@KevinBongart
Copy link
Copy Markdown

@chubchenko This is a great PR, thank you! I'm new to this repo but PTAT is currently preventing me from upgrading to Rails 7 so I was looking into this specific issue.

@westonganger has a WIP PR (#36) which overlaps a lot with this one. In case this helps getting either PR merged, here are two key differences:

# lib/paper_trail-association_tracking.rb
 
 if defined?(Rails)
     require "paper_trail/frameworks/active_record"
     require "paper_trail_association_tracking/frameworks/rails"
+ elsif defined?(ActiveRecord)
+    require "paper_trail/frameworks/active_record"
+    require "paper_trail_association_tracking/frameworks/active_record"

Any idea why is needed? @chubchenko's PR doesn't add these 3 lines: is the test suite supposed to catch an issue or are these lines superfluous?

In lib/paper_trail_association_tracking/model_config.rb (https://github.com/westonganger/paper_trail-association_tracking/pull/36/files#diff-7be495c9ecb9ce90475f9244dc6615b08ea7c6db7d003fab7847bb3819b764aaR45), the callbacks are set up differently depending on the ActiveRecord version:

         if ActiveRecord::VERSION::MAJOR >= 7
           callback = lambda do |*args|
             update_habtm_state(assoc_name, :"before_#{verb}", args[-2], args.last)
           end
         )

It seems like the way @chubchenko rewrote setup_habtm_change_callbacks in this PR should make it work with Rails 7 apps and below. Could anyone confirm whether this approach means if ActiveRecord::VERSION::MAJOR >= 7 is not needed?

@chubchenko
Copy link
Copy Markdown
Author

chubchenko commented Mar 14, 2022

@KevinBongart Good catch 👍.
From what I understand this difference becomes due to changes in the paper_trail gem. paper_trail is able to work with the classic (default) rails application and with the application which includes only active_record (non-rails). Probably we should have the same behavior, but the gem does not have tests for that. I will check what I can do.

Regarding callbacks, actually, it stays the same:

Before

# verb => `add` and `remove`
lambda do |*args|
  update_habtm_state(assoc_name, :"before_#{verb}", args[-2], args.last)
end

After

before_add_callback = lambda do |*args|
  update_habtm_state(association_name, :before_add, args[-2], args.last)
end

before_remove_callback = lambda do |*args|
  update_habtm_state(association_name, :before_remove, args[-2], args.last)
end

Not so long ago I have tried the same solution as in the @westonganger PR but my build was failed because of those lines

@model_class.send(
  assoc.macro, assoc_name, **assoc.options.merge("before_{verb}" => callback)
)

Those lines declare the same association one more time but with additional options (callback). In rails way it will be something like this:

class User < ApplicationRecord
  # defined by the application
  has_many :posts

  # defined by ptat
  has_many :posts, before_add: callback
end

To fix that problem I have found the ActiveRecord::Associations::Builder::CollectionAssociation class which is used as part of has_many/has_and_belongs_to_many to build an association, but I use only the define_callbacks method to define correspondent callbacks for ptat. And this API is used for all versions of rails, which is pretty good (we don't need any checks about the version of rails). The bad side of this solution is the lack of a public API (it may break in the next version of rails).

@KevinBongart
Copy link
Copy Markdown

This is great, thank you so much for the detailed explanation! I'll take a look at adding tests for a Rails-less ActiveRecord app. In the meantime, I've created this PR for updating the appraisal config: chubchenko#4

@westonganger
Copy link
Copy Markdown
Owner

westonganger commented Mar 15, 2022

@chubchenko thank you so much for your explanation of the build issue #37 (comment)

I have decided to go with my existing PR (#36) because of how I had already structured the test suite. Sorry I did not want to go through curating all those changes once again.

Once again big thanks to @chubchenko for your stellar work here! The community will be forever in your debt.

@chubchenko chubchenko closed this Mar 15, 2022
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

Successfully merging this pull request may close these issues.

3 participants