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?

score 77 · Answer 1 · edited May 22 '21 at 06:17

77

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

edited May 22 '21 at 06:17

Sagar Ranglani

5,491
4
34
47

answered Jan 13 '17 at 15:48

Andres

11,439
12
48
87

Really? It can be because it doesn't work here either. So why *TH* do the Rails guides tell us that `If any before callback method returns exactly false or raises an exception, the execution chain gets halted`?! – silverdr Apr 04 '17 at 22:05
1

@silverdr I dont know if the rails guide was like that at the time of your comment, but as of now, it is correct. `If any callback raises an exception, the execution chain gets halted and a ROLLBACK is issued. To intentionally stop a chain use: throw :abort` – rubyprince Jan 19 '18 at 16:26
Any ideas how to prevent destroy, but allow a mutation of the record? `return false` allows me to change the record, but the record still gets destroyed, `throw :abort` prevents destroy, but also rolls back any changes i've made to the object. – Tony Beninate Jul 08 '19 at 01:13
I updated the answer to add `prepend: true` and to use `self.errors.add()` syntax. – sandre89 Jun 29 '20 at 18:02

gringocl · Answer 2 · 2013-10-06T02:05:32.873

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.

Thanks, but I wonder why is not `prepend: true` as default behaviour — Albert Català, Nov 24 '16 at 12:20

Andrew Hacking · Accepted Answer · 2019-03-05T03:59:51.173

30

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

edited Mar 05 '19 at 03:59

answered Oct 02 '13 at 12:15

Andrew Hacking

6,296
31
37

1

Thanks Andrew. I chose your answer because not only does it solve the problem I was having but it also helps me to make my code better. I don't know what inverse_of: :submissions does. Do I have to modify any tables? – chell Oct 07 '13 at 05:21
No you don't have to modify any tables. It helps ActiveRecord maintain the associations when creating new model instances and optimise model loading. You can read more about it in the [docs](http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html) – Andrew Hacking Oct 07 '13 at 12:10
13

this code will not work. the payments will be deleted before the before_destroy callback is invoked because the dependent: destroy is defined as a before_destroy callback too, and since it was defined first it will be invoked prior to the 'check_for_payments'. I dont know why this answer has so many upvotes when it's wrong? – horseyguy Apr 14 '16 at 02:25
i agree with horseguy, dependent destroy will be called at first because it is defined first. – Matias Seguel Jan 25 '18 at 14:21
3

You can use `prepend: true` to run the callback before dependent destroy. http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html – causztic Apr 10 '18 at 00:53

score 1 · Answer 4 · answered Oct 05 '13 at 11:02

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.

Siva · Answer 5 · 2013-10-05T12:20:23.400

1

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

edited Oct 05 '13 at 12:20

answered Oct 05 '13 at 12:14

Siva

7,780
6
47
54

score 0 · Answer 6 · answered Nov 15 '17 at 10:20

0

In Rails 5 you could also:

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

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

answered Nov 15 '17 at 10:20

Kris

19,188
9
91
111

before_destroy callback not stopping record from being deleted

6 Answers6