From 2caab45801f2d1d6ee431f413efd84b210c95883 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 27 Mar 2023 13:21:50 +0200 Subject: [PATCH] feat: disallow clone toggle on change request enabled (#3383) --- .../feature/CopyFeature/CopyFeature.tsx | 10 ++++- .../change-request-access-read-model.ts | 1 + .../fake-change-request-access-read-model.ts | 4 ++ .../sql-change-request-access-read-model.ts | 14 +++++++ src/lib/services/feature-toggle-service.ts | 11 +++++- .../api/admin/project/features.e2e.test.ts | 16 +++++--- .../feature-toggle-service-v2.e2e.test.ts | 39 ++++++++++++++++--- 7 files changed, 81 insertions(+), 14 deletions(-) diff --git a/frontend/src/component/feature/CopyFeature/CopyFeature.tsx b/frontend/src/component/feature/CopyFeature/CopyFeature.tsx index 6e53465f36..995d10297a 100644 --- a/frontend/src/component/feature/CopyFeature/CopyFeature.tsx +++ b/frontend/src/component/feature/CopyFeature/CopyFeature.tsx @@ -18,6 +18,7 @@ import { getTogglePath } from 'utils/routePathHelpers'; import useFeatureApi from 'hooks/api/actions/useFeatureApi/useFeatureApi'; import { useFeature } from 'hooks/api/getters/useFeature/useFeature'; import { useRequiredPathParam } from 'hooks/useRequiredPathParam'; +import { useChangeRequestsEnabled } from '../../../hooks/useChangeRequestsEnabled'; const StyledPage = styled(Paper)(({ theme }) => ({ overflow: 'visible', @@ -65,6 +66,8 @@ export const CopyFeatureToggle = () => { const projectId = useRequiredPathParam('projectId'); const { feature } = useFeature(projectId, featureId); const navigate = useNavigate(); + const { isChangeRequestConfiguredInAnyEnv } = + useChangeRequestsEnabled(projectId); const setValue: ChangeEventHandler = event => { const value = trim(event.target.value); @@ -152,7 +155,12 @@ export const CopyFeatureToggle = () => { label="Replace groupId" /> - diff --git a/src/lib/features/change-request-access-service/change-request-access-read-model.ts b/src/lib/features/change-request-access-service/change-request-access-read-model.ts index 041b8c6abf..8ca9d977e4 100644 --- a/src/lib/features/change-request-access-service/change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/change-request-access-read-model.ts @@ -10,4 +10,5 @@ export interface IChangeRequestAccessReadModel { project: string, environment: string, ): Promise; + isChangeRequestsEnabledForProject(project: string): Promise; } diff --git a/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts index 036249db6f..53773dbf87 100644 --- a/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/fake-change-request-access-read-model.ts @@ -19,4 +19,8 @@ export class FakeChangeRequestAccessReadModel public async isChangeRequestsEnabled(): Promise { return this.isChangeRequestEnabled; } + + public async isChangeRequestsEnabledForProject(): Promise { + return this.isChangeRequestEnabled; + } } diff --git a/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts index 12edcddd3d..a4cde718e5 100644 --- a/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts +++ b/src/lib/features/change-request-access-service/sql-change-request-access-read-model.ts @@ -49,4 +49,18 @@ export class ChangeRequestAccessReadModel const { present } = result.rows[0]; return present; } + + public async isChangeRequestsEnabledForProject( + project: string, + ): Promise { + const result = await this.db.raw( + `SELECT EXISTS(SELECT 1 + FROM change_request_settings + WHERE project = ? + ) AS present`, + [project], + ); + const { present } = result.rows[0]; + return present; + } } diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 98f8db2bcf..079ed21313 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -882,6 +882,15 @@ class FeatureToggleService { replaceGroupId: boolean = true, // eslint-disable-line userName: string, ): Promise { + const changeRequestEnabled = + await this.changeRequestAccessReadModel.isChangeRequestsEnabledForProject( + projectId, + ); + if (changeRequestEnabled) { + throw new NoAccessError( + `Cloning not allowed. Project ${projectId} has change requests enabled.`, + ); + } this.logger.info( `${userName} clones feature toggle ${featureName} to ${newFeatureName}`, ); @@ -1754,7 +1763,7 @@ class FeatureToggleService { project: string, environment: string, featureName: string, - user: User, + user?: User, ) { const hasEnvironment = await this.featureEnvironmentStore.featureHasEnvironment( 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 23fab8a943..ead0637524 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -83,14 +83,18 @@ const updateStrategy = async ( beforeAll(async () => { db = await dbInit('feature_strategy_api_serial', getLogger); - app = await setupAppWithCustomConfig(db.stores, { - experimental: { - flags: { - strictSchemaValidation: true, - bulkOperations: true, + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + bulkOperations: true, + }, }, }, - }); + db.rawDatabase, + ); }); afterEach(async () => { 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 ae8727287f..51236152ef 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 @@ -29,6 +29,8 @@ const mockConstraints = (): IConstraint[] => { })); }; +const irrelevantDate = new Date(); + beforeAll(async () => { const config = createTestConfig(); db = await dbInit( @@ -54,9 +56,14 @@ beforeAll(async () => { }); afterAll(async () => { + await db.rawDatabase('change_request_settings').del(); await db.destroy(); }); +beforeEach(async () => { + await db.rawDatabase('change_request_settings').del(); +}); + test('Should create feature toggle strategy configuration', async () => { const projectId = 'default'; const username = 'feature-toggle'; @@ -263,7 +270,7 @@ test('adding and removing an environment preserves variants when variants per en const toggle = await service.getFeature({ featureName, - projectId: null, + projectId: undefined, environmentVariants: false, }); expect(toggle.variants).toHaveLength(1); @@ -327,6 +334,26 @@ test('cloning a feature toggle copies variant environments correctly', async () expect(newEnv.variants).toHaveLength(1); }); +test('cloning a feature toggle not allowed for change requests enabled', async () => { + await db.rawDatabase('change_request_settings').insert({ + project: 'default', + environment: 'default', + }); + await expect( + service.cloneFeatureToggle( + 'newToggleName', + 'default', + 'clonedToggleName', + true, + 'test-user', + ), + ).rejects.toEqual( + new NoAccessError( + `Cloning not allowed. Project default has change requests enabled.`, + ), + ); +}); + test('Cloning a feature toggle also clones segments correctly', async () => { const featureName = 'ToggleWithSegments'; const clonedFeatureName = 'AWholeNewFeatureToggle'; @@ -372,7 +399,7 @@ test('Cloning a feature toggle also clones segments correctly', async () => { let feature = await service.getFeature({ featureName: clonedFeatureName }); expect( - feature.environments.find((x) => x.name === 'default').strategies[0] + feature.environments.find((x) => x.name === 'default')?.strategies[0] .segments, ).toHaveLength(1); }); @@ -425,14 +452,14 @@ test('If change requests are enabled, cannot change variants without going via C 'default', [newVariant], { - createdAt: undefined, + createdAt: irrelevantDate, email: '', id: 0, imageUrl: '', loginAttempts: 0, name: '', permissions: [], - seenAt: undefined, + seenAt: irrelevantDate, username: '', generateImageUrl(): string { return ''; @@ -532,14 +559,14 @@ test('If CRs are protected for any environment in the project stops bulk update [enabledEnv.name, disabledEnv.name], newVariants, { - createdAt: undefined, + createdAt: irrelevantDate, email: '', id: 0, imageUrl: '', loginAttempts: 0, name: '', permissions: [], - seenAt: undefined, + seenAt: irrelevantDate, username: '', generateImageUrl(): string { return '';