From 27d48b3437dfd5ca3f4a04ec1eb840463e98d73c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 21 Mar 2023 20:28:43 +0000 Subject: [PATCH] Feat segments project safeguards (#3359) https://linear.app/unleash/issue/2-811/ability-to-move-project-specific-segments ![image](https://user-images.githubusercontent.com/14320932/226572047-19ed225c-13d5-4f2e-b10f-e17a7f7e6ae7.png) Provides some safeguards to project-specific segments when managing them on a global level (segments page): - You can always promote a segment to global; - You can't specify a project if the segment is currently in use in multiple projects; - You can't specify a different project if the segment is currently already in use in a project; --- .../component/segments/SegmentFormStepOne.tsx | 32 ++++- .../segments/SegmentProjectAlert.tsx | 95 ++++++++++++++ .../useStrategiesBySegment.ts | 13 +- src/lib/segments/segment-service-interface.ts | 6 + src/lib/services/segment-service.ts | 26 ++++ src/test/e2e/api/client/segment.e2e.test.ts | 123 ++++++++++++++++++ 6 files changed, 287 insertions(+), 8 deletions(-) create mode 100644 frontend/src/component/segments/SegmentProjectAlert.tsx diff --git a/frontend/src/component/segments/SegmentFormStepOne.tsx b/frontend/src/component/segments/SegmentFormStepOne.tsx index 4c41bdc170..d043720fce 100644 --- a/frontend/src/component/segments/SegmentFormStepOne.tsx +++ b/frontend/src/component/segments/SegmentFormStepOne.tsx @@ -13,6 +13,8 @@ import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig'; import useProjects from 'hooks/api/getters/useProjects/useProjects'; import { useOptionalPathParam } from 'hooks/useOptionalPathParam'; import { GO_BACK } from 'constants/navigate'; +import { useStrategiesBySegment } from 'hooks/api/getters/useStrategiesBySegment/useStrategiesBySegment'; +import { SegmentProjectAlert } from './SegmentProjectAlert'; interface ISegmentFormPartOneProps { name: string; @@ -67,10 +69,24 @@ export const SegmentFormStepOne: React.FC = ({ clearErrors, setCurrentStep, }) => { + const segmentId = useOptionalPathParam('segmentId'); const projectId = useOptionalPathParam('projectId'); const { uiConfig } = useUiConfig(); const navigate = useNavigate(); - const { projects } = useProjects(); + const { projects, loading: loadingProjects } = useProjects(); + + const { strategies, loading: loadingStrategies } = + useStrategiesBySegment(segmentId); + + const projectsUsed = new Set( + strategies.map(({ projectId }) => projectId!).filter(Boolean) + ); + + const availableProjects = projects.filter( + ({ id }) => + !projectsUsed.size || + (projectsUsed.size === 1 && projectsUsed.has(id)) + ); const [selectedProject, setSelectedProject] = React.useState( projects.find(({ id }) => id === project) ?? null @@ -80,6 +96,8 @@ export const SegmentFormStepOne: React.FC = ({ setSelectedProject(projects.find(({ id }) => id === project) ?? null); }, [project, projects]); + const loading = loadingProjects && loadingStrategies; + return ( @@ -110,7 +128,8 @@ export const SegmentFormStepOne: React.FC = ({ @@ -123,11 +142,18 @@ export const SegmentFormStepOne: React.FC = ({ onChange={(_, newValue) => { setProject(newValue?.id); }} - options={projects} + options={availableProjects} getOptionLabel={option => option.name} renderInput={params => ( )} + disabled={projectsUsed.size > 1} + /> + } diff --git a/frontend/src/component/segments/SegmentProjectAlert.tsx b/frontend/src/component/segments/SegmentProjectAlert.tsx new file mode 100644 index 0000000000..c323fdb413 --- /dev/null +++ b/frontend/src/component/segments/SegmentProjectAlert.tsx @@ -0,0 +1,95 @@ +import { Alert, styled } from '@mui/material'; +import { formatEditStrategyPath } from 'component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit'; +import { IProjectCard } from 'interfaces/project'; +import { IFeatureStrategy } from 'interfaces/strategy'; +import { Link } from 'react-router-dom'; +import { formatStrategyName } from 'utils/strategyNames'; + +const StyledUl = styled('ul')(({ theme }) => ({ + listStyle: 'none', + paddingLeft: 0, +})); + +const StyledAlert = styled(Alert)(({ theme }) => ({ + marginTop: theme.spacing(1), +})); + +interface ISegmentProjectAlertProps { + projects: IProjectCard[]; + strategies: IFeatureStrategy[]; + projectsUsed: string[]; + availableProjects: IProjectCard[]; +} + +export const SegmentProjectAlert = ({ + projects, + strategies, + projectsUsed, + availableProjects, +}: ISegmentProjectAlertProps) => { + const projectList = ( + + {Array.from(projectsUsed).map(projectId => ( +
  • + + {projects.find(({ id }) => id === projectId)?.name ?? + projectId} + +
      + {strategies + ?.filter( + strategy => strategy.projectId === projectId + ) + .map(strategy => ( +
    • + + {strategy.featureName!}{' '} + {formatStrategyNameParens(strategy)} + +
      • + ))} +
    +
    • + ))} +     + ); + + if (projectsUsed.length > 1) { + return ( + + You can't specify a project for this segment because it is used + in multiple projects: + {projectList} + + ); + } + + if (availableProjects.length === 1) { + return ( + + You can't specify a project other than{' '} + {availableProjects[0].name} for this segment + because it is used here: + {projectList} + + ); + } + + return null; +}; + +const formatStrategyNameParens = (strategy: IFeatureStrategy): string => { + if (!strategy.strategyName) { + return ''; + } + + return `(${formatStrategyName(strategy.strategyName)})`; +}; diff --git a/frontend/src/hooks/api/getters/useStrategiesBySegment/useStrategiesBySegment.ts b/frontend/src/hooks/api/getters/useStrategiesBySegment/useStrategiesBySegment.ts index 54efbd8d50..7c3a5306b4 100644 --- a/frontend/src/hooks/api/getters/useStrategiesBySegment/useStrategiesBySegment.ts +++ b/frontend/src/hooks/api/getters/useStrategiesBySegment/useStrategiesBySegment.ts @@ -1,28 +1,31 @@ -import useSWR, { mutate } from 'swr'; +import { mutate } from 'swr'; import { useCallback } from 'react'; import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler'; import { IFeatureStrategy } from 'interfaces/strategy'; +import { useConditionalSWR } from '../useConditionalSWR/useConditionalSWR'; export interface IUseStrategiesBySegmentOutput { - strategies?: IFeatureStrategy[]; + strategies: IFeatureStrategy[]; refetchUsedSegments: () => void; loading: boolean; error?: Error; } export const useStrategiesBySegment = ( - id: number + id?: string | number ): IUseStrategiesBySegmentOutput => { const path = formatApiPath(`api/admin/segments/${id}/strategies`); - const { data, error } = useSWR(path, () => fetchUsedSegment(path)); + const { data, error } = useConditionalSWR(id, [], path, () => + fetchUsedSegment(path) + ); const refetchUsedSegments = useCallback(() => { mutate(path).catch(console.warn); }, [path]); return { - strategies: data?.strategies, + strategies: data?.strategies || [], refetchUsedSegments, loading: !error && !data, error, diff --git a/src/lib/segments/segment-service-interface.ts b/src/lib/segments/segment-service-interface.ts index 7fc85c8791..12148e7344 100644 --- a/src/lib/segments/segment-service-interface.ts +++ b/src/lib/segments/segment-service-interface.ts @@ -22,6 +22,12 @@ export interface ISegmentService { user: Partial>, ): Promise; + update( + id: number, + data: UpsertSegmentSchema, + user: Partial>, + ): Promise; + delete(id: number, user: IUser): Promise; cloneStrategySegments( diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index b0241cf1f5..b65e0a2697 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -98,6 +98,8 @@ export class SegmentService implements ISegmentService { await this.validateName(input.name); } + await this.validateSegmentProject(id, input); + const segment = await this.segmentStore.update(id, input); await this.eventStore.store({ @@ -218,4 +220,28 @@ export class SegmentService implements ISegmentService { ); } } + + private async validateSegmentProject( + id: number, + segment: Omit, + ): Promise { + const strategies = + await this.featureStrategiesStore.getStrategiesBySegment(id); + + const projectsUsed = new Set( + strategies.map((strategy) => strategy.projectId), + ); + + if ( + segment.project && + (projectsUsed.size > 1 || + (projectsUsed.size === 1 && !projectsUsed.has(segment.project))) + ) { + throw new BadDataError( + `Invalid project. Segment is being used by strategies in other projects: ${Array.from( + projectsUsed, + ).join(', ')}`, + ); + } + } } diff --git a/src/test/e2e/api/client/segment.e2e.test.ts b/src/test/e2e/api/client/segment.e2e.test.ts index 3a9d89c756..baef701965 100644 --- a/src/test/e2e/api/client/segment.e2e.test.ts +++ b/src/test/e2e/api/client/segment.e2e.test.ts @@ -52,6 +52,15 @@ const createSegment = (postData: UpsertSegmentSchema): Promise => { }); }; +const updateSegment = ( + id: number, + postData: UpsertSegmentSchema, +): Promise => { + return app.services.segmentService.update(id, postData, { + email: 'test@example.com', + }); +}; + const mockStrategy = (segments: number[] = []) => { return { name: randomId(), @@ -436,4 +445,118 @@ describe('project-specific segments', () => { [{ status: 400 }], ); }); + + test(`can't set a different segment project when being used by another project`, async () => { + const segmentName = 'my-segment'; + const project1 = randomId(); + const project2 = randomId(); + await createProjects([project1, project2]); + const segment = await createSegment({ + name: segmentName, + project: project1, + constraints: [], + }); + const strategy = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + await createFeatureToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + [strategy], + project1, + ); + await expect(() => + updateSegment(segment.id, { + ...segment, + project: project2, + }), + ).rejects.toThrow( + `Invalid project. Segment is being used by strategies in other projects: ${project1}`, + ); + }); + + test('can promote a segment project to global even when being used by a specific project', async () => { + const segmentName = 'my-segment'; + const project1 = randomId(); + const project2 = randomId(); + await createProjects([project1, project2]); + const segment = await createSegment({ + name: segmentName, + project: project1, + constraints: [], + }); + const strategy = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + await createFeatureToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + [strategy], + project1, + ); + await expect(() => + updateSegment(segment.id, { + ...segment, + project: '', + }), + ).resolves; + }); + + test(`can't set a specific segment project when being used by multiple projects (global)`, async () => { + const segmentName = 'my-segment'; + const project1 = randomId(); + const project2 = randomId(); + await createProjects([project1, project2]); + const segment = await createSegment({ + name: segmentName, + project: '', + constraints: [], + }); + const strategy = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + const strategy2 = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + await createFeatureToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + [strategy], + project1, + ); + await createFeatureToggle( + { + name: 'second_feature', + description: 'the #2 feature', + }, + [strategy2], + project2, + ); + await expect(() => + updateSegment(segment.id, { + ...segment, + project: project1, + }), + ).rejects.toThrow( + `Invalid project. Segment is being used by strategies in other projects: ${project1}, ${project2}`, + ); + }); });