Bring get letters in line with patch letter

Author: francisco-videira-nhs

lambdas/api-handler/src/contracts/errors.ts

@@ -33,7 +33,9 @@ export enum ApiErrorDetail {
   InvalidRequestMissingBody = 'The request is missing the body',
   InvalidRequestMissingLetterIdPathParameter = 'The request is missing the letter id path parameter',
   InvalidRequestLetterIdsMismatch = 'The letter ID in the request body does not match the letter ID path parameter',
-  InvalidRequestBody = 'The request body is invalid'
+  InvalidRequestBody = 'The request body is invalid',
+  InvalidRequestLimitNotANumber = "The limit parameter is not a number",
+  InvalidRequestLimitNotPositive = "The limit parameter is not positive"
 }
 
 export function buildApiError(params: {

lambdas/api-handler/src/contracts/letter-api.ts

@@ -18,11 +18,11 @@ export const LetterApiStatusSchema = z.enum([
 export type LetterApiStatus = z.infer<typeof LetterApiStatusSchema>;
 
 export const LetterApiAttributesSchema = z.object({
-  reasonCode: z.number().optional(),
-  reasonText: z.string().optional(),
-  requestedProductionStatus: z.enum(["ACTIVE", "HOLD", "CANCEL"]).optional(),
   specificationId: z.string(),
   status: LetterApiStatusSchema,
+  reasonCode: z.number().optional(),
+  reasonText: z.string().optional(),
+  groupId: z.string().optional()
 });
 
 export type LetterApiAttributes = z.infer<typeof LetterApiAttributesSchema>;
@@ -36,7 +36,13 @@ export const LetterApiResourceSchema = z.object({
 export type LetterApiResource = z.infer<typeof LetterApiResourceSchema>;
 
 export const LetterApiDocumentSchema = z.object({
-  data: LetterApiResourceSchema,
+  data: LetterApiResourceSchema
+});
+
+export const LettersApiDocumentSchema = z.object({
+  data: z.array(LetterApiResourceSchema)
 });
 
 export type LetterApiDocument = z.infer<typeof LetterApiDocumentSchema>;
+
+export type LettersApiDocument = z.infer<typeof LettersApiDocumentSchema>;

lambdas/api-handler/src/handlers/__tests__/get-letters.test.ts

@@ -1,8 +1,17 @@
 import { getLetters } from '../../index';
-import type { Context } from 'aws-lambda';
+import type { APIGatewayProxyResult, Context } from 'aws-lambda';
 import { mockDeep } from 'jest-mock-extended';
 import { makeApiGwEvent } from './utils/test-utils';
 import * as letterService from '../../services/letter-operations';
+import { mapErrorToResponse } from '../../mappers/error-mapper';
+
+jest.mock('../../mappers/error-mapper');
+const mockedMapErrorToResponse = jest.mocked(mapErrorToResponse);
+const expectedErrorResponse: APIGatewayProxyResult = {
+  statusCode: 400,
+  body: "Error"
+};
+mockedMapErrorToResponse.mockReturnValue(expectedErrorResponse);
 
 jest.mock('../../services/letter-operations');
 
@@ -15,7 +24,7 @@ jest.mock("../../config/lambda-config", () => ({
 describe('API Lambda handler', () => {
 
   beforeEach(() => {
-    jest.resetAllMocks();
+    jest.clearAllMocks();
   });
 
   it('returns 200 OK with basic paginated resources', async () => {
@@ -52,18 +61,18 @@ describe('API Lambda handler', () => {
         {
           id: "l1",
           type: "Letter",
-          attributes: { specificationId: "s1", groupId: 'g1', status: "PENDING" },
+          attributes: { reasonCode: 123, reasonText: "Reason text", specificationId: "s1", status: "PENDING", groupId: 'g1' },
         },
         {
           id: "l2",
           type: "Letter",
-          attributes: { specificationId: "s1", groupId: 'g1', status: "PENDING" },
+          attributes: { reasonCode: 123, reasonText: "Reason text", specificationId: "s1", status: "PENDING", groupId: 'g1' },
         },
         {
           id: "l3",
           type: "Letter",
-          attributes: { specificationId: "s1", groupId: 'g1', status: "PENDING" },
-        },
+          attributes: { reasonCode: 123, reasonText: "Reason text", specificationId: "s1", status: "PENDING", groupId: 'g1' },
+        }
       ],
     };
 
@@ -73,46 +82,30 @@ describe('API Lambda handler', () => {
     });
   });
 
-  it('returns 404 Not Found for an unknown path', async () => {
-    const event = makeApiGwEvent({ path: '/unknown' });
-    const context = mockDeep<Context>();
-    const callback = jest.fn();
-    const result = await getLetters(event, context, callback);
-
-    expect(result).toEqual({
-      statusCode: 404,
-      body: 'Not Found',
-    });
-  });
-
   it("returns 400 if the limit parameter is not a number", async () => {
     const event = makeApiGwEvent({
       path: "/letters",
       queryStringParameters: { limit: "1%" },
-    });
+      headers: {'nhsd-supplier-id': 'supplier1'}});
+
     const context = mockDeep<Context>();
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: "Bad Request: limit parameter is not a number",
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
   it("returns 400 if the limit parameter is not positive", async () => {
     const event = makeApiGwEvent({
       path: "/letters",
       queryStringParameters: { limit: "-1" },
-    });
+      headers: {'nhsd-supplier-id': 'supplier1'}});
+
     const context = mockDeep<Context>();
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: "Bad Request: limit parameter is not positive",
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
   it('returns 400 for missing supplier ID (empty headers)', async () => {
@@ -121,10 +114,7 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: 'Bad Request: Missing supplier ID',
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 
   it('returns 400 for missing supplier ID (undefined headers)', async () => {
@@ -133,9 +123,6 @@ describe('API Lambda handler', () => {
     const callback = jest.fn();
     const result = await getLetters(event, context, callback);
 
-    expect(result).toEqual({
-      statusCode: 400,
-      body: 'Bad Request: Missing supplier ID',
-    });
+    expect(result).toEqual(expectedErrorResponse);
   });
 });

lambdas/api-handler/src/handlers/__tests__/patch-letters.test.ts

@@ -33,7 +33,6 @@ function makeLetterApiDocument(id: string, status: LetterApiStatus) : LetterApiD
       attributes: {
         reasonCode: 123,
         reasonText: "Reason text",
-        requestedProductionStatus: "ACTIVE",
         specificationId: "spec1",
         status
       },

lambdas/api-handler/src/handlers/get-letters.ts

@@ -1,58 +1,26 @@
-import { APIGatewayProxyHandler } from "aws-lambda";
+import { APIGatewayProxyEventQueryStringParameters, APIGatewayProxyHandler } from "aws-lambda";
 import { getLettersForSupplier } from "../services/letter-operations";
 import { createLetterRepository } from "../infrastructure/letter-repo-factory";
 import { LetterBase } from "../../../../internal/datastore/src";
+import { assertNotEmpty } from "../utils/validation";
+import * as errors from '../contracts/errors';
+import { lambdaConfig } from "../config/lambda-config";
 import pino from 'pino';
+import { mapErrorToResponse } from "../mappers/error-mapper";
+import { ValidationError } from "../errors";
+import { mapLetterBaseToApiDocument, mapLetterBaseToApiResource } from "../mappers/letter-mapper";
 
 const letterRepo = createLetterRepository();
 const log = pino();
 
+// The endpoint should only return pending letters for now
+const status = "PENDING";
+
 export const getLetters: APIGatewayProxyHandler = async (event) => {
-  if (event.path === "/letters") {
-    const supplierId = event.headers ? event.headers["NHSD-Supplier-ID"] : undefined;
-
-    if (!supplierId) {
-      log.info({
-        description: 'Supplier ID not provided'
-      });
-      return {
-        statusCode: 400,
-        body: "Bad Request: Missing supplier ID",
-      };
-    }
-
-    // The endpoint should only return pending letters for now
-    const status = "PENDING";
-
-    let limit = event.queryStringParameters?.limit;
-
-    if (!limit) {
-      limit = "10";
-    }
-
-    let limitNumber = Number(limit);
-
-    if (isNaN(limitNumber)) {
-      log.info({
-        description: "limit parameter is not a number",
-        limit,
-      });
-      return {
-        statusCode: 400,
-        body: "Bad Request: limit parameter is not a number",
-      };
-    }
-
-    if (limitNumber < 0) {
-      log.info({
-        description: "limit parameter is not positive",
-        limit,
-      });
-      return {
-        statusCode: 400,
-        body: "Bad Request: limit parameter is not positive",
-      };
-    }
+
+  try {
+    const supplierId = assertNotEmpty(event.headers[lambdaConfig.SUPPLIER_ID_HEADER], errors.ApiErrorDetail.InvalidRequestMissingSupplierId);
+    const limitNumber = validateLimit(event.queryStringParameters);
 
     const letters = await getLettersForSupplier(
       supplierId,
@@ -61,12 +29,14 @@ export const getLetters: APIGatewayProxyHandler = async (event) => {
       letterRepo,
     );
 
-    const response = createGetLettersResponse(letters);
+    const response = {
+      data: letters.map((letter: LetterBase) => (mapLetterBaseToApiResource(letter)))
+    };
 
     log.info({
       description: 'Pending letters successfully fetched',
       supplierId,
-      limit,
+      limitNumber,
       status,
       lettersCount: letters.length
     });
@@ -76,44 +46,43 @@ export const getLetters: APIGatewayProxyHandler = async (event) => {
       body: JSON.stringify(response, null, 2),
     };
   }
+  catch (error) {
+    return mapErrorToResponse(error);
+  }
+};
 
-  log.warn({
-    description: 'Unsupported event path',
-    path: event.path
-  });
+function validateLimit(queryStringParameters: APIGatewayProxyEventQueryStringParameters | null) : number {
 
-  return {
-    statusCode: 404,
-    body: "Not Found",
-  };
-};
+  let limit = queryStringParameters?.limit;
 
-interface GetLettersResponse {
-  data: Array<{
-    type: "Letter";
-    id: string;
-    attributes: {
-      specificationId: string;
-      groupId: string;
-      status: string;
-      reasonCode?: number;
-      reasonText?: string;
-    };
-  }>;
+  if (!limit) {
+    limit = "10";
+  }
+
+  let limitNumber = Number(limit);
+
+  assertIsNumber(limitNumber, limit);
+  assertIsPositive(limitNumber, limit);
+
+  return limitNumber;
 }
 
-function createGetLettersResponse(letters: LetterBase[]): GetLettersResponse {
-  return {
-    data: letters.map((letter) => ({
-      id: letter.id,
-      type: "Letter",
-      attributes: {
-        specificationId: letter.specificationId,
-        groupId: letter.groupId,
-        status: letter.status,
-        reasonCode: letter.reasonCode,
-        reasonText: letter.reasonText,
-      },
-    })),
-  };
+function assertIsNumber(limitNumber: number, limit: string) {
+  if (isNaN(limitNumber)) {
+    log.info({
+      description: "limit parameter is not a number",
+      limit,
+    });
+    throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotANumber);
+  }
+}
+
+function assertIsPositive(limitNumber: number, limit?: string) {
+  if (limitNumber < 0) {
+    log.info({
+      description: "limit parameter is not positive",
+      limit,
+    });
+    throw new ValidationError(errors.ApiErrorDetail.InvalidRequestLimitNotPositive);
+  }
 }

lambdas/api-handler/src/handlers/patch-letters.ts

@@ -6,6 +6,7 @@ import * as errors from '../contracts/errors';
 import { ValidationError } from '../errors';
 import { mapErrorToResponse } from '../mappers/error-mapper';
 import { lambdaConfig } from "../config/lambda-config";
+import { assertNotEmpty } from '../utils/validation';
 
 const letterRepo = createLetterRepository();
 export const patchLetters: APIGatewayProxyHandler = async (event) => {
@@ -37,10 +38,3 @@ export const patchLetters: APIGatewayProxyHandler = async (event) => {
     return mapErrorToResponse(error);
   }
 };
-
-function assertNotEmpty(value: string | null | undefined, detail: errors.ApiErrorDetail): string {
-  if (!value || value.trim() === '') {
-    throw new ValidationError(detail);
-  }
-  return value;
-}

lambdas/api-handler/src/mappers/__tests__/letter-mapper.test.ts

@@ -1,8 +1,8 @@
-import { toApiLetter } from '../letter-mapper';
+import { mapLetterBaseToApiDocument } from '../letter-mapper';
 import { Letter } from '../../../../../internal/datastore';
 import { LetterApiDocument } from '../../contracts/letter-api';
 
-describe('toApiLetter', () => {
+describe('mapLetterBaseToApiDocument', () => {
   it('maps a Letter to LetterApiDocument', () => {
     const letter: Letter = {
       id: 'abc123',
@@ -18,7 +18,7 @@ describe('toApiLetter', () => {
       ttl: 123
     };
 
-    const result: LetterApiDocument = toApiLetter(letter);
+    const result: LetterApiDocument = mapLetterBaseToApiDocument(letter);
 
     expect(result).toEqual({
       data: {
@@ -27,9 +27,9 @@ describe('toApiLetter', () => {
         attributes: {
           reasonCode: 123,
           reasonText: 'Reason text',
-          requestedProductionStatus: 'ACTIVE',
           specificationId: 'spec123',
-          status: 'PENDING'
+          status: 'PENDING',
+          groupId: 'group123'
         }
       }
     });