From be0e94105d76a098e726874f329e805a75645917 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 29 Jun 2023 10:38:51 +0200 Subject: [PATCH] bug(#3545): include strategy titles on playground evaluation results (#4084) This PR adds strategy titles as an optional bit of data added to client features. It's only added when prompted. ![image](https://github.com/Unleash/unleash/assets/17786332/99509679-2aab-4c2a-abff-c6e6f27d8074) ## Discussion points: ### getPlaygroundFeatures The optional `includeStrategyId` parameter has been replaced by a `getPlaygroundFeatures` in the service (and in the underlying store). The playground was the only place that used this specific include, so instead of adding more and making the interface for that method more complex, I created a new method that deals specifically with the playground. The underlying store still uses an `optionalIncludes` parameter, however. I have a plan to make that interface more fluid, but I'd like to propose that in a follow-up PR. --- src/lib/db/feature-toggle-client-store.ts | 57 ++++++++++++------- .../features/playground/playground-service.ts | 13 ++--- src/lib/services/feature-toggle-service.ts | 13 +++-- .../stores/feature-toggle-client-store.ts | 6 +- .../feature-toggle-service-v2.e2e.test.ts | 41 +++++++++++++ .../fake-feature-toggle-client-store.ts | 13 +++++ 6 files changed, 107 insertions(+), 36 deletions(-) diff --git a/src/lib/db/feature-toggle-client-store.ts b/src/lib/db/feature-toggle-client-store.ts index 2870449680..e3ece08327 100644 --- a/src/lib/db/feature-toggle-client-store.ts +++ b/src/lib/db/feature-toggle-client-store.ts @@ -16,13 +16,14 @@ import FeatureToggleStore from './feature-toggle-store'; import { Db } from './db'; import Raw = Knex.Raw; +type OptionalClientFeatures = Set<'strategy IDs' | 'strategy titles'>; + export interface IGetAllFeatures { featureQuery?: IFeatureToggleQuery; archived: boolean; isAdmin: boolean; - includeStrategyIds?: boolean; - includeDisabledStrategies?: boolean; userId?: number; + optionalIncludes?: OptionalClientFeatures; } export interface IGetAdminFeatures { @@ -54,8 +55,8 @@ export default class FeatureToggleClientStore featureQuery, archived, isAdmin, - includeStrategyIds, userId, + optionalIncludes, }: IGetAllFeatures): Promise { const environment = featureQuery?.environment || DEFAULT_ENV; const stopTimer = this.timer('getFeatureAdmin'); @@ -74,6 +75,7 @@ export default class FeatureToggleClientStore 'fe.environment as environment', 'fs.id as strategy_id', 'fs.strategy_name as strategy_name', + 'fs.title as strategy_title', 'fs.disabled as strategy_disabled', 'fs.parameters as parameters', 'fs.constraints as constraints', @@ -198,19 +200,32 @@ export default class FeatureToggleClientStore feature.lastSeenAt = r.last_seen_at; feature.createdAt = r.created_at; } + acc[r.name] = feature; return acc; }, {}); const features: IFeatureToggleClient[] = Object.values(featureToggles); - if (!isAdmin && !includeStrategyIds) { - // We should not send strategy IDs from the client API, - // as this breaks old versions of the Go SDK (at least). - FeatureToggleClientStore.removeIdsFromStrategies(features); - } + // strip away unwanted properties + const cleanedFeatures = features.map(({ strategies, ...rest }) => ({ + ...rest, + strategies: strategies?.map(({ id, title, ...strategy }) => ({ + ...strategy, - return features; + ...(optionalIncludes?.has('strategy titles') && title + ? { title } + : {}), + + // We should not send strategy IDs from the client API, + // as this breaks old versions of the Go SDK (at least). + ...(isAdmin || optionalIncludes?.has('strategy IDs') + ? { id } + : {}), + })), + })); + + return cleanedFeatures; } private static rowToStrategy(row: Record): IStrategyConfig { @@ -230,14 +245,6 @@ export default class FeatureToggleClientStore }; } - private static removeIdsFromStrategies(features: IFeatureToggleClient[]) { - features.forEach((feature) => { - feature.strategies.forEach((strategy) => { - delete strategy.id; - }); - }); - } - private isUnseenStrategyRow( feature: PartialDeep, row: Record, @@ -298,15 +305,22 @@ export default class FeatureToggleClientStore async getClient( featureQuery?: IFeatureToggleQuery, - includeStrategyIds?: boolean, - includeDisabledStrategies?: boolean, ): Promise { return this.getAll({ featureQuery, archived: false, isAdmin: false, - includeStrategyIds, - includeDisabledStrategies, + }); + } + + async getPlayground( + featureQuery?: IFeatureToggleQuery, + ): Promise { + return this.getAll({ + featureQuery, + archived: false, + isAdmin: false, + optionalIncludes: new Set(['strategy titles', 'strategy IDs']), }); } @@ -320,7 +334,6 @@ export default class FeatureToggleClientStore archived: Boolean(archived), isAdmin: true, userId, - includeDisabledStrategies: true, }); } } diff --git a/src/lib/features/playground/playground-service.ts b/src/lib/features/playground/playground-service.ts index 6a44c83c79..b1e4e410d3 100644 --- a/src/lib/features/playground/playground-service.ts +++ b/src/lib/features/playground/playground-service.ts @@ -194,14 +194,11 @@ export class PlaygroundService { environment: string; } > { - const features = await this.featureToggleService.getClientFeatures( - { - project: projects === ALL ? undefined : projects, - environment, - }, - true, - false, - ); + const features = await this.featureToggleService.getPlaygroundFeatures({ + project: projects === ALL ? undefined : projects, + environment, + }); + const featureProject: Record = features.reduce( (obj, feature) => { obj[feature.name] = feature.project; diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 0d4cd8a11d..ce94261df8 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -828,13 +828,9 @@ class FeatureToggleService { async getClientFeatures( query?: IFeatureToggleQuery, - includeIds?: boolean, - includeDisabledStrategies?: boolean, ): Promise { const result = await this.featureToggleClientStore.getClient( query || {}, - includeIds, - includeDisabledStrategies, ); if (this.flagResolver.isEnabled('cleanClientApi')) { return result.map( @@ -869,6 +865,15 @@ class FeatureToggleService { } } + async getPlaygroundFeatures( + query?: IFeatureToggleQuery, + ): Promise { + const result = await this.featureToggleClientStore.getPlayground( + query || {}, + ); + return result; + } + /** * @deprecated Legacy! * diff --git a/src/lib/types/stores/feature-toggle-client-store.ts b/src/lib/types/stores/feature-toggle-client-store.ts index 824429075f..090ec8fa61 100644 --- a/src/lib/types/stores/feature-toggle-client-store.ts +++ b/src/lib/types/stores/feature-toggle-client-store.ts @@ -4,8 +4,10 @@ import { IGetAdminFeatures } from '../../db/feature-toggle-client-store'; export interface IFeatureToggleClientStore { getClient( featureQuery: Partial, - includeStrategyIds?: boolean, - includeDisabledStrategies?: boolean, + ): Promise; + + getPlayground( + featureQuery: Partial, ): Promise; // @Deprecated diff --git a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts index 25b4370ad3..52bd074b08 100644 --- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts +++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts @@ -592,3 +592,44 @@ test('If CRs are protected for any environment in the project stops bulk update ), ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST)); }); + +test('getPlaygroundFeatures should return ids and titles (if they exist) on client strategies', async () => { + const featureName = 'check-returned-strategy-configuration'; + const projectId = 'default'; + + const title = 'custom strategy title'; + const userName = 'strategy'; + const config: Omit = { + name: 'default', + constraints: [], + parameters: {}, + title, + }; + await service.createFeatureToggle( + projectId, + { + name: featureName, + }, + userName, + ); + + await service.createStrategy( + config, + { projectId, featureName, environment: DEFAULT_ENV }, + userName, + ); + + const playgroundFeatures = await service.getPlaygroundFeatures(); + + const strategyWithTitle = playgroundFeatures.find( + (feature) => feature.name === featureName, + )!.strategies[0]; + + expect(strategyWithTitle.title).toStrictEqual(title); + + for (const strategy of playgroundFeatures.flatMap( + (feature) => feature.strategies, + )) { + expect(strategy.id).not.toBeUndefined(); + } +}); diff --git a/src/test/fixtures/fake-feature-toggle-client-store.ts b/src/test/fixtures/fake-feature-toggle-client-store.ts index d765b8bcfd..c2d3e7c713 100644 --- a/src/test/fixtures/fake-feature-toggle-client-store.ts +++ b/src/test/fixtures/fake-feature-toggle-client-store.ts @@ -53,6 +53,19 @@ export default class FakeFeatureToggleClientStore return this.getFeatures(query); } + async getPlayground( + query?: IFeatureToggleQuery, + ): Promise { + const features = await this.getFeatures(query); + return features.map(({ strategies, ...rest }) => ({ + ...rest, + strategies: strategies.map((strategy, index) => ({ + ...strategy, + id: `strategy#${index}`, + })), + })); + } + async getAdmin({ featureQuery: query, archived,