diff --git a/lib/closure_tree/hierarchy_maintenance.rb b/lib/closure_tree/hierarchy_maintenance.rb index 825181a6..ffb376bb 100644 --- a/lib/closure_tree/hierarchy_maintenance.rb +++ b/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,7 +41,17 @@ 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 @@ -46,7 +59,6 @@ def _ct_after_save 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 diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index ab49f321..e0e1644f 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/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 diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index f26ab056..b8fa91d2 100644 --- a/lib/closure_tree/support.rb +++ b/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 diff --git a/test/closure_tree/label_test.rb b/test/closure_tree/label_test.rb index 4bfab697..77a48f09 100644 --- a/test/closure_tree/label_test.rb +++ b/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 diff --git a/test/closure_tree/multi_database_test.rb b/test/closure_tree/multi_database_test.rb index daf191ca..fa4d3e1a 100644 --- a/test/closure_tree/multi_database_test.rb +++ b/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) diff --git a/test/closure_tree/order_value_cross_database_test.rb b/test/closure_tree/order_value_cross_database_test.rb new file mode 100644 index 00000000..f10e4917 --- /dev/null +++ b/test/closure_tree/order_value_cross_database_test.rb @@ -0,0 +1,271 @@ +# frozen_string_literal: true + +require 'test_helper' + +class OrderValueCrossDatabaseTest < ActiveSupport::TestCase + def setup + super + Label.delete_all + LabelHierarchy.delete_all + SecondaryTag.delete_all + SecondaryTagHierarchy.delete_all + end + + # =========================================== + # PostgreSQL (Label) Tests + # =========================================== + + test 'PostgreSQL: should reorder remaining root nodes when a root node becomes a child via prepend_child' do + node_1 = Label.create(name: 'node_1') + node_2 = Label.create(name: 'node_2') + node_3 = Label.create(name: 'node_3') + node_4 = Label.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.prepend_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'PostgreSQL: should reorder remaining root nodes when a root node becomes a child via append_child' do + node_1 = Label.create(name: 'node_1') + node_2 = Label.create(name: 'node_2') + node_3 = Label.create(name: 'node_3') + node_4 = Label.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.append_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'PostgreSQL: should reorder remaining root nodes when a root node becomes a child via add_sibling' do + node_1 = Label.create(name: 'node_1') + node_2 = Label.create(name: 'node_2') + node_3 = Label.create(name: 'node_3') + node_4 = Label.create(name: 'node_4') + + child = Label.create(name: 'child', parent: node_3) + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + assert_equal 0, child.order_value + + child.add_sibling(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + # =========================================== + # SQLite (MemoryTag) Tests + # =========================================== + + test 'SQLite: should reorder remaining root nodes when a root node becomes a child via prepend_child' do + node_1 = MemoryTag.create(name: 'node_1') + node_2 = MemoryTag.create(name: 'node_2') + node_3 = MemoryTag.create(name: 'node_3') + node_4 = MemoryTag.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.prepend_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'SQLite: should reorder remaining root nodes when a root node becomes a child via append_child' do + node_1 = MemoryTag.create(name: 'node_1') + node_2 = MemoryTag.create(name: 'node_2') + node_3 = MemoryTag.create(name: 'node_3') + node_4 = MemoryTag.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.append_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'SQLite: should reorder remaining root nodes when a root node becomes a child via add_sibling' do + node_1 = MemoryTag.create(name: 'node_1') + node_2 = MemoryTag.create(name: 'node_2') + node_3 = MemoryTag.create(name: 'node_3') + node_4 = MemoryTag.create(name: 'node_4') + + child = MemoryTag.create(name: 'child', parent: node_3) + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + assert_equal 0, child.order_value + + child.add_sibling(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + # =========================================== + # MySQL (SecondaryTag) Tests + # =========================================== + + test 'MySQL: should reorder remaining root nodes when a root node becomes a child via prepend_child' do + skip 'MySQL not configured' unless mysql?(SecondaryRecord.connection) + + node_1 = SecondaryTag.create(name: 'node_1') + node_2 = SecondaryTag.create(name: 'node_2') + node_3 = SecondaryTag.create(name: 'node_3') + node_4 = SecondaryTag.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.prepend_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'MySQL: should reorder remaining root nodes when a root node becomes a child via append_child' do + skip 'MySQL not configured' unless mysql?(SecondaryRecord.connection) + + node_1 = SecondaryTag.create(name: 'node_1') + node_2 = SecondaryTag.create(name: 'node_2') + node_3 = SecondaryTag.create(name: 'node_3') + node_4 = SecondaryTag.create(name: 'node_4') + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + + node_3.append_child(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + assert_equal 0, node_2.order_value + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end + + test 'MySQL: should reorder remaining root nodes when a root node becomes a child via add_sibling' do + skip 'MySQL not configured' unless mysql?(SecondaryRecord.connection) + + node_1 = SecondaryTag.create(name: 'node_1') + node_2 = SecondaryTag.create(name: 'node_2') + node_3 = SecondaryTag.create(name: 'node_3') + node_4 = SecondaryTag.create(name: 'node_4') + + child = SecondaryTag.create(name: 'child', parent: node_3) + + assert_equal 0, node_1.order_value + assert_equal 1, node_2.order_value + assert_equal 2, node_3.order_value + assert_equal 3, node_4.order_value + assert_equal 0, child.order_value + + child.add_sibling(node_2) + + node_1.reload + node_2.reload + node_3.reload + node_4.reload + + assert_equal node_3.id, node_2._ct_parent_id + + assert_equal 0, node_1.order_value + assert_equal 1, node_3.order_value + assert_equal 2, node_4.order_value + end +end diff --git a/test/closure_tree/scope_test.rb b/test/closure_tree/scope_test.rb index aa0e141b..2884fec2 100644 --- a/test/closure_tree/scope_test.rb +++ b/test/closure_tree/scope_test.rb @@ -234,4 +234,101 @@ def test_build_scope_where_clause_with_nil_value_sqlite assert_includes clause, 'IS NULL' assert_includes clause, '456' end + + def test_reorder_previous_scope_siblings_when_scope_changes + # Create items in scope user_id: 1 + item1 = ScopedItem.create!(name: 'item1', user_id: 1) + item2 = ScopedItem.create!(name: 'item2', user_id: 1) + item3 = ScopedItem.create!(name: 'item3', user_id: 1) + + # Verify initial order values + assert_equal 0, item1.order_value + assert_equal 1, item2.order_value + assert_equal 2, item3.order_value + + # Move item2 to a different scope (user_id: 2) + item2.update!(user_id: 2) + + # Reload all items + item1.reload + item2.reload + item3.reload + + # item2 should be first in its new scope + assert_equal 0, item2.order_value + + # item1 and item3 should be reordered without gaps in old scope + assert_equal 0, item1.order_value + assert_equal 1, item3.order_value + end + + def test_reorder_previous_scope_siblings_when_multiple_scope_changes + # Create items in scope user_id: 1, group_id: 10 + item1 = MultiScopedItem.create!(name: 'item1', user_id: 1, group_id: 10) + item2 = MultiScopedItem.create!(name: 'item2', user_id: 1, group_id: 10) + item3 = MultiScopedItem.create!(name: 'item3', user_id: 1, group_id: 10) + + # Verify initial order values + assert_equal 0, item1.order_value + assert_equal 1, item2.order_value + assert_equal 2, item3.order_value + + # Move item2 to a different scope (user_id: 2, group_id: 10) + item2.update!(user_id: 2) + + # Reload all items + item1.reload + item2.reload + item3.reload + + # item2 should be first in its new scope + assert_equal 0, item2.order_value + + # item1 and item3 should be reordered without gaps in old scope + assert_equal 0, item1.order_value + assert_equal 1, item3.order_value + end + + def test_scope_changed_detection + # Test scope_changed? by checking the actual reordering behavior + # which implicitly tests that scope_changed? works during callbacks + item1 = ScopedItem.create!(name: 'item1', user_id: 1) + item2 = ScopedItem.create!(name: 'item2', user_id: 1) + + assert_equal 0, item1.order_value + assert_equal 1, item2.order_value + + # Change scope - if scope_changed? works, item1 will be reordered in new scope + item2.update!(user_id: 2) + + item1.reload + item2.reload + + # item2 should be 0 in new scope (proves scope change was detected) + assert_equal 0, item2.order_value + # item1 should remain 0 (was already 0, reordering removes gap from item2) + assert_equal 0, item1.order_value + end + + def test_previous_scope_values_from_instance + # Test previous_scope_values by checking that OLD scope siblings are reordered + # which implicitly tests that previous scope values are correctly retrieved + item1 = ScopedItem.create!(name: 'item1', user_id: 1) + item2 = ScopedItem.create!(name: 'item2', user_id: 1) + item3 = ScopedItem.create!(name: 'item3', user_id: 1) + + assert_equal 0, item1.order_value + assert_equal 1, item2.order_value + assert_equal 2, item3.order_value + + # Move middle item to different scope + item2.update!(user_id: 2) + + item1.reload + item3.reload + + # If previous_scope_values works, item3 should now be 1 (gap filled) + assert_equal 0, item1.order_value + assert_equal 1, item3.order_value + end end diff --git a/test/dummy/app/models/memory_tag.rb b/test/dummy/app/models/memory_tag.rb index 8863898b..22b2a334 100644 --- a/test/dummy/app/models/memory_tag.rb +++ b/test/dummy/app/models/memory_tag.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class MemoryTag < LiteRecord - has_closure_tree + has_closure_tree order: :sort_order, numeric_order: true end diff --git a/test/dummy/app/models/secondary_tag.rb b/test/dummy/app/models/secondary_tag.rb index 1cd9eece..e565144f 100644 --- a/test/dummy/app/models/secondary_tag.rb +++ b/test/dummy/app/models/secondary_tag.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class SecondaryTag < SecondaryRecord - has_closure_tree + has_closure_tree order: :sort_order, numeric_order: true end diff --git a/test/dummy/db/secondary_schema.rb b/test/dummy/db/secondary_schema.rb index f35d629f..1267df18 100644 --- a/test/dummy/db/secondary_schema.rb +++ b/test/dummy/db/secondary_schema.rb @@ -16,6 +16,7 @@ create_table 'secondary_tags', force: true do |t| t.string 'name' t.integer 'parent_id' + t.integer 'sort_order' t.datetime 'created_at' t.datetime 'updated_at' end diff --git a/test/dummy/db/sqlite_schema.rb b/test/dummy/db/sqlite_schema.rb index 30e0fd0e..93b1563d 100644 --- a/test/dummy/db/sqlite_schema.rb +++ b/test/dummy/db/sqlite_schema.rb @@ -46,4 +46,22 @@ add_index 'memory_adoptable_tag_hierarchies', %i[ancestor_id descendant_id generations], unique: true, name: 'memory_adoptable_tag_anc_desc_idx' add_index 'memory_adoptable_tag_hierarchies', [:descendant_id], name: 'memory_adoptable_tag_desc_idx' + + create_table 'memory_tags', force: true do |t| + t.string 'name' + t.integer 'parent_id' + t.integer 'sort_order' + t.datetime 'created_at' + t.datetime 'updated_at' + end + + 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 + + add_index 'memory_tag_hierarchies', %i[ancestor_id descendant_id generations], unique: true, + name: 'memory_tag_anc_desc_idx' + add_index 'memory_tag_hierarchies', [:descendant_id], name: 'memory_tag_desc_idx' end diff --git a/test/dummy/lib/tasks/db.rake b/test/dummy/lib/tasks/db.rake index 368e5db1..62df8a4e 100644 --- a/test/dummy/lib/tasks/db.rake +++ b/test/dummy/lib/tasks/db.rake @@ -17,7 +17,12 @@ namespace :db do load Rails.root.join('db/secondary_schema.rb') end - # SQLite is in-memory so it will be created automatically + # Load SQLite (lite) database schema + ActiveRecord::Base.establish_connection(:lite) + ActiveRecord::Base.connection.disconnect! if ActiveRecord::Base.connection.active? + ActiveRecord::Base.establish_connection(:lite) + load Rails.root.join('db/sqlite_schema.rb') + ActiveRecord::Base.establish_connection(:primary) end