From fbd2d99d97107be940f308d11bdfde4e9b27fd1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 6 Feb 2023 16:00:58 +0100 Subject: [PATCH] fix: use the correct hook for permissions and CR (#3050) ## About the changes When checking for permissions we have 2 methods, one that is not change request aware and one that is. We were using the one that is not aware of CR while the feature we developed is aware of CR. This PR switches to the correct method and documents the hooks --- .../PermissionCheckboxMenuItem.tsx | 28 ++++++++++++++ .../PushVariantsButton/PushVariantsButton.tsx | 38 +++++-------------- frontend/src/hooks/useHasAccess.ts | 15 ++++++++ 3 files changed, 53 insertions(+), 28 deletions(-) create mode 100644 frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PermissionCheckboxMenuItem.tsx diff --git a/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PermissionCheckboxMenuItem.tsx b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PermissionCheckboxMenuItem.tsx new file mode 100644 index 0000000000..0763b13273 --- /dev/null +++ b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PermissionCheckboxMenuItem.tsx @@ -0,0 +1,28 @@ +import { FC } from 'react'; +import { useHasProjectEnvironmentAccess } from 'hooks/useHasAccess'; +import { Checkbox, MenuItem } from '@mui/material'; + +interface PermissionCheckboxMenuItemProps { + permission: string | string[]; + projectId: string; + environment: string; + checked: boolean; + onClick: () => void; +} + +export const PermissionCheckboxMenuItem: FC< + PermissionCheckboxMenuItemProps +> = ({ permission, projectId, environment, checked, onClick, ...props }) => { + const hasPermissions = useHasProjectEnvironmentAccess( + permission, + projectId, + environment + ); + + return ( + + + {environment} + + ); +}; diff --git a/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx index 98f2d363e6..6d867f3c92 100644 --- a/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx @@ -1,15 +1,8 @@ -import { - Button, - Checkbox, - Divider, - Menu, - MenuItem, - styled, -} from '@mui/material'; +import { Button, Divider, Menu, styled } from '@mui/material'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { IFeatureEnvironmentWithCrEnabled } from 'interfaces/featureToggle'; import { useState } from 'react'; -import { useCheckProjectAccess } from 'hooks/useHasAccess'; +import { PermissionCheckboxMenuItem } from './PermissionCheckboxMenuItem'; const StyledMenu = styled(Menu)(({ theme }) => ({ '& > div > ul': { @@ -54,12 +47,6 @@ export const PushVariantsButton = ({ IFeatureEnvironmentWithCrEnabled[] >([]); - const hasAccess = useCheckProjectAccess(projectId); - const hasAccessTo = environments.reduce((acc, env) => { - acc[env.name] = hasAccess(permission, env.name); - return acc; - }, {} as Record); - const addSelectedEnvironment = ( environment: IFeatureEnvironmentWithCrEnabled ) => { @@ -127,25 +114,20 @@ export const PushVariantsButton = ({ {environments .filter(environment => environment.name !== current) .map(otherEnvironment => ( - toggleSelectedEnvironment( otherEnvironment ) } - > - - {otherEnvironment.name} - + /> ))} diff --git a/frontend/src/hooks/useHasAccess.ts b/frontend/src/hooks/useHasAccess.ts index 6999d7dde1..1a802c0ea8 100644 --- a/frontend/src/hooks/useHasAccess.ts +++ b/frontend/src/hooks/useHasAccess.ts @@ -6,8 +6,13 @@ import { UPDATE_FEATURE_STRATEGY, DELETE_FEATURE_STRATEGY, UPDATE_FEATURE_ENVIRONMENT, + UPDATE_FEATURE_ENVIRONMENT_VARIANTS, } from '../component/providers/AccessProvider/permissions'; +/** + * This is for features not integrated with change request. + * If the feature is integrated with change request, use useCheckProjectAccess instead. + */ const useCheckProjectPermissions = (projectId?: string) => { const { hasAccess } = useContext(AccessContext); @@ -44,6 +49,11 @@ const useCheckProjectPermissions = (projectId?: string) => { }; }; +/** + * This is for features integrated with change request. + * If the feature is not integrated with change request, use useCheckProjectPermissions instead. + * When change request is enabled, the user will have access to the feature because permissions will be checked later + */ export const useCheckProjectAccess = (projectId: string) => { const { isChangeRequestConfigured } = useChangeRequestsEnabled(projectId); const checkAccess = useCheckProjectPermissions(projectId); @@ -61,12 +71,17 @@ const ALLOWED_CHANGE_REQUEST_PERMISSIONS = [ UPDATE_FEATURE_STRATEGY, DELETE_FEATURE_STRATEGY, UPDATE_FEATURE_ENVIRONMENT, + UPDATE_FEATURE_ENVIRONMENT_VARIANTS, ]; const intersect = (array1: string[], array2: string[]) => { return array1.filter(value => array2.includes(value)).length > 0; }; +/** + * This methods does the same as useCheckProjectAccess but also checks if the permission is in ALLOWED_CHANGE_REQUEST_PERMISSIONS + * If you know what you're doing you can skip that check and call useCheckProjectAccess + */ export const useHasProjectEnvironmentAccess = ( permission: string | string[], projectId: string,