From a4015802285f0e3e06c67e66acc062e668bafcd2 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 6 Oct 2021 09:25:34 +0200 Subject: [PATCH] task: Disables feature_environments without strategies (#1003) --- src/lib/db/feature-environment-store.ts | 21 ++- src/lib/routes/admin-api/project/features.ts | 3 +- src/lib/services/feature-toggle-service-v2.ts | 6 + .../types/stores/feature-environment-store.ts | 5 +- .../project/feature.strategy.e2e.test.ts | 137 ++++++++++++++++++ .../fake-feature-environment-store.ts | 9 ++ 6 files changed, 178 insertions(+), 3 deletions(-) diff --git a/src/lib/db/feature-environment-store.ts b/src/lib/db/feature-environment-store.ts index 219e758c4e..3467db7ca2 100644 --- a/src/lib/db/feature-environment-store.ts +++ b/src/lib/db/feature-environment-store.ts @@ -10,7 +10,10 @@ import { DB_TIME } from '../metric-events'; import { IFeatureEnvironment } from '../types/model'; import NotFoundError from '../error/notfound-error'; -const T = { featureEnvs: 'feature_environments' }; +const T = { + featureEnvs: 'feature_environments', + featureStrategies: 'feature_strategies', +}; interface IFeatureEnvironmentRow { environment: string; @@ -92,6 +95,22 @@ export class FeatureEnvironmentStore implements IFeatureEnvironmentStore { })); } + async disableEnvironmentIfNoStrategies( + featureName: string, + environment: string, + ): Promise { + const result = await this.db.raw( + `SELECT EXISTS (SELECT 1 FROM ${T.featureStrategies} WHERE feature_name = ? AND environment = ?) AS enabled`, + [featureName, environment], + ); + const { enabled } = result.rows[0]; + if (!enabled) { + await this.db(T.featureEnvs) + .update({ enabled: false }) + .where({ feature_name: featureName, environment }); + } + } + async addEnvironmentToFeature( featureName: string, environment: string, diff --git a/src/lib/routes/admin-api/project/features.ts b/src/lib/routes/admin-api/project/features.ts index 3ff54148de..868fd881c6 100644 --- a/src/lib/routes/admin-api/project/features.ts +++ b/src/lib/routes/admin-api/project/features.ts @@ -300,12 +300,13 @@ export default class ProjectFeaturesController extends Controller { res: Response, ): Promise { this.logger.info('Deleting strategy'); - const { environment, projectId } = req.params; + const { environment, projectId, featureName } = req.params; const userName = extractUsername(req); const { strategyId } = req.params; this.logger.info(strategyId); const strategy = await this.featureService.deleteStrategy( strategyId, + featureName, userName, projectId, environment, diff --git a/src/lib/services/feature-toggle-service-v2.ts b/src/lib/services/feature-toggle-service-v2.ts index 3853b4f555..8eba5f2b00 100644 --- a/src/lib/services/feature-toggle-service-v2.ts +++ b/src/lib/services/feature-toggle-service-v2.ts @@ -226,6 +226,7 @@ class FeatureToggleServiceV2 { */ async deleteStrategy( id: string, + featureName: string, userName: string, project: string = 'default', environment: string = DEFAULT_ENV, @@ -240,6 +241,11 @@ class FeatureToggleServiceV2 { id, }, }); + // If there are no strategies left for environment disable it + await this.featureEnvironmentStore.disableEnvironmentIfNoStrategies( + featureName, + environment, + ); } async getStrategiesForEnvironment( diff --git a/src/lib/types/stores/feature-environment-store.ts b/src/lib/types/stores/feature-environment-store.ts index 79fef3ad13..a5ff4c2fef 100644 --- a/src/lib/types/stores/feature-environment-store.ts +++ b/src/lib/types/stores/feature-environment-store.ts @@ -34,7 +34,10 @@ export interface IFeatureEnvironmentStore environment: string, enabled: boolean, ): Promise; - + disableEnvironmentIfNoStrategies( + featureName: string, + environment: string, + ): Promise; disconnectFeatures(environment: string, project: string): Promise; connectFeatures(environment: string, projectId: string): Promise; diff --git a/src/test/e2e/api/admin/project/feature.strategy.e2e.test.ts b/src/test/e2e/api/admin/project/feature.strategy.e2e.test.ts index 9a39c57fae..32d4777d6d 100644 --- a/src/test/e2e/api/admin/project/feature.strategy.e2e.test.ts +++ b/src/test/e2e/api/admin/project/feature.strategy.e2e.test.ts @@ -1108,3 +1108,140 @@ test('Feature strategies list should respect strategy sortorders for each enviro expect(strategies[1].sortOrder).toBe(sortOrderSecond); expect(strategies[2].sortOrder).toBe(sortOrderDefault); }); + +test('Deleting last strategy for feature environment should disable that environment', async () => { + const envName = 'last_strategy_delete_env'; + const featureName = 'last_strategy_delete_feature'; + // Create environment + await db.stores.environmentStore.create({ + name: envName, + type: 'test', + }); + // Connect environment to project + await app.request + .post('/api/admin/projects/default/environments') + .send({ + environment: envName, + }) + .expect(200); + await app.request + .post('/api/admin/projects/default/features') + .send({ name: featureName }) + .expect(201); + let strategyId; + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, + ) + .send({ + name: 'default', + parameters: { + userId: 'string', + }, + }) + .expect(200) + .expect((res) => { + strategyId = res.body.id; + }); + // Enable feature_environment + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/on`, + ) + .send({}) + .expect(200); + await app.request + .get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}`, + ) + .expect(200) + .expect((res) => { + expect(res.body.enabled).toBeTruthy(); + }); + // Delete last strategy, this should also disable the environment + await app.request.delete( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies/${strategyId}`, + ); + await app.request + .get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}`, + ) + .expect(200) + .expect((res) => { + expect(res.body.enabled).toBeFalsy(); + }); +}); + +test('Deleting strategy for feature environment should not disable that environment as long as there are other strategies', async () => { + const envName = 'any_strategy_delete_env'; + const featureName = 'any_strategy_delete_feature'; + // Create environment + await db.stores.environmentStore.create({ + name: envName, + type: 'test', + }); + // Connect environment to project + await app.request + .post('/api/admin/projects/default/environments') + .send({ + environment: envName, + }) + .expect(200); + await app.request + .post('/api/admin/projects/default/features') + .send({ name: featureName }) + .expect(201); + let strategyId; + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, + ) + .send({ + name: 'default', + parameters: { + userId: 'string', + }, + }) + .expect(200) + .expect((res) => { + strategyId = res.body.id; + }); + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, + ) + .send({ + name: 'default', + parameters: { + customerId: 'string', + }, + }) + .expect(200); + // Enable feature_environment + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/on`, + ) + .send({}) + .expect(200); + await app.request + .get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}`, + ) + .expect(200) + .expect((res) => { + expect(res.body.enabled).toBeTruthy(); + }); + // Delete a strategy, this should also disable the environment + await app.request.delete( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies/${strategyId}`, + ); + await app.request + .get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}`, + ) + .expect(200) + .expect((res) => { + expect(res.body.enabled).toBeTruthy(); + }); +}); diff --git a/src/test/fixtures/fake-feature-environment-store.ts b/src/test/fixtures/fake-feature-environment-store.ts index 013c89d55e..1efc17db5d 100644 --- a/src/test/fixtures/fake-feature-environment-store.ts +++ b/src/test/fixtures/fake-feature-environment-store.ts @@ -148,4 +148,13 @@ export default class FakeFeatureEnvironmentStore ): Promise { return Promise.reject(new Error('Not implemented')); } + + disableEnvironmentIfNoStrategies( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + featureName: string, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + environment: string, + ): Promise { + return Promise.reject(new Error('Not implemented')); + } }