Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added ability to set item positions directly (e.g. In a form) #38

Merged
merged 5 commits into from

3 participants

@dubroe

I'm working on an e-commerce site and one aspect of the site is that vendors are able to manipulate their menus. Here is an example of a test menu: https://skitch.com/dubroe/86en1/rhubs-menu-creator-test.

When a menu item is clicked, the vendor is presented with a form in which he can update the menu item attributes, including the item's position in the list:
https://skitch.com/dubroe/86ena/rhubs-edit-menu-item-test
https://skitch.com/dubroe/86edt/screen-shot-2012-06-02-at-1.58.03-pm

Before this patch, doing so messed everything up. Say I had 8 items in the menu, and I wanted to move the item in position 6 to position 4, this would end up with the positions being: 1,2,3,4,4,5,7,8. Basically the other positions weren't updated as they should be.

I've added an "after_update :update_positions" callback which checks to see if two items have the same position, and if they do, update the positions using the already existing "shuffle_positions_on_intermediate_items" function.

I had to make a small change to that function so that you can pass in an ID to ignore, so that the recently changed position doesn't get changed right after it was set.

Please let me know if you have any questions or issues with my code. I'm happy to make any modifications that you suggest. Overall though I think this is an important feature to add to this gem as without it lists are vulnerable to being corrupted if people manually update item positions.

Thanks,

~Elan Dubrofsky

@dubroe

Did you get a chance to read this by any chance? Curious to know your thoughts.

@swanandp
Owner

I didn't get time to try it out, but I am going to merge it and review later! Thanks for contributing!

@swanandp swanandp closed this
@swanandp swanandp reopened this
@swanandp swanandp merged commit b716182 into swanandp:master
@zhengjia

@dubroe @swanandp
It seems this pull request causes a problem when you have a scope in the list. For example acts_as_list :scope => :category. I have to downgrade the gem to 0.16 to get it working correctly

@dubroe

Haven't had a chance to look at this yet. @swanandp and @zhengjia do either of you have any insight into why it's breaking? @zhengjia any chance you can share some code to make it easier for me to recreate and then debug?

@zhengjia

@swanandp and @dubroe
This is a failing test: zhengjia@3df5412

  1) Failure:
test_reordering(ScopedListTest) [/Users/zjia/code/acts_as_list/test/test_list.rb:365]:
<[4, 1, 2, 3]> expected but was
<[4, 1, 1, 2]>.
@swanandp
Owner

@zhengjia Thanks for following up on this, I am on it now.

@dubroe

Ok great. Let me know if you'd like me to take a crack.

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.
Showing with 24 additions and 4 deletions.
  1. +12 −4 lib/acts_as_list/active_record/acts/list.rb
  2. +12 −0 test/test_list.rb
View
16 lib/acts_as_list/active_record/acts/list.rb
@@ -77,6 +77,7 @@ def position_column
after_destroy :decrement_positions_on_lower_items
before_create :add_to_list_#{configuration[:add_new_at]}
+ after_update :update_positions
EOV
end
end
@@ -276,16 +277,16 @@ def increment_positions_on_all_items
end
# Reorders intermediate items to support moving an item from old_position to new_position.
- def shuffle_positions_on_intermediate_items(old_position, new_position)
+ def shuffle_positions_on_intermediate_items(old_position, new_position, avoid_id = nil)
return if old_position == new_position
-
+ avoid_id_condition = avoid_id ? " AND id != #{avoid_id}" : ''
if old_position < new_position
# Decrement position of intermediate items
#
# e.g., if moving an item from 2 to 5,
# move [3, 4, 5] to [2, 3, 4]
acts_as_list_class.update_all(
- "#{position_column} = (#{position_column} - 1)", "#{scope_condition} AND #{position_column} > #{old_position} AND #{position_column} <= #{new_position}"
+ "#{position_column} = (#{position_column} - 1)", "#{scope_condition} AND #{position_column} > #{old_position} AND #{position_column} <= #{new_position}#{avoid_id_condition}"
)
else
# Increment position of intermediate items
@@ -293,7 +294,7 @@ def shuffle_positions_on_intermediate_items(old_position, new_position)
# e.g., if moving an item from 5 to 2,
# move [2, 3, 4] to [3, 4, 5]
acts_as_list_class.update_all(
- "#{position_column} = (#{position_column} + 1)", "#{scope_condition} AND #{position_column} >= #{new_position} AND #{position_column} < #{old_position}"
+ "#{position_column} = (#{position_column} + 1)", "#{scope_condition} AND #{position_column} >= #{new_position} AND #{position_column} < #{old_position}#{avoid_id_condition}"
)
end
end
@@ -317,6 +318,13 @@ def store_at_0
decrement_positions_on_lower_items(old_position)
end
end
+
+ def update_positions
+ old_position = send("#{position_column}_was").to_i
+ new_position = send(position_column).to_i
+ return unless acts_as_list_class.where("#{position_column} = #{new_position}").count > 1
+ shuffle_positions_on_intermediate_items old_position, new_position, id
+ end
end
end
end
View
12 test/test_list.rb
@@ -234,6 +234,18 @@ def test_insert_at
new4.reload
assert_equal 4, new4.pos
end
+
+ def test_update_position
+ assert_equal [1, 2, 3, 4], DefaultScopedMixin.find(:all).map(&:id)
+ DefaultScopedMixin.find(2).update_attribute(:pos, 4)
+ assert_equal [1, 3, 4, 2], DefaultScopedMixin.find(:all).map(&:id)
+ DefaultScopedMixin.find(2).update_attribute(:pos, 2)
+ assert_equal [1, 2, 3, 4], DefaultScopedMixin.find(:all).map(&:id)
+ DefaultScopedMixin.find(1).update_attribute(:pos, 4)
+ assert_equal [2, 3, 4, 1], DefaultScopedMixin.find(:all).map(&:id)
+ DefaultScopedMixin.find(1).update_attribute(:pos, 1)
+ assert_equal [1, 2, 3, 4], DefaultScopedMixin.find(:all).map(&:id)
+ end
end
Something went wrong with that request. Please try again.