Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple transition behavior is broken when one of the transitions does not have a "from" parameter #392

Closed
lserman opened this issue Aug 3, 2016 · 4 comments

Comments

@lserman
Copy link

lserman commented Aug 3, 2016

I want to allow administrators to transition a record to any state. Based on the documentation for "multiple transitions" I thought I could do something like this:

event(:test) do
  transition to: :state3, if: :admin_override?
  transition to: :state2, from: :state1
end

The AASM::Core::Event#_fire ends up checking the "from" states if any transitions define it, canceling out the behavior that omitting the "from" argument should allow a transition from any state:

if @transitions.map(&:from).any?
  transitions = @transitions.select { |t| t.from == obj.aasm(state_machine.name).current_state }
  return result if transitions.size == 0
else
  transitions = @transitions
end

The workaround is to define the admin transitions like:

transition to: :state3, from: state_machine.states.map(&:name), if: :admin_override?

@anilmaurya
Copy link
Member

@lserman

Can you mention which version of AASM you are using, I checked on latest version and its working.

Our specs for Transition without from state is passing.
Have a look at:

https://github.com/aasm/aasm/blob/master/spec/models/silencer.rb#L23

https://github.com/aasm/aasm/blob/master/spec/unit/transition_spec.rb#L30

BTW your guards syntax if: :admin_override? is not AASM recommended. I am getting syntax error with this syntax, instead use :if => :admin_override?

@lserman
Copy link
Author

lserman commented Sep 6, 2016

@anilmaurya that spec does not test the functionality I'm talking about. I pasted the offending code (the stuff from _fire method). The code checks if any transition on the event defines a from parameter and then sets the transitions variable to only the transitions where from is the current state. If one transition defines from and another does not, the one that does not is excluded from the transitions variable and will not run, which is the problem.

As for the syntax, the syntax I used is valid since Ruby 1.9.3 and has nothing to do with AASM.

If I find some time I can put together a PR for this.

@anilmaurya
Copy link
Member

@lserman Thank you for reporting this bug and explaining in detail. ♡

Somehow I missed multiple transitions in issue and only checked for single transition.

I don't know why JSON style condition is throwing syntax error in irb. 🤔

anilmaurya added a commit that referenced this issue Sep 6, 2016
@anilmaurya
Copy link
Member

@lserman This bug is resolved in latest release i.e 4.11.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants