Handle composite foreign keys in positioning scope changes

lib/positioning.rb

@@ -35,7 +35,7 @@ def positioned(on: [], column: :position)
             reflection = reflections[scope_component]
 
             if reflection&.belongs_to?
-              positioning_columns[column][:scope_columns] << reflection.foreign_key
+              positioning_columns[column][:scope_columns].concat Array(reflection.foreign_key)
               positioning_columns[column][:scope_columns] << reflection.foreign_type if reflection.polymorphic?
               positioning_columns[column][:scope_associations] << reflection.name
             else

lib/positioning/mechanisms.rb

@@ -222,7 +222,7 @@ def positioning_scope_changed?
 
     def destroyed_via_positioning_scope?
       @positioned.destroyed_by_association && scope_columns.any? do |scope_column|
-        @positioned.destroyed_by_association.foreign_key == scope_column
+        Array(@positioned.destroyed_by_association.foreign_key).include?(scope_column)
       end
     end
   end

test/models/composite_foreign_key_item.rb

@@ -0,0 +1,5 @@
+class CompositeForeignKeyItem < ActiveRecord::Base
+  belongs_to :composite_primary_key_item, foreign_key: [:cpki_item_id, :cpki_account_id]
+
+  positioned on: :composite_primary_key_item
+end

test/models/composite_primary_key_item.rb

@@ -3,5 +3,7 @@ class CompositePrimaryKeyItem < ActiveRecord::Base
 
   belongs_to :list
 
+  has_many :composite_foreign_key_items, foreign_key: [:cpki_item_id, :cpki_account_id], dependent: :destroy
+
   positioned on: :list
 end

test/support/active_record.rb

@@ -55,6 +55,15 @@
 
     add_index :composite_primary_key_items, [:list_id, :position], unique: true
 
+    create_table :composite_foreign_key_items, force: true do |t|
+      t.string :name
+      t.integer :position, null: false
+      t.integer :cpki_item_id, null: false
+      t.integer :cpki_account_id, null: false
+    end
+
+    add_index :composite_foreign_key_items, [:cpki_item_id, :cpki_account_id, :position], unique: true, name: "index_cfki_on_scope_and_position"
+
     create_table :categories, force: true do |t|
       t.string :name
       t.integer :position, null: false

test/test_helper.rb

@@ -11,6 +11,7 @@
 require_relative "models/new_item"
 require_relative "models/default_scope_item"
 require_relative "models/composite_primary_key_item"
+require_relative "models/composite_foreign_key_item"
 require_relative "models/product"
 require_relative "models/category"
 require_relative "models/categorised_item"

test/test_positioning.rb

@@ -566,6 +566,19 @@ def test_destroyed_via_positioning_scope?
     list.destroy
     assert mechanisms.send(:destroyed_via_positioning_scope?)
   end
+
+  def test_destroyed_via_positioning_scope_with_composite_foreign_key
+    list = List.create(name: "List")
+    parent = CompositePrimaryKeyItem.create(item_id: 1, account_id: 1, list: list, name: "Parent")
+    child = parent.composite_foreign_key_items.create(name: "Child 1")
+    parent.composite_foreign_key_items.create(name: "Child 2")
+
+    mechanisms = Positioning::Mechanisms.new(child, :position)
+    refute mechanisms.send(:destroyed_via_positioning_scope?)
+
+    parent.destroy
+    assert mechanisms.send(:destroyed_via_positioning_scope?)
+  end
 end
 
 class TestPositioningScopes < Minitest::Test
@@ -606,6 +619,13 @@ def test_that_position_columns_will_cope_with_polymorphic_belong_to
     assert_equal({position: {scope_columns: ["includable_id", "includable_type"], scope_associations: [:includable]}}, Entity.positioning_columns)
   end
 
+  def test_that_position_columns_will_cope_with_composite_foreign_key
+    assert_equal(
+      {position: {scope_columns: ["cpki_item_id", "cpki_account_id"], scope_associations: [:composite_primary_key_item]}},
+      CompositeForeignKeyItem.positioning_columns
+    )
+  end
+
   def test_that_position_columns_must_have_unique_keys
     assert_raises(Positioning::Error) do
       Item.send :positioned, on: :list
@@ -1135,8 +1155,6 @@ def test_destroying_multiple_items
 
 class TestCompositePrimaryKeyPositioning < TestPositioning
   def configure
-    skip if ActiveRecord.version < Gem::Version.new("7.1.0")
-
     @association = :composite_primary_key_items
     @id = Enumerator.new do |yielder|
       number = 1
@@ -1149,6 +1167,137 @@ def configure
   end
 end
 
+class TestCompositeForeignKeyPositioning < Minitest::Test
+  include Minitest::Hooks
+
+  def around
+    ActiveRecord::Base.transaction do
+      super
+      raise ActiveRecord::Rollback
+    end
+  end
+
+  def setup
+    list = List.create name: "List"
+    @first_parent = CompositePrimaryKeyItem.create(item_id: 1, account_id: 1, list: list, name: "First Parent")
+    @second_parent = CompositePrimaryKeyItem.create(item_id: 2, account_id: 2, list: list, name: "Second Parent")
+
+    @first_item = @first_parent.composite_foreign_key_items.create(name: "First Item")
+    @second_item = @first_parent.composite_foreign_key_items.create(name: "Second Item")
+    @third_item = @first_parent.composite_foreign_key_items.create(name: "Third Item")
+    @fourth_item = @second_parent.composite_foreign_key_items.create(name: "Fourth Item")
+    @fifth_item = @second_parent.composite_foreign_key_items.create(name: "Fifth Item")
+
+    @models = [
+      @first_item, @second_item, @third_item, @fourth_item, @fifth_item
+    ]
+
+    reload_models
+  end
+
+  def reload_models
+    @models.map(&:reload)
+  end
+
+  def test_initial_positioning
+    assert_equal [1, 2, 3], [@first_item, @second_item, @third_item].map(&:position)
+    assert_equal [1, 2], [@fourth_item, @fifth_item].map(&:position)
+  end
+
+  def test_scope_isolation
+    sixth_item = @second_parent.composite_foreign_key_items.create(name: "Sixth Item")
+    reload_models
+
+    assert_equal [1, 2, 3], [@first_item, @second_item, @third_item].map(&:position)
+    assert_equal [1, 2, 3], [@fourth_item, @fifth_item, sixth_item].map(&:position)
+  end
+
+  def test_position_on_create
+    new_item = @first_parent.composite_foreign_key_items.create(name: "New Item", position: 2)
+    reload_models
+
+    assert_equal [1, 2, 3, 4], [@first_item, new_item, @second_item, @third_item].map(&:position)
+    assert_equal [1, 2], [@fourth_item, @fifth_item].map(&:position)
+  end
+
+  def test_position_on_create_with_first
+    new_item = @first_parent.composite_foreign_key_items.create(name: "New Item", position: :first)
+    reload_models
+
+    assert_equal [1, 2, 3, 4], [new_item, @first_item, @second_item, @third_item].map(&:position)
+  end
+
+  def test_position_on_create_with_last
+    new_item = @first_parent.composite_foreign_key_items.create(name: "New Item", position: :last)
+    reload_models
+
+    assert_equal [1, 2, 3, 4], [@first_item, @second_item, @third_item, new_item].map(&:position)
+  end
+
+  def test_position_update
+    @first_item.update position: 3
+    reload_models
+
+    assert_equal [1, 2, 3], [@second_item, @third_item, @first_item].map(&:position)
+  end
+
+  def test_position_update_with_before
+    @third_item.update position: {before: @second_item}
+    reload_models
+
+    assert_equal [1, 2, 3], [@first_item, @third_item, @second_item].map(&:position)
+  end
+
+  def test_position_update_with_after
+    @first_item.update position: {after: @second_item}
+    reload_models
+
+    assert_equal [1, 2, 3], [@second_item, @first_item, @third_item].map(&:position)
+  end
+
+  def test_destroy_contracts_positions
+    @second_item.destroy
+    @models.delete @second_item
+    reload_models
+
+    assert_equal [1, 2], [@first_item, @third_item].map(&:position)
+    assert_equal [1, 2], [@fourth_item, @fifth_item].map(&:position)
+  end
+
+  def test_scope_change
+    @second_item.update(cpki_item_id: @second_parent.item_id, cpki_account_id: @second_parent.account_id)
+    reload_models
+
+    assert_equal [1, 2], [@first_item, @third_item].map(&:position)
+    assert_equal [1, 2, 3], [@fourth_item, @fifth_item, @second_item].map(&:position)
+  end
+
+  def test_scope_change_with_position
+    @second_item.update(cpki_item_id: @second_parent.item_id, cpki_account_id: @second_parent.account_id, position: 1)
+    reload_models
+
+    assert_equal [1, 2], [@first_item, @third_item].map(&:position)
+    assert_equal [1, 2, 3], [@second_item, @fourth_item, @fifth_item].map(&:position)
+  end
+
+  def test_destroying_parent_destroys_children
+    @first_parent.destroy
+
+    assert_equal 0, CompositeForeignKeyItem.where(cpki_item_id: @first_parent.item_id, cpki_account_id: @first_parent.account_id).count
+    assert_equal [1, 2], [@fourth_item, @fifth_item].map(&:reload).map(&:position)
+  end
+
+  def test_prior_and_subsequent
+    assert_nil @first_item.prior_position
+    assert_equal @first_item, @second_item.prior_position
+    assert_equal @second_item, @third_item.prior_position
+
+    assert_equal @second_item, @first_item.subsequent_position
+    assert_equal @third_item, @second_item.subsequent_position
+    assert_nil @third_item.subsequent_position
+  end
+end
+
 class TestNoScopePositioning < Minitest::Test
   include Minitest::Hooks