From b15d6412003731bac8e8c04331651463d4e56e33 Mon Sep 17 00:00:00 2001 From: agnisa-cap Date: Mon, 15 Apr 2024 16:03:57 +0200 Subject: [PATCH] N21-1337 removes deprecated ctl versioning (#4927) * N21-1337 removes the deprecated feature flags: FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED and FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED --- .../modules/server/api/dto/config.response.ts | 4 - .../server/api/test/server.api.spec.ts | 1 - .../service/common-tool.service.spec.ts | 304 +----------------- .../common/service/common-tool.service.ts | 43 --- .../context-external-tool.module.ts | 6 +- ...tool-configuration-status.service.spec.ts} | 91 +----- .../tool-configuration-status.service.ts | 64 ++++ .../service/tool-reference.service.spec.ts | 10 +- .../service/tool-reference.service.ts | 4 +- .../service/tool-version-service.ts | 81 ----- ...xternal-tool-configuration.service.spec.ts | 73 +---- .../external-tool-configuration.service.ts | 33 +- .../uc/external-tool-configuration.uc.spec.ts | 31 -- .../uc/external-tool-configuration.uc.ts | 11 +- ...l-external-tool-validation.service.spec.ts | 36 --- ...school-external-tool-validation.service.ts | 18 +- .../school-external-tool.service.spec.ts | 177 +++++----- .../service/school-external-tool.service.ts | 25 +- apps/server/src/modules/tool/tool-config.ts | 6 - .../service/tool-launch.service.spec.ts | 10 +- .../service/tool-launch.service.ts | 4 +- 21 files changed, 187 insertions(+), 845 deletions(-) rename apps/server/src/modules/tool/context-external-tool/service/{tool-version-service.spec.ts => tool-configuration-status.service.spec.ts} (80%) create mode 100644 apps/server/src/modules/tool/context-external-tool/service/tool-configuration-status.service.ts delete mode 100644 apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts diff --git a/apps/server/src/modules/server/api/dto/config.response.ts b/apps/server/src/modules/server/api/dto/config.response.ts index 2c7e0df485f..163dd9bf6e6 100644 --- a/apps/server/src/modules/server/api/dto/config.response.ts +++ b/apps/server/src/modules/server/api/dto/config.response.ts @@ -26,9 +26,6 @@ export class ConfigResponse { @ApiProperty() FEATURE_ENABLE_LDAP_SYNC_DURING_MIGRATION: boolean; - @ApiProperty() - FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED: boolean; - @ApiProperty() CTL_TOOLS_RELOAD_TIME_MS: number; @@ -251,7 +248,6 @@ export class ConfigResponse { this.FEATURE_LTI_TOOLS_TAB_ENABLED = config.ltiToolsTabEnabled; this.FEATURE_SHOW_OUTDATED_USERS = config.FEATURE_SHOW_OUTDATED_USERS; this.FEATURE_ENABLE_LDAP_SYNC_DURING_MIGRATION = config.FEATURE_ENABLE_LDAP_SYNC_DURING_MIGRATION; - this.FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED = config.contextConfigurationEnabled; this.CTL_TOOLS_RELOAD_TIME_MS = config.ctlToolsReloadTimeMs; this.FEATURE_SHOW_NEW_CLASS_VIEW_ENABLED = config.FEATURE_SHOW_NEW_CLASS_VIEW_ENABLED; this.FEATURE_CTL_TOOLS_COPY_ENABLED = config.ctlToolsCopyEnabled; diff --git a/apps/server/src/modules/server/api/test/server.api.spec.ts b/apps/server/src/modules/server/api/test/server.api.spec.ts index 2785dbbd871..2a271e7d256 100644 --- a/apps/server/src/modules/server/api/test/server.api.spec.ts +++ b/apps/server/src/modules/server/api/test/server.api.spec.ts @@ -49,7 +49,6 @@ describe('Server Controller (API)', () => { 'FEATURE_CONSENT_NECESSARY', 'FEATURE_COPY_SERVICE_ENABLED', 'FEATURE_COURSE_SHARE', - 'FEATURE_CTL_CONTEXT_CONFIGURATION_ENABLED', 'FEATURE_CTL_TOOLS_COPY_ENABLED', 'FEATURE_CTL_TOOLS_TAB_ENABLED', 'FEATURE_ENABLE_LDAP_SYNC_DURING_MIGRATION', diff --git a/apps/server/src/modules/tool/common/service/common-tool.service.spec.ts b/apps/server/src/modules/tool/common/service/common-tool.service.spec.ts index a47489f0892..b828c2ee8fc 100644 --- a/apps/server/src/modules/tool/common/service/common-tool.service.spec.ts +++ b/apps/server/src/modules/tool/common/service/common-tool.service.spec.ts @@ -1,16 +1,8 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { - contextExternalToolFactory, - externalToolFactory, - schoolExternalToolFactory, - toolConfigurationStatusFactory, -} from '@shared/testing'; -import { CommonToolService } from './common-tool.service'; +import { externalToolFactory } from '@shared/testing'; import { ExternalTool } from '../../external-tool/domain'; -import { SchoolExternalTool } from '../../school-external-tool/domain'; import { ToolContextType } from '../enum'; -import { ContextExternalToolConfigurationStatus } from '../domain'; -import { ContextExternalTool } from '../../context-external-tool/domain'; +import { CommonToolService } from './common-tool.service'; describe('CommonToolService', () => { let module: TestingModule; @@ -28,298 +20,6 @@ describe('CommonToolService', () => { await module.close(); }); - describe('determineToolConfigurationStatus', () => { - describe('when all versions are equal', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 0 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 0 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 0 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with latest true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - }) - ); - }); - }); - - describe('when externalTool version is greater than schoolExternalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 0 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 0 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return outdated status for school level', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: true, - isDeactivated: false, - }) - ); - }); - }); - - describe('when schoolExternalTool version is greater than contextExternalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 0 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return outdated status for context level', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: true, - isDeactivated: false, - }) - ); - }); - }); - - describe('when externalTool version is greater than contextExternalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 0 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return outdated status for context and school level', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: true, - }) - ); - }); - }); - - describe('when contextExternalTool version is greater than schoolExternalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 2 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with latest true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - }) - ); - }); - }); - - describe('when contextExternalTool version is greater than externalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 2 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with latest true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - isDeactivated: false, - }) - ); - }); - }); - - describe('when schoolExternalTool version is greater than externalTool version', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 2 }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 2 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with latest true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - isDeactivated: false, - }) - ); - }); - }); - - describe('when schoolExternalTool is deactivated', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1 }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ - toolVersion: 2, - status: { isDeactivated: true }, - }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 2 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with deactivated true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - isDeactivated: true, - }) - ); - }); - }); - - describe('when externalTool is deactivated', () => { - const setup = () => { - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 1, isDeactivated: true }); - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ - toolVersion: 2, - }); - const contextExternalTool: ContextExternalTool = contextExternalToolFactory.buildWithId({ toolVersion: 2 }); - - return { - externalTool, - schoolExternalTool, - contextExternalTool, - }; - }; - - it('should return a configuration status with deactivated true', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - const result: ContextExternalToolConfigurationStatus = service.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - expect(result).toEqual( - toolConfigurationStatusFactory.build({ - isOutdatedOnScopeContext: false, - isOutdatedOnScopeSchool: false, - isDeactivated: true, - }) - ); - }); - }); - }); - describe('isContextRestricted', () => { describe('when tool is not restricted to context', () => { const setup = () => { 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 b33982b3c3f..0ef0e368169 100644 --- a/apps/server/src/modules/tool/common/service/common-tool.service.ts +++ b/apps/server/src/modules/tool/common/service/common-tool.service.ts @@ -1,52 +1,9 @@ import { Injectable } from '@nestjs/common'; -import { ContextExternalTool } from '../../context-external-tool/domain'; import { ExternalTool } from '../../external-tool/domain'; -import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { ContextExternalToolConfigurationStatus } from '../domain'; import { ToolContextType } from '../enum'; -import { ToolVersion } from '../interface'; -// TODO N21-1337 remove class when tool versioning is removed @Injectable() export class CommonToolService { - /** - * @deprecated use ToolVersionService - */ - public determineToolConfigurationStatus( - externalTool: ExternalTool, - schoolExternalTool: SchoolExternalTool, - contextExternalTool: ContextExternalTool - ): ContextExternalToolConfigurationStatus { - const configurationStatus: ContextExternalToolConfigurationStatus = new ContextExternalToolConfigurationStatus({ - isOutdatedOnScopeContext: true, - isOutdatedOnScopeSchool: true, - isIncompleteOnScopeContext: false, - isDeactivated: false, - }); - - if ( - this.isLatest(schoolExternalTool, externalTool) && - this.isLatest(contextExternalTool, schoolExternalTool) && - this.isLatest(contextExternalTool, externalTool) - ) { - configurationStatus.isOutdatedOnScopeContext = false; - configurationStatus.isOutdatedOnScopeSchool = false; - } else { - configurationStatus.isOutdatedOnScopeContext = true; - configurationStatus.isOutdatedOnScopeSchool = true; - } - - if (externalTool.isDeactivated || schoolExternalTool.status?.isDeactivated) { - configurationStatus.isDeactivated = true; - } - - return configurationStatus; - } - - private isLatest(tool1: ToolVersion, tool2: ToolVersion): boolean { - return tool1.getVersion() >= tool2.getVersion(); - } - public isContextRestricted(externalTool: ExternalTool, context: ToolContextType): boolean { if (externalTool.restrictToContexts?.length && !externalTool.restrictToContexts.includes(context)) { return true; 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 102e033cb6b..da76ff8544e 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 @@ -6,7 +6,7 @@ import { SchoolExternalToolModule } from '../school-external-tool'; import { ToolConfigModule } from '../tool-config.module'; import { ContextExternalToolAuthorizableService, ContextExternalToolService, ToolReferenceService } from './service'; import { ContextExternalToolValidationService } from './service/context-external-tool-validation.service'; -import { ToolVersionService } from './service/tool-version-service'; +import { ToolConfigurationStatusService } from './service/tool-configuration-status.service'; @Module({ imports: [ @@ -21,14 +21,14 @@ import { ToolVersionService } from './service/tool-version-service'; ContextExternalToolValidationService, ContextExternalToolAuthorizableService, ToolReferenceService, - ToolVersionService, + ToolConfigurationStatusService, ], exports: [ ContextExternalToolService, ContextExternalToolValidationService, ContextExternalToolAuthorizableService, ToolReferenceService, - ToolVersionService, + ToolConfigurationStatusService, ], }) export class ContextExternalToolModule {} 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-configuration-status.service.spec.ts similarity index 80% rename from apps/server/src/modules/tool/context-external-tool/service/tool-version-service.spec.ts rename to apps/server/src/modules/tool/context-external-tool/service/tool-configuration-status.service.spec.ts index b9bf9c86184..e8d546c8d9b 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-configuration-status.service.spec.ts @@ -12,43 +12,28 @@ import { ToolParameterDuplicateLoggableException, ToolParameterValueMissingLoggableException, } from '../../common/domain'; -import { CommonToolService, CommonToolValidationService } from '../../common/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; -import { ToolVersionService } from './tool-version-service'; +import { CommonToolValidationService } from '../../common/service'; +import { ToolConfigurationStatusService } from './tool-configuration-status.service'; -describe('ToolVersionService', () => { +describe(ToolConfigurationStatusService.name, () => { let module: TestingModule; - let service: ToolVersionService; + let service: ToolConfigurationStatusService; let commonToolValidationService: DeepMocked; - let commonToolService: DeepMocked; - let toolFeatures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ providers: [ - ToolVersionService, + ToolConfigurationStatusService, { provide: CommonToolValidationService, useValue: createMock(), }, - { - provide: CommonToolService, - useValue: createMock(), - }, - { - provide: ToolFeatures, - useValue: { - toolStatusWithoutVersions: false, - }, - }, ], }).compile(); - service = module.get(ToolVersionService); + service = module.get(ToolConfigurationStatusService); commonToolValidationService = module.get(CommonToolValidationService); - commonToolService = module.get(CommonToolService); - toolFeatures = module.get(ToolFeatures); }); afterAll(async () => { @@ -60,39 +45,7 @@ describe('ToolVersionService', () => { }); 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', () => { - const { externalTool, schoolExternalTool, contextExternalTool } = setup(); - - 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', () => { + describe('when validation runs through', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -102,8 +55,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValue([]); return { @@ -147,7 +98,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation of SchoolExternalTool throws an error', () => { + describe('when validation of SchoolExternalTool throws an error', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -157,8 +108,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); commonToolValidationService.validateParameters.mockReturnValueOnce([]); @@ -203,7 +152,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation of ContextExternalTool throws an error', () => { + describe('when validation of ContextExternalTool throws an error', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -213,8 +162,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([]); commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); @@ -259,7 +206,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation of SchoolExternalTool and ContextExternalTool throws an error', () => { + describe('when validation of SchoolExternalTool and ContextExternalTool throws an error', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -269,8 +216,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); @@ -315,7 +260,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation of ContextExternalTool throws at least 1 missing value errors', () => { + describe('when validation of ContextExternalTool throws at least 1 missing value errors', () => { const setup = () => { const customParameter = customParameterFactory.build(); const externalTool = externalToolFactory.buildWithId({ parameters: [customParameter] }); @@ -326,8 +271,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([]); commonToolValidationService.validateParameters.mockReturnValueOnce([ new ToolParameterValueMissingLoggableException(customParameter), @@ -359,7 +302,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and SchoolExternalTool is deactivated', () => { + describe('when SchoolExternalTool is deactivated', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -370,8 +313,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); @@ -400,7 +341,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and externalTool is deactivated', () => { + describe('when externalTool is deactivated', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId({ isDeactivated: true }); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -410,8 +351,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); @@ -440,7 +379,7 @@ describe('ToolVersionService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true, externalTool and schoolExternalTool are not deactivated', () => { + describe('when externalTool and schoolExternalTool are not deactivated', () => { const setup = () => { const externalTool = externalToolFactory.buildWithId(); const schoolExternalTool = schoolExternalToolFactory.buildWithId({ @@ -450,8 +389,6 @@ describe('ToolVersionService', () => { .withSchoolExternalToolRef(schoolExternalTool.id as string) .buildWithId(); - toolFeatures.toolStatusWithoutVersions = true; - commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); commonToolValidationService.validateParameters.mockReturnValueOnce([new ValidationError('')]); diff --git a/apps/server/src/modules/tool/context-external-tool/service/tool-configuration-status.service.ts b/apps/server/src/modules/tool/context-external-tool/service/tool-configuration-status.service.ts new file mode 100644 index 00000000000..a5c4868ba25 --- /dev/null +++ b/apps/server/src/modules/tool/context-external-tool/service/tool-configuration-status.service.ts @@ -0,0 +1,64 @@ +import { Injectable } from '@nestjs/common/decorators/core/injectable.decorator'; +import { ValidationError } from '@shared/common'; +import { + ContextExternalToolConfigurationStatus, + ToolParameterValueMissingLoggableException, +} from '../../common/domain'; +import { CommonToolValidationService } from '../../common/service'; +import { ExternalTool } from '../../external-tool/domain'; +import { SchoolExternalTool } from '../../school-external-tool/domain'; +import { ContextExternalTool } from '../domain'; + +@Injectable() +export class ToolConfigurationStatusService { + constructor(private readonly commonToolValidationService: CommonToolValidationService) {} + + public determineToolConfigurationStatus( + externalTool: ExternalTool, + schoolExternalTool: SchoolExternalTool, + contextExternalTool: ContextExternalTool + ): ContextExternalToolConfigurationStatus { + const configurationStatus: ContextExternalToolConfigurationStatus = new ContextExternalToolConfigurationStatus({ + isOutdatedOnScopeContext: false, + isIncompleteOnScopeContext: false, + isOutdatedOnScopeSchool: false, + isDeactivated: this.isToolDeactivated(externalTool, schoolExternalTool), + }); + + const schoolParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( + externalTool, + schoolExternalTool + ); + + if (schoolParameterErrors.length) { + configurationStatus.isOutdatedOnScopeSchool = true; + } + + const contextParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( + externalTool, + contextExternalTool + ); + + if (contextParameterErrors.length) { + configurationStatus.isOutdatedOnScopeContext = true; + + if ( + contextParameterErrors.some( + (error: ValidationError) => error instanceof ToolParameterValueMissingLoggableException + ) + ) { + configurationStatus.isIncompleteOnScopeContext = true; + } + } + + return configurationStatus; + } + + private isToolDeactivated(externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool) { + if (externalTool.isDeactivated || (schoolExternalTool.status && schoolExternalTool.status.isDeactivated)) { + return true; + } + + return false; + } +} 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 f3c31e6e506..69071d4ede8 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 @@ -12,7 +12,7 @@ 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 { ToolVersionService } from './tool-version-service'; +import { ToolConfigurationStatusService } from './tool-configuration-status.service'; describe('ToolReferenceService', () => { let module: TestingModule; @@ -21,7 +21,7 @@ describe('ToolReferenceService', () => { let externalToolService: DeepMocked; let schoolExternalToolService: DeepMocked; let contextExternalToolService: DeepMocked; - let toolVersionService: DeepMocked; + let toolVersionService: DeepMocked; let externalToolLogoService: DeepMocked; beforeAll(async () => { @@ -41,8 +41,8 @@ describe('ToolReferenceService', () => { useValue: createMock(), }, { - provide: ToolVersionService, - useValue: createMock(), + provide: ToolConfigurationStatusService, + useValue: createMock(), }, { provide: ExternalToolLogoService, @@ -55,7 +55,7 @@ describe('ToolReferenceService', () => { externalToolService = module.get(ExternalToolService); schoolExternalToolService = module.get(SchoolExternalToolService); contextExternalToolService = module.get(ContextExternalToolService); - toolVersionService = module.get(ToolVersionService); + toolVersionService = module.get(ToolConfigurationStatusService); externalToolLogoService = module.get(ExternalToolLogoService); }); 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 46ae330e5a0..0d9cae1c23c 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 @@ -8,7 +8,7 @@ import { SchoolExternalToolService } from '../../school-external-tool/service'; import { ContextExternalTool, ToolReference } from '../domain'; import { ToolReferenceMapper } from '../mapper'; import { ContextExternalToolService } from './context-external-tool.service'; -import { ToolVersionService } from './tool-version-service'; +import { ToolConfigurationStatusService } from './tool-configuration-status.service'; @Injectable() export class ToolReferenceService { @@ -17,7 +17,7 @@ export class ToolReferenceService { private readonly schoolExternalToolService: SchoolExternalToolService, private readonly contextExternalToolService: ContextExternalToolService, private readonly externalToolLogoService: ExternalToolLogoService, - private readonly toolVersionService: ToolVersionService + private readonly toolVersionService: ToolConfigurationStatusService ) {} async getToolReference(contextExternalToolId: EntityId): Promise { 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 deleted file mode 100644 index 06677a4b60a..00000000000 --- a/apps/server/src/modules/tool/context-external-tool/service/tool-version-service.ts +++ /dev/null @@ -1,81 +0,0 @@ -import { Inject } from '@nestjs/common'; -import { Injectable } from '@nestjs/common/decorators/core/injectable.decorator'; -import { ValidationError } from '@shared/common'; -import { - ContextExternalToolConfigurationStatus, - ToolParameterValueMissingLoggableException, -} from '../../common/domain'; -import { CommonToolService, CommonToolValidationService } from '../../common/service'; -import { ExternalTool } from '../../external-tool/domain'; -import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; -import { ContextExternalTool } from '../domain'; - -@Injectable() -export class ToolVersionService { - constructor( - private readonly commonToolService: CommonToolService, - private readonly commonToolValidationService: CommonToolValidationService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures - ) {} - - public determineToolConfigurationStatus( - externalTool: ExternalTool, - schoolExternalTool: SchoolExternalTool, - contextExternalTool: ContextExternalTool - ): ContextExternalToolConfigurationStatus { - // TODO N21-1337 remove if statement, when feature flag is removed - if (this.toolFeatures.toolStatusWithoutVersions) { - const configurationStatus: ContextExternalToolConfigurationStatus = new ContextExternalToolConfigurationStatus({ - isOutdatedOnScopeContext: false, - isIncompleteOnScopeContext: false, - isOutdatedOnScopeSchool: false, - isDeactivated: this.isToolDeactivated(externalTool, schoolExternalTool), - }); - - const schoolParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( - externalTool, - schoolExternalTool - ); - - if (schoolParameterErrors.length) { - configurationStatus.isOutdatedOnScopeSchool = true; - } - - const contextParameterErrors: ValidationError[] = this.commonToolValidationService.validateParameters( - externalTool, - contextExternalTool - ); - - if (contextParameterErrors.length) { - configurationStatus.isOutdatedOnScopeContext = true; - - if ( - contextParameterErrors.some( - (error: ValidationError) => error instanceof ToolParameterValueMissingLoggableException - ) - ) { - configurationStatus.isIncompleteOnScopeContext = true; - } - } - - return configurationStatus; - } - - const status: ContextExternalToolConfigurationStatus = this.commonToolService.determineToolConfigurationStatus( - externalTool, - schoolExternalTool, - contextExternalTool - ); - - return status; - } - - private isToolDeactivated(externalTool: ExternalTool, schoolExternalTool: SchoolExternalTool) { - if (externalTool.isDeactivated || (schoolExternalTool.status && schoolExternalTool.status.isDeactivated)) { - return true; - } - - return false; - } -} diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts index ae0722d937c..70de18d757f 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.spec.ts @@ -1,32 +1,28 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { Test, TestingModule } from '@nestjs/testing'; import { Page } from '@shared/domain/domainobject'; import { EntityId } from '@shared/domain/types'; import { - contextExternalToolFactory, customParameterFactory, externalToolFactory, schoolExternalToolFactory, schoolToolConfigurationStatusFactory, setupEntities, } from '@shared/testing'; -import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { CustomParameter } from '../../common/domain'; import { CustomParameterScope, ToolContextType } from '../../common/enum'; -import { ContextExternalTool } from '../../context-external-tool/domain'; +import { CommonToolService } from '../../common/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; +import { ToolFeatures } from '../../tool-config'; import { ExternalTool } from '../domain'; import { ContextExternalToolTemplateInfo } from '../uc'; import { ExternalToolConfigurationService } from './external-tool-configuration.service'; -import { CommonToolService } from '../../common/service'; describe('ExternalToolConfigurationService', () => { let module: TestingModule; let service: ExternalToolConfigurationService; let commonToolService: DeepMocked; - let toolFeatures: IToolFeatures; - beforeAll(async () => { await setupEntities(); @@ -47,7 +43,6 @@ describe('ExternalToolConfigurationService', () => { }).compile(); service = module.get(ExternalToolConfigurationService); - toolFeatures = module.get(ToolFeatures); commonToolService = module.get(CommonToolService); }); @@ -112,68 +107,6 @@ describe('ExternalToolConfigurationService', () => { }); }); - describe('filterForAvailableSchoolExternalTools', () => { - describe('when context configuration is enabled', () => { - const setup = () => { - toolFeatures.contextConfigurationEnabled = true; - const usedSchoolExternalToolId = 'usedSchoolExternalToolId'; - const schoolExternalTools: SchoolExternalTool[] = [ - schoolExternalToolFactory.buildWithId(undefined, usedSchoolExternalToolId), - schoolExternalToolFactory.buildWithId(undefined, 'unusedSchoolExternalToolId'), - ]; - const contextExternalToolsInUse: ContextExternalTool[] = [ - contextExternalToolFactory.withSchoolExternalToolRef(usedSchoolExternalToolId).buildWithId(), - contextExternalToolFactory.buildWithId(undefined, 'unusedContextExternalToolId'), - ]; - - return { schoolExternalTools, contextExternalToolsInUse }; - }; - - it('should include all school external tools', () => { - const { schoolExternalTools, contextExternalToolsInUse } = setup(); - - const result: SchoolExternalTool[] = service.filterForAvailableSchoolExternalTools( - schoolExternalTools, - contextExternalToolsInUse - ); - - expect(result).toEqual(schoolExternalTools); - }); - }); - - describe('when context configuration is disabled', () => { - const setup = () => { - toolFeatures.contextConfigurationEnabled = false; - const usedSchoolExternalToolId = 'usedSchoolExternalToolId'; - const unusedSchoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId( - undefined, - 'unusedSchoolExternalToolId' - ); - const schoolExternalTools: SchoolExternalTool[] = [ - schoolExternalToolFactory.buildWithId(undefined, usedSchoolExternalToolId), - unusedSchoolExternalTool, - ]; - const contextExternalToolsInUse: ContextExternalTool[] = [ - contextExternalToolFactory.withSchoolExternalToolRef(usedSchoolExternalToolId).buildWithId(), - contextExternalToolFactory.buildWithId(undefined, 'unusedContextExternalToolId'), - ]; - - return { schoolExternalTools, contextExternalToolsInUse, unusedSchoolExternalTool }; - }; - - it('should filter out school external tools in use', () => { - const { schoolExternalTools, contextExternalToolsInUse, unusedSchoolExternalTool } = setup(); - - const result: SchoolExternalTool[] = service.filterForAvailableSchoolExternalTools( - schoolExternalTools, - contextExternalToolsInUse - ); - - expect(result).toEqual([unusedSchoolExternalTool]); - }); - }); - }); - describe('filterForAvailableExternalTools', () => { describe('when filtering for available external tools', () => { const setup = () => { diff --git a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts index 8fd21257815..a1868352ad7 100644 --- a/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts +++ b/apps/server/src/modules/tool/external-tool/service/external-tool-configuration.service.ts @@ -1,21 +1,16 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { Page } from '@shared/domain/domainobject'; import { EntityId } from '@shared/domain/types'; import { CustomParameter } from '../../common/domain'; import { CustomParameterScope, ToolContextType } from '../../common/enum'; -import { ContextExternalTool } from '../../context-external-tool/domain'; +import { CommonToolService } from '../../common/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { ExternalTool } from '../domain'; import { ContextExternalToolTemplateInfo } from '../uc/dto'; -import { CommonToolService } from '../../common/service'; @Injectable() export class ExternalToolConfigurationService { - constructor( - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures, - private readonly commonToolService: CommonToolService - ) {} + constructor(private readonly commonToolService: CommonToolService) {} public filterForAvailableTools(externalTools: Page, toolIdsInUse: EntityId[]): ExternalTool[] { const visibleTools: ExternalTool[] = externalTools.data.filter((tool: ExternalTool): boolean => !tool.isHidden); @@ -26,28 +21,6 @@ export class ExternalToolConfigurationService { return availableTools; } - public filterForAvailableSchoolExternalTools( - schoolExternalTools: SchoolExternalTool[], - contextExternalToolsInUse: ContextExternalTool[] - ): SchoolExternalTool[] { - const availableSchoolExternalTools: SchoolExternalTool[] = schoolExternalTools.filter( - (schoolExternalTool: SchoolExternalTool): boolean => { - if (this.toolFeatures.contextConfigurationEnabled) { - return true; - } - - const hasContextExternalTool: boolean = contextExternalToolsInUse.some( - (contextExternalTool: ContextExternalTool) => - contextExternalTool.schoolToolRef.schoolToolId === schoolExternalTool.id - ); - - return !hasContextExternalTool; - } - ); - - return availableSchoolExternalTools; - } - public filterForAvailableExternalTools( externalTools: ExternalTool[], availableSchoolExternalTools: SchoolExternalTool[] diff --git a/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.spec.ts b/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.spec.ts index a2de0d99460..6300138e809 100644 --- a/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.spec.ts +++ b/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.spec.ts @@ -285,9 +285,6 @@ describe('ExternalToolConfigurationUc', () => { contextExternalToolService.findContextExternalTools.mockResolvedValue([usedContextExternalTool]); authorizationService.getUserWithPermissions.mockResolvedValueOnce(user); - externalToolConfigurationService.filterForAvailableSchoolExternalTools.mockReturnValue([ - usedSchoolExternalTool, - ]); externalToolConfigurationService.filterForAvailableExternalTools.mockReturnValue([ { externalTool: usedTool, schoolExternalTool: usedSchoolExternalTool }, ]); @@ -330,28 +327,6 @@ describe('ExternalToolConfigurationUc', () => { expect(logoService.buildLogoUrl).toHaveBeenCalledWith('/v3/tools/external-tools/{id}/logo', usedTool); }); - it('should call filterForAvailableSchoolExternalTools', async () => { - const { schoolExternalTools, usedContextExternalTool } = setup(); - - await uc.getAvailableToolsForContext('userId', 'schoolId', 'contextId', ToolContextType.COURSE); - - expect(externalToolConfigurationService.filterForAvailableSchoolExternalTools).toHaveBeenCalledWith( - schoolExternalTools, - [usedContextExternalTool] - ); - }); - - it('should call filterForAvailableTools', async () => { - const { externalTools, usedSchoolExternalTool } = setup(); - - await uc.getAvailableToolsForContext('userId', 'schoolId', 'contextId', ToolContextType.COURSE); - - expect(externalToolConfigurationService.filterForAvailableExternalTools).toHaveBeenCalledWith( - externalTools.data, - [usedSchoolExternalTool] - ); - }); - it('should filter for restricted contexts', async () => { const { usedTool, usedSchoolExternalTool } = setup(); @@ -391,9 +366,6 @@ describe('ExternalToolConfigurationUc', () => { schoolExternalToolService.findSchoolExternalTools.mockResolvedValue([unusedSchoolExternalTool]); contextExternalToolService.findContextExternalTools.mockResolvedValue([]); - externalToolConfigurationService.filterForAvailableSchoolExternalTools.mockReturnValue([ - unusedSchoolExternalTool, - ]); externalToolConfigurationService.filterForAvailableExternalTools.mockReturnValue([]); externalToolConfigurationService.filterForContextRestrictions.mockReturnValue([]); @@ -431,9 +403,6 @@ describe('ExternalToolConfigurationUc', () => { schoolExternalToolService.findSchoolExternalTools.mockResolvedValue([usedSchoolExternalTool]); contextExternalToolService.findContextExternalTools.mockResolvedValue([usedContextExternalTool]); - externalToolConfigurationService.filterForAvailableSchoolExternalTools.mockReturnValue([ - usedSchoolExternalTool, - ]); externalToolConfigurationService.filterForAvailableExternalTools.mockReturnValue([ { externalTool: usedTool, schoolExternalTool: usedSchoolExternalTool }, ]); diff --git a/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.ts b/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.ts index c1b01ac33e2..4f81df0d75d 100644 --- a/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.ts +++ b/apps/server/src/modules/tool/external-tool/uc/external-tool-configuration.uc.ts @@ -101,17 +101,8 @@ export class ExternalToolConfigurationUc { const context: AuthorizationContext = AuthorizationContextBuilder.read([Permission.CONTEXT_TOOL_ADMIN]); await this.ensureContextPermissions(user, contextExternalToolsInUse, context); - const availableSchoolExternalTools: SchoolExternalTool[] = - this.externalToolConfigurationService.filterForAvailableSchoolExternalTools( - schoolExternalTools, - contextExternalToolsInUse - ); - let availableToolsForContext: ContextExternalToolTemplateInfo[] = - this.externalToolConfigurationService.filterForAvailableExternalTools( - externalTools.data, - availableSchoolExternalTools - ); + this.externalToolConfigurationService.filterForAvailableExternalTools(externalTools.data, schoolExternalTools); availableToolsForContext = this.externalToolConfigurationService.filterForContextRestrictions( availableToolsForContext, 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 f7e4d5687e4..8d1b0e040e4 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 @@ -5,7 +5,6 @@ import { externalToolFactory, schoolExternalToolFactory } from '@shared/testing' import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; @@ -15,7 +14,6 @@ describe(SchoolExternalToolValidationService.name, () => { let externalToolService: DeepMocked; let commonToolValidationService: DeepMocked; - let toolFeatures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -29,19 +27,12 @@ describe(SchoolExternalToolValidationService.name, () => { 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 +47,6 @@ describe(SchoolExternalToolValidationService.name, () => { externalToolService.findById.mockResolvedValue(externalTool); commonToolValidationService.validateParameters.mockReturnValueOnce([]); - toolFeatures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -95,7 +85,6 @@ describe(SchoolExternalToolValidationService.name, () => { externalToolService.findById.mockResolvedValue(externalTool); commonToolValidationService.validateParameters.mockReturnValueOnce([error]); - toolFeatures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -111,29 +100,4 @@ describe(SchoolExternalToolValidationService.name, () => { }); }); }); - - // 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 = () => { - const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({ toolVersion: 1337 }); - const externalTool: ExternalTool = externalToolFactory.buildWithId({ version: 8383 }); - - externalToolService.findById.mockResolvedValue(externalTool); - toolFeatures.toolStatusWithoutVersions = false; - - return { - schoolExternalTool, - }; - }; - - it('should throw error', async () => { - const { schoolExternalTool } = setup(); - - const func = () => service.validate(schoolExternalTool); - - await expect(func()).rejects.toThrowError('tool_version_mismatch'); - }); - }); - }); }); 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 17ac81faf10..ffa5a9f019c 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,26 +1,20 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { ValidationError } from '@shared/common'; import { CommonToolValidationService } from '../../common/service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { SchoolExternalTool } from '../domain'; @Injectable() export class SchoolExternalToolValidationService { constructor( private readonly externalToolService: ExternalToolService, - private readonly commonToolValidationService: CommonToolValidationService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + private readonly commonToolValidationService: CommonToolValidationService ) {} async validate(schoolExternalTool: SchoolExternalTool): Promise { const loadedExternalTool: ExternalTool = await this.externalToolService.findById(schoolExternalTool.toolId); - if (!this.toolFeatures.toolStatusWithoutVersions) { - this.checkVersionMatch(schoolExternalTool.toolVersion, loadedExternalTool.version); - } - const errors: ValidationError[] = this.commonToolValidationService.validateParameters( loadedExternalTool, schoolExternalTool @@ -30,12 +24,4 @@ export class SchoolExternalToolValidationService { throw errors[0]; } } - - private checkVersionMatch(schoolExternalToolVersion: number, externalToolVersion: number): void { - if (schoolExternalToolVersion !== externalToolVersion) { - throw new ValidationError( - `tool_version_mismatch: The version ${schoolExternalToolVersion} of given schoolExternalTool does not match the externalTool version ${externalToolVersion}.` - ); - } - } } 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 95f9499a7c9..4bb7156b5a3 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 @@ -9,21 +9,18 @@ import { } from '@shared/testing/factory'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; -import { SchoolExternalToolConfigurationStatus } from '../controller/domain/school-external-tool-configuration-status'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolQuery } from '../uc/dto/school-external-tool.types'; import { SchoolExternalToolValidationService } from './school-external-tool-validation.service'; import { SchoolExternalToolService } from './school-external-tool.service'; -describe('SchoolExternalToolService', () => { +describe(SchoolExternalToolService.name, () => { let module: TestingModule; let service: SchoolExternalToolService; let schoolExternalToolRepo: DeepMocked; let externalToolService: DeepMocked; let schoolExternalToolValidationService: DeepMocked; - let toolFearures: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ @@ -41,12 +38,6 @@ describe('SchoolExternalToolService', () => { provide: SchoolExternalToolValidationService, useValue: createMock(), }, - { - provide: ToolFeatures, - useValue: { - toolStatusWithoutVersions: false, - }, - }, ], }).compile(); @@ -54,28 +45,24 @@ describe('SchoolExternalToolService', () => { schoolExternalToolRepo = module.get(SchoolExternalToolRepo); externalToolService = module.get(ExternalToolService); schoolExternalToolValidationService = module.get(SchoolExternalToolValidationService); - 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(); + describe('findSchoolExternalTools', () => { + describe('when called with query', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); - schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); - toolFearures.toolStatusWithoutVersions = false; + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); - return { - schoolExternalTool, - schoolExternalToolId: schoolExternalTool.id as string, - externalTool, - }; - }; + return { + schoolExternalTool, + }; + }; - describe('findSchoolExternalTools', () => { - describe('when called with query', () => { it('should call repo with query', async () => { - const { schoolExternalTool } = legacySetup(); + const { schoolExternalTool } = setup(); await service.findSchoolExternalTools(schoolExternalTool); @@ -86,7 +73,7 @@ describe('SchoolExternalToolService', () => { }); it('should return schoolExternalTool array', async () => { - const { schoolExternalTool } = legacySetup(); + const { schoolExternalTool } = setup(); schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool, schoolExternalTool]); const result: SchoolExternalTool[] = await service.findSchoolExternalTools(schoolExternalTool); @@ -97,78 +84,30 @@ describe('SchoolExternalToolService', () => { }); describe('enrichDataFromExternalTool', () => { - it('should call the externalToolService', async () => { - const { schoolExternalTool } = legacySetup(); - schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + describe('when schoolExternalTool is given', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); - await service.findSchoolExternalTools(schoolExternalTool); + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); - expect(externalToolService.findById).toHaveBeenCalledWith(schoolExternalTool.toolId); - }); + return { + schoolExternalTool, + }; + }; - describe('when determine status', () => { - 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( - schoolToolConfigurationStatusFactory.build({ - isOutdatedOnScopeSchool: true, - }) - ); - }); - }); + it('should call the externalToolService', async () => { + const { schoolExternalTool } = setup(); - 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); - - const schoolExternalToolDOs: SchoolExternalTool[] = await service.findSchoolExternalTools( - schoolExternalTool - ); - - expect(schoolExternalToolDOs[0].status).toEqual( - schoolToolConfigurationStatusFactory.build({ - isOutdatedOnScopeSchool: false, - }) - ); - }); - }); + await service.findSchoolExternalTools(schoolExternalTool); - 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( - schoolToolConfigurationStatusFactory.build({ - isOutdatedOnScopeSchool: false, - }) - ); - }); - }); + expect(externalToolService.findById).toHaveBeenCalledWith(schoolExternalTool.toolId); }); + }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation goes through', () => { + describe('when determine status', () => { + describe('when validation goes through', () => { const setup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); const externalTool: ExternalTool = externalToolFactory.buildWithId(); @@ -176,7 +115,6 @@ describe('SchoolExternalToolService', () => { schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); schoolExternalToolValidationService.validate.mockResolvedValue(); - toolFearures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -210,7 +148,7 @@ describe('SchoolExternalToolService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and validation throws error', () => { + describe('when validation throws error', () => { const setup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); const externalTool: ExternalTool = externalToolFactory.buildWithId(); @@ -218,7 +156,6 @@ describe('SchoolExternalToolService', () => { schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); schoolExternalToolValidationService.validate.mockRejectedValue(ApiValidationError); - toolFearures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -239,7 +176,7 @@ describe('SchoolExternalToolService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and schoolExternalTool is deactivated', () => { + describe('when schoolExternalTool is deactivated', () => { const setup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({ status: schoolToolConfigurationStatusFactory.build({ isDeactivated: true }), @@ -249,7 +186,6 @@ describe('SchoolExternalToolService', () => { schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); schoolExternalToolValidationService.validate.mockRejectedValue(Promise.resolve()); - toolFearures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -270,7 +206,7 @@ describe('SchoolExternalToolService', () => { }); }); - describe('when FEATURE_COMPUTE_TOOL_STATUS_WITHOUT_VERSIONS_ENABLED is true and externalTool is deactivated', () => { + describe('when externalTool is deactivated', () => { const setup = () => { const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); const externalTool: ExternalTool = externalToolFactory.buildWithId({ isDeactivated: true }); @@ -278,7 +214,6 @@ describe('SchoolExternalToolService', () => { schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); externalToolService.findById.mockResolvedValue(externalTool); schoolExternalToolValidationService.validate.mockRejectedValue(Promise.resolve()); - toolFearures.toolStatusWithoutVersions = true; return { schoolExternalTool, @@ -303,8 +238,20 @@ describe('SchoolExternalToolService', () => { describe('deleteSchoolExternalToolById', () => { describe('when schoolExternalToolId is given', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); + + return { + schoolExternalToolId: schoolExternalTool.id as string, + }; + }; + it('should call the schoolExternalToolRepo', async () => { - const { schoolExternalToolId } = legacySetup(); + const { schoolExternalToolId } = setup(); await service.deleteSchoolExternalToolById(schoolExternalToolId); @@ -315,8 +262,20 @@ describe('SchoolExternalToolService', () => { describe('findById', () => { describe('when schoolExternalToolId is given', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); + + return { + schoolExternalToolId: schoolExternalTool.id as string, + }; + }; + it('should call schoolExternalToolRepo.findById', async () => { - const { schoolExternalToolId } = legacySetup(); + const { schoolExternalToolId } = setup(); await service.findById(schoolExternalToolId); @@ -327,8 +286,20 @@ describe('SchoolExternalToolService', () => { describe('saveSchoolExternalTool', () => { describe('when schoolExternalTool is given', () => { + const setup = () => { + const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build(); + const externalTool: ExternalTool = externalToolFactory.buildWithId(); + + schoolExternalToolRepo.find.mockResolvedValue([schoolExternalTool]); + externalToolService.findById.mockResolvedValue(externalTool); + + return { + schoolExternalTool, + }; + }; + it('should call schoolExternalToolRepo.save', async () => { - const { schoolExternalTool } = legacySetup(); + const { schoolExternalTool } = setup(); await service.saveSchoolExternalTool(schoolExternalTool); @@ -336,7 +307,7 @@ describe('SchoolExternalToolService', () => { }); it('should enrich data from externalTool', async () => { - const { schoolExternalTool } = legacySetup(); + const { schoolExternalTool } = setup(); await service.saveSchoolExternalTool(schoolExternalTool); 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 b2b30e5b6fe..6126dbbd93f 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,9 +1,8 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { EntityId } from '@shared/domain/types'; import { SchoolExternalToolRepo } from '@shared/repo'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; -import { IToolFeatures, ToolFeatures } from '../../tool-config'; import { SchoolExternalToolConfigurationStatus } from '../controller/dto'; import { SchoolExternalTool } from '../domain'; import { SchoolExternalToolQuery } from '../uc/dto/school-external-tool.types'; @@ -14,8 +13,7 @@ export class SchoolExternalToolService { constructor( private readonly schoolExternalToolRepo: SchoolExternalToolRepo, private readonly externalToolService: ExternalToolService, - private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService, - @Inject(ToolFeatures) private readonly toolFeatures: IToolFeatures + private readonly schoolExternalToolValidationService: SchoolExternalToolValidationService ) {} async findById(schoolExternalToolId: EntityId): Promise { @@ -63,25 +61,15 @@ export class SchoolExternalToolService { isDeactivated: this.isToolDeactivated(externalTool, tool), }); - if (this.toolFeatures.toolStatusWithoutVersions) { - try { - await this.schoolExternalToolValidationService.validate(tool); + try { + await this.schoolExternalToolValidationService.validate(tool); - status.isOutdatedOnScopeSchool = false; - - return status; - } catch (err) { - return status; - } - } - - if (externalTool.version <= tool.toolVersion) { status.isOutdatedOnScopeSchool = false; + return status; + } catch (err) { return status; } - - return status; } async deleteSchoolExternalToolById(schoolExternalToolId: EntityId): Promise { @@ -91,6 +79,7 @@ export class SchoolExternalToolService { async saveSchoolExternalTool(schoolExternalTool: SchoolExternalTool): Promise { let createdSchoolExternalTool: SchoolExternalTool = await this.schoolExternalToolRepo.save(schoolExternalTool); createdSchoolExternalTool = await this.enrichDataFromExternalTool(createdSchoolExternalTool); + return createdSchoolExternalTool; } diff --git a/apps/server/src/modules/tool/tool-config.ts b/apps/server/src/modules/tool/tool-config.ts index d84e1900361..ccd33a579ce 100644 --- a/apps/server/src/modules/tool/tool-config.ts +++ b/apps/server/src/modules/tool/tool-config.ts @@ -5,9 +5,6 @@ export const ToolFeatures = Symbol('ToolFeatures'); export interface IToolFeatures { ctlToolsTabEnabled: boolean; ltiToolsTabEnabled: boolean; - contextConfigurationEnabled: boolean; - // TODO N21-1337 refactor after feature flag is removed - toolStatusWithoutVersions: boolean; maxExternalToolLogoSizeInBytes: number; backEndUrl: string; ctlToolsCopyEnabled: boolean; @@ -18,9 +15,6 @@ export default class ToolConfiguration { static toolFeatures: IToolFeatures = { 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 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, ctlToolsCopyEnabled: Configuration.get('FEATURE_CTL_TOOLS_COPY_ENABLED') as boolean, 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 944c9b43f4e..0c544821f1f 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 @@ -23,7 +23,7 @@ import { ToolLaunchParams, } from './launch-strategy'; import { ToolLaunchService } from './tool-launch.service'; -import { ToolVersionService } from '../../context-external-tool/service/tool-version-service'; +import { ToolConfigurationStatusService } from '../../context-external-tool/service/tool-configuration-status.service'; describe('ToolLaunchService', () => { let module: TestingModule; @@ -32,7 +32,7 @@ describe('ToolLaunchService', () => { let schoolExternalToolService: DeepMocked; let externalToolService: DeepMocked; let basicToolLaunchStrategy: DeepMocked; - let toolVersionService: DeepMocked; + let toolVersionService: DeepMocked; beforeEach(async () => { module = await Test.createTestingModule({ @@ -59,8 +59,8 @@ describe('ToolLaunchService', () => { useValue: createMock(), }, { - provide: ToolVersionService, - useValue: createMock(), + provide: ToolConfigurationStatusService, + useValue: createMock(), }, ], }).compile(); @@ -69,7 +69,7 @@ describe('ToolLaunchService', () => { schoolExternalToolService = module.get(SchoolExternalToolService); externalToolService = module.get(ExternalToolService); basicToolLaunchStrategy = module.get(BasicToolLaunchStrategy); - toolVersionService = module.get(ToolVersionService); + toolVersionService = module.get(ToolConfigurationStatusService); }); afterAll(async () => { 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 261cd2cce94..362dec70c21 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 @@ -3,7 +3,7 @@ import { EntityId } from '@shared/domain/types'; import { ContextExternalToolConfigurationStatus } from '../../common/domain'; import { ToolConfigType } from '../../common/enum'; import { ContextExternalTool } from '../../context-external-tool/domain'; -import { ToolVersionService } from '../../context-external-tool/service/tool-version-service'; +import { ToolConfigurationStatusService } from '../../context-external-tool/service/tool-configuration-status.service'; import { ExternalTool } from '../../external-tool/domain'; import { ExternalToolService } from '../../external-tool/service'; import { SchoolExternalTool } from '../../school-external-tool/domain'; @@ -28,7 +28,7 @@ export class ToolLaunchService { private readonly basicToolLaunchStrategy: BasicToolLaunchStrategy, private readonly lti11ToolLaunchStrategy: Lti11ToolLaunchStrategy, private readonly oauth2ToolLaunchStrategy: OAuth2ToolLaunchStrategy, - private readonly toolVersionService: ToolVersionService + private readonly toolVersionService: ToolConfigurationStatusService ) { this.strategies = new Map(); this.strategies.set(ToolConfigType.BASIC, basicToolLaunchStrategy);