diff --git a/lib/paper_trail/events/create.rb b/lib/paper_trail/events/create.rb index 1b479e358..76c014b42 100644 --- a/lib/paper_trail/events/create.rb +++ b/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 } diff --git a/lib/paper_trail/events/destroy.rb b/lib/paper_trail/events/destroy.rb index 781771817..96d9ed2dd 100644 --- a/lib/paper_trail/events/destroy.rb +++ b/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 } diff --git a/lib/paper_trail/events/update.rb b/lib/paper_trail/events/update.rb index cfeb6d5fb..c2b59b6b7 100644 --- a/lib/paper_trail/events/update.rb +++ b/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 } diff --git a/lib/paper_trail/model_config.rb b/lib/paper_trail/model_config.rb index 6320f6d38..40adfc1ea 100644 --- a/lib/paper_trail/model_config.rb +++ b/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 diff --git a/lib/paper_trail/record_trail.rb b/lib/paper_trail/record_trail.rb index ee26316f5..4c0988c4f 100644 --- a/lib/paper_trail/record_trail.rb +++ b/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 diff --git a/spec/controllers/articles_controller_spec.rb b/spec/controllers/articles_controller_spec.rb index 1758e15e6..c524eee21 100644 --- a/spec/controllers/articles_controller_spec.rb +++ b/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 diff --git a/spec/dummy_app/app/models/management.rb b/spec/dummy_app/app/models/management.rb new file mode 100644 index 000000000..50fbd5cc5 --- /dev/null +++ b/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 diff --git a/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb b/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb index d102aa90a..869088970 100644 --- a/spec/dummy_app/db/migrate/20110208155312_set_up_test_tables.rb +++ b/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 diff --git a/spec/models/json_version_spec.rb b/spec/models/json_version_spec.rb index 750f4d543..42453dde0 100644 --- a/spec/models/json_version_spec.rb +++ b/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 diff --git a/spec/models/management_spec.rb b/spec/models/management_spec.rb new file mode 100644 index 000000000..ff7d51a37 --- /dev/null +++ b/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 diff --git a/spec/models/on/empty_array_spec.rb b/spec/models/on/empty_array_spec.rb index fbb6c83cb..b9236336f 100644 --- a/spec/models/on/empty_array_spec.rb +++ b/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 diff --git a/spec/models/pet_spec.rb b/spec/models/pet_spec.rb index 268d35d62..44d143db2 100644 --- a/spec/models/pet_spec.rb +++ b/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 diff --git a/spec/models/widget_spec.rb b/spec/models/widget_spec.rb index c13982629..df8fe5805 100644 --- a/spec/models/widget_spec.rb +++ b/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 diff --git a/spec/paper_trail/cleaner_spec.rb b/spec/paper_trail/cleaner_spec.rb index 46f7c9bef..78755bf96 100644 --- a/spec/paper_trail/cleaner_spec.rb +++ b/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 diff --git a/spec/paper_trail/model_spec.rb b/spec/paper_trail/model_spec.rb index cf1a8b295..9a17bba91 100644 --- a/spec/paper_trail/model_spec.rb +++ b/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