move object resources to new nav item #895

SanjalKatiyar · 2023-06-09T14:37:38Z

openshift-ci · 2023-06-09T14:37:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SanjalKatiyar

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:

~~OWNERS~~ [SanjalKatiyar]

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

SanjalKatiyar · 2023-06-09T14:42:02Z

packages/odf/components/mcg/CreateObjectBucketClaim.tsx

 } from '@openshift-console/dynamic-plugin-sdk';
+import { useActiveNamespace } from '@openshift-console/dynamic-plugin-sdk-internal';


@bipuladh ended up using this, which is the cleanest way to do things...
we have alternatives like passing NS via URL or using Context, but that will need handling multiple edge conditions (to prevent small small bugs) and IMHO is not the best way. Adding one more component for tech-debt for this is good to have in core sdk.

SanjalKatiyar · 2023-06-12T11:47:21Z

cypress/support/pages/app.ts

+    cy.byTestID('dropdown-text-filter')
+      .clear()
+      .then(() => cy.byTestID('dropdown-text-filter').should('be.empty'))
+      .then(() => cy.byTestID('dropdown-text-filter').type(projectName))
+      .then(() =>
+        cy.byTestID('dropdown-text-filter').should('have.value', projectName)
+      );


OBC tests are flaky on Project creation step... this change should fix that !

SanjalKatiyar · 2023-06-12T14:41:29Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-06-12T17:51:18Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-06-13T04:53:20Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-06-15T05:08:18Z

/test odf-console-e2e-aws

bipuladh · 2023-06-16T11:52:34Z

packages/odf/components/mcg/CreateObjectBucketClaim.tsx

-            isInline
-            title={t('Address form errors to proceed')}
+    <>
+      <NamespaceBar />


Instead of using useActiveNamespace why not pass a onNamespaceChange prop to this component?

there are 2 reasons for that:

We are already using "useActiveNamespace" at other places, so I used here as well.

Namespace is already stored and passed down by console via context, no need to use local "useState" variable to store that info again.

bipuladh · 2023-06-16T11:56:53Z

packages/odf/components/mcg/CreateObjectBucketClaim.tsx

+            const resourcePath = `${referenceForModel(
+              NooBaaObjectBucketClaimModel
+            )}/${resource.metadata.name}`;


Could we keep on using resourcePathFromModel we could perhaps append /odf/resources in front? Unless there is a specific reason to avoid that.

I don;t have anything for or against it, just "resourcePathFromModel" will add "k8s" to the path as well which made sense earlier because earlier list page and creation page were using generic Routes which had "k8s" in its path as well.

Now, when OB/OBC are separate tabs, list/creation page don;t have "k8s" in its path and to keep consistent I did same for the details page as well !!

bipuladh · 2023-06-16T12:04:35Z

packages/odf/components/object-service-nav-item/object-service.tsx

+  if (!haveAlreadyResolvedOnce.current && haveExtensionsResolved) {
+    haveAlreadyResolvedOnce.current = true;
+  }
+  useSortPages({ extensions, haveExtensionsResolved, pages, setPages });


Suggested change

useSortPages({ extensions, haveExtensionsResolved, pages, setPages });

const pages = useSortPages({ extensions, haveExtensionsResolved});

The root state of all these pages are the extensions which is stored in a state somewhere. If we are to use a hook then we can potentially avoid keeping an intermeddiate state to store pages. WDYT?

make sense !

bipuladh · 2023-06-20T03:48:05Z

cypress/views/obcPage.ts

@@ -3,6 +3,13 @@ import { app } from '../support/pages/app';
 import { MINUTE } from '../utils/consts';
 import { listPage } from './list-page';

+export const obcNavigate = {
+  navigateToOBC: () => {
+    cy.clickNavLink(['Storage', 'Object Storage']);


Suggested change

cy.clickNavLink(['Storage', 'Object Storage']);

commonFlows.navigateToObjectStore();

bipuladh · 2023-06-20T03:48:20Z

cypress/views/obcPage.ts

+export const obcNavigate = {
+  navigateToOBC: () => {
+    cy.clickNavLink(['Storage', 'Object Storage']);
+    cy.byLegacyTestID('horizontal-link-Object Bucket Claims').first().click();


Why do we need first?

I dont think it is needed, at many places we are using cy.byLegacyTestID('horizontal-link-Storage Systems').first().click();, I just copied this line and made it OBC instead of SS.

let's remove first if not necessary.

bipuladh · 2023-06-20T03:49:48Z

plugins/odf/console-extensions.json

-      "exact": false,
-      "path": "/k8s/ns/:ns/objectbucket.io~v1alpha1~ObjectBucketClaim/~new/form",
+      "exact": true,
+      "path": "/odf/resource/objectbucket.io~v1alpha1~ObjectBucketClaim/create/~new",


There is inconsistency in the URL patterns.
/odf/resource/ns/:namespace/objectbucket.io~v1alpha1~ObjectBucketClaim/:resourceName
Some URLS support NS and some don't.

actually this was intensional on my end, we need to decide what is the best way to go ??

here all tabs are horizaontalNavs and expect OBC others are either clusterwide resource or openshift-storage specific (hence no ns in URL).
now for OBC tab, I did not add ns to the url because:

so that it matches with other horizontal tabs.

currently I have added OBC tab via extension so that console can handle all the filtering based upon "RGW" "OBC" etc flags. If I need to pass ns to the URL of OBC list page then I need to add OBC tab as a static page (similar to how we add "Overview" page to ODF dashboard) and will have to again do all the filtering based on the required/disallow flags.

now since list page did not had URL, I kept it same for create page as well.

Only details page is something which has ns in URL and it is kind of separate from the list/create flow as well.

if we are moving everything under one root then lets keep consistency in the URLs. I am fine with creating another story for looking into URLs but this is an important detail.

ack... will create a story/task...

bipuladh · 2023-06-20T03:55:09Z

packages/odf/utils/horizontal-nav-tab.tsx

+  }, [extensions, haveExtensionsResolved, staticPages]);
+};
+
+export const convertHorizontalNavTabToNav = (


Suggested change

export const convertHorizontalNavTabToNav = (

export const convertHorizontalNavTabToNavPage = (

bipuladh · 2023-06-20T03:55:55Z

packages/odf/utils/horizontal-nav-tab.tsx

+  ResolvedCodeRefProperties<HorizontalNavTabExtensionProps>;
+
+const sortPages = (
+  horizontalNavTabs: ResolvedCodeRefProperties<HorizontalNavTabExtensionProps>[]


Suggested change

horizontalNavTabs: ResolvedCodeRefProperties<HorizontalNavTabExtensionProps>[]

horizontalNavTabs: HorizontalNavProps[]

bipuladh · 2023-06-20T03:56:04Z

packages/odf/utils/horizontal-nav-tab.tsx

+import {
+  HorizontalNavTabExtensionProps,
+  HorizontalNavTab,
+} from 'packages/odf-plugin-sdk/extensions';


This looks wrong to me.

u mean import packages/odf-plugin-sdk/extensions do not have @ ?? or something else...

bipuladh · 2023-06-20T03:56:17Z

packages/odf/constants/common.ts

+export const ALL_NAMESPACES = 'all-namespaces';
+export const DEFAULT_NS = 'default';


This is better suited for shared.

bipuladh · 2023-06-20T03:59:22Z

plugins/odf/console-extensions.json

+  {
+    "type": "console.page/route",
+    "properties": {
+      "path": ["/odf/object-storage"],


It's kind of confusing these URL patterns. This is the root path but for OBC details the route is /odf/resource/ns/:namespace/objectbucket.io~v1alpha1~ObjectBucketClaim/:resourceName

isn;t it same as what is happening right now ??
currently say for BackingStore list page we have /odf/cluster as root, but details page have /odf/resource/noobaa.io~v1alpha1~BackingStore/noobaa-default-backing-store

only change I did is to make /odf/cluster to /odf/object-storage, rest everything is same as current...

bipuladh · 2023-06-20T04:01:18Z

packages/odf/components/odf-dashboard/dashboard.tsx

+  const isDashboardTab = React.useMemo(
+    () =>
+      compose(isHorizontalNavTab, (e: Extension) => {
+        if (e.properties.contextId === ODF_DASHBOARD_CONTEXT) return e;
+      }),
+    []
+  );


Suggested change

const isDashboardTab = React.useMemo(

() =>

compose(isHorizontalNavTab, (e: Extension) => {

if (e.properties.contextId === ODF_DASHBOARD_CONTEXT) return e;

}),

[]

);

const isDashboardTab = (e) =>

isHorizontalNavTab(e) && (e.properties.contextId === ODF_DASHBOARD_CONTEXT)

);

This can prolly live outside the component safely.

make sense... not sure why I used a hook !

bipuladh · 2023-06-20T04:04:02Z

packages/odf/components/object-service-nav-item/object-service.tsx

+  const isObjectServiceTab = React.useMemo(
+    () =>
+      compose(isHorizontalNavTab, (e: Extension) => {
+        if (e.properties.contextId === OBJECT_SERVICE_CONTEXT) return e;
+      }),
+    []
+  );


useMemo should not return a function there is already useCallback for that.

SanjalKatiyar · 2023-06-21T06:42:26Z

/test odf-console-e2e-aws

bipuladh · 2023-06-21T07:55:53Z

/lgtm

SanjalKatiyar · 2023-06-21T09:15:55Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-06-21T10:35:35Z

/test odf-console-e2e-aws

SanjalKatiyar · 2023-06-21T14:13:09Z

/test odf-console-e2e-aws

openshift-ci bot added the approved label Jun 9, 2023

SanjalKatiyar changed the title ~~move object resources to new nav item~~ [WIP] move object resources to new nav item Jun 9, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Jun 9, 2023

SanjalKatiyar commented Jun 9, 2023

View reviewed changes

SanjalKatiyar force-pushed the object_nav branch from 32a1f3b to 9e1d7e9 Compare June 12, 2023 11:45

SanjalKatiyar changed the title ~~[WIP] move object resources to new nav item~~ move object resources to new nav item Jun 12, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 12, 2023

SanjalKatiyar commented Jun 12, 2023

View reviewed changes

bipuladh reviewed Jun 16, 2023

View reviewed changes

bipuladh self-requested a review June 16, 2023 12:08

SanjalKatiyar force-pushed the object_nav branch 2 times, most recently from 52629d4 to d8a5967 Compare June 16, 2023 13:03

bipuladh reviewed Jun 20, 2023

View reviewed changes

SanjalKatiyar force-pushed the object_nav branch 2 times, most recently from 7fb21fd to f620306 Compare June 20, 2023 07:27

SanjalKatiyar requested a review from bipuladh June 21, 2023 06:41

move object resources to new nav item

87849dd

SanjalKatiyar force-pushed the object_nav branch from f620306 to 87849dd Compare June 21, 2023 07:29

openshift-ci bot assigned bipuladh Jun 21, 2023

openshift-ci bot added the lgtm label Jun 21, 2023

openshift-merge-robot merged commit f6c11c1 into red-hat-storage:master Jun 21, 2023

		} from '@openshift-console/dynamic-plugin-sdk';
		import { useActiveNamespace } from '@openshift-console/dynamic-plugin-sdk-internal';

	useSortPages({ extensions, haveExtensionsResolved, pages, setPages });
	const pages = useSortPages({ extensions, haveExtensionsResolved});

	cy.clickNavLink(['Storage', 'Object Storage']);
	commonFlows.navigateToObjectStore();

	export const convertHorizontalNavTabToNav = (
	export const convertHorizontalNavTabToNavPage = (

	horizontalNavTabs: ResolvedCodeRefProperties<HorizontalNavTabExtensionProps>[]
	horizontalNavTabs: HorizontalNavProps[]

		export const ALL_NAMESPACES = 'all-namespaces';
		export const DEFAULT_NS = 'default';

move object resources to new nav item #895

move object resources to new nav item #895

Conversation

SanjalKatiyar commented Jun 9, 2023 • edited Loading

openshift-ci bot commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar commented Jun 12, 2023

SanjalKatiyar commented Jun 12, 2023

SanjalKatiyar commented Jun 13, 2023

SanjalKatiyar commented Jun 15, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Jun 20, 2023 • edited Loading

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

SanjalKatiyar Jun 20, 2023 • edited Loading

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

SanjalKatiyar commented Jun 21, 2023

bipuladh commented Jun 21, 2023

SanjalKatiyar commented Jun 21, 2023

SanjalKatiyar commented Jun 21, 2023

SanjalKatiyar commented Jun 21, 2023

SanjalKatiyar commented Jun 9, 2023 •

edited

Loading

SanjalKatiyar Jun 20, 2023 •

edited

Loading

SanjalKatiyar Jun 20, 2023 •

edited

Loading