fix setting position when previous position was nil #230
Conversation
There was a problem hiding this comment.
Thanks for this @StoneFrog :) Glad to finally be getting on top of this bug. Can you address my inline comments and we'll go from there :)
My thoughts with the bottom_position_in_list were that maybe there's a better way to get this value. Are we generating it like this elsewhere in the code?
| @@ -468,7 +468,7 @@ def store_at_0 | |||
| end | |||
|
|
|||
| def update_positions | |||
| old_position = send("#{position_column}_was").to_i | |||
There was a problem hiding this comment.
I wonder why we were converting to_i?
There was a problem hiding this comment.
As I wrote below, the only reason I can think of, is to prevent errors in shuffle_positions_on_intermediate_items when old_position is nil
| @@ -468,7 +468,7 @@ def store_at_0 | |||
| end | |||
|
|
|||
| def update_positions | |||
| old_position = send("#{position_column}_was").to_i | |||
| old_position = send("#{position_column}_was") || bottom_position_in_list + 1 | |||
There was a problem hiding this comment.
Does the bug disappear when we just remove the to_i? Maybe the rest isn't needed. I'm not sure though :) Let me know.
There was a problem hiding this comment.
I would say it's definitely needed. Without to_i old_position will be set as nil which will cause error in shuffle_positions_on_intermediate_items method which does comparison old_position < new_position.
I guess that to_i might have been originaly used to patch this error since it converts nil to 0. But in that case we have this reordering issue. When nil is converted to 0, in the shuffle_positions_on_intermediate_items old_position is considered to be lower than new_position, so it assumes that element that was nil was closer to the top of the list and as a result moves old element closer to the top (so we would have [0, 1] instead of [1, 2]).
After this tweak problem is solved, since if position is nil we simulate a situation when it is at the bottom of the list, so shuffle_positions_on_intermediate_items is feeded with proper data that allows all items below specific position be properly reordered.
I hope I clarified it enough, I did my best at least. Let me know if you need anything else.
There was a problem hiding this comment.
Thanks @StoneFrog, that's a great explanation :) Thanks for putting my mind at rest. I'll merge this.
Basically, when position is nil and we try to set new position, it didn't handle reording of other items properly. Fixes issue #109