Merge pull request take-five#11 from vkuznetsov/master

correct behavior of callbacks

lib/acts_as_ordered_tree/instance_methods.rb

@@ -306,6 +306,9 @@ def move_to(target, pos) #:nodoc:
         # nothing changed - quit
         return if parent_id == parent_id_was && position == position_was
 
+        self.class.find(self, :lock => true)
+        self[position_column], self[parent_column] = position, parent_id
+
         move_kind = case
           when id_was && parent_id != parent_id_was then :move
           when id_was && position  != position_was  then :reorder
@@ -363,55 +366,69 @@ def move!(id, parent_id_was, parent_id, position_was, position, depth) #:nodoc:
               "THEN :depth " +
               "ELSE #{depth_column} " +
           "END" if depth_column)
-      ].compact.join(', ')
+      ]
 
       conditions = arel[pk].eq(id).or(
         arel[parent_column].eq(parent_id_was)
       ).or(
         arel[parent_column].eq(parent_id)
       )
 
-      binds = {:id => id,
-               :parent_id_was => parent_id_was,
+      binds = {:parent_id_was => parent_id_was,
                :parent_id => parent_id,
                :position_was => position_was,
                :position => position,
                :depth => depth}
 
-      ordered_tree_scope.where(conditions).update_all([assignments, binds])
+      update_changed_attributes! conditions, assignments, binds
     end
 
     # Internal
     def reorder!(parent_id, position_was, position)
-      assignments = if position_was
-        <<-SQL
-        #{position_column} = CASE
-            WHEN #{position_column} = :position_was
-            THEN :position
-            WHEN #{position_column} <= :position AND #{position_column} > :position_was AND :position > :position_was
-            THEN #{position_column} - 1
-            WHEN #{position_column} >= :position AND #{position_column} < :position_was AND :position < :position_was
-            THEN #{position_column} + 1
-            ELSE #{position_column}
-        END
-        SQL
-      else
-        <<-SQL
-        #{position_column} = CASE
-            WHEN #{position_column} > :position
-            THEN #{position_column} + 1
-            WHEN #{position_column} IS NULL
-            THEN :position
-            ELSE #{position_column}
-        END
-        SQL
-      end
+      assignments = [
+        if position_was
+          <<-SQL
+          #{position_column} = CASE
+              WHEN #{position_column} = :position_was
+              THEN :position
+              WHEN #{position_column} <= :position AND #{position_column} > :position_was AND :position > :position_was
+              THEN #{position_column} - 1
+              WHEN #{position_column} >= :position AND #{position_column} < :position_was AND :position < :position_was
+              THEN #{position_column} + 1
+              ELSE #{position_column}
+          END
+          SQL
+        else
+          <<-SQL
+          #{position_column} = CASE
+              WHEN #{position_column} > :position
+              THEN #{position_column} + 1
+              WHEN #{position_column} IS NULL
+              THEN :position
+              ELSE #{position_column}
+          END
+          SQL
+        end
+      ]
 
       conditions = arel[parent_column].eq(parent_id)
-
       binds = {:position_was => position_was, :position => position}
 
-      ordered_tree_scope.where(conditions).update_all([assignments, binds])
+      update_changed_attributes! conditions, assignments, binds
+    end
+
+    def update_changed_attributes!(scope_conditions, assignments, binds)
+      # add assignments for externally changed attributes
+      internal_attributes = [parent_column.to_s, position_column.to_s, depth_column.to_s, self.class.primary_key]
+      external_changed_attrs = changed - internal_attributes
+      unless external_changed_attrs.empty?
+        external_changed_attrs.each do |attr|
+          assignments << "#{attr} = CASE WHEN #{self.class.primary_key} = :id THEN :#{attr} ELSE #{attr} END"
+          binds[attr.to_sym] = self[attr]
+        end
+      end
+
+      ordered_tree_scope.where(scope_conditions).update_all([assignments.compact.join(', '), {:id => id}.merge(binds)])
     end
 
     # recursively load descendants
@@ -448,7 +465,7 @@ def update_descendants_depth #:nodoc:
 
     # Used in built-in around_move routine
     def update_counter_cache #:nodoc:
-      parent_id_was = self[parent_column]
+      parent_id_was = send "#{parent_column}_was"
 
       yield
 

lib/acts_as_ordered_tree/version.rb

@@ -1,3 +1,3 @@
 module ActsAsOrderedTree
-  VERSION = "1.1.1"
+  VERSION = "1.1.2"
 end

spec/acts_as_ordered_tree_spec.rb

@@ -729,6 +729,32 @@
         child_3.move_to_child_of child_1
         record.reload.depth.should eq 3
       end
+
+      context "DefaultWithCallbacks" do
+        let!(:cb_root_1) { create :default_with_callbacks, :name => 'root_1' }
+        let!(:cb_root_2) { create :default_with_callbacks, :name => 'root_2' }
+        let!(:cb_child_1) { create :default_with_callbacks, :name => 'child_1', :parent => cb_root_1 }
+        let!(:cb_child_2) { create :default_with_callbacks, :name => 'child_2', :parent => cb_root_1 }
+
+        specify "new parent_id should be available in before_move" do
+          cb_root_2.stub(:before_move) { cb_root_2.parent_id.should eq cb_root_1.id }
+          cb_root_2.move_to_left_of cb_child_1
+        end
+
+        specify "new position should be available in before_reorder" do
+          cb_child_2.stub(:before_reorder) { cb_child_2.position.should eq 1 }
+          cb_child_2.move_to_left_of cb_child_1
+        end
+      end
+
+    end
+
+    context "changed attributes" do
+      specify "changed attributes should be saved" do
+        child_2.name = 'name100'
+        child_2.move_to_left_of child_1
+        child_2.reload.name.should eq 'name100'
+      end
     end
 
   end

spec/concurrency_support_spec.rb

@@ -7,14 +7,11 @@
     module Concurrency
       # run block in its own thread, create +size+ threads
       def pool(size)
-        body = proc do |x|
-          ActiveRecord::Base.connection_pool.with_connection do
-            yield x
+        size.times.map { |x|
+          Thread.new do
+            ActiveRecord::Base.connection_pool.with_connection { yield x }
           end
-        end
-        threads = size.times.map { |x| Thread.new { body.call(x) } }
-
-        threads.each(&:join)
+        }.each(&:join)
       end
     end
     include Concurrency

spec/spec_helper.rb

@@ -50,6 +50,7 @@
     ensure
       Default.delete_all
       DefaultWithCounterCache.delete_all
+      DefaultWithCallbacks.delete_all
       Scoped.delete_all
     end
   end

spec/support/factories.rb

@@ -7,6 +7,10 @@
     sequence(:name) { |n| "category #{n}" }
   end
 
+  factory :default_with_callbacks do
+    sequence(:name) { |n| "category #{n}" }
+  end
+
   factory :scoped do
     sequence(:scope_type) { |n| "type_#{n}" }
     sequence(:name) { |n| "category #{n}" }

spec/support/models.rb

@@ -16,6 +16,22 @@ class DefaultWithCounterCache < ActiveRecord::Base
   acts_as_ordered_tree :counter_cache => :categories_count
 end
 
+class DefaultWithCallbacks < ActiveRecord::Base
+  self.table_name = "categories"
+
+  acts_as_ordered_tree
+
+  after_move     :after_move
+  before_move    :before_move
+  after_reorder  :after_reorder
+  before_reorder :before_reorder
+
+  def after_move; end
+  def before_move; end
+  def after_reorder; end
+  def before_reorder; end
+end
+
 class Scoped < ActiveRecord::Base
   self.table_name = "scoped"