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-1447 user-login-migration refactorings #4556

Merged
merged 6 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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/authentication/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export { ICurrentUser } from './interface';
export { JWT, CurrentUser, Authenticate } from './decorator';
export { AuthenticationModule } from './authentication.module';
export { AuthenticationService } from './services';
2 changes: 2 additions & 0 deletions apps/server/src/modules/authentication/services/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './ldap.service';
export * from './authentication.service';
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { createMock, DeepMocked } from '@golevelup/ts-jest';
import { AccountService } from '@modules/account/services/account.service';
import { AccountDto } from '@modules/account/services/dto';
import { OAuthTokenDto } from '@modules/oauth';
import { OAuthService } from '@modules/oauth/service/oauth.service';
import { OAuthTokenDto, OAuthService } from '@modules/oauth';
import { UnauthorizedException } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { EntityId, RoleName } from '@shared/domain';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AccountService } from '@modules/account/services/account.service';
import { AccountDto } from '@modules/account/services/dto';
import { OAuthTokenDto } from '@modules/oauth';
import { OAuthService } from '@modules/oauth/service/oauth.service';
import { OAuthTokenDto, OAuthService } from '@modules/oauth';
import { Injectable, UnauthorizedException } from '@nestjs/common';
import { PassportStrategy } from '@nestjs/passport';
import { UserDO } from '@shared/domain/domainobject/user.do';
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/modules/oauth/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './oauth.module';
export * from './interface';
export * from './service';
3 changes: 3 additions & 0 deletions apps/server/src/modules/oauth/service/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export * from './hydra.service';
export * from './oauth.service';
export * from './oauth-adapter.service';
3 changes: 1 addition & 2 deletions apps/server/src/modules/oauth/service/oauth.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { DefaultEncryptionService, IEncryptionService } from '@infra/encryption';
import { LegacySchoolService } from '@modules/legacy-school';
import { ProvisioningService } from '@modules/provisioning';
import { OauthDataDto } from '@modules/provisioning/dto';
import { ProvisioningService, OauthDataDto } from '@modules/provisioning';
import { SystemService } from '@modules/system';
import { SystemDto } from '@modules/system/service';
import { UserService } from '@modules/user';
Expand Down
3 changes: 1 addition & 2 deletions apps/server/src/modules/oauth/uc/hydra-oauth.uc.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import { OauthConfig } from '@shared/domain';
import { axiosResponseFactory } from '@shared/testing';
import { LegacyLogger } from '@src/core/logger';
import { HydraRedirectDto } from '@modules/oauth/service/dto/hydra.redirect.dto';
import { HydraSsoService } from '@modules/oauth/service/hydra.service';
import { OAuthService } from '@modules/oauth/service/oauth.service';
import { OAuthService, HydraSsoService } from '@modules/oauth';
import { AxiosResponse } from 'axios';
import { HydraOauthUc } from '.';
import { AuthorizationParams } from '../controller/dto';
Expand Down
2 changes: 1 addition & 1 deletion apps/server/src/modules/provisioning/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from './provisioning.module';
export * from './dto/provisioning.dto';
export * from './dto';
export * from './service/provisioning.service';
export * from './strategy';
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export * from './user-login-migration-start.loggable';
export * from './user-login-migration-start-loggable-exception';
export * from './user-login-migration-mandatory.loggable';
arnegns marked this conversation as resolved.
Show resolved Hide resolved
export * from './school-number-missing.loggable-exception';
export * from './user-login-migration-already-closed.loggable-exception';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { EntityId } from '@shared/domain';
import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger';

export class UserLoginMigrationAlreadyClosedLoggableException extends UnprocessableEntityException implements Loggable {
constructor(private readonly userLoginMigrationId: EntityId, private readonly closedAt: Date) {
constructor(private readonly closedAt: Date, private readonly userLoginMigrationId?: EntityId) {
super();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from
export class UserLoginMigrationMandatoryLoggable implements Loggable {
constructor(
private readonly userId: EntityId,
private readonly userLoginMigrationId: EntityId,
private readonly mandatory: boolean
private readonly mandatory: boolean,
private readonly userLoginMigrationId?: EntityId
arnegns marked this conversation as resolved.
Show resolved Hide resolved
) {}

getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { ObjectId } from 'bson';
import { UserLoginMigrationStartLoggableException } from './user-login-migration-start-loggable-exception';

describe(UserLoginMigrationStartLoggableException.name, () => {
describe('getLogMessage', () => {
const setup = () => {
const userId = new ObjectId().toHexString();
const userLoginMigrationId = new ObjectId().toHexString();
const exception = new UserLoginMigrationStartLoggableException(userId, userLoginMigrationId);

return {
exception,
userId,
userLoginMigrationId,
};
};

it('should return the correct log message', () => {
const { exception, userId, userLoginMigrationId } = setup();

const message = exception.getLogMessage();

expect(message).toEqual({
message: 'The school administrator started the migration for his school.',
data: {
userId,
userLoginMigrationId,
},
});
});
});
});
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { EntityId } from '@shared/domain';
import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger';

export class UserLoginMigrationStartLoggable implements Loggable {
constructor(private readonly userId: EntityId, private readonly userLoginMigrationId: EntityId) {}
export class UserLoginMigrationStartLoggableException implements Loggable {
arnegns marked this conversation as resolved.
Show resolved Hide resolved
constructor(private readonly userId: EntityId, private readonly userLoginMigrationId?: EntityId) {}

getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage {
return {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from './migration.dto';
export * from './page-content.dto';
export * from './school-migration-flags';

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ describe('MigrationCheckService', () => {
targetSystemId: 'targetSystemId',
startedAt: new Date('2023-03-03'),
});

schoolService.getSchoolBySchoolNumber.mockResolvedValue(school);
userService.findByExternalId.mockResolvedValue(null);
userLoginMigrationRepo.findBySchoolId.mockResolvedValue(userLoginMigration);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { LegacySchoolService } from '@modules/legacy-school';
import { UserService } from '@modules/user';
import { Injectable } from '@nestjs/common';
import { EntityId, LegacySchoolDo, UserDO, UserLoginMigrationDO } from '@shared/domain';
import { UserLoginMigrationRepo } from '@shared/repo';
import { LegacySchoolService } from '@modules/legacy-school';
import { UserService } from '@modules/user';

@Injectable()
export class MigrationCheckService {
Expand All @@ -12,22 +12,39 @@ export class MigrationCheckService {
private readonly userLoginMigrationRepo: UserLoginMigrationRepo
) {}

async shouldUserMigrate(externalUserId: string, systemId: EntityId, officialSchoolNumber: string): Promise<boolean> {
public async shouldUserMigrate(
externalUserId: string,
systemId: EntityId,
officialSchoolNumber: string
): Promise<boolean> {
const school: LegacySchoolDo | null = await this.schoolService.getSchoolBySchoolNumber(officialSchoolNumber);
arnegns marked this conversation as resolved.
Show resolved Hide resolved
const userLoginMigration: UserLoginMigrationDO | null =
school && school.id ? await this.userLoginMigrationRepo.findBySchoolId(school.id) : null;
arnegns marked this conversation as resolved.
Show resolved Hide resolved

if (school && school.id) {
const userLoginMigration: UserLoginMigrationDO | null = await this.userLoginMigrationRepo.findBySchoolId(
school.id
);
if (!school || !userLoginMigration) {
arnegns marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId);
const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId);

if (user?.lastLoginSystemChange && userLoginMigration && !userLoginMigration.closedAt) {
const hasMigrated: boolean = user.lastLoginSystemChange > userLoginMigration.startedAt;
return !hasMigrated;
}
return !!userLoginMigration && !userLoginMigration.closedAt;
if (this.isMigrationDataValid(user, userLoginMigration)) {
return !this.isUserMigrated(user, userLoginMigration);
}
return false;

return this.isMigrationActive(userLoginMigration);
}

private isMigrationDataValid(user: UserDO | null, userLoginMigration: UserLoginMigrationDO): boolean {
return !!user?.lastLoginSystemChange && !userLoginMigration.closedAt;
}

private isUserMigrated(user: UserDO | null, userLoginMigration: UserLoginMigrationDO): boolean {
return (
!!user && user.lastLoginSystemChange !== undefined && user.lastLoginSystemChange > userLoginMigration.startedAt
);
}

private isMigrationActive(userLoginMigration: UserLoginMigrationDO): boolean {
return !userLoginMigration.closedAt;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,8 @@ describe(SchoolMigrationService.name, () => {
const closedAt: Date = new Date('2023-05-01');

const userLoginMigration: UserLoginMigrationDO = userLoginMigrationDOFactory.buildWithId({
schoolId: 'schoolId',
targetSystemId: 'targetSystemId',
schoolId: new ObjectId().toHexString(),
targetSystemId: new ObjectId().toHexString(),
startedAt: new Date('2023-05-01'),
closedAt,
finishedAt: new Date('2023-05-01'),
Expand Down Expand Up @@ -379,7 +379,7 @@ describe(SchoolMigrationService.name, () => {
};
};

it('should call userLoginMigrationRepo.findBySchoolId', async () => {
it('should find user login migration by school id', async () => {
setup();

await service.hasSchoolMigratedUser('schoolId');
Expand All @@ -399,7 +399,7 @@ describe(SchoolMigrationService.name, () => {
};
};

it('should call userService.findUsers', async () => {
it('should call user service to find users', async () => {
const { userLoginMigration } = setup();

await service.hasSchoolMigratedUser('schoolId');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ export class SchoolMigrationService {
private readonly userLoginMigrationRepo: UserLoginMigrationRepo
) {}

async migrateSchool(existingSchool: LegacySchoolDo, externalId: string, targetSystemId: string): Promise<void> {
public async migrateSchool(
existingSchool: LegacySchoolDo,
externalId: string,
targetSystemId: string
): Promise<void> {
const schoolDOCopy: LegacySchoolDo = new LegacySchoolDo({ ...existingSchool });

try {
Expand All @@ -45,7 +49,7 @@ export class SchoolMigrationService {
await this.schoolService.save(school);
}

private async tryRollbackMigration(originalSchoolDO: LegacySchoolDo) {
private async tryRollbackMigration(originalSchoolDO: LegacySchoolDo): Promise<void> {
try {
await this.schoolService.save(originalSchoolDO);
} catch (error: unknown) {
Expand All @@ -55,7 +59,7 @@ export class SchoolMigrationService {
}
}

async getSchoolForMigration(
public async getSchoolForMigration(
userId: string,
externalId: string,
officialSchoolNumber: string
Expand Down Expand Up @@ -89,7 +93,7 @@ export class SchoolMigrationService {
return isExternalIdEquivalent;
}

async markUnmigratedUsersAsOutdated(userLoginMigration: UserLoginMigrationDO): Promise<void> {
public async markUnmigratedUsersAsOutdated(userLoginMigration: UserLoginMigrationDO): Promise<void> {
const startTime: number = performance.now();

const notMigratedUsers: Page<UserDO> = await this.userService.findUsers({
Expand All @@ -112,7 +116,7 @@ export class SchoolMigrationService {
);
}

async unmarkOutdatedUsers(userLoginMigration: UserLoginMigrationDO): Promise<void> {
public async unmarkOutdatedUsers(userLoginMigration: UserLoginMigrationDO): Promise<void> {
const startTime: number = performance.now();

const migratedUsers: Page<UserDO> = await this.userService.findUsers({
Expand All @@ -132,7 +136,7 @@ export class SchoolMigrationService {
);
}

async hasSchoolMigratedUser(schoolId: string): Promise<boolean> {
public async hasSchoolMigratedUser(schoolId: string): Promise<boolean> {
const userLoginMigration: UserLoginMigrationDO | null = await this.userLoginMigrationRepo.findBySchoolId(schoolId);

if (!userLoginMigration) {
Expand Down
Loading
Loading