From 5d08e797b2f9365c04ecf532b79ccc22a41e39c9 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Tue, 2 Jan 2024 15:52:11 +0100 Subject: [PATCH 01/21] copy ctl tools in tab when course copy --- .../src/modules/learnroom/learnroom.module.ts | 3 +- .../learnroom/service/course-copy.service.ts | 14 ++++++- .../service/context-external-tool.service.ts | 39 +++++++++++++++++++ config/default.schema.json | 5 +++ src/services/config/publicAppConfigService.js | 1 + 5 files changed, 60 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/learnroom/learnroom.module.ts b/apps/server/src/modules/learnroom/learnroom.module.ts index f78d74dfafa..39bea93eefe 100644 --- a/apps/server/src/modules/learnroom/learnroom.module.ts +++ b/apps/server/src/modules/learnroom/learnroom.module.ts @@ -23,9 +23,10 @@ import { DashboardService, RoomsService, } from './service'; +import { ContextExternalToolModule } from '../tool/context-external-tool'; @Module({ - imports: [LessonModule, TaskModule, CopyHelperModule, BoardModule, LoggerModule], + imports: [LessonModule, TaskModule, CopyHelperModule, BoardModule, LoggerModule, ContextExternalToolModule], providers: [ { provide: 'DASHBOARD_REPO', diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index c4857f49416..86af24b3491 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -3,8 +3,11 @@ import { Injectable } from '@nestjs/common'; import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { BoardCopyService } from './board-copy.service'; import { RoomsService } from './rooms.service'; +import { ContextExternalToolService } from '../../tool/context-external-tool/service'; +import { ToolContextType } from '../../tool/common/enum'; type CourseCopyParams = { originalCourse: Course; @@ -20,7 +23,8 @@ export class CourseCopyService { private readonly roomsService: RoomsService, private readonly boardCopyService: BoardCopyService, private readonly copyHelperService: CopyHelperService, - private readonly userRepo: UserRepo + private readonly userRepo: UserRepo, + private readonly contextExternalToolService: ContextExternalToolService ) {} async copyCourse({ @@ -46,6 +50,9 @@ export class CourseCopyService { // copy course and board const courseCopy = await this.copyCourseEntity({ user, originalCourse, copyName }); + if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { + await this.contextExternalToolService.copyContextExternalTools(courseId, ToolContextType.COURSE, courseCopy.id); + } const boardStatus = await this.boardCopyService.copyBoard({ originalBoard, destinationCourse: courseCopy, user }); const finishedCourseCopy = await this.finishCourseCopying(courseCopy); @@ -82,6 +89,11 @@ export class CourseCopyService { type: CopyElementType.METADATA, status: CopyStatusEnum.SUCCESS, }, + /* { + // TODO N21-1507 WTH does this do? Implement logic for PARTIAL? + type: CopyElementType.EXTERNAL_TOOL_ELEMENT, + status: CopyStatusEnum.SUCCESS, + }, */ { type: CopyElementType.USER_GROUP, status: CopyStatusEnum.NOT_DOING, diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts index baf6bd82b06..b2951316a20 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts @@ -9,6 +9,8 @@ import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalToolService } from '../../school-external-tool/service'; import { RestrictedContextMismatchLoggable } from './restricted-context-mismatch-loggabble'; import { CommonToolService } from '../../common/service'; +import { CustomParameter, CustomParameterEntry } from '../../common/domain'; +import { ToolContextType } from '../../common/enum'; @Injectable() export class ContextExternalToolService { @@ -76,4 +78,41 @@ export class ContextExternalToolService { throw new RestrictedContextMismatchLoggable(externalTool.name, contextExternalTool.contextRef.type); } } + + public async copyContextExternalTools(courseId: EntityId, contextType: ToolContextType, courseCopyId: EntityId) { + const contextRef: ContextRef = { id: courseId, type: contextType }; + const contextExternalToolsInContext = await this.findAllByContext(contextRef); + + const copyableContextExternalTools: ContextExternalTool[] = await Promise.all( + contextExternalToolsInContext.map(async (tool: ContextExternalTool): Promise => { + tool.id = undefined; + tool.contextRef.id = courseCopyId; + + const schoolExternalTool: SchoolExternalTool = await this.schoolExternalToolService.findById( + tool.schoolToolRef.schoolToolId + ); + const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); + + if (externalTool.parameters) { + externalTool.parameters.map((parameter: CustomParameter): CustomParameter => { + if (parameter.isProtected) { + this.deleteProtectedValues(tool, parameter.name); + } + return parameter; + }); + } + return tool; + }) + ); + + await this.contextExternalToolRepo.saveAll(copyableContextExternalTools); + } + + private deleteProtectedValues(contextExternalTool: ContextExternalTool, protectedParameterName: string): void { + const protectedParameter: CustomParameterEntry = contextExternalTool.parameters.filter( + (param: CustomParameterEntry): boolean => param.name === protectedParameterName + )[0]; + + protectedParameter.value = undefined; + } } diff --git a/config/default.schema.json b/config/default.schema.json index 0104563c2d2..5053d6405c7 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -1400,6 +1400,11 @@ "default": false, "description": "Enables the calculation of the outdated status of an external tool without the usage of the db attribute version" }, + "FEATURE_CTL_TOOLS_COPY_ENABLED": { + "type": "boolean", + "default": false, + "description": "Enables the copying of ctl tools when copying a course." + }, "TSP_SCHOOL_SYNCER": { "type": "object", "description": "TSP School Syncer properties", diff --git a/src/services/config/publicAppConfigService.js b/src/services/config/publicAppConfigService.js index 0b429a46048..d264107bc5a 100644 --- a/src/services/config/publicAppConfigService.js +++ b/src/services/config/publicAppConfigService.js @@ -66,6 +66,7 @@ const exposedVars = [ 'FEATURE_SHOW_NEW_CLASS_VIEW_ENABLED', 'FEATURE_TLDRAW_ENABLED', 'FEATURE_PROVISIONING_OPTIONS_ENABLED', + 'FEATURE_CTL_TOOLS_COPY_ENABLED', ]; /** From fbed27a99f24352fd5d7a50c037e6744f3c005aa Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 3 Jan 2024 15:15:30 +0100 Subject: [PATCH 02/21] copy ctl tools in board when course copy --- .../board-do-copy.service.ts | 4 +- .../recursive-copy.visitor.ts | 21 ++++++- .../service/column-board-copy.service.ts | 10 +++- .../learnroom/service/course-copy.service.ts | 16 +++++- .../service/context-external-tool.service.ts | 55 +++++++++---------- 5 files changed, 71 insertions(+), 35 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index a398a807de9..e245965ff9d 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -3,16 +3,18 @@ import { Injectable } from '@nestjs/common'; import { AnyBoardDo } from '@shared/domain/domainobject'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; +import { ContextExternalToolService } from '../../../tool/context-external-tool/service'; export type BoardDoCopyParams = { original: AnyBoardDo; fileCopyService: SchoolSpecificFileCopyService; + contextExternalToolService: ContextExternalToolService; }; @Injectable() export class BoardDoCopyService { public async copy(params: BoardDoCopyParams): Promise { - const visitor = new RecursiveCopyVisitor(params.fileCopyService); + const visitor = new RecursiveCopyVisitor(params.fileCopyService, params.contextExternalToolService); const result = await visitor.copy(params.original); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index a7a0d3b6d1b..6d6ea6fdd5c 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -16,14 +16,20 @@ import { import { LinkElement } from '@shared/domain/domainobject/board/link-element.do'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; +import { ContextExternalTool } from '../../../tool/context-external-tool/domain'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { resultMap = new Map(); copyMap = new Map(); - constructor(private readonly fileCopyService: SchoolSpecificFileCopyService) {} + constructor( + private readonly fileCopyService: SchoolSpecificFileCopyService, + private readonly contextExternalToolService: ContextExternalToolService + ) {} async copy(original: AnyBoardDo): Promise { await original.acceptAsync(this); @@ -235,7 +241,7 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { return Promise.resolve(); } - visitExternalToolElementAsync(original: ExternalToolElement): Promise { + async visitExternalToolElementAsync(original: ExternalToolElement): Promise { const copy = new ExternalToolElement({ id: new ObjectId().toHexString(), contextExternalToolId: undefined, @@ -243,6 +249,17 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { createdAt: new Date(), updatedAt: new Date(), }); + + if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED') && original.contextExternalToolId) { + const tool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail( + original.contextExternalToolId + ); + + const copiedTool = await this.contextExternalToolService.copyContextExternalTool(tool, copy.id); + + copy.contextExternalToolId = copiedTool.id; + } + this.resultMap.set(original.id, { copyEntity: copy, type: CopyElementType.EXTERNAL_TOOL_ELEMENT, diff --git a/apps/server/src/modules/board/service/column-board-copy.service.ts b/apps/server/src/modules/board/service/column-board-copy.service.ts index 1317f4b5c22..f062d6dcc44 100644 --- a/apps/server/src/modules/board/service/column-board-copy.service.ts +++ b/apps/server/src/modules/board/service/column-board-copy.service.ts @@ -12,6 +12,7 @@ import { CourseRepo } from '@shared/repo'; import { BoardDoRepo } from '../repo'; import { BoardDoCopyService, SchoolSpecificFileCopyServiceFactory } from './board-do-copy-service'; import { SwapInternalLinksVisitor } from './board-do-copy-service/swap-internal-links.visitor'; +import { ContextExternalToolService } from '../../tool/context-external-tool/service'; @Injectable() export class ColumnBoardCopyService { @@ -20,7 +21,8 @@ export class ColumnBoardCopyService { private readonly courseRepo: CourseRepo, private readonly userService: UserService, private readonly boardDoCopyService: BoardDoCopyService, - private readonly fileCopyServiceFactory: SchoolSpecificFileCopyServiceFactory + private readonly fileCopyServiceFactory: SchoolSpecificFileCopyServiceFactory, + private readonly contextExternalToolService: ContextExternalToolService ) {} async copyColumnBoard(props: { @@ -43,7 +45,11 @@ export class ColumnBoardCopyService { userId: props.userId, }); - const copyStatus = await this.boardDoCopyService.copy({ original: originalBoard, fileCopyService }); + const copyStatus = await this.boardDoCopyService.copy({ + original: originalBoard, + fileCopyService, + contextExternalToolService: this.contextExternalToolService, + }); /* istanbul ignore next */ if (!isColumnBoard(copyStatus.copyEntity)) { diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index 86af24b3491..db12dca2b64 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -8,6 +8,7 @@ import { BoardCopyService } from './board-copy.service'; import { RoomsService } from './rooms.service'; import { ContextExternalToolService } from '../../tool/context-external-tool/service'; import { ToolContextType } from '../../tool/common/enum'; +import { ContextExternalTool, ContextRef } from '../../tool/context-external-tool/domain'; type CourseCopyParams = { originalCourse: Course; @@ -51,8 +52,21 @@ export class CourseCopyService { // copy course and board const courseCopy = await this.copyCourseEntity({ user, originalCourse, copyName }); if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { - await this.contextExternalToolService.copyContextExternalTools(courseId, ToolContextType.COURSE, courseCopy.id); + const contextRef: ContextRef = { id: courseId, type: ToolContextType.COURSE }; + const contextExternalToolsInContext = await this.contextExternalToolService.findAllByContext(contextRef); + + await Promise.all( + contextExternalToolsInContext.map(async (tool: ContextExternalTool): Promise => { + const copiedTool: ContextExternalTool = await this.contextExternalToolService.copyContextExternalTool( + tool, + courseCopy.id + ); + + return copiedTool; + }) + ); } + const boardStatus = await this.boardCopyService.copyBoard({ originalBoard, destinationCourse: courseCopy, user }); const finishedCourseCopy = await this.finishCourseCopying(courseCopy); diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts index b2951316a20..8998dfeab02 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts @@ -10,7 +10,6 @@ import { SchoolExternalToolService } from '../../school-external-tool/service'; import { RestrictedContextMismatchLoggable } from './restricted-context-mismatch-loggabble'; import { CommonToolService } from '../../common/service'; import { CustomParameter, CustomParameterEntry } from '../../common/domain'; -import { ToolContextType } from '../../common/enum'; @Injectable() export class ContextExternalToolService { @@ -79,40 +78,38 @@ export class ContextExternalToolService { } } - public async copyContextExternalTools(courseId: EntityId, contextType: ToolContextType, courseCopyId: EntityId) { - const contextRef: ContextRef = { id: courseId, type: contextType }; - const contextExternalToolsInContext = await this.findAllByContext(contextRef); - - const copyableContextExternalTools: ContextExternalTool[] = await Promise.all( - contextExternalToolsInContext.map(async (tool: ContextExternalTool): Promise => { - tool.id = undefined; - tool.contextRef.id = courseCopyId; - - const schoolExternalTool: SchoolExternalTool = await this.schoolExternalToolService.findById( - tool.schoolToolRef.schoolToolId - ); - const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - - if (externalTool.parameters) { - externalTool.parameters.map((parameter: CustomParameter): CustomParameter => { - if (parameter.isProtected) { - this.deleteProtectedValues(tool, parameter.name); - } - return parameter; - }); - } - return tool; - }) + public async copyContextExternalTool( + tool: ContextExternalTool, + contextCopyId: EntityId + ): Promise { + tool.id = undefined; + tool.contextRef.id = contextCopyId; + + const schoolExternalTool: SchoolExternalTool = await this.schoolExternalToolService.findById( + tool.schoolToolRef.schoolToolId ); + const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - await this.contextExternalToolRepo.saveAll(copyableContextExternalTools); + if (externalTool.parameters) { + externalTool.parameters.forEach((parameter: CustomParameter): CustomParameter => { + if (parameter.isProtected) { + this.deleteProtectedValues(tool, parameter.name); + } + return parameter; + }); + } + const copiedTool = await this.contextExternalToolRepo.save(tool); + + return copiedTool; } private deleteProtectedValues(contextExternalTool: ContextExternalTool, protectedParameterName: string): void { - const protectedParameter: CustomParameterEntry = contextExternalTool.parameters.filter( + const protectedParameter: CustomParameterEntry | undefined = contextExternalTool.parameters.find( (param: CustomParameterEntry): boolean => param.name === protectedParameterName - )[0]; + ); - protectedParameter.value = undefined; + if (protectedParameter) { + protectedParameter.value = undefined; + } } } From 7d39bd458179426363a153eee29662f3a9ad7d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= Date: Wed, 3 Jan 2024 16:49:25 +0100 Subject: [PATCH 03/21] change validation of context and school external tools --- ...rnal-tool-configuration-status.response.ts | 7 + ...text-external-tool-configuration-status.ts | 3 + .../modules/tool/common/domain/error/index.ts | 6 + ...meter-duplicate.loggable-exception.spec.ts | 20 ++ ...-parameter-duplicate.loggable-exception.ts | 19 ++ ...ameter-required.loggable-exception.spec.ts | 33 +++ ...l-parameter-required.loggable-exception.ts | 22 ++ ...r-type-mismatch.loggable-exception.spec.ts | 36 +++ ...ameter-type-mismatch.loggable-exception.ts | 23 ++ ...rameter-unknown.loggable-exception.spec.ts | 35 +++ ...ol-parameter-unknown.loggable-exception.ts | 20 ++ ...r-value-missing.loggable-exception.spec.ts | 35 +++ ...ameter-value-missing.loggable-exception.ts | 20 ++ ...ter-value-regex.loggable-exception.spec.ts | 35 +++ ...arameter-value-regex.loggable-exception.ts | 22 ++ .../src/modules/tool/common/domain/index.ts | 8 + .../mapper/tool-status-response.mapper.ts | 1 + .../service/common-tool-validation.service.ts | 114 -------- .../common/service/common-tool.service.ts | 5 +- .../src/modules/tool/common/service/index.ts | 2 +- .../common-tool-validation.service.spec.ts | 249 +++++++----------- .../common-tool-validation.service.ts | 42 +++ .../tool/common/service/validation/index.ts | 2 + .../common/service/validation/rules/index.ts | 8 + ...parameter-array-duplicate-key-validator.ts | 22 ++ .../rules/parameter-array-entry-validator.ts | 37 +++ .../parameter-array-unknown-key-validator.ts | 21 ++ .../rules/parameter-array-validator.ts | 6 + .../rules/parameter-entry-regex-validator.ts | 13 + .../rules/parameter-entry-type-validator.ts | 17 ++ .../rules/parameter-entry-validator.ts | 6 + .../rules/parameter-entry-value-validator.ts | 13 + ...ool-parameter-type-validation.util.spec.ts | 66 +++++ .../tool-parameter-type-validation.util.ts | 22 ++ ...t-external-tool-validation.service.spec.ts | 38 ++- ...ontext-external-tool-validation.service.ts | 9 +- .../service/tool-reference.service.spec.ts | 2 +- .../service/tool-reference.service.ts | 13 +- .../service/tool-version-service.spec.ts | 196 ++++++++------ .../service/tool-version-service.ts | 45 +++- ...-tool-parameter-validation.service.spec.ts | 5 - ...ernal-tool-parameter-validation.service.ts | 9 +- ...l-external-tool-validation.service.spec.ts | 2 +- ...school-external-tool-validation.service.ts | 9 +- ...l-configuration-status-response.factory.ts | 1 + .../tool/tool-configuration-status.factory.ts | 1 + 46 files changed, 933 insertions(+), 387 deletions(-) create mode 100644 apps/server/src/modules/tool/common/domain/error/index.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.spec.ts create mode 100644 apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.ts delete mode 100644 apps/server/src/modules/tool/common/service/common-tool-validation.service.ts rename apps/server/src/modules/tool/common/service/{ => validation}/common-tool-validation.service.spec.ts (67%) create mode 100644 apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/index.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/index.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.ts diff --git a/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts b/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts index ca64669c166..04ab2f9ed69 100644 --- a/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts +++ b/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts @@ -15,8 +15,15 @@ export class ContextExternalToolConfigurationStatusResponse { }) isOutdatedOnScopeContext: boolean; + @ApiProperty({ + type: Boolean, + description: 'true, if a configured parameter on the context external tool is missing a value', + }) + isIncompleteOnScopeContext: boolean; + constructor(props: ContextExternalToolConfigurationStatusResponse) { this.isOutdatedOnScopeSchool = props.isOutdatedOnScopeSchool; this.isOutdatedOnScopeContext = props.isOutdatedOnScopeContext; + this.isIncompleteOnScopeContext = props.isIncompleteOnScopeContext; } } diff --git a/apps/server/src/modules/tool/common/domain/context-external-tool-configuration-status.ts b/apps/server/src/modules/tool/common/domain/context-external-tool-configuration-status.ts index be533e50212..6975e310b4a 100644 --- a/apps/server/src/modules/tool/common/domain/context-external-tool-configuration-status.ts +++ b/apps/server/src/modules/tool/common/domain/context-external-tool-configuration-status.ts @@ -3,8 +3,11 @@ export class ContextExternalToolConfigurationStatus { isOutdatedOnScopeContext: boolean; + isIncompleteOnScopeContext: boolean; + constructor(props: ContextExternalToolConfigurationStatus) { this.isOutdatedOnScopeSchool = props.isOutdatedOnScopeSchool; this.isOutdatedOnScopeContext = props.isOutdatedOnScopeContext; + this.isIncompleteOnScopeContext = props.isIncompleteOnScopeContext; } } diff --git a/apps/server/src/modules/tool/common/domain/error/index.ts b/apps/server/src/modules/tool/common/domain/error/index.ts new file mode 100644 index 00000000000..4f0b5731e45 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/index.ts @@ -0,0 +1,6 @@ +export { ToolParameterDuplicateLoggableException } from './tool-parameter-duplicate.loggable-exception'; +export { ToolParameterRequiredLoggableException } from './tool-parameter-required.loggable-exception'; +export { ToolParameterUnknownLoggableException } from './tool-parameter-unknown.loggable-exception'; +export { ToolParameterValueRegexLoggableException } from './tool-parameter-value-regex.loggable-exception'; +export { ToolParameterTypeMismatchLoggableException } from './tool-parameter-type-mismatch.loggable-exception'; +export { ToolParameterValueMissingLoggableException } from './tool-parameter-value-missing.loggable-exception'; diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.spec.ts new file mode 100644 index 00000000000..f954eb20f25 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.spec.ts @@ -0,0 +1,20 @@ +import { ToolParameterDuplicateLoggableException } from './tool-parameter-duplicate.loggable-exception'; + +describe(ToolParameterDuplicateLoggableException.name, () => { + describe('getLogMessage', () => { + it('should return log message', () => { + const exception = new ToolParameterDuplicateLoggableException('parameter1'); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_DUPLICATE', + message: 'The parameter is defined multiple times.', + stack: exception.stack, + data: { + parameterName: 'parameter1', + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.ts new file mode 100644 index 00000000000..861d4247d6d --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-duplicate.loggable-exception.ts @@ -0,0 +1,19 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; + +export class ToolParameterDuplicateLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterName: string) { + super(`tool_param_duplicate: The parameter with name ${parameterName} is defined multiple times.`); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_DUPLICATE', + message: 'The parameter is defined multiple times.', + stack: this.stack, + data: { + parameterName: this.parameterName, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.spec.ts new file mode 100644 index 00000000000..a3f5aae51ad --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.spec.ts @@ -0,0 +1,33 @@ +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter } from '../custom-parameter.do'; +import { ToolParameterRequiredLoggableException } from './tool-parameter-required.loggable-exception'; + +describe(ToolParameterRequiredLoggableException.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const parameter: CustomParameter = customParameterFactory.build(); + + const exception: ToolParameterRequiredLoggableException = new ToolParameterRequiredLoggableException(parameter); + + return { + parameter, + exception, + }; + }; + + it('should return log message', () => { + const { exception, parameter } = setup(); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_REQUIRED', + message: 'The parameter is required, but not found in the tool.', + stack: exception.stack, + data: { + parameterName: parameter.name, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.ts new file mode 100644 index 00000000000..20448c1feb6 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-required.loggable-exception.ts @@ -0,0 +1,22 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { CustomParameter } from '../custom-parameter.do'; + +export class ToolParameterRequiredLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterDeclaration: CustomParameter) { + super( + `tool_param_required: The parameter with name ${parameterDeclaration.name} is required but not found in the tool.` + ); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_REQUIRED', + message: 'The parameter is required, but not found in the tool.', + stack: this.stack, + data: { + parameterName: this.parameterDeclaration.name, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.spec.ts new file mode 100644 index 00000000000..76384e44cf5 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.spec.ts @@ -0,0 +1,36 @@ +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter } from '../custom-parameter.do'; +import { ToolParameterTypeMismatchLoggableException } from './tool-parameter-type-mismatch.loggable-exception'; + +describe(ToolParameterTypeMismatchLoggableException.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const parameter: CustomParameter = customParameterFactory.build(); + + const exception: ToolParameterTypeMismatchLoggableException = new ToolParameterTypeMismatchLoggableException( + parameter + ); + + return { + parameter, + exception, + }; + }; + + it('should return log message', () => { + const { exception, parameter } = setup(); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_TYPE_MISMATCH', + message: 'The parameter value has the wrong type.', + stack: exception.stack, + data: { + parameterName: parameter.name, + parameterType: parameter.type, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.ts new file mode 100644 index 00000000000..34af9b0c86a --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-type-mismatch.loggable-exception.ts @@ -0,0 +1,23 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { CustomParameter } from '../custom-parameter.do'; + +export class ToolParameterTypeMismatchLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterDeclaration: CustomParameter) { + super( + `tool_param_type_mismatch: The value of parameter with name ${parameterDeclaration.name} should be of type ${parameterDeclaration.type}.` + ); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_TYPE_MISMATCH', + message: 'The parameter value has the wrong type.', + stack: this.stack, + data: { + parameterName: this.parameterDeclaration.name, + parameterType: this.parameterDeclaration.type, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.spec.ts new file mode 100644 index 00000000000..902107b3d3a --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.spec.ts @@ -0,0 +1,35 @@ +import { CustomParameterEntry } from '../custom-parameter-entry.do'; +import { ToolParameterUnknownLoggableException } from './tool-parameter-unknown.loggable-exception'; + +describe(ToolParameterUnknownLoggableException.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const parameter: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: 'value1', + }); + + const exception: ToolParameterUnknownLoggableException = new ToolParameterUnknownLoggableException(parameter); + + return { + parameter, + exception, + }; + }; + + it('should return log message', () => { + const { exception, parameter } = setup(); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_UNKNOWN', + message: 'The parameter is not part of this tool.', + stack: exception.stack, + data: { + parameterName: parameter.name, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.ts new file mode 100644 index 00000000000..4f269c5bfb2 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-unknown.loggable-exception.ts @@ -0,0 +1,20 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { CustomParameterEntry } from '../custom-parameter-entry.do'; + +export class ToolParameterUnknownLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterEntry: CustomParameterEntry) { + super(`tool_param_unknown: The parameter with name ${parameterEntry.name} is not part of this tool.`); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_UNKNOWN', + message: 'The parameter is not part of this tool.', + stack: this.stack, + data: { + parameterName: this.parameterEntry.name, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.spec.ts new file mode 100644 index 00000000000..0215bc714dd --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.spec.ts @@ -0,0 +1,35 @@ +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter } from '../custom-parameter.do'; +import { ToolParameterValueMissingLoggableException } from './tool-parameter-value-missing.loggable-exception'; + +describe(ToolParameterValueMissingLoggableException.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const parameter: CustomParameter = customParameterFactory.build(); + + const exception: ToolParameterValueMissingLoggableException = new ToolParameterValueMissingLoggableException( + parameter + ); + + return { + parameter, + exception, + }; + }; + + it('should return log message', () => { + const { exception, parameter } = setup(); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_VALUE_MISSING', + message: 'The parameter has no value.', + stack: exception.stack, + data: { + parameterName: parameter.name, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.ts new file mode 100644 index 00000000000..e31f92f01d6 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-missing.loggable-exception.ts @@ -0,0 +1,20 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { CustomParameter } from '../custom-parameter.do'; + +export class ToolParameterValueMissingLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterDeclaration: CustomParameter) { + super(`tool_param_value_missing: The parameter with name ${parameterDeclaration.name} has no value`); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_VALUE_MISSING', + message: 'The parameter has no value.', + stack: this.stack, + data: { + parameterName: this.parameterDeclaration.name, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.spec.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.spec.ts new file mode 100644 index 00000000000..e1a04a1f00c --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.spec.ts @@ -0,0 +1,35 @@ +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter } from '../custom-parameter.do'; +import { ToolParameterValueRegexLoggableException } from './tool-parameter-value-regex.loggable-exception'; + +describe(ToolParameterValueRegexLoggableException.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const parameter: CustomParameter = customParameterFactory.build(); + + const exception: ToolParameterValueRegexLoggableException = new ToolParameterValueRegexLoggableException( + parameter + ); + + return { + parameter, + exception, + }; + }; + + it('should return log message', () => { + const { exception, parameter } = setup(); + + const result = exception.getLogMessage(); + + expect(result).toEqual({ + type: 'TOOL_PARAMETER_VALUE_REGEX', + message: 'The parameter value does not fit the regex.', + stack: exception.stack, + data: { + parameterName: parameter.name, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.ts b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.ts new file mode 100644 index 00000000000..2650572c7c3 --- /dev/null +++ b/apps/server/src/modules/tool/common/domain/error/tool-parameter-value-regex.loggable-exception.ts @@ -0,0 +1,22 @@ +import { ValidationError } from '@shared/common'; +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { CustomParameter } from '../custom-parameter.do'; + +export class ToolParameterValueRegexLoggableException extends ValidationError implements Loggable { + constructor(private readonly parameterDeclaration: CustomParameter) { + super( + `tool_param_value_regex: The given entry for the parameter with name ${parameterDeclaration.name} does not fit the regex.` + ); + } + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'TOOL_PARAMETER_VALUE_REGEX', + message: 'The parameter value does not fit the regex.', + stack: this.stack, + data: { + parameterName: this.parameterDeclaration.name, + }, + }; + } +} diff --git a/apps/server/src/modules/tool/common/domain/index.ts b/apps/server/src/modules/tool/common/domain/index.ts index 27dd13a3fb5..9669e9fefe2 100644 --- a/apps/server/src/modules/tool/common/domain/index.ts +++ b/apps/server/src/modules/tool/common/domain/index.ts @@ -1,3 +1,11 @@ +export { + ToolParameterDuplicateLoggableException, + ToolParameterRequiredLoggableException, + ToolParameterUnknownLoggableException, + ToolParameterValueRegexLoggableException, + ToolParameterTypeMismatchLoggableException, + ToolParameterValueMissingLoggableException, +} from './error'; export * from './custom-parameter.do'; export * from './custom-parameter-entry.do'; export * from './context-external-tool-configuration-status'; diff --git a/apps/server/src/modules/tool/common/mapper/tool-status-response.mapper.ts b/apps/server/src/modules/tool/common/mapper/tool-status-response.mapper.ts index 7e75fdd81ae..e17b89dd0ac 100644 --- a/apps/server/src/modules/tool/common/mapper/tool-status-response.mapper.ts +++ b/apps/server/src/modules/tool/common/mapper/tool-status-response.mapper.ts @@ -7,6 +7,7 @@ export class ToolStatusResponseMapper { new ContextExternalToolConfigurationStatusResponse({ isOutdatedOnScopeSchool: status.isOutdatedOnScopeSchool, isOutdatedOnScopeContext: status.isOutdatedOnScopeContext, + isIncompleteOnScopeContext: status.isIncompleteOnScopeContext, }); return configurationStatus; diff --git a/apps/server/src/modules/tool/common/service/common-tool-validation.service.ts b/apps/server/src/modules/tool/common/service/common-tool-validation.service.ts deleted file mode 100644 index e6c3a31b288..00000000000 --- a/apps/server/src/modules/tool/common/service/common-tool-validation.service.ts +++ /dev/null @@ -1,114 +0,0 @@ -import { Injectable } from '@nestjs/common'; -import { ValidationError } from '@shared/common'; -import { isNaN } from 'lodash'; -import { ContextExternalTool } from '../../context-external-tool/domain'; -import { ExternalTool } from '../../external-tool/domain'; -import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { CustomParameter, CustomParameterEntry } from '../domain'; -import { CustomParameterScope, CustomParameterType } from '../enum'; - -export type ValidatableTool = SchoolExternalTool | ContextExternalTool; - -@Injectable() -export class CommonToolValidationService { - private static typeCheckers: { [key in CustomParameterType]: (val: string) => boolean } = { - [CustomParameterType.STRING]: () => true, - [CustomParameterType.NUMBER]: (val: string | undefined) => !isNaN(Number(val)), - [CustomParameterType.BOOLEAN]: (val: string | undefined) => val === 'true' || val === 'false', - [CustomParameterType.AUTO_CONTEXTID]: () => false, - [CustomParameterType.AUTO_CONTEXTNAME]: () => false, - [CustomParameterType.AUTO_SCHOOLID]: () => false, - [CustomParameterType.AUTO_SCHOOLNUMBER]: () => false, - }; - - public isValueValidForType(type: CustomParameterType, val: string): boolean { - const rule = CommonToolValidationService.typeCheckers[type]; - - const isValid: boolean = rule(val); - - return isValid; - } - - public checkCustomParameterEntries(loadedExternalTool: ExternalTool, validatableTool: ValidatableTool): void { - this.checkForDuplicateParameters(validatableTool); - - const parametersForScope: CustomParameter[] = (loadedExternalTool.parameters ?? []).filter( - (param: CustomParameter) => - (validatableTool instanceof SchoolExternalTool && param.scope === CustomParameterScope.SCHOOL) || - (validatableTool instanceof ContextExternalTool && param.scope === CustomParameterScope.CONTEXT) - ); - - this.checkForUnknownParameters(validatableTool, parametersForScope); - - this.checkValidityOfParameters(validatableTool, parametersForScope); - } - - private checkForDuplicateParameters(validatableTool: ValidatableTool): void { - const caseInsensitiveNames: string[] = validatableTool.parameters.map(({ name }: CustomParameterEntry) => name); - - const uniqueNames: Set = new Set(caseInsensitiveNames); - - if (uniqueNames.size !== validatableTool.parameters.length) { - throw new ValidationError( - `tool_param_duplicate: The tool ${validatableTool.id ?? ''} contains multiple of the same custom parameters.` - ); - } - } - - private checkForUnknownParameters(validatableTool: ValidatableTool, parametersForScope: CustomParameter[]): void { - for (const entry of validatableTool.parameters) { - const foundParameter: CustomParameter | undefined = parametersForScope.find( - ({ name }: CustomParameter): boolean => name === entry.name - ); - - if (!foundParameter) { - throw new ValidationError( - `tool_param_unknown: The parameter with name ${entry.name} is not part of this tool.` - ); - } - } - } - - private checkValidityOfParameters(validatableTool: ValidatableTool, parametersForScope: CustomParameter[]): void { - for (const param of parametersForScope) { - const foundEntry: CustomParameterEntry | undefined = validatableTool.parameters.find( - ({ name }: CustomParameterEntry): boolean => name === param.name - ); - - this.validateParameter(param, foundEntry); - } - } - - private validateParameter(param: CustomParameter, foundEntry: CustomParameterEntry | undefined): void { - this.checkOptionalParameter(param, foundEntry); - - if (foundEntry) { - this.checkParameterType(foundEntry, param); - this.checkParameterRegex(foundEntry, param); - } - } - - private checkOptionalParameter(param: CustomParameter, foundEntry: CustomParameterEntry | undefined): void { - if (!foundEntry?.value && !param.isOptional) { - throw new ValidationError( - `tool_param_required: The parameter with name ${param.name} is required but not found in the tool.` - ); - } - } - - private checkParameterType(foundEntry: CustomParameterEntry, param: CustomParameter): void { - if (foundEntry.value !== undefined && !this.isValueValidForType(param.type, foundEntry.value)) { - throw new ValidationError( - `tool_param_type_mismatch: The value of parameter with name ${foundEntry.name} should be of type ${param.type}.` - ); - } - } - - private checkParameterRegex(foundEntry: CustomParameterEntry, param: CustomParameter): void { - if (foundEntry.value !== undefined && param.regex && !new RegExp(param.regex).test(foundEntry.value ?? '')) { - throw new ValidationError( - `tool_param_value_regex: The given entry for the parameter with name ${foundEntry.name} does not fit the regex.` - ); - } - } -} diff --git a/apps/server/src/modules/tool/common/service/common-tool.service.ts b/apps/server/src/modules/tool/common/service/common-tool.service.ts index b1c8d0e8da2..96c9165c785 100644 --- a/apps/server/src/modules/tool/common/service/common-tool.service.ts +++ b/apps/server/src/modules/tool/common/service/common-tool.service.ts @@ -1,9 +1,9 @@ import { Injectable } from '@nestjs/common'; +import { ContextExternalTool } from '../../context-external-tool/domain'; import { ExternalTool } from '../../external-tool/domain'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { ContextExternalTool } from '../../context-external-tool/domain'; -import { ToolContextType } from '../enum'; import { ContextExternalToolConfigurationStatus } from '../domain'; +import { ToolContextType } from '../enum'; import { ToolVersion } from '../interface'; // TODO N21-1337 remove class when tool versioning is removed @@ -20,6 +20,7 @@ export class CommonToolService { const configurationStatus: ContextExternalToolConfigurationStatus = new ContextExternalToolConfigurationStatus({ isOutdatedOnScopeContext: true, isOutdatedOnScopeSchool: true, + isIncompleteOnScopeContext: false, }); if ( diff --git a/apps/server/src/modules/tool/common/service/index.ts b/apps/server/src/modules/tool/common/service/index.ts index 2b2b9acae68..b7f626f1e42 100644 --- a/apps/server/src/modules/tool/common/service/index.ts +++ b/apps/server/src/modules/tool/common/service/index.ts @@ -1,2 +1,2 @@ export * from './common-tool.service'; -export * from './common-tool-validation.service'; +export { CommonToolValidationService, ToolParameterTypeValidationUtil } from './validation'; diff --git a/apps/server/src/modules/tool/common/service/common-tool-validation.service.spec.ts b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts similarity index 67% rename from apps/server/src/modules/tool/common/service/common-tool-validation.service.spec.ts rename to apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts index 1f76ec01a14..fe30e6caded 100644 --- a/apps/server/src/modules/tool/common/service/common-tool-validation.service.spec.ts +++ b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts @@ -1,15 +1,16 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { ValidationError } from '@shared/common'; import { contextExternalToolFactory, customParameterFactory, externalToolFactory, schoolExternalToolFactory, } from '@shared/testing'; -import { ContextExternalTool } from '../../context-external-tool/domain'; -import { ExternalTool } from '../../external-tool/domain'; -import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { CustomParameter } from '../domain'; -import { CustomParameterScope, CustomParameterType } from '../enum'; +import { ContextExternalTool } from '../../../context-external-tool/domain'; +import { ExternalTool } from '../../../external-tool/domain'; +import { SchoolExternalTool } from '../../../school-external-tool/domain'; +import { CustomParameter } from '../../domain'; +import { CustomParameterScope, CustomParameterType } from '../../enum'; import { CommonToolValidationService } from './common-tool-validation.service'; describe('CommonToolValidationService', () => { @@ -23,93 +24,7 @@ describe('CommonToolValidationService', () => { service = module.get(CommonToolValidationService); }); - describe('isValueValidForType', () => { - describe('when parameter type is string', () => { - it('should return true', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.STRING, 'test'); - - expect(result).toEqual(true); - }); - }); - - describe('when parameter type is boolean', () => { - describe('when value is true', () => { - it('should return true', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.BOOLEAN, 'true'); - - expect(result).toEqual(true); - }); - }); - - describe('when value is false', () => { - it('should return true', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.BOOLEAN, 'false'); - - expect(result).toEqual(true); - }); - }); - - describe('when value is not true or false', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.BOOLEAN, 'other'); - - expect(result).toEqual(false); - }); - }); - }); - - describe('when parameter type is number', () => { - describe('when value is a number', () => { - it('should return true', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.NUMBER, '1234'); - - expect(result).toEqual(true); - }); - }); - - describe('when value is not a number', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.NUMBER, 'NaN'); - - expect(result).toEqual(false); - }); - }); - }); - - describe('when defining a value for parameter of type auto_contextId', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.AUTO_CONTEXTID, 'test'); - - expect(result).toEqual(false); - }); - }); - - describe('when defining a value for parameter of type auto_contextId', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.AUTO_CONTEXTNAME, 'test'); - - expect(result).toEqual(false); - }); - }); - - describe('when defining a value for parameter of type auto_contextId', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.AUTO_SCHOOLID, 'test'); - - expect(result).toEqual(false); - }); - }); - - describe('when defining a value for parameter of type auto_contextId', () => { - it('should return false', () => { - const result: boolean = service.isValueValidForType(CustomParameterType.AUTO_SCHOOLNUMBER, 'test'); - - expect(result).toEqual(false); - }); - }); - }); - - describe('checkCustomParameterEntries', () => { + describe('validateParameters', () => { const createTools = ( externalToolMock?: Partial, schoolExternalToolMock?: Partial, @@ -163,12 +78,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_duplicate'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_duplicate'), + }) + ); }); }); @@ -188,12 +108,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_unknown'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_unknown'), + }) + ); }); }); @@ -221,12 +146,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_required'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_value_missing'), + }) + ); }); }); }); @@ -258,12 +188,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should not fail because of missing required context param', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError(); + expect(result).toHaveLength(0); }); }); @@ -298,9 +228,9 @@ describe('CommonToolValidationService', () => { it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError(); + expect(result).toHaveLength(0); }); }); @@ -326,12 +256,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_required'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_required'), + }) + ); }); }); @@ -364,12 +299,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_required'); + expect(result).toHaveLength(0); }); }); }); @@ -399,12 +334,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, contextExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, contextExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); - expect(func).toThrowError('tool_param_required'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_required'), + }) + ); }); }); @@ -422,7 +362,7 @@ describe('CommonToolValidationService', () => { }, undefined, { - parameters: [{ name: 'anotherParam', value: 'value' }], + parameters: [], } ); @@ -432,12 +372,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error ', () => { + it('should return without any error', () => { const { externalTool, contextExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, contextExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); - expect(func).not.toThrowError('tool_param_required'); + expect(result).toHaveLength(0); }); }); }); @@ -463,12 +403,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_type_mismatch'); + expect(result).toHaveLength(0); }); }); @@ -494,12 +434,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_type_mismatch'); + expect(result).toHaveLength(0); }); }); @@ -524,12 +464,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_type_mismatch'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_type_mismatch'), + }) + ); }); }); }); @@ -556,12 +501,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_type_mismatch'); + expect(result).toHaveLength(0); }); }); @@ -586,12 +531,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_type_mismatch'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_type_mismatch'), + }) + ); }); }); }); @@ -621,12 +571,12 @@ describe('CommonToolValidationService', () => { }; }; - it('return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_value_regex'); + expect(result).toHaveLength(0); }); }); @@ -654,12 +604,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_value_regex'); + expect(result).toHaveLength(0); }); }); @@ -687,12 +637,17 @@ describe('CommonToolValidationService', () => { }; }; - it('should throw error', () => { + it('should return an error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).toThrowError('tool_param_value_regex'); + expect(result).toContainEqual( + expect.objectContaining>({ + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + message: expect.stringContaining('tool_param_value_regex'), + }) + ); }); }); @@ -710,7 +665,7 @@ describe('CommonToolValidationService', () => { parameters: [optionalRegex], }, { - parameters: [{ name: 'optionalRegex', value: undefined }], + parameters: [], } ); @@ -720,12 +675,12 @@ describe('CommonToolValidationService', () => { }; }; - it('should return without error', () => { + it('should return without any error', () => { const { externalTool, schoolExternalTool } = setup(); - const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(func).not.toThrowError('tool_param_value_regex'); + expect(result).toHaveLength(0); }); }); }); diff --git a/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.ts b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.ts new file mode 100644 index 00000000000..1ba0740d0dd --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.ts @@ -0,0 +1,42 @@ +import { Injectable } from '@nestjs/common'; +import { ValidationError } from '@shared/common'; +import { ContextExternalTool } from '../../../context-external-tool/domain'; +import { ExternalTool } from '../../../external-tool/domain'; +import { SchoolExternalTool } from '../../../school-external-tool/domain'; +import { CustomParameter } from '../../domain'; +import { CustomParameterScope } from '../../enum'; +import { + ParameterArrayDuplicateKeyValidator, + ParameterArrayEntryValidator, + ParameterArrayUnknownKeyValidator, + ParameterArrayValidator, +} from './rules'; + +export type ValidatableTool = SchoolExternalTool | ContextExternalTool; + +@Injectable() +export class CommonToolValidationService { + private readonly arrayValidators: ParameterArrayValidator[] = [ + new ParameterArrayDuplicateKeyValidator(), + new ParameterArrayUnknownKeyValidator(), + new ParameterArrayEntryValidator(), + ]; + + public validateParameters(loadedExternalTool: ExternalTool, validatableTool: ValidatableTool): ValidationError[] { + const errors: ValidationError[] = []; + + const parametersForScope: CustomParameter[] = (loadedExternalTool.parameters ?? []).filter( + (param: CustomParameter) => + (validatableTool instanceof SchoolExternalTool && param.scope === CustomParameterScope.SCHOOL) || + (validatableTool instanceof ContextExternalTool && param.scope === CustomParameterScope.CONTEXT) + ); + + this.arrayValidators.forEach((validator: ParameterArrayValidator) => { + const entryErrors: ValidationError[] = validator.validate(validatableTool.parameters, parametersForScope); + + errors.push(...entryErrors); + }); + + return errors; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/index.ts b/apps/server/src/modules/tool/common/service/validation/index.ts new file mode 100644 index 00000000000..0d065368f38 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/index.ts @@ -0,0 +1,2 @@ +export { CommonToolValidationService } from './common-tool-validation.service'; +export { ToolParameterTypeValidationUtil } from './tool-parameter-type-validation.util'; diff --git a/apps/server/src/modules/tool/common/service/validation/rules/index.ts b/apps/server/src/modules/tool/common/service/validation/rules/index.ts new file mode 100644 index 00000000000..e03736a6496 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/index.ts @@ -0,0 +1,8 @@ +export { ParameterArrayValidator } from './parameter-array-validator'; +export { ParameterEntryValidator } from './parameter-entry-validator'; +export { ParameterEntryTypeValidator } from './parameter-entry-type-validator'; +export { ParameterArrayEntryValidator } from './parameter-array-entry-validator'; +export { ParameterEntryRegexValidator } from './parameter-entry-regex-validator'; +export { ParameterEntryValueValidator } from './parameter-entry-value-validator'; +export { ParameterArrayUnknownKeyValidator } from './parameter-array-unknown-key-validator'; +export { ParameterArrayDuplicateKeyValidator } from './parameter-array-duplicate-key-validator'; diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.ts new file mode 100644 index 00000000000..f2b0ccbd075 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.ts @@ -0,0 +1,22 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterDuplicateLoggableException } from '../../../domain'; +import { ParameterArrayValidator } from './parameter-array-validator'; + +export class ParameterArrayDuplicateKeyValidator implements ParameterArrayValidator { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + validate(entries: CustomParameterEntry[], declarations: CustomParameter[]): ValidationError[] { + const caseInsensitiveNames: string[] = entries.map(({ name }: CustomParameterEntry) => name.toLowerCase()); + + const duplicates: string[] = caseInsensitiveNames.filter( + (item, index) => caseInsensitiveNames.indexOf(item) !== index + ); + + const uniqueDuplicates: Set = new Set(duplicates); + + const errors: ValidationError[] = Array.from(uniqueDuplicates).map( + (parameterName: string) => new ToolParameterDuplicateLoggableException(parameterName) + ); + + return errors; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.ts new file mode 100644 index 00000000000..66c7e34d553 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.ts @@ -0,0 +1,37 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterRequiredLoggableException } from '../../../domain'; +import { ParameterArrayValidator } from './parameter-array-validator'; +import { ParameterEntryRegexValidator } from './parameter-entry-regex-validator'; +import { ParameterEntryTypeValidator } from './parameter-entry-type-validator'; +import { ParameterEntryValidator } from './parameter-entry-validator'; +import { ParameterEntryValueValidator } from './parameter-entry-value-validator'; + +export class ParameterArrayEntryValidator implements ParameterArrayValidator { + private readonly entryValidators: ParameterEntryValidator[] = [ + new ParameterEntryValueValidator(), + new ParameterEntryTypeValidator(), + new ParameterEntryRegexValidator(), + ]; + + validate(entries: CustomParameterEntry[], declarations: CustomParameter[]): ValidationError[] { + const errors: ValidationError[] = []; + + for (const param of declarations) { + const foundEntry: CustomParameterEntry | undefined = entries.find( + ({ name }: CustomParameterEntry): boolean => name === param.name + ); + + if (foundEntry) { + this.entryValidators.forEach((validator: ParameterEntryValidator) => { + const entryErrors: ValidationError[] = validator.validate(foundEntry, param); + + errors.push(...entryErrors); + }); + } else if (!param.isOptional) { + errors.push(new ToolParameterRequiredLoggableException(param)); + } + } + + return errors; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.ts new file mode 100644 index 00000000000..ef8e5d1e922 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.ts @@ -0,0 +1,21 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterUnknownLoggableException } from '../../../domain'; +import { ParameterArrayValidator } from './parameter-array-validator'; + +export class ParameterArrayUnknownKeyValidator implements ParameterArrayValidator { + validate(entries: CustomParameterEntry[], declarations: CustomParameter[]): ValidationError[] { + const errors: ValidationError[] = []; + + for (const entry of entries) { + const foundParameter: CustomParameter | undefined = declarations.find( + ({ name }: CustomParameter): boolean => name === entry.name + ); + + if (!foundParameter) { + errors.push(new ToolParameterUnknownLoggableException(entry)); + } + } + + return errors; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-validator.ts new file mode 100644 index 00000000000..0360108332a --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-validator.ts @@ -0,0 +1,6 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry } from '../../../domain'; + +export interface ParameterArrayValidator { + validate(entries: CustomParameterEntry[], declarations: CustomParameter[]): ValidationError[]; +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.ts new file mode 100644 index 00000000000..9adfc87cbe6 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.ts @@ -0,0 +1,13 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterValueRegexLoggableException } from '../../../domain'; +import { ParameterEntryValidator } from './parameter-entry-validator'; + +export class ParameterEntryRegexValidator implements ParameterEntryValidator { + public validate(entry: CustomParameterEntry, declaration: CustomParameter): ValidationError[] { + if (entry.value !== undefined && declaration.regex && !new RegExp(declaration.regex).test(entry.value)) { + return [new ToolParameterValueRegexLoggableException(declaration)]; + } + + return []; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.ts new file mode 100644 index 00000000000..42164a58e67 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.ts @@ -0,0 +1,17 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterTypeMismatchLoggableException } from '../../../domain'; +import { ToolParameterTypeValidationUtil } from '../tool-parameter-type-validation.util'; +import { ParameterEntryValidator } from './parameter-entry-validator'; + +export class ParameterEntryTypeValidator implements ParameterEntryValidator { + public validate(entry: CustomParameterEntry, declaration: CustomParameter): ValidationError[] { + if ( + entry.value !== undefined && + !ToolParameterTypeValidationUtil.isValueValidForType(declaration.type, entry.value) + ) { + return [new ToolParameterTypeMismatchLoggableException(declaration)]; + } + + return []; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-validator.ts new file mode 100644 index 00000000000..af3c7501dee --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-validator.ts @@ -0,0 +1,6 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry } from '../../../domain'; + +export interface ParameterEntryValidator { + validate(entry: CustomParameterEntry, declaration: CustomParameter): ValidationError[]; +} diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.ts new file mode 100644 index 00000000000..952698ff104 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.ts @@ -0,0 +1,13 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameter, CustomParameterEntry, ToolParameterValueMissingLoggableException } from '../../../domain'; +import { ParameterEntryValidator } from './parameter-entry-validator'; + +export class ParameterEntryValueValidator implements ParameterEntryValidator { + public validate(entry: CustomParameterEntry, declaration: CustomParameter): ValidationError[] { + if (entry.value === undefined || entry.value === '') { + return [new ToolParameterValueMissingLoggableException(declaration)]; + } + + return []; + } +} diff --git a/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts new file mode 100644 index 00000000000..6ed1d14c07a --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts @@ -0,0 +1,66 @@ +import { CustomParameterType } from '../../enum'; +import { ToolParameterTypeValidationUtil } from './tool-parameter-type-validation.util'; + +describe(ToolParameterTypeValidationUtil.name, () => { + describe('isValueValidForType', () => { + describe('when the type is "string"', () => { + it('should return true', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.STRING, + '12345' + ); + + expect(result).toEqual(true); + }); + }); + + describe('when the type is "number" and the value is a number', () => { + it('should return true', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType(CustomParameterType.NUMBER, '17'); + + expect(result).toEqual(true); + }); + }); + + describe('when the type is "number" and the value is a not a number', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType(CustomParameterType.NUMBER, 'NaN'); + + expect(result).toEqual(false); + }); + }); + + describe('when the type is "boolean" and the value is a boolean', () => { + it('should return true', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.BOOLEAN, + 'true' + ); + + expect(result).toEqual(true); + }); + }); + + describe('when the type is "boolean" and the value is not a boolean', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.BOOLEAN, + 'not true' + ); + + expect(result).toEqual(false); + }); + }); + + describe('when the type is an auto type', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.AUTO_CONTEXTNAME, + 'any value' + ); + + expect(result).toEqual(false); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.ts b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.ts new file mode 100644 index 00000000000..f765cb68782 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.ts @@ -0,0 +1,22 @@ +import { isNaN } from 'lodash'; +import { CustomParameterType } from '../../enum'; + +export class ToolParameterTypeValidationUtil { + private static typeCheckers: { [key in CustomParameterType]: (val: string) => boolean } = { + [CustomParameterType.STRING]: () => true, + [CustomParameterType.NUMBER]: (val: string) => !isNaN(Number(val)), + [CustomParameterType.BOOLEAN]: (val: string) => val === 'true' || val === 'false', + [CustomParameterType.AUTO_CONTEXTID]: () => false, + [CustomParameterType.AUTO_CONTEXTNAME]: () => false, + [CustomParameterType.AUTO_SCHOOLID]: () => false, + [CustomParameterType.AUTO_SCHOOLNUMBER]: () => false, + }; + + public static isValueValidForType(type: CustomParameterType, val: string): boolean { + const rule = this.typeCheckers[type]; + + const isValid: boolean = rule(val); + + return isValid; + } +} diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.spec.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.spec.ts index 41e849b3d79..961ca2a8423 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.spec.ts @@ -1,7 +1,7 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { ValidationError } from '@mikro-orm/core'; import { UnprocessableEntityException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; +import { ValidationError } from '@shared/common'; import { contextExternalToolFactory, externalToolFactory } from '@shared/testing'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; @@ -70,6 +70,7 @@ describe('ContextExternalToolValidationService', () => { contextExternalToolService.findContextExternalTools.mockResolvedValue([ contextExternalToolFactory.buildWithId({ displayName: 'Tool 2' }), ]); + commonToolValidationService.validateParameters.mockReturnValue([]); return { externalTool, @@ -101,10 +102,7 @@ describe('ContextExternalToolValidationService', () => { await service.validate(contextExternalTool); - expect(commonToolValidationService.checkCustomParameterEntries).toBeCalledWith( - externalTool, - contextExternalTool - ); + expect(commonToolValidationService.validateParameters).toBeCalledWith(externalTool, contextExternalTool); }); it('should not throw UnprocessableEntityException', async () => { @@ -167,5 +165,35 @@ describe('ContextExternalToolValidationService', () => { }); }); }); + + describe('when the parameter validation fails', () => { + const setup = () => { + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + + const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ + displayName: 'Tool 1', + }); + + const error: ValidationError = new ValidationError(''); + + externalToolService.findById.mockResolvedValue(externalTool); + contextExternalToolService.findContextExternalTools.mockResolvedValue([ + contextExternalToolFactory.buildWithId({ displayName: 'Tool 2' }), + ]); + commonToolValidationService.validateParameters.mockReturnValue([error]); + + return { + externalTool, + contextExternalTool, + error, + }; + }; + + it('should throw an error', async () => { + const { contextExternalTool, error } = setup(); + + await expect(service.validate(contextExternalTool)).rejects.toThrow(error); + }); + }); }); }); diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.ts index b12193eadd5..cd467efc5d5 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool-validation.service.ts @@ -26,7 +26,14 @@ export class ContextExternalToolValidationService { const loadedExternalTool: ExternalTool = await this.externalToolService.findById(loadedSchoolExternalTool.toolId); - this.commonToolValidationService.checkCustomParameterEntries(loadedExternalTool, contextExternalTool); + const errors: ValidationError[] = this.commonToolValidationService.validateParameters( + loadedExternalTool, + contextExternalTool + ); + + if (errors.length) { + throw errors[0]; + } } private async checkDuplicateUsesInContext(contextExternalTool: ContextExternalTool) { diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.spec.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.spec.ts index d9e3e1a9c4a..f3c31e6e506 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.spec.ts @@ -83,7 +83,7 @@ describe('ToolReferenceService', () => { contextExternalToolService.findByIdOrFail.mockResolvedValueOnce(contextExternalTool); schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool); externalToolService.findById.mockResolvedValueOnce(externalTool); - toolVersionService.determineToolConfigurationStatus.mockResolvedValue( + toolVersionService.determineToolConfigurationStatus.mockReturnValue( toolConfigurationStatusFactory.build({ isOutdatedOnScopeSchool: true, isOutdatedOnScopeContext: false, diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.ts index 39894db0aa1..46ae330e5a0 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-reference.service.ts @@ -1,8 +1,8 @@ import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain/types'; +import { ContextExternalToolConfigurationStatus } from '../../common/domain'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; -import { ContextExternalToolConfigurationStatus } from '../../common/domain'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool, ToolReference } from '../domain'; @@ -29,12 +29,11 @@ export class ToolReferenceService { ); const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - const status: ContextExternalToolConfigurationStatus = - await this.toolVersionService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); + const status: ContextExternalToolConfigurationStatus = this.toolVersionService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); const toolReference: ToolReference = ToolReferenceMapper.mapToToolReference( externalTool, diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts index 7edeca8459e..61ef594520a 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts @@ -1,25 +1,25 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { Test, TestingModule } from '@nestjs/testing'; -import { ApiValidationError } from '@shared/common'; +import { ValidationError } from '@shared/common'; import { contextExternalToolFactory, + customParameterFactory, externalToolFactory, schoolExternalToolFactory, - toolConfigurationStatusFactory, } from '@shared/testing'; -import { ContextExternalToolConfigurationStatus } from '../../common/domain'; -import { CommonToolService } from '../../common/service'; -import { SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { + ContextExternalToolConfigurationStatus, + ToolParameterValueMissingLoggableException, +} from '../../common/domain'; +import { CommonToolService, CommonToolValidationService } from '../../common/service'; import { IToolFeatures, ToolFeatures } from '../../tool-config'; -import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; import { ToolVersionService } from './tool-version-service'; describe('ToolVersionService', () => { let module: TestingModule; let service: ToolVersionService; - let contextExternalToolValidationService: DeepMocked; - let schoolExternalToolValidationService: DeepMocked; + let commonToolValidationService: DeepMocked; let commonToolService: DeepMocked; let toolFeatures: DeepMocked; @@ -28,12 +28,8 @@ describe('ToolVersionService', () => { providers: [ ToolVersionService, { - provide: ContextExternalToolValidationService, - useValue: createMock(), - }, - { - provide: SchoolExternalToolValidationService, - useValue: createMock(), + provide: CommonToolValidationService, + useValue: createMock(), }, { provide: CommonToolService, @@ -49,8 +45,7 @@ describe('ToolVersionService', () => { }).compile(); service = module.get(ToolVersionService); - contextExternalToolValidationService = module.get(ContextExternalToolValidationService); - schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); + commonToolValidationService = module.get(CommonToolValidationService); commonToolService = module.get(CommonToolService); toolFeatures = module.get(ToolFeatures); }); @@ -83,10 +78,10 @@ describe('ToolVersionService', () => { }; }; - it('should call CommonToolService', async () => { + it('should call CommonToolService', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); expect(commonToolService.determineToolConfigurationStatus).toHaveBeenCalledWith( externalTool, @@ -108,8 +103,7 @@ describe('ToolVersionService', () => { toolFeatures.toolStatusWithoutVersions = true; - schoolExternalToolValidationService.validate.mockResolvedValue(); - contextExternalToolValidationService.validate.mockResolvedValueOnce(); + commonToolValidationService.validateParameters.mockReturnValue([]); return { externalTool, @@ -118,37 +112,36 @@ describe('ToolVersionService', () => { }; }; - it('should return latest tool status', async () => { + it('should return latest tool status', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - const status: ContextExternalToolConfigurationStatus = await service.determineToolConfigurationStatus( + const status: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( externalTool, schoolExternalTool, contextExternalTool ); - expect(status).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - }) - ); + expect(status).toEqual({ + isOutdatedOnScopeSchool: false, + isOutdatedOnScopeContext: false, + isIncompleteOnScopeContext: false, + }); }); - it('should call schoolExternalToolValidationService', async () => { + it('should validate the school external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, schoolExternalTool); }); - it('should call contextExternalToolValidationService', async () => { + it('should validate the context external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, contextExternalTool); }); }); @@ -164,8 +157,8 @@ describe('ToolVersionService', () => { toolFeatures.toolStatusWithoutVersions = true; - schoolExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - contextExternalToolValidationService.validate.mockResolvedValue(); + commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); + commonToolValidationService.validateParameters.mockReturnValueOnce([]); return { externalTool, @@ -174,37 +167,36 @@ describe('ToolVersionService', () => { }; }; - it('should return outdated tool status', async () => { + it('should return outdated tool status', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - const status: ContextExternalToolConfigurationStatus = await service.determineToolConfigurationStatus( + const status: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( externalTool, schoolExternalTool, contextExternalTool ); - expect(status).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: true, - }) - ); + expect(status).toEqual({ + isOutdatedOnScopeSchool: true, + isOutdatedOnScopeContext: false, + isIncompleteOnScopeContext: false, + }); }); - it('should call schoolExternalToolValidationService', async () => { + it('should validate the school external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, schoolExternalTool); }); - it('should call contextExternalToolValidationService', async () => { + it('should validate the context external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, contextExternalTool); }); }); @@ -220,8 +212,8 @@ describe('ToolVersionService', () => { toolFeatures.toolStatusWithoutVersions = true; - schoolExternalToolValidationService.validate.mockResolvedValue(); - contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); + commonToolValidationService.validateParameters.mockReturnValueOnce([]); + commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); return { externalTool, @@ -230,37 +222,36 @@ describe('ToolVersionService', () => { }; }; - it('should return outdated tool status', async () => { + it('should return outdated tool status', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - const status: ContextExternalToolConfigurationStatus = await service.determineToolConfigurationStatus( + const status: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( externalTool, schoolExternalTool, contextExternalTool ); - expect(status).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: false, - }) - ); + expect(status).toEqual({ + isOutdatedOnScopeSchool: false, + isOutdatedOnScopeContext: true, + isIncompleteOnScopeContext: false, + }); }); - it('should call schoolExternalToolValidationService', async () => { + it('should validate the school external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, schoolExternalTool); }); - it('should call contextExternalToolValidationService', async () => { + it('should validate the context external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, contextExternalTool); }); }); @@ -276,8 +267,8 @@ describe('ToolVersionService', () => { toolFeatures.toolStatusWithoutVersions = true; - schoolExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); + commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); + commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); return { externalTool, @@ -286,37 +277,78 @@ describe('ToolVersionService', () => { }; }; - it('should return outdated tool status', async () => { + it('should return outdated tool status', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - const status: ContextExternalToolConfigurationStatus = await service.determineToolConfigurationStatus( + const status: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( externalTool, schoolExternalTool, contextExternalTool ); - expect(status).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: true, - }) - ); + expect(status).toEqual({ + isOutdatedOnScopeSchool: true, + isOutdatedOnScopeContext: true, + isIncompleteOnScopeContext: false, + }); + }); + + it('should validate the school external tool', () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, schoolExternalTool); }); - it('should call schoolExternalToolValidationService', async () => { + it('should validate the context external tool', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, contextExternalTool); }); + }); + + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation of ContextExternalTool throws only missing value errors', () => { + const setup = () => { + const customParameter = customParameterFactory.build(); + const externalTool = externalToolFactory.buildWithId({ parameters: [customParameter] }); + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + toolId: externalTool.id as string, + }); + const contextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .buildWithId(); + + toolFeatures.toolStatusWithoutVersions = true; + + commonToolValidationService.validateParameters.mockReturnValueOnce([]); + commonToolValidationService.validateParameters.mockReturnValueOnce([ + new ToolParameterValueMissingLoggableException(customParameter), + ]); + + return { + externalTool, + schoolExternalTool, + contextExternalTool, + }; + }; - it('should call contextExternalToolValidationService', async () => { + it('should return incomplete as tool status', () => { const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + const status: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); - expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(status).toEqual({ + isOutdatedOnScopeSchool: false, + isOutdatedOnScopeContext: true, + isIncompleteOnScopeContext: true, + }); }); }); }); diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts index 191e1d0cc77..f24faa4ec99 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts @@ -1,49 +1,66 @@ import { Inject } from '@nestjs/common'; import { Injectable } from '@nestjs/common/decorators/core/injectable.decorator'; -import { ContextExternalToolConfigurationStatus } from '../../common/domain'; -import { CommonToolService } from '../../common/service'; +import { ValidationError } from '@shared/common'; +import { + ContextExternalToolConfigurationStatus, + ToolParameterValueMissingLoggableException, +} from '../../common/domain'; +import { CommonToolService, CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolValidationService } from '../../school-external-tool/service'; import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { ContextExternalTool } from '../domain'; -import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; @Injectable() export class ToolVersionService { constructor( - private readonly contextExternalToolValidationService: ContextExternalToolValidationService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, private readonly commonToolService: CommonToolService, + private readonly commonToolValidationService: CommonToolValidationService, @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} - async determineToolConfigurationStatus( + determineToolConfigurationStatus( externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool - ): Promise { + ): ContextExternalToolConfigurationStatus { // TODO N21-1337 remove if statement, when feature flag is removed if (this.toolFeatures.toolStatusWithoutVersions) { const configurationStatus: ContextExternalToolConfigurationStatus = new ContextExternalToolConfigurationStatus({ isOutdatedOnScopeContext: false, + isIncompleteOnScopeContext: false, isOutdatedOnScopeSchool: false, }); - try { - await this.schoolExternalToolValidationService.validate(schoolExternalTool); - } catch (err) { + const schoolParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( + externalTool, + schoolExternalTool + ); + + if (schoolParameterErrors.length) { configurationStatus.isOutdatedOnScopeSchool = true; } - try { - await this.contextExternalToolValidationService.validate(contextExternalTool); - } catch (err) { + const contextParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( + externalTool, + contextExternalTool + ); + + if (contextParameterErrors.length) { configurationStatus.isOutdatedOnScopeContext = true; + + if ( + contextParameterErrors.every( + (error: ValidationError) => error instanceof ToolParameterValueMissingLoggableException + ) + ) { + configurationStatus.isIncompleteOnScopeContext = true; + } } return configurationStatus; } + const status: ContextExternalToolConfigurationStatus = this.commonToolService.determineToolConfigurationStatus( externalTool, schoolExternalTool, diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.spec.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.spec.ts index b982f4cbf0a..fc02e6d4426 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.spec.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.spec.ts @@ -17,7 +17,6 @@ describe('ExternalToolParameterValidationService', () => { let service: ExternalToolParameterValidationService; let externalToolService: DeepMocked; - let commonToolValidationService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -36,9 +35,6 @@ describe('ExternalToolParameterValidationService', () => { service = module.get(ExternalToolParameterValidationService); externalToolService = module.get(ExternalToolService); - commonToolValidationService = module.get(CommonToolValidationService); - - commonToolValidationService.isValueValidForType.mockReturnValue(true); }); afterAll(async () => { @@ -342,7 +338,6 @@ describe('ExternalToolParameterValidationService', () => { const externalTool: ExternalTool = externalToolFactory.buildWithId({ parameters: [parameter] }); externalToolService.findExternalToolByName.mockResolvedValue(externalTool); - commonToolValidationService.isValueValidForType.mockReturnValue(false); return { externalTool, diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.ts index 87690959305..123efd0b165 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-parameter-validation.service.ts @@ -2,16 +2,13 @@ import { Injectable } from '@nestjs/common'; import { ValidationError } from '@shared/common'; import { CustomParameter } from '../../common/domain'; import { autoParameters, CustomParameterScope } from '../../common/enum'; -import { CommonToolValidationService } from '../../common/service'; +import { ToolParameterTypeValidationUtil } from '../../common/service'; import { ExternalTool } from '../domain'; import { ExternalToolService } from './external-tool.service'; @Injectable() export class ExternalToolParameterValidationService { - constructor( - private readonly externalToolService: ExternalToolService, - private readonly commonToolValidationService: CommonToolValidationService - ) {} + constructor(private readonly externalToolService: ExternalToolService) {} async validateCommon(externalTool: ExternalTool | Partial): Promise { if (!(await this.isNameUnique(externalTool))) { @@ -117,7 +114,7 @@ export class ExternalToolParameterValidationService { private isDefaultValueOfValidType(param: CustomParameter): boolean { if (param.default) { - const isValid: boolean = this.commonToolValidationService.isValueValidForType(param.type, param.default); + const isValid: boolean = ToolParameterTypeValidationUtil.isValueValidForType(param.type, param.default); return isValid; } diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts index 1f2ba7f5eb9..f10f8b4b620 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts @@ -87,7 +87,7 @@ describe('SchoolExternalToolValidationService', () => { await service.validate(schoolExternalTool); - expect(commonToolValidationService.checkCustomParameterEntries).toHaveBeenCalledWith( + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith( expect.anything(), schoolExternalTool ); diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.ts index 899055e321f..17ac81faf10 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.ts @@ -21,7 +21,14 @@ export class SchoolExternalToolValidationService { this.checkVersionMatch(schoolExternalTool.toolVersion, loadedExternalTool.version); } - this.commonToolValidationService.checkCustomParameterEntries(loadedExternalTool, schoolExternalTool); + const errors: ValidationError[] = this.commonToolValidationService.validateParameters( + loadedExternalTool, + schoolExternalTool + ); + + if (errors.length) { + throw errors[0]; + } } private checkVersionMatch(schoolExternalToolVersion: number, externalToolVersion: number): void { diff --git a/apps/server/src/shared/testing/factory/context-external-tool-configuration-status-response.factory.ts b/apps/server/src/shared/testing/factory/context-external-tool-configuration-status-response.factory.ts index a2e9ec6c2d2..e197732f60b 100644 --- a/apps/server/src/shared/testing/factory/context-external-tool-configuration-status-response.factory.ts +++ b/apps/server/src/shared/testing/factory/context-external-tool-configuration-status-response.factory.ts @@ -6,5 +6,6 @@ export const contextExternalToolConfigurationStatusResponseFactory = return { isOutdatedOnScopeContext: false, isOutdatedOnScopeSchool: false, + isIncompleteOnScopeContext: false, }; }); diff --git a/apps/server/src/shared/testing/factory/domainobject/tool/tool-configuration-status.factory.ts b/apps/server/src/shared/testing/factory/domainobject/tool/tool-configuration-status.factory.ts index e9d6e4f25d4..f8fb49d74c6 100644 --- a/apps/server/src/shared/testing/factory/domainobject/tool/tool-configuration-status.factory.ts +++ b/apps/server/src/shared/testing/factory/domainobject/tool/tool-configuration-status.factory.ts @@ -5,5 +5,6 @@ export const toolConfigurationStatusFactory = Factory.define Date: Thu, 4 Jan 2024 10:49:53 +0100 Subject: [PATCH 04/21] add tests --- .../board-do-copy.service.spec.ts | 228 +++++++++++++++--- .../board-do-copy.service.ts | 7 +- .../recursive-copy.visitor.spec.ts | 18 +- .../recursive-copy.visitor.ts | 23 +- .../service/column-board-copy.service.ts | 10 +- .../modules/copy-helper/types/copy.types.ts | 69 +++--- .../learnroom/service/course-copy.service.ts | 18 +- ...l-external-tool-validation.service.spec.ts | 96 ++++---- 8 files changed, 313 insertions(+), 156 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts index ecebe7ac65a..8de580de4f2 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts @@ -1,6 +1,9 @@ -import { createMock } from '@golevelup/ts-jest'; +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { FileRecordParentType } from '@infra/rabbitmq'; import { CopyElementType, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; +import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { Test, TestingModule } from '@nestjs/testing'; import { Card, @@ -9,9 +12,6 @@ import { DrawingElement, ExternalToolElement, FileElement, - LinkElement, - RichTextElement, - SubmissionContainerElement, isCard, isColumn, isColumnBoard, @@ -21,11 +21,15 @@ import { isLinkElement, isRichTextElement, isSubmissionContainerElement, + LinkElement, + RichTextElement, + SubmissionContainerElement, } from '@shared/domain/domainobject'; import { cardFactory, columnBoardFactory, columnFactory, + contextExternalToolFactory, drawingElementFactory, externalToolElementFactory, fileElementFactory, @@ -43,12 +47,21 @@ describe('recursive board copy visitor', () => { let module: TestingModule; let service: BoardDoCopyService; + let contextExternalToolService: DeepMocked; + beforeAll(async () => { module = await Test.createTestingModule({ - providers: [BoardDoCopyService], + providers: [ + BoardDoCopyService, + { + provide: ContextExternalToolService, + useValue: createMock(), + }, + ], }).compile(); service = module.get(BoardDoCopyService); + contextExternalToolService = module.get(ContextExternalToolService); await setupEntities(); }); @@ -711,61 +724,196 @@ describe('recursive board copy visitor', () => { }); }); - describe('when copying a external tool element', () => { - const setup = () => { - const original = externalToolElementFactory.build(); + describe('when copying an external tool element', () => { + describe('when the element has no linked tool', () => { + const setup = () => { + const original = externalToolElementFactory.build(); - return { original, ...setupfileCopyService() }; - }; + return { original, ...setupfileCopyService() }; + }; - const getExternalToolElementFromStatus = (status: CopyStatus): ExternalToolElement => { - const copy = status.copyEntity; + const getExternalToolElementFromStatus = (status: CopyStatus): ExternalToolElement => { + const copy = status.copyEntity; - expect(isExternalToolElement(copy)).toEqual(true); + expect(isExternalToolElement(copy)).toEqual(true); - return copy as ExternalToolElement; - }; + return copy as ExternalToolElement; + }; - it('should return a external tool element as copy', async () => { - const { original, fileCopyService } = setup(); + it('should return a external tool element as copy', async () => { + const { original, fileCopyService } = setup(); - const result = await service.copy({ original, fileCopyService }); + const result = await service.copy({ original, fileCopyService }); - expect(isExternalToolElement(result.copyEntity)).toEqual(true); - }); + expect(isExternalToolElement(result.copyEntity)).toEqual(true); + }); - it('should not copy tool', async () => { - const { original, fileCopyService } = setup(); + it('should not copy tool', async () => { + const { original, fileCopyService } = setup(); - const result = await service.copy({ original, fileCopyService }); - const copy = getExternalToolElementFromStatus(result); + const result = await service.copy({ original, fileCopyService }); + const copy = getExternalToolElementFromStatus(result); - expect(copy.contextExternalToolId).toBeUndefined(); - }); + expect(copy.contextExternalToolId).toBeUndefined(); + }); - it('should create new id', async () => { - const { original, fileCopyService } = setup(); + it('should create new id', async () => { + const { original, fileCopyService } = setup(); - const result = await service.copy({ original, fileCopyService }); - const copy = getExternalToolElementFromStatus(result); + const result = await service.copy({ original, fileCopyService }); + const copy = getExternalToolElementFromStatus(result); - expect(copy.id).not.toEqual(original.id); - }); + expect(copy.id).not.toEqual(original.id); + }); - it('should show status successful', async () => { - const { original, fileCopyService } = setup(); + it('should show status successful', async () => { + const { original, fileCopyService } = setup(); - const result = await service.copy({ original, fileCopyService }); + const result = await service.copy({ original, fileCopyService }); - expect(result.status).toEqual(CopyStatusEnum.SUCCESS); + expect(result.status).toEqual(CopyStatusEnum.SUCCESS); + }); + + it('should show type ExternalToolElement', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(result.type).toEqual(CopyElementType.EXTERNAL_TOOL_ELEMENT); + }); }); - it('should show type RichTextElement', async () => { - const { original, fileCopyService } = setup(); + describe('when the element has a linked tool and the feature is active', () => { + describe('when the linked tool exists', () => { + const setup = () => { + const originalTool: ContextExternalTool = contextExternalToolFactory.buildWithId(); + const copiedTool: ContextExternalTool = contextExternalToolFactory.buildWithId(); - const result = await service.copy({ original, fileCopyService }); + const original: ExternalToolElement = externalToolElementFactory.build({ + contextExternalToolId: originalTool.id, + }); + + contextExternalToolService.findById.mockResolvedValueOnce(originalTool); + contextExternalToolService.copyContextExternalTool.mockResolvedValueOnce(copiedTool); + + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + + return { original, ...setupfileCopyService(), copiedTool }; + }; + + const getExternalToolElementFromStatus = (status: CopyStatus): ExternalToolElement => { + const copy = status.copyEntity; + + expect(isExternalToolElement(copy)).toEqual(true); + + return copy as ExternalToolElement; + }; + + it('should return a external tool element as copy', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(isExternalToolElement(result.copyEntity)).toEqual(true); + }); + + it('should copy tool', async () => { + const { original, fileCopyService, copiedTool } = setup(); + + const result = await service.copy({ original, fileCopyService }); + const copy = getExternalToolElementFromStatus(result); + + expect(copy.contextExternalToolId).toEqual(copiedTool.id); + }); + + it('should create new id', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + const copy = getExternalToolElementFromStatus(result); + + expect(copy.id).not.toEqual(original.id); + }); + + it('should show status successful', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(result.status).toEqual(CopyStatusEnum.SUCCESS); + }); + + it('should show type ExternalToolElement', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(result.type).toEqual(CopyElementType.EXTERNAL_TOOL_ELEMENT); + }); + }); - expect(result.type).toEqual(CopyElementType.EXTERNAL_TOOL_ELEMENT); + describe('when the linked tool does not exist anymore', () => { + const setup = () => { + const original: ExternalToolElement = externalToolElementFactory.build({ + contextExternalToolId: new ObjectId().toHexString(), + }); + + contextExternalToolService.findById.mockResolvedValueOnce(null); + + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + + return { original, ...setupfileCopyService() }; + }; + + const getExternalToolElementFromStatus = (status: CopyStatus): ExternalToolElement => { + const copy = status.copyEntity; + + expect(isExternalToolElement(copy)).toEqual(true); + + return copy as ExternalToolElement; + }; + + it('should return a external tool element as copy', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(isExternalToolElement(result.copyEntity)).toEqual(true); + }); + + it('should not try to copy the tool', async () => { + const { original, fileCopyService } = setup(); + + await service.copy({ original, fileCopyService }); + + expect(contextExternalToolService.copyContextExternalTool).not.toHaveBeenCalled(); + }); + + it('should create new id', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + const copy = getExternalToolElementFromStatus(result); + + expect(copy.id).not.toEqual(original.id); + }); + + it('should show status fail', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(result.status).toEqual(CopyStatusEnum.FAIL); + }); + + it('should show type ExternalToolElement', async () => { + const { original, fileCopyService } = setup(); + + const result = await service.copy({ original, fileCopyService }); + + expect(result.type).toEqual(CopyElementType.EXTERNAL_TOOL_ELEMENT); + }); + }); }); }); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index e245965ff9d..9a2854af82a 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -1,20 +1,21 @@ +import { ContextExternalToolService } from '@modules//tool/context-external-tool/service'; import { CopyStatus } from '@modules/copy-helper'; import { Injectable } from '@nestjs/common'; import { AnyBoardDo } from '@shared/domain/domainobject'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; -import { ContextExternalToolService } from '../../../tool/context-external-tool/service'; export type BoardDoCopyParams = { original: AnyBoardDo; fileCopyService: SchoolSpecificFileCopyService; - contextExternalToolService: ContextExternalToolService; }; @Injectable() export class BoardDoCopyService { + constructor(private readonly contextExternalToolService: ContextExternalToolService) {} + public async copy(params: BoardDoCopyParams): Promise { - const visitor = new RecursiveCopyVisitor(params.fileCopyService, params.contextExternalToolService); + const visitor = new RecursiveCopyVisitor(params.fileCopyService, this.contextExternalToolService); const result = await visitor.copy(params.original); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts index 3fe6c7b6cbe..6a134bc9d52 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts @@ -3,6 +3,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { LinkElement } from '@shared/domain/domainobject'; import { linkElementFactory, setupEntities } from '@shared/testing'; import { CopyFileDto } from '@src/modules/files-storage-client/dto'; +import { ContextExternalToolService } from '../../../tool/context-external-tool/service'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyServiceFactory } from './school-specific-file-copy-service.factory'; @@ -10,20 +11,27 @@ import { SchoolSpecificFileCopyService } from './school-specific-file-copy.inter describe(RecursiveCopyVisitor.name, () => { let module: TestingModule; + let fileCopyServiceFactory: DeepMocked; + let contextExternalToolService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ providers: [ + RecursiveCopyVisitor, { provide: SchoolSpecificFileCopyServiceFactory, useValue: createMock(), }, - RecursiveCopyVisitor, + { + provide: ContextExternalToolService, + useValue: createMock(), + }, ], }).compile(); fileCopyServiceFactory = module.get(SchoolSpecificFileCopyServiceFactory); + contextExternalToolService = module.get(ContextExternalToolService); await setupEntities(); }); @@ -57,7 +65,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup(); const linkElement = linkElementFactory.build(); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); @@ -70,7 +78,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); @@ -83,7 +91,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl, newFileId } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; @@ -97,7 +105,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl: `https://abc.de/file/unknown-file-id` }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index 6d6ea6fdd5c..cba1554ce17 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -1,5 +1,7 @@ +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { FileRecordParentType } from '@infra/rabbitmq'; import { CopyElementType, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { AnyBoardDo, BoardCompositeVisitorAsync, @@ -16,10 +18,8 @@ import { import { LinkElement } from '@shared/domain/domainobject/board/link-element.do'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; -import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; -import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; import { ContextExternalTool } from '../../../tool/context-external-tool/domain'; +import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { resultMap = new Map(); @@ -242,6 +242,8 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { } async visitExternalToolElementAsync(original: ExternalToolElement): Promise { + let status: CopyStatusEnum = CopyStatusEnum.SUCCESS; + const copy = new ExternalToolElement({ id: new ObjectId().toHexString(), contextExternalToolId: undefined, @@ -251,19 +253,26 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { }); if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED') && original.contextExternalToolId) { - const tool: ContextExternalTool = await this.contextExternalToolService.findByIdOrFail( + const tool: ContextExternalTool | null = await this.contextExternalToolService.findById( original.contextExternalToolId ); - const copiedTool = await this.contextExternalToolService.copyContextExternalTool(tool, copy.id); + if (tool) { + const copiedTool: ContextExternalTool = await this.contextExternalToolService.copyContextExternalTool( + tool, + copy.id + ); - copy.contextExternalToolId = copiedTool.id; + copy.contextExternalToolId = copiedTool.id; + } else { + status = CopyStatusEnum.FAIL; + } } this.resultMap.set(original.id, { copyEntity: copy, type: CopyElementType.EXTERNAL_TOOL_ELEMENT, - status: CopyStatusEnum.SUCCESS, + status, }); this.copyMap.set(original.id, copy); diff --git a/apps/server/src/modules/board/service/column-board-copy.service.ts b/apps/server/src/modules/board/service/column-board-copy.service.ts index f062d6dcc44..1317f4b5c22 100644 --- a/apps/server/src/modules/board/service/column-board-copy.service.ts +++ b/apps/server/src/modules/board/service/column-board-copy.service.ts @@ -12,7 +12,6 @@ import { CourseRepo } from '@shared/repo'; import { BoardDoRepo } from '../repo'; import { BoardDoCopyService, SchoolSpecificFileCopyServiceFactory } from './board-do-copy-service'; import { SwapInternalLinksVisitor } from './board-do-copy-service/swap-internal-links.visitor'; -import { ContextExternalToolService } from '../../tool/context-external-tool/service'; @Injectable() export class ColumnBoardCopyService { @@ -21,8 +20,7 @@ export class ColumnBoardCopyService { private readonly courseRepo: CourseRepo, private readonly userService: UserService, private readonly boardDoCopyService: BoardDoCopyService, - private readonly fileCopyServiceFactory: SchoolSpecificFileCopyServiceFactory, - private readonly contextExternalToolService: ContextExternalToolService + private readonly fileCopyServiceFactory: SchoolSpecificFileCopyServiceFactory ) {} async copyColumnBoard(props: { @@ -45,11 +43,7 @@ export class ColumnBoardCopyService { userId: props.userId, }); - const copyStatus = await this.boardDoCopyService.copy({ - original: originalBoard, - fileCopyService, - contextExternalToolService: this.contextExternalToolService, - }); + const copyStatus = await this.boardDoCopyService.copy({ original: originalBoard, fileCopyService }); /* istanbul ignore next */ if (!isColumnBoard(copyStatus.copyEntity)) { diff --git a/apps/server/src/modules/copy-helper/types/copy.types.ts b/apps/server/src/modules/copy-helper/types/copy.types.ts index 0dff9249331..e1c4269bbc9 100644 --- a/apps/server/src/modules/copy-helper/types/copy.types.ts +++ b/apps/server/src/modules/copy-helper/types/copy.types.ts @@ -12,40 +12,41 @@ export type CopyStatus = { }; export enum CopyElementType { - 'BOARD' = 'BOARD', - 'CARD' = 'CARD', - 'COLUMN' = 'COLUMN', - 'COLUMNBOARD' = 'COLUMNBOARD', - 'CONTENT' = 'CONTENT', - 'COURSE' = 'COURSE', - 'COURSEGROUP_GROUP' = 'COURSEGROUP_GROUP', - 'EXTERNAL_TOOL_ELEMENT' = 'EXTERNAL_TOOL_ELEMENT', - 'FILE' = 'FILE', - 'FILE_ELEMENT' = 'FILE_ELEMENT', - 'DRAWING_ELEMENT' = 'DRAWING_ELEMENT', - 'FILE_GROUP' = 'FILE_GROUP', - 'LEAF' = 'LEAF', - 'LESSON' = 'LESSON', - 'LESSON_CONTENT_ETHERPAD' = 'LESSON_CONTENT_ETHERPAD', - 'LESSON_CONTENT_GEOGEBRA' = 'LESSON_CONTENT_GEOGEBRA', - 'LESSON_CONTENT_GROUP' = 'LESSON_CONTENT_GROUP', - 'LESSON_CONTENT_LERNSTORE' = 'LESSON_CONTENT_LERNSTORE', - 'LESSON_CONTENT_NEXBOARD' = 'LESSON_CONTENT_NEXBOARD', - 'LESSON_CONTENT_TASK' = 'LESSON_CONTENT_TASK', - 'LESSON_CONTENT_TEXT' = 'LESSON_CONTENT_TEXT', - 'LERNSTORE_MATERIAL' = 'LERNSTORE_MATERIAL', - 'LERNSTORE_MATERIAL_GROUP' = 'LERNSTORE_MATERIAL_GROUP', - 'LINK_ELEMENT' = 'LINK_ELEMENT', - 'LTITOOL_GROUP' = 'LTITOOL_GROUP', - 'METADATA' = 'METADATA', - 'RICHTEXT_ELEMENT' = 'RICHTEXT_ELEMENT', - 'SUBMISSION_CONTAINER_ELEMENT' = 'SUBMISSION_CONTAINER_ELEMENT', - 'SUBMISSION_ITEM' = 'SUBMISSION_ITEM', - 'SUBMISSION_GROUP' = 'SUBMISSION_GROUP', - 'TASK' = 'TASK', - 'TASK_GROUP' = 'TASK_GROUP', - 'TIME_GROUP' = 'TIME_GROUP', - 'USER_GROUP' = 'USER_GROUP', + BOARD = 'BOARD', + CARD = 'CARD', + COLUMN = 'COLUMN', + COLUMNBOARD = 'COLUMNBOARD', + CONTENT = 'CONTENT', + COURSE = 'COURSE', + COURSEGROUP_GROUP = 'COURSEGROUP_GROUP', + EXTERNAL_TOOL = 'EXTERNAL_TOOL', + EXTERNAL_TOOL_ELEMENT = 'EXTERNAL_TOOL_ELEMENT', + FILE = 'FILE', + FILE_ELEMENT = 'FILE_ELEMENT', + DRAWING_ELEMENT = 'DRAWING_ELEMENT', + FILE_GROUP = 'FILE_GROUP', + LEAF = 'LEAF', + LESSON = 'LESSON', + LESSON_CONTENT_ETHERPAD = 'LESSON_CONTENT_ETHERPAD', + LESSON_CONTENT_GEOGEBRA = 'LESSON_CONTENT_GEOGEBRA', + LESSON_CONTENT_GROUP = 'LESSON_CONTENT_GROUP', + LESSON_CONTENT_LERNSTORE = 'LESSON_CONTENT_LERNSTORE', + LESSON_CONTENT_NEXBOARD = 'LESSON_CONTENT_NEXBOARD', + LESSON_CONTENT_TASK = 'LESSON_CONTENT_TASK', + LESSON_CONTENT_TEXT = 'LESSON_CONTENT_TEXT', + LERNSTORE_MATERIAL = 'LERNSTORE_MATERIAL', + LERNSTORE_MATERIAL_GROUP = 'LERNSTORE_MATERIAL_GROUP', + LINK_ELEMENT = 'LINK_ELEMENT', + LTITOOL_GROUP = 'LTITOOL_GROUP', + METADATA = 'METADATA', + RICHTEXT_ELEMENT = 'RICHTEXT_ELEMENT', + SUBMISSION_CONTAINER_ELEMENT = 'SUBMISSION_CONTAINER_ELEMENT', + SUBMISSION_ITEM = 'SUBMISSION_ITEM', + SUBMISSION_GROUP = 'SUBMISSION_GROUP', + TASK = 'TASK', + TASK_GROUP = 'TASK_GROUP', + TIME_GROUP = 'TIME_GROUP', + USER_GROUP = 'USER_GROUP', } export enum CopyStatusEnum { diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index db12dca2b64..9b35c2cb288 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -1,14 +1,14 @@ +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { CopyElementType, CopyHelperService, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; import { Injectable } from '@nestjs/common'; import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; -import { BoardCopyService } from './board-copy.service'; -import { RoomsService } from './rooms.service'; -import { ContextExternalToolService } from '../../tool/context-external-tool/service'; import { ToolContextType } from '../../tool/common/enum'; import { ContextExternalTool, ContextRef } from '../../tool/context-external-tool/domain'; +import { ContextExternalToolService } from '../../tool/context-external-tool/service'; +import { BoardCopyService } from './board-copy.service'; +import { RoomsService } from './rooms.service'; type CourseCopyParams = { originalCourse: Course; @@ -53,7 +53,8 @@ export class CourseCopyService { const courseCopy = await this.copyCourseEntity({ user, originalCourse, copyName }); if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { const contextRef: ContextRef = { id: courseId, type: ToolContextType.COURSE }; - const contextExternalToolsInContext = await this.contextExternalToolService.findAllByContext(contextRef); + const contextExternalToolsInContext: ContextExternalTool[] = + await this.contextExternalToolService.findAllByContext(contextRef); await Promise.all( contextExternalToolsInContext.map(async (tool: ContextExternalTool): Promise => { @@ -103,11 +104,10 @@ export class CourseCopyService { type: CopyElementType.METADATA, status: CopyStatusEnum.SUCCESS, }, - /* { - // TODO N21-1507 WTH does this do? Implement logic for PARTIAL? - type: CopyElementType.EXTERNAL_TOOL_ELEMENT, + { + type: CopyElementType.EXTERNAL_TOOL, status: CopyStatusEnum.SUCCESS, - }, */ + }, { type: CopyElementType.USER_GROUP, status: CopyStatusEnum.NOT_DOING, diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts index f10f8b4b620..f7e4d5687e4 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool-validation.service.spec.ts @@ -1,6 +1,7 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { Test, TestingModule } from '@nestjs/testing'; -import { externalToolFactory, schoolExternalToolFactory } from '@shared/testing/factory/domainobject/tool'; +import { ValidationError } from '@shared/common'; +import { externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; @@ -8,7 +9,7 @@ import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; -describe('SchoolExternalToolValidationService', () => { +describe(SchoolExternalToolValidationService.name, () => { let module: TestingModule; let service: SchoolExternalToolValidationService; @@ -48,32 +49,21 @@ describe('SchoolExternalToolValidationService', () => { }); describe('validate', () => { - const setup = ( - externalToolDoMock?: Partial, - schoolExternalToolDoMock?: Partial - ) => { - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({ - ...schoolExternalToolFactory.buildWithId(), - ...schoolExternalToolDoMock, - }); - const externalTool: ExternalTool = new ExternalTool({ - ...externalToolFactory.buildWithId(), - ...externalToolDoMock, - }); - - const schoolExternalToolId = schoolExternalTool.id as string; + describe('when the schoolExternalTool is valid', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1337 }); + const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 8383 }); - externalToolService.findById.mockResolvedValue(externalTool); - toolFeatures.toolStatusWithoutVersions = true; + externalToolService.findById.mockResolvedValue(externalTool); + commonToolValidationService.validateParameters.mockReturnValueOnce([]); + toolFeatures.toolStatusWithoutVersions = true; - return { - schoolExternalTool, - ExternalTool, - schoolExternalToolId, + return { + schoolExternalTool, + externalTool, + }; }; - }; - describe('when schoolExternalTool is given', () => { it('should call externalToolService.findExternalToolById', async () => { const { schoolExternalTool } = setup(); @@ -83,22 +73,41 @@ describe('SchoolExternalToolValidationService', () => { }); it('should call commonToolValidationService.checkCustomParameterEntries', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool, externalTool } = setup(); await service.validate(schoolExternalTool); - expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith( - expect.anything(), - schoolExternalTool - ); + expect(commonToolValidationService.validateParameters).toHaveBeenCalledWith(externalTool, schoolExternalTool); }); it('should not throw error', async () => { - const { schoolExternalTool } = setup({ version: 8383 }, { toolVersion: 1337 }); + const { schoolExternalTool } = setup(); - const func = () => service.validate(schoolExternalTool); + await expect(service.validate(schoolExternalTool)).resolves.not.toThrow(); + }); + }); + + describe('when the schoolExternalTool is invalid', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + const error: ValidationError = new ValidationError(''); + + externalToolService.findById.mockResolvedValue(externalTool); + commonToolValidationService.validateParameters.mockReturnValueOnce([error]); + toolFeatures.toolStatusWithoutVersions = true; - await expect(func()).resolves.not.toThrow(); + return { + schoolExternalTool, + externalTool, + error, + }; + }; + + it('should throw an error', async () => { + const { schoolExternalTool, error } = setup(); + + await expect(service.validate(schoolExternalTool)).rejects.toThrow(error); }); }); }); @@ -106,37 +115,24 @@ describe('SchoolExternalToolValidationService', () => { // TODO N21-1337 refactor after feature flag is removed describe('validate with FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED on false', () => { describe('when version of externalTool and schoolExternalTool are different', () => { - const setup = ( - externalToolDoMock?: Partial, - schoolExternalToolDoMock?: Partial - ) => { - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({ - ...schoolExternalToolFactory.buildWithId(), - ...schoolExternalToolDoMock, - }); - const externalTool: ExternalTool = new ExternalTool({ - ...externalToolFactory.buildWithId(), - ...externalToolDoMock, - }); - - const schoolExternalToolId = schoolExternalTool.id as string; + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1337 }); + const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 8383 }); externalToolService.findById.mockResolvedValue(externalTool); toolFeatures.toolStatusWithoutVersions = false; return { schoolExternalTool, - ExternalTool, - schoolExternalToolId, }; }; it('should throw error', async () => { - const { schoolExternalTool } = setup({ version: 8383 }, { toolVersion: 1337 }); + const { schoolExternalTool } = setup(); const func = () => service.validate(schoolExternalTool); - await expect(func()).rejects.toThrowError('tool_version_mismatch:'); + await expect(func()).rejects.toThrowError('tool_version_mismatch'); }); }); }); From 6941217a3ee39ae14242e92ce04c06a7403f7c23 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 11:17:58 +0100 Subject: [PATCH 05/21] course copy unit tests --- .../service/course-copy.service.spec.ts | 55 +++++++++++++++++++ .../learnroom/service/course-copy.service.ts | 8 +-- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index c6982645c93..cb5882ed5d7 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -6,12 +6,17 @@ import { Course } from '@shared/domain/entity'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; import { boardFactory, + contextExternalToolFactory, courseFactory, courseGroupFactory, schoolFactory, setupEntities, userFactory, } from '@shared/testing'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; +import { ToolContextType } from '@modules/tool/common/enum'; +import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; import { BoardCopyService } from './board-copy.service'; import { CourseCopyService } from './course-copy.service'; import { RoomsService } from './rooms.service'; @@ -26,6 +31,7 @@ describe('course copy service', () => { let lessonCopyService: DeepMocked; let copyHelperService: DeepMocked; let userRepo: DeepMocked; + let contextExternalToolService: DeepMocked; afterAll(async () => { await module.close(); @@ -68,6 +74,10 @@ describe('course copy service', () => { provide: UserRepo, useValue: createMock(), }, + { + provide: ContextExternalToolService, + useValue: createMock(), + }, ], }).compile(); @@ -79,6 +89,7 @@ describe('course copy service', () => { lessonCopyService = module.get(LessonCopyService); copyHelperService = module.get(CopyHelperService); userRepo = module.get(UserRepo); + contextExternalToolService = module.get(ContextExternalToolService); }); beforeEach(() => { @@ -93,12 +104,14 @@ describe('course copy service', () => { const originalBoard = boardFactory.build({ course }); const courseCopy = courseFactory.buildWithId({ teachers: [user] }); const boardCopy = boardFactory.build({ course: courseCopy }); + const tools: ContextExternalTool[] = contextExternalToolFactory.buildList(2); userRepo.findById.mockResolvedValue(user); courseRepo.findById.mockResolvedValue(course); courseRepo.findAllByUserId.mockResolvedValue([allCourses, allCourses.length]); boardRepo.findByCourseId.mockResolvedValue(originalBoard); roomsService.updateBoard.mockResolvedValue(originalBoard); + contextExternalToolService.findAllByContext.mockResolvedValue(tools); const courseCopyName = 'Copy'; copyHelperService.deriveCopyName.mockReturnValue(courseCopyName); @@ -124,6 +137,7 @@ describe('course copy service', () => { courseCopyName, allCourses, boardCopyStatus, + tools, }; }; @@ -310,6 +324,47 @@ describe('course copy service', () => { expect(courseCopy.color).toEqual(course.color); }); + + describe('when FEATURE_CTL_TOOLS_COPY_ENABLED is true', () => { + it('should find all ctl tools for this course', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); + + expect(contextExternalToolService.findAllByContext).toHaveBeenCalledWith({ + id: course.id, + type: ToolContextType.COURSE, + }); + }); + + it('should copy all ctl tools', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + const { course, user, tools } = setup(); + const status = await service.copyCourse({ userId: user.id, courseId: course.id }); + const courseCopy = status.copyEntity as Course; + + expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[0], courseCopy.id); + expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[1], courseCopy.id); + }); + }); + + describe('when FEATURE_CTL_TOOLS_COPY_ENABLED is false', () => { + it('should not find ctl tools', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); + + expect(contextExternalToolService.findAllByContext).not.toHaveBeenCalled(); + }); + + it('should not copy ctl tools', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); + + expect(contextExternalToolService.copyContextExternalTool).not.toHaveBeenCalled(); + }); + }); }); describe('when course is empty', () => { diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index db12dca2b64..69e9f7de9c6 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -4,11 +4,11 @@ import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; import { Configuration } from '@hpi-schul-cloud/commons/lib'; -import { BoardCopyService } from './board-copy.service'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; +import { ToolContextType } from '@modules/tool/common/enum'; +import { ContextExternalTool, ContextRef } from '@modules/tool/context-external-tool/domain'; import { RoomsService } from './rooms.service'; -import { ContextExternalToolService } from '../../tool/context-external-tool/service'; -import { ToolContextType } from '../../tool/common/enum'; -import { ContextExternalTool, ContextRef } from '../../tool/context-external-tool/domain'; +import { BoardCopyService } from './board-copy.service'; type CourseCopyParams = { originalCourse: Course; From 48d60fba3d726e1f402271159f38bb358b2a56a0 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 13:20:17 +0100 Subject: [PATCH 06/21] tools copy unit tests --- .../context-external-tool.service.spec.ts | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.spec.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.spec.ts index a05a3bd7370..d2dfc41ae6a 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.spec.ts @@ -4,6 +4,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { ContextExternalToolRepo } from '@shared/repo'; import { contextExternalToolFactory, + customParameterFactory, externalToolFactory, legacySchoolDoFactory, schoolExternalToolFactory, @@ -20,6 +21,7 @@ import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalToolService } from '../../school-external-tool/service'; import { RestrictedContextMismatchLoggable } from './restricted-context-mismatch-loggabble'; import { CommonToolService } from '../../common/service'; +import { CustomParameter } from '../../common/domain'; describe('ContextExternalToolService', () => { let module: TestingModule; @@ -400,4 +402,87 @@ describe('ContextExternalToolService', () => { }); }); }); + + describe('copyContextExternalTool', () => { + const setup = () => { + const courseId: string = new ObjectId().toHexString(); + const contextCopyId: string = new ObjectId().toHexString(); + + const protectedParam: CustomParameter = customParameterFactory.build({ isProtected: true }); + const unprotectedParam: CustomParameter = customParameterFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId({ + parameters: [protectedParam, unprotectedParam], + }); + + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolId: externalTool.id }); + const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ + contextRef: { type: ToolContextType.COURSE, id: courseId }, + schoolToolRef: { schoolToolId: schoolExternalTool.id, schoolId: schoolExternalTool.schoolId }, + parameters: [ + { name: protectedParam.name, value: 'paramValue1' }, + { name: unprotectedParam.name, value: 'paramValue2' }, + ], + }); + + schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); + externalToolService.findById.mockResolvedValue(externalTool); + jest + .spyOn(contextExternalToolRepo, 'save') + .mockImplementation((tool: ContextExternalTool) => Promise.resolve(tool)); + + return { + contextCopyId, + contextExternalTool, + schoolExternalTool, + }; + }; + + it('should find schoolExternalTool', async () => { + const { contextExternalTool, contextCopyId } = setup(); + + await service.copyContextExternalTool(contextExternalTool, contextCopyId); + + expect(schoolExternalToolService.findById).toHaveBeenCalledWith(contextExternalTool.schoolToolRef.schoolToolId); + }); + + it('should find externalTool', async () => { + const { contextExternalTool, contextCopyId, schoolExternalTool } = setup(); + + await service.copyContextExternalTool(contextExternalTool, contextCopyId); + + expect(externalToolService.findById).toHaveBeenCalledWith(schoolExternalTool.toolId); + }); + + it('should remove values from protected parameters', async () => { + const { contextExternalTool, contextCopyId } = setup(); + + const copiedTool: ContextExternalTool = await service.copyContextExternalTool(contextExternalTool, contextCopyId); + + expect(copiedTool).toEqual>({ + id: undefined, + contextRef: { id: contextCopyId, type: ToolContextType.COURSE }, + displayName: contextExternalTool.displayName, + schoolToolRef: contextExternalTool.schoolToolRef, + toolVersion: contextExternalTool.toolVersion, + parameters: [ + { + name: contextExternalTool.parameters[0].name, + value: undefined, + }, + { + name: contextExternalTool.parameters[1].name, + value: contextExternalTool.parameters[1].value, + }, + ], + }); + }); + + it('should save copied tool', async () => { + const { contextExternalTool, contextCopyId } = setup(); + + await service.copyContextExternalTool(contextExternalTool, contextCopyId); + + expect(contextExternalToolRepo.save).toHaveBeenCalledWith(contextExternalTool); + }); + }); }); From d2f144417e53a9163cd0027184f5bbab1d8df9b7 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 13:30:11 +0100 Subject: [PATCH 07/21] linter fix --- .../tool-launch/service/tool-launch.service.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.ts b/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.ts index bb323017967..18f484c5f40 100644 --- a/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.ts +++ b/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.ts @@ -54,7 +54,7 @@ export class ToolLaunchService { const { externalTool, schoolExternalTool } = await this.loadToolHierarchy(schoolExternalToolId); - await this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); + this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); const strategy: ToolLaunchStrategy | undefined = this.strategies.get(externalTool.config.type); @@ -84,18 +84,17 @@ export class ToolLaunchService { }; } - private async isToolStatusLatestOrThrow( + private isToolStatusLatestOrThrow( userId: EntityId, externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool - ): Promise { - const status: ContextExternalToolConfigurationStatus = - await this.toolVersionService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); + ): void { + const status: ContextExternalToolConfigurationStatus = this.toolVersionService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); if (status.isOutdatedOnScopeSchool || status.isOutdatedOnScopeContext) { throw new ToolStatusOutdatedLoggableException( From 81a4f61ac42e1b43f295b1052bc301bb8da0987c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= Date: Thu, 4 Jan 2024 14:16:58 +0100 Subject: [PATCH 08/21] add migration --- .../service/course-copy.service.spec.ts | 8 +- .../learnroom/service/course-copy.service.ts | 26 +++--- backup/setup/migrations.json | 11 +++ ...undefined-parameters-from-external-tool.js | 82 +++++++++++++++++++ 4 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 migrations/1704369994725-remove-undefined-parameters-from-external-tool.js diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index cb5882ed5d7..b9ea5428636 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -1,6 +1,10 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { CopyElementType, CopyHelperService, CopyStatusEnum } from '@modules/copy-helper'; import { LessonCopyService } from '@modules/lesson/service'; +import { ToolContextType } from '@modules/tool/common/enum'; +import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { Test, TestingModule } from '@nestjs/testing'; import { Course } from '@shared/domain/entity'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; @@ -13,10 +17,6 @@ import { setupEntities, userFactory, } from '@shared/testing'; -import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; -import { ToolContextType } from '@modules/tool/common/enum'; -import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; import { BoardCopyService } from './board-copy.service'; import { CourseCopyService } from './course-copy.service'; import { RoomsService } from './rooms.service'; diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index fd1789ff0f6..a9b51056e1f 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -1,14 +1,14 @@ +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { CopyElementType, CopyHelperService, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; +import { ToolContextType } from '@modules/tool/common/enum'; +import { ContextExternalTool, ContextRef } from '@modules/tool/context-external-tool/domain'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { Injectable } from '@nestjs/common'; import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; -import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; -import { ToolContextType } from '@modules/tool/common/enum'; -import { ContextExternalTool, ContextRef } from '@modules/tool/context-external-tool/domain'; -import { RoomsService } from './rooms.service'; import { BoardCopyService } from './board-copy.service'; +import { RoomsService } from './rooms.service'; type CourseCopyParams = { originalCourse: Course; @@ -104,10 +104,6 @@ export class CourseCopyService { type: CopyElementType.METADATA, status: CopyStatusEnum.SUCCESS, }, - { - type: CopyElementType.EXTERNAL_TOOL, - status: CopyStatusEnum.SUCCESS, - }, { type: CopyElementType.USER_GROUP, status: CopyStatusEnum.NOT_DOING, @@ -123,9 +119,19 @@ export class CourseCopyService { boardStatus, ]; + if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { + elements.push({ + type: CopyElementType.EXTERNAL_TOOL, + status: CopyStatusEnum.SUCCESS, + }); + } + const courseGroupsExist = originalCourse.getCourseGroupItems().length > 0; if (courseGroupsExist) { - elements.push({ type: CopyElementType.COURSEGROUP_GROUP, status: CopyStatusEnum.NOT_IMPLEMENTED }); + elements.push({ + type: CopyElementType.COURSEGROUP_GROUP, + status: CopyStatusEnum.NOT_IMPLEMENTED, + }); } const status = { diff --git a/backup/setup/migrations.json b/backup/setup/migrations.json index 10c4d4b98b6..1f0458a975f 100644 --- a/backup/setup/migrations.json +++ b/backup/setup/migrations.json @@ -394,5 +394,16 @@ "$date": "2023-12-20T11:44:04.729Z" }, "__v": 0 + }, + { + "_id": { + "$oid": "65969f4adbb61670101696fb" + }, + "state": "up", + "name": "remove-undefined-parameters-from-external-tool", + "createdAt": { + "$date": "2024-01-04T12:06:34.725Z" + }, + "__v": 0 } ] diff --git a/migrations/1704369994725-remove-undefined-parameters-from-external-tool.js b/migrations/1704369994725-remove-undefined-parameters-from-external-tool.js new file mode 100644 index 00000000000..c46a469d51d --- /dev/null +++ b/migrations/1704369994725-remove-undefined-parameters-from-external-tool.js @@ -0,0 +1,82 @@ +const mongoose = require('mongoose'); +const { info, alert } = require('../src/logger'); + +const { connect, close } = require('../src/utils/database'); + +const ContextExternalTool = mongoose.model( + 'contextExternalTools202401041311', + new mongoose.Schema( + { + parameters: [ + { + name: { type: String, required: true }, + value: { type: String, required: false }, + }, + ], + }, + { + timestamps: true, + } + ), + 'context-external-tools' +); + +const SchoolExternalTool = mongoose.model( + 'schoolExternalTools202401041311', + new mongoose.Schema( + { + schoolParameters: [ + { + name: { type: String, required: true }, + value: { type: String, required: false }, + }, + ], + }, + { + timestamps: true, + } + ), + 'school-external-tools' +); + +module.exports = { + up: async function up() { + await connect(); + + const contextExternalToolResponse = await ContextExternalTool.updateMany( + { $or: [{ 'parameters.value': undefined }, { 'parameters.value': '' }] }, + { + $pull: { + parameters: { + $or: [{ value: undefined }, { value: '' }], + }, + }, + } + ) + .lean() + .exec(); + + info(`Removed ${contextExternalToolResponse.nModified} parameter(s) in context-external-tools`); + + const schoolExternalToolResponse = await SchoolExternalTool.updateMany( + { $or: [{ 'schoolParameters.value': undefined }, { 'schoolParameters.value': '' }] }, + { + $pull: { + schoolParameters: { + $or: [{ value: undefined }, { value: '' }], + }, + }, + } + ) + .lean() + .exec(); + + info(`Removed ${schoolExternalToolResponse.nModified} parameter(s) in school-external-tools`); + + await close(); + }, + + down: async function down() { + alert('This migration cannot be undone'); + }, +}; From c2db774406974225cbe4c88255557341fd5b321c Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 14:18:23 +0100 Subject: [PATCH 09/21] test fix --- .../board-do-copy-service/board-do-copy.service.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts index 8de580de4f2..f1e0702af55 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts @@ -66,6 +66,10 @@ describe('recursive board copy visitor', () => { await setupEntities(); }); + afterEach(() => { + jest.clearAllMocks(); + }); + const setupfileCopyService = () => { const fileCopyService = createMock(); From f9efbca2ba6ee15676e6fa92028731229523880f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= Date: Thu, 4 Jan 2024 15:08:08 +0100 Subject: [PATCH 10/21] improve tests for validation rules --- .../common-tool-validation.service.spec.ts | 661 ++---------------- ...eter-array-duplicate-key-validator.spec.ts | 57 ++ .../parameter-array-entry-validator.spec.ts | 90 +++ ...ameter-array-unknown-key-validator.spec.ts | 60 ++ .../parameter-entry-regex-validator.spec.ts | 60 ++ .../parameter-entry-type-validator.spec.ts | 61 ++ .../parameter-entry-value-validator.spec.ts | 82 +++ 7 files changed, 476 insertions(+), 595 deletions(-) create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.spec.ts create mode 100644 apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.spec.ts diff --git a/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts index fe30e6caded..8fe0c12d4b7 100644 --- a/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts +++ b/apps/server/src/modules/tool/common/service/validation/common-tool-validation.service.spec.ts @@ -6,11 +6,8 @@ import { externalToolFactory, schoolExternalToolFactory, } from '@shared/testing'; -import { ContextExternalTool } from '../../../context-external-tool/domain'; -import { ExternalTool } from '../../../external-tool/domain'; -import { SchoolExternalTool } from '../../../school-external-tool/domain'; -import { CustomParameter } from '../../domain'; -import { CustomParameterScope, CustomParameterType } from '../../enum'; +import { CustomParameterEntry } from '../../domain'; +import { CustomParameterScope } from '../../enum'; import { CommonToolValidationService } from './common-tool-validation.service'; describe('CommonToolValidationService', () => { @@ -25,50 +22,22 @@ describe('CommonToolValidationService', () => { }); describe('validateParameters', () => { - const createTools = ( - externalToolMock?: Partial, - schoolExternalToolMock?: Partial, - contextExternalToolMock?: Partial - ) => { - const externalTool: ExternalTool = new ExternalTool({ - ...externalToolFactory.buildWithId(), - ...externalToolMock, - }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({ - ...schoolExternalToolFactory.buildWithId(), - ...schoolExternalToolMock, - }); - const schoolExternalToolId = schoolExternalTool.id as string; - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.build({ - ...contextExternalToolFactory.buildWithId(), - ...contextExternalToolMock, - }); - - return { - externalTool, - schoolExternalTool, - schoolExternalToolId, - contextExternalTool, - }; - }; - - describe('when a parameter is a duplicate', () => { + describe('when validating a valid school external tool', () => { const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ + const externalTool = externalToolFactory.buildWithId({ parameters: [ customParameterFactory.build({ - name: 'duplicate', + name: 'param1', scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - isOptional: true, }), ], }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ - toolId: externalTool.id, + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ parameters: [ - { name: 'duplicate', value: undefined }, - { name: 'duplicate', value: undefined }, + new CustomParameterEntry({ + name: 'param1', + value: 'test', + }), ], }); @@ -78,28 +47,31 @@ describe('CommonToolValidationService', () => { }; }; - it('should return an error', () => { + it('should return an empty array', () => { const { externalTool, schoolExternalTool } = setup(); const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_duplicate'), - }) - ); + expect(result).toHaveLength(0); }); }); - describe('when a parameter is unknown', () => { + describe('when validating an invalid school external tool', () => { const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ - parameters: [], + const externalTool = externalToolFactory.buildWithId({ + parameters: [ + customParameterFactory.build({ + name: 'param1', + scope: CustomParameterScope.SCHOOL, + }), + ], }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ - toolId: externalTool.id, - parameters: [{ name: 'unknownParameter', value: undefined }], + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + parameters: [ + new CustomParameterEntry({ + name: 'param1', + }), + ], }); return { @@ -108,580 +80,79 @@ describe('CommonToolValidationService', () => { }; }; - it('should return an error', () => { + it('should return a validation error', () => { const { externalTool, schoolExternalTool } = setup(); const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_unknown'), - }) - ); + expect(result).toHaveLength(1); }); }); - describe('when checking parameter is required', () => { - describe('and given parameter is not optional and parameter value is empty', () => { - const setup = () => { - const requiredParam: CustomParameter = customParameterFactory.build({ - name: 'requiredParam', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - isOptional: false, - }); - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [requiredParam], - }, - { - parameters: [{ name: 'requiredParam', value: '' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_value_missing'), - }) - ); - }); - }); - }); - - describe('when checking parameters of school external tool', () => { + describe('when validating a valid context external tool', () => { const setup = () => { - const requiredContextParam: CustomParameter = customParameterFactory.build({ - name: 'missingContextParam', - isOptional: false, - scope: CustomParameterScope.CONTEXT, - type: CustomParameterType.BOOLEAN, + const externalTool = externalToolFactory.buildWithId({ + parameters: [ + customParameterFactory.build({ + name: 'param1', + scope: CustomParameterScope.CONTEXT, + }), + ], }); - const schoolParam: CustomParameter = customParameterFactory.build({ - name: 'schoolParam', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.BOOLEAN, + const contextExternalTool = contextExternalToolFactory.buildWithId({ + parameters: [ + new CustomParameterEntry({ + name: 'param1', + value: 'test', + }), + ], }); - const { externalTool, schoolExternalTool } = createTools( - { parameters: [requiredContextParam, schoolParam] }, - { - parameters: [{ name: 'schoolParam', value: 'true' }], - } - ); - return { externalTool, - schoolExternalTool, + contextExternalTool, }; }; - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); + it('should return an empty array', () => { + const { externalTool, contextExternalTool } = setup(); - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); + const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); expect(result).toHaveLength(0); }); }); - describe('when parameter is not school or context', () => { + describe('when validating an invalid context external tool', () => { const setup = () => { - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [ - customParameterFactory.build({ - name: 'notSchoolParam', - scope: CustomParameterScope.GLOBAL, - type: CustomParameterType.BOOLEAN, - }), - customParameterFactory.build({ - name: 'schoolParam', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.BOOLEAN, - }), - ], - }, - { - parameters: [{ name: 'schoolParam', value: 'true' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when parameter scope is school', () => { - describe('when required parameter is missing', () => { - const setup = () => { - const missingParam: CustomParameter = customParameterFactory.build({ - name: 'isMissing', - isOptional: false, - scope: CustomParameterScope.SCHOOL, - }); - - const { externalTool, schoolExternalTool } = createTools( - { parameters: [missingParam] }, - { - parameters: [], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_required'), - }) - ); - }); - }); - - describe('when parameter is optional and was not defined', () => { - const setup = () => { - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [ - customParameterFactory.build({ - name: 'optionalParameter', - scope: CustomParameterScope.SCHOOL, - isOptional: true, - }), - customParameterFactory.build({ - name: 'requiredParameter', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - isOptional: false, - }), - ], - }, - { - parameters: [{ name: 'requiredParameter', value: 'value' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - }); - - describe('when parameter scope is context', () => { - describe('when required parameter is missing', () => { - const setup = () => { - const missingParam: CustomParameter = customParameterFactory.build({ - name: 'isMissing', - isOptional: false, - scope: CustomParameterScope.CONTEXT, - }); - - const { externalTool, contextExternalTool } = createTools( - { - parameters: [missingParam], - }, - undefined, - { - parameters: [], - } - ); - - return { - externalTool, - contextExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, contextExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_required'), - }) - ); - }); - }); - - describe('when parameter is optional but is missing on params', () => { - const setup = () => { - const param: CustomParameter = customParameterFactory.build({ - name: 'notChecked', - scope: CustomParameterScope.CONTEXT, - isOptional: true, - }); - - const { externalTool, contextExternalTool } = createTools( - { - parameters: [param], - }, - undefined, - { - parameters: [], - } - ); - - return { - externalTool, - contextExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, contextExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); - - expect(result).toHaveLength(0); + const externalTool = externalToolFactory.buildWithId({ + parameters: [ + customParameterFactory.build({ + name: 'param1', + scope: CustomParameterScope.CONTEXT, + }), + ], }); - }); - }); - - describe('when checking parameter type string', () => { - const setup = () => { - const correctTypeParam: CustomParameter = customParameterFactory.build({ - name: 'correctType', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, + const contextExternalTool = contextExternalToolFactory.buildWithId({ + parameters: [ + new CustomParameterEntry({ + name: 'param1', + }), + ], }); - const { externalTool, schoolExternalTool } = createTools( - { parameters: [correctTypeParam] }, - { - parameters: [{ name: correctTypeParam.name, value: 'dasIstEinString' }], - } - ); - return { externalTool, - schoolExternalTool, + contextExternalTool, }; }; - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when checking parameter type number', () => { - describe('when type matches param value', () => { - const setup = () => { - const correctTypeParam: CustomParameter = customParameterFactory.build({ - name: 'correctType', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.NUMBER, - }); - - const { externalTool, schoolExternalTool } = createTools( - { parameters: [correctTypeParam] }, - { - parameters: [{ name: correctTypeParam.name, value: '1234' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; + it('should return a validation error', () => { + const { externalTool, contextExternalTool } = setup(); - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); + const result: ValidationError[] = service.validateParameters(externalTool, contextExternalTool); - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when type not matches param value', () => { - const setup = () => { - const wrongTypeParam: CustomParameter = customParameterFactory.build({ - name: 'wrongType', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.NUMBER, - }); - - const { externalTool, schoolExternalTool } = createTools( - { parameters: [wrongTypeParam] }, - { - parameters: [{ name: wrongTypeParam.name, value: '17271hsadas' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_type_mismatch'), - }) - ); - }); - }); - }); - - describe('when checking parameter type boolean', () => { - describe('when type matches param value', () => { - const setup = () => { - const correctTypeParam: CustomParameter = customParameterFactory.build({ - name: 'correctType', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.BOOLEAN, - }); - - const { externalTool, schoolExternalTool } = createTools( - { parameters: [correctTypeParam] }, - { - parameters: [{ name: correctTypeParam.name, value: 'true' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when type not matches param value', () => { - const setup = () => { - const wrongTypeParam: CustomParameter = customParameterFactory.build({ - name: 'wrongType', - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.BOOLEAN, - }); - - const { externalTool, schoolExternalTool } = createTools( - { parameters: [wrongTypeParam] }, - { - parameters: [{ name: wrongTypeParam.name, value: '17271hsadas' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_type_mismatch'), - }) - ); - }); - }); - }); - - describe('when validating regex', () => { - describe('when no regex is given', () => { - const setup = () => { - const undefinedRegex: CustomParameter = customParameterFactory.build({ - name: 'undefinedRegex', - isOptional: false, - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - regex: undefined, - }); - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [undefinedRegex], - }, - { - parameters: [{ name: 'undefinedRegex', value: 'xxxx' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when regex is given and param value is valid', () => { - const setup = () => { - const validRegex: CustomParameter = customParameterFactory.build({ - name: 'validRegex', - isOptional: false, - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - regex: '[x]', - }); - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [validRegex], - }, - { - parameters: [{ name: 'validRegex', value: 'xxxx' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); - }); - - describe('when regex is given and param value is invalid', () => { - const setup = () => { - const validRegex: CustomParameter = customParameterFactory.build({ - name: 'validRegex', - isOptional: false, - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - regex: '[x]', - }); - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [validRegex], - }, - { - parameters: [{ name: 'validRegex', value: 'abcdefasdhasd' }], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return an error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toContainEqual( - expect.objectContaining>({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - message: expect.stringContaining('tool_param_value_regex'), - }) - ); - }); - }); - - describe('when parameter is optional and a regex is given, but the param value is undefined', () => { - const setup = () => { - const optionalRegex: CustomParameter = customParameterFactory.build({ - name: 'optionalRegex', - isOptional: true, - scope: CustomParameterScope.SCHOOL, - type: CustomParameterType.STRING, - regex: '[x]', - }); - const { externalTool, schoolExternalTool } = createTools( - { - parameters: [optionalRegex], - }, - { - parameters: [], - } - ); - - return { - externalTool, - schoolExternalTool, - }; - }; - - it('should return without any error', () => { - const { externalTool, schoolExternalTool } = setup(); - - const result: ValidationError[] = service.validateParameters(externalTool, schoolExternalTool); - - expect(result).toHaveLength(0); - }); + expect(result).toHaveLength(1); }); }); }); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.spec.ts new file mode 100644 index 00000000000..cad2091ad4e --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-duplicate-key-validator.spec.ts @@ -0,0 +1,57 @@ +import { ValidationError } from '@shared/common'; +import { CustomParameterEntry, ToolParameterDuplicateLoggableException } from '../../../domain'; +import { ParameterArrayDuplicateKeyValidator } from './parameter-array-duplicate-key-validator'; + +describe(ParameterArrayDuplicateKeyValidator.name, () => { + describe('validate', () => { + describe('when there are no duplicate parameters', () => { + const setup = () => { + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'unique1', + }), + new CustomParameterEntry({ + name: 'unique2', + }), + ]; + + return { + entries, + }; + }; + + it('should return an empty array', () => { + const { entries } = setup(); + + const result: ValidationError[] = new ParameterArrayDuplicateKeyValidator().validate(entries, []); + + expect(result).toHaveLength(0); + }); + }); + + describe('when there are duplicate parameters', () => { + const setup = () => { + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'duplicate', + }), + new CustomParameterEntry({ + name: 'duplicate', + }), + ]; + + return { + entries, + }; + }; + + it('should return a validation error', () => { + const { entries } = setup(); + + const result: ValidationError[] = new ParameterArrayDuplicateKeyValidator().validate(entries, []); + + expect(result[0]).toBeInstanceOf(ToolParameterDuplicateLoggableException); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.spec.ts new file mode 100644 index 00000000000..f09acbc2e90 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-entry-validator.spec.ts @@ -0,0 +1,90 @@ +import { ValidationError } from '@shared/common'; +import { customParameterFactory } from '@shared/testing'; +import { + CustomParameter, + CustomParameterEntry, + ToolParameterRequiredLoggableException, + ToolParameterValueMissingLoggableException, +} from '../../../domain'; +import { ParameterArrayEntryValidator } from './parameter-array-entry-validator'; + +describe(ParameterArrayEntryValidator.name, () => { + describe('validate', () => { + describe('when all parameters are valid', () => { + const setup = () => { + const declarations: CustomParameter[] = customParameterFactory.buildList(1, { + name: 'param1', + }); + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'param1', + value: 'test', + }), + ]; + + return { + entries, + declarations, + }; + }; + + it('should return an empty array', () => { + const { entries, declarations } = setup(); + + const result: ValidationError[] = new ParameterArrayEntryValidator().validate(entries, declarations); + + expect(result).toHaveLength(0); + }); + }); + + describe('when a required parameter is not defined', () => { + const setup = () => { + const declarations: CustomParameter[] = customParameterFactory.buildList(1, { + name: 'param1', + isOptional: false, + }); + const entries: CustomParameterEntry[] = []; + + return { + declarations, + entries, + }; + }; + + it('should return a validation error', () => { + const { entries, declarations } = setup(); + + const result: ValidationError[] = new ParameterArrayEntryValidator().validate(entries, declarations); + + expect(result[0]).toBeInstanceOf(ToolParameterRequiredLoggableException); + }); + }); + + describe('when a required parameter fails the validations', () => { + const setup = () => { + const declarations: CustomParameter[] = customParameterFactory.buildList(1, { + name: 'param1', + isOptional: false, + }); + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'param1', + }), + ]; + + return { + declarations, + entries, + }; + }; + + it('should return a validation error', () => { + const { entries, declarations } = setup(); + + const result: ValidationError[] = new ParameterArrayEntryValidator().validate(entries, declarations); + + expect(result[0]).toBeInstanceOf(ToolParameterValueMissingLoggableException); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.spec.ts new file mode 100644 index 00000000000..c73e6430406 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-array-unknown-key-validator.spec.ts @@ -0,0 +1,60 @@ +import { ValidationError } from '@shared/common'; +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter, CustomParameterEntry, ToolParameterUnknownLoggableException } from '../../../domain'; +import { ParameterArrayUnknownKeyValidator } from './parameter-array-unknown-key-validator'; + +describe(ParameterArrayUnknownKeyValidator.name, () => { + describe('validate', () => { + describe('when there are no unknown parameters', () => { + const setup = () => { + const declarations: CustomParameter[] = customParameterFactory.buildList(1, { + name: 'param1', + }); + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'param1', + }), + ]; + + return { + entries, + declarations, + }; + }; + + it('should return an empty array', () => { + const { entries, declarations } = setup(); + + const result: ValidationError[] = new ParameterArrayUnknownKeyValidator().validate(entries, declarations); + + expect(result).toHaveLength(0); + }); + }); + + describe('when there are unknown parameters', () => { + const setup = () => { + const declarations: CustomParameter[] = customParameterFactory.buildList(1, { + name: 'param1', + }); + const entries: CustomParameterEntry[] = [ + new CustomParameterEntry({ + name: 'unknownParameter', + }), + ]; + + return { + declarations, + entries, + }; + }; + + it('should return a validation error', () => { + const { entries, declarations } = setup(); + + const result: ValidationError[] = new ParameterArrayUnknownKeyValidator().validate(entries, declarations); + + expect(result[0]).toBeInstanceOf(ToolParameterUnknownLoggableException); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.spec.ts new file mode 100644 index 00000000000..9dc245fe8d6 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-regex-validator.spec.ts @@ -0,0 +1,60 @@ +import { ValidationError } from '@shared/common'; +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter, CustomParameterEntry, ToolParameterValueRegexLoggableException } from '../../../domain'; +import { ParameterEntryRegexValidator } from './parameter-entry-regex-validator'; + +describe(ParameterEntryRegexValidator.name, () => { + describe('validate', () => { + describe('when the parameter fulfills the regex', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + regex: '^123$', + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: '123', + }); + + return { + entry, + declaration, + }; + }; + + it('should return an empty array', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryRegexValidator().validate(entry, declaration); + + expect(result).toHaveLength(0); + }); + }); + + describe('when the parameter does not fulfills the regex', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + regex: '^123$', + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: '456', + }); + + return { + entry, + declaration, + }; + }; + + it('should return a validation error', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryRegexValidator().validate(entry, declaration); + + expect(result[0]).toBeInstanceOf(ToolParameterValueRegexLoggableException); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.spec.ts new file mode 100644 index 00000000000..485d89a5354 --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-type-validator.spec.ts @@ -0,0 +1,61 @@ +import { ValidationError } from '@shared/common'; +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter, CustomParameterEntry, ToolParameterTypeMismatchLoggableException } from '../../../domain'; +import { CustomParameterType } from '../../../enum'; +import { ParameterEntryTypeValidator } from './parameter-entry-type-validator'; + +describe(ParameterEntryTypeValidator.name, () => { + describe('validate', () => { + describe('when the parameter has the correct type', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + type: CustomParameterType.NUMBER, + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: '123', + }); + + return { + entry, + declaration, + }; + }; + + it('should return an empty array', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryTypeValidator().validate(entry, declaration); + + expect(result).toHaveLength(0); + }); + }); + + describe('when the parameter does not have the correct type', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + type: CustomParameterType.NUMBER, + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: 'NaN', + }); + + return { + entry, + declaration, + }; + }; + + it('should return a validation error', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryTypeValidator().validate(entry, declaration); + + expect(result[0]).toBeInstanceOf(ToolParameterTypeMismatchLoggableException); + }); + }); + }); +}); diff --git a/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.spec.ts b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.spec.ts new file mode 100644 index 00000000000..449f688e93b --- /dev/null +++ b/apps/server/src/modules/tool/common/service/validation/rules/parameter-entry-value-validator.spec.ts @@ -0,0 +1,82 @@ +import { ValidationError } from '@shared/common'; +import { customParameterFactory } from '@shared/testing'; +import { CustomParameter, CustomParameterEntry, ToolParameterValueMissingLoggableException } from '../../../domain'; +import { ParameterEntryValueValidator } from './parameter-entry-value-validator'; + +describe(ParameterEntryValueValidator.name, () => { + describe('validate', () => { + describe('when the parameter has a value', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: '123', + }); + + return { + entry, + declaration, + }; + }; + + it('should return an empty array', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryValueValidator().validate(entry, declaration); + + expect(result).toHaveLength(0); + }); + }); + + describe('when the parameter value is an empty string', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + value: '', + }); + + return { + entry, + declaration, + }; + }; + + it('should return a validation error', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryValueValidator().validate(entry, declaration); + + expect(result[0]).toBeInstanceOf(ToolParameterValueMissingLoggableException); + }); + }); + + describe('when the parameter value is undefined', () => { + const setup = () => { + const declaration: CustomParameter = customParameterFactory.build({ + name: 'param1', + }); + const entry: CustomParameterEntry = new CustomParameterEntry({ + name: 'param1', + }); + + return { + entry, + declaration, + }; + }; + + it('should return a validation error', () => { + const { entry, declaration } = setup(); + + const result: ValidationError[] = new ParameterEntryValueValidator().validate(entry, declaration); + + expect(result[0]).toBeInstanceOf(ToolParameterValueMissingLoggableException); + }); + }); + }); +}); From d2c6fe0404a7dc55e4328f822092d42f13787b97 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 15:55:52 +0100 Subject: [PATCH 11/21] requested changes --- .../board-do-copy.service.ts | 2 +- .../recursive-copy.visitor.spec.ts | 2 +- .../recursive-copy.visitor.ts | 2 +- .../src/modules/learnroom/learnroom.module.ts | 2 +- .../service/course-copy.service.spec.ts | 93 +++++++++++++------ ...rnal-tool-configuration-status.response.ts | 2 +- .../service/tool-version-service.ts | 2 +- 7 files changed, 69 insertions(+), 36 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index 9a2854af82a..e58adbb5409 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -1,4 +1,4 @@ -import { ContextExternalToolService } from '@modules//tool/context-external-tool/service'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { CopyStatus } from '@modules/copy-helper'; import { Injectable } from '@nestjs/common'; import { AnyBoardDo } from '@shared/domain/domainobject'; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts index 6a134bc9d52..a1d8c889204 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts @@ -3,7 +3,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { LinkElement } from '@shared/domain/domainobject'; import { linkElementFactory, setupEntities } from '@shared/testing'; import { CopyFileDto } from '@src/modules/files-storage-client/dto'; -import { ContextExternalToolService } from '../../../tool/context-external-tool/service'; +import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyServiceFactory } from './school-specific-file-copy-service.factory'; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index cba1554ce17..4900f11d611 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -18,7 +18,7 @@ import { import { LinkElement } from '@shared/domain/domainobject/board/link-element.do'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; -import { ContextExternalTool } from '../../../tool/context-external-tool/domain'; +import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { diff --git a/apps/server/src/modules/learnroom/learnroom.module.ts b/apps/server/src/modules/learnroom/learnroom.module.ts index 39bea93eefe..432c85a4f83 100644 --- a/apps/server/src/modules/learnroom/learnroom.module.ts +++ b/apps/server/src/modules/learnroom/learnroom.module.ts @@ -13,6 +13,7 @@ import { UserRepo, } from '@shared/repo'; import { LoggerModule } from '@src/core/logger'; +import { ContextExternalToolModule } from '@modules/tool/context-external-tool'; import { BoardCopyService, ColumnBoardTargetService, @@ -23,7 +24,6 @@ import { DashboardService, RoomsService, } from './service'; -import { ContextExternalToolModule } from '../tool/context-external-tool'; @Module({ imports: [LessonModule, TaskModule, CopyHelperModule, BoardModule, LoggerModule, ContextExternalToolModule], diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index b9ea5428636..ea36970f099 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -128,6 +128,8 @@ describe('course copy service', () => { lessonCopyService.updateCopiedEmbeddedTasks.mockReturnValue(boardCopyStatus); + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + return { user, course, @@ -325,45 +327,76 @@ describe('course copy service', () => { expect(courseCopy.color).toEqual(course.color); }); - describe('when FEATURE_CTL_TOOLS_COPY_ENABLED is true', () => { - it('should find all ctl tools for this course', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); - const { course, user } = setup(); - await service.copyCourse({ userId: user.id, courseId: course.id }); + it('should find all ctl tools for this course', async () => { + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); - expect(contextExternalToolService.findAllByContext).toHaveBeenCalledWith({ - id: course.id, - type: ToolContextType.COURSE, - }); + expect(contextExternalToolService.findAllByContext).toHaveBeenCalledWith({ + id: course.id, + type: ToolContextType.COURSE, }); + }); - it('should copy all ctl tools', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); - const { course, user, tools } = setup(); - const status = await service.copyCourse({ userId: user.id, courseId: course.id }); - const courseCopy = status.copyEntity as Course; + it('should copy all ctl tools', async () => { + const { course, user, tools } = setup(); + const status = await service.copyCourse({ userId: user.id, courseId: course.id }); + const courseCopy = status.copyEntity as Course; - expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[0], courseCopy.id); - expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[1], courseCopy.id); - }); + expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[0], courseCopy.id); + expect(contextExternalToolService.copyContextExternalTool).toHaveBeenCalledWith(tools[1], courseCopy.id); }); + }); - describe('when FEATURE_CTL_TOOLS_COPY_ENABLED is false', () => { - it('should not find ctl tools', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); - const { course, user } = setup(); - await service.copyCourse({ userId: user.id, courseId: course.id }); + describe('when FEATURE_CTL_TOOLS_COPY_ENABLED is false', () => { + const setup = () => { + const user = userFactory.buildWithId(); + const allCourses = courseFactory.buildList(3, { teachers: [user] }); + const course = allCourses[0]; + const originalBoard = boardFactory.build({ course }); - expect(contextExternalToolService.findAllByContext).not.toHaveBeenCalled(); - }); + userRepo.findById.mockResolvedValue(user); + courseRepo.findById.mockResolvedValue(course); + courseRepo.findAllByUserId.mockResolvedValue([allCourses, allCourses.length]); + boardRepo.findByCourseId.mockResolvedValue(originalBoard); + roomsService.updateBoard.mockResolvedValue(originalBoard); - it('should not copy ctl tools', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); - const { course, user } = setup(); - await service.copyCourse({ userId: user.id, courseId: course.id }); + const courseCopyName = 'Copy'; + copyHelperService.deriveCopyName.mockReturnValue(courseCopyName); + copyHelperService.deriveStatusFromElements.mockReturnValue(CopyStatusEnum.SUCCESS); - expect(contextExternalToolService.copyContextExternalTool).not.toHaveBeenCalled(); - }); + const boardCopyStatus = { + title: 'boardCopy', + type: CopyElementType.BOARD, + status: CopyStatusEnum.SUCCESS, + copyEntity: boardFactory.build(), + elements: [], + }; + boardCopyService.copyBoard.mockResolvedValue(boardCopyStatus); + + lessonCopyService.updateCopiedEmbeddedTasks.mockReturnValue(boardCopyStatus); + + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + + return { + user, + course, + }; + }; + + it('should not find ctl tools', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); + + expect(contextExternalToolService.findAllByContext).not.toHaveBeenCalled(); + }); + + it('should not copy ctl tools', async () => { + Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + const { course, user } = setup(); + await service.copyCourse({ userId: user.id, courseId: course.id }); + + expect(contextExternalToolService.copyContextExternalTool).not.toHaveBeenCalled(); }); }); diff --git a/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts b/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts index 04ab2f9ed69..49beb8d6e5a 100644 --- a/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts +++ b/apps/server/src/modules/tool/common/controller/dto/context-external-tool-configuration-status.response.ts @@ -17,7 +17,7 @@ export class ContextExternalToolConfigurationStatusResponse { @ApiProperty({ type: Boolean, - description: 'true, if a configured parameter on the context external tool is missing a value', + description: 'True, if a configured parameter on the context external tool is missing a value', }) isIncompleteOnScopeContext: boolean; diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts index f24faa4ec99..f8411d1ee38 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts @@ -19,7 +19,7 @@ export class ToolVersionService { @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} - determineToolConfigurationStatus( + public determineToolConfigurationStatus( externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool From 7d769b2cb769fac330131a53360b87a19f9a3a8e Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 4 Jan 2024 16:55:31 +0100 Subject: [PATCH 12/21] requested changes - use tool features --- apps/server/src/modules/board/board.module.ts | 2 ++ .../board-do-copy-service/board-do-copy.service.ts | 14 +++++++++++--- .../recursive-copy.visitor.ts | 7 +++++-- .../src/modules/learnroom/learnroom.module.ts | 11 ++++++++++- .../learnroom/service/course-copy.service.ts | 10 ++++++---- .../service/context-external-tool.service.ts | 14 ++++++-------- apps/server/src/modules/tool/tool-config.ts | 2 ++ 7 files changed, 42 insertions(+), 18 deletions(-) diff --git a/apps/server/src/modules/board/board.module.ts b/apps/server/src/modules/board/board.module.ts index 1c66d759695..15c7b04409f 100644 --- a/apps/server/src/modules/board/board.module.ts +++ b/apps/server/src/modules/board/board.module.ts @@ -20,6 +20,7 @@ import { } from './service'; import { BoardDoCopyService, SchoolSpecificFileCopyServiceFactory } from './service/board-do-copy-service'; import { ColumnBoardCopyService } from './service/column-board-copy.service'; +import { ToolConfigModule } from '../tool/tool-config.module'; @Module({ imports: [ @@ -29,6 +30,7 @@ import { ColumnBoardCopyService } from './service/column-board-copy.service'; UserModule, ContextExternalToolModule, HttpModule, + ToolConfigModule, ], providers: [ BoardDoAuthorizableService, diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index e58adbb5409..1bfc5f33933 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -1,9 +1,10 @@ import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; import { CopyStatus } from '@modules/copy-helper'; -import { Injectable } from '@nestjs/common'; +import { Inject, Injectable } from '@nestjs/common'; import { AnyBoardDo } from '@shared/domain/domainobject'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; +import { IToolFeatures, ToolFeatures } from '../../../tool/tool-config'; export type BoardDoCopyParams = { original: AnyBoardDo; @@ -12,10 +13,17 @@ export type BoardDoCopyParams = { @Injectable() export class BoardDoCopyService { - constructor(private readonly contextExternalToolService: ContextExternalToolService) {} + constructor( + private readonly contextExternalToolService: ContextExternalToolService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + ) {} public async copy(params: BoardDoCopyParams): Promise { - const visitor = new RecursiveCopyVisitor(params.fileCopyService, this.contextExternalToolService); + const visitor = new RecursiveCopyVisitor( + params.fileCopyService, + this.contextExternalToolService, + this.toolFeatures + ); const result = await visitor.copy(params.original); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index 4900f11d611..37263ca0f80 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -19,7 +19,9 @@ import { LinkElement } from '@shared/domain/domainobject/board/link-element.do'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; +import { Inject } from '@nestjs/common'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; +import { IToolFeatures, ToolFeatures } from '../../../tool/tool-config'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { resultMap = new Map(); @@ -28,7 +30,8 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { constructor( private readonly fileCopyService: SchoolSpecificFileCopyService, - private readonly contextExternalToolService: ContextExternalToolService + private readonly contextExternalToolService: ContextExternalToolService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} async copy(original: AnyBoardDo): Promise { @@ -252,7 +255,7 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { updatedAt: new Date(), }); - if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED') && original.contextExternalToolId) { + if (this.toolFeatures.ctlToolsCopyEnabled && original.contextExternalToolId) { const tool: ContextExternalTool | null = await this.contextExternalToolService.findById( original.contextExternalToolId ); diff --git a/apps/server/src/modules/learnroom/learnroom.module.ts b/apps/server/src/modules/learnroom/learnroom.module.ts index 432c85a4f83..d6516fb7f84 100644 --- a/apps/server/src/modules/learnroom/learnroom.module.ts +++ b/apps/server/src/modules/learnroom/learnroom.module.ts @@ -24,9 +24,18 @@ import { DashboardService, RoomsService, } from './service'; +import { ToolConfigModule } from '../tool/tool-config.module'; @Module({ - imports: [LessonModule, TaskModule, CopyHelperModule, BoardModule, LoggerModule, ContextExternalToolModule], + imports: [ + LessonModule, + TaskModule, + CopyHelperModule, + BoardModule, + LoggerModule, + ContextExternalToolModule, + ToolConfigModule, + ], providers: [ { provide: 'DASHBOARD_REPO', diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index a9b51056e1f..b06b0c28147 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -3,12 +3,13 @@ import { CopyElementType, CopyHelperService, CopyStatus, CopyStatusEnum } from ' import { ToolContextType } from '@modules/tool/common/enum'; import { ContextExternalTool, ContextRef } from '@modules/tool/context-external-tool/domain'; import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; -import { Injectable } from '@nestjs/common'; +import { Inject, Injectable } from '@nestjs/common'; import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; import { BoardCopyService } from './board-copy.service'; import { RoomsService } from './rooms.service'; +import { IToolFeatures, ToolFeatures } from '../../tool/tool-config'; type CourseCopyParams = { originalCourse: Course; @@ -25,7 +26,8 @@ export class CourseCopyService { private readonly boardCopyService: BoardCopyService, private readonly copyHelperService: CopyHelperService, private readonly userRepo: UserRepo, - private readonly contextExternalToolService: ContextExternalToolService + private readonly contextExternalToolService: ContextExternalToolService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} async copyCourse({ @@ -51,7 +53,7 @@ export class CourseCopyService { // copy course and board const courseCopy = await this.copyCourseEntity({ user, originalCourse, copyName }); - if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { + if (this.toolFeatures.ctlToolsCopyEnabled) { const contextRef: ContextRef = { id: courseId, type: ToolContextType.COURSE }; const contextExternalToolsInContext: ContextExternalTool[] = await this.contextExternalToolService.findAllByContext(contextRef); @@ -119,7 +121,7 @@ export class CourseCopyService { boardStatus, ]; - if (Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) { + if (this.toolFeatures.ctlToolsCopyEnabled) { elements.push({ type: CopyElementType.EXTERNAL_TOOL, status: CopyStatusEnum.SUCCESS, diff --git a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts index 8998dfeab02..e8ffb1fdc3e 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/context-external-tool.service.ts @@ -90,14 +90,12 @@ export class ContextExternalToolService { ); const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - if (externalTool.parameters) { - externalTool.parameters.forEach((parameter: CustomParameter): CustomParameter => { - if (parameter.isProtected) { - this.deleteProtectedValues(tool, parameter.name); - } - return parameter; - }); - } + externalTool.parameters?.forEach((parameter: CustomParameter): void => { + if (parameter.isProtected) { + this.deleteProtectedValues(tool, parameter.name); + } + }); + const copiedTool = await this.contextExternalToolRepo.save(tool); return copiedTool; diff --git a/apps/server/src/modules/tool/tool-config.ts b/apps/server/src/modules/tool/tool-config.ts index 1405f0c7a1d..a7ee1f65d8a 100644 --- a/apps/server/src/modules/tool/tool-config.ts +++ b/apps/server/src/modules/tool/tool-config.ts @@ -10,6 +10,7 @@ export interface IToolFeatures { toolStatusWithoutVersions: boolean; maxExternalToolLogoSizeInBytes: number; backEndUrl: string; + ctlToolsCopyEnabled: boolean; } export default class ToolConfiguration { @@ -21,5 +22,6 @@ export default class ToolConfiguration { toolStatusWithoutVersions: Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED') as boolean, maxExternalToolLogoSizeInBytes: Configuration.get('CTL_TOOLS__EXTERNAL_TOOL_MAX_LOGO_SIZE_IN_BYTES') as number, backEndUrl: Configuration.get('PUBLIC_BACKEND_URL') as string, + ctlToolsCopyEnabled: Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED') as boolean, }; } From 2bd840e561b36c1f16d9f7f0adb45485c35a6a2e Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 09:33:18 +0100 Subject: [PATCH 13/21] requested changes - use tool features - unit tests --- .../board-do-copy-service/board-do-copy.service.ts | 2 +- .../recursive-copy.visitor.ts | 3 +-- .../learnroom/service/course-copy.service.spec.ts | 14 ++++++++++++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index 1bfc5f33933..92c207646a3 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -2,9 +2,9 @@ import { ContextExternalToolService } from '@modules/tool/context-external-tool/ import { CopyStatus } from '@modules/copy-helper'; import { Inject, Injectable } from '@nestjs/common'; import { AnyBoardDo } from '@shared/domain/domainobject'; +import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; -import { IToolFeatures, ToolFeatures } from '../../../tool/tool-config'; export type BoardDoCopyParams = { original: AnyBoardDo; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index 37263ca0f80..bf533c6abf6 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -1,4 +1,3 @@ -import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { FileRecordParentType } from '@infra/rabbitmq'; import { CopyElementType, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; @@ -20,8 +19,8 @@ import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; import { Inject } from '@nestjs/common'; +import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; -import { IToolFeatures, ToolFeatures } from '../../../tool/tool-config'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { resultMap = new Map(); diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index ea36970f099..8ef8ee4b0b5 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -17,6 +17,8 @@ import { setupEntities, userFactory, } from '@shared/testing'; +import { IToolFeatures } from '@src/modules/tool/tool-config'; +import { ToolFeatures } from '@modules/tool/tool-config'; import { BoardCopyService } from './board-copy.service'; import { CourseCopyService } from './course-copy.service'; import { RoomsService } from './rooms.service'; @@ -32,6 +34,7 @@ describe('course copy service', () => { let copyHelperService: DeepMocked; let userRepo: DeepMocked; let contextExternalToolService: DeepMocked; + let toolFeatures: IToolFeatures; afterAll(async () => { await module.close(); @@ -78,6 +81,12 @@ describe('course copy service', () => { provide: ContextExternalToolService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + ctlToolsTabEnabled: false, + }, + }, ], }).compile(); @@ -90,6 +99,7 @@ describe('course copy service', () => { copyHelperService = module.get(CopyHelperService); userRepo = module.get(UserRepo); contextExternalToolService = module.get(ContextExternalToolService); + toolFeatures = module.get(ToolFeatures); }); beforeEach(() => { @@ -128,7 +138,7 @@ describe('course copy service', () => { lessonCopyService.updateCopiedEmbeddedTasks.mockReturnValue(boardCopyStatus); - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + toolFeatures.ctlToolsCopyEnabled = true; return { user, @@ -375,7 +385,7 @@ describe('course copy service', () => { lessonCopyService.updateCopiedEmbeddedTasks.mockReturnValue(boardCopyStatus); - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); + toolFeatures.ctlToolsCopyEnabled = false; return { user, From 57bf1d1a912881e58d0dc66b7230a6a6d38925a2 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 09:54:27 +0100 Subject: [PATCH 14/21] fix tests --- .../board-do-copy.service.spec.ts | 14 +++++++++++--- .../recursive-copy.visitor.spec.ts | 17 +++++++++++++---- .../learnroom/service/course-copy.service.ts | 1 - 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts index f1e0702af55..28f39f33343 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts @@ -1,5 +1,4 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { FileRecordParentType } from '@infra/rabbitmq'; import { CopyElementType, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; @@ -40,6 +39,7 @@ import { submissionItemFactory, } from '@shared/testing'; import { ObjectId } from 'bson'; +import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { BoardDoCopyService } from './board-do-copy.service'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; @@ -48,6 +48,7 @@ describe('recursive board copy visitor', () => { let service: BoardDoCopyService; let contextExternalToolService: DeepMocked; + let toolFeatures: IToolFeatures; beforeAll(async () => { module = await Test.createTestingModule({ @@ -57,11 +58,18 @@ describe('recursive board copy visitor', () => { provide: ContextExternalToolService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + ctlToolsTabEnabled: false, + }, + }, ], }).compile(); service = module.get(BoardDoCopyService); contextExternalToolService = module.get(ContextExternalToolService); + toolFeatures = module.get(ToolFeatures); await setupEntities(); }); @@ -800,7 +808,7 @@ describe('recursive board copy visitor', () => { contextExternalToolService.findById.mockResolvedValueOnce(originalTool); contextExternalToolService.copyContextExternalTool.mockResolvedValueOnce(copiedTool); - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + toolFeatures.ctlToolsCopyEnabled = true; return { original, ...setupfileCopyService(), copiedTool }; }; @@ -864,7 +872,7 @@ describe('recursive board copy visitor', () => { contextExternalToolService.findById.mockResolvedValueOnce(null); - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', true); + toolFeatures.ctlToolsCopyEnabled = true; return { original, ...setupfileCopyService() }; }; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts index a1d8c889204..4b5ca521755 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts @@ -5,6 +5,7 @@ import { linkElementFactory, setupEntities } from '@shared/testing'; import { CopyFileDto } from '@src/modules/files-storage-client/dto'; import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; +import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyServiceFactory } from './school-specific-file-copy-service.factory'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; @@ -14,6 +15,7 @@ describe(RecursiveCopyVisitor.name, () => { let fileCopyServiceFactory: DeepMocked; let contextExternalToolService: DeepMocked; + let toolFeatures: IToolFeatures; beforeAll(async () => { module = await Test.createTestingModule({ @@ -27,11 +29,18 @@ describe(RecursiveCopyVisitor.name, () => { provide: ContextExternalToolService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + ctlToolsTabEnabled: false, + }, + }, ], }).compile(); fileCopyServiceFactory = module.get(SchoolSpecificFileCopyServiceFactory); contextExternalToolService = module.get(ContextExternalToolService); + toolFeatures = module.get(ToolFeatures); await setupEntities(); }); @@ -65,7 +74,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup(); const linkElement = linkElementFactory.build(); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); @@ -78,7 +87,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); @@ -91,7 +100,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl, newFileId } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; @@ -105,7 +114,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl: `https://abc.de/file/unknown-file-id` }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index b06b0c28147..1b29b0f518e 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -1,4 +1,3 @@ -import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { CopyElementType, CopyHelperService, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; import { ToolContextType } from '@modules/tool/common/enum'; import { ContextExternalTool, ContextRef } from '@modules/tool/context-external-tool/domain'; From ecea339f4df1f02c4d748cf5ba9398e16fd5f7df Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 11:35:54 +0100 Subject: [PATCH 15/21] change order of cunstructor injections --- .../board-do-copy.service.ts | 8 ++++---- .../recursive-copy.visitor.spec.ts | 20 +++++++++---------- .../recursive-copy.visitor.ts | 4 ++-- .../learnroom/service/course-copy.service.ts | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index 92c207646a3..80fb37d84f2 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -14,15 +14,15 @@ export type BoardDoCopyParams = { @Injectable() export class BoardDoCopyService { constructor( - private readonly contextExternalToolService: ContextExternalToolService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, + private readonly contextExternalToolService: ContextExternalToolService ) {} public async copy(params: BoardDoCopyParams): Promise { const visitor = new RecursiveCopyVisitor( + this.toolFeatures, params.fileCopyService, - this.contextExternalToolService, - this.toolFeatures + this.contextExternalToolService ); const result = await visitor.copy(params.original); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts index 4b5ca521755..05d2a01c74c 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts @@ -21,6 +21,12 @@ describe(RecursiveCopyVisitor.name, () => { module = await Test.createTestingModule({ providers: [ RecursiveCopyVisitor, + { + provide: ToolFeatures, + useValue: { + ctlToolsTabEnabled: false, + }, + }, { provide: SchoolSpecificFileCopyServiceFactory, useValue: createMock(), @@ -29,12 +35,6 @@ describe(RecursiveCopyVisitor.name, () => { provide: ContextExternalToolService, useValue: createMock(), }, - { - provide: ToolFeatures, - useValue: { - ctlToolsTabEnabled: false, - }, - }, ], }).compile(); @@ -74,7 +74,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup(); const linkElement = linkElementFactory.build(); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); + const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); @@ -87,7 +87,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); + const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); @@ -100,7 +100,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl, newFileId } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); + const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; @@ -114,7 +114,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl: `https://abc.de/file/unknown-file-id` }); - const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); + const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index bf533c6abf6..c5ee49e3353 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -28,9 +28,9 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { copyMap = new Map(); constructor( + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, private readonly fileCopyService: SchoolSpecificFileCopyService, - private readonly contextExternalToolService: ContextExternalToolService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + private readonly contextExternalToolService: ContextExternalToolService ) {} async copy(original: AnyBoardDo): Promise { diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index 1b29b0f518e..5a59492825a 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -19,14 +19,14 @@ type CourseCopyParams = { @Injectable() export class CourseCopyService { constructor( + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, private readonly courseRepo: CourseRepo, private readonly boardRepo: BoardRepo, private readonly roomsService: RoomsService, private readonly boardCopyService: BoardCopyService, private readonly copyHelperService: CopyHelperService, private readonly userRepo: UserRepo, - private readonly contextExternalToolService: ContextExternalToolService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + private readonly contextExternalToolService: ContextExternalToolService ) {} async copyCourse({ From 59efca63ffc08495e005ab8da4725e5027bdd8db Mon Sep 17 00:00:00 2001 From: Arne Gnisa Date: Fri, 5 Jan 2024 11:53:34 +0100 Subject: [PATCH 16/21] N21-1507 fixes injection --- .../board-do-copy.service.spec.ts | 3 +-- .../recursive-copy.visitor.spec.ts | 22 +++++++++---------- .../recursive-copy.visitor.ts | 9 ++++---- .../service/external-tool.service.ts | 1 + 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts index 28f39f33343..50764cd0e50 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts @@ -61,7 +61,7 @@ describe('recursive board copy visitor', () => { { provide: ToolFeatures, useValue: { - ctlToolsTabEnabled: false, + ctlToolsCopyEnabled: true, }, }, ], @@ -69,7 +69,6 @@ describe('recursive board copy visitor', () => { service = module.get(BoardDoCopyService); contextExternalToolService = module.get(ContextExternalToolService); - toolFeatures = module.get(ToolFeatures); await setupEntities(); }); diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts index 05d2a01c74c..6cbea74cda3 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.spec.ts @@ -4,7 +4,6 @@ import { LinkElement } from '@shared/domain/domainobject'; import { linkElementFactory, setupEntities } from '@shared/testing'; import { CopyFileDto } from '@src/modules/files-storage-client/dto'; import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; - import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { RecursiveCopyVisitor } from './recursive-copy.visitor'; import { SchoolSpecificFileCopyServiceFactory } from './school-specific-file-copy-service.factory'; @@ -15,18 +14,13 @@ describe(RecursiveCopyVisitor.name, () => { let fileCopyServiceFactory: DeepMocked; let contextExternalToolService: DeepMocked; + let toolFeatures: IToolFeatures; beforeAll(async () => { module = await Test.createTestingModule({ providers: [ RecursiveCopyVisitor, - { - provide: ToolFeatures, - useValue: { - ctlToolsTabEnabled: false, - }, - }, { provide: SchoolSpecificFileCopyServiceFactory, useValue: createMock(), @@ -35,6 +29,12 @@ describe(RecursiveCopyVisitor.name, () => { provide: ContextExternalToolService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + ctlToolsCopyEnabled: true, + }, + }, ], }).compile(); @@ -74,7 +74,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup(); const linkElement = linkElementFactory.build(); - const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); @@ -87,7 +87,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); @@ -100,7 +100,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock, imageUrl, newFileId } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl }); - const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; @@ -114,7 +114,7 @@ describe(RecursiveCopyVisitor.name, () => { const { fileCopyServiceMock } = setup({ withFileCopy: true }); const linkElement = linkElementFactory.build({ imageUrl: `https://abc.de/file/unknown-file-id` }); - const visitor = new RecursiveCopyVisitor(toolFeatures, fileCopyServiceMock, contextExternalToolService); + const visitor = new RecursiveCopyVisitor(fileCopyServiceMock, contextExternalToolService, toolFeatures); await visitor.visitLinkElementAsync(linkElement); const copy = visitor.copyMap.get(linkElement.id) as LinkElement; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts index c5ee49e3353..b2a4e6a652d 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/recursive-copy.visitor.ts @@ -1,6 +1,8 @@ import { FileRecordParentType } from '@infra/rabbitmq'; import { CopyElementType, CopyStatus, CopyStatusEnum } from '@modules/copy-helper'; +import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; import { ContextExternalToolService } from '@modules/tool/context-external-tool/service'; +import { IToolFeatures } from '@modules/tool/tool-config'; import { AnyBoardDo, BoardCompositeVisitorAsync, @@ -17,9 +19,6 @@ import { import { LinkElement } from '@shared/domain/domainobject/board/link-element.do'; import { EntityId } from '@shared/domain/types'; import { ObjectId } from 'bson'; -import { ContextExternalTool } from '@modules/tool/context-external-tool/domain'; -import { Inject } from '@nestjs/common'; -import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { @@ -28,9 +27,9 @@ export class RecursiveCopyVisitor implements BoardCompositeVisitorAsync { copyMap = new Map(); constructor( - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, private readonly fileCopyService: SchoolSpecificFileCopyService, - private readonly contextExternalToolService: ContextExternalToolService + private readonly contextExternalToolService: ContextExternalToolService, + private readonly toolFeatures: IToolFeatures ) {} async copy(original: AnyBoardDo): Promise { diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts index 67f7edd161b..a12c59ce406 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts @@ -7,6 +7,7 @@ import { IFindOptions } from '@shared/domain/interface'; import { EntityId } from '@shared/domain/types'; import { ContextExternalToolRepo, ExternalToolRepo, SchoolExternalToolRepo } from '@shared/repo'; import { LegacyLogger } from '@src/core/logger'; +import { isDefined } from 'class-validator'; import { TokenEndpointAuthMethod } from '../../common/enum'; import { ExternalToolSearchQuery } from '../../common/interface'; import { SchoolExternalTool } from '../../school-external-tool/domain'; From 7245bf1b9695c6a7fa4494068331a1a86a567f2c Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 12:11:56 +0100 Subject: [PATCH 17/21] fix tests --- .../board-do-copy-service/board-do-copy.service.spec.ts | 7 +------ .../service/board-do-copy-service/board-do-copy.service.ts | 4 ++-- .../tool/external-tool/service/external-tool.service.ts | 1 - 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts index 50764cd0e50..b41e7d3e811 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.spec.ts @@ -39,7 +39,7 @@ import { submissionItemFactory, } from '@shared/testing'; import { ObjectId } from 'bson'; -import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; +import { ToolFeatures } from '@modules/tool/tool-config'; import { BoardDoCopyService } from './board-do-copy.service'; import { SchoolSpecificFileCopyService } from './school-specific-file-copy.interface'; @@ -48,7 +48,6 @@ describe('recursive board copy visitor', () => { let service: BoardDoCopyService; let contextExternalToolService: DeepMocked; - let toolFeatures: IToolFeatures; beforeAll(async () => { module = await Test.createTestingModule({ @@ -807,8 +806,6 @@ describe('recursive board copy visitor', () => { contextExternalToolService.findById.mockResolvedValueOnce(originalTool); contextExternalToolService.copyContextExternalTool.mockResolvedValueOnce(copiedTool); - toolFeatures.ctlToolsCopyEnabled = true; - return { original, ...setupfileCopyService(), copiedTool }; }; @@ -871,8 +868,6 @@ describe('recursive board copy visitor', () => { contextExternalToolService.findById.mockResolvedValueOnce(null); - toolFeatures.ctlToolsCopyEnabled = true; - return { original, ...setupfileCopyService() }; }; diff --git a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts index 80fb37d84f2..f981653ec37 100644 --- a/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts +++ b/apps/server/src/modules/board/service/board-do-copy-service/board-do-copy.service.ts @@ -20,9 +20,9 @@ export class BoardDoCopyService { public async copy(params: BoardDoCopyParams): Promise { const visitor = new RecursiveCopyVisitor( - this.toolFeatures, params.fileCopyService, - this.contextExternalToolService + this.contextExternalToolService, + this.toolFeatures ); const result = await visitor.copy(params.original); diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts index a12c59ce406..67f7edd161b 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool.service.ts @@ -7,7 +7,6 @@ import { IFindOptions } from '@shared/domain/interface'; import { EntityId } from '@shared/domain/types'; import { ContextExternalToolRepo, ExternalToolRepo, SchoolExternalToolRepo } from '@shared/repo'; import { LegacyLogger } from '@src/core/logger'; -import { isDefined } from 'class-validator'; import { TokenEndpointAuthMethod } from '../../common/enum'; import { ExternalToolSearchQuery } from '../../common/interface'; import { SchoolExternalTool } from '../../school-external-tool/domain'; From 7057498612adc446801b6cf80fdc814bed97f027 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 14:57:49 +0100 Subject: [PATCH 18/21] fix tests --- .../controller/api-test/tool-context.api.spec.ts | 4 ++-- .../controller/api-test/tool-school.api.spec.ts | 4 ++-- .../tool/tool-launch/service/tool-launch.service.spec.ts | 9 ++++++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/server/src/modules/tool/context-external-tool/controller/api-test/tool-context.api.spec.ts b/apps/server/src/modules/tool/context-external-tool/controller/api-test/tool-context.api.spec.ts index 6a645cc944d..2561db5489a 100644 --- a/apps/server/src/modules/tool/context-external-tool/controller/api-test/tool-context.api.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/controller/api-test/tool-context.api.spec.ts @@ -98,7 +98,7 @@ describe('ToolContextController (API)', () => { contextType: ToolContextType.COURSE, parameters: [ { name: 'param1', value: 'value' }, - { name: 'param2', value: '' }, + { name: 'param2', value: 'true' }, ], toolVersion: 1, }; @@ -128,7 +128,7 @@ describe('ToolContextController (API)', () => { contextType: postParams.contextType, parameters: [ { name: 'param1', value: 'value' }, - { name: 'param2', value: undefined }, + { name: 'param2', value: 'true' }, ], toolVersion: postParams.toolVersion, }); diff --git a/apps/server/src/modules/tool/school-external-tool/controller/api-test/tool-school.api.spec.ts b/apps/server/src/modules/tool/school-external-tool/controller/api-test/tool-school.api.spec.ts index 8f7f13c1d13..ebd44a94268 100644 --- a/apps/server/src/modules/tool/school-external-tool/controller/api-test/tool-school.api.spec.ts +++ b/apps/server/src/modules/tool/school-external-tool/controller/api-test/tool-school.api.spec.ts @@ -94,7 +94,7 @@ describe('ToolSchoolController (API)', () => { version: 1, parameters: [ { name: 'param1', value: 'value' }, - { name: 'param2', value: '' }, + { name: 'param2', value: 'false' }, ], }; @@ -147,7 +147,7 @@ describe('ToolSchoolController (API)', () => { toolVersion: postParams.version, parameters: [ { name: 'param1', value: 'value' }, - { name: 'param2', value: undefined }, + { name: 'param2', value: 'false' }, ], }); diff --git a/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.spec.ts b/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.spec.ts index 8bfae71cce1..82e03325ae9 100644 --- a/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.spec.ts +++ b/apps/server/src/modules/tool/tool-launch/service/tool-launch.service.spec.ts @@ -108,10 +108,11 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce( + toolVersionService.determineToolConfigurationStatus.mockReturnValueOnce( toolConfigurationStatusFactory.build({ isOutdatedOnScopeContext: false, isOutdatedOnScopeSchool: false, + isIncompleteOnScopeContext: false, }) ); @@ -179,10 +180,11 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); - toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce( + toolVersionService.determineToolConfigurationStatus.mockReturnValueOnce( toolConfigurationStatusFactory.build({ isOutdatedOnScopeContext: false, isOutdatedOnScopeSchool: false, + isIncompleteOnScopeContext: false, }) ); @@ -229,10 +231,11 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce( + toolVersionService.determineToolConfigurationStatus.mockReturnValueOnce( toolConfigurationStatusFactory.build({ isOutdatedOnScopeContext: true, isOutdatedOnScopeSchool: true, + isIncompleteOnScopeContext: false, }) ); From b94cb3f2b7a0681190ff3646d5b509bf370c95c2 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Fri, 5 Jan 2024 17:03:48 +0100 Subject: [PATCH 19/21] coverage up --- ...ool-parameter-type-validation.util.spec.ts | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts index 6ed1d14c07a..4949a840f11 100644 --- a/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts +++ b/apps/server/src/modules/tool/common/service/validation/tool-parameter-type-validation.util.spec.ts @@ -52,7 +52,7 @@ describe(ToolParameterTypeValidationUtil.name, () => { }); }); - describe('when the type is an auto type', () => { + describe('when the type is AUTO_CONTEXTNAME', () => { it('should return false', () => { const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( CustomParameterType.AUTO_CONTEXTNAME, @@ -62,5 +62,38 @@ describe(ToolParameterTypeValidationUtil.name, () => { expect(result).toEqual(false); }); }); + + describe('when the type is AUTO_CONTEXTID', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.AUTO_CONTEXTID, + 'any value' + ); + + expect(result).toEqual(false); + }); + }); + + describe('when the type is AUTO_SCHOOLID', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.AUTO_SCHOOLID, + 'any value' + ); + + expect(result).toEqual(false); + }); + }); + + describe('when the type is AUTO_SCHOOLNUMBER', () => { + it('should return false', () => { + const result: boolean = ToolParameterTypeValidationUtil.isValueValidForType( + CustomParameterType.AUTO_SCHOOLNUMBER, + 'any value' + ); + + expect(result).toEqual(false); + }); + }); }); }); From 33aae67a4ba3bc12cbd6fe45bb97569962aeeab2 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Mon, 8 Jan 2024 09:42:04 +0100 Subject: [PATCH 20/21] requested changes --- apps/server/src/modules/board/board.module.ts | 2 +- .../src/modules/learnroom/service/course-copy.service.spec.ts | 2 -- .../server/src/modules/learnroom/service/course-copy.service.ts | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/board/board.module.ts b/apps/server/src/modules/board/board.module.ts index 15c7b04409f..514d6fc1f19 100644 --- a/apps/server/src/modules/board/board.module.ts +++ b/apps/server/src/modules/board/board.module.ts @@ -8,6 +8,7 @@ import { CourseRepo } from '@shared/repo'; import { LoggerModule } from '@src/core/logger'; import { DrawingElementAdapterService } from '@modules/tldraw-client/service/drawing-element-adapter.service'; import { HttpModule } from '@nestjs/axios'; +import { ToolConfigModule } from '@modules/tool/tool-config.module'; import { BoardDoRepo, BoardNodeRepo, RecursiveDeleteVisitor } from './repo'; import { BoardDoAuthorizableService, @@ -20,7 +21,6 @@ import { } from './service'; import { BoardDoCopyService, SchoolSpecificFileCopyServiceFactory } from './service/board-do-copy-service'; import { ColumnBoardCopyService } from './service/column-board-copy.service'; -import { ToolConfigModule } from '../tool/tool-config.module'; @Module({ imports: [ diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index 8ef8ee4b0b5..562688cbbb2 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -394,7 +394,6 @@ describe('course copy service', () => { }; it('should not find ctl tools', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); const { course, user } = setup(); await service.copyCourse({ userId: user.id, courseId: course.id }); @@ -402,7 +401,6 @@ describe('course copy service', () => { }); it('should not copy ctl tools', async () => { - Configuration.set('FEATURE_CTL_TOOLS_COPY_ENABLED', false); const { course, user } = setup(); await service.copyCourse({ userId: user.id, courseId: course.id }); diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.ts b/apps/server/src/modules/learnroom/service/course-copy.service.ts index 5a59492825a..15b67e8fbd6 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.ts @@ -6,9 +6,9 @@ import { Inject, Injectable } from '@nestjs/common'; import { Course, User } from '@shared/domain/entity'; import { EntityId } from '@shared/domain/types'; import { BoardRepo, CourseRepo, UserRepo } from '@shared/repo'; +import { IToolFeatures, ToolFeatures } from '@modules/tool/tool-config'; import { BoardCopyService } from './board-copy.service'; import { RoomsService } from './rooms.service'; -import { IToolFeatures, ToolFeatures } from '../../tool/tool-config'; type CourseCopyParams = { originalCourse: Course; From fff685c14a9535d3d10581d1e44f12b15200d908 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Mon, 8 Jan 2024 10:17:17 +0100 Subject: [PATCH 21/21] fix import --- .../src/modules/learnroom/service/course-copy.service.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts index 562688cbbb2..5a5dab7c5ac 100644 --- a/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts +++ b/apps/server/src/modules/learnroom/service/course-copy.service.spec.ts @@ -1,5 +1,4 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { CopyElementType, CopyHelperService, CopyStatusEnum } from '@modules/copy-helper'; import { LessonCopyService } from '@modules/lesson/service'; import { ToolContextType } from '@modules/tool/common/enum';