From c84df3b7e34dce80231736a740dc93af71660f0c Mon Sep 17 00:00:00 2001 From: virgilchiriac Date: Fri, 24 Jan 2025 17:17:01 +0100 Subject: [PATCH] fixes from revviews --- ...deletion-execution-options.builder.spec.ts | 9 +++----- .../deletion-client/deletion.client.ts | 4 ++-- .../deletion-request-create.api.spec.ts | 8 +++---- .../api/uc/deletion-request.uc.spec.ts | 17 ++++++++++---- .../deletion/api/uc/deletion-request.uc.ts | 23 +++++++++---------- .../service/deletion-request.service.ts | 4 ++-- .../deletion/repo/deletion-request.repo.ts | 4 ++-- config/default.schema.json | 2 +- 8 files changed, 37 insertions(+), 34 deletions(-) diff --git a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts index 87ba8d4ec1f..0b4eb6cd53c 100644 --- a/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts +++ b/apps/server/src/modules/deletion-console/builder/trigger-deletion-execution-options.builder.spec.ts @@ -1,4 +1,3 @@ -import { TriggerDeletionExecutionOptions } from '../interface'; import { TriggerDeletionExecutionOptionsBuilder } from './trigger-deletion-execution-options.builder'; describe(TriggerDeletionExecutionOptionsBuilder.name, () => { @@ -8,17 +7,15 @@ describe(TriggerDeletionExecutionOptionsBuilder.name, () => { const limit = 1000; const runFailed = false; - const expectedOutput: TriggerDeletionExecutionOptions = { limit, runFailed }; - - return { expectedOutput, limit, runFailed }; + return { limit, runFailed }; }; it('should return valid object with expected values', () => { - const { expectedOutput, limit, runFailed } = setup(); + const { limit, runFailed } = setup(); const output = TriggerDeletionExecutionOptionsBuilder.build(limit, runFailed); - expect(output).toStrictEqual(expectedOutput); + expect(output).toStrictEqual({ limit, runFailed }); }); }); }); diff --git a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts index bc4f36abe56..c37fd77062d 100644 --- a/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts +++ b/apps/server/src/modules/deletion-console/deletion-client/deletion.client.ts @@ -55,7 +55,7 @@ export class DeletionClient { const postDeletionExecutionsEndpoint = new URL(endpoint, baseUrl); const params = new URLSearchParams(postDeletionExecutionsEndpoint.search); - if (limit && this.isLimitGeaterZero(limit)) { + if (limit && this.isLimitGreaterZero(limit)) { params.append('limit', limit.toString()); } if (runFailed) { @@ -99,7 +99,7 @@ export class DeletionClient { } } - private isLimitGeaterZero(limit?: number): boolean { + private isLimitGreaterZero(limit?: number): boolean { return typeof limit === 'number' && limit > 0; } diff --git a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts index e2d09c9be53..6b7324a95f4 100644 --- a/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts +++ b/apps/server/src/modules/deletion/api/controller/api-test/deletion-request-create.api.spec.ts @@ -88,14 +88,14 @@ describe(`deletionRequest create (api)`, () => { deleteAfterMinutes: 0, }; - const defaultdeleteAfterMinutes = 43200; + const defaultDeleteAfterMinutes = 43200; const operationalTimeToleranceInSeconds = 30; return { deletionRequestToCreate, deletionRequestToImmediateRemoval, - defaultdeleteAfterMinutes, + defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds, }; }; @@ -119,7 +119,7 @@ describe(`deletionRequest create (api)`, () => { describe('when the "delete after minutes" param has not been provided', () => { it('should set the "deleteAfter" date to the date after the default DELETION_DELETE_AFTER_MINUTES ', async () => { - const { deletionRequestToCreate, defaultdeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); + const { deletionRequestToCreate, defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds } = setup(); const response = await testApiClient.post('', deletionRequestToCreate); @@ -131,7 +131,7 @@ describe(`deletionRequest create (api)`, () => { const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange( createdItem.createdAt, createdItem.deleteAfter, - defaultdeleteAfterMinutes, + defaultDeleteAfterMinutes, operationalTimeToleranceInSeconds ); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts index 040f2180fb2..f2235990d77 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.spec.ts @@ -1,10 +1,9 @@ import { LegacyLogger } from '@core/logger'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { MikroORM } from '@mikro-orm/core'; -import { ConfigModule, ConfigService } from '@nestjs/config'; +import { ConfigService } from '@nestjs/config'; import { EventBus } from '@nestjs/cqrs'; import { Test, TestingModule } from '@nestjs/testing'; -import { createConfigModuleOptions } from '@shared/common/config-module-options'; import { setupEntities } from '@testing/setup-entities'; import { ObjectId } from 'bson'; import { DomainDeletionReportBuilder, DomainOperationReportBuilder } from '../../domain/builder'; @@ -31,7 +30,6 @@ describe(DeletionRequestUc.name, () => { const orm = await setupEntities(); module = await Test.createTestingModule({ - imports: [ConfigModule.forRoot(createConfigModuleOptions(deletionTestConfig))], providers: [ DeletionRequestUc, { @@ -161,9 +159,13 @@ describe(DeletionRequestUc.name, () => { const setup = () => { const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + configService.get.mockImplementation((key) => deletionTestConfig()[key]); + return { deletionRequest }; }; it('should call deletionRequestService.findAllItemsToExecute', async () => { + setup(); + await uc.executeDeletionRequests(); expect(deletionRequestService.findAllItemsToExecute).toHaveBeenCalled(); @@ -192,8 +194,13 @@ describe(DeletionRequestUc.name, () => { }); it('should work with a delay of 0', async () => { - const { deletionRequest } = setup(); - configService.get.mockReturnValueOnce(0); + const deletionRequest = deletionRequestFactory.buildWithId({ deleteAfter: new Date('2023-01-01') }); + configService.get.mockImplementation((key) => { + if (key === 'ADMIN_API__DELETION_DELAY_MILLISECONDS') { + return 0; + } + return deletionTestConfig()[key]; + }); deletionRequestService.findAllItemsToExecute.mockResolvedValueOnce([deletionRequest]); diff --git a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts index fdab1ed224b..7c565cecc78 100644 --- a/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts +++ b/apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts @@ -17,7 +17,7 @@ import { DeletionConfig } from '../../deletion.config'; @Injectable() @EventsHandler(DataDeletedEvent) export class DeletionRequestUc implements IEventHandler { - config: string[]; + private config: string[]; constructor( private readonly deletionRequestService: DeletionRequestService, @@ -78,31 +78,31 @@ export class DeletionRequestUc implements IEventHandler { return result; } - async executeDeletionRequests(limit?: number, failed?: boolean): Promise { + async executeDeletionRequests(limit?: number, getFailed?: boolean): Promise { this.logger.debug({ action: 'executeDeletionRequests', limit }); limit = limit ?? this.configService.get('ADMIN_API__DELETION_MAX_CONCURRENT_DELETION_REQUESTS'); let deletionRequests: DeletionRequest[] = []; - do { - // eslint-disable-next-line no-await-in-loop - if (limit > 0) { + if (limit > 0) { + do { // eslint-disable-next-line no-await-in-loop - deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, failed); + deletionRequests = await this.deletionRequestService.findAllItemsToExecute(limit, getFailed); this.logger.debug({ action: 'processing deletion request', deletionRequests }); + // eslint-disable-next-line no-await-in-loop await Promise.all( deletionRequests.map(async (req) => { await this.executeDeletionRequest(req); }) ); - } - // eslint-disable-next-line no-await-in-loop - await this.sleep(); - } while (deletionRequests.length > 0); + // eslint-disable-next-line no-await-in-loop + await this.delayForDeletion(); + } while (deletionRequests.length > 0); + } this.logger.debug({ action: 'deletion process completed' }); } @@ -143,8 +143,7 @@ export class DeletionRequestUc implements IEventHandler { } } - // short sleep mode to give time for deletion process to do its work - private async sleep() { + private async delayForDeletion() { const delay = this.configService.get('ADMIN_API__DELETION_DELAY_MILLISECONDS'); if (delay > 0) { return new Promise((resolve) => { diff --git a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts index a04edd17d98..ba1e336df61 100644 --- a/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts +++ b/apps/server/src/modules/deletion/domain/service/deletion-request.service.ts @@ -48,8 +48,8 @@ export class DeletionRequestService { return deletionRequest; } - async findAllItemsToExecute(limit: number, failed = false): Promise { - const deletionRequests = failed + async findAllItemsToExecute(limit: number, getFailed = false): Promise { + const deletionRequests = getFailed ? await this.deletionRequestRepo.findAllFailedItems(limit, this.olderThan, this.newerThan) : await this.deletionRequestRepo.findAllItems(limit); diff --git a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts index 868a3d90a18..3bf50d79e4c 100644 --- a/apps/server/src/modules/deletion/repo/deletion-request.repo.ts +++ b/apps/server/src/modules/deletion/repo/deletion-request.repo.ts @@ -39,7 +39,7 @@ export class DeletionRequestRepo { const order = { createdAt: SortOrder.asc }; - const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { + const deletionRequestEntities = await this.em.find(DeletionRequestEntity, scope.query, { limit, orderBy: order, }); @@ -59,7 +59,7 @@ export class DeletionRequestRepo { const order = { createdAt: SortOrder.asc }; - const [deletionRequestEntities] = await this.em.findAndCount(DeletionRequestEntity, scope.query, { + const deletionRequestEntities = await this.em.find(DeletionRequestEntity, scope.query, { limit, orderBy: order, }); diff --git a/config/default.schema.json b/config/default.schema.json index 50fd5715fa6..312f3f5d9a9 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -1415,7 +1415,7 @@ "DELETION_DELAY_MILLISECONDS": { "type": "number", "default": 5000, - "description": "delay in milliseconds to sleep before processing another batch of of deletion requests" + "description": "delay in milliseconds to sleep before processing another batch of deletion requests" } }, "default": {}