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

after_transition doesn't have previous state scoped #259

Open
msxavi opened this issue Jul 27, 2017 · 3 comments
Open

after_transition doesn't have previous state scoped #259

msxavi opened this issue Jul 27, 2017 · 3 comments

Comments

@msxavi
Copy link

msxavi commented Jul 27, 2017

Let's say we need to send an email after every state transition and the requirement is to include in this email subject: "Changed From: #{previous_state} TO: #{current_state}"

The way I currently do it:

  after_transition do |model, transition|
    previous_state = model.transitions.where(most_recent: false).last.to_state
    StateTransitionNotifier.new(previous_state, model.current_state).perform
  end

Why not having the "from" state scoped here? It would not be correct doing this on before_transition callback as something could go wrong, right? Even though, we could rely on validations, using before_transition would be semantically incorrect.

@stevebissett
Copy link

stevebissett commented Aug 22, 2017

@msxavi

previous_state = model.transitions.where(most_recent: false).last.to_state

It seems like this won't give the correct state in the case that we are in the second state (i.e. There has only been one transition).

Going to need to use initial_state if that is the case.

previous_state = model.transitions.where(most_recent: false).last.to_state || initial_state

I can't find any reference to previous state in this repo? I am a bit confused how this is not something required regularly by other users of this gem?

@stevebissett
Copy link

stevebissett commented Aug 28, 2017

@msxavi

In Statesman::Machine the current_state looks like this:

    def current_state(force_reload: false)
      last_action = last_transition(force_reload: force_reload)
      last_action ? last_action.to_state : self.class.initial_state
    end

This means that it can potentially pull the most recent transition (if persisted).
If previous_state was scoped here:

model.previous_state
model.current_state

Then this means that we could potentially pull the transition twice.
I am looking at forking for now and adding the from_state field in the DB and adding last_transition on the model:

last_transition = model.last_transition
StateTransitionNotifier.new(last_transition.from_state, last_transition.to_state).perform

@msxavi
Copy link
Author

msxavi commented Aug 28, 2017

@stevebissett Looks interesting and more appropriate, IMO. One transition should be considered as the movement from state to something else. Would be handy.

Funny mentioning the current_state method as I'm facing issues within RSpec lately. But it must be something related to the spec itself. Instead of returning current_state it returns initial_state as per implementation above.

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