Review comments 2024-12-16

hpi-schul-cloud · Dec 16, 2024 · f73ba1d · f73ba1d
1 parent c8be3a7
commit f73ba1d
Show file tree

Hide file tree

Showing 8 changed files with 44 additions and 22 deletions.
diff --git a/apps/server/src/modules/board/service/internal/board-node-copy-context.ts b/apps/server/src/modules/board/service/internal/board-node-copy-context.ts
@@ -19,6 +19,7 @@ export type BoardNodeCopyContextProps = {
 };
 
 export class BoardNodeCopyContext implements CopyContext {
+	// Remove and add public getter Method
 	readonly targetSchoolId: EntityId;
 
 	constructor(private readonly props: BoardNodeCopyContextProps) {

diff --git a/apps/server/src/modules/board/service/internal/board-node-copy-specific.service.spec.ts b/apps/server/src/modules/board/service/internal/board-node-copy-specific.service.spec.ts
@@ -451,7 +451,7 @@ describe(BoardNodeCopyService.name, () => {
 							result.copyEntity?.id,
 							copyContext.targetSchoolId
 						);
-						expect(result.copyEntity instanceof ExternalToolElement).toEqual(true);
+						expect(result.copyEntity).toBeInstanceOf(ExternalToolElement);
 						expect((result.copyEntity as ExternalToolElement).contextExternalToolId).toEqual(copiedTool.id);
 						expect(result.type).toEqual(CopyElementType.EXTERNAL_TOOL_ELEMENT);
 						expect(result.status).toEqual(CopyStatusEnum.SUCCESS);

diff --git a/apps/server/src/modules/board/service/internal/board-node-copy.service.ts b/apps/server/src/modules/board/service/internal/board-node-copy.service.ts
@@ -285,14 +285,15 @@ export class BoardNodeCopyService {
 		return Promise.resolve(result);
 	}
 
-	async copyExternalToolElement(original: ExternalToolElement, context: CopyContext): Promise<CopyStatus> {
+	public async copyExternalToolElement(original: ExternalToolElement, context: CopyContext): Promise<CopyStatus> {
 		let copy: ExternalToolElement | DeletedElement;
+		// ExternalToolElementFactory
 		copy = new ExternalToolElement({
 			...original.getProps(),
 			...this.buildSpecificProps([]),
 		});
 
-		if (!this.configService.get('FEATURE_CTL_TOOLS_COPY_ENABLED') || !original.contextExternalToolId) {
+		if (!this.configService.get('FEATURE_CTL_TOOLS_COPY_ENABLED', { infer: true }) || !original.contextExternalToolId) {
 			const copyStatus: CopyStatus = {
 				copyEntity: copy,
 				type: CopyElementType.EXTERNAL_TOOL_ELEMENT,
@@ -318,6 +319,7 @@ export class BoardNodeCopyService {
 
 		let copyStatus: CopyStatusEnum = CopyStatusEnum.SUCCESS;
 		if (contextToolCopyResult instanceof CopyContextExternalToolRejectData) {
+			// Naming DeletedContentElement and please add a factory for it.
 			copy = new DeletedElement({
 				id: new ObjectId().toHexString(),
 				path: copy.path,
@@ -335,6 +337,7 @@ export class BoardNodeCopyService {
 			copy.contextExternalToolId = contextToolCopyResult.id;
 		}
 
+		// factory ?
 		const result: CopyStatus = {
 			copyEntity: copy,
 			type: CopyElementType.EXTERNAL_TOOL_ELEMENT,

diff --git a/apps/server/src/modules/board/service/internal/column-board-copy.service.ts b/apps/server/src/modules/board/service/internal/column-board-copy.service.ts
@@ -27,7 +27,7 @@ export class ColumnBoardCopyService {
 		private readonly filesStorageClientAdapterService: FilesStorageClientAdapterService
 	) {}
 
-	async copyColumnBoard(params: CopyColumnBoardParams): Promise<CopyStatus> {
+	public async copyColumnBoard(params: CopyColumnBoardParams): Promise<CopyStatus> {
 		const originalBoard = await this.boardNodeService.findByClassAndId(ColumnBoard, params.originalColumnBoardId);
 
 		this.checkSupportedExternalReferenceType(params.targetExternalReference.type);

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
@@ -22,6 +22,7 @@ import {
 import {
 	contextExternalToolFactory,
 	copyContextExternalToolRejectDataFactory,
+	// import @modules/tool
 } from '../../tool/context-external-tool/testing';
 import { BoardCopyService } from './board-copy.service';
 import { CourseCopyService } from './course-copy.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
@@ -34,7 +34,7 @@ export class CourseCopyService {
 		private readonly contextExternalToolService: ContextExternalToolService
 	) {}
 
-	async copyCourse({
+	public async copyCourse({
 		userId,
 		courseId,
 		newName,
@@ -59,23 +59,8 @@ export class CourseCopyService {
 		const courseCopy = await this.copyCourseEntity({ user, originalCourse, copyName });
 
 		let courseToolsCopyStatus: CopyStatus | null = null;
-		if (this.configService.get('FEATURE_CTL_TOOLS_COPY_ENABLED')) {
-			const contextRef: ContextRef = { id: courseId, type: ToolContextType.COURSE };
-			const contextExternalToolsInContext: ContextExternalTool[] =
-				await this.contextExternalToolService.findAllByContext(contextRef);
-
-			const copyCourseToolsResult = await Promise.all(
-				contextExternalToolsInContext.map(
-					async (tool: ContextExternalTool): Promise<ContextExternalTool | CopyContextExternalToolRejectData> => {
-						const copiedResult: ContextExternalTool | CopyContextExternalToolRejectData =
-							await this.contextExternalToolService.copyContextExternalTool(tool, courseCopy.id, user.school.id);
-
-						return copiedResult;
-					}
-				)
-			);
-
-			courseToolsCopyStatus = this.deriveCourseToolCopyStatus(copyCourseToolsResult);
+		if (this.configService.get('FEATURE_CTL_TOOLS_COPY_ENABLED', { infer: true })) {
+			const courseToolsCopyStatus = await this.yyy(courseId);
 		}
 
 		const boardStatus = await this.boardCopyService.copyBoard({
@@ -96,6 +81,32 @@ export class CourseCopyService {
 		return courseStatus;
 	}
 
+	private async yyy(courseId: string): Promise<CopyStatus | null> {
+		const contextRef: ContextRef = { id: courseId, type: ToolContextType.COURSE };
+		const contextExternalToolsInContext = await this.contextExternalToolService.findAllByContext(contextRef);
+
+		const promises = contextExternalToolsInContext.map(this.xxx(tool, user, courseCopy.id));
+		const copyCourseToolsResult = await Promise.all(promises);
+
+		courseToolsCopyStatus = this.deriveCourseToolCopyStatus(copyCourseToolsResult);
+
+		return courseToolsCopyStatus;
+	}
+
+	private async xxx(
+		tool: ContextExternalTool,
+		user: User,
+		courseCopyId: string
+	): Promise<ContextExternalTool | CopyContextExternalToolRejectData> {
+		const copiedResult = await this.contextExternalToolService.copyContextExternalTool(
+			tool,
+			courseCopyId,
+			user.school.id
+		);
+
+		return copiedResult;
+	}
+
 	private async copyCourseEntity(params: CourseCopyParams): Promise<Course> {
 		const { originalCourse, user, copyName } = params;
 		const courseCopy = new Course({

diff --git a/...r/src/modules/tool/context-external-tool/domain/copy-context-external-tool-reject-data.ts b/...r/src/modules/tool/context-external-tool/domain/copy-context-external-tool-reject-data.ts
@@ -1,5 +1,7 @@
 import { EntityId } from '@shared/domain/types';
 
+// Reject as naming of a data reprasentation can be missunderstand. Please rename.
+// Maybe this class is not needed anymore if the code is re-sorted.
 export class CopyContextExternalToolRejectData {
 	readonly sourceContextExternalToolId: EntityId;
 

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
@@ -84,6 +84,7 @@ export class ContextExternalToolService {
 		contextExternalTool: ContextExternalTool,
 		contextCopyId: EntityId,
 		targetSchoolId: EntityId
+		// copy execution method do the copy and return a non copy -> is this always a ContextExternalTool with only a flag?
 	): Promise<ContextExternalTool | CopyContextExternalToolRejectData> {
 		const copy = new ContextExternalTool({
 			...contextExternalTool.getProps(),
@@ -112,7 +113,9 @@ export class ContextExternalToolService {
 			}
 		});
 
+		// private method this.isNotSameSchool())
 		if (schoolExternalTool.schoolId !== targetSchoolId) {
+			// private method this.handleLicenceInTargetSchool()
 			const correctSchoolExternalTools: SchoolExternalTool[] =
 				await this.schoolExternalToolService.findSchoolExternalTools({
 					toolId: schoolExternalTool.toolId,
@@ -123,6 +126,7 @@ export class ContextExternalToolService {
 				copy.schoolToolRef.schoolToolId = correctSchoolExternalTools[0].id;
 				copy.schoolToolRef.schoolId = correctSchoolExternalTools[0].schoolId;
 			} else {
+				// remove this and add key to ContextExternalTool
 				const copyRejectData = new CopyContextExternalToolRejectData(contextExternalTool.id, externalTool.name);
 
 				return copyRejectData;