refactor: address custom root roles PR comments (#3994)

https://linear.app/unleash/issue/2-1135/address-3975-pr-comments-by-refactoring-some-of-the-new-custom-root

This pull request addresses the majority of the comments raised in issue
#3975 and lays the groundwork for unifying roles. The idea is for
project roles to also be managed in the "Roles" tab, and several
components, such as `RoleForm` and the `useRoleForm` can potentially be
reused.

I'll leave the further investigation and implementation of unifying
roles to be addressed in a separate task.

As a mostly unrelated UI fix, this also adds an arrow to the tooltip in
the `RoleBadge` component.

frontend/src/component/admin/projectRoles/ProjectRoleForm/PermissionAccordion/PermissionAccordion.tsx

@@ -48,7 +48,7 @@ export const PermissionAccordion: VFC<IEnvironmentPermissionAccordionProps> = ({
     permissions,
     checkedPermissions,
     Icon,
-    isInitiallyExpanded,
+    isInitiallyExpanded = false,
     context,
     onPermissionChange,
     onCheckAll,

frontend/src/component/admin/roles/RoleForm/RoleForm.tsx

@@ -5,7 +5,7 @@ import { Person as UserIcon } from '@mui/icons-material';
 import { ICheckedPermissions, IPermission } from 'interfaces/permissions';
 import { IRoleFormErrors } from './useRoleForm';
 import { ROOT_PERMISSION_CATEGORIES } from '@server/types/permissions';
-import cloneDeep from 'lodash.clonedeep';
+import { toggleAllPermissions, togglePermission } from 'utils/permissions';
 
 const StyledInputDescription = styled('p')(({ theme }) => ({
     display: 'flex',
@@ -30,7 +30,6 @@ interface IRoleFormProps {
     setCheckedPermissions: React.Dispatch<
         React.SetStateAction<ICheckedPermissions>
     >;
-    handlePermissionChange: (permission: IPermission) => void;
     permissions: IPermission[];
     errors: IRoleFormErrors;
 }
@@ -42,7 +41,6 @@ export const RoleForm = ({
     setDescription,
     checkedPermissions,
     setCheckedPermissions,
-    handlePermissionChange,
     permissions,
     errors,
 }: IRoleFormProps) => {
@@ -61,30 +59,25 @@ export const RoleForm = ({
         categorizedPermissions.map(({ category }) => category).sort()
     );
 
-    const onToggleAllPermissions = (category: string) => {
-        let checkedPermissionsCopy = cloneDeep(checkedPermissions);
+    const onPermissionChange = (permission: IPermission) => {
+        const newCheckedPermissions = togglePermission(
+            checkedPermissions,
+            permission
+        );
+        setCheckedPermissions(newCheckedPermissions);
+    };
 
+    const onCheckAll = (category: string) => {
         const categoryPermissions = categorizedPermissions
             .filter(({ category: pCategory }) => pCategory === category)
             .map(({ permission }) => permission);
 
-        const allChecked = categoryPermissions.every(
-            (permission: IPermission) => checkedPermissionsCopy[permission.id]
+        const newCheckedPermissions = toggleAllPermissions(
+            checkedPermissions,
+            categoryPermissions
         );
 
-        if (allChecked) {
-            categoryPermissions.forEach((permission: IPermission) => {
-                delete checkedPermissionsCopy[permission.id];
-            });
-        } else {
-            categoryPermissions.forEach((permission: IPermission) => {
-                checkedPermissionsCopy[permission.id] = {
-                    ...permission,
-                };
-            });
-        }
-
-        setCheckedPermissions(checkedPermissionsCopy);
+        setCheckedPermissions(newCheckedPermissions);
     };
 
     return (
@@ -128,9 +121,9 @@ export const RoleForm = ({
                         .map(({ permission }) => permission)}
                     checkedPermissions={checkedPermissions}
                     onPermissionChange={(permission: IPermission) =>
-                        handlePermissionChange(permission)
+                        onPermissionChange(permission)
                     }
-                    onCheckAll={() => onToggleAllPermissions(category)}
+                    onCheckAll={() => onCheckAll(category)}
                 />
             ))}
         </div>

frontend/src/component/admin/roles/RoleForm/useRoleForm.ts

@@ -1,9 +1,8 @@
 import { useEffect, useState } from 'react';
 import { IPermission, ICheckedPermissions } from 'interfaces/permissions';
-import cloneDeep from 'lodash.clonedeep';
-import usePermissions from 'hooks/api/getters/usePermissions/usePermissions';
 import IRole from 'interfaces/role';
 import { useRoles } from 'hooks/api/getters/useRoles/useRoles';
+import { permissionsToCheckedPermissions } from 'utils/permissions';
 
 enum ErrorField {
     NAME = 'name',
@@ -19,33 +18,11 @@ export const useRoleForm = (
     initialPermissions: IPermission[] = []
 ) => {
     const { roles } = useRoles();
-    const { permissions } = usePermissions({
-        revalidateIfStale: false,
-        revalidateOnReconnect: false,
-        revalidateOnFocus: false,
-    });
-
-    const rootPermissions = permissions.root.filter(
-        ({ name }) => name !== 'ADMIN'
-    );
 
     const [name, setName] = useState(initialName);
     const [description, setDescription] = useState(initialDescription);
     const [checkedPermissions, setCheckedPermissions] =
         useState<ICheckedPermissions>({});
-
-    useEffect(() => {
-        setCheckedPermissions(
-            initialPermissions.reduce(
-                (acc: { [key: string]: IPermission }, curr: IPermission) => {
-                    acc[curr.id] = curr;
-                    return acc;
-                },
-                {}
-            )
-        );
-    }, [initialPermissions.length]);
-
     const [errors, setErrors] = useState<IRoleFormErrors>({});
 
     useEffect(() => {
@@ -56,44 +33,16 @@ export const useRoleForm = (
         setDescription(initialDescription);
     }, [initialDescription]);
 
-    const handlePermissionChange = (permission: IPermission) => {
-        let checkedPermissionsCopy = cloneDeep(checkedPermissions);
+    useEffect(() => {
+        const newCheckedPermissions =
+            permissionsToCheckedPermissions(initialPermissions);
+        setCheckedPermissions(newCheckedPermissions);
+    }, [initialPermissions.length]);
 
-        if (checkedPermissionsCopy[permission.id]) {
-            delete checkedPermissionsCopy[permission.id];
-        } else {
-            checkedPermissionsCopy[permission.id] = { ...permission };
-        }
-
-        setCheckedPermissions(checkedPermissionsCopy);
-    };
-
-    const onToggleAllPermissions = () => {
-        let checkedPermissionsCopy = cloneDeep(checkedPermissions);
-
-        const allChecked = rootPermissions.every(
-            (permission: IPermission) => checkedPermissionsCopy[permission.id]
-        );
-
-        if (allChecked) {
-            rootPermissions.forEach((permission: IPermission) => {
-                delete checkedPermissionsCopy[permission.id];
-            });
-        } else {
-            rootPermissions.forEach((permission: IPermission) => {
-                checkedPermissionsCopy[permission.id] = {
-                    ...permission,
-                };
-            });
-        }
-
-        setCheckedPermissions(checkedPermissionsCopy);
-    };
-
-    const getRolePayload = () => ({
+    const getRolePayload = (type: 'root-custom' | 'custom' = 'custom') => ({
         name,
         description,
-        type: 'root-custom',
+        type,
         permissions: Object.values(checkedPermissions),
     });
 
@@ -121,14 +70,11 @@ export const useRoleForm = (
     return {
         name,
         description,
-        errors,
         checkedPermissions,
-        rootPermissions,
+        errors,
         setName,
         setDescription,
         setCheckedPermissions,
-        handlePermissionChange,
-        onToggleAllPermissions,
         getRolePayload,
         clearError,
         setError,

frontend/src/component/admin/roles/RoleModal/RoleModal.tsx

@@ -10,6 +10,7 @@ import { formatUnknownError } from 'utils/formatUnknownError';
 import { FormEvent } from 'react';
 import { useRolesApi } from 'hooks/api/actions/useRolesApi/useRolesApi';
 import { useRole } from 'hooks/api/getters/useRole/useRole';
+import usePermissions from 'hooks/api/getters/usePermissions/usePermissions';
 
 const StyledForm = styled('form')(() => ({
     display: 'flex',
@@ -44,12 +45,10 @@ export const RoleModal = ({ roleId, open, setOpen }: IRoleModalProps) => {
         setDescription,
         checkedPermissions,
         setCheckedPermissions,
-        handlePermissionChange,
         getRolePayload,
         isNameUnique,
         isNotEmpty,
         hasPermissions,
-        rootPermissions,
         errors,
         setError,
         clearError,
@@ -57,9 +56,18 @@ export const RoleModal = ({ roleId, open, setOpen }: IRoleModalProps) => {
     } = useRoleForm(role?.name, role?.description, role?.permissions);
     const { refetch: refetchRoles } = useRoles();
     const { addRole, updateRole, loading } = useRolesApi();
+    const { permissions } = usePermissions({
+        revalidateIfStale: false,
+        revalidateOnReconnect: false,
+        revalidateOnFocus: false,
+    });
     const { setToastData, setToastApiError } = useToast();
     const { uiConfig } = useUiConfig();
 
+    const rootPermissions = permissions.root.filter(
+        ({ name }) => name !== 'ADMIN'
+    );
+
     const editing = role !== undefined;
     const isValid =
         isNameUnique(name) &&
@@ -67,13 +75,15 @@ export const RoleModal = ({ roleId, open, setOpen }: IRoleModalProps) => {
         isNotEmpty(description) &&
         hasPermissions(checkedPermissions);
 
+    const payload = getRolePayload('root-custom');
+
     const formatApiCode = () => {
         return `curl --location --request ${editing ? 'PUT' : 'POST'} '${
             uiConfig.unleashUrl
         }/api/admin/roles${editing ? `/${role.id}` : ''}' \\
     --header 'Authorization: INSERT_API_KEY' \\
     --header 'Content-Type: application/json' \\
-    --data-raw '${JSON.stringify(getRolePayload(), undefined, 2)}'`;
+    --data-raw '${JSON.stringify(payload, undefined, 2)}'`;
     };
 
     const onSetName = (name: string) => {
@@ -96,9 +106,9 @@ export const RoleModal = ({ roleId, open, setOpen }: IRoleModalProps) => {
 
         try {
             if (editing) {
-                await updateRole(role.id, getRolePayload());
+                await updateRole(role.id, payload);
             } else {
-                await addRole(getRolePayload());
+                await addRole(payload);
             }
             setToastData({
                 title: `Role ${editing ? 'updated' : 'added'} successfully`,
@@ -136,7 +146,6 @@ export const RoleModal = ({ roleId, open, setOpen }: IRoleModalProps) => {
                         setDescription={setDescription}
                         checkedPermissions={checkedPermissions}
                         setCheckedPermissions={setCheckedPermissions}
-                        handlePermissionChange={handlePermissionChange}
                         permissions={rootPermissions}
                         errors={errors}
                     />

frontend/src/component/common/RoleBadge/RoleBadge.tsx

@@ -14,7 +14,7 @@ export const RoleBadge = ({ roleId }: IRoleBadgeProps) => {
     if (!role) return null;
 
     return (
-        <HtmlTooltip title={<RoleDescription roleId={roleId} tooltip />}>
+        <HtmlTooltip title={<RoleDescription roleId={roleId} tooltip />} arrow>
             <Badge
                 color="success"
                 icon={<UserIcon />}

frontend/src/hooks/api/getters/usePermissions/usePermissions.ts

@@ -2,21 +2,11 @@ import useSWR, { mutate, SWRConfiguration } from 'swr';
 import { useState, useEffect } from 'react';
 import { formatApiPath } from 'utils/formatPath';
 
-import {
-    IProjectEnvironmentPermissions,
-    IPermissions,
-    IPermission,
-} from 'interfaces/permissions';
+import { IPermissions } from 'interfaces/permissions';
 import handleErrorResponses from '../httpErrorResponseHandler';
 
 interface IUsePermissions {
-    permissions:
-        | IPermissions
-        | {
-              root: IPermission[];
-              project: IPermission[];
-              environments: IProjectEnvironmentPermissions[];
-          };
+    permissions: IPermissions;
     loading: boolean;
     refetch: () => void;
     error: any;

frontend/src/hooks/api/getters/useRole/useRole.ts

@@ -2,12 +2,12 @@ import { SWRConfiguration } from 'swr';
 import { useMemo } from 'react';
 import { formatApiPath } from 'utils/formatPath';
 import handleErrorResponses from '../httpErrorResponseHandler';
-import IRole from 'interfaces/role';
+import IRole, { IRoleWithPermissions } from 'interfaces/role';
 import useUiConfig from '../useUiConfig/useUiConfig';
 import { useConditionalSWR } from '../useConditionalSWR/useConditionalSWR';
 
 export interface IUseRoleOutput {
-    role?: IRole;
+    role?: IRoleWithPermissions;
     refetch: () => void;
     loading: boolean;
     error?: Error;
@@ -42,16 +42,19 @@ export const useRole = (
     return useMemo(() => {
         if (!isEnterprise()) {
             return {
-                role: ((ossData?.rootRoles ?? []) as IRole[]).find(
-                    ({ id: rId }) => rId === +id!
-                ),
+                role: {
+                    ...((ossData?.rootRoles ?? []) as IRole[]).find(
+                        ({ id: rId }) => rId === +id!
+                    ),
+                    permissions: [],
+                } as IRoleWithPermissions,
                 loading: !ossError && !ossData,
                 refetch: () => ossMutate(),
                 error: ossError,
             };
         } else {
             return {
-                role: data as IRole,
+                role: data as IRoleWithPermissions,
                 loading: !error && !data,
                 refetch: () => mutate(),
                 error,

frontend/src/interfaces/role.ts

@@ -6,7 +6,10 @@ interface IRole {
     project: string | null;
     description: string;
     type: string;
-    permissions?: IPermission[];
+}
+
+export interface IRoleWithPermissions extends IRole {
+    permissions: IPermission[];
 }
 
 export interface IProjectRole {

frontend/src/utils/permissions.ts

@@ -0,0 +1,63 @@
+import { IPermission, ICheckedPermissions } from 'interfaces/permissions';
+import cloneDeep from 'lodash.clonedeep';
+
+const getRoleKey = (permission: IPermission): string => {
+    return permission.environment
+        ? `${permission.id}-${permission.environment}`
+        : `${permission.id}`;
+};
+
+export const permissionsToCheckedPermissions = (
+    permissions: IPermission[]
+): ICheckedPermissions =>
+    permissions.reduce(
+        (
+            checkedPermissions: { [key: string]: IPermission },
+            permission: IPermission
+        ) => {
+            checkedPermissions[getRoleKey(permission)] = permission;
+            return checkedPermissions;
+        },
+        {}
+    );
+
+export const togglePermission = (
+    checkedPermissions: ICheckedPermissions,
+    permission: IPermission
+): ICheckedPermissions => {
+    let checkedPermissionsCopy = cloneDeep(checkedPermissions);
+
+    if (checkedPermissionsCopy[getRoleKey(permission)]) {
+        delete checkedPermissionsCopy[getRoleKey(permission)];
+    } else {
+        checkedPermissionsCopy[getRoleKey(permission)] = { ...permission };
+    }
+
+    return checkedPermissionsCopy;
+};
+
+export const toggleAllPermissions = (
+    checkedPermissions: ICheckedPermissions,
+    toggledPermissions: IPermission[]
+): ICheckedPermissions => {
+    let checkedPermissionsCopy = cloneDeep(checkedPermissions);
+
+    const allChecked = toggledPermissions.every(
+        (permission: IPermission) =>
+            checkedPermissionsCopy[getRoleKey(permission)]
+    );
+
+    if (allChecked) {
+        toggledPermissions.forEach((permission: IPermission) => {
+            delete checkedPermissionsCopy[getRoleKey(permission)];
+        });
+    } else {
+        toggledPermissions.forEach((permission: IPermission) => {
+            checkedPermissionsCopy[getRoleKey(permission)] = {
+                ...permission,
+            };
+        });
+    }
+
+    return checkedPermissionsCopy;
+};