document.destroy now returns false if before_destroy callback returns fa...

[tpjg]

I'm rendering different views in my rails controller depending on the outcome of @record.destroy. It could be just me, I'm still a relative riak/ripple newbie but it appears that when a callback returned false this didn't get through. I've changed a single line and wrote (my first) RSpec test for it.

[seancribbs]

If destroy! returns false, this line seems unnecessary. Just return the value of destroy! or !!destroy!.

[tpjg]

I agree. It's an unnecessary check - although it makes sure it only ever returns true or false and not somethings else (like a Ripple::Document).

[myronmarston]

I'm not sure if the behavior demonstrated by the spec is actually behavior we want. @seancribbs and I had a conversation a while back about callbacks, and we both disliked the fact that ActiveRecord halts the action if the callback returns false. This conflates command/query separation semantics and, in my experience, can be a subtle source of bugs. In ripple we decided we didn't want that behavior.

Furthermore, I'm not sure that before_destroy { false } is actually preventing the destroy action here. I think what you have here passes the spec because destroy! is defined in lib/ripple/callbacks.rb as:

    # @private
    def destroy!(*args, &block)
      run_callbacks(:destroy) do
        super
      end
    end

I'm not totally sure about this, but I think that run_callbacks here may be returning the value of the last callback (which is why the spec passes), while still yielding to the block (which supers to the main implementation of destroy!)--hence, I think the destroy actually may be succeeding. If my hunch is true, then I think this change is unhelpful, since it makes the API confusing. destroy should only return false if the deletion did not happen.

[tpjg]

@myronmarston you could be right about the destroy actually succeeding and maybe the ActiveRecord conventions are not the best solution. So how to implement some checks before allowing a destroy? I'm currently using (trying to use...) a before_destroy to prevent a document from getting destroyed if it has links to some other documents and using before_destroy seemed like an elegant solution to me.

[myronmarston]

@tpjg -- you should be able to override destroy, and conditionally super to ripple's implementation based on whatever logic you need.

[tpjg]

I have written a different spec and it seems that the destroy is not succeeding, so it is as expected from a typical ActiveRecord implementation.

  it "destroy should return false and not destroy the document when a callback returns false" do
    User.before_destroy { false }
    u = User.create!(:email => 'nobody@domain.com')
    u.destroy.should be false
    User.find(u.key).should be_an_instance_of(User)
  end