Skip to content

[Workflow] Add transition completed event #22587

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

Merged
merged 1 commit into from
Aug 7, 2017
Merged

Conversation

izzyp
Copy link
Contributor

@izzyp izzyp commented Apr 30, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#8213

Because the "entered" event is the only event dispatched after the new marking is applied, and publish's an event upon entering into a "Place" (as opposed to completing a transition), it is not sufficient for a lot of use cases and is causing bugs.

Example:
Enabled Transitions:

  1. A -> B
  2. B -> C
  3. C -> B

Transition 1 and transition 3, will dispatch an "entered" event on Place B, forcing post transition behaviour to be the same for both transition 1 and 3.

A user might need different behaviour depending on the transition, rather the the destination.
A concrete use case would be when applying an "undo" transition to a subject. One may or may not want to re-trigger all the events associated with the original transition to that Place.

I propose adding a "completed" event (ie. Transition completed) in addition to the entered event.

@izzyp izzyp changed the title Add transition completed event [Workflow] Add transition completed event Apr 30, 2017
@xabbuh
Copy link
Member

xabbuh commented Apr 30, 2017

I am not convinced that we really need this additional event. You can already make use of the entered event and retrieve the applied transition from the Event object.

@izzyp
Copy link
Contributor Author

izzyp commented Apr 30, 2017

Are you suggesting that in the EventSubscriber->getSubscribedEvents() you would subscribe to an event like so:

$events = [
    'workflow.workflow_name.entered.place_name'         => 'onEnteredPlaceName',
    ...
]

and in the onEnteredPlaceName() method you would check and apply logic depending on the transition passed with the event?

like so:

if ($event->getTransition()->getName() == 'draft') {
	some stuff ...
}
elseif ($event->getTransition()->getName() == 'undo') {
	some other stuff ...
}
etc...

This doesn't seem right to me at all. It forces unrelated logic into the same listener methods, the listeners become responsible for knowing which transitions are being called, etc.

It is much cleaner and simpler to dispatch a completed transition event.

The Entered event relates to the place, the Completed event relates to the transition. They serve different purposes.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 2, 2017
@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 May 23, 2017 17:02
@Padam87
Copy link
Contributor

Padam87 commented Jun 25, 2017

Huge 👍 on this. @izzyp gave a very good use case, this is very useful for circular state machines. I had the same problem recently.

I have solved this by checking the transition name as @xabbuh suggested, but it would be nice to be able to tie this to transition complete event.

@nicolas-grekas
Copy link
Member

ping @lyrixx @Nyholm

@Nyholm
Copy link
Member

Nyholm commented Jul 19, 2017

Thank you for the ping.

Thank you @izzyp for this suggestion. Can you tell me how this differ from the "annouce" event? I fail to see any difference. But maybe you see something I don't.
Also, be aware, master Workflow is a bit updated from where you based you PR.

@Padam87
Copy link
Contributor

Padam87 commented Jul 19, 2017

@Nyholm The announce event is for the next available transitions after the event is complete. This one is for the just completed event.

For example:
A -> B
B -> C
C -> B

Applying the A->B transition triggers the events, but none of them are right for this:

  • enter/entered : Refers to the state, instead of the transition, and in this case multiple transitions can move the state to B.
  • announce: Refers to the next available transitions. B->C in this case.
  • completed: Refers to the just completed transition A->B

I understand the reluctance to add a new event yet again, but keep in mind that state machines usually have events for transitions, not states, due to their circular nature. This is not necessarily true for workflows.
Since Symfony supports both, it should have events for both.

@Nyholm
Copy link
Member

Nyholm commented Jul 19, 2017

Thank you. I understand better now.

If applying the A->B transition triggers the events in your example, You wound get:

  • workflow.foo.leave.a
  • workflow.foo.transition.ab_transition
  • workflow.foo.enter.b
  • workflow.foo.entered.b
  • workflow.foo.completed.ab_transition
  • workflow.foo.announce.bc_transition
  • a lot of other related (less detailed) events.

I can understand why you want the new event. I do not think we need workflow.foo.completed and workflow.foo.completed because they exact copies of the "announce" (and "entered"!) events.

I would (but Im not sure the other agree) modify the announce() function and add your one new event there. Maybe even rename it to workflow.foo.completed_transaction.ab_transition

@Padam87
Copy link
Contributor

Padam87 commented Jul 19, 2017

As I understand announce only meant to announce new possibilities.
A->B is complete (new event here: complete.A->B), now you can apply the B->C transition (announce.B->C).

announce is very useful when you want to auto apply a transition. I use this for billing. When the billing conditions are met, apply the bill transition which creates an invoice.

I see these as totally different events, for different use cases. enter and entered refer to the state, announce refers to future transitions, complete would refer to the current transition.

@lyrixx
Copy link
Member

lyrixx commented Jul 20, 2017

Hello. (Sorry for the delay.)

I understand clearly why others event are not suitable for you but right now I fail to see a real use case for this event. Could you (you or @Padam87) tell us more about what you will do in your listener?

Thanks

(note: I'm OK with this PR, I just be sure we really need theses events)

@lyrixx
Copy link
Member

lyrixx commented Jul 20, 2017

Oh, And you need to update the CHANGELOG also.
Thanks.

@Padam87
Copy link
Contributor

Padam87 commented Jul 20, 2017

At printmagus.com (online printing shop) I have recently replaced finite with the new workflow component. I was glad to see that it was an easy swap, just this one issue, with 4 occurrences.

files state machine:
files

The uploaded state can be reached by 2 transitions: upload and recheck.
upload listener does the following:

  • calculates the deadline for the order (deadlines start when the files are uploaded)
  • sends an email confirming the upload and the new deadline
  • emits a message to our WS, notifying the team about the new file ready for preflight

recheck on the other hand:

  • removes any comments about the files
  • removes preflight data
    (basically resets the validation process)

I hope this is enough, the other 3 cases I have are part of our production state machine with 19 states and 30 something transitions, and I don't want a headache right now :D

@lyrixx
Copy link
Member

lyrixx commented Jul 21, 2017

👍

@izzyp
Copy link
Contributor Author

izzyp commented Jul 21, 2017

@Nyholm @lyrixx

The uses outlined by @Padam87 are spot on.

I have many different use cases in my projects that this is indispensable for. The simplest example i can think of is as follows:

An application process follows this workflow:

screen shot 2017-07-21 at 13 19 40

We listen for the "workflow.foo.completed.approve" event - and for example send an email to the applicant requesting "further info" amongst other actions.

If the applicant doesn't respond within a certain amount of time, the cancel transition is applied.

If the applicant responds after the application is already cancelled - we apply the reapprove transition to the application and listen for the "workflow.foo.completed.reapprove" event. For this transition to Approve, we do not need to resend the "further info" email or trigger the other actions that were raised on the original approval, the actions raised in this case are different.

Bottom line - same place is Entered but via 2 (or more) different Transitions and each different transition needs to trigger different behaviour and actions, even though they end in the same Place.

@lyrixx
Copy link
Member

lyrixx commented Jul 21, 2017

@izzyp Don't worry I'm gonna merge this PR ;) I already added a 👍

I'm just waiting for the CHANGELOG and for the fabbot fix

@xabbuh
Copy link
Member

xabbuh commented Jul 24, 2017

@izzyp Looks like something went wrong while rebasing. Did you rebase on master instead of 3.4?

@lyrixx
Copy link
Member

lyrixx commented Aug 4, 2017

@izzyp I can finish this PR for you if you lake time, just tell me;)

@izzyp
Copy link
Contributor Author

izzyp commented Aug 5, 2017

Thanks @lyrixx

I buggered something up while attempting to rebase and i've been too swamped for time to work out what i did wrong or how to fix.

Would appreciate any assistance to get this finished up!

@lyrixx
Copy link
Member

lyrixx commented Aug 7, 2017

Here we go. I cleaned the git history (was not easy, I don't know what you did ;) ) and I updated the PR description.


👍

@izzyp
Copy link
Contributor Author

izzyp commented Aug 7, 2017

Thanks @lyrixx !

@lyrixx
Copy link
Member

lyrixx commented Aug 7, 2017

Failures on appveyor seems not related.

@lyrixx
Copy link
Member

lyrixx commented Aug 7, 2017

It took time, but here we go, this is in now. Thank you very much @izzyp.

@lyrixx lyrixx merged commit c254cac into symfony:3.4 Aug 7, 2017
lyrixx added a commit that referenced this pull request Aug 7, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

[Workflow] Add transition completed event

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR | symfony/symfony-docs#8213

---

Because the "entered" event is the only event dispatched after the new marking is applied, and publish's an event upon entering into a "Place" (as opposed to completing a transition), it is not sufficient for a lot of use cases and is causing bugs.

Example:
Enabled Transitions:
1. A -> B
2. B -> C
3. C -> B

Transition 1 and transition 3, will dispatch an "entered" event on Place B, forcing post transition behaviour to be the same for both transition 1 and 3.

A user might need different behaviour depending on the transition, rather the the destination.
A concrete use case would be when applying an "undo" transition to a subject. One may or may not want to re-trigger all the events associated with the original transition to that Place.

I propose adding a "completed" event (ie. Transition completed) in addition to the entered event.

Commits
-------

c254cac [Workflow] Added an transition completed event
This was referenced Oct 18, 2017
@Guite Guite mentioned this pull request Dec 13, 2017
17 tasks
javiereguiluz pushed a commit to symfony/symfony-docs that referenced this pull request Jan 10, 2018
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 10, 2018
…zzyp)

This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #8213).

Discussion
----------

[Workflow] Document the new transition completed event

as per PR - symfony/symfony#22587

Commits
-------

b8249ab [Workflow] Document the new transition completed event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants