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 7525052cd..3e0b1de0c 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 @@ -770,6 +770,8 @@ describe('RoutingConfigClient', () => { test('returns updated routing config', async () => { const { client, mocks } = setup(); + const campaignId = 'campaign-1'; + const update: UpdateRoutingConfig = { cascade: [ ...routingConfig.cascade, @@ -801,6 +803,10 @@ describe('RoutingConfigClient', () => { }, }); + mocks.routingConfigRepository.get.mockResolvedValueOnce({ + data: routingConfig, + }); + mocks.routingConfigRepository.update.mockResolvedValueOnce({ data: updated, }); @@ -816,7 +822,8 @@ describe('RoutingConfigClient', () => { routingConfig.id, update, user, - 42 + 42, + campaignId ); expect(result).toEqual({ @@ -956,6 +963,10 @@ describe('RoutingConfigClient', () => { }, }); + mocks.routingConfigRepository.get.mockResolvedValueOnce({ + data: routingConfig, + }); + const result = await client.updateRoutingConfig( routingConfig.id, update, @@ -1010,6 +1021,51 @@ describe('RoutingConfigClient', () => { }, }); }); + + test('returns routingConfig error without updating when routing config fetch fails', async () => { + const { client, mocks } = setup(); + + const update = { + cascade: routingConfig.cascade, + cascadeGroupOverrides: routingConfig.cascadeGroupOverrides, + name: routingConfig.name, + } as unknown as UpdateRoutingConfig; + + mocks.clientConfigRepository.get.mockResolvedValueOnce({ + data: { + features: { routing: true }, + campaignIds: [routingConfig.campaignId], + }, + }); + + mocks.routingConfigRepository.get.mockResolvedValueOnce({ + error: { + actualError: new Error('Routing config not found'), + errorMeta: { + code: 404, + description: 'Routing config not found', + }, + }, + }); + + const result = await client.updateRoutingConfig( + routingConfig.id, + update, + user, + '42' + ); + + expect(mocks.routingConfigRepository.update).not.toHaveBeenCalled(); + expect(result).toEqual({ + error: { + actualError: new Error('Routing config not found'), + errorMeta: { + code: 404, + description: 'Routing config not found', + }, + }, + }); + }); }); describe('getRoutingConfigsByTemplateId', () => { 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 e5f1d3e2f..abc0bd3ca 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 @@ -1614,7 +1614,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -1662,7 +1668,14 @@ describe('RoutingConfigRepository', () => { id: '90e46ece-4a3b-47bd-b781-f986b42a5a09', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, ], @@ -1692,7 +1705,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -1764,7 +1783,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -1810,7 +1835,14 @@ describe('RoutingConfigRepository', () => { id: 'c003b4f1-d788-423d-a948-0df511d07a23', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, ], @@ -1863,7 +1895,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -1909,7 +1947,14 @@ describe('RoutingConfigRepository', () => { id: 'default-template-id', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, { @@ -1919,7 +1964,14 @@ describe('RoutingConfigRepository', () => { id: 'conditional-template-1', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, { @@ -1929,7 +1981,14 @@ describe('RoutingConfigRepository', () => { id: 'conditional-template-2', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, { @@ -1939,7 +1998,14 @@ describe('RoutingConfigRepository', () => { id: 'accessible-template-1', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, ], @@ -1977,7 +2043,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -2021,7 +2093,14 @@ describe('RoutingConfigRepository', () => { id: 'c003b4f1-d788-423d-a948-0df511d07a23', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, ], @@ -2052,7 +2131,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: updated }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: updated }); @@ -2114,7 +2199,13 @@ describe('RoutingConfigRepository', () => { mocks.dynamo.on(TransactWriteCommand).resolves({}); mocks.dynamo.on(GetCommand).resolves({ Item: routingConfig }); - const result = await repo.update(routingConfig.id, update, user, 2); + const result = await repo.update( + routingConfig.id, + update, + user, + 2, + routingConfig.campaignId + ); expect(result).toEqual({ data: routingConfig }); @@ -2162,7 +2253,14 @@ describe('RoutingConfigRepository', () => { id: '90e46ece-4a3b-47bd-b781-f986b42a5a09', owner: clientOwnerKey, }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': routingConfig.campaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, }, ], @@ -2192,7 +2290,8 @@ describe('RoutingConfigRepository', () => { name: routingConfig.name, }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2225,7 +2324,8 @@ describe('RoutingConfigRepository', () => { name: routingConfig.name, }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2269,7 +2369,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new-name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2302,7 +2403,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new-name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2335,7 +2437,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2372,7 +2475,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new-name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2420,7 +2524,8 @@ describe('RoutingConfigRepository', () => { cascadeGroupOverrides: [], }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2435,6 +2540,75 @@ describe('RoutingConfigRepository', () => { }); }); + test('returns 400 failure if some templates are invalid', async () => { + const { repo, mocks } = setup(); + + const err = new TransactionCanceledException({ + CancellationReasons: [ + { + Code: 'None', // Update succeeded + }, + { + Code: 'ConditionalCheckFailed', + Item: { id: { S: 'template1' } }, // Invalid template + }, + { + Code: 'ProvisionedThroughputExceeded', // should be ignored + }, + ], + $metadata: {}, + message: 'Transaction cancelled', + }); + + mocks.dynamo.on(TransactWriteCommand).rejects(err); + + const result = await repo.update( + routingConfig.id, + { + name: 'new-name', + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'SMS', + channelType: 'primary', + defaultTemplateId: 'template1', + }, + { + cascadeGroups: ['accessible'], + channel: 'EMAIL', + channelType: 'secondary', + conditionalTemplates: [ + { templateId: 'template2', accessibleFormat: 'x1' }, + ], + }, + { + cascadeGroups: ['translations'], + channel: 'NHSAPP', + channelType: 'primary', + conditionalTemplates: [ + { templateId: 'template3', language: 'ar' }, + ], + }, + ], + cascadeGroupOverrides: [], + }, + user, + 2, + routingConfig.campaignId + ); + + expect(result).toEqual({ + error: { + actualError: err, + errorMeta: { + code: 400, + description: 'Some templates failed validation', + details: { templateIds: 'template1' }, + }, + }, + }); + }); + test('returns 500 failure if GetCommand fails after TransactWriteCommand succeeds', async () => { const { repo, mocks } = setup(); @@ -2447,7 +2621,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new-name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ @@ -2476,7 +2651,8 @@ describe('RoutingConfigRepository', () => { routingConfig.id, { name: 'new-name' }, user, - 2 + 2, + routingConfig.campaignId ); expect(result).toEqual({ diff --git a/lambdas/backend-api/src/app/routing-config-client.ts b/lambdas/backend-api/src/app/routing-config-client.ts index 04a8fc1b2..1c559f2f3 100644 --- a/lambdas/backend-api/src/app/routing-config-client.ts +++ b/lambdas/backend-api/src/app/routing-config-client.ts @@ -91,11 +91,17 @@ export class RoutingConfigClient { if (validationResult.error) return validationResult; + // Fetch routing config + const routingConfig = await this.getRoutingConfig(routingConfigId, user); + + if (routingConfig.error) return routingConfig; + return this.routingConfigRepository.update( routingConfigId, validationResult.data, user, - lockNumberValidation.data + lockNumberValidation.data, + routingConfig.data.campaignId ); } 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 c9ed22ec5..f00c7e95e 100644 --- a/lambdas/backend-api/src/infra/routing-config-repository/repository.ts +++ b/lambdas/backend-api/src/infra/routing-config-repository/repository.ts @@ -97,7 +97,8 @@ export class RoutingConfigRepository { id: string, updateData: UpdateRoutingConfig, user: User, - lockNumber: number + lockNumber: number, + expectedCampaignId: string ): Promise> { const { cascade, cascadeGroupOverrides, name } = updateData; @@ -139,7 +140,14 @@ export class RoutingConfigRepository { id: templateId, owner: this.clientOwnerKey(user.clientId), }, - ConditionExpression: 'attribute_exists(id)', + ConditionExpression: + 'attribute_exists(id) AND (templateType <> :letter OR campaignId = :expectedCampaignId)', + ExpressionAttributeValues: { + ':expectedCampaignId': expectedCampaignId, + ':letter': 'LETTER', + }, + ReturnValuesOnConditionCheckFailure: + ReturnValuesOnConditionCheckFailure.ALL_OLD, }, })), ], @@ -558,16 +566,32 @@ export class RoutingConfigRepository { ); } - const templatesMissing = templateReasons.some( - (r) => r.Code === 'ConditionalCheckFailed' - ); + const notFoundTemplateIds: string[] = []; + const invalidTemplateIds: string[] = []; - if (templatesMissing) { + for (const [index, reason] of templateReasons.entries()) { + if (reason?.Code !== 'ConditionalCheckFailed') continue; + const templateId = templateIds[index]; + if (reason.Item) { + invalidTemplateIds.push(templateId); + } else { + notFoundTemplateIds.push(templateId); + } + } + if (notFoundTemplateIds.length > 0) { return failure( ErrorCase.ROUTING_CONFIG_TEMPLATES_NOT_FOUND, 'Some templates not found', err, - { templateIds: templateIds.join(',') } + { templateIds: notFoundTemplateIds.join(',') } + ); + } + if (invalidTemplateIds.length > 0) { + return failure( + ErrorCase.ROUTING_CONFIG_TEMPLATES_INVALID, + 'Some templates failed validation', + err, + { templateIds: invalidTemplateIds.join(',') } ); } 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 86a60cc7b..eb06639ad 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 @@ -675,4 +675,141 @@ test.describe('PATCH /v1/routing-configuration/:routingConfigId', () => { 'Lock number mismatch - Routing configuration has been modified since last read', }); }); + + test('update fails when adding template with a different campaignId to the routing campaignId', async ({ + request, + }) => { + const { dbEntry } = RoutingConfigFactory.create(userMultiCampaign, { + campaignId: 'campaign-id', + }); + const letterTemplate = TemplateFactory.createAuthoringLetterTemplate( + randomUUID(), + userMultiCampaign, + 'Plan campaign mismatch template', + 'PROOF_APPROVED', + { + letterVariantId: 'variant', + longFormRender: { status: 'RENDERED' }, + shortFormRender: { status: 'RENDERED' }, + campaignId: 'other-campaign-id', + } + ); + + await storageHelper.seed([dbEntry]); + await templateStorageHelper.seedTemplateData([letterTemplate]); + + const update = { + cascade: [ + { + cascadeGroups: ['accessible'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: letterTemplate.id, + }, + ], + cascadeGroupOverrides: [], + }; + + const updateResponse = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}`, + { + headers: { + Authorization: await userMultiCampaign.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + data: update, + } + ); + + expect(updateResponse.status()).toBe(400); + + const updated = await updateResponse.json(); + + expect(updated).toEqual({ + statusCode: 400, + technicalMessage: 'Some templates failed validation', + details: { + templateIds: letterTemplate.id, + }, + }); + }); + + test('update fails when adding multiple templates where at least one template does not belong to the routing config campaign', async ({ + request, + }) => { + const { dbEntry } = RoutingConfigFactory.create(userMultiCampaign, { + campaignId: 'campaign-id', + }); + const letterTemplate1 = TemplateFactory.createAuthoringLetterTemplate( + randomUUID(), + userMultiCampaign, + 'Plan campaign mismatch template - matching campaign', + 'PROOF_APPROVED', + { + letterVariantId: 'variant', + longFormRender: { status: 'RENDERED' }, + shortFormRender: { status: 'RENDERED' }, + campaignId: 'campaign-id', + } + ); + const letterTemplate2 = TemplateFactory.createAuthoringLetterTemplate( + randomUUID(), + userMultiCampaign, + 'Plan campaign mismatch template - different campaign', + 'PROOF_APPROVED', + { + letterVariantId: 'variant', + longFormRender: { status: 'RENDERED' }, + shortFormRender: { status: 'RENDERED' }, + campaignId: 'other-campaign-id', + } + ); + + await storageHelper.seed([dbEntry]); + await templateStorageHelper.seedTemplateData([ + letterTemplate1, + letterTemplate2, + ]); + + const update = { + cascade: [ + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'primary', + defaultTemplateId: letterTemplate1.id, + }, + { + cascadeGroups: ['standard'], + channel: 'LETTER', + channelType: 'secondary', + defaultTemplateId: letterTemplate2.id, + }, + ], + cascadeGroupOverrides: [], + }; + + const updateResponse = await request.patch( + `${process.env.API_BASE_URL}/v1/routing-configuration/${dbEntry.id}`, + { + headers: { + Authorization: await userMultiCampaign.getAccessToken(), + 'X-Lock-Number': String(dbEntry.lockNumber), + }, + data: update, + } + ); + + expect(updateResponse.status()).toBe(400); + + const updated = await updateResponse.json(); + + expect(updated).toEqual({ + statusCode: 400, + technicalMessage: 'Some templates failed validation', + details: { + templateIds: letterTemplate2.id, + }, + }); + }); }); diff --git a/tests/test-team/template-mgmt-routing-component-tests/letter/preview-british-sign-language-letter-template.routing-component.spec.ts b/tests/test-team/template-mgmt-routing-component-tests/letter/preview-british-sign-language-letter-template.routing-component.spec.ts index ec8ea410d..e68276678 100644 --- a/tests/test-team/template-mgmt-routing-component-tests/letter/preview-british-sign-language-letter-template.routing-component.spec.ts +++ b/tests/test-team/template-mgmt-routing-component-tests/letter/preview-british-sign-language-letter-template.routing-component.spec.ts @@ -78,6 +78,7 @@ function createTemplates(user: TestUser, letterVariant: LetterVariant) { letterType: 'q4', shortFormRender: { status: 'RENDERED' }, longFormRender: { status: 'RENDERED' }, + letterVariantId: letterVariant.id, } ), };