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

Callbacks for Implicit Event Transitions #47

Open
mateuszgorniak opened this issue Jul 28, 2016 · 8 comments
Open

Callbacks for Implicit Event Transitions #47

mateuszgorniak opened this issue Jul 28, 2016 · 8 comments

Comments

@mateuszgorniak
Copy link

Hi! I've noticed that callbacks for implicit event transitions don't work.
Example:

state_machine :state, initial: :profile_details do      
event :pending do
  transition from: all - [:pending, :approved, :rejected], to: :pending
end

after_transition on: :pending, do: :generate_something

and now

profile.pending
profile.reload.something != nil # => true

but

profile.state = 'pending'
profile.save
profile.reload.something # => nil

Is there any solution to enable callbacks using normal assignment of the data (without using events)?

Gems:
Rails 5.0
state_machines-activerecord 0.4.0

@mateuszgorniak
Copy link
Author

Any news? :-)

@robotfelix
Copy link

I'd love to know the answer too. It's not clear if the lack of callback firing is a bug or an intended feature!

@SirRawlins
Copy link

This is a big one for me. To my mind, the callbacks should be triggered regardless of the way in which the state changes, be it events or setting the state manually.

@SirRawlins
Copy link

Doing a little hunting around and it seems that this might be 'by design' in the upstream state_machines library. The callbacks are triggered by the Transition class, which is dependant on an event being present,

https://github.com/state-machines/state_machines/blob/master/lib/state_machines/transition.rb#L2

When calling the simple state= setter it has a from and to available, but no way to determine the associated event, and therefore run the callbacks.

I'd be interested in @rosskevin and @seuros thoughts on this - should we upstream the idea? Is it likely to be something we'd want to see in the core?

@obie
Copy link

obie commented Aug 19, 2017

bump. ran into this just now

@xn
Copy link

xn commented Sep 25, 2017

Bump. Me too.

@bobwhitelock
Copy link

I ran into this as well, and found it quite confusing. I can see it not being as simple though as just having state= work out what the event should be and triggering the corresponding callback, as it's possible there could be multiple events which transition between the same 2 states and there would be no way to know which is meant.

However the current behaviour of having multiple ways to change state which may or may not run callbacks seems confusing and means it's possible to run into this behaviour and not notice until it's too late (things have been deployed etc.).

Possible improvements I can imagine to this might be:

  • calling the callbacks for the event which would have transitioned between the two states when state= runs, but only when this can be uniquely determined (and erroring otherwise?);

  • preventing state= from being called by users directly so they must explicitly choose an event to use (not sure if this is possible though, and might break other things which rely on AR attributes being settable).

There's probably other options as well. If any of these options was implemented it could be made opt-in to avoid breaking existing code.

I'm happy to help with improving this but would need some guidance as to what approach would be preferred for improving this.

@philihp
Copy link

philihp commented Jul 29, 2019

What value would this have?

I think it would be best to avoid setting state directly. Instead, do as the documentation suggests, and name the event to trigger the transition:

vehicle.state_event = "ignite"
vehicle.save
...
vehicle.ignite
...
vehicle.fire_events(:ignite)

The one example in the README that shows setting state directly has the following comment, indicating that this is skipping the state_machine

# Skipping state_machine and writing to attributes directly
vehicle.state = "parked"

It feels like to me there are valid reasons for setting state (e.g. test setup, scripting such as in seeds.rb, resetting state of an object from console, trap doors for admin overrides to go around state_machine), but it shouldn't be encouraged.

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

7 participants