before_destroy callback not stopping record from being deleted

Question

I am trying to prevent a record from being destroyed if there are children.

class Submission < ActiveRecord::Base

has_many :quotations, :dependent => :destroy

 before_destroy :check_for_payments


  def quoted?
    quotations.any?
  end


  def has_payments?
   true if quotations.detect {|q| q.payment}
  end


  private

  def check_for_payments
    if quoted? && has_payments?
      errors[:base] << "cannot delete submission that has already been paid"
      false
    end
  end

end

class Quotation < ActiveRecord::Base

    #associations
    belongs_to :submission
        has_one :payment_notification   
        has_one :payment

         before_destroy :check_for_payments

private 

def check_for_payments
  if payment_notification || payment
    errors[:base] << "cannot delete quotation while payment exist"
    return false
  end
end
end

When I test this code the before_destroy :check_for_payments prevents the Quotation record from being deleted.

However the :check_for_payments in the Submission before_destroy callback does not stop the Submission from being deleted.

How can I stop the Submission with payments from being destroyed?

Answer 1

In Rails 5 you have to throw :abort otherwise it won't work. (even returning false)

Also, you should add prepend: true to the callback, to make sure it runs before the dependent: :destroy on the parent models.

Something like this should work:

class Something < ApplicationRecord

  before_destroy :can_destroy?, prepend: true

  private

  def can_destroy?
    if model.something?
      self.errors.add(:base, "Can't be destroy because of something")
      throw :abort
    end
  end
end

Answer 2

I would try the code below where I have:

1. used a has_many :through association for payments
2. avoided unnecessary record retrieval of quotations and payments by using any? without a block which results in using the association counter cache if defined, or the size of the association if already loaded and failing that an SQL COUNT if needed.
3. avoided enumeration of quotations
4. avoided testing for truthiness/presence of the q.payment association proxy directly which does not work for has_xxx. If you want to test for presence use q.payment.present?

Try the following and see how you go:

class Submission < ActiveRecord::Base

  has_many :quotations,
    inverse_of: :submission,
    dependent: :destroy

  has_many :payments,
    through: :quotations

  before_destroy :check_for_payments, prepend: true

private

  def check_for_payments
    if payments.any?
      errors[:base] << "cannot delete submission that has already been paid"
      return false
    end
  end
end

Answer 3

In Rails 5 you could also:

def destroy
  quoted? && has_payments? ? self : super
end

submission.destroy # => submission
submission.destroyed? # => true/false

Answer 4

http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html ordering callbacks ( I changed the wording to this specific example)

Sometimes the code needs that the callbacks execute in a specific order. For example, a before_destroy callback (check_for_payments in this case) should be executed before the quotations get destroyed by the +dependent: destroy+ option.

In this case, the problem is that when the before_destroy callback is executed, the quotation are not available because the destroy callback gets executed first. You can use the prepend option on the before_destroy callback to avoid this.

before_destroy :check_for_payments, prepend: true

I made a new app with the same models as you described above and then a Submission Test. Its pretty ugly, I'm just learning...

class Submission < ActiveRecord::Base

  has_many :quotations, :dependent => :destroy

  before_destroy :check_for_payments, prepend: true

  def quoted?
    quotations.any?
  end

  def has_payments?
    true if quotations.detect {|q| q.payment }
  end

  private

    def check_for_payments
      if quoted? && has_payments?
        errors[:base] << "error message"
        false
      end
    end

end

class Quotation < ActiveRecord::Base

  belongs_to :submission
  has_one :payment_notification   
  has_one :payment

  before_destroy :check_for_payments

  private 

    def check_for_payments
      if payment_notification || payment
        errors[:base] << "cannot delete quotation while payment exist"
        return false
      end
    end
end

require 'test_helper'

class SubmissionTest < ActiveSupport::TestCase


  test "can't destroy" do

    sub = Submission.new
    sub.save

    quote = Quotation.new
    quote.submission_id = sub.id
    quote.save

    pay = Payment.new
    pay.quotation_id = quote.id
    pay.save

    refute sub.destroy, "destroyed record"
  end
end

It passed!. I Hope that helps.

Answer 5

Make sure quoted? and has_payments? doesn't return false.

For debugging try this

def check_for_payments
    raise "Debugging #{quoted?} #{has_payments?}" # Make sure both are true
    if quoted? && has_payments?
      errors[:base] << "cannot delete submission that has already been paid"
      false
    end
  end

Answer 6

As far as i know, when the destroy is called on an object of a Class(Submission) having an association with dependent => :destroy, and if any callback in the associated model fails, in your case Quotation, then the Submission class object will still be deleted.

So to prevent this behaviour, we have to methods that i can currently think of:

1) Instead of returning false in Quotation#check_for_payments, you could raise an exception and handle it gracefully in the Submission Model, which would do a complete ROLLBACK, and not let any record be destroyed.

2) You can check whether any quotations for a Submission instance have a payment/payment_notification in Submission#check_for_payments method itself, which would prevent deletion of the Submission Object.