Bug 1796465: Fix for aggregate edge selection #4081

jeff-phillips-18 · 2020-01-27T18:31:15Z

Fixes https://issues.redhat.com/browse/ODC-2852

jeff-phillips-18 · 2020-01-27T18:32:34Z

frontend/packages/dev-console/src/utils/add-resources-menu-utils.ts

@@ -78,6 +78,6 @@ export const createKebabAction: KebabFactory = (label, icon, importType, checkAc
    icon,
    path: getMenuPath(hasApplication, connectorSourceContext),
    href: getAddPageUrl(obj, importType, hasApplication, connectorSourceContext),
-    accessReview: checkAccess && asAccessReview(resourceModel, obj, 'create'),
+    accessReview: checkAccess ? asAccessReview(resourceModel, obj, 'create') : null,


This fixes a warning that shows up in the console stating a boolean was being passed when an object is expected.

I've removed the change for easy rebasing..

christianvogt · 2020-01-29T14:24:39Z

frontend/packages/dev-console/src/components/topology/TopologyEdgePanel.tsx

@@ -49,10 +49,11 @@ const connectorTypeToTitle = (type: string): string => {
 };

 const TopologyEdgePanel: React.FC<TopologyEdgePanelProps> = ({ edge, data, consoleLinks }) => {
-  const source: TopologyDataObject = edge.getSource().getData();
-  const target: TopologyDataObject = edge.getTarget().getData();
+  const panelEdge = edge.getChildren()?.length === 1 ? (edge.getChildren()[0] as Edge) : edge;


Instead of checking edge.getChildren()?.length === 1 could we be specific and check the edge type for the aggregate edge?

The type for aggregate edges depicting a single edge is the original edge type allowing it to create the correct Edge element for display purposes.

christianvogt · 2020-01-29T14:32:24Z

frontend/packages/dev-console/src/components/topology/components/removeConnection.tsx

@@ -28,11 +28,12 @@ export const removeConnection = (edge: Edge): Promise<any> => {
    btnText: 'Delete',
    submitDanger: true,
    executeFn: () => {
+      const removeEdge = edge.getChildren()?.length === 1 ? (edge.getChildren()[0] as Edge) : edge;


Same comment.

andrewballantyne · 2020-01-29T14:41:51Z

/kind bug

jeff-phillips-18 · 2020-01-30T13:40:26Z

/retitle Bug 1796465: Fix for aggregate edge selection

openshift-ci-robot · 2020-01-30T13:40:33Z

@jeff-phillips-18: This pull request references Bugzilla bug 1796465, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1796465: Fix for aggregate edge selection

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

christianvogt · 2020-01-30T18:13:53Z

frontend/packages/topology/src/components/ElementWrapper.tsx

  if (isEdge(element)) {
-    const source = element.getSource();
-    const target = element.getTarget();
+    const source = element.getSourceAnchorNode();
+    const target = element.getTargetAnchorNode();
    if ((source && !source.isVisible()) || (target && !target.isVisible())) {
      return null;
    }


Not sure why the isVisible checks were done in ElementComponent but it makes sense to move all these checks to ElementWrapper instead.

Also please change the comment of this component so that interaction handlers => behaviors

christianvogt · 2020-01-30T18:26:34Z

frontend/packages/topology/src/elements/BaseElement.ts

+    let { visible } = this;
+    try {
+      let parent = this.getParent();
+
+      while (visible && parent) {
+        visible = parent.isVisible() && (!isNode(parent) || !(parent as Node).isCollapsed());
+        parent = parent.getParent();
+      }
+      // eslint-disable-next-line no-empty
+    } catch (e) {}
+
+    return visible;


You shouldn't need a loop because each call to parent.isVisible() will move up.
Use this.parent to avoid the try / catch.

return ( this.visible && (!this.parent || (this.parent.isVisible() && (!isNode(this.parent) || !this.parent.isCollapsed()))) );

christianvogt · 2020-01-30T18:33:03Z

frontend/packages/topology/src/elements/BaseEdge.ts

+    if (!this.source) {
+      throw new Error(`Edge with ID '${this.getId()}' has no source.`);
+    }
+    return getClosestVisibleParent(this.source);


I don't think the we should be involving visibility at this point.
We should only be looking at anchoring to a different source if a parent is collapsed. Therefore we should find the last collapsed parent in the hierarchy. Otherwise return this.source.

This would also mean that we can ensure a source anchor is always present getSourceAnchorNode(): Node {
It may not be visible, but always present. Otherwise we throw.

christianvogt · 2020-01-30T18:34:01Z

frontend/packages/topology/src/elements/BaseEdge.ts

    }
-    return this.sourceAnchor.getLocation(referencePoint);
+    return this.sourceAnchor ? this.sourceAnchor.getLocation(referencePoint) : Point.EMPTY;


We should always have an anchor. If not the topology will be broken with a Point at 0,0.
With the other suggested changes, we can omit this change.

christianvogt · 2020-01-30T18:34:25Z

frontend/packages/topology/src/types.ts

@@ -130,6 +130,8 @@ export interface Edge<E extends EdgeModel = EdgeModel, D = any> extends GraphEle
  setSource(source: Node): void;
  getTarget(): Node;
  setTarget(target: Node): void;
+  getSourceAnchorNode(): Node | null;


Suggested change

getSourceAnchorNode(): Node | null;

getSourceAnchorNode(): Node;

christianvogt · 2020-01-30T18:42:06Z

frontend/packages/topology/src/utils/createAggregateEdges.ts

  return _.reduce(
    edges,
    (newEdges: EdgeModel[], edge: EdgeModel) => {
      const source = getDisplayedNodeForNode(edge.source, nodes);
      const target = getDisplayedNodeForNode(edge.target, nodes);
+
+      // Make sure visible is defined so that changes override what could already be in the element
+      edge.visible = edge.visible === undefined ? true : edge.visible;


Suggested change

edge.visible = edge.visible === undefined ? true : edge.visible;

edge.visible = typeof edge.visible === 'undefined' ? true : edge.visible;

or

Suggested change

edge.visible = edge.visible === undefined ? true : edge.visible;

edge.visible = 'visible' in edge ? edge.visible : true;

christianvogt · 2020-01-30T18:47:15Z

frontend/packages/topology/src/utils/createAggregateEdges.ts

@@ -55,13 +81,14 @@ const createAggregateEdges = (
              source,
              target,
              id: `${source}_${target}`,


Just to be safe, could we prefix this id:

Suggested change

id: `${source}_${target}`,

id: `aggregate:${source}_${target}`,

jeff-phillips-18 · 2020-02-02T15:23:21Z

/retest

christianvogt · 2020-02-03T21:05:30Z

/lgtm
/approve

openshift-bot · 2020-02-03T23:15:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-03T23:42:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-03T23:54:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-04T00:20:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-04T02:04:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-04T02:17:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jeff-phillips-18 · 2020-02-04T12:50:26Z

@christianvogt Rebased

jeff-phillips-18 · 2020-02-04T14:25:35Z

/retest

christianvogt · 2020-02-05T03:13:46Z

/lgtm

openshift-ci-robot · 2020-02-05T03:14:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt]
~~frontend/packages/topology/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-02-05T04:58:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-02-05T05:11:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-02-05T05:24:19Z

@jeff-phillips-18: All pull requests linked via external trackers have merged. Bugzilla bug 1796465 has been moved to the MODIFIED state.

In response to this:

Bug 1796465: Fix for aggregate edge selection

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 27, 2020

jeff-phillips-18 commented Jan 27, 2020

View reviewed changes

jeff-phillips-18 force-pushed the aggregate-edges branch from f3ce3a2 to ad6f2c2 Compare January 29, 2020 12:24

jeff-phillips-18 changed the title ~~Fix for aggregate edge selection, display~~ Fix for aggregate edge selection Jan 29, 2020

jeff-phillips-18 force-pushed the aggregate-edges branch 2 times, most recently from ea45188 to 0c8782e Compare January 29, 2020 14:28

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2020

christianvogt reviewed Jan 29, 2020

View reviewed changes

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 29, 2020

jeff-phillips-18 force-pushed the aggregate-edges branch from 0c8782e to d9abc4d Compare January 30, 2020 12:34

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2020

jeff-phillips-18 force-pushed the aggregate-edges branch 2 times, most recently from 615a9b4 to 28619c0 Compare January 30, 2020 13:25

openshift-ci-robot changed the title ~~Fix for aggregate edge selection~~ Bug 1796465: Fix for aggregate edge selection Jan 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 30, 2020

christianvogt reviewed Jan 30, 2020

View reviewed changes

jeff-phillips-18 force-pushed the aggregate-edges branch from d8fc36a to 552a551 Compare January 30, 2020 20:40

openshift-ci-robot assigned christianvogt Feb 3, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 3, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2020

Fix for aggregate edge selection

5487537

jeff-phillips-18 force-pushed the aggregate-edges branch from 552a551 to 5487537 Compare February 4, 2020 12:49

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 4, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2020

openshift-merge-robot merged commit 333b7ac into openshift:master Feb 5, 2020

spadgett added this to the v4.4 milestone Feb 6, 2020

jeff-phillips-18 deleted the aggregate-edges branch December 2, 2020 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1796465: Fix for aggregate edge selection #4081

Bug 1796465: Fix for aggregate edge selection #4081

jeff-phillips-18 commented Jan 27, 2020 •

edited

jeff-phillips-18 Jan 27, 2020

christianvogt Jan 28, 2020

jeff-phillips-18 Jan 29, 2020

christianvogt Jan 29, 2020

jeff-phillips-18 Jan 29, 2020

christianvogt Jan 29, 2020

andrewballantyne commented Jan 29, 2020

jeff-phillips-18 commented Jan 30, 2020

openshift-ci-robot commented Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

christianvogt Jan 30, 2020

jeff-phillips-18 commented Feb 2, 2020

christianvogt commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 4, 2020

openshift-bot commented Feb 4, 2020

openshift-bot commented Feb 4, 2020

jeff-phillips-18 commented Feb 4, 2020

jeff-phillips-18 commented Feb 4, 2020

christianvogt commented Feb 5, 2020

openshift-ci-robot commented Feb 5, 2020

openshift-bot commented Feb 5, 2020

openshift-bot commented Feb 5, 2020

openshift-ci-robot commented Feb 5, 2020

	getSourceAnchorNode(): Node \| null;
	getSourceAnchorNode(): Node;

	edge.visible = edge.visible === undefined ? true : edge.visible;
	edge.visible = typeof edge.visible === 'undefined' ? true : edge.visible;

	edge.visible = edge.visible === undefined ? true : edge.visible;
	edge.visible = 'visible' in edge ? edge.visible : true;

	id: `${source}_${target}`,
	id: `aggregate:${source}_${target}`,

Bug 1796465: Fix for aggregate edge selection #4081

Bug 1796465: Fix for aggregate edge selection #4081

Conversation

jeff-phillips-18 commented Jan 27, 2020 • edited

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

Choose a reason for hiding this comment

andrewballantyne commented Jan 29, 2020

jeff-phillips-18 commented Jan 30, 2020

openshift-ci-robot commented Jan 30, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Feb 2, 2020

christianvogt commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-bot commented Feb 4, 2020

openshift-bot commented Feb 4, 2020

openshift-bot commented Feb 4, 2020

jeff-phillips-18 commented Feb 4, 2020

jeff-phillips-18 commented Feb 4, 2020

christianvogt commented Feb 5, 2020

openshift-ci-robot commented Feb 5, 2020

openshift-bot commented Feb 5, 2020

openshift-bot commented Feb 5, 2020

openshift-ci-robot commented Feb 5, 2020

jeff-phillips-18 commented Jan 27, 2020 •

edited