Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 19 commits into from
May 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
62fe6d3
Add unit tests for Call Activity:
csouchet Apr 21, 2023
15eabfd
Add unit tests for Sub-Process:
csouchet Apr 21, 2023
5aac3de
Add unit tests for flow nodes:
csouchet Apr 21, 2023
5df3e62
Add unit tests for events:
csouchet Apr 21, 2023
12f2339
Add unit tests for text annotation in process:
csouchet Apr 21, 2023
711ac90
Add unit tests for pool:
csouchet Apr 20, 2023
9b7df68
assign incoming and outgoing ids of ShapeBpmnElement from flows
csouchet Apr 21, 2023
6e20d89
Fix how to fill incomingIds
csouchet Apr 24, 2023
263277e
Refactor assignIncomingAndOutgoingIdsFromFlows function
csouchet Apr 24, 2023
c355f70
Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…
csouchet Apr 25, 2023
fd744ae
Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…
csouchet May 3, 2023
0b05c2f
Remove unnecessary variable
csouchet May 3, 2023
d78b627
Change some types of test parmaters
csouchet May 3, 2023
0e2105a
extract expected bounds in a variable
csouchet May 3, 2023
8019ae6
Rename tests
csouchet May 3, 2023
c5712a9
Simplify how to get all converted flows
csouchet May 3, 2023
dedaaf9
Merge remote-tracking branch 'origin/master' into 2521-Enrich_links_b…
csouchet May 3, 2023
1ad20e5
Fix import
csouchet May 3, 2023
f2c963a
Merge branch 'master' into 2521-Enrich_links_between_edges_and_shapes…
csouchet May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/component/parser/json/converter/ProcessConverter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,11 @@ export default class ProcessConverter {

deserialize(processes: string | TProcess | (string | TProcess)[]): void {
ensureIsArray(processes).forEach(process => this.parseProcess(process));

// Need to call this after all processes have been parsed, because to link a call activity to the elements of the called process, we have to parse all processes before.
ensureIsArray(processes).forEach(process => this.assignParentOfProcessElementsCalledByCallActivity(process.id));

this.assignIncomingAndOutgoingIdsFromFlows();
}

private assignParentOfProcessElementsCalledByCallActivity(processId: string): void {
Expand All @@ -87,6 +90,25 @@ export default class ProcessConverter {
}
}

private assignIncomingAndOutgoingIdsFromFlows(): void {
const fillShapeBpmnElementAttribute = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. ^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

shapeBpmnElementId: string,
shapeBpmnElementAttributeName: keyof Pick<ShapeBpmnElement, 'outgoingIds' | 'incomingIds'>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙂

Copy link
Member

@tbouffard tbouffard May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's fine.

valueToAdd: string,
): void => {
const shapeBpmnElement =
this.convertedElements.findFlowNode(shapeBpmnElementId) ?? this.convertedElements.findLane(shapeBpmnElementId) ?? this.convertedElements.findPoolById(shapeBpmnElementId);
if (shapeBpmnElement && !shapeBpmnElement[shapeBpmnElementAttributeName].includes(valueToAdd)) {
shapeBpmnElement[shapeBpmnElementAttributeName].push(valueToAdd);
}
};

this.convertedElements.getFlows().forEach(flow => {
fillShapeBpmnElementAttribute(flow.sourceRefId, 'outgoingIds', flow.id);
fillShapeBpmnElementAttribute(flow.targetRefId, 'incomingIds', flow.id);
});
}

private parseProcess(process: TProcess): void {
const processRef = process.id;
const pool = this.convertedElements.findPoolByProcessRef(processRef);
Expand All @@ -106,7 +128,7 @@ export default class ProcessConverter {
ShapeUtil.flowNodeKinds()
.filter(kind => kind != ShapeBpmnElementKind.EVENT_BOUNDARY)
.forEach(kind => this.buildFlowNodeBpmnElements(process[kind], kind, parentId, process.id));
// process boundary events afterwards as we need its parent activity to be available when building it
// process boundary events afterward as we need its parent activity to be available when building it
this.buildFlowNodeBpmnElements(process.boundaryEvent, ShapeBpmnElementKind.EVENT_BOUNDARY, parentId, process.id);

// containers
Expand Down
5 changes: 5 additions & 0 deletions src/component/parser/json/converter/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import type { Flow } from '../../../../model/bpmn/internal/edge/flows';
import ShapeBpmnElement from '../../../../model/bpmn/internal/shape/ShapeBpmnElement';
import type { AssociationFlow, MessageFlow, SequenceFlow } from '../../../../model/bpmn/internal/edge/flows';
import type { GlobalTaskKind, ShapeBpmnEventDefinitionKind } from '../../../../model/bpmn/internal';
Expand All @@ -26,6 +27,10 @@ import { GroupUnknownCategoryValueWarning } from '../warnings';
* @internal
*/
export class ConvertedElements {
getFlows(): Flow[] {
return [...this.messageFlows.values(), ...this.sequenceFlows.values(), ...this.associationFlows.values()];
}

private poolsById: Map<string, ShapeBpmnElement> = new Map();
findPoolById(id: string): ShapeBpmnElement {
return this.poolsById.get(id);
Expand Down
330 changes: 302 additions & 28 deletions test/unit/component/parser/json/BpmnJsonParser.callActivity.test.ts
View file
Open in desktop

Large diffs are not rendered by default.

149 changes: 128 additions & 21 deletions test/unit/component/parser/json/BpmnJsonParser.event.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,23 @@ import type { TEventDefinition } from '../../../../../src/model/bpmn/json/baseEl
import { ShapeBpmnElementKind, ShapeBpmnEventDefinitionKind } from '../../../../../src/model/bpmn/internal';
import { BoundaryEventNotAttachedToActivityWarning, ShapeUnknownBpmnElementWarning } from '../../../../../src/component/parser/json/warnings';

import { expectAsWarning, parseJsonAndExpectEvent, parseJsonAndExpectOnlyFlowNodes, parsingMessageCollector } from '../../../helpers/JsonTestUtils';
import type { ExpectedBoundaryEventShape, ExpectedEventShape } from '../../../helpers/bpmn-model-expect';
import {
expectAsWarning,
parseJsonAndExpectEvent,
parseJsonAndExpectOnlyEdgesAndFlowNodes,
parseJsonAndExpectOnlyFlowNodes,
parsingMessageCollector,
} from '../../../helpers/JsonTestUtils';
import type { ExpectedBoundaryEventShape, ExpectedEventShape, ExpectedShape } from '../../../helpers/bpmn-model-expect';
import { verifyShape } from '../../../helpers/bpmn-model-expect';
import type { BuildDefinitionParameter, BuildEventsParameter, OtherBuildEventKind } from '../../../helpers/JsonBuilder';
import { buildDefinitions, EventDefinitionOn } from '../../../helpers/JsonBuilder';
import { getEventShapes } from '../../../helpers/TestUtils';

type OmitExpectedEventShape = Omit<ExpectedEventShape, 'shapeId' | 'bpmnElementId' | 'bounds'> | Omit<ExpectedBoundaryEventShape, 'shapeId' | 'bpmnElementId' | 'bounds'>;

const expectedBounds = { x: 362, y: 232, width: 36, height: 45 };

function testMustConvertShapes(buildEventParameter: BuildEventsParameter | BuildEventsParameter[], omitExpectedShape: OmitExpectedEventShape, processIsArray = false): void {
const process = { event: buildEventParameter, task: {} };
const json = buildDefinitions({ process: processIsArray ? [process] : process });
Expand All @@ -39,7 +47,7 @@ function testMustConvertShapes(buildEventParameter: BuildEventsParameter | Build
...omitExpectedShape,
shapeId: `shape_event_id_0_${index}`,
bpmnElementId: `event_id_0_${index}`,
bounds: { x: 362, y: 232, width: 36, height: 45 },
bounds: expectedBounds,
};
verifyShape(shape, expectedShape);
});
Expand Down Expand Up @@ -73,39 +81,147 @@ function testMustNotConvertBoundaryEvent(definitionParameter: BuildDefinitionPar
expect(warning1.bpmnElementId).toBe('event_id_0_0');
}

function executeEventCommonTests(buildEventParameter: BuildEventsParameter, omitExpectedShape: OmitExpectedEventShape, titleSuffix = ''): void {
function executeEventCommonTests(buildEventParameter: BuildEventsParameter, omitExpectedShape: OmitExpectedEventShape, titleSuffix: string): void {
it.each([
['object', 'object'],
['object', 'array'],
['array', 'object'],
['array', 'array'],
])(`should convert as Shape, when 'process' (as %s) has '${buildEventParameter.bpmnKind}' (as %s)${titleSuffix}`, (processParameterType: string, eventParameterType: string) => {
testMustConvertShapes(eventParameterType === 'array' ? [buildEventParameter, buildEventParameter] : buildEventParameter, omitExpectedShape, processParameterType === 'array');
});
])(
`should convert as Shape, when 'process' (as %s) has '${buildEventParameter.bpmnKind}' (as %s), ${titleSuffix}`,
(processParameterType: string, eventParameterType: string) => {
testMustConvertShapes(eventParameterType === 'array' ? [buildEventParameter, buildEventParameter] : buildEventParameter, omitExpectedShape, processParameterType === 'array');
},
);

it.each([
["'name'", 'event name'],
["no 'name'", undefined],
])(`should convert as Shape, when '${buildEventParameter.bpmnKind}' has %s${titleSuffix}`, (title: string, eventName: string) => {
])(`should convert as Shape, when '${buildEventParameter.bpmnKind}' has %s, ${titleSuffix}`, (title: string, eventName: string) => {
testMustConvertShapes({ ...buildEventParameter, name: eventName }, { ...omitExpectedShape, bpmnElementName: eventName });
});

describe(`incoming/outgoing management for ${buildEventParameter.bpmnKind}`, () => {
it.each`
title | inputAttribute | expectedAttribute
${'string'} | ${'incoming'} | ${'bpmnElementIncomingIds'}
${'array'} | ${'incoming'} | ${'bpmnElementIncomingIds'}
${'string'} | ${'outgoing'} | ${'bpmnElementOutgoingIds'}
${'array'} | ${'outgoing'} | ${'bpmnElementOutgoingIds'}
`(
`should convert as Shape with $inputAttribute attribute calculated from ${buildEventParameter.bpmnKind} attribute as $title`,
({ title, inputAttribute, expectedAttribute }: { title: string; inputAttribute: 'incoming' | 'outgoing'; expectedAttribute: keyof ExpectedShape }) => {
testMustConvertShapes(
{ ...buildEventParameter, [inputAttribute]: title === 'array' ? [`flow_${inputAttribute}_1`, `flow_${inputAttribute}_2`] : `flow_${inputAttribute}_1` },
{ ...omitExpectedShape, [expectedAttribute]: title === 'array' ? [`flow_${inputAttribute}_1`, `flow_${inputAttribute}_2`] : [`flow_${inputAttribute}_1`] },
);
},
);

it.each`
title | flowKind | expectedAttribute
${'incoming'} | ${'sequenceFlow'} | ${'bpmnElementIncomingIds'}
${'outgoing'} | ${'sequenceFlow'} | ${'bpmnElementOutgoingIds'}
${'incoming'} | ${'association'} | ${'bpmnElementIncomingIds'}
${'outgoing'} | ${'association'} | ${'bpmnElementOutgoingIds'}
`(
`should convert as Shape with $title attribute calculated from $flowKind`,
({ title, flowKind, expectedAttribute }: { title: string; flowKind: 'sequenceFlow' | 'association'; expectedAttribute: keyof ExpectedShape }) => {
const json = buildDefinitions({
process: {
event: buildEventParameter,
task: {},
[flowKind]: {
id: `flow_${title}`,
sourceRef: title === 'incoming' ? 'unknown' : 'event_id_0_0',
targetRef: title === 'incoming' ? 'event_id_0_0' : 'unknown',
},
},
});

const model = parseJsonAndExpectOnlyEdgesAndFlowNodes(json, 1, 2);

verifyShape(model.flowNodes[1], {
...omitExpectedShape,
shapeId: `shape_event_id_0_0`,
bpmnElementId: `event_id_0_0`,
bounds: expectedBounds,
[expectedAttribute]: [`flow_${title}`],
});
},
);

it.each`
title | expectedAttribute
${'incoming'} | ${'bpmnElementIncomingIds'}
${'outgoing'} | ${'bpmnElementOutgoingIds'}
`(`should convert as Shape with $title attribute calculated from message flow`, ({ title, expectedAttribute }: { title: string; expectedAttribute: keyof ExpectedShape }) => {
const json = buildDefinitions({
process: {
event: buildEventParameter,
task: {},
},
messageFlows: {
id: `flow_${title}`,
sourceRef: title === 'incoming' ? 'unknown' : 'event_id_0_0',
targetRef: title === 'incoming' ? 'event_id_0_0' : 'unknown',
},
});

const model = parseJsonAndExpectOnlyEdgesAndFlowNodes(json, 1, 2);

verifyShape(model.flowNodes[1], {
...omitExpectedShape,
shapeId: `shape_event_id_0_0`,
bpmnElementId: `event_id_0_0`,
bounds: expectedBounds,
[expectedAttribute]: [`flow_${title}`],
});
});

it(`should convert as Shape with incoming/outgoing attributes calculated from ${buildEventParameter.bpmnKind} attributes and from flows`, () => {
const json = buildDefinitions({
process: {
event: { ...buildEventParameter, incoming: 'flow_in_1', outgoing: ['flow_out_1', 'flow_out_2'] },
task: {},
sequenceFlow: [
{ id: 'flow_in_1', sourceRef: 'unknown', targetRef: 'event_id_0_0' },
{ id: 'flow_out_2', sourceRef: 'event_id_0_0', targetRef: 'unknown' },
],
association: [{ id: 'flow_out_3', sourceRef: 'event_id_0_0', targetRef: 'unknown' }],
},
messageFlows: { id: 'flow_in_2', sourceRef: 'unknown', targetRef: 'event_id_0_0' },
});

const model = parseJsonAndExpectOnlyEdgesAndFlowNodes(json, 4, 2);

verifyShape(model.flowNodes[1], {
...omitExpectedShape,
shapeId: `shape_event_id_0_0`,
bpmnElementId: `event_id_0_0`,
bounds: expectedBounds,
bpmnElementIncomingIds: ['flow_in_1', 'flow_in_2'],
bpmnElementOutgoingIds: ['flow_out_1', 'flow_out_2', 'flow_out_3'],
});
});
});

if (omitExpectedShape.eventDefinitionKind !== ShapeBpmnEventDefinitionKind.NONE) {
it(`should NOT convert, when there are '${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition' and another 'EventDefinition' in the same element${titleSuffix}`, () => {
it(`should NOT convert, when there are '${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition' and another 'EventDefinition' in the same element, ${titleSuffix}`, () => {
testMustNotConvertEvent({
...buildEventParameter,
eventDefinitionParameter: { ...buildEventParameter.eventDefinitionParameter, withDifferentDefinition: true },
});
});

it(`should NOT convert, when there are several '${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition' in the same element${titleSuffix}`, () => {
it(`should NOT convert, when there are several '${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition' in the same element, ${titleSuffix}`, () => {
testMustNotConvertEvent({
...buildEventParameter,
eventDefinitionParameter: { ...buildEventParameter.eventDefinitionParameter, withMultipleDefinitions: true },
});
});

it(`should NOT convert, when 'definitions' has ${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition and '${buildEventParameter.bpmnKind}' has ${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition & eventDefinitionRef${titleSuffix}`, () => {
it(`should NOT convert, when 'definitions' has ${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition and '${buildEventParameter.bpmnKind}' has ${buildEventParameter.eventDefinitionParameter.eventDefinitionKind}EventDefinition & eventDefinitionRef, ${titleSuffix}`, () => {
testMustNotConvertEvent({
...buildEventParameter,
eventDefinitionParameter: { ...buildEventParameter.eventDefinitionParameter, eventDefinitionOn: EventDefinitionOn.BOTH },
Expand Down Expand Up @@ -195,14 +311,10 @@ describe('parse bpmn as json for all events', () => {
{
bpmnKind: expectedShapeBpmnElementKind as OtherBuildEventKind | 'startEvent',
eventDefinitionParameter: { eventDefinitionKind, eventDefinitionOn, eventDefinition },
incoming: 'flow_in',
outgoing: 'flow_out',
},
{
bpmnElementKind: expectedShapeBpmnElementKind,
bpmnElementName: undefined,
bpmnElementIncomingIds: ['flow_in'],
bpmnElementOutgoingIds: ['flow_out'],
eventDefinitionKind: expectedEventDefinitionKind,
},
);
Expand Down Expand Up @@ -330,20 +442,15 @@ describe('parse bpmn as json for all events', () => {
eventDefinitionParameter: { eventDefinitionKind, eventDefinitionOn },
isInterrupting,
attachedToRef: 'task_id_0_0',
// in practice, the BPMN semantic only defines incoming or outgoing. Tests both at the same time to simplify the tests
incoming: ['flow_in'],
outgoing: ['flow_out'],
},
{
parentId: 'task_id_0_0',
bpmnElementKind: ShapeBpmnElementKind.EVENT_BOUNDARY,
bpmnElementName: undefined,
eventDefinitionKind: expectedEventDefinitionKind,
isInterrupting,
bpmnElementIncomingIds: ['flow_in'],
bpmnElementOutgoingIds: ['flow_out'],
},
`, 'boundaryEvent' is ${isInterruptingTitle} & attached to an 'activity', (${titleForEventDefinitionIsAttributeOf})`,
`'boundaryEvent' is ${isInterruptingTitle} & attached to an 'activity', (${titleForEventDefinitionIsAttributeOf})`,
);

if (isInterrupting) {
Expand Down
Loading