From 8c32b934c2119787a75970b60e2a595b29785665 Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 15:23:55 +0000 Subject: [PATCH 1/6] CCM-12685: Validate routing config submit --- .../src/__tests__/utils/routing-utils.test.ts | 73 --- frontend/src/utils/routing-utils.ts | 13 +- .../module_submit_routing_config_lambda.tf | 14 + .../modules/backend-api/spec.tmpl.json | 2 - .../app/routing-config-client.test.ts | 36 +- .../repository.test.ts | 499 ++++++++++++++++-- .../src/app/routing-config-client.ts | 2 +- .../routing-config-repository/repository.ts | 167 +++++- .../__tests__/schemas/routing-config.test.ts | 158 +++++- .../src/schemas/routing-config.ts | 35 +- .../backend-client/src/schemas/template.ts | 4 +- .../src/types/generated/types.gen.ts | 4 +- .../get-routing-config.api.spec.ts | 6 +- .../submit-routing-config.api.spec.ts | 413 ++++++++++++++- .../routing-config.event.spec.ts | 16 +- 15 files changed, 1261 insertions(+), 181 deletions(-) diff --git a/frontend/src/__tests__/utils/routing-utils.test.ts b/frontend/src/__tests__/utils/routing-utils.test.ts index 84413decb..696ef20ea 100644 --- a/frontend/src/__tests__/utils/routing-utils.test.ts +++ b/frontend/src/__tests__/utils/routing-utils.test.ts @@ -176,27 +176,6 @@ describe('getSelectedLanguageTemplateIds', () => { ]); }); - it('should filter out language templates with null templateId', () => { - const cascadeItem: CascadeItem = { - cascadeGroups: ['translations'], - channel: 'LETTER', - channelType: 'primary', - defaultTemplateId: 'default-template', - conditionalTemplates: [ - { templateId: 'template-1', language: 'fr' }, - { templateId: null, language: 'pl' }, - { templateId: 'template-3', language: 'es' }, - ], - }; - - const result = getSelectedLanguageTemplateIds(cascadeItem); - - expect(result).toEqual([ - { language: 'fr', templateId: 'template-1' }, - { language: 'es', templateId: 'template-3' }, - ]); - }); - it('should return empty array when no conditional templates exist', () => { const cascadeItem: CascadeItem = { cascadeGroups: ['standard'], @@ -287,20 +266,6 @@ describe('removeTemplatesFromConditionalTemplates', () => { expect(result).toEqual([{ templateId: 'template-2', language: 'es' }]); }); - it('should keep templates with null templateId', () => { - const conditionalTemplates: ConditionalTemplate[] = [ - { templateId: 'template-1', language: 'fr' }, - { templateId: null, language: 'es' }, - ]; - - const result = removeTemplatesFromConditionalTemplates( - conditionalTemplates, - ['template-1'] - ); - - expect(result).toEqual([{ templateId: null, language: 'es' }]); - }); - it('should return empty array when all templates are removed', () => { const conditionalTemplates: ConditionalTemplate[] = [ { templateId: 'template-1', language: 'fr' }, @@ -512,26 +477,6 @@ describe('getConditionalTemplatesForItem', () => { }); }); - it('should filter out templates with a missing/invalid templateId', () => { - const cascadeItem: CascadeItem = { - cascadeGroups: ['standard', 'translations'], - channel: 'LETTER', - channelType: 'primary', - defaultTemplateId: 'template-1', - conditionalTemplates: [ - { templateId: 'template-2', language: 'fr' }, - { templateId: null, language: 'es' }, - { accessibleFormat: 'x1' } as ConditionalTemplate, - ], - }; - - const result = getConditionalTemplatesForItem(cascadeItem, templates); - - expect(result).toEqual({ - 'template-2': templates['template-2'], - }); - }); - it('should not include templates that are missing from templates object', () => { const cascadeItem: CascadeItem = { cascadeGroups: ['standard', 'translations'], @@ -635,24 +580,6 @@ describe('buildCascadeGroupsForItem', () => { ]); }); - it('should return only standard group when conditional templates have missing templateIds', () => { - const cascadeItem: CascadeItem = { - cascadeGroups: ['standard'], - channel: 'LETTER', - channelType: 'primary', - defaultTemplateId: 'template-1', - conditionalTemplates: [ - { templateId: null, accessibleFormat: 'q4' }, - { templateId: 'template-2', language: 'fr' }, - ], - }; - - expect(buildCascadeGroupsForItem(cascadeItem)).toEqual([ - 'standard', - 'translations', - ]); - }); - it('should return only standard when conditional templates array is empty', () => { const cascadeItem: CascadeItem = { cascadeGroups: ['standard'], diff --git a/frontend/src/utils/routing-utils.ts b/frontend/src/utils/routing-utils.ts index a3efb13ac..f21a0deca 100644 --- a/frontend/src/utils/routing-utils.ts +++ b/frontend/src/utils/routing-utils.ts @@ -51,7 +51,7 @@ export function getConditionalTemplatesForItem( return Object.fromEntries( conditionalTemplateIds - .filter((id): id is string => id != null && id in templates) + .filter((id) => id in templates) .map((id) => [id, templates[id]]) ); } @@ -90,10 +90,8 @@ export function getSelectedLanguageTemplateIds( return cascadeItem.conditionalTemplates .filter( - ( - template - ): template is ConditionalTemplateLanguage & { templateId: string } => - 'language' in template && template.templateId !== null + (template): template is ConditionalTemplateLanguage => + 'language' in template ) .map(({ language, templateId }) => ({ language, @@ -137,10 +135,10 @@ export function buildCascadeGroupsForItem( cascadeItem.conditionalTemplates.length > 0 ) { const hasAccessibleFormat = cascadeItem.conditionalTemplates.some( - (template) => 'accessibleFormat' in template && template.templateId + (template) => 'accessibleFormat' in template ); const hasLanguage = cascadeItem.conditionalTemplates.some( - (template) => 'language' in template && template.templateId + (template) => 'language' in template ); if (hasAccessibleFormat) { @@ -190,7 +188,6 @@ export function removeTemplatesFromCascadeItem( return updatedCascadeItem; } - /** * Add default template to cascade at specific index */ diff --git a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf index f3b5f1ec4..8f8dd9f99 100644 --- a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf +++ b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf @@ -42,6 +42,7 @@ data "aws_iam_policy_document" "submit_routing_config_lambda_policy" { actions = [ "dynamodb:UpdateItem", + "dynamodb:GetItem", ] resources = [ @@ -49,6 +50,19 @@ data "aws_iam_policy_document" "submit_routing_config_lambda_policy" { ] } + statement { + sid = "AllowConditionCheckDynamoAccess" + effect = "Allow" + + actions = [ + "dynamodb:ConditionCheckItem", + ] + + resources = [ + aws_dynamodb_table.templates.arn, + ] + } + statement { sid = "AllowKMSAccess" effect = "Allow" diff --git a/infrastructure/terraform/modules/backend-api/spec.tmpl.json b/infrastructure/terraform/modules/backend-api/spec.tmpl.json index 504c2b3ac..42d17a46c 100644 --- a/infrastructure/terraform/modules/backend-api/spec.tmpl.json +++ b/infrastructure/terraform/modules/backend-api/spec.tmpl.json @@ -385,7 +385,6 @@ }, "templateId": { "format": "uuid", - "nullable": true, "type": "string" } }, @@ -409,7 +408,6 @@ }, "templateId": { "format": "uuid", - "nullable": true, "type": "string" } }, diff --git a/lambdas/backend-api/src/__tests__/app/routing-config-client.test.ts b/lambdas/backend-api/src/__tests__/app/routing-config-client.test.ts index 141ee19bb..f11478a74 100644 --- a/lambdas/backend-api/src/__tests__/app/routing-config-client.test.ts +++ b/lambdas/backend-api/src/__tests__/app/routing-config-client.test.ts @@ -102,7 +102,10 @@ describe('RoutingConfigClient', () => { expect(result).toEqual({ error: { - errorMeta: { code: 404, description: 'Routing Config not found' }, + errorMeta: { + code: 404, + description: 'Routing configuration not found', + }, }, }); @@ -516,6 +519,7 @@ describe('RoutingConfigClient', () => { const completed: RoutingConfig = { ...routingConfig, + id, status: 'COMPLETED', }; @@ -536,6 +540,36 @@ describe('RoutingConfigClient', () => { }); }); + test('returns failures from repository', async () => { + const { client, mocks } = setup(); + + mocks.clientConfigRepository.get.mockResolvedValueOnce({ + data: { features: { routing: true } }, + }); + + mocks.routingConfigRepository.submit.mockResolvedValueOnce({ + error: { + errorMeta: { + code: 400, + description: + 'All cascade items must have either a defaultTemplateId or conditionalTemplates', + }, + }, + }); + + const result = await client.submitRoutingConfig('some-id', user, '42'); + + expect(result).toEqual({ + error: { + errorMeta: { + code: 400, + description: + 'All cascade items must have either a defaultTemplateId or conditionalTemplates', + }, + }, + }); + }); + test('returns failures from client config repository', async () => { const { client, mocks } = setup(); diff --git a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts index 43172a18f..58282091d 100644 --- a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts +++ b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts @@ -51,8 +51,10 @@ const TEMPLATE_TABLE_NAME = 'template-table-name'; const user = { internalUserId: 'user', clientId: 'nhs-notify-client-id' }; const clientOwnerKey = `CLIENT#${user.clientId}`; +const dynamo = mockClient(DynamoDBDocumentClient); + function setup() { - const dynamo = mockClient(DynamoDBDocumentClient); + dynamo.reset(); const mocks = { dynamo }; @@ -110,7 +112,10 @@ describe('RoutingConfigRepository', () => { expect(result).toEqual({ error: { - errorMeta: { code: 404, description: 'Routing Config not found' }, + errorMeta: { + code: 404, + description: 'Routing configuration not found', + }, }, }); @@ -252,51 +257,93 @@ describe('RoutingConfigRepository', () => { test('updates routing config to COMPLETED', async () => { const { repo, mocks } = setup(); + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + const completed: RoutingConfig = { ...routingConfig, status: 'COMPLETED', + lockNumber: 3, }; - mocks.dynamo.on(UpdateCommand).resolves({ Attributes: completed }); + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLock }) + .resolvesOnce({ Item: completed }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); const result = await repo.submit(routingConfig.id, user, 2); expect(result).toEqual({ data: completed }); - expect(mocks.dynamo).toHaveReceivedCommandWith(UpdateCommand, { - ConditionExpression: - '#status = :condition_1_status AND #lockNumber = :condition_2_lockNumber', - ExpressionAttributeNames: { - '#status': 'status', - '#updatedAt': 'updatedAt', - '#updatedBy': 'updatedBy', - '#lockNumber': 'lockNumber', - }, - ExpressionAttributeValues: { - ':condition_1_status': 'DRAFT', - ':condition_2_lockNumber': 2, - ':lockNumber': 1, - ':status': 'COMPLETED', - ':updatedAt': date.toISOString(), - ':updatedBy': `INTERNAL_USER#${user.internalUserId}`, - }, - Key: { - id: routingConfig.id, - owner: clientOwnerKey, + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { + TransactItems: expect.arrayContaining([ + { + Update: expect.objectContaining({ + ConditionExpression: + '#status = :condition_1_status AND #lockNumber = :condition_2_lockNumber', + ExpressionAttributeNames: { + '#status': 'status', + '#updatedAt': 'updatedAt', + '#updatedBy': 'updatedBy', + '#lockNumber': 'lockNumber', + }, + ExpressionAttributeValues: { + ':condition_1_status': 'DRAFT', + ':condition_2_lockNumber': 2, + ':lockNumber': 1, + ':status': 'COMPLETED', + ':updatedAt': date.toISOString(), + ':updatedBy': `INTERNAL_USER#${user.internalUserId}`, + }, + Key: { + id: routingConfig.id, + owner: clientOwnerKey, + }, + ReturnValues: 'ALL_NEW', + TableName: TABLE_NAME, + UpdateExpression: + 'SET #status = :status, #updatedAt = :updatedAt, #updatedBy = :updatedBy ADD #lockNumber :lockNumber', + }), + }, + ]), + }); + }); + + test('returns failure on client error during initial get', async () => { + const { repo, mocks } = setup(); + + const err = new Error('ddb err'); + + mocks.dynamo.on(GetCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 500, + description: 'Error retrieving Routing Config', + }, }, - ReturnValues: 'ALL_NEW', - TableName: TABLE_NAME, - UpdateExpression: - 'SET #status = :status, #updatedAt = :updatedAt, #updatedBy = :updatedBy ADD #lockNumber :lockNumber', }); }); - test('returns failure on client error', async () => { + test('returns failure on client error during transaction', async () => { const { repo, mocks } = setup(); const err = new Error('ddb err'); - mocks.dynamo.on(UpdateCommand).rejects(err); + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -314,13 +361,22 @@ describe('RoutingConfigRepository', () => { test('returns failure if updated template is invalid', async () => { const { repo, mocks } = setup(); + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + const completedInvalid: RoutingConfig = { ...routingConfig, status: 'COMPLETED', name: 0 as unknown as string, }; - mocks.dynamo.on(UpdateCommand).resolves({ Attributes: completedInvalid }); + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLock }) + .resolvesOnce({ Item: completedInvalid }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); const result = await repo.submit(routingConfig.id, user, 2); @@ -347,12 +403,36 @@ describe('RoutingConfigRepository', () => { test('returns 404 failure if routing config does not exist', async () => { const { repo, mocks } = setup(); - const err = new ConditionalCheckFailedException({ + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: undefined }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + errorMeta: { + code: 404, + description: 'Routing configuration not found', + }, + }, + }); + }); + + test('returns 404 failure if transaction fails because routing config does not exist', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + const err = new TransactionCanceledException({ $metadata: {}, message: 'msg', + CancellationReasons: [{ Code: 'ConditionalCheckFailed' }], }); - mocks.dynamo.on(UpdateCommand).rejects(err); + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -369,13 +449,12 @@ describe('RoutingConfigRepository', () => { test('returns 404 failure if routing config is DELETED', async () => { const { repo, mocks } = setup(); - const err = new ConditionalCheckFailedException({ - Item: { status: { S: 'DELETED' satisfies RoutingConfigStatus } }, - $metadata: {}, - message: 'msg', - }); + const deletedRoutingConfig: RoutingConfig = { + ...routingConfig, + status: 'DELETED', + }; - mocks.dynamo.on(UpdateCommand).rejects(err); + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: deletedRoutingConfig }); const result = await repo.submit(routingConfig.id, user, 2); @@ -387,18 +466,21 @@ describe('RoutingConfigRepository', () => { }, }, }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); test('returns 400 failure if routing config is COMPLETED', async () => { const { repo, mocks } = setup(); - const err = new ConditionalCheckFailedException({ - Item: { status: { S: 'COMPLETED' satisfies RoutingConfigStatus } }, - $metadata: {}, - message: 'msg', - }); + const completedRoutingConfig: RoutingConfig = { + ...routingConfig, + status: 'COMPLETED', + }; - mocks.dynamo.on(UpdateCommand).rejects(err); + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: completedRoutingConfig }); const result = await repo.submit(routingConfig.id, user, 2); @@ -411,21 +493,134 @@ describe('RoutingConfigRepository', () => { }, }, }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); test("returns 409 failure if lock number doesn't match", async () => { const { repo, mocks } = setup(); - const err = new ConditionalCheckFailedException({ - Item: { - status: { S: 'DRAFT' satisfies RoutingConfigStatus }, - lockNumber: { N: '3' }, + const mismatchedLockRoutingConfig: RoutingConfig = { + ...routingConfig, + lockNumber: 3, + }; + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: mismatchedLockRoutingConfig }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + errorMeta: { + code: 409, + description: + 'Lock number mismatch - Message Plan has been modified since last read', + }, + }, + }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); + }); + + test('returns validation failure if cascade item has no defaultTemplateId or conditionalTemplates', async () => { + const { repo, mocks } = setup(); + + const invalidRoutingConfig: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + conditionalTemplates: [], + }, + ], + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: invalidRoutingConfig }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: expect.any(Error), + errorMeta: { + code: 400, + description: + 'Routing config is not ready for submission: all cascade items must have templates assigned', + }, }, + }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); + }); + + test('returns validation failure if defaultTemplateId is null with no conditionalTemplates', async () => { + const { repo, mocks } = setup(); + + const invalidRoutingConfig: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: null, + }, + ], + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: invalidRoutingConfig }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: expect.any(Error), + errorMeta: { + code: 400, + description: + 'Routing config is not ready for submission: all cascade items must have templates assigned', + }, + }, + }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); + }); + + test('returns 400 failure if template not found during submit', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithTemplate: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: 'missing-template-id', + }, + ], + }; + + const err = new TransactionCanceledException({ $metadata: {}, message: 'msg', + CancellationReasons: [ + { Code: 'None' }, // Update succeeded + { Code: 'ConditionalCheckFailed', Item: undefined }, // Template not found (no Item returned) + ], }); - mocks.dynamo.on(UpdateCommand).rejects(err); + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithTemplate }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -433,9 +628,211 @@ describe('RoutingConfigRepository', () => { error: { actualError: err, errorMeta: { - code: 409, + code: 400, + description: 'Some templates not found', + details: { templateIds: 'missing-template-id' }, + }, + }, + }); + }); + + test('returns 400 failure if template has DELETED status', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithTemplate: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: 'deleted-template-id', + }, + ], + }; + + const err = new TransactionCanceledException({ + $metadata: {}, + message: 'msg', + CancellationReasons: [ + { Code: 'None' }, // Update succeeded + { + Code: 'ConditionalCheckFailed', + Item: { + id: { S: 'deleted-template-id' }, + templateType: { S: 'SMS' }, + templateStatus: { S: 'DELETED' }, + }, + }, + ], + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithTemplate }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 400, + description: 'Some templates not found', + details: { templateIds: 'deleted-template-id' }, + }, + }, + }); + }); + + test('returns 400 failure if LETTER template has invalid status', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLetter: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: 'letter-template-id', + }, + ], + }; + + const err = new TransactionCanceledException({ + $metadata: {}, + message: 'msg', + CancellationReasons: [ + { Code: 'None' }, + { + Code: 'ConditionalCheckFailed', + Item: { + id: { S: 'letter-template-id' }, + templateType: { S: 'LETTER' }, + templateStatus: { S: 'DRAFT' }, + }, + }, + ], + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLetter }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 400, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Letter templates must have status PROOF_APPROVED or SUBMITTED', + details: { templateIds: 'letter-template-id' }, + }, + }, + }); + + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { + TransactItems: expect.arrayContaining([ + { + ConditionCheck: { + TableName: TEMPLATE_TABLE_NAME, + Key: { + id: 'letter-template-id', + owner: clientOwnerKey, + }, + ConditionExpression: + 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', + ExpressionAttributeValues: { + ':deleted': 'DELETED', + ':letterType': 'LETTER', + ':proofApproved': 'PROOF_APPROVED', + ':submitted': 'SUBMITTED', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, + }, + }, + ]), + }); + }); + + test('returns 500 failure on unexpected TransactionCanceledException', async () => { + const { repo, mocks } = setup(); + + // Transaction cancelled with update succeeded but no template failures + // This is an edge case that shouldn't normally happen + const err = new TransactionCanceledException({ + $metadata: {}, + message: 'Unexpected cancellation', + CancellationReasons: [ + { Code: 'None' }, // Update succeeded + { Code: 'None' }, // Template check also passed (unexpected) + ], + }); + + const routingConfigWithTemplate: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: 'some-template-id', + }, + ], + }; + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithTemplate }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 500, + description: 'Failed to update routing config', + }, + }, + }); + }); + + test('returns 500 failure on TransactionCanceledException with undefined CancellationReasons', async () => { + const { repo, mocks } = setup(); + + const err = new TransactionCanceledException({ + $metadata: {}, + message: 'Transaction cancelled with no reasons', + CancellationReasons: undefined, + }); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 500, + description: 'Failed to update routing config', }, }, }); diff --git a/lambdas/backend-api/src/app/routing-config-client.ts b/lambdas/backend-api/src/app/routing-config-client.ts index 7e220e1ac..32b46dacb 100644 --- a/lambdas/backend-api/src/app/routing-config-client.ts +++ b/lambdas/backend-api/src/app/routing-config-client.ts @@ -189,7 +189,7 @@ export class RoutingConfigClient { const result = await this.routingConfigRepository.get(id, user.clientId); if (result.data?.status === 'DELETED') { - return failure(ErrorCase.NOT_FOUND, 'Routing Config not found'); + return failure(ErrorCase.NOT_FOUND, 'Routing configuration not found'); } return result; diff --git a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts index 5d34a209a..905bfa03a 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -13,6 +13,7 @@ import { } from '@backend-api/utils/result'; import { $RoutingConfig, + $SubmittableCascade, CascadeItem, type CreateRoutingConfig, ErrorCase, @@ -26,6 +27,7 @@ import { RoutingConfigQuery } from './query'; import { RoutingConfigUpdateBuilder } from 'nhs-notify-entity-update-command-builder'; import { ConditionalCheckFailedException, + ReturnValuesOnConditionCheckFailure, TransactionCanceledException, } from '@aws-sdk/client-dynamodb'; import { calculateTTL } from '@backend-api/utils/calculate-ttl'; @@ -168,7 +170,47 @@ export class RoutingConfigRepository { user: User, lockNumber: number ): Promise> { - const cmdInput = new RoutingConfigUpdateBuilder( + const existingConfig = await this.get(id, user.clientId); + + if (existingConfig.error) { + return existingConfig; + } + + const { + status, + cascade, + lockNumber: currentLockNumber, + } = existingConfig.data; + + // Check status before cascade validation + if (status === 'DELETED') { + return failure(ErrorCase.NOT_FOUND, 'Routing configuration not found'); + } + + if (status === 'COMPLETED') { + return failure( + ErrorCase.ALREADY_SUBMITTED, + 'Routing configuration with status COMPLETED cannot be updated' + ); + } + + // Check lock number before cascade validation + if (currentLockNumber !== lockNumber) { + return failure( + ErrorCase.CONFLICT, + 'Lock number mismatch - Message Plan has been modified since last read' + ); + } + + const submittableValidationError = + this.validateRoutingConfigIsSubmittable(cascade); + if (submittableValidationError) { + return submittableValidationError; + } + + const templateIds = this.extractTemplateIds(cascade); + + const update = new RoutingConfigUpdateBuilder( this.tableName, user.clientId, id, @@ -182,9 +224,45 @@ export class RoutingConfigRepository { .build(); try { - const result = await this.client.send(new UpdateCommand(cmdInput)); + await this.client.send( + new TransactWriteCommand({ + TransactItems: [ + { + Update: update, + }, + // Template existence check + For LETTER templates, check they have PROOF_APPROVED or SUBMITTED status + // Also exclude DELETED templates for all template types + ...templateIds.map((templateId) => ({ + ConditionCheck: { + TableName: this.templateTableName, + Key: { + id: templateId, + owner: this.clientOwnerKey(user.clientId), + }, + ConditionExpression: + 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', + ExpressionAttributeValues: { + ':deleted': 'DELETED', + ':letterType': 'LETTER', + ':proofApproved': 'PROOF_APPROVED', + ':submitted': 'SUBMITTED', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, + }, + })), + ], + }) + ); - const parsed = $RoutingConfig.safeParse(result.Attributes); + const getResult = await this.client.send( + new GetCommand({ + TableName: this.tableName, + Key: { id, owner: this.clientOwnerKey(user.clientId) }, + }) + ); + + const parsed = $RoutingConfig.safeParse(getResult.Item); if (!parsed.success) { return failure( @@ -196,7 +274,7 @@ export class RoutingConfigRepository { return success(parsed.data); } catch (error) { - return this.handleUpdateError(error, lockNumber); + return this.handleSubmitTransactionError(error, lockNumber, templateIds); } } @@ -254,7 +332,7 @@ export class RoutingConfigRepository { ); if (!result.Item) { - return failure(ErrorCase.NOT_FOUND, 'Routing Config not found'); + return failure(ErrorCase.NOT_FOUND, 'Routing configuration not found'); } const parsed = $RoutingConfig.parse(result.Item); @@ -401,4 +479,83 @@ export class RoutingConfigRepository { ]) .filter((id): id is string => id != null); } + + private validateRoutingConfigIsSubmittable(cascade: CascadeItem[]) { + const result = $SubmittableCascade.safeParse(cascade); + + if (!result.success) { + return failure( + ErrorCase.VALIDATION_FAILED, + 'Routing config is not ready for submission: all cascade items must have templates assigned', + result.error + ); + } + + return null; + } + + private handleSubmitTransactionError( + err: unknown, + lockNumber: number, + templateIds: string[] + ): ApplicationResult { + if (!(err instanceof TransactionCanceledException)) { + return this.handleUpdateError(err, lockNumber); + } + + // Note: The first item will always be the update + // https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_CancellationReason.html + const [updateReason, ...templateReasons] = err.CancellationReasons ?? []; + + if (updateReason && updateReason.Code !== 'None') { + return this.handleUpdateError( + new ConditionalCheckFailedException({ + message: updateReason.Message!, + Item: updateReason.Item, + $metadata: err.$metadata, + }), + lockNumber + ); + } + + // Find which templates failed the condition check + const missingTemplateIds: string[] = []; + const invalidLetterTemplateIds: string[] = []; + + for (const [index, reason] of templateReasons.entries()) { + if (reason.Code === 'ConditionalCheckFailed') { + const templateId = templateIds[index]; + + if ( + !reason.Item || + reason.Item.templateStatus?.S === + ('DELETED' satisfies RoutingConfigStatus) + ) { + missingTemplateIds.push(templateId); + } else { + invalidLetterTemplateIds.push(templateId); + } + } + } + + if (missingTemplateIds.length > 0) { + return failure( + ErrorCase.ROUTING_CONFIG_TEMPLATES_NOT_FOUND, + 'Some templates not found', + err, + { templateIds: missingTemplateIds.join(',') } + ); + } + + if (invalidLetterTemplateIds.length > 0) { + return failure( + ErrorCase.VALIDATION_FAILED, + 'Letter templates must have status PROOF_APPROVED or SUBMITTED', + err, + { templateIds: invalidLetterTemplateIds.join(',') } + ); + } + + return this.handleUpdateError(err, lockNumber); + } } diff --git a/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts b/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts index 575ae856b..085cb3337 100644 --- a/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts +++ b/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts @@ -3,6 +3,8 @@ import { $ListRoutingConfigFilters, $RoutingConfig, $UpdateRoutingConfig, + $SubmittableCascade, + $SubmittableCascadeItem, } from '../../schemas/routing-config'; const cascadeItemDefault = { @@ -90,33 +92,6 @@ describe.each([ expect(res.success).toBe(true); }); - test('valid with conditional template ids set to null', () => { - const res = $Schema.safeParse({ - ...baseInput, - cascade: [ - { - ...cascadeCondAcc, - conditionalTemplates: [ - { - accessibleFormat: 'x1', - templateId: null, - }, - ], - }, - { - ...cascadeCondLang, - conditionalTemplates: [ - { - language: 'ar', - templateId: null, - }, - ], - }, - ], - }); - expect(res.success).toBe(true); - }); - test('snapshot full error', () => { const res = $Schema.safeParse({}); @@ -416,3 +391,132 @@ describe('UpdateRoutingConfig', () => { expect(res.error).toMatchSnapshot(); }); }); + +describe('$SubmittableCascadeItem', () => { + test('valid with defaultTemplateId', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09', + }); + expect(res.success).toBe(true); + }); + + test('valid with defaultTemplateId and conditionalTemplates', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard', 'translations'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09', + conditionalTemplates: [ + { language: 'ar', templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09' }, + ], + }); + expect(res.success).toBe(true); + }); + + test('valid with conditionalTemplates only (non-empty)', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['translations'], + channel: 'LETTER', + channelType: 'secondary', + conditionalTemplates: [ + { language: 'ar', templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09' }, + ], + }); + expect(res.success).toBe(true); + }); + + test('valid with accessible conditional templates only', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['accessible'], + channel: 'LETTER', + channelType: 'secondary', + conditionalTemplates: [ + { + accessibleFormat: 'x1', + templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09', + }, + ], + }); + expect(res.success).toBe(true); + }); + + test('invalid without defaultTemplateId and without conditionalTemplates', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + }); + expect(res.success).toBe(false); + }); + + test('invalid with empty conditionalTemplates and no defaultTemplateId', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + conditionalTemplates: [], + }); + expect(res.success).toBe(false); + }); + + test('invalid with empty defaultTemplateId', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: '', + }); + expect(res.success).toBe(false); + }); +}); + +describe('$SubmittableCascade', () => { + const validCascadeItem = { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09', + }; + + test('valid with single item', () => { + const res = $SubmittableCascade.safeParse([validCascadeItem]); + expect(res.success).toBe(true); + }); + + test('valid with multiple items', () => { + const res = $SubmittableCascade.safeParse([ + validCascadeItem, + { + cascadeGroups: ['translations'], + channel: 'EMAIL', + channelType: 'secondary', + conditionalTemplates: [ + { + language: 'ar', + templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09', + }, + ], + }, + ]); + expect(res.success).toBe(true); + }); + + test('invalid with empty array', () => { + const res = $SubmittableCascade.safeParse([]); + expect(res.success).toBe(false); + }); + + test('invalid with invalid cascade item in array', () => { + const res = $SubmittableCascade.safeParse([ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + }, + ]); + expect(res.success).toBe(false); + }); +}); diff --git a/lambdas/backend-client/src/schemas/routing-config.ts b/lambdas/backend-client/src/schemas/routing-config.ts index 450418e99..a40826e7d 100644 --- a/lambdas/backend-client/src/schemas/routing-config.ts +++ b/lambdas/backend-client/src/schemas/routing-config.ts @@ -58,14 +58,14 @@ const $CascadeGroup = schemaFor()( ]) ); -export const $Channel = schemaFor()(z.enum(CHANNEL_LIST)); +const $Channel = schemaFor()(z.enum(CHANNEL_LIST)); const $ChannelType = schemaFor()(z.enum(CHANNEL_TYPE_LIST)); const $ConditionalTemplateLanguage = schemaFor()( z.object({ language: $Language, - templateId: z.string().nonempty().nullable(), + templateId: z.string().nonempty(), supplierReferences: z.record(z.string(), z.string()).optional(), }) ); @@ -74,7 +74,7 @@ const $ConditionalTemplateAccessible = schemaFor()( z.object({ accessibleFormat: $LetterType, - templateId: z.string().nonempty().nullable(), + templateId: z.string().nonempty(), supplierReferences: z.record(z.string(), z.string()).optional(), }) ); @@ -115,6 +115,35 @@ const $CascadeItem = schemaFor()( ) ); +export const $SubmittableCascadeItem = $CascadeItemBase.and( + z.union([ + z.object({ + defaultTemplateId: z.string().nonempty(), + conditionalTemplates: z + .array( + z.union([ + $ConditionalTemplateAccessible, + $ConditionalTemplateLanguage, + ]) + ) + .optional(), + }), + z.object({ + defaultTemplateId: z.string().nonempty().optional(), + conditionalTemplates: z + .array( + z.union([ + $ConditionalTemplateAccessible, + $ConditionalTemplateLanguage, + ]) + ) + .nonempty(), + }), + ]) +); + +export const $SubmittableCascade = z.array($SubmittableCascadeItem).nonempty(); + export const $CreateRoutingConfig = schemaFor()( z.object({ campaignId: z.string(), diff --git a/lambdas/backend-client/src/schemas/template.ts b/lambdas/backend-client/src/schemas/template.ts index e58f4d378..dee1ac4bb 100644 --- a/lambdas/backend-client/src/schemas/template.ts +++ b/lambdas/backend-client/src/schemas/template.ts @@ -84,7 +84,7 @@ export const $SmsProperties = schemaFor()( }) ); -export const $BaseLetterTemplateProperties = z.object({ +const $BaseLetterTemplateProperties = z.object({ templateType: z.literal('LETTER'), letterType: z.enum(LETTER_TYPE_LIST), language: z.enum(LANGUAGE_LIST), @@ -134,7 +134,7 @@ export const $LetterProperties = z.discriminatedUnion('letterVersion', [ $AuthoringLetterProperties, ]); -export const $BaseTemplateSchema = schemaFor()( +const $BaseTemplateSchema = schemaFor()( z.object({ name: z.string().trim().min(1), templateType: z.enum(TEMPLATE_TYPE_LIST), diff --git a/lambdas/backend-client/src/types/generated/types.gen.ts b/lambdas/backend-client/src/types/generated/types.gen.ts index 1006e804e..ebf392677 100644 --- a/lambdas/backend-client/src/types/generated/types.gen.ts +++ b/lambdas/backend-client/src/types/generated/types.gen.ts @@ -107,7 +107,7 @@ export type ConditionalTemplateAccessible = { supplierReferences?: { [key: string]: string; }; - templateId: string | null; + templateId: string; }; export type ConditionalTemplateLanguage = { @@ -115,7 +115,7 @@ export type ConditionalTemplateLanguage = { supplierReferences?: { [key: string]: string; }; - templateId: string | null; + templateId: string; }; export type CountSuccess = { diff --git a/tests/test-team/template-mgmt-api-tests/get-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/get-routing-config.api.spec.ts index 3c8224ec3..3c2e0e2cc 100644 --- a/tests/test-team/template-mgmt-api-tests/get-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/get-routing-config.api.spec.ts @@ -83,7 +83,7 @@ test.describe('GET /v1/routing-configuration/:routingConfigId', () => { expect(response.status()).toBe(404); expect(await response.json()).toEqual({ statusCode: 404, - technicalMessage: 'Routing Config not found', + technicalMessage: 'Routing configuration not found', }); }); @@ -104,7 +104,7 @@ test.describe('GET /v1/routing-configuration/:routingConfigId', () => { expect(response.status()).toBe(404); expect(await response.json()).toEqual({ statusCode: 404, - technicalMessage: 'Routing Config not found', + technicalMessage: 'Routing configuration not found', }); }); @@ -125,7 +125,7 @@ test.describe('GET /v1/routing-configuration/:routingConfigId', () => { expect(response.status()).toBe(404); expect(await response.json()).toEqual({ statusCode: 404, - technicalMessage: 'Routing Config not found', + technicalMessage: 'Routing configuration not found', }); }); diff --git a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts index ae68c2c73..3896523b9 100644 --- a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '@playwright/test'; +import { randomUUID } from 'node:crypto'; import { createAuthHelper, type TestUser, @@ -6,12 +7,15 @@ import { } from '../helpers/auth/cognito-auth-helper'; import { isoDateRegExp } from 'nhs-notify-web-template-management-test-helper-utils'; import { RoutingConfigStorageHelper } from '../helpers/db/routing-config-storage-helper'; +import { TemplateStorageHelper } from '../helpers/db/template-storage-helper'; import { RoutingConfigFactory } from '../helpers/factories/routing-config-factory'; +import { TemplateFactory } from '../helpers/factories/template-factory'; import { RoutingConfigStatus } from 'nhs-notify-backend-client'; test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { const authHelper = createAuthHelper(); const storageHelper = new RoutingConfigStorageHelper(); + const templateStorageHelper = new TemplateStorageHelper(); let user1: TestUser; let userDifferentClient: TestUser; let userSharedClient: TestUser; @@ -28,6 +32,7 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { test.afterAll(async () => { await storageHelper.deleteSeeded(); + await templateStorageHelper.deleteSeededTemplates(); }); test('returns 401 if no auth token', async ({ request }) => { @@ -97,7 +102,27 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { test('returns 200 and the updated routing config data', async ({ request, }) => { - const { dbEntry, apiResponse } = RoutingConfigFactory.create(user1); + const templateId = randomUUID(); + + // Create a template for the routing config to reference + const template = TemplateFactory.createNhsAppTemplate( + templateId, + user1, + 'Test Template for Submit' + ); + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry, apiResponse } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); await storageHelper.seed([dbEntry]); @@ -193,7 +218,27 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { test('user belonging to the same client as the creator can submit', async ({ request, }) => { - const { dbEntry, apiResponse } = RoutingConfigFactory.create(user1); + const templateId = randomUUID(); + + // Create a template for the routing config to reference + const template = TemplateFactory.createNhsAppTemplate( + templateId, + user1, + 'Test Template for Shared Client Submit' + ); + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry, apiResponse } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); await storageHelper.seed([dbEntry]); @@ -298,4 +343,368 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { 'Lock number mismatch - Message Plan has been modified since last read', }); }); + + test.describe('cascade validation', () => { + test('returns 400 if cascade item has no defaultTemplateId and no conditionalTemplates', async ({ + request, + }) => { + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: null, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(400); + + expect(await response.json()).toEqual({ + statusCode: 400, + technicalMessage: + 'Routing config is not ready for submission: all cascade items must have templates assigned', + }); + }); + + test('returns 400 if cascade item has empty conditionalTemplates array', async ({ + request, + }) => { + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + conditionalTemplates: [], + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(400); + + expect(await response.json()).toEqual({ + statusCode: 400, + technicalMessage: + 'Routing config is not ready for submission: all cascade items must have templates assigned', + }); + }); + + test('returns 200 if cascade item has conditionalTemplates instead of defaultTemplateId', async ({ + request, + }) => { + const frenchTemplateId = randomUUID(); + const arabicTemplateId = randomUUID(); + + // Create templates for conditional templates + const frenchTemplate = TemplateFactory.uploadLetterTemplate( + frenchTemplateId, + user1, + 'French Letter Template', + 'PROOF_APPROVED', + 'PASSED', + { language: 'fr' } + ); + + const arabicTemplate = TemplateFactory.uploadLetterTemplate( + arabicTemplateId, + user1, + 'Arabic Letter Template', + 'PROOF_APPROVED', + 'PASSED', + { language: 'ar' } + ); + + await templateStorageHelper.seedTemplateData([ + frenchTemplate, + arabicTemplate, + ]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['translations'], + channel: 'LETTER', + channelType: 'primary', + // No defaultTemplateId - using conditionalTemplates instead + conditionalTemplates: [ + { language: 'fr', templateId: frenchTemplateId }, + { language: 'ar', templateId: arabicTemplateId }, + ], + }, + ], + cascadeGroupOverrides: [ + { name: 'translations', language: ['fr', 'ar'] }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + const responseBody = await response.json(); + expect(responseBody.data.status).toBe('COMPLETED'); + }); + }); + + test('returns 400 if referenced template does not exist', async ({ + request, + }) => { + const nonExistentTemplateId = 'non-existent-template-id'; + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: nonExistentTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(400); + + expect(await response.json()).toEqual({ + statusCode: 400, + technicalMessage: 'Some templates not found', + details: { + templateIds: nonExistentTemplateId, + }, + }); + }); + + test('returns 400 if referenced template has DELETED status', async ({ + request, + }) => { + const deletedTemplateId = randomUUID(); + + // Create a template with DELETED status + const deletedTemplate = TemplateFactory.createNhsAppTemplate( + deletedTemplateId, + user1, + 'Deleted Template' + ); + deletedTemplate.templateStatus = 'DELETED'; + + await templateStorageHelper.seedTemplateData([deletedTemplate]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: deletedTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(400); + + expect(await response.json()).toEqual({ + statusCode: 400, + technicalMessage: 'Some templates not found', + details: { + templateIds: deletedTemplateId, + }, + }); + }); + + test('returns 400 if LETTER template has status NOT_YET_SUBMITTED', async ({ + request, + }) => { + const letterTemplateId = randomUUID(); + + // Create a LETTER template with NOT_YET_SUBMITTED status + const letterTemplate = TemplateFactory.uploadLetterTemplate( + letterTemplateId, + user1, + 'Test Letter Template', + 'NOT_YET_SUBMITTED' + ); + + await templateStorageHelper.seedTemplateData([letterTemplate]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: letterTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(400); + + expect(await response.json()).toEqual({ + statusCode: 400, + technicalMessage: + 'Letter templates must have status PROOF_APPROVED or SUBMITTED', + details: { + templateIds: letterTemplateId, + }, + }); + }); + + test('returns 200 if LETTER template has status PROOF_APPROVED', async ({ + request, + }) => { + const letterTemplateId = randomUUID(); + + // Create a LETTER template with PROOF_APPROVED status + const letterTemplate = TemplateFactory.uploadLetterTemplate( + letterTemplateId, + user1, + 'Test Letter Template', + 'PROOF_APPROVED' + ); + + await templateStorageHelper.seedTemplateData([letterTemplate]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: letterTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + const responseBody = await response.json(); + expect(responseBody.data.status).toBe('COMPLETED'); + }); + + test('returns 200 if LETTER template has status SUBMITTED', async ({ + request, + }) => { + const letterTemplateId = randomUUID(); + + // Create a LETTER template with SUBMITTED status + const letterTemplate = TemplateFactory.uploadLetterTemplate( + letterTemplateId, + user1, + 'Test Letter Template', + 'SUBMITTED' + ); + + await templateStorageHelper.seedTemplateData([letterTemplate]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: letterTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + const responseBody = await response.json(); + expect(responseBody.data.status).toBe('COMPLETED'); + }); }); diff --git a/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts b/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts index 8bbbffaef..8bb1f20a3 100644 --- a/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts +++ b/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { test, expect } from '@playwright/test'; import { createAuthHelper, @@ -7,10 +8,13 @@ import { import { EventCacheHelper } from '../helpers/events/event-cache-helper'; import { RoutingConfigStorageHelper } from 'helpers/db/routing-config-storage-helper'; import { RoutingConfigFactory } from 'helpers/factories/routing-config-factory'; +import { TemplateFactory } from 'helpers/factories/template-factory'; +import { TemplateStorageHelper } from 'helpers/db/template-storage-helper'; test.describe('Event publishing - Routing Config', () => { const authHelper = createAuthHelper(); const storageHelper = new RoutingConfigStorageHelper(); + const templateStorageHelper = new TemplateStorageHelper(); const eventCacheHelper = new EventCacheHelper(); let user: TestUser; @@ -170,13 +174,23 @@ test.describe('Event publishing - Routing Config', () => { }); test('Expect a draft event and a completed event', async ({ request }) => { + const templateId = randomUUID(); + + const template = TemplateFactory.createNhsAppTemplate( + templateId, + user, + 'Test Template for Submit' + ); + + await templateStorageHelper.seedTemplateData([template]); + const payload = RoutingConfigFactory.create(user, { cascade: [ { cascadeGroups: ['standard'], channel: 'NHSAPP', channelType: 'primary', - defaultTemplateId: 'b1854a33-fc1b-4e7d-99d0-6f7b92b8c530', + defaultTemplateId: templateId, }, ], }).apiPayload; From 2e1c34f947de45e8acb0971c0ef3ff0843598a0d Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 15:23:55 +0000 Subject: [PATCH 2/6] Review comments --- .../module_submit_routing_config_lambda.tf | 2 +- .../repository.test.ts | 31 +++++++++++++++---- .../routing-config-repository/repository.ts | 6 ++-- .../routing-config.event.spec.ts | 1 + 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf index 8f8dd9f99..0e5613fd6 100644 --- a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf +++ b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf @@ -41,8 +41,8 @@ data "aws_iam_policy_document" "submit_routing_config_lambda_policy" { effect = "Allow" actions = [ - "dynamodb:UpdateItem", "dynamodb:GetItem", + "dynamodb:UpdateItem", ] resources = [ diff --git a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts index 58282091d..5917ba6f0 100644 --- a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts +++ b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts @@ -279,16 +279,16 @@ describe('RoutingConfigRepository', () => { expect(result).toEqual({ data: completed }); expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { - TransactItems: expect.arrayContaining([ + TransactItems: [ { - Update: expect.objectContaining({ + Update: { ConditionExpression: '#status = :condition_1_status AND #lockNumber = :condition_2_lockNumber', ExpressionAttributeNames: { + '#lockNumber': 'lockNumber', '#status': 'status', '#updatedAt': 'updatedAt', '#updatedBy': 'updatedBy', - '#lockNumber': 'lockNumber', }, ExpressionAttributeValues: { ':condition_1_status': 'DRAFT', @@ -303,12 +303,31 @@ describe('RoutingConfigRepository', () => { owner: clientOwnerKey, }, ReturnValues: 'ALL_NEW', - TableName: TABLE_NAME, + ReturnValuesOnConditionCheckFailure: 'ALL_OLD', + TableName: 'routing-config-table-name', UpdateExpression: 'SET #status = :status, #updatedAt = :updatedAt, #updatedBy = :updatedBy ADD #lockNumber :lockNumber', - }), + }, }, - ]), + { + ConditionCheck: { + ConditionExpression: + 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', + ExpressionAttributeValues: { + ':deleted': 'DELETED', + ':letterType': 'LETTER', + ':proofApproved': 'PROOF_APPROVED', + ':submitted': 'SUBMITTED', + }, + Key: { + id: routingConfig.cascade[0].defaultTemplateId!, + owner: clientOwnerKey, + }, + ReturnValuesOnConditionCheckFailure: 'ALL_OLD', + TableName: 'template-table-name', + }, + }, + ], }); }); diff --git a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts index 905bfa03a..c557e8173 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -203,7 +203,7 @@ export class RoutingConfigRepository { } const submittableValidationError = - this.validateRoutingConfigIsSubmittable(cascade); + this.parseSubmittableRoutingConfig(cascade); if (submittableValidationError) { return submittableValidationError; } @@ -480,7 +480,7 @@ export class RoutingConfigRepository { .filter((id): id is string => id != null); } - private validateRoutingConfigIsSubmittable(cascade: CascadeItem[]) { + private parseSubmittableRoutingConfig(cascade: CascadeItem[]) { const result = $SubmittableCascade.safeParse(cascade); if (!result.success) { @@ -490,8 +490,6 @@ export class RoutingConfigRepository { result.error ); } - - return null; } private handleSubmitTransactionError( diff --git a/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts b/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts index 8bb1f20a3..75c13f23e 100644 --- a/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts +++ b/tests/test-team/template-mgmt-event-tests/routing-config.event.spec.ts @@ -25,6 +25,7 @@ test.describe('Event publishing - Routing Config', () => { test.afterAll(async () => { await storageHelper.deleteSeeded(); + await templateStorageHelper.deleteSeededTemplates(); }); test('Expect a draft event and a deleted event when some template IDs are null', async ({ From acca8d53de6e90f65224a1f60534dd2897e73578 Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 15:23:55 +0000 Subject: [PATCH 3/6] Code review --- .../api/delete-routing-config.test.ts | 4 +-- .../api/submit-routing-config.test.ts | 4 +-- .../api/update-routing-config.test.ts | 4 +-- .../repository.test.ts | 16 +++++----- .../routing-config-repository/repository.ts | 32 +++++++------------ .../__tests__/schemas/routing-config.test.ts | 18 ++++++++++- .../delete-routing-config.api.spec.ts | 2 +- .../submit-routing-config.api.spec.ts | 2 +- .../update-routing-config.api.spec.ts | 2 +- 9 files changed, 46 insertions(+), 38 deletions(-) diff --git a/lambdas/backend-api/src/__tests__/api/delete-routing-config.test.ts b/lambdas/backend-api/src/__tests__/api/delete-routing-config.test.ts index 43141ba18..aeb3b8e42 100644 --- a/lambdas/backend-api/src/__tests__/api/delete-routing-config.test.ts +++ b/lambdas/backend-api/src/__tests__/api/delete-routing-config.test.ts @@ -169,7 +169,7 @@ describe('Delete Routing Config Handler', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); @@ -192,7 +192,7 @@ describe('Delete Routing Config Handler', () => { body: JSON.stringify({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }), }); diff --git a/lambdas/backend-api/src/__tests__/api/submit-routing-config.test.ts b/lambdas/backend-api/src/__tests__/api/submit-routing-config.test.ts index 681255539..48d9abea6 100644 --- a/lambdas/backend-api/src/__tests__/api/submit-routing-config.test.ts +++ b/lambdas/backend-api/src/__tests__/api/submit-routing-config.test.ts @@ -173,7 +173,7 @@ describe('Submit Routing Config Handler', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); @@ -196,7 +196,7 @@ describe('Submit Routing Config Handler', () => { body: JSON.stringify({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }), }); diff --git a/lambdas/backend-api/src/__tests__/api/update-routing-config.test.ts b/lambdas/backend-api/src/__tests__/api/update-routing-config.test.ts index 99a38d6c8..ff4c44014 100644 --- a/lambdas/backend-api/src/__tests__/api/update-routing-config.test.ts +++ b/lambdas/backend-api/src/__tests__/api/update-routing-config.test.ts @@ -255,7 +255,7 @@ describe('Update Routing Config Handler', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); @@ -283,7 +283,7 @@ describe('Update Routing Config Handler', () => { body: JSON.stringify({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }), }); diff --git a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts index 5917ba6f0..0bc78b049 100644 --- a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts +++ b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts @@ -345,7 +345,7 @@ describe('RoutingConfigRepository', () => { actualError: err, errorMeta: { code: 500, - description: 'Error retrieving Routing Config', + description: 'Error retrieving routing configuration', }, }, }); @@ -419,7 +419,7 @@ describe('RoutingConfigRepository', () => { }); }); - test('returns 404 failure if routing config does not exist', async () => { + test('returns 404 failure if routing config does not exist on get', async () => { const { repo, mocks } = setup(); mocks.dynamo.on(GetCommand).resolvesOnce({ Item: undefined }); @@ -436,7 +436,7 @@ describe('RoutingConfigRepository', () => { }); }); - test('returns 404 failure if transaction fails because routing config does not exist', async () => { + test('returns 404 failure if routing config does not exist on write', async () => { const { repo, mocks } = setup(); const routingConfigWithLock: RoutingConfig = { @@ -535,7 +535,7 @@ describe('RoutingConfigRepository', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); @@ -543,7 +543,7 @@ describe('RoutingConfigRepository', () => { expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); - test('returns validation failure if cascade item has no defaultTemplateId or conditionalTemplates', async () => { + test('returns validation failure if cascade item conditionalTemplates is empty with no defaultTemplateId', async () => { const { repo, mocks } = setup(); const invalidRoutingConfig: RoutingConfig = { @@ -577,7 +577,7 @@ describe('RoutingConfigRepository', () => { expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); - test('returns validation failure if defaultTemplateId is null with no conditionalTemplates', async () => { + test('returns validation failure if defaultTemplateId is null', async () => { const { repo, mocks } = setup(); const invalidRoutingConfig: RoutingConfig = { @@ -1047,7 +1047,7 @@ describe('RoutingConfigRepository', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); @@ -1911,7 +1911,7 @@ describe('RoutingConfigRepository', () => { errorMeta: { code: 409, description: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }, }, }); diff --git a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts index c557e8173..37b4347cf 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -198,14 +198,18 @@ export class RoutingConfigRepository { if (currentLockNumber !== lockNumber) { return failure( ErrorCase.CONFLICT, - 'Lock number mismatch - Message Plan has been modified since last read' + 'Lock number mismatch - Routing configuration has been modified since last read' ); } - const submittableValidationError = - this.parseSubmittableRoutingConfig(cascade); - if (submittableValidationError) { - return submittableValidationError; + const submittableCascadeValidation = $SubmittableCascade.safeParse(cascade); + + if (!submittableCascadeValidation.success) { + return failure( + ErrorCase.VALIDATION_FAILED, + 'Routing config is not ready for submission: all cascade items must have templates assigned', + submittableCascadeValidation.error + ); } const templateIds = this.extractTemplateIds(cascade); @@ -230,7 +234,7 @@ export class RoutingConfigRepository { { Update: update, }, - // Template existence check + For LETTER templates, check they have PROOF_APPROVED or SUBMITTED status + // Template existence & ownership check + For LETTER templates, check they have PROOF_APPROVED or SUBMITTED status // Also exclude DELETED templates for all template types ...templateIds.map((templateId) => ({ ConditionCheck: { @@ -341,7 +345,7 @@ export class RoutingConfigRepository { } catch (error) { return failure( ErrorCase.INTERNAL, - 'Error retrieving Routing Config', + 'Error retrieving routing configuration', error ); } @@ -376,7 +380,7 @@ export class RoutingConfigRepository { if (item.lockNumber !== expectedLockNumber) { return failure( ErrorCase.CONFLICT, - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', err ); } @@ -480,18 +484,6 @@ export class RoutingConfigRepository { .filter((id): id is string => id != null); } - private parseSubmittableRoutingConfig(cascade: CascadeItem[]) { - const result = $SubmittableCascade.safeParse(cascade); - - if (!result.success) { - return failure( - ErrorCase.VALIDATION_FAILED, - 'Routing config is not ready for submission: all cascade items must have templates assigned', - result.error - ); - } - } - private handleSubmitTransactionError( err: unknown, lockNumber: number, diff --git a/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts b/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts index 085cb3337..1da4f5288 100644 --- a/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts +++ b/lambdas/backend-client/src/__tests__/schemas/routing-config.test.ts @@ -452,7 +452,7 @@ describe('$SubmittableCascadeItem', () => { expect(res.success).toBe(false); }); - test('invalid with empty conditionalTemplates and no defaultTemplateId', () => { + test('invalid with empty conditionalTemplates without defaultTemplateId', () => { const res = $SubmittableCascadeItem.safeParse({ cascadeGroups: ['standard'], channel: 'LETTER', @@ -468,6 +468,22 @@ describe('$SubmittableCascadeItem', () => { channel: 'LETTER', channelType: 'primary', defaultTemplateId: '', + conditionalTemplates: [ + { language: 'ar', templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09' }, + ], + }); + expect(res.success).toBe(false); + }); + + test('invalid with null defaultTemplateId', () => { + const res = $SubmittableCascadeItem.safeParse({ + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: null, + conditionalTemplates: [ + { language: 'ar', templateId: '90e46ece-4a3b-47bd-b781-f986b42a5a09' }, + ], }); expect(res.success).toBe(false); }); diff --git a/tests/test-team/template-mgmt-api-tests/delete-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/delete-routing-config.api.spec.ts index a1cdd7022..3d59f3214 100644 --- a/tests/test-team/template-mgmt-api-tests/delete-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/delete-routing-config.api.spec.ts @@ -254,7 +254,7 @@ test.describe('DELETE /v1/routing-configuration/:routingConfigId', () => { expect(await response.json()).toEqual({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }); }); }); diff --git a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts index 3896523b9..8ccf2bfdd 100644 --- a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts @@ -340,7 +340,7 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { expect(await response.json()).toEqual({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }); }); diff --git a/tests/test-team/template-mgmt-api-tests/update-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/update-routing-config.api.spec.ts index 0fbd7553e..59bb1c974 100644 --- a/tests/test-team/template-mgmt-api-tests/update-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/update-routing-config.api.spec.ts @@ -720,7 +720,7 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId', () => { expect(await response.json()).toEqual({ statusCode: 409, technicalMessage: - 'Lock number mismatch - Message Plan has been modified since last read', + 'Lock number mismatch - Routing configuration has been modified since last read', }); }); }); From d055d74f0f4b472d143810308c6ae187a082b5b4 Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 15:23:56 +0000 Subject: [PATCH 4/6] Update templates to SUBMITTED --- .../repository.test.ts | 567 +++++++++++++++--- .../routing-config-repository/repository.ts | 203 +++++-- .../submit-routing-config.api.spec.ts | 381 ++++++++++++ 3 files changed, 1016 insertions(+), 135 deletions(-) diff --git a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts index 0bc78b049..8e9385c46 100644 --- a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts +++ b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts @@ -1,4 +1,5 @@ import { + BatchGetCommand, DynamoDBDocumentClient, GetCommand, PutCommand, @@ -53,6 +54,25 @@ const clientOwnerKey = `CLIENT#${user.clientId}`; const dynamo = mockClient(DynamoDBDocumentClient); +// Helper to create valid template mocks that pass $TemplateDto validation +const makeTemplateMock = ( + overrides: { + id?: string; + templateType?: 'SMS' | 'EMAIL' | 'NHS_APP' | 'LETTER'; + templateStatus?: string; + lockNumber?: number; + } = {} +) => ({ + id: overrides.id ?? 'template-id', + name: 'Test Template', + templateType: overrides.templateType ?? 'SMS', + templateStatus: overrides.templateStatus ?? 'NOT_YET_SUBMITTED', + lockNumber: overrides.lockNumber ?? 1, + message: 'Test message content', + createdAt: date.toISOString(), + updatedAt: date.toISOString(), +}); + function setup() { dynamo.reset(); @@ -254,7 +274,7 @@ describe('RoutingConfigRepository', () => { }); describe('submit', () => { - test('updates routing config to COMPLETED', async () => { + test('updates routing config to COMPLETED and templates to SUBMITTED', async () => { const { repo, mocks } = setup(); const routingConfigWithLock: RoutingConfig = { @@ -268,16 +288,41 @@ describe('RoutingConfigRepository', () => { lockNumber: 3, }; + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateType: 'SMS', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithLock }) .resolvesOnce({ Item: completed }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); const result = await repo.submit(routingConfig.id, user, 2); expect(result).toEqual({ data: completed }); + expect(mocks.dynamo).toHaveReceivedCommandWith(BatchGetCommand, { + RequestItems: { + [TEMPLATE_TABLE_NAME]: { + Keys: [ + { + id: routingConfig.cascade[0].defaultTemplateId!, + owner: clientOwnerKey, + }, + ], + }, + }, + }); + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { TransactItems: [ { @@ -310,27 +355,155 @@ describe('RoutingConfigRepository', () => { }, }, { - ConditionCheck: { + Update: { ConditionExpression: - 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', + '#templateStatus = :condition_1_templateStatus AND (#lockNumber = :condition_2_1_lockNumber OR attribute_not_exists (#lockNumber))', + ExpressionAttributeNames: { + '#lockNumber': 'lockNumber', + '#templateStatus': 'templateStatus', + '#updatedAt': 'updatedAt', + '#updatedBy': 'updatedBy', + }, ExpressionAttributeValues: { - ':deleted': 'DELETED', - ':letterType': 'LETTER', - ':proofApproved': 'PROOF_APPROVED', - ':submitted': 'SUBMITTED', + ':condition_1_templateStatus': 'NOT_YET_SUBMITTED', + ':condition_2_1_lockNumber': 1, + ':lockNumber': 1, + ':templateStatus': 'SUBMITTED', + ':updatedAt': date.toISOString(), + ':updatedBy': `INTERNAL_USER#${user.internalUserId}`, }, Key: { id: routingConfig.cascade[0].defaultTemplateId!, owner: clientOwnerKey, }, + ReturnValues: 'ALL_NEW', ReturnValuesOnConditionCheckFailure: 'ALL_OLD', TableName: 'template-table-name', + UpdateExpression: + 'SET #templateStatus = :templateStatus, #updatedAt = :updatedAt, #updatedBy = :updatedBy ADD #lockNumber :lockNumber', }, }, ], }); }); + test('uses ConditionCheck for templates already SUBMITTED', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + const completed: RoutingConfig = { + ...routingConfig, + status: 'COMPLETED', + lockNumber: 3, + }; + + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateType: 'SMS', + templateStatus: 'SUBMITTED', + lockNumber: 5, + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLock }) + .resolvesOnce({ Item: completed }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ data: completed }); + + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { + TransactItems: [ + { + Update: expect.objectContaining({ + TableName: TABLE_NAME, + Key: { + id: routingConfig.id, + owner: clientOwnerKey, + }, + }), + }, + { + ConditionCheck: { + TableName: TEMPLATE_TABLE_NAME, + Key: { + id: routingConfig.cascade[0].defaultTemplateId!, + owner: clientOwnerKey, + }, + ConditionExpression: + 'attribute_exists(id) AND lockNumber = :lockNumber AND templateStatus = :submitted', + ExpressionAttributeValues: { + ':lockNumber': 5, + ':submitted': 'SUBMITTED', + }, + ReturnValuesOnConditionCheckFailure: 'ALL_OLD', + }, + }, + ], + }); + }); + + test('uses ConditionCheck with default lockNumber 0 for SUBMITTED template without lockNumber', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + const completed: RoutingConfig = { + ...routingConfig, + status: 'COMPLETED', + lockNumber: 3, + }; + + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateType: 'SMS', + templateStatus: 'SUBMITTED', + lockNumber: undefined, // intentionally omitted + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLock }) + .resolvesOnce({ Item: completed }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ data: completed }); + + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { + TransactItems: expect.arrayContaining([ + expect.objectContaining({ + ConditionCheck: expect.objectContaining({ + ExpressionAttributeValues: { + ':lockNumber': 0, // defaults to 0 when lockNumber is undefined + ':submitted': 'SUBMITTED', + }, + }), + }), + ]), + }); + }); + test('returns failure on client error during initial get', async () => { const { repo, mocks } = setup(); @@ -361,7 +534,19 @@ describe('RoutingConfigRepository', () => { lockNumber: 2, }; + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateType: 'SMS', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -391,10 +576,22 @@ describe('RoutingConfigRepository', () => { name: 0 as unknown as string, }; + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateType: 'SMS', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithLock }) .resolvesOnce({ Item: completedInvalid }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); const result = await repo.submit(routingConfig.id, user, 2); @@ -444,6 +641,12 @@ describe('RoutingConfigRepository', () => { lockNumber: 2, }; + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + const err = new TransactionCanceledException({ $metadata: {}, message: 'msg', @@ -451,6 +654,11 @@ describe('RoutingConfigRepository', () => { }); mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -627,25 +835,19 @@ describe('RoutingConfigRepository', () => { ], }; - const err = new TransactionCanceledException({ - $metadata: {}, - message: 'msg', - CancellationReasons: [ - { Code: 'None' }, // Update succeeded - { Code: 'ConditionalCheckFailed', Item: undefined }, // Template not found (no Item returned) - ], - }); - mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithTemplate }); - mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [], + }, + }); const result = await repo.submit(routingConfig.id, user, 2); expect(result).toEqual({ error: { - actualError: err, errorMeta: { code: 400, description: 'Some templates not found', @@ -653,6 +855,8 @@ describe('RoutingConfigRepository', () => { }, }, }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); test('returns 400 failure if template has DELETED status', async () => { @@ -671,32 +875,28 @@ describe('RoutingConfigRepository', () => { ], }; - const err = new TransactionCanceledException({ - $metadata: {}, - message: 'msg', - CancellationReasons: [ - { Code: 'None' }, // Update succeeded - { - Code: 'ConditionalCheckFailed', - Item: { - id: { S: 'deleted-template-id' }, - templateType: { S: 'SMS' }, - templateStatus: { S: 'DELETED' }, - }, - }, - ], - }); - mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithTemplate }); - mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + // Template exists but is DELETED + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [ + { + id: 'deleted-template-id', + owner: clientOwnerKey, + templateType: 'SMS', + templateStatus: 'DELETED', + lockNumber: 1, + }, + ], + }, + }); const result = await repo.submit(routingConfig.id, user, 2); expect(result).toEqual({ error: { - actualError: err, errorMeta: { code: 400, description: 'Some templates not found', @@ -704,6 +904,8 @@ describe('RoutingConfigRepository', () => { }, }, }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); test('returns 400 failure if LETTER template has invalid status', async () => { @@ -722,32 +924,28 @@ describe('RoutingConfigRepository', () => { ], }; - const err = new TransactionCanceledException({ - $metadata: {}, - message: 'msg', - CancellationReasons: [ - { Code: 'None' }, - { - Code: 'ConditionalCheckFailed', - Item: { - id: { S: 'letter-template-id' }, - templateType: { S: 'LETTER' }, - templateStatus: { S: 'DRAFT' }, - }, - }, - ], - }); - mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithLetter }); - mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + // LETTER template with invalid status (not PROOF_APPROVED or SUBMITTED) + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [ + { + id: 'letter-template-id', + owner: clientOwnerKey, + templateType: 'LETTER', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }, + ], + }, + }); const result = await repo.submit(routingConfig.id, user, 2); expect(result).toEqual({ error: { - actualError: err, errorMeta: { code: 400, description: @@ -757,29 +955,7 @@ describe('RoutingConfigRepository', () => { }, }); - expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { - TransactItems: expect.arrayContaining([ - { - ConditionCheck: { - TableName: TEMPLATE_TABLE_NAME, - Key: { - id: 'letter-template-id', - owner: clientOwnerKey, - }, - ConditionExpression: - 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', - ExpressionAttributeValues: { - ':deleted': 'DELETED', - ':letterType': 'LETTER', - ':proofApproved': 'PROOF_APPROVED', - ':submitted': 'SUBMITTED', - }, - ReturnValuesOnConditionCheckFailure: - ReturnValuesOnConditionCheckFailure.ALL_OLD, - }, - }, - ]), - }); + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); }); test('returns 500 failure on unexpected TransactionCanceledException', async () => { @@ -792,7 +968,7 @@ describe('RoutingConfigRepository', () => { message: 'Unexpected cancellation', CancellationReasons: [ { Code: 'None' }, // Update succeeded - { Code: 'None' }, // Template check also passed (unexpected) + { Code: 'None' }, // Template update also passed (unexpected) ], }); @@ -809,9 +985,20 @@ describe('RoutingConfigRepository', () => { ], }; + const template = makeTemplateMock({ + id: 'some-template-id', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + mocks.dynamo .on(GetCommand) .resolvesOnce({ Item: routingConfigWithTemplate }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -841,7 +1028,18 @@ describe('RoutingConfigRepository', () => { lockNumber: 2, }; + const template = makeTemplateMock({ + id: routingConfig.cascade[0].defaultTemplateId!, + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); const result = await repo.submit(routingConfig.id, user, 2); @@ -856,6 +1054,219 @@ describe('RoutingConfigRepository', () => { }, }); }); + + test('returns 409 failure if template was modified since retrieval', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithTemplate: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: 'modified-template-id', + }, + ], + }; + + const template = makeTemplateMock({ + id: 'modified-template-id', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + + const err = new TransactionCanceledException({ + $metadata: {}, + message: 'Transaction cancelled', + CancellationReasons: [ + { Code: 'None' }, // Routing config update succeeded + { Code: 'ConditionalCheckFailed' }, // Template lock number mismatch + ], + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithTemplate }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); + mocks.dynamo.on(TransactWriteCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 409, + description: + 'Some templates have been modified since they were retrieved', + details: { templateIds: 'modified-template-id' }, + }, + }, + }); + }); + + test('returns 500 failure if BatchGetCommand fails', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + const err = new Error('BatchGet failed'); + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).rejectsOnce(err); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 500, + description: 'Failed to retrieve templates', + }, + }, + }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); + }); + + test('succeeds when routing config has no templates (empty cascade)', async () => { + const { repo, mocks } = setup(); + + const routingConfigNoTemplates: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + conditionalTemplates: [ + { language: 'en', templateId: 'english-template' }, + ], + }, + ], + }; + + const completed: RoutingConfig = { + ...routingConfigNoTemplates, + status: 'COMPLETED', + lockNumber: 3, + }; + + const template = makeTemplateMock({ + id: 'english-template', + templateType: 'NHS_APP', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + }); + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigNoTemplates }) + .resolvesOnce({ Item: completed }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ data: completed }); + }); + + test('handles template with undefined lockNumber', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + const completed: RoutingConfig = { + ...routingConfig, + status: 'COMPLETED', + lockNumber: 3, + }; + + const template = { + id: routingConfig.cascade[0].defaultTemplateId!, + owner: clientOwnerKey, + templateType: 'SMS', + templateStatus: 'NOT_YET_SUBMITTED', + name: 'Test Template', + message: 'Test message content', + createdAt: date.toISOString(), + updatedAt: date.toISOString(), + }; + + mocks.dynamo + .on(GetCommand) + .resolvesOnce({ Item: routingConfigWithLock }) + .resolvesOnce({ Item: completed }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [template], + }, + }); + mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ data: completed }); + + expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { + TransactItems: expect.arrayContaining([ + expect.objectContaining({ + Update: expect.objectContaining({ + ExpressionAttributeValues: expect.objectContaining({ + ':condition_2_1_lockNumber': 0, + }), + }), + }), + ]), + }); + }); + + test('handles BatchGetCommand returning empty Responses', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: {}, + }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: undefined, + errorMeta: { + code: 400, + description: 'Some templates not found', + details: { + templateIds: routingConfig.cascade[0].defaultTemplateId, + }, + }, + }, + }); + }); }); describe('delete', () => { diff --git a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts index 37b4347cf..ed6d2226c 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -1,5 +1,6 @@ import { randomUUID } from 'node:crypto'; import { + BatchGetCommand, GetCommand, PutCommand, TransactWriteCommand, @@ -14,17 +15,22 @@ import { import { $RoutingConfig, $SubmittableCascade, + $TemplateDto, CascadeItem, type CreateRoutingConfig, ErrorCase, type RoutingConfig, type RoutingConfigReference, RoutingConfigStatus, + type TemplateDto, type UpdateRoutingConfig, } from 'nhs-notify-backend-client'; import type { User } from 'nhs-notify-web-template-management-utils'; import { RoutingConfigQuery } from './query'; -import { RoutingConfigUpdateBuilder } from 'nhs-notify-entity-update-command-builder'; +import { + RoutingConfigUpdateBuilder, + TemplateUpdateBuilder, +} from 'nhs-notify-entity-update-command-builder'; import { ConditionalCheckFailedException, ReturnValuesOnConditionCheckFailure, @@ -182,7 +188,6 @@ export class RoutingConfigRepository { lockNumber: currentLockNumber, } = existingConfig.data; - // Check status before cascade validation if (status === 'DELETED') { return failure(ErrorCase.NOT_FOUND, 'Routing configuration not found'); } @@ -194,7 +199,6 @@ export class RoutingConfigRepository { ); } - // Check lock number before cascade validation if (currentLockNumber !== lockNumber) { return failure( ErrorCase.CONFLICT, @@ -214,7 +218,47 @@ export class RoutingConfigRepository { const templateIds = this.extractTemplateIds(cascade); - const update = new RoutingConfigUpdateBuilder( + const templatesResult = await this.getTemplates(templateIds, user.clientId); + + if (templatesResult.error) { + return templatesResult; + } + + const templates = templatesResult.data; + + const missingTemplateIds = templateIds.filter( + (tid) => + !templates.some((t) => t.id === tid && t.templateStatus !== 'DELETED') + ); + + if (missingTemplateIds.length > 0) { + return failure( + ErrorCase.ROUTING_CONFIG_TEMPLATES_NOT_FOUND, + 'Some templates not found', + undefined, + { templateIds: missingTemplateIds.join(',') } + ); + } + + const invalidLetterTemplateIds = templates + .filter( + (t) => + t.templateType === 'LETTER' && + t.templateStatus !== 'PROOF_APPROVED' && + t.templateStatus !== 'SUBMITTED' + ) + .map((t) => t.id); + + if (invalidLetterTemplateIds.length > 0) { + return failure( + ErrorCase.VALIDATION_FAILED, + 'Letter templates must have status PROOF_APPROVED or SUBMITTED', + undefined, + { templateIds: invalidLetterTemplateIds.join(',') } + ); + } + + const routingConfigUpdate = new RoutingConfigUpdateBuilder( this.tableName, user.clientId, id, @@ -227,34 +271,50 @@ export class RoutingConfigRepository { .incrementLockNumber() .build(); + // add an update to each template to set status to SUBMITTED, or a condition check if already SUBMITTED + const templateUpdatesAndChecks = templates.map((template) => + template.templateStatus === 'SUBMITTED' + ? { + ConditionCheck: { + TableName: this.templateTableName, + Key: { + id: template.id, + owner: this.clientOwnerKey(user.clientId), + }, + ConditionExpression: + 'attribute_exists(id) AND lockNumber = :lockNumber AND templateStatus = :submitted', + ExpressionAttributeValues: { + ':lockNumber': template.lockNumber ?? 0, + ':submitted': 'SUBMITTED', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, + }, + } + : { + Update: new TemplateUpdateBuilder( + this.templateTableName, + user.clientId, + template.id, + this.updateCmdOpts + ) + .setStatus('SUBMITTED') + .expectStatus(template.templateStatus) + .expectLockNumber(template.lockNumber ?? 0) + .incrementLockNumber() + .setUpdatedByUserAt(this.internalUserKey(user)) + .build(), + } + ); + try { await this.client.send( new TransactWriteCommand({ TransactItems: [ { - Update: update, + Update: routingConfigUpdate, }, - // Template existence & ownership check + For LETTER templates, check they have PROOF_APPROVED or SUBMITTED status - // Also exclude DELETED templates for all template types - ...templateIds.map((templateId) => ({ - ConditionCheck: { - TableName: this.templateTableName, - Key: { - id: templateId, - owner: this.clientOwnerKey(user.clientId), - }, - ConditionExpression: - 'attribute_exists(id) AND templateStatus <> :deleted AND (templateType <> :letterType OR templateStatus IN (:proofApproved, :submitted))', - ExpressionAttributeValues: { - ':deleted': 'DELETED', - ':letterType': 'LETTER', - ':proofApproved': 'PROOF_APPROVED', - ':submitted': 'SUBMITTED', - }, - ReturnValuesOnConditionCheckFailure: - ReturnValuesOnConditionCheckFailure.ALL_OLD, - }, - })), + ...templateUpdatesAndChecks, ], }) ); @@ -282,6 +342,53 @@ export class RoutingConfigRepository { } } + private async getTemplates( + templateIds: string[], + clientId: string + ): Promise> { + // istanbul ignore next -- defensive check; $SubmittableCascade validation ensures at least one template + if (templateIds.length === 0) { + return success([]); + } + + try { + const result = await this.client.send( + new BatchGetCommand({ + RequestItems: { + [this.templateTableName]: { + Keys: templateIds.map((tid) => ({ + id: tid, + owner: this.clientOwnerKey(clientId), + })), + }, + }, + }) + ); + + const rawTemplates = result.Responses?.[this.templateTableName] ?? []; + + const templates: TemplateDto[] = []; + + for (const item of rawTemplates) { + const parsed = $TemplateDto.safeParse(item); + + if (!parsed.success) { + return failure( + ErrorCase.INTERNAL, + 'Error parsing template from database', + parsed.error + ); + } + + templates.push(parsed.data); + } + + return success(templates); + } catch (error) { + return failure(ErrorCase.INTERNAL, 'Failed to retrieve templates', error); + } + } + async delete( id: string, user: User, @@ -493,56 +600,38 @@ export class RoutingConfigRepository { return this.handleUpdateError(err, lockNumber); } - // Note: The first item will always be the update + // Note: The first item will always be the routing config update // https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_CancellationReason.html - const [updateReason, ...templateReasons] = err.CancellationReasons ?? []; + const [routingConfigReason, ...templateReasons] = + err.CancellationReasons ?? []; - if (updateReason && updateReason.Code !== 'None') { + if (routingConfigReason && routingConfigReason.Code !== 'None') { return this.handleUpdateError( new ConditionalCheckFailedException({ - message: updateReason.Message!, - Item: updateReason.Item, + message: routingConfigReason.Message!, + Item: routingConfigReason.Item, $metadata: err.$metadata, }), lockNumber ); } - // Find which templates failed the condition check - const missingTemplateIds: string[] = []; - const invalidLetterTemplateIds: string[] = []; + // Find which template updates failed - likely due to lock number mismatch + const failedTemplateIds: string[] = []; for (const [index, reason] of templateReasons.entries()) { if (reason.Code === 'ConditionalCheckFailed') { const templateId = templateIds[index]; - - if ( - !reason.Item || - reason.Item.templateStatus?.S === - ('DELETED' satisfies RoutingConfigStatus) - ) { - missingTemplateIds.push(templateId); - } else { - invalidLetterTemplateIds.push(templateId); - } + failedTemplateIds.push(templateId); } } - if (missingTemplateIds.length > 0) { - return failure( - ErrorCase.ROUTING_CONFIG_TEMPLATES_NOT_FOUND, - 'Some templates not found', - err, - { templateIds: missingTemplateIds.join(',') } - ); - } - - if (invalidLetterTemplateIds.length > 0) { + if (failedTemplateIds.length > 0) { return failure( - ErrorCase.VALIDATION_FAILED, - 'Letter templates must have status PROOF_APPROVED or SUBMITTED', + ErrorCase.CONFLICT, + 'Some templates have been modified since they were retrieved', err, - { templateIds: invalidLetterTemplateIds.join(',') } + { templateIds: failedTemplateIds.join(',') } ); } diff --git a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts index 8ccf2bfdd..b99b6f3c9 100644 --- a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts @@ -707,4 +707,385 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { const responseBody = await response.json(); expect(responseBody.data.status).toBe('COMPLETED'); }); + + test.describe('template status updates on submit', () => { + test('updates NHSAPP template status to SUBMITTED after routing config submit', async ({ + request, + }) => { + const templateId = randomUUID(); + + // Create a template with NOT_YET_SUBMITTED status + const template = TemplateFactory.createNhsAppTemplate( + templateId, + user1, + 'Test Template for Status Update' + ); + template.templateStatus = 'NOT_YET_SUBMITTED'; + template.lockNumber = 5; + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify template was updated to SUBMITTED + const updatedTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId, + }); + + expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedTemplate.lockNumber).toBe(6); + }); + + test('updates EMAIL template status to SUBMITTED after routing config submit', async ({ + request, + }) => { + const templateId = randomUUID(); + + // Create an EMAIL template with NOT_YET_SUBMITTED status + const template = TemplateFactory.createEmailTemplate( + templateId, + user1, + 'Test Email Template for Status Update' + ); + template.templateStatus = 'NOT_YET_SUBMITTED'; + template.lockNumber = 3; + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'EMAIL', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify template was updated to SUBMITTED + const updatedTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId, + }); + + expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedTemplate.lockNumber).toBe(4); + }); + + test('updates SMS template status to SUBMITTED after routing config submit', async ({ + request, + }) => { + const templateId = randomUUID(); + + // Create an SMS template with NOT_YET_SUBMITTED status + const template = TemplateFactory.createSmsTemplate( + templateId, + user1, + 'Test SMS Template for Status Update' + ); + template.templateStatus = 'NOT_YET_SUBMITTED'; + template.lockNumber = 1; + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify template was updated to SUBMITTED + const updatedTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId, + }); + + expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedTemplate.lockNumber).toBe(2); + }); + + test('updates LETTER template lockNumber without changing SUBMITTED status', async ({ + request, + }) => { + const templateId = randomUUID(); + + // Create a LETTER template that is already SUBMITTED + const template = TemplateFactory.uploadLetterTemplate( + templateId, + user1, + 'Test Letter Template Already Submitted', + 'SUBMITTED' + ); + template.lockNumber = 10; + + await templateStorageHelper.seedTemplateData([template]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: templateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify template lockNumber was incremented but status remains SUBMITTED + const updatedTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId, + }); + + expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedTemplate.lockNumber).toBe(11); + }); + + test('updates multiple templates to SUBMITTED after routing config submit', async ({ + request, + }) => { + const nhsAppTemplateId = randomUUID(); + const emailTemplateId = randomUUID(); + const smsTemplateId = randomUUID(); + + // Create multiple templates with NOT_YET_SUBMITTED status + const nhsAppTemplate = TemplateFactory.createNhsAppTemplate( + nhsAppTemplateId, + user1, + 'NHS App Template' + ); + nhsAppTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + nhsAppTemplate.lockNumber = 1; + + const emailTemplate = TemplateFactory.createEmailTemplate( + emailTemplateId, + user1, + 'Email Template' + ); + emailTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + emailTemplate.lockNumber = 2; + + const smsTemplate = TemplateFactory.createSmsTemplate( + smsTemplateId, + user1, + 'SMS Template' + ); + smsTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + smsTemplate.lockNumber = 3; + + await templateStorageHelper.seedTemplateData([ + nhsAppTemplate, + emailTemplate, + smsTemplate, + ]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: nhsAppTemplateId, + }, + { + cascadeGroups: ['standard'], + channel: 'EMAIL', + channelType: 'secondary', + defaultTemplateId: emailTemplateId, + }, + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'secondary', + defaultTemplateId: smsTemplateId, + }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify all templates were updated to SUBMITTED + const updatedNhsAppTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: nhsAppTemplateId, + }); + const updatedEmailTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: emailTemplateId, + }); + const updatedSmsTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: smsTemplateId, + }); + + expect(updatedNhsAppTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedNhsAppTemplate.lockNumber).toBe(2); + + expect(updatedEmailTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedEmailTemplate.lockNumber).toBe(3); + + expect(updatedSmsTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedSmsTemplate.lockNumber).toBe(4); + }); + + test('updates conditionalTemplates to SUBMITTED after routing config submit', async ({ + request, + }) => { + const englishTemplateId = randomUUID(); + const frenchTemplateId = randomUUID(); + + // Create conditional templates + const englishTemplate = TemplateFactory.createNhsAppTemplate( + englishTemplateId, + user1, + 'English Template' + ); + englishTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + englishTemplate.lockNumber = 1; + + const frenchTemplate = TemplateFactory.createNhsAppTemplate( + frenchTemplateId, + user1, + 'French Template' + ); + frenchTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + frenchTemplate.lockNumber = 2; + + await templateStorageHelper.seedTemplateData([ + englishTemplate, + frenchTemplate, + ]); + + const { dbEntry } = RoutingConfigFactory.create(user1, { + cascade: [ + { + cascadeGroups: ['translations'], + channel: 'NHSAPP', + channelType: 'primary', + conditionalTemplates: [ + { language: 'en', templateId: englishTemplateId }, + { language: 'fr', templateId: frenchTemplateId }, + ], + }, + ], + cascadeGroupOverrides: [ + { name: 'translations', language: ['en', 'fr'] }, + ], + }); + + await storageHelper.seed([dbEntry]); + + const response = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, + { + headers: { + Authorization: await user1.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + } + ); + + expect(response.status()).toBe(200); + + // Verify both conditional templates were updated to SUBMITTED + const updatedEnglishTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: englishTemplateId, + }); + const updatedFrenchTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: frenchTemplateId, + }); + + expect(updatedEnglishTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedEnglishTemplate.lockNumber).toBe(2); + + expect(updatedFrenchTemplate.templateStatus).toBe('SUBMITTED'); + expect(updatedFrenchTemplate.lockNumber).toBe(3); + }); + }); }); From ad7bf1db84fc978ed91b2b73177444db6b8aeed7 Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 17:50:00 +0000 Subject: [PATCH 5/6] terraform and test fixes --- .../module_submit_routing_config_lambda.tf | 2 + .../repository.test.ts | 109 ++-- .../routing-config-repository/repository.ts | 9 +- .../submit-routing-config.api.spec.ts | 499 +++--------------- 4 files changed, 138 insertions(+), 481 deletions(-) diff --git a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf index 0e5613fd6..490ad0e96 100644 --- a/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf +++ b/infrastructure/terraform/modules/backend-api/module_submit_routing_config_lambda.tf @@ -55,6 +55,8 @@ data "aws_iam_policy_document" "submit_routing_config_lambda_policy" { effect = "Allow" actions = [ + "dynamodb:BatchGetItem", + "dynamodb:UpdateItem", "dynamodb:ConditionCheckItem", ] diff --git a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts index 8e9385c46..73e99fa4b 100644 --- a/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts +++ b/lambdas/backend-api/src/__tests__/infra/routing-config-repository/repository.test.ts @@ -454,56 +454,6 @@ describe('RoutingConfigRepository', () => { }); }); - test('uses ConditionCheck with default lockNumber 0 for SUBMITTED template without lockNumber', async () => { - const { repo, mocks } = setup(); - - const routingConfigWithLock: RoutingConfig = { - ...routingConfig, - lockNumber: 2, - }; - - const completed: RoutingConfig = { - ...routingConfig, - status: 'COMPLETED', - lockNumber: 3, - }; - - const template = makeTemplateMock({ - id: routingConfig.cascade[0].defaultTemplateId!, - templateType: 'SMS', - templateStatus: 'SUBMITTED', - lockNumber: undefined, // intentionally omitted - }); - - mocks.dynamo - .on(GetCommand) - .resolvesOnce({ Item: routingConfigWithLock }) - .resolvesOnce({ Item: completed }); - mocks.dynamo.on(BatchGetCommand).resolvesOnce({ - Responses: { - [TEMPLATE_TABLE_NAME]: [template], - }, - }); - mocks.dynamo.on(TransactWriteCommand).resolvesOnce({}); - - const result = await repo.submit(routingConfig.id, user, 2); - - expect(result).toEqual({ data: completed }); - - expect(mocks.dynamo).toHaveReceivedCommandWith(TransactWriteCommand, { - TransactItems: expect.arrayContaining([ - expect.objectContaining({ - ConditionCheck: expect.objectContaining({ - ExpressionAttributeValues: { - ':lockNumber': 0, // defaults to 0 when lockNumber is undefined - ':submitted': 'SUBMITTED', - }, - }), - }), - ]), - }); - }); - test('returns failure on client error during initial get', async () => { const { repo, mocks } = setup(); @@ -616,6 +566,47 @@ describe('RoutingConfigRepository', () => { }); }); + test('returns failure if template from database is invalid', async () => { + const { repo, mocks } = setup(); + + const routingConfigWithLock: RoutingConfig = { + ...routingConfig, + lockNumber: 2, + }; + + // Invalid template - missing required fields like name, createdAt, etc. + const invalidTemplate = { + id: routingConfig.cascade[0].defaultTemplateId!, + owner: clientOwnerKey, + templateType: 'SMS', + templateStatus: 'NOT_YET_SUBMITTED', + lockNumber: 1, + // Missing: name, message, createdAt, updatedAt + }; + + mocks.dynamo.on(GetCommand).resolvesOnce({ Item: routingConfigWithLock }); + mocks.dynamo.on(BatchGetCommand).resolvesOnce({ + Responses: { + [TEMPLATE_TABLE_NAME]: [invalidTemplate], + }, + }); + + const result = await repo.submit(routingConfig.id, user, 2); + + expect(result).toEqual({ + error: { + actualError: expect.any(Error), + errorMeta: { + code: 500, + description: 'Error parsing template from database', + details: undefined, + }, + }, + }); + + expect(mocks.dynamo).not.toHaveReceivedCommand(TransactWriteCommand); + }); + test('returns 404 failure if routing config does not exist on get', async () => { const { repo, mocks } = setup(); @@ -882,13 +873,12 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(BatchGetCommand).resolvesOnce({ Responses: { [TEMPLATE_TABLE_NAME]: [ - { + makeTemplateMock({ id: 'deleted-template-id', - owner: clientOwnerKey, templateType: 'SMS', templateStatus: 'DELETED', lockNumber: 1, - }, + }), ], }, }); @@ -934,9 +924,22 @@ describe('RoutingConfigRepository', () => { { id: 'letter-template-id', owner: clientOwnerKey, + name: 'Test Letter Template', templateType: 'LETTER', templateStatus: 'NOT_YET_SUBMITTED', lockNumber: 1, + letterType: 'x0', + language: 'en', + letterVersion: 'PDF', + files: { + pdfTemplate: { + fileName: 'test.pdf', + currentVersion: '1', + virusScanStatus: 'PASSED', + }, + }, + createdAt: date.toISOString(), + updatedAt: date.toISOString(), }, ], }, diff --git a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts index ed6d2226c..0dbd21ea0 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -284,7 +284,7 @@ export class RoutingConfigRepository { ConditionExpression: 'attribute_exists(id) AND lockNumber = :lockNumber AND templateStatus = :submitted', ExpressionAttributeValues: { - ':lockNumber': template.lockNumber ?? 0, + ':lockNumber': template.lockNumber, ':submitted': 'SUBMITTED', }, ReturnValuesOnConditionCheckFailure: @@ -300,7 +300,7 @@ export class RoutingConfigRepository { ) .setStatus('SUBMITTED') .expectStatus(template.templateStatus) - .expectLockNumber(template.lockNumber ?? 0) + .expectLockNumber(template.lockNumber) .incrementLockNumber() .setUpdatedByUserAt(this.internalUserKey(user)) .build(), @@ -346,11 +346,6 @@ export class RoutingConfigRepository { templateIds: string[], clientId: string ): Promise> { - // istanbul ignore next -- defensive check; $SubmittableCascade validation ensures at least one template - if (templateIds.length === 0) { - return success([]); - } - try { const result = await this.client.send( new BatchGetCommand({ diff --git a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts index b99b6f3c9..fd788d423 100644 --- a/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts +++ b/tests/test-team/template-mgmt-api-tests/submit-routing-config.api.spec.ts @@ -620,72 +620,80 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { }); }); - test('returns 200 if LETTER template has status PROOF_APPROVED', async ({ + test('Template statuses are updated on routing config submit', async ({ request, }) => { - const letterTemplateId = randomUUID(); + const appTemplateId = randomUUID(); + const emailTemplateId = randomUUID(); + const englishTemplateId = randomUUID(); + const frenchTemplateId = randomUUID(); - // Create a LETTER template with PROOF_APPROVED status - const letterTemplate = TemplateFactory.uploadLetterTemplate( - letterTemplateId, + const nhsAppTemplate = TemplateFactory.createNhsAppTemplate( + appTemplateId, user1, - 'Test Letter Template', - 'PROOF_APPROVED' + 'Nhs app template' ); + nhsAppTemplate.templateStatus = 'NOT_YET_SUBMITTED'; + nhsAppTemplate.lockNumber = 1; - await templateStorageHelper.seedTemplateData([letterTemplate]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'LETTER', - channelType: 'primary', - defaultTemplateId: letterTemplateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } + const emailTemplate = TemplateFactory.createEmailTemplate( + emailTemplateId, + user1, + 'Email Template' ); + emailTemplate.templateStatus = 'SUBMITTED'; + emailTemplate.lockNumber = 2; - expect(response.status()).toBe(200); - - const responseBody = await response.json(); - expect(responseBody.data.status).toBe('COMPLETED'); - }); - - test('returns 200 if LETTER template has status SUBMITTED', async ({ - request, - }) => { - const letterTemplateId = randomUUID(); - - // Create a LETTER template with SUBMITTED status - const letterTemplate = TemplateFactory.uploadLetterTemplate( - letterTemplateId, + const englishTemplate = TemplateFactory.createAuthoringLetterTemplate( + englishTemplateId, user1, - 'Test Letter Template', - 'SUBMITTED' + 'English Template' ); + englishTemplate.templateStatus = 'PROOF_APPROVED'; + englishTemplate.lockNumber = 3; - await templateStorageHelper.seedTemplateData([letterTemplate]); + const frenchTemplate = TemplateFactory.createAuthoringLetterTemplate( + frenchTemplateId, + user1, + 'French Template' + ); + frenchTemplate.templateStatus = 'PROOF_APPROVED'; + frenchTemplate.lockNumber = 4; + + await templateStorageHelper.seedTemplateData([ + nhsAppTemplate, + emailTemplate, + englishTemplate, + frenchTemplate, + ]); + + const startingEmailTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: emailTemplateId, + }); const { dbEntry } = RoutingConfigFactory.create(user1, { cascade: [ { cascadeGroups: ['standard'], + channel: 'NHSAPP', + channelType: 'primary', + defaultTemplateId: appTemplateId, + }, + { + cascadeGroups: ['standard'], + channel: 'EMAIL', + channelType: 'primary', + defaultTemplateId: emailTemplateId, + }, + { + cascadeGroups: ['translations'], channel: 'LETTER', channelType: 'primary', - defaultTemplateId: letterTemplateId, + conditionalTemplates: [ + { language: 'en', templateId: englishTemplateId }, + { language: 'fr', templateId: frenchTemplateId }, + ], }, ], }); @@ -703,389 +711,38 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId/submit', () => { ); expect(response.status()).toBe(200); - const responseBody = await response.json(); expect(responseBody.data.status).toBe('COMPLETED'); - }); - - test.describe('template status updates on submit', () => { - test('updates NHSAPP template status to SUBMITTED after routing config submit', async ({ - request, - }) => { - const templateId = randomUUID(); - - // Create a template with NOT_YET_SUBMITTED status - const template = TemplateFactory.createNhsAppTemplate( - templateId, - user1, - 'Test Template for Status Update' - ); - template.templateStatus = 'NOT_YET_SUBMITTED'; - template.lockNumber = 5; - - await templateStorageHelper.seedTemplateData([template]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'NHSAPP', - channelType: 'primary', - defaultTemplateId: templateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - - expect(response.status()).toBe(200); - - // Verify template was updated to SUBMITTED - const updatedTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId, - }); - - expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedTemplate.lockNumber).toBe(6); - }); - - test('updates EMAIL template status to SUBMITTED after routing config submit', async ({ - request, - }) => { - const templateId = randomUUID(); - - // Create an EMAIL template with NOT_YET_SUBMITTED status - const template = TemplateFactory.createEmailTemplate( - templateId, - user1, - 'Test Email Template for Status Update' - ); - template.templateStatus = 'NOT_YET_SUBMITTED'; - template.lockNumber = 3; - - await templateStorageHelper.seedTemplateData([template]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'EMAIL', - channelType: 'primary', - defaultTemplateId: templateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - expect(response.status()).toBe(200); - - // Verify template was updated to SUBMITTED - const updatedTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId, - }); - - expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedTemplate.lockNumber).toBe(4); + const dbAppTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: appTemplateId, }); - - test('updates SMS template status to SUBMITTED after routing config submit', async ({ - request, - }) => { - const templateId = randomUUID(); - - // Create an SMS template with NOT_YET_SUBMITTED status - const template = TemplateFactory.createSmsTemplate( - templateId, - user1, - 'Test SMS Template for Status Update' - ); - template.templateStatus = 'NOT_YET_SUBMITTED'; - template.lockNumber = 1; - - await templateStorageHelper.seedTemplateData([template]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'SMS', - channelType: 'primary', - defaultTemplateId: templateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - - expect(response.status()).toBe(200); - - // Verify template was updated to SUBMITTED - const updatedTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId, - }); - - expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedTemplate.lockNumber).toBe(2); + const dbEmailTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: emailTemplateId, }); - - test('updates LETTER template lockNumber without changing SUBMITTED status', async ({ - request, - }) => { - const templateId = randomUUID(); - - // Create a LETTER template that is already SUBMITTED - const template = TemplateFactory.uploadLetterTemplate( - templateId, - user1, - 'Test Letter Template Already Submitted', - 'SUBMITTED' - ); - template.lockNumber = 10; - - await templateStorageHelper.seedTemplateData([template]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'LETTER', - channelType: 'primary', - defaultTemplateId: templateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - - expect(response.status()).toBe(200); - - // Verify template lockNumber was incremented but status remains SUBMITTED - const updatedTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId, - }); - - expect(updatedTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedTemplate.lockNumber).toBe(11); + const dbEnglishTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: englishTemplateId, }); - - test('updates multiple templates to SUBMITTED after routing config submit', async ({ - request, - }) => { - const nhsAppTemplateId = randomUUID(); - const emailTemplateId = randomUUID(); - const smsTemplateId = randomUUID(); - - // Create multiple templates with NOT_YET_SUBMITTED status - const nhsAppTemplate = TemplateFactory.createNhsAppTemplate( - nhsAppTemplateId, - user1, - 'NHS App Template' - ); - nhsAppTemplate.templateStatus = 'NOT_YET_SUBMITTED'; - nhsAppTemplate.lockNumber = 1; - - const emailTemplate = TemplateFactory.createEmailTemplate( - emailTemplateId, - user1, - 'Email Template' - ); - emailTemplate.templateStatus = 'NOT_YET_SUBMITTED'; - emailTemplate.lockNumber = 2; - - const smsTemplate = TemplateFactory.createSmsTemplate( - smsTemplateId, - user1, - 'SMS Template' - ); - smsTemplate.templateStatus = 'NOT_YET_SUBMITTED'; - smsTemplate.lockNumber = 3; - - await templateStorageHelper.seedTemplateData([ - nhsAppTemplate, - emailTemplate, - smsTemplate, - ]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['standard'], - channel: 'NHSAPP', - channelType: 'primary', - defaultTemplateId: nhsAppTemplateId, - }, - { - cascadeGroups: ['standard'], - channel: 'EMAIL', - channelType: 'secondary', - defaultTemplateId: emailTemplateId, - }, - { - cascadeGroups: ['standard'], - channel: 'SMS', - channelType: 'secondary', - defaultTemplateId: smsTemplateId, - }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - - expect(response.status()).toBe(200); - - // Verify all templates were updated to SUBMITTED - const updatedNhsAppTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId: nhsAppTemplateId, - }); - const updatedEmailTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId: emailTemplateId, - }); - const updatedSmsTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId: smsTemplateId, - }); - - expect(updatedNhsAppTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedNhsAppTemplate.lockNumber).toBe(2); - - expect(updatedEmailTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedEmailTemplate.lockNumber).toBe(3); - - expect(updatedSmsTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedSmsTemplate.lockNumber).toBe(4); + const dbFrenchTemplate = await templateStorageHelper.getTemplate({ + clientId: user1.clientId, + templateId: frenchTemplateId, }); - test('updates conditionalTemplates to SUBMITTED after routing config submit', async ({ - request, - }) => { - const englishTemplateId = randomUUID(); - const frenchTemplateId = randomUUID(); - - // Create conditional templates - const englishTemplate = TemplateFactory.createNhsAppTemplate( - englishTemplateId, - user1, - 'English Template' - ); - englishTemplate.templateStatus = 'NOT_YET_SUBMITTED'; - englishTemplate.lockNumber = 1; - - const frenchTemplate = TemplateFactory.createNhsAppTemplate( - frenchTemplateId, - user1, - 'French Template' - ); - frenchTemplate.templateStatus = 'NOT_YET_SUBMITTED'; - frenchTemplate.lockNumber = 2; - - await templateStorageHelper.seedTemplateData([ - englishTemplate, - frenchTemplate, - ]); - - const { dbEntry } = RoutingConfigFactory.create(user1, { - cascade: [ - { - cascadeGroups: ['translations'], - channel: 'NHSAPP', - channelType: 'primary', - conditionalTemplates: [ - { language: 'en', templateId: englishTemplateId }, - { language: 'fr', templateId: frenchTemplateId }, - ], - }, - ], - cascadeGroupOverrides: [ - { name: 'translations', language: ['en', 'fr'] }, - ], - }); - - await storageHelper.seed([dbEntry]); - - const response = await request.patch( - `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}/submit`, - { - headers: { - Authorization: await user1.getAccessToken(), - 'X-Lock-Number': String(dbEntry.lockNumber), - }, - } - ); - - expect(response.status()).toBe(200); + expect(dbAppTemplate.templateStatus).toBe('SUBMITTED'); + expect(dbAppTemplate.lockNumber).toBe(2); - // Verify both conditional templates were updated to SUBMITTED - const updatedEnglishTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId: englishTemplateId, - }); - const updatedFrenchTemplate = await templateStorageHelper.getTemplate({ - clientId: user1.clientId, - templateId: frenchTemplateId, - }); + // email was already submitted so should not have changed + expect(dbEmailTemplate.templateStatus).toBe('SUBMITTED'); + expect(dbEmailTemplate.lockNumber).toBe(2); + expect(dbEmailTemplate.updatedAt).toEqual(startingEmailTemplate.updatedAt); - expect(updatedEnglishTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedEnglishTemplate.lockNumber).toBe(2); + expect(dbEnglishTemplate.templateStatus).toBe('SUBMITTED'); + expect(dbEnglishTemplate.lockNumber).toBe(4); - expect(updatedFrenchTemplate.templateStatus).toBe('SUBMITTED'); - expect(updatedFrenchTemplate.lockNumber).toBe(3); - }); + expect(dbFrenchTemplate.templateStatus).toBe('SUBMITTED'); + expect(dbFrenchTemplate.lockNumber).toBe(5); }); }); From 0ec14c432498422de4b536c602663b3fddab9abf Mon Sep 17 00:00:00 2001 From: Clare Jones Date: Wed, 4 Feb 2026 19:36:42 +0000 Subject: [PATCH 6/6] trivvy --- package-lock.json | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/package-lock.json b/package-lock.json index a47557758..b426ccbc7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7746,9 +7746,9 @@ } }, "node_modules/@isaacs/brace-expansion": { - "version": "5.0.0", - "resolved": "https://registry.npmjs.org/@isaacs/brace-expansion/-/brace-expansion-5.0.0.tgz", - "integrity": "sha512-ZT55BDLV0yv0RBm2czMiZ+SqCGO7AvmOM3G/w2xhVPH+te0aKgFjmBvGlL1dH+ql2tgGO3MVrbb3jCKyvpgnxA==", + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/@isaacs/brace-expansion/-/brace-expansion-5.0.1.tgz", + "integrity": "sha512-WMz71T1JS624nWj2n2fnYAuPovhv7EUhk69R6i9dsVyzxt5eM3bjwvgk9L+APE1TRscGysAVMANkB0jh0LQZrQ==", "license": "MIT", "dependencies": { "@isaacs/balanced-match": "^4.0.1" @@ -16164,7 +16164,7 @@ "dev": true, "license": "BlueOak-1.0.0", "dependencies": { - "@isaacs/brace-expansion": "^5.0.0" + "@isaacs/brace-expansion": "^5.0.1" }, "engines": { "node": "20 || >=22" @@ -17198,12 +17198,12 @@ } }, "node_modules/glob/node_modules/minimatch": { - "version": "10.1.1", - "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.1.1.tgz", - "integrity": "sha512-enIvLvRAFZYXJzkCYG5RKmPfrFArdLv+R+lbQ53BmIMLIry74bjKzX6iHAm8WYamJkhSSEabrWN5D97XnKObjQ==", + "version": "10.1.2", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.1.2.tgz", + "integrity": "sha512-fu656aJ0n2kcXwsnwnv9g24tkU5uSmOlTjd6WyyaKm2Z+h1qmY6bAjrcaIxF/BslFqbZ8UBtbJi7KgQOZD2PTw==", "license": "BlueOak-1.0.0", "dependencies": { - "@isaacs/brace-expansion": "^5.0.0" + "@isaacs/brace-expansion": "^5.0.1" }, "engines": { "node": "20 || >=22" @@ -23289,13 +23289,6 @@ "url": "https://github.com/chalk/ansi-styles?sponsor=1" } }, - "node_modules/pretty-format/node_modules/react-is": { - "version": "17.0.2", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-17.0.2.tgz", - "integrity": "sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==", - "dev": true, - "license": "MIT" - }, "node_modules/process": { "version": "0.11.10", "resolved": "https://registry.npmjs.org/process/-/process-0.11.10.tgz",