Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

N21-2317 Skip failed migrations in migration wizard #5398

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading