acts_as_list
acts_as_list copied to clipboard
Position update issue when creating with explicit positions
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.
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.
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.
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 :)