Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Correctly set the ancestry column when creating a child within an after_* callback #57

Open
wants to merge 2 commits into from

3 participants

@jstirk

I noticed an issue where creating a child record with the ancestry field set to just the parent id, rather than the entire parent path.

I have added a test reproducing the behaviour, and a fix to the code.

The main issue looked to be that the child_ancestry method was using the ancestry path before the save (using _was) which for new records was obviously blank.

This revision uses the current value of the parent's ancestry path, and adds a child_ancestry_was method to return the old behaviour (with a more descriptive name).

The change to update_descendants_with_new_ancestry was due to it relying on the old behaviour. The revised version explicitly replaces the old parent ancestry snippet (using child_ancestry_was) with the new path.

Please let me know if you need any additional information, or if you would like me to revise the patch.

Jason Stirk added some commits
Jason Stirk Failing test: create child in after_create callback
Creating a child in a after_* callback results in the child's ancestry
column containing only the parent id, and not the entire parent ancestry
path.
65786f0
Jason Stirk Fix creating children in after_* callbacks
Previously creating children in an after_* callback would cause the
child's ancestry column to contain just the parent id and not the entire
ancestry path.

A workaround was to call self.reload in the after_* callback, but this
commit makes the process work automatically.

The main issue looked to be that the child_ancestry method was using the
ancestry path before the save (using _was) which for new records was
obviously blank.

This revision uses the current value of the parent's ancestry, and adds
a child_ancestry_was method to return the old behaviour (with a more
descriptive name).

The change to update_descendants_with_new_ancestry was due to it relying
on the old behaviour. The revised version explicitly replaces the old
parent ancestry snippet (using child_ancestry_was) with the new one
path.
9095963
@sony-phoenix-dev

I just burned 12 hours on the same problem. Why was this pull request not committed?

@stefankroes
Owner

This pull request is three years old. I don't know why it was never merged. If you were to update it so it can be merged, I'll take another look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 10, 2011
  1. Failing test: create child in after_create callback

    Jason Stirk authored
    Creating a child in a after_* callback results in the child's ancestry
    column containing only the parent id, and not the entire parent ancestry
    path.
  2. Fix creating children in after_* callbacks

    Jason Stirk authored
    Previously creating children in an after_* callback would cause the
    child's ancestry column to contain just the parent id and not the entire
    ancestry path.
    
    A workaround was to call self.reload in the after_* callback, but this
    commit makes the process work automatically.
    
    The main issue looked to be that the child_ancestry method was using the
    ancestry path before the save (using _was) which for new records was
    obviously blank.
    
    This revision uses the current value of the parent's ancestry, and adds
    a child_ancestry_was method to return the old behaviour (with a more
    descriptive name).
    
    The change to update_descendants_with_new_ancestry was due to it relying
    on the old behaviour. The revised version explicitly replaces the old
    parent ancestry snippet (using child_ancestry_was) with the new one
    path.
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 5 deletions.
  1. +14 −5 lib/ancestry/instance_methods.rb
  2. +16 −0 test/has_ancestry_test.rb
View
19 lib/ancestry/instance_methods.rb
@@ -18,8 +18,8 @@ def update_descendants_with_new_ancestry
descendant.update_attribute(
self.base_class.ancestry_column,
descendant.read_attribute(descendant.class.ancestry_column).gsub(
- /^#{self.child_ancestry}/,
- if read_attribute(self.class.ancestry_column).blank? then id.to_s else "#{read_attribute self.class.ancestry_column }/#{id}" end
+ /^#{self.child_ancestry_was}/,
+ self.child_ancestry
)
)
end
@@ -61,7 +61,16 @@ 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?
- if self.send("#{self.base_class.ancestry_column}_was").blank? then id.to_s else "#{self.send "#{self.base_class.ancestry_column}_was"}/#{id}" end
+ ancestry = self.send(self.ancestry_column)
+ if ancestry.blank? then self.id.to_s else ancestry.to_s + "/#{self.id.to_s}" end
+ end
+
+ def child_ancestry_was
+ # New records cannot have children
+ raise Ancestry::AncestryException.new('No child ancestry for new record. Save record before performing tree operations.') if new_record?
+
+ ancestry = self.send("#{self.ancestry_column}_was")
+ if ancestry.blank? then self.id.to_s else ancestry.to_s + "/#{self.id.to_s}" end
end
# Ancestors
@@ -171,7 +180,7 @@ def is_only_child?
# Descendants
def descendant_conditions
- ["#{self.base_class.table_name}.#{self.base_class.ancestry_column} like ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} = ?", "#{child_ancestry}/%", child_ancestry]
+ ["#{self.base_class.table_name}.#{self.base_class.ancestry_column} like ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} = ?", "#{child_ancestry_was}/%", child_ancestry_was]
end
def descendants depth_options = {}
@@ -184,7 +193,7 @@ def descendant_ids depth_options = {}
# Subtree
def subtree_conditions
- ["#{self.base_class.table_name}.#{self.base_class.primary_key} = ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} like ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} = ?", self.id, "#{child_ancestry}/%", child_ancestry]
+ ["#{self.base_class.table_name}.#{self.base_class.primary_key} = ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} like ? or #{self.base_class.table_name}.#{self.base_class.ancestry_column} = ?", self.id, "#{child_ancestry_was}/%", child_ancestry_was]
end
def subtree depth_options = {}
View
16 test/has_ancestry_test.rb
@@ -448,6 +448,22 @@ def test_node_creation_though_scope
end
end
+ def test_node_creation_in_after_create
+ AncestryTestDatabase.with_model do |model|
+ children=[]
+ model.instance_eval do
+ attr_accessor :idx
+ self.after_create do
+ children << self.children.create!(:idx => self.idx - 1) if self.idx > 0
+ end
+ end
+ node = model.create!(:idx => 3)
+ # In the error case, the ancestry on each item will only contain the parent's id,
+ # and not the entire ancestry tree.
+ assert_equal '1/2/3', children.first.ancestry
+ end
+ end
+
def test_validate_ancestry_exclude_self
AncestryTestDatabase.with_model do |model|
parent = model.create!
Something went wrong with that request. Please try again.