add warning when migration rollback fails

apps/server/src/core/logger/types/logging.types.ts

@@ -7,7 +7,7 @@ export type ErrorLogMessage = {
 	error?: Error;
 	type: string; // TODO: use enum
 	stack?: string;
-	data?: { [key: string]: string | number | undefined };
+	data?: { [key: string]: string | number | boolean | undefined };
 };
 
 export type ValidationErrorLogMessage = {

apps/server/src/modules/user-login-migration/loggable/school-migration-database-operation-failed.loggable-exception.spec.ts

@@ -1,29 +1,30 @@
-import { ObjectId } from '@mikro-orm/mongodb';
+import { legacySchoolDoFactory } from '@shared/testing';
 import { SchoolMigrationDatabaseOperationFailedLoggableException } from './school-migration-database-operation-failed.loggable-exception';
 
 describe(SchoolMigrationDatabaseOperationFailedLoggableException.name, () => {
 	describe('getLogMessage', () => {
 		const setup = () => {
-			const schoolId = new ObjectId().toHexString();
+			const school = legacySchoolDoFactory.buildWithId();
 
-			const exception = new SchoolMigrationDatabaseOperationFailedLoggableException(schoolId, new Error());
+			const exception = new SchoolMigrationDatabaseOperationFailedLoggableException(school, 'migration', new Error());
 
 			return {
 				exception,
-				schoolId,
+				school,
 			};
 		};
 
 		it('should return the correct log message', () => {
-			const { exception, schoolId } = setup();
+			const { exception, school } = setup();
 
 			const message = exception.getLogMessage();
 
 			expect(message).toEqual({
 				type: 'SCHOOL_LOGIN_MIGRATION_DATABASE_OPERATION_FAILED',
 				stack: expect.any(String),
 				data: {
-					schoolId,
+					schoolId: school.id,
+					operation: 'migration',
 				},
 			});
 		});

apps/server/src/modules/user-login-migration/loggable/school-migration-database-operation-failed.loggable-exception.ts

@@ -1,5 +1,5 @@
 import { InternalServerErrorException } from '@nestjs/common';
-import { EntityId } from '@shared/domain';
+import { LegacySchoolDo } from '@shared/domain';
 import { ErrorUtils } from '@src/core/error/utils';
 import { ErrorLogMessage, Loggable } from '@src/core/logger';
 
@@ -8,7 +8,11 @@ export class SchoolMigrationDatabaseOperationFailedLoggableException
 	implements Loggable
 {
 	// TODO: Remove undefined type from schoolId when using the new School DO
-	constructor(private readonly schoolId: EntityId | undefined, error: unknown) {
+	constructor(
+		private readonly school: LegacySchoolDo,
+		private readonly operation: 'migration' | 'rollback',
+		error: unknown
+	) {
 		super(ErrorUtils.createHttpExceptionOptions(error));
 	}
 
@@ -17,7 +21,8 @@ export class SchoolMigrationDatabaseOperationFailedLoggableException
 			type: 'SCHOOL_LOGIN_MIGRATION_DATABASE_OPERATION_FAILED',
 			stack: this.stack,
 			data: {
-				schoolId: this.schoolId,
+				schoolId: this.school.id,
+				operation: this.operation,
 			},
 		};
 	}

apps/server/src/modules/user-login-migration/loggable/user-migration-database-operation-failed.loggable-exception.spec.ts

@@ -6,7 +6,7 @@ describe(UserMigrationDatabaseOperationFailedLoggableException.name, () => {
 		const setup = () => {
 			const userId = new ObjectId().toHexString();
 
-			const exception = new UserMigrationDatabaseOperationFailedLoggableException(userId, new Error());
+			const exception = new UserMigrationDatabaseOperationFailedLoggableException(userId, 'migration', new Error());
 
 			return {
 				exception,
@@ -24,6 +24,7 @@ describe(UserMigrationDatabaseOperationFailedLoggableException.name, () => {
 				stack: expect.any(String),
 				data: {
 					userId,
+					operation: 'migration',
 				},
 			});
 		});

apps/server/src/modules/user-login-migration/loggable/user-migration-database-operation-failed.loggable-exception.ts

@@ -7,7 +7,7 @@ export class UserMigrationDatabaseOperationFailedLoggableException
 	extends InternalServerErrorException
 	implements Loggable
 {
-	constructor(private readonly userId: EntityId, error: unknown) {
+	constructor(private readonly userId: EntityId, private readonly operation: 'migration' | 'rollback', error: unknown) {
 		super(ErrorUtils.createHttpExceptionOptions(error));
 	}
 
@@ -17,6 +17,7 @@ export class UserMigrationDatabaseOperationFailedLoggableException
 			stack: this.stack,
 			data: {
 				userId: this.userId,
+				operation: this.operation,
 			},
 		};
 	}

apps/server/src/modules/user-login-migration/service/school-migration.service.spec.ts

@@ -6,7 +6,7 @@ import { Test, TestingModule } from '@nestjs/testing';
 import { LegacySchoolDo, Page, UserDO, UserLoginMigrationDO } from '@shared/domain';
 import { UserLoginMigrationRepo } from '@shared/repo/userloginmigration/user-login-migration.repo';
 import { legacySchoolDoFactory, setupEntities, userDoFactory, userLoginMigrationDOFactory } from '@shared/testing';
-import { LegacyLogger } from '@src/core/logger';
+import { LegacyLogger, Logger } from '@src/core/logger';
 import {
 	SchoolMigrationDatabaseOperationFailedLoggableException,
 	SchoolNumberMismatchLoggableException,
@@ -20,6 +20,7 @@ describe(SchoolMigrationService.name, () => {
 	let userService: DeepMocked<UserService>;
 	let schoolService: DeepMocked<LegacySchoolService>;
 	let userLoginMigrationRepo: DeepMocked<UserLoginMigrationRepo>;
+	let logger: DeepMocked<Logger>;
 
 	beforeAll(async () => {
 		jest.useFakeTimers();
@@ -39,6 +40,10 @@ describe(SchoolMigrationService.name, () => {
 					provide: LegacyLogger,
 					useValue: createMock<LegacyLogger>(),
 				},
+				{
+					provide: Logger,
+					useValue: createMock<Logger>(),
+				},
 				{
 					provide: UserLoginMigrationRepo,
 					useValue: createMock<UserLoginMigrationRepo>(),
@@ -50,6 +55,7 @@ describe(SchoolMigrationService.name, () => {
 		schoolService = module.get(LegacySchoolService);
 		userService = module.get(UserService);
 		userLoginMigrationRepo = module.get(UserLoginMigrationRepo);
+		logger = module.get(Logger);
 
 		await setupEntities();
 	});
@@ -141,6 +147,7 @@ describe(SchoolMigrationService.name, () => {
 
 				const error = new Error('Cannot save');
 
+				schoolService.save.mockRejectedValueOnce(error);
 				schoolService.save.mockRejectedValueOnce(error);
 
 				return {
@@ -160,11 +167,21 @@ describe(SchoolMigrationService.name, () => {
 				expect(schoolService.save).toHaveBeenLastCalledWith(school);
 			});
 
+			it('should log a rollback error', async () => {
+				const { school, targetSystemId, targetExternalId, error } = setup();
+
+				await expect(service.migrateSchool({ ...school }, targetExternalId, targetSystemId)).rejects.toThrow();
+
+				expect(logger.warning).toHaveBeenCalledWith(
+					new SchoolMigrationDatabaseOperationFailedLoggableException(school, 'rollback', error)
+				);
+			});
+
 			it('should throw an error', async () => {
 				const { school, targetSystemId, targetExternalId, error } = setup();
 
 				await expect(service.migrateSchool({ ...school }, targetExternalId, targetSystemId)).rejects.toThrow(
-					new SchoolMigrationDatabaseOperationFailedLoggableException(school.id, error)
+					new SchoolMigrationDatabaseOperationFailedLoggableException(school, 'migration', error)
 				);
 			});
 		});

apps/server/src/modules/user-login-migration/service/school-migration.service.ts

@@ -3,7 +3,7 @@ import { UserService } from '@modules/user';
 import { Injectable } from '@nestjs/common';
 import { LegacySchoolDo, Page, UserDO, UserLoginMigrationDO } from '@shared/domain';
 import { UserLoginMigrationRepo } from '@shared/repo';
-import { LegacyLogger } from '@src/core/logger';
+import { LegacyLogger, Logger } from '@src/core/logger';
 import { performance } from 'perf_hooks';
 import {
 	SchoolMigrationDatabaseOperationFailedLoggableException,
@@ -14,7 +14,8 @@ import {
 export class SchoolMigrationService {
 	constructor(
 		private readonly schoolService: LegacySchoolService,
-		private readonly logger: LegacyLogger,
+		private readonly legacyLogger: LegacyLogger,
+		private readonly logger: Logger,
 		private readonly userService: UserService,
 		private readonly userLoginMigrationRepo: UserLoginMigrationRepo
 	) {}
@@ -25,9 +26,9 @@ export class SchoolMigrationService {
 		try {
 			await this.doMigration(externalId, existingSchool, targetSystemId);
 		} catch (error: unknown) {
-			await this.rollbackMigration(schoolDOCopy);
+			await this.tryRollbackMigration(schoolDOCopy);
 
-			throw new SchoolMigrationDatabaseOperationFailedLoggableException(existingSchool.id, error);
+			throw new SchoolMigrationDatabaseOperationFailedLoggableException(existingSchool, 'migration', error);
 		}
 	}
 
@@ -44,8 +45,14 @@ export class SchoolMigrationService {
 		await this.schoolService.save(school);
 	}
 
-	private async rollbackMigration(originalSchoolDO: LegacySchoolDo) {
-		await this.schoolService.save(originalSchoolDO);
+	private async tryRollbackMigration(originalSchoolDO: LegacySchoolDo) {
+		try {
+			await this.schoolService.save(originalSchoolDO);
+		} catch (error: unknown) {
+			this.logger.warning(
+				new SchoolMigrationDatabaseOperationFailedLoggableException(originalSchoolDO, 'rollback', error)
+			);
+		}
 	}
 
 	async getSchoolForMigration(
@@ -98,7 +105,7 @@ export class SchoolMigrationService {
 		await this.userService.saveAll(notMigratedUsers.data);
 
 		const endTime: number = performance.now();
-		this.logger.warn(
+		this.legacyLogger.warn(
 			`markUnmigratedUsersAsOutdated for schoolId ${userLoginMigration.schoolId} took ${
 				endTime - startTime
 			} milliseconds`
@@ -120,7 +127,7 @@ export class SchoolMigrationService {
 		await this.userService.saveAll(migratedUsers.data);
 
 		const endTime: number = performance.now();
-		this.logger.warn(
+		this.legacyLogger.warn(
 			`unmarkOutdatedUsers for schoolId ${userLoginMigration.schoolId} took ${endTime - startTime} milliseconds`
 		);
 	}

apps/server/src/modules/user-login-migration/service/user-migration.service.spec.ts

@@ -6,6 +6,7 @@ import { UserService } from '@modules/user';
 import { Test, TestingModule } from '@nestjs/testing';
 import { UserDO } from '@shared/domain';
 import { roleFactory, setupEntities, userDoFactory } from '@shared/testing';
+import { Logger } from '@src/core/logger';
 import { UserMigrationDatabaseOperationFailedLoggableException } from '../loggable';
 import { UserMigrationService } from './user-migration.service';
 
@@ -15,6 +16,7 @@ describe(UserMigrationService.name, () => {
 
 	let userService: DeepMocked<UserService>;
 	let accountService: DeepMocked<AccountService>;
+	let logger: DeepMocked<Logger>;
 
 	beforeAll(async () => {
 		module = await Test.createTestingModule({
@@ -28,12 +30,17 @@ describe(UserMigrationService.name, () => {
 					provide: AccountService,
 					useValue: createMock<AccountService>(),
 				},
+				{
+					provide: Logger,
+					useValue: createMock<Logger>(),
+				},
 			],
 		}).compile();
 
 		service = module.get(UserMigrationService);
 		userService = module.get(UserService);
 		accountService = module.get(AccountService);
+		logger = module.get(Logger);
 
 		await setupEntities();
 	});
@@ -177,6 +184,7 @@ describe(UserMigrationService.name, () => {
 				accountService.findByUserIdOrFail.mockResolvedValueOnce({ ...accountDto });
 
 				userService.save.mockRejectedValueOnce(error);
+				accountService.save.mockRejectedValueOnce(error);
 
 				return {
 					user,
@@ -206,11 +214,21 @@ describe(UserMigrationService.name, () => {
 				expect(accountService.save).toHaveBeenLastCalledWith(accountDto);
 			});
 
+			it('should log a rollback error', async () => {
+				const { userId, targetExternalId, targetSystemId, error } = setup();
+
+				await expect(service.migrateUser(userId, targetExternalId, targetSystemId)).rejects.toThrow();
+
+				expect(logger.warning).toHaveBeenCalledWith(
+					new UserMigrationDatabaseOperationFailedLoggableException(userId, 'rollback', error)
+				);
+			});
+
 			it('should throw an error', async () => {
 				const { userId, targetExternalId, targetSystemId, error } = setup();
 
 				await expect(service.migrateUser(userId, targetExternalId, targetSystemId)).rejects.toThrow(
-					new UserMigrationDatabaseOperationFailedLoggableException(userId, error)
+					new UserMigrationDatabaseOperationFailedLoggableException(userId, 'migration', error)
 				);
 			});
 		});

apps/server/src/modules/user-login-migration/service/user-migration.service.ts

@@ -2,14 +2,20 @@ import { AccountService } from '@modules/account/services/account.service';
 import { AccountDto } from '@modules/account/services/dto';
 import { UserService } from '@modules/user';
 import { Injectable } from '@nestjs/common';
+import { EntityId } from '@shared/domain';
 import { UserDO } from '@shared/domain/domainobject/user.do';
+import { Logger } from '@src/core/logger';
 import { UserMigrationDatabaseOperationFailedLoggableException } from '../loggable';
 
 @Injectable()
 export class UserMigrationService {
-	constructor(private readonly userService: UserService, private readonly accountService: AccountService) {}
+	constructor(
+		private readonly userService: UserService,
+		private readonly accountService: AccountService,
+		private readonly logger: Logger
+	) {}
 
-	async migrateUser(currentUserId: string, externalUserId: string, targetSystemId: string): Promise<void> {
+	async migrateUser(currentUserId: EntityId, externalUserId: string, targetSystemId: EntityId): Promise<void> {
 		const userDO: UserDO = await this.userService.findById(currentUserId);
 		const account: AccountDto = await this.accountService.findByUserIdOrFail(currentUserId);
 
@@ -19,9 +25,9 @@ export class UserMigrationService {
 		try {
 			await this.doMigration(userDO, externalUserId, account, targetSystemId);
 		} catch (error: unknown) {
-			await this.rollbackMigration(userDOCopy, accountCopy);
+			await this.tryRollbackMigration(currentUserId, userDOCopy, accountCopy);
 
-			throw new UserMigrationDatabaseOperationFailedLoggableException(currentUserId, error);
+			throw new UserMigrationDatabaseOperationFailedLoggableException(currentUserId, 'migration', error);
 		}
 	}
 
@@ -40,8 +46,16 @@ export class UserMigrationService {
 		await this.accountService.save(account);
 	}
 
-	private async rollbackMigration(userDOCopy: UserDO, accountCopy: AccountDto): Promise<void> {
-		await this.userService.save(userDOCopy);
-		await this.accountService.save(accountCopy);
+	private async tryRollbackMigration(
+		currentUserId: EntityId,
+		userDOCopy: UserDO,
+		accountCopy: AccountDto
+	): Promise<void> {
+		try {
+			await this.userService.save(userDOCopy);
+			await this.accountService.save(accountCopy);
+		} catch (error: unknown) {
+			this.logger.warning(new UserMigrationDatabaseOperationFailedLoggableException(currentUserId, 'rollback', error));
+		}
 	}
 }