Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Specifying a position with add_new_at: :top fails to insert at that position #220

Merged
merged 3 commits into from
Aug 23, 2016
Merged

Bug: Specifying a position with add_new_at: :top fails to insert at that position #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d4d220
Bug: Test written to confirm bug
brendon Aug 14, 2016
760361d
Make add_to_list_top behave the same as _bottom
brendon Aug 14, 2016
c2c0228
Merge branch 'master' into add_new_at_top_bug
brendon Aug 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions lib/acts_as_list/active_record/acts/list.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,17 +324,24 @@ def acts_as_list_list
end
end

# Poorly named methods. They will insert the item at the desired position if the position
# has been set manually using position=, not necessarily the top or bottom of the list:

def add_to_list_top
increment_positions_on_all_items
self[position_column] = acts_as_list_top
if not_in_list? || internal_scope_changed? && !position_changed || default_position?
increment_positions_on_all_items
self[position_column] = acts_as_list_top
else
increment_positions_on_lower_items(self[position_column], id)
end

# Make sure we know that we've processed this scope change already
@scope_changed = false
#dont halt the callback chain

# Don't halt the callback chain
true
end

# A poorly named method. It will insert the item at the desired position if the position
# has been set manually using position=, not necessarily the bottom of the list
def add_to_list_bottom
if not_in_list? || internal_scope_changed? && !position_changed || default_position?
self[position_column] = bottom_position_in_list.to_i + 1
Expand All @@ -345,7 +352,7 @@ def add_to_list_bottom
# Make sure we know that we've processed this scope change already
@scope_changed = false

#dont halt the callback chain
# Don't halt the callback chain
true
end

Expand Down
30 changes: 18 additions & 12 deletions test/shared_top_addition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,26 @@ def setup
(1..4).each { |counter| TopAdditionMixin.create! pos: counter, parent_id: 5 }
end

def test_reordering
assert_equal [4, 3, 2, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
def test_setup_state
# If we explicitly define a position (as above) then that position is what gets applied
assert_equal [1, 2, 3, 4], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
end

def test_reordering
TopAdditionMixin.where(id: 2).first.move_lower
assert_equal [4, 3, 1, 2], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [1, 3, 2, 4], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

TopAdditionMixin.where(id: 2).first.move_higher
assert_equal [4, 3, 2, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [1, 2, 3, 4], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

TopAdditionMixin.where(id: 1).first.move_to_bottom
assert_equal [4, 3, 2, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [2, 3, 4, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

TopAdditionMixin.where(id: 1).first.move_to_top
assert_equal [1, 4, 3, 2], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [1, 2, 3, 4], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

TopAdditionMixin.where(id: 2).first.move_to_bottom
assert_equal [1, 4, 3, 2], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [1, 3, 4, 2], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

TopAdditionMixin.where(id: 4).first.move_to_top
assert_equal [4, 1, 3, 2], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
Expand Down Expand Up @@ -71,16 +74,19 @@ def test_insert_at
assert_equal 3, new4.pos
end

def test_delete_middle
assert_equal [4, 3, 2, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
def test_supplied_position
new = TopAdditionMixin.create(parent_id: 20, pos: 3)
assert_equal 3, new.pos
end

def test_delete_middle
TopAdditionMixin.where(id: 2).first.destroy

assert_equal [4, 3, 1], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)
assert_equal [1, 3, 4], TopAdditionMixin.where(parent_id: 5).order('pos').map(&:id)

assert_equal 3, TopAdditionMixin.where(id: 1).first.pos
assert_equal 1, TopAdditionMixin.where(id: 1).first.pos
assert_equal 2, TopAdditionMixin.where(id: 3).first.pos
assert_equal 1, TopAdditionMixin.where(id: 4).first.pos
assert_equal 3, TopAdditionMixin.where(id: 4).first.pos
end

end
Expand Down