Update trackable association to optional

[smridge]

Currently, this gem does not make it explicitly clear whether the trackable association should be required or not. When upgrading a Rails application and start enabling new defaults one by one, the Rails.application.config.active_record.belongs_to_required_by_default = true completely breaks without this change. It's difficult to know when a codebase suddenly decides to start enabling new configurations. For example, you can be on Rails 7 and have this set to false or have it not enabled if your config/application.rb still has framework defaults set to say Rails 5.0.

• One example of a gem that sets polymorphic associations to optional

Overall, by the gem not having this specified, the intent is questionable and I'd say it's a BREAKING CHANGE when upgrading your application/enabling different configurations. This PR makes the intent clear and the changes more backwards compatible. I understand it can also break people that want it required. However, the gem should express explicit intent.

[smridge]

@ur5us / @pokonski curious on your thoughts for this...

[ur5us]

@smridge I’m in 2 minds about this change. I totally get your point about changing config.active_record.belongs_to_required_by_default = false|true potentially having a huge impact as you described. In the upgrade scenario, I can see how your proposed change to make trackable, optional: true could also result in breaking behavior for some apps that rely on belongs_to_required_by_default = true to cause validation errors if trackable hasn’t been set.

As the current behavior to rely on app wide settings has been in place for a while accepting this PR would warrant a major version release IMO. However, I don’t think that’d be a good default in the first place. I’d be keen to learn more about your use case though as not setting trackable seems to go against the intention of this gem. Moreover, I think it’d definitely be worthwhile to document that behavior in more detail and describe how to get around it, e.g. use an initializer to re-open the class and redefine the belongs_to parameters.

Long term, I think that https://github.com/public-activity/public_activity/pull/154 would be a better solution overall, I just haven’t had the time to rebase that longstanding PR yet. Thoughts welcome.

[smridge]

I definitely agree it's a BREAKING CHANGE either way. Right now, it's a BREAKING CHANGE depending on your app configuration and not the gem setup. I do strongly feel the gem should provide explicit intent, optional: true or optional: false - which one is it? I did see other issues and likes related to this when it came to having default scopes such as https://github.com/public-activity/public_activity/issues/248 . With polymorphic associations, not all models will have the same scopes. In the case of the opened issue, let's say foo model has soft deleting but bar does not. Have fun working with that. Therefore, my vote is to say trackable is optional: true since it's rather impossible for the gem to know how your default scopes are working. Perhaps that is a reason why the ahoy gem made visitable (can be compared to trackable) optional here.

Separately, for my specific use case, I was not able to override/monkey patch for the life of me. Here are the things I tried:

attempt one

# lib/public_activity/orm/active_record/activity.rb

module PublicActivity
  module ORM
    module ActiveRecord
      class Activity < ::ActiveRecord::Base
        belongs_to :trackable, polymorphic: true, optional: true
      end
    end
  end
end

attempt two

# app/models/activity.rb (model already in app)

class Activity < PublicActivity::Activity
  belongs_to :trackable, polymorphic: true, optional: true
  ...
end

attempt three

# config/initializers/public_activity.rb

PublicActivity::Activity.class_eval do
  belongs_to :trackable, polymorphic: true, optional: true
end

I also tried a require: false on installing the gem and no dice there, many things broke. I'm pretty sure I shotgun debugged a few other ways, but those are the ones I can recall.

[ur5us]

@smridge

I do strongly feel the gem should provide explicit intent, optional: true or optional: false - which one is it?

Neither. The 3rd option is “depending on your app configuration” which is a valid option IMO.

With polymorphic associations, not all models will have the same scopes. In the case of the opened issue, let's say foo model has soft deleting but bar does not. Have fun working with that.

What you want to happen in this case will be application specific.

Therefore, my vote is to say trackable is optional: true since it's rather impossible for the gem to know how your default scopes are working.

Point taken.

Perhaps that is a reason why the ahoy gem made visitable (can be compared to trackable) optional here.

I haven’t looked into their history for making this choice so I’m not qualified to comment, yet.

Separately, for my specific use case, I was not able to override/monkey patch for the life of me.

https://github.com/public-activity/public_activity/issues/248 mentions a working solution similar to yours.

Personally, I use the 3rd variant in one of the apps I maintain. Here’s the code copied verbatim from said app:

# frozen_string_literal: true

PublicActivity::Activity.class_eval do
  belongs_to :owner,
             -> { unscope(where: :active) },
             polymorphic: true,
             optional: true

  def parameter_for(*args)
    parameters
      .with_indifferent_access
      .fetch(*args)
      .then { |value| value.is_a?(Array) ? value[1] : value }
  end

  def updated_attributes elements = %w[updated_at]
    parameters.without(*elements)
  end
end

Do you use the spring gem by any chance?