feat: sibling reordering when node changes parent or scope (#484)

Author: sampatbadhe

lib/closure_tree/hierarchy_maintenance.rb

@@ -18,7 +18,10 @@ def _ct_skip_cycle_detection!
     end
 
     def _ct_skip_sort_order_maintenance!
-      @_ct_skip_sort_order_maintenance = true
+      ActiveSupport::Deprecation.new.warn(
+        '_ct_skip_sort_order_maintenance! is deprecated and will be removed in the next major version. ' \
+        'Sort order maintenance is now handled automatically.'
+      )
     end
 
     def _ct_validate
@@ -38,15 +41,24 @@ def _ct_before_save
     end
 
     def _ct_after_save
-      rebuild! if saved_changes[_ct.parent_column_name] || @was_new_record
+      scope_changed = _ct.order_is_numeric? && _ct.scope_changed?(self)
+
+      if saved_changes[_ct.parent_column_name] || @was_new_record
+        rebuild!
+      elsif scope_changed
+        # Scope changed without parent change - reorder old scope's siblings
+        _ct_reorder_prior_siblings_if_parent_changed
+        _ct_reorder_siblings
+      elsif _ct.order_option? && saved_changes[_ct.order_column_sym]
+        _ct_reorder_siblings(saved_changes[_ct.order_column_sym].min)
+      end
       if saved_changes[_ct.parent_column_name] && !@was_new_record
         # Resetting the ancestral collections addresses
         # https://github.com/mceachen/closure_tree/issues/68
         ancestor_hierarchies.reload
         self_and_ancestors.reload
       end
       @was_new_record = false # we aren't new anymore.
-      @_ct_skip_sort_order_maintenance = false # only skip once.
       true # don't cancel anything.
     end
 
@@ -86,7 +98,7 @@ def rebuild!(called_by_rebuild = false)
           SQL
         end
 
-        if _ct.order_is_numeric? && !@_ct_skip_sort_order_maintenance
+        if _ct.order_is_numeric?
           _ct_reorder_prior_siblings_if_parent_changed
           # Prevent double-reordering of siblings:
           _ct_reorder_siblings unless called_by_rebuild

lib/closure_tree/numeric_deterministic_ordering.rb

@@ -12,11 +12,16 @@ module NumericDeterministicOrdering
     end
 
     def _ct_reorder_prior_siblings_if_parent_changed
-      return unless saved_change_to_attribute?(_ct.parent_column_name) && !@was_new_record
+      return if @was_new_record
+
+      parent_changed = saved_change_to_attribute?(_ct.parent_column_name)
+      scope_changed = _ct.scope_changed?(self)
+
+      return unless parent_changed || scope_changed
 
       was_parent_id = attribute_before_last_save(_ct.parent_column_name)
-      scope_conditions = _ct.scope_values_from_instance(self)
-      _ct.reorder_with_parent_id(was_parent_id, nil, scope_conditions)
+      previous_scope_conditions = _ct.previous_scope_values_from_instance(self)
+      _ct.reorder_with_parent_id(was_parent_id, nil, previous_scope_conditions)
     end
 
     def _ct_reorder_siblings(minimum_sort_order_value = nil)
@@ -132,9 +137,7 @@ def append_child(child_node)
     def prepend_child(child_node)
       child_node.order_value = -1
       child_node.parent = self
-      child_node._ct_skip_sort_order_maintenance!
       if child_node.save
-        _ct_reorder_children
         child_node.reload
       else
         child_node
@@ -161,19 +164,11 @@ def add_sibling(sibling, add_after = true)
 
       _ct.with_advisory_lock do
         prior_sibling_parent = sibling.parent
-        reorder_from_value = if prior_sibling_parent == parent
-                               [order_value, sibling.order_value].compact.min
-                             else
-                               order_value
-                             end
 
         sibling.order_value = order_value
         sibling.parent = parent
-        sibling._ct_skip_sort_order_maintenance!
         sibling.save # may be a no-op
 
-        _ct_reorder_siblings(reorder_from_value)
-
         # The sort order should be correct now except for self and sibling, which may need to flip:
         sibling_is_after = reload.order_value < sibling.reload.order_value
         if add_after != sibling_is_after

lib/closure_tree/support.rb

@@ -257,6 +257,45 @@ def scope_values_from_instance(instance)
       scope_hash
     end
 
+    def previous_scope_values_from_instance(instance)
+      return {} unless options[:scope] && instance
+
+      scope_option = options[:scope]
+      scope_hash = {}
+
+      case scope_option
+      when Symbol
+        value = instance.attribute_before_last_save(scope_option)
+        scope_hash[scope_option] = value
+      when Array
+        scope_option.each do |item|
+          if item.is_a?(Symbol)
+            value = instance.attribute_before_last_save(item)
+            scope_hash[item] = value
+          end
+        end
+      end
+
+      scope_hash
+    end
+
+    def scope_changed?(instance)
+      return false unless options[:scope] && instance
+
+      scope_option = options[:scope]
+
+      case scope_option
+      when Symbol
+        instance.saved_change_to_attribute?(scope_option)
+      when Array
+        scope_option.any? do |item|
+          item.is_a?(Symbol) && instance.saved_change_to_attribute?(item)
+        end
+      else
+        false
+      end
+    end
+
     def apply_scope_conditions(scope, instance = nil)
       return scope unless options[:scope] && instance
 

test/closure_tree/label_test.rb

@@ -70,12 +70,12 @@ def create_preorder_tree(suffix = '')
   Label.roots.each_with_index do |root, root_idx|
     root.order_value = root_idx
     yield(root) if block_given?
-    root.save!
+    root.update_columns(root._ct.order_column => root_idx, type: root.type)
     root.self_and_descendants.each do |ea|
-      ea.children.to_a.sort_by(&:name).each_with_index do |ea, idx|
-        ea.order_value = idx
-        yield(ea) if block_given?
-        ea.save!
+      ea.children.to_a.sort_by(&:name).each_with_index do |child, idx|
+        child.order_value = idx
+        yield(child) if block_given?
+        child.update_columns(child._ct.order_column => idx, type: child.type)
       end
     end
   end

test/closure_tree/multi_database_test.rb

@@ -3,32 +3,6 @@
 require 'test_helper'
 
 class MultiDatabaseTest < ActiveSupport::TestCase
-  def setup
-    super
-    # Create memory tables - always recreate for clean state
-    LiteRecord.connection.create_table :memory_tags, force: true do |t|
-      t.string :name
-      t.integer :parent_id
-      t.timestamps
-    end
-
-    LiteRecord.connection.create_table :memory_tag_hierarchies, id: false, force: true do |t|
-      t.integer :ancestor_id, null: false
-      t.integer :descendant_id, null: false
-      t.integer :generations, null: false
-    end
-
-    LiteRecord.connection.add_index :memory_tag_hierarchies, %i[ancestor_id descendant_id generations],
-                                      unique: true, name: 'memory_tag_anc_desc_idx'
-    LiteRecord.connection.add_index :memory_tag_hierarchies, [:descendant_id], name: 'memory_tag_desc_idx'
-  end
-
-  def teardown
-    # Clean up SQLite tables after each test
-    LiteRecord.connection.drop_table :memory_tag_hierarchies, if_exists: true
-    LiteRecord.connection.drop_table :memory_tags, if_exists: true
-    super
-  end
 
   def test_postgresql_with_advisory_lock
     skip 'PostgreSQL not configured' unless postgresql?(ApplicationRecord.connection)