CCM-15562: Add campaignId check to update routing config by m-salaudeen · Pull Request #899 · NHSDigital/nhs-notify-web-template-management

m-salaudeen · 2026-04-08T09:42:16Z

Description

Context

Type of changes

Refactoring (non-breaking change)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would change existing functionality)
Bug fix (non-breaking change which fixes an issue)

Checklist

I am familiar with the contributing guidelines
I have followed the code style of the project
I have added tests to cover my changes
I have updated the documentation accordingly
This PR is a result of pair or mob programming

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

…_campaign_id_api

alexnuttall · 2026-04-08T10:32:03Z

lambdas/backend-api/src/app/routing-config-client.ts

+    // get the letter templateIds from the validated and make sure they are the same type
+    const templateIds =
+      validated.cascade
+        ?.map((c) => c.conditionalTemplates?.map((t) => t.templateId))


you're missing the non-conditional templates, those would come from the 'defaultTemplateId' field

agree with @alexnuttall
only conditionalTemplates IDs are extracted here, we also need the defaultTemplateId on each cascade item
the repository layer already has an extractTemplateIds helper that handles both here

alexnuttall · 2026-04-08T10:38:21Z

lambdas/backend-api/src/app/routing-config-client.ts

+    // fetch all the templates of these templateIds
+    const templates = await Promise.all(
+      templateIds.map((templateId) =>
+        this.templateClient.getTemplate(templateId as string, user)


I think using BatchGetItem would be better than many concurrent individual get requests

alexnuttall · 2026-04-08T10:39:46Z

lambdas/backend-api/src/app/routing-config-client.ts

    private readonly routingConfigRepository: RoutingConfigRepository,
-    private readonly clientConfigRepository: ClientConfigRepository
+    private readonly clientConfigRepository: ClientConfigRepository,
+    private readonly templateClient: TemplateClient


you haven't updated the 'container' file which constructs this class, so this can't work currently.

I also think it might not be possible for the two repositories to mutually depend on each other?

agree, need to update here

alexnuttall · 2026-04-08T10:46:15Z

lambdas/backend-api/src/app/routing-config-client.ts

+      );
+    }
+
+    // if the payload (validateResult) has a campaignId, check it matches with the template campaignId


Since we're blocking all updates to a routing config's campaign ID soon, it might not be worth us implementing stuff specific to handling updates which combine campaignId and templates in the payload. What do you think @nicki-nhs ?

If we are handling those cases, this doesn't seem correct. If there is a new campaignId in the payload, we can't expect the existing database entry to have a matching ID already. The expect condition should only be applied if the payload is not updating the campaignId

I agree
we should just be fetching the routing config from the ID and using the campaign ID on that to validate against

I don't think there's necesarily any need to fetch the routing config. A conditional update is sufficient - it's just the (current) support for patching campaign which makes things more complicated

Then again the code might be a little simpler with pre-fetching, at a perfomance cost

@nicki-nhs since we're already coverting conditional update errors into failure codes I don't see the problem with adding 'campaign mismatch' there?

yeah could do, what are you thinking the condition would be that we'd be able to use to differentiate it?
I think its equally valid in this file too, with the other campaign ID validation on L137+
but dont feel strongly either way

You would need to pass through expectedCampaign into handleError, similarly to expectedLockNumber.

There would still be a need to return a similar error from the client layer, because you need to handle a patch payload where some template campaigns don't match others.

poss a monday discussion but I think I dont understand the need to catch that specifically tho, if they dont match the routing config, its irrelevant if they match each other isnt it?

You need select one to use as the condition, if there are multiple, you already know the update is invalid

nicki-nhs · 2026-04-10T15:31:16Z

lambdas/backend-api/src/app/routing-config-client.ts

    private readonly routingConfigRepository: RoutingConfigRepository,
-    private readonly clientConfigRepository: ClientConfigRepository
+    private readonly clientConfigRepository: ClientConfigRepository,
+    private readonly templateClient: TemplateClient


should be using TemplateRepository rather than the client

nicki-nhs · 2026-04-10T15:35:48Z

lambdas/backend-api/src/app/routing-config-client.ts

+    // check all the templates have campaignIds if they don't have campaignIds, reject the request
+    if (templates.some((template) => !template.data?.campaignId)) {
+      return failure(
+        ErrorCase.VALIDATION_FAILED,
+        'One or more templates do not have a campaign ID'
+      );
+    }
+
+    // if they do check that those campaignIds are the same if not reject the request
+    const uniqueCampaignIds = [
+      ...new Set(templates.map((t) => t.data?.campaignId)),
+    ];
+    if (uniqueCampaignIds.length > 1) {
+      return failure(
+        ErrorCase.VALIDATION_FAILED,
+        'All templates must have the same campaign ID'
+      );
+    }


I'm happy to discuss this but I'm not sure we need these additional checks, given if we check if each one matches the campaign ID on the routing config, the case where its missing or they dont all match each other would be caught anyway. feels like a lot of code to add for not much benefit imo

nicki-nhs · 2026-04-10T15:37:47Z

lambdas/backend-api/src/app/routing-config-client.ts

+    const expectedCampaignId =
+      validated.campaignId === templates[0]?.data?.campaignId
+        ? validated.campaignId
+        : templates[0]?.data?.campaignId;


the source of truth for the expected campaignId should be the routing config already in the database, not the templates being added. the approach should be: fetch the existing routing config first, get its campaignId then validate that the templates match it

nicki-nhs · 2026-04-10T15:39:02Z

lambdas/backend-api/src/app/routing-config-client.ts

      user,
-      lockNumberValidation.data
+      lockNumberValidation.data,
+      expectedCampaignId as string


I dont think we want this

nicki-nhs · 2026-04-10T15:39:26Z

lambdas/backend-api/src/__tests__/app/routing-config-client.test.ts

  UpdateRoutingConfig,
 } from 'nhs-notify-web-template-management-types';
 import { ClientConfigRepository } from '../../infra/client-config-repository';
+import { TemplateClient } from '@backend-api/app/template-client';


needs updating to be the repository
and tests missing in here

nicki-nhs · 2026-04-10T15:40:14Z

lambdas/backend-api/src/infra/routing-config-repository/repository.ts

+    } else {
+      update.expectCampaignId(expectedCampaignId);


shouldnt need this, can revert this file

nicki-nhs · 2026-04-10T15:40:26Z

utils/entity-update-command-builder/src/routing-config-update-builder.ts

+  expectCampaignId(expectedCampaignId: string) {
+    this.updateBuilder.conditions.and('campaignId', '=', expectedCampaignId);
+    return this;
+  }
+


can revert this, shouldnt be needed

CCM-15562: Add campaignId check to update routing config

9c456c1

m-salaudeen self-assigned this Apr 8, 2026

m-salaudeen added the skip-trivy-package Skip the Trivy Package Scan label Apr 8, 2026

Merge remote-tracking branch 'origin' into feature/CCM-15562_matching…

60525e9

…_campaign_id_api

alexnuttall reviewed Apr 8, 2026

View reviewed changes

nicki-nhs reviewed Apr 10, 2026

View reviewed changes

Conversation

m-salaudeen commented Apr 8, 2026

Description

Context

Type of changes

Checklist

Sensitive Information Declaration

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexnuttall Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexnuttall Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexnuttall Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexnuttall Apr 8, 2026 •

edited

Loading

alexnuttall Apr 8, 2026 •

edited

Loading

alexnuttall Apr 10, 2026 •

edited

Loading