feat: Add additional logging for file uploads and refactor handler lo…

…gic (#1286)

* refactor: refactor handleUploadRequest into a standalone handler, instead of a uploadService function. refactor prehandlers into their own directory
* feat: add `showFilenamesOnSummaryPage` to formModel
* fix: display previously uploaded file on file upload pages

e2e/cypress/e2e/runner/files.feature

@@ -0,0 +1,22 @@
+Feature: File upload fields
+
+  Background:
+    Given the form "files" exists
+    And the form "files-show-filenames-enabled" exists
+
+
+  Scenario Outline: showFilenamesOnSummaryPage shows "Uploaded" or the filename correctly on summary pages
+    And I navigate to the "<form>" form
+    When I upload the file "passes.png"
+    Then I see "<summaryValue>"
+    Examples:
+      | form                         | summaryValue |
+      | files                        | Uploaded     |
+      | files-show-filenames-enabled | passes.png   |
+
+
+  Scenario: Uploading a file shows "You previously uploaded" message
+    And I navigate to the "files" form
+    When I upload the file "passes.png"
+    And I navigate to "/files/file-one"
+    Then I see "You previously uploaded the file ‘passes.png’"

e2e/cypress/e2e/runner/files.js

@@ -0,0 +1,6 @@
+import { When } from "@badeball/cypress-cucumber-preprocessor";
+
+When("I upload the file {string}", (filename) => {
+  cy.get("input[type=file]").attachFile(filename);
+  cy.findByRole("button", { name: /continue/i }).click();
+});

e2e/cypress/fixtures/files-show-filenames-enabled.json

@@ -0,0 +1,48 @@
+{
+  "startPage": "/file-one",
+  "pages": [
+    {
+      "path": "/file-one",
+      "components": [
+        {
+          "type": "FileUploadField",
+          "name": "file1",
+          "title": "File 1",
+          "options": {
+            "required": true
+          },
+          "schema": {}
+        }
+      ],
+      "section": "checkBeforeYouStart",
+      "next": [
+        {
+          "path": "/summary"
+        }
+      ],
+      "title": "Upload file 1"
+    },
+    {
+      "path": "/summary",
+      "controller": "./pages/summary.js",
+      "title": "Summary",
+      "components": [],
+      "next": []
+    }
+  ],
+  "lists": [],
+  "sections": [
+    {
+      "name": "checkBeforeYouStart",
+      "title": "Check before you start"
+    }
+  ],
+  "phaseBanner": {},
+  "fees": [],
+  "payApiKey": "",
+  "outputs": [],
+  "declaration": "<p class=\"govuk-body\">All the answers you have provided are true to the best of your knowledge.</p>",
+  "version": 2,
+  "conditions": [],
+  "showFilenamesOnSummaryPage": true
+}

e2e/cypress/fixtures/files.json

@@ -0,0 +1,47 @@
+{
+  "startPage": "/file-one",
+  "pages": [
+    {
+      "path": "/file-one",
+      "components": [
+        {
+          "type": "FileUploadField",
+          "name": "file1",
+          "title": "File 1",
+          "options": {
+            "required": true
+          },
+          "schema": {}
+        }
+      ],
+      "section": "checkBeforeYouStart",
+      "next": [
+        {
+          "path": "/summary"
+        }
+      ],
+      "title": "Upload file 1"
+    },
+    {
+      "path": "/summary",
+      "controller": "./pages/summary.js",
+      "title": "Summary",
+      "components": [],
+      "next": []
+    }
+  ],
+  "lists": [],
+  "sections": [
+    {
+      "name": "checkBeforeYouStart",
+      "title": "Check before you start"
+    }
+  ],
+  "phaseBanner": {},
+  "fees": [],
+  "payApiKey": "",
+  "outputs": [],
+  "declaration": "<p class=\"govuk-body\">All the answers you have provided are true to the best of your knowledge.</p>",
+  "version": 2,
+  "conditions": []
+}

model/src/schema/schema.ts

@@ -327,6 +327,7 @@ export const Schema = joi
     specialPages: specialPagesSchema.optional(),
     feeOptions: feeOptionSchema,
     exitOptions: exitSchema.optional(),
+    showFilenamesOnSummaryPage: joi.boolean().optional(),
   });
 
 /**

runner/src/server/plugins/engine/models/FormModel.ts

@@ -63,6 +63,7 @@ export class FormModel {
   specialPages: FormDefinition["specialPages"];
   exitOptions?: ExitOptions;
   allowExit: boolean = false;
+  showFilenamesOnSummaryPage: boolean = false;
 
   constructor(def, options) {
     const result = Schema.validate(def, { abortEarly: false });
@@ -120,6 +121,8 @@ export class FormModel {
       this.allowExit = true;
     }
 
+    this.showFilenamesOnSummaryPage = def.showFilenamesOnSummaryPage ?? false;
+
     // @ts-ignore
     this.pages = def.pages.map((pageDef) => this.makePage(pageDef));
     this.startPage = this.pages.find((page) => page.path === def.startPage);

runner/src/server/plugins/engine/models/SummaryViewModel.ts

@@ -158,6 +158,8 @@ export class SummaryViewModel {
       const items: any[] = [];
       let sectionState = section ? state[section.name] || {} : state;
 
+      sectionState.originalFilenames = state.originalFilenames ?? {};
+
       const sectionPages = relevantPages.filter(
         (page) => page.section === section
       );
@@ -352,7 +354,7 @@ function Item(
     });
   }
 
-  return {
+  const item = {
     name: component.name,
     path: page.path,
     label: component.localisedString(component.title),
@@ -365,4 +367,14 @@ function Item(
     dataType: component.dataType,
     immutable: component.options.disableChangingFromSummary,
   };
+
+  if (
+    component.type === "FileUploadField" &&
+    model.showFilenamesOnSummaryPage
+  ) {
+    item.filename =
+      sectionState.originalFilenames[component.name]?.originalFilename;
+  }
+
+  return item;
 }

runner/src/server/plugins/engine/pageControllers/PageControllerBase.ts

@@ -611,7 +611,7 @@ export class PageControllerBase {
     /**
      * other file related errors.. assuming file fields will be on their own page. This will replace all other errors from the page if not..
      */
-    if (preHandlerErrors) {
+    if (preHandlerErrors?.length) {
       const reformattedErrors: any[] = [];
       preHandlerErrors.forEach((error) => {
         const reformatted = error;

runner/src/server/plugins/engine/plugin.ts

@@ -11,6 +11,11 @@ import { FormPayload } from "./types";
 import { shouldLogin } from "server/plugins/auth";
 import config from "../../config";
 import * as exit from "./pluginHandlers/exit";
+import {
+  getFiles,
+  handleUpload,
+  validateContentTypes,
+} from "./pluginHandlers/files/prehandlers";
 
 configure([
   // Configure Nunjucks to allow rendering of content that is revealed conditionally.
@@ -280,15 +285,6 @@ export const plugin = {
 
     const { uploadService } = server.services([]);
 
-    const handleFiles = (request: HapiRequest, h: HapiResponseToolkit) => {
-      const { path, id } = request.params;
-      const model = forms[id];
-      const page = model?.pages.find(
-        (page) => normalisePath(page.path) === normalisePath(path)
-      );
-      return uploadService.handleUploadRequest(request, h, page.pageDef);
-    };
-
     //TODO:- Merge with POST /{id}/{path*} route, and move to ./pluginHandlers/id/*
     const postHandler = async (
       request: HapiRequest,
@@ -330,7 +326,11 @@ export const plugin = {
             return h.continue;
           },
         },
-        pre: [{ method: handleFiles }],
+        pre: [
+          { method: getFiles, assign: "files" },
+          { method: validateContentTypes, assign: "validFiles" },
+          { method: handleUpload },
+        ],
         handler: postHandler,
       },
     });

runner/src/server/plugins/engine/pluginHandlers/files/prehandlers/getFiles.ts

@@ -0,0 +1,18 @@
+import { HapiRequest, HapiResponseToolkit } from "server/types";
+
+/**
+ * Prehandler that parses the payload and finds Buffers (i.e. files).
+ */
+export function getFiles(request: HapiRequest, _h: HapiResponseToolkit) {
+  const { uploadService } = request.services([]);
+  const files = uploadService.fileStreamsFromPayload(request.payload);
+  if (files.length) {
+    request.server.logger.info(
+      { id: request.yar.id, path: request.path },
+      `Found ${uploadService.fileSummary(files)} to process on ${request.path}`
+    );
+    return files;
+  }
+
+  return null;
+}

runner/src/server/plugins/engine/pluginHandlers/files/prehandlers/handleUpload.ts

@@ -0,0 +1,119 @@
+import { HapiRequest, HapiResponseToolkit } from "server/types";
+
+const parsedError = (key: string, error?: string) => {
+  return {
+    path: key,
+    href: `#${key}`,
+    name: key,
+    text: error,
+  };
+};
+
+export async function handleUpload(
+  request: HapiRequest,
+  h: HapiResponseToolkit
+) {
+  const { cacheService, uploadService } = request.services([]);
+  const state = await cacheService.getState(request);
+  let originalFilenames = state?.originalFilenames ?? {};
+  let logger = request.server.logger;
+  const files = request.pre.files;
+
+  if (!files) {
+    return h.continue;
+  }
+
+  /**
+   * files is an array of tuples containing key and value.
+   * value may be an array of file data where multiple files have been uploaded
+   */
+  const validFiles = request.pre.validFiles;
+
+  for (const entry of validFiles) {
+    const [fieldName, streams] = entry;
+    const loggerIdentifier = uploadService.getLoggerIdentifier(request, {
+      fieldName,
+    });
+    const previousUpload = originalFilenames[fieldName];
+
+    if (previousUpload) {
+      logger.info(
+        loggerIdentifier,
+        `User is attempting to overwrite ${fieldName} with location ${previousUpload.location}`
+      );
+    }
+
+    let response;
+
+    try {
+      response = await uploadService.uploadDocuments(streams);
+    } catch (err) {
+      if (err.data?.res) {
+        const { error } = uploadService.parsedDocumentUploadResponse(err.data);
+        request.pre.errors = [
+          ...request.pre.errors,
+          parsedError(fieldName, error),
+        ];
+      } else if (err.code === "EPIPE") {
+        // ignore this error, it happens when the request is responded to by the doc upload service before the
+        // body has finished being sent. A valid response is still received.
+        logger.warn(loggerIdentifier, `Ignoring EPIPE response ${err.message}`);
+      } else {
+        logger.error(
+          { ...loggerIdentifier, err },
+          `Error uploading document: ${err.message}`
+        );
+        request.pre.errors = [
+          ...(h.request.pre.errors || []),
+          parsedError(fieldName, err),
+        ];
+      }
+    }
+
+    const { location, warning, error } = response;
+
+    if (location) {
+      const originalFilename = streams
+        .map((stream) => stream.hapi?.filename)
+        .join(", ");
+
+      logger.info(
+        loggerIdentifier,
+        `Uploaded ${fieldName} successfully to ${location}`
+      );
+
+      originalFilenames[fieldName] = { location, originalFilename };
+      const {
+        originalFilenames: updatedFilenames,
+      } = await cacheService.mergeState(request, { originalFilenames });
+
+      logger.info(
+        { ...loggerIdentifier, allFiles: updatedFilenames },
+        `Updated originalFileNames for user`
+      );
+      request.payload[fieldName] = location;
+      originalFilenames = updatedFilenames;
+    }
+
+    if (warning) {
+      request.pre.warning = warning;
+      logger.warn(
+        loggerIdentifier,
+        `File was uploaded successfully but there was a warning ${warning}`
+      );
+    }
+
+    if (error) {
+      logger.error(
+        loggerIdentifier,
+        `Document upload API responded with an error ${error}`
+      );
+      request.pre.errors = [
+        ...(request.pre.errors || []),
+        parsedError(fieldName, error),
+      ];
+    }
+  }
+
+  return h.continue;
+}

runner/src/server/plugins/engine/pluginHandlers/files/prehandlers/index.ts

@@ -0,0 +1,3 @@
+export { getFiles } from "./getFiles";
+export { validateContentTypes } from "./validateContentTypes";
+export { handleUpload } from "./handleUpload";