Don't try to reload new records

[tfausak]

This should fix #122.

[houndci-bot]

Redundant self detected.

[tfausak]

Thanks for the help, Hound.

[brendon]

Is self required here? I get the funny feeling that it's for STI support...

[tfausak]

It is not required. I left it there because I wanted to change as little as possible.

[brendon]

I'd like to remove it but I remember doing something like this before and then it got put back because it broke the way STI worked. But I think it was more to do with returning the class name for the AAL model.

[brendon]

Yea I can't see the harm in removing it if all the tests pass. It's not like it's retuning anything we need.

[tfausak]

I removed the redundant self.

[brendon]

Thanks for that. Can you give me a good use case for trying to delete a records that isn't persisted yet? I can see your example in the issue, but that one seems a bit contrived.

Also, I'm wondering if we should do:

reload if persisted? as that means we won't reload destroyed objects too (I think).

[tfausak]

I hit this case in a test suite. I was using non-persisted objects and destroying them to test callbacks. Something like this:

it do
  list = List.new
  element = list.elements.build
  expect(element).to receive(:destroy)
  list.destroy
end

[brendon]

Ah I see. I suppose it's reasonable that we don't raise an error when Rails doesn't raise an error upon destroying non-persisted objects.

How about

before_destroy :reload, if: :persisted?

Saves a few lines.

Though, the only reason we're reloading is for the purposes of the after_destroy method. Perhaps the reloading needs to happen here (decrement_positions_on_lower_items) instead?

[tfausak]

¯_(ツ)_/¯ Doesn't matter to me. I'm not familiar with the internals of this library. However you want to get it done is good with me.

[brendon]

Lol, such enthusiasm ;) I'm thinking perhaps that row should be locked before deletion. Could you try updating your branch with this instead?

before_destroy :lock!

If that causes the same error for you then try:

before_destroy :lock!, if: :persisted?

if that works then I think that's the best outcome :)

[tfausak]

Neither of those worked. Here's the test case I dropped into test/test_list.rb:

class List < ActiveRecord::Base
  has_many :elements
end

class Element < ActiveRecord::Base
  belongs_to :list
  acts_as_list scope: :list
end

class X < ActsAsListTestCase
  def setup
    ActiveRecord::Schema.define do
      create_table :lists
      create_table :elements do |table|
        table.integer :list_id
        table.integer :position
      end
    end
  end

  def test_x
    list = List.new
    element = list.elements.build
    begin
      list.elements.each &:destroy
      assert true
    rescue
      assert false
    end
  end
end

[brendon]

That's strange. Try this branch:

https://github.com/swanandp/acts_as_list/tree/lock-before-destroy

lock! conditionally calls reload if the record is persisted:

def lock!(lock = true)
  reload(:lock => lock) if persisted?
  self
end

I've even added a failing test. It's as simple as building a new (un-persisted) record (I don't think it has to be from a has_many structure), then calling destroy on it to trigger the bug.

[tfausak]

👍 That looks good!

[brendon]

Very good. I'll close this PR and merge my branch: #201

Glad it works for you :)