[HOTE-977] feat: order confirmed notification#321
Conversation
There was a problem hiding this comment.
Pull request overview
Adds “order confirmed” notification publishing from the order-status-lambda, including new notify message payload structure and DB audit persistence to support downstream Notify integration.
Changes:
- Extend order status handling to accept
confirmedand enqueue an OrderConfirmed notify message to SQS. - Introduce notification audit DB service and wire it into
order-status-lambdainit/handler. - Update shared notify message types + Zod schema to match the new payload shape.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| local-environment/infra/main.tf | Grants lambda permission to publish to the notify SQS queue and injects required env vars locally. |
| lambdas/src/order-status-lambda/utils.ts | Adds CONFIRMED mapping for incoming → internal business status. |
| lambdas/src/order-status-lambda/types.ts | Adds CONFIRMED to allowed incoming/internal status enums. |
| lambdas/src/order-status-lambda/notify-message-builder.ts | Builds OrderConfirmed notify payload (date + tracking link). |
| lambdas/src/order-status-lambda/notify-message-builder.test.ts | Unit test for the notify payload builder. |
| lambdas/src/order-status-lambda/init.ts | Wires SQS client + notification audit DB + required env vars. |
| lambdas/src/order-status-lambda/init.test.ts | Extends init tests for new dependencies and required env vars. |
| lambdas/src/order-status-lambda/index.ts | On confirmed status, fetches notify details, publishes to SQS, and writes an audit record. |
| lambdas/src/order-status-lambda/index.test.ts | Adds handler tests for confirmed status notification/audit behavior and failure cases. |
| lambdas/src/lib/types/notify-message.ts | Refactors notify message shape (top-level nhsNumber, required personalisation). |
| lambdas/src/lib/types/notify-message-schema.ts | Updates Zod schema to validate the new notify message shape. |
| lambdas/src/lib/types/notify-message-schema.test.ts | Updates schema tests to match new payload requirements. |
| lambdas/src/lib/db/order-status-db.ts | Adds getNotifyOrderDetails query used by the lambda when confirmed. |
| lambdas/src/lib/db/order-status-db.test.ts | Unit tests for getNotifyOrderDetails. |
| lambdas/src/lib/db/notification-audit-db.ts | New DB service for inserting notification audit records. |
| lambdas/src/lib/db/notification-audit-db.test.ts | Unit tests for notification audit inserts and failure paths. |
| await orderStatusDb.addOrderStatusUpdate(statusOrderUpdateParams); | ||
|
|
||
| commons.logInfo(name, "Order status update added successfully", statusOrderUpdateParams); | ||
|
|
||
| await orderStatusNotifyService.handleOrderStatusUpdated({ | ||
| orderId, | ||
| patientId: orderPatientId, | ||
| correlationId, | ||
| statusUpdate: statusOrderUpdateParams, | ||
| }); |
There was a problem hiding this comment.
If notification publishing/auditing fails, the status update has already been persisted but the handler returns 500; a client retry with the same X-Correlation-ID will hit the idempotency short-circuit and never re-attempt the notification, so the confirmed notification can be dropped permanently. Decouple status persistence from notification side-effects (e.g., outbox/audit written transactionally with the status update and processed asynchronously) or ensure the duplicate-correlationId path still triggers notification retry when no successful (QUEUED/SENT) audit exists.
| await sqsClient.sendMessage(notifyMessagesQueueUrl, JSON.stringify(notifyMessage)); | ||
|
|
||
| await notificationAuditDbClient.insertNotificationAuditEntry({ | ||
| messageReference: notifyMessage.messageReference, | ||
| eventCode: notifyMessage.eventCode, | ||
| correlationId, | ||
| status: NotificationAuditStatus.QUEUED, | ||
| }); |
There was a problem hiding this comment.
SQS publish happens before inserting the QUEUED audit row, so if the DB insert fails after a successful send you can end up re-sending the same notification on retry (latest audit is FAILED/absent but the message is already on the queue). To make this idempotent, persist an audit/outbox record before publishing (or in the same DB transaction as the status update), and update/append status after publish so retries can safely detect that a message was already sent/queued.
| await sqsClient.sendMessage(notifyMessagesQueueUrl, JSON.stringify(notifyMessage)); | |
| await notificationAuditDbClient.insertNotificationAuditEntry({ | |
| messageReference: notifyMessage.messageReference, | |
| eventCode: notifyMessage.eventCode, | |
| correlationId, | |
| status: NotificationAuditStatus.QUEUED, | |
| }); | |
| await notificationAuditDbClient.insertNotificationAuditEntry({ | |
| messageReference: notifyMessage.messageReference, | |
| eventCode: notifyMessage.eventCode, | |
| correlationId, | |
| status: NotificationAuditStatus.QUEUED, | |
| }); | |
| await sqsClient.sendMessage(notifyMessagesQueueUrl, JSON.stringify(notifyMessage)); |
usmaana-nhs
force-pushed
the
feature/hote-977/order-confirmed-notification
branch
from
81b46c4 to
7c612fa
Compare
Lambdas Coverage Report
|
UI Coverage Report
|
| async getLatestAuditByCorrelationIdAndEventCode( | ||
| correlationId: string, | ||
| eventCode: NotifyEventCode, | ||
| ): Promise<NotificationAuditRecord | null> { | ||
| const query = ` | ||
| SELECT message_reference, event_code, correlation_id, status, created_at | ||
| FROM notification_audit | ||
| WHERE correlation_id = $1::uuid | ||
| AND event_code = $2 | ||
| ORDER BY created_at DESC | ||
| LIMIT 1 | ||
| `; |
There was a problem hiding this comment.
getLatestAuditByCorrelationIdAndEventCode queries by (correlation_id, event_code) and orders by created_at, but the notification_audit table migration only indexes (message_reference, created_at); without a supporting index this lookup will degrade as the audit table grows. Add a DB index aligned to this access pattern (e.g., on (correlation_id, event_code, created_at DESC) or similar) to keep the dedupe check efficient.
usmaana-nhs
force-pushed
the
feature/hote-977/order-confirmed-notification
branch
from
7c612fa to
3c421d4
Compare
| const notifyMessage = await buildNotifyMessage({ | ||
| patientId, | ||
| correlationId, | ||
| orderId, | ||
| createdAt: statusUpdate.createdAt, | ||
| ...(orderedAt ? { orderedAt } : {}), | ||
| }); | ||
|
|
||
| await sqsClient.sendMessage(notifyMessagesQueueUrl, JSON.stringify(notifyMessage)); |
There was a problem hiding this comment.
Failures from sqsClient.sendMessage() / notificationAuditDbClient.insertNotificationAuditEntry() are caught later in this method and only logged, so the handler will still return 201 even when the notification was not queued/audited and no retry is triggered. Consider rethrowing after logging (or writing an outbox/failure record for async retry) to avoid silently dropping notifications.
|
| private async buildOrderStatusNotifyMessage(input: { | ||
| patientId: string; | ||
| patientId?: string; | ||
| recipient?: NotifyRecipient; | ||
| correlationId: string; | ||
| orderId: string; | ||
| eventCode: NotifyEventCode; | ||
| personalisation: Record<string, string>; | ||
| }): Promise<NotifyMessage> { | ||
| const { patientId, correlationId, eventCode, personalisation } = input; | ||
| const { patientId, recipient, correlationId, eventCode, personalisation } = input; | ||
|
|
||
| const patient = await this.patientDbClient.get(patientId); | ||
| const recipient: NotifyRecipient = { | ||
| nhsNumber: patient.nhsNumber, | ||
| dateOfBirth: patient.birthDate, | ||
| }; | ||
| const notifyRecipient = recipient ?? (await this.getRecipientFromPatientId(patientId)); |
There was a problem hiding this comment.
buildOrderStatusNotifyMessage accepts both patientId? and recipient?, but the implementation requires at least one and otherwise throws at runtime; model this constraint in the type (e.g., a union of { patientId: string; recipient?: never } | { recipient: NotifyRecipient; patientId?: never }) so invalid internal calls are caught at compile time.
Description
Context
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.