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.

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

kie-issues#1298: Decision Services & multiple DRDs: make it possible o add external Decisions to a Decision Service #2508

Merged
merged 39 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b690ebb
kie-issues#1298: Decision Services & multiple DRDs: make it possible …
danielzhe Jul 31, 2024
33de8ea
Tests e2e
danielzhe Aug 2, 2024
2db60f6
webkit
danielzhe Aug 2, 2024
b55113d
Code review. More tests. Fixed an issue about moving nodes from DS to…
danielzhe Aug 5, 2024
24f1b15
Tests screenshots 1
danielzhe Aug 5, 2024
f916099
Tests screenshots 2
danielzhe Aug 5, 2024
99924ea
Tests screenshots 3
danielzhe Aug 6, 2024
d64a04e
Better screenshots for test.
danielzhe Aug 6, 2024
a078041
Mutations now does not count with "correct parameters".
danielzhe Aug 6, 2024
c9f88bf
Href.
danielzhe Aug 7, 2024
411e61d
Merge branch 'main' into included-ds
danielzhe Aug 7, 2024
6cd536f
Update packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts
danielzhe Aug 7, 2024
a44b7da
Update packages/dmn-editor/src/diagram/Diagram.tsx
danielzhe Aug 7, 2024
fad6167
Update packages/dmn-editor/src/mutations/deleteDecisionFromDecisionSe…
danielzhe Aug 7, 2024
3ee4271
Fix build error (??????)
danielzhe Aug 7, 2024
a84bea2
Merge branch 'main' into included-ds
danielzhe Aug 9, 2024
90c03ff
Fixed the invocation string
danielzhe Aug 14, 2024
c3c23dd
Merge branch 'main' into included-ds
danielzhe Aug 14, 2024
54b419c
Fixed hooks
danielzhe Aug 19, 2024
40a0fdf
Merge branch 'main' into included-ds
danielzhe Aug 22, 2024
227ee5a
Merge branch 'main' into included-ds
danielzhe Sep 2, 2024
3d8abb5
Handles 3rd level of included models
danielzhe Sep 2, 2024
9d7a04e
Fix
danielzhe Sep 3, 2024
a5f3e71
Update packages/dmn-editor/src/propertiesPanel/DecisionServicePropert…
danielzhe Sep 5, 2024
8bbb1cf
Fixing hooks and renaming variable
danielzhe Sep 5, 2024
81d260f
Reduce nesting. Remove unused parameters.
danielzhe Sep 9, 2024
1bdc614
.
tiagobento Sep 11, 2024
ffa1867
Merge pull request #8 from tiagobento/included-ds-pair-programming-da…
danielzhe Sep 11, 2024
2921171
Merge branch 'main' into included-ds
danielzhe Sep 11, 2024
e22fcd1
Refactor DmnEditorRoot
danielzhe Sep 12, 2024
3cb35a5
Refactor DmnEditorRoot
danielzhe Sep 12, 2024
334cf35
Clean
danielzhe Sep 12, 2024
bd5786c
clean
danielzhe Sep 12, 2024
0e90a76
Update packages/dmn-editor/src/mutations/deleteImport.ts
danielzhe Sep 19, 2024
a03e182
Fixes issues 3 and 4
danielzhe Sep 25, 2024
d9408e7
decisionHref
danielzhe Sep 25, 2024
4759ae3
hook
danielzhe Sep 25, 2024
91a815f
Revert
danielzhe Sep 26, 2024
702f44f
Fix 3 and 4 issues
danielzhe Sep 26, 2024
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
4 changes: 3 additions & 1 deletion packages/dmn-editor/src/DmnEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,9 @@ export const DmnEditorInternal = ({
</Tab>

<Tab eventKey={DmnEditorTab.INCLUDED_MODELS} title={tabTitle.includedModels}>
{navigationTab === DmnEditorTab.INCLUDED_MODELS && <IncludedModels />}
<div data-testid={"kie-tools--dmn-editor--included-models-container"}>
{navigationTab === DmnEditorTab.INCLUDED_MODELS && <IncludedModels />}
</div>
</Tab>
</Tabs>
</div>
Expand Down
6 changes: 4 additions & 2 deletions packages/dmn-editor/src/diagram/Diagram.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -992,8 +992,9 @@ export const Diagram = React.forwardRef<DiagramRef, { container: React.RefObject
for (let i = 0; i < selectedNodes.length; i++) {
deleteDecisionFromDecisionService({
definitions: state.dmn.model.definitions,
decisionId: selectedNodes[i].data.dmnObject!["@_id"]!, // We can assume that all selected nodes are Decisions because the contaiment was validated above.
decisionHref: selectedNodes[i].id, // We can assume that all selected nodes are Decisions because the contaiment was validated above.
decisionServiceId: p.data.dmnObject!["@_id"]!,
externalModelsByNamespace,
});
}
} else {
Expand All @@ -1009,12 +1010,13 @@ export const Diagram = React.forwardRef<DiagramRef, { container: React.RefObject
addDecisionToDecisionService({
definitions: state.dmn.model.definitions,
drdIndex: state.computed(state).getDrdIndex(),
decisionId: selectedNodes[i].data.dmnObject!["@_id"]!, // We can assume that all selected nodes are Decisions because the contaiment was validated above.
decisionHref: selectedNodes[i].id, // We can assume that all selected nodes are Decisions because the contaiment was validated above.
decisionServiceId: state
.computed(state)
.getDiagramData(externalModelsByNamespace)
.nodesById.get(dropTargetNode.id)!.data.dmnObject!["@_id"]!,
snapGrid: state.diagram.snapGrid,
externalModelsByNamespace: externalModelsByNamespace,
danielzhe marked this conversation as resolved.
Show resolved Hide resolved
});
}
} else {
Expand Down
6 changes: 5 additions & 1 deletion packages/dmn-editor/src/diagram/Palette.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,11 @@ export function Palette({ pulse }: { pulse: boolean }) {
<br />
<aside className={"kie-dmn-editor--external-nodes-panel-toggle"}>
{diagram.openLhsPanel === DiagramLhsPanel.EXTERNAL_NODES && (
<div className={"kie-dmn-editor--palette-nodes-popover"} style={{ maxHeight }}>
<div
className={"kie-dmn-editor--palette-nodes-popover"}
style={{ maxHeight }}
data-testid={"kie-tools--dmn-editor--external-nodes-popover"}
>
<ExternalNodesPanel />
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export function ExternalNodesPanel() {
const _import = importsByNamespace.get(namespace);
if (!_import) {
console.debug(
`DMN EDITOR: Couldn't find import for namespace '${namespace}', although there's an external DMN referncing it.`
`DMN EDITOR: Couldn't find import for namespace '${namespace}', although there's an external DMN referencing it.`
tiagobento marked this conversation as resolved.
Show resolved Hide resolved
);
return [];
}
Expand All @@ -159,6 +159,7 @@ export function ExternalNodesPanel() {
externalDrgElementId: drgElement["@_id"]!,
})
}
data-testid={`kie-tools--dmn-editor--external-node-${_import["@_name"] === "" ? _import["@_id"] : _import["@_name"]}-${drgElement["@_name"]}`}
tiagobento marked this conversation as resolved.
Show resolved Hide resolved
>
<Flex
alignItems={{ default: "alignItemsCenter" }}
Expand Down
6 changes: 5 additions & 1 deletion packages/dmn-editor/src/includedModels/IncludedModels.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ export function IncludedModels() {
return (
<>
<Modal
data-testid={"kie-tools--dmn-editor--included-models-modal"}
isOpen={isModalOpen}
onClose={() => cancel()}
title={"Include model"}
Expand Down Expand Up @@ -553,7 +554,10 @@ function IncludedModelCard({
ev.preventDefault();
}}
variant={"link"}
style={{ color: "var(--pf-global--danger-color--200)", fontWeight: "bold" }}
style={{
color: "var(--pf-global--danger-color--200)",
fontWeight: "bold",
}}
>
{`Yes, remove included ${extension.toUpperCase()}`}
</AlertActionLink>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,48 @@ import { SnapGrid } from "../store/Store";
import { MIN_NODE_SIZES } from "../diagram/nodes/DefaultSizes";
import { NODE_TYPES } from "../diagram/nodes/NodeTypes";
import { Normalized } from "../normalization/normalize";
import { ExternalModelsIndex } from "../DmnEditor";
import { buildXmlHref, parseXmlHref } from "../xml/xmlHrefs";
import { DmnLatestModel } from "@kie-tools/dmn-marshaller/dist";
import { xmlHrefToQName } from "../xml/xmlHrefToQName";

export function addDecisionToDecisionService({
definitions,
decisionId,
decisionHref,
decisionServiceId,
drdIndex,
snapGrid,
externalModelsByNamespace,
}: {
definitions: Normalized<DMN15__tDefinitions>;
decisionId: string;
decisionHref: string;
decisionServiceId: string;
drdIndex: number;
snapGrid: SnapGrid;
externalModelsByNamespace: ExternalModelsIndex | undefined;
}) {
console.debug(`DMN MUTATION: Adding Decision '${decisionId}' to Decision Service '${decisionServiceId}'`);
console.debug(`DMN MUTATION: Adding Decision '${decisionHref}' to Decision Service '${decisionServiceId}'`);

const decision = definitions.drgElement?.find((s) => s["@_id"] === decisionId);
if (decision?.__$$element !== "decision") {
throw new Error(`DMN MUTATION: DRG Element with id '${decisionId}' is either not a Decision or doesn't exist.`);
const href = parseXmlHref(decisionHref);

const externalModel = externalModelsByNamespace?.[href.namespace ?? ""];
if (href.namespace && !externalModel) {
throw new Error(`DMN MUTATION: Namespace '${href.namespace}' not found.`);
}

if (externalModel && (externalModel.model as Normalized<DmnLatestModel>).definitions) {
danielzhe marked this conversation as resolved.
Show resolved Hide resolved
const externalDrgs = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement;
const decision = externalDrgs?.find((drgElement) => drgElement["@_id"] === href.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal preference, but I always think we should avoid intermediate variables to avoid giving it a name. It's one more thing to hold onto my head when reading code...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very good topic.
I like to split in variables because I find it easier to "think" step-by-step and have less complexity per line.

Also, in this case, I'm giving "meaning" to what (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement: it is the "external drgs". So I don't have to care where it comes from in line 59 or what it is. It is the "external drgs that come from somewhere". Ok, "somewhere" is just a line above, but in my mind, looking at line 59, it is a "problem solved" that I don't have to care about anymore.

But the main point is that if someone new looks at the code it can easily get what (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement means.

if (decision?.__$$element !== "decision") {
throw new Error(
`DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist in the external model '${href.namespace}'`
);
}
Comment on lines +64 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message is targeting two different cases. Not having an element and the element not being a Decision are two distinguish scenarios. I think this should be break into two errors. If decision === undefined the element doesn't exist message. And the decision.__$$element !== "decision" the error telling isn't a Decision. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following the existing error message patterns used in this mutation and in deleteDecisionFromDecisionService. In my opinion, we will only be adding more code to do something that we can do in fewer lines.

} else {
const decision = definitions.drgElement?.find((s) => s["@_id"] === href.id);
if (decision?.__$$element !== "decision") {
throw new Error(`DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist.`);
}
}

const decisionService = definitions.drgElement?.find((s) => s["@_id"] === decisionServiceId);
Expand All @@ -54,8 +77,11 @@ export function addDecisionToDecisionService({
}

const diagram = addOrGetDrd({ definitions, drdIndex });
const hrefString = buildXmlHref({ namespace: href.namespace, id: href.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

The decisionHref isn't the same as the hrefString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The decisionHref can be something like uri#id, id, or even #id.

The buildXmlHref makes sure that we have uri#id or #id

Copy link
Contributor

Choose a reason for hiding this comment

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

A href will always have the the # on its structure. If it's possible to have a href variable without it (an id value), the variable is wrongly named. Now, looking at the source of this value, it comes from the computeDiagramData.ts file which creates the selectedNodesById map. The map id is a value made with the buildXmlHref, so all ids are href, meaning the decisionHref is correctly named.

I think this is a problem in our codebase, where we mix the id and the href, and at some point we need to address this. FYI @tiagobento

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielzhe @tiagobento how are we going to proceed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielzhe Please use decisionHref where hrefString is currently being used, since they are exactly the same string.

const dmnElementRef = xmlHrefToQName(hrefString, definitions);

const decisionShape = diagram.diagramElements.find(
(s) => s["@_dmnElementRef"] === decisionId && s.__$$element === "dmndi:DMNShape"
(s) => s["@_dmnElementRef"] === dmnElementRef && s.__$$element === "dmndi:DMNShape"
) as Normalized<DMNDI15__DMNShape>;

const decisionServiceShape = diagram.diagramElements.find(
Expand All @@ -65,10 +91,10 @@ export function addDecisionToDecisionService({
const section = getSectionForDecisionInsideDecisionService({ decisionShape, decisionServiceShape, snapGrid });
if (section === "encapsulated") {
decisionService.encapsulatedDecision ??= [];
decisionService.encapsulatedDecision.push({ "@_href": `#${decisionId}` });
decisionService.encapsulatedDecision.push({ "@_href": `${hrefString}` });
} else if (section === "output") {
decisionService.outputDecision ??= [];
decisionService.outputDecision.push({ "@_href": `#${decisionId}` });
decisionService.outputDecision.push({ "@_href": `${hrefString}` });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

} else {
throw new Error(`DMN MUTATION: Invalid section to add decision to: '${section}' `);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,43 @@
import { DMN15__tDefinitions } from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { repopulateInputDataAndDecisionsOnDecisionService } from "./repopulateInputDataAndDecisionsOnDecisionService";
import { Normalized } from "../normalization/normalize";
import { buildXmlHref, parseXmlHref } from "../xml/xmlHrefs";
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

import { DmnLatestModel } from "@kie-tools/dmn-marshaller/dist";
import { ExternalModelsIndex } from "../DmnEditor";

export function deleteDecisionFromDecisionService({
definitions,
decisionId,
decisionHref,
decisionServiceId,
externalModelsByNamespace,
}: {
definitions: Normalized<DMN15__tDefinitions>;
decisionId: string;
decisionHref: string;
decisionServiceId: string;
externalModelsByNamespace: ExternalModelsIndex | undefined;
}) {
console.debug(`DMN MUTATION: Deleting Decision '${decisionId}' from Decision Service '${decisionServiceId}'`);
console.debug(`DMN MUTATION: Deleting Decision '${decisionHref}' from Decision Service '${decisionServiceId}'`);

const decision = definitions.drgElement?.find((s) => s["@_id"] === decisionId);
if (decision?.__$$element !== "decision") {
throw new Error(`DMN MUTATION: DRG Element with id '${decisionId}' is either not a Decision or doesn't exist.`);
const href = parseXmlHref(decisionHref);

const externalModel = externalModelsByNamespace?.[href.namespace ?? ""];
if (href.namespace && !externalModel) {
throw new Error(`DMN MUTATION: Namespace '${href.namespace}' not found.`);
}

if (externalModel && (externalModel.model as Normalized<DmnLatestModel>).definitions) {
danielzhe marked this conversation as resolved.
Show resolved Hide resolved
const externalDrgs = (externalModel.model as Normalized<DmnLatestModel>).definitions.drgElement;
const decision = externalDrgs?.find((drgElement) => drgElement["@_id"] === href.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also same as above.

if (decision?.__$$element !== "decision") {
throw new Error(
`DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist in the external model '${href.namespace}'`
);
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

The same with this error.

} else {
const decision = definitions.drgElement?.find((s) => s["@_id"] === href.id);
if (decision?.__$$element !== "decision") {
throw new Error(`DMN MUTATION: DRG Element with id '${href.id}' is either not a Decision or doesn't exist.`);
}
}

const decisionService = definitions.drgElement?.find((s) => s["@_id"] === decisionServiceId);
Expand All @@ -44,11 +66,10 @@ export function deleteDecisionFromDecisionService({
);
}

decisionService.outputDecision = (decisionService.outputDecision ?? []).filter(
(s) => s["@_href"] !== `#${decisionId}`
);
const xmlHref = buildXmlHref({ namespace: href.namespace, id: href.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

The same here. xmlHref will not have the same value as the decisionHref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

xmlHref is exactly the same as href... you're even building it with the same values. No need to have this xmlHref variable...

decisionService.outputDecision = (decisionService.outputDecision ?? []).filter((s) => s["@_href"] !== `${xmlHref}`);
decisionService.encapsulatedDecision = (decisionService.encapsulatedDecision ?? []).filter(
(s) => s["@_href"] !== `#${decisionId}`
(s) => s["@_href"] !== `${xmlHref}`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice


repopulateInputDataAndDecisionsOnDecisionService({ definitions, decisionService });
Expand Down
7 changes: 3 additions & 4 deletions packages/dmn-editor/stories/dev/DevWebApp.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,19 @@
*/

import * as React from "react";
import type { Meta, StoryObj } from "@storybook/react";
import { useCallback, useMemo, useRef, useState } from "react";
import type { Meta, StoryObj } from "@storybook/react";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those changes are made automatically by the Prettier

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Prettier doesn't change the imports... Maybe it's your VS Code "Organize imports"?

import "@patternfly/react-core/dist/styles/base.css";
import { Flex, FlexItem } from "@patternfly/react-core/dist/js/layouts/Flex";
import { Page, PageSection } from "@patternfly/react-core/dist/js/components/Page";
import { DmnLatestModel, getMarshaller, DmnMarshaller } from "@kie-tools/dmn-marshaller";
import { Normalized, normalize } from "@kie-tools/dmn-editor/dist/normalization/normalize";
import { DmnLatestModel, DmnMarshaller, getMarshaller } from "@kie-tools/dmn-marshaller";
import { normalize, Normalized } from "@kie-tools/dmn-editor/dist/normalization/normalize";
import { availableModelsByPath, modelsByNamespace } from "./availableModelsToInclude";
import { generateEmptyDmn15 } from "../misc/empty/Empty.stories";
import { loanPreQualificationDmn } from "../useCases/loanPreQualification/LoanPreQualification.stories";
import { DmnEditorWrapper } from "../dmnEditorStoriesWrapper";
import {
DmnEditorProps,
DmnEditorRef,
ExternalModelsIndex,
OnDmnModelChange,
OnRequestExternalModelByPath,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import EmptyWithAvailableExternalModelsStories from "./EmptyWithAvailableExternalModels.stories";
import { Meta } from "@storybook/blocks";

<Meta title="MDX/Misc/EmptyWithAvailableExternalModelsStories" of={EmptyWithAvailableExternalModelsStories} />

## Empty With Available External Models

When user starts with an empty diagram, a wizard for quick content initialization is displayed.
The wizard enables to create simple diagram with one input and one output or a diagram with one decision table expression.
This model also have some external models available to be included.
Loading
Loading