From 24e9cf7c8f6ed224ef9df7ca335353252e1030bf Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 22 Jun 2023 09:19:35 +0200 Subject: [PATCH] feat: add "edit" link to playground strategies (#4027) This change adds an "edit" link to all playground strategies when they are returned from the API, allowing the user to jump directly to the evaluated strategy edit screen. This change applies to both old and new strategies, so it should even work in the old playground. This does not use this information in the frontend yet. ## Discussion points: Should "links" be an object or a singular string? I know the notifications service uses just "link", but using an object will make it easier to potentially add more actions in the future (such as "enable"/"disable", maybe?) Do we need to supply basePath? I noticed that the notifications links only ever use an empty string for base path, so it might not be necessary and can potentially be left out. ## Changes I've implemented the link building in a new view model file. Inspecting the output after the result is fully ready requires some gnarly introspection and mapping, but it's tested. Further, I've done a little bit of work to stop the playground service using the schema types directly as the schema types now contain extra information. This PR also updates the `urlFriendlyString` arbitrary to not produce strings that contain only periods. This causes issues when parsing URLs (and is also something we struggle with in the UI). --- .../playground/feature-evaluator/client.ts | 9 +- .../features/playground/playground-service.ts | 46 +++++- .../playground/playground-view-model.test.ts | 143 ++++++++++++++++++ .../playground/playground-view-model.ts | 109 +++++++++++++ src/lib/features/playground/playground.ts | 42 ++--- .../spec/playground-feature-schema.test.ts | 3 + .../spec/playground-strategy-schema.ts | 15 ++ src/test/arbitraries.test.ts | 4 +- .../__snapshots__/openapi.e2e.test.ts.snap | 15 ++ .../e2e/services/playground-service.test.ts | 14 +- 10 files changed, 372 insertions(+), 28 deletions(-) create mode 100644 src/lib/features/playground/playground-view-model.test.ts create mode 100644 src/lib/features/playground/playground-view-model.ts diff --git a/src/lib/features/playground/feature-evaluator/client.ts b/src/lib/features/playground/feature-evaluator/client.ts index ab9e9477a2..b070bbab04 100644 --- a/src/lib/features/playground/feature-evaluator/client.ts +++ b/src/lib/features/playground/feature-evaluator/client.ts @@ -17,9 +17,14 @@ export type StrategyEvaluationResult = Pick< 'result' | 'segments' | 'constraints' >; +export type EvaluatedPlaygroundStrategy = Omit< + PlaygroundStrategySchema, + 'links' +>; + export type FeatureStrategiesEvaluationResult = { result: boolean | typeof playgroundStrategyEvaluation.unknownResult; - strategies: PlaygroundStrategySchema[]; + strategies: EvaluatedPlaygroundStrategy[]; }; export default class UnleashClient { @@ -83,7 +88,7 @@ export default class UnleashClient { } const strategies = feature.strategies.map( - (strategySelector): PlaygroundStrategySchema => { + (strategySelector): EvaluatedPlaygroundStrategy => { const getStrategy = () => { // the application hostname strategy relies on external // variables to calculate its result. As such, we can't diff --git a/src/lib/features/playground/playground-service.ts b/src/lib/features/playground/playground-service.ts index 3e5f76cd90..6a44c83c79 100644 --- a/src/lib/features/playground/playground-service.ts +++ b/src/lib/features/playground/playground-service.ts @@ -7,7 +7,10 @@ import { Logger } from '../../logger'; import { ISegment, IUnleashConfig } from 'lib/types'; import { offlineUnleashClient } from './offline-unleash-client'; import { FeatureInterface } from 'lib/features/playground/feature-evaluator/feature'; -import { FeatureStrategiesEvaluationResult } from 'lib/features/playground/feature-evaluator/client'; +import { + EvaluatedPlaygroundStrategy, + FeatureStrategiesEvaluationResult, +} from 'lib/features/playground/feature-evaluator/client'; import { ISegmentService } from 'lib/segments/segment-service-interface'; import { FeatureConfigurationClient } from '../../types/stores/feature-strategies-store'; import { generateObjectCombinations } from './generateObjectCombinations'; @@ -16,6 +19,7 @@ import { omitKeys } from '../../util'; import { AdvancedPlaygroundFeatureSchema } from '../../openapi/spec/advanced-playground-feature-schema'; import { AdvancedPlaygroundEnvironmentFeatureSchema } from '../../openapi/spec/advanced-playground-environment-feature-schema'; import { validateQueryComplexity } from './validateQueryComplexity'; +import { playgroundStrategyEvaluation } from 'lib/openapi'; type EvaluationInput = { features: FeatureConfigurationClient[]; @@ -25,6 +29,36 @@ type EvaluationInput = { environment: string; }; +export type AdvancedPlaygroundEnvironmentFeatureEvaluationResult = Omit< + AdvancedPlaygroundEnvironmentFeatureSchema, + 'strategies' +> & { + strategies: { + result: boolean | typeof playgroundStrategyEvaluation.unknownResult; + data: EvaluatedPlaygroundStrategy[]; + }; +}; + +export type AdvancedPlaygroundFeatureEvaluationResult = Omit< + AdvancedPlaygroundFeatureSchema, + 'environments' +> & { + environments: Record< + string, + AdvancedPlaygroundEnvironmentFeatureEvaluationResult[] + >; +}; + +export type PlaygroundFeatureEvaluationResult = Omit< + PlaygroundFeatureSchema, + 'strategies' +> & { + strategies: { + result: boolean | typeof playgroundStrategyEvaluation.unknownResult; + data: EvaluatedPlaygroundStrategy[]; + }; +}; + export class PlaygroundService { private readonly logger: Logger; @@ -49,7 +83,7 @@ export class PlaygroundService { environments: string[], context: SdkContextSchema, limit: number, - ): Promise { + ): Promise { const segments = await this.segmentService.getActive(); const environmentFeatures = await Promise.all( environments.map((env) => this.resolveFeatures(projects, env)), @@ -98,7 +132,9 @@ export class PlaygroundService { segments, context, environment, - }: EvaluationInput): Promise { + }: EvaluationInput): Promise< + AdvancedPlaygroundEnvironmentFeatureEvaluationResult[] + > { const [head, ...rest] = features; if (!head) { return []; @@ -121,6 +157,7 @@ export class PlaygroundService { ? new Date(context.currentTime) : undefined, }; + return client .getFeatureToggleDefinitions() .map((feature: FeatureInterface) => { @@ -179,7 +216,7 @@ export class PlaygroundService { projects: typeof ALL | string[], environment: string, context: SdkContextSchema, - ): Promise { + ): Promise { const [{ features, featureProject }, segments] = await Promise.all([ this.resolveFeatures(projects, environment), this.segmentService.getActive(), @@ -192,6 +229,7 @@ export class PlaygroundService { context, environment, }); + return result.map((item) => omitKeys(item, 'environment', 'context')); } } diff --git a/src/lib/features/playground/playground-view-model.test.ts b/src/lib/features/playground/playground-view-model.test.ts new file mode 100644 index 0000000000..3c5c99fc3b --- /dev/null +++ b/src/lib/features/playground/playground-view-model.test.ts @@ -0,0 +1,143 @@ +import { + advancedPlaygroundViewModel, + playgroundViewModel, +} from './playground-view-model'; + +describe('playground result to view model', () => { + it('adds edit links to playground models', () => { + const input = { + environment: 'development', + projects: '*' as '*', + context: { appName: 'playground', userId: '1' }, + }; + const featureResult = { + isEnabled: false, + isEnabledInCurrentEnvironment: false, + strategies: { + result: false, + data: [ + { + name: 'flexibleRollout', + id: 'ea2c22c1-07fc-4cbe-934d-ffff57cd774b', + disabled: false, + parameters: { + groupId: 'test-playground', + rollout: '32', + stickiness: 'default', + }, + result: { + enabled: false, + evaluationStatus: 'complete' as 'complete', + }, + constraints: [], + segments: [], + }, + ], + }, + projectId: 'default', + variant: { + name: 'disabled', + enabled: false, + }, + name: 'test-playground', + variants: [], + }; + + const viewModel = playgroundViewModel(input, [featureResult]); + const transformedStrategy = viewModel.features[0].strategies.data[0]; + + // check that we're adding links correctly + expect(transformedStrategy).toMatchObject({ + links: { + edit: + expect.stringMatching( + `/projects/${featureResult.projectId}/features/${featureResult.name}/strategies/edit?`, + ) && + expect.stringMatching(`environmentId=development`) && + expect.stringMatching( + `strategyId=${transformedStrategy.id}`, + ), + }, + }); + + // check that we're not changing anything else + expect(viewModel).toMatchObject({ input, features: [featureResult] }); + }); + + it('adds edit links to advanced playground models', () => { + const input = { + environments: ['development'], + projects: '*' as '*', + context: { appName: 'playground', userId: '1' }, + }; + + const featureResult = { + name: 'test-playground', + projectId: 'default', + environments: { + development: [ + { + isEnabled: false, + isEnabledInCurrentEnvironment: true, + strategies: { + result: false, + data: [ + { + name: 'flexibleRollout', + id: '2a7dfda6-acf1-4e53-8813-6559e8bd66b0', + disabled: false, + parameters: { + groupId: 'test-playground', + rollout: '50', + stickiness: 'default', + }, + result: { + enabled: false, + evaluationStatus: + 'complete' as 'complete', + }, + constraints: [], + segments: [], + }, + ], + }, + projectId: 'default', + variant: { + name: 'disabled', + enabled: false, + }, + name: 'test-playground', + environment: 'development', + context: { + appName: 'playground', + userId: '1', + }, + variants: [], + }, + ], + }, + }; + + const viewModel = advancedPlaygroundViewModel(input, [featureResult]); + const transformedStrategy = + viewModel.features[0].environments.development[0].strategies + .data[0]; + + // ensure that we're adding the required data + expect(transformedStrategy).toMatchObject({ + links: { + edit: + expect.stringMatching( + `/projects/${featureResult.projectId}/features/${featureResult.name}/strategies/edit?`, + ) && + expect.stringMatching(`environmentId=development`) && + expect.stringMatching( + `strategyId=${transformedStrategy.id}`, + ), + }, + }); + + // check that we're not changing anything else + expect(viewModel).toMatchObject({ input, features: [featureResult] }); + }); +}); diff --git a/src/lib/features/playground/playground-view-model.ts b/src/lib/features/playground/playground-view-model.ts new file mode 100644 index 0000000000..b1c2d56232 --- /dev/null +++ b/src/lib/features/playground/playground-view-model.ts @@ -0,0 +1,109 @@ +import { + AdvancedPlaygroundRequestSchema, + AdvancedPlaygroundResponseSchema, + PlaygroundRequestSchema, + PlaygroundResponseSchema, + PlaygroundStrategySchema, +} from 'lib/openapi'; +import { + AdvancedPlaygroundFeatureEvaluationResult, + PlaygroundFeatureEvaluationResult, +} from './playground-service'; + +const buildStrategyLink = ( + project: string, + feature: string, + environment: string, + strategyId: string, +): string => + `/projects/${project}/features/${feature}/strategies/edit?environmentId=${environment}&strategyId=${strategyId}`; + +const addStrategyEditLink = ( + environmentId: string, + projectId: string, + featureName: string, + strategy: Omit, +): PlaygroundStrategySchema => { + return { + ...strategy, + links: { + edit: buildStrategyLink( + projectId, + featureName, + environmentId, + strategy.id, + ), + }, + }; +}; + +export const advancedPlaygroundViewModel = ( + input: AdvancedPlaygroundRequestSchema, + playgroundResult: AdvancedPlaygroundFeatureEvaluationResult[], +): AdvancedPlaygroundResponseSchema => { + const features = playgroundResult.map(({ environments, ...rest }) => { + const transformedEnvironments = Object.entries(environments).map( + ([envName, envFeatures]) => { + const transformedFeatures = envFeatures.map( + ({ + name, + strategies, + environment, + projectId, + ...featRest + }) => ({ + ...featRest, + name, + environment, + projectId, + strategies: { + ...strategies, + data: strategies.data.map((strategy) => + addStrategyEditLink( + environment, + projectId, + name, + strategy, + ), + ), + }, + }), + ); + return [envName, transformedFeatures]; + }, + ); + + return { + ...rest, + environments: Object.fromEntries(transformedEnvironments), + }; + }); + + return { features, input }; +}; + +export const playgroundViewModel = ( + input: PlaygroundRequestSchema, + playgroundResult: PlaygroundFeatureEvaluationResult[], +): PlaygroundResponseSchema => { + const features = playgroundResult.map( + ({ name, strategies, projectId, ...rest }) => ({ + ...rest, + name, + projectId, + strategies: { + ...strategies, + data: strategies.data.map((strategy) => + addStrategyEditLink( + input.environment, + projectId, + name, + strategy, + ), + ), + }, + }), + ); + + return { input, features }; +}; diff --git a/src/lib/features/playground/playground.ts b/src/lib/features/playground/playground.ts index 122f1c4ec3..564c76f96d 100644 --- a/src/lib/features/playground/playground.ts +++ b/src/lib/features/playground/playground.ts @@ -17,6 +17,10 @@ import { PlaygroundService } from './playground-service'; import { IFlagResolver } from '../../types'; import { AdvancedPlaygroundRequestSchema } from '../../openapi/spec/advanced-playground-request-schema'; import { AdvancedPlaygroundResponseSchema } from '../../openapi/spec/advanced-playground-response-schema'; +import { + advancedPlaygroundViewModel, + playgroundViewModel, +} from './playground-view-model'; export default class PlaygroundController extends Controller { private openApiService: OpenApiService; @@ -84,14 +88,15 @@ export default class PlaygroundController extends Controller { req: Request, res: Response, ): Promise { - const response = { - input: req.body, - features: await this.playgroundService.evaluateQuery( - req.body.projects || '*', - req.body.environment, - req.body.context, - ), - }; + const result = await this.playgroundService.evaluateQuery( + req.body.projects || '*', + req.body.environment, + req.body.context, + ); + const response: PlaygroundResponseSchema = playgroundViewModel( + req.body, + result, + ); this.openApiService.respondWithValidation( 200, @@ -112,15 +117,18 @@ export default class PlaygroundController extends Controller { payload?.value && Number.isInteger(parseInt(payload?.value)) ? parseInt(payload?.value) : 15000; - res.json({ - input: req.body, - features: await this.playgroundService.evaluateAdvancedQuery( - req.body.projects || '*', - req.body.environments, - req.body.context, - limit, - ), - }); + + const result = await this.playgroundService.evaluateAdvancedQuery( + req.body.projects || '*', + req.body.environments, + req.body.context, + limit, + ); + + const response: AdvancedPlaygroundResponseSchema = + advancedPlaygroundViewModel(req.body, result); + + res.json(response); } else { res.status(409).end(); } diff --git a/src/lib/openapi/spec/playground-feature-schema.test.ts b/src/lib/openapi/spec/playground-feature-schema.test.ts index 3da9ef6f98..7c99fd3855 100644 --- a/src/lib/openapi/spec/playground-feature-schema.test.ts +++ b/src/lib/openapi/spec/playground-feature-schema.test.ts @@ -65,6 +65,9 @@ const playgroundStrategy = ( constraints: playgroundStrategyConstraints(), segments: fc.array(playgroundSegment()), disabled: fc.boolean(), + links: fc.constant({ + edit: '/projects/some-project/features/some-feature/strategies/edit?environmentId=some-env&strategyId=some-strat', + }), }); const playgroundStrategies = (): Arbitrary => diff --git a/src/lib/openapi/spec/playground-strategy-schema.ts b/src/lib/openapi/spec/playground-strategy-schema.ts index 13580f224b..d1494166a2 100644 --- a/src/lib/openapi/spec/playground-strategy-schema.ts +++ b/src/lib/openapi/spec/playground-strategy-schema.ts @@ -68,6 +68,7 @@ export const playgroundStrategySchema = { 'constraints', 'parameters', 'disabled', + 'links', ], properties: { name: { @@ -82,6 +83,7 @@ export const playgroundStrategySchema = { id: { description: "The strategy's id.", type: 'string', + example: '3AECCF7E-FF82-4174-8287-8EBE06079A50', }, result: { description: `The strategy's evaluation result. If the strategy is a custom strategy that Unleash can't evaluate, \`evaluationStatus\` will be \`${playgroundStrategyEvaluation.unknownResult}\`. Otherwise, it will be \`true\` or \`false\``, @@ -118,6 +120,19 @@ export const playgroundStrategySchema = { }, $ref: parametersSchema.$id, }, + links: { + description: + 'A set of links to actions you can perform on this strategy', + type: 'object', + required: ['edit'], + properties: { + edit: { + type: 'string', + example: + '/projects/some-project/features/some-feature/strategies/edit?environmentId=some-env&strategyId= 3AECCF7E-FF82-4174-8287-8EBE06079A50', + }, + }, + }, }, components: { schemas: { diff --git a/src/test/arbitraries.test.ts b/src/test/arbitraries.test.ts index d99e92e42f..1d12631a0e 100644 --- a/src/test/arbitraries.test.ts +++ b/src/test/arbitraries.test.ts @@ -19,7 +19,9 @@ export const urlFriendlyString = (): Arbitrary => ), { minLength: 1 }, ) - .map((arr) => arr.join('')); + .map((arr) => arr.join('')) + // filter out strings that are only dots because they mess with url parsing + .filter((string) => ![...string].every((char) => char === '.')); export const commonISOTimestamp = (): Arbitrary => fc 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 99f6de426b..06d996d4e7 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 @@ -4016,8 +4016,22 @@ The provider you choose for your addon dictates what properties the \`parameters }, "id": { "description": "The strategy's id.", + "example": "3AECCF7E-FF82-4174-8287-8EBE06079A50", "type": "string", }, + "links": { + "description": "A set of links to actions you can perform on this strategy", + "properties": { + "edit": { + "example": "/projects/some-project/features/some-feature/strategies/edit?environmentId=some-env&strategyId= 3AECCF7E-FF82-4174-8287-8EBE06079A50", + "type": "string", + }, + }, + "required": [ + "edit", + ], + "type": "object", + }, "name": { "description": "The strategy's name.", "type": "string", @@ -4110,6 +4124,7 @@ The provider you choose for your addon dictates what properties the \`parameters "constraints", "parameters", "disabled", + "links", ], "type": "object", }, diff --git a/src/test/e2e/services/playground-service.test.ts b/src/test/e2e/services/playground-service.test.ts index 58a94aa721..091df88c0b 100644 --- a/src/test/e2e/services/playground-service.test.ts +++ b/src/test/e2e/services/playground-service.test.ts @@ -1,4 +1,7 @@ -import { PlaygroundService } from '../../../lib/features/playground/playground-service'; +import { + PlaygroundFeatureEvaluationResult, + PlaygroundService, +} from '../../../lib/features/playground/playground-service'; import { clientFeaturesAndSegments, commonISOTimestamp, @@ -196,7 +199,7 @@ describe('the playground service (e2e)', () => { context: SdkContextSchema; env?: string; segments?: SegmentSchema[]; - }): Promise => { + }): Promise => { await seedDatabaseForPlaygroundTest(db, features, env, segments); // const activeSegments = await db.stores.segmentStore.getAllFeatureStrategySegments() @@ -204,8 +207,11 @@ describe('the playground service (e2e)', () => { const projects = '*'; - const serviceFeatures: PlaygroundFeatureSchema[] = - await service.evaluateQuery(projects, env, context); + const serviceFeatures = await service.evaluateQuery( + projects, + env, + context, + ); return serviceFeatures; };