RHSTOR-6347: Enable recipe-based DR enrollment for discovered applications #1589

GowthamShanmugam · 2024-09-19T12:02:52Z

No description provided.

openshift-ci-robot · 2024-09-19T12:02:56Z

@GowthamShanmugam: This pull request references RHSTOR-6347 which is a valid jira issue.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

GowthamShanmugam · 2024-10-07T10:35:02Z

packages/shared/src/utils/AsyncLoader.tsx


  const prevLoader = React.useRef<PromiseComponent>(null);

  const setComponent = React.useCallback(
    (value) => {
      Component.current = value;
-      setLoaded(true);
+      if (mounted.current) {
+        setLoaded(true);


To fix unit test case waring, React state update on an unmounted component

Just curious: there are other state variables too, why jest is complaining only about this one in particular ??
Also, why doesn't it do so for other components that we have in this repo, why only this ??

are we sure there isn't some other reason for this warning ??

I verified in the console warning that this error is only being raised in jest, and I am not receiving it. The async function's state update, which was carried out after the component unmounts, is the cause of this error. This is occurring as a result of the jest test case being executed more quickly.

adding act() function has resolved this issue.

GowthamShanmugam · 2024-10-07T10:36:17Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

@@ -295,23 +294,23 @@ export const PVCDetailsWizardContent: React.FC<PVCDetailsWizardContentProps> =
    return (
      <Form>
        <FormGroup>
-          <Text>
+          <span>


To fix unit test case waring, div cannot appear as a descendant of p

GowthamShanmugam · 2024-10-07T10:50:56Z

SanjalKatiyar · 2024-10-10T10:58:56Z

packages/shared/src/table/selectable-table.tsx

@@ -152,7 +156,7 @@ export const SelectableTable: SelectableTableProps = <
                        onSelect: onSelect,
                        isSelected: isRowSelected(getUID(row), selectedRows),
                        isDisabled:
-                          !isRowSelectable?.(row) ||
+                          (!!isRowSelectable && !isRowSelectable?.(row)) ||


this is not needed... !isRowSelectable?.(row) syntax will only execute when function exists, else not...

Suggested change

(!!isRowSelectable && !isRowSelectable?.(row)) ||

(!isRowSelectable?.(row) ||

original line was correct...

The issue in original condition is,
isRowSelectable?: IsRowSelectable; is mentioned as optional. But if the component is not passing isRowSelectable, then !isRowSelectable?.(row)always returns True, and all the rows are disabled by default. It's not correct.

got it !!
then probably we can remove ?. part entirely... it's not needed when we are using condition like !!isRowSelectable && ...

ack, is it won't affect fusion UI? i think fusion is using this table

it should not affect in this case...

oh! got it. which ? part you have mentioned

I meant using !!isRowSelectable && !isRowSelectable(row)) instead of !!isRowSelectable && !isRowSelectable?.(row)).

?., when we optionally call isRowSelectable.

ack, changes done

SanjalKatiyar · 2024-10-10T11:00:03Z

packages/shared/src/utils/AsyncLoader.tsx

@@ -47,13 +47,18 @@ const useAsynchronousLoading: UseAsynchronousLoading = (
  const Component = React.useRef<React.ComponentType>(null);
  const [loadingStarted, setLoadingStarted] = React.useState(false);
  const [loaded, setLoaded] = React.useState(false);
+  // Mount status for safty state updates


Suggested change

// Mount status for safty state updates

// Mount status for safely state updates

we can remove the comment as well... it's kind of self-explanatory from variable name itself...

SanjalKatiyar · 2024-10-21T10:49:15Z

packages/shared/src/table/selectable-table.tsx

+      sortedRows?.filter((selectedRow) =>
+        !!isRowSelectable
+          ? isRowSelectable(selectedRow)
+          : true && hasNoDeletionTimestamp(selectedRow)
+      ) || [];


why do we need this ??
sortedRows?.filter(isRowSelectable || hasNoDeletionTimestamp) || []; format is correct, filter takes a callback as it's argument.

even if isRowSelectable is undefined here that should work as expected...

right! my bad

alfonsomthd · 2024-10-24T06:50:17Z

packages/mco/components/discovered-application-wizard/enroll-discovered-application.spec.tsx

-    // fireEvent.click(screen.getByText('Recipe'));
-    // fireEvent.click(screen.getByText('Select a recipe'));
-    // fireEvent.click(screen.getByText('mock-recipe-1'));
+    fireEvent.click(screen.getByText('Recipe'));


Please don't use fireEvent: use user-event instead.

You have n example of user-event usage here.

…tions Signed-off-by: Gowtham Shanmugasundaram <[email protected]>

SanjalKatiyar · 2024-10-24T09:27:48Z

/approve
/lgtm

openshift-ci · 2024-10-24T09:27:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam, 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~~ [GowthamShanmugam,SanjalKatiyar]

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 added the jira/valid-reference label Sep 19, 2024

GowthamShanmugam force-pushed the RHSTOR-6347 branch from c079f30 to 536c3a3 Compare October 7, 2024 09:31

openshift-ci bot added the approved label Oct 7, 2024

GowthamShanmugam force-pushed the RHSTOR-6347 branch 2 times, most recently from f677a51 to 4384c78 Compare October 7, 2024 10:26

GowthamShanmugam commented Oct 7, 2024

View reviewed changes

GowthamShanmugam force-pushed the RHSTOR-6347 branch from 4384c78 to 761ed9b Compare October 7, 2024 10:44

GowthamShanmugam force-pushed the RHSTOR-6347 branch from 761ed9b to 74d61f1 Compare October 7, 2024 11:08

SanjalKatiyar reviewed Oct 10, 2024

View reviewed changes

GowthamShanmugam force-pushed the RHSTOR-6347 branch 2 times, most recently from 47a7fc9 to ca5546c Compare October 21, 2024 09:26

SanjalKatiyar reviewed Oct 21, 2024

View reviewed changes

alfonsomthd reviewed Oct 24, 2024

View reviewed changes

RHSTOR-6347: Enable recipe-based DR enrollment for discovered applica…

1fe79bd

…tions Signed-off-by: Gowtham Shanmugasundaram <[email protected]>

GowthamShanmugam force-pushed the RHSTOR-6347 branch from ca5546c to 1fe79bd Compare October 24, 2024 09:18

openshift-ci bot assigned SanjalKatiyar Oct 24, 2024

openshift-ci bot added the lgtm label Oct 24, 2024

openshift-merge-bot bot merged commit 4bae5b0 into red-hat-storage:master Oct 24, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHSTOR-6347: Enable recipe-based DR enrollment for discovered applications #1589

RHSTOR-6347: Enable recipe-based DR enrollment for discovered applications #1589

GowthamShanmugam commented Sep 19, 2024

openshift-ci-robot commented Sep 19, 2024 •

edited by openshift-ci bot

Loading

GowthamShanmugam Oct 7, 2024

SanjalKatiyar Oct 10, 2024

SanjalKatiyar Oct 10, 2024

GowthamShanmugam Oct 21, 2024

GowthamShanmugam Oct 7, 2024

GowthamShanmugam commented Oct 7, 2024

SanjalKatiyar Oct 10, 2024

SanjalKatiyar Oct 10, 2024

GowthamShanmugam Oct 21, 2024

SanjalKatiyar Oct 21, 2024

GowthamShanmugam Oct 21, 2024

SanjalKatiyar Oct 21, 2024

GowthamShanmugam Oct 21, 2024

SanjalKatiyar Oct 21, 2024

SanjalKatiyar Oct 21, 2024

GowthamShanmugam Oct 21, 2024

SanjalKatiyar Oct 10, 2024

SanjalKatiyar Oct 10, 2024

SanjalKatiyar Oct 21, 2024

SanjalKatiyar Oct 21, 2024

GowthamShanmugam Oct 24, 2024

alfonsomthd Oct 24, 2024

alfonsomthd Oct 24, 2024 •

edited

Loading

SanjalKatiyar commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

	(!!isRowSelectable && !isRowSelectable?.(row)) \|\|
	(!isRowSelectable?.(row) \|\|

	// Mount status for safty state updates
	// Mount status for safely state updates

RHSTOR-6347: Enable recipe-based DR enrollment for discovered applications #1589

RHSTOR-6347: Enable recipe-based DR enrollment for discovered applications #1589

Conversation

GowthamShanmugam commented Sep 19, 2024

openshift-ci-robot commented Sep 19, 2024 • edited by openshift-ci bot 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

GowthamShanmugam commented Oct 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsomthd Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

SanjalKatiyar commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

openshift-ci-robot commented Sep 19, 2024 •

edited by openshift-ci bot

Loading

alfonsomthd Oct 24, 2024 •

edited

Loading