Skip to content

Commit b7ffba3

Browse files
alexnuttallalexnuttall
authored
CCM-12045: event pub error fix (#670)
1 parent 9a23c16 commit b7ffba3

File tree

8 files changed

+328
-55
lines changed

8 files changed

+328
-55
lines changed

‎lambdas/event-publisher/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
"esbuild": "^0.25.9",
1818
"jest": "^29.7.0",
1919
"jest-mock-extended": "^3.0.7",
20+
"nhs-notify-web-template-management-test-helper-utils": "^0.0.1",
2021
"typescript": "^5.8.2"
2122
},
2223
"name": "nhs-notify-templates-event-publisher",

‎lambdas/event-publisher/src/__tests__/domain/event-builder.test.ts‎

Lines changed: 140 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import { mockDeep } from 'jest-mock-extended';
21
import { EventBuilder } from '../../domain/event-builder';
3-
import { Logger } from 'nhs-notify-web-template-management-utils/logger';
2+
import { createMockLogger } from 'nhs-notify-web-template-management-test-helper-utils/mock-logger';
43
import { PublishableEventRecord } from '../../domain/input-schemas';
54
import { shouldPublish } from '../../domain/should-publish';
65

@@ -18,11 +17,11 @@ beforeEach(() => {
1817

1918
const shouldPublishMock = jest.mocked(shouldPublish);
2019

21-
const mockLogger = mockDeep<Logger>();
20+
const { logger: mockLogger } = createMockLogger();
2221

2322
const eventBuilder = new EventBuilder('table-name', 'event-source', mockLogger);
2423

25-
const publishableEventRecord = (status: string): PublishableEventRecord => ({
24+
const publishableEventRecord = (newStatus: string): PublishableEventRecord => ({
2625
dynamodb: {
2726
SequenceNumber: '4',
2827
NewImage: {
@@ -45,7 +44,89 @@ const publishableEventRecord = (status: string): PublishableEventRecord => ({
4544
S: 'name',
4645
},
4746
templateStatus: {
48-
S: status,
47+
S: newStatus,
48+
},
49+
updatedAt: {
50+
S: 'updated-at',
51+
},
52+
updatedBy: {
53+
S: 'updated-by',
54+
},
55+
templateType: {
56+
S: 'LETTER',
57+
},
58+
language: {
59+
S: 'fr',
60+
},
61+
letterType: {
62+
S: 'x0',
63+
},
64+
proofingEnabled: {
65+
BOOL: true,
66+
},
67+
files: {
68+
M: {
69+
pdfTemplate: {
70+
M: {
71+
currentVersion: {
72+
S: 'current-version',
73+
},
74+
fileName: {
75+
S: 'file-name',
76+
},
77+
virusScanStatus: {
78+
S: 'PASSED',
79+
},
80+
},
81+
},
82+
proofs: {
83+
M: {
84+
proof1: {
85+
M: {
86+
supplier: {
87+
S: 'WTMMOCK',
88+
},
89+
fileName: {
90+
S: 'file-name',
91+
},
92+
virusScanStatus: {
93+
S: 'PASSED',
94+
},
95+
},
96+
},
97+
},
98+
},
99+
},
100+
},
101+
personalisationParameters: {
102+
L: [
103+
{
104+
S: 'test',
105+
},
106+
],
107+
},
108+
},
109+
OldImage: {
110+
owner: {
111+
S: 'owner',
112+
},
113+
id: {
114+
S: 'id',
115+
},
116+
clientId: {
117+
S: 'client-id',
118+
},
119+
createdAt: {
120+
S: 'created-at',
121+
},
122+
createdBy: {
123+
S: 'created-by',
124+
},
125+
name: {
126+
S: 'name',
127+
},
128+
templateStatus: {
129+
S: 'PENDING_PROOF_REQUEST',
49130
},
50131
updatedAt: {
51132
S: 'updated-at',
@@ -158,6 +239,37 @@ test('errors on unrecognised event type', () => {
158239
);
159240
});
160241

242+
test('errors on output schema validation failure', () => {
243+
const valid = publishableEventRecord('SUBMITTED');
244+
245+
const invalidDomainEventRecord = {
246+
...valid,
247+
dynamodb: {
248+
...valid.dynamodb,
249+
NewImage: {
250+
...valid.dynamodb.NewImage,
251+
language: { N: 0 },
252+
},
253+
},
254+
};
255+
256+
expect(() =>
257+
eventBuilder.buildEvent(
258+
invalidDomainEventRecord as unknown as PublishableEventRecord
259+
)
260+
).toThrow(
261+
expect.objectContaining({
262+
name: 'ZodError',
263+
issues: [
264+
expect.objectContaining({
265+
code: 'invalid_value',
266+
path: ['data', 'language'],
267+
}),
268+
],
269+
})
270+
);
271+
});
272+
161273
test('builds template completed event', () => {
162274
const event = eventBuilder.buildEvent(publishableEventRecord('SUBMITTED'));
163275

@@ -184,6 +296,29 @@ test('builds template drafted event', () => {
184296
);
185297
});
186298

299+
test('builds event when no old image is available', () => {
300+
// although not required by this lambda, an old image would be expected here in real usage
301+
const mockEvent = publishableEventRecord('SUBMITTED');
302+
303+
const noOldImage = {
304+
...mockEvent,
305+
dynamodb: {
306+
SequenceNumber: mockEvent.dynamodb.SequenceNumber,
307+
NewImage: mockEvent.dynamodb.NewImage,
308+
},
309+
};
310+
311+
const event = eventBuilder.buildEvent(noOldImage);
312+
313+
expect(event).toEqual(
314+
expectedEvent(
315+
'SUBMITTED',
316+
'uk.nhs.notify.template-management.TemplateCompleted.v1',
317+
'https://notify.nhs.uk/events/schemas/TemplateCompleted/v1.json'
318+
)
319+
);
320+
});
321+
187322
test('builds template deleted event', () => {
188323
const event = eventBuilder.buildEvent(publishableEventRecord('DELETED'));
189324

‎lambdas/event-publisher/src/__tests__/domain/should-publish.test.ts‎

Lines changed: 93 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,50 +1,117 @@
1-
import {
2-
LetterProperties,
3-
TemplateDto,
4-
TemplateStatus,
5-
TemplateType,
6-
} from 'nhs-notify-backend-client';
1+
import { TemplateStatus, TemplateType } from 'nhs-notify-backend-client';
72
import { shouldPublish } from '../../domain/should-publish';
83

94
describe('shouldPublish', () => {
105
test.each(['EMAIL', 'SMS', 'NHS_APP'] satisfies TemplateType[])(
116
'templateType %p should return true',
127
(type) => {
13-
expect(shouldPublish({ templateType: type } as TemplateDto)).toEqual(
14-
true
8+
const publish = shouldPublish(
9+
{
10+
templateType: type,
11+
id: 'id',
12+
templateStatus: 'NOT_YET_SUBMITTED',
13+
},
14+
{
15+
templateType: type,
16+
id: 'id',
17+
templateStatus: 'SUBMITTED',
18+
}
1519
);
20+
21+
expect(publish).toEqual(true);
1622
}
1723
);
1824

1925
test.each([false, undefined])(
20-
'templateType LETTER should return false when proofingEnabled %p',
26+
'templateType LETTER should return false when proofingEnabled is %p',
2127
(proofingEnabled) => {
22-
expect(
23-
shouldPublish({
28+
const publish = shouldPublish(
29+
{
30+
id: 'id',
31+
templateType: 'LETTER',
32+
templateStatus: 'PROOF_AVAILABLE',
33+
proofingEnabled,
34+
},
35+
{
36+
id: 'id',
2437
templateType: 'LETTER',
2538
templateStatus: 'SUBMITTED',
2639
proofingEnabled,
27-
} as TemplateDto & LetterProperties)
28-
).toEqual(false);
40+
}
41+
);
42+
43+
expect(publish).toEqual(false);
2944
}
3045
);
3146

32-
test.each([
33-
'DELETED',
34-
'PENDING_PROOF_REQUEST',
35-
'PROOF_AVAILABLE',
36-
'SUBMITTED',
37-
'WAITING_FOR_PROOF',
38-
] satisfies TemplateStatus[])(
39-
'templateType LETTER should return true when templateStatus is %p and proofingEnabled is true',
40-
(templateStatus) => {
41-
expect(
42-
shouldPublish({
47+
type LetterStatus = Exclude<TemplateStatus, 'NOT_YET_SUBMITTED'>;
48+
49+
const letterPublishCases: Record<LetterStatus, boolean> = {
50+
DELETED: true,
51+
PENDING_PROOF_REQUEST: true,
52+
PROOF_AVAILABLE: true,
53+
SUBMITTED: true,
54+
WAITING_FOR_PROOF: true,
55+
PENDING_UPLOAD: false,
56+
PENDING_VALIDATION: false,
57+
VALIDATION_FAILED: false,
58+
VIRUS_SCAN_FAILED: false,
59+
};
60+
61+
// not all of these transitions are expected in real usage
62+
test.each(Object.entries(letterPublishCases) as [TemplateStatus, boolean][])(
63+
'templateType LETTER with current status %p should return %p when proofingEnabled is true and status of previous is not restrictive',
64+
(templateStatus, publishable) => {
65+
const publish = shouldPublish(
66+
{
67+
id: 'id',
68+
templateType: 'LETTER',
69+
templateStatus: 'PENDING_PROOF_REQUEST',
70+
proofingEnabled: true,
71+
},
72+
{
73+
id: 'id',
4374
templateType: 'LETTER',
4475
templateStatus,
4576
proofingEnabled: true,
46-
} as TemplateDto & LetterProperties)
47-
).toEqual(true);
77+
}
78+
);
79+
80+
expect(publish).toEqual(publishable);
4881
}
4982
);
83+
84+
// not all of these transitions are expected in real usage
85+
test.each(Object.entries(letterPublishCases) as [TemplateStatus, boolean][])(
86+
'templateType LETTER with new status DELETED and previous status %p should return %p when proofingEnabled is true',
87+
(templateStatus, publishable) => {
88+
const publish = shouldPublish(
89+
{
90+
id: 'id',
91+
templateType: 'LETTER',
92+
templateStatus,
93+
proofingEnabled: true,
94+
},
95+
{
96+
id: 'id',
97+
templateType: 'LETTER',
98+
templateStatus: 'DELETED',
99+
proofingEnabled: true,
100+
}
101+
);
102+
103+
expect(publish).toEqual(publishable);
104+
}
105+
);
106+
107+
test('does not publish if templateType is LETTER and no previous template is available', () => {
108+
expect(
109+
shouldPublish(undefined, {
110+
id: 'id',
111+
templateType: 'LETTER',
112+
templateStatus: 'DELETED',
113+
proofingEnabled: true,
114+
})
115+
).toEqual(false);
116+
});
50117
});

‎lambdas/event-publisher/src/domain/event-builder.ts‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -65,12 +65,19 @@ export class EventBuilder {
6565

6666
return undefined;
6767
}
68-
69-
const dynamoRecord = unmarshall(publishableEventRecord.dynamodb.NewImage);
70-
71-
const databaseTemplate = $DynamoDBTemplate.parse(dynamoRecord);
72-
73-
if (!shouldPublish(databaseTemplate)) {
68+
const dynamoRecordNew = unmarshall(
69+
publishableEventRecord.dynamodb.NewImage
70+
);
71+
const dynamoRecordOld = publishableEventRecord.dynamodb.OldImage
72+
? unmarshall(publishableEventRecord.dynamodb.OldImage)
73+
: undefined;
74+
75+
const databaseTemplateNew = $DynamoDBTemplate.parse(dynamoRecordNew);
76+
const databaseTemplateOld = $DynamoDBTemplate
77+
.optional()
78+
.parse(dynamoRecordOld);
79+
80+
if (!shouldPublish(databaseTemplateOld, databaseTemplateNew)) {
7481
this.logger.debug({
7582
description: 'Not publishing event',
7683
publishableEventRecord,
@@ -79,14 +86,25 @@ export class EventBuilder {
7986
return undefined;
8087
}
8188

82-
return $Event.parse({
83-
...this.buildTemplateSavedEventMetadata(
84-
publishableEventRecord.eventID,
85-
databaseTemplate.templateStatus,
86-
databaseTemplate.id
87-
),
88-
data: dynamoRecord,
89-
});
89+
try {
90+
return $Event.parse({
91+
...this.buildTemplateSavedEventMetadata(
92+
publishableEventRecord.eventID,
93+
databaseTemplateNew.templateStatus,
94+
databaseTemplateNew.id
95+
),
96+
data: dynamoRecordNew,
97+
});
98+
} catch (error) {
99+
this.logger
100+
.child({
101+
description: 'Failed to parse outgoing event',
102+
publishableEventRecord,
103+
})
104+
.error(error);
105+
106+
throw error;
107+
}
90108
}
91109

92110
buildEvent(

0 commit comments

Comments
 (0)