From a9a75d5e824581fd83637b5e28efc763cec56837 Mon Sep 17 00:00:00 2001 From: andreas-unleash Date: Fri, 13 Oct 2023 10:38:18 +0300 Subject: [PATCH] fix: disable all environments when reviving a feature (#4999) Disable all environments when reviving a feature Closes # [SR-93](https://linear.app/unleash/issue/SR-93/disable-all-environments-when-reviving-a-feature) --------- Signed-off-by: andreas-unleash --- .../__snapshots__/create-config.test.ts.snap | 2 + .../fakes/fake-feature-toggle-store.ts | 6 +- .../feature-toggle/feature-toggle-service.ts | 11 ++++ .../feature-toggle/feature-toggle-store.ts | 8 +++ .../types/feature-toggle-store-type.ts | 2 + src/lib/routes/admin-api/archive.ts | 26 +++++++- src/lib/routes/admin-api/index.ts | 6 +- src/lib/routes/admin-api/project/index.ts | 9 ++- .../admin-api/project/project-archive.ts | 29 ++++++++- src/lib/types/experimental.ts | 7 +- src/test/e2e/api/admin/archive.test.ts | 65 +++++++++++++++++-- .../e2e/api/admin/feature-archive.e2e.test.ts | 19 ++++-- 12 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index de3ceed2bb..1712c56be1 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -83,6 +83,7 @@ exports[`should create default config 1`] = ` "demo": false, "dependentFeatures": false, "disableBulkToggle": false, + "disableEnvsOnRevive": false, "disableMetrics": false, "disableNotifications": false, "doraMetrics": false, @@ -127,6 +128,7 @@ exports[`should create default config 1`] = ` "demo": false, "dependentFeatures": false, "disableBulkToggle": false, + "disableEnvsOnRevive": false, "disableMetrics": false, "disableNotifications": false, "doraMetrics": false, diff --git a/src/lib/features/feature-toggle/fakes/fake-feature-toggle-store.ts b/src/lib/features/feature-toggle/fakes/fake-feature-toggle-store.ts index bebdaa7b89..57da33ad40 100644 --- a/src/lib/features/feature-toggle/fakes/fake-feature-toggle-store.ts +++ b/src/lib/features/feature-toggle/fakes/fake-feature-toggle-store.ts @@ -1,6 +1,6 @@ import { - IFeatureToggleStoreQuery, IFeatureToggleStore, + IFeatureToggleStoreQuery, } from '../types/feature-toggle-store-type'; import NotFoundError from '../../../error/notfound-error'; import { @@ -67,6 +67,10 @@ export default class FakeFeatureToggleStore implements IFeatureToggleStore { return features; } + disableAllEnvironmentsForFeatures(names: string[]): Promise { + throw new Error('Method not implemented.'); + } + async count(query: Partial): Promise { return this.features.filter(this.getFilterQuery(query)).length; } diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index a0372c6a2e..ba7dfdef5a 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -1915,6 +1915,12 @@ class FeatureToggleService { ); await this.featureToggleStore.batchRevive(eligibleFeatureNames); + if (this.flagResolver.isEnabled('disableEnvsOnRevive')) { + await this.featureToggleStore.disableAllEnvironmentsForFeatures( + eligibleFeatureNames, + ); + } + await this.eventService.storeEvents( eligibleFeatures.map( (feature) => @@ -1931,6 +1937,11 @@ class FeatureToggleService { async reviveFeature(featureName: string, createdBy: string): Promise { const toggle = await this.featureToggleStore.revive(featureName); + if (this.flagResolver.isEnabled('disableEnvsOnRevive')) { + await this.featureToggleStore.disableAllEnvironmentsForFeatures([ + featureName, + ]); + } await this.eventService.storeEvent( new FeatureRevivedEvent({ createdBy, diff --git a/src/lib/features/feature-toggle/feature-toggle-store.ts b/src/lib/features/feature-toggle/feature-toggle-store.ts index a735781947..c8cd967a3b 100644 --- a/src/lib/features/feature-toggle/feature-toggle-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-store.ts @@ -575,6 +575,7 @@ export default class FeatureToggleStore implements IFeatureToggleStore { .where({ name }) .update({ archived_at: null }) .returning(FEATURE_COLUMNS); + return this.rowToFeature(row[0]); } @@ -583,9 +584,16 @@ export default class FeatureToggleStore implements IFeatureToggleStore { .whereIn('name', names) .update({ archived_at: null }) .returning(FEATURE_COLUMNS); + return rows.map((row) => this.rowToFeature(row)); } + async disableAllEnvironmentsForFeatures(names: string[]): Promise { + await this.db(FEATURE_ENVIRONMENTS_TABLE) + .whereIn('feature_name', names) + .update({ enabled: false }); + } + async getVariants(featureName: string): Promise { if (!(await this.exists(featureName))) { throw new NotFoundError('No feature toggle found'); diff --git a/src/lib/features/feature-toggle/types/feature-toggle-store-type.ts b/src/lib/features/feature-toggle/types/feature-toggle-store-type.ts index c0b546773e..cbc3ec10cd 100644 --- a/src/lib/features/feature-toggle/types/feature-toggle-store-type.ts +++ b/src/lib/features/feature-toggle/types/feature-toggle-store-type.ts @@ -66,4 +66,6 @@ export interface IFeatureToggleStore extends Store { featureName: string, newVariants: IVariant[], ): Promise; + + disableAllEnvironmentsForFeatures(names: string[]): Promise; } diff --git a/src/lib/routes/admin-api/archive.ts b/src/lib/routes/admin-api/archive.ts index 92345cc479..360a8846a9 100644 --- a/src/lib/routes/admin-api/archive.ts +++ b/src/lib/routes/admin-api/archive.ts @@ -17,22 +17,36 @@ import { emptyResponse, getStandardResponses, } from '../../openapi/util/standard-responses'; +import { TransactionCreator, UnleashTransaction } from '../../db/transaction'; export default class ArchiveController extends Controller { private featureService: FeatureToggleService; - + private transactionalFeatureToggleService: ( + db: UnleashTransaction, + ) => FeatureToggleService; + private readonly startTransaction: TransactionCreator; private openApiService: OpenApiService; constructor( config: IUnleashConfig, { + transactionalFeatureToggleService, featureToggleServiceV2, openApiService, - }: Pick, + }: Pick< + IUnleashServices, + | 'transactionalFeatureToggleService' + | 'featureToggleServiceV2' + | 'openApiService' + >, + startTransaction: TransactionCreator, ) { super(config); this.featureService = featureToggleServiceV2; this.openApiService = openApiService; + this.transactionalFeatureToggleService = + transactionalFeatureToggleService; + this.startTransaction = startTransaction; this.route({ method: 'get', @@ -172,7 +186,13 @@ export default class ArchiveController extends Controller { ): Promise { const userName = extractUsername(req); const { featureName } = req.params; - await this.featureService.reviveFeature(featureName, userName); + + await this.startTransaction(async (tx) => + this.transactionalFeatureToggleService(tx).reviveFeature( + featureName, + userName, + ), + ); res.status(200).end(); } } diff --git a/src/lib/routes/admin-api/index.ts b/src/lib/routes/admin-api/index.ts index 929272f35a..75ca0f8825 100644 --- a/src/lib/routes/admin-api/index.ts +++ b/src/lib/routes/admin-api/index.ts @@ -49,7 +49,11 @@ class AdminApi extends Controller { ); this.app.use( '/archive', - new ArchiveController(config, services).router, + new ArchiveController( + config, + services, + createKnexTransactionStarter(db), + ).router, ); this.app.use( '/strategies', diff --git a/src/lib/routes/admin-api/project/index.ts b/src/lib/routes/admin-api/project/index.ts index 402e9d7bba..d786a5fc8a 100644 --- a/src/lib/routes/admin-api/project/index.ts +++ b/src/lib/routes/admin-api/project/index.ts @@ -125,7 +125,14 @@ export default class ProjectApi extends Controller { this.use('/', new ProjectHealthReport(config, services).router); this.use('/', new VariantsController(config, services).router); this.use('/', new ProjectApiTokenController(config, services).router); - this.use('/', new ProjectArchiveController(config, services).router); + this.use( + '/', + new ProjectArchiveController( + config, + services, + createKnexTransactionStarter(db), + ).router, + ); } async getProjects( diff --git a/src/lib/routes/admin-api/project/project-archive.ts b/src/lib/routes/admin-api/project/project-archive.ts index 7669e59a8b..91a1acd2c8 100644 --- a/src/lib/routes/admin-api/project/project-archive.ts +++ b/src/lib/routes/admin-api/project/project-archive.ts @@ -18,6 +18,10 @@ import { } from '../../../openapi/util/standard-responses'; import { BatchFeaturesSchema, createRequestSchema } from '../../../openapi'; import Controller from '../../controller'; +import { + TransactionCreator, + UnleashTransaction, +} from '../../../db/transaction'; const PATH = '/:projectId'; const PATH_ARCHIVE = `${PATH}/archive`; @@ -29,6 +33,11 @@ export default class ProjectArchiveController extends Controller { private featureService: FeatureToggleService; + private transactionalFeatureToggleService: ( + db: UnleashTransaction, + ) => FeatureToggleService; + private readonly startTransaction: TransactionCreator; + private openApiService: OpenApiService; private flagResolver: IFlagResolver; @@ -36,15 +45,25 @@ export default class ProjectArchiveController extends Controller { constructor( config: IUnleashConfig, { + transactionalFeatureToggleService, featureToggleServiceV2, openApiService, - }: Pick, + }: Pick< + IUnleashServices, + | 'transactionalFeatureToggleService' + | 'featureToggleServiceV2' + | 'openApiService' + >, + startTransaction: TransactionCreator, ) { super(config); this.logger = config.getLogger('/admin-api/archive.js'); this.featureService = featureToggleServiceV2; this.openApiService = openApiService; this.flagResolver = config.flagResolver; + this.transactionalFeatureToggleService = + transactionalFeatureToggleService; + this.startTransaction = startTransaction; this.route({ method: 'post', @@ -130,7 +149,13 @@ export default class ProjectArchiveController extends Controller { const { projectId } = req.params; const { features } = req.body; const user = extractUsername(req); - await this.featureService.reviveFeatures(features, projectId, user); + await this.startTransaction(async (tx) => + this.transactionalFeatureToggleService(tx).reviveFeatures( + features, + projectId, + user, + ), + ); res.status(200).end(); } diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 855be38613..d058f1a3a6 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -37,7 +37,8 @@ export type IFlagKey = | 'useLastSeenRefactor' | 'internalMessageBanners' | 'internalMessageBanner' - | 'separateAdminClientApi'; + | 'separateAdminClientApi' + | 'disableEnvsOnRevive'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -173,6 +174,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_SEPARATE_ADMIN_CLIENT_API, false, ), + disableEnvsOnRevive: parseEnvVarBoolean( + process.env.UNLEASH_EXPERIMENTAL_DISABLE_ENVS_ON_REVIVE, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = { diff --git a/src/test/e2e/api/admin/archive.test.ts b/src/test/e2e/api/admin/archive.test.ts index daad050e1d..02be9c33b7 100644 --- a/src/test/e2e/api/admin/archive.test.ts +++ b/src/test/e2e/api/admin/archive.test.ts @@ -10,13 +10,18 @@ let db: ITestDb; beforeAll(async () => { db = await dbInit('archive_test_serial', getLogger); - app = await setupAppWithCustomConfig(db.stores, { - experimental: { - flags: { - strictSchemaValidation: true, + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + disableEnvsOnRevive: true, + }, }, }, - }); + db.rawDatabase, + ); }); afterAll(async () => { @@ -142,6 +147,56 @@ test('Should be able to revive toggle', async () => { .expect(200); }); +test('Should disable all environments when reviving a toggle', async () => { + await db.stores.featureToggleStore.deleteAll(); + await db.stores.featureToggleStore.create('default', { + name: 'feat-proj-1', + archived: true, + }); + + await db.stores.environmentStore.create({ + name: 'development', + enabled: true, + type: 'development', + sortOrder: 1, + }); + + await db.stores.environmentStore.create({ + name: 'production', + enabled: true, + type: 'production', + sortOrder: 2, + }); + + await db.stores.featureEnvironmentStore.addEnvironmentToFeature( + 'feat-proj-1', + 'default', + true, + ); + await db.stores.featureEnvironmentStore.addEnvironmentToFeature( + 'feat-proj-1', + 'production', + true, + ); + await db.stores.featureEnvironmentStore.addEnvironmentToFeature( + 'feat-proj-1', + 'development', + true, + ); + await app.request + .post('/api/admin/archive/revive/feat-proj-1') + .send({}) + .expect(200); + + const { body } = await app.request + .get( + '/api/admin/projects/default/features/feat-proj-1?variantEnvironments=true', + ) + .expect(200); + + expect(body.environments.every((env) => !env.enabled)); +}); + test('Reviving a non-existing toggle should yield 404', async () => { await app.request .post('/api/admin/archive/revive/non.existing') diff --git a/src/test/e2e/api/admin/feature-archive.e2e.test.ts b/src/test/e2e/api/admin/feature-archive.e2e.test.ts index 9fecb9c16b..0fe0f19ee8 100644 --- a/src/test/e2e/api/admin/feature-archive.e2e.test.ts +++ b/src/test/e2e/api/admin/feature-archive.e2e.test.ts @@ -11,13 +11,18 @@ let db: ITestDb; beforeAll(async () => { db = await dbInit('archive_serial', getLogger); - app = await setupAppWithCustomConfig(db.stores, { - experimental: { - flags: { - strictSchemaValidation: true, + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + disableEnvsOnRevive: true, + }, }, }, - }); + db.rawDatabase, + ); await app.createFeature({ name: 'featureX', description: 'the #1 feature', @@ -212,9 +217,11 @@ test('can bulk revive features', async () => { .send({ features }) .expect(200); for (const feature of features) { - await app.request + const { body } = await app.request .get(`/api/admin/projects/default/features/${feature}`) .expect(200); + + expect(body.environments.every((env) => !env.enabled)); } });