Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

remove some wordy lines of code and changed #move_to_root implementation #176

Merged
merged 1 commit into from

2 participants

@yuszuv

Hi,

I did a slight refactoring on the code.

  • the where clause added in 5395e63 should be now more readable
  • node.move_to_root is now implemented as node.move_to_right_of(node.root), because in the current implementation node.lft is set to 1, which can cause a performance issue (namely: it updates all records!) if you have many roots and you want to move one subtree of the last root to root level
  • all move_to variants are able to take record objects too, so the comments, that the can't are removed.

I hope that helps.

Best regards

Jan

@parndt
Owner

@yuszuv thanks for this.. unfortunately it seems like it's having trouble in some of the configurations that we support - are you able to take a look please?

@yuszuv

I'd like to be able to take a look, but I'm not. I've never used travis so far, so I don.t know how to use or how to develop und run the travis specs locally. But if you give me some hints, I will try.

But anyway, it doesn't look to me, that my commit let the travis build fail. The failing tests with 'mysql' don't complete becaues of

$ bundle exec rspec spec
NameError: uninitialized constant Combustion::Database::Mysql2

and the 'postgresql' tests fail on methods that never touch any of my changes. So is there perhaps any 'bug' at travis or isn't the travis setup 100% correct?

@parndt
Owner

@yuszuv I fixed the specs :-)

@parndt parndt merged commit acd09d3 into from
@parndt
Owner

Epic refactor -- thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
43 lib/awesome_nested_set/awesome_nested_set.rb
@@ -421,22 +421,22 @@ def move_right
move_to_right_of right_sibling
end
- # Move the node to the left of another node (you can pass id only)
+ # Move the node to the left of another node
def move_to_left_of(node)
move_to node, :left
end
- # Move the node to the left of another node (you can pass id only)
+ # Move the node to the left of another node
def move_to_right_of(node)
move_to node, :right
end
- # Move the node to the child of another node (you can pass id only)
+ # Move the node to the child of another node
def move_to_child_of(node)
move_to node, :child
end
- # Move the node to the child of another node with specify index (you can pass id only)
+ # Move the node to the child of another node with specify index
def move_to_child_with_index(node, index)
if node.children.empty?
move_to_child_of(node)
@@ -449,7 +449,7 @@ def move_to_child_with_index(node, index)
# Move the node to root nodes
def move_to_root
- move_to nil, :root
+ move_to_right_of(root)
end
# Order children in a nested set by an attribute
@@ -610,12 +610,11 @@ def move_to(target, position)
raise ActiveRecord::ActiveRecordError, "You cannot move a new node" if self.new_record?
run_callbacks :move do
in_tenacious_transaction do
- if target.is_a? self.class.base_class
- target.reload_nested_set
- elsif position != :root
- # load object if node is not an object
- target = nested_set_scope.find(target)
- end
+ target = if target.is_a? self.class.base_class
+ target.reload
+ else
+ nested_set_scope.find(target)
+ end
self.reload_nested_set
unless position == :root || move_possible?(target)
@@ -626,7 +625,6 @@ def move_to(target, position)
when :child; target[right_column_name]
when :left; target[left_column_name]
when :right; target[right_column_name] + 1
- when :root; 1
else raise ActiveRecord::ActiveRecordError, "Position should be :child, :left, :right or :root ('#{position}' received)."
end
@@ -651,29 +649,10 @@ def move_to(target, position)
new_parent = case position
when :child; target.id
- when :root; nil
else target[parent_column_name]
end
- where_statement = ["not (#{quoted_left_column_name} = CASE " +
- "WHEN #{quoted_left_column_name} BETWEEN :a AND :b " +
- "THEN #{quoted_left_column_name} + :d - :b " +
- "WHEN #{quoted_left_column_name} BETWEEN :c AND :d " +
- "THEN #{quoted_left_column_name} + :a - :c " +
- "ELSE #{quoted_left_column_name} END AND " +
- "#{quoted_right_column_name} = CASE " +
- "WHEN #{quoted_right_column_name} BETWEEN :a AND :b " +
- "THEN #{quoted_right_column_name} + :d - :b " +
- "WHEN #{quoted_right_column_name} BETWEEN :c AND :d " +
- "THEN #{quoted_right_column_name} + :a - :c " +
- "ELSE #{quoted_right_column_name} END AND " +
- "#{quoted_parent_column_name} = CASE " +
- "WHEN #{self.class.base_class.primary_key} = :id THEN :new_parent " +
- "ELSE #{quoted_parent_column_name} END)" ,
- {:a => a, :b => b, :c => c, :d => d, :id => self.id, :new_parent => new_parent} ]
-
-
-
+ where_statement = ["(#{quoted_left_column_name} BETWEEN :a AND :d) OR (#{quoted_right_column_name} BETWEEN :a AND :d)", {:a => a, :d => d}]
self.nested_set_scope.where(*where_statement).update_all([
"#{quoted_left_column_name} = CASE " +
View
4 spec/awesome_nested_set_spec.rb
@@ -433,8 +433,8 @@
categories(:child_2).parent.should be_nil
categories(:child_2).level.should == 0
categories(:child_2_1).level.should == 1
- categories(:child_2).left.should == 1
- categories(:child_2).right.should == 4
+ categories(:child_2).left.should == 7
+ categories(:child_2).right.should == 10
Category.valid?.should be_true
end
Something went wrong with that request. Please try again.