Fixes #49

lib/closure_tree/model.rb

@@ -45,6 +45,14 @@ module Model
         :through => :descendant_hierarchies,
         :source => :descendant,
         :order => "#{_ct.quoted_hierarchy_table_name}.generations asc")
+
+      scope :without, lambda { |instance|
+        if instance.new_record?
+          scoped
+        else
+          where(["#{_ct.quoted_table_name}.#{_ct.base_class.primary_key} != ?", instance.id])
+        end
+      }
     end
 
     # Delegate to the Support instance on the class:
@@ -248,8 +256,7 @@ def delete_hierarchy_references
     end
 
     def without_self(scope)
-      return scope if self.new_record?
-      scope.where(["#{_ct.quoted_table_name}.#{_ct.base_class.primary_key} != ?", self])
+      scope.without(self)
     end
 
     module ClassMethods

lib/closure_tree/numeric_deterministic_ordering.rb

@@ -52,41 +52,35 @@ def roots_and_descendants_preordered
       end
     end
 
-    def append_sibling(sibling_node, use_update_all = true)
-      add_sibling(sibling_node, use_update_all, true)
+    def append_sibling(sibling_node)
+      add_sibling(sibling_node, true)
     end
 
-    def prepend_sibling(sibling_node, use_update_all = true)
-      add_sibling(sibling_node, use_update_all, false)
+    def prepend_sibling(sibling_node)
+      add_sibling(sibling_node, false)
     end
 
-    def add_sibling(sibling_node, use_update_all = true, add_after = true)
+    def add_sibling(sibling_node, add_after = true)
       fail "can't add self as sibling" if self == sibling_node
       # issue 40: we need to lock the parent to prevent deadlocks on parallel sibling additions
       ct_with_advisory_lock do
-        # issue 18: we need to set the order_value explicitly so subsequent orders will work.
-        update_attribute(:order_value, 0) if self.order_value.nil?
-        sibling_node.order_value = self.order_value.to_i + (add_after ? 1 : -1)
-        # We need to incr the before_siblings to make room for sibling_node:
-        if use_update_all
-          col = _ct.quoted_order_column(false)
-          # issue 21: we have to use the base class, so STI doesn't get in the way of only updating the child class instances:
-          _ct.base_class.update_all(
-            ["#{col} = #{col} #{add_after ? '+' : '-'} 1", "updated_at = now()"],
-            ["#{_ct.quoted_parent_column_name} = ? AND #{col} #{add_after ? '>=' : '<='} ?",
-              parent_id,
-              sibling_node.order_value])
-        else
-          last_value = sibling_node.order_value.to_i
-          (add_after ? siblings_after : siblings_before.reverse).each do |ea|
-            last_value += (add_after ? 1 : -1)
-            ea.order_value = last_value
-            ea.save!
-          end
+        if self.order_value.nil? || siblings_before.without(sibling_node).empty?
+          update_attribute(:order_value, 0)
         end
         sibling_node.parent = self.parent
-        sibling_node.save!
-        sibling_node.reload
+        starting_order_value = self.order_value.to_i
+        to_reorder = siblings_after.without(sibling_node).to_a
+        if add_after
+          to_reorder.unshift(sibling_node)
+        else
+          to_reorder.unshift(self)
+          sibling_node.update_attribute(:order_value, starting_order_value)
+        end
+
+        to_reorder.each_with_index do |ea, idx|
+          ea.update_attribute(:order_value, starting_order_value + idx + 1)
+        end
+        sibling_node.reload # because the parent may have changed.
       end
     end
   end

spec/label_spec.rb

@@ -230,7 +230,7 @@ def create_preorder_tree(suffix = "")
     end
 
     it "when inserted before" do
-      @b.append_sibling(@a, use_update_all = false)
+      @b.append_sibling(@a)
       # Have to reload because the sort_order will have changed out from under the references:
       @b.reload.sort_order.should be < @a.reload.sort_order
       @a.reload.sort_order.should be < @c.reload.sort_order
@@ -245,15 +245,19 @@ def create_preorder_tree(suffix = "")
 
     a.append_sibling(b)
     root.reload.children.collect(&:name).should == %w(a b)
+    root.reload.children.collect(&:sort_order).should == [0, 1]
 
     a.prepend_sibling(b)
     root.reload.children.collect(&:name).should == %w(b a)
+    root.reload.children.collect(&:sort_order).should == [0, 1]
 
     a.append_sibling(c)
     root.reload.children.collect(&:name).should == %w(b a c)
+    root.reload.children.collect(&:sort_order).should == [0, 1, 2]
 
     b.append_sibling(c)
     root.reload.children.collect(&:name).should == %w(b c a)
+    root.reload.children.collect(&:sort_order).should == [0, 1, 2]
   end
 
   context "Deterministic siblings sort with custom integer column" do
@@ -280,9 +284,9 @@ def create_preorder_tree(suffix = "")
     end
 
     it "should prepend a node as a sibling of another node (!update_all)" do
-      labels(:c16).prepend_sibling(labels(:c17), false)
+      labels(:c16).prepend_sibling(labels(:c17))
       labels(:c16).self_and_siblings.to_a.should == [labels(:c17), labels(:c16), labels(:c18), labels(:c19)]
-      labels(:c19).reload.prepend_sibling(labels(:c16).reload, false)
+      labels(:c19).reload.prepend_sibling(labels(:c16).reload)
       labels(:c16).self_and_siblings.to_a.should == [labels(:c17), labels(:c18), labels(:c16), labels(:c19)]
       labels(:c16).siblings_before.to_a.should == [labels(:c17), labels(:c18)]
       labels(:c16).siblings_after.to_a.should == [labels(:c19)]
@@ -316,7 +320,7 @@ def create_preorder_tree(suffix = "")
 
     it "should move a node before another node" do
       labels(:c2).ancestry_path.should == %w{a1 b2 c2}
-      labels(:b2).prepend_sibling(labels(:c2), false)
+      labels(:b2).prepend_sibling(labels(:c2))
       labels(:c2).ancestry_path.should == %w{a1 c2}
       labels(:c2).self_and_siblings.to_a.should == [labels(:b1), labels(:c2), labels(:b2)]
     end
@@ -343,10 +347,10 @@ def create_preorder_tree(suffix = "")
 
     it "should move a node after another node" do
       labels(:c2).ancestry_path.should == %w{a1 b2 c2}
-      labels(:b2).append_sibling(labels(:c2), false)
+      labels(:b2).append_sibling(labels(:c2))
       labels(:c2).ancestry_path.should == %w{a1 c2}
       labels(:c2).self_and_siblings.to_a.should == [labels(:b1), labels(:b2), labels(:c2)]
-      labels(:c2).append_sibling(labels(:e2), false)
+      labels(:c2).append_sibling(labels(:e2))
       labels(:e2).self_and_siblings.to_a.should == [labels(:b1), labels(:b2), labels(:c2), labels(:e2)]
       labels(:a1).self_and_descendants.collect(&:name).should == %w(a1 b1 b2 c2 e2 d2 c1-six c1-seven c1-eight c1-nine)
       labels(:a1).leaves.collect(&:name).should == %w(b2 e2 d2 c1-six c1-seven c1-eight c1-nine)