refactor(role): remove transactions and tidy up logic in model (#2141)

* refactor(role): remove returning this in model methods

* refactor(role): assert altering admin in model before update and delete

* refactor(role): rename overridePermissions with updatePermissions in model

* refactor(role): remove transactions in model

* refactor(role): remove transactions in model

* refactor(role): return with permissions upon update in model

* fix(role): assert admin check on old instance in model

* refactor(role): fetch and use current role in preventAlteringAdmin

packages/backend/src/controllers/api/v1/admin/roles/delete-role.ee.test.js

@@ -92,21 +92,4 @@ describe('DELETE /api/v1/admin/roles/:roleId', () => {
       },
     });
   });
-
-  it('should not delete role and permissions on unsuccessful response', async () => {
-    const role = await createRole();
-    const permission = await createPermission({ roleId: role.id });
-    await createUser({ roleId: role.id });
-
-    await request(app)
-      .delete(`/api/v1/admin/roles/${role.id}`)
-      .set('Authorization', token)
-      .expect(422);
-
-    const refetchedRole = await role.$query();
-    const refetchedPermission = await permission.$query();
-
-    expect(refetchedRole).toStrictEqual(role);
-    expect(refetchedPermission).toStrictEqual(permission);
-  });
 });

packages/backend/src/models/role.js

@@ -52,17 +52,19 @@ class Role extends Base {
     return await this.query().findOne({ name: 'Admin' });
   }
 
-  preventAlteringAdmin() {
-    if (this.isAdmin) {
+  async preventAlteringAdmin() {
+    const currentRole = await Role.query().findById(this.id);
+
+    if (currentRole.isAdmin) {
       throw new NotAuthorizedError('The admin role cannot be altered!');
     }
   }
 
-  async deletePermissions(trx) {
-    return await this.$relatedQuery('permissions', trx).delete();
+  async deletePermissions() {
+    return await this.$relatedQuery('permissions').delete();
   }
 
-  async createPermissions(permissions, trx) {
+  async createPermissions(permissions) {
     if (permissions?.length) {
       const validPermissions = Permission.filter(permissions).map(
         (permission) => ({
@@ -71,50 +73,40 @@ class Role extends Base {
         })
       );
 
-      await Permission.query(trx).insert(validPermissions);
+      await Permission.query().insert(validPermissions);
+    }
   }
 
-    return this;
-  }
+  async updatePermissions(permissions) {
+    await this.deletePermissions();
 
-  async overridePermissions(permissions, trx) {
-    await this.deletePermissions(trx);
-
-    await this.createPermissions(permissions, trx);
-
-    return this;
+    await this.createPermissions(permissions);
   }
 
   async updateWithPermissions(data) {
-    this.preventAlteringAdmin();
-
     const { name, description, permissions } = data;
 
-    return await Role.transaction(async (trx) => {
-      await this.overridePermissions(permissions, trx);
+    await this.updatePermissions(permissions);
 
-      await this.$query(trx).patch({
+    await this.$query().patchAndFetch({
+      id: this.id,
       name,
       description,
     });
 
-      // TODO: consider removing returning permissions as they're not utilized
-      return await this.$query(trx)
+    return await this.$query()
       .leftJoinRelated({
         permissions: true,
       })
       .withGraphFetched({
         permissions: true,
       });
-    });
   }
 
   async deleteWithPermissions() {
-    return await Role.transaction(async (trx) => {
-      await this.deletePermissions(trx);
+    await this.deletePermissions();
 
-      return await this.$query(trx).delete();
-    });
+    return await this.$query().delete();
   }
 
   async assertNoRoleUserExists() {
@@ -164,10 +156,16 @@ class Role extends Base {
     await this.assertNoConfigurationUsage();
   }
 
+  async $beforeUpdate(opt, queryContext) {
+    await super.$beforeUpdate(opt, queryContext);
+
+    await this.preventAlteringAdmin();
+  }
+
   async $beforeDelete(queryContext) {
     await super.$beforeDelete(queryContext);
 
-    this.preventAlteringAdmin();
+    await this.preventAlteringAdmin();
 
     await this.assertRoleIsNotUsed();
   }

packages/backend/src/models/role.test.js

@@ -71,25 +71,22 @@ describe('Role model', () => {
   });
 
   describe('preventAlteringAdmin', () => {
-    it('preventAlteringAdmin should throw an error when altering admin role', () => {
-      const role = new Role();
-      role.name = 'Admin';
+    it('preventAlteringAdmin should throw an error when altering admin role', async () => {
+      const role = await createRole({ name: 'Admin' });
 
-      expect(() => role.preventAlteringAdmin()).toThrowError(
+      await expect(() => role.preventAlteringAdmin()).rejects.toThrowError(
         'The admin role cannot be altered!'
       );
     });
 
-    it('preventAlteringAdmin should not throw an error when altering non-admin roles', () => {
-      const role = new Role();
-      role.name = 'User';
+    it('preventAlteringAdmin should not throw an error when altering non-admin roles', async () => {
+      const role = await createRole({ name: 'User' });
 
-      expect(() => role.preventAlteringAdmin()).not.toThrowError();
+      expect(await role.preventAlteringAdmin()).toBe(undefined);
     });
   });
 
-  describe('deletePermissions', () => {
-    it("should delete role's permissions", async () => {
+  it("deletePermissions should delete role's permissions", async () => {
     const role = await createRole({ name: 'User' });
     await createPermission({ roleId: role.id });
 
@@ -98,22 +95,6 @@ describe('Role model', () => {
     expect(await role.$relatedQuery('permissions')).toStrictEqual([]);
   });
 
-    it('should accept transaction', async () => {
-      const transaction = vi.fn();
-
-      const relatedQuerySpy = vi
-        .spyOn(Role.prototype, '$relatedQuery')
-        .mockReturnValue({ delete: () => {} });
-
-      const role = await createRole({ name: 'User' });
-
-      await role.deletePermissions(transaction);
-
-      expect(relatedQuerySpy).toHaveBeenCalledWith('permissions', transaction);
-      1;
-    });
-  });
-
   describe('createPermissions', () => {
     it('should create permissions', async () => {
       const role = await createRole({ name: 'User' });
@@ -144,32 +125,13 @@ describe('Role model', () => {
 
       expect(permissionFilterSpy).toHaveBeenCalledWith(permissions);
     });
-
-    it('should accept transaction', async () => {
-      const transaction = vi.fn();
-
-      const permissionQuerySpy = vi
-        .spyOn(Permission, 'query')
-        .mockReturnValue({ insert: () => {} });
-
-      const role = await createRole({ name: 'User' });
-
-      await role.createPermissions(
-        [{ action: 'read', subject: 'Flow', conditions: [] }],
-        transaction
-      );
-
-      expect(permissionQuerySpy).toHaveBeenCalledWith(transaction);
-      1;
-    });
   });
 
-  it('overridePermissions should delete existing permissions and create new permissions', async () => {
+  it('updatePermissions should delete existing permissions and create new permissions', async () => {
     const permissionsData = [
       { action: 'read', subject: 'Flow', conditions: [] },
     ];
 
-    const transaction = vi.fn();
     const deletePermissionsSpy = vi
       .spyOn(Role.prototype, 'deletePermissions')
       .mockResolvedValueOnce();
@@ -179,18 +141,14 @@ describe('Role model', () => {
 
     const role = await createRole({ name: 'User' });
 
-    await role.overridePermissions(permissionsData, transaction);
+    await role.updatePermissions(permissionsData);
 
     expect(deletePermissionsSpy.mock.invocationCallOrder[0]).toBeLessThan(
       createPermissionsSpy.mock.invocationCallOrder[0]
     );
 
-    expect(deletePermissionsSpy).toHaveBeenNthCalledWith(1, transaction);
-    expect(createPermissionsSpy).toHaveBeenNthCalledWith(
-      1,
-      permissionsData,
-      transaction
-    );
+    expect(deletePermissionsSpy).toHaveBeenNthCalledWith(1);
+    expect(createPermissionsSpy).toHaveBeenNthCalledWith(1, permissionsData);
   });
 
   describe('updateWithPermissions', () => {
@@ -224,83 +182,6 @@ describe('Role model', () => {
 
       expect(roleWithPermissions).toMatchObject(newRoleData);
     });
-
-    it('should throw an error while updating the admin role', async () => {
-      const role = new Role();
-      role.name = 'Admin';
-
-      await expect(() => role.updateWithPermissions()).rejects.toThrowError(
-        'The admin role cannot be altered!'
-      );
-    });
-
-    it('should use transaction', async () => {
-      const transaction = vi.fn();
-      const overridePermissionsSpy = vi
-        .spyOn(Role.prototype, 'overridePermissions')
-        .mockResolvedValue();
-
-      const querySpy = vi.spyOn(Role.prototype, '$query').mockReturnValue({
-        patch: vi.fn().mockReturnValue(Promise.resolve()),
-        leftJoinRelated: vi.fn().mockReturnThis(),
-        withGraphFetched: vi.fn().mockResolvedValue({}),
-      });
-
-      const transactionSpy = vi
-        .spyOn(Role, 'transaction')
-        .mockImplementation(async (callback) => {
-          return await callback(transaction);
-        });
-      const role = await createRole({ name: 'User' });
-
-      const newRoleData = {
-        name: 'New user',
-        description: 'Updated user role',
-        permissions: [{ action: 'read', subject: 'Flow', conditions: [] }],
-      };
-
-      await role.updateWithPermissions(newRoleData);
-
-      expect(transactionSpy).toHaveBeenCalledOnce();
-
-      expect(overridePermissionsSpy).toHaveBeenCalledWith(
-        newRoleData.permissions,
-        transaction
-      );
-
-      expect(querySpy).toHaveBeenNthCalledWith(2, transaction);
-    });
-
-    it('should revert changes when an error occurs', async () => {
-      const role = await createRole({ name: 'User' });
-      await createPermission({
-        roleId: role.id,
-        subject: 'Flow',
-        action: 'read',
-        conditions: [],
-      });
-
-      const roleWithPermissions = await role
-        .$query()
-        .leftJoinRelated({ permissions: true })
-        .withGraphFetched({ permissions: true });
-
-      await expect(() =>
-        role.updateWithPermissions({
-          name: false,
-          description: 123,
-        })
-      ).rejects.toThrowError(
-        'name: must be string, description: must be string,null'
-      );
-
-      const refetchedRoleWithPermissions = await role
-        .$query()
-        .leftJoinRelated({ permissions: true })
-        .withGraphFetched({ permissions: true });
-
-      expect(roleWithPermissions).toStrictEqual(refetchedRoleWithPermissions);
-    });
   });
 
   describe('deleteWithPermissions', () => {
@@ -323,32 +204,6 @@ describe('Role model', () => {
       expect(refetchedRole).toBe(undefined);
       expect(rolePermissions).toStrictEqual([]);
     });
-
-    it('should use transaction', async () => {
-      const transaction = vi.fn();
-      const deletePermissionsSpy = vi
-        .spyOn(Role.prototype, 'deletePermissions')
-        .mockResolvedValue();
-
-      const querySpy = vi.spyOn(Role.prototype, '$query').mockReturnValue({
-        delete: vi.fn().mockReturnValue(Promise.resolve()),
-      });
-
-      const transactionSpy = vi
-        .spyOn(Role, 'transaction')
-        .mockImplementation(async (callback) => {
-          return await callback(transaction);
-        });
-      const role = await createRole({ name: 'User' });
-
-      await role.deleteWithPermissions();
-
-      expect(transactionSpy).toHaveBeenCalledOnce();
-
-      expect(deletePermissionsSpy).toHaveBeenCalledWith(transaction);
-
-      expect(querySpy).toHaveBeenCalledWith(transaction);
-    });
   });
 
   describe('assertNoRoleUserExists', () => {