brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Bug: Possibly duplicated positions if not given `add_new_at: top` option #410

Closed Gee-Bee closed 1 year ago

Gee-Bee commented 1 year ago

According to 0.8.0 Upgrade Notes:

If you specify add_new_at: :top, new items will be added to the top of the list like always. But now, if you specify a position at insert time: .create(position: 3), the position will be respected.

This works as expected, repositioning other items' positions if position is taken.

In my use case:

If user specify position that is already taken, DB is left in inconsistent state (or uniqueness constraint violation error is raised).

Expected result: reordering other items, like in add_new_at: :top scenario.

As a workaround I included the following concern in my class:

module ActsAsListCreateWithPositionFix
  extend ActiveSupport::Concern

  included do
    before_create -> { @_tmp_position = position and self.position = nil }
    after_create -> { insert_at(@_tmp_position) }, if: -> { @_tmp_position.present? }
  end
end

This workaround seems like a "bad example" from the 1) Use the Most Concise API in README:

# Good
TodoItem.create(todo_list: todo_list, position: 1)

# Bad
item = TodoItem.create(todo_list: todo_list)
item.insert_at(1)

but at least it works. The "Good example" generates possible duplicates when add_new_at: :top is not given.

brendon commented 1 year ago

Hi @Gee-Bee, I see what you mean. Would you be interested in creating a PR to fix this? I'll take a look too but am pretty short on time lately. If you have any ideas please let me know :)

brendon commented 1 year ago

Actually, I think I see a hopefully easy solution to the problem. I'll keep you posted.

brendon commented 1 year ago

Try https://github.com/brendon/acts_as_list/tree/avoid_collision to see if that helps. I've added tests that confirm it works.

brendon commented 1 year ago

Actually, since the tests are green I've merged it with master. Give master a go and see how you get on :)

Gee-Bee commented 1 year ago

Wow, that was fast. Tried master branch. It's working like a gold. You managed to resolve the issue and clean up the code along the way. Big props for your work.

brendon commented 1 year ago

Thanks @Gee-Bee! :) I'll spin a new release.