Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
463c957
RHSTOR-6347: Enable recipe-based DR enrollment for discovered applica…
GowthamShanmugam Sep 19, 2024
74d61f1
Unit test warning fix
GowthamShanmugam Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion locales/en/plugin__odf-console.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@
"You have selected {{count}} namespaces, to view or change your selection go back to the previous step._other": "You have selected {{count}} namespaces, to view or change your selection go back to the previous step.",
"Secure selected namespace by defining resource label expressions.": "Secure selected namespace by defining resource label expressions.",
"Resource label": "Resource label",
"Secure namespaces as per Recipe definition.": "Secure namespaces as per Recipe definition.",
"Recipe": "Recipe",
"Recipe list": "Recipe list",
"Only recipes of the selected namespaces will appear in the list.": "Only recipes of the selected namespaces will appear in the list.",
"Select a recipe": "Select a recipe",
Expand Down Expand Up @@ -124,7 +126,6 @@
"Name:": "Name:",
"Configuration": "Configuration",
"Type:": "Type:",
"Recipe": "Recipe",
"Recipe name:": "Recipe name:",
"Recipe namespace:": "Recipe namespace:",
"Label expressions:": "Label expressions:",
Expand Down
20 changes: 8 additions & 12 deletions packages/mco/components/discovered-application-wizard/enroll-discovered-application.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,12 +288,11 @@ const moveToStep = async (step: number) => {
fireEvent.click(screen.getByText('Next'));
}

// Commented out recipe-based steps
if (step > 2) {
// Select recipe
// fireEvent.click(screen.getByText('Recipe'));
// fireEvent.click(screen.getByText('Select a recipe'));
// fireEvent.click(screen.getByText('mock-recipe-1'));
fireEvent.click(screen.getByText('Recipe'));
fireEvent.click(screen.getByText('Select a recipe'));
fireEvent.click(screen.getByText('mock-recipe-1'));

// Next wizard step
fireEvent.click(screen.getByText('Next'));
Expand Down Expand Up @@ -441,10 +440,7 @@ describe('Test namespace step', () => {
});
});

// The Recipe-bsaed dr protection is in Dev Preview for ODF 4.16.
// All Recipe-based test cases are skipped.
// https://bugzilla.redhat.com/show_bug.cgi?id=2291301
describe.skip('Test configure step', () => {
describe('Test configure step', () => {
beforeEach(() => {
render(<EnrollDiscoveredApplication />);
});
Expand Down Expand Up @@ -476,7 +472,7 @@ describe.skip('Test configure step', () => {

// Recipe selection
fireEvent.click(screen.getByText('Recipe'));
expect(screen.getByText('Recipe list')).toBeInTheDocument();
/*expect(screen.getByText('Recipe list')).toBeInTheDocument();
expect(
screen.getByText(
'Only recipes of the selected namespaces will appear in the list.'
Expand Down Expand Up @@ -506,11 +502,11 @@ describe.skip('Test configure step', () => {
expect(screen.getByText('namespace-2')).toBeInTheDocument();
fireEvent.click(screen.getByText('mock-recipe-1'));
// Ensure recipe selection
expect(screen.getByText('mock-recipe-1')).toBeInTheDocument();
expect(screen.getByText('mock-recipe-1')).toBeInTheDocument();*/
});
});

describe.skip('Test replication step', () => {
describe('Test replication step', () => {
beforeEach(() => {
render(<EnrollDiscoveredApplication />);
});
Expand Down Expand Up @@ -576,7 +572,7 @@ describe.skip('Test replication step', () => {
expect(screen.getByText('days')).toBeInTheDocument();
});
});
describe.skip('Test review step', () => {
describe('Test review step', () => {
beforeEach(() => {
render(<EnrollDiscoveredApplication />);
});
Expand Down
9 changes: 3 additions & 6 deletions ...ents/discovered-application-wizard/wizard-steps/configuration-step/configuration-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,11 @@ export const Configuration: React.FC<ConfigurationProps> = ({
onChange={(event, _unUsed) =>
setProtectionMethod(_unUsed, event)
}
checked={
isChecked={
protectionMethod === ProtectionMethodType.RESOURCE_LABEL
}
/>
</GridItem>
{/* The Recipe-bsaed dr protection is in Dev Preview for ODF 4.16.
https://bugzilla.redhat.com/show_bug.cgi?id=2291301

<GridItem
span={6}
className="mco-configuration-step__radio pf-v5-u-p-lg"
Expand All @@ -97,9 +94,9 @@ export const Configuration: React.FC<ConfigurationProps> = ({
onChange={(event, _unUsed) =>
setProtectionMethod(_unUsed, event)
}
checked={protectionMethod === ProtectionMethodType.RECIPE}
isChecked={protectionMethod === ProtectionMethodType.RECIPE}
/>
</GridItem> */}
</GridItem>
</Grid>
</FormGroup>
{protectionMethod === ProtectionMethodType.RECIPE && (
Expand Down
25 changes: 12 additions & 13 deletions packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
Button,
Form,
FormGroup,
Text,
Grid,
GridItem,
Popover,
Expand Down Expand Up @@ -295,23 +294,23 @@ export const PVCDetailsWizardContent: React.FC<PVCDetailsWizardContentProps> =
return (
<Form>
<FormGroup>
<Text>
<span>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{t(
'Use PVC label selectors to effortlessly specify the application resources that need protection. You can also create a custom PVC label selector if one doesn’t exists. For more information, '
)}
<Popover
</span>
<Popover
aria-label={t('Help')}
bodyContent={getLabelValidationMessage(t)}
>
<Button
aria-label={t('Help')}
bodyContent={getLabelValidationMessage(t)}
variant={ButtonVariant.link}
isInline
>
<Button
aria-label={t('Help')}
variant={ButtonVariant.link}
isInline
>
{t('see PVC label selector requirements.')}
</Button>
</Popover>
</Text>
{t('see PVC label selector requirements.')}
</Button>
</Popover>
</FormGroup>
{loaded && !error ? (
<LazyNameValueEditor
Expand Down
8 changes: 6 additions & 2 deletions packages/shared/src/table/selectable-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ export const SelectableTable: SelectableTableProps = <

const [selectableRows, rowIds] = React.useMemo(() => {
const selectableRows =
sortedRows?.filter(isRowSelectable || hasNoDeletionTimestamp) || [];
sortedRows?.filter((selectedRow) =>
!!isRowSelectable
? isRowSelectable(selectedRow)
: true && hasNoDeletionTimestamp(selectedRow)
) || [];
const rowIds = new Set(selectableRows?.map(getUID));
return [selectableRows, rowIds];
}, [sortedRows, isRowSelectable]);
Expand Down Expand Up @@ -152,7 +156,7 @@ export const SelectableTable: SelectableTableProps = <
onSelect: onSelect,
isSelected: isRowSelected(getUID(row), selectedRows),
isDisabled:
!isRowSelectable?.(row) ||
(!!isRowSelectable && !isRowSelectable?.(row)) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original line was correct...

!hasNoDeletionTimestamp(row),
props: {
id: getUID(row),
Expand Down
7 changes: 6 additions & 1 deletion packages/shared/src/utils/AsyncLoader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const mounted = React.useRef(true);
React.useEffect(() => () => (mounted.current = false), []);

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

const setComponent = React.useCallback(
(value) => {
Component.current = value;
setLoaded(true);
if (mounted.current) {
setLoaded(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ??

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
},
[Component]
);
Expand Down
Loading