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_commit callback behaves strange when cascading events #163

Closed
bitboxer opened this Issue Aug 14, 2014 · 11 comments

Comments

Projects
None yet
2 participants
@bitboxer
Copy link

bitboxer commented Aug 14, 2014

I have a state machine that has a little bit of cascading in it with guards that determine in which state the state machine should end up with.

Sadly the after_commit hook does not work for such a use case when using the active record persistence.

What happens is that it will call the after_commit hook of the final state as often as there have been recursions in the state machine and does not call the after_commit hook for all the other states it went through.

The problem right now is the naiv implementation of the after_commit hook in the active_record_persistence. It will call super for each recursion and when the recursion is done and goes up the chain again it calls the fire_callback for every step, but only with the final state.

I would love to help to fix this, but currently I have no Idea how to do this.

@bitboxer

This comment has been minimized.

Copy link
Author

bitboxer commented Aug 14, 2014

The only current solution I could offer would be to remember every transition in aasm_write_state and use that information later in the aasm_fire_event method and call all the after_commits there. Would that be okay for everybody here? If yes I will create a pull request based on that Idea.

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 14, 2014

Can you show me your state machine, and the calling sequence for your use case, please?

And, please, don't call the implementation naive. This has a very insulting aftertaste. It works fine for most use cases.

@alto alto self-assigned this Aug 14, 2014

@alto alto added the maybe bug label Aug 14, 2014

@bitboxer

This comment has been minimized.

Copy link
Author

bitboxer commented Aug 14, 2014

Sorry, I did not want to insult anyone.

The state machine looks something like this:

state :draft,
state :unassigned,                    after_enter: [:process], after_commit: [:notify_new_object]
state :assigend_to_operator,          after_enter: [:process], after_commit: [:notify_object_assigned]

event :process do
    transitions from: :draft, to: :unassigned
    transitions from: :unassigned, to: :assigned_to_operator, guard: :operator_available?
end

So if we call process in the state draft the system automatically switches the state to assigned_to_operator if it is possible.

Is there another way to do this that works with the after_commit hooks?

The notification creates a new message in the message queue and we need to make sure that the object is saved before it can be processed from the worker.

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 14, 2014

I just learned that the term naive implementation is not meant to be insulting or bad at all. It just describes the code within its contextual properties. So, it's my time to say sorry for being such a puss.

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 14, 2014

Could you try if that works:

state :draft,
state :unassigned,                    after_commit: [:notify_new_object, :process]
state :assigend_to_operator,          after_commit: [:notify_object_assigned, :process]

event :process do
    transitions from: :draft, to: :unassigned
    transitions from: :unassigned, to: :assigned_to_operator, guard: :operator_available?
end

please?

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 14, 2014

By the way, I never saw anyone – including myself – to call an event trigger within any of the AASM callbacks. But it's an interesting use case.

@bitboxer

This comment has been minimized.

Copy link
Author

bitboxer commented Aug 14, 2014

I tried that first, but this does not work because the switch is done after the save, right? I need to commit each change then? I never investigated exactly why that did not work, but I think that's the reason why.

And I saw this approach to state machines a lot. The best thing is that the machine knows how to process the stuff and just stops where it needs more input to go further. This is one of the big big advantages of this kind of thing against using huge if then else things or even worse solutions 😉 .

@bitboxer

This comment has been minimized.

Copy link
Author

bitboxer commented Aug 17, 2014

Any thoughts on this?

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 17, 2014

So, you mean it (putting the :process into each :after_commit hook) basically works, but it commits (and saves) multiple times then to the database? Yes, I would think so.

@alto

This comment has been minimized.

Copy link
Member

alto commented Aug 17, 2014

But that's what I would expect when putting something into the :after_commit callback. That the corresponding state is saved to the database.

What you want is a state machine, which determines the resulting state AND fires all callbacks on its way. Is that right?

This is not how AASM is designed. I already have plans to extend it to support non-deterministic state machines as well, but even then I wouldn't design it as to collect callbacks. I assume there is always one old (or source) and one new (or destination) state, and just one transition, even if that is dynamically calculated.

@alto alto removed the maybe bug label Aug 17, 2014

@alto

This comment has been minimized.

Copy link
Member

alto commented May 29, 2015

Closing for now. Let me know if you have further questions or suggestions.

@alto alto closed this May 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment