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;
2025-10-27 11:02:16 +01:00 · 2023-03-21 20:28:43 +00:00 · 2023-03-21 20:28:43 +00:00 · 27d48b3437
commit 27d48b3437
parent 2147206b49
6 changed files with 287 additions and 8 deletions
--- 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<ISegmentFormPartOneProps> = ({
    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<string>(
+        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<ISegmentFormPartOneProps> = ({
        setSelectedProject(projects.find(({ id }) => id === project) ?? null);
    }, [project, projects]);

+    const loading = loadingProjects && loadingStrategies;
+
    return (
        <StyledForm>
            <StyledContainer>
@ -110,7 +128,8 @@ export const SegmentFormStepOne: React.FC<ISegmentFormPartOneProps> = ({
                <ConditionallyRender
                    condition={
                        Boolean(uiConfig.flags.projectScopedSegments) &&
-                        !projectId
+                        !projectId &&
+                        !loading
                    }
                    show={
                        <>
@ -123,11 +142,18 @@ export const SegmentFormStepOne: React.FC<ISegmentFormPartOneProps> = ({
                                onChange={(_, newValue) => {
                                    setProject(newValue?.id);
                                }}
-                                options={projects}
+                                options={availableProjects}
                                getOptionLabel={option => option.name}
                                renderInput={params => (
                                    <TextField {...params} label="Project" />
                                )}
+                                disabled={projectsUsed.size > 1}
+                            />
+                            <SegmentProjectAlert
+                                projects={projects}
+                                strategies={strategies}
+                                projectsUsed={Array.from(projectsUsed)}
+                                availableProjects={availableProjects}
                            />
                        </>
                    }
--- a/frontend/src/component/segments/SegmentProjectAlert.tsx
+++ 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 = (
+        <StyledUl>
+            {Array.from(projectsUsed).map(projectId => (
+                <li key={projectId}>
+                    <Link to={`/projects/${projectId}`} target="_blank">
+                        {projects.find(({ id }) => id === projectId)?.name ??
+                            projectId}
+                    </Link>
+                    <ul>
+                        {strategies
+                            ?.filter(
+                                strategy => strategy.projectId === projectId
+                            )
+                            .map(strategy => (
+                                <li key={strategy.id}>
+                                    <Link
+                                        to={formatEditStrategyPath(
+                                            strategy.projectId!,
+                                            strategy.featureName!,
+                                            strategy.environment!,
+                                            strategy.id
+                                        )}
+                                        target="_blank"
+                                    >
+                                        {strategy.featureName!}{' '}
+                                        {formatStrategyNameParens(strategy)}
+                                    </Link>
+                                </li>
+                            ))}
+                    </ul>
+                </li>
+            ))}
+        </StyledUl>
+    );
+
+    if (projectsUsed.length > 1) {
+        return (
+            <StyledAlert severity="info">
+                You can't specify a project for this segment because it is used
+                in multiple projects:
+                {projectList}
+            </StyledAlert>
+        );
+    }
+
+    if (availableProjects.length === 1) {
+        return (
+            <StyledAlert severity="info">
+                You can't specify a project other than{' '}
+                <strong>{availableProjects[0].name}</strong> for this segment
+                because it is used here:
+                {projectList}
+            </StyledAlert>
+        );
+    }
+
+    return null;
+};
+
+const formatStrategyNameParens = (strategy: IFeatureStrategy): string => {
+    if (!strategy.strategyName) {
+        return '';
+    }
+
+    return `(${formatStrategyName(strategy.strategyName)})`;
+};
--- 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,
--- 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<Pick<IUser, 'username' | 'email'>>,
    ): Promise<ISegment>;

+    update(
+        id: number,
+        data: UpsertSegmentSchema,
+        user: Partial<Pick<IUser, 'username' | 'email'>>,
+    ): Promise<void>;
+
    delete(id: number, user: IUser): Promise<void>;

    cloneStrategySegments(
--- 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<ISegment, 'id'>,
+    ): Promise<void> {
+        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(', ')}`,
+            );
+        }
+    }
 }
--- 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<ISegment> => {
    });
 };

+const updateSegment = (
+    id: number,
+    postData: UpsertSegmentSchema,
+): Promise<void> => {
+    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}`,
+        );
+    });
 });