task: changing variants blocked by cr (#2966)

## About the changes This PR adds two new functions that is protected by CR. When used instead of the current setVariantOnEnv and setVariantsOnEnv if the flag UNLEASH_EXPERIMENTAL_CR_ON_VARIANTS is set, the call is blocked. This leaves the old functions, which is used from the CR flow in place, and adds new methods protected by CR. Also adds e2e tests verifying that the methods will block requests if CR is enabled for project:environment pair, as well as not block if CR is not enabled. Tests already in place should confirm that the default flow, without the flag enabled just works.
2025-01-25 00:07:47 +01:00 · 2023-01-24 10:43:10 +01:00 · 2023-01-24 10:43:10 +01:00 · c961374b24
commit c961374b24
parent 3713764a4b
6 changed files with 240 additions and 21 deletions
--- a/src/lib/snapshots/create-config.test.ts.snap
+++ b/src/lib/snapshots/create-config.test.ts.snap
@ -71,6 +71,7 @@ exports[`should create default config 1`] = `
      "anonymiseEventLog": false,
      "batchMetrics": false,
      "caseInsensitiveInOperators": false,
+      "crOnVariants": false,
      "embedProxy": true,
      "embedProxyFrontend": true,
      "featuresExportImport": false,
@ -92,6 +93,7 @@ exports[`should create default config 1`] = `
      "anonymiseEventLog": false,
      "batchMetrics": false,
      "caseInsensitiveInOperators": false,
+      "crOnVariants": false,
      "embedProxy": true,
      "embedProxyFrontend": true,
      "featuresExportImport": false,
--- a/src/lib/db/access-store.ts
+++ b/src/lib/db/access-store.ts
@ -481,8 +481,10 @@ export class AccessStore implements IAccessStore {
        environment: string,
    ): Promise<boolean> {
        const result = await this.db.raw(
-            `SELECT EXISTS(SELECT 1 FROM ${T.CHANGE_REQUEST_SETTINGS}
-                       WHERE environment = ? and project = ?) AS present`,
+            `SELECT EXISTS(SELECT 1
+                           FROM ${T.CHANGE_REQUEST_SETTINGS}
+                           WHERE environment = ?
+                             and project = ?) AS present`,
            [environment, project],
        );
        const { present } = result.rows[0];
--- a/src/lib/routes/admin-api/project/variants.ts
+++ b/src/lib/routes/admin-api/project/variants.ts
@ -189,12 +189,11 @@ export default class VariantsController extends Controller {
        res: Response<FeatureVariantsSchema>,
    ): Promise<void> {
        const { projectId, featureName } = req.params;
-        const userName = extractUsername(req);
        const updatedFeature = await this.featureService.updateVariants(
            featureName,
            projectId,
            req.body,
-            userName,
+            req.user,
        );
        res.status(200).json({
            version: 1,
@ -231,7 +230,6 @@ export default class VariantsController extends Controller {
    ): Promise<void> {
        const { projectId, featureName } = req.params;
        const { environments, variants } = req.body;
-        const userName = extractUsername(req);

        if (environments === undefined || environments.length === 0) {
            throw new BadDataError('No environments provided');
@ -250,12 +248,12 @@ export default class VariantsController extends Controller {
            ...variant,
        }));

-        await this.featureService.setVariantsOnEnvs(
+        await this.featureService.crProtectedSetVariantsOnEnvs(
            projectId,
            featureName,
            environments,
            variantsWithDefaults,
-            userName,
+            req.user,
        );
        res.status(200).json({
            version: 1,
@ -303,14 +301,13 @@ export default class VariantsController extends Controller {
        res: Response<FeatureVariantsSchema>,
    ): Promise<void> {
        const { projectId, featureName, environment } = req.params;
-        const userName = extractUsername(req);

        const variants = await this.featureService.updateVariantsOnEnv(
            featureName,
            projectId,
            environment,
            req.body,
-            userName,
+            req.user,
        );
        res.status(200).json({
            version: 1,
@ -323,13 +320,12 @@ export default class VariantsController extends Controller {
        res: Response<FeatureVariantsSchema>,
    ): Promise<void> {
        const { featureName, environment, projectId } = req.params;
-        const userName = extractUsername(req);
-        const variants = await this.featureService.saveVariantsOnEnv(
+        const variants = await this.featureService.crProtectedSaveVariantsOnEnv(
            projectId,
            featureName,
            environment,
            req.body,
-            userName,
+            req.user,
        );
        res.status(200).json({
            version: 1,
--- a/src/lib/services/feature-toggle-service.ts
+++ b/src/lib/services/feature-toggle-service.ts
@ -1,5 +1,5 @@
 import { IUnleashConfig } from '../types/option';
-import { IUnleashStores } from '../types';
+import { IFlagResolver, IUnleashStores } from '../types';
 import { Logger } from '../logger';
 import BadDataError from '../error/bad-data-error';
 import NameExistsError from '../error/name-exists-error';
@ -127,6 +127,8 @@ class FeatureToggleService {

    private accessService: AccessService;

+    private flagResolver: IFlagResolver;
+
    constructor(
        {
            featureStrategiesStore,
@ -148,7 +150,10 @@ class FeatureToggleService {
            | 'featureEnvironmentStore'
            | 'contextFieldStore'
        >,
-        { getLogger }: Pick<IUnleashConfig, 'getLogger'>,
+        {
+            getLogger,
+            flagResolver,
+        }: Pick<IUnleashConfig, 'getLogger' | 'flagResolver'>,
        segmentService: SegmentService,
        accessService: AccessService,
    ) {
@ -163,6 +168,7 @@ class FeatureToggleService {
        this.contextFieldStore = contextFieldStore;
        this.segmentService = segmentService;
        this.accessService = accessService;
+        this.flagResolver = flagResolver;
    }

    async validateFeatureContext({
@ -1248,7 +1254,7 @@ class FeatureToggleService {
        featureName: string,
        project: string,
        newVariants: Operation[],
-        createdBy: string,
+        user: User,
    ): Promise<FeatureToggle> {
        const ft =
            await this.featureStrategiesStore.getFeatureToggleWithVariantEnvs(
@ -1260,7 +1266,7 @@ class FeatureToggleService {
                project,
                env.name,
                newVariants,
-                createdBy,
+                user,
            ).then((resultingVariants) => (env.variants = resultingVariants)),
        );
        await Promise.all(promises);
@ -1273,19 +1279,19 @@ class FeatureToggleService {
        project: string,
        environment: string,
        newVariants: Operation[],
-        createdBy: string,
+        user: User,
    ): Promise<IVariant[]> {
        const oldVariants = await this.getVariantsForEnv(
            featureName,
            environment,
        );
        const { newDocument } = await applyPatch(oldVariants, newVariants);
-        return this.saveVariantsOnEnv(
+        return this.crProtectedSaveVariantsOnEnv(
            project,
            featureName,
            environment,
            newDocument,
-            createdBy,
+            user,
            oldVariants,
        );
    }
@ -1357,6 +1363,52 @@ class FeatureToggleService {
        return fixedVariants;
    }

+    async crProtectedSaveVariantsOnEnv(
+        projectId: string,
+        featureName: string,
+        environment: string,
+        newVariants: IVariant[],
+        user: User,
+        oldVariants?: IVariant[],
+    ): Promise<IVariant[]> {
+        if (this.flagResolver.isEnabled('crOnVariants')) {
+            await this.stopWhenChangeRequestsEnabled(
+                projectId,
+                environment,
+                user,
+            );
+        }
+        return this.saveVariantsOnEnv(
+            projectId,
+            featureName,
+            environment,
+            newVariants,
+            user.username,
+            oldVariants,
+        );
+    }
+
+    async crProtectedSetVariantsOnEnvs(
+        projectId: string,
+        featureName: string,
+        environments: string[],
+        newVariants: IVariant[],
+        user: User,
+    ): Promise<IVariant[]> {
+        if (this.flagResolver.isEnabled('crOnVariants')) {
+            for (const env of environments) {
+                await this.stopWhenChangeRequestsEnabled(projectId, env);
+            }
+        }
+        return this.setVariantsOnEnvs(
+            projectId,
+            featureName,
+            environments,
+            newVariants,
+            user.username,
+        );
+    }
+
    async setVariantsOnEnvs(
        projectId: string,
        featureName: string,
@ -1390,7 +1442,6 @@ class FeatureToggleService {
                    }),
            ),
        );
-
        await this.featureEnvironmentStore.setVariantsToFeatureEnvironments(
            featureName,
            environments,
--- a/src/lib/types/experimental.ts
+++ b/src/lib/types/experimental.ts
@ -66,6 +66,10 @@ const flags = {
        process.env.UNLEASH_EXPERIMENTAL_CASE_INSENSITIVE_IN_OPERATORS,
        false,
    ),
+    crOnVariants: parseEnvVarBoolean(
+        process.env.UNLEASH_EXPERIMENTAL_CR_ON_VARIANTS,
+        false,
+    ),
 };

 export const defaultExperimentalOptions: IExperimentalOptions = {
--- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts
+++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts
@ -5,10 +5,12 @@ import { DEFAULT_ENV } from '../../../lib/util/constants';
 import { SegmentService } from '../../../lib/services/segment-service';
 import { FeatureStrategySchema } from '../../../lib/openapi/spec/feature-strategy-schema';
 import User from '../../../lib/types/user';
-import { IConstraint } from '../../../lib/types/model';
+import { IConstraint, IVariant } from '../../../lib/types/model';
 import { AccessService } from '../../../lib/services/access-service';
 import { GroupService } from '../../../lib/services/group-service';
 import EnvironmentService from '../../../lib/services/environment-service';
+import { NoAccessError } from '../../../lib/error';
+import { SKIP_CHANGE_REQUEST } from '../../../lib/types';

 let stores;
 let db;
@ -367,3 +369,165 @@ test('Cloning a feature toggle also clones segments correctly', async () => {
            .segments,
    ).toHaveLength(1);
 });
+
+test('If change requests are enabled, cannot change variants without going via CR', async () => {
+    const featureName = 'feature-with-variants-per-env-and-cr';
+    await service.createFeatureToggle(
+        'default',
+        { name: featureName },
+        'test-user',
+    );
+    const groupService = new GroupService(stores, unleashConfig);
+    const accessService = new AccessService(
+        stores,
+        unleashConfig,
+        groupService,
+    );
+    // Force all feature flags on to make sure we have Change requests on
+    const customFeatureService = new FeatureToggleService(
+        stores,
+        {
+            ...unleashConfig,
+            flagResolver: {
+                isEnabled: () => true,
+            },
+        },
+        segmentService,
+        accessService,
+    );
+
+    const newVariant: IVariant = {
+        name: 'cr-enabled',
+        weight: 100,
+        weightType: 'variable',
+        stickiness: 'default',
+    };
+    await db.rawDatabase('change_request_settings').insert({
+        project: 'default',
+        environment: 'default',
+    });
+    return expect(async () =>
+        customFeatureService.crProtectedSaveVariantsOnEnv(
+            'default',
+            featureName,
+            'default',
+            [newVariant],
+            {
+                createdAt: undefined,
+                email: '',
+                id: 0,
+                imageUrl: '',
+                loginAttempts: 0,
+                name: '',
+                permissions: [],
+                seenAt: undefined,
+                username: '',
+                generateImageUrl(): string {
+                    return '';
+                },
+                isAPI: true,
+            },
+            [],
+        ),
+    ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST));
+});
+
+test('If CRs are protected for any environment in the project stops bulk update of variants', async () => {
+    const project = await stores.projectStore.create({
+        id: 'crOnVariantsProject',
+        name: 'crOnVariantsProject',
+    });
+    const enabledEnv = await stores.environmentStore.create({
+        name: 'crenabledenv',
+        type: 'production',
+    });
+    const disabledEnv = await stores.environmentStore.create({
+        name: 'crdisabledenv',
+        type: 'production',
+    });
+
+    await stores.projectStore.addEnvironmentToProject(
+        project.id,
+        enabledEnv.name,
+    );
+    await stores.projectStore.addEnvironmentToProject(
+        project.id,
+        disabledEnv.name,
+    );
+    const groupService = new GroupService(stores, unleashConfig);
+    const accessService = new AccessService(
+        stores,
+        unleashConfig,
+        groupService,
+    );
+    // Force all feature flags on to make sure we have Change requests on
+    const customFeatureService = new FeatureToggleService(
+        stores,
+        {
+            ...unleashConfig,
+            flagResolver: {
+                isEnabled: () => true,
+            },
+        },
+        segmentService,
+        accessService,
+    );
+
+    const toggle = await service.createFeatureToggle(
+        project.id,
+        { name: 'crOnVariantToggle' },
+        'test-user',
+    );
+
+    const variant: IVariant = {
+        name: 'cr-enabled',
+        weight: 100,
+        weightType: 'variable',
+        stickiness: 'default',
+    };
+    await db.rawDatabase('change_request_settings').insert({
+        project: project.id,
+        environment: enabledEnv.name,
+    });
+
+    await customFeatureService.setVariantsOnEnvs(
+        project.id,
+        toggle.name,
+        [enabledEnv.name, disabledEnv.name],
+        [variant],
+        'test-user',
+    );
+
+    const newVariants = [
+        { ...variant, weight: 500 },
+        {
+            name: 'cr-enabled-2',
+            weight: 500,
+            weightType: 'fix',
+            stickiness: 'default',
+        },
+    ];
+    return expect(async () =>
+        customFeatureService.crProtectedSetVariantsOnEnvs(
+            project.id,
+            toggle.name,
+            [enabledEnv.name, disabledEnv.name],
+            newVariants,
+            {
+                createdAt: undefined,
+                email: '',
+                id: 0,
+                imageUrl: '',
+                loginAttempts: 0,
+                name: '',
+                permissions: [],
+                seenAt: undefined,
+                username: '',
+                generateImageUrl(): string {
+                    return '';
+                },
+                isAPI: true,
+            },
+        ),
+    ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST));
+});