tiagobento commented on code in PR #2508:
URL:
https://github.com/apache/incubator-kie-tools/pull/2508#discussion_r1704255818
##########
packages/dmn-editor/src/diagram/Palette.tsx:
##########
@@ -245,7 +245,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-container"}
+ >
Review Comment:
The "DRG nodes" panel doesn't have a `data-testid`, why don't we use the
same strategy we're using for it?
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
Review Comment:
Can we rename this to `externalNodes`? I mean, this is the first test we
have on that "domain".
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from "../__fixtures__/base";
+import { TabName } from "../__fixtures__/editor";
+import { DefaultNodeName, NodeType } from "../__fixtures__/nodes";
+
+test.beforeEach(async ({ stories }) => {
+ await stories.openEmptyWithAvailableExternalModels();
+});
+
+test.describe("Add included node - Decision", () => {
+ test("add to a Decision Service", async ({ editor, page, palette, diagram,
nodes, includedModels }) => {
+ await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+ await includedModels.getIncludeModelButton().click();
+ await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
+ await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });
+ await includedModels.fillModelName({ modelName: "MY_MODEL" });
+ await includedModels.includeModel();
+
+ await editor.changeTab({ tab: TabName.EDITOR });
+ await palette.dragNewNode({ type: NodeType.DECISION_SERVICE,
targetPosition: { x: 400, y: 200 } });
+
+ await palette.getExternalNodesButton().click();
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Sum",
+ targetPosition: {
+ x: 200,
+ y: 200,
+ },
+ });
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Diff",
+ targetPosition: {
+ x: 300,
+ y: 300,
+ },
+ });
+
+ // We click on the button again to close the external nodes panel.
+ await palette.getExternalNodesButton().click();
Review Comment:
Same? Maybe instead of `openExternalNodesPanel` you could call it
`toggleExternalNodesPanel`
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from "../__fixtures__/base";
+import { TabName } from "../__fixtures__/editor";
+import { DefaultNodeName, NodeType } from "../__fixtures__/nodes";
+
+test.beforeEach(async ({ stories }) => {
+ await stories.openEmptyWithAvailableExternalModels();
+});
+
+test.describe("Add included node - Decision", () => {
+ test("add to a Decision Service", async ({ editor, page, palette, diagram,
nodes, includedModels }) => {
+ await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+ await includedModels.getIncludeModelButton().click();
+ await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
+ await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });
+ await includedModels.fillModelName({ modelName: "MY_MODEL" });
+ await includedModels.includeModel();
+
+ await editor.changeTab({ tab: TabName.EDITOR });
+ await palette.dragNewNode({ type: NodeType.DECISION_SERVICE,
targetPosition: { x: 400, y: 200 } });
+
+ await palette.getExternalNodesButton().click();
Review Comment:
I think this is leaking internal details... I would expect something like
`palette.openExternalNodesPanel()`.
##########
packages/dmn-editor/src/diagram/Diagram.tsx:
##########
@@ -1009,12 +1009,14 @@ 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.
+ drgElement: selectedNodes[i].data.dmnObject as DrgElement,
decisionServiceId: state
.computed(state)
.getDiagramData(externalModelsByNamespace)
.nodesById.get(dropTargetNode.id)!.data.dmnObject!["@_id"]!,
snapGrid: state.diagram.snapGrid,
+ decisionShape: selectedNodes[i].data.shape,
+ elementId: selectedNodes[i].id, // The "real" id is here,
which can be the local id or an imported node id (uri + external element id).
Review Comment:
Please see my comment about the mutation itself...
##########
packages/dmn-editor/src/mutations/addDecisionToDecisionService.ts:
##########
@@ -25,25 +25,42 @@ 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 {
+ DMN15__tBusinessKnowledgeModel,
+ DMN15__tDecision,
+ DMN15__tDecisionService,
+ DMN15__tInputData,
+ DMN15__tKnowledgeSource,
+} from "@kie-tools/dmn-marshaller/src/schemas/dmn-1_5/ts-gen/types";
+
+export type DrgElement =
+ | Normalized<{ __$$element: "decision" } & DMN15__tDecision>
+ | Normalized<{ __$$element: "businessKnowledgeModel" } &
DMN15__tBusinessKnowledgeModel>
+ | Normalized<{ __$$element: "decisionService" } & DMN15__tDecisionService>
+ | Normalized<{ __$$element: "inputData" } & DMN15__tInputData>
+ | Normalized<{ __$$element: "knowledgeSource" } & DMN15__tKnowledgeSource>;
export function addDecisionToDecisionService({
definitions,
- decisionId,
+ drgElement,
decisionServiceId,
drdIndex,
snapGrid,
+ decisionShape,
+ elementId,
}: {
definitions: Normalized<DMN15__tDefinitions>;
- decisionId: string;
+ drgElement: DrgElement;
decisionServiceId: string;
drdIndex: number;
snapGrid: SnapGrid;
+ decisionShape: Normalized<DMNDI15__DMNShape>;
+ elementId: string;
}) {
Review Comment:
`elementId` --> `decisionId`
I understand `drgElement` doesn't necessarily is inside `definitions`, so
please make this action dependent on external models too. There's no way to
know whether or not `drgElement` and `elementId` are aligned...
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from "../__fixtures__/base";
+import { TabName } from "../__fixtures__/editor";
+import { DefaultNodeName, NodeType } from "../__fixtures__/nodes";
+
+test.beforeEach(async ({ stories }) => {
+ await stories.openEmptyWithAvailableExternalModels();
+});
+
+test.describe("Add included node - Decision", () => {
+ test("add to a Decision Service", async ({ editor, page, palette, diagram,
nodes, includedModels }) => {
+ await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+ await includedModels.getIncludeModelButton().click();
+ await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
+ await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });
+ await includedModels.fillModelName({ modelName: "MY_MODEL" });
+ await includedModels.includeModel();
+
+ await editor.changeTab({ tab: TabName.EDITOR });
+ await palette.dragNewNode({ type: NodeType.DECISION_SERVICE,
targetPosition: { x: 400, y: 200 } });
+
+ await palette.getExternalNodesButton().click();
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Sum",
+ targetPosition: {
+ x: 200,
+ y: 200,
+ },
+ });
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Diff",
+ targetPosition: {
+ x: 300,
+ y: 300,
+ },
+ });
+
+ // We click on the button again to close the external nodes panel.
+ await palette.getExternalNodesButton().click();
+
+ await diagram.resetFocus();
+ await nodes.move({ name: "Sum", targetPosition: { x: 500, y: 300 } });
+ await nodes.move({ name: "Diff", targetPosition: { x: 500, y: 430 } });
+
+ await diagram.resetFocus();
+
+ // We move it to make it sure that the nodes are added inside the Decision
Services
+ // and are really attached to it, not only "visually over it".
+ await nodes.move({ name: DefaultNodeName.DECISION_SERVICE, targetPosition:
{ x: 120, y: 120 } });
Review Comment:
Great comment.
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from "../__fixtures__/base";
+import { TabName } from "../__fixtures__/editor";
+import { DefaultNodeName, NodeType } from "../__fixtures__/nodes";
+
+test.beforeEach(async ({ stories }) => {
+ await stories.openEmptyWithAvailableExternalModels();
+});
+
+test.describe("Add included node - Decision", () => {
+ test("add to a Decision Service", async ({ editor, page, palette, diagram,
nodes, includedModels }) => {
+ await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+ await includedModels.getIncludeModelButton().click();
Review Comment:
I think this is leaking internal details... I would expect something like
includedModels.addNew(). Not sure how this is being done in other parts of our
test suite, though.
##########
packages/dmn-editor/tests-e2e/drgElements/addIncludedNodes.spec.ts:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { expect, test } from "../__fixtures__/base";
+import { TabName } from "../__fixtures__/editor";
+import { DefaultNodeName, NodeType } from "../__fixtures__/nodes";
+
+test.beforeEach(async ({ stories }) => {
+ await stories.openEmptyWithAvailableExternalModels();
+});
+
+test.describe("Add included node - Decision", () => {
+ test("add to a Decision Service", async ({ editor, page, palette, diagram,
nodes, includedModels }) => {
+ await editor.changeTab({ tab: TabName.INCLUDED_MODELS });
+
+ await includedModels.getIncludeModelButton().click();
+ await includedModels.fillModelToInclude({ modelName: "sumDiffDs.dmn" });
+ await includedModels.selectModel({ modelName: "sumDiffDs.dmn" });
+ await includedModels.fillModelName({ modelName: "MY_MODEL" });
+ await includedModels.includeModel();
+
+ await editor.changeTab({ tab: TabName.EDITOR });
+ await palette.dragNewNode({ type: NodeType.DECISION_SERVICE,
targetPosition: { x: 400, y: 200 } });
+
+ await palette.getExternalNodesButton().click();
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Sum",
+ targetPosition: {
+ x: 200,
+ y: 200,
+ },
+ });
+
+ await palette.dragExternalNode({
+ includedModelName: "MY_MODEL",
+ nodeName: "Diff",
+ targetPosition: {
+ x: 300,
+ y: 300,
+ },
+ });
+
+ // We click on the button again to close the external nodes panel.
+ await palette.getExternalNodesButton().click();
+
+ await diagram.resetFocus();
+ await nodes.move({ name: "Sum", targetPosition: { x: 500, y: 300 } });
+ await nodes.move({ name: "Diff", targetPosition: { x: 500, y: 430 } });
+
+ await diagram.resetFocus();
+
+ // We move it to make it sure that the nodes are added inside the Decision
Services
+ // and are really attached to it, not only "visually over it".
+ await nodes.move({ name: DefaultNodeName.DECISION_SERVICE, targetPosition:
{ x: 120, y: 120 } });
+
+ await
expect(diagram.get()).toHaveScreenshot("add-included-node-inside-decision-service.png");
+ });
+});
Review Comment:
Please add more tests to the basic operations, like changing from one DS to
another. Removing an external Decision from a DS, things like that.
##########
packages/dmn-editor/tests-e2e/__fixtures__/nodes.ts:
##########
@@ -140,9 +140,19 @@ export class Nodes {
}
public async move(args: { name: string; targetPosition: { x: number; y:
number } }) {
- await this.get({ name: args.name }).dragTo(this.diagram.get(), {
- targetPosition: args.targetPosition,
- });
+ if (DefaultNodeName.DECISION_SERVICE === args.name) {
+ // Decision Services only have some draggable areas near the borders.
+ // If you drag it to the center, you'll drag the divide line.
+ // Also, neither the entire upper area nor the entire downer area is
draggable.
+ await this.get({ name: args.name }).dragTo(this.diagram.get(), {
+ targetPosition: args.targetPosition,
+ sourcePosition: { x: 20, y: 20 },
+ });
Review Comment:
Great comment also.
--
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]
