From 66d247dfcc7b59e2006327b331de0a8c84521e50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Sat, 7 Dec 2024 09:43:36 +0000 Subject: [PATCH 1/4] fix: Improve typechecking for file upload --- .../modules/file/service/uploadFile.ts | 20 ++++++++++++++++++- api.planx.uk/modules/send/s3/index.ts | 4 ++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/api.planx.uk/modules/file/service/uploadFile.ts b/api.planx.uk/modules/file/service/uploadFile.ts index ead3cee9af..d272018c70 100644 --- a/api.planx.uk/modules/file/service/uploadFile.ts +++ b/api.planx.uk/modules/file/service/uploadFile.ts @@ -7,6 +7,7 @@ import mime from "mime"; import { customAlphabet } from "nanoid"; import { isLiveEnv } from "../../../helpers.js"; import { s3Factory } from "./utils.js"; +import { Readable } from "stream"; const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); export const uploadPublicFile = async ( @@ -28,10 +29,14 @@ export const uploadPublicFile = async ( }; export const uploadPrivateFile = async ( - file: Express.Multer.File, + file: Express.Multer.File | Buffer, filename: string, filekey?: string, ) => { + if (file instanceof Buffer) { + file = convertToMulterFile(file); + } + const s3 = s3Factory(); const { params, key, fileType } = generateFileParams(file, filename, filekey); @@ -63,6 +68,19 @@ const buildFileUrl = async (key: string, path: "public" | "private") => { return `${process.env.API_URL_EXT}/file/${path}${s3Pathname}`; }; +const convertToMulterFile = (buffer: Buffer): Express.Multer.File => ({ + buffer: buffer, + originalname: "${data.id}.json", + mimetype: "application/json", + size: buffer.length, + fieldname: "file", + encoding: "7bit", + stream: Readable.from(buffer), + destination: "", + filename: "", + path: "", +}); + export function generateFileParams( file: Express.Multer.File, filename: string, diff --git a/api.planx.uk/modules/send/s3/index.ts b/api.planx.uk/modules/send/s3/index.ts index 2a414d4e94..766e3ad629 100644 --- a/api.planx.uk/modules/send/s3/index.ts +++ b/api.planx.uk/modules/send/s3/index.ts @@ -10,6 +10,10 @@ import { isApplicationTypeSupported } from "../utils/helpers.js"; import type { SendIntegrationController } from "../types.js"; import { convertObjectToMulterJSONFile } from "../../file/service/utils.js"; +// TS fails to validate this complex type when awaited +// Represent as a simple object to allow enough typechecking to kick in +type DigitalPlanningPayload = Record; + interface CreateS3Application { insertS3Application: { id: string; From 950453d25bcc4cab10ff64dc935e00a62ee05e77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Sat, 7 Dec 2024 09:53:54 +0000 Subject: [PATCH 2/4] refactor: Move to utils, only use in s3 upload --- .../modules/file/service/uploadFile.ts | 20 +------------------ api.planx.uk/modules/send/s3/index.ts | 4 ---- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/api.planx.uk/modules/file/service/uploadFile.ts b/api.planx.uk/modules/file/service/uploadFile.ts index d272018c70..ead3cee9af 100644 --- a/api.planx.uk/modules/file/service/uploadFile.ts +++ b/api.planx.uk/modules/file/service/uploadFile.ts @@ -7,7 +7,6 @@ import mime from "mime"; import { customAlphabet } from "nanoid"; import { isLiveEnv } from "../../../helpers.js"; import { s3Factory } from "./utils.js"; -import { Readable } from "stream"; const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); export const uploadPublicFile = async ( @@ -29,14 +28,10 @@ export const uploadPublicFile = async ( }; export const uploadPrivateFile = async ( - file: Express.Multer.File | Buffer, + file: Express.Multer.File, filename: string, filekey?: string, ) => { - if (file instanceof Buffer) { - file = convertToMulterFile(file); - } - const s3 = s3Factory(); const { params, key, fileType } = generateFileParams(file, filename, filekey); @@ -68,19 +63,6 @@ const buildFileUrl = async (key: string, path: "public" | "private") => { return `${process.env.API_URL_EXT}/file/${path}${s3Pathname}`; }; -const convertToMulterFile = (buffer: Buffer): Express.Multer.File => ({ - buffer: buffer, - originalname: "${data.id}.json", - mimetype: "application/json", - size: buffer.length, - fieldname: "file", - encoding: "7bit", - stream: Readable.from(buffer), - destination: "", - filename: "", - path: "", -}); - export function generateFileParams( file: Express.Multer.File, filename: string, diff --git a/api.planx.uk/modules/send/s3/index.ts b/api.planx.uk/modules/send/s3/index.ts index 766e3ad629..2a414d4e94 100644 --- a/api.planx.uk/modules/send/s3/index.ts +++ b/api.planx.uk/modules/send/s3/index.ts @@ -10,10 +10,6 @@ import { isApplicationTypeSupported } from "../utils/helpers.js"; import type { SendIntegrationController } from "../types.js"; import { convertObjectToMulterJSONFile } from "../../file/service/utils.js"; -// TS fails to validate this complex type when awaited -// Represent as a simple object to allow enough typechecking to kick in -type DigitalPlanningPayload = Record; - interface CreateS3Application { insertS3Application: { id: string; From d065437a4ce855b8f24026cc7f60204826282158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Sat, 7 Dec 2024 15:07:03 +0000 Subject: [PATCH 3/4] test(api): Coverage for send/s3 module --- api.planx.uk/modules/send/s3/index.test.ts | 352 ++++++++++++++++----- api.planx.uk/vitest.config.ts | 8 +- 2 files changed, 285 insertions(+), 75 deletions(-) diff --git a/api.planx.uk/modules/send/s3/index.test.ts b/api.planx.uk/modules/send/s3/index.test.ts index 5570fb7205..4215e17a84 100644 --- a/api.planx.uk/modules/send/s3/index.test.ts +++ b/api.planx.uk/modules/send/s3/index.test.ts @@ -1,58 +1,47 @@ import supertest from "supertest"; -import type planxCore from "@opensystemslab/planx-core"; import app from "../../../server.js"; import { expectedPlanningPermissionPayload } from "../../../tests/mocks/digitalPlanningDataMocks.js"; import { queryMock } from "../../../tests/graphqlQueryMock.js"; +import { $api } from "../../../client/index.js"; +import { mockLowcalSession } from "../../../tests/mocks/saveAndReturnMocks.js"; +import axios, { type AxiosRequestConfig } from "axios"; +import { markSessionAsSubmitted } from "../../saveAndReturn/service/utils.js"; + +const sessionId = "3188f052-a032-4755-be63-72b0ba497eb6"; +const mockPowerAutomateWebhookURL = "http://www.example.com"; +const mockPowerAutomateAPIKey = "my-power-automate-api-key"; vi.mock("../../saveAndReturn/service/utils", () => ({ markSessionAsSubmitted: vi.fn(), })); -vi.mock("@opensystemslab/planx-core", async (importOriginal) => { - const actualCore = await importOriginal(); - const actualCoreDomainClient = actualCore.CoreDomainClient; - - return { - CoreDomainClient: class extends actualCoreDomainClient { - constructor() { - super(); - this.export.digitalPlanningDataPayload = async () => - vi.fn().mockResolvedValue({ - exportData: expectedPlanningPermissionPayload, - }); - } - }, - }; -}); +vi.mock("../../file/service/uploadFile.js", () => ({ + uploadPrivateFile: vi + .fn() + .mockResolvedValue({ fileUrl: "https://my-file-url.com" }), +})); + +vi.mock("../../client/index.js"); + +vi.mock("axios", () => ({ + default: vi.fn(), +})); +const mockedAxios = vi.mocked(axios, true); -describe(`uploading an application to S3`, () => { +describe("uploading an application to S3", () => { beforeEach(() => { - queryMock.mockQuery({ - name: "GetStagingIntegrations", - data: { - teams: [ - { - integrations: { - powerAutomateWebhookURL: "test.azure.com/whatever", - powerAutomateAPIKey: "secret", - }, - }, - ], - }, - variables: { - slug: "barnet", - }, + $api.team.getIntegrations = vi.fn().mockResolvedValue({ + powerAutomateWebhookURL: mockPowerAutomateWebhookURL, + powerAutomateAPIKey: mockPowerAutomateAPIKey, }); - queryMock.mockQuery({ - name: "GetStagingIntegrations", - data: { - teams: [], - }, - variables: { - slug: "unsupported-team", - }, - }); + $api.session.find = vi.fn().mockResolvedValue(mockLowcalSession); + + $api.export.digitalPlanningDataPayload = vi + .fn() + .mockResolvedValue(expectedPlanningPermissionPayload); + + mockedAxios.mockResolvedValue({ data: { success: true }, status: 200 }); queryMock.mockQuery({ name: "CreateS3Application", @@ -63,39 +52,260 @@ describe(`uploading an application to S3`, () => { }); }); - it("requires auth", async () => { - await supertest(app) - .post("/upload-submission/barnet") - .send({ payload: { sessionId: "123" } }) - .expect(401); - }); + describe("request validation", () => { + it("requires auth", async () => { + await supertest(app) + .post("/upload-submission/barnet") + .send({ payload: { sessionId: "123" } }) + .expect(401); + }); - it("throws an error if payload is missing", async () => { - await supertest(app) - .post("/upload-submission/barnet") - .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) - .send({ payload: null }) - .expect(400) - .then((res) => { - expect(res.body).toHaveProperty("issues"); - expect(res.body).toHaveProperty("name", "ZodError"); + it("throws an error if payload is missing", async () => { + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ payload: null }) + .expect(400) + .then((res) => { + expect(res.body).toHaveProperty("issues"); + expect(res.body).toHaveProperty("name", "ZodError"); + }); + }); + + it("throws an error if powerAutomateWebhookURL is not set", async () => { + $api.team.getIntegrations = vi.fn().mockResolvedValueOnce({ + powerAutomateWebhookURL: null, + powerAutomateAPIKey: "some-key", }); - }); - it("throws an error if team is unsupported", async () => { - await supertest(app) - .post("/upload-submission/unsupported-team") - .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) - .send({ - payload: { sessionId: "3188f052-a032-4755-be63-72b0ba497eb6" }, - }) - .expect(500) - .then((res) => { - expect(res.body.error).toMatch( - /No team matching "unsupported-team" found/, - ); + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }) + .expect(400) + .then((res) => { + expect(res.body.error).toMatch( + /Upload to S3 is not enabled for this local authority/, + ); + }); + }); + + it("throws an error if powerAutomateAPIKey is not set", async () => { + $api.team.getIntegrations = vi.fn().mockResolvedValueOnce({ + powerAutomateWebhookURL: "https://www.example.com", + powerAutomateAPIKey: null, }); + + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }) + .expect(400) + .then((res) => { + expect(res.body.error).toMatch( + /Upload to S3 is not enabled for this local authority/, + ); + }); + }); + }); + + describe("payload validation", () => { + it("validates statutory payloads", async () => { + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }); + + // Verify mock session is a statutory application + expect( + mockLowcalSession.data.passport.data?.["application.type"]?.[0], + ).toBe("ldc.proposed"); + + expect($api.export.digitalPlanningDataPayload).toHaveBeenCalledTimes(1); + expect($api.export.digitalPlanningDataPayload).toHaveBeenCalledWith( + sessionId, + false, // Validation not skipped + ); + + // Validation status passed to webhook + expect(mockedAxios).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + payload: "Validated ODP Schema", + }), + }), + ); + }); + + it("does not validate discretionary payloads", async () => { + // Set up mock discretionary payload + const mockDiscretionarySession = structuredClone(mockLowcalSession); + mockDiscretionarySession.data.passport.data["application.type"][0] = + "reportAPlanningBreach"; + $api.session.find = vi + .fn() + .mockResolvedValueOnce(mockDiscretionarySession); + + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }); + + expect($api.export.digitalPlanningDataPayload).toHaveBeenCalledTimes(1); + expect($api.export.digitalPlanningDataPayload).toHaveBeenCalledWith( + sessionId, + true, // Validation skipped + ); + + // Validation status passed to webhook + expect(mockedAxios).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + payload: "Discretionary", + }), + }), + ); + }); }); - it.todo("succeeds"); // mock uploadPrivateFile ?? + describe("error handling", () => { + it("throws an error if the webhook request fails", async () => { + mockedAxios.mockRejectedValueOnce( + new Error( + "Something went wrong with the webhook request to Power Automate", + ), + ); + + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }) + .expect(500) + .then((res) => { + expect(res.body.error).toMatch( + /Failed to send submission notification/, + ); + }); + }); + + it("throws an error if an internal process fails", async () => { + $api.session.find = vi + .fn() + .mockRejectedValueOnce(new Error("Something went wrong!")); + + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }) + .expect(500) + .then((res) => { + expect(res.body.error).toMatch(/Failed to upload submission to S3/); + }); + }); + }); + + describe("success", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const callAPI = async () => + await supertest(app) + .post("/upload-submission/barnet") + .set({ Authorization: process.env.HASURA_PLANX_API_KEY! }) + .send({ + payload: { sessionId }, + }); + + it("makes a request to the configured Power Automate webhook", async () => { + await callAPI(); + + expect(mockedAxios).toHaveBeenCalledOnce(); + const request = mockedAxios.mock.calls[0][0] as AxiosRequestConfig; + expect(request.url).toEqual(mockPowerAutomateWebhookURL); + }); + + it("sets Power Automate API key in request header", async () => { + await callAPI(); + + expect(mockedAxios).toHaveBeenCalledOnce(); + const request = mockedAxios.mock.calls[0][0] as AxiosRequestConfig; + expect(request.headers).toHaveProperty("apiKey", mockPowerAutomateAPIKey); + }); + + it("generates the expected body for the Power Automate webhook", async () => { + await callAPI(); + + expect(mockedAxios).toHaveBeenCalledOnce(); + const request = mockedAxios.mock.calls[0][0] as AxiosRequestConfig; + expect(request.data).toEqual({ + message: "New submission from PlanX", + environment: "staging", + file: "https://my-file-url.com", + payload: "Validated ODP Schema", + service: "Apply for a Lawful Development Certificate", + }); + }); + + it("passes along the correct application environment details", async () => { + vi.stubEnv("APP_ENVIRONMENT", "production"); + + await callAPI(); + + expect(mockedAxios).toHaveBeenCalledOnce(); + const request = mockedAxios.mock.calls[0][0] as AxiosRequestConfig; + expect(request.data.environment).toEqual("production"); + }); + + it("marks a session as submitted", async () => { + await callAPI(); + + expect(markSessionAsSubmitted).toHaveBeenCalledOnce(); + expect(markSessionAsSubmitted).toHaveBeenCalledWith(sessionId); + }); + + it("writes an audit record the the s3_applications table", async () => { + await callAPI(); + + const graphQLCalls = queryMock.getCalls(); + + expect(graphQLCalls).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: "CreateS3Application", + }), + ]), + ); + }); + + it("returns a success message upon completion", async () => { + const res = await callAPI(); + + expect($api.export.digitalPlanningDataPayload).toHaveBeenCalledWith( + sessionId, + false, + ); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ + message: + "Successfully uploaded submission to S3: https://my-file-url.com", + payload: "Validated ODP Schema", + webhookResponse: 200, + auditEntryId: 1, + }); + }); + }); }); diff --git a/api.planx.uk/vitest.config.ts b/api.planx.uk/vitest.config.ts index 77fcbeff21..16b2a43efd 100644 --- a/api.planx.uk/vitest.config.ts +++ b/api.planx.uk/vitest.config.ts @@ -13,10 +13,10 @@ export default defineConfig({ // html reporter required to inspect coverage in Vitest UI dashboard reporter: ["lcov", "html", "text-summary"], thresholds: { - statements: 73.28, - branches: 55.27, - functions: 73.59, - lines: 73.42, + statements: 73.41, + branches: 55.76, + functions: 74.09, + lines: 73.54, autoUpdate: true, }, }, From 821a77e38448f3ecbafa687f80570ac2b34890d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Sat, 7 Dec 2024 16:43:57 +0000 Subject: [PATCH 4/4] refactor: Unnest async calls --- api.planx.uk/modules/send/s3/index.ts | 96 +++++++++++++-------------- 1 file changed, 46 insertions(+), 50 deletions(-) diff --git a/api.planx.uk/modules/send/s3/index.ts b/api.planx.uk/modules/send/s3/index.ts index 2a414d4e94..eed4ea0b0a 100644 --- a/api.planx.uk/modules/send/s3/index.ts +++ b/api.planx.uk/modules/send/s3/index.ts @@ -73,62 +73,58 @@ const sendToS3: SendIntegrationController = async (_req, res, next) => { payload: skipValidation ? "Discretionary" : "Validated ODP Schema", }, }; - const webhookResponse = await axios(webhookRequest) - .then(async (res) => { - // Mark session as submitted so that reminder and expiry emails are not triggered - markSessionAsSubmitted(sessionId); - // Create an audit entry - const applicationId = await $api.client.request( - gql` - mutation CreateS3Application( - $session_id: String! - $team_slug: String! - $webhook_request: jsonb! - $webhook_response: jsonb = {} - ) { - insertS3Application: insert_s3_applications_one( - object: { - session_id: $session_id - team_slug: $team_slug - webhook_request: $webhook_request - webhook_response: $webhook_response - } - ) { - id - } - } - `, - { - session_id: sessionId, - team_slug: localAuthority, - webhook_request: webhookRequest, - webhook_response: { - status: res.status, - statusText: res.statusText, - headers: res.headers, - config: res.config, - data: res.data, - }, - }, - ); + const webhookResponse = await axios(webhookRequest).catch((error) => { + throw new Error( + `Failed to send submission notification to ${localAuthority}'s Power Automate Webhook (${sessionId}): ${error}`, + ); + }); - return { - id: applicationId.insertS3Application?.id, - axiosResponse: res, - }; - }) - .catch((error) => { - throw new Error( - `Failed to send submission notification to ${localAuthority}'s Power Automate Webhook (${sessionId}): ${error}`, - ); - }); + // Mark session as submitted so that reminder and expiry emails are not triggered + markSessionAsSubmitted(sessionId); + + // Create an audit entry + const { + insertS3Application: { id: auditEntryId }, + } = await $api.client.request( + gql` + mutation CreateS3Application( + $session_id: String! + $team_slug: String! + $webhook_request: jsonb! + $webhook_response: jsonb = {} + ) { + insertS3Application: insert_s3_applications_one( + object: { + session_id: $session_id + team_slug: $team_slug + webhook_request: $webhook_request + webhook_response: $webhook_response + } + ) { + id + } + } + `, + { + session_id: sessionId, + team_slug: localAuthority, + webhook_request: webhookRequest, + webhook_response: { + status: webhookResponse.status, + statusText: webhookResponse.statusText, + headers: webhookResponse.headers, + config: webhookResponse.config, + data: webhookResponse.data, + }, + }, + ); res.status(200).send({ message: `Successfully uploaded submission to S3: ${fileUrl}`, payload: skipValidation ? "Discretionary" : "Validated ODP Schema", - webhookResponse: webhookResponse.axiosResponse.status, - auditEntryId: webhookResponse.id, + webhookResponse: webhookResponse.status, + auditEntryId, }); } catch (error) { return next({