From 3fb53737c6557740f5186d8e57c54f66b9baa83d Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 26 Apr 2024 07:21:29 +0200 Subject: [PATCH] feat: include CR envs enabled on creation in event and update validation (#6931) This PR improves the handling of change request enables on project creation in two ways: 1. We now verify that the envs you try to enable CRs for exist before passing them on to the enterprise functionality. 2. We include data about environments and change request environments in the project created events. --- .../features/project/project-service.test.ts | 274 +++++++++++++++++- src/lib/features/project/project-service.ts | 71 +++-- src/lib/services/project-schema.ts | 7 + src/lib/types/model.ts | 2 + 4 files changed, 326 insertions(+), 28 deletions(-) diff --git a/src/lib/features/project/project-service.test.ts b/src/lib/features/project/project-service.test.ts index 6b9fbf5c7f0..283ff64a5c8 100644 --- a/src/lib/features/project/project-service.test.ts +++ b/src/lib/features/project/project-service.test.ts @@ -1,15 +1,20 @@ import { createTestConfig } from '../../../test/config/test-config'; -import { RoleName, TEST_AUDIT_USER } from '../../types'; +import { BadDataError } from '../../error'; +import { type IBaseEvent, RoleName, TEST_AUDIT_USER } from '../../types'; import { createFakeProjectService } from './createProjectService'; describe('enterprise extension: enable change requests', () => { - test('it calls the change request enablement function', async () => { - expect.assertions(1); - + const createService = () => { const config = createTestConfig(); const service = createFakeProjectService(config); + // @ts-expect-error: we're setting this up to test the change request + service.flagResolver = { + isEnabled: () => true, + }; + // @ts-expect-error: we're setting this up to test the change request + service.isEnterprise = true; - // @ts-expect-error: if we don't set this up, the test will fail due to a missing role. + // @ts-expect-error: if we don't set this up, the tests will fail due to a missing role. service.accessService.createRole( { name: RoleName.OWNER, @@ -19,11 +24,19 @@ describe('enterprise extension: enable change requests', () => { TEST_AUDIT_USER, ); + return service; + }; + + test('it calls the change request enablement function on enterprise after creating the project', async () => { + expect.assertions(1); + const service = createService(); + const projectId = 'fake-project-id'; await service.createProject( { id: projectId, name: 'fake-project-name', + changeRequestEnvironments: [], }, { id: 5, @@ -40,4 +53,255 @@ describe('enterprise extension: enable change requests', () => { }, ); }); + + test("it does not call the change request enablement function if we're not enterprise", async () => { + const service = createService(); + // @ts-expect-error + service.isEnterprise = false; + + const fn = jest.fn(); + + const projectId = 'fake-project-id'; + await service.createProject( + { + id: projectId, + name: 'fake-project-name', + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + fn, + ); + + expect(fn).not.toHaveBeenCalled(); + }); + + test('the emitted event contains an empty list of changeRequestEnvironments if the input had none', async () => { + expect.assertions(1); + const service = createService(); + + const storeEvent = async (e: IBaseEvent) => { + expect(e.data.changeRequestEnvironments).toStrictEqual([]); + }; + + // @ts-expect-error: for testing purposes + service.eventService.storeEvent = storeEvent; + + const projectId = 'fake-project-id'; + await service.createProject( + { + id: projectId, + name: 'fake-project-name', + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + ); + }); + + test('the emitted event contains no changeRequestEnvironments if we are not on enterprise', async () => { + expect.assertions(1); + const service = createService(); + // @ts-expect-error + service.isEnterprise = false; + + const storeEvent = async (e: IBaseEvent) => { + expect('changeRequestEnvironments' in e.data).toBeFalsy(); + }; + + // @ts-expect-error: for testing purposes + service.eventService.storeEvent = storeEvent; + + const projectId = 'fake-project-id'; + await service.createProject( + { + id: projectId, + name: 'fake-project-name', + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + async () => { + []; + }, + ); + }); + + test('the emitted event contains the list of change request envs returned from the extension function, *not* what was passed in', async () => { + expect.assertions(1); + const service = createService(); + + // @ts-expect-error: we want this to pass to test the functionality + service.environmentStore.exists = () => true; + + const crEnvs = [{ name: 'prod', requiredApprovals: 10 }]; + + const storeEvent = async (e: IBaseEvent) => { + expect(e.data.changeRequestEnvironments).toMatchObject(crEnvs); + }; + + // @ts-expect-error: for testing purposes + service.eventService.storeEvent = storeEvent; + + const projectId = 'fake-project-id'; + await service.createProject( + { + id: projectId, + name: 'fake-project-name', + changeRequestEnvironments: [ + { name: 'dev', requiredApprovals: 1 }, + ], + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + async () => crEnvs, + ); + }); + + test('the create project function returns the list of change request envs returned from the extension function, *not* what was passed in', async () => { + const service = createService(); + + const crEnvs = [{ name: 'prod', requiredApprovals: 10 }]; + + // @ts-expect-error: we want this to pass to test the functionality + service.environmentStore.exists = () => true; + + const projectId = 'fake-project-id'; + const result = await service.createProject( + { + id: projectId, + name: 'fake-project-name', + changeRequestEnvironments: [ + { name: 'dev', requiredApprovals: 1 }, + ], + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + async () => crEnvs, + ); + + expect(result.changeRequestEnvironments).toStrictEqual(crEnvs); + }); + + test('the create project function defaults to returning an empty list if the input had no cr envs', async () => { + const service = createService(); + + const projectId = 'fake-project-id'; + const result = await service.createProject( + { + id: projectId, + name: 'fake-project-name', + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + async () => { + return; + }, + ); + + expect(result.changeRequestEnvironments).toStrictEqual([]); + }); + + test('the create project function does not return a list of change request envs if we are not on enterprise', async () => { + const service = createService(); + // @ts-expect-error + service.isEnterprise = false; + + const crEnvs = [{ name: 'prod', requiredApprovals: 10 }]; + + const projectId = 'fake-project-id'; + const result = await service.createProject( + { + id: projectId, + name: 'fake-project-name', + changeRequestEnvironments: [ + { name: 'dev', requiredApprovals: 1 }, + ], + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + async () => { + crEnvs; + }, + ); + + expect('changeRequestEnvironments' in result).toBeFalsy(); + }); + + test("it throws an error if you provide it with environments that don't exist", async () => { + const service = createService(); + // @ts-expect-error + service.environmentStore.exists = () => false; + + const projectId = 'fake-project-id'; + expect( + service.createProject( + { + id: projectId, + name: 'fake-project-name', + changeRequestEnvironments: [ + { name: 'dev', requiredApprovals: 1 }, + ], + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + ), + ).rejects.toThrow(BadDataError); + }); + + test("it does not throw if an error if you provide it with environments that don't exist but aren't on enterprise", async () => { + const service = createService(); + // @ts-expect-error + service.isEnterprise = false; + // @ts-expect-error + service.environmentStore.exists = () => false; + + const projectId = 'fake-project-id'; + expect( + service.createProject( + { + id: projectId, + name: 'fake-project-name', + changeRequestEnvironments: [ + { name: 'dev', requiredApprovals: 1 }, + ], + }, + { + id: 5, + permissions: [], + isAPI: false, + }, + TEST_AUDIT_USER, + ), + ).resolves.toBeTruthy(); + }); }); diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 0723adfc8dc..e1e55faebad 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -248,6 +248,27 @@ export default class ProjectService { return featureNaming; }; + private async validateEnvironmentsExist(environments: string[]) { + const projectsAndExistence = await Promise.all( + environments.map(async (env) => [ + env, + await this.environmentStore.exists(env), + ]), + ); + + const invalidEnvs = projectsAndExistence + .filter(([_, exists]) => !exists) + .map(([env]) => env); + + if (invalidEnvs.length > 0) { + throw new BadDataError( + `These environments do not exist: ${invalidEnvs + .map((env) => `'${env}'`) + .join(', ')}.`, + ); + } + } + async validateProjectEnvironments(environments: string[] | undefined) { if ( this.flagResolver.isEnabled('createProjectWithEnvironmentConfig') && @@ -259,24 +280,7 @@ export default class ProjectService { ); } - const projectsAndExistence = await Promise.all( - environments.map(async (env) => [ - env, - await this.environmentStore.exists(env), - ]), - ); - - const invalidEnvs = projectsAndExistence - .filter(([_, exists]) => !exists) - .map(([env]) => env); - - if (invalidEnvs.length > 0) { - throw new BadDataError( - `These environments do not exist and can not be selected for the project: ${invalidEnvs - .map((env) => `'${env}'`) - .join(', ')}.`, - ); - } + await this.validateEnvironmentsExist(environments); } } @@ -284,12 +288,18 @@ export default class ProjectService { newProject: CreateProject, user: IUser, auditUser: IAuditUser, - enableChangeRequestsForSpecifiedEnvironments: () => Promise = async () => {}, + enableChangeRequestsForSpecifiedEnvironments: ( + environments: CreateProject['changeRequestEnvironments'], + ) => Promise< + void | ProjectCreated['changeRequestEnvironments'] + > = async () => { + return; + }, ): Promise { await this.validateProjectEnvironments(newProject.environments); const validatedData = await projectSchema.validateAsync(newProject); - const data = this.removeModeForNonEnterprise(validatedData); + const data = this.removePropertiesForNonEnterprise(validatedData); await this.validateUniqueId(data.id); await this.projectStore.create(data); @@ -310,7 +320,22 @@ export default class ProjectService { }), ); - await enableChangeRequestsForSpecifiedEnvironments(); + if ( + this.isEnterprise && + this.flagResolver.isEnabled('createProjectWithEnvironmentConfig') + ) { + // todo: this is a workaround for backwards compatibility + // (i.e. not breaking enterprise tests) that we can change + // once these changes have been merged and enterprise + // updated. Instead, we can exit early if there are no cr + // envs + const crEnvs = newProject.changeRequestEnvironments || []; + await this.validateEnvironmentsExist(crEnvs.map((env) => env.name)); + const changeRequestEnvironments = + await enableChangeRequestsForSpecifiedEnvironments(crEnvs); + + data.changeRequestEnvironments = changeRequestEnvironments ?? []; + } await this.accessService.createDefaultProjectRoles(user, data.id); @@ -1357,11 +1382,11 @@ export default class ProjectService { } // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types - removeModeForNonEnterprise(data): any { + removePropertiesForNonEnterprise(data): any { if (this.isEnterprise) { return data; } - const { mode, ...proData } = data; + const { mode, changeRequestEnvironments, ...proData } = data; return proData; } } diff --git a/src/lib/services/project-schema.ts b/src/lib/services/project-schema.ts index 440899f0b35..49085efb137 100644 --- a/src/lib/services/project-schema.ts +++ b/src/lib/services/project-schema.ts @@ -18,5 +18,12 @@ export const projectSchema = joi example: joi.string().allow(null).allow('').optional(), description: joi.string().allow(null).allow('').optional(), }), + environments: joi.array().items(joi.string()), + changeRequestEnvironments: joi.array().items( + joi.object({ + name: joi.string(), + requiredApprovals: joi.number(), + }), + ), }) .options({ allowUnknown: false, stripUnknown: true }); diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index e5ba9fdd924..5c572d312c4 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -483,6 +483,7 @@ export type CreateProject = Pick & { mode?: ProjectMode; defaultStickiness?: string; environments?: string[]; + changeRequestEnvironments?: { name: string; requiredApprovals?: number }[]; }; // Create project aligns with #/components/schemas/projectCreatedSchema @@ -496,6 +497,7 @@ export type ProjectCreated = Pick< | 'featureLimit' > & { environments: string[]; + changeRequestEnvironments?: { name: string; requiredApprovals: number }[]; }; export interface IProject {