Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present #5258

vikram-raj · 2020-05-01T02:11:59Z

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

Analysis / Root cause:
When viewing the Pipeline Runs in the overview sidebar of topology, the consoledeveloper doesn't see the button when there are no pipeline runs, but does as soon as there are.

Solution Description:
Hide 'Start Last Run' button on topology overview page when no PLR present. For admin as well as for developer.

Screen shots / Gifs for design review:

Browser conformance:

Chrome
Firefox
Safari
Edge

vikram-raj · 2020-05-01T02:20:19Z

/retitle Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present

openshift-ci-robot · 2020-05-01T02:20:39Z

@vikram-raj: This pull request references Bugzilla bug 1830181, 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 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present

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.

andrewballantyne

A change of the current UX needs to be run by @openshift/team-devconsole-ux @vikram-raj Please make sure you tag them when doing so.

With that said, I'm personally not in the favour of hiding the button. My stance on when to hide and when to disable a button is around "can the user find use in seeing a disabled button". I think in this case there isn't a lot, but there is some.

The user can head over to the Pipelines page (clicking on the Pipeline name/link provided) to start the Pipeline for the first time. This allows them to fix the disabled button. With no button, it breaks the current view expectations on "some are startable - they have buttons; some are not startable - they do not have buttons". Take Pods for instance, how do we know there is not supposed to be a button there and the user just needs to do one thing before making that happen?

To close off my rant 😄 I think our solution here is actually to show a disabled button if the current user permissions allow them to access that resource. So your 'consoledeveloper' (and 'kube:admin') user would see a disabled button even if there was not a Pipeline Run yet.

@openshift/team-devconsole-ux Please feel free to override what I just said if you'd prefer the button always hidden when it's disabled.

andrewballantyne · 2020-05-01T17:05:38Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

-    isAllowed && (
-      <Button variant="secondary" onClick={callback} isDisabled={pipelineRuns.length === 0}>
+    isAllowed &&
+    pipelineRuns.length && (


Please avoid doing array.length && logic @vikram-raj - it is currently rendering 0 when it's false.

andrewballantyne · 2020-05-01T17:32:22Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

@@ -23,8 +23,9 @@ const TriggerLastRunButton: React.FC<TriggerLastRunButtonProps> = ({
  const { label, callback, accessReview } = rerunPipelineAndStay(PipelineModel, latestRun);


Could you also change this to PipelineRunModel I missed it in my original review of this code. We are not passing a Pipeline, but a Pipeline Run.

vikram-raj · 2020-05-04T15:11:52Z

@andrewballantyne Currently, we are hiding all actions on the overview panel if the user has no access to edit.

andrewballantyne · 2020-05-04T16:10:09Z

@andrewballantyne Currently, we are hiding all actions on the overview panel if the user has no access to edit.

If the user has no access to do any action no matter the state of things, then that's by design. They lack the permissions to do the action, and no matter the state of the app, their permissions do not grant them access to buttons / actions.

If you start hiding buttons that are not disabled because of permission reasons, you are giving the wrong impression imo. The user should see the disabled button if they are an Admin or a user with 'edit' access. Because the state of the app can change and present the button for use to that user.

Naturally all the above is conditional on their permissions not changing.

Please make sure UX provides feedback on this @vikram-raj

beaumorley · 2020-05-04T18:57:12Z

@vikram-raj Thanks for looping me in. I agree with @andrewballantyne that we should disable the button for the developer view instead of removing for the admin/developer view. If this was never an option (permission related) I think your solution would be correct. The disabled button gives them a cue that there is something they can do. I would ask @serenamarie125 to weigh in as well.

openshift-ci-robot · 2020-05-04T20:28:05Z

@vikram-raj: This pull request references Bugzilla bug 1830181, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present

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.

vikram-raj · 2020-05-04T20:29:12Z

@andrewballantyne @beaumorley I updated the PR and screenshots PTAL.

andrewballantyne · 2020-05-04T20:29:53Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

+    <Button
+      variant="secondary"
+      onClick={callback}
+      isDisabled={!isAllowed || pipelineRuns.length === 0}


This will show the button for View-only users. This breaks the first criteria of the sidebar.

If the user has no access to do any action no matter the state of things, then that's by design. They lack the permissions to do the action, and no matter the state of the app, their permissions do not grant them access to buttons / actions.

andrewballantyne

This solution aligns better with the desired UX. Just a couple minor changes left I think.

andrewballantyne · 2020-05-05T15:27:24Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

  impersonate?;
 };

 const TriggerLastRunButton: React.FC<TriggerLastRunButtonProps> = ({
  pipelineRuns,
+  namespace,
  impersonate,
 }) => {
  const latestRun = usePipelineRunWithUserLabel(


While debugging the solution... I noticed a bug I accidentally made, can correct at the same time here?

In pipelineruns/triggered-by/hooks.ts:

// Today return mergeLabelsWithResource(labels, plr); // Suggested Change return plr && mergeLabelsWithResource(labels, plr);

That way it doesn't merge a label onto nothing creating an invalid k8s object.

andrewballantyne · 2020-05-05T15:29:14Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

-  const { label, callback, accessReview } = rerunPipelineAndStay(PipelineModel, latestRun);
+  const { label, callback } = rerunPipelineAndStay(PipelineRunModel, latestRun);
+  const accessReview: AccessReviewResourceAttributes = {
+    group: PipelineRunModel.apiGroup,
+    resource: PipelineRunModel.plural,
+    namespace,
+    verb: 'create',
+  };


Not sure we want to toss out the accessReview that the utility provides... perhaps we do this:

const { label, callback, accessReview: utilityAccessReview } = rerunPipelineAndStay(PipelineRunModel, latestRun); const defaultAccessReview: AccessReviewResourceAttributes = { group: PipelineRunModel.apiGroup, resource: PipelineRunModel.plural, namespace, verb: 'create', }; const accessReview = _.isEmpty(utilityAccessReview) ? defaultAccessReview : utilityAccessReview;

andrewballantyne · 2020-05-05T15:30:53Z

...end/packages/dev-console/src/components/pipelines/pipeline-overview/TriggerLastRunButton.tsx

+    resource: PipelineRunModel.plural,
+    namespace,
+    verb: 'create',
+  };
  const isAllowed = useAccessReview(accessReview, impersonate);
  return (
    isAllowed && (


Could you also change the button logic to be more deterministic?

<Button variant="secondary" onClick={callback} isDisabled={pipelineRuns.length === 0 && !callback}>

Making sure we have a callback to use before enabling the button will help if there is ever an issue with the callback, we don't have a button enabled without a callback.

EDIT: Updated the logic, thanks for catching the mistake

…esent

andrewballantyne

/lgtm

"consoledeveloper" does not see any button when only having View access
"consoledeveloper" see a disabled button when having Edit access and no Pipeline Runs
"consoledeveloper" see an enabled button when having Edit access and having at least 1 Pipeline Run
Admin is unaffected

andrewballantyne · 2020-05-05T18:42:31Z

/retest

openshift-ci-robot · 2020-05-05T18:42:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, 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-bot · 2020-05-05T18:43:49Z

/retest

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

openshift-ci-robot · 2020-05-05T19:39:36Z

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

In response to this:

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present

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 rohitkrai03 May 1, 2020 02:12

openshift-ci-robot added the component/dev-console Related to dev-console label May 1, 2020

openshift-ci-robot changed the title ~~Hide 'Start Last Run' button on topology overview page when no PLR present~~ Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present May 1, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label May 1, 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 May 1, 2020

andrewballantyne suggested changes May 1, 2020

View reviewed changes

andrewballantyne reviewed May 1, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne May 1, 2020

vikram-raj force-pushed the odc-2403 branch from b58c0c1 to 9e7cb75 Compare May 4, 2020 06:13

vikram-raj force-pushed the odc-2403 branch from 9e7cb75 to c8d55f9 Compare May 4, 2020 20:24

andrewballantyne reviewed May 4, 2020

View reviewed changes

vikram-raj force-pushed the odc-2403 branch from c8d55f9 to 71dcbd5 Compare May 5, 2020 13:36

andrewballantyne suggested changes May 5, 2020

View reviewed changes

Hide 'Start Last Run' button on topology overview page when no PLR pr…

40e1ae0

…esent

vikram-raj force-pushed the odc-2403 branch from 71dcbd5 to 40e1ae0 Compare May 5, 2020 18:06

andrewballantyne approved these changes May 5, 2020

View reviewed changes

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

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

openshift-merge-robot merged commit 325beb0 into openshift:master May 5, 2020

vikram-raj deleted the odc-2403 branch May 5, 2020 19:41

spadgett added this to the v4.5 milestone May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present #5258

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present #5258

vikram-raj commented May 1, 2020 •

edited

vikram-raj commented May 1, 2020

openshift-ci-robot commented May 1, 2020

andrewballantyne left a comment

andrewballantyne May 1, 2020 •

edited

andrewballantyne May 1, 2020

vikram-raj commented May 4, 2020

andrewballantyne commented May 4, 2020

beaumorley commented May 4, 2020

openshift-ci-robot commented May 4, 2020

vikram-raj commented May 4, 2020

andrewballantyne May 4, 2020 •

edited

andrewballantyne left a comment

andrewballantyne May 5, 2020

andrewballantyne May 5, 2020

andrewballantyne May 5, 2020 •

edited

andrewballantyne left a comment

andrewballantyne commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

		@@ -23,8 +23,9 @@ const TriggerLastRunButton: React.FC<TriggerLastRunButtonProps> = ({
		const { label, callback, accessReview } = rerunPipelineAndStay(PipelineModel, latestRun);

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present #5258

Bug 1830181: Hide 'Start Last Run' button on topology overview page when no PLR present #5258

Conversation

vikram-raj commented May 1, 2020 • edited

vikram-raj commented May 1, 2020

openshift-ci-robot commented May 1, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne May 1, 2020 • edited

Choose a reason for hiding this comment

andrewballantyne May 1, 2020

Choose a reason for hiding this comment

vikram-raj commented May 4, 2020

andrewballantyne commented May 4, 2020

beaumorley commented May 4, 2020

openshift-ci-robot commented May 4, 2020

vikram-raj commented May 4, 2020

andrewballantyne May 4, 2020 • edited

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne May 5, 2020

Choose a reason for hiding this comment

andrewballantyne May 5, 2020

Choose a reason for hiding this comment

andrewballantyne May 5, 2020 • edited

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

vikram-raj commented May 1, 2020 •

edited

andrewballantyne May 1, 2020 •

edited

andrewballantyne May 4, 2020 •

edited

andrewballantyne May 5, 2020 •

edited