diff --git a/infrastructure/terraform/components/api/module_lambda_patch_letter.tf b/infrastructure/terraform/components/api/module_lambda_patch_letter.tf index 942335b53..b45164602 100644 --- a/infrastructure/terraform/components/api/module_lambda_patch_letter.tf +++ b/infrastructure/terraform/components/api/module_lambda_patch_letter.tf @@ -35,7 +35,9 @@ module "patch_letter" { log_destination_arn = local.destination_arn log_subscription_role_arn = local.acct.log_subscription_role_arn - lambda_env_vars = merge(local.common_lambda_env_vars, {}) + lambda_env_vars = merge(local.common_lambda_env_vars, { + QUEUE_URL = module.letter_status_updates_queue.sqs_queue_url + }) } data "aws_iam_policy_document" "patch_letter_lambda" { @@ -54,21 +56,16 @@ data "aws_iam_policy_document" "patch_letter_lambda" { } statement { - sid = "AllowDynamoDBAccess" + sid = "AllowQueueAccess" effect = "Allow" actions = [ - "dynamodb:BatchGetItem", - "dynamodb:BatchWriteItem", - "dynamodb:GetItem", - "dynamodb:PutItem", - "dynamodb:Query", - "dynamodb:Scan", - "dynamodb:UpdateItem", + "sqs:SendMessage", + "sqs:GetQueueAttributes", ] resources = [ - aws_dynamodb_table.letters.arn, + module.letter_status_updates_queue.sqs_queue_arn ] } } diff --git a/lambdas/api-handler/src/handlers/__tests__/patch-letter.test.ts b/lambdas/api-handler/src/handlers/__tests__/patch-letter.test.ts index dc573c5eb..968758306 100644 --- a/lambdas/api-handler/src/handlers/__tests__/patch-letter.test.ts +++ b/lambdas/api-handler/src/handlers/__tests__/patch-letter.test.ts @@ -1,7 +1,7 @@ // mock service jest.mock('../../services/letter-operations'); import * as letterService from '../../services/letter-operations'; -const mockedPatchLetterStatus = jest.mocked(letterService.patchLetterStatus); +const mockedBatchUpdateStatus = jest.mocked(letterService.enqueueLetterUpdateRequests); // mock mapper jest.mock('../../mappers/error-mapper'); @@ -59,7 +59,7 @@ describe('patchLetter API Handler', () => { } as unknown as EnvVars } as Deps; - it('returns 200 OK with updated resource', async () => { + it('returns 202 Accepted', async () => { const event = makeApiGwEvent({ path: '/letters/id1', body: requestBody, @@ -86,14 +86,14 @@ describe('patchLetter API Handler', () => { } } }; - mockedPatchLetterStatus.mockResolvedValue(updateLetterServiceResponse); + mockedBatchUpdateStatus.mockResolvedValue(); const patchLetterHandler = createPatchLetterHandler(mockedDeps); const result = await patchLetterHandler(event, context, callback); expect(result).toEqual({ - statusCode: 200, - body: JSON.stringify(updateLetterServiceResponse, null, 2) + statusCode: 202, + body: '' }); }); @@ -137,16 +137,12 @@ describe('patchLetter API Handler', () => { expect(result).toEqual(expectedErrorResponse); }); - it('returns error response when error is thrown by service', async () => { - const error = new Error('Service error'); - mockedPatchLetterStatus.mockRejectedValue(error); - + it('returns error when supplier id is missing', async () => { const event = makeApiGwEvent({ path: '/letters/id1', body: requestBody, pathParameters: {id: 'id1'}, headers: { - 'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId' } @@ -157,16 +153,17 @@ describe('patchLetter API Handler', () => { const patchLetterHandler = createPatchLetterHandler(mockedDeps); const result = await patchLetterHandler(event, context, callback); - expect(mockedProcessError).toHaveBeenCalledWith(error, 'correlationId', mockedDeps.logger); + expect(mockedProcessError).toHaveBeenCalledWith(new Error('The supplier ID is missing from the request'), 'correlationId', mockedDeps.logger); expect(result).toEqual(expectedErrorResponse); }); - it('returns error when supplier id is missing', async () => { + it('returns error when request body does not have correct shape', async () => { const event = makeApiGwEvent({ path: '/letters/id1', - body: requestBody, + body: "{test: 'test'}", pathParameters: {id: 'id1'}, headers: { + 'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', 'x-request-id': 'requestId' } @@ -177,14 +174,14 @@ describe('patchLetter API Handler', () => { const patchLetterHandler = createPatchLetterHandler(mockedDeps); const result = await patchLetterHandler(event, context, callback); - expect(mockedProcessError).toHaveBeenCalledWith(new Error('The supplier ID is missing from the request'), 'correlationId', mockedDeps.logger); + expect(mockedProcessError).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestBody), 'correlationId', mockedDeps.logger); expect(result).toEqual(expectedErrorResponse); }); - it('returns error when request body does not have correct shape', async () => { + it('returns error when request body is not json', async () => { const event = makeApiGwEvent({ path: '/letters/id1', - body: "{test: 'test'}", + body: '{#invalidJSON', pathParameters: {id: 'id1'}, headers: { 'nhsd-supplier-id': 'supplier1', @@ -202,11 +199,11 @@ describe('patchLetter API Handler', () => { expect(result).toEqual(expectedErrorResponse); }); - it('returns error when request body is not json', async () => { + it('returns error if path letterId and body letterId do not match', async () => { const event = makeApiGwEvent({ - path: '/letters/id1', - body: '{#invalidJSON', - pathParameters: {id: 'id1'}, + path: '/letters/id2', + body: requestBody, + pathParameters: {id: 'id2'}, headers: { 'nhsd-supplier-id': 'supplier1', 'nhsd-correlation-id': 'correlationId', @@ -219,7 +216,7 @@ describe('patchLetter API Handler', () => { const patchLetterHandler = createPatchLetterHandler(mockedDeps); const result = await patchLetterHandler(event, context, callback); - expect(mockedProcessError).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestBody), 'correlationId', mockedDeps.logger); + expect(mockedProcessError).toHaveBeenCalledWith(new ValidationError(errors.ApiErrorDetail.InvalidRequestLetterIdsMismatch), 'correlationId', mockedDeps.logger); expect(result).toEqual(expectedErrorResponse); }); diff --git a/lambdas/api-handler/src/handlers/patch-letter.ts b/lambdas/api-handler/src/handlers/patch-letter.ts index 245052290..8ad221e57 100644 --- a/lambdas/api-handler/src/handlers/patch-letter.ts +++ b/lambdas/api-handler/src/handlers/patch-letter.ts @@ -1,6 +1,6 @@ import { APIGatewayProxyHandler } from 'aws-lambda'; -import { patchLetterStatus } from '../services/letter-operations'; -import { PatchLetterRequest, PatchLetterRequestSchema } from '../contracts/letters'; +import { enqueueLetterUpdateRequests } from '../services/letter-operations'; +import { LetterDto, PatchLetterRequest, PatchLetterRequestSchema } from '../contracts/letters'; import { ApiErrorDetail } from '../contracts/errors'; import { ValidationError } from '../errors'; import { processError } from '../mappers/error-mapper'; @@ -36,11 +36,17 @@ export function createPatchLetterHandler(deps: Deps): APIGatewayProxyHandler { else throw error; } - const updatedLetter = await patchLetterStatus(mapPatchLetterToDto(patchLetterRequest, commonIds.value.supplierId), letterId, deps.letterRepo); + const letterToUpdate: LetterDto = mapPatchLetterToDto(patchLetterRequest, commonIds.value.supplierId); + + if (letterToUpdate.id !== letterId) { + throw new ValidationError(ApiErrorDetail.InvalidRequestLetterIdsMismatch); + } + + await enqueueLetterUpdateRequests([letterToUpdate], commonIds.value.correlationId, deps); return { - statusCode: 200, - body: JSON.stringify(updatedLetter, null, 2) + statusCode: 202, + body: '' }; } catch (error) { diff --git a/lambdas/api-handler/src/handlers/post-letters.ts b/lambdas/api-handler/src/handlers/post-letters.ts index d7182199d..26a3ffc12 100644 --- a/lambdas/api-handler/src/handlers/post-letters.ts +++ b/lambdas/api-handler/src/handlers/post-letters.ts @@ -4,6 +4,7 @@ import { ApiErrorDetail } from '../contracts/errors'; import { PostLettersRequest, PostLettersRequestSchema } from '../contracts/letters'; import { ValidationError } from '../errors'; import { processError } from '../mappers/error-mapper'; +import { mapPostLettersToDtoArray } from '../mappers/letter-mapper'; import { enqueueLetterUpdateRequests } from '../services/letter-operations'; import { extractCommonIds } from '../utils/commonIds'; import { assertNotEmpty, requireEnvVar } from '../utils/validation'; @@ -43,7 +44,11 @@ export function createPostLettersHandler(deps: Deps): APIGatewayProxyHandler { throw new ValidationError(ApiErrorDetail.InvalidRequestDuplicateLetterId); } - await enqueueLetterUpdateRequests(postLettersRequest, commonIds.value.supplierId, commonIds.value.correlationId, deps); + await enqueueLetterUpdateRequests( + mapPostLettersToDtoArray(postLettersRequest, commonIds.value.supplierId), + commonIds.value.correlationId, + deps + ); return { statusCode: 202, diff --git a/lambdas/api-handler/src/mappers/letter-mapper.ts b/lambdas/api-handler/src/mappers/letter-mapper.ts index 7d6efd734..44fb1ad24 100644 --- a/lambdas/api-handler/src/mappers/letter-mapper.ts +++ b/lambdas/api-handler/src/mappers/letter-mapper.ts @@ -11,14 +11,14 @@ export function mapPatchLetterToDto(request: PatchLetterRequest, supplierId: str }; } -export function mapPostLetterResourceToDto(request: PostLettersRequestResource, supplierId: string): LetterDto { - return { - id: request.id, +export function mapPostLettersToDtoArray(request: PostLettersRequest, supplierId: string): LetterDto[] { + return request.data.map( (letterToUpdate: PostLettersRequestResource) => ({ + id: letterToUpdate.id, supplierId, - status: LetterStatus.parse(request.attributes.status), - reasonCode: request.attributes.reasonCode, - reasonText: request.attributes.reasonText, - }; + status: LetterStatus.parse(letterToUpdate.attributes.status), + reasonCode: letterToUpdate.attributes.reasonCode, + reasonText: letterToUpdate.attributes.reasonText, + })); } export function mapToPatchLetterResponse(letter: LetterBase): PatchLetterResponse { diff --git a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts index 5c4c47cdf..88ea9433d 100644 --- a/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts +++ b/lambdas/api-handler/src/services/__tests__/letter-operations.test.ts @@ -1,7 +1,7 @@ import { Letter, LetterRepository } from '@internal/datastore'; import { Deps } from '../../config/deps'; import { LetterDto, PostLettersRequest } from '../../contracts/letters'; -import { enqueueLetterUpdateRequests, getLetterById, getLetterDataUrl, getLettersForSupplier, patchLetterStatus } from '../letter-operations'; +import { enqueueLetterUpdateRequests, getLetterById, getLetterDataUrl, getLettersForSupplier } from '../letter-operations'; import pino from 'pino'; jest.mock('@aws-sdk/s3-request-presigner', () => ({ @@ -93,67 +93,6 @@ describe("getLetterById", () => { }); -describe('patchLetterStatus function', () => { - - beforeEach(() => { - jest.clearAllMocks(); - }); - - const updatedLetterDto: LetterDto = { - id: 'letter1', - supplierId: 'supplier1', - status: 'REJECTED', - reasonCode: 'R01', - reasonText: 'Reason text' - }; - - const updatedLetter = makeLetter("letter1", "REJECTED"); - - it('should update the letter status successfully', async () => { - const mockRepo = { - updateLetterStatus: jest.fn().mockResolvedValue(updatedLetter) - }; - - const result = await patchLetterStatus(updatedLetterDto, 'letter1', mockRepo as any); - - expect(result).toEqual({ - data: - { - id: 'letter1', - type: 'Letter', - attributes: { - status: 'REJECTED', - reasonCode: updatedLetter.reasonCode, - reasonText: updatedLetter.reasonText, - specificationId: updatedLetter.specificationId, - groupId: updatedLetter.groupId - }, - } - }); - }); - - it('should throw validationError when letterIds differ', async () => { - await expect(patchLetterStatus(updatedLetterDto, 'letter2', {} as any)).rejects.toThrow("The letter ID in the request body does not match the letter ID path parameter"); - }); - - it('should throw notFoundError when letter does not exist', async () => { - const mockRepo = { - updateLetterStatus: jest.fn().mockRejectedValue(new Error('Letter with id l1 not found for supplier s1')) - }; - - await expect(patchLetterStatus(updatedLetterDto, 'letter1', mockRepo as any)).rejects.toThrow("No resource found with that ID"); - }); - - it('should throw unexpected error', async () => { - - const mockRepo = { - updateLetterStatus: jest.fn().mockRejectedValue(new Error('unexpected error')) - }; - - await expect(patchLetterStatus(updatedLetterDto, 'letter1', mockRepo as any)).rejects.toThrow("unexpected error"); - }); -}); - describe('getLetterDataUrl function', () => { beforeEach(() => { @@ -235,28 +174,25 @@ describe('enqueueLetterUpdateRequests function', () => { jest.clearAllMocks(); }); + const lettersToUpdate: LetterDto[] = [ + { + id: 'id1', + status: 'REJECTED', + supplierId: 's1', + specificationId: 'spec1', + groupId: 'g1', + reasonCode: '123', + reasonText: 'Reason text' + }, + { + id: 'id2', + status: 'ACCEPTED', + supplierId: 's1' + } + ]; + it('should update the letter status successfully', async () => { - const updateLettersRequest : PostLettersRequest = { - data: [ - { - id: 'id1', - type: 'Letter', - attributes: { - status: 'REJECTED', - reasonCode: 123, - reasonText: 'Reason text', - } - }, - { - id: 'id2', - type: 'Letter', - attributes: { - status: 'ACCEPTED' - } - } - ] - }; const sqsClient = { send: jest.fn() } as unknown as SQSClient; const logger = { error: jest.fn() } as unknown as pino.Logger; const env = { @@ -264,7 +200,7 @@ describe('enqueueLetterUpdateRequests function', () => { }; const deps: Deps = { sqsClient, logger, env } as Deps; - const result = await enqueueLetterUpdateRequests(updateLettersRequest, 'supplier1', 'correlationId1', deps); + const result = await enqueueLetterUpdateRequests(lettersToUpdate, 'correlationId1', deps); expect(result).toBeUndefined(); @@ -278,11 +214,13 @@ describe('enqueueLetterUpdateRequests function', () => { } }, MessageBody: JSON.stringify({ - id: updateLettersRequest.data[0].id, - supplierId: 'supplier1', - status: updateLettersRequest.data[0].attributes.status, - reasonCode: updateLettersRequest.data[0].attributes.reasonCode, - reasonText: updateLettersRequest.data[0].attributes.reasonText + id: lettersToUpdate[0].id, + status: lettersToUpdate[0].status, + supplierId: lettersToUpdate[0].supplierId, + specificationId: lettersToUpdate[0].specificationId, + groupId: lettersToUpdate[0].groupId, + reasonCode: lettersToUpdate[0].reasonCode, + reasonText: lettersToUpdate[0].reasonText }) } })); @@ -297,9 +235,9 @@ describe('enqueueLetterUpdateRequests function', () => { } }, MessageBody: JSON.stringify({ - id: updateLettersRequest.data[1].id, - supplierId: 'supplier1', - status: updateLettersRequest.data[1].attributes.status + id: lettersToUpdate[1].id, + status: lettersToUpdate[1].status, + supplierId: lettersToUpdate[1].supplierId }) } })); @@ -308,27 +246,8 @@ describe('enqueueLetterUpdateRequests function', () => { it('should log error if enqueueing fails', async () => { const mockError = new Error('error'); - - const updateLettersRequest : PostLettersRequest = { - data: [ - { - id: 'id1', - type: 'Letter', - attributes: { - status: 'ACCEPTED' - } - }, - { - id: 'id2', - type: 'Letter', - attributes: { - status: 'REJECTED' - } - } - ] - }; const sqsClient = { send: jest.fn() - .mockRejectedValue(mockError) + .mockRejectedValueOnce(mockError) .mockResolvedValueOnce({ MessageId: 'm1' }) } as unknown as SQSClient; const logger = { error: jest.fn() } as unknown as pino.Logger; @@ -337,7 +256,7 @@ describe('enqueueLetterUpdateRequests function', () => { }; const deps: Deps = { sqsClient, logger, env } as Deps; - const result = await enqueueLetterUpdateRequests(updateLettersRequest, 'supplier1', 'correlationId1', deps); + const result = await enqueueLetterUpdateRequests(lettersToUpdate, 'correlationId1', deps); expect(result).toBeUndefined(); @@ -351,19 +270,20 @@ describe('enqueueLetterUpdateRequests function', () => { } }, MessageBody: JSON.stringify({ - id: updateLettersRequest.data[1].id, - supplierId: 'supplier1', - status: updateLettersRequest.data[1].attributes.status + id: lettersToUpdate[1].id, + status: lettersToUpdate[1].status, + supplierId: lettersToUpdate[1].supplierId }) } })); + expect(deps.logger.error).toHaveBeenCalledTimes(1); expect(deps.logger.error).toHaveBeenCalledWith({ err: mockError, correlationId: 'correlationId1', - letterId: 'id2', - letterStatus: 'REJECTED', - supplierId: 'supplier1' + letterId: lettersToUpdate[0].id, + letterStatus: lettersToUpdate[0].status, + supplierId: lettersToUpdate[0].supplierId }, 'Error enqueuing letter status update'); }); }); diff --git a/lambdas/api-handler/src/services/letter-operations.ts b/lambdas/api-handler/src/services/letter-operations.ts index 839a1f819..4d559dda5 100644 --- a/lambdas/api-handler/src/services/letter-operations.ts +++ b/lambdas/api-handler/src/services/letter-operations.ts @@ -1,7 +1,6 @@ import { LetterBase, LetterRepository } from '@internal/datastore'; -import { NotFoundError, ValidationError } from '../errors'; -import { LetterDto, PatchLetterResponse, PostLettersRequest } from '../contracts/letters'; -import { mapPostLetterResourceToDto, mapToPatchLetterResponse } from '../mappers/letter-mapper'; +import { NotFoundError } from '../errors'; +import { LetterDto } from '../contracts/letters'; import { ApiErrorDetail } from '../contracts/errors'; import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; import { S3Client, GetObjectCommand } from '@aws-sdk/client-s3'; @@ -30,26 +29,6 @@ export const getLetterById = async (supplierId: string, letterId: string, letter return letter; } -export const patchLetterStatus = async (letterToUpdate: LetterDto, letterId: string, letterRepo: LetterRepository): Promise => { - - if (letterToUpdate.id !== letterId) { - throw new ValidationError(ApiErrorDetail.InvalidRequestLetterIdsMismatch); - } - - let updatedLetter; - - try { - updatedLetter = await letterRepo.updateLetterStatus(letterToUpdate); - } catch (error) { - if (isNotFoundError(error)) { - throw new NotFoundError(ApiErrorDetail.NotFoundLetterId); - } - throw error; - } - - return mapToPatchLetterResponse(updatedLetter); -} - export const getLetterDataUrl = async (supplierId: string, letterId: string, deps: Deps): Promise => { let letter; @@ -79,16 +58,16 @@ async function getDownloadUrl(s3Uri: string, s3Client: S3Client, expiry: number) return await getSignedUrl(s3Client, command, { expiresIn: expiry }); } -export async function enqueueLetterUpdateRequests(postLettersRequest: PostLettersRequest, supplierId: string, correlationId: string, deps: Deps) { +export async function enqueueLetterUpdateRequests(updateRequests: LetterDto[], correlationId: string, deps: Deps) { - const tasks = postLettersRequest.data.map(async (request) => { + const tasks = updateRequests.map(async (request: LetterDto) => { try { const command = new SendMessageCommand({ QueueUrl: deps.env.QUEUE_URL, MessageAttributes: { CorrelationId: { DataType: 'String', StringValue: correlationId }, }, - MessageBody: JSON.stringify(mapPostLetterResourceToDto(request, supplierId)), + MessageBody: JSON.stringify(request), }); await deps.sqsClient.send(command); } catch (err) { @@ -96,8 +75,8 @@ export async function enqueueLetterUpdateRequests(postLettersRequest: PostLetter err, correlationId: correlationId, letterId: request.id, - letterStatus: request.attributes.status, - supplierId: supplierId + letterStatus: request.status, + supplierId: request.supplierId }, 'Error enqueuing letter status update'); } });