From bca9aa5edf140e0be5f0f095b3667189fb3b619a Mon Sep 17 00:00:00 2001 From: Phil Coggins Date: Tue, 13 Apr 2021 18:25:29 -0600 Subject: [PATCH] Removes item_subtype requirement for model-specific limits. * When enforcing limits for a model, it was previously required to have the item_subtype column on the versions table to obtain the configured limit for a given model type, especially for models subclassed via STI. * This change instead references the item on the version record, which is already available via the association cache, to determine the properly configured limit. This change drops the requirement of having item_subtype to configure model-specific limits. --- CHANGELOG.md | 3 ++- README.md | 4 ---- lib/paper_trail/model_config.rb | 16 ---------------- lib/paper_trail/reifier.rb | 2 -- lib/paper_trail/version_concern.rb | 22 ++++++++++++++-------- spec/paper_trail/config_spec.rb | 11 ----------- 6 files changed, 16 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d1c2f05..2b086661 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ This project follows [semver 2.0.0](http://semver.org/spec/v2.0.0.html) and the recommendations of [keepachangelog.com](http://keepachangelog.com/). ## Unreleased - +- [#1309](https://github.com/paper-trail-gem/paper_trail/pull/1309) - + Removes `item_subtype` requirement when specifying model-specific limits. ### Breaking Changes - None diff --git a/README.md b/README.md index 9fdc7a34..f7a1562b 100644 --- a/README.md +++ b/README.md @@ -613,10 +613,6 @@ has_paper_trail limit: 2 has_paper_trail limit: nil ``` -To use a per-model limit, your `versions` table must have an -`item_subtype` column. See [Section -4.b.1](https://github.com/paper-trail-gem/paper_trail#4b1-the-optional-item_subtype-column). - ## 3. Working With Versions ### 3.a. Reverting And Undeleting A Model diff --git a/lib/paper_trail/model_config.rb b/lib/paper_trail/model_config.rb index c7fc8b21..b976bd5b 100644 --- a/lib/paper_trail/model_config.rb +++ b/lib/paper_trail/model_config.rb @@ -18,11 +18,6 @@ module PaperTrail `abstract_class`. This is fine, but all application models must be configured to use concrete (not abstract) version models. STR - E_MODEL_LIMIT_REQUIRES_ITEM_SUBTYPE = <<~STR.squish.freeze - To use PaperTrail's per-model limit in your %s model, you must have an - item_subtype column in your versions table. See documentation sections - 2.e.1 Per-model limit, and 4.b.1 The optional item_subtype column. - STR DPR_PASSING_ASSOC_NAME_DIRECTLY_TO_VERSIONS_OPTION = <<~STR.squish Passing versions association name as `has_paper_trail versions: %{versions_name}` is deprecated. Use `has_paper_trail versions: {name: %{versions_name}}` instead. @@ -124,7 +119,6 @@ module PaperTrail @model_class.send :include, ::PaperTrail::Model::InstanceMethods setup_options(options) setup_associations(options) - check_presence_of_item_subtype_column(options) @model_class.after_rollback { paper_trail.clear_rolled_back_versions } setup_callbacks_from_options options[:on] end @@ -151,16 +145,6 @@ module PaperTrail ::ActiveRecord::Base.belongs_to_required_by_default end - # Some options require the presence of the `item_subtype` column. Currently - # only `limit`, but in the future there may be others. - # - # @api private - def check_presence_of_item_subtype_column(options) - return unless options.key?(:limit) - return if version_class.item_subtype_column_present? - raise Error, format(E_MODEL_LIMIT_REQUIRES_ITEM_SUBTYPE, @model_class.name) - end - def check_version_class_name(options) # @api private - `version_class_name` @model_class.class_attribute :version_class_name diff --git a/lib/paper_trail/reifier.rb b/lib/paper_trail/reifier.rb index fbb587fb..e7d192de 100644 --- a/lib/paper_trail/reifier.rb +++ b/lib/paper_trail/reifier.rb @@ -117,8 +117,6 @@ module PaperTrail # this method returns the constant `Dog`. If `attrs["species"]` is blank, # this method returns the constant `Animal`. You can see this particular # example in action in `spec/models/animal_spec.rb`. - # - # TODO: Duplication: similar `constantize` in VersionConcern#version_limit def version_reification_class(version, attrs) inheritance_column_name = version.item_type.constantize.inheritance_column inher_col_value = attrs[inheritance_column_name] diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 398cdac2..c1defe04 100644 --- a/lib/paper_trail/version_concern.rb +++ b/lib/paper_trail/version_concern.rb @@ -379,16 +379,22 @@ module PaperTrail # The version limit can be global or per-model. # # @api private - # - # TODO: Duplication: similar `constantize` in Reifier#version_reification_class def version_limit - if self.class.item_subtype_column_present? - klass = (item_subtype || item_type).constantize - if klass&.paper_trail_options&.key?(:limit) - return klass.paper_trail_options[:limit] - end + if limit_option?(item.class) + item.class.paper_trail_options[:limit] + elsif base_class_limit_option?(item.class) + item.class.base_class.paper_trail_options[:limit] + else + PaperTrail.config.version_limit end - PaperTrail.config.version_limit + end + + def limit_option?(klass) + klass.respond_to?(:paper_trail_options) && klass.paper_trail_options.key?(:limit) + end + + def base_class_limit_option?(klass) + klass.respond_to?(:base_class) && limit_option?(klass.base_class) end end end diff --git a/spec/paper_trail/config_spec.rb b/spec/paper_trail/config_spec.rb index f91d1ba7..c2e2eafa 100644 --- a/spec/paper_trail/config_spec.rb +++ b/spec/paper_trail/config_spec.rb @@ -52,17 +52,6 @@ module PaperTrail limited_bike.save assert_equal 2, limited_bike.versions.length end - - context "when item_subtype column is absent" do - it "uses global version_limit" do - PaperTrail.config.version_limit = 6 - names = PaperTrail::Version.column_names - ["item_subtype"] - allow(PaperTrail::Version).to receive(:column_names).and_return(names) - bike = LimitedBicycle.create!(name: "My Bike") # has_paper_trail limit: 3 - 10.times { bike.update(name: SecureRandom.hex(8)) } - assert_equal 7, bike.versions.length - end - end end end end