Always interpet nil return values from actions as failed attempts

CHANGELOG.rdoc

@@ -1,5 +1,6 @@
 == master
 
+* Always interpet nil return values from actions as failed attempts
 * Fix loopbacks not causing records to save in ORM integrations if no other fields were changed
 * Fix events not failing with useful errors when an object's state is invalid
 * Use more friendly NoMethodError messages for state-driven behaviors

lib/state_machine/guard.rb

@@ -24,8 +24,8 @@ class Guard
     # requirements contain a mapping of {:from => matcher, :to => matcher}.
     attr_reader :state_requirements
 
-    # The requirement for verifying the result of the event
-    attr_reader :result_requirement
+    # The requirement for verifying the success of the event
+    attr_reader :success_requirement
 
     # A list of all of the states known to this guard.  This will pull states
     # from the following options (in the same order):
@@ -42,8 +42,8 @@ def initialize(options = {}) #:nodoc:
       # Build event requirement
       @event_requirement = build_matcher(options, :on, :except_on)
 
-      # Build result
-      @result_requirement = options.delete(:include_failures) ? AllMatcher.instance : BlacklistMatcher.new([false])
+      # Build success requirement
+      @success_requirement = options.delete(:include_failures) ? AllMatcher.instance : BlacklistMatcher.new([false])
 
       if (options.keys - [:from, :to, :on, :except_from, :except_to, :except_on]).empty?
         # Explicit from/to requirements specified
@@ -191,14 +191,14 @@ def build_matcher(options, whitelist_option, blacklist_option)
       def match_query(query)
         query ||= {}
 
-        if match_result(query) && match_event(query) && (state_requirement = match_states(query))
+        if match_success(query) && match_event(query) && (state_requirement = match_states(query))
           state_requirement.merge(:on => event_requirement)
         end
       end
 
-      # Verifies that the result requirement matches the given query
-      def match_result(query)
-        matches_requirement?(query, :result, result_requirement)
+      # Verifies that the success requirement matches the given query
+      def match_success(query)
+        matches_requirement?(query, :success, success_requirement)
       end
 
       # Verifies that the event requirement matches the given query

lib/state_machine/integrations/sequel.rb

@@ -314,11 +314,7 @@ def transaction(object)
         def add_callback(type, options, &block)
           options[:bind_to_object] = true
           options[:terminator] = @terminator ||= lambda {|result| result == false}
-
-          # nil is also a failed value for the result requirement in callbacks
-          callback = super
-          callback.guard.result_requirement.values << nil if options[:include_failures] != true
-          callback
+          super
         end
     end
   end

lib/state_machine/transition.rb

@@ -46,7 +46,7 @@ def perform(transitions, options = {})
                 # Block was given: use the result for each transition
                 result = yield
                 transitions.each {|transition| results[transition.action] = result}
-                result
+                !!result
               elsif options[:action] == false
                 # Skip the action
                 true
@@ -66,7 +66,7 @@ def perform(transitions, options = {})
 
           # Run after callbacks even when the actions failed. The :after option
           # is ignored if the transitions were unsuccessful.
-          transitions.each {|transition| transition.after(results[transition.action])} unless options[:after] == false && success
+          transitions.each {|transition| transition.after(results[transition.action], success)} unless options[:after] == false && success
 
           # Rollback the transitions if the transaction was unsuccessful
           transitions.each {|transition| transition.rollback} unless success
@@ -278,7 +278,7 @@ def persist
     # callbacks that are configured to match the event, from state, and to
     # state will be invoked.
     # 
-    # The result is used to indicate whether the associated machine action
+    # The result can be used to indicate whether the associated machine action
     # was executed successfully.
     # 
     # Once the callbacks are run, they cannot be run again until this transition
@@ -306,12 +306,12 @@ def persist
     #   vehicle = Vehicle.new
     #   transition = StateMachine::Transition.new(vehicle, Vehicle.state_machine, :ignite, :parked, :idling)
     #   transition.after(true)
-    def after(result = nil)
+    def after(result = nil, success = true)
       @result = result
 
       catch(:halt) do
         unless @after_run
-          callback(:after, :result => result)
+          callback(:after, :success => success)
           @after_run = true
         end
       end

test/unit/guard_test.rb

@@ -411,23 +411,19 @@ def setup
   end
 
   def test_should_use_a_blacklist_matcher
-    assert_instance_of StateMachine::BlacklistMatcher, @guard.result_requirement
+    assert_instance_of StateMachine::BlacklistMatcher, @guard.success_requirement
   end
 
   def test_should_match_if_not_specified
     assert @guard.matches?(@object)
   end
 
   def test_should_match_if_true
-    assert @guard.matches?(@object, :result => true)
-  end
-
-  def test_should_match_if_object
-    assert @guard.matches?(@object, :result => Object.new)
+    assert @guard.matches?(@object, :success => true)
   end
 
   def test_should_not_match_if_false
-    assert !@guard.matches?(@object, :result => false)
+    assert !@guard.matches?(@object, :success => false)
   end
 end
 
@@ -438,23 +434,19 @@ def setup
   end
 
   def test_should_use_all_matcher
-    assert_equal StateMachine::AllMatcher.instance, @guard.result_requirement
+    assert_equal StateMachine::AllMatcher.instance, @guard.success_requirement
   end
 
   def test_should_match_if_not_specified
     assert @guard.matches?(@object)
   end
 
   def test_should_match_if_true
-    assert @guard.matches?(@object, :result => true)
-  end
-
-  def test_should_match_if_object
-    assert @guard.matches?(@object, :result => Object.new)
+    assert @guard.matches?(@object, :success => true)
   end
 
   def test_should_match_if_false
-    assert @guard.matches?(@object, :result => false)
+    assert @guard.matches?(@object, :success => false)
   end
 end
 

test/unit/integrations/sequel_test.rb

@@ -433,12 +433,6 @@ def setup
         @transition = StateMachine::Transition.new(@record, @machine, :ignite, :parked, :idling)
       end
 
-      def test_should_blacklist_nil_for_before_callback_result_requirement
-        callback = @machine.before_transition(lambda {called = true})
-        assert_instance_of StateMachine::BlacklistMatcher, callback.guard.result_requirement
-        assert_equal [false, nil], callback.guard.result_requirement.values
-      end
-
       def test_should_run_before_callbacks
         called = false
         @machine.before_transition(lambda {called = true})
@@ -471,18 +465,6 @@ def test_should_run_before_callbacks_within_the_context_of_the_record
         assert_equal @record, context
       end
 
-      def test_should_blacklist_nil_for_after_callback_result_requirement_if_excluding_failures
-        callback = @machine.after_transition(lambda {called = true})
-        assert_instance_of StateMachine::BlacklistMatcher, callback.guard.result_requirement
-        assert_equal [false, nil], callback.guard.result_requirement.values
-      end
-
-      def test_should_not_modify_after_callback_result_requirement_if_including_failures
-        callback = @machine.after_transition(lambda {called = true}, :include_failures => true)
-        assert_instance_of StateMachine::AllMatcher, callback.guard.result_requirement
-        assert_equal [], callback.guard.result_requirement.values
-      end
-
       def test_should_run_after_callbacks
         called = false
         @machine.after_transition(lambda {called = true})

test/unit/transition_test.rb

@@ -477,6 +477,12 @@ def test_should_only_run_after_callbacks_that_match_transition_context
     assert_equal 1, @count
   end
 
+  def test_should_not_run_after_callbacks_if_not_successful
+    @machine.after_transition(lambda {|object| @run = true})
+    @transition.after(nil, false)
+    assert !@run
+  end
+
   def test_should_pass_transition_to_after_callbacks
     @machine.after_transition(lambda {|*args| @args = args})
 
@@ -615,6 +621,7 @@ def save
     @machine = StateMachine::Machine.new(@klass, :action => :save)
     @machine.state :parked, :idling
     @machine.event :ignite
+    @machine.after_transition(lambda {|object| @run_after = true})
 
     @object = @klass.new
     @object.state = 'parked'
@@ -641,6 +648,10 @@ def test_should_change_the_current_state
   def test_should_not_run_the_action
     assert !@object.saved
   end
+
+  def test_should_run_after_callbacks
+    assert @run_after
+  end
 end
 
 class TransitionWithTransactionsTest < Test::Unit::TestCase
@@ -737,7 +748,7 @@ def within_transaction(object)
   end
 
   def test_should_not_be_successful
-    assert !@result
+    assert_equal false, @result
   end
 
   def test_should_not_change_current_state
@@ -794,7 +805,7 @@ def within_transaction(object)
   end
 
   def test_should_be_successful
-    assert @result
+    assert_equal true, @result
   end
 
   def test_should_change_current_state
@@ -842,28 +853,41 @@ def within_transaction(object)
 
     @object = @klass.new
     @transition = StateMachine::Transition.new(@object, @machine, :ignite, :parked, :idling)
-    @result = @transition.perform
   end
 
   def test_should_not_be_successful
-    assert !@result
+    assert_equal false, @transition.perform
   end
 
   def test_should_not_change_current_state
+    @transition.perform
     assert_nil @object.state
   end
 
   def test_should_run_before_callbacks
+    @transition.perform
     assert_equal 1, @before_count
   end
 
   def test_should_only_run_after_callbacks_that_include_failures
+    @transition.perform
     assert_equal 1, @after_count
   end
 
   def test_should_cancel_the_transaction
+    @transition.perform
     assert @klass.cancelled_transaction
   end
+
+  def test_should_interpret_nil_as_failure
+    @klass.class_eval do
+      def save
+        nil
+      end
+    end
+
+    assert_equal false, @transition.perform
+  end
 end
 
 class TransitionWithActionErrorTest < Test::Unit::TestCase
@@ -1163,13 +1187,19 @@ def setup
   end
 
   def test_should_be_perform_if_result_is_not_false
-    assert StateMachine::Transition.perform([@state_transition, @status_transition]) { true }
+    assert_equal true, StateMachine::Transition.perform([@state_transition, @status_transition]) { true }
     assert_equal 'idling', @object.state
     assert_equal 'second_gear', @object.status
   end
 
   def test_should_not_perform_if_result_is_false
-    assert !StateMachine::Transition.perform([@state_transition, @status_transition]) { false }
+    assert_equal false, StateMachine::Transition.perform([@state_transition, @status_transition]) { false }
+    assert_equal 'parked', @object.state
+    assert_equal 'first_gear', @object.status
+  end
+
+  def test_should_not_perform_if_result_is_nil
+    assert_equal false, StateMachine::Transition.perform([@state_transition, @status_transition]) { nil }
     assert_equal 'parked', @object.state
     assert_equal 'first_gear', @object.status
   end