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

No easy way to find previous state #263

Open
stevebissett opened this issue Aug 21, 2017 · 6 comments · May be fixed by #348
Open

No easy way to find previous state #263

stevebissett opened this issue Aug 21, 2017 · 6 comments · May be fixed by #348

Comments

@stevebissett
Copy link

Thanks for a great approach to a state machine.

It seems like there is no easy method of finding the previous state for a state machine. I would have thought this would be something very common, so perhaps I am missing something?

The to_state is captured in the DB for a transition, and I thought the from_state would either be saved in the DB, or deduced from transition information.

Is there a way of finding this easily? Or may you explain the reason this was excluded if it was done intentionally?

@stevebissett
Copy link
Author

I am currently using project as the model with state machine.
I am working around this with:

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

I am not aware of a simpler way to do this, or avoiding the DB hit to transitions table.

@nickcampbell18
Copy link
Contributor

I'm not a statesman expert, but it looks like we have some very similar code in our application:

previous_state = model.transitions.order(:sort_key).reject(&:most_recent?).last.to_state

I'm not sure why such a helper method does not exist (but I think you're right, it would require a separate DB call to figure out the answer).

@marclerodrigues marclerodrigues linked a pull request May 27, 2019 that will close this issue
@dmitry
Copy link
Contributor

dmitry commented Jan 22, 2021

model.transitions.where(most_recent: true).first&.to_state || model.initial_state

@ManickYoj
Copy link

ManickYoj commented Jun 10, 2021

This issue is old, but I'm just starting to use Statesman particularly because of it's transition history and I ran into the lack of a 'previous state' feature. I don't think any of these proposals solves the issues thoroughly.

Issue 1

To the phrasing by @stevebissett, which I think is the most complete for this purpose, previous_state = project.transitions.where(most_recent: false).last.to_state || project.initial_state, in the case where no transitions have occurred previous_state will be evaluated to initial state. However, if we're still in the initial state there is no previous state so I'd expect this to return nil.

Also - though this is a minor point on this one phrasing - you'll also get a NoMethodError if no rows are returned for transitions where most_recent is false because last will return nil which will not have a method to_state.

I'd suggest the following code to fix this issue.

if model.transitions.exists?
  # EDIT: It doesn't look like there _is_ a Model.initial_state method.
  # I assume there's a public method or attribute somewhere but I haven't spotted it in the docs yet.
  previous_state = instance.transitions.order(:sort_key).second_to_last&.to_state || Model.initial_state
else
  previous_state = nil
end

--

Issue 2

If the code defining the initial state changes, any models that have only one transition recorded will be return the incorrect result.

eg. On June 1st code defines:

state :foo, initial: true
state :bar
state :baz

We record our record moving from state :foo to state :baz. If we run our previous_state method, foo (the current initial state) will be returned. Good! That's correct.

However on June 15th we refactor our state machine to read

state :foo
state :bar, initial: true
state :baz

Now previous_state for the same record returns :bar even though our record has never been in that state!

Unfortunately, I don't see a way to get around this without persisting the from_state to memory as proposed in #348. Since that neatly solves the issue and we're newly using the repo, for my purposes I'm just going to use that branch instead of the main branch.

@thom-oman
Copy link
Contributor

Hey @ManickYoj, thanks for your thorough comment.

Summarising my understanding of the comments above and on #348

  • Statesman doesn't provide a way to find the previous_state from both the model and, I assume, a given transition
  • There have been multiple variations of what the actual code would look like, all directly query the transitions table but the issue is that this requires another DB call. We would also need to ensure that it handles when the current state is the initial state as well when the previous state is the initial state.
  • No easy way to find previous state #348 solves this by persisting from_state to the transition, however this is a breaking change and backfilling data would likely be problematic (there was a suggestion from @marclerodrigues to include this in the metadata, which I kinda like, though it also requires a potentially fiddly backfill)

I will try find some time in the coming days/weeks to give this some further thought (unfortunately don't have much time to dedicate to Statesman) but my initial thoughts are that adding a previous_state method (likely on both model and transition), similar to suggestions made by many on this thread and that we've used internally, would be a easy first step, even if it requires an additional DB call.

It seems like this is a quite common feature that people would find useful and after that we can explore improvements.

@ManickYoj
Copy link

ManickYoj commented Jun 15, 2021

I think you captured it all, @thom-oman . FWIW, my team decided to work with aasm and the aasm_history gem instead in large part because of Statesman's currently limited ability to accurately and easily determine the previous state. That is to say, I don't particularly have a dog in this fight anymore, but can confirm that it's a common feature that people would find useful 😄

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

Successfully merging a pull request may close this issue.

5 participants