Bug 1873143: fix pipeline details page breadcrumbs in admin perspective #6212

vikram-raj · 2020-08-04T21:57:34Z

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

Analysis / Root cause:
Pipeline details pages on Admin to have breadcrumbs that take the user to default list page of Pipelines instead of to the tabbed list page from where the user came from

Solution Description:
Pipeline details pages on Admin to have breadcrumbs that take the user back to the tabbed list page from where the user came from.

Screen shots / Gifs for design review:

Unit test coverage report:

vikram-raj · 2020-08-04T21:58:01Z

/kind bug

vikram-raj · 2020-08-04T21:58:16Z

/assign @andrewballantyne

debsmita1 · 2020-08-10T12:19:50Z

you would need to fix the same for the Task details page as well

vikram-raj · 2020-08-10T14:51:15Z

you would need to fix the same for the Task details page as well

Thanks @debsmita1, I fix the same for tasks and triggers pages as well.

andrewballantyne

I'm seeing a lot of repeat code here... is there not a utility you can create? "DRY" is a principle we should strive to uphold whenever possible.

Recommend 4 hook methods

usePipelinesBreadcrumbsFor
useTasksBreadcrumbsFor
useTriggersBreadcrumbsFor
useTabbedTableBreadcrumbsFor

That way the generic logic of routing to a new location can be handled in one spot and then you can apply the various differences at a shallow wrapper and each Details page can avoid the repeat logic.

This may not work out 1:1, so please work around any issues with the above idea, with the goal of keeping it simple + avoiding repeating yourself.

vikram-raj · 2020-08-26T14:37:44Z

@andrewballantyne I added a hook to avoid code repeat. PTAL again.

vikram-raj · 2020-08-27T13:28:26Z

/retitle Bug 1873143: fix pipeline details page breadcrumbs in admin perspective

openshift-ci-robot · 2020-08-27T13:28:51Z

@vikram-raj: This pull request references Bugzilla bug 1873143, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective

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-08-27T13:29:30Z

/bugzilla refresh

openshift-ci-robot · 2020-08-27T13:29:37Z

@vikram-raj: This pull request references Bugzilla bug 1873143, 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.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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

@vikram-raj What was wrong with my suggested path? I feel it's a lot less weight for the caller to do the same thing, making it a simpler interface. Plus moving all namespace calls internal to the breadcrumbsFor method, you align on the one implementation and don't need to wiggle for another one with the use of the selector in the cluster resources cases.

i.e, something like this:

type Match = RMatch<{ url: string }>;

const useTabbedTableBreadcrumbsFor = (kindObj: K8sKind, match: Match, navOption: string) => {
  const currentNamespace = useSelector((state: RootState) => getActiveNamespace(state));
  const isAdminPerspective =
    useSelector((state: RootState) => getActivePerspective(state)) === 'admin';
  const namespace = ALL_NAMESPACES_KEY === currentNamespace ? 'all-namespaces' : currentNamespace;
  const subTab = pipelinesTab(kindObj);

  if (subTab == null) {
    return [];
  }

  return [
    {
      name: kindObj.labelPlural,
      path: isAdminPerspective
        ? `/${navOption}/ns/${namespace}/${subTab}`
        : getBreadcrumbPath(match),
    },
    { name: `${kindObj.label} Details`, path: match.url },
  ];
};

export const usePipelinesBreadcrumbsFor = (kindObj: K8sKind, match: Match) =>
  useTabbedTableBreadcrumbsFor(kindObj, match, 'pipelines');

export const useTasksBreadcrumbsFor = (kindObj: K8sKind, match: Match) =>
  useTabbedTableBreadcrumbsFor(kindObj, match, 'tasks');

export const useTriggersBreadcrumbsFor = (kindObj: K8sKind, match: Match) =>
  useTabbedTableBreadcrumbsFor(kindObj, match, 'triggers');

andrewballantyne · 2020-08-27T22:19:08Z

frontend/packages/dev-console/src/components/cluster-tasks/ClusterTaskDetailsPage.tsx

+
+const ClusterTaskDetailsPage: React.FC<DetailsPageProps> = (props) => {
+  const { kindObj, match, kind } = props;
+  const activeNamespace = useSelector((state: RootState) => getActiveNamespace(state));


I think you can avoid doing this out here if you just bake this functionality into the hook.

This is a bit of annoying usage due to the 3 imports + ts-ignore override.

andrewballantyne · 2020-08-27T22:23:06Z

frontend/packages/dev-console/src/components/pipelines/hooks.ts

+      name: kindObj.labelPlural,
+      path:
+        activePerspective === 'admin'
+          ? `/${navOption}/ns/${namespace}${


You have a bug here if you're in "all-namespaces" when you go into the details page - leaving via the breadcrumb causes a not found issue with #ALL_NS#.

andrewballantyne · 2020-08-27T22:23:34Z

frontend/packages/dev-console/src/models/pipelines.ts

@@ -105,7 +105,7 @@ export const ConditionModel: K8sKind = {
  id: 'condition',
  labelPlural: 'Conditions',
  crd: true,
-  badge: BadgeType.DEV,
+  badge: BadgeType.TECH,


Thanks for aligning this.

andrewballantyne · 2020-08-27T22:26:52Z

frontend/packages/dev-console/src/utils/pipeline-utils.ts

@@ -403,3 +414,26 @@ export const getSecretAnnotations = (annotation: KeyValuePair) => {
  }
  return annotations;
 };
+
+export const pipelinesTab = (kindObj: K8sKind) => {
+  switch (kindObj.kind) {


I think for this to be an accurate usage, we need to cover the situation where they pass valid models that don't have unique values (the "root" pages).

case PipelineModel.kind: case TaskModel.kind: case EventListenerModel.kind: return '';

That way there is a distinction between null (not a pipeline tab) and '' a "root" pipeline tab. This would be my expectation when using a function called pipelinesTab to get a tab extension for the pipelines pages.

vikram-raj · 2020-08-28T09:23:15Z

@andrewballantyne Nothing wrong with your suggestion. I was just trying to do it using one hook. Thanks.

andrewballantyne · 2020-08-28T12:59:37Z

@andrewballantyne Nothing wrong with your suggestion. I was just trying to do it using one hook. Thanks.

It's important to remember in the functional world, functions are not bad. If you can get a readable and easily understandable set of functions, which avoid magic exposed strings, you have less errors in usage and more understandable use-cases.

Wrapping functions should not be frowned upon in the functional world. This is one of the main wins over the OO world.

andrewballantyne · 2020-08-28T18:01:06Z

i.e, something like this:

Oops, I created a bug with my implementation. Was testing around and I noticed all-namespaces is not ns/all-namespaces. Please correct that @vikram-raj

There is another bug here but I think it's unrelated, and that's viewing this tab view in the Dev perspective, it breaks the "single project" methodology... as it has no "dev perspective" blocks to show projects. Not sure what the best solution is for that right now. I will raise it in the tmp-odc-pipelines channel.

vikram-raj · 2020-08-31T08:57:25Z

Oops, I created a bug with my implementation. Was testing around and I noticed all-namespaces is not ns/all-namespaces. Please correct that @vikram-raj

Fixed this issue.

andrewballantyne · 2020-08-31T15:01:11Z

cc @divyanshiGupta FYI on this solution as we had discussed this several weeks back.

Vikram needs another reviewer, if @debsmita1 wants to finish her review or if you want to do it @divyanshiGupta

divyanshiGupta

Code looks good to me, also verified it locally as well and it works fine for me.
/lgtm

openshift-ci-robot · 2020-08-31T16:31:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, divyanshiGupta, 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-08-31T21:18:54Z

@vikram-raj: All pull requests linked via external trackers have merged:

openshift/console#6212

Bugzilla bug 1873143 has been moved to the MODIFIED state.

In response to this:

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective

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 the component/dev-console Related to dev-console label Aug 4, 2020

openshift-ci-robot requested review from abhinandan13jan and andrewballantyne August 4, 2020 21:57

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

openshift-ci-robot assigned andrewballantyne Aug 4, 2020

vikram-raj force-pushed the ODC-4220 branch from 6e07d11 to 555a452 Compare August 10, 2020 14:48

andrewballantyne suggested changes Aug 17, 2020

View reviewed changes

vikram-raj force-pushed the ODC-4220 branch from 555a452 to 3aae9d0 Compare August 26, 2020 14:34

vikram-raj requested a review from andrewballantyne August 26, 2020 14:35

vikram-raj force-pushed the ODC-4220 branch from 3aae9d0 to 45af4aa Compare August 26, 2020 14:36

openshift-ci-robot changed the title ~~fix pipeline details page breadcrumbs in admin perspective~~ Bug 1873143: fix pipeline details page breadcrumbs in admin perspective Aug 27, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Aug 27, 2020

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

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 27, 2020

andrewballantyne suggested changes Aug 27, 2020

View reviewed changes

vikram-raj force-pushed the ODC-4220 branch from 45af4aa to a941193 Compare August 28, 2020 09:19

vikram-raj requested a review from andrewballantyne August 28, 2020 09:23

fix pipeline details page breadcrumbs in admin perspective

082a6de

vikram-raj force-pushed the ODC-4220 branch from a941193 to 082a6de Compare August 31, 2020 08:56

andrewballantyne approved these changes Aug 31, 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 Aug 31, 2020

divyanshiGupta approved these changes Aug 31, 2020

View reviewed changes

openshift-ci-robot assigned divyanshiGupta Aug 31, 2020

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

openshift-merge-robot merged commit d98477c into openshift:master Aug 31, 2020

spadgett added this to the v4.6 milestone Sep 1, 2020

vikram-raj deleted the ODC-4220 branch September 2, 2020 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective #6212

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective #6212

vikram-raj commented Aug 4, 2020

vikram-raj commented Aug 4, 2020

vikram-raj commented Aug 4, 2020

debsmita1 commented Aug 10, 2020

vikram-raj commented Aug 10, 2020

andrewballantyne left a comment •

edited

vikram-raj commented Aug 26, 2020

vikram-raj commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

vikram-raj commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

andrewballantyne left a comment

andrewballantyne Aug 27, 2020

andrewballantyne Aug 27, 2020

andrewballantyne Aug 27, 2020

andrewballantyne Aug 27, 2020

vikram-raj commented Aug 28, 2020

andrewballantyne commented Aug 28, 2020

andrewballantyne commented Aug 28, 2020

vikram-raj commented Aug 31, 2020

andrewballantyne commented Aug 31, 2020

divyanshiGupta left a comment

openshift-ci-robot commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective #6212

Bug 1873143: fix pipeline details page breadcrumbs in admin perspective #6212

Conversation

vikram-raj commented Aug 4, 2020

vikram-raj commented Aug 4, 2020

vikram-raj commented Aug 4, 2020

debsmita1 commented Aug 10, 2020

vikram-raj commented Aug 10, 2020

andrewballantyne left a comment • edited

Choose a reason for hiding this comment

vikram-raj commented Aug 26, 2020

vikram-raj commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

vikram-raj commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Aug 27, 2020

Choose a reason for hiding this comment

andrewballantyne Aug 27, 2020

Choose a reason for hiding this comment

andrewballantyne Aug 27, 2020

Choose a reason for hiding this comment

andrewballantyne Aug 27, 2020

Choose a reason for hiding this comment

vikram-raj commented Aug 28, 2020

andrewballantyne commented Aug 28, 2020

andrewballantyne commented Aug 28, 2020

vikram-raj commented Aug 31, 2020

andrewballantyne commented Aug 31, 2020

divyanshiGupta left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

andrewballantyne left a comment •

edited