Fix: [AEA-0000] - Update how the callback is handled. (#2132)

## Summary

- Routine Change

### Details

The callback lambda currently kinda works, but not really. It rejects
requests that lack a request ID, and notify do not provide one, so that
needs to be fixed.

Also, I realised I never unit tested the new callback type, so I did
that.

Additionally, the logging of incoming updates is not really in line with
how statuses actually flow. I have tweaked it to be more generalised.

Author: wildjames

packages/common/commonTypes/src/index.ts

@@ -26,7 +26,10 @@ export interface LastNotificationStateType {
   ODSCode: string
   RequestId: string // x-request-id header
   SQSMessageID?: string // The SQS message ID
-  DeliveryStatus: string
+  MessageStatus: string
+  // These are undefined until we get callbacks from Notify
+  ChannelStatus?: string
+  SupplierStatus?: string
   NotifyMessageID?: string // The UUID we got back from Notify for the submitted message
   NotifyMessageReference: string // The references we generated for the message
   NotifyMessageBatchReference?: string // As above

packages/nhsNotifyLambda/src/utils/dynamo.ts

@@ -37,7 +37,7 @@ export async function addPrescriptionMessagesToNotificationStateStore(
       RequestId: data.PSUDataItem.RequestID,
       SQSMessageID: data.MessageId,
       LastNotifiedPrescriptionStatus: data.PSUDataItem.Status,
-      DeliveryStatus: data.deliveryStatus ?? "unknown", // Fall back to unknown if not set
+      MessageStatus: data.messageStatus ?? "unknown", // Fall back to unknown if not set
       NotifyMessageID: data.notifyMessageId, // This is a GSI, but leaving it blank is fine
       NotifyMessageReference: data.messageReference,
       NotifyMessageBatchReference: data.messageBatchReference, // Will be undefined when request fails

packages/nhsNotifyLambda/src/utils/notify.ts

@@ -108,7 +108,7 @@ export async function makeBatchNotifyRequest(
     logger.info("Not doing real Notify requests. Simply waiting for some time and returning success on all messages")
     await new Promise(f => setTimeout(f, DUMMY_NOTIFY_DELAY_MS))
 
-    const deliveryStatus = "silent running"
+    const messageStatus = "silent running"
 
     logger.info("Requested notifications OK!", {
       messageBatchReference,
@@ -117,15 +117,15 @@ export async function makeBatchNotifyRequest(
         messageReference: e.messageReference,
         psuRequestId: data.find((el) => el.messageReference === e.messageReference)?.PSUDataItem.RequestID
       })),
-      deliveryStatus
+      deliveryStatus: messageStatus // TODO: change splunk report query to messageStatus
     })
 
     // Map each input item to a "successful" NotifyDataItemMessage
     return data.map(item => {
       return {
         ...item,
         messageBatchReference,
-        deliveryStatus,
+        messageStatus,
         notifyMessageId: v4() // Create a dummy UUID
       }
     })
@@ -169,7 +169,7 @@ export async function makeBatchNotifyRequest(
           messageReference: e.messageReference,
           psuRequestId: data.find((el) => el.messageReference === e.messageReference)?.PSUDataItem.RequestID
         })),
-        deliveryStatus: "requested"
+        deliveryStatus: "requested" // TODO: change splunk report query to messageStatus
       })
 
       // Map each input item to a NotifyDataItemMessage, marking success and attaching the notify ID
@@ -182,7 +182,7 @@ export async function makeBatchNotifyRequest(
         return {
           ...item,
           messageBatchReference,
-          deliveryStatus: match ? "requested" : "notify request failed",
+          messageStatus: match ? "requested" : "notify request failed",
           notifyMessageId: match?.id
         }
       })
@@ -197,7 +197,7 @@ export async function makeBatchNotifyRequest(
           messageReference: e.messageReference,
           psuRequestId: data.find((el) => el.messageReference === e.messageReference)?.PSUDataItem.RequestID
         })),
-        deliveryStatus: "notify request failed"
+        messageStatus: "notify request failed"
       })
       throw new Error("Notify batch request failed")
     }
@@ -206,7 +206,7 @@ export async function makeBatchNotifyRequest(
     logger.error("Notify batch request failed", {error})
     return data.map(item => ({
       ...item,
-      deliveryStatus: "notify request failed",
+      messageStatus: "notify request failed",
       messageBatchReference,
       messageReferences: messages.map(e => ({
         nhsNumber: e.recipient.nhsNumber,

packages/nhsNotifyLambda/src/utils/types.ts

@@ -5,7 +5,7 @@ import {NotifyDataItem} from "@PrescriptionStatusUpdate_common/commonTypes"
 // and helps track the nhs notify results
 export interface NotifyDataItemMessage extends Message {
   PSUDataItem: NotifyDataItem
-  deliveryStatus?: string
+  messageStatus?: string
   messageBatchReference?: string,
   // message reference is our internal UUID for the message
   messageReference: string

packages/nhsNotifyLambda/tests/testUtils.test.ts

@@ -509,14 +509,14 @@ describe("NHS notify lambda helper functions", () => {
       expect(result).toHaveLength(2)
       expect(result[0]).toMatchObject({
         PSUDataItem: data[0].PSUDataItem,
-        deliveryStatus: "requested",
+        messageStatus: "requested",
         notifyMessageId: "msg-id-1",
         messageBatchReference: expect.any(String),
         messageReference: expect.any(String)
       })
       expect(result[1]).toMatchObject({
         PSUDataItem: data[1].PSUDataItem,
-        deliveryStatus: "notify request failed",
+        messageStatus: "notify request failed",
         notifyMessageId: undefined,
         messageBatchReference: expect.any(String),
         messageReference: expect.any(String)
@@ -549,7 +549,7 @@ describe("NHS notify lambda helper functions", () => {
       expect(result).toMatchObject([
         {
           PSUDataItem: data[0].PSUDataItem,
-          deliveryStatus: "notify request failed",
+          messageStatus: "notify request failed",
           notifyMessageId: undefined,
           messageBatchReference: expect.any(String),
           messageReference: expect.any(String)
@@ -598,7 +598,7 @@ describe("NHS notify lambda helper functions", () => {
       result.forEach((r) =>
         expect(r).toEqual(
           expect.objectContaining({
-            deliveryStatus: "notify request failed",
+            messageStatus: "notify request failed",
             notifyMessageId: undefined
           })
         )
@@ -745,14 +745,14 @@ describe("NHS notify lambda helper functions", () => {
       expect(result).toHaveLength(2)
       expect(result[0]).toMatchObject({
         PSUDataItem: data[0].PSUDataItem,
-        deliveryStatus: "silent running",
+        messageStatus: "silent running",
         notifyMessageId: expect.any(String), // it will be assigned a dummy ID
         messageBatchReference: expect.any(String),
         messageReference: expect.any(String)
       })
       expect(result[1]).toMatchObject({
         PSUDataItem: data[1].PSUDataItem,
-        deliveryStatus: "silent running",
+        messageStatus: "silent running",
         notifyMessageId: expect.any(String),
         messageBatchReference: expect.any(String),
         messageReference: expect.any(String)

packages/nhsNotifyUpdateCallback/src/helpers.ts

@@ -130,22 +130,30 @@ export async function updateNotificationsTable(
   // For each callback resource, return a promise
   const callbackPromises = bodyData.data.map(async (resource) => {
     let messageId: string
-    let messageStatus: string
+    let messageStatus: string | undefined
+    let channelStatus: string | undefined
+    let supplierStatus: string | undefined
     let timestamp: string
 
     if (resource.type === CallbackType.message) {
       messageId = resource.attributes.messageId
       messageStatus = resource.attributes.messageStatus
+      channelStatus = resource.attributes.channels?.[0]?.channelStatus // If missing, undefined
+      supplierStatus = undefined
       timestamp = resource.attributes.timestamp
     } else if (resource.type === CallbackType.channel) {
       messageId = resource.attributes.messageId
-      messageStatus = resource.attributes.channelStatus
+      messageStatus = undefined
+      channelStatus = resource.attributes.channelStatus
+      supplierStatus = resource.attributes.supplierStatus
       timestamp = resource.attributes.timestamp
     } else {
       logger.error("Unknown data structure - cannot store to notifications table.", {resource})
       // Set to junk data, so that when we try and update the table we will fail. This is fine, and handled later.
       messageId = "unknown"
-      messageStatus = "unknown"
+      messageStatus = undefined
+      channelStatus = undefined
+      supplierStatus = undefined
       timestamp = "unknown"
     }
 
@@ -190,28 +198,54 @@ export async function updateNotificationsTable(
         NHSNumber: item.NHSNumber,
         RequestId: item.RequestId
       }
+
+      // Build UpdateExpression so undefined statuses are not written/updated.
+      const sets: Array<string> = [
+        "LastNotificationRequestTimestamp = :ts",
+        "ExpiryTime = :et"
+      ]
+      const eav: Record<string, unknown> = {
+        ":ts": timestamp,
+        ":et": newExpiry
+      }
+
+      if (messageStatus !== undefined) {
+        sets.push("MessageStatus = :ms")
+        eav[":ms"] = messageStatus
+      }
+      if (channelStatus !== undefined) {
+        sets.push("ChannelStatus = :cs")
+        eav[":cs"] = channelStatus
+      }
+      if (supplierStatus !== undefined) {
+        sets.push("SupplierStatus = :ss")
+        eav[":ss"] = supplierStatus
+      }
+
+      const UpdateExpression = `SET ${sets.join(", ")}`
+
       try {
         await docClient.send(new UpdateCommand({
           TableName: dynamoTable,
           Key: key,
-          UpdateExpression: [
-            "SET DeliveryStatus = :ds",
-            " , LastNotificationRequestTimestamp = :ts",
-            " , ExpiryTime = :et"
-          ].join(""),
-          ExpressionAttributeValues: {
-            ":ds": messageStatus,
-            ":ts": timestamp,
-            ":et": newExpiry
-          }
+          UpdateExpression,
+          ExpressionAttributeValues: eav
         }))
+
         logger.info(
           "Updated notification state",
           {
             NotifyMessageID: item.NotifyMessageID,
             nhsNumber: item.NHSNumber,
             psuRequestId: item.RequestId,
-            newStatus: messageStatus,
+            // The overall delivery status is whichever of
+            // messageStatus or channelStatus is defined (prefer messageStatus)
+            // TODO: Update the splunk query to use the below statuses
+            deliveryStatus: messageStatus ?? channelStatus,
+            // Parse to a string, or else undefined stuff doesn't get logged (thanks aws)
+            messageStatus: `${messageStatus}`,
+            channelStatus: `${channelStatus}`,
+            supplierStatus: `${supplierStatus}`,
             newTimestamp: timestamp,
             newExpiryTime: newExpiry
           }

packages/nhsNotifyUpdateCallback/src/lambdaHandler.ts

@@ -16,14 +16,10 @@ export const logger = new Logger({serviceName: "nhsNotifyUpdateCallback"})
 
 const lambdaHandler = async (event: APIGatewayProxyEvent): Promise<APIGatewayProxyResult> => {
   logger.appendKeys({
-    "x-correlation-id": event.headers["x-correlation-id"],
     "apigw-request-id": event.headers["apigw-request-id"],
-    "x-request-id": event.headers["x-request-id"]
+    "nhsd-correlation-id": event.headers["nhsd-correlation-id"]
   })
 
-  // Require a request ID
-  if (!event.headers["x-request-id"]) return response(400, {message: "No x-request-id given"})
-
   // Check the request signature
   const isErr = await checkSignature(logger, event)
   if (isErr) return isErr
@@ -39,34 +35,26 @@ const lambdaHandler = async (event: APIGatewayProxyEvent): Promise<APIGatewayPro
   }
 
   let receivedUnknownCallbackType = false
+
   payload.data.forEach(m => {
-    let logPayload = {}
-    if (m.type === CallbackType.message) {
-      logPayload = {
-        callbackType: m.type,
-        messageStatus: m.attributes.messageStatus,
-        messageReference: m.attributes.messageReference,
-        messageId: m.attributes.messageId,
-        receivedTimestamp: m.attributes.timestamp
-      }
+    let logPayload = {
+      callbackType: m.type,
+      messageId: m.attributes.messageId,
+      // Only defined for message status callbacks
+      messageStatus: m.type === CallbackType.message ? m.attributes.messageStatus : undefined,
+      // Only defined for channel/supplier status callbacks
+      channelStatus: m.type === CallbackType.channel ? m.attributes.channelStatus : undefined,
+      supplierStatus: m.type === CallbackType.channel ? m.attributes.supplierStatus : undefined,
+      retryCount: m.type === CallbackType.channel ? m.attributes.retryCount : undefined,
+      callbackMessageTimestamp: m.attributes.timestamp,
+      messageReference: m.attributes.messageReference
+    }
+    logger.info("Message state updated", logPayload)
 
-    } else if (m.type === CallbackType.channel) {
-      logPayload = {
-        callbackType: m.type,
-        messageStatus: m.attributes.channelStatus,
-        supplierStatus: m.attributes.supplierStatus ?? "not given",
-        retryCount: m.attributes.retryCount,
-        messageReference: m.attributes.messageReference,
-        messageId: m.attributes.messageId,
-        receivedTimestamp: m.attributes.timestamp
-      }
-    } else {
+    if ((m.type !== CallbackType.message) && (m.type !== CallbackType.channel)) {
       logger.warn("Unknown callback data structure.", {data: m})
       receivedUnknownCallbackType = true
     }
-
-    // If we have populated the logPayload object, then log it.
-    if (Object.keys(logPayload).length > 0) logger.info("Message state updated", logPayload)
   })
 
   try {