Fix logic on disabling commit callbacks

Commit callbacks are intentionally disabled when errors occur when calling the callback chain in order to reset the internal record state. However, the implicit order of operations on the logic for checking if callbacks are disabled is wrong. The result is that callbacks can be unexpectedly when errors occur in transactions.

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix logic on disabling commit callbacks so they are not called unexpectedly when errors occur.
+
+    *Brian Durand*
+
 *   Ensure `Associations::CollectionAssociation#size` and `Associations::CollectionAssociation#empty?`
     use loaded association ids if present.
 

activerecord/lib/active_record/transactions.rb

@@ -340,7 +340,7 @@ def before_committed! # :nodoc:
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
     def committed!(should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && destroyed? || persisted?
+      if should_run_callbacks && (destroyed? || persisted?)
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
       end

activerecord/test/cases/transaction_callbacks_test.rb

@@ -367,6 +367,26 @@ def test_after_commit_callbacks_should_validate_on_condition
     assert_match(/:on conditions for after_commit and after_rollback callbacks have to be one of \[:create, :destroy, :update\]/, e.message)
   end
 
+  def test_after_commit_chain_not_called_on_errors
+    record_1 = TopicWithCallbacks.create!
+    record_2 = TopicWithCallbacks.create!
+    record_3 = TopicWithCallbacks.create!
+    callbacks = []
+    record_1.after_commit_block { raise }
+    record_2.after_commit_block { callbacks << record_2.id }
+    record_3.after_commit_block { callbacks << record_3.id }
+    begin
+      TopicWithCallbacks.transaction do
+        record_1.save!
+        record_2.save!
+        record_3.save!
+      end
+    rescue
+      # From record_1.after_commit
+    end
+    assert_equal [], callbacks
+  end
+
   def test_saving_a_record_with_a_belongs_to_that_specifies_touching_the_parent_should_call_callbacks_on_the_parent_object
     pet   = Pet.first
     owner = pet.owner