From 562c7f09cc7452c9e6294a48291345b16733b146 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 8 Nov 2023 12:27:13 +0100 Subject: [PATCH 01/10] coompute tool status via paramter validation and feature flag --- .../service/tool-reference.service.ts | 36 +++++++++++++++---- ...school-external-tool-validation.service.ts | 6 ++-- .../service/tool-launch.service.ts | 15 ++++++-- config/default.schema.json | 5 +++ 4 files changed, 50 insertions(+), 12 deletions(-) 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 02c6a08677e..f53ac754546 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,14 +1,16 @@ import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; +import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService } from '../../school-external-tool/service'; +import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; import { ContextExternalTool, ToolReference } from '../domain'; import { ToolReferenceMapper } from '../mapper'; import { ContextExternalToolService } from './context-external-tool.service'; +import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; @Injectable() export class ToolReferenceService { @@ -17,7 +19,9 @@ export class ToolReferenceService { private readonly schoolExternalToolService: SchoolExternalToolService, private readonly contextExternalToolService: ContextExternalToolService, private readonly commonToolService: CommonToolService, - private readonly externalToolLogoService: ExternalToolLogoService + private readonly externalToolLogoService: ExternalToolLogoService, + private readonly contextExternalToolValidationService: ContextExternalToolValidationService, + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService ) {} async getToolReference(contextExternalToolId: EntityId): Promise { @@ -29,11 +33,16 @@ export class ToolReferenceService { ); const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - const status: ToolConfigurationStatus = this.commonToolService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); + let status: ToolConfigurationStatus; + if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + status = await this.determineToolConfigurationStatus(schoolExternalTool, contextExternalTool); + } else { + status = this.commonToolService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); + } const toolReference: ToolReference = ToolReferenceMapper.mapToToolReference( externalTool, @@ -47,4 +56,17 @@ export class ToolReferenceService { return toolReference; } + + private async determineToolConfigurationStatus( + schoolExternalTool: SchoolExternalTool, + contextExternalTool: ContextExternalTool + ): Promise { + try { + await this.schoolExternalToolValidationService.validate(schoolExternalTool); + await this.contextExternalToolValidationService.validate(contextExternalTool); + return ToolConfigurationStatus.LATEST; + } catch (err) { + return ToolConfigurationStatus.OUTDATED; + } + } } 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 315d738ca64..c951387de19 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 @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { ValidationError } from '@shared/common'; +import { Configuration } from '@hpi-schul-cloud/commons'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; @@ -17,8 +18,9 @@ export class SchoolExternalToolValidationService { const loadedExternalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - this.checkVersionMatch(schoolExternalTool.toolVersion, loadedExternalTool.version); - + if (!Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + this.checkVersionMatch(schoolExternalTool.toolVersion, loadedExternalTool.version); + } this.commonToolValidationService.checkCustomParameterEntries(loadedExternalTool, schoolExternalTool); } 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 abb5598796f..a8d1488db9f 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 @@ -1,12 +1,13 @@ import { Injectable, InternalServerErrorException } from '@nestjs/common'; import { EntityId } from '@shared/domain'; +import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService } from '../../school-external-tool/service'; +import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; import { ToolStatusOutdatedLoggableException } from '../error'; import { ToolLaunchMapper } from '../mapper'; import { ToolLaunchData, ToolLaunchRequest } from '../types'; @@ -16,6 +17,7 @@ import { Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrategy, } from './launch-strategy'; +import { ContextExternalToolValidationService } from '../../context-external-tool/service'; @Injectable() export class ToolLaunchService { @@ -27,7 +29,9 @@ export class ToolLaunchService { private readonly basicToolLaunchStrategy: BasicToolLaunchStrategy, private readonly lti11ToolLaunchStrategy: Lti11ToolLaunchStrategy, private readonly oauth2ToolLaunchStrategy: OAuth2ToolLaunchStrategy, - private readonly commonToolService: CommonToolService + private readonly commonToolService: CommonToolService, + private readonly contextExternalToolValidationService: ContextExternalToolValidationService, + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService ) { this.strategies = new Map(); this.strategies.set(ToolConfigType.BASIC, basicToolLaunchStrategy); @@ -53,7 +57,12 @@ export class ToolLaunchService { const { externalTool, schoolExternalTool } = await this.loadToolHierarchy(schoolExternalToolId); - this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); + if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + await this.schoolExternalToolValidationService.validate(schoolExternalTool); + await this.contextExternalToolValidationService.validate(contextExternalTool); + } else { + this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); + } const strategy: IToolLaunchStrategy | undefined = this.strategies.get(externalTool.config.type); diff --git a/config/default.schema.json b/config/default.schema.json index a34d8e899ad..0411d4b804f 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -1327,6 +1327,11 @@ "default": false, "description": "Enables groups of type class in courses" }, + "FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED": { + "type": "boolean", + "default": false, + "description": "Enables the calculation of the outdated status of an external tool without the usage of the db attribute version" + }, "TSP_SCHOOL_SYNCER": { "type": "object", "description": "TSP School Syncer properties", From 0d292a766b956ec23db418b5beee3268af8a5663 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 8 Nov 2023 14:17:09 +0100 Subject: [PATCH 02/10] unit test and refactoring --- .../service/tool-reference.service.spec.ts | 55 +++++++++++++- .../service/tool-reference.service.ts | 4 +- ...l-external-tool-validation.service.spec.ts | 42 ++++++++++- .../service/tool-launch.service.spec.ts | 73 ++++++++++++++++++- .../service/tool-launch.service.ts | 41 +++++++---- 5 files changed, 196 insertions(+), 19 deletions(-) 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 e434ab49527..a646c50f96b 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 @@ -2,13 +2,15 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { contextExternalToolFactory, externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; +import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; -import { SchoolExternalToolService } from '../../school-external-tool/service'; +import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; import { ToolReference } from '../domain'; import { ContextExternalToolService } from './context-external-tool.service'; import { ToolReferenceService } from './tool-reference.service'; +import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; describe('ToolReferenceService', () => { let module: TestingModule; @@ -19,6 +21,8 @@ describe('ToolReferenceService', () => { let contextExternalToolService: DeepMocked; let commonToolService: DeepMocked; let externalToolLogoService: DeepMocked; + let contextExternalToolValidationService: DeepMocked; + let schoolExternalToolValidationService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -44,6 +48,14 @@ describe('ToolReferenceService', () => { provide: ExternalToolLogoService, useValue: createMock(), }, + { + provide: ContextExternalToolValidationService, + useValue: createMock(), + }, + { + provide: SchoolExternalToolValidationService, + useValue: createMock(), + }, ], }).compile(); @@ -53,6 +65,8 @@ describe('ToolReferenceService', () => { contextExternalToolService = module.get(ContextExternalToolService); commonToolService = module.get(CommonToolService); externalToolLogoService = module.get(ExternalToolLogoService); + contextExternalToolValidationService = module.get(ContextExternalToolValidationService); + schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); }); afterAll(async () => { @@ -82,6 +96,8 @@ describe('ToolReferenceService', () => { commonToolService.determineToolConfigurationStatus.mockReturnValue(ToolConfigurationStatus.OUTDATED); externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + return { contextExternalToolId, externalTool, @@ -128,5 +144,42 @@ describe('ToolReferenceService', () => { }); }); }); + + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { + const setup = () => { + const contextExternalToolId = new ObjectId().toHexString(); + const externalTool = externalToolFactory.buildWithId(); + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + toolId: externalTool.id as string, + }); + const contextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .buildWithId(undefined, contextExternalToolId); + const logoUrl = 'logoUrl'; + + contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool); + schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool); + externalToolService.findById.mockResolvedValueOnce(externalTool); + commonToolService.determineToolConfigurationStatus.mockReturnValue(ToolConfigurationStatus.OUTDATED); + externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); + + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + + return { + contextExternalToolId, + schoolExternalTool, + contextExternalTool, + }; + }; + + it('should determine the tool status through validation', async () => { + const { contextExternalToolId, schoolExternalTool, contextExternalTool } = setup(); + + await service.getToolReference(contextExternalToolId); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + }); + }); }); }); 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 f53ac754546..59c3f3d25d7 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 @@ -35,7 +35,7 @@ export class ToolReferenceService { let status: ToolConfigurationStatus; if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { - status = await this.determineToolConfigurationStatus(schoolExternalTool, contextExternalTool); + status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); } else { status = this.commonToolService.determineToolConfigurationStatus( externalTool, @@ -57,7 +57,7 @@ export class ToolReferenceService { return toolReference; } - private async determineToolConfigurationStatus( + private async determineToolConfigurationStatusThroughValidation( schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool ): Promise { 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 e43bdeb42e0..e42f2bb22da 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 { Configuration } from '@hpi-schul-cloud/commons'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; @@ -51,8 +52,12 @@ describe('SchoolExternalToolValidationService', () => { ...externalToolFactory.buildWithId(), ...externalToolDoMock, }); - externalToolService.findById.mockResolvedValue(externalTool); + const schoolExternalToolId = schoolExternalTool.id as string; + + externalToolService.findById.mockResolvedValue(externalTool); + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + return { schoolExternalTool, ExternalTool, @@ -87,9 +92,44 @@ describe('SchoolExternalToolValidationService', () => { schoolExternalTool ); }); + + it('should not throw error', async () => { + const { schoolExternalTool } = setup({ version: 8383 }, { toolVersion: 1337 }); + + const func = () => service.validate(schoolExternalTool); + + await expect(func()).resolves.not.toThrow(); + }); }); + }); + 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; + + externalToolService.findById.mockResolvedValue(externalTool); + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + + return { + schoolExternalTool, + ExternalTool, + schoolExternalToolId, + }; + }; + it('should throw error', async () => { const { schoolExternalTool } = setup({ version: 8383 }, { toolVersion: 1337 }); 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 e4f9eaa6113..9e1afff1e40 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 @@ -7,13 +7,14 @@ import { externalToolFactory, schoolExternalToolFactory, } from '@shared/testing'; +import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; import { BasicToolConfig, ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService } from '../../school-external-tool/service'; +import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; import { ToolStatusOutdatedLoggableException } from '../error'; import { LaunchRequestMethod, ToolLaunchData, ToolLaunchDataType, ToolLaunchRequest } from '../types'; import { @@ -23,6 +24,7 @@ import { OAuth2ToolLaunchStrategy, } from './launch-strategy'; import { ToolLaunchService } from './tool-launch.service'; +import { ContextExternalToolValidationService } from '../../context-external-tool/service'; describe('ToolLaunchService', () => { let module: TestingModule; @@ -32,6 +34,8 @@ describe('ToolLaunchService', () => { let externalToolService: DeepMocked; let basicToolLaunchStrategy: DeepMocked; let commonToolService: DeepMocked; + let contextExternalToolValidationService: DeepMocked; + let schoolExternalToolValidationService: DeepMocked; beforeEach(async () => { module = await Test.createTestingModule({ @@ -61,6 +65,14 @@ describe('ToolLaunchService', () => { provide: CommonToolService, useValue: createMock(), }, + { + provide: ContextExternalToolValidationService, + useValue: createMock(), + }, + { + provide: SchoolExternalToolValidationService, + useValue: createMock(), + }, ], }).compile(); @@ -69,6 +81,8 @@ describe('ToolLaunchService', () => { externalToolService = module.get(ExternalToolService); basicToolLaunchStrategy = module.get(BasicToolLaunchStrategy); commonToolService = module.get(CommonToolService); + contextExternalToolValidationService = module.get(ContextExternalToolValidationService); + schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); }); afterAll(async () => { @@ -109,6 +123,8 @@ describe('ToolLaunchService', () => { basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + return { launchDataDO, launchParams, @@ -146,6 +162,14 @@ describe('ToolLaunchService', () => { expect(externalToolService.findById).toHaveBeenCalledWith(launchParams.schoolExternalTool.toolId); }); + + it('should call determineToolConfigurationStatus', async () => { + const { launchParams } = setup(); + + await service.getLaunchData('userId', launchParams.contextExternalTool); + + expect(commonToolService.determineToolConfigurationStatus).toHaveBeenCalled(); + }); }); describe('when the tool config type is unknown', () => { @@ -213,7 +237,6 @@ describe('ToolLaunchService', () => { commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); return { - launchDataDO, launchParams, userId, contextExternalToolId: contextExternalTool.id as string, @@ -228,6 +251,52 @@ describe('ToolLaunchService', () => { await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); }); }); + + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); + const contextExternalTool: ContextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .build(); + const basicToolConfigDO: BasicToolConfig = basicToolConfigFactory.build(); + const externalTool: ExternalTool = externalToolFactory.build({ + config: basicToolConfigDO, + }); + + const launchDataDO: ToolLaunchData = new ToolLaunchData({ + type: ToolLaunchDataType.BASIC, + baseUrl: 'https://www.basic-baseurl.com/', + properties: [], + openNewTab: false, + }); + + const launchParams: IToolLaunchParams = { + externalTool, + schoolExternalTool, + contextExternalTool, + }; + + schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); + externalToolService.findById.mockResolvedValue(externalTool); + basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); + commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); + + Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + + return { + launchParams, + }; + }; + + it('should should call validate', async () => { + const { launchParams } = setup(); + + await service.getLaunchData('userId', launchParams.contextExternalTool); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalled(); + expect(contextExternalToolValidationService.validate).toHaveBeenCalled(); + }); + }); }); describe('generateLaunchRequest', () => { 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 a8d1488db9f..31784d0527a 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 @@ -57,12 +57,7 @@ export class ToolLaunchService { const { externalTool, schoolExternalTool } = await this.loadToolHierarchy(schoolExternalToolId); - if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { - await this.schoolExternalToolValidationService.validate(schoolExternalTool); - await this.contextExternalToolValidationService.validate(contextExternalTool); - } else { - this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); - } + await this.isToolStatusLatestOrThrow(userId, externalTool, schoolExternalTool, contextExternalTool); const strategy: IToolLaunchStrategy | undefined = this.strategies.get(externalTool.config.type); @@ -92,17 +87,37 @@ export class ToolLaunchService { }; } - private isToolStatusLatestOrThrow( + private async determineToolConfigurationStatusThroughValidation( + schoolExternalTool: SchoolExternalTool, + contextExternalTool: ContextExternalTool + ): Promise { + try { + await this.schoolExternalToolValidationService.validate(schoolExternalTool); + await this.contextExternalToolValidationService.validate(contextExternalTool); + return ToolConfigurationStatus.LATEST; + } catch (err) { + return ToolConfigurationStatus.OUTDATED; + } + } + + private async isToolStatusLatestOrThrow( userId: EntityId, externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool - ): void { - const status: ToolConfigurationStatus = this.commonToolService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); + ): Promise { + let status: ToolConfigurationStatus; + + if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); + } else { + status = this.commonToolService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); + } + if (status !== ToolConfigurationStatus.LATEST) { throw new ToolStatusOutdatedLoggableException(userId, contextExternalTool.id ?? ''); } From a3abd6e598bf2d2559b5a3e9f48ac80333d86bcc Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Wed, 8 Nov 2023 15:18:21 +0100 Subject: [PATCH 03/10] coverage up, merge main --- .../service/tool-reference.service.spec.ts | 13 ++++++++++++- .../service/tool-launch.service.spec.ts | 16 +++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) 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 a646c50f96b..9182a641f95 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 @@ -3,6 +3,7 @@ import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { contextExternalToolFactory, externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; import { Configuration } from '@hpi-schul-cloud/commons'; +import { ApiValidationError } from '@shared/common'; import { ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; @@ -11,6 +12,7 @@ import { ToolReference } from '../domain'; import { ContextExternalToolService } from './context-external-tool.service'; import { ToolReferenceService } from './tool-reference.service'; import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; +import objectContaining = jasmine.objectContaining; describe('ToolReferenceService', () => { let module: TestingModule; @@ -160,9 +162,10 @@ describe('ToolReferenceService', () => { contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool); schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool); externalToolService.findById.mockResolvedValueOnce(externalTool); - commonToolService.determineToolConfigurationStatus.mockReturnValue(ToolConfigurationStatus.OUTDATED); + commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); + schoolExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); return { @@ -180,6 +183,14 @@ describe('ToolReferenceService', () => { expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); }); + + it('should get the outdated status', async () => { + const { contextExternalToolId } = setup(); + + const toolReference: ToolReference = await service.getToolReference(contextExternalToolId); + + expect(toolReference).toEqual(expect.objectContaining({ status: ToolConfigurationStatus.OUTDATED })); + }); }); }); }); 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 9e1afff1e40..9f814aaacd8 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 @@ -8,6 +8,7 @@ import { schoolExternalToolFactory, } from '@shared/testing'; import { Configuration } from '@hpi-schul-cloud/commons'; +import { ApiValidationError } from '@shared/common'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; @@ -270,6 +271,8 @@ describe('ToolLaunchService', () => { openNewTab: false, }); + const userId = 'userId'; + const launchParams: IToolLaunchParams = { externalTool, schoolExternalTool, @@ -279,12 +282,15 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); + commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); + contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); return { launchParams, + userId, + contextExternalToolId: contextExternalTool.id as string, }; }; @@ -296,6 +302,14 @@ describe('ToolLaunchService', () => { expect(schoolExternalToolValidationService.validate).toHaveBeenCalled(); expect(contextExternalToolValidationService.validate).toHaveBeenCalled(); }); + + it('should throw ToolStatusOutdatedLoggableException', async () => { + const { launchParams, userId, contextExternalToolId } = setup(); + + const func = () => service.getLaunchData(userId, launchParams.contextExternalTool); + + await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); + }); }); }); From 2028064305e34e4184acc86259d03d8169bfe20f Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 9 Nov 2023 12:35:30 +0100 Subject: [PATCH 04/10] add feature flag to tool config and fix unit tests --- .../service/tool-reference.service.spec.ts | 19 ++++++---- apps/server/src/modules/tool/tool-config.ts | 2 ++ .../service/tool-launch.service.spec.ts | 36 +++++++++++++------ 3 files changed, 40 insertions(+), 17 deletions(-) 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 9182a641f95..3de57e5531f 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 @@ -2,7 +2,6 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { contextExternalToolFactory, externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { ApiValidationError } from '@shared/common'; import { ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; @@ -12,7 +11,7 @@ import { ToolReference } from '../domain'; import { ContextExternalToolService } from './context-external-tool.service'; import { ToolReferenceService } from './tool-reference.service'; import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; -import objectContaining = jasmine.objectContaining; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; describe('ToolReferenceService', () => { let module: TestingModule; @@ -25,6 +24,7 @@ describe('ToolReferenceService', () => { let externalToolLogoService: DeepMocked; let contextExternalToolValidationService: DeepMocked; let schoolExternalToolValidationService: DeepMocked; + let toolFeatures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -58,6 +58,12 @@ describe('ToolReferenceService', () => { provide: SchoolExternalToolValidationService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + toolStatusWithoutVersions: false, + }, + }, ], }).compile(); @@ -69,6 +75,7 @@ describe('ToolReferenceService', () => { externalToolLogoService = module.get(ExternalToolLogoService); contextExternalToolValidationService = module.get(ContextExternalToolValidationService); schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); + toolFeatures = module.get(ToolFeatures); }); afterAll(async () => { @@ -98,7 +105,7 @@ describe('ToolReferenceService', () => { commonToolService.determineToolConfigurationStatus.mockReturnValue(ToolConfigurationStatus.OUTDATED); externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + toolFeatures.toolStatusWithoutVersions = false; return { contextExternalToolId, @@ -166,7 +173,7 @@ describe('ToolReferenceService', () => { externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); schoolExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + toolFeatures.toolStatusWithoutVersions = true; return { contextExternalToolId, @@ -176,12 +183,12 @@ describe('ToolReferenceService', () => { }; it('should determine the tool status through validation', async () => { - const { contextExternalToolId, schoolExternalTool, contextExternalTool } = setup(); + const { contextExternalToolId, schoolExternalTool } = setup(); await service.getToolReference(contextExternalToolId); expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); - expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(contextExternalToolValidationService.validate).not.toHaveBeenCalled(); }); it('should get the outdated status', async () => { diff --git a/apps/server/src/modules/tool/tool-config.ts b/apps/server/src/modules/tool/tool-config.ts index 2bd47089286..ea8641727ff 100644 --- a/apps/server/src/modules/tool/tool-config.ts +++ b/apps/server/src/modules/tool/tool-config.ts @@ -6,6 +6,7 @@ export interface IToolFeatures { ctlToolsTabEnabled: boolean; ltiToolsTabEnabled: boolean; contextConfigurationEnabled: boolean; + toolStatusWithoutVersions: boolean; maxExternalToolLogoSizeInBytes: number; backEndUrl: string; } @@ -15,6 +16,7 @@ export default class ToolConfiguration { ctlToolsTabEnabled: Configuration.get('FEATURE_CTL_TOOLS_TAB_ENABLED') as boolean, ltiToolsTabEnabled: Configuration.get('FEATURE_LTI_TOOLS_TAB_ENABLED') as boolean, contextConfigurationEnabled: Configuration.get('FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED') as boolean, + 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, }; 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 9f814aaacd8..30e87c2433a 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 @@ -7,7 +7,6 @@ import { externalToolFactory, schoolExternalToolFactory, } from '@shared/testing'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { ApiValidationError } from '@shared/common'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; @@ -26,6 +25,7 @@ import { } from './launch-strategy'; import { ToolLaunchService } from './tool-launch.service'; import { ContextExternalToolValidationService } from '../../context-external-tool/service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; describe('ToolLaunchService', () => { let module: TestingModule; @@ -37,6 +37,7 @@ describe('ToolLaunchService', () => { let commonToolService: DeepMocked; let contextExternalToolValidationService: DeepMocked; let schoolExternalToolValidationService: DeepMocked; + let toolFeatures: DeepMocked; beforeEach(async () => { module = await Test.createTestingModule({ @@ -74,6 +75,12 @@ describe('ToolLaunchService', () => { provide: SchoolExternalToolValidationService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + toolStatusWithoutVersions: false, + }, + }, ], }).compile(); @@ -84,6 +91,7 @@ describe('ToolLaunchService', () => { commonToolService = module.get(CommonToolService); contextExternalToolValidationService = module.get(ContextExternalToolValidationService); schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); + toolFeatures = module.get(ToolFeatures); }); afterAll(async () => { @@ -124,7 +132,7 @@ describe('ToolLaunchService', () => { basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + toolFeatures.toolStatusWithoutVersions = false; return { launchDataDO, @@ -192,6 +200,8 @@ describe('ToolLaunchService', () => { externalToolService.findById.mockResolvedValue(externalTool); commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); + toolFeatures.toolStatusWithoutVersions = false; + return { launchParams, }; @@ -237,6 +247,8 @@ describe('ToolLaunchService', () => { basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); + toolFeatures.toolStatusWithoutVersions = false; + return { launchParams, userId, @@ -283,9 +295,11 @@ describe('ToolLaunchService', () => { externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); + commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + contextExternalToolValidationService.validate.mockResolvedValueOnce(); + toolFeatures.toolStatusWithoutVersions = true; return { launchParams, @@ -294,6 +308,14 @@ describe('ToolLaunchService', () => { }; }; + it('should throw ToolStatusOutdatedLoggableException', async () => { + const { launchParams, userId, contextExternalToolId } = setup(); + + const func = () => service.getLaunchData(userId, launchParams.contextExternalTool); + + await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); + }); + it('should should call validate', async () => { const { launchParams } = setup(); @@ -302,14 +324,6 @@ describe('ToolLaunchService', () => { expect(schoolExternalToolValidationService.validate).toHaveBeenCalled(); expect(contextExternalToolValidationService.validate).toHaveBeenCalled(); }); - - it('should throw ToolStatusOutdatedLoggableException', async () => { - const { launchParams, userId, contextExternalToolId } = setup(); - - const func = () => service.getLaunchData(userId, launchParams.contextExternalTool); - - await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); - }); }); }); From 8902f34bb1b1974cbe719d3879876562fc7d4804 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 9 Nov 2023 13:29:55 +0100 Subject: [PATCH 05/10] add feature flag dependent status determination in school service --- .../service/school-external-tool.service.ts | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.ts index 2f011560f6a..e654f82f96a 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.ts @@ -1,4 +1,4 @@ -import { Injectable } from '@nestjs/common'; +import { Inject, Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; import { SchoolExternalToolRepo } from '@shared/repo'; import { ToolConfigurationStatus } from '../../common/enum'; @@ -6,12 +6,16 @@ import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolQuery } from '../uc/dto/school-external-tool.types'; +import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; @Injectable() export class SchoolExternalToolService { constructor( private readonly schoolExternalToolRepo: SchoolExternalToolRepo, - private readonly externalToolService: ExternalToolService + private readonly externalToolService: ExternalToolService, + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} async findById(schoolExternalToolId: EntityId): Promise { @@ -39,7 +43,7 @@ export class SchoolExternalToolService { private async enrichDataFromExternalTool(tool: SchoolExternalTool): Promise { const externalTool: ExternalTool = await this.externalToolService.findById(tool.toolId); - const status: ToolConfigurationStatus = this.determineStatus(tool, externalTool); + const status: ToolConfigurationStatus = await this.determineStatus(tool, externalTool); const schoolExternalTool: SchoolExternalTool = new SchoolExternalTool({ ...tool, status, @@ -49,7 +53,19 @@ export class SchoolExternalToolService { return schoolExternalTool; } - private determineStatus(tool: SchoolExternalTool, externalTool: ExternalTool): ToolConfigurationStatus { + private async determineStatus( + tool: SchoolExternalTool, + externalTool: ExternalTool + ): Promise { + if (this.toolFeatures.toolStatusWithoutVersions) { + try { + await this.schoolExternalToolValidationService.validate(tool); + return ToolConfigurationStatus.LATEST; + } catch (err) { + return ToolConfigurationStatus.OUTDATED; + } + } + if (externalTool.version <= tool.toolVersion) { return ToolConfigurationStatus.LATEST; } From 9944948652178c4ff057a0df4a277e05fa73e284 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 9 Nov 2023 15:30:10 +0100 Subject: [PATCH 06/10] request changes --- .../context-external-tool.module.ts | 3 ++- .../service/tool-reference.service.spec.ts | 1 + .../service/tool-reference.service.ts | 9 +++++---- .../tool/external-tool/external-tool.module.ts | 6 +++--- ...=> external-tool-version-increment.service.ts} | 2 +- .../service/external-tool-version.service.spec.ts | 6 +++--- .../service/external-tool.service.spec.ts | 10 +++++----- .../service/external-tool.service.ts | 4 ++-- .../modules/tool/external-tool/service/index.ts | 2 +- .../school-external-tool.module.ts | 3 ++- ...chool-external-tool-validation.service.spec.ts | 15 ++++++++++++--- .../school-external-tool-validation.service.ts | 9 +++++---- apps/server/src/modules/tool/tool-config.ts | 2 ++ .../service/tool-launch.service.spec.ts | 1 + .../tool-launch/service/tool-launch.service.ts | 9 +++++---- .../tool/tool-launch/tool-launch.module.ts | 2 ++ 16 files changed, 52 insertions(+), 32 deletions(-) rename apps/server/src/modules/tool/external-tool/service/{external-tool-version.service.ts => external-tool-version-increment.service.ts} (98%) diff --git a/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts b/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts index 1afd639f1e7..7e2e329ec9f 100644 --- a/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts +++ b/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts @@ -9,9 +9,10 @@ import { ContextExternalToolValidationService, ToolReferenceService, } from './service'; +import { ToolConfigModule } from '../tool-config.module'; @Module({ - imports: [CommonToolModule, ExternalToolModule, SchoolExternalToolModule, LoggerModule], + imports: [CommonToolModule, ExternalToolModule, SchoolExternalToolModule, LoggerModule, ToolConfigModule], providers: [ ContextExternalToolService, ContextExternalToolValidationService, 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 3de57e5531f..7c3103d170a 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 @@ -154,6 +154,7 @@ describe('ToolReferenceService', () => { }); }); + // TODO N21-1337 refactor after feature flag is removed describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { const setup = () => { const contextExternalToolId = new ObjectId().toHexString(); 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 59c3f3d25d7..dea8346ed22 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,6 +1,5 @@ -import { Injectable } from '@nestjs/common'; +import { Inject, Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; @@ -11,6 +10,7 @@ import { ContextExternalTool, ToolReference } from '../domain'; import { ToolReferenceMapper } from '../mapper'; import { ContextExternalToolService } from './context-external-tool.service'; import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; @Injectable() export class ToolReferenceService { @@ -21,7 +21,8 @@ export class ToolReferenceService { private readonly commonToolService: CommonToolService, private readonly externalToolLogoService: ExternalToolLogoService, private readonly contextExternalToolValidationService: ContextExternalToolValidationService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} async getToolReference(contextExternalToolId: EntityId): Promise { @@ -34,7 +35,7 @@ export class ToolReferenceService { const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); let status: ToolConfigurationStatus; - if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + if (this.toolFeatures.toolStatusWithoutVersions) { status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); } else { status = this.commonToolService.determineToolConfigurationStatus( diff --git a/apps/server/src/modules/tool/external-tool/external-tool.module.ts b/apps/server/src/modules/tool/external-tool/external-tool.module.ts index 2fbd2f28edd..9aa0a11e448 100644 --- a/apps/server/src/modules/tool/external-tool/external-tool.module.ts +++ b/apps/server/src/modules/tool/external-tool/external-tool.module.ts @@ -12,7 +12,7 @@ import { ExternalToolService, ExternalToolServiceMapper, ExternalToolValidationService, - ExternalToolVersionService, + ExternalToolVersionIncrementService, } from './service'; import { CommonToolModule } from '../common'; @@ -23,7 +23,7 @@ import { CommonToolModule } from '../common'; ExternalToolServiceMapper, ExternalToolParameterValidationService, ExternalToolValidationService, - ExternalToolVersionService, + ExternalToolVersionIncrementService, ExternalToolConfigurationService, ExternalToolLogoService, ExternalToolRepo, @@ -31,7 +31,7 @@ import { CommonToolModule } from '../common'; exports: [ ExternalToolService, ExternalToolValidationService, - ExternalToolVersionService, + ExternalToolVersionIncrementService, ExternalToolConfigurationService, ExternalToolLogoService, ], diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-version-increment.service.ts similarity index 98% rename from apps/server/src/modules/tool/external-tool/service/external-tool-version.service.ts rename to apps/server/src/modules/tool/external-tool/service/external-tool-version-increment.service.ts index c71e3b49b91..9e645b97ad2 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-version-increment.service.ts @@ -3,7 +3,7 @@ import { ExternalTool } from '../domain'; import { CustomParameter } from '../../common/domain'; @Injectable() -export class ExternalToolVersionService { +export class ExternalToolVersionIncrementService { increaseVersionOfNewToolIfNecessary(oldTool: ExternalTool, newTool: ExternalTool): void { if (!oldTool.parameters || !newTool.parameters) { return; diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.spec.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.spec.ts index 91566609a8f..dabfd70996e 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.spec.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-version.service.spec.ts @@ -1,14 +1,14 @@ import { customParameterFactory, externalToolFactory } from '@shared/testing/factory/domainobject/tool'; -import { ExternalToolVersionService } from './external-tool-version.service'; +import { ExternalToolVersionIncrementService } from './external-tool-version-increment.service'; import { CustomParameterLocation, CustomParameterScope, CustomParameterType } from '../../common/enum'; import { CustomParameter } from '../../common/domain'; import { ExternalTool } from '../domain'; describe('ExternalToolVersionService', () => { - let service: ExternalToolVersionService; + let service: ExternalToolVersionIncrementService; beforeEach(() => { - service = new ExternalToolVersionService(); + service = new ExternalToolVersionIncrementService(); }); const setup = () => { diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool.service.spec.ts b/apps/server/src/modules/tool/external-tool/service/external-tool.service.spec.ts index ddb88ca4ff3..be146f30941 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool.service.spec.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool.service.spec.ts @@ -16,7 +16,7 @@ import { ExternalToolSearchQuery } from '../../common/interface'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { ExternalTool, Lti11ToolConfig, Oauth2ToolConfig } from '../domain'; import { ExternalToolServiceMapper } from './external-tool-service.mapper'; -import { ExternalToolVersionService } from './external-tool-version.service'; +import { ExternalToolVersionIncrementService } from './external-tool-version-increment.service'; import { ExternalToolService } from './external-tool.service'; describe('ExternalToolService', () => { @@ -29,7 +29,7 @@ describe('ExternalToolService', () => { let oauthProviderService: DeepMocked; let mapper: DeepMocked; let encryptionService: DeepMocked; - let versionService: DeepMocked; + let versionService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -64,8 +64,8 @@ describe('ExternalToolService', () => { useValue: createMock(), }, { - provide: ExternalToolVersionService, - useValue: createMock(), + provide: ExternalToolVersionIncrementService, + useValue: createMock(), }, ], }).compile(); @@ -77,7 +77,7 @@ describe('ExternalToolService', () => { oauthProviderService = module.get(OauthProviderService); mapper = module.get(ExternalToolServiceMapper); encryptionService = module.get(DefaultEncryptionService); - versionService = module.get(ExternalToolVersionService); + versionService = module.get(ExternalToolVersionIncrementService); }); afterAll(async () => { 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 0aa6181682c..d8481c28c71 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 @@ -10,7 +10,7 @@ import { ExternalToolSearchQuery } from '../../common/interface'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { ExternalTool, Oauth2ToolConfig } from '../domain'; import { ExternalToolServiceMapper } from './external-tool-service.mapper'; -import { ExternalToolVersionService } from './external-tool-version.service'; +import { ExternalToolVersionIncrementService } from './external-tool-version-increment.service'; @Injectable() export class ExternalToolService { @@ -22,7 +22,7 @@ export class ExternalToolService { private readonly contextExternalToolRepo: ContextExternalToolRepo, @Inject(DefaultEncryptionService) private readonly encryptionService: IEncryptionService, private readonly legacyLogger: LegacyLogger, - private readonly externalToolVersionService: ExternalToolVersionService + private readonly externalToolVersionService: ExternalToolVersionIncrementService ) {} async createExternalTool(externalTool: ExternalTool): Promise { diff --git a/apps/server/src/modules/tool/external-tool/service/index.ts b/apps/server/src/modules/tool/external-tool/service/index.ts index 8cb1df69bc1..f2290ca8969 100644 --- a/apps/server/src/modules/tool/external-tool/service/index.ts +++ b/apps/server/src/modules/tool/external-tool/service/index.ts @@ -1,6 +1,6 @@ export * from './external-tool.service'; export * from './external-tool-service.mapper'; -export * from './external-tool-version.service'; +export * from './external-tool-version-increment.service'; export * from './external-tool-validation.service'; export * from './external-tool-parameter-validation.service'; export * from './external-tool-configuration.service'; diff --git a/apps/server/src/modules/tool/school-external-tool/school-external-tool.module.ts b/apps/server/src/modules/tool/school-external-tool/school-external-tool.module.ts index 790f4e716c2..898e159348a 100644 --- a/apps/server/src/modules/tool/school-external-tool/school-external-tool.module.ts +++ b/apps/server/src/modules/tool/school-external-tool/school-external-tool.module.ts @@ -2,9 +2,10 @@ import { Module } from '@nestjs/common'; import { CommonToolModule } from '../common'; import { SchoolExternalToolService, SchoolExternalToolValidationService } from './service'; import { ExternalToolModule } from '../external-tool'; +import { ToolConfigModule } from '../tool-config.module'; @Module({ - imports: [CommonToolModule, ExternalToolModule], + imports: [CommonToolModule, ExternalToolModule, ToolConfigModule], providers: [SchoolExternalToolService, SchoolExternalToolValidationService], exports: [SchoolExternalToolService, SchoolExternalToolValidationService], }) 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 e42f2bb22da..6b4f69d6686 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,12 +1,12 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { Test, TestingModule } from '@nestjs/testing'; import { externalToolFactory, schoolExternalToolFactory } from '@shared/testing/factory/domainobject/tool'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; describe('SchoolExternalToolValidationService', () => { let module: TestingModule; @@ -14,6 +14,7 @@ describe('SchoolExternalToolValidationService', () => { let externalToolService: DeepMocked; let commonToolValidationService: DeepMocked; + let toolFeatures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -27,12 +28,19 @@ describe('SchoolExternalToolValidationService', () => { provide: CommonToolValidationService, useValue: createMock(), }, + { + provide: ToolFeatures, + useValue: { + toolStatusWithoutVersions: false, + }, + }, ], }).compile(); service = module.get(SchoolExternalToolValidationService); externalToolService = module.get(ExternalToolService); commonToolValidationService = module.get(CommonToolValidationService); + toolFeatures = module.get(ToolFeatures); }); afterEach(() => { @@ -56,7 +64,7 @@ describe('SchoolExternalToolValidationService', () => { const schoolExternalToolId = schoolExternalTool.id as string; externalToolService.findById.mockResolvedValue(externalTool); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', true); + toolFeatures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -103,6 +111,7 @@ 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 = ( @@ -121,7 +130,7 @@ describe('SchoolExternalToolValidationService', () => { const schoolExternalToolId = schoolExternalTool.id as string; externalToolService.findById.mockResolvedValue(externalTool); - Configuration.set('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED', false); + toolFeatures.toolStatusWithoutVersions = false; return { 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 c951387de19..3474aa90f5e 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 @@ -1,16 +1,17 @@ -import { Injectable } from '@nestjs/common'; +import { Inject, Injectable } from '@nestjs/common'; import { ValidationError } from '@shared/common'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../domain'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; @Injectable() export class SchoolExternalToolValidationService { constructor( private readonly externalToolService: ExternalToolService, - private readonly commonToolValidationService: CommonToolValidationService + private readonly commonToolValidationService: CommonToolValidationService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) {} async validate(schoolExternalTool: SchoolExternalTool): Promise { @@ -18,7 +19,7 @@ export class SchoolExternalToolValidationService { const loadedExternalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - if (!Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + if (!this.toolFeatures.toolStatusWithoutVersions) { this.checkVersionMatch(schoolExternalTool.toolVersion, loadedExternalTool.version); } this.commonToolValidationService.checkCustomParameterEntries(loadedExternalTool, schoolExternalTool); diff --git a/apps/server/src/modules/tool/tool-config.ts b/apps/server/src/modules/tool/tool-config.ts index ea8641727ff..2b0af584e53 100644 --- a/apps/server/src/modules/tool/tool-config.ts +++ b/apps/server/src/modules/tool/tool-config.ts @@ -6,6 +6,7 @@ export interface IToolFeatures { ctlToolsTabEnabled: boolean; ltiToolsTabEnabled: boolean; contextConfigurationEnabled: boolean; + // TODO N21-1337 toolStatusWithoutVersions: boolean; maxExternalToolLogoSizeInBytes: number; backEndUrl: string; @@ -16,6 +17,7 @@ export default class ToolConfiguration { ctlToolsTabEnabled: Configuration.get('FEATURE_CTL_TOOLS_TAB_ENABLED') as boolean, ltiToolsTabEnabled: Configuration.get('FEATURE_LTI_TOOLS_TAB_ENABLED') as boolean, contextConfigurationEnabled: Configuration.get('FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED') as boolean, + // TODO N21-1337 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, 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 30e87c2433a..ec4d9848d47 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 @@ -265,6 +265,7 @@ describe('ToolLaunchService', () => { }); }); + // TODO N21-1337 refactor after feature flag is removed describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { const setup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); 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 31784d0527a..99411b66db7 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 @@ -1,6 +1,5 @@ -import { Injectable, InternalServerErrorException } from '@nestjs/common'; +import { Inject, Injectable, InternalServerErrorException } from '@nestjs/common'; import { EntityId } from '@shared/domain'; -import { Configuration } from '@hpi-schul-cloud/commons'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; @@ -18,6 +17,7 @@ import { OAuth2ToolLaunchStrategy, } from './launch-strategy'; import { ContextExternalToolValidationService } from '../../context-external-tool/service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; @Injectable() export class ToolLaunchService { @@ -31,7 +31,8 @@ export class ToolLaunchService { private readonly oauth2ToolLaunchStrategy: OAuth2ToolLaunchStrategy, private readonly commonToolService: CommonToolService, private readonly contextExternalToolValidationService: ContextExternalToolValidationService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) { this.strategies = new Map(); this.strategies.set(ToolConfigType.BASIC, basicToolLaunchStrategy); @@ -108,7 +109,7 @@ export class ToolLaunchService { ): Promise { let status: ToolConfigurationStatus; - if (Configuration.get('FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED')) { + if (this.toolFeatures.toolStatusWithoutVersions) { status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); } else { status = this.commonToolService.determineToolConfigurationStatus( diff --git a/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts b/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts index b8ff7623586..d214ef41436 100644 --- a/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts +++ b/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts @@ -16,6 +16,7 @@ import { AutoSchoolNumberStrategy, } from './service/auto-parameter-strategy'; import { BasicToolLaunchStrategy, Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrategy } from './service/launch-strategy'; +import { ToolConfigModule } from '../tool-config.module'; @Module({ imports: [ @@ -28,6 +29,7 @@ import { BasicToolLaunchStrategy, Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrat forwardRef(() => PseudonymModule), // i do not like this solution, the root problem is on other place but not detectable for me LearnroomModule, BoardModule, + ToolConfigModule, ], providers: [ ToolLaunchService, From 30787e123a3dd8b09e28a4166ad4c4a9701cc4e6 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Thu, 9 Nov 2023 16:27:05 +0100 Subject: [PATCH 07/10] request changes, TODO adjust unit tests --- .../context-external-tool.module.ts | 11 +++-- .../context-external-tool/service/index.ts | 1 - .../service/tool-reference.service.ts | 40 ++++------------- .../service/tool-version-service.ts | 44 +++++++++++++++++++ .../uc/context-external-tool.uc.ts | 3 +- .../service/tool-launch.service.ts | 38 ++++------------ 6 files changed, 68 insertions(+), 69 deletions(-) create mode 100644 apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts diff --git a/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts b/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts index 7e2e329ec9f..d73e8a25171 100644 --- a/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts +++ b/apps/server/src/modules/tool/context-external-tool/context-external-tool.module.ts @@ -3,13 +3,10 @@ import { LoggerModule } from '@src/core/logger'; import { CommonToolModule } from '../common'; import { ExternalToolModule } from '../external-tool'; import { SchoolExternalToolModule } from '../school-external-tool'; -import { - ContextExternalToolAuthorizableService, - ContextExternalToolService, - ContextExternalToolValidationService, - ToolReferenceService, -} from './service'; +import { ContextExternalToolAuthorizableService, ContextExternalToolService, ToolReferenceService } from './service'; +import { ContextExternalToolValidationService } from './service/context-external-tool-validation.service'; import { ToolConfigModule } from '../tool-config.module'; +import { ToolVersionService } from './service/tool-version-service'; @Module({ imports: [CommonToolModule, ExternalToolModule, SchoolExternalToolModule, LoggerModule, ToolConfigModule], @@ -18,12 +15,14 @@ import { ToolConfigModule } from '../tool-config.module'; ContextExternalToolValidationService, ContextExternalToolAuthorizableService, ToolReferenceService, + ToolVersionService, ], exports: [ ContextExternalToolService, ContextExternalToolValidationService, ContextExternalToolAuthorizableService, ToolReferenceService, + ToolVersionService, ], }) export class ContextExternalToolModule {} diff --git a/apps/server/src/modules/tool/context-external-tool/service/index.ts b/apps/server/src/modules/tool/context-external-tool/service/index.ts index 31fedbe42af..30458b4b9c3 100644 --- a/apps/server/src/modules/tool/context-external-tool/service/index.ts +++ b/apps/server/src/modules/tool/context-external-tool/service/index.ts @@ -1,4 +1,3 @@ export * from './context-external-tool.service'; -export * from './context-external-tool-validation.service'; export * from './context-external-tool-authorizable.service'; export * from './tool-reference.service'; 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 dea8346ed22..ffaca189acb 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,16 +1,14 @@ import { Inject, Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; -import { ToolConfigurationStatus } from '../../common/enum'; -import { CommonToolService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool, ToolReference } from '../domain'; import { ToolReferenceMapper } from '../mapper'; import { ContextExternalToolService } from './context-external-tool.service'; -import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolVersionService } from './tool-version-service'; @Injectable() export class ToolReferenceService { @@ -18,11 +16,9 @@ export class ToolReferenceService { private readonly externalToolService: ExternalToolService, private readonly schoolExternalToolService: SchoolExternalToolService, private readonly contextExternalToolService: ContextExternalToolService, - private readonly commonToolService: CommonToolService, private readonly externalToolLogoService: ExternalToolLogoService, - private readonly contextExternalToolValidationService: ContextExternalToolValidationService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, + private readonly toolVersionService: ToolVersionService ) {} async getToolReference(contextExternalToolId: EntityId): Promise { @@ -34,16 +30,11 @@ export class ToolReferenceService { ); const externalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - let status: ToolConfigurationStatus; - if (this.toolFeatures.toolStatusWithoutVersions) { - status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); - } else { - status = this.commonToolService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - } + const status = await this.toolVersionService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); const toolReference: ToolReference = ToolReferenceMapper.mapToToolReference( externalTool, @@ -57,17 +48,4 @@ export class ToolReferenceService { return toolReference; } - - private async determineToolConfigurationStatusThroughValidation( - schoolExternalTool: SchoolExternalTool, - contextExternalTool: ContextExternalTool - ): Promise { - try { - await this.schoolExternalToolValidationService.validate(schoolExternalTool); - await this.contextExternalToolValidationService.validate(contextExternalTool); - return ToolConfigurationStatus.LATEST; - } catch (err) { - return ToolConfigurationStatus.OUTDATED; - } - } } 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 new file mode 100644 index 00000000000..2ba0709372a --- /dev/null +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts @@ -0,0 +1,44 @@ +import { Injectable } from '@nestjs/common/decorators/core/injectable.decorator'; +import { Inject } from '@nestjs/common'; +import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; +import { SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ExternalTool } from '../../external-tool/domain'; +import { SchoolExternalTool } from '../../school-external-tool/domain'; +import { ContextExternalTool } from '../domain'; +import { ToolConfigurationStatus } from '../../common/enum'; +import { CommonToolService } from '../../common/service'; + +@Injectable() +export class ToolVersionService { + constructor( + private readonly contextExternalToolValidationService: ContextExternalToolValidationService, + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, + private readonly commonToolService: CommonToolService, + @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + ) {} + + async determineToolConfigurationStatus( + externalTool: ExternalTool, + schoolExternalTool: SchoolExternalTool, + contextExternalTool: ContextExternalTool + ): Promise { + // TODO N21-1337 remove if statement, when feature flag is removed + if (this.toolFeatures.toolStatusWithoutVersions) { + try { + await this.schoolExternalToolValidationService.validate(schoolExternalTool); + await this.contextExternalToolValidationService.validate(contextExternalTool); + return ToolConfigurationStatus.LATEST; + } catch (err) { + return ToolConfigurationStatus.OUTDATED; + } + } + const status: ToolConfigurationStatus = this.commonToolService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); + + return status; + } +} diff --git a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts index 587ecb01c64..80a85321933 100644 --- a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts +++ b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.ts @@ -12,7 +12,8 @@ import { ToolPermissionHelper } from '../../common/uc/tool-permission-helper'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool, ContextRef } from '../domain'; -import { ContextExternalToolService, ContextExternalToolValidationService } from '../service'; +import { ContextExternalToolService } from '../service'; +import { ContextExternalToolValidationService } from '../service/context-external-tool-validation.service'; import { ContextExternalToolDto } from './dto/context-external-tool.types'; @Injectable() 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 99411b66db7..fe5820deb6d 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 @@ -1,12 +1,11 @@ import { Inject, Injectable, InternalServerErrorException } from '@nestjs/common'; import { EntityId } from '@shared/domain'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; -import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ToolStatusOutdatedLoggableException } from '../error'; import { ToolLaunchMapper } from '../mapper'; import { ToolLaunchData, ToolLaunchRequest } from '../types'; @@ -16,8 +15,8 @@ import { Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrategy, } from './launch-strategy'; -import { ContextExternalToolValidationService } from '../../context-external-tool/service'; import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolVersionService } from '../../context-external-tool/service/tool-version-service'; @Injectable() export class ToolLaunchService { @@ -29,9 +28,7 @@ export class ToolLaunchService { private readonly basicToolLaunchStrategy: BasicToolLaunchStrategy, private readonly lti11ToolLaunchStrategy: Lti11ToolLaunchStrategy, private readonly oauth2ToolLaunchStrategy: OAuth2ToolLaunchStrategy, - private readonly commonToolService: CommonToolService, - private readonly contextExternalToolValidationService: ContextExternalToolValidationService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, + private readonly toolVersionService: ToolVersionService, @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures ) { this.strategies = new Map(); @@ -88,36 +85,17 @@ export class ToolLaunchService { }; } - private async determineToolConfigurationStatusThroughValidation( - schoolExternalTool: SchoolExternalTool, - contextExternalTool: ContextExternalTool - ): Promise { - try { - await this.schoolExternalToolValidationService.validate(schoolExternalTool); - await this.contextExternalToolValidationService.validate(contextExternalTool); - return ToolConfigurationStatus.LATEST; - } catch (err) { - return ToolConfigurationStatus.OUTDATED; - } - } - private async isToolStatusLatestOrThrow( userId: EntityId, externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, contextExternalTool: ContextExternalTool ): Promise { - let status: ToolConfigurationStatus; - - if (this.toolFeatures.toolStatusWithoutVersions) { - status = await this.determineToolConfigurationStatusThroughValidation(schoolExternalTool, contextExternalTool); - } else { - status = this.commonToolService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - } + const status = await this.toolVersionService.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); if (status !== ToolConfigurationStatus.LATEST) { throw new ToolStatusOutdatedLoggableException(userId, contextExternalTool.id ?? ''); From e12b82405de3beac534de910f3c5044a742b5285 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Mon, 13 Nov 2023 12:56:48 +0100 Subject: [PATCH 08/10] unit test adjustments for requested changes --- .../service/tool-reference.service.spec.ts | 88 +-------- .../service/tool-reference.service.ts | 4 +- .../service/tool-version-service.spec.ts | 168 ++++++++++++++++++ .../service/tool-launch.service.spec.ts | 113 ++---------- .../service/tool-launch.service.ts | 6 +- .../tool/tool-launch/tool-launch.module.ts | 2 - 6 files changed, 190 insertions(+), 191 deletions(-) create mode 100644 apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts 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 7c3103d170a..1796b2f681d 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 @@ -2,16 +2,13 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { contextExternalToolFactory, externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; -import { ApiValidationError } from '@shared/common'; import { ToolConfigurationStatus } from '../../common/enum'; -import { CommonToolService } from '../../common/service'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; -import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ToolReference } from '../domain'; import { ContextExternalToolService } from './context-external-tool.service'; import { ToolReferenceService } from './tool-reference.service'; -import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolVersionService } from './tool-version-service'; describe('ToolReferenceService', () => { let module: TestingModule; @@ -20,11 +17,8 @@ describe('ToolReferenceService', () => { let externalToolService: DeepMocked; let schoolExternalToolService: DeepMocked; let contextExternalToolService: DeepMocked; - let commonToolService: DeepMocked; + let toolVersionService: DeepMocked; let externalToolLogoService: DeepMocked; - let contextExternalToolValidationService: DeepMocked; - let schoolExternalToolValidationService: DeepMocked; - let toolFeatures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -43,27 +37,13 @@ describe('ToolReferenceService', () => { useValue: createMock(), }, { - provide: CommonToolService, - useValue: createMock(), + provide: ToolVersionService, + useValue: createMock(), }, { provide: ExternalToolLogoService, useValue: createMock(), }, - { - provide: ContextExternalToolValidationService, - useValue: createMock(), - }, - { - provide: SchoolExternalToolValidationService, - useValue: createMock(), - }, - { - provide: ToolFeatures, - useValue: { - toolStatusWithoutVersions: false, - }, - }, ], }).compile(); @@ -71,11 +51,8 @@ describe('ToolReferenceService', () => { externalToolService = module.get(ExternalToolService); schoolExternalToolService = module.get(SchoolExternalToolService); contextExternalToolService = module.get(ContextExternalToolService); - commonToolService = module.get(CommonToolService); + toolVersionService = module.get(ToolVersionService); externalToolLogoService = module.get(ExternalToolLogoService); - contextExternalToolValidationService = module.get(ContextExternalToolValidationService); - schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); - toolFeatures = module.get(ToolFeatures); }); afterAll(async () => { @@ -102,11 +79,9 @@ describe('ToolReferenceService', () => { contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool); schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool); externalToolService.findById.mockResolvedValueOnce(externalTool); - commonToolService.determineToolConfigurationStatus.mockReturnValue(ToolConfigurationStatus.OUTDATED); + toolVersionService.determineToolConfigurationStatus.mockResolvedValue(ToolConfigurationStatus.OUTDATED); externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); - toolFeatures.toolStatusWithoutVersions = false; - return { contextExternalToolId, externalTool, @@ -121,7 +96,7 @@ describe('ToolReferenceService', () => { await service.getToolReference(contextExternalToolId); - expect(commonToolService.determineToolConfigurationStatus).toHaveBeenCalledWith( + expect(toolVersionService.determineToolConfigurationStatus).toHaveBeenCalledWith( externalTool, schoolExternalTool, contextExternalTool @@ -153,52 +128,5 @@ describe('ToolReferenceService', () => { }); }); }); - - // TODO N21-1337 refactor after feature flag is removed - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { - const setup = () => { - const contextExternalToolId = new ObjectId().toHexString(); - const externalTool = externalToolFactory.buildWithId(); - const schoolExternalTool = schoolExternalToolFactory.buildWithId({ - toolId: externalTool.id as string, - }); - const contextExternalTool = contextExternalToolFactory - .withSchoolExternalToolRef(schoolExternalTool.id as string) - .buildWithId(undefined, contextExternalToolId); - const logoUrl = 'logoUrl'; - - contextExternalToolService.findById.mockResolvedValueOnce(contextExternalTool); - schoolExternalToolService.findById.mockResolvedValueOnce(schoolExternalTool); - externalToolService.findById.mockResolvedValueOnce(externalTool); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); - externalToolLogoService.buildLogoUrl.mockReturnValue(logoUrl); - - schoolExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - toolFeatures.toolStatusWithoutVersions = true; - - return { - contextExternalToolId, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should determine the tool status through validation', async () => { - const { contextExternalToolId, schoolExternalTool } = setup(); - - await service.getToolReference(contextExternalToolId); - - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); - expect(contextExternalToolValidationService.validate).not.toHaveBeenCalled(); - }); - - it('should get the outdated status', async () => { - const { contextExternalToolId } = setup(); - - const toolReference: ToolReference = await service.getToolReference(contextExternalToolId); - - expect(toolReference).toEqual(expect.objectContaining({ status: ToolConfigurationStatus.OUTDATED })); - }); - }); }); }); 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 ffaca189acb..1f677e3e159 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,4 +1,4 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolLogoService, ExternalToolService } from '../../external-tool/service'; @@ -7,7 +7,6 @@ import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool, ToolReference } from '../domain'; import { ToolReferenceMapper } from '../mapper'; import { ContextExternalToolService } from './context-external-tool.service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { ToolVersionService } from './tool-version-service'; @Injectable() @@ -17,7 +16,6 @@ export class ToolReferenceService { private readonly schoolExternalToolService: SchoolExternalToolService, private readonly contextExternalToolService: ContextExternalToolService, private readonly externalToolLogoService: ExternalToolLogoService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, private readonly toolVersionService: ToolVersionService ) {} 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 new file mode 100644 index 00000000000..dbc5ea7fe77 --- /dev/null +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts @@ -0,0 +1,168 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { Test, TestingModule } from '@nestjs/testing'; +import { contextExternalToolFactory, externalToolFactory, schoolExternalToolFactory } from '@shared/testing'; +import { ApiValidationError } from '@shared/common'; +import { SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { ToolVersionService } from './tool-version-service'; +import { ContextExternalToolValidationService } from './context-external-tool-validation.service'; +import { CommonToolService } from '../../common/service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolConfigurationStatus } from '../../common/enum'; + +describe('ToolVersionService', () => { + let module: TestingModule; + let service: ToolVersionService; + + let contextExternalToolValidationService: DeepMocked; + let schoolExternalToolValidationService: DeepMocked; + let commonToolService: DeepMocked; + let toolFeatures: DeepMocked; + + beforeAll(async () => { + module = await Test.createTestingModule({ + providers: [ + ToolVersionService, + { + provide: ContextExternalToolValidationService, + useValue: createMock(), + }, + { + provide: SchoolExternalToolValidationService, + useValue: createMock(), + }, + { + provide: CommonToolService, + useValue: createMock(), + }, + { + provide: ToolFeatures, + useValue: { + toolStatusWithoutVersions: false, + }, + }, + ], + }).compile(); + + service = module.get(ToolVersionService); + contextExternalToolValidationService = module.get(ContextExternalToolValidationService); + schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); + commonToolService = module.get(CommonToolService); + toolFeatures = module.get(ToolFeatures); + }); + + afterAll(async () => { + await module.close(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('determineToolConfigurationStatus', () => { + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is false', () => { + const setup = () => { + const externalTool = externalToolFactory.buildWithId(); + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + toolId: externalTool.id as string, + }); + const contextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .buildWithId(); + + toolFeatures.toolStatusWithoutVersions = false; + + return { + externalTool, + schoolExternalTool, + contextExternalTool, + }; + }; + + it('should call CommonToolService', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + + expect(commonToolService.determineToolConfigurationStatus).toHaveBeenCalledWith( + externalTool, + schoolExternalTool, + contextExternalTool + ); + }); + }); + + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation runs through', () => { + const setup = () => { + const externalTool = externalToolFactory.buildWithId(); + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + toolId: externalTool.id as string, + }); + const contextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .buildWithId(); + + toolFeatures.toolStatusWithoutVersions = true; + + schoolExternalToolValidationService.validate.mockResolvedValue(); + contextExternalToolValidationService.validate.mockResolvedValueOnce(); + + return { + externalTool, + schoolExternalTool, + contextExternalTool, + }; + }; + + it('should return latest tool status', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + const status: ToolConfigurationStatus = await service.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(status).toEqual(ToolConfigurationStatus.LATEST); + }); + }); + + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation throws an error', () => { + const setup = () => { + const externalTool = externalToolFactory.buildWithId(); + const schoolExternalTool = schoolExternalToolFactory.buildWithId({ + toolId: externalTool.id as string, + }); + const contextExternalTool = contextExternalToolFactory + .withSchoolExternalToolRef(schoolExternalTool.id as string) + .buildWithId(); + + toolFeatures.toolStatusWithoutVersions = true; + + schoolExternalToolValidationService.validate.mockResolvedValue(); + contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); + + return { + externalTool, + schoolExternalTool, + contextExternalTool, + }; + }; + + it('should return outdated tool status', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + const status: ToolConfigurationStatus = await service.determineToolConfigurationStatus( + externalTool, + schoolExternalTool, + contextExternalTool + ); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); + expect(status).toEqual(ToolConfigurationStatus.OUTDATED); + }); + }); + }); +}); 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 ec4d9848d47..16e8e093e9e 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 @@ -7,14 +7,12 @@ import { externalToolFactory, schoolExternalToolFactory, } from '@shared/testing'; -import { ApiValidationError } from '@shared/common'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; -import { CommonToolService } from '../../common/service'; import { ContextExternalTool } from '../../context-external-tool/domain'; import { BasicToolConfig, ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { SchoolExternalToolService, SchoolExternalToolValidationService } from '../../school-external-tool/service'; +import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ToolStatusOutdatedLoggableException } from '../error'; import { LaunchRequestMethod, ToolLaunchData, ToolLaunchDataType, ToolLaunchRequest } from '../types'; import { @@ -24,8 +22,7 @@ import { OAuth2ToolLaunchStrategy, } from './launch-strategy'; import { ToolLaunchService } from './tool-launch.service'; -import { ContextExternalToolValidationService } from '../../context-external-tool/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolVersionService } from '../../context-external-tool/service/tool-version-service'; describe('ToolLaunchService', () => { let module: TestingModule; @@ -34,10 +31,7 @@ describe('ToolLaunchService', () => { let schoolExternalToolService: DeepMocked; let externalToolService: DeepMocked; let basicToolLaunchStrategy: DeepMocked; - let commonToolService: DeepMocked; - let contextExternalToolValidationService: DeepMocked; - let schoolExternalToolValidationService: DeepMocked; - let toolFeatures: DeepMocked; + let toolVersionService: DeepMocked; beforeEach(async () => { module = await Test.createTestingModule({ @@ -64,22 +58,8 @@ describe('ToolLaunchService', () => { useValue: createMock(), }, { - provide: CommonToolService, - useValue: createMock(), - }, - { - provide: ContextExternalToolValidationService, - useValue: createMock(), - }, - { - provide: SchoolExternalToolValidationService, - useValue: createMock(), - }, - { - provide: ToolFeatures, - useValue: { - toolStatusWithoutVersions: false, - }, + provide: ToolVersionService, + useValue: createMock(), }, ], }).compile(); @@ -88,10 +68,7 @@ describe('ToolLaunchService', () => { schoolExternalToolService = module.get(SchoolExternalToolService); externalToolService = module.get(ExternalToolService); basicToolLaunchStrategy = module.get(BasicToolLaunchStrategy); - commonToolService = module.get(CommonToolService); - contextExternalToolValidationService = module.get(ContextExternalToolValidationService); - schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); - toolFeatures = module.get(ToolFeatures); + toolVersionService = module.get(ToolVersionService); }); afterAll(async () => { @@ -130,9 +107,7 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); - - toolFeatures.toolStatusWithoutVersions = false; + toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce(ToolConfigurationStatus.LATEST); return { launchDataDO, @@ -172,12 +147,12 @@ describe('ToolLaunchService', () => { expect(externalToolService.findById).toHaveBeenCalledWith(launchParams.schoolExternalTool.toolId); }); - it('should call determineToolConfigurationStatus', async () => { + it('should call toolVersionService', async () => { const { launchParams } = setup(); await service.getLaunchData('userId', launchParams.contextExternalTool); - expect(commonToolService.determineToolConfigurationStatus).toHaveBeenCalled(); + expect(toolVersionService.determineToolConfigurationStatus).toHaveBeenCalled(); }); }); @@ -198,9 +173,7 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); - - toolFeatures.toolStatusWithoutVersions = false; + toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce(ToolConfigurationStatus.LATEST); return { launchParams, @@ -245,9 +218,7 @@ describe('ToolLaunchService', () => { schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); externalToolService.findById.mockResolvedValue(externalTool); basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); - - toolFeatures.toolStatusWithoutVersions = false; + toolVersionService.determineToolConfigurationStatus.mockResolvedValueOnce(ToolConfigurationStatus.OUTDATED); return { launchParams, @@ -264,68 +235,6 @@ describe('ToolLaunchService', () => { await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); }); }); - - // TODO N21-1337 refactor after feature flag is removed - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true', () => { - const setup = () => { - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId(); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory - .withSchoolExternalToolRef(schoolExternalTool.id as string) - .build(); - const basicToolConfigDO: BasicToolConfig = basicToolConfigFactory.build(); - const externalTool: ExternalTool = externalToolFactory.build({ - config: basicToolConfigDO, - }); - - const launchDataDO: ToolLaunchData = new ToolLaunchData({ - type: ToolLaunchDataType.BASIC, - baseUrl: 'https://www.basic-baseurl.com/', - properties: [], - openNewTab: false, - }); - - const userId = 'userId'; - - const launchParams: IToolLaunchParams = { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - - schoolExternalToolService.findById.mockResolvedValue(schoolExternalTool); - externalToolService.findById.mockResolvedValue(externalTool); - basicToolLaunchStrategy.createLaunchData.mockResolvedValue(launchDataDO); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.OUTDATED); - commonToolService.determineToolConfigurationStatus.mockReturnValueOnce(ToolConfigurationStatus.LATEST); - - contextExternalToolValidationService.validate.mockRejectedValueOnce(ApiValidationError); - contextExternalToolValidationService.validate.mockResolvedValueOnce(); - toolFeatures.toolStatusWithoutVersions = true; - - return { - launchParams, - userId, - contextExternalToolId: contextExternalTool.id as string, - }; - }; - - it('should throw ToolStatusOutdatedLoggableException', async () => { - const { launchParams, userId, contextExternalToolId } = setup(); - - const func = () => service.getLaunchData(userId, launchParams.contextExternalTool); - - await expect(func).rejects.toThrow(new ToolStatusOutdatedLoggableException(userId, contextExternalToolId)); - }); - - it('should should call validate', async () => { - const { launchParams } = setup(); - - await service.getLaunchData('userId', launchParams.contextExternalTool); - - expect(schoolExternalToolValidationService.validate).toHaveBeenCalled(); - expect(contextExternalToolValidationService.validate).toHaveBeenCalled(); - }); - }); }); describe('generateLaunchRequest', () => { 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 fe5820deb6d..5b090f9778a 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 @@ -1,4 +1,4 @@ -import { Inject, Injectable, InternalServerErrorException } from '@nestjs/common'; +import { Injectable, InternalServerErrorException } from '@nestjs/common'; import { EntityId } from '@shared/domain'; import { ToolConfigType, ToolConfigurationStatus } from '../../common/enum'; import { ContextExternalTool } from '../../context-external-tool/domain'; @@ -15,7 +15,6 @@ import { Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrategy, } from './launch-strategy'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { ToolVersionService } from '../../context-external-tool/service/tool-version-service'; @Injectable() @@ -28,8 +27,7 @@ export class ToolLaunchService { private readonly basicToolLaunchStrategy: BasicToolLaunchStrategy, private readonly lti11ToolLaunchStrategy: Lti11ToolLaunchStrategy, private readonly oauth2ToolLaunchStrategy: OAuth2ToolLaunchStrategy, - private readonly toolVersionService: ToolVersionService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + private readonly toolVersionService: ToolVersionService ) { this.strategies = new Map(); this.strategies.set(ToolConfigType.BASIC, basicToolLaunchStrategy); diff --git a/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts b/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts index d214ef41436..b8ff7623586 100644 --- a/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts +++ b/apps/server/src/modules/tool/tool-launch/tool-launch.module.ts @@ -16,7 +16,6 @@ import { AutoSchoolNumberStrategy, } from './service/auto-parameter-strategy'; import { BasicToolLaunchStrategy, Lti11ToolLaunchStrategy, OAuth2ToolLaunchStrategy } from './service/launch-strategy'; -import { ToolConfigModule } from '../tool-config.module'; @Module({ imports: [ @@ -29,7 +28,6 @@ import { ToolConfigModule } from '../tool-config.module'; forwardRef(() => PseudonymModule), // i do not like this solution, the root problem is on other place but not detectable for me LearnroomModule, BoardModule, - ToolConfigModule, ], providers: [ ToolLaunchService, From dc3bf1d38a3e7c1ebdc2784af9411fe864b605e0 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Mon, 13 Nov 2023 13:37:36 +0100 Subject: [PATCH 09/10] fix unit test and merge main --- .../uc/context-external-tool.uc.spec.ts | 3 +- .../school-external-tool.service.spec.ts | 130 ++++++++++++++---- 2 files changed, 105 insertions(+), 28 deletions(-) diff --git a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.spec.ts b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.spec.ts index 0fcca404049..9239e4db2a9 100644 --- a/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.spec.ts +++ b/apps/server/src/modules/tool/context-external-tool/uc/context-external-tool.uc.spec.ts @@ -16,7 +16,8 @@ import { ToolPermissionHelper } from '../../common/uc/tool-permission-helper'; import { SchoolExternalTool } from '../../school-external-tool/domain'; import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool } from '../domain'; -import { ContextExternalToolService, ContextExternalToolValidationService } from '../service'; +import { ContextExternalToolService } from '../service'; +import { ContextExternalToolValidationService } from '../service/context-external-tool-validation.service'; import { ContextExternalToolUc } from './context-external-tool.uc'; describe('ContextExternalToolUc', () => { diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts index 52f9b0a4c02..819e0dfcbf8 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts @@ -3,11 +3,14 @@ import { Test, TestingModule } from '@nestjs/testing'; import { SchoolExternalToolRepo } from '@shared/repo'; import { externalToolFactory } from '@shared/testing/factory/domainobject/tool/external-tool.factory'; import { schoolExternalToolFactory } from '@shared/testing/factory/domainobject/tool/school-external-tool.factory'; +import { ApiValidationError } from '@shared/common'; import { ToolConfigurationStatus } from '../../common/enum'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolService } from './school-external-tool.service'; +import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; describe('SchoolExternalToolService', () => { let module: TestingModule; @@ -15,6 +18,8 @@ describe('SchoolExternalToolService', () => { let schoolExternalToolRepo: DeepMocked; let externalToolService: DeepMocked; + let schoolExternalToolValidationService: DeepMocked; + let toolFearures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -28,19 +33,32 @@ describe('SchoolExternalToolService', () => { provide: ExternalToolService, useValue: createMock(), }, + { + provide: SchoolExternalToolValidationService, + useValue: createMock(), + }, + { + provide: ToolFeatures, + useValue: { + toolStatusWithoutVersions: false, + }, + }, ], }).compile(); service = module.get(SchoolExternalToolService); schoolExternalToolRepo = module.get(SchoolExternalToolRepo); externalToolService = module.get(ExternalToolService); + schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); + toolFearures = module.get(ToolFeatures); }); - const setup = () => { + const legacySetup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); const externalTool: ExternalTool = externalToolFactory.buildWithId(); schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + toolFearures.toolStatusWithoutVersions = false; return { schoolExternalTool, @@ -52,7 +70,7 @@ describe('SchoolExternalToolService', () => { describe('findSchoolExternalTools', () => { describe('when called with query', () => { it('should call repo with query', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool } = legacySetup(); await service.findSchoolExternalTools(schoolExternalTool); @@ -60,7 +78,7 @@ describe('SchoolExternalToolService', () => { }); it('should return schoolExternalTool array', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool } = legacySetup(); schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool, schoolExternalTool]); const result: SchoolExternalTool[] = await service.findSchoolExternalTools(schoolExternalTool); @@ -72,7 +90,7 @@ describe('SchoolExternalToolService', () => { describe('enrichDataFromExternalTool', () => { it('should call the externalToolService', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool } = legacySetup(); schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); await service.findSchoolExternalTools(schoolExternalTool); @@ -81,44 +99,102 @@ describe('SchoolExternalToolService', () => { }); describe('when determine status', () => { - describe('when external tool version is greater', () => { - it('should return status outdated', async () => { - const { schoolExternalTool, externalTool } = setup(); - externalTool.version = 1337; - schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); - externalToolService.findById.mockResolvedValue(externalTool); + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is false', () => { + describe('when external tool version is greater', () => { + it('should return status outdated', async () => { + const { schoolExternalTool, externalTool } = legacySetup(); + externalTool.version = 1337; + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); + + const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools( + schoolExternalTool + ); + + expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.OUTDATED); + }); + }); - const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools(schoolExternalTool); + describe('when external tool version is lower', () => { + it('should return status latest', async () => { + const { schoolExternalTool, externalTool } = legacySetup(); + schoolExternalTool.toolVersion = 1; + externalTool.version = 0; + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); - expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.OUTDATED); + const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools( + schoolExternalTool + ); + + expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.LATEST); + }); + }); + + describe('when external tool version is equal', () => { + it('should return status latest', async () => { + const { schoolExternalTool, externalTool } = legacySetup(); + schoolExternalTool.toolVersion = 1; + externalTool.version = 1; + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); + + const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools( + schoolExternalTool + ); + + expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.LATEST); + }); }); }); - describe('when external tool version is lower', () => { - it('should return status latest', async () => { - const { schoolExternalTool, externalTool } = setup(); - schoolExternalTool.toolVersion = 1; - externalTool.version = 0; + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation goes through', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); + schoolExternalToolValidationService.validate.mockResolvedValue(); + toolFearures.toolStatusWithoutVersions = true; + + return { + schoolExternalTool, + }; + }; + + it('should return latest tool status', async () => { + const { schoolExternalTool } = setup(); const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools(schoolExternalTool); + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.LATEST); }); }); - describe('when external tool version is equal', () => { - it('should return status latest', async () => { - const { schoolExternalTool, externalTool } = setup(); - schoolExternalTool.toolVersion = 1; - externalTool.version = 1; + describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation throws error', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); + schoolExternalToolValidationService.validate.mockRejectedValue(ApiValidationError); + toolFearures.toolStatusWithoutVersions = true; + + return { + schoolExternalTool, + }; + }; + + it('should return outdated tool status', async () => { + const { schoolExternalTool } = setup(); const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools(schoolExternalTool); - expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.LATEST); + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + expect(schoolExternalToolDOs[0].status).toEqual(ToolConfigurationStatus.OUTDATED); }); }); }); @@ -127,7 +203,7 @@ describe('SchoolExternalToolService', () => { describe('deleteSchoolExternalToolById', () => { describe('when schoolExternalToolId is given', () => { it('should call the schoolExternalToolRepo', async () => { - const { schoolExternalToolId } = setup(); + const { schoolExternalToolId } = legacySetup(); await service.deleteSchoolExternalToolById(schoolExternalToolId); @@ -139,7 +215,7 @@ describe('SchoolExternalToolService', () => { describe('findById', () => { describe('when schoolExternalToolId is given', () => { it('should call schoolExternalToolRepo.findById', async () => { - const { schoolExternalToolId } = setup(); + const { schoolExternalToolId } = legacySetup(); await service.findById(schoolExternalToolId); @@ -151,7 +227,7 @@ describe('SchoolExternalToolService', () => { describe('saveSchoolExternalTool', () => { describe('when schoolExternalTool is given', () => { it('should call schoolExternalToolRepo.save', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool } = legacySetup(); await service.saveSchoolExternalTool(schoolExternalTool); @@ -159,7 +235,7 @@ describe('SchoolExternalToolService', () => { }); it('should enrich data from externalTool', async () => { - const { schoolExternalTool } = setup(); + const { schoolExternalTool } = legacySetup(); await service.saveSchoolExternalTool(schoolExternalTool); From 9e1c878f30121e187192379774ceefe438d8ba15 Mon Sep 17 00:00:00 2001 From: Igor Richter Date: Tue, 14 Nov 2023 10:53:36 +0100 Subject: [PATCH 10/10] requested changes --- .../common/service/common-tool.service.ts | 4 +++ .../service/tool-version-service.spec.ts | 34 +++++++++++++++++-- .../school-external-tool.service.spec.ts | 1 + apps/server/src/modules/tool/tool-config.ts | 4 +-- 4 files changed, 38 insertions(+), 5 deletions(-) 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 71f643c81ac..1dccc42ab1f 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 @@ -5,8 +5,12 @@ import { ContextExternalTool } from '../../context-external-tool/domain'; import { ToolConfigurationStatus } from '../enum'; import { ToolVersion } from '../interface'; +// TODO N21-1337 remove class when tool versioning is removed @Injectable() export class CommonToolService { + /** + * @deprecated use ToolVersionService + */ determineToolConfigurationStatus( externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool, 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 dbc5ea7fe77..33c77ecb7ea 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 @@ -122,10 +122,24 @@ describe('ToolVersionService', () => { contextExternalTool ); - expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); - expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); expect(status).toEqual(ToolConfigurationStatus.LATEST); }); + + it('should call schoolExternalToolValidationService', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + }); + + it('should call contextExternalToolValidationService', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + }); }); describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation throws an error', () => { @@ -159,9 +173,23 @@ describe('ToolVersionService', () => { contextExternalTool ); + expect(status).toEqual(ToolConfigurationStatus.OUTDATED); + }); + + it('should call schoolExternalToolValidationService', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + expect(schoolExternalToolValidationService.validate).toHaveBeenCalledWith(schoolExternalTool); + }); + + it('should call contextExternalToolValidationService', async () => { + const { externalTool, schoolExternalTool, contextExternalTool } = setup(); + + await service.determineToolConfigurationStatus(externalTool, schoolExternalTool, contextExternalTool); + expect(contextExternalToolValidationService.validate).toHaveBeenCalledWith(contextExternalTool); - expect(status).toEqual(ToolConfigurationStatus.OUTDATED); }); }); }); diff --git a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts index 819e0dfcbf8..b8c2789322d 100644 --- a/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts +++ b/apps/server/src/modules/tool/school-external-tool/service/school-external-tool.service.spec.ts @@ -53,6 +53,7 @@ describe('SchoolExternalToolService', () => { toolFearures = module.get(ToolFeatures); }); + // TODO N21-1337 refactor setup into the describe blocks const legacySetup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); const externalTool: ExternalTool = externalToolFactory.buildWithId(); diff --git a/apps/server/src/modules/tool/tool-config.ts b/apps/server/src/modules/tool/tool-config.ts index 2b0af584e53..1405f0c7a1d 100644 --- a/apps/server/src/modules/tool/tool-config.ts +++ b/apps/server/src/modules/tool/tool-config.ts @@ -6,7 +6,7 @@ export interface IToolFeatures { ctlToolsTabEnabled: boolean; ltiToolsTabEnabled: boolean; contextConfigurationEnabled: boolean; - // TODO N21-1337 + // TODO N21-1337 refactor after feature flag is removed toolStatusWithoutVersions: boolean; maxExternalToolLogoSizeInBytes: number; backEndUrl: string; @@ -17,7 +17,7 @@ export default class ToolConfiguration { ctlToolsTabEnabled: Configuration.get('FEATURE_CTL_TOOLS_TAB_ENABLED') as boolean, ltiToolsTabEnabled: Configuration.get('FEATURE_LTI_TOOLS_TAB_ENABLED') as boolean, contextConfigurationEnabled: Configuration.get('FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED') as boolean, - // TODO N21-1337 + // TODO N21-1337 refactor after feature flag is removed 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,