Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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]
  • Loading branch information...
commit 7a0e7c7270548138a333bc39aab5aec80580174b 1 parent c891d68
@lovitt lovitt authored NZKoz committed
View
4 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]
View
5 activerecord/lib/active_record/callbacks.rb
@@ -219,8 +219,9 @@ def before_save() end
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
View
44 activerecord/test/cases/callbacks_test.rb
@@ -121,9 +121,19 @@ def before_destroy
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 @@ def test_before_save_returning_false
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)
Please sign in to comment.
Something went wrong with that request. Please try again.