feat: improve handling of compensation association #2039

smbea · 2023-12-01T08:44:14Z

Allow to connect compensate boundary event to activity, turning it into compensation activity.
Turn sequence flow connected to boundary event into compensation association
when boundary event is turned replaced with compensate boundary event.

Closes #2038

nikku

I integration tested this and indeed it feels like an overall less blocking UX, which is a major improvement.

What I found is a couple of quirks:

When transforming to a compensation boundary event the connection turns into an undirected association, where I'd expect a directed association. Expected: We use directed association and the task turns into a compensation task.

Existing sequence flows are not removed, despite being illegal for compensation tasks. Expected: Remove all other existing connections.

Expected: Preview reflects future association layout.

lib/features/rules/BpmnRules.js

smbea · 2023-12-04T16:26:58Z

Thanks for looking into this @nikku. I'm going to look into all these details tomorrow

smbea · 2023-12-11T13:50:57Z

When transforming to a compensation boundary event the connection turns into an undirected association, where I'd expect a directed association. Expected: We use directed association and the task turns into a compensation task.

✅ This was a bug fixed via aa2fd41

Existing sequence flows are not removed, despite being illegal for compensation tasks. Expected: Remove all other existing connections.

✅

Expected: Preview reflects future association layout.

drawPreview is being called so I'm still not sure what the issue here is. Will have to look into the connection preview to understand more of how it works. I accept pointers if you have them @nikku 😅

This likely prevents connecting event sub processes to text annotations?

Nope, doesn't affect text annotations

lib/features/modeling/behavior/CompensateBoundaryEventBehaviour.js

nikku

I really like the UX progress you made 🎉

Feels really good. Let's ignore the preview bug for now.

smbea · 2023-12-15T08:36:51Z

Agreed with @barmac that he would take a look at this

barmac · 2023-12-18T15:50:13Z

If there are more than one outgoing sequence flows, we end up with incorrect diagram:

My proposal is to convert only if there is a single sequence flow, and otherwise remove all of the connections.

barmac · 2023-12-18T17:35:29Z

Another edge case found ;):

Screen.Recording.2023-12-18.at.18.34.41.mov

barmac · 2023-12-18T17:37:47Z

There is more:

Screen.Recording.2023-12-18.at.18.37.29.mov

barmac · 2023-12-18T17:40:33Z

However, I got the basic interaction to work:

Screen.Recording.2023-12-18.at.18.40.07.mov

barmac · 2023-12-18T18:06:25Z

There is more:

Screen.Recording.2023-12-18.at.18.37.29.mov

This is an old issue, so I will create a separate issue for this.

barmac · 2023-12-21T08:05:30Z

I retargetted this to develop as it's more a feature than a fix.

nikku · 2023-12-21T12:58:40Z

lib/features/modeling/behavior/CompensateBoundaryEventBehavior.js

+
+      // newSource no longer valid
+      if (isForCompensation(target) && !isCompensationBoundaryEvent(newSource)) {
+        removeIsForCompensationProperty(target);


What scenario did we not cover here?

I am sorry, I don't understand the question. Can you make it more verbose?

This line is not covered by tests, so I wonder which scenario this line represents that we do not not cover through tests.

I will add test cases for that.

This allowed to detect another last bug.

However, this can be recreated only when we forcefully reconnect start. It's not allowed by the rules.

I will leave it out of scope, and remove the respective source code.

nikku · 2023-12-21T12:58:49Z

lib/features/modeling/behavior/CompensateBoundaryEventBehavior.js

+
+      // newSource valid
+      if (canBeForCompensation(target) && isCompensationBoundaryEvent(newSource)) {
+        addIsForCompensationProperty(target);


What scenario did we not cover here?

OK so I debugged it and source changes are handled via ReplaceConnectionBehavior. We don't need explicit compensation-specific code.

lib/features/rules/BpmnRules.js

Association removal is handled in CompensateBoundaryEventBehavior.

Thinks look good!

nikku

This works great!

nikku · 2023-12-21T16:57:10Z

From my point of view you can go ahead and merge this. Consider cleaning up the commits before.

barmac · 2023-12-22T13:56:05Z

Let's 🎃 it!

bpmn-io-tasks bot added the in progress Currently worked on label Dec 1, 2023

bpmn-io-tasks bot assigned smbea Dec 1, 2023

smbea force-pushed the fix-compensation branch from 6cad5af to a4b94e1 Compare December 1, 2023 12:40

smbea changed the base branch from develop to master December 1, 2023 17:45

smbea marked this pull request as ready for review December 1, 2023 17:46

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 1, 2023

smbea requested review from a team, philippfromme and marstamm and removed request for a team December 1, 2023 17:46

nikku previously requested changes Dec 4, 2023

View reviewed changes

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 4, 2023

nikku reviewed Dec 4, 2023

View reviewed changes

lib/features/rules/BpmnRules.js Outdated Show resolved Hide resolved

nikku reviewed Dec 11, 2023

View reviewed changes

lib/features/modeling/behavior/CompensateBoundaryEventBehaviour.js Outdated Show resolved Hide resolved

nikku reviewed Dec 11, 2023

View reviewed changes

lib/features/modeling/behavior/CompensateBoundaryEventBehaviour.js Outdated Show resolved Hide resolved

nikku reviewed Dec 11, 2023

View reviewed changes

smbea force-pushed the fix-compensation branch from b91090c to 134eef7 Compare December 12, 2023 10:08

barmac self-assigned this Dec 15, 2023

barmac force-pushed the fix-compensation branch from 7aa3703 to ed0168c Compare December 18, 2023 12:51

barmac mentioned this pull request Dec 18, 2023

Directed association (from compensation event) can be connected to text annotation #2061

Open

barmac added 8 commits December 21, 2023 09:04

fix: prevent across-scope compensation association connection

7322383

test: more than one candidate forCompensation activities

b06c731

feat: handle missing cases

eeda9b6

feat: disallow connecting to host

b557145

chore: remove unnecessary code

c765b73

feat: handle last interactions

b30f8e0

fix: first save target and then remove flows

45883f1

chore: refactor and document

4e84d70

barmac force-pushed the fix-compensation branch from bc30eda to 4e84d70 Compare December 21, 2023 08:04

barmac changed the base branch from main to develop December 21, 2023 08:05

nikku reviewed Dec 21, 2023

View reviewed changes

lib/features/rules/BpmnRules.js Outdated Show resolved Hide resolved

barmac added 2 commits December 21, 2023 16:16

feat: provide rule for compensation association

406b215

chore: remove redundant code

74ce91c

Association removal is handled in CompensateBoundaryEventBehavior.

barmac requested a review from nikku December 21, 2023 15:21

nikku approved these changes Dec 21, 2023

View reviewed changes

nikku self-requested a review December 21, 2023 16:56

barmac added 3 commits December 22, 2023 14:21

test: test reconnect start

1ced06c

chore: refactor

e3d7773

chore: remove redundant code

ef9f3f4

barmac merged commit 3dbaf50 into develop Dec 22, 2023
12 checks passed

barmac deleted the fix-compensation branch December 22, 2023 14:00

bpmn-io-tasks bot removed the needs review Review pending label Dec 22, 2023

barmac changed the title ~~fix: add isForCompensation on connect~~ feat: improve handling of compensation association Dec 22, 2023

philippfromme mentioned this pull request Jan 5, 2024

Copy and Paste of Comensation Handler Results in an Error #2068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve handling of compensation association #2039

feat: improve handling of compensation association #2039

smbea commented Dec 1, 2023 •

edited by barmac

nikku left a comment

smbea commented Dec 4, 2023

smbea commented Dec 11, 2023 •

edited

nikku left a comment

smbea commented Dec 15, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 21, 2023

nikku Dec 21, 2023

barmac Dec 21, 2023

nikku Dec 21, 2023

barmac Dec 22, 2023

barmac Dec 22, 2023

barmac Dec 22, 2023 •

edited

barmac Dec 22, 2023

nikku Dec 21, 2023

barmac Dec 22, 2023

nikku left a comment

nikku commented Dec 21, 2023

barmac commented Dec 22, 2023

feat: improve handling of compensation association #2039

feat: improve handling of compensation association #2039

Conversation

smbea commented Dec 1, 2023 • edited by barmac

nikku left a comment

Choose a reason for hiding this comment

smbea commented Dec 4, 2023

smbea commented Dec 11, 2023 • edited

nikku left a comment

Choose a reason for hiding this comment

smbea commented Dec 15, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 18, 2023

barmac commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barmac Dec 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

nikku commented Dec 21, 2023

barmac commented Dec 22, 2023

smbea commented Dec 1, 2023 •

edited by barmac

smbea commented Dec 11, 2023 •

edited

barmac Dec 22, 2023 •

edited