From 64b8df7ee0ec50c120b82aa8d25da9d4cfa3de49 Mon Sep 17 00:00:00 2001 From: andreas-unleash <104830839+andreas-unleash@users.noreply.github.com> Date: Mon, 10 Oct 2022 15:32:34 +0300 Subject: [PATCH] fixed segments not being copied (#2105) * fixed segments not being copied * fix fmt * bug fix * return segmentId[] when getting a feature strategy * do not return segments if they are not there * Update src/lib/services/feature-toggle-service.ts Co-authored-by: Fredrik Strand Oseberg * fix lint * fix: more explicit column sorting and bug fix * update snapshot * rollback * add segment ids to feature strategies * bug fix Co-authored-by: Fredrik Strand Oseberg --- .../FeatureStrategyEmpty.tsx | 3 + .../CopyStrategyIconMenu.tsx | 6 +- frontend/src/interfaces/strategy.ts | 1 + src/lib/db/environment-store.ts | 5 +- src/lib/db/feature-strategy-store.ts | 5 +- .../spec/create-feature-strategy-schema.ts | 3 + .../spec/public-signup-token-schema.ts | 5 ++ src/lib/routes/admin-api/project/features.ts | 35 ++++++++-- src/lib/routes/public-invite.ts | 3 + src/lib/services/feature-toggle-service.ts | 69 +++++++++++++++---- src/lib/services/segment-service.ts | 1 - .../__snapshots__/openapi.e2e.test.ts.snap | 8 +++ 12 files changed, 123 insertions(+), 21 deletions(-) diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx index a108f84dece..8b372e4c9ff 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyEmpty/FeatureStrategyEmpty.tsx @@ -12,6 +12,8 @@ import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmu import { getFeatureStrategyIcon } from 'utils/strategyNames'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { CopyButton } from './CopyButton/CopyButton'; +import { useSegments } from '../../../../hooks/api/getters/useSegments/useSegments'; +import { IFeatureStrategyPayload } from '../../../../interfaces/strategy'; interface IFeatureStrategyEmptyProps { projectId: string; @@ -65,6 +67,7 @@ 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 e16482a8c16..6207a2e762f 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'; @@ -19,6 +19,7 @@ import useFeatureStrategyApi from 'hooks/api/actions/useFeatureStrategyApi/useFe import useToast from 'hooks/useToast'; import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmutable'; import { formatUnknownError } from 'utils/formatUnknownError'; +import { useSegments } from '../../../../../../../../../../hooks/api/getters/useSegments/useSegments'; interface ICopyStrategyIconMenuProps { environments: IFeatureEnvironment['name'][]; @@ -31,6 +32,8 @@ export const CopyStrategyIconMenu: VFC = ({ }) => { const projectId = useRequiredPathParam('projectId'); const featureId = useRequiredPathParam('featureId'); + const { segments } = useSegments(strategy.id); + const [anchorEl, setAnchorEl] = useState(null); const open = Boolean(anchorEl); const { addStrategyToFeature } = useFeatureStrategyApi(); @@ -48,6 +51,7 @@ export const CopyStrategyIconMenu: VFC = ({ const { id, ...strategyCopy } = { ...strategy, environment: environmentId, + copyOf: strategy.id, }; try { diff --git a/frontend/src/interfaces/strategy.ts b/frontend/src/interfaces/strategy.ts index a9564f1aca8..23c820b9fa2 100644 --- a/frontend/src/interfaces/strategy.ts +++ b/frontend/src/interfaces/strategy.ts @@ -19,6 +19,7 @@ export interface IFeatureStrategyPayload { name?: string; constraints: IConstraint[]; parameters: IFeatureStrategyParameters; + copyOf?: string; } export interface IStrategy { diff --git a/src/lib/db/environment-store.ts b/src/lib/db/environment-store.ts index 25132ea5afc..7902a1a615c 100644 --- a/src/lib/db/environment-store.ts +++ b/src/lib/db/environment-store.ts @@ -101,7 +101,10 @@ export default class EnvironmentStore implements IEnvironmentStore { async getAll(query?: Object): Promise { let qB = this.db(TABLE) .select('*') - .orderBy('sort_order', 'created_at'); + .orderBy([ + { column: 'sort_order', order: 'asc' }, + { column: 'created_at', order: 'asc' }, + ]); if (query) { qB = qB.where(query); } diff --git a/src/lib/db/feature-strategy-store.ts b/src/lib/db/feature-strategy-store.ts index cf4caa605fa..48a4e854a03 100644 --- a/src/lib/db/feature-strategy-store.ts +++ b/src/lib/db/feature-strategy-store.ts @@ -193,7 +193,10 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { feature_name: featureName, environment, }) - .orderBy('sort_order', 'created_at'); + .orderBy([ + { column: 'sort_order', order: 'asc' }, + { column: 'created_at', order: 'asc' }, + ]); stopTimer(); return rows.map(mapRow); } diff --git a/src/lib/openapi/spec/create-feature-strategy-schema.ts b/src/lib/openapi/spec/create-feature-strategy-schema.ts index 40ad81e5662..d6e06257e13 100644 --- a/src/lib/openapi/spec/create-feature-strategy-schema.ts +++ b/src/lib/openapi/spec/create-feature-strategy-schema.ts @@ -19,6 +19,9 @@ export const createFeatureStrategySchema = { $ref: '#/components/schemas/constraintSchema', }, }, + copyOf: { + type: 'string', + }, parameters: { $ref: '#/components/schemas/parametersSchema', }, diff --git a/src/lib/openapi/spec/public-signup-token-schema.ts b/src/lib/openapi/spec/public-signup-token-schema.ts index afc6294dc55..d3cc42c9ea2 100644 --- a/src/lib/openapi/spec/public-signup-token-schema.ts +++ b/src/lib/openapi/spec/public-signup-token-schema.ts @@ -21,6 +21,8 @@ export const publicSignupTokenSchema = { type: 'string', }, url: { + description: + 'The public signup link for the token. Users who follow this link will be taken to a signup page where they can create an Unleash user.', type: 'string', }, name: { @@ -43,12 +45,15 @@ export const publicSignupTokenSchema = { }, users: { type: 'array', + description: 'Array of users that have signed up using the token', items: { $ref: '#/components/schemas/userSchema', }, nullable: true, }, role: { + description: + 'Users who sign up using this token will be given this role.', $ref: '#/components/schemas/roleSchema', }, }, diff --git a/src/lib/routes/admin-api/project/features.ts b/src/lib/routes/admin-api/project/features.ts index bb8c917fc57..29591226599 100644 --- a/src/lib/routes/admin-api/project/features.ts +++ b/src/lib/routes/admin-api/project/features.ts @@ -39,6 +39,7 @@ import { FeatureEnvironmentSchema } from '../../../openapi/spec/feature-environm import { SetStrategySortOrderSchema } from '../../../openapi/spec/set-strategy-sort-order-schema'; import { emptyResponse } from '../../../openapi/util/standard-responses'; +import { SegmentService } from '../../../services/segment-service'; interface FeatureStrategyParams { projectId: string; @@ -68,7 +69,10 @@ const PATH_STRATEGY = `${PATH_STRATEGIES}/:strategyId`; type ProjectFeaturesServices = Pick< IUnleashServices, - 'featureToggleServiceV2' | 'projectHealthService' | 'openApiService' + | 'featureToggleServiceV2' + | 'projectHealthService' + | 'openApiService' + | 'segmentService' >; export default class ProjectFeaturesController extends Controller { @@ -76,15 +80,22 @@ export default class ProjectFeaturesController extends Controller { private openApiService: OpenApiService; + private segmentService: SegmentService; + private readonly logger: Logger; constructor( config: IUnleashConfig, - { featureToggleServiceV2, openApiService }: ProjectFeaturesServices, + { + featureToggleServiceV2, + openApiService, + segmentService, + }: ProjectFeaturesServices, ) { super(config); this.featureService = featureToggleServiceV2; this.openApiService = openApiService; + this.segmentService = segmentService; this.logger = config.getLogger('/admin-api/project/features.ts'); this.route({ @@ -557,13 +568,29 @@ export default class ProjectFeaturesController extends Controller { res: Response, ): Promise { const { projectId, featureName, environment } = req.params; + const { copyOf, ...strategyConfig } = req.body; + const userName = extractUsername(req); const strategy = await this.featureService.createStrategy( - req.body, + strategyConfig, { environment, projectId, featureName }, userName, ); - res.status(200).json(strategy); + + 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, + ); + res.status(200).json(updatedStrategy); } async getFeatureStrategies( diff --git a/src/lib/routes/public-invite.ts b/src/lib/routes/public-invite.ts index d9b0ad65933..463a8e3d1d7 100644 --- a/src/lib/routes/public-invite.ts +++ b/src/lib/routes/public-invite.ts @@ -53,6 +53,7 @@ export class PublicInviteController extends Controller { openApiService.validPath({ tags: ['Public signup tokens'], operationId: 'validatePublicSignupToken', + summary: `Validates a public signup token exists, has not expired and is enabled`, responses: { 200: emptyResponse, ...getStandardResponses(400), @@ -70,6 +71,8 @@ export class PublicInviteController extends Controller { openApiService.validPath({ tags: ['Public signup tokens'], operationId: 'addPublicSignupTokenUser', + summary: + 'Create a user with the "viewer" root role and link them to a signup token', requestBody: createRequestSchema('createInvitedUserSchema'), responses: { 200: createResponseSchema('userSchema'), diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 2afd2a82a60..43c32b98ada 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -45,6 +45,7 @@ import { IFeatureOverview, IFeatureStrategy, IFeatureToggleQuery, + ISegment, IStrategyConfig, IVariant, WeightType, @@ -283,12 +284,14 @@ class FeatureToggleService { featureStrategyToPublic( featureStrategy: IFeatureStrategy, + segments: ISegment[] = [], ): Saved { return { id: featureStrategy.id, name: featureStrategy.strategyName, constraints: featureStrategy.constraints || [], parameters: featureStrategy.parameters, + segments: segments.map((segment) => segment.id) ?? [], }; } @@ -330,7 +333,13 @@ class FeatureToggleService { }); const tags = await this.tagStore.getAllTagsForFeature(featureName); - const strategy = this.featureStrategyToPublic(newFeatureStrategy); + const segments = await this.segmentService.getByStrategy( + newFeatureStrategy.id, + ); + const strategy = this.featureStrategyToPublic( + newFeatureStrategy, + segments, + ); await this.eventStore.store( new FeatureStrategyAddEvent({ project: projectId, @@ -385,10 +394,17 @@ class FeatureToggleService { updates, ); + const segments = await this.segmentService.getByStrategy( + strategy.id, + ); + // Store event! const tags = await this.tagStore.getAllTagsForFeature(featureName); - const data = this.featureStrategyToPublic(strategy); - const preData = this.featureStrategyToPublic(existingStrategy); + const data = this.featureStrategyToPublic(strategy, segments); + const preData = this.featureStrategyToPublic( + existingStrategy, + segments, + ); await this.eventStore.store( new FeatureStrategyUpdateEvent({ project: projectId, @@ -424,8 +440,14 @@ class FeatureToggleService { existingStrategy, ); const tags = await this.tagStore.getAllTagsForFeature(featureName); - const data = this.featureStrategyToPublic(strategy); - const preData = this.featureStrategyToPublic(existingStrategy); + const segments = await this.segmentService.getByStrategy( + strategy.id, + ); + const data = this.featureStrategyToPublic(strategy, segments); + const preData = this.featureStrategyToPublic( + existingStrategy, + segments, + ); await this.eventStore.store( new FeatureStrategyUpdateEvent({ featureName, @@ -488,6 +510,7 @@ class FeatureToggleService { featureName: string, environment: string = DEFAULT_ENV, ): Promise[]> { + this.logger.debug('getStrategiesForEnvironment'); const hasEnv = await this.featureEnvironmentStore.featureHasEnvironment( environment, featureName, @@ -499,13 +522,22 @@ class FeatureToggleService { featureName, environment, ); - return featureStrategies.map((strat) => ({ - id: strat.id, - name: strat.strategyName, - constraints: strat.constraints, - parameters: strat.parameters, - sortOrder: strat.sortOrder, - })); + const result = []; + for (const strat of featureStrategies) { + const segments = + (await this.segmentService.getByStrategy(strat.id)).map( + (segment) => segment.id, + ) ?? []; + result.push({ + id: strat.id, + name: strat.strategyName, + constraints: strat.constraints, + parameters: strat.parameters, + sortOrder: strat.sortOrder, + segments, + }); + } + return result; } throw new NotFoundError( `Feature ${featureName} does not have environment ${environment}`, @@ -727,12 +759,23 @@ class FeatureToggleService { const strategy = await this.featureStrategiesStore.getStrategyById( strategyId, ); - return { + + const segments = await this.segmentService.getByStrategy(strategyId); + let result: Saved = { id: strategy.id, name: strategy.strategyName, constraints: strategy.constraints || [], parameters: strategy.parameters, + segments: [], }; + + if (segments && segments.length > 0) { + result = { + ...result, + segments: segments.map((segment) => segment.id), + }; + } + return result; } async getEnvironmentInfo( diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index d12ae260bbd..e7b35604ab5 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -124,7 +124,6 @@ export class SegmentService { const sourceStrategySegments = await this.getByStrategy( sourceStrategyId, ); - await Promise.all( sourceStrategySegments.map((sourceStrategySegment) => { return this.addToStrategy( 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 cc22c165305..ed07e76605c 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,6 +709,9 @@ exports[`should serve the OpenAPI spec 1`] = ` }, "type": "array", }, + "copyOf": { + "type": "string", + }, "name": { "type": "string", }, @@ -2483,14 +2486,17 @@ exports[`should serve the OpenAPI spec 1`] = ` }, "role": { "$ref": "#/components/schemas/roleSchema", + "description": "Users who sign up using this token will be given this role.", }, "secret": { "type": "string", }, "url": { + "description": "The public signup link for the token. Users who follow this link will be taken to a signup page where they can create an Unleash user.", "type": "string", }, "users": { + "description": "Array of users that have signed up using the token", "items": { "$ref": "#/components/schemas/userSchema", }, @@ -7472,6 +7478,7 @@ If the provided project does not exist, the list of events will be empty.", "description": "The provided resource can not be created or updated because it would conflict with the current state of the resource or with an already existing resource, respectively.", }, }, + "summary": "Create a user with the "viewer" root role and link them to a signup token", "tags": [ "Public signup tokens", ], @@ -7498,6 +7505,7 @@ If the provided project does not exist, the list of events will be empty.", "description": "The request data does not match what we expect.", }, }, + "summary": "Validates a public signup token exists, has not expired and is enabled", "tags": [ "Public signup tokens", ],