refactor: roles - make better plan assumptions (#4113)

https://linear.app/unleash/issue/2-1171/refactor-custom-root-roles-with-correct-plan-assumptions This cleans up the hotfix `RoleSelect2` component and makes `RoleSelect` take in a `roles` prop from the parent component. This also simplifies the role hooks again to assume Enterprise plan by default. This means, however, that we must ensure that we only call these hooks in Enterprise features or, if we do call them in other plans, that we provide a graceful fallback for non-Enterprise. Non-Enterprise instances do not have this endpoint, and so they are currently grabbing role information from e.g. `useUsers` and `useServiceAccounts`. I'm not sure how I feel about this. Roles are an overarching concept of Unleash. To me, having to be extremely conscious about the exact scenario in which you're using such a hook feels like a trap, instead of "I need roles, so I'll grab the `useRoles` hook and not think much about it". I also don't like the way `roles` are currently tied to the users, service accounts, project access, (...) instead of being its own thing. This could be solved by a `RoleController` exposing the GET endpoints in OSS, since all of the logic we need for this use-case lives there anyways. This would then be overridden with the Enterprise-specific controller when wrapped. This way we could assume the endpoint is always there, no matter the plan. This is just an idea and not something I explored in the PR. For now I'm just focusing on leaving this feature in a sane state. Tested this manually on `Pro` and `Enterprise` and I believe everything is acting the way we intend, but would love some extra eyes.
2025-02-04 00:18:01 +01:00 · 2023-06-28 16:00:14 +01:00 · 2023-06-28 16:00:14 +01:00 · 0b3ed79ecc
commit 0b3ed79ecc
parent bc68b5d265
12 changed files with 54 additions and 169 deletions
--- a/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
+++ b/frontend/src/component/admin/groups/GroupForm/GroupForm.tsx
@ -194,6 +194,7 @@ export const GroupForm: FC<IGroupForm> = ({
                            <StyledAutocompleteWrapper>
                                <RoleSelect
                                    data-testid="GROUP_ROOT_ROLE"
+                                    roles={roles}
                                    value={roleIdToRole(rootRole)}
                                    setValue={role =>
                                        setRootRole(role?.id || null)
--- a/frontend/src/component/admin/serviceAccounts/ServiceAccountsTable/ServiceAccountModal/ServiceAccountModal.tsx
+++ b/frontend/src/component/admin/serviceAccounts/ServiceAccountsTable/ServiceAccountModal/ServiceAccountModal.tsx
@ -306,6 +306,7 @@ export const ServiceAccountModal = ({
                            What is your service account allowed to do?
                        </StyledInputDescription>
                        <StyledRoleSelect
+                            roles={roles}
                            value={rootRole}
                            setValue={setRootRole}
                            required
--- a/frontend/src/component/admin/users/UserForm/UserForm.tsx
+++ b/frontend/src/component/admin/users/UserForm/UserForm.tsx
@ -6,6 +6,7 @@ import { EDIT } from 'constants/misc';
 import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
 import { RoleSelect } from 'component/common/RoleSelect/RoleSelect';
 import { IRole } from 'interfaces/role';
+import { useUsers } from 'hooks/api/getters/useUsers/useUsers';

 const StyledForm = styled('form')(() => ({
    display: 'flex',
@ -78,6 +79,7 @@ const UserForm: React.FC<IUserForm> = ({
    mode,
 }) => {
    const { uiConfig } = useUiConfig();
+    const { roles } = useUsers();

    return (
        <StyledForm onSubmit={handleSubmit}>
@ -106,7 +108,12 @@ const UserForm: React.FC<IUserForm> = ({
                <StyledRoleSubtitle variant="subtitle1" data-loading>
                    What is your team member allowed to do?
                </StyledRoleSubtitle>
-                <RoleSelect value={rootRole} setValue={setRootRole} required />
+                <RoleSelect
+                    roles={roles}
+                    value={rootRole}
+                    setValue={setRootRole}
+                    required
+                />
                <ConditionallyRender
                    condition={mode !== EDIT && Boolean(uiConfig?.emailEnabled)}
                    show={
--- a/frontend/src/component/admin/users/hooks/useAddUserForm.ts
+++ b/frontend/src/component/admin/users/hooks/useAddUserForm.ts
@ -2,7 +2,6 @@ import { useEffect, useState } from 'react';
 import { useUsers } from 'hooks/api/getters/useUsers/useUsers';
 import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
 import { IRole } from 'interfaces/role';
-import { useRoles } from 'hooks/api/getters/useRoles/useRoles';

 const useCreateUserForm = (
    initialName = '',
@ -10,7 +9,7 @@ const useCreateUserForm = (
    initialRootRole = null
 ) => {
    const { uiConfig } = useUiConfig();
-    const { roles } = useRoles();
+    const { users, roles } = useUsers();
    const [name, setName] = useState(initialName);
    const [email, setEmail] = useState(initialEmail);
    const [sendEmail, setSendEmail] = useState(false);
@ -19,8 +18,6 @@ const useCreateUserForm = (
    );
    const [errors, setErrors] = useState({});

-    const { users } = useUsers();
-
    useEffect(() => {
        setName(initialName);
    }, [initialName]);
--- a/frontend/src/component/common/RoleBadge/RoleBadge.tsx
+++ b/frontend/src/component/common/RoleBadge/RoleBadge.tsx
@ -6,12 +6,21 @@ import { RoleDescription } from 'component/common/RoleDescription/RoleDescriptio

 interface IRoleBadgeProps {
    roleId: number;
+    children?: string;
 }

-export const RoleBadge = ({ roleId }: IRoleBadgeProps) => {
+export const RoleBadge = ({ roleId, children }: IRoleBadgeProps) => {
    const { role } = useRole(roleId.toString());

-    if (!role) return null;
+    if (!role) {
+        if (children)
+            return (
+                <Badge color="success" icon={<UserIcon />}>
+                    {children}
+                </Badge>
+            );
+        return null;
+    }

    return (
        <HtmlTooltip title={<RoleDescription roleId={roleId} tooltip />} arrow>
--- a/frontend/src/component/common/RoleSelect/RoleSelect.tsx
+++ b/frontend/src/component/common/RoleSelect/RoleSelect.tsx
@ -4,11 +4,9 @@ import {
    TextField,
    styled,
 } from '@mui/material';
-import { useRoles } from 'hooks/api/getters/useRoles/useRoles';
-import { IRole, PredefinedRoleType } from 'interfaces/role';
+import { IRole } from 'interfaces/role';
 import { RoleDescription } from '../RoleDescription/RoleDescription';
 import { ConditionallyRender } from '../ConditionallyRender/ConditionallyRender';
-import { ROOT_ROLE_TYPE } from '@server/util/constants';

 const StyledRoleOption = styled('div')(({ theme }) => ({
    display: 'flex',
@ -21,23 +19,19 @@ const StyledRoleOption = styled('div')(({ theme }) => ({

 interface IRoleSelectProps
    extends Partial<AutocompleteProps<IRole, false, false, false>> {
-    type?: PredefinedRoleType;
+    roles: IRole[];
    value: IRole | null;
    setValue: (role: IRole | null) => void;
    required?: boolean;
 }

 export const RoleSelect = ({
-    type = ROOT_ROLE_TYPE,
+    roles,
    value,
    setValue,
    required,
    ...rest
 }: IRoleSelectProps) => {
-    const { roles: rootRoles, projectRoles } = useRoles();
-
-    const roles = type === ROOT_ROLE_TYPE ? rootRoles : projectRoles;
-
    const renderRoleOption = (
        props: React.HTMLAttributes<HTMLLIElement>,
        option: IRole
--- a/frontend/src/component/common/RoleSelect/RoleSelect2.tsx
+++ b/frontend/src/component/common/RoleSelect/RoleSelect2.tsx
@ -1,71 +0,0 @@
-import {
-    Autocomplete,
-    AutocompleteProps,
-    TextField,
-    styled,
-} from '@mui/material';
-import { useRoles } from 'hooks/api/getters/useRoles/useRoles';
-import { IRole, PredefinedRoleType } from 'interfaces/role';
-import { RoleDescription } from '../RoleDescription/RoleDescription';
-import { ConditionallyRender } from '../ConditionallyRender/ConditionallyRender';
-
-const StyledRoleOption = styled('div')(({ theme }) => ({
-    display: 'flex',
-    flexDirection: 'column',
-    '& > span:last-of-type': {
-        fontSize: theme.fontSizes.smallerBody,
-        color: theme.palette.text.secondary,
-    },
-}));
-
-interface IRoleSelectProps
-    extends Partial<AutocompleteProps<IRole, false, false, false>> {
-    value: IRole | null;
-    setValue: (role: IRole | null) => void;
-    roles: IRole[];
-    required?: boolean;
-}
-
-export const RoleSelect = ({
-    value,
-    setValue,
-    required,
-    roles,
-    ...rest
-}: IRoleSelectProps) => {
-    const renderRoleOption = (
-        props: React.HTMLAttributes<HTMLLIElement>,
-        option: IRole
-    ) => (
-        <li {...props}>
-            <StyledRoleOption>
-                <span>{option.name}</span>
-                <span>{option.description}</span>
-            </StyledRoleOption>
-        </li>
-    );
-
-    return (
-        <>
-            <Autocomplete
-                openOnFocus
-                size="small"
-                value={value}
-                onChange={(_, role) => setValue(role || null)}
-                options={roles}
-                renderOption={renderRoleOption}
-                getOptionLabel={option => option.name}
-                renderInput={params => (
-                    <TextField {...params} label="Role" required={required} />
-                )}
-                {...rest}
-            />
-            <ConditionallyRender
-                condition={Boolean(value)}
-                show={() => (
-                    <RoleDescription sx={{ marginTop: 1 }} roleId={value!.id} />
-                )}
-            />
-        </>
-    );
-};
--- a/frontend/src/component/common/Table/cells/RoleCell/RoleCell.tsx
+++ b/frontend/src/component/common/Table/cells/RoleCell/RoleCell.tsx
@ -10,9 +10,9 @@ interface IRoleCellProps {
 }

 export const RoleCell: VFC<IRoleCellProps> = ({ roleId, value }) => {
-    const { isEnterprise, uiConfig } = useUiConfig();
+    const { isEnterprise } = useUiConfig();

-    if (isEnterprise() && uiConfig.flags.customRootRoles) {
+    if (isEnterprise()) {
        return (
            <TextCell>
                <TooltipLink
--- a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx
+++ b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx
@ -35,8 +35,7 @@ import {
 } from 'utils/testIds';
 import { caseInsensitiveSearch } from 'utils/search';
 import { IServiceAccount } from 'interfaces/service-account';
-import { RoleSelect } from 'component/common/RoleSelect/RoleSelect2';
-import { PROJECT_ROLE_TYPE } from '@server/util/constants';
+import { RoleSelect } from 'component/common/RoleSelect/RoleSelect';

 const StyledForm = styled('form')(() => ({
    display: 'flex',
--- a/frontend/src/component/user/Profile/ProfileTab/ProfileTab.tsx
+++ b/frontend/src/component/user/Profile/ProfileTab/ProfileTab.tsx
@ -141,7 +141,9 @@ export const ProfileTab = ({ user }: IProfileTabProps) => {
                                    <Typography variant="body2">
                                        Your root role
                                    </Typography>
-                                    <RoleBadge roleId={profile?.rootRole.id!} />
+                                    <RoleBadge roleId={profile?.rootRole.id!}>
+                                        {profile?.rootRole.name}
+                                    </RoleBadge>
                                </>
                            )}
                        />
--- a/frontend/src/hooks/api/getters/useRole/useRole.ts
+++ b/frontend/src/hooks/api/getters/useRole/useRole.ts
@ -2,7 +2,7 @@ import { SWRConfiguration } from 'swr';
 import { useMemo } from 'react';
 import { formatApiPath } from 'utils/formatPath';
 import handleErrorResponses from '../httpErrorResponseHandler';
-import { IRole, IRoleWithPermissions } from 'interfaces/role';
+import { IRoleWithPermissions } from 'interfaces/role';
 import useUiConfig from '../useUiConfig/useUiConfig';
 import { useConditionalSWR } from '../useConditionalSWR/useConditionalSWR';

@ -27,40 +27,15 @@ export const useRole = (
        options
    );

-    const {
-        data: ossData,
-        error: ossError,
-        mutate: ossMutate,
-    } = useConditionalSWR(
-        Boolean(id) && !isEnterprise(),
-        { rootRoles: [] },
-        formatApiPath(`api/admin/user-admin`),
-        fetcher,
-        options
+    return useMemo(
+        () => ({
+            role: data as IRoleWithPermissions,
+            loading: !error && !data,
+            refetch: () => mutate(),
+            error,
+        }),
+        [data, error, mutate]
    );
-
-    return useMemo(() => {
-        if (!isEnterprise()) {
-            return {
-                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 IRoleWithPermissions,
-                loading: !error && !data,
-                refetch: () => mutate(),
-                error,
-            };
-        }
-    }, [data, error, mutate, ossData, ossError, ossMutate]);
 };

 const fetcher = (path: string) => {
--- a/frontend/src/hooks/api/getters/useRoles/useRoles.ts
+++ b/frontend/src/hooks/api/getters/useRoles/useRoles.ts
@ -6,7 +6,6 @@ import { useConditionalSWR } from '../useConditionalSWR/useConditionalSWR';
 import useUiConfig from '../useUiConfig/useUiConfig';
 import {
    PROJECT_ROLE_TYPES,
-    ROOT_ROLE_TYPE,
    ROOT_ROLE_TYPES,
    PREDEFINED_ROLE_TYPES,
 } from '@server/util/constants';
@ -20,7 +19,7 @@ interface IUseRolesOutput {
 }

 export const useRoles = (): IUseRolesOutput => {
-    const { isEnterprise, uiConfig } = useUiConfig();
+    const { isEnterprise } = useUiConfig();

    const { data, error, mutate } = useConditionalSWR(
        isEnterprise(),
@ -29,48 +28,20 @@ export const useRoles = (): IUseRolesOutput => {
        fetcher
    );

-    const {
-        data: ossData,
-        error: ossError,
-        mutate: ossMutate,
-    } = useConditionalSWR(
-        !isEnterprise(),
-        { rootRoles: [] },
-        formatApiPath(`api/admin/user-admin`),
-        fetcher
+    return useMemo(
+        () => ({
+            roles: (data?.roles
+                .filter(({ type }: IRole) => ROOT_ROLE_TYPES.includes(type))
+                .sort(sortRoles) ?? []) as IRole[],
+            projectRoles: (data?.roles
+                .filter(({ type }: IRole) => PROJECT_ROLE_TYPES.includes(type))
+                .sort(sortRoles) ?? []) as IRole[],
+            loading: !error && !data,
+            refetch: () => mutate(),
+            error,
+        }),
+        [data, error, mutate]
    );
-
-    return useMemo(() => {
-        if (!isEnterprise()) {
-            return {
-                roles: ossData?.rootRoles
-                    .filter(({ type }: IRole) => type === ROOT_ROLE_TYPE)
-                    .sort(sortRoles) as IRole[],
-                projectRoles: [],
-                loading: !ossError && !ossData,
-                refetch: () => ossMutate(),
-                error: ossError,
-            };
-        } else {
-            return {
-                roles: (data?.roles
-                    .filter(({ type }: IRole) =>
-                        uiConfig.flags.customRootRoles
-                            ? ROOT_ROLE_TYPES.includes(type)
-                            : type === ROOT_ROLE_TYPE
-                    )
-                    .sort(sortRoles) ?? []) as IRole[],
-                projectRoles: (data?.roles
-                    .filter(({ type }: IRole) =>
-                        PROJECT_ROLE_TYPES.includes(type)
-                    )
-                    .sort(sortRoles) ?? []) as IRole[],
-                loading: !error && !data,
-                refetch: () => mutate(),
-                error,
-            };
-        }
-    }, [data, error, mutate, ossData, ossError, ossMutate]);
 };

 const fetcher = (path: string) => {