-
Notifications
You must be signed in to change notification settings - Fork 3
CCM-13475: Send print events #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
61703dc to
d9a4b1a
Compare
16dabad to
d9a4b1a
Compare
| event_pattern = jsonencode({ | ||
| "detail" : { | ||
| "type" : [ | ||
| "uk.nhs.notify.letter-rendering.letter-request.prepared.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, in the confluence page https://nhsd-confluence.digital.nhs.uk/spaces/RIS/pages/1196005217/REFCOM-2025-079+%F0%9F%9F%A2+EVENT+letter-rendering.letter-request.PREPARED.v1 at the top of the page it appears as:
uk.nhs.notify.letter-rendering.letter-request.prepared.v1
where as the other 4 references are with PREPARED i.e.
uk.nhs.notify.letter-rendering.letter-request.PREPARED.v1
I presume is the way you got it, but just double checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! The event validator supplied by the supplier API event package requires lower case otherwise it fails validation. That's why I went with prepared I'm not 100% sure that's correct.
| ] | ||
|
|
||
| resources = [ | ||
| "arn:aws:sqs:${var.region}:${var.aws_account_id}:${var.project}-${var.environment}-${local.component}-print-sender-queue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny one, is it worth using ${local.csi} instead of ${var.project}-${var.environment}-${local.component}? from the file terraform/components/dl/locals_tfscaffold.tf:
csi = replace(
format(
"%s-%s-%s",
var.project,
var.environment,
local.component,
),
"_",
"",
)
a35e9f1 to
4ba840b
Compare
|
|
||
| kms_key_arn = module.kms.key_arn | ||
| log_retention_in_days = var.log_retention_in_days | ||
| log_level = "INFO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the variable "log_level" to define the log level for the lambdas, do we want another variable for the evenpub? or do we want to reuse it as we do in "log_retention_in_days"?
I presume for Prod more likely we use INFO level, but probably is a question for infra whether we need a new variable or we're ok to leave it as INFO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use the "log_level" variable. I'll make the change, thanks!
| status: 'PREPARED', | ||
| url: item.data.letterUri, | ||
| clientId: item.data.senderId, | ||
| letterVariantId: 'notify-digital-letter-standard', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://nhsd-jira.digital.nhs.uk/browse/CCM-13475, do we need the campaignId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
| import expectToPassEventually from 'helpers/expectations'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| test.describe('Digital Letters - Print Sender', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- do we want to add a test to cover the scenario of messages going to the DLQ?
- I've noticed we have in terraform modules_eventpub.tf, is that the way that we expose our event to the digital letters API? If so, just wondering if the test should have an extra assert by subscribing to an SNS topic and getting the message
| module "eventpub" { | ||
| source = "https://github.com/NHSDigital/nhs-notify-shared-modules/releases/download/v2.0.30/terraform-eventpub.zip" | ||
|
|
||
| name = "eventpub" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this how we expose the events to the digital letters API? just wondering if the name should be a bit more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file need changing, if we've not touched anything else in the docs directory?
| @@ -0,0 +1,25 @@ | |||
| { | |||
| "dependencies": { | |||
| "@aws-sdk/lib-dynamodb": "^3.908.0", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency isn't used (we're not using DynamoDB in this lambda!).
| it('should return failed when event validation fails', async () => { | ||
| mockEventPublisher.sendEvents.mockRejectedValue( | ||
| new Error('Validation error'), | ||
| ); | ||
|
|
||
| const result = await printSender.send(mockPDFAnalysed); | ||
|
|
||
| expect(result).toBe('failed'); | ||
| expect(mockLogger.error).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is doing the same thing as should return failed when event publisher throws error. The only difference is the message of the Error.
Could you pass an invalid event into send to verify that it handles validation failures instead?
| it('should handle different sender IDs and message references', async () => { | ||
| const customInput: PDFAnalysed = { | ||
| ...mockPDFAnalysed, | ||
| data: { | ||
| ...mockPDFAnalysed.data, | ||
| senderId: 'custom-sender-999', | ||
| messageReference: 'custom-ref-abc', | ||
| }, | ||
| }; | ||
|
|
||
| await printSender.send(customInput); | ||
|
|
||
| const [[events]] = mockEventPublisher.sendEvents.mock.calls; | ||
| const event = events[0] as LetterRequestPreparedEvent; | ||
|
|
||
| expect(event.data.domainId).toBe('custom-sender-999_custom-ref-abc'); | ||
| expect(event.data.clientId).toBe('custom-sender-999'); | ||
| expect(event.data.requestItemPlanId).toBe('custom-ref-abc'); | ||
| expect(event.subject).toBe( | ||
| 'client/custom-sender-999/letter-request/custom-ref-abc', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this verify anything that should successfully send a letter prepared event doesn't?
| }, | ||
| ); | ||
|
|
||
| const results = await Promise.allSettled(promises); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All we do with the results array is check the length of it for the log statement, and given we always return a result for every record I think we could just use sqsEvent.Records.length instead, and remove the whole mechanism of returning a result from each promise.
| it('should process valid SQS messages successfully', async () => { | ||
| const validEvent = createValidEvent(); | ||
| const sqsEvent: SQSEvent = { | ||
| Records: [ | ||
| createSQSRecord(JSON.stringify({ detail: validEvent }), 'message-1'), | ||
| ], | ||
| }; | ||
|
|
||
| mockPrintSender.send.mockResolvedValue('sent'); | ||
|
|
||
| const result = await handler(sqsEvent); | ||
|
|
||
| expect(result.batchItemFailures).toEqual([]); | ||
| expect(mockPrintSender.send).toHaveBeenCalledWith(validEvent); | ||
| expect(mockLogger.info).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| description: 'Processed SQS Event.', | ||
| retrieved: 1, | ||
| sent: 1, | ||
| failed: 0, | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be nicer to have this happy path test send multiple records, as it's verifying the counts of items logged that should handle multiple messages in batch doesn't.
| it('should handle partial batch failures', async () => { | ||
| const event1 = createValidEvent({ id: 'id-1' }); | ||
| const event2 = createValidEvent({ id: 'id-2' }); | ||
| const sqsEvent: SQSEvent = { | ||
| Records: [ | ||
| createSQSRecord(JSON.stringify({ detail: event1 }), 'message-1'), | ||
| createSQSRecord(JSON.stringify({ detail: event2 }), 'message-2'), | ||
| ], | ||
| }; | ||
|
|
||
| mockPrintSender.send | ||
| .mockResolvedValueOnce('sent') | ||
| .mockResolvedValueOnce('failed'); | ||
|
|
||
| const result = await handler(sqsEvent); | ||
|
|
||
| expect(result.batchItemFailures).toEqual([{ itemIdentifier: 'message-2' }]); | ||
| expect(mockLogger.info).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| retrieved: 2, | ||
| sent: 1, | ||
| failed: 1, | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one would be good if it included more than one failing record, to verify we're building the batchItemFailures list correctly.
|
|
||
| resources = [ | ||
| "arn:aws:sqs:${var.region}:${var.aws_account_id}:${local.csi}-print-sender-queue" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a condition here in the same way as we do for module_sqd_print_analyser.tf:
| ] | |
| ] | |
| condition { | |
| test = "ArnEquals" | |
| variable = "aws:SourceArn" | |
| values = [aws_cloudwatch_event_rule.pdf_analysed.arn] | |
| } |
Description
Create new Print Sender component that triggers a Letter Prepared Event to be eventually passed to the Supplier API from a Letter Analysed Event.
Contains changes from #174 to pull packages from GitHub.
Note: When merging main, @nhsdigital/nhs-notify-event-schemas-supplier-api was updated to 1.0.10, which introduces a breaking change requiring billingRef in the events. As this change is not yet supported, the dependency has been temporarily pinned to 1.0.6.
Test evidence
Print Sender logs
Event Bus logs
Input Letter Analysed event + Output Letter Prepared event

Event is published to the cross domain event bus

Type of changes
Checklist
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.