From 8c1d4838b8a14b33009d04dde08f474fc50c5825 Mon Sep 17 00:00:00 2001 From: olav Date: Wed, 18 May 2022 11:07:01 +0200 Subject: [PATCH] fix: require equal environments when moving toggles (#1595) * fix: apply query from checkProjectsCompatibility * fix: require equal environments when moving toggles * refactor: clean up project service tests * refactor: add test for project compatibility check * refactor: improve arraysHaveSameItems name --- src/lib/db/feature-environment-store.ts | 9 ++- src/lib/services/project-service.ts | 11 ++- src/lib/util/arraysHaveSameItems.test.ts | 17 ++++ src/lib/util/arraysHaveSameItems.ts | 12 +++ .../e2e/services/project-service.e2e.test.ts | 79 +++++++++++-------- 5 files changed, 91 insertions(+), 37 deletions(-) create mode 100644 src/lib/util/arraysHaveSameItems.test.ts create mode 100644 src/lib/util/arraysHaveSameItems.ts diff --git a/src/lib/db/feature-environment-store.ts b/src/lib/db/feature-environment-store.ts index dbf27a4aaf..1ee7ae7600 100644 --- a/src/lib/db/feature-environment-store.ts +++ b/src/lib/db/feature-environment-store.ts @@ -86,9 +86,12 @@ export class FeatureEnvironmentStore implements IFeatureEnvironmentStore { ); } - async getAll(): Promise { - const rows = await this.db(T.featureEnvs); - return rows.map((r) => ({ + async getAll(query?: Object): Promise { + let rows = this.db(T.featureEnvs); + if (query) { + rows = rows.where(query); + } + return (await rows).map((r) => ({ enabled: r.enabled, featureName: r.feature_name, environment: r.environment, diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index 35fa3b6d12..104a3c7776 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -38,6 +38,7 @@ import { DEFAULT_PROJECT } from '../types/project'; import { IFeatureTagStore } from 'lib/types/stores/feature-tag-store'; import ProjectWithoutOwnerError from '../error/project-without-owner-error'; import { IUserStore } from 'lib/types/stores/user-store'; +import { arraysHaveSameItems } from '../util/arraysHaveSameItems'; const getCreatedBy = (user: User) => user.email || user.username; @@ -115,7 +116,10 @@ export default class ProjectService { return this.store.get(id); } - async createProject(newProject: IProject, user: User): Promise { + async createProject( + newProject: Pick, + user: User, + ): Promise { const data = await projectSchema.validateAsync(newProject); await this.validateUniqueId(data.id); @@ -172,8 +176,9 @@ export default class ProjectService { const newEnvs = await this.store.getEnvironmentsForProject( newProjectId, ); - return featureEnvs.every( - (e) => !e.enabled || newEnvs.includes(e.environment), + return arraysHaveSameItems( + featureEnvs.map((env) => env.environment), + newEnvs, ); } diff --git a/src/lib/util/arraysHaveSameItems.test.ts b/src/lib/util/arraysHaveSameItems.test.ts new file mode 100644 index 0000000000..bdb1b04ad4 --- /dev/null +++ b/src/lib/util/arraysHaveSameItems.test.ts @@ -0,0 +1,17 @@ +import { arraysHaveSameItems } from './arraysHaveSameItems'; + +test('arraysHaveSameItems', () => { + expect(arraysHaveSameItems([], [])).toEqual(true); + expect(arraysHaveSameItems([1], [1])).toEqual(true); + expect(arraysHaveSameItems([1], [1, 1])).toEqual(true); + expect(arraysHaveSameItems([1, 1], [1])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [1, 2])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [2, 1])).toEqual(true); + expect(arraysHaveSameItems([1, 2], [2, 2, 1])).toEqual(true); + expect(arraysHaveSameItems([1], [])).toEqual(false); + expect(arraysHaveSameItems([1], [2])).toEqual(false); + expect(arraysHaveSameItems([1, 2], [1, 3])).toEqual(false); + expect(arraysHaveSameItems([1, 2], [1, 2, 3])).toEqual(false); + expect(arraysHaveSameItems([1, 2, 3], [1, 2])).toEqual(false); + expect(arraysHaveSameItems([1, 2, 3], [1, 2, 4])).toEqual(false); +}); diff --git a/src/lib/util/arraysHaveSameItems.ts b/src/lib/util/arraysHaveSameItems.ts new file mode 100644 index 0000000000..b7a098e3c6 --- /dev/null +++ b/src/lib/util/arraysHaveSameItems.ts @@ -0,0 +1,12 @@ +export const arraysHaveSameItems = (a: T[], b: T[]): boolean => { + const setA = new Set(a); + const setB = new Set(b); + + if (setA.size !== setB.size) { + return false; + } + + return [...setA].every((itemA) => { + return setB.has(itemA); + }); +}; diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index ee3faf6f48..a7f26bbf15 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -6,12 +6,16 @@ import { AccessService } from '../../../lib/services/access-service'; import { MOVE_FEATURE_TOGGLE } from '../../../lib/types/permissions'; import { createTestConfig } from '../../config/test-config'; import { RoleName } from '../../../lib/types/model'; +import { randomId } from '../../../lib/util/random-id'; +import EnvironmentService from '../../../lib/services/environment-service'; +import IncompatibleProjectError from '../../../lib/error/incompatible-project-error'; let stores; let db: ITestDb; -let projectService; -let accessService; +let projectService: ProjectService; +let accessService: AccessService; +let environmentService: EnvironmentService; let featureToggleService: FeatureToggleService; let user; @@ -29,6 +33,7 @@ beforeAll(async () => { }); accessService = new AccessService(stores, config); featureToggleService = new FeatureToggleService(stores, config); + environmentService = new EnvironmentService(stores, config); projectService = new ProjectService( stores, config, @@ -209,7 +214,7 @@ test('should get list of users with access to project', async () => { description: 'Blah', }; await projectService.createProject(project, user); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const member = await stores.roleStore.getRoleByName(RoleName.MEMBER); const owner = await stores.roleStore.getRoleByName(RoleName.OWNER); @@ -243,7 +248,7 @@ test('should add a member user to the project', async () => { await projectService.addUser(project.id, memberRole.id, projectMember1.id); await projectService.addUser(project.id, memberRole.id, projectMember2.id); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const memberUsers = users.filter((u) => u.roleId === memberRole.id); expect(memberUsers).toHaveLength(2); @@ -275,7 +280,7 @@ test('should add admin users to the project', async () => { await projectService.addUser(project.id, ownerRole.id, projectAdmin1.id); await projectService.addUser(project.id, ownerRole.id, projectAdmin2.id); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const adminUsers = users.filter((u) => u.roleId === ownerRole.id); @@ -332,7 +337,7 @@ test('should remove user from the project', async () => { projectMember1.id, ); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const memberUsers = users.filter((u) => u.roleId === memberRole.id); expect(memberUsers).toHaveLength(0); @@ -444,34 +449,47 @@ test('should fail if user is not authorized', async () => { }); test('should change project when checks pass', async () => { - const project = { - id: 'test-change-project-4', - name: 'New project', - description: 'Blah', - }; - - const projectDestination = { - id: 'test-change-project-dest-2', - name: 'New project 2', - description: 'Blah', - }; - - const toggle = { name: 'test-toggle-4' }; - - await projectService.createProject(project, user); - await projectService.createProject(projectDestination, user); - await featureToggleService.createFeatureToggle(project.id, toggle, user); + const projectA = { id: randomId(), name: randomId() }; + const projectB = { id: randomId(), name: randomId() }; + const toggle = { name: randomId() }; + await projectService.createProject(projectA, user); + await projectService.createProject(projectB, user); + await featureToggleService.createFeatureToggle(projectA.id, toggle, user); await projectService.changeProject( - projectDestination.id, + projectB.id, toggle.name, user, - project.id, + projectA.id, ); const updatedFeature = await featureToggleService.getFeature(toggle.name); + expect(updatedFeature.project).toBe(projectB.id); +}); - expect(updatedFeature.project).toBe(projectDestination.id); +test('should require equal project environments to move features', async () => { + const projectA = { id: randomId(), name: randomId() }; + const projectB = { id: randomId(), name: randomId() }; + const environment = { name: randomId(), type: 'production' }; + const toggle = { name: randomId() }; + + await projectService.createProject(projectA, user); + await projectService.createProject(projectB, user); + await featureToggleService.createFeatureToggle(projectA.id, toggle, user); + await stores.environmentStore.create(environment); + await environmentService.addEnvironmentToProject( + environment.name, + projectB.id, + ); + + await expect(() => + projectService.changeProject( + projectB.id, + toggle.name, + user, + projectA.id, + ), + ).rejects.toThrowError(IncompatibleProjectError); }); test('A newly created project only gets connected to enabled environments', async () => { @@ -536,7 +554,7 @@ test('should add a user to the project with a custom role', async () => { await projectService.addUser(project.id, customRole.id, projectMember1.id); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const customRoleMember = users.filter((u) => u.roleId === customRole.id); @@ -633,7 +651,7 @@ test('should change a users role in the project', async () => { const member = await stores.roleStore.getRoleByName(RoleName.MEMBER); await projectService.addUser(project.id, member.id, projectUser.id); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); let memberUser = users.filter((u) => u.roleId === member.id); expect(memberUser).toHaveLength(1); @@ -644,7 +662,6 @@ test('should change a users role in the project', async () => { let { users: updatedUsers } = await projectService.getUsersWithAccess( project.id, - user, ); const customUser = updatedUsers.filter((u) => u.roleId === customRole.id); @@ -677,7 +694,7 @@ test('should update role for user on project', async () => { 'test', ); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const memberUsers = users.filter((u) => u.roleId === memberRole.id); const ownerUsers = users.filter((u) => u.roleId === ownerRole.id); @@ -714,7 +731,7 @@ test('should able to assign role without existing members', async () => { 'test', ); - const { users } = await projectService.getUsersWithAccess(project.id, user); + const { users } = await projectService.getUsersWithAccess(project.id); const memberUsers = users.filter((u) => u.roleId === memberRole.id); const testUsers = users.filter((u) => u.roleId === testRole.id);