diff --git a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx b/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx index fc3efe8e61..1b94501796 100644 --- a/frontend/src/component/common/StrategyItemContainer/StrategyItemContainer.tsx +++ b/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; onDragEnd?: DragEventHandler; actions?: ReactNode; diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx index 39dc5cdb4a..80954a24cf 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx +++ b/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)) { diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx index 083a849646..3c8ab0a8e0 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx +++ b/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(); - } + + 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 + strategy: Partial, + segments: ISegment[] ): IFeatureStrategyPayload => ({ name: strategy.name, constraints: strategy.constraints ?? [], parameters: strategy.parameters ?? {}, + segments: segments.map(segment => segment.id), }); export const formatFeaturePath = ( diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx index e0277a9fe0..c37c4a1106 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx +++ b/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( diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/CopyStrategyIconMenu/CopyStrategyIconMenu.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/CopyStrategyIconMenu/CopyStrategyIconMenu.tsx index 0be3774aee..19fb28ffe5 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/CopyStrategyIconMenu/CopyStrategyIconMenu.tsx +++ b/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 = ({ @@ -60,15 +60,14 @@ export const CopyStrategyIconMenu: VFC = ({ onChangeRequestAddStrategyConfirm, } = useChangeRequestAddStrategy(projectId, featureId, 'addStrategy'); - const onCopyStrategy = async (environment: string) => { + const onCopyStrategy = async (targetEnvironment: string) => { const { id, ...strategyCopy } = { ...strategy, - environment, - copyOf: strategy.id, + targetEnvironment, }; - if (isChangeRequestConfigured(environmentId)) { + if (isChangeRequestConfigured(targetEnvironment)) { await onChangeRequestAddStrategy( - environment, + targetEnvironment, { id, ...strategyCopy, @@ -82,14 +81,14 @@ export const CopyStrategyIconMenu: VFC = ({ 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) { diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/FeatureStrategyItem.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/FeatureStrategyItem.tsx index 4d2abc390f..1cd32c3aa5 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/FeatureStrategyItem.tsx +++ b/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; diff --git a/frontend/src/interfaces/strategy.ts b/frontend/src/interfaces/strategy.ts index cd36b82393..6d8f3428cc 100644 --- a/frontend/src/interfaces/strategy.ts +++ b/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 { diff --git a/src/lib/db/feature-strategy-store.ts b/src/lib/db/feature-strategy-store.ts index 0fb6f88326..3e61a3d221 100644 --- a/src/lib/db/feature-strategy-store.ts +++ b/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,9 +298,17 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { env.strategies = []; } if (r.strategy_id) { - env.strategies.push( - FeatureStrategiesStore.getAdminStrategy(r), + 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, + row: Record, + ) { + 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, diff --git a/src/lib/openapi/spec/create-feature-strategy-schema.ts b/src/lib/openapi/spec/create-feature-strategy-schema.ts index d6e06257e1..40ad81e566 100644 --- a/src/lib/openapi/spec/create-feature-strategy-schema.ts +++ b/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', }, diff --git a/src/lib/routes/admin-api/project/features.ts b/src/lib/routes/admin-api/project/features.ts index ef1ee12929..31c5ba4eae 100644 --- a/src/lib/routes/admin-api/project/features.ts +++ b/src/lib/routes/admin-api/project/features.ts @@ -598,7 +598,11 @@ export default class ProjectFeaturesController extends Controller { res: Response, ): Promise { 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 { 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, diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index e558b18e98..2a14ec4a70 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/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, ); diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index e7b35604ab..bb6819f4d7 100644 --- a/src/lib/services/segment-service.ts +++ b/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 { + 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 { await this.segmentStore.removeFromStrategy(id, strategyId); diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index 33572b2a56..120fc786b6 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -36,6 +36,7 @@ export interface IFeatureStrategy { sortOrder?: number; constraints: IConstraint[]; createdAt?: Date; + segments?: number[]; } export interface FeatureToggleDTO { diff --git a/src/test/e2e/api/admin/project/features.e2e.test.ts b/src/test/e2e/api/admin/project/features.e2e.test.ts index b5b8613654..7767679c2c 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/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, + ]); + }); +}); diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 4d2a55dfd7..f4573509a6 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/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", },