review comments

Author: alexnuttall

lambdas/backend-api/src/__tests__/templates/api/validate-letter-template-files.test.ts

@@ -683,6 +683,68 @@ describe('guard duty handler', () => {
     ).not.toHaveBeenCalled();
   });
 
+  test('does not attempt to fetch client configuration if no clientId', async () => {
+    const { handler, mocks } = setup();
+
+    const event = makeGuardDutyMalwareScanResultNotificationEvent({
+      detail: {
+        s3ObjectDetails: {
+          bucketName: 'quarantine-bucket',
+          objectKey: `pdf-template/${owner}/${templateId}/${versionId}.pdf`,
+        },
+        scanResultDetails: {
+          scanResultStatus: 'NO_THREATS_FOUND',
+        },
+      },
+    });
+
+    const template = mock<DatabaseTemplate>({
+      files: {
+        pdfTemplate: {
+          fileName: '',
+          virusScanStatus: 'PASSED',
+          currentVersion: versionId,
+        },
+        testDataCsv: undefined,
+      },
+      templateStatus: 'PENDING_VALIDATION',
+      language: 'en',
+      owner,
+      clientId: undefined,
+    });
+
+    mocks.templateRepository.get.mockResolvedValueOnce({
+      data: template,
+    });
+
+    mocks.letterUploadRepository.download.mockResolvedValueOnce(
+      Uint8Array.from('pdf')
+    );
+
+    const pdf = mock<TemplatePdf>({
+      personalisationParameters: ['a'],
+    });
+
+    mocks.TemplatePdf.mockImplementation(() => pdf);
+
+    mocks.validateLetterTemplateFiles.mockReturnValueOnce(true);
+
+    await handler(event);
+
+    expect(mocks.clientConfigRepository.get).not.toHaveBeenCalled();
+
+    expect(
+      mocks.templateRepository.setLetterValidationResult
+    ).toHaveBeenCalledWith(
+      { id: templateId, owner },
+      versionId,
+      true,
+      pdf.personalisationParameters,
+      [],
+      false
+    );
+  });
+
   test("errors if the template data can't be loaded", async () => {
     const { handler, mocks } = setup();
 

lambdas/backend-api/src/templates/api/validate-letter-template-files.ts

@@ -1,4 +1,7 @@
-import { logger } from 'nhs-notify-web-template-management-utils/logger';
+import {
+  logger,
+  type Logger,
+} from 'nhs-notify-web-template-management-utils/logger';
 import {
   guardDutyEventValidator,
   isRightToLeft,
@@ -66,19 +69,18 @@ export class ValidateLetterTemplateFilesLambda {
       'version-id': versionId,
     } = metadata;
 
-    const getTemplateResult = await this.templateRepository.get(templateId, {
-      userId: owner,
-      clientId: owner,
-    });
+    const { error: getTemplateError, data: template } =
+      await this.templateRepository.get(templateId, {
+        userId: owner,
+        clientId: owner,
+      });
 
-    if (getTemplateResult.error) {
-      log.error('Unable to load template data', getTemplateResult.error);
+    if (getTemplateError) {
+      log.error('Unable to load template data', getTemplateError);
 
       throw new Error('Unable to load template data');
     }
 
-    const template = getTemplateResult.data;
-
     if (!template.files) {
       log.error("Can't process non-letter template");
       return;
@@ -154,23 +156,13 @@ export class ValidateLetterTemplateFilesLambda {
     }
 
     const pdf = new TemplatePdf({ id: templateId, owner }, pdfBuff);
-    let csv;
 
-    const clientConfigurationResult = await this.clientConfigRepository.get(
-      String(getTemplateResult.data.clientId)
+    const clientProofingEnabled = await this.isProofingEnabled(
+      template.clientId,
+      log
     );
 
-    if (clientConfigurationResult.error) {
-      log.error(
-        'Unable to fetch client configuration',
-        clientConfigurationResult.error
-      );
-
-      throw new Error('Unable to fetch client configuration');
-    }
-
-    const clientProofingEnabled =
-      clientConfigurationResult.data?.features?.proofing || false;
+    let csv;
 
     try {
       await pdf.parse();
@@ -184,7 +176,7 @@ export class ValidateLetterTemplateFilesLambda {
       log.error('File parsing error:', error);
 
       await this.templateRepository.setLetterValidationResult(
-        { id: templateId, owner: getTemplateResult.data.owner },
+        { id: templateId, owner: template.owner },
         versionId,
         false,
         [],
@@ -199,12 +191,26 @@ export class ValidateLetterTemplateFilesLambda {
     const valid = rtl || validateLetterTemplateFiles(pdf, csv);
 
     await this.templateRepository.setLetterValidationResult(
-      { id: templateId, owner: getTemplateResult.data.owner },
+      { id: templateId, owner: template.owner },
       versionId,
       valid,
       pdf.personalisationParameters,
       csv?.parameters || [],
       clientProofingEnabled
     );
   };
+
+  private async isProofingEnabled(clientId: string | undefined, log: Logger) {
+    const { data: clientConfiguration, error: clientConfigError } = clientId
+      ? await this.clientConfigRepository.get(clientId)
+      : { data: null };
+
+    if (clientConfigError) {
+      log.error('Unable to fetch client configuration', clientConfigError);
+
+      throw new Error('Unable to fetch client configuration');
+    }
+
+    return clientConfiguration?.features?.proofing || false;
+  }
 }

lambdas/backend-api/src/templates/domain/validate-letter-template-files.ts

@@ -35,27 +35,22 @@ export function validateLetterTemplateFiles(
       );
   }
 
-  const valid =
-    correctAddressLength &&
-    correctAddressLines &&
-    customParametersSensiblyFormatted &&
-    testFileHasExpectedNumberOfParameters &&
-    allCustomPersonalisationIsInTestFile;
+  const failedTests = Object.entries({
+    correctAddressLength,
+    correctAddressLines,
+    customParametersSensiblyFormatted,
+    requiredTestFileExists,
+    testFileHasExpectedNumberOfParameters,
+    allCustomPersonalisationIsInTestFile,
+  }).flatMap(([test, passed]) => (passed ? [] : [test]));
+
+  const valid = failedTests.length === 0;
 
   logger.info('Template file validation complete', {
     templateId: pdf.templateId,
     owner: pdf.owner,
     valid,
-    ...(!valid && {
-      checks: {
-        correctAddressLength,
-        correctAddressLines,
-        customParametersSensiblyFormatted,
-        requiredTestFileExists,
-        testFileHasExpectedNumberOfParameters,
-        allCustomPersonalisationIsInTestFile,
-      },
-    }),
+    failedTests,
   });
 
   return valid;