acts_as_list icon indicating copy to clipboard operation
acts_as_list copied to clipboard

Position update issue when creating with explicit positions

Open fnordfish opened this issue 7 years ago • 3 comments

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.

fnordfish avatar Nov 06 '17 13:11 fnordfish

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.

brendon avatar Nov 06 '17 20:11 brendon

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.

fnordfish avatar Nov 07 '17 11:11 fnordfish

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 :)

brendon avatar Nov 07 '17 23:11 brendon