Fix last TODOs in code.

hpi-schul-cloud · Jan 15, 2024 · ab84809 · ab84809
1 parent 373ddb6
commit ab84809
Show file tree

Hide file tree

Showing 13 changed files with 72 additions and 94 deletions.
diff --git a/apps/server/src/modules/account/entity/account.entity.ts b/apps/server/src/modules/account/entity/account.entity.ts
@@ -1,6 +1,6 @@
 import { Entity, Property, Index } from '@mikro-orm/core';
-import { ObjectId } from '@mikro-orm/mongodb';
 import { BaseEntityWithTimestamps } from '@shared/domain/entity/base.entity';
+import { EntityId } from '@shared/domain/types';
 
 export type IdmAccountProperties = Readonly<Omit<AccountEntity, keyof BaseEntityWithTimestamps>>;
 
@@ -21,10 +21,10 @@ export class AccountEntity extends BaseEntityWithTimestamps {
 	credentialHash?: string;
 
 	@Property({ nullable: true, unique: false })
-	userId?: ObjectId;
+	userId?: EntityId;
 
 	@Property({ nullable: true })
-	systemId?: ObjectId;
+	systemId?: EntityId;
 
 	@Property({ nullable: true })
 	lasttriedFailedLogin?: Date;

diff --git a/apps/server/src/modules/account/repo/account.repo.integration.spec.ts b/apps/server/src/modules/account/repo/account.repo.integration.spec.ts
@@ -53,7 +53,7 @@ describe('account repo', () => {
 	describe('findByUsernameAndSystemId', () => {
 		describe('When username and systemId are given', () => {
 			const setup = async () => {
-				const accountToFind = accountFactory.withSystemId(new ObjectId(10)).build();
+				const accountToFind = accountFactory.withSystemId(new ObjectId(10).toHexString()).build();
 				await em.persistAndFlush(accountToFind);
 				em.clear();
 				return accountToFind;
@@ -68,7 +68,7 @@ describe('account repo', () => {
 
 		describe('When username and systemId are not given', () => {
 			it('should return null', async () => {
-				const account = await repo.findByUsernameAndSystemId('', new ObjectId(undefined));
+				const account = await repo.findByUsernameAndSystemId('', new ObjectId(undefined).toHexString());
 				expect(account).toBeNull();
 			});
 		});

diff --git a/apps/server/src/modules/account/repo/account.repo.ts b/apps/server/src/modules/account/repo/account.repo.ts
@@ -1,5 +1,4 @@
 import { AnyEntity, EntityName, Primary } from '@mikro-orm/core';
-import { ObjectId } from '@mikro-orm/mongodb';
 import { Injectable } from '@nestjs/common';
 import { SortOrder } from '@shared/domain/interface';
 import { Counted, EntityId } from '@shared/domain/types';
@@ -16,23 +15,20 @@ export class AccountRepo extends BaseRepo<AccountEntity> {
 	 * Finds an account by user id.
 	 * @param userId the user id
 	 */
-	// TODO: here only EntityIds should arrive => hard to determine because this is used by feathers/js part
-	async findByUserId(userId: EntityId | ObjectId): Promise<AccountEntity | null> {
-		// TODO: you can use userId directly, without constructing an objectId => AccountEntity still uses ObjectId
-		return this._em.findOne(AccountEntity, { userId: new ObjectId(userId) });
+	async findByUserId(userId: EntityId): Promise<AccountEntity | null> {
+		return this._em.findOne(AccountEntity, { userId });
 	}
 
-	async findMultipleByUserId(userIds: EntityId[] | ObjectId[]): Promise<AccountEntity[]> {
-		const objectIds = userIds.map((id: EntityId | ObjectId) => new ObjectId(id));
-		return this._em.find(AccountEntity, { userId: objectIds });
+	async findMultipleByUserId(userIds: EntityId[]): Promise<AccountEntity[]> {
+		return this._em.find(AccountEntity, { userId: userIds });
 	}
 
-	async findByUserIdOrFail(userId: EntityId | ObjectId): Promise<AccountEntity> {
-		return this._em.findOneOrFail(AccountEntity, { userId: new ObjectId(userId) });
+	async findByUserIdOrFail(userId: EntityId): Promise<AccountEntity> {
+		return this._em.findOneOrFail(AccountEntity, { userId });
 	}
 
-	async findByUsernameAndSystemId(username: string, systemId: EntityId | ObjectId): Promise<AccountEntity | null> {
-		return this._em.findOne(AccountEntity, { username, systemId: new ObjectId(systemId) });
+	async findByUsernameAndSystemId(username: string, systemId: EntityId): Promise<AccountEntity | null> {
+		return this._em.findOne(AccountEntity, { username, systemId });
 	}
 
 	getObjectReference<Entity extends AnyEntity<Entity>>(
@@ -58,7 +54,7 @@ export class AccountRepo extends BaseRepo<AccountEntity> {
 		return this.searchByUsername(username, skip, limit, false);
 	}
 
-	async deleteById(accountId: EntityId | ObjectId): Promise<void> {
+	async deleteById(accountId: EntityId): Promise<void> {
 		const account = await this.findById(accountId);
 		return this.delete(account);
 	}

diff --git a/apps/server/src/modules/account/review-comments.md b/apps/server/src/modules/account/review-comments.md
@@ -1,7 +1,6 @@
 # Review Comments 14.7.23
 
 - Remove lookup service, move to idm and db
-- TODO questions
 
 - write an md file or flow diagram describing how things work
 - in what layer do the services belong?

diff --git a/apps/server/src/modules/account/services/account-db.service.spec.ts b/apps/server/src/modules/account/services/account-db.service.spec.ts
@@ -107,7 +107,7 @@ describe('AccountDbService', () => {
 
 				const mockTeacherAccount = accountFactory.buildWithId();
 
-				accountRepo.findByUserId.mockImplementation((userId: EntityId | ObjectId): Promise<AccountEntity | null> => {
+				accountRepo.findByUserId.mockImplementation((userId: EntityId): Promise<AccountEntity | null> => {
 					if (userId === mockTeacherUser.id) {
 						return Promise.resolve(mockTeacherAccount);
 					}
@@ -134,7 +134,7 @@ describe('AccountDbService', () => {
 	describe('findByUsernameAndSystemId', () => {
 		describe('when user name and system id exists', () => {
 			const setup = () => {
-				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId()).build();
+				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId().toHexString()).build();
 				accountRepo.findByUsernameAndSystemId.mockResolvedValue(mockAccountWithSystemId);
 				return { mockAccountWithSystemId };
 			};
@@ -150,9 +150,9 @@ describe('AccountDbService', () => {
 
 		describe('when only system id exists', () => {
 			const setup = () => {
-				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId()).build();
+				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId().toHexString()).build();
 				accountRepo.findByUsernameAndSystemId.mockImplementation(
-					(username: string, systemId: EntityId | ObjectId): Promise<AccountEntity | null> => {
+					(username: string, systemId: EntityId): Promise<AccountEntity | null> => {
 						if (mockAccountWithSystemId.username === username && mockAccountWithSystemId.systemId === systemId) {
 							return Promise.resolve(mockAccountWithSystemId);
 						}
@@ -173,9 +173,9 @@ describe('AccountDbService', () => {
 
 		describe('when only user name exists', () => {
 			const setup = () => {
-				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId()).build();
+				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId().toHexString()).build();
 				accountRepo.findByUsernameAndSystemId.mockImplementation(
-					(username: string, systemId: EntityId | ObjectId): Promise<AccountEntity | null> => {
+					(username: string, systemId: EntityId): Promise<AccountEntity | null> => {
 						if (mockAccountWithSystemId.username === username && mockAccountWithSystemId.systemId === systemId) {
 							return Promise.resolve(mockAccountWithSystemId);
 						}
@@ -210,14 +210,12 @@ describe('AccountDbService', () => {
 					password: defaultPassword,
 				});
 
-				accountRepo.findMultipleByUserId.mockImplementation(
-					(userIds: (EntityId | ObjectId)[]): Promise<AccountEntity[]> => {
-						const accounts = [mockStudentAccount, mockTeacherAccount].filter((tempAccount) =>
-							userIds.find((userId) => tempAccount.userId?.toString() === userId)
-						);
-						return Promise.resolve(accounts);
-					}
-				);
+				accountRepo.findMultipleByUserId.mockImplementation((userIds: EntityId[]): Promise<AccountEntity[]> => {
+					const accounts = [mockStudentAccount, mockTeacherAccount].filter((tempAccount) =>
+						userIds.find((userId) => tempAccount.userId?.toString() === userId)
+					);
+					return Promise.resolve(accounts);
+				});
 				return { mockStudentUser, mockStudentAccount, mockTeacherUser, mockTeacherAccount };
 			};
 			it('should return multiple accountDtos', async () => {
@@ -234,14 +232,12 @@ describe('AccountDbService', () => {
 				const mockTeacherAccount = accountFactory.buildWithId();
 				const mockStudentAccount = accountFactory.buildWithId();
 
-				accountRepo.findMultipleByUserId.mockImplementation(
-					(userIds: (EntityId | ObjectId)[]): Promise<AccountEntity[]> => {
-						const accounts = [mockStudentAccount, mockTeacherAccount].filter((tempAccount) =>
-							userIds.find((userId) => tempAccount.userId?.toString() === userId)
-						);
-						return Promise.resolve(accounts);
-					}
-				);
+				accountRepo.findMultipleByUserId.mockImplementation((userIds: EntityId[]): Promise<AccountEntity[]> => {
+					const accounts = [mockStudentAccount, mockTeacherAccount].filter((tempAccount) =>
+						userIds.find((userId) => tempAccount.userId?.toString() === userId)
+					);
+					return Promise.resolve(accounts);
+				});
 				return {};
 			};
 			it('should return empty array on mismatch', async () => {
@@ -280,7 +276,7 @@ describe('AccountDbService', () => {
 					userId: mockTeacherUser.id,
 					password: defaultPassword,
 				});
-				accountRepo.findByUserIdOrFail.mockImplementation((userId: EntityId | ObjectId): Promise<AccountEntity> => {
+				accountRepo.findByUserIdOrFail.mockImplementation((userId: EntityId): Promise<AccountEntity> => {
 					if (mockTeacherUser.id === userId) {
 						return Promise.resolve(mockTeacherAccount);
 					}
@@ -304,7 +300,7 @@ describe('AccountDbService', () => {
 				mockTeacherAccountDto.username = '[email protected]';
 				mockTeacherAccountDto.activated = false;
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 				accountRepo.save.mockResolvedValue();
 
 				return { mockTeacherAccountDto, mockTeacherAccount };
@@ -334,7 +330,7 @@ describe('AccountDbService', () => {
 				mockTeacherAccountDto.username = '[email protected]';
 				mockTeacherAccountDto.systemId = '123456789012';
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 				accountRepo.save.mockResolvedValue();
 
 				return { mockTeacherAccountDto, mockTeacherAccount };
@@ -348,7 +344,7 @@ describe('AccountDbService', () => {
 					id: mockTeacherAccount.id,
 					username: mockTeacherAccountDto.username,
 					activated: mockTeacherAccount.activated,
-					systemId: new ObjectId(mockTeacherAccountDto.systemId),
+					systemId: mockTeacherAccountDto.systemId,
 					userId: mockTeacherAccount.userId,
 				});
 			});
@@ -363,7 +359,7 @@ describe('AccountDbService', () => {
 				mockTeacherAccountDto.username = '[email protected]';
 				mockTeacherAccountDto.userId = mockStudentUser.id;
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 				accountRepo.save.mockResolvedValue();
 
 				return { mockStudentUser, mockTeacherAccountDto, mockTeacherAccount };
@@ -378,7 +374,7 @@ describe('AccountDbService', () => {
 					username: mockTeacherAccountDto.username,
 					activated: mockTeacherAccount.activated,
 					systemId: mockTeacherAccount.systemId,
-					userId: new ObjectId(mockStudentUser.id),
+					userId: mockStudentUser.id,
 				});
 			});
 		});
@@ -392,7 +388,7 @@ describe('AccountDbService', () => {
 				mockTeacherAccountDto.systemId = undefined;
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 				accountRepo.save.mockResolvedValue();
 
 				return { mockTeacherAccountDto, mockTeacherAccount };
@@ -436,8 +432,8 @@ describe('AccountDbService', () => {
 				expect(ret).toBeDefined();
 				expect(ret).toMatchObject({
 					username: accountToSave.username,
-					userId: new ObjectId(accountToSave.userId),
-					systemId: new ObjectId(accountToSave.systemId),
+					userId: accountToSave.userId,
+					systemId: accountToSave.systemId,
 					createdAt: accountToSave.createdAt,
 					updatedAt: accountToSave.updatedAt,
 				});
@@ -536,7 +532,7 @@ describe('AccountDbService', () => {
 				} as Account;
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 				accountRepo.save.mockResolvedValue();
 
 				return { mockTeacherAccount, spy, account };
@@ -562,7 +558,7 @@ describe('AccountDbService', () => {
 				const newUsername = 'newUsername';
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { mockTeacherAccount, mockTeacherAccountDto, newUsername };
 			};
@@ -586,7 +582,7 @@ describe('AccountDbService', () => {
 				const theNewDate = new Date();
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { mockTeacherAccount, mockTeacherAccountDto, theNewDate };
 			};
@@ -657,7 +653,7 @@ describe('AccountDbService', () => {
 				const newPassword = 'newPassword';
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { mockTeacherAccount, newPassword };
 			};
@@ -682,14 +678,14 @@ describe('AccountDbService', () => {
 				const mockTeacherAccount = accountFactory.buildWithId();
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { mockTeacherAccount };
 			};
 			it('should delete account via repo', async () => {
 				const { mockTeacherAccount } = setup();
 				await accountService.delete(mockTeacherAccount.id);
-				expect(accountRepo.deleteById).toHaveBeenCalledWith(new ObjectId(mockTeacherAccount.id));
+				expect(accountRepo.deleteById).toHaveBeenCalledWith(new ObjectId(mockTeacherAccount.id).toHexString());
 			});
 		});
 
@@ -721,7 +717,7 @@ describe('AccountDbService', () => {
 				});
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { mockTeacherUser, mockTeacherAccount };
 			};
@@ -742,15 +738,15 @@ describe('AccountDbService', () => {
 				const limit = 10;
 				const mockTeacherAccount = accountFactory.buildWithId();
 				const mockStudentAccount = accountFactory.buildWithId();
-				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId()).build();
+				const mockAccountWithSystemId = accountFactory.withSystemId(new ObjectId().toHexString()).build();
 				const mockAccounts = [mockTeacherAccount, mockStudentAccount, mockAccountWithSystemId];
 
 				accountRepo.findById.mockResolvedValue(mockTeacherAccount);
 				accountRepo.searchByUsernamePartialMatch.mockResolvedValue([
 					[mockTeacherAccount, mockStudentAccount, mockAccountWithSystemId],
 					3,
 				]);
-				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id);
+				accountLookupServiceMock.getInternalId.mockResolvedValue(mockTeacherAccount._id.toHexString());
 
 				return { partialUserName, skip, limit, mockTeacherAccount, mockAccounts };
 			};

diff --git a/apps/server/src/modules/account/services/account-db.service.ts b/apps/server/src/modules/account/services/account-db.service.ts
@@ -39,7 +39,7 @@ export class AccountServiceDb extends AbstractAccountService {
 		return AccountEntityToDoMapper.mapToDto(accountEntity);
 	}
 
-	async findByUsernameAndSystemId(username: string, systemId: EntityId | ObjectId): Promise<Account | null> {
+	async findByUsernameAndSystemId(username: string, systemId: EntityId): Promise<Account | null> {
 		const accountEntity = await this.accountRepo.findByUsernameAndSystemId(username, systemId);
 		return accountEntity ? AccountEntityToDoMapper.mapToDto(accountEntity) : null;
 	}
@@ -51,8 +51,8 @@ export class AccountServiceDb extends AbstractAccountService {
 		if (accountSave.id) {
 			const internalId = await this.getInternalId(accountSave.id);
 			account = await this.accountRepo.findById(internalId);
-			account.userId = new ObjectId(accountSave.userId);
-			account.systemId = accountSave.systemId ? new ObjectId(accountSave.systemId) : undefined;
+			account.userId = accountSave.userId;
+			account.systemId = accountSave.systemId ? accountSave.systemId : undefined;
 			account.username = accountSave.username;
 			account.activated = accountSave.activated;
 			account.expiresAt = accountSave.expiresAt;
@@ -66,8 +66,8 @@ export class AccountServiceDb extends AbstractAccountService {
 			await this.accountRepo.save(account);
 		} else {
 			account = new AccountEntity({
-				userId: new ObjectId(accountSave.userId),
-				systemId: accountSave.systemId ? new ObjectId(accountSave.systemId) : undefined,
+				userId: accountSave.userId,
+				systemId: accountSave.systemId ? accountSave.systemId : undefined,
 				username: accountSave.username,
 				activated: accountSave.activated,
 				expiresAt: accountSave.expiresAt,
@@ -133,7 +133,7 @@ export class AccountServiceDb extends AbstractAccountService {
 		return bcrypt.compare(comparePassword, account.password); // hint: first get result, then return seperately
 	}
 
-	private async getInternalId(id: EntityId | ObjectId): Promise<ObjectId> {
+	private async getInternalId(id: EntityId): Promise<EntityId> {
 		const internalId = await this.accountLookupService.getInternalId(id);
 		if (!internalId) {
 			throw new EntityNotFoundError(`Account with id ${id.toString()} not found`);