state-machines / state_machines-activerecord

StateMachines Active Record Integration
https://github.com/state-machines/state_machines-activerecord
MIT License
401 stars 83 forks source link

Callbacks for Implicit Event Transitions #47

Open mateuszgorniak opened 8 years ago

mateuszgorniak commented 8 years ago

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 commented 7 years ago

Any news? :-)

robotfelix commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

bump. ran into this just now

xn commented 6 years ago

Bump. Me too.

bobwhitelock commented 6 years ago

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:

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 commented 5 years ago

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.