geekq / workflow

Ruby finite-state-machine-inspired API for modeling workflow
MIT License
1.75k stars 207 forks source link

Fix AcriveRecord adapter to save the full object state. #154

Closed vitaly closed 5 years ago

vitaly commented 9 years ago

Also, it is important we use the 'bang' version of save, as otherwise the state machine would proceed with entering the 'to' state etc.

lwoodson commented 9 years ago

Not necessarily my call, but the build failed, and would probably like to see a test verifying that updated_at gets set on a transition.

vitaly commented 9 years ago

well, I couldn't get the tests to run at all. it simply breaks. not sure whats the cause, but there were no activerecord adapter tests to begin with, and the change is completely obvious, so I didn't investigate much ;)

vitaly commented 9 years ago

ok, found the test looking at travis. will update the test.

lwoodson commented 9 years ago

Cool, thanks for getting this together, it will be nice to have.

mbhnyc commented 9 years ago

can we get this in? it will help us debug an issue we're seeing with overlapping transactions

jkingsbery commented 9 years ago

+1

the state machine would proceed with entering the 'to' state etc.

We're actually seeing this. Would be great to see this change folded in.

mkcode commented 8 years ago

This will cause some strange behavior if merged as is. In order to prevent the transition events from happening, you need to raise an error or halt before or during the event specific proc.

Check out: https://github.com/geekq/workflow/blob/403a9e44bf49f4d154156d5701e3d67b115ed6da/lib/workflow.rb#L109-L118. Line 118 (after the event specific proc) is the last place in the flow where you can properly stop a state transition.

Anyway, if you would like this behavior, a better way to do it is to add the following:

workflow do
  before_transition do
    halt unless save
  end
end

This will save all model attributes if they valid, and properly halt the transition if it is not valid. Perhaps this should be added to the readme.

dapi commented 5 years ago

Good idea. I think it is better to get it as an option, to save on old behaviour for current users.