Fix non regular sequence movement

[tiagotex]

223 Introduced a bug, by adding the branch condition to move_lower and move_higher, the order is still changed but if you are using a before save callback to spy on position change, the position for the second element is the same as the original due to a mistake in lib/acts_as_list/active_record/acts/list.rb:221

Previously:

# lib/acts_as_list/active_record/acts/list.rb:74
def move_lower
  return unless lower_item

  acts_as_list_class.transaction do
    if lower_item.send(position_column) == self.send(position_column)
      swap_positions(lower_item, self)
    else
      lower_item.decrement_position
      increment_position
    end
  end
end

This condition is checking that booth elements don't have the same position.

This changed behaviour not only for non regular sequence positions, but for all elements that don't have the same position, I argue this should only be entered for elements with a absolute distance between them bigger that 1.

Elements with a pair of positions [1,2] and [1,5] will enter this condition, was this on propose?

swap_position doesn't do the same as the other part of the conditional branch for elements with positions 1 and 2.

# lib/acts_as_list/active_record/acts/list.rb:219
def swap_positions(item1, item2)
  item1.set_list_position(item2.send(position_column))
  item2.set_list_position(item1.send("#{position_column}_was"))
end

item1.send("#{position_column}_was") returns the same as item1.send(position_column) so my proposed solution it's to store item1 original position in a variable before we change it's position.

[brendon]

Hi @tiagotex, thanks for looking into this. Does assert_equal [5, 10, 15, 20], ListMixin.where(parent_id: 5000).map(&:pos) fail without your patch?

I must say I don't quite get the gist of your issue unless you're saying that our test coverage wasn't complete (not surprising! :smile:), and wasn't catching that _was wasn't returning what was required.

I'm happy to accept the change since it's fairly straightforward but would like to get a better understanding of what the problem was :)

[tiagotex]

@brendon No, assert_equal [5, 10, 15, 20], ListMixin.where(parent_id: 5000).map(&:pos) is only to assert the initial state.

ListMixin.where(id: 2).first.move_lower
assert_equal [1, 3, 2, 4], ListMixin.where(parent_id: 5000).order('pos').map(&:id)
assert_equal [5, 15, 10, 20], ListMixin.where(parent_id: 5000).map(&:pos)
# before the change this would return [5, 11, 10, 20]

---

I think I digressed to much in my explanation. The real problem that brought me to this discover was that my tests would fail because we hook into a before save callback and log the new position, and since item1.send("#{position_column}_was") was returning the same as item1.send(position_column) this would break the logs. I think it's easier if we look at code:

# when sending item1 with position 1 and item2 with position 2
def swap_positions(item1, item2)
  # item1.send(position_column)          => 1
  # item2.send(position_column)          => 2
  item1.set_list_position(item2.send(position_column)) # => item1.set_list_position(2)
  # item1.send(position_column)          => 2
  # item2.send(position_column)          => 2
  # item1.send("#{position_column}_was") => 2
  item2.set_list_position(item1.send("#{position_column}_was")) # => item2.set_list_position(2)
end

As you can see in item2.set_list_position we should send the previous position of item1 but we send the current position.

In the new implementation:

# when sending item1 with position 1 and item2 with position 2
def swap_positions(item1, item2)
  item1_position = item1.send(position_column) #=> 1
  # item1.send(position_column)          => 1
  # item2.send(position_column)          => 2
  item1.set_list_position(item2.send(position_column)) # => item1.set_list_position(2)
  # item1.send(position_column)          => 2
  # item2.send(position_column)          => 2
  # item1_position                       => 1
  item2.set_list_position(item1_position) # => item2.set_list_position(1)
end

I would like to do some more researching into acts_as_list to verify we don't have more inconsistencies but this patch could be merged for now, since I don't think I'm the only one who encountered this problem and this change in behaviour was introduced in a minor version unintentionally.

[brendon]

Thanks @tiagotex, that makes more sense. The strange part is why the _was isn't working as expected. I suspect that because the record is saved, the _was value goes away?

def set_list_position(new_position)
  write_attribute position_column, new_position
  save(validate: false)
end

Or is it something to do with (validate: false).

Anyway, there's no harm in caching the value locally instead of using the dirty functionality.

Just looking at the documentation, we should probably have accessed .previous_changes.