before_destroy callback not stopping record from being deleted
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.
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.
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
I would try the code below where I have:
- used a has_many :through association for payments
- 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. - avoided enumeration of quotations
- 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 useq.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