From 2b565aeef7d0b50cb0e7407c93418467fbf48073 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Wed, 26 Jul 2023 11:31:26 +0200 Subject: [PATCH] feat: incrementing sort order for strategies (#4343) --- src/lib/db/feature-strategy-store.test.ts | 52 +++++++++++++++++++ src/lib/db/feature-strategy-store.ts | 24 ++++++++- .../export-import.e2e.test.ts | 4 +- .../api/admin/project/features.e2e.test.ts | 29 +++++------ 4 files changed, 90 insertions(+), 19 deletions(-) diff --git a/src/lib/db/feature-strategy-store.test.ts b/src/lib/db/feature-strategy-store.test.ts index 7ef1e4a95d..156ab07373 100644 --- a/src/lib/db/feature-strategy-store.test.ts +++ b/src/lib/db/feature-strategy-store.test.ts @@ -3,6 +3,7 @@ import getLogger from '../../test/fixtures/no-logger'; import FeatureStrategiesStore from './feature-strategy-store'; import FeatureToggleStore from './feature-toggle-store'; import StrategyStore from './strategy-store'; +import { IFeatureStrategy, PartialSome } from '../types'; let db; @@ -94,3 +95,54 @@ test('counts custom strategies in use', async () => { // Assert expect(inUseCount).toEqual(1); }); + +const baseStrategy: PartialSome = { + projectId: 'default', + featureName: 'test-toggle-increment', + strategyName: 'strategy-1', + environment: 'default', + parameters: {}, + constraints: [], + variants: [], +}; +test('increment sort order on each new insert', async () => { + const featureToggleStore: FeatureToggleStore = db.stores.featureToggleStore; + const featureStrategiesStore: FeatureStrategiesStore = + db.stores.featureStrategiesStore; + + await featureToggleStore.create('default', { + name: 'test-toggle-increment', + }); + + const { id: firstId } = + await featureStrategiesStore.createStrategyFeatureEnv({ + ...baseStrategy, + featureName: 'test-toggle-increment', + strategyName: 'strategy-1', + // sort order implicitly 0 + }); + const { id: secondId } = + await featureStrategiesStore.createStrategyFeatureEnv({ + ...baseStrategy, + featureName: 'test-toggle-increment', + strategyName: 'strategy-2', + sortOrder: 50, // explicit sort order + }); + const { id: thirdId } = + await featureStrategiesStore.createStrategyFeatureEnv({ + ...baseStrategy, + featureName: 'test-toggle-increment', + strategyName: 'strategy-2', + // implicit sort order incremented by 1 + }); + + const firstStrategy = await featureStrategiesStore.getStrategyById(firstId); + const secondStrategy = await featureStrategiesStore.getStrategyById( + secondId, + ); + const thirdStrategy = await featureStrategiesStore.getStrategyById(thirdId); + + expect(firstStrategy.sortOrder).toEqual(0); + expect(secondStrategy.sortOrder).toEqual(50); + expect(thirdStrategy.sortOrder).toEqual(51); +}); diff --git a/src/lib/db/feature-strategy-store.ts b/src/lib/db/feature-strategy-store.ts index 3cba3ac5e7..5e2d709822 100644 --- a/src/lib/db/feature-strategy-store.ts +++ b/src/lib/db/feature-strategy-store.ts @@ -106,7 +106,7 @@ function mapInput(input: IFeatureStrategy): IFeatureStrategiesTable { constraints: JSON.stringify(input.constraints || []), variants: JSON.stringify(input.variants || []), created_at: input.createdAt, - sort_order: input.sortOrder, + sort_order: input.sortOrder ?? 9999, disabled: input.disabled, }; } @@ -197,10 +197,30 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { return mapRow(row); } + private async nextSortOrder(featureName: string, environment: string) { + const [{ max }] = await this.db(T.featureStrategies) + .max('sort_order as max') + .where({ + feature_name: featureName, + environment, + }); + return Number.isInteger(max) ? max + 1 : 0; + } + async createStrategyFeatureEnv( strategyConfig: PartialSome, ): Promise { - const strategyRow = mapInput({ id: uuidv4(), ...strategyConfig }); + const sortOrder = + strategyConfig.sortOrder ?? + (await this.nextSortOrder( + strategyConfig.featureName, + strategyConfig.environment, + )); + const strategyRow = mapInput({ + id: uuidv4(), + ...strategyConfig, + sortOrder, + }); const rows = await this.db(T.featureStrategies) .insert(strategyRow) .returning('*'); diff --git a/src/lib/features/export-import-toggles/export-import.e2e.test.ts b/src/lib/features/export-import-toggles/export-import.e2e.test.ts index 647d5de15c..3d984f92db 100644 --- a/src/lib/features/export-import-toggles/export-import.e2e.test.ts +++ b/src/lib/features/export-import-toggles/export-import.e2e.test.ts @@ -693,7 +693,7 @@ test('import features to existing project and environment', async () => { featureName: defaultFeature, parameters: {}, constraints, - sortOrder: 9999, + sortOrder: 0, name: 'default', }, ], @@ -748,7 +748,7 @@ test('importing same JSON should work multiple times in a row', async () => { featureName: defaultFeature, parameters: {}, constraints, - sortOrder: 9999, + sortOrder: 0, name: 'default', }, ], 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 d028e0a92b..63d673108a 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -32,7 +32,6 @@ let app: IUnleashTest; let db: ITestDb; const sortOrderFirst = 0; const sortOrderSecond = 10; -const sortOrderDefault = 9999; const createSegment = async (segmentName: string) => { const segment = await app.services.segmentService.create( @@ -1629,9 +1628,9 @@ test('List of strategies should respect sortOrder', async () => { `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, ); const strategies = body; - expect(strategies[0].sortOrder).toBe(sortOrderFirst); - expect(strategies[1].sortOrder).toBe(sortOrderSecond); - expect(strategies[2].sortOrder).toBe(sortOrderDefault); + expect(strategies[0].sortOrder).toBe(0); + expect(strategies[1].sortOrder).toBe(0); + expect(strategies[2].sortOrder).toBe(10); }); test('Feature strategies list should respect strategy sortorders for each environment', async () => { @@ -1674,13 +1673,13 @@ test('Feature strategies list should respect strategy sortorders for each enviro ); const { body } = response; let { strategies } = body.environments.find((e) => e.name === envName); - expect(strategies[0].sortOrder).toBe(sortOrderFirst); - expect(strategies[1].sortOrder).toBe(sortOrderSecond); - expect(strategies[2].sortOrder).toBe(sortOrderDefault); + expect(strategies[0].sortOrder).toBe(0); + expect(strategies[1].sortOrder).toBe(0); + expect(strategies[2].sortOrder).toBe(10); strategies = body.environments.find((e) => e.name === secondEnv).strategies; - expect(strategies[0].sortOrder).toBe(sortOrderFirst); - expect(strategies[1].sortOrder).toBe(sortOrderSecond); - expect(strategies[2].sortOrder).toBe(sortOrderDefault); + expect(strategies[0].sortOrder).toBe(0); + expect(strategies[1].sortOrder).toBe(0); + expect(strategies[2].sortOrder).toBe(10); }); test('Deleting last strategy for feature environment should disable that environment', async () => { @@ -2594,9 +2593,9 @@ test('should change strategy sort order when payload is valid', async () => { `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ); - expect(strategies[0].sortOrder).toBe(9999); + expect(strategies[0].sortOrder).toBe(0); expect(strategies[0].id).toBe(strategyOne.id); - expect(strategies[1].sortOrder).toBe(9999); + expect(strategies[1].sortOrder).toBe(1); expect(strategies[1].id).toBe(strategyTwo.id); await app.request @@ -2680,9 +2679,9 @@ test('should return strategies in correct order when new strategies are added', `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ); - expect(strategies[0].sortOrder).toBe(9999); + expect(strategies[0].sortOrder).toBe(0); expect(strategies[0].id).toBe(strategyOne.id); - expect(strategies[1].sortOrder).toBe(9999); + expect(strategies[1].sortOrder).toBe(1); expect(strategies[1].id).toBe(strategyTwo.id); await app.request @@ -2760,7 +2759,7 @@ test('should return strategies in correct order when new strategies are added', expect(strategiesReOrdered[1].id).toBe(strategyTwo.id); expect(strategiesReOrdered[2].sortOrder).toBe(2); expect(strategiesReOrdered[2].id).toBe(strategyOne.id); - expect(strategiesReOrdered[3].sortOrder).toBe(9999); + expect(strategiesReOrdered[3].sortOrder).toBe(3); expect(strategiesReOrdered[3].id).toBe(strategyThree.id); });