From 159c54ed3711e13f66a333a99f555d7e770bb710 Mon Sep 17 00:00:00 2001 From: olav Date: Wed, 18 May 2022 11:07:19 +0200 Subject: [PATCH] fix: resolve issues around changing a toggle's project (#978) * refactor: show save button before using the dropdown * refactor: simplify FeatureSettingsProject toast message * refactor: fix FeatureProjectSelect filter prop type * refactor: hide change project page for non-enterprise * refactor: derive move targets from projects list instead of from permissions * refactor: align frontend project compat check with backend * refactor: fix useProject object stability * refactor: disable the save button for the current project * refactor: require equal environments when moving toggles * refactor: improve arraysHaveSameItems name --- .../FeatureSettings/FeatureSettings.tsx | 5 +- .../FeatureProjectSelect.tsx | 2 +- .../FeatureSettingsProject.tsx | 110 ++++++------------ .../FeatureSettingsProjectConfirm.styles.ts | 45 +------ .../FeatureSettingsProjectConfirm.tsx | 100 +++++----------- .../api/getters/useProject/useProject.ts | 20 ++-- frontend/src/hooks/useSort.ts | 42 ++++--- .../src/utils/arraysHaveSameItems.test.ts | 17 +++ frontend/src/utils/arraysHaveSameItems.ts | 12 ++ frontend/src/utils/projectFilterGenerator.ts | 10 +- 10 files changed, 135 insertions(+), 228 deletions(-) create mode 100644 frontend/src/utils/arraysHaveSameItems.test.ts create mode 100644 frontend/src/utils/arraysHaveSameItems.ts diff --git a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettings.tsx b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettings.tsx index 53f995bb1c..82b04f0661 100644 --- a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettings.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettings.tsx @@ -6,6 +6,7 @@ import { ConditionallyRender } from 'component/common/ConditionallyRender/Condit import FeatureSettingsProject from './FeatureSettingsProject/FeatureSettingsProject'; import { FeatureSettingsInformation } from './FeatureSettingsInformation/FeatureSettingsInformation'; import { useRequiredPathParam } from 'hooks/useRequiredPathParam'; +import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; const METADATA = 'metadata'; const PROJECT = 'project'; @@ -15,6 +16,7 @@ export const FeatureSettings = () => { const projectId = useRequiredPathParam('projectId'); const featureId = useRequiredPathParam('featureId'); const [settings, setSettings] = useState(METADATA); + const { uiConfig } = useUiConfig(); return ( @@ -36,6 +38,7 @@ export const FeatureSettings = () => { button onClick={() => setSettings(PROJECT)} selected={settings === PROJECT} + hidden={!uiConfig.flags.P} > Project @@ -52,7 +55,7 @@ export const FeatureSettings = () => { } /> } /> diff --git a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureProjectSelect/FeatureProjectSelect.tsx b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureProjectSelect/FeatureProjectSelect.tsx index 4176fa257b..59cdf17916 100644 --- a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureProjectSelect/FeatureProjectSelect.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureProjectSelect/FeatureProjectSelect.tsx @@ -10,7 +10,7 @@ interface IFeatureProjectSelectProps extends Omit { enabled: boolean; value: string; - filter: (project: string) => void; + filter: (projectId: string) => boolean; } const FeatureProjectSelect = ({ diff --git a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProject.tsx b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProject.tsx index 8cc5c65d7b..b4cee80b8f 100644 --- a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProject.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProject.tsx @@ -1,121 +1,79 @@ -import { useContext, useEffect, useState } from 'react'; +import { useContext, useState, useMemo } from 'react'; import { useNavigate } from 'react-router'; import AccessContext from 'contexts/AccessContext'; import useFeatureApi from 'hooks/api/actions/useFeatureApi/useFeatureApi'; import { useFeature } from 'hooks/api/getters/useFeature/useFeature'; import useToast from 'hooks/useToast'; import { MOVE_FEATURE_TOGGLE } from 'component/providers/AccessProvider/permissions'; -import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import PermissionButton from 'component/common/PermissionButton/PermissionButton'; import FeatureProjectSelect from './FeatureProjectSelect/FeatureProjectSelect'; import FeatureSettingsProjectConfirm from './FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm'; -import { IPermission } from 'interfaces/user'; -import { useAuthPermissions } from 'hooks/api/getters/useAuth/useAuthPermissions'; import { formatUnknownError } from 'utils/formatUnknownError'; import { useRequiredPathParam } from 'hooks/useRequiredPathParam'; +import useProjects from 'hooks/api/getters/useProjects/useProjects'; const FeatureSettingsProject = () => { const { hasAccess } = useContext(AccessContext); const projectId = useRequiredPathParam('projectId'); const featureId = useRequiredPathParam('featureId'); const { feature, refetchFeature } = useFeature(projectId, featureId); - const [project, setProject] = useState(feature.project); - const [dirty, setDirty] = useState(false); const [showConfirmDialog, setShowConfirmDialog] = useState(false); - const editable = hasAccess(MOVE_FEATURE_TOGGLE, projectId); - const { permissions = [] } = useAuthPermissions(); const { changeFeatureProject } = useFeatureApi(); const { setToastData, setToastApiError } = useToast(); + const [project, setProject] = useState(projectId); + const { projects } = useProjects(); const navigate = useNavigate(); - useEffect(() => { - if (project !== feature.project) { - setDirty(true); - return; - } - setDirty(false); - /* eslint-disable-next-line react-hooks/exhaustive-deps */ - }, [project]); - - useEffect(() => { - const movableTargets = createMoveTargets(); - - if (!movableTargets[project]) { - setDirty(false); - setProject(projectId); - } - /* eslint-disable-next-line */ - }, [permissions.length]); - - const updateProject = async () => { - const newProject = project; + const onConfirm = async () => { try { - await changeFeatureProject(projectId, featureId, newProject); - refetchFeature(); - setToastData({ - title: 'Updated project', - confetti: true, - type: 'success', - text: 'Successfully updated toggle project.', - }); - setDirty(false); - setShowConfirmDialog(false); - navigate(`/projects/${newProject}/features/${featureId}/settings`, { - replace: true, - }); + if (project) { + await changeFeatureProject(projectId, featureId, project); + refetchFeature(); + setToastData({ title: 'Project changed', type: 'success' }); + setShowConfirmDialog(false); + navigate( + `/projects/${project}/features/${featureId}/settings`, + { replace: true } + ); + } } catch (error: unknown) { setToastApiError(formatUnknownError(error)); } }; - const createMoveTargets = () => { - return permissions.reduce( - (acc: { [key: string]: boolean }, p: IPermission) => { - if (p.project && p.permission === MOVE_FEATURE_TOGGLE) { - acc[p.project] = true; - } - return acc; - }, - {} - ); - }; + const targetProjectIds = useMemo(() => { + return projects + .map(project => project.id) + .filter(projectId => hasAccess(MOVE_FEATURE_TOGGLE, projectId)); + }, [projects, hasAccess]); - const filterProjects = () => { - const validTargets = createMoveTargets(); + if (targetProjectIds.length === 0) { + return null; + } - return (project: string) => { - if (validTargets[project]) { - return project; - } - }; - }; return ( <> - setShowConfirmDialog(true)} - projectId={projectId} - > - Save changes - - } + filter={projectId => targetProjectIds.includes(projectId)} + enabled /> + setShowConfirmDialog(true)} + disabled={project === projectId} + projectId={projectId} + > + Save + setShowConfirmDialog(false)} - onClick={updateProject} + onClick={onConfirm} /> ); diff --git a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.styles.ts b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.styles.ts index 24d24e3ba9..6525f8ecf5 100644 --- a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.styles.ts +++ b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.styles.ts @@ -1,46 +1,9 @@ import { makeStyles } from 'tss-react/mui'; export const useStyles = makeStyles()(theme => ({ - compatability: { - padding: '1rem', - border: `1px solid ${theme.palette.grey[300]}`, - marginTop: '1rem', - display: 'flex', - alignItems: 'center', - }, - iconContainer: { - width: '50px', - height: '50px', - backgroundColor: theme.palette.success.main, - borderRadius: '50%', - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - }, - errorIconContainer: { - width: '50px', - height: '50px', - backgroundColor: theme.palette.error.main, - borderRadius: '50%', - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - }, - topContent: { - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - }, - check: { - fill: '#fff', - width: '30px', - height: '30px', - }, - paragraph: { - marginTop: '1rem', - }, - cloud: { - fill: theme.palette.grey[500], - marginRight: '0.5rem', + container: { + display: 'grid', + gap: theme.spacing(2), + paddingTop: theme.spacing(2), }, })); diff --git a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.tsx b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.tsx index 867d3d34e1..4ab38c3096 100644 --- a/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureSettings/FeatureSettingsProject/FeatureSettingsProjectConfirm/FeatureSettingsProjectConfirm.tsx @@ -1,11 +1,11 @@ -import { List, ListItem } from '@mui/material'; -import { Check, Error, Cloud } from '@mui/icons-material'; -import { useState, useEffect } from 'react'; +import { useMemo } from 'react'; import useProject from 'hooks/api/getters/useProject/useProject'; -import { IFeatureEnvironment, IFeatureToggle } from 'interfaces/featureToggle'; +import { IFeatureToggle } from 'interfaces/featureToggle'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { Dialogue } from 'component/common/Dialogue/Dialogue'; import { useStyles } from './FeatureSettingsProjectConfirm.styles'; +import { arraysHaveSameItems } from 'utils/arraysHaveSameItems'; +import { Alert } from '@mui/material'; interface IFeatureSettingsProjectConfirm { projectId: string; @@ -23,37 +23,18 @@ const FeatureSettingsProjectConfirm = ({ feature, }: IFeatureSettingsProjectConfirm) => { const { project } = useProject(projectId); - const [incompatibleEnvs, setIncompatibleEnvs] = useState([]); const { classes: styles } = useStyles(); - useEffect(() => { - calculateCompatability(); - /* eslint-disable-next-line react-hooks/exhaustive-deps */ - }, [projectId, project.name]); - - const calculateCompatability = () => { - const featureEnvWithStrategies = feature.environments - .filter((env: IFeatureEnvironment) => { - return env.strategies.length > 0; - }) - .map((env: IFeatureEnvironment) => env.name); - - const destinationProjectActiveEnvironments = project.environments; - - let incompatible: string[] = []; - - featureEnvWithStrategies.forEach((env: string) => { - if (destinationProjectActiveEnvironments.indexOf(env) === -1) { - incompatible = [...incompatible, env]; - } - }); - // @ts-expect-error - setIncompatibleEnvs(incompatible); - }; + const hasSameEnvironments: boolean = useMemo(() => { + return arraysHaveSameItems( + feature.environments.map(env => env.name), + project.environments + ); + }, [feature, project]); return ( - Are you sure you want to change the project for this feature - toggle? -
- This feature toggle is 100% compatible with the new - project. -
- -
+
+ + This feature toggle is compatible with the new + project. + +

+ Are you sure you want to change the project for this + toggle? +

} @@ -81,44 +63,16 @@ const FeatureSettingsProjectConfirm = ({ title="Confirm change project" secondaryButtonText="OK" > -
+
+ + Incompatible project environments +

- {' '} - This feature toggle is not compatible with the new - project destination. + In order to move a feature toggle between two + projects, both projects must have the exact same + environments enabled.

-
-
- -
-
-
-
-

- This feature toggle has strategy configuration - in an environment that is not activated in the - target project: -

- - {incompatibleEnvs.map(env => { - return ( - - - {env} - - ); - })} - -
-
-

- In order to move this feature toggle, make sure you - enable the required environments in the target project. -

} /> diff --git a/frontend/src/hooks/api/getters/useProject/useProject.ts b/frontend/src/hooks/api/getters/useProject/useProject.ts index 35675096af..51f9fd1633 100644 --- a/frontend/src/hooks/api/getters/useProject/useProject.ts +++ b/frontend/src/hooks/api/getters/useProject/useProject.ts @@ -1,5 +1,5 @@ -import useSWR, { mutate, SWRConfiguration } from 'swr'; -import { useState, useEffect } from 'react'; +import useSWR, { SWRConfiguration } from 'swr'; +import { useCallback } from 'react'; import { getProjectFetcher } from './getProjectFetcher'; import { IProject } from 'interfaces/project'; @@ -15,22 +15,16 @@ const fallbackProject: IProject = { const useProject = (id: string, options: SWRConfiguration = {}) => { const { KEY, fetcher } = getProjectFetcher(id); + const { data, error, mutate } = useSWR(KEY, fetcher, options); - const { data, error } = useSWR(KEY, fetcher, options); - const [loading, setLoading] = useState(!error && !data); - - const refetch = () => { - mutate(KEY); - }; - - useEffect(() => { - setLoading(!error && !data); - }, [data, error]); + const refetch = useCallback(() => { + mutate(); + }, [mutate]); return { project: data || fallbackProject, + loading: !error && !data, error, - loading, refetch, }; }; diff --git a/frontend/src/hooks/useSort.ts b/frontend/src/hooks/useSort.ts index 8e95f01e09..ff5454e029 100644 --- a/frontend/src/hooks/useSort.ts +++ b/frontend/src/hooks/useSort.ts @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useState, useCallback } from 'react'; import { sortFeaturesByNameAscending, sortFeaturesByNameDescending, @@ -63,25 +63,29 @@ const useSort = () => { return sortFeaturesByStatusDescending(features); }; - const sort = (features: IFeatureToggleListItem[]) => { - switch (sortData.sortKey) { - case 'name': - return handleSortName(features); - case 'last-seen': - return handleSortLastSeen(features); - case 'created': - return handleSortCreatedAt(features); - case 'expired': - case 'report': - return handleSortExpiredAt(features); - case 'status': - return handleSortStatus(features); - default: - return features; - } - }; + const sort = useCallback( + (features: IFeatureToggleListItem[]): IFeatureToggleListItem[] => { + switch (sortData.sortKey) { + case 'name': + return handleSortName(features); + case 'last-seen': + return handleSortLastSeen(features); + case 'created': + return handleSortCreatedAt(features); + case 'expired': + case 'report': + return handleSortExpiredAt(features); + case 'status': + return handleSortStatus(features); + default: + return features; + } + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [sortData] + ); - return [sort, setSortData]; + return [sort, setSortData] as const; }; export default useSort; diff --git a/frontend/src/utils/arraysHaveSameItems.test.ts b/frontend/src/utils/arraysHaveSameItems.test.ts new file mode 100644 index 0000000000..5323eb9932 --- /dev/null +++ b/frontend/src/utils/arraysHaveSameItems.test.ts @@ -0,0 +1,17 @@ +import { arraysHaveSameItems } from 'utils/arraysHaveSameItems'; + +test('arraysHaveSameItems', () => { + expect(arraysHaveSameItems([], [])).toEqual(true); + expect(arraysHaveSameItems([1], [1])).toEqual(true); + expect(arraysHaveSameItems([1], [1, 1])).toEqual(true); + expect(arraysHaveSameItems([1, 1], [1])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [1, 2])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [2, 1])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [2, 2, 1])).toEqual(true); + expect(arraysHaveSameItems([1], [])).toEqual(false); + expect(arraysHaveSameItems([1], [2])).toEqual(false); + expect(arraysHaveSameItems([1, 2], [1, 3])).toEqual(false); + expect(arraysHaveSameItems([1, 2], [1, 2, 3])).toEqual(false); + expect(arraysHaveSameItems([1, 2, 3], [1, 2])).toEqual(false); + expect(arraysHaveSameItems([1, 2, 3], [1, 2, 4])).toEqual(false); +}); diff --git a/frontend/src/utils/arraysHaveSameItems.ts b/frontend/src/utils/arraysHaveSameItems.ts new file mode 100644 index 0000000000..51cde1edef --- /dev/null +++ b/frontend/src/utils/arraysHaveSameItems.ts @@ -0,0 +1,12 @@ +export const arraysHaveSameItems = (a: T[], b: T[]): boolean => { + const setA = new Set(a); + const setB = new Set(b); + + if (setA.size !== setB.size) { + return false; + } + + return [...setA].every(itemA => { + return setB.has(itemA); + }); +}; diff --git a/frontend/src/utils/projectFilterGenerator.ts b/frontend/src/utils/projectFilterGenerator.ts index 68ddcc5aa2..514d2f777d 100644 --- a/frontend/src/utils/projectFilterGenerator.ts +++ b/frontend/src/utils/projectFilterGenerator.ts @@ -1,5 +1,5 @@ -import { ADMIN } from '../component/providers/AccessProvider/permissions'; -import { IPermission } from '../interfaces/user'; +import { ADMIN } from 'component/providers/AccessProvider/permissions'; +import { IPermission } from 'interfaces/user'; type objectIdx = { [key: string]: string; @@ -8,8 +8,9 @@ type objectIdx = { export const projectFilterGenerator = ( permissions: IPermission[] = [], matcherPermission: string -) => { +): ((projectId: string) => boolean) => { let admin = false; + const permissionMap: objectIdx = permissions.reduce( (acc: objectIdx, p: IPermission) => { if (p.permission === ADMIN) { @@ -24,7 +25,8 @@ export const projectFilterGenerator = ( }, {} ); + return (projectId: string) => { - return admin || permissionMap[projectId]; + return admin || Boolean(permissionMap[projectId]); }; };