Add the ability to pass options to the has_many :versions association macro

[TylerRick]

Users can already customize some aspects of the has_many :versions association, including the association name (via :versions option) and the class name (via :class_name option).

This changeset allows other options, such as :extend, to be passed on to this has_many.

[jaredbeck]

Hi Tyler, Thanks for the contribution.

People who use PT-AT have a second has_many association.

has_many :versions, ..
has_many :version_associations, ..

We need to be clear about which association the options apply to.

So, instead of has_paper_trail(x: 'y'), I think we should nest them, like has_paper_trail(has_many_versions_options: { x: 'y' }.

Alternatively, we could allow people to disable the automatic definition of the association, allowing them to define the association themselves. Perhaps has_paper_trail(has_many_versions: false).

[TylerRick]

So, instead of has_paper_trail(x: 'y'), I think we should nest them, like has_paper_trail(has_many_versions_options: { x: 'y' }.

My initial thinking was that these options should be top-level to match the existing class_name option, but I'm okay with nesting those options like you suggest, since they will be less commonly used. I hope we can find a less verbose name though. I'll try to think of a few options...

Good point about has_many :version_associations. I wasn't aware of that.(https://github.com/westonganger/paper_trail-association_tracking doesn't even mention the has_many, only the version_associations table.) Hmm.

Shouldn't one be able to assume that any options you pass to has_paper_trail are for the core PaperTrail association though? In particular, the class_name option isn't prefixed like that (maybe because it was added before you became maintainer) and always refers to the PaperTrail::Version model or its replacement, not PaperTrail::VersionAssociation. There is no versions_association_class_name option (yet), but they are free to add one. My point is that maybe it's okay to let unqualified/unprefixed options refer to the main versions association.

has_paper_trail versions_options: { option: 'option', ... }

Maybe we don't even need the word "_options" in the top-level option, since that's the only thing it could be...

has_paper_trail has_many: { option: 'option', ... }

Hmm. But we have other options related to the has_many. Might be more confusing to add new similar options that are synonyms (versions is the has_many this is referring to). Can we just combine them all?

has_paper_trail versions: { option: 'option', ... }

This is what I'm leaning towards currently (and is what I just pushed up an implementation for).

There is an existing versions option already for association name, of course, but we could continue treat it as a name if it's a non-Hash and as a hash of any options that you want to override the defaults if it's a Hash.

This would continue to work:

  has_paper_trail versions: :paper_trail_versions

but this would also now be possible:

  has_paper_trail versions: { name: :paper_trail_versions }

and this:

  has_paper_trail versions: { extend: MyExtensions }

(see new tests in spec/paper_trail/model_config_spec.rb)

This has the benefit that most of the options for the versions association are neatly under a single key.

This is similar to how you can pass options to validations if you need to, or just a boolean if you don't:

validates :something, presence: true
validates :something, presence: {message: 'must be checked'}

---

Alternatively, we could allow people to disable the automatic definition of the association, allowing them to define the association themselves. Perhaps has_paper_trail(has_many_versions: false).

That's not a bad idea, either! I'd be okay with having this as an additional option, though I think the ability to pass in an options hash gets us 90% of the way there and should be more user-friendly for most.

The only case I can think of where the option path wouldn't be configurable enough is if you wanted to pass a block to the has_many (but really, you could just throw it in a module and use extend:), like in their example:

has_many :employees do
  def find_or_create_by_name(name)
    ...
  end
end

Anyway, please see what you think of the new version I pushed up.

[jaredbeck]

Shouldn't one be able to assume that any options you pass to has_paper_trail are for the core PaperTrail association though?

The PT-AT gem currently uses skip and save_changes. They may want to add their own options in the future.

has_paper_trail versions: { option: 'option', ... }

There is an existing versions option already for association name, of course, but we could continue treat it as a name if it's a non-Hash and as a hash of any options that you want to override the defaults if it's a Hash.

Sounds good. Let's deprecate the old string form though, with a helpful message telling people to use the new form has_paper_trail versions: { name: :bananas } instead.

This has the benefit that most of the options for the versions association are neatly under a single key.

Yeah, we want to be consistent. Which options would we be missing? class_name? I'd be OK with moving that under the versions: option and deprecating the top-level class_name: option.

Alternatively, we could allow people to disable the automatic definition of the association, allowing them to define the association themselves. Perhaps has_paper_trail(has_many_versions: false).

That's not a bad idea, either!

Cool. Let's tackle it separately though. There's already a lot going on in this PR.

[TylerRick]

Yeah, we want to be consistent. Which options would we be missing? class_name? I'd be OK with moving that under the versions: option and deprecating the top-level class_name: option.

Yeah, I believe that's the only other one besides name.

Sounds good. I'll follow through and push up those other changes next week.

Cool. Let's tackle it separately though. There's already a lot going on in this PR.

Agreed.

[TylerRick]

@jaredbeck Just realized we have a version: option too, for setting version_association_name (usually :version).

Would we want to change that to follow the same version: {name: :version} convention that we're proposing using for versions: {name: :versions}?

Or should we rationalize that inconsistency with the fact that the version "association" isn't really a real association like versions is, so it probably won't ever need to have other options besides a name... or might it conceivably have other options down the road?

[jaredbeck]

@jaredbeck Just realized we have a version: option too, for setting version_association_name (usually :version). Would we want to change that to follow the same version: {name: :version} convention that we're proposing ..

Good question. I've never understood the purpose of that feature.

version = # ...
record = version.reify
# we have local variable `version`, why do we need `record.version`?

So, I'd like to deprecate that feature, actually. But, let's discuss it in a separate issue, please.

[jaredbeck]

This is getting very close!

[TylerRick]

Good question. I've never understood the purpose of that feature.

version = # ...
record = version.reify
# we have local variable `version`, why do we need `record.version`?

So, I'd like to deprecate that feature, actually. But, let's discuss it in a separate issue, please.

Sounds good. I don't recall ever using :version option myself.

[TylerRick]

All right, just pushed up the deprecation stuff. Hopefully it's done now!

[TylerRick]

Thought of another question: If we are going to deprecate the top-level class_name: option, do we only need to detect and show a warning if they try to set that configuration option, or also detect and show a warning if they try to read that configuration option? If we need it for reading too, that will be harder so I'm hoping not...

Ironically, that might have been easier to accomplish prior to changing this from a public to private API:

@model_class.class_attribute :version_class_name

because now I guess folks have to go through paper_trail_options hash as that is the public API.

(Reading nested hashes is not an extremely user friendly API, but we'll at least initialize paper_trail_options[:versions] so it's always a Hash and they don't have to worry about that...)

Looks like RailsAdmin is no longer using that private API, by the way, so we can remove the comment about that (I just did). Now they're using paper_trail_options[:class_name] instead to get that class name:

        def version_class_for(model)
          model.paper_trail_options[:class_name].try(:constantize) || @version_class
        end

My concern is that someone like RailsAdmin could upgrade, not see any deprecation warning, and continue to reference paper_trail_options[:class_name] forever... which would always be nil now, and not realize they need to change that to paper_trail_options[:versions][:class_name]

The code/project that sets that option and sees the deprecation warning and updates their code is going to be individual Rails applications that use those engines/libraries, not necessarily the libraries themselves like RailsAdmin. They could catch it if they had a more comprehensive test suite but the RailsAdmin test suite does not specifically have a test for that option currently (paper_trail_test.rb).

I guess we just rely on people to read the Changelog and notice this change (and if we're nice, we submit a PR to https://github.com/sferik/rails_admin/pulls :))... because I can't think of a nice way to detect that someone is relying on paper_trail_options[:class_name] — without building our own Hash class and overriding [] ... but that seems like overkill to me, so I'm inclined to call this good enough. What do you think?

[jaredbeck]

I can't think of a nice way to detect that someone is relying on paper_trail_options[:class_name] — without building our own Hash class and overriding []

If someone wants to do this in a separate PR, that's fine with me.

if we're nice, we submit a PR to https://github.com/sferik/rails_admin/pulls :)

👍 they should also use their gemspec to constrain PT major versions, if they aren't already

[jaredbeck]

Thanks for your work on this, Tyler. I especially appreciate how thorough your tests are.