From 8781c621681e79e390cd968b07ee24668a770c8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marvin=20=C3=96hlerking?= Date: Fri, 13 Dec 2024 11:17:35 +0100 Subject: [PATCH] stop wizard from failing --- .../src/modules/user-import/loggable/index.ts | 1 + .../user-migration-failed.loggable.spec.ts | 38 +++++++++++ .../user-migration-failed.loggable.ts | 20 ++++++ .../user-import/uc/user-import.uc.spec.ts | 67 ++++++++++++++++++- .../modules/user-import/uc/user-import.uc.ts | 26 ++++--- 5 files changed, 142 insertions(+), 10 deletions(-) create mode 100644 apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.spec.ts create mode 100644 apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.ts diff --git a/apps/server/src/modules/user-import/loggable/index.ts b/apps/server/src/modules/user-import/loggable/index.ts index 5866aa22e61..3d18659984b 100644 --- a/apps/server/src/modules/user-import/loggable/index.ts +++ b/apps/server/src/modules/user-import/loggable/index.ts @@ -12,3 +12,4 @@ export { UserMigrationIsNotEnabledLoggableException } from './user-migration-not export { UserMigrationCanceledLoggable } from './user-migration-canceled.loggable'; export { UserAlreadyMigratedLoggable } from './user-already-migrated.loggable'; export { UserLoginMigrationNotActiveLoggableException } from './user-login-migration-not-active.loggable-exception'; +export { UserMigrationFailedLoggable } from './user-migration-failed.loggable'; diff --git a/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.spec.ts b/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.spec.ts new file mode 100644 index 00000000000..96363bb1ec9 --- /dev/null +++ b/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.spec.ts @@ -0,0 +1,38 @@ +import { NotFoundException } from '@nestjs/common'; +import { importUserFactory, setupEntities } from '@shared/testing'; +import { UserMigrationFailedLoggable } from './user-migration-failed.loggable'; + +describe(UserMigrationFailedLoggable.name, () => { + describe('getLogMessage', () => { + const setup = async () => { + await setupEntities(); + const importUser = importUserFactory.build(); + const error = new NotFoundException('user not found'); + const loggable = new UserMigrationFailedLoggable(importUser, error); + + return { + loggable, + importUser, + error, + }; + }; + + it('should return the correct log message', async () => { + const { loggable, importUser, error } = await setup(); + + const message = loggable.getLogMessage(); + + expect(message).toEqual({ + type: 'USER_MIGRATION_FAILED', + message: 'An error occurred while migrating a user with the migration wizard.', + stack: error.stack, + data: { + externalUserId: importUser.externalId, + userId: importUser.user?.id, + errorName: error.name, + errorMsg: error.message, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.ts b/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.ts new file mode 100644 index 00000000000..8f382e8424e --- /dev/null +++ b/apps/server/src/modules/user-import/loggable/user-migration-failed.loggable.ts @@ -0,0 +1,20 @@ +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; +import { ImportUser } from '../entity'; + +export class UserMigrationFailedLoggable implements Loggable { + constructor(private readonly importUser: ImportUser, private readonly error: Error) {} + + public getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + type: 'USER_MIGRATION_FAILED', + message: 'An error occurred while migrating a user with the migration wizard.', + stack: this.error.stack, + data: { + externalUserId: this.importUser.externalId, + userId: this.importUser.user?.id, + errorName: this.error.name, + errorMsg: this.error.message, + }, + }; + } +} diff --git a/apps/server/src/modules/user-import/uc/user-import.uc.spec.ts b/apps/server/src/modules/user-import/uc/user-import.uc.spec.ts index af5c6d96fca..b924f67f54f 100644 --- a/apps/server/src/modules/user-import/uc/user-import.uc.spec.ts +++ b/apps/server/src/modules/user-import/uc/user-import.uc.spec.ts @@ -33,7 +33,11 @@ import { import { Logger } from '@src/core/logger'; import { ImportUserFilter, ImportUserMatchCreatorScope } from '../domain/interface'; import { ImportUser, MatchCreator } from '../entity'; -import { SchoolNotMigratedLoggableException, UserAlreadyMigratedLoggable } from '../loggable'; +import { + SchoolNotMigratedLoggableException, + UserAlreadyMigratedLoggable, + UserMigrationFailedLoggable, +} from '../loggable'; import { ImportUserRepo } from '../repo'; import { UserImportService } from '../service'; import { UserImportConfig } from '../user-import-config'; @@ -699,6 +703,7 @@ describe('[ImportUserModule]', () => { ); }); }); + describe('when user is already migrated', () => { const setup = () => { const system = systemEntityFactory.buildWithId(); @@ -762,6 +767,66 @@ describe('[ImportUserModule]', () => { expect(logger.notice).toHaveBeenCalledWith(new UserAlreadyMigratedLoggable(importUser.user!.id)); }); }); + + describe('when a user migration fails', () => { + const setup = () => { + const system = systemEntityFactory.buildWithId(); + const schoolEntity = schoolEntityFactory.buildWithId(); + const user = userFactory.buildWithId({ + school: schoolEntity, + }); + const school = legacySchoolDoFactory.build({ + id: schoolEntity.id, + externalId: 'externalId', + officialSchoolNumber: 'officialSchoolNumber', + inUserMigration: true, + inMaintenanceSince: new Date(), + systems: [system.id], + }); + const importUser = importUserFactory.buildWithId({ + school: schoolEntity, + user: userFactory.buildWithId({ + school: schoolEntity, + }), + matchedBy: MatchCreator.AUTO, + system, + externalId: 'externalId', + }); + const importUserWithoutUser = importUserFactory.buildWithId({ + school: schoolEntity, + system, + }); + const error = new Error(); + + userRepo.findById.mockResolvedValueOnce(user); + userService.findByExternalId.mockResolvedValueOnce(null); + schoolService.getSchoolById.mockResolvedValueOnce(school); + importUserRepo.findImportUsers.mockResolvedValueOnce([[importUser, importUserWithoutUser], 2]); + userMigrationService.migrateUser.mockRejectedValueOnce(error); + config.FEATURE_MIGRATION_WIZARD_WITH_USER_LOGIN_MIGRATION = true; + + return { + user, + importUser, + importUserWithoutUser, + error, + }; + }; + + it('should not throw', async () => { + const { user } = setup(); + + await expect(uc.saveAllUsersMatches(user.id)).resolves.not.toThrow(); + }); + + it('should log information for skipped user ', async () => { + const { user, importUser, error } = setup(); + + await uc.saveAllUsersMatches(user.id); + + expect(logger.warning).toHaveBeenCalledWith(new UserMigrationFailedLoggable(importUser, error)); + }); + }); }); describe('when the user does not have an account', () => { diff --git a/apps/server/src/modules/user-import/uc/user-import.uc.ts b/apps/server/src/modules/user-import/uc/user-import.uc.ts index b33363583c4..69e8868ce7e 100644 --- a/apps/server/src/modules/user-import/uc/user-import.uc.ts +++ b/apps/server/src/modules/user-import/uc/user-import.uc.ts @@ -15,6 +15,10 @@ import { IFindOptions, Permission } from '@shared/domain/interface'; import { Counted, EntityId } from '@shared/domain/types'; import { UserRepo } from '@shared/repo'; import { Logger } from '@src/core/logger'; +import { isError } from 'lodash'; + +import { ImportUserFilter, ImportUserMatchCreatorScope, ImportUserNameMatchFilter } from '../domain/interface'; +import { ImportUser, MatchCreator } from '../entity'; import { MigrationMayBeCompleted, MigrationMayNotBeCompleted, @@ -23,10 +27,8 @@ import { SchoolInUserMigrationStartLoggable, SchoolNotMigratedLoggableException, UserAlreadyMigratedLoggable, + UserMigrationFailedLoggable, } from '../loggable'; - -import { ImportUserMatchCreatorScope, ImportUserNameMatchFilter, ImportUserFilter } from '../domain/interface'; -import { ImportUser, MatchCreator } from '../entity'; import { ImportUserRepo } from '../repo'; import { UserImportService } from '../service'; import { UserImportConfig } from '../user-import-config'; @@ -200,12 +202,18 @@ export class UserImportUc { }, }); for (const importUser of importUsers) { - // TODO: Find a better solution for this loop - // this needs to be synchronous, because otherwise it was leading to - // server crush when working with larger number of users (e.g. 1000) - // eslint-disable-next-line no-await-in-loop - await this.updateUserAndAccount(importUser, school); - migratedUser += 1; + try { + // TODO: Find a better solution for this loop + // this needs to be synchronous, because otherwise it was leading to + // server crush when working with larger number of users (e.g. 1000) + // eslint-disable-next-line no-await-in-loop + await this.updateUserAndAccount(importUser, school); + migratedUser += 1; + } catch (error: unknown) { + if (isError(error)) { + this.logger.warning(new UserMigrationFailedLoggable(importUser, error)); + } + } } }