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! 😄), 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 :)

@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.

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.