From 22fc71c0bd0d7a1eea7f103ae9f19a1c788e9e43 Mon Sep 17 00:00:00 2001 From: Maximilian Kreuzkam Date: Tue, 21 Jan 2025 14:49:59 +0100 Subject: [PATCH] Make order of calls clear and use Promise.all --- ...gacy-migration.service.integration.spec.ts | 6 +- .../tsp/tsp-legacy-migration.service.ts | 2 +- .../strategy/tsp/tsp-sync.service.spec.ts | 4 +- .../sync/strategy/tsp/tsp-sync.service.ts | 2 +- .../strategy/tsp/tsp-sync.strategy.spec.ts | 6 +- .../sync/strategy/tsp/tsp-sync.strategy.ts | 58 ++++++++++++------- 6 files changed, 47 insertions(+), 31 deletions(-) diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.integration.spec.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.integration.spec.ts index 6d4ef1f1ba..5b23c17f09 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.integration.spec.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.integration.spec.ts @@ -46,10 +46,10 @@ describe('account repo', () => { await cleanupCollections(em); }); - describe('migrateLegacyData', () => { + describe('prepareLegacySyncDataForNewSync', () => { describe('when legacy system is not found', () => { it('should log TspLegacyMigrationSystemMissingLoggable', async () => { - await sut.migrateLegacyData(''); + await sut.prepareLegacySyncDataForNewSync(''); expect(logger.info).toHaveBeenCalledWith(new TspLegacyMigrationSystemMissingLoggable()); }); @@ -94,7 +94,7 @@ describe('account repo', () => { it('should update the school to the new format', async () => { const { newSystem, legacySchool, schoolId: schoolIdentifier } = await setup(); - await sut.migrateLegacyData(newSystem.id); + await sut.prepareLegacySyncDataForNewSync(newSystem.id); const migratedSchool = await em.findOne(SchoolEntity.name, { id: legacySchool.id, diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.ts index 1bff95ef2b..b7c0153843 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-legacy-migration.service.ts @@ -24,7 +24,7 @@ export class TspLegacyMigrationService { logger.setContext(TspLegacyMigrationService.name); } - public async migrateLegacyData(newSystemId: EntityId): Promise { + public async prepareLegacySyncDataForNewSync(newSystemId: EntityId): Promise { this.logger.info(new TspLegacyMigrationStartLoggable()); const legacySystemId = await this.findLegacySystemId(); diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.spec.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.spec.ts index a225cef623..495e2447ca 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.spec.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.spec.ts @@ -140,7 +140,7 @@ describe(TspSyncService.name, () => { }); }); - describe('findSchoolsForSystem', () => { + describe('findAllSchoolsForSystem', () => { describe('when findSchoolsForSystem is called', () => { const setup = () => { const system = systemFactory.build(); @@ -154,7 +154,7 @@ describe(TspSyncService.name, () => { it('should return an array of schools', async () => { const { system, school } = setup(); - const schools = await sut.findSchoolsForSystem(system); + const schools = await sut.findAllSchoolsForSystem(system); expect(schools).toEqual([school]); }); diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.ts index 4ec2fa7cac..cf874af5dc 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.service.ts @@ -49,7 +49,7 @@ export class TspSyncService { return schools[0]; } - public async findSchoolsForSystem(system: System): Promise { + public async findAllSchoolsForSystem(system: System): Promise { const schools = await this.schoolService.getSchools({ systemId: system.id, }); diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.spec.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.spec.ts index 2baa741dff..1da6c8e0fc 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.spec.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.spec.ts @@ -148,7 +148,7 @@ describe(TspSyncStrategy.name, () => { tspFetchService.fetchTspStudentMigrations.mockResolvedValueOnce(params.fetchedStudentMigrations ?? []); tspSyncService.findSchool.mockResolvedValue(params.foundSchool ?? undefined); - tspSyncService.findSchoolsForSystem.mockResolvedValueOnce(params.foundSystemSchools ?? []); + tspSyncService.findAllSchoolsForSystem.mockResolvedValueOnce(params.foundSystemSchools ?? []); tspSyncService.findTspSystemOrFail.mockResolvedValueOnce(params.foundSystem ?? systemFactory.build()); tspOauthDataMapper.mapTspDataToOauthData.mockReturnValueOnce(params.mappedOauthDto ?? []); @@ -210,7 +210,7 @@ describe(TspSyncStrategy.name, () => { await sut.sync(); - expect(tspLegacyMigrationService.migrateLegacyData).toHaveBeenCalled(); + expect(tspLegacyMigrationService.prepareLegacySyncDataForNewSync).toHaveBeenCalled(); }); it('should fetch the schools', async () => { @@ -236,7 +236,7 @@ describe(TspSyncStrategy.name, () => { await sut.sync(); - expect(tspSyncService.findSchoolsForSystem).toHaveBeenCalled(); + expect(tspSyncService.findAllSchoolsForSystem).toHaveBeenCalled(); }); it('should map to OauthDataDto', async () => { diff --git a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.ts b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.ts index ce4d8e5692..b1ba48d623 100644 --- a/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.ts +++ b/apps/server/src/infra/sync/strategy/tsp/tsp-sync.strategy.ts @@ -2,11 +2,12 @@ import { School } from '@modules/school'; import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { Logger } from '@src/core/logger'; +import { RobjExportKlasse, RobjExportLehrer, RobjExportSchueler } from '@src/infra/tsp-client'; import { ProvisioningService } from '@src/modules/provisioning'; import { System } from '@src/modules/system'; import pLimit from 'p-limit'; -import { SyncStrategy } from '../sync-strategy'; import { SyncStrategyTarget } from '../../sync-strategy.types'; +import { SyncStrategy } from '../sync-strategy'; import { TspDataFetchedLoggable } from './loggable/tsp-data-fetched.loggable'; import { TspSchoolsFetchedLoggable } from './loggable/tsp-schools-fetched.loggable'; import { TspSchoolsSyncedLoggable } from './loggable/tsp-schools-synced.loggable'; @@ -21,6 +22,12 @@ import { TspSyncMigrationService } from './tsp-sync-migration.service'; import { TspSyncConfig } from './tsp-sync.config'; import { TspSyncService } from './tsp-sync.service'; +type TspSchoolData = { + tspTeachers: RobjExportLehrer[]; + tspStudents: RobjExportSchueler[]; + tspClasses: RobjExportKlasse[]; +}; + @Injectable() export class TspSyncStrategy extends SyncStrategy { constructor( @@ -42,22 +49,23 @@ export class TspSyncStrategy extends SyncStrategy { } public async sync(): Promise { + // Please keep the order of this steps/methods as each relies on the data processed in the ones before. const system = await this.tspSyncService.findTspSystemOrFail(); - await this.tspLegacyMigrationService.migrateLegacyData(system.id); + await this.tspLegacyMigrationService.prepareLegacySyncDataForNewSync(system.id); - await this.syncSchools(system); + await this.syncTspSchools(system); - const schools = await this.tspSyncService.findSchoolsForSystem(system); + const schools = await this.tspSyncService.findAllSchoolsForSystem(system); if (this.configService.getOrThrow('FEATURE_TSP_MIGRATION_ENABLED', { infer: true })) { - await this.runMigration(system); + await this.runMigrationOfExistingUsers(system); } - await this.syncData(system, schools); + await this.syncDataOfSyncedTspSchools(system, schools); } - private async syncSchools(system: System): Promise { + private async syncTspSchools(system: System): Promise { const schoolDaysToFetch = this.configService.getOrThrow('TSP_SYNC_SCHOOL_DAYS_TO_FETCH', { infer: true }); const tspSchools = await this.tspFetchService.fetchTspSchools(system, schoolDaysToFetch); this.logger.info(new TspSchoolsFetchedLoggable(tspSchools.length, schoolDaysToFetch)); @@ -99,14 +107,8 @@ export class TspSyncStrategy extends SyncStrategy { return scSchools.filter((scSchool) => scSchool != null).map((scSchool) => scSchool.school); } - private async syncData(system: System, schools: School[]): Promise { - const schoolDataDaysToFetch = this.configService.getOrThrow('TSP_SYNC_DATA_DAYS_TO_FETCH', { infer: true }); - const tspTeachers = await this.tspFetchService.fetchTspTeachers(system, schoolDataDaysToFetch); - const tspStudents = await this.tspFetchService.fetchTspStudents(system, schoolDataDaysToFetch); - const tspClasses = await this.tspFetchService.fetchTspClasses(system, schoolDataDaysToFetch); - this.logger.info( - new TspDataFetchedLoggable(tspTeachers.length, tspStudents.length, tspClasses.length, schoolDataDaysToFetch) - ); + private async syncDataOfSyncedTspSchools(system: System, schools: School[]): Promise { + const { tspTeachers, tspStudents, tspClasses } = await this.fetchSchoolData(system); const oauthDataDtos = this.tspOauthDataMapper.mapTspDataToOauthData( system, @@ -120,26 +122,40 @@ export class TspSyncStrategy extends SyncStrategy { const dataLimit = this.configService.getOrThrow('TSP_SYNC_DATA_LIMIT', { infer: true }); const dataLimitFn = pLimit(dataLimit); - const dataPromises = oauthDataDtos.map((oauthDataDto) => dataLimitFn(() => this.provisioningService.provisionData(oauthDataDto)) ); - const results = await Promise.allSettled(dataPromises); this.logger.info(new TspSyncedUsersLoggable(results.length)); } - private async runMigration(system: System): Promise { + private async fetchSchoolData(system: System): Promise { + const schoolDataDaysToFetch = this.configService.getOrThrow('TSP_SYNC_DATA_DAYS_TO_FETCH', { infer: true }); + const [tspTeachers, tspStudents, tspClasses] = await Promise.all([ + this.tspFetchService.fetchTspTeachers(system, schoolDataDaysToFetch), + this.tspFetchService.fetchTspStudents(system, schoolDataDaysToFetch), + this.tspFetchService.fetchTspClasses(system, schoolDataDaysToFetch), + ]); + this.logger.info( + new TspDataFetchedLoggable(tspTeachers.length, tspStudents.length, tspClasses.length, schoolDataDaysToFetch) + ); + + return { tspTeachers, tspStudents, tspClasses }; + } + + private async runMigrationOfExistingUsers(system: System): Promise { const oldToNewMappings = new Map(); - const teacherMigrations = await this.tspFetchService.fetchTspTeacherMigrations(system); + const [teacherMigrations, studentsMigrations] = await Promise.all([ + this.tspFetchService.fetchTspTeacherMigrations(system), + this.tspFetchService.fetchTspStudentMigrations(system), + ]); + teacherMigrations.forEach(({ lehrerUidAlt, lehrerUidNeu }) => { if (lehrerUidAlt && lehrerUidNeu) { oldToNewMappings.set(lehrerUidAlt, lehrerUidNeu); } }); - - const studentsMigrations = await this.tspFetchService.fetchTspStudentMigrations(system); studentsMigrations.forEach(({ schuelerUidAlt, schuelerUidNeu }) => { if (schuelerUidAlt && schuelerUidNeu) { oldToNewMappings.set(schuelerUidAlt, schuelerUidNeu);