From fc9b53dd97a120caf1ec12791517b011b99326d5 Mon Sep 17 00:00:00 2001 From: Arne Gnisa Date: Tue, 14 Nov 2023 15:10:04 +0100 Subject: [PATCH] N21-1447 review fixes --- .../user-login-migration/loggable/index.ts | 2 +- ...login-migration-mandatory.loggable.spec.ts | 33 +++++++++++++++++++ ...user-login-migration-mandatory.loggable.ts | 4 +-- ...er-login-migration-start.loggable.spec.ts} | 6 ++-- ...=> user-login-migration-start.loggable.ts} | 4 +-- .../service/migration-check.service.ts | 16 +++++---- .../user-login-migration.service.spec.ts | 10 +++--- .../service/user-login-migration.service.ts | 7 ++-- .../uc/restart-user-login-migration.uc.ts | 4 +-- .../uc/start-user-login-migration.uc.ts | 4 +-- .../uc/toggle-user-login-migration.uc.spec.ts | 18 +++++----- .../uc/toggle-user-login-migration.uc.ts | 4 +-- 12 files changed, 75 insertions(+), 37 deletions(-) create mode 100644 apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.spec.ts rename apps/server/src/modules/user-login-migration/loggable/{user-login-migration-start-loggable-exception.spec.ts => user-login-migration-start.loggable.spec.ts} (70%) rename apps/server/src/modules/user-login-migration/loggable/{user-login-migration-start-loggable-exception.ts => user-login-migration-start.loggable.ts} (80%) diff --git a/apps/server/src/modules/user-login-migration/loggable/index.ts b/apps/server/src/modules/user-login-migration/loggable/index.ts index 0389f8313fb..e31de36b72a 100644 --- a/apps/server/src/modules/user-login-migration/loggable/index.ts +++ b/apps/server/src/modules/user-login-migration/loggable/index.ts @@ -1,4 +1,4 @@ -export * from './user-login-migration-start-loggable-exception'; +export * from './user-login-migration-start.loggable'; export * from './user-login-migration-mandatory.loggable'; export * from './school-number-missing.loggable-exception'; export * from './user-login-migration-already-closed.loggable-exception'; diff --git a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.spec.ts b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.spec.ts new file mode 100644 index 00000000000..a2b1cde606b --- /dev/null +++ b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.spec.ts @@ -0,0 +1,33 @@ +import { ObjectId } from 'bson'; +import { UserLoginMigrationMandatoryLoggable } from './user-login-migration-mandatory.loggable'; + +describe(UserLoginMigrationMandatoryLoggable.name, () => { + describe('getLogMessage', () => { + const setup = () => { + const userId = new ObjectId().toHexString(); + const userLoginMigrationId = new ObjectId().toHexString(); + const exception = new UserLoginMigrationMandatoryLoggable(userId, userLoginMigrationId, true); + + 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 changed the requirement status of the user login migration for his school.', + data: { + userId, + userLoginMigrationId, + mandatory: true, + }, + }); + }); + }); +}); diff --git a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.ts b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.ts index fcd5dd9edf2..b3bf5724aaf 100644 --- a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.ts +++ b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.ts @@ -4,8 +4,8 @@ import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from export class UserLoginMigrationMandatoryLoggable implements Loggable { constructor( private readonly userId: EntityId, - private readonly mandatory: boolean, - private readonly userLoginMigrationId?: EntityId + private readonly userLoginMigrationId: EntityId | undefined, + private readonly mandatory: boolean ) {} getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { diff --git a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.spec.ts b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.spec.ts similarity index 70% rename from apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.spec.ts rename to apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.spec.ts index ff5b81b2c06..f15ac763e5c 100644 --- a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.spec.ts +++ b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.spec.ts @@ -1,12 +1,12 @@ import { ObjectId } from 'bson'; -import { UserLoginMigrationStartLoggableException } from './user-login-migration-start-loggable-exception'; +import { UserLoginMigrationStartLoggable } from './user-login-migration-start.loggable'; -describe(UserLoginMigrationStartLoggableException.name, () => { +describe(UserLoginMigrationStartLoggable.name, () => { describe('getLogMessage', () => { const setup = () => { const userId = new ObjectId().toHexString(); const userLoginMigrationId = new ObjectId().toHexString(); - const exception = new UserLoginMigrationStartLoggableException(userId, userLoginMigrationId); + const exception = new UserLoginMigrationStartLoggable(userId, userLoginMigrationId); return { exception, diff --git a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.ts b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.ts similarity index 80% rename from apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.ts rename to apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.ts index a562c04e0d3..150ce3117b1 100644 --- a/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start-loggable-exception.ts +++ b/apps/server/src/modules/user-login-migration/loggable/user-login-migration-start.loggable.ts @@ -1,8 +1,8 @@ import { EntityId } from '@shared/domain'; import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; -export class UserLoginMigrationStartLoggableException implements Loggable { - constructor(private readonly userId: EntityId, private readonly userLoginMigrationId?: EntityId) {} +export class UserLoginMigrationStartLoggable implements Loggable { + constructor(private readonly userId: EntityId, private readonly userLoginMigrationId: EntityId | undefined) {} getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { return { diff --git a/apps/server/src/modules/user-login-migration/service/migration-check.service.ts b/apps/server/src/modules/user-login-migration/service/migration-check.service.ts index b39592f88ff..2c2836dc2e0 100644 --- a/apps/server/src/modules/user-login-migration/service/migration-check.service.ts +++ b/apps/server/src/modules/user-login-migration/service/migration-check.service.ts @@ -18,20 +18,24 @@ export class MigrationCheckService { officialSchoolNumber: string ): Promise { const school: LegacySchoolDo | null = await this.schoolService.getSchoolBySchoolNumber(officialSchoolNumber); - const userLoginMigration: UserLoginMigrationDO | null = - school && school.id ? await this.userLoginMigrationRepo.findBySchoolId(school.id) : null; - if (!school || !userLoginMigration) { + if (!school?.id) { + return false; + } + + const userLoginMigration: UserLoginMigrationDO | null = await this.userLoginMigrationRepo.findBySchoolId(school.id); + + if (!userLoginMigration || !this.isMigrationActive(userLoginMigration)) { return false; } const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId); - if (this.isMigrationDataValid(user, userLoginMigration)) { - return !this.isUserMigrated(user, userLoginMigration); + if (this.isUserMigrated(user, userLoginMigration)) { + return false; } - return this.isMigrationActive(userLoginMigration); + return true; } private isMigrationDataValid(user: UserDO | null, userLoginMigration: UserLoginMigrationDO): boolean { diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration.service.spec.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration.service.spec.ts index ea50b1b60b9..17dcd716c54 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration.service.spec.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration.service.spec.ts @@ -538,7 +538,7 @@ describe(UserLoginMigrationService.name, () => { it('should call save the user login migration', async () => { const { userLoginMigration } = setup(); - await service.setMigrationMandatory(userLoginMigration); + await service.setMigrationMandatory(userLoginMigration, true); expect(userLoginMigrationRepo.save).toHaveBeenCalledWith(userLoginMigration); }); @@ -559,7 +559,7 @@ describe(UserLoginMigrationService.name, () => { it('should call save the user login migration', async () => { const { userLoginMigration } = setup(); - await service.setMigrationMandatory(userLoginMigration); + await service.setMigrationMandatory(userLoginMigration, true); expect(userLoginMigrationRepo.save).toHaveBeenCalledWith(userLoginMigration); }); @@ -582,7 +582,7 @@ describe(UserLoginMigrationService.name, () => { it('should not save the user login migration again', async () => { const { userLoginMigration } = setup(); - await expect(service.setMigrationMandatory({ ...userLoginMigration })).rejects.toThrow(); + await expect(service.setMigrationMandatory({ ...userLoginMigration }, true)).rejects.toThrow(); expect(userLoginMigrationRepo.save).not.toHaveBeenCalled(); }); @@ -590,7 +590,7 @@ describe(UserLoginMigrationService.name, () => { it('should return throw an error', async () => { const { userLoginMigration, dateInThePast } = setup(); - await expect(service.setMigrationMandatory({ ...userLoginMigration })).rejects.toThrow( + await expect(service.setMigrationMandatory({ ...userLoginMigration }, true)).rejects.toThrow( new UserLoginMigrationGracePeriodExpiredLoggableException(userLoginMigration.id as string, dateInThePast) ); }); @@ -610,7 +610,7 @@ describe(UserLoginMigrationService.name, () => { it('should throw a UserLoginMigrationAlreadyClosedLoggableException', async () => { const { userLoginMigration } = setup(); - const func = async () => service.setMigrationMandatory(userLoginMigration); + const func = async () => service.setMigrationMandatory(userLoginMigration, true); await expect(func).rejects.toThrow(UserLoginMigrationAlreadyClosedLoggableException); }); diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration.service.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration.service.ts index bc3f4aeb969..459b163f119 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration.service.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration.service.ts @@ -47,14 +47,17 @@ export class UserLoginMigrationService { return updatedUserLoginMigration; } - public async setMigrationMandatory(userLoginMigration: UserLoginMigrationDO): Promise { + public async setMigrationMandatory( + userLoginMigration: UserLoginMigrationDO, + mandatory: boolean + ): Promise { this.checkGracePeriod(userLoginMigration); if (userLoginMigration.closedAt) { throw new UserLoginMigrationAlreadyClosedLoggableException(userLoginMigration.closedAt, userLoginMigration.id); } - if (userLoginMigration.mandatorySince) { + if (mandatory) { userLoginMigration.mandatorySince = userLoginMigration.mandatorySince ?? new Date(); } else { userLoginMigration.mandatorySince = undefined; diff --git a/apps/server/src/modules/user-login-migration/uc/restart-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/restart-user-login-migration.uc.ts index 6de441c6644..60be2ca956b 100644 --- a/apps/server/src/modules/user-login-migration/uc/restart-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/restart-user-login-migration.uc.ts @@ -2,7 +2,7 @@ import { AuthorizationContextBuilder, AuthorizationService } from '@modules/auth import { Injectable } from '@nestjs/common/decorators/core/injectable.decorator'; import { Permission, User, UserLoginMigrationDO } from '@shared/domain'; import { Logger } from '@src/core/logger'; -import { UserLoginMigrationNotFoundLoggableException, UserLoginMigrationStartLoggableException } from '../loggable'; +import { UserLoginMigrationNotFoundLoggableException, UserLoginMigrationStartLoggable } from '../loggable'; import { SchoolMigrationService, UserLoginMigrationService } from '../service'; @Injectable() @@ -38,7 +38,7 @@ export class RestartUserLoginMigrationUc { await this.schoolMigrationService.unmarkOutdatedUsers(updatedUserLoginMigration); - this.logger.info(new UserLoginMigrationStartLoggableException(userId, updatedUserLoginMigration.id)); + this.logger.info(new UserLoginMigrationStartLoggable(userId, updatedUserLoginMigration.id)); return updatedUserLoginMigration; } diff --git a/apps/server/src/modules/user-login-migration/uc/start-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/start-user-login-migration.uc.ts index dee97d79487..97199182f0a 100644 --- a/apps/server/src/modules/user-login-migration/uc/start-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/start-user-login-migration.uc.ts @@ -6,7 +6,7 @@ import { Logger } from '@src/core/logger'; import { SchoolNumberMissingLoggableException, UserLoginMigrationAlreadyClosedLoggableException, - UserLoginMigrationStartLoggableException, + UserLoginMigrationStartLoggable, } from '../loggable'; import { UserLoginMigrationService } from '../service'; @@ -31,7 +31,7 @@ export class StartUserLoginMigrationUc { if (!userLoginMigration) { userLoginMigration = await this.userLoginMigrationService.startMigration(schoolId); - this.logger.info(new UserLoginMigrationStartLoggableException(userId, userLoginMigration.id)); + this.logger.info(new UserLoginMigrationStartLoggable(userId, userLoginMigration.id)); } else if (userLoginMigration.closedAt) { throw new UserLoginMigrationAlreadyClosedLoggableException(userLoginMigration.closedAt, userLoginMigration.id); } diff --git a/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.spec.ts b/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.spec.ts index e1dc68a59f1..3416250a67e 100644 --- a/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.spec.ts +++ b/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.spec.ts @@ -95,7 +95,7 @@ describe(ToggleUserLoginMigrationUc.name, () => { await uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), true); - expect(userLoginMigrationService.setMigrationMandatory).toHaveBeenCalledWith(migrationBeforeMandatory); + expect(userLoginMigrationService.setMigrationMandatory).toHaveBeenCalledWith(migrationBeforeMandatory, true); }); it('should return a UserLoginMigration', async () => { @@ -147,7 +147,7 @@ describe(ToggleUserLoginMigrationUc.name, () => { await uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), false); - expect(userLoginMigrationService.setMigrationMandatory).toHaveBeenCalledWith(migrationBeforeOptional); + expect(userLoginMigrationService.setMigrationMandatory).toHaveBeenCalledWith(migrationBeforeOptional, false); }); it('should return a UserLoginMigration', async () => { @@ -179,10 +179,9 @@ describe(ToggleUserLoginMigrationUc.name, () => { it('should throw an exception', async () => { setup(); - const func = async () => - uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), true); - - await expect(func).rejects.toThrow(ForbiddenException); + await expect( + uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), true) + ).rejects.toThrow(ForbiddenException); }); }); @@ -200,10 +199,9 @@ describe(ToggleUserLoginMigrationUc.name, () => { it('should throw a UserLoginMigrationNotFoundLoggableException', async () => { setup(); - const func = async () => - uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), true); - - await expect(func).rejects.toThrow(UserLoginMigrationNotFoundLoggableException); + await expect( + uc.setMigrationMandatory(new ObjectId().toHexString(), new ObjectId().toHexString(), true) + ).rejects.toThrow(UserLoginMigrationNotFoundLoggableException); }); }); }); diff --git a/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.ts index 57d83e1ee5f..cb964ff7fa6 100644 --- a/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/toggle-user-login-migration.uc.ts @@ -26,9 +26,9 @@ export class ToggleUserLoginMigrationUc { throw new UserLoginMigrationNotFoundLoggableException(schoolId); } - userLoginMigration = await this.userLoginMigrationService.setMigrationMandatory(userLoginMigration); + userLoginMigration = await this.userLoginMigrationService.setMigrationMandatory(userLoginMigration, mandatory); - this.logger.debug(new UserLoginMigrationMandatoryLoggable(userId, mandatory, userLoginMigration.id)); + this.logger.debug(new UserLoginMigrationMandatoryLoggable(userId, userLoginMigration.id, mandatory)); return userLoginMigration; }