BC-5170 - code review

apps/server/src/modules/files-storage/controller/files-storage.consumer.spec.ts

@@ -7,6 +7,7 @@ import { courseFactory, fileRecordFactory, setupEntities } from '@shared/testing
 import { LegacyLogger } from '@src/core/logger';
 import { FileRecord, FileRecordParentType } from '../entity';
 import { FilesStorageService } from '../service/files-storage.service';
+import { PreviewService } from '../service/preview.service';
 import { FileRecordResponse } from './dto';
 import { FilesStorageConsumer } from './files-storage.consumer';
 
@@ -33,6 +34,10 @@ describe('FilesStorageConsumer', () => {
 					provide: FilesStorageService,
 					useValue: createMock<FilesStorageService>(),
 				},
+				{
+					provide: PreviewService,
+					useValue: createMock<PreviewService>(),
+				},
 				{
 					provide: LegacyLogger,
 					useValue: createMock<LegacyLogger>(),
@@ -165,17 +170,25 @@ describe('FilesStorageConsumer', () => {
 				const parentId = new ObjectId().toHexString();
 
 				const fileRecords = fileRecordFactory.buildList(3);
-				filesStorageService.deleteFilesOfParent.mockResolvedValue([fileRecords, fileRecords.length]);
+				filesStorageService.getFileRecordsOfParent.mockResolvedValue([fileRecords, fileRecords.length]);
 
-				return { parentId };
+				return { parentId, fileRecords };
 			};
 
 			it('should call filesStorageService.deleteFilesOfParent with params', async () => {
 				const { parentId } = setup();
 
 				await service.deleteFilesOfParent(parentId);
 
-				expect(filesStorageService.deleteFilesOfParent).toBeCalledWith(parentId);
+				expect(filesStorageService.getFileRecordsOfParent).toBeCalledWith(parentId);
+			});
+
+			it('should call filesStorageService.deleteFilesOfParent with params', async () => {
+				const { parentId, fileRecords } = setup();
+
+				await service.deleteFilesOfParent(parentId);
+
+				expect(filesStorageService.deleteFilesOfParent).toBeCalledWith(fileRecords);
 			});
 
 			it('should return array instances of FileRecordResponse', async () => {
@@ -191,7 +204,7 @@ describe('FilesStorageConsumer', () => {
 			const setup = () => {
 				const parentId = new ObjectId().toHexString();
 
-				filesStorageService.deleteFilesOfParent.mockResolvedValue([[], 0]);
+				filesStorageService.getFileRecordsOfParent.mockResolvedValue([[], 0]);
 
 				return { parentId };
 			};

apps/server/src/modules/files-storage/controller/files-storage.consumer.ts

@@ -7,12 +7,14 @@ import { LegacyLogger } from '@src/core/logger';
 import { FilesStorageEvents, FilesStorageExchange, ICopyFileDO, IFileDO } from '@src/shared/infra/rabbitmq';
 import { FilesStorageMapper } from '../mapper';
 import { FilesStorageService } from '../service/files-storage.service';
+import { PreviewService } from '../service/preview.service';
 import { CopyFilesOfParentPayload, FileRecordParams } from './dto';
 
 @Injectable()
 export class FilesStorageConsumer {
 	constructor(
 		private readonly filesStorageService: FilesStorageService,
+		private readonly previewService: PreviewService,
 		private logger: LegacyLogger,
 		// eslint-disable-next-line @typescript-eslint/no-unused-vars
 		private readonly orm: MikroORM // don't remove it, we need it for @UseRequestContext
@@ -61,7 +63,11 @@ export class FilesStorageConsumer {
 	public async deleteFilesOfParent(@RabbitPayload() payload: EntityId): Promise<RpcMessage<IFileDO[]>> {
 		this.logger.debug({ action: 'deleteFilesOfParent', payload });
 
-		const [fileRecords, total] = await this.filesStorageService.deleteFilesOfParent(payload);
+		const [fileRecords, total] = await this.filesStorageService.getFileRecordsOfParent(payload);
+
+		await this.previewService.deletePreviews(fileRecords);
+		await this.filesStorageService.deleteFilesOfParent(fileRecords);
+
 		const response = FilesStorageMapper.mapToFileRecordListResponse(fileRecords, total);
 
 		return { message: response.data };

apps/server/src/modules/files-storage/controller/files-storage.controller.ts

@@ -1,3 +1,4 @@
+import { Authenticate, CurrentUser, ICurrentUser } from '@modules/authentication';
 import {
 	BadRequestException,
 	Body,
@@ -22,10 +23,10 @@ import {
 	UseInterceptors,
 } from '@nestjs/common';
 import { ApiConsumes, ApiHeader, ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger';
-import { ApiValidationError, RequestLoggingInterceptor } from '@shared/common';
+import { ApiValidationError, RequestLoggingInterceptor, RequestTimeout } from '@shared/common';
 import { PaginationParams } from '@shared/controller';
-import { ICurrentUser, Authenticate, CurrentUser } from '@modules/authentication';
 import { Request, Response } from 'express';
+import { config } from '../files-storage.config';
 import { GetFileResponse } from '../interface';
 import { FilesStorageMapper } from '../mapper';
 import { FileRecordMapper } from '../mapper/file-record.mapper';
@@ -126,6 +127,7 @@ export class FilesStorageController {
 	@ApiResponse({ status: 500, type: InternalServerErrorException })
 	@ApiHeader({ name: 'Range', required: false })
 	@Get('/preview/:fileRecordId/:fileName')
+	@RequestTimeout(config().INCOMING_REQUEST_TIMEOUT)
 	async downloadPreview(
 		@Param() params: DownloadFileParams,
 		@CurrentUser() currentUser: ICurrentUser,

apps/server/src/modules/files-storage/dto/file.dto.ts

@@ -1,6 +1,7 @@
+import { File } from '@shared/infra/s3-client';
 import { Readable } from 'stream';
 
-export class FileDto {
+export class FileDto implements File {
 	constructor(file: FileDto) {
 		this.name = file.name;
 		this.data = file.data;

apps/server/src/modules/files-storage/service/files-storage-delete.service.spec.ts

@@ -171,35 +171,13 @@ describe('FilesStorageService delete methods', () => {
 				return { parentId, fileRecords };
 			};
 
-			it('should call findBySchoolIdAndParentId once with correct params', async () => {
-				const { parentId } = setup();
-
-				await service.deleteFilesOfParent(parentId);
-
-				expect(fileRecordRepo.findByParentId).toHaveBeenNthCalledWith(1, parentId);
-			});
-
 			it('should call delete with correct params', async () => {
-				const { parentId, fileRecords } = setup();
+				const { fileRecords } = setup();
 
-				await service.deleteFilesOfParent(parentId);
+				await service.deleteFilesOfParent(fileRecords);
 
 				expect(service.delete).toHaveBeenCalledWith(fileRecords);
 			});
-
-			it('should return file records and count', async () => {
-				const { parentId, fileRecords } = setup();
-
-				const responseData = await service.deleteFilesOfParent(parentId);
-				expect(responseData[0]).toEqual(
-					expect.arrayContaining([
-						expect.objectContaining({ ...fileRecords[0] }),
-						expect.objectContaining({ ...fileRecords[1] }),
-						expect.objectContaining({ ...fileRecords[2] }),
-					])
-				);
-				expect(responseData[1]).toEqual(fileRecords.length);
-			});
 		});
 
 		describe('WHEN no files exists', () => {
@@ -215,43 +193,17 @@ describe('FilesStorageService delete methods', () => {
 				const { parentId } = params;
 
 				spy = jest.spyOn(service, 'delete');
-				fileRecordRepo.findByParentId.mockResolvedValueOnce([fileRecords, fileRecords.length]);
 
-				return { parentId };
+				return { parentId, fileRecords };
 			};
 
 			it('should not call delete', async () => {
-				const { parentId } = setup();
+				const { fileRecords } = setup();
 
-				await service.deleteFilesOfParent(parentId);
+				await service.deleteFilesOfParent(fileRecords);
 
 				expect(service.delete).toHaveBeenCalledTimes(0);
 			});
-
-			it('should return empty counted type', async () => {
-				const { parentId } = setup();
-
-				const result = await service.deleteFilesOfParent(parentId);
-
-				expect(result).toEqual([[], 0]);
-			});
-		});
-
-		describe('WHEN repository throw an error', () => {
-			const setup = () => {
-				const { params } = buildFileRecordsWithParams();
-				const { parentId } = params;
-
-				fileRecordRepo.findByParentId.mockRejectedValueOnce(new Error('bla'));
-
-				return { parentId };
-			};
-
-			it('should pass the error', async () => {
-				const { parentId } = setup();
-
-				await expect(service.deleteFilesOfParent(parentId)).rejects.toThrow(new Error('bla'));
-			});
 		});
 
 		describe('WHEN service.delete throw an error', () => {
@@ -272,9 +224,9 @@ describe('FilesStorageService delete methods', () => {
 			};
 
 			it('should pass the error', async () => {
-				const { parentId } = setup();
+				const { fileRecords } = setup();
 
-				await expect(service.deleteFilesOfParent(parentId)).rejects.toThrow(new Error('bla'));
+				await expect(service.deleteFilesOfParent(fileRecords)).rejects.toThrow(new Error('bla'));
 			});
 		});
 	});

apps/server/src/modules/files-storage/service/files-storage.service.ts

@@ -304,14 +304,10 @@ export class FilesStorageService {
 		await this.deleteWithRollbackByError(fileRecords);
 	}
 
-	public async deleteFilesOfParent(parentId: EntityId): Promise<Counted<FileRecord[]>> {
-		const [fileRecords, count] = await this.getFileRecordsOfParent(parentId);
-
-		if (count > 0) {
+	public async deleteFilesOfParent(fileRecords: FileRecord[]): Promise<void> {
+		if (fileRecords.length > 0) {
 			await this.delete(fileRecords);
 		}
-
-		return [fileRecords, count];
 	}
 
 	// restore

apps/server/src/modules/files-storage/service/preview.service.spec.ts

@@ -594,10 +594,10 @@ describe('PreviewService', () => {
 				};
 			};
 
-			it('does not pass error', async () => {
-				const { fileRecord } = setup();
+			it('should throw error', async () => {
+				const { fileRecord, error } = setup();
 
-				await previewService.deletePreviews([fileRecord]);
+				await expect(previewService.deletePreviews([fileRecord])).rejects.toThrowError(error);
 			});
 		});
 	});

apps/server/src/modules/files-storage/service/preview.service.ts

@@ -45,27 +45,17 @@ export class PreviewService {
 			bytesRange,
 		};
 
-		if (previewParams.forceUpdate) {
-			await this.generatePreview(previewFileParams);
-		}
-
 		const response = await this.tryGetPreviewOrGenerate(previewFileParams);
 
 		return response;
 	}
 
 	public async deletePreviews(fileRecords: FileRecord[]): Promise<void> {
-		try {
-			const paths = fileRecords.map((fileRecord) =>
-				createPreviewDirectoryPath(fileRecord.getSchoolId(), fileRecord.id)
-			);
+		const paths = fileRecords.map((fileRecord) => createPreviewDirectoryPath(fileRecord.getSchoolId(), fileRecord.id));
 
-			const promises = paths.map((path) => this.storageClient.deleteDirectory(path));
+		const promises = paths.map((path) => this.storageClient.deleteDirectory(path));
 
-			await Promise.all(promises);
-		} catch (error) {
-			this.logger.warn(error);
-		}
+		await Promise.all(promises);
 	}
 
 	private checkIfPreviewPossible(fileRecord: FileRecord): void | UnprocessableEntityException {
@@ -79,6 +69,10 @@ export class PreviewService {
 		let file: GetFileResponse;
 
 		try {
+			if (params.previewParams.forceUpdate) {
+				await this.generatePreview(params);
+			}
+
 			file = await this.getPreviewFile(params);
 		} catch (error) {
 			if (!(error instanceof NotFoundException)) {

apps/server/src/modules/files-storage/uc/files-storage-delete.uc.spec.ts

@@ -1,5 +1,6 @@
 import { createMock, DeepMocked } from '@golevelup/ts-jest';
 import { ObjectId } from '@mikro-orm/mongodb';
+import { AuthorizationReferenceService } from '@modules/authorization/domain';
 import { HttpService } from '@nestjs/axios';
 import { ForbiddenException } from '@nestjs/common';
 import { Test, TestingModule } from '@nestjs/testing';
@@ -8,7 +9,6 @@ import { AntivirusService } from '@shared/infra/antivirus';
 import { S3ClientAdapter } from '@shared/infra/s3-client';
 import { fileRecordFactory, setupEntities } from '@shared/testing';
 import { LegacyLogger } from '@src/core/logger';
-import { AuthorizationReferenceService } from '@modules/authorization/domain';
 import { FileRecordParams } from '../controller/dto';
 import { FileRecord, FileRecordParentType } from '../entity';
 import { FileStorageAuthorizationContext } from '../files-storage.const';
@@ -123,7 +123,7 @@ describe('FilesStorageUC delete methods', () => {
 				const mockedResult = [[fileRecord], 0] as Counted<FileRecord[]>;
 
 				authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce();
-				filesStorageService.deleteFilesOfParent.mockResolvedValueOnce(mockedResult);
+				filesStorageService.getFileRecordsOfParent.mockResolvedValueOnce(mockedResult);
 
 				return { params, userId, mockedResult, requestParams, fileRecord };
 			};
@@ -143,11 +143,11 @@ describe('FilesStorageUC delete methods', () => {
 			});
 
 			it('should call service with correct params', async () => {
-				const { requestParams, userId } = setup();
+				const { requestParams, userId, fileRecord } = setup();
 
 				await filesStorageUC.deleteFilesOfParent(userId, requestParams);
 
-				expect(filesStorageService.deleteFilesOfParent).toHaveBeenCalledWith(requestParams.parentId);
+				expect(filesStorageService.deleteFilesOfParent).toHaveBeenCalledWith([fileRecord]);
 			});
 
 			it('should call deletePreviews', async () => {
@@ -189,10 +189,14 @@ describe('FilesStorageUC delete methods', () => {
 
 		describe('WHEN service throws error', () => {
 			const setup = () => {
+				const { fileRecords } = buildFileRecordsWithParams();
+
+				authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce();
 				const { requestParams, userId } = createParams();
 				const error = new Error('test');
 
 				authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce();
+				filesStorageService.getFileRecordsOfParent.mockResolvedValueOnce([fileRecords, fileRecords.length]);
 				filesStorageService.deleteFilesOfParent.mockRejectedValueOnce(error);
 
 				return { requestParams, userId, error };

apps/server/src/modules/files-storage/uc/files-storage.uc.ts

@@ -1,9 +1,9 @@
+import { AuthorizationContext } from '@modules/authorization';
+import { AuthorizationReferenceService } from '@modules/authorization/domain';
 import { HttpService } from '@nestjs/axios';
 import { Injectable, NotFoundException } from '@nestjs/common';
 import { Counted, EntityId } from '@shared/domain';
 import { LegacyLogger } from '@src/core/logger';
-import { AuthorizationContext } from '@modules/authorization';
-import { AuthorizationReferenceService } from '@modules/authorization/domain';
 import { AxiosRequestConfig, AxiosResponse } from 'axios';
 import busboy from 'busboy';
 import { Request } from 'express';
@@ -164,8 +164,9 @@ export class FilesStorageUC {
 	// delete
 	public async deleteFilesOfParent(userId: EntityId, params: FileRecordParams): Promise<Counted<FileRecord[]>> {
 		await this.checkPermission(userId, params.parentType, params.parentId, FileStorageAuthorizationContext.delete);
-		const [fileRecords, count] = await this.filesStorageService.deleteFilesOfParent(params.parentId);
+		const [fileRecords, count] = await this.filesStorageService.getFileRecordsOfParent(params.parentId);
 		await this.previewService.deletePreviews(fileRecords);
+		await this.filesStorageService.deleteFilesOfParent(fileRecords);
 
 		return [fileRecords, count];
 	}
@@ -175,8 +176,8 @@ export class FilesStorageUC {
 		const { parentType, parentId } = fileRecord.getParentInfo();
 
 		await this.checkPermission(userId, parentType, parentId, FileStorageAuthorizationContext.delete);
-		await this.filesStorageService.delete([fileRecord]);
 		await this.previewService.deletePreviews([fileRecord]);
+		await this.filesStorageService.delete([fileRecord]);
 
 		return fileRecord;
 	}