Event transitions should work with multiple state targets

[pacso]

Currently, triggering an event will only try to transition_to! the first allowed state for the current_state in the given event.

In the case that two (or more) states are allowed, only the first state transition is ever attempted. If a guard fails on the first transition, an exception is raised and the event fails. This is regardless of whether the second transition would have been successful.

Tests are not complete for this pull request yet - I've just added enough to highlight the example and cover the small code-change I've made. I'll flesh out the test coverage tomorrow.

Any idea why events are currently only capable of transitioning to a single state? In this implementation they offer almost nothing over the transition_to methods?

[pacso]

Any thoughts/comments on this?

[jackfranklin]

This isn't something we personally do in our state machines but I quite like the idea - don't see any reason why it couldn't be something to pull in.

@mrappleton what do you think?

[isaacseymour]

Seems a bit funky that a guard failure raises TransitionFailedError, since this occurs only when all possible target_states are guarded against. Maybe something like this would be nicer:

raise Statesman::GuardFailedError,
      "Guards on transitions from #{current_state} to #{target_states.join(", ")} returned false"

[pacso]

Would it be more obvious if a failed event raised an EventFailedError? Or do you think we should stick with a GuardFailedError in this case?

[markquezada]

@pacso This functionality is exactly what I was referring to in #56. I'm looking forward to seeing it merged in!

[pacso]

You're right @isaacseymour - this will only be raised if guards on all transitions fail. This is a more accurate error to raise.

I also added tests for successful and failed transitions from/to the same state. This new condition against the failed states rather than simply the post-event current_state now covers that scenario.

[isaacseymour]

First off, thanks to everyone involved (especially @pacso), we appreciate both the comments and the code that you contributed. After some deep discussion internally we've come to the conclusion that this doesn't really fit with the philosophy behind Statesman. Our intention is to provide a lightweight state machine gem which handles basic state machine functionality (defining states and transitions, and guarding them) really well, tidily, and with a nice DSL.

Allowing multiple to states for an event will make it very trivial to start pushing application logic into the state machine, which we feel is the wrong place for it, and don't want to provide.

If this is something you need, and the to states are so closely related that they have the same transitions, it may be more logical to merge them into a single state and use metadata to store the differences. If they are not that similar, then we feel it would be a bad idea for the state machine to allow anyone to apply one of several transitions without being explicit. Events were introduced as syntactic sugar for transitions, rather than as a way of encapsulating logic.

[pacso]

Some thoughts on that ...

1. If you aren't going to support events properly, I don't think you should have any event related syntax in your state machine DSL. It's simply confusing and provides the misconception that events will work as expected (as they do in every other state machine implementation) when in actual fact it's simply an alias for transition_to
2. If you (as you claim) do not want to "push application logic into the state machine" then you absolutely should not provide guards on transitions. Surely putting this logic into a guard is a direct transfer of application logic into the state machine, which contradicts your desire to have the application handle the all of the logic and the state machine be a simple system of states and transitions.

With my own branch of this gem I can get by for now until I have time to write my own state machine for Rails 4, so the outcome of this PR doesn't really matter. However if you're going to take a particular stance on the implementation of a state machine, you really ought to stick by it.

Either remove events entirely, or implement them in full.
Either allow the state machine to handle the state-transition logic entirely, or not at all.

A half-way house is no good to anybody.

[markquezada]

I completely agree with @pacso and I urge you to reconsider your stance on this. His points above are spot on. Either completely remove events, or fix them. In their current state, they're not very useful.

There's a good reason every other state machine gem allows this functionality. You could argue statesman is not really a state machine (emphasis on the machine aspect) without it.

I'll try one last approach to convince you 😃

I'll pose this question to you: when would you use the state machine without the associated event logic? The answer for me is plainly "never." Which means I'd be forced to create a wrapping "service object" for all but the most trivial state machines. That service object would be so coupled to the underlying state machine, that neither object could be used without the other. That seems like a major code smell to me.

@isaacseymour Re: your point above on states "being so closely related that they have the same transitions" I think you've missed the actual issue. Even though the result of an event transition can be two different states, that doesn't mean they're related. In fact, in many cases, it's quite the opposite. Take the example I wrote in #56:

    event :deliver do
      # The first transition that matches the state and passes its conditions
      # will be used
      transition :pending => :delivered, :if => :deliver_text_message
      transition :pending => :delivery_failed
    end

The states delivered and delivery_failed are in fact polar opposites, even though they are both valid transitions from pending. The result of the deliver event is one of two valid, but opposite states dependent on the guard. With statesman, there's no built-in way to get from pending to one of these two valid states with a single event. I think that's a huge oversight.

How would you model this with Statesman? It's a bit silly; you'd have to check the current state of the machine to ensure it's "pending" and then do try doing the work, manually triggering the correct state depending on the result:

# pseudo-code from some wrapping object or controller
if machine.pending?
  if deliver_text_message
    machine.transition_to(:delivered)
  else
    machine.transition_to(:delivery_failed)
  end
end

How is this logic not directly related to the state machine?

Pushing this logic into the controller as you suggested doesn't make any sense because that's not the only place I'll use the state machine. Perhaps I'm also using it in a background job or in two different versioned API endpoints. As I mentioned above, I'd have to either replicate the logic in multiple places or wrap it in another, highly coupled object that I always use instead. Neither are good options.

Of course, as the gem's maintainers it's ultimately your call. I really do not understand how you are using statesman in a DRY way without this functionality though. None of the examples you've provided seem to address this issue. Please help me understand if I'm missing something.

[barisbalic]

@pacso thanks for your continued interest.

First off, the addition of events is not something we had planned, we never really thought we would use them, but the implementation seemed harmless and the volume of requests pushed us to consider it seriously.
They are not simply an alias for transition_to, as they allow for multiple 'from' states to reach a single 'to' state. For example:

event :retry do
  transition from: :failed, to: :sending
  transition from: :deferred, to: :sending
end

Secondly, although guards allow you to inject some amount of logic into the state machine, they do so to enforce additional constraints that prohibit entering strange or incorrect states. This is something that we use very heavily in our own applications. Whilst this could definitely be abused, it would need to be done intentionally.
In contrast allowing injection of application logic into the events seems like a way to let more people fall into those undesirable scenarios.

@markquezada given your example from above:

event :deliver do
  # The first transition that matches the state and passes its conditions
  # will be used
  transition :pending => :delivered, :if => :deliver_text_message
  transition :pending => :delivery_failed
end

We don't feel like that knowledge, is knowledge that belongs within the state machine. The state machine in our opinion shouldn't know if the text message has delivered successfully or not. That should be dealt with elsewhere, and then depending on the outcome, the state machine can be transitioned. I would suggest that the knowledge of how to do those things could reside in the model class: the model knows its own state and could be responsible for sending the message.

We won't implement something just because other libraries do. If those libraries did what we wanted, we wouldn't have written Statesman. All that is needed to qualify as a state machine is a finite set of states and a way to move between them whilst explicitly being in one. We support this, we're just putting emphasis on being explicit.

[markquezada]

@barisbalic I understand and appreciate your reasoning. Obviously as the gem maintainers, this is your call. Thanks for hearing us out.

I agree that you shouldn't do something just because other libraries do. That wasn't what I meant. I meant more that I think it's important to have an answer for this type of use case, whatever that may be. It sounds like the answer you're prescribing is that this logic should live on the parent model of the state machine. I think that's reasonable since those two objects are already pretty tightly coupled.

It would be helpful if some of the examples covered how to implement the example I used above as part of the model, instead of directly on the state machine itself. Given a model Invitation how would you implement the event described in the above example? I took a quick stab at it:

class InvitationStateMachine
  include Statesman::Machine

  state :pending, initial: true
  state :delivered
  state :delivery_failed

  transition from: :pending to: [:delivered, :delivery_failed]

end

class Invitation
  include Statesman::Adapters::ActiveRecordModel

  def state_machine
    InvitationStateMachine.new(self)
  end

  def deliver!
    if deliver_text_message
      state_machine.transition_to!(:delivered)
    else
      state_machine.transition_to!(:delivery_failed)
    end
  end

  def deliver_text_message
    SomeExternalAPI.send_text_message('Hello!')
  end
end

[isaacseymour]

@markquezada I'd make deliver_text_message private, but that looks ok to me.

[markquezada]

@isaacseymour Good catch. It should definitely be private.

This looks reasonable and pretty DRY to me. I appreciate you all taking the time to explain it.

[barisbalic]

@markquezada no problem, that's why it's open-sourced and discussion about these things will help us make it better.

[edslocomb]

After this discussion, I'm left wondering what the best practices are for using Statesman's lifecycle hooks.

I'm working on a project where there's a considerable amount of domain/business logic in before_transition() and after_transition() blocks in an object's StateMachine companion object. Is this project misusing the Statesman gem?

And incidentally, what is the intended use of before_transition() and after_transition(), if not to provide a way to add business/domain logic to the state machine class?

[appleton]

You can put business logic into those blocks, but that's not to say that you should. I would do roughly what you're suggesting but factor the actual logic into another class which is called from the transition, something like this:

before_transition(to: :shipped) do |order, transition|
  ShippingService.new(order).prepare
end

after_transition(to: :shipped) do |order, transition|
  ShippingService.new(order).ship
end

This way your business logic is all contained in a logical reusable class - IMO that's the real benefit (and point) of saying 'no business logic in state machines'.

[edslocomb]

In that example, you're still putting business/domain logic in before_transition() and after_transition(); the fact that you do it via a calls to methods that have been moved to another file doesn't remove the business logic from the State Machine class.

The way to remove it from the StateMachine class is to define wrappers around transition_to() that include the business/domain logic, and call those wrappers wherever you're currently calling transition_to()

You could leave the wrappers in your model, or you could put them in another file and call it Concerns::TransitionHandler or what have you.

But if that's the way we're supposed to do things, what are the lifecycle hooks meant to be used for?

[barisbalic]

I agree with @mrappleton. This separation allows you to work with, and test, the behaviours that would have been embedded in the state machine, separately from the state machine. I can see, however, that the state machine is still pulling the trigger, which may feel contradictory.

If this were a language other than ruby, I would expect to see Statesman publish events about things that are happening. This would allow for event listeners, wherever they may be implemented, to do any work, but as we don't have that option these callbacks feel like a reasonable substitute.
I would be interested to hear other possibilities if you have any thoughts.

[edslocomb]

If before_transition() and after_transition() are just a workaround you settled for because ruby doesn't have a native event handling framework, then I think what you probably want to do in some future iteration is get rid of the lifecycle hooks and use the Observable api in ruby's standard library instead.

I'm personally not at all a fan of lifecycle hooks; I feel like the last thing any rails project needs is more business/domain logic called from persistence/lifecycle hooks. Using Observer has its own tradeoffs, of course, but it seems to me like a better fit for Statesman's design, if I'm reading this discussion right.

[barisbalic]

@edslocomb I'm not settling as such, it was already implemented when I arrived, it's just how I think of it. Your suggestion sounds very sensible and when I have some time (within a few weeks) I will try it out on a branch to see how it feels, if you do so beforehand we'd love to see it.