Skip to content

Commit

Permalink
fix: use the correct hook for permissions and CR (#3050)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
Gastón Fournier committed Feb 6, 2023
1 parent e7f68ee commit fbd2d99
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 28 deletions.
@@ -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 (
<MenuItem disabled={!hasPermissions} onClick={onClick} {...props}>
<Checkbox checked={checked} />
{environment}
</MenuItem>
);
};
@@ -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': {
Expand Down Expand Up @@ -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<string, boolean>);

const addSelectedEnvironment = (
environment: IFeatureEnvironmentWithCrEnabled
) => {
Expand Down Expand Up @@ -127,25 +114,20 @@ export const PushVariantsButton = ({
{environments
.filter(environment => environment.name !== current)
.map(otherEnvironment => (
<MenuItem
<PermissionCheckboxMenuItem
projectId={projectId}
permission={permission}
environment={otherEnvironment.name}
key={otherEnvironment.name}
disabled={
!hasAccessTo[otherEnvironment.name] ??
false
}
checked={selectedEnvironments.includes(
otherEnvironment
)}
onClick={() =>
toggleSelectedEnvironment(
otherEnvironment
)
}
>
<Checkbox
checked={selectedEnvironments.includes(
otherEnvironment
)}
/>
{otherEnvironment.name}
</MenuItem>
/>
))}
<StyledActions>
<Divider />
Expand Down
15 changes: 15 additions & 0 deletions frontend/src/hooks/useHasAccess.ts
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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,
Expand Down

0 comments on commit fbd2d99

Please sign in to comment.