Fixed broken after_save callback; was being called when before_create was canceled or before_update was canceled

Signed-off-by: Michael Koziarski <michael@koziarski.com>
[#1735 state:committed]

3 files changed, 48 insertions, 5 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 35172ae110..21731f16ca 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,9 @@
 *2.3.0/3.0*
 
+* Make after_save callbacks fire only if the record was successfully saved.  #1735 [Michael Lovitt]
+
+  Previously the callbacks would fire if a before_save cancelled saving.
+
 * Support nested transactions using database savepoints.  #383 [Jonathan Viney, Hongli Lai]
 
 * Added dynamic scopes ala dynamic finders #1648 [Yaroslav Markin]
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index 42bfe34505..9f5384d39a 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/activerecord/lib/active_record/callbacks.rb
@@ -219,8 +219,9 @@ module ActiveRecord
     def after_save()  end
     def create_or_update_with_callbacks #:nodoc:
       return false if callback(:before_save) == false
-      result = create_or_update_without_callbacks
-      callback(:after_save)
+      if result = create_or_update_without_callbacks
+        callback(:after_save)
+      end
       result
     end
     private :create_or_update_with_callbacks
diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb
index 11830a2ef6..33b1ea034d 100644
--- a/activerecord/test/cases/callbacks_test.rb
+++ b/activerecord/test/cases/callbacks_test.rb
@@ -121,9 +121,19 @@ end
 
 class CallbackCancellationDeveloper < ActiveRecord::Base
   set_table_name 'developers'
-  def before_create
-    false
-  end
+
+  attr_reader   :after_save_called, :after_create_called, :after_update_called, :after_destroy_called
+  attr_accessor :cancel_before_save, :cancel_before_create, :cancel_before_update, :cancel_before_destroy
+
+  def before_save;    !@cancel_before_save;    end
+  def before_create;  !@cancel_before_create;  end
+  def before_update;  !@cancel_before_update;  end
+  def before_destroy; !@cancel_before_destroy; end
+
+  def after_save;      @after_save_called    = true; end
+  def after_update;    @after_update_called  = true; end
+  def after_create;    @after_create_called  = true; end
+  def after_destroy;   @after_destroy_called = true; end
 end
 
 class CallbacksTest < ActiveRecord::TestCase
@@ -349,19 +359,47 @@ class CallbacksTest < ActiveRecord::TestCase
     assert !david.valid?
     assert !david.save
     assert_raises(ActiveRecord::RecordInvalid) { david.save! }
+
+    someone = CallbackCancellationDeveloper.find(1)
+    someone.cancel_before_save = true
+    assert someone.valid?
+    assert !someone.save
+    assert_save_callbacks_not_called(someone)
   end
 
   def test_before_create_returning_false
     someone = CallbackCancellationDeveloper.new
+    someone.cancel_before_create = true
     assert someone.valid?
     assert !someone.save
+    assert_save_callbacks_not_called(someone)
+  end
+
+  def test_before_update_returning_false
+    someone = CallbackCancellationDeveloper.find(1)
+    someone.cancel_before_update = true
+    assert someone.valid?
+    assert !someone.save
+    assert_save_callbacks_not_called(someone)
   end
 
   def test_before_destroy_returning_false
     david = ImmutableDeveloper.find(1)
     assert !david.destroy
     assert_not_nil ImmutableDeveloper.find_by_id(1)
+
+    someone = CallbackCancellationDeveloper.find(1)
+    someone.cancel_before_destroy = true
+    assert !someone.destroy
+    assert !someone.after_destroy_called
+  end
+
+  def assert_save_callbacks_not_called(someone)
+    assert !someone.after_save_called
+    assert !someone.after_create_called
+    assert !someone.after_update_called
   end
+  private :assert_save_callbacks_not_called
 
   def test_zzz_callback_returning_false # must be run last since we modify CallbackDeveloper
     david = CallbackDeveloper.find(1)