Bug 1835425: Don't show Remove Trigger when there are no triggers #5433

vikram-raj · 2020-05-13T18:20:05Z

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

Analysis / Root cause:
When there are no triggers, the modal opens with a disabled dropdown.

We should do better, like disabling the kebab menu option... unfortunately the information is not readably available... so we'll have to fetch it which may not be cheap. Some thinking needs to be done here.

Solution Description:
Remove the Remove Trigger option from the kebab menu of the pipeline when there are no triggers.

Remove the Remove Trigger option from the actions menu of the pipeline details page when there are no triggers.

Screen shots / Gifs for design review:

Browser conformance:

Chrome
Firefox
Safari
Edge

vikram-raj · 2020-05-13T18:25:19Z

/retitle Bug 1835425: Don't show Remove Trigger when there are no triggers

openshift-ci-robot · 2020-05-13T18:25:38Z

@vikram-raj: This pull request references Bugzilla bug 1835425, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1835425: Don't show Remove Trigger when there are no triggers

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.

karthikjeeyar · 2020-05-15T10:23:39Z

...s/dev-console/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineDetails.tsx

+  const {
+    metadata: { name, namespace },
+  } = pipeline;
+  const routeTemplates: RouteTemplate[] = usePipelineTriggerTemplateNames(name, namespace) || [];


we are already calling usePipelineTriggerTemplateNames hook in the parent component PipelineDetailsPage.tsx, instead of calling it again here, we can pass the routeTemplates data to the child component via customData property in the DetailsPage component in the pipelineDetailsPage.tsx.

Suggested change

const {

metadata: { name, namespace },

} = pipeline;

const routeTemplates: RouteTemplate[] = usePipelineTriggerTemplateNames(name, namespace) || [];

@karthikjeeyar Done.

vikram-raj · 2020-05-15T11:47:36Z

/assign @andrewballantyne

karthikjeeyar · 2020-05-15T13:04:01Z

frontend/packages/dev-console/src/utils/pipeline-actions.tsx

@@ -228,11 +228,14 @@ const removeTrigger: KebabAction = (kind: K8sKind, pipeline: Pipeline) => ({
    verb: 'delete',
  },
 });
-export const getPipelineKebabActions = (pipelineRun?: PipelineRun): KebabAction[] => [
+export const getPipelineKebabActions = (


Verified the functionality, it works as expected. can we add tests for getPipelineKebabActions utils?

Added tests for this utils.

andrewballantyne · 2020-05-21T09:38:19Z

frontend/packages/dev-console/src/components/pipelines/PipelineDetailsPage.tsx

@@ -42,7 +44,9 @@ const PipelineDetailsPage: React.FC<DetailsPageProps> = (props) => {
      .catch((error) => setErrorCode(error.response.status));
  }, [name, namespace]);

-  const augmentedMenuActions: KebabAction[] = useMenuActionsWithUserLabel(menuActions);
+  const augmentedMenuActions: KebabAction[] = useMenuActionsWithUserLabel(
+    getPipelineKebabActions(latestPipelineRun, !_.isEmpty(templateNames)),


Nit: No need to use lodash.

Suggested change

getPipelineKebabActions(latestPipelineRun, !_.isEmpty(templateNames)),

getPipelineKebabActions(latestPipelineRun, templateNames.length > 0),

Recommend doing this for both cases you did this.

andrewballantyne · 2020-05-21T09:54:25Z

frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts

-export const usePipelineTriggerTemplateNames = (pipeline: Pipeline): RouteTemplate[] | null => {
-  const eventListenerResources = useAllEventListeners(pipeline);
-  const {
-    metadata: { namespace },
-  } = pipeline;
+export const usePipelineTriggerTemplateNames = (
+  pipelineName: string,
+  namespace: string,
+): RouteTemplate[] | null => {
+  const eventListenerResources = useAllEventListeners(namespace);


Why did you make this change? As far as I can tell you possessed the Pipeline object in all cases where you used this function. These changes are undesirable and lead to coupling of functionality.

Prior to this change, the caller just knew they had a Pipeline and needed to get related objects of type X. usePipelineTriggerTemplates makes a lot less sense when you're passing a name and a namespace, rather than the whole Pipeline.

I disagree with this change, and unless you have a solid reason I can't see, I recommend undoing this.

I make this change so that I can use this usePipelineTriggerTemplateNames hook in PipelineDetailsPage as the whole pipeline object is not present on pipeline details age.

Really? 🤔 That seems odd.

Yes, this is the only reason to make this change.

andrewballantyne · 2020-05-21T09:56:17Z

frontend/packages/dev-console/src/components/pipelines/list-page/PipelineRow.tsx

+const PipelineRow: RowFunction<Pipeline> = ({ obj, index, style }) => {
+  return <PipelineTableRow obj={obj} index={index} style={style} />;
+};
+


This is my biggest concern of this PR. This is not the pattern we are used to for Table rows. Perhaps we need Christian to comment on the architecture of this decision. I'm going to investigate an alternative for the table rows.

I wonder if we just override my solution for the hook injection of the user label. I wrapped the Kebab component instead of the whole row. Most of the content is unaffected by this state and thus doesn't need this change.

Recommend you try that, maybe another wrapper 🤔 Maybe a prop 🤔 Consider this path instead of making the whole row a component. Let me know if you want to discuss options.

Ah - crud. This may be actually more difficult. This is done all up front AND on scroll (if there are enough to be virtually rendered). This solution may need to focus on a hover of the Kebab. Perhaps we can use a useEffect and a useRef hook on a wrapping div 🤔

As PipelineRow is not a FC so, I am not able to use hooks. That is why I make this change.

I understand that @vikram-raj This pattern of creating a component off a function to use hooks... especially for the row component and always on the row component is a performance issue. We need to do better otherwise we'll just be trading bugs.

@andrewballantyne Updated this. Now, I wrap only the pipeline kebab component instead of wrapping the whole row component.

andrewballantyne · 2020-05-22T18:17:44Z

frontend/packages/dev-console/src/components/pipelines/list-page/PipelineRow.tsx

+  pipeline: Pipeline;
+};
+
+const PipelineRowKebabActions: React.FC<PipelineRowKebabActionsProps> = ({ pipeline }) => {


This needs to be in a new file. Also as discussed on slack, we need to lazy do this hook fetch as we cannot be fetching always for every row... it's too much.

@vikram-raj We discussed this on slack. For posterity, I'll look into this more this weekend. There is something off about this and I will figure out what it is or retract my request 😛

Looks like there is caching in play here that prevents too much duplicate action. We still need to convert this component into it's own file, but let's run with this design for now. It doesn't seem to have too much problems. At least none more than the current Pipeline (which fetches all Pipeline Runs and gets the latest).

Moved the PipelineRowKebabActions to separate files.

andrewballantyne · 2020-05-27T01:21:40Z

/approve

Make the needed new component and look for a peer review to get this underway tomorrow during India day @vikram-raj

If needed, I'll be available reasonably early for additional questions / feedback.

karthikjeeyar · 2020-05-27T08:42:25Z

verfied locally, works as expected.
/lgtm

openshift-ci-robot · 2020-05-27T08:42:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, karthikjeeyar, vikram-raj

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~~ [andrewballantyne]

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

openshift-ci-robot · 2020-05-27T12:57:51Z

@vikram-raj: All pull requests linked via external trackers have merged: openshift/console#5433. Bugzilla bug 1835425 has been moved to the MODIFIED state.

In response to this:

Bug 1835425: Don't show Remove Trigger when there are no triggers

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 requested review from debsmita1 and divyanshiGupta May 13, 2020 18:20

openshift-ci-robot changed the title ~~Don't show Remove Trigger when there are no triggers~~ Bug 1835425: Don't show Remove Trigger when there are no triggers May 13, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 13, 2020

vikram-raj force-pushed the odc-3589 branch 2 times, most recently from 1ec11aa to 049d53d Compare May 14, 2020 05:16

karthikjeeyar reviewed May 15, 2020

View reviewed changes

vikram-raj force-pushed the odc-3589 branch from 049d53d to cf10bd8 Compare May 15, 2020 11:46

openshift-ci-robot assigned andrewballantyne May 15, 2020

karthikjeeyar reviewed May 15, 2020

View reviewed changes

vikram-raj force-pushed the odc-3589 branch from cf10bd8 to 2e4141b Compare May 15, 2020 14:39

vikram-raj requested a review from karthikjeeyar May 15, 2020 14:41

andrewballantyne suggested changes May 21, 2020

View reviewed changes

vikram-raj force-pushed the odc-3589 branch 3 times, most recently from 674fd6c to 21c9fab Compare May 22, 2020 18:01

andrewballantyne suggested changes May 22, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020

Don't show Remove Trigger when there are no triggers

b145c46

vikram-raj force-pushed the odc-3589 branch from 21c9fab to b145c46 Compare May 27, 2020 04:17

openshift-ci-robot assigned karthikjeeyar May 27, 2020

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

openshift-merge-robot merged commit 24c9cce into openshift:master May 27, 2020

vikram-raj deleted the odc-3589 branch May 27, 2020 12:58

spadgett added this to the v4.5 milestone Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1835425: Don't show Remove Trigger when there are no triggers #5433

Bug 1835425: Don't show Remove Trigger when there are no triggers #5433

vikram-raj commented May 13, 2020

vikram-raj commented May 13, 2020

openshift-ci-robot commented May 13, 2020

karthikjeeyar May 15, 2020

vikram-raj May 15, 2020

vikram-raj commented May 15, 2020

karthikjeeyar May 15, 2020 •

edited

vikram-raj May 15, 2020

andrewballantyne May 21, 2020 •

edited

andrewballantyne May 21, 2020

vikram-raj May 21, 2020

andrewballantyne May 21, 2020

vikram-raj May 21, 2020

andrewballantyne May 21, 2020

andrewballantyne May 21, 2020

andrewballantyne May 21, 2020

vikram-raj May 21, 2020

andrewballantyne May 21, 2020

vikram-raj May 22, 2020

andrewballantyne May 22, 2020 •

edited

andrewballantyne May 22, 2020

andrewballantyne May 27, 2020

vikram-raj May 27, 2020

andrewballantyne commented May 27, 2020

karthikjeeyar commented May 27, 2020

openshift-ci-robot commented May 27, 2020

openshift-ci-robot commented May 27, 2020

	getPipelineKebabActions(latestPipelineRun, !_.isEmpty(templateNames)),
	getPipelineKebabActions(latestPipelineRun, templateNames.length > 0),

Bug 1835425: Don't show Remove Trigger when there are no triggers #5433

Bug 1835425: Don't show Remove Trigger when there are no triggers #5433

Conversation

vikram-raj commented May 13, 2020

vikram-raj commented May 13, 2020

openshift-ci-robot commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikram-raj commented May 15, 2020

karthikjeeyar May 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne May 21, 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

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 May 22, 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

andrewballantyne commented May 27, 2020

karthikjeeyar commented May 27, 2020

openshift-ci-robot commented May 27, 2020

openshift-ci-robot commented May 27, 2020

karthikjeeyar May 15, 2020 •

edited

andrewballantyne May 21, 2020 •

edited

andrewballantyne May 22, 2020 •

edited