From 44f2dfac4026530b11e8af2c9526f6719c44a2ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thorsten=20B=C3=B6ttger?= Date: Tue, 9 Sep 2014 23:02:11 +0200 Subject: [PATCH] clean up callbacks (closing #96) --- AASM4.md | 6 +-- HOWTO_MIGRATE_FROM_AASM_3_TO_4.md | 41 ----------------- README.md | 26 ++++++++--- README_FROM_VERSION_3_TO_4.md | 18 ++++++++ callbacks.txt | 34 ++++++++++++++ lib/aasm/aasm.rb | 16 ++++--- lib/aasm/event.rb | 2 +- lib/aasm/instance_base.rb | 2 +- spec/models/auth_machine.rb | 6 +-- spec/models/callback_new_dsl.rb | 74 ++++++++++++++++++------------- spec/models/double_definer.rb | 2 +- spec/models/foo.rb | 8 ++-- spec/unit/callbacks_spec.rb | 68 ++++++++++++++++++++++++---- 13 files changed, 198 insertions(+), 105 deletions(-) delete mode 100644 HOWTO_MIGRATE_FROM_AASM_3_TO_4.md diff --git a/AASM4.md b/AASM4.md index 0996f4a..5ea76a7 100644 --- a/AASM4.md +++ b/AASM4.md @@ -1,8 +1,8 @@ # Planned changes for AASM 4 - * **done**: firing an event does not require `to_state` parameter anymore (closing issues #11, #58, #80) - * don't allow direct assignment of state attribute (see #53) - * remove old callbacks (see #96) + * **done**: firing an event does not require `to_state` parameter anymore (closing [issue #11](https://github.com/aasm/aasm/issues/11), [issue #58](https://github.com/aasm/aasm/issues/58) and [issue #80](https://github.com/aasm/aasm/issues/80)) + * **done**: don't allow direct assignment of state attribute (see [issue #53](https://github.com/aasm/aasm/issues/53)) + * remove old callbacks (see [issue #96](https://github.com/aasm/aasm/issues/96)) * remove old aasm DSL (like `aasm_state`) diff --git a/HOWTO_MIGRATE_FROM_AASM_3_TO_4.md b/HOWTO_MIGRATE_FROM_AASM_3_TO_4.md deleted file mode 100644 index fec9369..0000000 --- a/HOWTO_MIGRATE_FROM_AASM_3_TO_4.md +++ /dev/null @@ -1,41 +0,0 @@ -# How to migrate from AASM version 3 to 4? - -## callback arguments - -On one hand, when using callbacks like this - -```ruby -class MyClass - aasm do - ... - event :close, :before => :before do - transitions :to => :closed, :from => :open, :on_transition => :transition_proc - end - end - def transition_proc(arg1, arg2); end - def before(arg1, arg2); end - ... -end -``` - -you don't have to provide the target state as first argument anymore. So, instead of - -```ruby -my_class = MyClass.new -my_class.close(:closed, arg1, arg2) -``` - -you can leave that away now - -```ruby -my_class.close(arg1, args) -``` - -On the other hand, you have to accept the arguments for **all** callback methods (and procs) -you provide and use. If you don't want to provide these, you can splat them - -```ruby -def before(*args); end -# or -def before(*_); end # to indicate that you don't want to use the arguments -``` diff --git a/README.md b/README.md index ea085a6..320252a 100644 --- a/README.md +++ b/README.md @@ -131,13 +131,25 @@ is finished. Here you can see a list of all possible callbacks, together with their order of calling: ```ruby - event:before - previous_state:before_exit - new_state:before_enter - ...update state... - previous_state:after_exit - new_state:after_enter - event:after +begin + event before + event guards # test run + transition guards # test run + old_state before_exit + old_state exit + new_state before_enter + new_state enter + event guards + transition guards + transition on_transition + ...update state... + event success # if persist successful + old_state after_exit + new_state after_enter + event after +rescue + event error +end ``` Also, you can pass parameters to events: diff --git a/README_FROM_VERSION_3_TO_4.md b/README_FROM_VERSION_3_TO_4.md index ba76e46..4a176f7 100644 --- a/README_FROM_VERSION_3_TO_4.md +++ b/README_FROM_VERSION_3_TO_4.md @@ -2,6 +2,15 @@ ## Must +### Callback order has been changed + +The first callback to be run is `:before` of the event. A state's `:before_exit' callback +is now run directly before its `:exit` callback. Event-based guards are now run before +any of the transition guards are run. And finally, before running any state callbacks, +all (event- and transition-based) guards are run to check whether the state callbacks +can be run or not. + + ### `after_commit` hooks are now event-based The `after_commit` hooks have been move from the state level to the event level. @@ -90,6 +99,15 @@ job.run("we want to run") job.run(:running, "we want to run") # still supported to select the target state (the _to_state_) ``` +On the other hand, you have to accept the arguments for **all** callback methods (and procs) +you provide and use. If you don't want to provide these, you can splat them + +```ruby +def before(*args); end +# or +def before(*_); end # to indicate that you don't want to use the arguments +``` + ### New configuration option: `no_direct_assignment` If you want to make sure that the _AASM_ column for storing the state is not directly assigned, diff --git a/callbacks.txt b/callbacks.txt index fffd994..1b0bdff 100644 --- a/callbacks.txt +++ b/callbacks.txt @@ -1,5 +1,7 @@ callbacks +AASM 3 + begin old_state exit # old? should be deprecated -> use old_state.before_exit instead event before @@ -8,6 +10,7 @@ begin new_state enter # old? should be deprecated -> use new_state.before_enter instead ...update state... transition guard + event guard transition on_transition event success # if persist successful old_state after_exit @@ -15,3 +18,34 @@ begin event after rescue event error +end + +AASM 4 + +todo + +done +- move event.before before everything else +- move old_state.before_exit before old_state.exit +- move event.guard before transition.guard +- fire guards before running state callbacks (test run) + +begin + event before + event guard # test run + transition guard # test run + old_state before_exit + old_state exit + new_state before_enter + new_state enter + event guard + transition guard + transition on_transition + ...update state... + event success # if persist successful + old_state after_exit + new_state after_enter + event after +rescue + event error +end diff --git a/lib/aasm/aasm.rb b/lib/aasm/aasm.rb index 4b5c4a5..411ad49 100644 --- a/lib/aasm/aasm.rb +++ b/lib/aasm/aasm.rb @@ -163,7 +163,6 @@ private event = self.class.aasm.events[event_name] begin old_state = aasm.state_object_for_name(aasm.current_state) - old_state.fire_callbacks(:exit, self) # new event before callback event.fire_callbacks( @@ -172,8 +171,15 @@ private *process_args(event, aasm.current_state, *args) ) - if new_state_name = event.fire(self, *args) - aasm_fired(event, old_state, new_state_name, options, *args, &block) + if event.may_fire?(self, *args) + old_state.fire_callbacks(:before_exit, self) + old_state.fire_callbacks(:exit, self) # TODO: remove for AASM 4? + + if new_state_name = event.fire(self, *args) + aasm_fired(event, old_state, new_state_name, options, *args, &block) + else + aasm_failed(event_name, old_state) + end else aasm_failed(event_name, old_state) end @@ -187,11 +193,9 @@ private new_state = aasm.state_object_for_name(new_state_name) - # new before_ callbacks - old_state.fire_callbacks(:before_exit, self) new_state.fire_callbacks(:before_enter, self) - new_state.fire_callbacks(:enter, self) + new_state.fire_callbacks(:enter, self) # TODO: remove for AASM 4? persist_successful = true if persist diff --git a/lib/aasm/event.rb b/lib/aasm/event.rb index fff5896..a9845f4 100644 --- a/lib/aasm/event.rb +++ b/lib/aasm/event.rb @@ -77,7 +77,7 @@ module AASM def attach_event_guards(definitions) unless @guards.empty? given_guards = Array(definitions.delete(:guard) || definitions.delete(:guards)) - definitions[:guards] = given_guards + @guards + definitions[:guards] = @guards + given_guards end definitions end diff --git a/lib/aasm/instance_base.rb b/lib/aasm/instance_base.rb index 151c3a8..deaf33f 100644 --- a/lib/aasm/instance_base.rb +++ b/lib/aasm/instance_base.rb @@ -21,7 +21,7 @@ module AASM state_object = state_object_for_name(state_name) state_object.fire_callbacks(:before_enter, @instance) - state_object.fire_callbacks(:enter, @instance) + # state_object.fire_callbacks(:enter, @instance) self.current_state = state_name state_object.fire_callbacks(:after_enter, @instance) diff --git a/spec/models/auth_machine.rb b/spec/models/auth_machine.rb index 8bd3652..e39e391 100644 --- a/spec/models/auth_machine.rb +++ b/spec/models/auth_machine.rb @@ -5,10 +5,10 @@ class AuthMachine aasm do state :passive - state :pending, :initial => true, :enter => :make_activation_code - state :active, :enter => :do_activate + state :pending, :initial => true, :before_enter => :make_activation_code + state :active, :before_enter => :do_activate state :suspended - state :deleted, :enter => :do_delete, :exit => :do_undelete + state :deleted, :before_enter => :do_delete#, :exit => :do_undelete state :waiting event :register do diff --git a/spec/models/callback_new_dsl.rb b/spec/models/callback_new_dsl.rb index d0e89af..5f28d41 100644 --- a/spec/models/callback_new_dsl.rb +++ b/spec/models/callback_new_dsl.rb @@ -1,9 +1,15 @@ class CallbackNewDsl include AASM + def initialize(options={}) + @fail_event_guard = options[:fail_event_guard] + @fail_transition_guard = options[:fail_transition_guard] + end + aasm do state :open, :initial => true, :before_enter => :before_enter_open, + :enter => :enter_open, :after_enter => :after_enter_open, :before_exit => :before_exit_open, :exit => :exit_open, @@ -14,10 +20,11 @@ class CallbackNewDsl :enter => :enter_closed, :after_enter => :after_enter_closed, :before_exit => :before_exit_closed, + :exit => :exit_closed, :after_exit => :after_exit_closed - event :close, :before => :before, :after => :after do - transitions :to => :closed, :from => [:open] + event :close, :before => :before, :after => :after, :guard => :event_guard do + transitions :to => :closed, :from => [:open], :guard => :transition_guard, :on_transition => :transitioning end event :open, :before => :before, :after => :after do @@ -25,21 +32,30 @@ class CallbackNewDsl end end - def before_enter_open; end - def before_exit_open; end - def after_enter_open; end - def after_exit_open; end + def log(text) + # puts text + end - def before_enter_closed; end - def before_exit_closed; end - def after_enter_closed; end - def after_exit_closed; end + def before_enter_open; log('before_enter_open'); end + def enter_open; log('enter_open'); end + def before_exit_open; log('before_exit_open'); end + def after_enter_open; log('after_enter_open'); end + def exit_open; log('exit_open'); end + def after_exit_open; log('after_exit_open'); end - def before; end - def after; end + def before_enter_closed; log('before_enter_closed'); end + def enter_closed; log('enter_closed'); end + def before_exit_closed; log('before_exit_closed'); end + def exit_closed; log('exit_closed'); end + def after_enter_closed; log('after_enter_closed'); end + def after_exit_closed; log('after_exit_closed'); end - def enter_closed; end - def exit_open; end + def event_guard; log('event_guard'); !@fail_event_guard; end + def transition_guard; log('transition_guard'); !@fail_transition_guard; end + def transitioning; log('transitioning'); end + + def before; log('before'); end + def after; log('after'); end end class CallbackNewDslArgs @@ -50,12 +66,10 @@ class CallbackNewDslArgs :before_enter => :before_enter_open, :after_enter => :after_enter_open, :before_exit => :before_exit_open, - :exit => :exit_open, :after_exit => :after_exit_open state :closed, :before_enter => :before_enter_closed, - :enter => :enter_closed, :after_enter => :after_enter_closed, :before_exit => :before_exit_closed, :after_exit => :after_exit_closed @@ -69,23 +83,23 @@ class CallbackNewDslArgs end end - def before_enter_open; end - def before_exit_open; end - def after_enter_open; end - def after_exit_open; end + def log(text) + # puts text + end - def before_enter_closed; end - def before_exit_closed; end - def after_enter_closed; end - def after_exit_closed; end + def before_enter_open; log('before_enter_open'); end + def before_exit_open; log('before_exit_open'); end + def after_enter_open; log('after_enter_open'); end + def after_exit_open; log('after_exit_open'); end - def before(*args); end - def transition_proc(arg1, arg2); end - def after(*args); end - - def enter_closed; end - def exit_open; end + def before_enter_closed; log('before_enter_closed'); end + def before_exit_closed; log('before_enter_closed'); end + def after_enter_closed; log('after_enter_closed'); end + def after_exit_closed; log('after_exit_closed'); end + def before(*args); log('before'); end + def transition_proc(arg1, arg2); log('transition_proc'); end + def after(*args); log('after'); end end class CallbackWithStateArg diff --git a/spec/models/double_definer.rb b/spec/models/double_definer.rb index 621c0ce..e95fb01 100644 --- a/spec/models/double_definer.rb +++ b/spec/models/double_definer.rb @@ -10,7 +10,7 @@ class DoubleDefiner end # simulating a reload - state :finished, :enter => :do_enter + state :finished, :before_enter => :do_enter event :finish do transitions :from => :started, :to => :finished, :on_transition => :do_on_transition end diff --git a/spec/models/foo.rb b/spec/models/foo.rb index 3396c08..b299979 100644 --- a/spec/models/foo.rb +++ b/spec/models/foo.rb @@ -1,8 +1,8 @@ class Foo include AASM aasm do - state :open, :initial => true, :exit => :exit - state :closed, :enter => :enter + state :open, :initial => true, :before_exit => :before_exit + state :closed, :before_enter => :before_enter state :final event :close, :success => :success_callback do @@ -21,9 +21,9 @@ class Foo def success_callback end - def enter + def before_enter end - def exit + def before_exit end end diff --git a/spec/unit/callbacks_spec.rb b/spec/unit/callbacks_spec.rb index 646db24..c1ccff2 100644 --- a/spec/unit/callbacks_spec.rb +++ b/spec/unit/callbacks_spec.rb @@ -1,12 +1,19 @@ require 'spec_helper' describe 'callbacks for the new DSL' do - let(:callback) {CallbackNewDsl.new} it "be called in order" do - expect(callback).to receive(:exit_open).once.ordered + callback = CallbackNewDsl.new + callback.aasm.current_state + expect(callback).to receive(:before).once.ordered + expect(callback).to receive(:event_guard).once.ordered.and_return(true) + expect(callback).to receive(:transition_guard).once.ordered.and_return(true) expect(callback).to receive(:before_exit_open).once.ordered # these should be before the state changes + expect(callback).to receive(:exit_open).once.ordered + expect(callback).to receive(:event_guard).once.ordered.and_return(true) + expect(callback).to receive(:transition_guard).once.ordered.and_return(true) + expect(callback).to receive(:transitioning).once.ordered expect(callback).to receive(:before_enter_closed).once.ordered expect(callback).to receive(:enter_closed).once.ordered expect(callback).to receive(:aasm_write_state).once.ordered.and_return(true) # this is when the state changes @@ -14,17 +21,62 @@ describe 'callbacks for the new DSL' do expect(callback).to receive(:after_enter_closed).once.ordered expect(callback).to receive(:after).once.ordered + # puts "------- close!" callback.close! end + it "does not run any state callback if the event guard fails" do + callback = CallbackNewDsl.new + callback.aasm.current_state + + expect(callback).to receive(:before).once.ordered + expect(callback).to receive(:event_guard).once.ordered.and_return(false) + expect(callback).to_not receive(:transition_guard) + expect(callback).to_not receive(:before_exit_open) + expect(callback).to_not receive(:exit_open) + expect(callback).to_not receive(:transitioning) + expect(callback).to_not receive(:before_enter_closed) + expect(callback).to_not receive(:enter_closed) + expect(callback).to_not receive(:aasm_write_state) + expect(callback).to_not receive(:after_exit_open) + expect(callback).to_not receive(:after_enter_closed) + expect(callback).to_not receive(:after) + + expect { + callback.close! + }.to raise_error(AASM::InvalidTransition) + end + + it "does not run any state callback if the transition guard fails" do + callback = CallbackNewDsl.new + callback.aasm.current_state + + expect(callback).to receive(:before).once.ordered + expect(callback).to receive(:event_guard).once.ordered.and_return(true) + expect(callback).to receive(:transition_guard).once.ordered.and_return(false) + expect(callback).to_not receive(:before_exit_open) + expect(callback).to_not receive(:exit_open) + expect(callback).to_not receive(:transitioning) + expect(callback).to_not receive(:before_enter_closed) + expect(callback).to_not receive(:enter_closed) + expect(callback).to_not receive(:aasm_write_state) + expect(callback).to_not receive(:after_exit_open) + expect(callback).to_not receive(:after_enter_closed) + expect(callback).to_not receive(:after) + + expect { + callback.close! + }.to raise_error(AASM::InvalidTransition) + end + it "should properly pass arguments" do cb = CallbackNewDslArgs.new - cb.should_receive(:exit_open).once.ordered + + # TODO: use expect syntax here cb.should_receive(:before).with(:arg1, :arg2).once.ordered - cb.should_receive(:transition_proc).with(:arg1, :arg2).once.ordered cb.should_receive(:before_exit_open).once.ordered # these should be before the state changes + cb.should_receive(:transition_proc).with(:arg1, :arg2).once.ordered cb.should_receive(:before_enter_closed).once.ordered - cb.should_receive(:enter_closed).once.ordered cb.should_receive(:aasm_write_state).once.ordered.and_return(true) # this is when the state changes cb.should_receive(:after_exit_open).once.ordered # these should be after the state changes cb.should_receive(:after_enter_closed).once.ordered @@ -84,19 +136,19 @@ describe 'event callbacks' do it "should run error_callback if an exception is raised and error_callback defined" do def @foo.error_callback(e); end - allow(@foo).to receive(:enter).and_raise(e=StandardError.new) + allow(@foo).to receive(:before_enter).and_raise(e=StandardError.new) expect(@foo).to receive(:error_callback).with(e) @foo.safe_close! end it "should raise NoMethodError if exceptionis raised and error_callback is declared but not defined" do - allow(@foo).to receive(:enter).and_raise(StandardError) + allow(@foo).to receive(:before_enter).and_raise(StandardError) expect{@foo.safe_close!}.to raise_error(NoMethodError) end it "should propagate an error if no error callback is declared" do - allow(@foo).to receive(:enter).and_raise("Cannot enter safe") + allow(@foo).to receive(:before_enter).and_raise("Cannot enter safe") expect{@foo.close!}.to raise_error(StandardError, "Cannot enter safe") end end