feat: update strategy segments with edit / create strategy (#2420)

* Refactors how we add / edit segments to make it more ergonomic to work with in regards to change requests

frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx

@@ -10,9 +10,10 @@ import {
 import StringTruncator from 'component/common/StringTruncator/StringTruncator';
 import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
 import { useStyles } from './StrategyItemContainer.styles';
+import { PlaygroundStrategySchema } from 'component/playground/Playground/interfaces/playground.model';
 
 interface IStrategyItemContainerProps {
-    strategy: IFeatureStrategy;
+    strategy: IFeatureStrategy | PlaygroundStrategySchema;
     onDragStart?: DragEventHandler<HTMLButtonElement>;
     onDragEnd?: DragEventHandler<HTMLButtonElement>;
     actions?: ReactNode;

frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx

@@ -90,14 +90,7 @@ export const FeatureStrategyCreate = () => {
             environmentId,
             payload
         );
-        if (uiConfig.flags.SE) {
+
-            await setStrategySegments({
-                environmentId,
-                projectId,
-                strategyId: created.id,
-                segmentIds: segments.map(s => s.id),
-            });
-        }
         setToastData({
             title: 'Strategy created',
             type: 'success',
@@ -121,7 +114,7 @@ export const FeatureStrategyCreate = () => {
     };
 
     const onSubmit = async () => {
-        const payload = createStrategyPayload(strategy);
+        const payload = createStrategyPayload(strategy, segments);
 
         try {
             if (isChangeRequestConfigured(environmentId)) {

frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx

@@ -101,15 +101,8 @@ export const FeatureStrategyEdit = () => {
             strategyId,
             payload
         );
-        if (uiConfig.flags.SE) {
+
-            await setStrategySegments({
-                environmentId,
-                projectId,
-                strategyId,
-                segmentIds: segments.map(s => s.id),
-            });
         await refetchSavedStrategySegments();
-        }
         setToastData({
             title: 'Strategy updated',
             type: 'success',
@@ -133,7 +126,8 @@ export const FeatureStrategyEdit = () => {
     };
 
     const onSubmit = async () => {
-        const payload = createStrategyPayload(strategy);
+        const segmentsToSubmit = uiConfig?.flags.SE ? segments : [];
+        const payload = createStrategyPayload(strategy, segmentsToSubmit);
 
         try {
             if (isChangeRequestConfigured(environmentId)) {
@@ -191,11 +185,13 @@ export const FeatureStrategyEdit = () => {
 };
 
 export const createStrategyPayload = (
-    strategy: Partial<IFeatureStrategy>
+    strategy: Partial<IFeatureStrategy>,
+    segments: ISegment[]
 ): IFeatureStrategyPayload => ({
     name: strategy.name,
     constraints: strategy.constraints ?? [],
     parameters: strategy.parameters ?? {},
+    segments: segments.map(segment => segment.id),
 });
 
 export const formatFeaturePath = (

frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx

@@ -86,7 +86,6 @@ export const FeatureStrategyEmpty = ({
                     const { id, ...strategyCopy } = {
                         ...strategy,
                         environment: environmentId,
-                        copyOf: strategy.id,
                     };
 
                     return addStrategyToFeature(

frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/CopyStrategyIconMenu/CopyStrategyIconMenu.tsx

@@ -8,7 +8,7 @@ import {
     Tooltip,
 } from '@mui/material';
 import { AddToPhotos as CopyIcon, Lock } from '@mui/icons-material';
-import { IFeatureStrategy } from 'interfaces/strategy';
+import { IFeatureStrategy, IFeatureStrategyPayload } from 'interfaces/strategy';
 import { useRequiredPathParam } from 'hooks/useRequiredPathParam';
 import { IFeatureEnvironment } from 'interfaces/featureToggle';
 import AccessContext from 'contexts/AccessContext';
@@ -27,7 +27,7 @@ import { useChangeRequestsEnabled } from 'hooks/useChangeRequestsEnabled';
 interface ICopyStrategyIconMenuProps {
     environmentId: string;
     environments: IFeatureEnvironment['name'][];
-    strategy: IFeatureStrategy;
+    strategy: IFeatureStrategyPayload;
 }
 
 export const CopyStrategyIconMenu: VFC<ICopyStrategyIconMenuProps> = ({
@@ -60,15 +60,14 @@ export const CopyStrategyIconMenu: VFC<ICopyStrategyIconMenuProps> = ({
         onChangeRequestAddStrategyConfirm,
     } = useChangeRequestAddStrategy(projectId, featureId, 'addStrategy');
 
-    const onCopyStrategy = async (environment: string) => {
+    const onCopyStrategy = async (targetEnvironment: string) => {
         const { id, ...strategyCopy } = {
             ...strategy,
-            environment,
+            targetEnvironment,
-            copyOf: strategy.id,
         };
-        if (isChangeRequestConfigured(environmentId)) {
+        if (isChangeRequestConfigured(targetEnvironment)) {
             await onChangeRequestAddStrategy(
-                environment,
+                targetEnvironment,
                 {
                     id,
                     ...strategyCopy,
@@ -82,14 +81,14 @@ export const CopyStrategyIconMenu: VFC<ICopyStrategyIconMenuProps> = ({
             await addStrategyToFeature(
                 projectId,
                 featureId,
-                environment,
+                targetEnvironment,
                 strategy
             );
             refetchFeature();
             refetchFeatureImmutable();
             setToastData({
                 title: `Strategy created`,
-                text: `Successfully copied a strategy to ${environment}`,
+                text: `Successfully copied a strategy to ${targetEnvironment}`,
                 type: 'success',
             });
         } catch (error) {

frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/FeatureStrategyItem.tsx

@@ -8,6 +8,7 @@ import { StrategyExecution } from './StrategyExecution/StrategyExecution';
 import { useStyles } from './FeatureStrategyItem.styles';
 import { StrategyItemContainer } from 'component/common/StrategyItemContainer/StrategyItemContainer';
 import { objectId } from 'utils/objectId';
+import { IFeatureStrategy } from 'interfaces/strategy';
 
 interface IFeatureStrategyItemProps {
     strategy: PlaygroundStrategySchema;

frontend/src/interfaces/strategy.ts

@@ -9,6 +9,7 @@ export interface IFeatureStrategy {
     featureName?: string;
     projectId?: string;
     environment?: string;
+    segments?: number[];
 }
 
 export interface IFeatureStrategyParameters {
@@ -20,7 +21,7 @@ export interface IFeatureStrategyPayload {
     name?: string;
     constraints: IConstraint[];
     parameters: IFeatureStrategyParameters;
-    copyOf?: string;
+    segments?: number[];
 }
 
 export interface IStrategy {

src/lib/db/feature-strategy-store.ts

@@ -11,11 +11,12 @@ import {
     IEnvironmentOverview,
     IFeatureOverview,
     IFeatureStrategy,
+    IFeatureToggleClient,
     IStrategyConfig,
     ITag,
 } from '../types/model';
 import { IFeatureStrategiesStore } from '../types/stores/feature-strategies-store';
-import { PartialSome } from '../types/partial';
+import { PartialDeep, PartialSome } from '../types/partial';
 import FeatureToggleStore from './feature-toggle-store';
 import { ensureStringValue } from '../util/ensureStringValue';
 import { mapValues } from '../util/map-values';
@@ -237,6 +238,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                 'feature_strategies.parameters as parameters',
                 'feature_strategies.constraints as constraints',
                 'feature_strategies.sort_order as sort_order',
+                'fss.segment_id as segments',
             )
             .leftJoin(
                 'feature_environments',
@@ -259,6 +261,11 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                 'feature_environments.environment',
                 'environments.name',
             )
+            .leftJoin(
+                'feature_strategy_segment as fss',
+                `fss.feature_strategy_id`,
+                `feature_strategies.id`,
+            )
             .where('features.name', featureName)
             .modify(FeatureToggleStore.filterByArchived, archived);
         stopTimer();
@@ -267,6 +274,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                 if (acc.environments === undefined) {
                     acc.environments = {};
                 }
+
                 acc.name = r.name;
                 acc.impressionData = r.impression_data;
                 acc.description = r.description;
@@ -281,6 +289,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                         name: r.environment,
                     };
                 }
+
                 const env = acc.environments[r.environment];
                 env.enabled = r.enabled;
                 env.type = r.environment_type;
@@ -289,10 +298,18 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                     env.strategies = [];
                 }
                 if (r.strategy_id) {
+                    const found = env.strategies.find(
+                        (strategy) => strategy.id === r.strategy_id,
+                    );
+                    if (!found) {
                         env.strategies.push(
                             FeatureStrategiesStore.getAdminStrategy(r),
                         );
                     }
+                }
+                if (r.segments) {
+                    this.addSegmentIdsToStrategy(env, r);
+                }
                 acc.environments[r.environment] = env;
                 return acc;
             }, {});
@@ -318,6 +335,22 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
         );
     }
 
+    private addSegmentIdsToStrategy(
+        feature: PartialDeep<IFeatureToggleClient>,
+        row: Record<string, any>,
+    ) {
+        const strategy = feature.strategies.find(
+            (s) => s.id === row.strategy_id,
+        );
+        if (!strategy) {
+            return;
+        }
+        if (!strategy.segments) {
+            strategy.segments = [];
+        }
+        strategy.segments.push(row.segments);
+    }
+
     private static getEnvironment(r: any): IEnvironmentOverview {
         return {
             name: r.environment,

src/lib/openapi/spec/create-feature-strategy-schema.ts

@@ -19,9 +19,6 @@ export const createFeatureStrategySchema = {
                 $ref: '#/components/schemas/constraintSchema',
             },
         },
-        copyOf: {
-            type: 'string',
-        },
         parameters: {
             $ref: '#/components/schemas/parametersSchema',
         },

src/lib/routes/admin-api/project/features.ts

@@ -598,7 +598,11 @@ export default class ProjectFeaturesController extends Controller {
         res: Response<FeatureStrategySchema>,
     ): Promise<void> {
         const { projectId, featureName, environment } = req.params;
-        const { copyOf, ...strategyConfig } = req.body;
+        const { ...strategyConfig } = req.body;
+
+        if (!strategyConfig.segmentIds) {
+            strategyConfig.segmentIds = [];
+        }
 
         const userName = extractUsername(req);
         const strategy = await this.featureService.createStrategy(
@@ -607,16 +611,6 @@ export default class ProjectFeaturesController extends Controller {
             userName,
         );
 
-        if (copyOf) {
-            this.logger.info(
-                `Cloning segments from: strategyId=${copyOf} to: strategyId=${strategy.id} `,
-            );
-            await this.segmentService.cloneStrategySegments(
-                copyOf,
-                strategy.id,
-            );
-        }
-
         const updatedStrategy = await this.featureService.getStrategy(
             strategy.id,
         );
@@ -661,6 +655,11 @@ export default class ProjectFeaturesController extends Controller {
     ): Promise<void> {
         const { strategyId, environment, projectId, featureName } = req.params;
         const userName = extractUsername(req);
+
+        if (!req.body.segmentIds) {
+            req.body.segmentIds = [];
+        }
+
         const updatedStrategy = await this.featureService.updateStrategy(
             strategyId,
             req.body,

src/lib/services/feature-toggle-service.ts

@@ -332,6 +332,16 @@ class FeatureToggleService {
                     environment,
                 });
 
+            if (
+                strategyConfig.segments &&
+                Array.isArray(strategyConfig.segments)
+            ) {
+                await this.segmentService.updateStrategySegments(
+                    newFeatureStrategy.id,
+                    strategyConfig.segments,
+                );
+            }
+
             const tags = await this.tagStore.getAllTagsForFeature(featureName);
             const segments = await this.segmentService.getByStrategy(
                 newFeatureStrategy.id,
@@ -394,6 +404,13 @@ class FeatureToggleService {
                 updates,
             );
 
+            if (updates.segments && Array.isArray(updates.segments)) {
+                await this.segmentService.updateStrategySegments(
+                    strategy.id,
+                    updates.segments,
+                );
+            }
+
             const segments = await this.segmentService.getByStrategy(
                 strategy.id,
             );

src/lib/services/segment-service.ts

@@ -140,6 +140,40 @@ export class SegmentService {
         await this.segmentStore.addToStrategy(id, strategyId);
     }
 
+    async updateStrategySegments(
+        strategyId: string,
+        segmentIds: number[],
+    ): Promise<void> {
+        if (segmentIds.length > this.config.strategySegmentsLimit) {
+            throw new BadDataError(
+                `Strategies may not have more than ${this.config.strategySegmentsLimit} segments`,
+            );
+        }
+
+        const segments = await this.getByStrategy(strategyId);
+        const currentSegmentIds = segments.map((segment) => segment.id);
+
+        const segmentIdsToRemove = currentSegmentIds.filter(
+            (id) => !segmentIds.includes(id),
+        );
+
+        await Promise.all(
+            segmentIdsToRemove.map((segmentId) =>
+                this.removeFromStrategy(segmentId, strategyId),
+            ),
+        );
+
+        const segmentIdsToAdd = segmentIds.filter(
+            (id) => !currentSegmentIds.includes(id),
+        );
+
+        await Promise.all(
+            segmentIdsToAdd.map((segmentId) =>
+                this.addToStrategy(segmentId, strategyId),
+            ),
+        );
+    }
+
     // Used by unleash-enterprise.
     async removeFromStrategy(id: number, strategyId: string): Promise<void> {
         await this.segmentStore.removeFromStrategy(id, strategyId);

src/lib/types/model.ts

@@ -36,6 +36,7 @@ export interface IFeatureStrategy {
     sortOrder?: number;
     constraints: IConstraint[];
     createdAt?: Date;
+    segments?: number[];
 }
 
 export interface FeatureToggleDTO {

src/test/e2e/api/admin/project/features.e2e.test.ts

@@ -13,7 +13,12 @@ import {
 import ApiUser from '../../../../../lib/types/api-user';
 import { ApiTokenType } from '../../../../../lib/types/models/api-token';
 import IncompatibleProjectError from '../../../../../lib/error/incompatible-project-error';
-import { IVariant, RoleName, WeightType } from '../../../../../lib/types/model';
+import {
+    IStrategyConfig,
+    IVariant,
+    RoleName,
+    WeightType,
+} from '../../../../../lib/types/model';
 import { v4 as uuidv4 } from 'uuid';
 import supertest from 'supertest';
 import { randomId } from '../../../../../lib/util/random-id';
@@ -24,6 +29,60 @@ const sortOrderFirst = 0;
 const sortOrderSecond = 10;
 const sortOrderDefault = 9999;
 
+const createFeatureToggle = (featureName: string, project = 'default') => {
+    return app.request.post(`/api/admin/projects/${project}/features`).send({
+        name: featureName,
+    });
+};
+
+const createSegment = async (segmentName: string) => {
+    const segment = await app.services.segmentService.create(
+        {
+            name: segmentName,
+            description: '',
+            constraints: [
+                {
+                    contextName: 'appName',
+                    operator: 'IN',
+                    values: ['test'],
+                    caseInsensitive: false,
+                    inverted: false,
+                },
+            ],
+        },
+        { username: 'testuser', email: 'test@test.com' },
+    );
+
+    return segment;
+};
+
+const createStrategy = async (
+    featureName: string,
+    payload: IStrategyConfig,
+) => {
+    return app.request
+        .post(
+            `/api/admin/projects/default/features/${featureName}/environments/default/strategies`,
+        )
+        .send(payload)
+        .expect(200);
+};
+
+const updateStrategy = async (
+    featureName: string,
+    strategyId: string,
+    payload: IStrategyConfig,
+) => {
+    const { body } = await app.request
+        .put(
+            `/api/admin/projects/default/features/${featureName}/environments/default/strategies/${strategyId}`,
+        )
+        .send(payload)
+        .expect(200);
+
+    return body;
+};
+
 beforeAll(async () => {
     db = await dbInit('feature_strategy_api_serial', getLogger);
     app = await setupApp(db.stores);
@@ -2570,3 +2629,84 @@ test('should return strategies in correct order when new strategies are added',
     expect(strategiesReOrdered[3].sortOrder).toBe(9999);
     expect(strategiesReOrdered[3].id).toBe(strategyThree.id);
 });
+
+test.only('should create a strategy with segments', async () => {
+    const feature = { name: uuidv4(), impressionData: false };
+    await createFeatureToggle(feature.name);
+    const segment = await createSegment('segmentOne');
+    const { body: strategyOne } = await createStrategy(feature.name, {
+        name: 'default',
+        parameters: {
+            userId: 'string',
+        },
+        segments: [segment.id],
+    });
+
+    // Can get the strategy with segment ids
+    await app.request
+        .get(`/api/admin/projects/default/features/${feature.name}`)
+        .expect((res) => {
+            const defaultEnv = res.body.environments.find(
+                (env) => env.name === 'default',
+            );
+            const strategy = defaultEnv.strategies.find(
+                (strat) => strat.id === strategyOne.id,
+            );
+
+            expect(strategy.segments).toEqual([segment.id]);
+        });
+
+    await updateStrategy(feature.name, strategyOne.id, {
+        name: 'default',
+        parameters: {
+            userId: 'string',
+        },
+        segments: [],
+    });
+
+    await app.request
+        .get(`/api/admin/projects/default/features/${feature.name}`)
+        .expect((res) => {
+            const defaultEnv = res.body.environments.find(
+                (env) => env.name === 'default',
+            );
+            const strategy = defaultEnv.strategies.find(
+                (strat) => strat.id === strategyOne.id,
+            );
+
+            expect(strategy.segments).toBe(undefined);
+        });
+});
+
+test.only('should add multiple segments to a strategy', async () => {
+    const feature = { name: uuidv4(), impressionData: false };
+    await createFeatureToggle(feature.name);
+    const segment = await createSegment('seg1');
+    const segmentTwo = await createSegment('seg2');
+    const segmentThree = await createSegment('seg3');
+    const { body: strategyOne } = await createStrategy(feature.name, {
+        name: 'default',
+        parameters: {
+            userId: 'string',
+        },
+        segments: [segment.id, segmentTwo.id, segmentThree.id],
+    });
+
+    // Can get the strategy with segment ids
+    await app.request
+        .get(`/api/admin/projects/default/features/${feature.name}`)
+        .expect((res) => {
+            const defaultEnv = res.body.environments.find(
+                (env) => env.name === 'default',
+            );
+            const strategy = defaultEnv.strategies.find(
+                (strat) => strat.id === strategyOne.id,
+            );
+
+            expect(strategy.segments).toEqual([
+                segment.id,
+                segmentTwo.id,
+                segmentThree.id,
+            ]);
+        });
+});

src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap

@@ -709,9 +709,6 @@ exports[`should serve the OpenAPI spec 1`] = `
             },
             "type": "array",
           },
-          "copyOf": {
-            "type": "string",
-          },
           "name": {
             "type": "string",
           },