Skip to content

Commit

Permalink
fixes from revviews
Browse files Browse the repository at this point in the history
  • Loading branch information
virgilchiriac committed Jan 24, 2025
1 parent 2ec2d7d commit c84df3b
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 34 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { TriggerDeletionExecutionOptions } from '../interface';
import { TriggerDeletionExecutionOptionsBuilder } from './trigger-deletion-execution-options.builder';

describe(TriggerDeletionExecutionOptionsBuilder.name, () => {
Expand All @@ -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 });
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -99,7 +99,7 @@ export class DeletionClient {
}
}

private isLimitGeaterZero(limit?: number): boolean {
private isLimitGreaterZero(limit?: number): boolean {
return typeof limit === 'number' && limit > 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
};
Expand All @@ -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);

Expand All @@ -131,7 +131,7 @@ describe(`deletionRequest create (api)`, () => {
const isDeletionPlannedAtDateCorrect = isDeletionPlannedWithinAcceptableRange(
createdItem.createdAt,
createdItem.deleteAfter,
defaultdeleteAfterMinutes,
defaultDeleteAfterMinutes,
operationalTimeToleranceInSeconds
);

Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -31,7 +30,6 @@ describe(DeletionRequestUc.name, () => {
const orm = await setupEntities();

module = await Test.createTestingModule({
imports: [ConfigModule.forRoot(createConfigModuleOptions(deletionTestConfig))],
providers: [
DeletionRequestUc,
{
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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]);

Expand Down
23 changes: 11 additions & 12 deletions apps/server/src/modules/deletion/api/uc/deletion-request.uc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { DeletionConfig } from '../../deletion.config';
@Injectable()
@EventsHandler(DataDeletedEvent)
export class DeletionRequestUc implements IEventHandler<DataDeletedEvent> {
config: string[];
private config: string[];

constructor(
private readonly deletionRequestService: DeletionRequestService,
Expand Down Expand Up @@ -78,31 +78,31 @@ export class DeletionRequestUc implements IEventHandler<DataDeletedEvent> {
return result;
}

async executeDeletionRequests(limit?: number, failed?: boolean): Promise<void> {
async executeDeletionRequests(limit?: number, getFailed?: boolean): Promise<void> {
this.logger.debug({ action: 'executeDeletionRequests', limit });

limit = limit ?? this.configService.get<number>('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' });
}
Expand Down Expand Up @@ -143,8 +143,7 @@ export class DeletionRequestUc implements IEventHandler<DataDeletedEvent> {
}
}

// short sleep mode to give time for deletion process to do its work
private async sleep() {
private async delayForDeletion() {
const delay = this.configService.get<number>('ADMIN_API__DELETION_DELAY_MILLISECONDS');
if (delay > 0) {
return new Promise((resolve) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ export class DeletionRequestService {
return deletionRequest;
}

async findAllItemsToExecute(limit: number, failed = false): Promise<DeletionRequest[]> {
const deletionRequests = failed
async findAllItemsToExecute(limit: number, getFailed = false): Promise<DeletionRequest[]> {
const deletionRequests = getFailed
? await this.deletionRequestRepo.findAllFailedItems(limit, this.olderThan, this.newerThan)
: await this.deletionRequestRepo.findAllItems(limit);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand All @@ -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,
});
Expand Down
2 changes: 1 addition & 1 deletion config/default.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {}
Expand Down

0 comments on commit c84df3b

Please sign in to comment.