fix(rbac): enable save on remove-all button click (janus-idp#1712)

ciiay · May 28, 2024 · 0502332 · 0502332
1 parent d56f4af
commit 0502332
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 19 deletions.
diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionalAccessSidebar.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionalAccessSidebar.tsx
@@ -42,7 +42,7 @@ const useDrawerContentStyles = makeStyles(theme => ({
 type ConditionalAccessSidebarProps = {
   open: boolean;
   onClose: () => void;
-  onSave: (conditions: ConditionsData) => void;
+  onSave: (conditions?: ConditionsData) => void;
   selPluginResourceType: string;
   conditionRulesData?: RulesData;
   conditionsFormVal?: ConditionsData;

diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.test.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.test.tsx
@@ -0,0 +1,79 @@
+import React from 'react';
+
+import { fireEvent, render, screen } from '@testing-library/react';
+
+import { mockTransformedConditionRules } from '../../__fixtures__/mockTransformedConditionRules';
+import { ConditionsForm } from './ConditionsForm';
+
+describe('ConditionsForm', () => {
+  const selPluginResourceType = 'catalog-entity';
+  const onSaveMock = jest.fn();
+  const onCloseMock = jest.fn();
+
+  const renderComponent = (props = {}) =>
+    render(
+      <ConditionsForm
+        selPluginResourceType={selPluginResourceType}
+        conditionRulesData={
+          mockTransformedConditionRules.catalog['catalog-entity']
+        }
+        onClose={onCloseMock}
+        onSave={onSaveMock}
+        {...props}
+      />,
+    );
+
+  beforeEach(() => {
+    onSaveMock.mockClear();
+    onCloseMock.mockClear();
+  });
+
+  it('renders without crashing', () => {
+    const { getByTestId } = renderComponent();
+    expect(getByTestId('conditions-row')).toBeInTheDocument();
+    expect(getByTestId('save-conditions')).toBeInTheDocument();
+    expect(getByTestId('cancel-conditions')).toBeInTheDocument();
+    expect(getByTestId('remove-conditions')).toBeInTheDocument();
+  });
+
+  it('calls onClose when Cancel button is clicked', () => {
+    const { getByTestId } = renderComponent();
+    fireEvent.click(getByTestId('cancel-conditions'));
+    expect(onCloseMock).toHaveBeenCalled();
+  });
+
+  it('resets conditions data when Remove all button is clicked', () => {
+    const { getByTestId } = renderComponent({
+      conditionsFormVal: {
+        condition: {
+          rule: 'HAS_LABEL',
+          resourceType: selPluginResourceType,
+          params: {},
+        },
+      },
+    });
+
+    fireEvent.click(getByTestId('remove-conditions'));
+    fireEvent.click(getByTestId('save-conditions'));
+
+    expect(onSaveMock).toHaveBeenCalledWith(undefined);
+  });
+
+  it('disables Save button if conditions are unchanged', () => {
+    const { getByTestId } = renderComponent();
+    expect(getByTestId('save-conditions')).toBeDisabled();
+  });
+
+  it('disables Save button if no rule is selected', () => {
+    const { getByTestId } = renderComponent();
+
+    fireEvent.change(
+      screen.getByRole('textbox', {
+        name: /rule/i,
+      }),
+      { target: { value: '' } },
+    );
+
+    expect(getByTestId('save-conditions')).toBeDisabled();
+  });
+});
diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx
@@ -33,7 +33,7 @@ type ConditionFormProps = {
   conditionsFormVal?: ConditionsData;
   selPluginResourceType: string;
   onClose: () => void;
-  onSave: (conditions: ConditionsData) => void;
+  onSave: (conditions?: ConditionsData) => void;
 };
 
 export const ConditionsForm = ({
@@ -58,30 +58,38 @@ export const ConditionsForm = ({
   );
   const [errors, setErrors] = React.useState<RuleParamsErrors>();
 
-  const isSaveDisabled = () => {
-    const hasErrors = !!errors?.[criteria]?.length;
-    let noRuleSelected;
+  const [removeAllClicked, setRemoveAllClicked] =
+    React.useState<boolean>(false);
+
+  const isNoRuleSelected = () => {
     switch (criteria) {
       case criterias.condition: {
-        noRuleSelected = !conditions.condition?.rule;
-        break;
+        return !conditions.condition?.rule;
       }
       case criterias.not: {
-        noRuleSelected = !conditions.not?.rule;
-        break;
+        return !conditions.not?.rule;
       }
       case criterias.allOf: {
-        noRuleSelected = !!conditions.allOf?.find(c => !c.rule);
-        break;
+        return !!conditions.allOf?.find(c => !c.rule);
       }
       case criterias.anyOf: {
-        noRuleSelected = !!conditions.anyOf?.find(c => !c.rule);
-        break;
+        return !!conditions.anyOf?.find(c => !c.rule);
       }
       default:
-        noRuleSelected = true;
+        return true;
     }
-    return hasErrors || noRuleSelected;
+  };
+
+  const isSaveDisabled = () => {
+    const hasErrors = !!errors?.[criteria]?.length;
+
+    if (removeAllClicked) return false;
+
+    return (
+      hasErrors ||
+      isNoRuleSelected() ||
+      Object.is(conditionsFormVal, conditions)
+    );
   };
 
   return (
@@ -95,6 +103,7 @@ export const ConditionsForm = ({
           onRuleChange={newCondition => setConditions(newCondition)}
           setCriteria={setCriteria}
           setErrors={setErrors}
+          setRemoveAllClicked={setRemoveAllClicked}
         />
       </Box>
       <Box className={classes.footer}>
@@ -103,17 +112,25 @@ export const ConditionsForm = ({
           data-testid="save-conditions"
           disabled={isSaveDisabled()}
           onClick={() => {
-            onSave(conditions);
+            if (removeAllClicked) {
+              onSave(undefined);
+            } else onSave(conditions);
           }}
         >
           Save
         </Button>
-        <Button variant="outlined" onClick={onClose}>
+        <Button
+          variant="outlined"
+          onClick={onClose}
+          data-testid="cancel-conditions"
+        >
           Cancel
         </Button>
         <Button
           variant="text"
+          disabled={removeAllClicked || isNoRuleSelected()}
           onClick={() => {
+            setRemoveAllClicked(true);
             setCriteria(criterias.condition);
             setConditions({
               condition: {
@@ -123,6 +140,7 @@ export const ConditionsForm = ({
               },
             });
           }}
+          data-testid="remove-conditions"
         >
           Remove all
         </Button>

diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRow.tsx
@@ -53,6 +53,7 @@ type ConditionFormRowProps = {
   criteria: string;
   setCriteria: React.Dispatch<React.SetStateAction<string>>;
   setErrors: React.Dispatch<React.SetStateAction<RuleParamsErrors | undefined>>;
+  setRemoveAllClicked: React.Dispatch<React.SetStateAction<boolean>>;
 };
 
 export const ConditionsFormRow = ({
@@ -63,6 +64,7 @@ export const ConditionsFormRow = ({
   onRuleChange,
   setCriteria,
   setErrors,
+  setRemoveAllClicked,
 }: ConditionFormRowProps) => {
   const classes = useStyles();
   const theme = useTheme();
@@ -126,7 +128,7 @@ export const ConditionsFormRow = ({
   };
 
   return (
-    <Box className={classes.conditionRow}>
+    <Box className={classes.conditionRow} data-testid="conditions-row">
       <ButtonGroup size="large" className={classes.criteriaButtonGroup}>
         {conditionButtons.map(({ val, label }) => (
           <Button
@@ -164,6 +166,7 @@ export const ConditionsFormRow = ({
                   optionDisabled={ruleOption =>
                     ruleOptionDisabled(ruleOption, conditionRow.allOf)
                   }
+                  setRemoveAllClicked={setRemoveAllClicked}
                 />
                 <IconButton
                   title="Remove"
@@ -194,6 +197,7 @@ export const ConditionsFormRow = ({
                   optionDisabled={ruleOption =>
                     ruleOptionDisabled(ruleOption, conditionRow.anyOf)
                   }
+                  setRemoveAllClicked={setRemoveAllClicked}
                 />
                 <IconButton
                   title="Remove"
@@ -252,6 +256,7 @@ export const ConditionsFormRow = ({
               conditionRow.condition ? [conditionRow.condition] : undefined,
             )
           }
+          setRemoveAllClicked={setRemoveAllClicked}
         />
       )}
       {criteria === criterias.not && (
@@ -274,6 +279,7 @@ export const ConditionsFormRow = ({
               conditionRow.not ? [conditionRow.not] : undefined,
             )
           }
+          setRemoveAllClicked={setRemoveAllClicked}
         />
       )}
     </Box>

diff --git a/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx b/plugins/rbac/src/components/ConditionalAccess/ConditionsFormRowFields.tsx
@@ -31,6 +31,7 @@ type ConditionFormRowFieldsProps = {
   conditionRulesData?: RulesData;
   setErrors: React.Dispatch<React.SetStateAction<RuleParamsErrors | undefined>>;
   optionDisabled: (ruleOption: string) => boolean;
+  setRemoveAllClicked: React.Dispatch<React.SetStateAction<boolean>>;
 };
 
 export const ConditionsFormRowFields = ({
@@ -42,6 +43,7 @@ export const ConditionsFormRowFields = ({
   conditionRulesData,
   setErrors,
   optionDisabled,
+  setRemoveAllClicked,
 }: ConditionFormRowFieldsProps) => {
   const classes = useStyles();
   const rules = conditionRulesData?.rules ?? [];
@@ -60,6 +62,7 @@ export const ConditionsFormRowFields = ({
   const customFields: RegistryFieldsType = { ArrayField: CustomArrayField };
 
   const handleConditionChange = (newCondition: PermissionCondition) => {
+    setRemoveAllClicked(false);
     switch (criteria) {
       case criterias.condition: {
         onRuleChange({ condition: newCondition });

diff --git a/plugins/rbac/src/components/CreateRole/PermissionPoliciesForm.tsx b/plugins/rbac/src/components/CreateRole/PermissionPoliciesForm.tsx
@@ -126,6 +126,8 @@ export const PermissionPoliciesForm = ({
 
   const onAddConditions = (index: number, conditions?: ConditionsData) => {
     setFieldValue(`permissionPoliciesRows[${index}].conditions`, conditions);
+    if (!conditions)
+      setFieldValue(`permissionPoliciesRows[${index}].id`, undefined);
   };
 
   const onRowRemove = (index: number) => {

diff --git a/plugins/rbac/src/components/CreateRole/PermissionPoliciesFormRow.tsx b/plugins/rbac/src/components/CreateRole/PermissionPoliciesFormRow.tsx
@@ -204,7 +204,7 @@ export const PermissionPoliciesFormRow = ({
         onClose={() => {
           setSidebarOpen(false);
         }}
-        onSave={(conditions: ConditionsData) => {
+        onSave={(conditions?: ConditionsData) => {
           onAddConditions(conditions);
           setSidebarOpen(false);
         }}