feat: enrich links between edges and shapes in the internal model #2638

csouchet · 2023-04-20T15:23:41Z

After parsing all process, assign IncomingIds and OutgoingIds of ShapeBpmnElement from the converted Flow.

Add unit tests

Pool

with incoming message flow,
with outgoing message flow,
with incoming/outgoing message flows

Text annotation in process

with incoming association,
with outgoing association,
with incoming/outgoing associations

Events

with incoming attribute,
with outgoing attribute,
with incoming sequence flow,
with outgoing sequence flow,
with incoming association,
with outgoing association,
with incoming message flow,
with outgoing message flow,
with incoming/outgoing attributes and incoming/outgoing flows

Tasks & Gateways

with incoming attribute,
with outgoing attribute,
with incoming sequence flow,
with outgoing sequence flow,
with incoming association,
with outgoing association,
with incoming message flow,
with outgoing message flow,
with incoming/outgoing attributes and incoming/outgoing flows

Sub-Process

with incoming attribute,
with outgoing attribute,
with incoming sequence flow,
with outgoing sequence flow,
with incoming association,
with outgoing association,
with incoming message flow,
with outgoing message flow,
with incoming/outgoing attributes and incoming/outgoing flows

Call Activity

with incoming attribute,
with outgoing attribute,
with incoming sequence flow,
with outgoing sequence flow,
with incoming association,
with outgoing association,
with incoming message flow,
with outgoing message flow,
with incoming/outgoing attributes and incoming/outgoing flows

Closes #2521

github-actions · 2023-04-21T16:49:25Z

♻️ PR Preview f2c963a has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2023-04-21T16:49:26Z

♻️ PR Preview f2c963a has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

- with incoming attribute, - with outgoing attribute, - with incoming sequence flow, - with outgoing sequence flow, - with incoming association, - with outgoing association, - with incoming message flow, - with outgoing message flow, - with incoming/outgoing attributes and incoming/outgoing flows

- with incoming association, - with outgoing association, - with incoming/outgoing associations

- with incoming message flow, - with outgoing message flow, - with incoming/outgoing message flows

…_in_the_internal_model_

tbouffard · 2023-04-27T13:16:06Z

src/component/parser/json/converter/ProcessConverter.ts

@@ -87,6 +90,26 @@ export default class ProcessConverter {
    }
  }

+  private assignIncomingAndOutgoingIdsFromFlows(): void {
+    const fillShapeBpmnElementAttribute = (


question: why defining this function here and not as a private method of the class?
This is not consistent with the rest of the code (we always create private methods in this case).
It may be a good reason, in that case, I suggest to document it as this is unclear to me

As this is a technical method, only used within this context and I wasn't sure where to put it, I preferred to leave it here. But I am open to any ideas. ^^

OK fine. Let's keep it as it is.

tbouffard · 2023-04-27T13:19:39Z

src/component/parser/json/converter/ProcessConverter.ts

+  private assignIncomingAndOutgoingIdsFromFlows(): void {
+    const fillShapeBpmnElementAttribute = (
+      shapeBpmnElementId: string,
+      shapeBpmnElementAttributeName: keyof Pick<ShapeBpmnElement, 'outgoingIds' | 'incomingIds'>,


thought: for simplicity, we may declare instead:

shapeBpmnElementAttributeName: 'outgoingIds' | 'incomingIds'

The 2 attributes won't be renamed (as they are part of the public API) so there is no risk of error.
Anyway, let's keep this as it is now.

Hi @tbouffard,

I understand your suggestion to simplify the code by declaring the property names directly.
However, I personally prefer using keyof Pick to avoid potential naming errors in the future, especially using replace feature of IDE. This approach ensures that only the allowed property names are used, reducing the risk of introducing bugs or errors.

Thanks for sharing your thoughts though! 🙂

Okay, that's fine.

tbouffard · 2023-04-27T13:29:16Z

src/component/parser/json/converter/ProcessConverter.ts

+      }
+    };
+
+    const flows = [...this.convertedElements.getMessageFlows(), ...this.convertedElements.getSequenceFlows(), ...this.convertedElements.getAssociationFlows()];


question: I guess we didn't measure the impact of this new lookup on the parsing performance in particular on large diagram.
The lookup done with convertedElements.findXXX involves retrieving elements from a Map, so it should be fast.
But having real figures would be better.

We could modify the parseBpmn utils to make it display the processing time or create a dedicated new tool for this purpose.

As we decided with @tbouffard, we will manage the perfomance tests in another issue.

After discussions with @csouchet, we decided to create a dedicated issue for the "parser performance": #2665
We will quickly handle it.
We have never done specific parsing performance test, so we have to setup the whole infrastructure. Performance tests involved the whole diagram loading and/or navigation.

tbouffard

✔️ the implementation is OK. We could/should check the performance impact of the new implementation.
✔️ the tests are checking all cases of the implementation
❌ the tests should be refactored to improve the understanding of the reader IHMO.

About the test refactoring

I often mentioned that the usage of the JsonBuilder and highly parametrized tests inputs and checks make them hard to follow/understand. This is particularly true here. It took me more time that I expected to understand what they were doing.
As we are not going to change the way we write tests in this PR, I am pretty sure this could be improved here with a more accurate test names and some test signature simplification as I suggested.

@csouchet I let you read my comments, you can answer to them but I am pretty sure that we should discuss in direct to converge to something that is OK for both of us.

test/unit/component/parser/json/BpmnJsonParser.flowNode.test.ts

tbouffard · 2023-04-27T13:40:51Z

test/unit/component/parser/json/BpmnJsonParser.flowNode.test.ts

-                bpmnElement: `${bpmnKind}_id_0`,
-                Bounds: { x: 10, y: 12, width: 40, height: 45 },
-              },
+      title       | input         | expectedAttribute


praise: much more readable 👍🏿

test/unit/component/parser/json/BpmnJsonParser.flowNode.test.ts

test/unit/component/parser/json/BpmnJsonParser.sub.process.test.ts

…_in_the_internal_model_

src/component/parser/json/converter/utils.ts

tbouffard

LGTM, thanks for the discussions and the integration of my feedback.
Good job! 👍🏿

tbouffard · 2023-05-03T13:56:26Z

src/component/parser/json/converter/ProcessConverter.ts

+  private assignIncomingAndOutgoingIdsFromFlows(): void {
+    const fillShapeBpmnElementAttribute = (
+      shapeBpmnElementId: string,
+      shapeBpmnElementAttributeName: keyof Pick<ShapeBpmnElement, 'outgoingIds' | 'incomingIds'>,


Okay, that's fine.

…etween_edges_and_shapes_in_the_internal_model_

…_in_the_internal_model_

sonarcloud · 2023-05-03T14:55:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

csouchet added enhancement New feature or request depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first labels Apr 20, 2023

csouchet force-pushed the 2521-Enrich_links_between_edges_and_shapes_in_the_internal_model_ branch 2 times, most recently from a11de26 to 6066c9f Compare April 21, 2023 16:48

csouchet added the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Apr 21, 2023

csouchet force-pushed the 2521-Enrich_links_between_edges_and_shapes_in_the_internal_model_ branch from f633cef to 2da3801 Compare April 24, 2023 08:52

csouchet removed the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Apr 24, 2023

csouchet added 9 commits April 25, 2023 14:18

Add unit tests for text annotation in process:

12f2339

- with incoming association, - with outgoing association, - with incoming/outgoing associations

Add unit tests for pool:

711ac90

- with incoming message flow, - with outgoing message flow, - with incoming/outgoing message flows

assign incoming and outgoing ids of ShapeBpmnElement from flows

9b7df68

Fix how to fill incomingIds

6e20d89

Refactor assignIncomingAndOutgoingIdsFromFlows function

263277e

csouchet force-pushed the 2521-Enrich_links_between_edges_and_shapes_in_the_internal_model_ branch from 2da3801 to 263277e Compare April 25, 2023 12:19

csouchet removed the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Apr 25, 2023

csouchet marked this pull request as ready for review April 25, 2023 12:47

csouchet requested a review from tbouffard April 25, 2023 12:47

Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…

c355f70

…_in_the_internal_model_

tbouffard reviewed Apr 27, 2023

View reviewed changes

tbouffard requested changes Apr 27, 2023

View reviewed changes

csouchet and others added 4 commits May 3, 2023 10:47

Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…

fd744ae

…_in_the_internal_model_

Remove unnecessary variable

0b05c2f

Change some types of test parmaters

d78b627

extract expected bounds in a variable

0e2105a

tbouffard reviewed May 3, 2023

View reviewed changes

src/component/parser/json/converter/utils.ts Outdated Show resolved Hide resolved

csouchet added 2 commits May 3, 2023 14:20

Rename tests

8019ae6

Simplify how to get all converted flows

c5712a9

csouchet requested a review from tbouffard May 3, 2023 13:46

tbouffard approved these changes May 3, 2023

View reviewed changes

csouchet and others added 3 commits May 3, 2023 16:02

Merge remote-tracking branch 'origin/master' into 2521-Enrich_links_b…

dedaaf9

…etween_edges_and_shapes_in_the_internal_model_

Fix import

1ad20e5

Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…

f2c963a

…_in_the_internal_model_

csouchet merged commit e39e50d into master May 3, 2023

csouchet deleted the 2521-Enrich_links_between_edges_and_shapes_in_the_internal_model_ branch May 3, 2023 15:12

tbouffard added the BPMN diagram usability Something about the way we can interact with BPMN diagrams label May 3, 2023

tbouffard mentioned this pull request Sep 12, 2023

[FEAT] Only infer incoming/outgoing ids #2852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enrich links between edges and shapes in the internal model #2638

feat: enrich links between edges and shapes in the internal model #2638

csouchet commented Apr 20, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

tbouffard Apr 27, 2023

csouchet May 3, 2023

tbouffard May 3, 2023

tbouffard Apr 27, 2023

csouchet May 3, 2023

tbouffard May 3, 2023 •

edited

Loading

tbouffard Apr 27, 2023

csouchet May 3, 2023

tbouffard May 3, 2023

tbouffard left a comment

tbouffard Apr 27, 2023

tbouffard left a comment

tbouffard May 3, 2023 •

edited

Loading

sonarcloud bot commented May 3, 2023

feat: enrich links between edges and shapes in the internal model #2638

feat: enrich links between edges and shapes in the internal model #2638

Conversation

csouchet commented Apr 20, 2023 • edited Loading

Add unit tests

Pool

Text annotation in process

Events

Tasks & Gateways

Sub-Process

Call Activity

github-actions bot commented Apr 21, 2023 • edited Loading

github-actions bot commented Apr 21, 2023 • edited Loading

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

tbouffard May 3, 2023 • edited Loading

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

tbouffard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouffard left a comment

Choose a reason for hiding this comment

tbouffard May 3, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented May 3, 2023

csouchet commented Apr 20, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

tbouffard May 3, 2023 •

edited

Loading

tbouffard May 3, 2023 •

edited

Loading