brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Position not updated when moving item between scopes #370

Closed markoj3s closed 4 years ago

markoj3s commented 4 years ago

Let's say we have:

item X inside scope A at position 1 item Y inside scope B at position 1

If I try to move item X from scope A to scope B (to position 1), item Y will remain at position 1 (instead of being moved to position 2) and the model will end up with 2 items which have same scopes and positions.

Also, in the case of:

item X inside scope A at position 1 item Y inside scope A at position 2

If I try to move item X into scope B, item Y will remain at position 2 instead of being moved to position 1.

Maybe I am not using properly the gem, or it is simply a bug ?

brendon commented 4 years ago

Hi @markoj3s, if you update the scope, the positions should be handled automatically. Generally the moved item will be added to the end of the list.

Can you show me your code?

markoj3s commented 4 years ago

Hi @brendon, thank you for your prompt reply. Sorry I have noticed your answer just now.

The code is quite straightforward and looks like:

@member.update_column(:position, params[:position])
@member.insert_at!(params[:rank])

I've checked acts_as_list source code, and seems like somewhere there is a logic like "if old position == new position, then return and don't re-order the list". So if position is the same but scope changed, the list in the new scope won't be re-ordered I believe.

Inside below method

def insert_at_position(position, raise_exception_if_save_fails=false)
    raise ArgumentError.new("position cannot be lower than top") if position < acts_as_list_top
    return set_list_position(position, raise_exception_if_save_fails) if new_record?
    with_lock do
        if in_list?
            old_position = current_position
            return if position == old_position
            # temporary move after bottom with gap, avoiding duplicate values
            # gap is required to leave room for position increments
            # positive number will be valid with unique not null check (>= 0) db constraint
            temporary_position = bottom_position_in_list + 2
            set_list_position(temporary_position, raise_exception_if_save_fails)
            shuffle_positions_on_intermediate_items(old_position, position, id)
        else
          increment_positions_on_lower_items(position)
        end
        set_list_position(position, raise_exception_if_save_fails)
    end
end

return if position == old_position might be the reason, unless I missed something?

brendon commented 4 years ago

Hi @markoj3s, you shouldn't be using update_column as that issues UPDATE SQL directly and skips all the callbacks on the model. We rely on these callbacks to detect scope change and reorder things.

Instead:

@member.update(position: params[:position]) # To change the position but not the scope
@member.update(the_scope: params[:new_scope]) # To change the scope - positions will be automatically handled.

Let me know how you get on :)

markoj3s commented 4 years ago

@brendon wow my bad... I thought that I had to use insert_at method to update the position which made me update the table with two different statements (and unfortunately use update_column). Indeed by updating in a single statement with update it is properly adjusting the elements in the old and new scope ! Sorry for having bothered you for nothing...

brendon commented 4 years ago

@markoj3s that's not a problem at all. Don't feel bad! I'm glad we were able to figure it out. And this thread will no-doubt help others in the future :)

markoj3s commented 4 years ago

Thanks ! Hopefully this would be useful then !

While we're at it, there is one more thing I would like to double check about the gem's behavior :) If I do @member.update(the_scope: params[:new_scope], position: params[:position]) having params[:position] being equal to 2 and originally no item in the new_scope, is the gem supposed to automatically set the position to 1 ? Or will it keep the position to 2 ?

brendon commented 4 years ago

Hi @markoj3s, my intuition would say that the gem will respect your desired position. It should still shuffle things around to accomodate your intentions on the new list. If you don't care about the position, don't set it (it'll append to the start or end of the list per your configuration), but if you do care, I think it's safe to set it (e.g. drag and drop where you know both lists).