CCM-9247: Changes in response to review comments

Author: chris-elliott-nhsd

lambdas/backend-api/src/__tests__/templates/api/process-proof.test.ts

@@ -4,11 +4,29 @@ import { TemplateRepository } from '../../../templates/infra';
 import { LetterFileRepository } from '@backend-api/templates/infra/letter-file-repository';
 
 test.each([
-  ['NO_THREATS_FOUND', 'PASSED'],
-  ['THREATS_FOUND', 'FAILED'],
+  [
+    'NO_THREATS_FOUND',
+    'PASSED',
+    (letterFileRepository: LetterFileRepository) =>
+      expect(
+        letterFileRepository.copyFromQuarantineToInternal
+      ).toHaveBeenCalledWith(
+        'proofs/template-id/proof.pdf',
+        'version-id',
+        'proofs/template-owner/template-id/proof.pdf'
+      ),
+  ],
+  [
+    'THREATS_FOUND',
+    'FAILED',
+    (letterFileRepository) =>
+      expect(
+        letterFileRepository.copyFromQuarantineToInternal
+      ).not.toHaveBeenCalled(),
+  ],
 ])(
   'calls dependencies as expected for a %s virus scan',
-  async (scanResultStatus, virusScanStatus) => {
+  async (scanResultStatus, virusScanStatus, s3Expectation) => {
     const templateRepository = mockDeep<TemplateRepository>({
       getOwner: jest.fn().mockReturnValue('template-owner'),
     });
@@ -31,13 +49,7 @@ test.each([
 
     expect(templateRepository.getOwner).toHaveBeenCalledWith('template-id');
 
-    expect(
-      letterFileRepository.copyFromQuarantineToInternal
-    ).toHaveBeenCalledWith(
-      'proofs/template-id/proof.pdf',
-      'version-id',
-      'proofs/template-owner/template-id/proof.pdf'
-    );
+    s3Expectation(letterFileRepository);
 
     expect(
       templateRepository.setLetterFileVirusScanStatusForProof

lambdas/backend-api/src/__tests__/templates/infra/template-repository.test.ts

@@ -841,6 +841,11 @@ describe('templateRepository', () => {
   describe('setLetterFileVirusScanStatusForProof', () => {
     it('adds the virus scan status of the proof to the database record and updates the template status if scan status is passed', async () => {
       const { templateRepository, mocks } = setup();
+      mocks.ddbDocClient.on(UpdateCommand).resolves({
+        Attributes: {
+          templateStatus: 'WAITING_FOR_PROOF',
+        },
+      });
 
       await templateRepository.setLetterFileVirusScanStatusForProof(
         'template-owner',
@@ -887,6 +892,11 @@ describe('templateRepository', () => {
 
     it('adds the virus scan status of the proof to the database record and does not update the template status if scan status is failed', async () => {
       const { templateRepository, mocks } = setup();
+      mocks.ddbDocClient.on(UpdateCommand).resolves({
+        Attributes: {
+          templateStatus: 'WAITING_FOR_PROOF',
+        },
+      });
 
       await templateRepository.setLetterFileVirusScanStatusForProof(
         'template-owner',
@@ -945,7 +955,11 @@ describe('templateRepository', () => {
 
       mocks.ddbDocClient
         .on(UpdateCommand)
-        .resolvesOnce({})
+        .resolvesOnce({
+          Attributes: {
+            templateStatus: 'WAITING_FOR_PROOF',
+          },
+        })
         .rejects(
           new ConditionalCheckFailedException({
             $metadata: {},
@@ -983,10 +997,13 @@ describe('templateRepository', () => {
 
     it('raises other exceptions from the database for the second update', async () => {
       const { templateRepository, mocks } = setup();
-
       mocks.ddbDocClient
         .on(UpdateCommand)
-        .resolvesOnce({})
+        .resolvesOnce({
+          Attributes: {
+            templateStatus: 'WAITING_FOR_PROOF',
+          },
+        })
         .rejects(new Error('Something went wrong'));
 
       await expect(

lambdas/backend-api/src/templates/api/process-proof.ts

@@ -29,17 +29,19 @@ export const createHandler =
       templateId,
       fileName
     );
+    const virusScanResult =
+      scanResultStatus === 'NO_THREATS_FOUND' ? 'PASSED' : 'FAILED';
 
-    await letterFileRepository.copyFromQuarantineToInternal(
-      objectKey,
-      versionId,
-      internalKey
-    );
+    if (virusScanResult === 'PASSED') {
+      await letterFileRepository.copyFromQuarantineToInternal(
+        objectKey,
+        versionId,
+        internalKey
+      );
+    }
 
     // we will copy to the download bucket here as well
 
-    const virusScanResult =
-      scanResultStatus === 'NO_THREATS_FOUND' ? 'PASSED' : 'FAILED';
     await templateRepository.setLetterFileVirusScanStatusForProof(
       owner,
       templateId,

lambdas/backend-api/src/templates/infra/letter-proof-repository.ts

@@ -11,7 +11,11 @@ export class LetterProofRepository extends LetterFileRepository {
     const [fileType, templateId, fileNameWithExtension] = keyParts;
     const [fileName, extension] = fileNameWithExtension.split('.');
 
-    if (keyParts.length !== 3 || fileType !== 'proofs' || extension !== 'pdf') {
+    if (
+      keyParts.length !== 3 ||
+      fileType !== 'proofs' ||
+      extension.toLowerCase() !== 'pdf'
+    ) {
       throw new Error(`Unexpected object key "${key}"`);
     }
 

lambdas/backend-api/src/templates/infra/letter-upload-repository.ts

@@ -131,7 +131,7 @@ export class LetterUploadRepository extends LetterFileRepository {
     const expectedExtension =
       parsed['file-type'] === 'test-data' ? 'csv' : 'pdf';
 
-    if (extension !== expectedExtension) {
+    if (extension.toLowerCase() !== expectedExtension) {
       throw new Error(`Unexpected object key "${key}"`);
     }
 

lambdas/backend-api/src/templates/infra/template-repository.ts

@@ -385,6 +385,58 @@ export class TemplateRepository {
     }
   }
 
+  private async appendFileToProofs(
+    templateKey: TemplateKey,
+    fileName: string,
+    fullFilePath: string,
+    virusScanStatus: Extract<VirusScanStatus, 'PASSED' | 'FAILED'>
+  ) {
+    const dynamoResponse = await this.client.send(
+      new UpdateCommand({
+        TableName: this.templatesTableName,
+        Key: templateKey,
+        UpdateExpression:
+          'SET files.proofs.#fileName = :virusScanResult, updatedAt = :updatedAt',
+        ExpressionAttributeNames: {
+          '#fileName': fileName,
+        },
+        ExpressionAttributeValues: {
+          ':templateStatusDeleted': 'DELETED' satisfies TemplateStatus,
+          ':templateStatusSubmitted': 'SUBMITTED' satisfies TemplateStatus,
+          ':updatedAt': new Date().toISOString(),
+          ':virusScanResult': {
+            fileName: fullFilePath,
+            virusScanStatus,
+          } satisfies FileDetails,
+        },
+        ConditionExpression:
+          'attribute_not_exists(files.proofs.#fileName) and not templateStatus in (:templateStatusDeleted, :templateStatusSubmitted)',
+        ReturnValues: 'ALL_NEW',
+      })
+    );
+
+    return dynamoResponse.Attributes;
+  }
+
+  private async updateStatusToProofAvailable(templateKey: TemplateKey) {
+    await this.client.send(
+      new UpdateCommand({
+        TableName: this.templatesTableName,
+        Key: templateKey,
+        UpdateExpression:
+          'SET templateStatus = :templateStatusProofAvailable, updatedAt = :updatedAt',
+        ExpressionAttributeValues: {
+          ':templateStatusWaitingForProof':
+            'WAITING_FOR_PROOF' satisfies TemplateStatus,
+          ':templateStatusProofAvailable':
+            'PROOF_AVAILABLE' satisfies TemplateStatus,
+          ':updatedAt': new Date().toISOString(),
+        },
+        ConditionExpression: 'templateStatus = :templateStatusWaitingForProof',
+      })
+    );
+  }
+
   async setLetterFileVirusScanStatusForProof(
     owner: string,
     templateId: string,
@@ -395,28 +447,20 @@ export class TemplateRepository {
     const templateKey = { owner, id: templateId };
 
     try {
-      await this.client.send(
-        new UpdateCommand({
-          TableName: this.templatesTableName,
-          Key: templateKey,
-          UpdateExpression:
-            'SET files.proofs.#fileName = :virusScanResult, updatedAt = :updatedAt',
-          ExpressionAttributeNames: {
-            '#fileName': fileName,
-          },
-          ExpressionAttributeValues: {
-            ':templateStatusDeleted': 'DELETED' satisfies TemplateStatus,
-            ':templateStatusSubmitted': 'SUBMITTED' satisfies TemplateStatus,
-            ':updatedAt': new Date().toISOString(),
-            ':virusScanResult': {
-              fileName: fullFilePath,
-              virusScanStatus,
-            } satisfies FileDetails,
-          },
-          ConditionExpression:
-            'attribute_not_exists(files.proofs.#fileName) and not templateStatus in (:templateStatusDeleted, :templateStatusSubmitted)',
-        })
+      const updatedItem = await this.appendFileToProofs(
+        templateKey,
+        fileName,
+        fullFilePath,
+        virusScanStatus
       );
+
+      // we do not want to try and update the status to PROOF_AVAILABLE if the scan has not passed or if the status has already been changed by another process
+      if (
+        virusScanStatus === 'FAILED' ||
+        updatedItem?.templateStatus !== 'WAITING_FOR_PROOF'
+      ) {
+        return;
+      }
     } catch (error) {
       if (error instanceof ConditionalCheckFailedException) {
         logger.error(
@@ -427,34 +471,13 @@ export class TemplateRepository {
 
         // the second update has a stronger condition than the first, so if this one fails no need to try the second
         return;
-      } else {
-        throw error;
       }
-    }
 
-    // we do not want to try and update the status to PROOF_AVAILABLE if the scan has not passed
-    if (virusScanStatus === 'FAILED') {
-      return;
+      throw error;
     }
 
     try {
-      await this.client.send(
-        new UpdateCommand({
-          TableName: this.templatesTableName,
-          Key: templateKey,
-          UpdateExpression:
-            'SET templateStatus = :templateStatusProofAvailable, updatedAt = :updatedAt',
-          ExpressionAttributeValues: {
-            ':templateStatusWaitingForProof':
-              'WAITING_FOR_PROOF' satisfies TemplateStatus,
-            ':templateStatusProofAvailable':
-              'PROOF_AVAILABLE' satisfies TemplateStatus,
-            ':updatedAt': new Date().toISOString(),
-          },
-          ConditionExpression:
-            'templateStatus = :templateStatusWaitingForProof',
-        })
-      );
+      await this.updateStatusToProofAvailable(templateKey);
     } catch (error) {
       if (error instanceof ConditionalCheckFailedException) {
         logger.error('Conditional check setting template status', error, {