Revert "Fix last TODOs in code."

This reverts commit ab84809.

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?: EntityId;
+	userId?: ObjectId;
 
 	@Property({ nullable: true })
-	systemId?: EntityId;
+	systemId?: ObjectId;
 
 	@Property({ nullable: true })
 	lasttriedFailedLogin?: Date;

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).toHexString()).build();
+				const accountToFind = accountFactory.withSystemId(new ObjectId(10)).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).toHexString());
+				const account = await repo.findByUsernameAndSystemId('', new ObjectId(undefined));
 				expect(account).toBeNull();
 			});
 		});

apps/server/src/modules/account/repo/account.repo.ts

@@ -1,4 +1,5 @@
 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';
@@ -15,20 +16,23 @@ export class AccountRepo extends BaseRepo<AccountEntity> {
 	 * Finds an account by user id.
 	 * @param userId the user id
 	 */
-	async findByUserId(userId: EntityId): Promise<AccountEntity | null> {
-		return this._em.findOne(AccountEntity, { userId });
+	// 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 findMultipleByUserId(userIds: EntityId[]): Promise<AccountEntity[]> {
-		return this._em.find(AccountEntity, { userId: userIds });
+	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 findByUserIdOrFail(userId: EntityId): Promise<AccountEntity> {
-		return this._em.findOneOrFail(AccountEntity, { userId });
+	async findByUserIdOrFail(userId: EntityId | ObjectId): Promise<AccountEntity> {
+		return this._em.findOneOrFail(AccountEntity, { userId: new ObjectId(userId) });
 	}
 
-	async findByUsernameAndSystemId(username: string, systemId: EntityId): Promise<AccountEntity | null> {
-		return this._em.findOne(AccountEntity, { username, systemId });
+	async findByUsernameAndSystemId(username: string, systemId: EntityId | ObjectId): Promise<AccountEntity | null> {
+		return this._em.findOne(AccountEntity, { username, systemId: new ObjectId(systemId) });
 	}
 
 	getObjectReference<Entity extends AnyEntity<Entity>>(
@@ -54,7 +58,7 @@ export class AccountRepo extends BaseRepo<AccountEntity> {
 		return this.searchByUsername(username, skip, limit, false);
 	}
 
-	async deleteById(accountId: EntityId): Promise<void> {
+	async deleteById(accountId: EntityId | ObjectId): Promise<void> {
 		const account = await this.findById(accountId);
 		return this.delete(account);
 	}

apps/server/src/modules/account/review-comments.md

@@ -1,6 +1,7 @@
 # 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?

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

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): Promise<Account | null> {
+	async findByUsernameAndSystemId(username: string, systemId: EntityId | ObjectId): 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 = accountSave.userId;
-			account.systemId = accountSave.systemId ? accountSave.systemId : undefined;
+			account.userId = new ObjectId(accountSave.userId);
+			account.systemId = accountSave.systemId ? new ObjectId(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: accountSave.userId,
-				systemId: accountSave.systemId ? accountSave.systemId : undefined,
+				userId: new ObjectId(accountSave.userId),
+				systemId: accountSave.systemId ? new ObjectId(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): Promise<EntityId> {
+	private async getInternalId(id: EntityId | ObjectId): Promise<ObjectId> {
 		const internalId = await this.accountLookupService.getInternalId(id);
 		if (!internalId) {
 			throw new EntityNotFoundError(`Account with id ${id.toString()} not found`);