Skip to content

Commit

Permalink
N21-1553 Fix deletion of board elements, when the external tool does …
Browse files Browse the repository at this point in the history 
…not exist anymore (#4591)
  • Loading branch information
@MarvinOehlerkingCap @bergatco
MarvinOehlerkingCap authored and bergatco committed Dec 6, 2023
1 parent e1d062c commit acdd957
Show file tree
Hide file tree
Showing 16 changed files with 225 additions and 62 deletions.
94 changes: 68 additions & 26 deletions apps/server/src/modules/board/repo/recursive-delete.visitor.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { FileRecordParentType } from '@infra/rabbitmq';
import { EntityManager } from '@mikro-orm/mongodb';
import { EntityManager, ObjectId } from '@mikro-orm/mongodb';
import { FileDto, FilesStorageClientAdapterService } from '@modules/files-storage-client';
import { DrawingElementAdapterService } from '@modules/tldraw-client/service/drawing-element-adapter.service';
import { ContextExternalToolService } from '@modules/tool/context-external-tool/service';
import { Test, TestingModule } from '@nestjs/testing';
import {
Expand All @@ -16,7 +17,6 @@ import {
submissionContainerElementFactory,
submissionItemFactory,
} from '@shared/testing';
import { DrawingElementAdapterService } from '@modules/tldraw-client/service/drawing-element-adapter.service';
import { RecursiveDeleteVisitor } from './recursive-delete.vistor';

describe(RecursiveDeleteVisitor.name, () => {
Expand Down Expand Up @@ -255,40 +255,82 @@ describe(RecursiveDeleteVisitor.name, () => {
});

describe('visitExternalToolElementAsync', () => {
const setup = () => {
const contextExternalTool = contextExternalToolFactory.buildWithId();
const childExternalToolElement = externalToolElementFactory.build();
const externalToolElement = externalToolElementFactory.build({
children: [childExternalToolElement],
contextExternalToolId: contextExternalTool.id,
});
describe('when the linked context external tool exists', () => {
const setup = () => {
const contextExternalTool = contextExternalToolFactory.buildWithId();
const childExternalToolElement = externalToolElementFactory.build();
const externalToolElement = externalToolElementFactory.build({
children: [childExternalToolElement],
contextExternalToolId: contextExternalTool.id,
});

contextExternalToolService.findById.mockResolvedValue(contextExternalTool);
contextExternalToolService.findById.mockResolvedValue(contextExternalTool);

return {
externalToolElement,
childExternalToolElement,
contextExternalTool,
return {
externalToolElement,
childExternalToolElement,
contextExternalTool,
};
};
};

it('should delete the context external tool that is linked to the element', async () => {
const { externalToolElement, contextExternalTool } = setup();
it('should delete the context external tool that is linked to the element', async () => {
const { externalToolElement, contextExternalTool } = setup();

await service.visitExternalToolElementAsync(externalToolElement);
await service.visitExternalToolElementAsync(externalToolElement);

expect(contextExternalToolService.deleteContextExternalTool).toHaveBeenCalledWith(contextExternalTool);
expect(contextExternalToolService.deleteContextExternalTool).toHaveBeenCalledWith(contextExternalTool);
});

it('should call entity remove', async () => {
const { externalToolElement, childExternalToolElement } = setup();

await service.visitExternalToolElementAsync(externalToolElement);

expect(em.remove).toHaveBeenCalledWith(
em.getReference(externalToolElement.constructor, externalToolElement.id)
);
expect(em.remove).toHaveBeenCalledWith(
em.getReference(childExternalToolElement.constructor, childExternalToolElement.id)
);
});
});

it('should call entity remove', async () => {
const { externalToolElement, childExternalToolElement } = setup();
describe('when the external tool does not exist anymore', () => {
const setup = () => {
const childExternalToolElement = externalToolElementFactory.build();
const externalToolElement = externalToolElementFactory.build({
children: [childExternalToolElement],
contextExternalToolId: new ObjectId().toHexString(),
});

await service.visitExternalToolElementAsync(externalToolElement);
contextExternalToolService.findById.mockResolvedValue(null);

expect(em.remove).toHaveBeenCalledWith(em.getReference(externalToolElement.constructor, externalToolElement.id));
expect(em.remove).toHaveBeenCalledWith(
em.getReference(childExternalToolElement.constructor, childExternalToolElement.id)
);
return {
externalToolElement,
childExternalToolElement,
};
};

it('should not try to delete the context external tool that is linked to the element', async () => {
const { externalToolElement } = setup();

await service.visitExternalToolElementAsync(externalToolElement);

expect(contextExternalToolService.deleteContextExternalTool).not.toHaveBeenCalled();
});

it('should call entity remove', async () => {
const { externalToolElement, childExternalToolElement } = setup();

await service.visitExternalToolElementAsync(externalToolElement);

expect(em.remove).toHaveBeenCalledWith(
em.getReference(externalToolElement.constructor, externalToolElement.id)
);
expect(em.remove).toHaveBeenCalledWith(
em.getReference(childExternalToolElement.constructor, childExternalToolElement.id)
);
});
});
});
});
10 changes: 6 additions & 4 deletions apps/server/src/modules/board/repo/recursive-delete.vistor.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { EntityManager } from '@mikro-orm/mongodb';
import { FilesStorageClientAdapterService } from '@modules/files-storage-client';
import { DrawingElementAdapterService } from '@modules/tldraw-client/service/drawing-element-adapter.service';
import { ContextExternalTool } from '@modules/tool/context-external-tool/domain';
import { ContextExternalToolService } from '@modules/tool/context-external-tool/service';
import { Injectable } from '@nestjs/common';
Expand All @@ -16,9 +17,8 @@ import {
SubmissionContainerElement,
SubmissionItem,
} from '@shared/domain';
import { LinkElement } from '@shared/domain/domainobject/board/link-element.do';
import { DrawingElement } from '@shared/domain/domainobject/board/drawing-element.do';
import { DrawingElementAdapterService } from '@modules/tldraw-client/service/drawing-element-adapter.service';
import { LinkElement } from '@shared/domain/domainobject/board/link-element.do';

@Injectable()
export class RecursiveDeleteVisitor implements BoardCompositeVisitorAsync {
Expand Down Expand Up @@ -82,11 +82,13 @@ export class RecursiveDeleteVisitor implements BoardCompositeVisitorAsync {

async visitExternalToolElementAsync(externalToolElement: ExternalToolElement): Promise<void> {
if (externalToolElement.contextExternalToolId) {
const linkedTool: ContextExternalTool = await this.contextExternalToolService.findById(
const linkedTool: ContextExternalTool | null = await this.contextExternalToolService.findById(
externalToolElement.contextExternalToolId
);

await this.contextExternalToolService.deleteContextExternalTool(linkedTool);
if (linkedTool) {
await this.contextExternalToolService.deleteContextExternalTool(linkedTool);
}
}

this.deleteNode(externalToolElement);
Expand Down
48 changes: 45 additions & 3 deletions ...rver/src/modules/tool/context-external-tool/service/context-external-tool.service.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { AuthorizationService } from '@modules/authorization';
import { NotFoundException } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { ContextExternalToolRepo } from '@shared/repo';
Expand All @@ -7,7 +8,6 @@ import {
legacySchoolDoFactory,
schoolExternalToolFactory,
} from '@shared/testing/factory/domainobject';
import { AuthorizationService } from '@modules/authorization';
import { ToolContextType } from '../../common/enum';
import { SchoolExternalTool } from '../../school-external-tool/domain';
import { ContextExternalTool, ContextRef } from '../domain';
Expand Down Expand Up @@ -151,7 +151,7 @@ describe('ContextExternalToolService', () => {
it('should return a contextExternalTool', async () => {
const { contextExternalTool } = setup();

const result: ContextExternalTool = await service.findById(contextExternalTool.id as string);
const result: ContextExternalTool = await service.findByIdOrFail(contextExternalTool.id as string);

expect(result).toEqual(contextExternalTool);
});
Expand All @@ -165,13 +165,55 @@ describe('ContextExternalToolService', () => {
it('should throw a not found exception', async () => {
setup();

const func = () => service.findById('unknownContextExternalToolId');
const func = () => service.findByIdOrFail('unknownContextExternalToolId');

await expect(func()).rejects.toThrow(NotFoundException);
});
});
});

describe('findByIdOrNull', () => {
describe('when contextExternalToolId is given', () => {
const setup = () => {
const schoolId: string = legacySchoolDoFactory.buildWithId().id as string;
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
schoolId,
});
const contextExternalTool: ContextExternalTool = contextExternalToolFactory
.withSchoolExternalToolRef(schoolExternalTool.id as string, schoolExternalTool.schoolId)
.build();

contextExternalToolRepo.findByIdOrNull.mockResolvedValue(contextExternalTool);

return {
contextExternalTool,
};
};

it('should return a contextExternalTool', async () => {
const { contextExternalTool } = setup();

const result: ContextExternalTool | null = await service.findById(contextExternalTool.id as string);

expect(result).toEqual(contextExternalTool);
});
});

describe('when contextExternalTool could not be found', () => {
const setup = () => {
contextExternalToolRepo.findByIdOrNull.mockResolvedValueOnce(null);
};

it('should throw a not found exception', async () => {
setup();

const result: ContextExternalTool | null = await service.findById('unknownContextExternalToolId');

expect(result).toBeNull();
});
});
});

describe('deleteContextExternalTool', () => {
describe('when contextExternalToolId is given', () => {
const setup = () => {
Expand Down
8 changes: 7 additions & 1 deletion apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,18 @@ export class ContextExternalToolService {
return contextExternalTools;
}

async findById(contextExternalToolId: EntityId): Promise<ContextExternalTool> {
async findByIdOrFail(contextExternalToolId: EntityId): Promise<ContextExternalTool> {
const tool: ContextExternalTool = await this.contextExternalToolRepo.findById(contextExternalToolId);

return tool;
}

async findById(contextExternalToolId: EntityId): Promise<ContextExternalTool | null> {
const tool: ContextExternalTool | null = await this.contextExternalToolRepo.findByIdOrNull(contextExternalToolId);

return tool;
}

async saveContextExternalTool(contextExternalTool: ContextExternalTool): Promise<ContextExternalTool> {
const savedContextExternalTool: ContextExternalTool = await this.contextExternalToolRepo.save(contextExternalTool);

Expand Down
2 changes: 1 addition & 1 deletion apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describe('ToolReferenceService', () => {
.buildWithId(undefined, contextExternalToolId);
const logoUrl = 'logoUrl';

contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);
schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool);
externalToolService.findById.mockResolvedValueOnce(externalTool);
toolVersionService.determineToolConfigurationStatus.mockResolvedValue(ToolConfigurationStatus.OUTDATED);
Expand Down
2 changes: 1 addition & 1 deletion apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export class ToolReferenceService {
) {}

async getToolReference(contextExternalToolId: EntityId): Promise<ToolReference> {
const contextExternalTool: ContextExternalTool = await this.contextExternalToolService.findById(
const contextExternalTool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail(
contextExternalToolId
);
const schoolExternalTool: SchoolExternalTool = await this.schoolExternalToolService.findById(
Expand Down
14 changes: 7 additions & 7 deletions apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ describe('ContextExternalToolUc', () => {

schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool);
contextExternalToolService.saveContextExternalTool.mockResolvedValue(contextExternalTool);
contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);

return {
contextExternalTool,
Expand Down Expand Up @@ -424,7 +424,7 @@ describe('ContextExternalToolUc', () => {
const error = new ForbiddenException();

schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool);
contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);
toolPermissionHelper.ensureContextPermissions.mockRejectedValue(error);

return {
Expand Down Expand Up @@ -470,7 +470,7 @@ describe('ContextExternalToolUc', () => {
const error = new UnprocessableEntityException();

schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool);
contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);
contextExternalToolValidationService.validate.mockRejectedValue(error);

return {
Expand Down Expand Up @@ -501,7 +501,7 @@ describe('ContextExternalToolUc', () => {
const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId();

toolPermissionHelper.ensureContextPermissions.mockResolvedValue();
contextExternalToolService.findById.mockResolvedValue(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValue(contextExternalTool);

return {
contextExternalTool,
Expand Down Expand Up @@ -658,7 +658,7 @@ describe('ContextExternalToolUc', () => {
},
});

contextExternalToolService.findById.mockResolvedValue(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValue(contextExternalTool);
toolPermissionHelper.ensureContextPermissions.mockResolvedValue(Promise.resolve());

return {
Expand Down Expand Up @@ -686,7 +686,7 @@ describe('ContextExternalToolUc', () => {

await uc.getContextExternalTool(userId, contextExternalTool.id as string);

expect(contextExternalToolService.findById).toHaveBeenCalledWith(contextExternalTool.id);
expect(contextExternalToolService.findByIdOrFail).toHaveBeenCalledWith(contextExternalTool.id);
});
});

Expand All @@ -704,7 +704,7 @@ describe('ContextExternalToolUc', () => {
},
});

contextExternalToolService.findById.mockResolvedValue(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValue(contextExternalTool);
toolPermissionHelper.ensureContextPermissions.mockRejectedValue(
new ForbiddenLoggableException(
userId,
Expand Down
6 changes: 3 additions & 3 deletions apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export class ContextExternalToolUc {
throw new ForbiddenLoggableException(userId, AuthorizableReferenceType.ContextExternalToolEntity, context);
}

let contextExternalTool: ContextExternalTool = await this.contextExternalToolService.findById(
let contextExternalTool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail(
contextExternalToolId
);

Expand All @@ -91,7 +91,7 @@ export class ContextExternalToolUc {
}

public async deleteContextExternalTool(userId: EntityId, contextExternalToolId: EntityId): Promise<void> {
const tool: ContextExternalTool = await this.contextExternalToolService.findById(contextExternalToolId);
const tool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail(contextExternalToolId);

const context = AuthorizationContextBuilder.write([Permission.CONTEXT_TOOL_ADMIN]);
await this.toolPermissionHelper.ensureContextPermissions(userId, tool, context);
Expand All @@ -114,7 +114,7 @@ export class ContextExternalToolUc {
}

async getContextExternalTool(userId: EntityId, contextToolId: EntityId) {
const tool: ContextExternalTool = await this.contextExternalToolService.findById(contextToolId);
const tool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail(contextToolId);
const context: AuthorizationContext = AuthorizationContextBuilder.read([Permission.CONTEXT_TOOL_ADMIN]);

await this.toolPermissionHelper.ensureContextPermissions(userId, tool, context);
Expand Down
6 changes: 3 additions & 3 deletions apps/server/src/modules/tool/context-external-tool/uc/tool-reference.uc.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { AuthorizationContextBuilder } from '@modules/authorization';
import { ForbiddenException } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { Permission } from '@shared/domain';
import { contextExternalToolFactory, externalToolFactory } from '@shared/testing';
import { AuthorizationContextBuilder } from '@modules/authorization';
import { ToolConfigurationStatus, ToolContextType } from '../../common/enum';
import { ToolPermissionHelper } from '../../common/uc/tool-permission-helper';
import { ExternalTool } from '../../external-tool/domain';
Expand Down Expand Up @@ -150,7 +150,7 @@ describe('ToolReferenceUc', () => {
openInNewTab: externalTool.openNewTab,
});

contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);
toolPermissionHelper.ensureContextPermissions.mockResolvedValueOnce();
toolReferenceService.getToolReference.mockResolvedValue(toolReference);

Expand Down Expand Up @@ -195,7 +195,7 @@ describe('ToolReferenceUc', () => {
);
const error = new ForbiddenException();

contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool);
contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool);
toolPermissionHelper.ensureContextPermissions.mockRejectedValueOnce(error);

return {
Expand Down
Loading

0 comments on commit acdd957

Please sign in to comment.