Skip to content

Commit

Permalink
N21-2317 Skip failed migrations in migration wizard (#5398)
Browse files Browse the repository at this point in the history
(cherry picked from commit 9737e90)
  • Loading branch information
MarvinOehlerkingCap authored and IgorCapCoder committed Dec 16, 2024
1 parent 5c27bf7 commit cf0a9f2
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 10 deletions.
1 change: 1 addition & 0 deletions apps/server/src/modules/user-import/loggable/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
@@ -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,
},
});
});
});
});
Original file line number Diff line number Diff line change
@@ -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,
},
};
}
}
67 changes: 66 additions & 1 deletion apps/server/src/modules/user-import/uc/user-import.uc.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -699,6 +703,7 @@ describe('[ImportUserModule]', () => {
);
});
});

describe('when user is already migrated', () => {
const setup = () => {
const system = systemEntityFactory.buildWithId();
Expand Down Expand Up @@ -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', () => {
Expand Down
26 changes: 17 additions & 9 deletions apps/server/src/modules/user-import/uc/user-import.uc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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';
Expand Down Expand Up @@ -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));
}
}
}
}

Expand Down

0 comments on commit cf0a9f2

Please sign in to comment.