Browse files

fixing bug when holes are in list

  • Loading branch information...
1 parent 8fb5608 commit 672e13fd90199bc1226c0a21a33eaf89aa53858b @ryanb committed Jul 21, 2008
Showing with 47 additions and 10 deletions.
  1. +10 −10 lib/acts_as_list.rb
  2. +37 −0 test/list_test.rb
View
20 lib/acts_as_list.rb
@@ -79,21 +79,21 @@ def insert_at(position = 1)
# Swap positions with the next lower item, if one exists.
def move_lower
- return unless lower_item
-
+ lower = lower_item
+ return unless lower
acts_as_list_class.transaction do
- lower_item.decrement_position
- increment_position
+ self.update_attribute(position_column, lower.send(position_column))
+ lower.decrement_position
end
end
# Swap positions with the next higher item, if one exists.
def move_higher
- return unless higher_item
-
+ higher = higher_item
+ return unless higher
acts_as_list_class.transaction do
- higher_item.increment_position
- decrement_position
+ self.update_attribute(position_column, higher.send(position_column))
+ higher.increment_position
end
end
@@ -153,15 +153,15 @@ def last?
def higher_item
return nil unless in_list?
acts_as_list_class.find(:first, :conditions =>
- "#{scope_condition} AND #{position_column} = #{(send(position_column).to_i - 1).to_s}"
+ "#{scope_condition} AND #{position_column} < #{send(position_column).to_s}", :order => "#{position_column} DESC"
)
end
# Return the next lower item in the list.
def lower_item
return nil unless in_list?
acts_as_list_class.find(:first, :conditions =>
- "#{scope_condition} AND #{position_column} = #{(send(position_column).to_i + 1).to_s}"
+ "#{scope_condition} AND #{position_column} > #{send(position_column).to_s}", :order => "#{position_column} ASC"
)
end
View
37 test/list_test.rb
@@ -221,6 +221,43 @@ def test_remove_before_destroy_does_not_shift_lower_items_twice
assert_equal 3, ListMixin.find(4).pos
end
+ # special thanks to openhood on github
+ def test_delete_middle_with_holes
+ # first we check everything is at expected place
+ assert_equal [1, 2, 3, 4], ListMixin.find(:all, :conditions => 'parent_id = 5', :order => 'pos').map(&:id)
+
+ # then we create a hole in the list, say you're working with existing data in which you already have holes
+ # or your scope is very complex
+ ListMixin.delete(2)
+
+ # we ensure the hole is really here
+ assert_equal [1, 3, 4], ListMixin.find(:all, :conditions => 'parent_id = 5', :order => 'pos').map(&:id)
+ assert_equal 1, ListMixin.find(1).pos
+ assert_equal 3, ListMixin.find(3).pos
+ assert_equal 4, ListMixin.find(4).pos
+
+ # can we retrieve lower item despite the hole?
+ assert_equal 3, ListMixin.find(1).lower_item.id
+
+ # can we move an item lower jumping more than one position?
+ ListMixin.find(1).move_lower
+ assert_equal [3, 1, 4], ListMixin.find(:all, :conditions => 'parent_id = 5', :order => 'pos').map(&:id)
+ assert_equal 2, ListMixin.find(3).pos
+ assert_equal 3, ListMixin.find(1).pos
+ assert_equal 4, ListMixin.find(4).pos
+
+ # create another hole
+ ListMixin.delete(1)
+
+ # can we retrieve higher item despite the hole?
+ assert_equal 3, ListMixin.find(4).higher_item.id
+
+ # can we move an item higher jumping more than one position?
+ ListMixin.find(4).move_higher
+ assert_equal [4, 3], ListMixin.find(:all, :conditions => 'parent_id = 5', :order => 'pos').map(&:id)
+ assert_equal 2, ListMixin.find(4).pos
+ assert_equal 3, ListMixin.find(3).pos
+ end
end
class ListSubTest < Test::Unit::TestCase

0 comments on commit 672e13f

Please sign in to comment.