Merge pull request #913 from airblade/various_2016-12-13

Continue improving complexity metrics

.rubocop_todo.yml

@@ -2,13 +2,13 @@
 # one by one as the offenses are removed from the code base.
 
 Metrics/AbcSize:
-  Max: 24 # Goal: 15
+  Max: 22 # Goal: 15
 
 Metrics/CyclomaticComplexity:
   Max: 8 # Goal: 6
 
 Metrics/PerceivedComplexity:
-  Max: 10 # Goal: 7
+  Max: 9 # Goal: 7
 
 Style/FrozenStringLiteralComment:
   Enabled: false

lib/paper_trail/model_config.rb

@@ -124,22 +124,12 @@ module PaperTrail
 
       @model_class.send :attr_accessor, :paper_trail_event
 
-      # In rails 4, the `has_many` syntax for specifying order uses a lambda.
+      @model_class.has_many(
-      if ::ActiveRecord::VERSION::MAJOR >= 4
+        @model_class.versions_association_name,
-        @model_class.has_many(
+        -> { order(model.timestamp_sort_order) },
-          @model_class.versions_association_name,
+        class_name: @model_class.version_class_name,
-          -> { order(model.timestamp_sort_order) },
+        as: :item
-          class_name: @model_class.version_class_name,
+      )
-          as: :item
-        )
-      else
-        @model_class.has_many(
-          @model_class.versions_association_name,
-          class_name: @model_class.version_class_name,
-          as: :item,
-          order: @model_class.paper_trail.version_class.timestamp_sort_order
-        )
-      end
     end
 
     # Adds callbacks to record changes to habtm associations such that on save

lib/paper_trail/record_trail.rb

@@ -118,32 +118,49 @@ module PaperTrail
       source_version.nil?
     end
 
+    # Updates `data` from the model's `meta` option and from `controller_info`.
     # @api private
-    def merge_metadata(data)
+    def merge_metadata_into(data)
-      # First we merge the model-level metadata in `meta`.
+      merge_metadata_from_model_into(data)
-      @record.paper_trail_options[:meta].each do |k, v|
+      merge_metadata_from_controller_into(data)
-        data[k] =
+    end
-          if v.respond_to?(:call)
-            v.call(@record)
-          elsif v.is_a?(Symbol) && @record.respond_to?(v, true)
-            # If it is an attribute that is changing in an existing object,
-            # be sure to grab the current version.
-            if @record.has_attribute?(v) &&
-                attribute_changed_in_latest_version?(v) &&
-                data[:event] != "create"
-              attribute_in_previous_version(v)
-            else
-              @record.send(v)
-            end
-          else
-            v
-          end
-      end
 
-      # Second we merge any extra data from the controller (if available).
+    # Updates `data` from `controller_info`.
+    # @api private
+    def merge_metadata_from_controller_into(data)
       data.merge(PaperTrail.controller_info || {})
     end
 
+    # Updates `data` from the model's `meta` option.
+    # @api private
+    def merge_metadata_from_model_into(data)
+      @record.paper_trail_options[:meta].each do |k, v|
+        data[k] = model_metadatum(v, data[:event])
+      end
+    end
+
+    # Given a `value` from the model's `meta` option, returns an object to be
+    # persisted. The `value` can be a simple scalar value, but it can also
+    # be a symbol that names a model method, or even a Proc.
+    # @api private
+    def model_metadatum(value, event)
+      if value.respond_to?(:call)
+        value.call(@record)
+      elsif value.is_a?(Symbol) && @record.respond_to?(value, true)
+        # If it is an attribute that is changing in an existing object,
+        # be sure to grab the current version.
+        if event != "create" &&
+            @record.has_attribute?(value) &&
+            attribute_changed_in_latest_version?(value)
+          attribute_in_previous_version(value)
+        else
+          @record.send(value)
+        end
+      else
+        value
+      end
+    end
+
     # Returns the object (not a Version) as it became next.
     # NOTE: if self (the item) was not reified from a version, i.e. it is the
     #  "live" item, we return nil.  Perhaps we should return self instead?
@@ -210,7 +227,7 @@ module PaperTrail
         data[:object_changes] = recordable_object_changes
       end
       add_transaction_id_to(data)
-      merge_metadata(data)
+      merge_metadata_into(data)
     end
 
     def record_destroy
@@ -238,7 +255,7 @@ module PaperTrail
         whodunnit: PaperTrail.whodunnit
       }
       add_transaction_id_to(data)
-      merge_metadata(data)
+      merge_metadata_into(data)
     end
 
     # Returns a boolean indicating whether to store serialized version diffs
@@ -280,7 +297,7 @@ module PaperTrail
         data[:object_changes] = recordable_object_changes
       end
       add_transaction_id_to(data)
-      merge_metadata(data)
+      merge_metadata_into(data)
     end
 
     # Returns an object which can be assigned to the `object` attribute of a
@@ -329,29 +346,15 @@ module PaperTrail
     # Saves associations if the join table for `VersionAssociation` exists.
     def save_associations(version)
       return unless PaperTrail.config.track_associations?
-      save_associations_belongs_to(version)
+      save_bt_associations(version)
-      save_associations_habtm(version)
+      save_habtm_associations(version)
     end
 
-    def save_associations_belongs_to(version)
+    # Save all `belongs_to` associations.
+    # @api private
+    def save_bt_associations(version)
       @record.class.reflect_on_all_associations(:belongs_to).each do |assoc|
-        assoc_version_args = {
+        save_bt_association(assoc, version)
-          version_id: version.id,
-          foreign_key_name: assoc.foreign_key
-        }
-
-        if assoc.options[:polymorphic]
-          associated_record = @record.send(assoc.name) if @record.send(assoc.foreign_type)
-          if associated_record && associated_record.class.paper_trail.enabled?
-            assoc_version_args[:foreign_key_id] = associated_record.id
-          end
-        elsif assoc.klass.paper_trail.enabled?
-          assoc_version_args[:foreign_key_id] = @record.send(assoc.foreign_key)
-        end
-
-        if assoc_version_args.key?(:foreign_key_id)
-          PaperTrail::VersionAssociation.create(assoc_version_args)
-        end
       end
     end
 
@@ -359,7 +362,8 @@ module PaperTrail
     # HABTM associations looked like before any changes were made, by using
     # the `paper_trail_habtm` data structure. Then, we create
     # `VersionAssociation` records for each of the associated records.
-    def save_associations_habtm(version)
+    # @api private
+    def save_habtm_associations(version)
       @record.class.reflect_on_all_associations(:has_and_belongs_to_many).each do |a|
         next unless save_habtm_association?(a)
         habtm_assoc_ids(a).each do |id|
@@ -508,6 +512,28 @@ module PaperTrail
       )
     end
 
+    # Save a single `belongs_to` association.
+    # @api private
+    def save_bt_association(assoc, version)
+      assoc_version_args = {
+        version_id: version.id,
+        foreign_key_name: assoc.foreign_key
+      }
+
+      if assoc.options[:polymorphic]
+        associated_record = @record.send(assoc.name) if @record.send(assoc.foreign_type)
+        if associated_record && associated_record.class.paper_trail.enabled?
+          assoc_version_args[:foreign_key_id] = associated_record.id
+        end
+      elsif assoc.klass.paper_trail.enabled?
+        assoc_version_args[:foreign_key_id] = @record.send(assoc.foreign_key)
+      end
+
+      if assoc_version_args.key?(:foreign_key_id)
+        PaperTrail::VersionAssociation.create(assoc_version_args)
+      end
+    end
+
     # Returns true if the given HABTM association should be saved.
     # @api private
     def save_habtm_association?(assoc)

lib/paper_trail/reifier.rb

@@ -117,23 +117,12 @@ module PaperTrail
       end
 
       # @api private
-      def hmt_collection_through_belongs_to(through_collection, assoc, options, transaction_id)
+      def hmt_collection_through_belongs_to(through_collection, assoc, options, tx_id)
-        collection_keys = through_collection.map { |through_model|
+        ids = through_collection.map { |through_model|
           through_model.send(assoc.source_reflection.foreign_key)
         }
-        version_id_subquery = assoc.klass.paper_trail.version_class.
+        versions = load_versions_for_hmt_association(assoc, ids, tx_id, options[:version_at])
-          select("MIN(id)").
+        collection = Array.new assoc.klass.where(assoc.klass.primary_key => ids)
-          where("item_type = ?", assoc.class_name).
-          where("item_id IN (?)", collection_keys).
-          where(
-            "created_at >= ? OR transaction_id = ?",
-            options[:version_at],
-            transaction_id
-          ).
-          group("item_id").
-          to_sql
-        versions = versions_by_id(assoc.klass, version_id_subquery)
-        collection = Array.new assoc.klass.where(assoc.klass.primary_key => collection_keys)
         prepare_array_for_has_many(collection, options, versions)
         collection
       end
@@ -217,6 +206,25 @@ module PaperTrail
         versions_by_id(model.class, version_id_subquery)
       end
 
+      # Given a `has_many(through:)` association and an array of `ids`, return
+      # the version records from the point in time identified by `tx_id` or
+      # `version_at`.
+      # @api private
+      def load_versions_for_hmt_association(assoc, ids, tx_id, version_at)
+        version_id_subquery = assoc.klass.paper_trail.version_class.
+          select("MIN(id)").
+          where("item_type = ?", assoc.class_name).
+          where("item_id IN (?)", ids).
+          where(
+            "created_at >= ? OR transaction_id = ?",
+            version_at,
+            tx_id
+          ).
+          group("item_id").
+          to_sql
+        versions_by_id(assoc.klass, version_id_subquery)
+      end
+
       # Set all the attributes in this version on the model.
       def reify_attributes(model, version, attrs)
         enums = model.class.respond_to?(:defined_enums) ? model.class.defined_enums : {}