N21-1447 review fixes

hpi-schul-cloud · Nov 14, 2023 · fc9b53d · fc9b53d
1 parent 1ab3bdf
commit fc9b53d
Show file tree

Hide file tree

Showing 12 changed files with 75 additions and 37 deletions.
diff --git 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/...src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.spec.ts b/...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/...rver/src/modules/user-login-migration/loggable/user-login-migration-mandatory.loggable.ts b/...rver/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/...igration-start-loggable-exception.spec.ts → ...er-login-migration-start.loggable.spec.ts b/...igration-start-loggable-exception.spec.ts → ...er-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/...gin-migration-start-loggable-exception.ts → ...le/user-login-migration-start.loggable.ts b/...gin-migration-start-loggable-exception.ts → ...le/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
@@ -18,20 +18,24 @@ export class MigrationCheckService {
 		officialSchoolNumber: string
 	): Promise<boolean> {
 		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
@@ -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,15 +582,15 @@ 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();
 			});
 
 			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
@@ -47,14 +47,17 @@ export class UserLoginMigrationService {
 		return updatedUserLoginMigration;
 	}
 
-	public async setMigrationMandatory(userLoginMigration: UserLoginMigrationDO): Promise<UserLoginMigrationDO> {
+	public async setMigrationMandatory(
+		userLoginMigration: UserLoginMigrationDO,
+		mandatory: boolean
+	): Promise<UserLoginMigrationDO> {
 		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
@@ -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
@@ -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
@@ -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
@@ -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;
 	}