brendon / ranked-model

An acts_as_sortable/acts_as_list replacement built for Rails 4+
https://github.com/mixonic/ranked-model
MIT License
1.09k stars 134 forks source link

Order is incorrect when updating multiple records with accepts_nested_attributes_for #184

Closed fcheung closed 3 months ago

fcheung commented 2 years ago

I've come across an issue while using ranked model and updating the position of multiple records in one go. I've written a failing test case here: https://github.com/fcheung/ranked-model/tree/multiple-updates

In the app I'm working on, conceptually we have lists, whose items are ordered with ranked model. The UI allows the user to reorder the list items (& make changes to their content). When they hit the save button the new list, with the row_order_position attribute set to what the new position of the item should be, is sent to the app backend, where it is saved using accepts_nested_attribute. Activerecord assigns all those row_order_positions and then we call save on the List object.

This is where the problem occurs: when activerecord saves the objects from the associations, it traverses the objects in the order that they are in the association target - in general this is the current order from the db. if the array of objects to saved looked like

{row_order_position: 2}
{row_order_position: 3}
{row_order_position: 1}
{row_order_position: 0}

then the end result is incorrect - the rows moved early on end up getting moved a second time when objects are added/removed ahead of them

I can work around this with an after save that looks like this

def fix_sort_order_around_save
    list_items.target.sort_by!.with_index {|l| [ -(l.row_order_position || -1)]}
    yield
    list_items.target.sort_by! {|record| record.row_order}
end

but this is a bit clunky (and assumes you only ever use integer positions). I'm not sure how ranked-model could address this since an object being saved has no knowledge of other objects around it being updated, but if I have overlooked something I'd love to hear it

brendon commented 2 years ago

Hi @fcheung, my instinct says that this is happening because the callbacks are relying on stale position information. After each item is moved you'd need to reload the rest so that they knew their correct place in the rank. Reloading the records would also overwrite any other changes that you'd made to them though, so that's problematic.

Did you think of any more ideas around the problem?

brendon commented 2 years ago

It might be that you'll need to not use accepts_nested_attributes and just manually loop the incoming list and update each list item one at a time (Which is what happens eventually anyway). It's less clean for sure.

fcheung commented 2 years ago

I don’t think it is stale data - I think it’s more that the _position aren’t converted to the raw values atomically - if you move an item from position 4 to position 3 and then move an item from position 10 to position 1 then that moves all those items up by 1, so that item you have moved from 4 to 3 is now at 4 again.

brendon commented 2 years ago

Ah I see, that's pretty tricky! Essentially your incoming positions become incorrect after the first one is saved (potentially).

I don't see an elegant way around this. I don't think nested attributes will help the situation.

It might be better to have your own custom update routine that takes all of this into account (As you've done).

brendon commented 3 months ago

@fcheung, this PR solved this issue I think (hope!): https://github.com/brendon/ranked-model/commit/f9145d42041ed7186a893315350f01f87d39c76b

Try 0.4.9 to see if that works better :D