Merge pull request #589 from kshnurov/update_descendants_after_update

update_descendants_with_new_ancestry in after_update

lib/ancestry/has_ancestry.rb

@@ -45,8 +45,8 @@ def has_ancestry options = {}
       # Validate that the ancestor ids don't include own id
       validate :ancestry_exclude_self
 
-      # Update descendants with new ancestry before save
-      before_save :update_descendants_with_new_ancestry
+      # Update descendants with new ancestry after update
+      after_update :update_descendants_with_new_ancestry
 
       # Apply orphan strategy before destroy
       before_destroy :apply_orphan_strategy

lib/ancestry/instance_methods.rb

@@ -5,15 +5,15 @@ def ancestry_exclude_self
       errors.add(:base, I18n.t("ancestry.exclude_self", class_name: self.class.name.humanize)) if ancestor_ids.include? self.id
     end
 
-    # Update descendants with new ancestry (before save)
+    # Update descendants with new ancestry (after update)
     def update_descendants_with_new_ancestry
       # If enabled and node is existing and ancestry was updated and the new ancestry is sane ...
       if !ancestry_callbacks_disabled? && !new_record? && ancestry_changed? && sane_ancestor_ids?
         # ... for each descendant ...
-        unscoped_descendants.each do |descendant|
+        unscoped_descendants_before_save.each do |descendant|
           # ... replace old ancestry with new ancestry
           descendant.without_ancestry_callbacks do
-            new_ancestor_ids = path_ids + (descendant.ancestor_ids - path_ids_in_database)
+            new_ancestor_ids = path_ids + (descendant.ancestor_ids - path_ids_before_last_save)
             descendant.update_attribute(:ancestor_ids, new_ancestor_ids)
           end
         end
@@ -133,8 +133,8 @@ def path_ids
       ancestor_ids + [id]
     end
 
-    def path_ids_in_database
-      ancestor_ids_in_database + [id]
+    def path_ids_before_last_save
+      ancestor_ids_before_last_save + [id]
     end
 
     def path depth_options = {}
@@ -312,6 +312,12 @@ def unscoped_descendants
       end
     end
 
+    def unscoped_descendants_before_save
+      unscoped_where do |scope|
+        scope.where self.ancestry_base_class.descendant_before_save_conditions(self)
+      end
+    end
+
     # works with after save context (hence before_last_save)
     def unscoped_current_and_previous_ancestors
       unscoped_where do |scope|

lib/ancestry/materialized_path.rb

@@ -50,11 +50,19 @@ def descendants_of(object)
       indirects_of(node).or(children_of(node))
     end
 
-    # deprecated
-    def descendant_conditions(object)
+    def descendants_by_ancestry(ancestry)
       t = arel_table
+      t[ancestry_column].matches("#{ancestry}/%", nil, true).or(t[ancestry_column].eq(ancestry))
+    end
+
+    def descendant_conditions(object)
+      node = to_node(object)
+      descendants_by_ancestry( node.child_ancestry )
+    end
+
+    def descendant_before_save_conditions(object)
       node = to_node(object)
-      t[ancestry_column].matches("#{node.child_ancestry}/%", nil, true).or(t[ancestry_column].eq(node.child_ancestry))
+      descendants_by_ancestry( node.child_ancestry_before_save )
     end
 
     def subtree_of(object)
@@ -102,10 +110,6 @@ def ancestor_ids
         parse_ancestry_column(read_attribute(self.ancestry_base_class.ancestry_column))
       end
 
-      def ancestor_ids_in_database
-        parse_ancestry_column(send("#{self.ancestry_base_class.ancestry_column}#{IN_DATABASE_SUFFIX}"))
-      end
-
       def ancestor_ids_before_last_save
         parse_ancestry_column(send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}"))
       end
@@ -132,6 +136,13 @@ def child_ancestry
         path_was.blank? ? id.to_s : "#{path_was}#{ANCESTRY_DELIMITER}#{id}"
       end
 
+      def child_ancestry_before_save
+        # New records cannot have children
+        raise Ancestry::AncestryException.new(I18n.t("ancestry.no_child_for_new_record")) if new_record?
+        path_was = self.send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}")
+        path_was.blank? ? id.to_s : "#{path_was}#{ANCESTRY_DELIMITER}#{id}"
+      end
+
       def parse_ancestry_column(obj)
         return [] if obj == ROOT
         obj_ids = obj.split(ANCESTRY_DELIMITER)

lib/ancestry/materialized_path2.rb

@@ -25,21 +25,25 @@ def ordered_by_ancestry(order = nil)
       reorder(Arel::Nodes::Ascending.new(arel_table[ancestry_column]), order)
     end
 
-    # deprecated
-    def descendant_conditions(object)
-      t = arel_table
-      node = to_node(object)
-      t[ancestry_column].matches("#{node.child_ancestry}%", nil, true)
+    def descendants_by_ancestry(ancestry)
+      arel_table[ancestry_column].matches("#{ancestry}/%", nil, true)
     end
 
     module InstanceMethods
       def child_ancestry
         # New records cannot have children
-        raise Ancestry::AncestryException.new('No child ancestry for new record. Save record before performing tree operations.') if new_record?
+        raise Ancestry::AncestryException.new(I18n.t("ancestry.no_child_for_new_record")) if new_record?
         path_was = self.send("#{self.ancestry_base_class.ancestry_column}#{IN_DATABASE_SUFFIX}")
         "#{path_was}#{id}#{ANCESTRY_DELIMITER}"
       end
 
+      def child_ancestry_before_save
+        # New records cannot have children
+        raise Ancestry::AncestryException.new(I18n.t("ancestry.no_child_for_new_record")) if new_record?
+        path_was = self.send("#{self.ancestry_base_class.ancestry_column}#{BEFORE_LAST_SAVE_SUFFIX}")
+        "#{path_was}#{id}#{ANCESTRY_DELIMITER}"
+      end
+
       def parse_ancestry_column(obj)
         return [] if obj == ROOT
         obj_ids = obj.split(ANCESTRY_DELIMITER).delete_if(&:blank?)

lib/ancestry/materialized_path_pg.rb

@@ -1,11 +1,11 @@
 module Ancestry
   module MaterializedPathPg
-    # Update descendants with new ancestry (before save)
+    # Update descendants with new ancestry (after update)
     def update_descendants_with_new_ancestry
       # If enabled and node is existing and ancestry was updated and the new ancestry is sane ...
       if !ancestry_callbacks_disabled? && !new_record? && ancestry_changed? && sane_ancestor_ids?
         ancestry_column = ancestry_base_class.ancestry_column
-        old_ancestry = path_ids_in_database.join(Ancestry::MaterializedPath::ANCESTRY_DELIMITER)
+        old_ancestry = path_ids_before_last_save.join(Ancestry::MaterializedPath::ANCESTRY_DELIMITER)
         new_ancestry = path_ids.join(Ancestry::MaterializedPath::ANCESTRY_DELIMITER)
         update_clause = [
           "#{ancestry_column} = regexp_replace(#{ancestry_column}, '^#{old_ancestry}', '#{new_ancestry}')"
@@ -16,7 +16,7 @@ def update_descendants_with_new_ancestry
           update_clause << "#{depth_cache_column} = length(regexp_replace(regexp_replace(ancestry, '^#{old_ancestry}', '#{new_ancestry}'), '\\d', '', 'g')) + 1"
         end
 
-        unscoped_descendants.update_all update_clause.join(', ')
+        unscoped_descendants_before_save.update_all update_clause.join(', ')
       end
     end
   end

test/concerns/hooks_test.rb

@@ -31,6 +31,44 @@ def before_save_hook
     end
   end
 
+  def test_update_descendants_with_changed_parent_value
+    return if Ancestry.default_update_strategy == :sql # sql stragery doesn't trigger callbacks
+
+    AncestryTestDatabase.with_model(
+      extra_columns: { name: :string, name_path: :string }
+    ) do |model|
+
+      model.class_eval do
+        before_save :update_name_path
+
+        def update_name_path
+          self.name_path = [parent&.name_path, name].compact.join('/')
+        end
+      end
+
+      m1 = model.create!( name: "parent" )
+      m2 = model.create( parent: m1, name: "child" )
+      m3 = model.create( parent: m2, name: "grandchild" )
+      m4 = model.create( parent: m3, name: "grandgrandchild" )
+      assert_equal([m1.id], m2.ancestor_ids)
+      assert_equal("parent", m1.reload.name_path)
+      assert_equal("parent/child", m2.reload.name_path)
+      assert_equal("parent/child/grandchild", m3.reload.name_path)
+      assert_equal("parent/child/grandchild/grandgrandchild", m4.reload.name_path)
+
+      m5 = model.create!( name: "changed" )
+
+      m2.update!( parent_id: m5.id )
+      assert_equal("changed", m5.reload.name_path)
+      assert_equal([m5.id], m2.reload.ancestor_ids)
+      assert_equal("changed/child", m2.reload.name_path)
+      assert_equal([m5.id,m2.id], m3.reload.ancestor_ids)
+      assert_equal("changed/child/grandchild", m3.reload.name_path)
+      assert_equal([m5.id,m2.id,m3.id], m4.reload.ancestor_ids)
+      assert_equal("changed/child/grandchild/grandgrandchild", m4.reload.name_path)
+    end
+  end
+
   def test_has_ancestry_detects_changes_in_after_save
     AncestryTestDatabase.with_model(:extra_columns => {:name => :string, :name_path => :string}) do |model|
       model.class_eval do