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

Issue with `move_to_bottom` and `bottom cannot be lower than top` with an empty list #420

Closed arielj closed 1 year ago

arielj commented 1 year ago

When calling move_to_bottom, it tries to insert the element at the bottom of the list calling bottom_position_in_list.to_i

https://github.com/brendon/acts_as_list/blob/f0daafb0251c48f1c7badfbd91043e8d59b48f21/lib/acts_as_list/active_record/acts/list.rb#L115

If there's no current bottom_item, this method returns acts_as_list_top - 1:

https://github.com/brendon/acts_as_list/blob/f0daafb0251c48f1c7badfbd91043e8d59b48f21/lib/acts_as_list/active_record/acts/list.rb#L268

So, if acts_as_list_top is 1 (the default), then this tries to insert the element at position 0 of the list.

But then, if position is < than acts_as_list_top, it raises an exception:

https://github.com/brendon/acts_as_list/blob/f0daafb0251c48f1c7badfbd91043e8d59b48f21/lib/acts_as_list/active_record/acts/list.rb#L383

So the code itself is setting acts_as_list_top - 1 and then failing with that.

I want to make a PR updating this, but I wanted to be sure what's the expected behavior here. I feel like bottom_position_in_list should fallback to acts_as_list_top instead of acts_as_list_top - 1, so it puts the element at the first position if the list is empty.

EDIT: I was playing around with this and looks like changing bottom_position_in_list creates issues (first element added is 2 instead of 1, but looks like changing insert_at_position to raise if position < acts_as_list_top - 1 work better

brendon commented 1 year ago

Hi @arielj, just giving this a quick initial glance. If the list is empty (which it would be if there's no bottom_posotion_in_list) then the method would return early here: https://github.com/brendon/acts_as_list/blob/f0daafb0251c48f1c7badfbd91043e8d59b48f21/lib/acts_as_list/active_record/acts/list.rb#L114C16-L114C16

Essentially you can't move an item to the bottom of the list if it's not in the list already. You first need to put it in the list with either an explicit position or just lets acts_as_list do that for you. Then move it there yourself. Of course, the default is to add the item to the bottom of the list anyway.

Let me know if you need more help :)

arielj commented 1 year ago

I see, thanks

I did more digging in the application I'm working on and I found the issue. The code was calling move_to_bottom before saving the object (so it was not yet part of the list in the database), that's why acts_as_list was not finding the item to move it and falling back. This was working on an older version of acts_as_list but updating the gem uncovered this inconsistency. So I just swapped some lines to first save the element so the db gets updated and then call move_to_bottom.

Thanks!

brendon commented 1 year ago

No worries @arielj. I don't think you'd need to call move_to_bottom at all unless you have overridden the default and are adding to the top of the list? That's a configuration setting, and will also happen if you set the position to be 1 or 0 when creating the item. If you don't set a position, it should normally default to the bottom of the list.

arielj commented 1 year ago

Just to give more context, I'm upgrading a legacy app and the case here is that the item is already created in the db but in another list (acts_as_list is configured with a scope) so it already has a position.

The action is about moving elements between lists, so it's changing the attribute for that scope (making it part of the another "list") and them calling move_to_bottom to update its position. But when the new list was empty it created this issue. By first moving the element to the new list and saving it, the list is not empty anymore and we can move its position.

brendon commented 1 year ago

No worries @arielj. I wonder if it's work to just change the scope column and set position to null in the same update to get the default behaviour of adding to the bottom of the list?