Fixed issue with transitions using STI (#503)

Prior to 7.2.0 the code to unset the most_recent column of old transitions when a new transition was created relied on transitions_for_parent, which would build the correct query in the case that the transition class was using single table inheritance.

However, after #399, the code no longer uses the parent association to create the query and the current custom query lacks this functionality that existed prior to 7.2.0.

This PR addresses this issue by adding an AND clause to the query to filter by transition type if the transition class contains an inheritance column.

Author: pedro-pedrosa

lib/statesman/adapters/active_record.rb

@@ -158,13 +158,24 @@ def update_most_recents(most_recent_id = nil)
 
       def most_recent_transitions(most_recent_id = nil)
         if most_recent_id
-          transitions_of_parent.and(
+          concrete_transitions_of_parent.and(
             transition_table[:id].eq(most_recent_id).or(
               transition_table[:most_recent].eq(true),
             ),
           )
         else
-          transitions_of_parent.and(transition_table[:most_recent].eq(true))
+          concrete_transitions_of_parent.and(transition_table[:most_recent].eq(true))
+        end
+      end
+
+      def concrete_transitions_of_parent
+        if transition_sti?
+          transitions_of_parent.and(
+            transition_table[transition_class.inheritance_column].
+              eq(transition_class.name),
+          )
+        else
+          transitions_of_parent
         end
       end
 
@@ -263,13 +274,18 @@ def unique_indexes
           end
       end
 
-      def parent_join_foreign_key
-        association =
-          parent_model.class.
-            reflect_on_all_associations(:has_many).
-            find { |r| r.name.to_s == @association_name.to_s }
+      def transition_sti?
+        transition_class.column_names.include?(transition_class.inheritance_column)
+      end
 
-        association_join_primary_key(association)
+      def parent_association
+        parent_model.class.
+          reflect_on_all_associations(:has_many).
+          find { |r| r.name.to_s == @association_name.to_s }
+      end
+
+      def parent_join_foreign_key
+        association_join_primary_key(parent_association)
       end
 
       def association_join_primary_key(association)

spec/spec_helper.rb

@@ -48,6 +48,8 @@ def connection_failure
       my_namespace_my_active_record_model_transitions
       other_active_record_models
       other_active_record_model_transitions
+      sti_active_record_models
+      sti_active_record_model_transitions
     ]
     tables.each do |table_name|
       sql = "DROP TABLE IF EXISTS #{table_name};"
@@ -72,6 +74,15 @@ def prepare_other_transitions_table
       OtherActiveRecordModelTransition.reset_column_information
     end
 
+    def prepare_sti_model_table
+      CreateStiActiveRecordModelMigration.migrate(:up)
+    end
+
+    def prepare_sti_transitions_table
+      CreateStiActiveRecordModelTransitionMigration.migrate(:up)
+      StiActiveRecordModelTransition.reset_column_information
+    end
+
     MyNamespace::MyActiveRecordModelTransition.serialize(:metadata, JSON)
   end
 end

spec/statesman/adapters/active_record_spec.rb

@@ -12,6 +12,9 @@
 
     MyActiveRecordModelTransition.serialize(:metadata, JSON)
 
+    prepare_sti_model_table
+    prepare_sti_transitions_table
+
     Statesman.configure do
       # Rubocop requires described_class to be used, but this block
       # is instance_eval'd and described_class won't be defined
@@ -300,6 +303,57 @@
             from(true).to be_falsey
         end
       end
+
+      context "when transition uses STI" do
+        let(:sti_model) { StiActiveRecordModel.create }
+
+        let(:adapter_a) do
+          described_class.new(
+            StiAActiveRecordModelTransition,
+            sti_model,
+            observer,
+            { association_name: :sti_a_active_record_model_transitions },
+          )
+        end
+        let(:adapter_b) do
+          described_class.new(
+            StiBActiveRecordModelTransition,
+            sti_model,
+            observer,
+            { association_name: :sti_b_active_record_model_transitions },
+          )
+        end
+        let(:create) { adapter_a.create(from, to) }
+
+        context "with a previous unrelated transition" do
+          let!(:transition_b) { adapter_b.create(from, to) }
+
+          its(:most_recent) { is_expected.to eq(true) }
+
+          it "doesn't update the previous transition's most_recent flag" do
+            expect { create }.
+              to_not(change { transition_b.reload.most_recent })
+          end
+        end
+
+        context "with previous related and unrelated transitions" do
+          let!(:transition_a) { adapter_a.create(from, to) }
+          let!(:transition_b) { adapter_b.create(from, to) }
+
+          its(:most_recent) { is_expected.to eq(true) }
+
+          it "updates the previous transition's most_recent flag" do
+            expect { create }.
+              to change { transition_a.reload.most_recent }.
+              from(true).to be_falsey
+          end
+
+          it "doesn't update the previous unrelated transition's most_recent flag" do
+            expect { create }.
+              to_not(change { transition_b.reload.most_recent })
+          end
+        end
+      end
     end
   end
 

spec/support/active_record.rb

@@ -278,3 +278,94 @@ def change
     end
   end
 end
+
+class StiActiveRecordModel < ActiveRecord::Base
+  has_many :sti_a_active_record_model_transitions, autosave: false
+  has_many :sti_b_active_record_model_transitions, autosave: false
+
+  def state_machine_a
+    @state_machine_a ||= MyStateMachine.new(
+      self, transition_class: StiAActiveRecordModelTransition
+    )
+  end
+
+  def state_machine_b
+    @state_machine_b ||= MyStateMachine.new(
+      self, transition_class: StiBActiveRecordModelTransition
+    )
+  end
+
+  def metadata
+    super || {}
+  end
+
+  def reload(*)
+    state_machine_a.reset
+    state_machine_b.reset
+    super
+  end
+end
+
+class StiActiveRecordModelTransition < ActiveRecord::Base
+  include Statesman::Adapters::ActiveRecordTransition
+
+  belongs_to :sti_active_record_model
+  serialize :metadata, JSON
+end
+
+class StiAActiveRecordModelTransition < StiActiveRecordModelTransition
+end
+
+class StiBActiveRecordModelTransition < StiActiveRecordModelTransition
+end
+
+class CreateStiActiveRecordModelMigration < MIGRATION_CLASS
+  def change
+    create_table :sti_active_record_models do |t|
+      t.timestamps null: false
+    end
+  end
+end
+
+class CreateStiActiveRecordModelTransitionMigration < MIGRATION_CLASS
+  def change
+    create_table :sti_active_record_model_transitions do |t|
+      t.string  :to_state
+      t.integer :sti_active_record_model_id
+      t.integer :sort_key
+      t.string  :type
+
+      # MySQL doesn't allow default values on text fields
+      if ActiveRecord::Base.connection.adapter_name == "Mysql2"
+        t.text :metadata
+      else
+        t.text :metadata, default: "{}"
+      end
+
+      if Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
+        t.boolean :most_recent, default: true, null: false
+      else
+        t.boolean :most_recent, default: true
+      end
+
+      t.timestamps null: false
+    end
+
+    add_index :sti_active_record_model_transitions,
+              %i[type sti_active_record_model_id sort_key],
+              unique: true, name: "sti_sort_key_index"
+
+    if Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
+      add_index :sti_active_record_model_transitions,
+                %i[type sti_active_record_model_id most_recent],
+                unique: true,
+                where: "most_recent",
+                name: "index_sti_active_record_model_transitions_parent_latest"
+    else
+      add_index :sti_active_record_model_transitions,
+                %i[type sti_active_record_model_id most_recent],
+                unique: true,
+                name: "index_sti_active_record_model_transitions_parent_latest"
+    end
+  end
+end