From d0a785ac6391bee75804e2831d05983a245362ea Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Tue, 29 Oct 2024 16:08:35 +0100 Subject: [PATCH 1/3] get rid of findByExternalIdsAndProvidedBySystemId --- .../modules/idp-console/idp-console.module.ts | 2 + .../idp-console/uc/synchronization.uc.spec.ts | 30 ++++++++++++--- .../idp-console/uc/synchronization.uc.ts | 10 +++-- .../modules/user/service/user.service.spec.ts | 38 +++++++------------ .../src/modules/user/service/user.service.ts | 8 ---- 5 files changed, 47 insertions(+), 41 deletions(-) diff --git a/apps/server/src/modules/idp-console/idp-console.module.ts b/apps/server/src/modules/idp-console/idp-console.module.ts index 84568dcce3d..508d7b514dc 100644 --- a/apps/server/src/modules/idp-console/idp-console.module.ts +++ b/apps/server/src/modules/idp-console/idp-console.module.ts @@ -2,6 +2,7 @@ import { ConsoleWriterModule } from '@infra/console'; import { RabbitMQWrapperModule } from '@infra/rabbitmq'; import { SchulconnexClientModule } from '@infra/schulconnex-client/schulconnex-client.module'; import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { AccountModule } from '@modules/account'; import { defaultMikroOrmOptions } from '@modules/server'; import { SynchronizationEntity, SynchronizationModule } from '@modules/synchronization'; import { UserModule } from '@modules/user'; @@ -30,6 +31,7 @@ import { SynchronizationUc } from './uc'; // debug: true, // use it for locally debugging of queries }), UserModule, + AccountModule, LoggerModule, RabbitMQWrapperModule, ConsoleWriterModule, diff --git a/apps/server/src/modules/idp-console/uc/synchronization.uc.spec.ts b/apps/server/src/modules/idp-console/uc/synchronization.uc.spec.ts index feaf80daf3b..767405c8fd5 100644 --- a/apps/server/src/modules/idp-console/uc/synchronization.uc.spec.ts +++ b/apps/server/src/modules/idp-console/uc/synchronization.uc.spec.ts @@ -13,6 +13,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { setupEntities } from '@shared/testing'; import { createConfigModuleOptions } from '@src/config'; import { Logger } from '@src/core/logger'; +import { AccountService } from '@src/modules/account'; import { ObjectId } from 'bson'; import { FailedUpdateLastSyncedAtLoggableException, @@ -25,6 +26,7 @@ describe(SynchronizationUc.name, () => { let module: TestingModule; let uc: SynchronizationUc; let userService: DeepMocked; + let accountService: DeepMocked; let synchronizationService: DeepMocked; let schulconnexRestClient: DeepMocked; @@ -45,6 +47,10 @@ describe(SynchronizationUc.name, () => { provide: UserService, useValue: createMock(), }, + { + provide: AccountService, + useValue: createMock(), + }, { provide: SchulconnexRestClient, useValue: createMock(), @@ -55,6 +61,7 @@ describe(SynchronizationUc.name, () => { uc = module.get(SynchronizationUc); synchronizationService = module.get(SynchronizationService); userService = module.get(UserService); + accountService = module.get(AccountService); schulconnexRestClient = module.get(SchulconnexRestClient); await setupEntities(); }); @@ -354,26 +361,39 @@ describe(SynchronizationUc.name, () => { const systemId = new ObjectId().toHexString(); const userAId = new ObjectId().toHexString(); const userBId = new ObjectId().toHexString(); - const usersToCheck = [userAId, userBId]; + const userCId = new ObjectId().toHexString(); + const userDId = new ObjectId().toHexString(); + const usersToCheck = [userAId, userBId, userCId, userDId]; + const usersFound = [userAId, userBId, userCId]; const usersToSync = [userAId, userBId]; const userSyncCount = 2; - userService.findByExternalIdsAndProvidedBySystemId.mockResolvedValueOnce(usersToSync); + userService.findMultipleByExternalIds.mockResolvedValueOnce(usersFound); + accountService.findByUserIdsAndSystemId.mockResolvedValueOnce(usersToSync); return { systemId, userSyncCount, usersToCheck, + usersFound, usersToSync, }; }; - it('should call the userService.findByExternalIdsAndProvidedBySystemId to get array of users to sync', async () => { + it('should call the userService.findMultipleByExternalIds to get array of users to sync', async () => { const { systemId, usersToCheck } = setup(); await uc.updateLastSyncedAt(usersToCheck, systemId); - expect(userService.findByExternalIdsAndProvidedBySystemId).toHaveBeenCalledWith(usersToCheck, systemId); + expect(userService.findMultipleByExternalIds).toHaveBeenCalledWith(usersToCheck); + }); + + it('should call the accountService.findByUserIdsAndSystemId confirm users', async () => { + const { systemId, usersToCheck, usersFound } = setup(); + + await uc.updateLastSyncedAt(usersToCheck, systemId); + + expect(accountService.findByUserIdsAndSystemId).toHaveBeenCalledWith(usersFound, systemId); }); it('should call the userService.updateLastSyncedAt to update users', async () => { @@ -401,7 +421,7 @@ describe(SynchronizationUc.name, () => { const usersToCheck = [userAId, userBId]; const usersToSync = [userAId, userBId]; - userService.findByExternalIdsAndProvidedBySystemId.mockResolvedValueOnce(usersToSync); + userService.findMultipleByExternalIds.mockResolvedValueOnce(usersToSync); const error = new Error('testError'); const expectedError = new FailedUpdateLastSyncedAtLoggableException(systemId); diff --git a/apps/server/src/modules/idp-console/uc/synchronization.uc.ts b/apps/server/src/modules/idp-console/uc/synchronization.uc.ts index edd53a3aa11..e254da66461 100644 --- a/apps/server/src/modules/idp-console/uc/synchronization.uc.ts +++ b/apps/server/src/modules/idp-console/uc/synchronization.uc.ts @@ -5,6 +5,7 @@ import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { Logger } from '@src/core/logger'; import { ErrorLogMessage } from '@src/core/logger/types'; +import { AccountService } from '@src/modules/account'; import { SynchronizationConfig } from '../interface'; import { StartSynchronizationLoggable, SucessSynchronizationLoggable } from './loggable'; import { @@ -20,6 +21,7 @@ export class SynchronizationUc { private readonly schulconnexRestClient: SchulconnexRestClient, private readonly synchronizationService: SynchronizationService, private readonly userService: UserService, + private readonly accountService: AccountService, private readonly logger: Logger ) { this.logger.setContext(SynchronizationUc.name); @@ -70,11 +72,13 @@ export class SynchronizationUc { public async updateLastSyncedAt(usersToCheck: string[], systemId: string): Promise { try { - const usersToSync = await this.userService.findByExternalIdsAndProvidedBySystemId(usersToCheck, systemId); + const foundUsers = await this.userService.findMultipleByExternalIds(usersToCheck); - await this.userService.updateLastSyncedAt(usersToSync); + const verifiedUsers = await this.accountService.findByUserIdsAndSystemId(foundUsers, systemId); - return usersToSync.length; + await this.userService.updateLastSyncedAt(verifiedUsers); + + return verifiedUsers.length; } catch { throw new FailedUpdateLastSyncedAtLoggableException(systemId); } diff --git a/apps/server/src/modules/user/service/user.service.spec.ts b/apps/server/src/modules/user/service/user.service.spec.ts index ad5986d259d..8d436ffcea7 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -946,7 +946,7 @@ describe('UserService', () => { }); }); - describe('findByExternalIdsAndProvidedBySystemId', () => { + describe('findMultipleByExternalIds', () => { const setup = () => { const systemId = new ObjectId().toHexString(); const userA = userFactory.buildWithId({ externalId: '111' }); @@ -962,39 +962,27 @@ describe('UserService', () => { }; }; - describe('when find users By externalIds and systemId', () => { - it('should call findMultipleByExternalIds in userService with externalIds', async () => { - const { externalIds, foundUsers, systemId } = setup(); - - jest.spyOn(service, 'findMultipleByExternalIds').mockResolvedValueOnce(foundUsers); - - await service.findByExternalIdsAndProvidedBySystemId(externalIds, systemId); - - expect(service.findMultipleByExternalIds).toHaveBeenCalledWith(externalIds); - }); - - it('should call accountService.findByUserIdsAndSystemId with foundUsers and systemId', async () => { - const { externalIds, foundUsers, systemId } = setup(); + it('should call findMultipleByExternalIds in userService with externalIds', async () => { + const { externalIds, foundUsers } = setup(); - jest.spyOn(service, 'findMultipleByExternalIds').mockResolvedValueOnce(foundUsers); + userRepo.findByExternalIds.mockResolvedValueOnce(foundUsers); - await service.findByExternalIdsAndProvidedBySystemId(externalIds, systemId); + await service.findMultipleByExternalIds(externalIds); - expect(accountService.findByUserIdsAndSystemId).toHaveBeenCalledWith(foundUsers, systemId); - }); + expect(userRepo.findByExternalIds).toHaveBeenCalledWith(externalIds); + }); - it('should return array with verified Users', async () => { - const { externalIds, foundUsers, systemId } = setup(); + it('should return array with verified Users', async () => { + const { externalIds, foundUsers } = setup(); - jest.spyOn(service, 'findMultipleByExternalIds').mockResolvedValueOnce(foundUsers); - jest.spyOn(accountService, 'findByUserIdsAndSystemId').mockResolvedValueOnce(foundUsers); + userRepo.findByExternalIds.mockResolvedValueOnce(foundUsers); - const result = await service.findByExternalIdsAndProvidedBySystemId(externalIds, systemId); + const result = await service.findMultipleByExternalIds(externalIds); - expect(result).toEqual(foundUsers); - }); + expect(result).toEqual(foundUsers); }); }); + describe('findUnsynchronizedUserIds', () => { const setup = () => { const currentDate = new Date(); diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index 2e788707300..606e3a0da11 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -261,14 +261,6 @@ export class UserService implements DeletionService, IEventHandler { - const foundUsers = await this.findMultipleByExternalIds(externalIds); - - const verifiedUsers = await this.accountService.findByUserIdsAndSystemId(foundUsers, systemId); - - return verifiedUsers; - } - public async findMultipleByExternalIds(externalIds: string[]): Promise { return this.userRepo.findByExternalIds(externalIds); } From ccc3e234a4bb4dc53e900dd81e59166715b6d5cf Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Wed, 30 Oct 2024 09:57:21 +0100 Subject: [PATCH 2/3] remove accountModule from UserModule Dependencies --- .../modules/user/service/user.service.spec.ts | 56 ------------------- .../src/modules/user/service/user.service.ts | 14 ----- apps/server/src/modules/user/user.module.ts | 2 - 3 files changed, 72 deletions(-) diff --git a/apps/server/src/modules/user/service/user.service.spec.ts b/apps/server/src/modules/user/service/user.service.spec.ts index 8d436ffcea7..9cc8efd2ede 100644 --- a/apps/server/src/modules/user/service/user.service.spec.ts +++ b/apps/server/src/modules/user/service/user.service.spec.ts @@ -1,9 +1,7 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { ICurrentUser } from '@infra/auth-guard'; import { CalendarService } from '@infra/calendar'; import { EntityManager, MikroORM } from '@mikro-orm/core'; import { ObjectId } from '@mikro-orm/mongodb'; -import { Account, AccountService } from '@modules/account'; import { DataDeletedEvent, DeletionErrorLoggableException, @@ -41,7 +39,6 @@ describe('UserService', () => { let userDORepo: DeepMocked; let config: DeepMocked; let roleService: DeepMocked; - let accountService: DeepMocked; let registrationPinService: DeepMocked; let calendarService: DeepMocked; let eventBus: DeepMocked; @@ -72,10 +69,6 @@ describe('UserService', () => { provide: RoleService, useValue: createMock(), }, - { - provide: AccountService, - useValue: createMock(), - }, { provide: RegistrationPinService, useValue: createMock(), @@ -106,7 +99,6 @@ describe('UserService', () => { userDORepo = module.get(UserDORepo); config = module.get(ConfigService); roleService = module.get(RoleService); - accountService = module.get(AccountService); registrationPinService = module.get(RegistrationPinService); eventBus = module.get(EventBus); calendarService = module.get(CalendarService); @@ -261,54 +253,6 @@ describe('UserService', () => { }); }); - describe('getResolvedUser is called', () => { - describe('when a resolved user is requested', () => { - const setup = () => { - const systemId = 'systemId'; - const role: Role = roleFactory.buildWithId({ - name: RoleName.STUDENT, - permissions: [Permission.DASHBOARD_VIEW], - }); - const user: UserDO = userDoFactory.buildWithId({ roles: [role] }); - const account: Account = new Account({ - id: 'accountId', - systemId, - username: 'username', - createdAt: new Date(), - updatedAt: new Date(), - activated: true, - }); - - userDORepo.findById.mockResolvedValue(user); - accountService.findByUserIdOrFail.mockResolvedValue(account); - - return { - userId: user.id as string, - user, - account, - role, - systemId, - }; - }; - - it('should return the current user', async () => { - const { userId, user, account, role, systemId } = setup(); - - const result = await service.getResolvedUser(userId); - - expect(result).toEqual({ - userId, - systemId, - schoolId: user.schoolId, - accountId: account.id, - roles: [role.id], - isExternalUser: false, - support: false, - }); - }); - }); - }); - describe('getDisplayName', () => { let role: Role; diff --git a/apps/server/src/modules/user/service/user.service.ts b/apps/server/src/modules/user/service/user.service.ts index 606e3a0da11..4546dcb9226 100644 --- a/apps/server/src/modules/user/service/user.service.ts +++ b/apps/server/src/modules/user/service/user.service.ts @@ -1,8 +1,4 @@ import { MikroORM, UseRequestContext } from '@mikro-orm/core'; -import { Account, AccountService } from '@modules/account'; -// invalid import -import { OauthCurrentUser } from '@modules/authentication/interface'; -import { CurrentUserMapper } from '@modules/authentication/mapper'; import { DataDeletedEvent, DataDeletionDomainOperationLoggable, @@ -44,7 +40,6 @@ export class UserService implements DeletionService, IEventHandler, private readonly roleService: RoleService, - private readonly accountService: AccountService, private readonly registrationPinService: RegistrationPinService, private readonly calendarService: CalendarService, private readonly logger: Logger, @@ -84,15 +79,6 @@ export class UserService implements DeletionService, IEventHandler { - const user: UserDO = await this.findById(userId); - const account: Account = await this.accountService.findByUserIdOrFail(userId); - - const resolvedUser: OauthCurrentUser = CurrentUserMapper.mapToOauthCurrentUser(account.id, user, account.systemId); - - return resolvedUser; - } - async findById(id: string): Promise { const userDO = await this.userDORepo.findById(id, true); diff --git a/apps/server/src/modules/user/user.module.ts b/apps/server/src/modules/user/user.module.ts index 501226c6d5f..ac2c39285a8 100644 --- a/apps/server/src/modules/user/user.module.ts +++ b/apps/server/src/modules/user/user.module.ts @@ -1,4 +1,3 @@ -import { AccountModule } from '@modules/account'; import { LegacySchoolModule } from '@modules/legacy-school'; import { RoleModule } from '@modules/role'; import { forwardRef, Module } from '@nestjs/common'; @@ -14,7 +13,6 @@ import { UserService } from './service'; imports: [ forwardRef(() => LegacySchoolModule), RoleModule, - AccountModule, LoggerModule, CqrsModule, RegistrationPinModule, From a6a681dd8f3f7ebda94cd15a06635882bdebc0a2 Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Wed, 30 Oct 2024 12:45:10 +0100 Subject: [PATCH 3/3] remove LegacyschoolModule from UserModule --- apps/server/src/modules/user/user.module.ts | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/apps/server/src/modules/user/user.module.ts b/apps/server/src/modules/user/user.module.ts index ac2c39285a8..10f0d678834 100644 --- a/apps/server/src/modules/user/user.module.ts +++ b/apps/server/src/modules/user/user.module.ts @@ -1,23 +1,15 @@ -import { LegacySchoolModule } from '@modules/legacy-school'; +import { CalendarModule } from '@infra/calendar'; +import { RegistrationPinModule } from '@modules/registration-pin'; import { RoleModule } from '@modules/role'; -import { forwardRef, Module } from '@nestjs/common'; +import { Module } from '@nestjs/common'; +import { CqrsModule } from '@nestjs/cqrs'; import { UserRepo } from '@shared/repo'; import { UserDORepo } from '@shared/repo/user/user-do.repo'; import { LoggerModule } from '@src/core/logger'; -import { CqrsModule } from '@nestjs/cqrs'; -import { RegistrationPinModule } from '@modules/registration-pin'; -import { CalendarModule } from '@infra/calendar'; import { UserService } from './service'; @Module({ - imports: [ - forwardRef(() => LegacySchoolModule), - RoleModule, - LoggerModule, - CqrsModule, - RegistrationPinModule, - CalendarModule, - ], + imports: [RoleModule, LoggerModule, CqrsModule, RegistrationPinModule, CalendarModule], providers: [UserRepo, UserDORepo, UserService], exports: [UserService, UserRepo], })