brendon / acts_as_list

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

Position update issue when creating with explicit positions #289

Closed fnordfish closed 3 months ago

fnordfish commented 6 years ago

Given the models, where the Product has many ProductImage (which "acts_as_list").

When creating some referenced object and set the position explicitly (in random order), the positions will not be homogeneous:

image_2 = product.product_images.create(position: 2)
image_3 = product.product_images.create(position: 3)
image_1 = product.product_images.create(position: 1)

pp image_1.slice(:id, :position)
#=> {"id"=>3, "position"=>1}
pp image_2.slice(:id, :position)
#=> {"id"=>1, "position"=>2}
pp image_3.slice(:id, :position)
#=> {"id"=>2, "position"=>3}

# now, after reload:

pp image_1.reload.slice(:id, :position)
#=> {"id"=>3, "position"=>1}
pp image_2.reload.slice(:id, :position)
#=> {"id"=>1, "position"=>3}
pp image_3.reload.slice(:id, :position)
#=> {"id"=>2, "position"=>4}

When creating in reverse order (positions [3,2,1]):

image_3 = product.product_images.create(position: 3)
image_2 = product.product_images.create(position: 2)
image_1 = product.product_images.create(position: 1)

pp image_1.reload.slice(:id, :position)
#=> {"id"=>3, "position"=>1}
pp image_2.reload.slice(:id, :position)
#=> {"id"=>2, "position"=>3}
pp image_3.reload.slice(:id, :position)
#=> {"id"=>1, "position"=>5}

Of course, creating in correct order works just fine.

brendon commented 6 years ago

Hi @fnordfish, that's an interesting one. Do you have time to dig into the code and find out why this is happening? Start a PR with one or two failing tests and let's see where it goes :) I'm a bit stretched for time over the next two weeks.

fnordfish commented 6 years ago

Hi @brendon , here is a very first step in fixing this: https://github.com/fnordfish/acts_as_list/commit/90f38c2cdef5a2fafb351bc79ea28a93cceeae8e

There are probably a ton of tests missing to make this an actual fix. The underlying problem is, that when inserting a new "higher" position triggers an increment of all "lower" ones (pushing them even lower). Which makes sense, if that new position would collide with an existing one. But if the spot is free, there's no need to change anything.

brendon commented 6 years ago

Thanks @fnordfish, ah yes we've had that raised in the past. It also is a concern when there is two items with the same position in a scope. There's nothing to rebalance those at present. We can certainly work toward not incrementing items above or below if there is an integer space there. Why don't you turn this into a PR and we can go from there. Keep contributing if you want, but I'll be unable to do more than cheer you on for a couple of weeks :)

Santhanu-Suruliram commented 3 months ago

@brendon, @fnordfish Is this issue resolved?

fnordfish commented 3 months ago

Hi. I have no way of testing this right now. Proceed as you please:)

brendon commented 3 months ago

@Santhanu-Suruliram, this issue isn't resolved. I'd suggest you check out my new positioning gem: https://github.com/brendon/positioning

It works hard to prevent gaps and has automatic clamping so you can't add a record with a position outside the range of 1..last_position_in_the_list + 1. It encourages relative positioning (so declare an items position relative to other items) but you can provide a specific position as long as it fits within those bounds. You could make an item with position 4 in a list with only one item with position 1. The new item would end up with position 2.