clean up callbacks (closing #96)

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

HOWTO_MIGRATE_FROM_AASM_3_TO_4.md

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

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
+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...
-      previous_state:after_exit
-    new_state:after_enter
-  event:after
+    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:

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,

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

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,11 +171,18 @@ private
         *process_args(event, aasm.current_state, *args)
       )
 
+      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
     rescue StandardError => e
       event.fire_callbacks(:error, self, e) || raise(e)
     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

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

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)
 

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

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

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

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
 

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