Make STI .joins() fix more consistent with existing tests

lib/paper_trail/events/create.rb

@@ -13,8 +13,6 @@ class Create < Base
       # @api private
       def data
         data = {
-          item_id: @record.id,
-          item_type: @record.class.paper_trail.versions_association_item_type,
           event: @record.paper_trail_event || "create",
           whodunnit: PaperTrail.request.whodunnit
         }

lib/paper_trail/events/destroy.rb

@@ -14,7 +14,12 @@ class Destroy < Base
       def data
         data = {
           item_id: @record.id,
-          item_type: @record.class.paper_trail.versions_association_item_type,
+          item_type:
+            if @record.respond_to?(@record.class.inheritance_column)
+              @record.class
+            else
+              @record.class.base_class
+            end.name,
           event: @record.paper_trail_event || "destroy",
           whodunnit: PaperTrail.request.whodunnit
         }

lib/paper_trail/events/update.rb

@@ -25,8 +25,6 @@ def initialize(record, in_after_callback, is_touch, force_changes)
       # @api private
       def data
         data = {
-          item_id: @record.id,
-          item_type: @record.class.paper_trail.versions_association_item_type,
           event: @record.paper_trail_event || "update",
           whodunnit: PaperTrail.request.whodunnit
         }

lib/paper_trail/model_config.rb

@@ -110,14 +110,6 @@ def version_class
       @_version_class ||= @model_class.version_class_name.constantize
     end
 
-    def versions_association_item_type
-      if @model_class.descends_from_active_record?
-        @model_class.base_class.name
-      else
-        @model_class.name
-      end
-    end
-
     private
 
     def active_record_gem_version
@@ -147,20 +139,40 @@ def cannot_record_after_destroy?
     # is "Dog". If `attrs["species"]` is blank, `item_type` is "Animal". See
     # `spec/models/animal_spec.rb`.
     def setup_versions_association(klass)
-      klass.has_many(
+      has_manys = klass.has_many(
         klass.versions_association_name,
         lambda do
           relation = order(model.timestamp_sort_order)
-          item_type = klass.paper_trail.send(:versions_association_item_type)
-          relation = relation.unscope(where: :item_type).where(item_type: item_type)
+          # Unless it's not a subclassed model
+          unless klass.descendants.any? &&
+              # Or an STI devoid of an `inheritance_column`
+              klass.columns.exclude?(klass.inheritance_column)
+            # Search for Versions based on the real class name
+            relation = relation.unscope(where: :item_type).where(item_type: klass.name)
+          end
           relation
         end,
         class_name: klass.version_class_name,
         as: :item
       )
 
-      # We override the assocation when STI models are created from a base class
-      # in order to use the right `item_type`.
+      # Only for this .versions association override HasManyAssociation#collection?
+      # (that normally always returns true) so it returns false when referring to
+      # a subclassed model that uses STI.  Allows .create() events not to revert
+      # back to base_class at the final stages when Association#creation_attributes
+      # gets called.
+      unless klass.descends_from_active_record?
+        has_manys[klass.versions_association_name.to_s].define_singleton_method(:collection?) do
+          false
+        end
+      end
+
+      setup_versions_association_when_inheriting(klass)
+    end
+
+    def setup_versions_association_when_inheriting(klass)
+      # When STI models are created from another class, override the otherwise-inherited
+      # has_many :versions so it will use the right `item_type`.
       klass.singleton_class.prepend(Module.new {
         def inherited(klass)
           super

lib/paper_trail/record_trail.rb

@@ -84,7 +84,7 @@ def record_create
       # `data_for_create` but PT-AT still does.
       data = event.data.merge(data_for_create)
 
-      version = @record.class.paper_trail.version_class.new(data)
+      version = @record.send(@record.class.versions_association_name).new(data)
       version.save!
       version
     end
@@ -138,12 +138,7 @@ def record_update(force:, in_after_callback:, is_touch:)
       # `data_for_update` but PT-AT still does.
       data = event.data.merge(data_for_update)
 
-      version = @record.class.paper_trail.version_class.new(data)
-      if version.save
-        version
-      else
-        log_version_errors(version, :update)
-      end
+      update_record(data)
     end
 
     # PT-AT extends this method to add its transaction id.
@@ -164,11 +159,25 @@ def record_update_columns(changes)
       # `data_for_update_columns` but PT-AT still does.
       data = event.data.merge(data_for_update_columns)
 
-      version = @record.class.paper_trail.version_class.new(data)
-      if version.save
-        version
+      update_record(data)
+    end
+
+    # Used by both #record_update and #record_update_columns
+    def update_record(data)
+      versions_assoc = @record.send(@record.class.versions_association_name)
+      if @record.respond_to?(@record.class.inheritance_column)
+        # Version item_type will be the real class name
+        version = versions_assoc.new(data)
+        version.save
       else
+        # Version item_type will be the base_class name
+        version = versions_assoc.create(data)
+      end
+
+      if version.errors.any?
         log_version_errors(version, :update)
+      else
+        version
       end
     end
 

spec/controllers/articles_controller_spec.rb

@@ -12,7 +12,7 @@
         post :create, params_wrapper(article: { title: "Doh", content: FFaker::Lorem.sentence })
         expect(assigns(:article)).not_to be_nil
         expect(PaperTrail.request.enabled?).to eq(true)
-        expect(assigns(:article).versions.count).to eq(1)
+        expect(assigns(:article).versions.length).to eq(1)
       end
 
       after { PaperTrail.enabled = false }
@@ -23,7 +23,7 @@
         expect(PaperTrail.enabled?).to eq(false)
         post :create, params_wrapper(article: { title: "Doh", content: FFaker::Lorem.sentence })
         expect(PaperTrail.request.enabled?).to eq(false)
-        expect(assigns(:article).versions.count).to eq(0)
+        expect(assigns(:article).versions.length).to eq(0)
       end
     end
   end

spec/dummy_app/app/models/management.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+# Note that there is no `type` column for this subclassed model, so changes to Management objects
+# should result in Versions which have an item_type of Customer.
+class Management < Customer
+end

spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb

@@ -297,6 +297,7 @@ def up
     end
 
     create_table :callback_modifiers, force: true do |t|
+      t.string  :type
       t.string  :some_content
       t.boolean :deleted, default: false
     end

spec/models/json_version_spec.rb

@@ -33,11 +33,11 @@
       context "valid arguments", versioning: true do
         it "locates versions according to their `object` contents" do
           fruit = Fruit.create!(name: "apple")
-          expect(fruit.versions.count).to eq(1)
+          expect(fruit.versions.length).to eq(1)
           fruit.update_attributes!(name: "banana", color: "aqua")
-          expect(fruit.versions.count).to eq(2)
+          expect(fruit.versions.length).to eq(2)
           fruit.update_attributes!(name: "coconut", color: "black")
-          expect(fruit.versions.count).to eq(3)
+          expect(fruit.versions.length).to eq(3)
           where_apple = described_class.where_object(name: "apple")
           expect(where_apple.to_sql).to eq(
             <<-SQL.squish

spec/models/management_spec.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Management, type: :model, versioning: true do
+  it { is_expected.to be_versioned }
+
+  it "utilises the base_class for STI classes having no type column" do
+    # Specific to the store_base_sti_class gem:
+    if ActiveRecord::Base.respond_to?(:store_base_sti_class) &&
+        !ActiveRecord::Base.store_base_sti_class
+      skip("When using store_base_sti_class it is necessary to implement a type column")
+    end
+
+    expect(Management.inheritance_column).to eq("type")
+    expect(Management.columns.map(&:name)).not_to include("type")
+
+    # Create, update, and destroy a Management and a Customer
+    customer1 = Customer.create(name: "Cust 1")
+    customer2 = Management.create(name: "Cust 2")
+    customer1.update(name: "Cust 1a")
+    customer2.update(name: "Cust 2a")
+    customer1.destroy
+    customer2.destroy
+
+    # All versions end up with an `item_type` of Customer
+    cust_pts = PaperTrail::Version.where(item_type: "Customer")
+    mgmt_pts = PaperTrail::Version.where(item_type: "Management")
+    expect(cust_pts.count).to eq(6)
+    expect(mgmt_pts.count).to eq(0)
+  end
+end

spec/models/on/empty_array_spec.rb

@@ -15,9 +15,9 @@ module On
     describe ".paper_trail.update_columns" do
       it "creates a version record" do
         widget = Widget.create
-        assert_equal 1, widget.versions.count
+        assert_equal 1, widget.versions.length
         widget.paper_trail.update_columns(name: "Bugle")
-        assert_equal 2, widget.versions.count
+        assert_equal 2, widget.versions.length
       end
     end
 

spec/models/pet_spec.rb

@@ -82,6 +82,10 @@
       expect(versions.third.reify).to be_a(Cat) # Cheshire
       expect(versions.fourth.reify).to be_a(Cat) # Cheshire that was destroyed
 
+      # Properly records "destroy" events according to Issue #37 from the store_base_sti_class gem:
+      #   https://github.com/appfolio/store_base_sti_class/issues/37
+      expect(cat.versions.last.event).to eq("destroy")
+
       # Creating an object from the base class is correctly identified as "Animal"
       expect(versions[5].reify).to be_an(Animal) # Muppets Drummer
       expect(versions[6].reify).to be_an(Animal) # Animal that was destroyed
@@ -112,7 +116,7 @@
 
     # After creating a bunch of records above, we change the inheritance_column
     # so that we can demonstrate passing hints to the migration generator.
-    xcontext "simulate a historical change to inheritance_column" do
+    context "simulate a historical change to inheritance_column" do
       before do
         Animal.inheritance_column = "species_xyz"
       end

spec/models/widget_spec.rb

@@ -249,9 +249,9 @@
   describe "#update", versioning: true do
     it "creates a version record" do
       widget = Widget.create
-      assert_equal 1, widget.versions.count
+      assert_equal 1, widget.versions.length
       widget.update_attributes(name: "Bugle")
-      assert_equal 2, widget.versions.count
+      assert_equal 2, widget.versions.length
     end
   end
 end

spec/paper_trail/cleaner_spec.rb

@@ -54,11 +54,11 @@ module PaperTrail
           date = animal.versions.first.created_at.to_date
           animal.update_attribute(:name, FFaker::Name.name)
           expect(PaperTrail::Version.count).to(eq(10))
-          expect(animal.versions.count).to(eq(4))
+          expect(animal.versions.size).to(eq(4))
           expect(animal.paper_trail.versions_between(date, (date + 1.day)).size).to(eq(3))
           PaperTrail.clean_versions!(date: date)
           expect(PaperTrail::Version.count).to(eq(8))
-          expect(animal.versions.count).to(eq(2))
+          expect(animal.versions.reload.size).to(eq(2))
           expect(animal.versions.first.created_at.to_date).to(eq(date))
           # Why use `equal?` here instead of something less strict?
           # Doesn't `to_date` always produce a new date object?
@@ -100,7 +100,7 @@ module PaperTrail
             date = animal.versions.first.created_at.to_date
             expect(PaperTrail::Version.count).to(eq(11))
             [animal, dog].each do |animal|
-              expect(animal.versions.count).to(eq(4))
+              expect(animal.versions.size).to(eq(4))
               expect(animal.versions.between(date, (date + 1.day)).size).to(eq(3))
             end
           end

spec/paper_trail/model_spec.rb

@@ -416,7 +416,7 @@
           before { @widget.update_attributes(name: "Ford") }
 
           it "add to its trail" do
-            expect(@widget.versions.count).to(eq((@count + 1)))
+            expect(@widget.versions.length).to(eq((@count + 1)))
           end
         end
       end