ljmotta commented on code in PR #3580:
URL:
https://github.com/apache/incubator-kie-tools/pull/3580#discussion_r3327086502
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -34,55 +35,58 @@ export function moveNodesInsideSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`BPMN Element with id ${__readonly_subProcessId} is not a
subProcess.`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
Review Comment:
```suggestion
const flowElementsToMove:
Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>[] = [];
```
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -34,55 +35,58 @@ export function moveNodesInsideSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`BPMN Element with id ${__readonly_subProcessId} is not a
subProcess.`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
const artifactsToMove: Normalized<
- ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
+ ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
- subProcess.flowElement?.forEach((flowElement) => {
+ const subProcessNodes = new Set<string>();
+ subProcess.flowElement?.forEach((flowElement: Normalized<Unpacked<typeof
subProcess.flowElement>>) => {
if (flowElement.__$$element !== "sequenceFlow") {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
- flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
+ const toMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
Review Comment:
```suggestion
const toMove: Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>[] = [];
```
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -34,55 +35,58 @@ export function moveNodesInsideSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`BPMN Element with id ${__readonly_subProcessId} is not a
subProcess.`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
const artifactsToMove: Normalized<
- ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
+ ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
- subProcess.flowElement?.forEach((flowElement) => {
+ const subProcessNodes = new Set<string>();
+ subProcess.flowElement?.forEach((flowElement: Normalized<Unpacked<typeof
subProcess.flowElement>>) => {
if (flowElement.__$$element !== "sequenceFlow") {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
- flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
+ const toMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
+
+ const collectElements = (flowElements:
Normalized<BPMN20__tProcess>["flowElement"]): void => {
+ if (!flowElements) {
+ return;
}
- }
+ for (let i = flowElements.length - 1; i >= 0; i--) {
+ const flowElement = flowElements[i];
+ if (
+ nodeIdsToMoveInside.has(flowElement["@_id"]) ||
+ (flowElement.__$$element === "boundaryEvent" &&
+ flowElement["@_attachedToRef"] &&
+ nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
+ ) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (shouldMoveSequenceFlow(flowElement, nodeIdsToMoveInside,
subProcessNodes)) {
+ // If the source and target are both outside of the sub-process
+ // or if the source and target is already in the sub process the
sequenceFlow must be copied
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (isSubProcessElement(flowElement) && flowElement.flowElement) {
+ collectElements(flowElement.flowElement);
+ }
+ }
+ };
+
+ collectElements(process.flowElement ?? []);
Review Comment:
This is a bad pattern. It is a function that is not returning and it is not
modifying its input. Please, pass the `toMove` to the function call.
##########
packages/bpmn-editor/src/mutations/moveNodesOutOfSubProcess.ts:
##########
@@ -24,61 +24,178 @@ import { Normalized } from "../normalization/normalize";
import { State } from "../store/Store";
import { addOrGetProcessAndDiagramElements } from
"./addOrGetProcessAndDiagramElements";
+export type SubProcessElement = Normalized<
+ Extract<
+ Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>,
+ { __$$element: "subProcess" | "adHocSubProcess" | "transaction" }
+ >
+>;
+
+export function isSubProcessElement(element: { __$$element?: string }):
element is SubProcessElement {
+ return (
+ element.__$$element === "subProcess" ||
+ element.__$$element === "adHocSubProcess" ||
+ element.__$$element === "transaction"
+ );
+}
+
+export function findSubProcessRecursively(
+ flowElements: Normalized<BPMN20__tProcess>["flowElement"],
+ subProcessId: string
+): SubProcessElement | undefined {
+ if (!flowElements) {
+ return undefined;
+ }
+ for (const element of flowElements) {
+ if (element["@_id"] === subProcessId && isSubProcessElement(element)) {
+ return element;
+ }
+ if (isSubProcessElement(element) && element.flowElement) {
+ const found = findSubProcessRecursively(element.flowElement,
subProcessId);
+ if (found) {
+ return found;
+ }
+ }
+ }
+ return undefined;
+}
+
+export function findParentFlowElements(
+ flowElements: Normalized<BPMN20__tProcess>["flowElement"],
+ elementId: string
+): Normalized<BPMN20__tProcess>["flowElement"] | undefined {
+ if (!flowElements) {
+ return undefined;
+ }
+ for (const element of flowElements) {
+ if (element["@_id"] === elementId) {
+ return flowElements;
+ }
+ if (isSubProcessElement(element) && element.flowElement) {
+ const found = findParentFlowElements(element.flowElement, elementId);
+ if (found) {
+ return found;
+ }
+ }
+ }
+ return undefined;
+}
+
+export function findParentSubProcess(
+ flowElements: Normalized<BPMN20__tProcess>["flowElement"],
+ elementId: string
+): SubProcessElement | undefined {
+ if (!flowElements) {
+ return undefined;
+ }
+ for (const element of flowElements) {
+ if (isSubProcessElement(element) && element.flowElement) {
+ // Check if this subprocess directly contains the target
+ for (const child of element.flowElement) {
+ if (child["@_id"] === elementId) {
+ return element;
+ }
+ }
+ // Recurse into nested subprocesses
+ const found = findParentSubProcess(element.flowElement, elementId);
+ if (found) {
+ return found;
+ }
+ }
+ }
+ return undefined;
+}
+
+export function shouldMoveSequenceFlow(
+ flowElement: Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>,
+ nodeIds: Set<string>,
+ existingNodesIds: Set<string>
+): boolean {
+ return (
+ flowElement.__$$element === "sequenceFlow" &&
+ !!flowElement["@_sourceRef"] &&
+ !!flowElement["@_targetRef"] &&
+ ((nodeIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (existingNodesIds.has(flowElement["@_sourceRef"]) &&
nodeIds.has(flowElement["@_targetRef"])) ||
+ (nodeIds.has(flowElement["@_sourceRef"]) &&
existingNodesIds.has(flowElement["@_targetRef"])))
+ );
+}
+
export function moveNodesOutOfSubProcess({
definitions,
__readonly_subProcessId,
__readonly_nodeIds,
+ __readonly_targetSubProcessId,
}: {
definitions: State["bpmn"]["model"]["definitions"];
__readonly_subProcessId: string | undefined;
__readonly_nodeIds: string[];
+ __readonly_targetSubProcessId?: string | null;
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`Can't find subProcess with ID
${__readonly_subProcessId}`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ let parentFlowElements: Normalized<BPMN20__tProcess>["flowElement"];
+ let targetSubProcess: SubProcessElement | undefined;
+
+ if (__readonly_targetSubProcessId) {
+ targetSubProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_targetSubProcessId);
+ if (!targetSubProcess) {
+ throw new Error(`Cannot find target subprocess with ID:
${__readonly_targetSubProcessId}`);
+ }
+ targetSubProcess.flowElement ??= [];
+ parentFlowElements = targetSubProcess.flowElement;
+ } else if (__readonly_targetSubProcessId === null) {
+ process.flowElement ??= [];
+ parentFlowElements = process.flowElement;
+ } else {
+ parentFlowElements =
+ findParentFlowElements(process.flowElement ?? [],
__readonly_subProcessId ?? "") ?? process.flowElement ?? [];
+ }
+
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
const artifactsToMove: Normalized<
- ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
+ ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">
>[] = [];
Review Comment:
```suggestion
const flowElementsToMove:
Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>[] = [];
const artifactsToMove:
ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">[] = [];
```
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -34,55 +35,58 @@ export function moveNodesInsideSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`BPMN Element with id ${__readonly_subProcessId} is not a
subProcess.`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
const artifactsToMove: Normalized<
- ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
+ ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">
>[] = [];
const nodeIdsToMoveInside = new Set(__readonly_nodeIds);
- const subProcessNodes = new Set();
- subProcess.flowElement?.forEach((flowElement) => {
+ const subProcessNodes = new Set<string>();
+ subProcess.flowElement?.forEach((flowElement: Normalized<Unpacked<typeof
subProcess.flowElement>>) => {
if (flowElement.__$$element !== "sequenceFlow") {
subProcessNodes.add(flowElement["@_id"]);
}
});
- for (let i = 0; i < (process.flowElement ?? []).length; i++) {
- const flowElement = (process.flowElement ?? [])[i];
- if (
- nodeIdsToMoveInside.has(flowElement["@_id"]) ||
- (flowElement.__$$element === "boundaryEvent" &&
nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
- ) {
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
- } else if (
- flowElement.__$$element === "sequenceFlow" &&
- ((nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (subProcessNodes.has(flowElement["@_sourceRef"]) &&
nodeIdsToMoveInside.has(flowElement["@_targetRef"])) ||
- (nodeIdsToMoveInside.has(flowElement["@_sourceRef"]) &&
subProcessNodes.has(flowElement["@_targetRef"])))
- ) {
- // If the source and target are both outside of the sub-process
- // or if the source and target is already in the sub process the
sequenceFlow must be copied
- flowElementsToMove.push(...((process.flowElement?.splice(i, 1) ?? []) as
typeof flowElementsToMove));
- i--; // repeat one index because we just altered the array we're
iterating over.
+ const toMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
+
+ const collectElements = (flowElements:
Normalized<BPMN20__tProcess>["flowElement"]): void => {
+ if (!flowElements) {
+ return;
}
- }
+ for (let i = flowElements.length - 1; i >= 0; i--) {
+ const flowElement = flowElements[i];
+ if (
+ nodeIdsToMoveInside.has(flowElement["@_id"]) ||
+ (flowElement.__$$element === "boundaryEvent" &&
+ flowElement["@_attachedToRef"] &&
+ nodeIdsToMoveInside.has(flowElement["@_attachedToRef"]))
+ ) {
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (shouldMoveSequenceFlow(flowElement, nodeIdsToMoveInside,
subProcessNodes)) {
+ // If the source and target are both outside of the sub-process
+ // or if the source and target is already in the sub process the
sequenceFlow must be copied
+ toMove.push(...flowElements.splice(i, 1));
+ } else if (isSubProcessElement(flowElement) && flowElement.flowElement) {
+ collectElements(flowElement.flowElement);
+ }
+ }
+ };
+
+ collectElements(process.flowElement ?? []);
+ flowElementsToMove.push(...toMove);
for (let i = 0; i < (process.artifact ?? []).length; i++) {
const artifact = (process.artifact ?? [])[i];
- if (nodeIdsToMoveInside.has(artifact["@_id"])) {
- artifactsToMove.push(...((process.artifact?.splice(i, 1) ?? []) as
typeof artifactsToMove));
+ if (artifact.__$$element !== "association" &&
nodeIdsToMoveInside.has(artifact["@_id"])) {
+ const spliced = process.artifact?.splice(i, 1) ?? [];
Review Comment:
Why we are doing the `splice`? It would be good to give a good name to this
variable.
##########
packages/bpmn-editor/src/mutations/moveNodesInsideSubProcess.ts:
##########
@@ -34,55 +35,58 @@ export function moveNodesInsideSubProcess({
__readonly_nodeIds: string[];
}) {
const { process } = addOrGetProcessAndDiagramElements({ definitions });
- const subProcess = process.flowElement?.find((s) => s["@_id"] ===
__readonly_subProcessId);
- if (
- !(
- subProcess?.__$$element === "subProcess" ||
- subProcess?.__$$element === "adHocSubProcess" ||
- subProcess?.__$$element === "transaction"
- )
- ) {
- throw new Error(`BPMN Element with id ${__readonly_subProcessId} is not a
subProcess.`);
+
+ const subProcess = findSubProcessRecursively(process.flowElement ?? [],
__readonly_subProcessId ?? "");
+ if (!subProcess) {
+ throw new Error(`Cannot find subprocess with ID:
${__readonly_subProcessId}`);
}
- const flowElementsToMove:
Normalized<Unpacked<NonNullable<BPMN20__tProcess["flowElement"]>>>[] = [];
+ const flowElementsToMove:
Normalized<Unpacked<Normalized<BPMN20__tProcess>["flowElement"]>>[] = [];
const artifactsToMove: Normalized<
- ElementExclusion<Unpacked<NonNullable<BPMN20__tProcess["artifact"]>>,
"association">
+ ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">
>[] = [];
Review Comment:
```suggestion
const artifactsToMove:
ElementExclusion<Unpacked<Normalized<BPMN20__tProcess>["artifact"]>,
"association">[] = [];
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
