From 8ea4d65d0e4f135442fc34ff5fa1cbb01ae7fb8e Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Fri, 25 Aug 2023 14:16:18 +0200 Subject: [PATCH 01/10] N21-841 fix close migration --- .../service/migration-check.service.ts | 7 +++++- ...ser-login-migration-revert.service.spec.ts | 18 ++++++++++++- .../user-login-migration-revert.service.ts | 14 ++++++++++- .../service/user-login-migration.service.ts | 6 +++-- .../uc/close-user-login-migration.uc.spec.ts | 25 +++++++++++++++++++ .../uc/close-user-login-migration.uc.ts | 8 +++--- .../domainobject/user-login-migration.do.ts | 2 +- .../entity/user-login-migration.entity.ts | 2 +- 8 files changed, 72 insertions(+), 10 deletions(-) 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 d2bfc6cc065..035c58ae8b6 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 @@ -24,7 +24,12 @@ export class MigrationCheckService { const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId); - if (user?.lastLoginSystemChange && userLoginMigration && !userLoginMigration.closedAt) { + if ( + user?.lastLoginSystemChange && + userLoginMigration && + !userLoginMigration.closedAt && + userLoginMigration.startedAt + ) { const hasMigrated: boolean = user.lastLoginSystemChange > userLoginMigration.startedAt; return !hasMigrated; } diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts index 66b13122d13..ee7406e93cc 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts @@ -1,6 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { SchoolFeatures } from '@shared/domain'; +import { SchoolFeatures, UserLoginMigrationDO } from '@shared/domain'; import { SchoolService } from '@src/modules/school'; import { setupEntities, userLoginMigrationDOFactory } from '@shared/testing'; import { UserLoginMigrationRevertService } from './user-login-migration-revert.service'; @@ -71,6 +71,22 @@ describe('UserLoginMigrationRevertService', () => { expect(userLoginMigrationService.deleteUserLoginMigration).toHaveBeenCalledWith(userLoginMigration); }); + + it('should return reverted user login migration', async () => { + const { userLoginMigration } = setup(); + + const result: UserLoginMigrationDO = await service.revertUserLoginMigration(userLoginMigration); + + expect(result).toEqual({ + schoolId: userLoginMigration.schoolId, + targetSystemId: userLoginMigration.targetSystemId, + startedAt: undefined, + finishedAt: undefined, + mandatorySince: undefined, + closedAt: undefined, + sourceSystemId: undefined, + }); + }); }); }); }); diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts index 105f546fc0c..f843bb4af3b 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts @@ -10,8 +10,20 @@ export class UserLoginMigrationRevertService { private readonly schoolService: SchoolService ) {} - async revertUserLoginMigration(userLoginMigration: UserLoginMigrationDO): Promise { + async revertUserLoginMigration(userLoginMigration: UserLoginMigrationDO): Promise { await this.schoolService.removeFeature(userLoginMigration.schoolId, SchoolFeatures.OAUTH_PROVISIONING_ENABLED); await this.userLoginMigrationService.deleteUserLoginMigration(userLoginMigration); + + const revertedUserLoginMigration = new UserLoginMigrationDO({ + schoolId: userLoginMigration.schoolId, + targetSystemId: userLoginMigration.targetSystemId, + startedAt: undefined, + finishedAt: undefined, + mandatorySince: undefined, + closedAt: undefined, + sourceSystemId: undefined, + }); + + return revertedUserLoginMigration; } } 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 2808775a653..95aba3963b3 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 @@ -195,8 +195,10 @@ export class UserLoginMigrationService { return null; } - const hasUserMigrated: boolean = - !!userDO.lastLoginSystemChange && userDO.lastLoginSystemChange > userLoginMigration.startedAt; + let hasUserMigrated = false; + if (userLoginMigration.startedAt) { + hasUserMigrated = !!userDO.lastLoginSystemChange && userDO.lastLoginSystemChange > userLoginMigration.startedAt; + } if (hasUserMigrated) { return null; diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts index 1ea3fa50954..4f01f77cdfe 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts @@ -152,6 +152,14 @@ describe('CloseUserLoginMigrationUc', () => { authorizationService.getUserWithPermissions.mockResolvedValue(user); userLoginMigrationService.closeMigration.mockResolvedValue(closedUserLoginMigration); schoolMigrationService.hasSchoolMigratedUser.mockResolvedValue(false); + userLoginMigrationRevertService.revertUserLoginMigration.mockResolvedValue({ + ...closedUserLoginMigration, + startedAt: undefined, + closedAt: undefined, + finishedAt: undefined, + sourceSystemId: undefined, + mandatorySince: undefined, + }); return { user, @@ -179,6 +187,23 @@ describe('CloseUserLoginMigrationUc', () => { expect(userLoginMigrationRevertService.revertUserLoginMigration).toHaveBeenCalledWith(closedUserLoginMigration); }); + + it('should return reverted user login migration', async () => { + const { user, schoolId, closedUserLoginMigration } = setup(); + + const result: UserLoginMigrationDO = await uc.closeMigration(user.id, schoolId); + + expect(result).toEqual({ + id: closedUserLoginMigration.id, + schoolId: closedUserLoginMigration.schoolId, + targetSystemId: closedUserLoginMigration.targetSystemId, + startedAt: undefined, + finishedAt: undefined, + mandatorySince: undefined, + closedAt: undefined, + sourceSystemId: undefined, + }); + }); }); describe('when the user login migration was already closed', () => { diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts index ad6646edae8..638f084d1a5 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts @@ -46,10 +46,12 @@ export class CloseUserLoginMigrationUc { const hasSchoolMigratedUser: boolean = await this.schoolMigrationService.hasSchoolMigratedUser(schoolId); if (!hasSchoolMigratedUser) { - await this.userLoginMigrationRevertService.revertUserLoginMigration(updatedUserLoginMigration); - } else { - await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); + const revertedUserLoginMigration: UserLoginMigrationDO = + await this.userLoginMigrationRevertService.revertUserLoginMigration(updatedUserLoginMigration); + + return revertedUserLoginMigration; } + await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); return updatedUserLoginMigration; } diff --git a/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts b/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts index 88e58aa8a2d..a80d9935278 100644 --- a/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts +++ b/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts @@ -10,7 +10,7 @@ export class UserLoginMigrationDO extends BaseDO { mandatorySince?: Date; - startedAt: Date; + startedAt?: Date; closedAt?: Date; diff --git a/apps/server/src/shared/domain/entity/user-login-migration.entity.ts b/apps/server/src/shared/domain/entity/user-login-migration.entity.ts index 6e24e7338e6..d74935ab711 100644 --- a/apps/server/src/shared/domain/entity/user-login-migration.entity.ts +++ b/apps/server/src/shared/domain/entity/user-login-migration.entity.ts @@ -21,7 +21,7 @@ export class UserLoginMigration extends BaseEntityWithTimestamps { mandatorySince?: Date; @Property() - startedAt: Date; + startedAt?: Date; @Property({ nullable: true }) closedAt?: Date; From 95c4850898d0b4dd121fa74f6c408af95ef837e0 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Fri, 25 Aug 2023 15:49:06 +0200 Subject: [PATCH 02/10] N21-841 fix startedAt --- .../controller/dto/response/user-login-migration.response.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts b/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts index 51c02793d09..c7f54bde511 100644 --- a/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts +++ b/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts @@ -19,7 +19,7 @@ export class UserLoginMigrationResponse { @ApiProperty({ description: 'Date when the migration was started', }) - startedAt: Date; + startedAt?: Date; @ApiPropertyOptional({ description: 'Date when the migration was completed', From 30754402ab1a401d82e2d59bedcf9b58ef8cf10c Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Fri, 8 Sep 2023 07:52:59 +0200 Subject: [PATCH 03/10] N21-1247 fix restart --- .../controller/api-test/user-login-migration.api.spec.ts | 8 ++++---- .../service/user-login-migration.service.ts | 1 - .../uc/restart-user-login-migration.uc.ts | 2 -- .../uc/start-user-login-migration.uc.ts | 2 -- 4 files changed, 4 insertions(+), 9 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts index 24dfd650bd8..c1d04e8e0f7 100644 --- a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts +++ b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts @@ -109,7 +109,7 @@ describe('UserLoginMigrationController (API)', () => { { sourceSystemId: sourceSystem.id, targetSystemId: targetSystem.id, - startedAt: userLoginMigration.startedAt.toISOString(), + startedAt: userLoginMigration.startedAt?.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), finishedAt: userLoginMigration.finishedAt?.toISOString(), mandatorySince: userLoginMigration.mandatorySince?.toISOString(), @@ -173,7 +173,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ sourceSystemId: sourceSystem.id, targetSystemId: targetSystem.id, - startedAt: userLoginMigration.startedAt.toISOString(), + startedAt: userLoginMigration.startedAt?.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), finishedAt: userLoginMigration.finishedAt?.toISOString(), mandatorySince: userLoginMigration.mandatorySince?.toISOString(), @@ -1030,7 +1030,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ targetSystemId: userLoginMigration.targetSystem.id, sourceSystemId: userLoginMigration.sourceSystem?.id, - startedAt: userLoginMigration.startedAt.toISOString(), + startedAt: userLoginMigration.startedAt?.toISOString(), closedAt: expect.any(String), finishedAt: expect.any(String), }); @@ -1109,7 +1109,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ targetSystemId: userLoginMigration.targetSystem.id, sourceSystemId: userLoginMigration.sourceSystem?.id, - startedAt: userLoginMigration.startedAt.toISOString(), + startedAt: userLoginMigration.startedAt?.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), }); }); 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 95aba3963b3..2ba7810fa08 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 @@ -162,7 +162,6 @@ export class UserLoginMigrationService { } private async updateExistingMigration(userLoginMigrationDO: UserLoginMigrationDO) { - userLoginMigrationDO.startedAt = new Date(); userLoginMigrationDO.closedAt = undefined; userLoginMigrationDO.finishedAt = 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 a515755830a..8ae7d3f5701 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 @@ -39,8 +39,6 @@ export class RestartUserLoginMigrationUc { userLoginMigration = await this.userLoginMigrationService.restartMigration(schoolId); this.logger.info(new UserLoginMigrationStartLoggable(userId, schoolId)); - } else { - // Do nothing, if migration is already started but not stopped. } return userLoginMigration; 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 d97fe1d0ff8..44703bb76e2 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 @@ -34,8 +34,6 @@ export class StartUserLoginMigrationUc { userLoginMigration.id as string, userLoginMigration.closedAt ); - } else { - // Do nothing, if migration is already started but not stopped. } return userLoginMigration; From 9e2e520de7ec205915abb4f5f691b43a1b8946ea Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Mon, 18 Sep 2023 08:48:20 +0200 Subject: [PATCH 04/10] N21-1247 revert close migration --- .../service/user-login-migration-revert.service.ts | 14 +------------- .../uc/close-user-login-migration.uc.ts | 8 +++----- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts index f843bb4af3b..105f546fc0c 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.ts @@ -10,20 +10,8 @@ export class UserLoginMigrationRevertService { private readonly schoolService: SchoolService ) {} - async revertUserLoginMigration(userLoginMigration: UserLoginMigrationDO): Promise { + async revertUserLoginMigration(userLoginMigration: UserLoginMigrationDO): Promise { await this.schoolService.removeFeature(userLoginMigration.schoolId, SchoolFeatures.OAUTH_PROVISIONING_ENABLED); await this.userLoginMigrationService.deleteUserLoginMigration(userLoginMigration); - - const revertedUserLoginMigration = new UserLoginMigrationDO({ - schoolId: userLoginMigration.schoolId, - targetSystemId: userLoginMigration.targetSystemId, - startedAt: undefined, - finishedAt: undefined, - mandatorySince: undefined, - closedAt: undefined, - sourceSystemId: undefined, - }); - - return revertedUserLoginMigration; } } diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts index 638f084d1a5..ad6646edae8 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts @@ -46,12 +46,10 @@ export class CloseUserLoginMigrationUc { const hasSchoolMigratedUser: boolean = await this.schoolMigrationService.hasSchoolMigratedUser(schoolId); if (!hasSchoolMigratedUser) { - const revertedUserLoginMigration: UserLoginMigrationDO = - await this.userLoginMigrationRevertService.revertUserLoginMigration(updatedUserLoginMigration); - - return revertedUserLoginMigration; + await this.userLoginMigrationRevertService.revertUserLoginMigration(updatedUserLoginMigration); + } else { + await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); } - await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); return updatedUserLoginMigration; } From bbad4d91eed3987862fabb443d7b7e46737de245 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Tue, 19 Sep 2023 09:22:25 +0200 Subject: [PATCH 05/10] revert close migration tests --- ...ser-login-migration-revert.service.spec.ts | 18 +------------ .../uc/close-user-login-migration.uc.spec.ts | 25 ------------------- 2 files changed, 1 insertion(+), 42 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts index ee7406e93cc..66b13122d13 100644 --- a/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts +++ b/apps/server/src/modules/user-login-migration/service/user-login-migration-revert.service.spec.ts @@ -1,6 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { SchoolFeatures, UserLoginMigrationDO } from '@shared/domain'; +import { SchoolFeatures } from '@shared/domain'; import { SchoolService } from '@src/modules/school'; import { setupEntities, userLoginMigrationDOFactory } from '@shared/testing'; import { UserLoginMigrationRevertService } from './user-login-migration-revert.service'; @@ -71,22 +71,6 @@ describe('UserLoginMigrationRevertService', () => { expect(userLoginMigrationService.deleteUserLoginMigration).toHaveBeenCalledWith(userLoginMigration); }); - - it('should return reverted user login migration', async () => { - const { userLoginMigration } = setup(); - - const result: UserLoginMigrationDO = await service.revertUserLoginMigration(userLoginMigration); - - expect(result).toEqual({ - schoolId: userLoginMigration.schoolId, - targetSystemId: userLoginMigration.targetSystemId, - startedAt: undefined, - finishedAt: undefined, - mandatorySince: undefined, - closedAt: undefined, - sourceSystemId: undefined, - }); - }); }); }); }); diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts index 4f01f77cdfe..1ea3fa50954 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts @@ -152,14 +152,6 @@ describe('CloseUserLoginMigrationUc', () => { authorizationService.getUserWithPermissions.mockResolvedValue(user); userLoginMigrationService.closeMigration.mockResolvedValue(closedUserLoginMigration); schoolMigrationService.hasSchoolMigratedUser.mockResolvedValue(false); - userLoginMigrationRevertService.revertUserLoginMigration.mockResolvedValue({ - ...closedUserLoginMigration, - startedAt: undefined, - closedAt: undefined, - finishedAt: undefined, - sourceSystemId: undefined, - mandatorySince: undefined, - }); return { user, @@ -187,23 +179,6 @@ describe('CloseUserLoginMigrationUc', () => { expect(userLoginMigrationRevertService.revertUserLoginMigration).toHaveBeenCalledWith(closedUserLoginMigration); }); - - it('should return reverted user login migration', async () => { - const { user, schoolId, closedUserLoginMigration } = setup(); - - const result: UserLoginMigrationDO = await uc.closeMigration(user.id, schoolId); - - expect(result).toEqual({ - id: closedUserLoginMigration.id, - schoolId: closedUserLoginMigration.schoolId, - targetSystemId: closedUserLoginMigration.targetSystemId, - startedAt: undefined, - finishedAt: undefined, - mandatorySince: undefined, - closedAt: undefined, - sourceSystemId: undefined, - }); - }); }); describe('when the user login migration was already closed', () => { From c68fab9f7eeeb93d56d8814926af6581433d1d73 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Wed, 20 Sep 2023 11:07:18 +0200 Subject: [PATCH 06/10] fix close endpoint --- .../user-login-migration.controller.ts | 16 ++++++++++------ .../uc/close-user-login-migration.uc.ts | 6 +++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/controller/user-login-migration.controller.ts b/apps/server/src/modules/user-login-migration/controller/user-login-migration.controller.ts index 7ef6242eb72..19098167d96 100644 --- a/apps/server/src/modules/user-login-migration/controller/user-login-migration.controller.ts +++ b/apps/server/src/modules/user-login-migration/controller/user-login-migration.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, Get, Param, Post, Put, Query } from '@nestjs/common'; import { ApiForbiddenResponse, ApiInternalServerErrorResponse, + ApiNoContentResponse, ApiNotFoundResponse, ApiOkResponse, ApiOperation, @@ -196,16 +197,19 @@ export class UserLoginMigrationController { @ApiOkResponse({ description: 'User login migration closed', type: UserLoginMigrationResponse }) @ApiUnauthorizedResponse() @ApiForbiddenResponse() - async closeMigration(@CurrentUser() currentUser: ICurrentUser): Promise { - const userLoginMigration: UserLoginMigrationDO = await this.closeUserLoginMigrationUc.closeMigration( + @ApiNoContentResponse({ description: 'User login migration was reverted' }) + async closeMigration(@CurrentUser() currentUser: ICurrentUser): Promise { + const userLoginMigration: UserLoginMigrationDO | undefined = await this.closeUserLoginMigrationUc.closeMigration( currentUser.userId, currentUser.schoolId ); - const migrationResponse: UserLoginMigrationResponse = - UserLoginMigrationMapper.mapUserLoginMigrationDoToResponse(userLoginMigration); - - return migrationResponse; + if (userLoginMigration) { + const migrationResponse: UserLoginMigrationResponse = + UserLoginMigrationMapper.mapUserLoginMigrationDoToResponse(userLoginMigration); + return migrationResponse; + } + return undefined; } @Post('migrate-to-oauth2') diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts index ad6646edae8..13058e64c49 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.ts @@ -16,7 +16,7 @@ export class CloseUserLoginMigrationUc { private readonly authorizationService: AuthorizationService ) {} - async closeMigration(userId: EntityId, schoolId: EntityId): Promise { + async closeMigration(userId: EntityId, schoolId: EntityId): Promise { const userLoginMigration: UserLoginMigrationDO | null = await this.userLoginMigrationService.findMigrationBySchool( schoolId ); @@ -47,9 +47,9 @@ export class CloseUserLoginMigrationUc { if (!hasSchoolMigratedUser) { await this.userLoginMigrationRevertService.revertUserLoginMigration(updatedUserLoginMigration); - } else { - await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); + return undefined; } + await this.schoolMigrationService.markUnmigratedUsersAsOutdated(schoolId); return updatedUserLoginMigration; } From 6ce024a1982d5dcb9f0b8a2d973531897ad07447 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Thu, 21 Sep 2023 08:48:32 +0200 Subject: [PATCH 07/10] fix optional startedAt --- .../controller/api-test/user-login-migration.api.spec.ts | 8 ++++---- .../dto/response/user-login-migration.response.ts | 2 +- .../service/migration-check.service.ts | 7 +------ .../service/user-login-migration.service.ts | 6 ++---- .../shared/domain/domainobject/user-login-migration.do.ts | 2 +- .../shared/domain/entity/user-login-migration.entity.ts | 2 +- 6 files changed, 10 insertions(+), 17 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts index c6c86cf0cbe..9f19a3ae71b 100644 --- a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts +++ b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts @@ -109,7 +109,7 @@ describe('UserLoginMigrationController (API)', () => { { sourceSystemId: sourceSystem.id, targetSystemId: targetSystem.id, - startedAt: userLoginMigration.startedAt?.toISOString(), + startedAt: userLoginMigration.startedAt.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), finishedAt: userLoginMigration.finishedAt?.toISOString(), mandatorySince: userLoginMigration.mandatorySince?.toISOString(), @@ -173,7 +173,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ sourceSystemId: sourceSystem.id, targetSystemId: targetSystem.id, - startedAt: userLoginMigration.startedAt?.toISOString(), + startedAt: userLoginMigration.startedAt.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), finishedAt: userLoginMigration.finishedAt?.toISOString(), mandatorySince: userLoginMigration.mandatorySince?.toISOString(), @@ -1029,7 +1029,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ targetSystemId: userLoginMigration.targetSystem.id, sourceSystemId: userLoginMigration.sourceSystem?.id, - startedAt: userLoginMigration.startedAt?.toISOString(), + startedAt: userLoginMigration.startedAt.toISOString(), closedAt: expect.any(String), finishedAt: expect.any(String), }); @@ -1108,7 +1108,7 @@ describe('UserLoginMigrationController (API)', () => { expect(response.body).toEqual({ targetSystemId: userLoginMigration.targetSystem.id, sourceSystemId: userLoginMigration.sourceSystem?.id, - startedAt: userLoginMigration.startedAt?.toISOString(), + startedAt: userLoginMigration.startedAt.toISOString(), closedAt: userLoginMigration.closedAt?.toISOString(), }); }); diff --git a/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts b/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts index c7f54bde511..51c02793d09 100644 --- a/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts +++ b/apps/server/src/modules/user-login-migration/controller/dto/response/user-login-migration.response.ts @@ -19,7 +19,7 @@ export class UserLoginMigrationResponse { @ApiProperty({ description: 'Date when the migration was started', }) - startedAt?: Date; + startedAt: Date; @ApiPropertyOptional({ description: 'Date when the migration was completed', 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 ca8d4cc59a5..7a0596e3599 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 @@ -22,12 +22,7 @@ export class MigrationCheckService { const user: UserDO | null = await this.userService.findByExternalId(externalUserId, systemId); - if ( - user?.lastLoginSystemChange && - userLoginMigration && - !userLoginMigration.closedAt && - userLoginMigration.startedAt - ) { + if (user?.lastLoginSystemChange && userLoginMigration && !userLoginMigration.closedAt) { const hasMigrated: boolean = user.lastLoginSystemChange > userLoginMigration.startedAt; return !hasMigrated; } 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 f608a6d3060..9f3d6c59e84 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 @@ -201,10 +201,8 @@ export class UserLoginMigrationService { return null; } - let hasUserMigrated = false; - if (userLoginMigration.startedAt) { - hasUserMigrated = !!userDO.lastLoginSystemChange && userDO.lastLoginSystemChange > userLoginMigration.startedAt; - } + const hasUserMigrated: boolean = + !!userDO.lastLoginSystemChange && userDO.lastLoginSystemChange > userLoginMigration.startedAt; if (hasUserMigrated) { return null; diff --git a/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts b/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts index a80d9935278..88e58aa8a2d 100644 --- a/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts +++ b/apps/server/src/shared/domain/domainobject/user-login-migration.do.ts @@ -10,7 +10,7 @@ export class UserLoginMigrationDO extends BaseDO { mandatorySince?: Date; - startedAt?: Date; + startedAt: Date; closedAt?: Date; diff --git a/apps/server/src/shared/domain/entity/user-login-migration.entity.ts b/apps/server/src/shared/domain/entity/user-login-migration.entity.ts index a596a0fcfce..15703688aff 100644 --- a/apps/server/src/shared/domain/entity/user-login-migration.entity.ts +++ b/apps/server/src/shared/domain/entity/user-login-migration.entity.ts @@ -21,7 +21,7 @@ export class UserLoginMigration extends BaseEntityWithTimestamps { mandatorySince?: Date; @Property() - startedAt?: Date; + startedAt: Date; @Property({ nullable: true }) closedAt?: Date; From 801ba56ec5eb38635d4dd3155a330588f80bb557 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Mon, 25 Sep 2023 14:53:41 +0200 Subject: [PATCH 08/10] cov up --- .../api-test/user-login-migration.api.spec.ts | 49 +++++++++++++++++++ .../uc/close-user-login-migration.uc.spec.ts | 16 ++++++ 2 files changed, 65 insertions(+) diff --git a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts index 9f19a3ae71b..07ff7f53abb 100644 --- a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts +++ b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts @@ -1202,5 +1202,54 @@ describe('UserLoginMigrationController (API)', () => { expect(response.status).toEqual(HttpStatus.FORBIDDEN); }); }); + + describe('when no user has migrate', () => { + const setup = async () => { + const sourceSystem: System = systemFactory.withLdapConfig().buildWithId({ alias: 'SourceSystem' }); + const targetSystem: System = systemFactory.withOauthConfig().buildWithId({ alias: 'SANIS' }); + const school: SchoolEntity = schoolFactory.buildWithId({ + systems: [sourceSystem], + officialSchoolNumber: '12345', + }); + const userLoginMigration: UserLoginMigration = userLoginMigrationFactory.buildWithId({ + school, + targetSystem, + sourceSystem, + startedAt: new Date(2023, 1, 4), + }); + + const user: User = userFactory.buildWithId(); + + const { adminAccount, adminUser } = UserAndAccountTestFactory.buildAdmin({ school }, [ + Permission.USER_LOGIN_MIGRATION_ADMIN, + ]); + + await em.persistAndFlush([ + sourceSystem, + targetSystem, + school, + adminAccount, + adminUser, + userLoginMigration, + user, + ]); + em.clear(); + + const loggedInClient = await testApiClient.login(adminAccount); + + return { + loggedInClient, + userLoginMigration, + }; + }; + + it('should return nothing', async () => { + const { loggedInClient } = await setup(); + + const response: Response = await loggedInClient.post('/close'); + + expect(response.body).toEqual({}); + }); + }); }); }); diff --git a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts index 1ea3fa50954..6b796e69bba 100644 --- a/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts +++ b/apps/server/src/modules/user-login-migration/uc/close-user-login-migration.uc.spec.ts @@ -179,6 +179,22 @@ describe('CloseUserLoginMigrationUc', () => { expect(userLoginMigrationRevertService.revertUserLoginMigration).toHaveBeenCalledWith(closedUserLoginMigration); }); + + it('should not mark all un-migrated users as outdated', async () => { + const { user, schoolId } = setup(); + + await uc.closeMigration(user.id, schoolId); + + expect(schoolMigrationService.markUnmigratedUsersAsOutdated).not.toHaveBeenCalled(); + }); + + it('should return undefined', async () => { + const { user, schoolId } = setup(); + + const result = await uc.closeMigration(user.id, schoolId); + + expect(result).toBeUndefined(); + }); }); describe('when the user login migration was already closed', () => { From e87895a9ec9512baeb5d5e8b9cfcd32a1020562a Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Mon, 25 Sep 2023 17:10:59 +0200 Subject: [PATCH 09/10] fix linter --- .../api-test/user-login-migration.api.spec.ts | 49 ------------------- 1 file changed, 49 deletions(-) diff --git a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts index 68132ded81c..95cb8fdde5e 100644 --- a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts +++ b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts @@ -1202,54 +1202,5 @@ describe('UserLoginMigrationController (API)', () => { expect(response.status).toEqual(HttpStatus.FORBIDDEN); }); }); - - describe('when no user has migrate', () => { - const setup = async () => { - const sourceSystem: System = systemFactory.withLdapConfig().buildWithId({ alias: 'SourceSystem' }); - const targetSystem: System = systemFactory.withOauthConfig().buildWithId({ alias: 'SANIS' }); - const school: SchoolEntity = schoolFactory.buildWithId({ - systems: [sourceSystem], - officialSchoolNumber: '12345', - }); - const userLoginMigration: UserLoginMigration = userLoginMigrationFactory.buildWithId({ - school, - targetSystem, - sourceSystem, - startedAt: new Date(2023, 1, 4), - }); - - const user: User = userFactory.buildWithId(); - - const { adminAccount, adminUser } = UserAndAccountTestFactory.buildAdmin({ school }, [ - Permission.USER_LOGIN_MIGRATION_ADMIN, - ]); - - await em.persistAndFlush([ - sourceSystem, - targetSystem, - school, - adminAccount, - adminUser, - userLoginMigration, - user, - ]); - em.clear(); - - const loggedInClient = await testApiClient.login(adminAccount); - - return { - loggedInClient, - userLoginMigration, - }; - }; - - it('should return nothing', async () => { - const { loggedInClient } = await setup(); - - const response: Response = await loggedInClient.post('/close'); - - expect(response.body).toEqual({}); - }); - }); }); }); From 3a879ec58bf4353e2bd21dc1fdb00f36553e6364 Mon Sep 17 00:00:00 2001 From: Mrika Llabani Date: Tue, 26 Sep 2023 11:35:05 +0200 Subject: [PATCH 10/10] adjust api test --- .../api-test/user-login-migration.api.spec.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts index 95cb8fdde5e..486c814ccd7 100644 --- a/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts +++ b/apps/server/src/modules/user-login-migration/controller/api-test/user-login-migration.api.spec.ts @@ -1202,5 +1202,54 @@ describe('UserLoginMigrationController (API)', () => { expect(response.status).toEqual(HttpStatus.FORBIDDEN); }); }); + + describe('when no user has migrate', () => { + const setup = async () => { + const sourceSystem: SystemEntity = systemFactory.withLdapConfig().buildWithId({ alias: 'SourceSystem' }); + const targetSystem: SystemEntity = systemFactory.withOauthConfig().buildWithId({ alias: 'SANIS' }); + const school: SchoolEntity = schoolFactory.buildWithId({ + systems: [sourceSystem], + officialSchoolNumber: '12345', + }); + const userLoginMigration: UserLoginMigrationEntity = userLoginMigrationFactory.buildWithId({ + school, + targetSystem, + sourceSystem, + startedAt: new Date(2023, 1, 4), + }); + + const user: User = userFactory.buildWithId(); + + const { adminAccount, adminUser } = UserAndAccountTestFactory.buildAdmin({ school }, [ + Permission.USER_LOGIN_MIGRATION_ADMIN, + ]); + + await em.persistAndFlush([ + sourceSystem, + targetSystem, + school, + adminAccount, + adminUser, + userLoginMigration, + user, + ]); + em.clear(); + + const loggedInClient = await testApiClient.login(adminAccount); + + return { + loggedInClient, + userLoginMigration, + }; + }; + + it('should return nothing', async () => { + const { loggedInClient } = await setup(); + + const response: Response = await loggedInClient.post('/close'); + + expect(response.body).toEqual({}); + }); + }); }); });