From b6d5a0d7fae7ef568b8fe50a34cf543dfc620dbf Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Wed, 21 Oct 2015 19:38:07 -0700 Subject: [PATCH 1/9] Add failed callback(s) into InvalidTransition exception --- lib/aasm/aasm.rb | 8 ++++---- lib/aasm/core/event.rb | 4 ++++ lib/aasm/core/transition.rb | 7 +++++-- lib/aasm/errors.rb | 12 ++++++++---- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/aasm/aasm.rb b/lib/aasm/aasm.rb index cf9879c..ef4d05f 100644 --- a/lib/aasm/aasm.rb +++ b/lib/aasm/aasm.rb @@ -91,10 +91,10 @@ private if new_state_name = event.fire(self, {:may_fire => may_fire_to}, *args) aasm_fired(state_machine_name, event, old_state, new_state_name, options, *args, &block) else - aasm_failed(state_machine_name, event_name, old_state) + aasm_failed(state_machine_name, event_name, old_state, event.failed_callbacks) end else - aasm_failed(state_machine_name, event_name, old_state) + aasm_failed(state_machine_name, event_name, old_state, event.failed_callbacks) end rescue StandardError => e event.fire_callbacks(:error, self, e, *process_args(event, aasm(state_machine_name).current_state, *args)) || raise(e) @@ -143,13 +143,13 @@ private persist_successful end - def aasm_failed(state_machine_name, event_name, old_state) + def aasm_failed(state_machine_name, event_name, old_state, failures = []) if self.respond_to?(:aasm_event_failed) self.aasm_event_failed(event_name, old_state.name) end if AASM::StateMachine[self.class][state_machine_name].config.whiny_transitions - raise AASM::InvalidTransition.new(self, event_name, state_machine_name) + raise AASM::InvalidTransition.new(self, event_name, state_machine_name, failures) else false end diff --git a/lib/aasm/core/event.rb b/lib/aasm/core/event.rb index 9aea427..5e076fa 100644 --- a/lib/aasm/core/event.rb +++ b/lib/aasm/core/event.rb @@ -72,6 +72,10 @@ module AASM::Core @transitions end + def failed_callbacks + @transitions.flat_map(&:failures) + end + private def attach_event_guards(definitions) diff --git a/lib/aasm/core/transition.rb b/lib/aasm/core/transition.rb index d13fa9a..767ed19 100644 --- a/lib/aasm/core/transition.rb +++ b/lib/aasm/core/transition.rb @@ -2,12 +2,13 @@ module AASM::Core class Transition include DslHelper - attr_reader :from, :to, :event, :opts + attr_reader :from, :to, :event, :opts, :failures alias_method :options, :opts def initialize(event, opts, &block) add_options_from_dsl(opts, [:on_transition, :guard, :after], &block) if block + @failures = [] @event = event @from = opts[:from] @to = opts[:to] @@ -52,7 +53,9 @@ module AASM::Core case code when Symbol, String arity = record.send(:method, code.to_sym).arity - arity == 0 ? record.send(code) : record.send(code, *args) + result = (arity == 0 ? record.send(code) : result = record.send(code, *args)) + failures << code unless result + result when Proc code.arity == 0 ? record.instance_exec(&code) : record.instance_exec(*args, &code) when Array diff --git a/lib/aasm/errors.rb b/lib/aasm/errors.rb index 72e2864..69d2bc3 100644 --- a/lib/aasm/errors.rb +++ b/lib/aasm/errors.rb @@ -3,14 +3,18 @@ module AASM class UnknownStateMachineError < RuntimeError; end class InvalidTransition < RuntimeError - attr_reader :object, :event_name, :state_machine_name + attr_reader :object, :event_name, :state_machine_name, :failures - def initialize(object, event_name, state_machine_name) - @object, @event_name, @state_machine_name = object, event_name, state_machine_name + def initialize(object, event_name, state_machine_name, failures = []) + @object, @event_name, @state_machine_name, @failures = object, event_name, state_machine_name, failures end def message - "Event '#{event_name}' cannot transition from '#{object.aasm(state_machine_name).current_state}'" + "Event '#{event_name}' cannot transition from '#{object.aasm(state_machine_name).current_state}'#{reasoning}" + end + + def reasoning + ". Failed callback(s): #{@failures}." unless failures.empty? end end From 9702bf7a888e8b482aa75074bdc27eb859a586a2 Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 01:21:09 -0700 Subject: [PATCH 2/9] Add specs for new behavior, experiment with error matching --- lib/aasm/core/event.rb | 2 +- lib/aasm/core/transition.rb | 4 ++- spec/unit/callback_multiple_spec.rb | 53 +++++++++++++++++++++++------ spec/unit/event_spec.rb | 29 ++++++++++++++++ spec/unit/transition_spec.rb | 11 ++++++ 5 files changed, 86 insertions(+), 13 deletions(-) diff --git a/lib/aasm/core/event.rb b/lib/aasm/core/event.rb index 5e076fa..114abd0 100644 --- a/lib/aasm/core/event.rb +++ b/lib/aasm/core/event.rb @@ -73,7 +73,7 @@ module AASM::Core end def failed_callbacks - @transitions.flat_map(&:failures) + transitions.flat_map(&:failures) end private diff --git a/lib/aasm/core/transition.rb b/lib/aasm/core/transition.rb index 767ed19..449db99 100644 --- a/lib/aasm/core/transition.rb +++ b/lib/aasm/core/transition.rb @@ -57,7 +57,9 @@ module AASM::Core failures << code unless result result when Proc - code.arity == 0 ? record.instance_exec(&code) : record.instance_exec(*args, &code) + result = (code.arity == 0 ? record.instance_exec(&code) : record.instance_exec(*args, &code)) + failures << code.source_location.join('#') unless result + result when Array if options[:guard] # invoke guard callbacks diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index c5c6ef2..8e94558 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -1,6 +1,16 @@ require 'spec_helper' Dir[File.dirname(__FILE__) + "/../models/callbacks/*.rb"].sort.each { |f| require File.expand_path(f) } +def safe_error(callback = nil) + error = nil + begin + yield + rescue Exception => e + error = e + return error + end +end + describe 'callbacks for the new DSL' do it "be called in order" do @@ -47,9 +57,13 @@ describe 'callbacks for the new DSL' do expect(callback).to_not receive(:after_enter_closed) expect(callback).to_not receive(:after_event) - expect { - callback.left_close! - }.to raise_error(AASM::InvalidTransition) + error = safe_error { callback.left_close! } + + expect(error.class).to eq AASM::InvalidTransition + expect(error.message).to eq( + "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard]." + ) + end it "handles private callback methods as well" do @@ -85,9 +99,12 @@ describe 'callbacks for the new DSL' do expect(callback).to_not receive(:after_event) end - expect { - callback.left_close! - }.to raise_error(AASM::InvalidTransition) + error = safe_error { callback.left_close! } + + expect(error.class).to eq AASM::InvalidTransition + expect(error.message).to eq( + "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard, :transition_guard]." + ) end it "does not run transition_guard twice for multiple permitted transitions" do @@ -133,9 +150,10 @@ describe 'callbacks for the new DSL' do expect(callback).to_not receive(:after_enter_closed) expect(callback).to_not receive(:after) - expect { - callback.close! - }.to raise_error(AASM::InvalidTransition) + error = safe_error(callback) { callback.close! } + + expect(error.class).to eq AASM::InvalidTransition + expect(error.message).to eq "Event 'close' cannot transition from 'open'. Failed callback(s): [\"/Users/woodrich/Dropbox/personal/aasm/spec/models/callbacks/guard_within_block_multiple.rb#30\"]." end end @@ -277,12 +295,25 @@ describe 'event callbacks' do it 'should call it when transition failed for bang fire' do expect(@foo).to receive(:aasm_event_failed).with(:null, :open) - expect {@foo.null!}.to raise_error(AASM::InvalidTransition) + #expect {@foo.null!}.to raise_error(AASM::InvalidTransition) + + error = safe_error { @foo.null! } + + expect(error.class).to eq AASM::InvalidTransition + expect(error.message).to eq( + "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false]." + ) + end it 'should call it when transition failed for non-bang fire' do expect(@foo).to receive(:aasm_event_failed).with(:null, :open) - expect {@foo.null}.to raise_error(AASM::InvalidTransition) + error = safe_error { @foo.null } + + expect(error.class).to eq AASM::InvalidTransition + expect(error.message).to eq( + "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false]." + ) end it 'should not call it if persist fails for bang fire' do diff --git a/spec/unit/event_spec.rb b/spec/unit/event_spec.rb index cddeea9..fa0ea9e 100644 --- a/spec/unit/event_spec.rb +++ b/spec/unit/event_spec.rb @@ -109,6 +109,35 @@ describe 'firing an event' do expect(event.fire(obj, {}, nil, 'arg1', 'arg2')).to eq(:closed) end + context 'when given a gaurd proc' do + it 'should have access to callback failures in the transitions' do + event = AASM::Core::Event.new(:graduate, state_machine) do + transitions :to => :alumni, :from => [:student, :applicant], + :guard => Proc.new { 1 + 1 == 3 } + end + line_number = __LINE__ - 2 + obj = double('object', :aasm => double('aasm', :current_state => :student)) + + event.fire(obj, {}, nil) + expect(event.failed_callbacks).to eq ["#{__FILE__}##{line_number}"] + end + end + + context 'when given a guard symbol' do + it 'should have access to callback failures in the transitions' do + event = AASM::Core::Event.new(:graduate, state_machine) do + transitions :to => :alumni, :from => [:student, :applicant], + guard: :paid_tuition? + end + + obj = double('object', :aasm => double('aasm', :current_state => :student)) + allow(obj).to receive(:paid_tuition?).and_return(false) + + event.fire(obj, {}, nil) + expect(event.failed_callbacks).to eq [:paid_tuition?] + end + end + end describe 'should fire callbacks' do diff --git a/spec/unit/transition_spec.rb b/spec/unit/transition_spec.rb index 3fcbca1..e841cfd 100644 --- a/spec/unit/transition_spec.rb +++ b/spec/unit/transition_spec.rb @@ -145,6 +145,17 @@ describe AASM::Core::Transition, '- when performing guard checks' do expect(st.allowed?(obj)).to be false end + it 'should add the name of the failed method calls to the failures instance var' do + opts = {:from => 'foo', :to => 'bar', :guard => :test} + st = AASM::Core::Transition.new(event, opts) + + obj = double('object') + expect(obj).to receive(:test) + + st.allowed?(obj) + expect(st.failures).to eq [:test] + end + it 'should call the method on the object if unless is a symbol' do opts = {:from => 'foo', :to => 'bar', :unless => :test} st = AASM::Core::Transition.new(event, opts) From 05c72837c95e7efe7f9826ed2b2185fd843c476c Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 01:22:57 -0700 Subject: [PATCH 3/9] Fix spec --- spec/unit/callback_multiple_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index 8e94558..77c13c9 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -312,7 +312,7 @@ describe 'event callbacks' do expect(error.class).to eq AASM::InvalidTransition expect(error.message).to eq( - "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false]." + "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false, :always_false]." ) end From 23927f1429dbd833470b51992b9dff4aabf72e86 Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 01:37:26 -0700 Subject: [PATCH 4/9] Remove local reference in spec --- spec/unit/callback_multiple_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index 77c13c9..0473127 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -153,7 +153,7 @@ describe 'callbacks for the new DSL' do error = safe_error(callback) { callback.close! } expect(error.class).to eq AASM::InvalidTransition - expect(error.message).to eq "Event 'close' cannot transition from 'open'. Failed callback(s): [\"/Users/woodrich/Dropbox/personal/aasm/spec/models/callbacks/guard_within_block_multiple.rb#30\"]." + expect(error.message[0..63]).to eq "Event 'close' cannot transition from 'open'. Failed callback(s):" end end From dbaa50ddcef7a33277ef4cfa76b40b7def9c5e59 Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 10:20:48 -0700 Subject: [PATCH 5/9] Instantiate instance variables in logical order --- lib/aasm/core/transition.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/aasm/core/transition.rb b/lib/aasm/core/transition.rb index 449db99..b6db626 100644 --- a/lib/aasm/core/transition.rb +++ b/lib/aasm/core/transition.rb @@ -8,12 +8,12 @@ module AASM::Core def initialize(event, opts, &block) add_options_from_dsl(opts, [:on_transition, :guard, :after], &block) if block - @failures = [] @event = event @from = opts[:from] @to = opts[:to] @guards = Array(opts[:guards]) + Array(opts[:guard]) + Array(opts[:if]) @unless = Array(opts[:unless]) #TODO: This could use a better name + @failures = [] if opts[:on_transition] warn '[DEPRECATION] :on_transition is deprecated, use :after instead' From 53d333cfc5f053e44c1e00b92d579537646aae0c Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 18:01:48 -0700 Subject: [PATCH 6/9] Move exception helper method into spec_helper --- spec/spec_helper.rb | 10 ++++++++++ spec/unit/callback_multiple_spec.rb | 10 ---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f812a2c..b47d7be 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -22,3 +22,13 @@ Dir[File.dirname(__FILE__) + "/spec_helpers/**/*.rb"].sort.each { |f| require Fi # example model classes Dir[File.dirname(__FILE__) + "/models/*.rb"].sort.each { |f| require File.expand_path(f) } + +def safe_error(callback = nil) + error = nil + begin + yield + rescue Exception => e + error = e + return error + end +end diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index 0473127..d025a8d 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -1,16 +1,6 @@ require 'spec_helper' Dir[File.dirname(__FILE__) + "/../models/callbacks/*.rb"].sort.each { |f| require File.expand_path(f) } -def safe_error(callback = nil) - error = nil - begin - yield - rescue Exception => e - error = e - return error - end -end - describe 'callbacks for the new DSL' do it "be called in order" do From 892e2bc05d84a9f8ca380bd9c4a036880a59555a Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Thu, 22 Oct 2015 18:13:11 -0700 Subject: [PATCH 7/9] Clean up comments --- spec/spec_helper.rb | 2 +- spec/unit/callback_multiple_spec.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b47d7be..b5d69a5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,7 +23,7 @@ Dir[File.dirname(__FILE__) + "/spec_helpers/**/*.rb"].sort.each { |f| require Fi # example model classes Dir[File.dirname(__FILE__) + "/models/*.rb"].sort.each { |f| require File.expand_path(f) } -def safe_error(callback = nil) +def safe_error(callback = nil) # Need callack to maintain scope for Proc testing error = nil begin yield diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index d025a8d..2e0de55 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -285,8 +285,6 @@ describe 'event callbacks' do it 'should call it when transition failed for bang fire' do expect(@foo).to receive(:aasm_event_failed).with(:null, :open) - #expect {@foo.null!}.to raise_error(AASM::InvalidTransition) - error = safe_error { @foo.null! } expect(error.class).to eq AASM::InvalidTransition From bedf98a1502a0b50c6680bfc8d6caf4326919958 Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Sun, 15 Nov 2015 20:22:58 -0800 Subject: [PATCH 8/9] Conform error expectations to rspec standard, remove #safe_error --- spec/spec_helper.rb | 10 ------- spec/unit/callback_multiple_spec.rb | 44 ++++++++++------------------- 2 files changed, 15 insertions(+), 39 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b5d69a5..f812a2c 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -22,13 +22,3 @@ Dir[File.dirname(__FILE__) + "/spec_helpers/**/*.rb"].sort.each { |f| require Fi # example model classes Dir[File.dirname(__FILE__) + "/models/*.rb"].sort.each { |f| require File.expand_path(f) } - -def safe_error(callback = nil) # Need callack to maintain scope for Proc testing - error = nil - begin - yield - rescue Exception => e - error = e - return error - end -end diff --git a/spec/unit/callback_multiple_spec.rb b/spec/unit/callback_multiple_spec.rb index 2e0de55..d7fe11f 100644 --- a/spec/unit/callback_multiple_spec.rb +++ b/spec/unit/callback_multiple_spec.rb @@ -47,12 +47,9 @@ describe 'callbacks for the new DSL' do expect(callback).to_not receive(:after_enter_closed) expect(callback).to_not receive(:after_event) - error = safe_error { callback.left_close! } - - expect(error.class).to eq AASM::InvalidTransition - expect(error.message).to eq( - "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard]." - ) + expect { + callback.left_close! + }.to raise_error(AASM::InvalidTransition, "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard].") end @@ -89,12 +86,9 @@ describe 'callbacks for the new DSL' do expect(callback).to_not receive(:after_event) end - error = safe_error { callback.left_close! } - - expect(error.class).to eq AASM::InvalidTransition - expect(error.message).to eq( - "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard, :transition_guard]." - ) + expect { + callback.left_close! + }.to raise_error(AASM::InvalidTransition, "Event 'left_close' cannot transition from 'open'. Failed callback(s): [:after_transition, :event_guard, :transition_guard].") end it "does not run transition_guard twice for multiple permitted transitions" do @@ -139,11 +133,10 @@ describe 'callbacks for the new DSL' do 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) - error = safe_error(callback) { callback.close! } - - expect(error.class).to eq AASM::InvalidTransition - expect(error.message[0..63]).to eq "Event 'close' cannot transition from 'open'. Failed callback(s):" end end @@ -285,23 +278,16 @@ describe 'event callbacks' do it 'should call it when transition failed for bang fire' do expect(@foo).to receive(:aasm_event_failed).with(:null, :open) - error = safe_error { @foo.null! } - - expect(error.class).to eq AASM::InvalidTransition - expect(error.message).to eq( - "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false]." - ) - + expect{ + @foo.null! + }.to raise_error(AASM::InvalidTransition, "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false].") end it 'should call it when transition failed for non-bang fire' do expect(@foo).to receive(:aasm_event_failed).with(:null, :open) - error = safe_error { @foo.null } - - expect(error.class).to eq AASM::InvalidTransition - expect(error.message).to eq( - "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false, :always_false]." - ) + expect{ + @foo.null + }.to raise_error(AASM::InvalidTransition, "Event 'null' cannot transition from 'open'. Failed callback(s): [:always_false, :always_false].") end it 'should not call it if persist fails for bang fire' do From 134129ee8235eba9b6aa1f773c35bfa0c90d6914 Mon Sep 17 00:00:00 2001 From: Chris Woodrich Date: Sun, 10 Jan 2016 15:13:38 -0800 Subject: [PATCH 9/9] Restructure error sentence per @alto suggestion Fix failing spec by adding period Use upstream version of Proc handling Fix spacing Restart travis --- lib/aasm/core/transition.rb | 2 +- lib/aasm/errors.rb | 4 ++-- spec/unit/transition_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/aasm/core/transition.rb b/lib/aasm/core/transition.rb index b5b7033..87bd0e0 100644 --- a/lib/aasm/core/transition.rb +++ b/lib/aasm/core/transition.rb @@ -58,7 +58,7 @@ module AASM::Core failures << code unless result result when Proc - result = (code.arity == 0 ? record.instance_exec(&code) : record.instance_exec(*args, &code)) + result = (code.parameters.size == 0 ? record.instance_exec(&code) : record.instance_exec(*args, &code)) failures << code.source_location.join('#') unless result result when Array diff --git a/lib/aasm/errors.rb b/lib/aasm/errors.rb index 69d2bc3..d9b5792 100644 --- a/lib/aasm/errors.rb +++ b/lib/aasm/errors.rb @@ -10,11 +10,11 @@ module AASM end def message - "Event '#{event_name}' cannot transition from '#{object.aasm(state_machine_name).current_state}'#{reasoning}" + "Event '#{event_name}' cannot transition from '#{object.aasm(state_machine_name).current_state}'. #{reasoning}" end def reasoning - ". Failed callback(s): #{@failures}." unless failures.empty? + "Failed callback(s): #{@failures}." unless failures.empty? end end diff --git a/spec/unit/transition_spec.rb b/spec/unit/transition_spec.rb index ec9205f..05b313b 100644 --- a/spec/unit/transition_spec.rb +++ b/spec/unit/transition_spec.rb @@ -6,7 +6,7 @@ describe 'transitions' do process = ProcessWithNewDsl.new expect { process.stop! }.to raise_error do |err| expect(err.class).to eql(AASM::InvalidTransition) - expect(err.message).to eql("Event 'stop' cannot transition from 'sleeping'") + expect(err.message).to eql("Event 'stop' cannot transition from 'sleeping'. ") expect(err.object).to eql(process) expect(err.event_name).to eql(:stop) end