From 5fa4c38714a80013de511a09425b8308536e337e Mon Sep 17 00:00:00 2001 From: jungabunga Date: Thu, 6 Feb 2025 12:53:34 +0000 Subject: [PATCH 01/15] version bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 515969c..be49d28 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "fcp-fd-comms", - "version": "1.0.2", + "version": "1.0.3", "description": "Common communications service for Single Front Door", "homepage": "https://github.com/DEFRA/fcp-fd-comms", "main": "app/index.js", From 2d94111cb2e82f8427e819b5a1c4924dbde70c1a Mon Sep 17 00:00:00 2001 From: jungabunga Date: Fri, 7 Feb 2025 13:34:02 +0000 Subject: [PATCH 02/15] added code to handle duplicate requests --- .../check-notify-status/get-notify-status.js | 4 +- .../comms-request/send-notification.js | 16 +-- app/repos/notification-log.js | 13 ++- .../comms-request/send-notification.test.js | 99 ++++++++++++++++++- test/unit/repos/notification-log.test.js | 57 ++++++++++- 5 files changed, 176 insertions(+), 13 deletions(-) diff --git a/app/jobs/check-notify-status/get-notify-status.js b/app/jobs/check-notify-status/get-notify-status.js index 59c8ec9..634a616 100644 --- a/app/jobs/check-notify-status/get-notify-status.js +++ b/app/jobs/check-notify-status/get-notify-status.js @@ -2,10 +2,10 @@ import notifyClient from '../../clients/notify-client.js' const getNotifyStatus = async (id) => { const { data } = await notifyClient.getNotificationById(id) - return { id: data.id, - status: data.status + status: data.status, + emailAddress: data.email_address } } diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 6ed8245..353db87 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -1,13 +1,16 @@ import crypto from 'crypto' - import notifyClient from '../../../clients/notify-client.js' import notifyStatus from '../../../constants/notify-statuses.js' - import { logCreatedNotification, logRejectedNotification } from '../../../repos/notification-log.js' import { publishStatus } from '../../outbound/notification-status/publish.js' +import { checkDuplicateNotification } from '../../../jobs/check-notify-status/check-duplicate-notification.js' +let statusCode const trySendViaNotify = async (message, emailAddress) => { try { + const duplicateError = await checkDuplicateNotification(message, emailAddress) + if (duplicateError) throw duplicateError + const response = await notifyClient.sendEmail( message.data.notifyTemplateId, emailAddress, { @@ -18,10 +21,8 @@ const trySendViaNotify = async (message, emailAddress) => { return [response, null] } catch (error) { - const status = error.response.data.status_code - - console.error('Error sending email with code:', status) - + statusCode = error.response?.data?.status_code + console.error('Error sending email with code:', statusCode) return [null, error] } } @@ -41,12 +42,11 @@ const sendNotification = async (message) => { } else { const status = notifyStatus.INTERNAL_FAILURE const notifyErrorData = notifyError.response.data - await publishStatus(message, emailAddress, status, notifyErrorData) await logRejectedNotification(message, emailAddress, notifyError) } } catch (error) { - console.error('Error logging notification: ', error) + console.error('Error logging notification:', error) } } } diff --git a/app/repos/notification-log.js b/app/repos/notification-log.js index 1add335..3a11eb8 100644 --- a/app/repos/notification-log.js +++ b/app/repos/notification-log.js @@ -54,9 +54,20 @@ const updateNotificationStatus = async (notificationId, status) => { await notification.save() } +const findNotificationByIdAndEmail = async (messageId, recipient) => { + const existingNotification = await db.notifyApiRequestSuccess.findOne({ + where: { + 'message.id': messageId, + recipient + } + }) + return existingNotification +} + export { logCreatedNotification, logRejectedNotification, getPendingNotifications, - updateNotificationStatus + updateNotificationStatus, + findNotificationByIdAndEmail } diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index f743a97..79af879 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -2,6 +2,12 @@ import { jest, test } from '@jest/globals' import crypto from 'crypto' const mockSendEmail = jest.fn() +const mockfindNotificationByIdAndEmail = jest.fn() +const mockGetNotifyStatus = jest.fn() + +jest.unstable_mockModule('../../../../../app/jobs/check-notify-status/get-notify-status.js', () => ({ + getNotifyStatus: mockGetNotifyStatus +})) jest.unstable_mockModule('../../../../../app/clients/notify-client.js', () => ({ default: { @@ -11,7 +17,8 @@ jest.unstable_mockModule('../../../../../app/clients/notify-client.js', () => ({ jest.unstable_mockModule('../../../../../app/repos/notification-log.js', () => ({ logCreatedNotification: jest.fn(), - logRejectedNotification: jest.fn() + logRejectedNotification: jest.fn(), + findNotificationByIdAndEmail: mockfindNotificationByIdAndEmail })) jest.unstable_mockModule('../../../../../app/messages/outbound/notification-status/publish.js', () => ({ @@ -32,6 +39,96 @@ console.log = jest.fn() describe('Send Notification', () => { beforeEach(() => { jest.clearAllMocks() + // Set default behavior for non-duplicate cases + mockfindNotificationByIdAndEmail.mockResolvedValue(null) + mockGetNotifyStatus.mockResolvedValue(null) + mockSendEmail.mockResolvedValue({ + data: { + id: 'mock-notify-response-id' + } + }) + }) + + test('should allow sending when existing notification has non-active status', async () => { + const existingNotification = { + notifyResponseId: 'existing-notify-id', + status: 'previous-status' + } + mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) + mockGetNotifyStatus.mockResolvedValue({ status: 'permanent-failure' }) + const message = { + id: 'message-id-123', + data: { + notifyTemplateId: 'mock-notify-template-id', + commsAddresses: 'mock-email@test.com', + personalisation: { + reference: 'mock-reference', + agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' + } + } + } + await sendNotification(message) + expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') + expect(mockGetNotifyStatus).toHaveBeenCalledWith('existing-notify-id') + expect(mockSendEmail).toHaveBeenCalled() + expect(logRejectedNotification).not.toHaveBeenCalled() + }) + + test('should check for duplicate notifications before sending', async () => { + const message = { + id: 'message-id-123', + data: { + notifyTemplateId: 'mock-notify-template-id', + commsAddresses: 'mock-email@test.com', + personalisation: { + reference: 'mock-reference', + agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' + } + } + } + + await sendNotification(message) + + expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') + expect(mockSendEmail).toHaveBeenCalled() + }) + + test('should not send email if duplicate notification exists and is active', async () => { + const existingNotification = { + notifyResponseId: 'existing-notify-id', + status: 'created' + } + mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) + mockGetNotifyStatus.mockResolvedValue({ status: 'sending' }) + + const message = { + id: 'message-id-123', + data: { + notifyTemplateId: 'mock-notify-template-id', + commsAddresses: 'mock-email@test.com', + personalisation: { + reference: 'mock-reference', + agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' + } + } + } + + await sendNotification(message) + + expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') + expect(mockGetNotifyStatus).toHaveBeenCalledWith('existing-notify-id') + expect(mockSendEmail).not.toHaveBeenCalled() + expect(logRejectedNotification).toHaveBeenCalledWith( + message, + 'mock-email@test.com', + expect.objectContaining({ + response: { + data: { + status_code: 409 + } + } + }) + ) }) test('should send an email with the correct arguments to a single email address', async () => { diff --git a/test/unit/repos/notification-log.test.js b/test/unit/repos/notification-log.test.js index b1157d6..3470a56 100644 --- a/test/unit/repos/notification-log.test.js +++ b/test/unit/repos/notification-log.test.js @@ -18,7 +18,8 @@ jest.unstable_mockModule('../../../app/data/index.js', () => ({ const { logCreatedNotification, logRejectedNotification, - updateNotificationStatus + updateNotificationStatus, + findNotificationByIdAndEmail } = await import('../../../app/repos/notification-log.js') describe('Notification Log Repository', () => { @@ -96,4 +97,58 @@ describe('Notification Log Repository', () => { expect(notification.completed).toEqual(new Date('2024-01-01T15:00:00.000Z')) } ) + + describe('findNotificationByIdAndEmail', () => { + test('should return existing notification when found', async () => { + const mockNotification = { + notifyResponseId: '123456789', + message: { + id: 'test-message-id' + }, + recipient: 'test@example.com', + status: 'created' + } + + mockFindOne.mockResolvedValue(mockNotification) + + const result = await findNotificationByIdAndEmail('test-message-id', 'test@example.com') + + expect(mockFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + expect(result).toEqual(mockNotification) + }) + + test('should return null when notification is not found', async () => { + mockFindOne.mockResolvedValue(null) + + const result = await findNotificationByIdAndEmail('test-message-id', 'test@example.com') + + expect(mockFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + expect(result).toBeNull() + }) + + test('should handle database errors appropriately', async () => { + const mockError = new Error('Database connection failed') + mockFindOne.mockRejectedValue(mockError) + + await expect(findNotificationByIdAndEmail('test-message-id', 'test@example.com')) + .rejects.toThrow('Database connection failed') + + expect(mockFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + }) + }) }) From d60532283308c61cafcdd7aa92505bfef3d72c8b Mon Sep 17 00:00:00 2001 From: jungabunga Date: Fri, 7 Feb 2025 13:34:24 +0000 Subject: [PATCH 03/15] added code to handle duplicate requests --- .../check-duplicate-notification.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 app/jobs/check-notify-status/check-duplicate-notification.js diff --git a/app/jobs/check-notify-status/check-duplicate-notification.js b/app/jobs/check-notify-status/check-duplicate-notification.js new file mode 100644 index 0000000..b88d770 --- /dev/null +++ b/app/jobs/check-notify-status/check-duplicate-notification.js @@ -0,0 +1,22 @@ +import { findNotificationByIdAndEmail } from '../../repos/notification-log.js' +import { getNotifyStatus } from './../../jobs/check-notify-status/get-notify-status.js' + +const checkDuplicateNotification = async (message, emailAddress) => { + const existingNotification = await findNotificationByIdAndEmail(message.id, emailAddress) + if (!existingNotification) { + return null + } + const notifyStatusResponse = await getNotifyStatus(existingNotification.notifyResponseId) + if (['sending', 'delivered', 'created'].includes(notifyStatusResponse?.status)) { + return { + response: { + data: { + status_code: 409 + } + } + } + } + return null +} + +export { checkDuplicateNotification } From 1ce900be90aade8dfbb3be1cd5b0608a20930356 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Fri, 7 Feb 2025 13:53:42 +0000 Subject: [PATCH 04/15] fixed code smell, sonar does not allow straigth if --- app/messages/inbound/comms-request/send-notification.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 353db87..483bb5f 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -9,7 +9,9 @@ let statusCode const trySendViaNotify = async (message, emailAddress) => { try { const duplicateError = await checkDuplicateNotification(message, emailAddress) - if (duplicateError) throw duplicateError + if (duplicateError) { + throw duplicateError + } const response = await notifyClient.sendEmail( message.data.notifyTemplateId, From 61987d0b1f65322895f8237d9396af01dd754153 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Fri, 7 Feb 2025 14:04:27 +0000 Subject: [PATCH 05/15] removed unused let assigment --- app/messages/inbound/comms-request/send-notification.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 483bb5f..413d4cb 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -5,7 +5,6 @@ import { logCreatedNotification, logRejectedNotification } from '../../../repos/ import { publishStatus } from '../../outbound/notification-status/publish.js' import { checkDuplicateNotification } from '../../../jobs/check-notify-status/check-duplicate-notification.js' -let statusCode const trySendViaNotify = async (message, emailAddress) => { try { const duplicateError = await checkDuplicateNotification(message, emailAddress) @@ -23,7 +22,7 @@ const trySendViaNotify = async (message, emailAddress) => { return [response, null] } catch (error) { - statusCode = error.response?.data?.status_code + const statusCode = error.response?.data?.status_code console.error('Error sending email with code:', statusCode) return [null, error] } From e05acf359586f363ab9027a6a7e2fc8239de7ff9 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Mon, 10 Feb 2025 08:59:48 +0000 Subject: [PATCH 06/15] removed duplicate interaction with govNotify when check for duplicate --- .../check-duplicate-notification.js | 7 +------ .../inbound/comms-request/send-notification.test.js | 10 +--------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/app/jobs/check-notify-status/check-duplicate-notification.js b/app/jobs/check-notify-status/check-duplicate-notification.js index b88d770..4ef95c3 100644 --- a/app/jobs/check-notify-status/check-duplicate-notification.js +++ b/app/jobs/check-notify-status/check-duplicate-notification.js @@ -1,13 +1,8 @@ import { findNotificationByIdAndEmail } from '../../repos/notification-log.js' -import { getNotifyStatus } from './../../jobs/check-notify-status/get-notify-status.js' const checkDuplicateNotification = async (message, emailAddress) => { const existingNotification = await findNotificationByIdAndEmail(message.id, emailAddress) - if (!existingNotification) { - return null - } - const notifyStatusResponse = await getNotifyStatus(existingNotification.notifyResponseId) - if (['sending', 'delivered', 'created'].includes(notifyStatusResponse?.status)) { + if (['sending', 'delivered', 'created'].includes(existingNotification?.status)) { return { response: { data: { diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 79af879..9ed1d74 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -39,7 +39,6 @@ console.log = jest.fn() describe('Send Notification', () => { beforeEach(() => { jest.clearAllMocks() - // Set default behavior for non-duplicate cases mockfindNotificationByIdAndEmail.mockResolvedValue(null) mockGetNotifyStatus.mockResolvedValue(null) mockSendEmail.mockResolvedValue({ @@ -52,10 +51,9 @@ describe('Send Notification', () => { test('should allow sending when existing notification has non-active status', async () => { const existingNotification = { notifyResponseId: 'existing-notify-id', - status: 'previous-status' + status: 'permanent-failure' } mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) - mockGetNotifyStatus.mockResolvedValue({ status: 'permanent-failure' }) const message = { id: 'message-id-123', data: { @@ -69,7 +67,6 @@ describe('Send Notification', () => { } await sendNotification(message) expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') - expect(mockGetNotifyStatus).toHaveBeenCalledWith('existing-notify-id') expect(mockSendEmail).toHaveBeenCalled() expect(logRejectedNotification).not.toHaveBeenCalled() }) @@ -99,8 +96,6 @@ describe('Send Notification', () => { status: 'created' } mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) - mockGetNotifyStatus.mockResolvedValue({ status: 'sending' }) - const message = { id: 'message-id-123', data: { @@ -112,11 +107,8 @@ describe('Send Notification', () => { } } } - await sendNotification(message) - expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') - expect(mockGetNotifyStatus).toHaveBeenCalledWith('existing-notify-id') expect(mockSendEmail).not.toHaveBeenCalled() expect(logRejectedNotification).toHaveBeenCalledWith( message, From e5a917f4aae2bda0353bbe58c142b77aeaa82326 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Mon, 10 Feb 2025 09:25:00 +0000 Subject: [PATCH 07/15] moved duplicate check to utils folder --- .../check-duplicate-notification.js | 17 ----------------- .../inbound/comms-request/send-notification.js | 2 +- 2 files changed, 1 insertion(+), 18 deletions(-) delete mode 100644 app/jobs/check-notify-status/check-duplicate-notification.js diff --git a/app/jobs/check-notify-status/check-duplicate-notification.js b/app/jobs/check-notify-status/check-duplicate-notification.js deleted file mode 100644 index 4ef95c3..0000000 --- a/app/jobs/check-notify-status/check-duplicate-notification.js +++ /dev/null @@ -1,17 +0,0 @@ -import { findNotificationByIdAndEmail } from '../../repos/notification-log.js' - -const checkDuplicateNotification = async (message, emailAddress) => { - const existingNotification = await findNotificationByIdAndEmail(message.id, emailAddress) - if (['sending', 'delivered', 'created'].includes(existingNotification?.status)) { - return { - response: { - data: { - status_code: 409 - } - } - } - } - return null -} - -export { checkDuplicateNotification } diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 413d4cb..736feba 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -3,7 +3,7 @@ import notifyClient from '../../../clients/notify-client.js' import notifyStatus from '../../../constants/notify-statuses.js' import { logCreatedNotification, logRejectedNotification } from '../../../repos/notification-log.js' import { publishStatus } from '../../outbound/notification-status/publish.js' -import { checkDuplicateNotification } from '../../../jobs/check-notify-status/check-duplicate-notification.js' +import { checkDuplicateNotification } from '../../../utils/check-duplicate-notification.js' const trySendViaNotify = async (message, emailAddress) => { try { From 7998c144b21b1fd2b67f8455504d595949ac7eaf Mon Sep 17 00:00:00 2001 From: jungabunga Date: Mon, 10 Feb 2025 12:46:49 +0000 Subject: [PATCH 08/15] refactored idempotent behavior --- app/messages/inbound/comms-request/handler.js | 3 +- .../comms-request/send-notification.js | 6 - app/repos/notification-log.js | 15 +- app/utils/check-duplicate-notification.js | 31 ++++ .../inbound/comms-request/handler.test.js | 55 +++++++ .../comms-request/send-notification.test.js | 84 +---------- test/unit/repos/notification-log.test.js | 68 ++++++++- .../check-duplicate-notification.test.js | 139 ++++++++++++++++++ 8 files changed, 307 insertions(+), 94 deletions(-) create mode 100644 app/utils/check-duplicate-notification.js create mode 100644 test/unit/utils/check-duplicate-notification.test.js diff --git a/app/messages/inbound/comms-request/handler.js b/app/messages/inbound/comms-request/handler.js index 9e6eeca..76aa4b5 100644 --- a/app/messages/inbound/comms-request/handler.js +++ b/app/messages/inbound/comms-request/handler.js @@ -4,6 +4,7 @@ import { } from '../../../schemas/comms-request/index.js' import { publishInvalidRequest, publishReceived } from '../../outbound/notification-status/publish.js' import { sendNotification } from './send-notification.js' +import { checkDuplicateNotification } from '../../../utils/check-duplicate-notification.js' const handleCommsRequest = async (message, receiver) => { const commsRequest = message.body @@ -24,7 +25,7 @@ const handleCommsRequest = async (message, receiver) => { return } - + await checkDuplicateNotification(validated, validated.data.commsAddresses) await publishReceived(validated) await sendNotification(validated) diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 736feba..b796bd3 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -3,15 +3,9 @@ import notifyClient from '../../../clients/notify-client.js' import notifyStatus from '../../../constants/notify-statuses.js' import { logCreatedNotification, logRejectedNotification } from '../../../repos/notification-log.js' import { publishStatus } from '../../outbound/notification-status/publish.js' -import { checkDuplicateNotification } from '../../../utils/check-duplicate-notification.js' const trySendViaNotify = async (message, emailAddress) => { try { - const duplicateError = await checkDuplicateNotification(message, emailAddress) - if (duplicateError) { - throw duplicateError - } - const response = await notifyClient.sendEmail( message.data.notifyTemplateId, emailAddress, { diff --git a/app/repos/notification-log.js b/app/repos/notification-log.js index 3a11eb8..ac423d3 100644 --- a/app/repos/notification-log.js +++ b/app/repos/notification-log.js @@ -54,7 +54,7 @@ const updateNotificationStatus = async (notificationId, status) => { await notification.save() } -const findNotificationByIdAndEmail = async (messageId, recipient) => { +const findSuccessNotificationByIdAndEmail = async (messageId, recipient) => { const existingNotification = await db.notifyApiRequestSuccess.findOne({ where: { 'message.id': messageId, @@ -64,10 +64,21 @@ const findNotificationByIdAndEmail = async (messageId, recipient) => { return existingNotification } +const findFailNotificationByIdAndEmail = async (messageId, recipient) => { + const existingNotification = await db.notifyApiRequestFailure.findOne({ + where: { + 'message.id': messageId, + recipient + } + }) + return existingNotification +} + export { logCreatedNotification, logRejectedNotification, getPendingNotifications, updateNotificationStatus, - findNotificationByIdAndEmail + findSuccessNotificationByIdAndEmail, + findFailNotificationByIdAndEmail } diff --git a/app/utils/check-duplicate-notification.js b/app/utils/check-duplicate-notification.js new file mode 100644 index 0000000..fe71f35 --- /dev/null +++ b/app/utils/check-duplicate-notification.js @@ -0,0 +1,31 @@ +import { findSuccessNotificationByIdAndEmail, findFailNotificationByIdAndEmail } from '../repos/notification-log.js' +import notifyStatus from '../constants/notify-statuses.js' + +const checkDuplicateNotification = async (message, emailAddresses) => { + const emailList = Array.isArray(emailAddresses) ? emailAddresses : [emailAddresses] + for (const emailAddress of emailList) { + const existingSuccessNotification = await findSuccessNotificationByIdAndEmail(message.id, emailAddress) + if (existingSuccessNotification) { + if (existingSuccessNotification.status === notifyStatus.SENDING || + existingSuccessNotification.status === notifyStatus.DELIVERED || + existingSuccessNotification.status === notifyStatus.CREATED) { + const error = new Error('Duplicate notification detected') + error.response = { + data: { + status_code: 409, + message: `Duplicate notification for ${existingSuccessNotification.id} detected` + } + } + throw error + } + } else { + const existingFailNotification = await findFailNotificationByIdAndEmail(message.id, emailAddress) + if (existingFailNotification) { + console.log('resending failed notification') + return null + } + } + } +} + +export { checkDuplicateNotification } diff --git a/test/unit/messages/inbound/comms-request/handler.test.js b/test/unit/messages/inbound/comms-request/handler.test.js index 35cea54..c5fe54c 100644 --- a/test/unit/messages/inbound/comms-request/handler.test.js +++ b/test/unit/messages/inbound/comms-request/handler.test.js @@ -1,6 +1,10 @@ import { expect, jest, test } from '@jest/globals' import commsMessage from '../../../../mocks/comms-message.js' +jest.unstable_mockModule('../../../../../app/utils/check-duplicate-notification.js', () => ({ + checkDuplicateNotification: jest.fn() +})) + const mockReceiver = { completeMessage: jest.fn(), abandonMessage: jest.fn(), @@ -20,14 +24,63 @@ const { publishReceived } = await import('../../../../../app/messages/outbound/n const { publishInvalidRequest } = await import('../../../../../app/messages/outbound/notification-status/publish.js') const { sendNotification } = await import('../../../../../app/messages/inbound/comms-request/send-notification.js') const { handleCommsRequest } = await import('../../../../../app/messages/inbound/comms-request/handler.js') +const { checkDuplicateNotification } = await import('../../../../../app/utils/check-duplicate-notification.js') describe('Handle Message', () => { beforeEach(() => { jest.clearAllMocks() }) + test('should abandon message when duplicate notification is detected', async () => { + const message = { body: commsMessage } + const duplicateError = new Error('Duplicate notification detected') + duplicateError.response = { + data: { + status_code: 409, + message: 'Duplicate notification detected' + } + } + checkDuplicateNotification.mockRejectedValue(duplicateError) + + await handleCommsRequest(message, mockReceiver) + + expect(mockReceiver.abandonMessage).toHaveBeenCalledWith(message) + expect(sendNotification).not.toHaveBeenCalled() + }) + + test('should continue processing when notification is not a duplicate', async () => { + const message = { body: commsMessage } + checkDuplicateNotification.mockResolvedValue(null) + + await handleCommsRequest(message, mockReceiver) + + expect(checkDuplicateNotification).toHaveBeenCalledWith(message.body, message.body.data.commsAddresses) + expect(publishReceived).toHaveBeenCalled() + expect(sendNotification).toHaveBeenCalled() + expect(mockReceiver.completeMessage).toHaveBeenCalled() + }) + test('should continue processing when notification had previous failure', async () => { + const message = { body: commsMessage } + checkDuplicateNotification.mockResolvedValue(null) + await handleCommsRequest(message, mockReceiver) + expect(checkDuplicateNotification).toHaveBeenCalledWith(message.body, message.body.data.commsAddresses) + expect(publishReceived).toHaveBeenCalled() + expect(sendNotification).toHaveBeenCalled() + expect(mockReceiver.completeMessage).toHaveBeenCalled() + }) + test('should log error when duplicate check fails', async () => { + const message = { body: commsMessage } + const consoleErrorSpy = jest.spyOn(console, 'error') + const error = new Error('Database error') + checkDuplicateNotification.mockRejectedValue(error) + + await handleCommsRequest(message, mockReceiver) + expect(consoleErrorSpy).toHaveBeenCalledWith('Error handling message: ', error) + expect(mockReceiver.abandonMessage).toHaveBeenCalledWith(message) + }) test('should call publishReceived', async () => { const message = { body: commsMessage } + checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) @@ -36,6 +89,7 @@ describe('Handle Message', () => { test('should call sendNotification', async () => { const message = { body: commsMessage } + checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) @@ -44,6 +98,7 @@ describe('Handle Message', () => { test('should call completeMessage', async () => { const message = { body: commsMessage } + checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 9ed1d74..38cf84a 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -2,7 +2,8 @@ import { jest, test } from '@jest/globals' import crypto from 'crypto' const mockSendEmail = jest.fn() -const mockfindNotificationByIdAndEmail = jest.fn() +const mockFindSuccessNotificationByIdAndEmail = jest.fn() +const mockFindFailNotificationByIdAndEmail = jest.fn() const mockGetNotifyStatus = jest.fn() jest.unstable_mockModule('../../../../../app/jobs/check-notify-status/get-notify-status.js', () => ({ @@ -17,8 +18,7 @@ jest.unstable_mockModule('../../../../../app/clients/notify-client.js', () => ({ jest.unstable_mockModule('../../../../../app/repos/notification-log.js', () => ({ logCreatedNotification: jest.fn(), - logRejectedNotification: jest.fn(), - findNotificationByIdAndEmail: mockfindNotificationByIdAndEmail + logRejectedNotification: jest.fn() })) jest.unstable_mockModule('../../../../../app/messages/outbound/notification-status/publish.js', () => ({ @@ -39,7 +39,8 @@ console.log = jest.fn() describe('Send Notification', () => { beforeEach(() => { jest.clearAllMocks() - mockfindNotificationByIdAndEmail.mockResolvedValue(null) + mockFindSuccessNotificationByIdAndEmail.mockResolvedValue(null) + mockFindFailNotificationByIdAndEmail.mockResolvedValue(null) mockGetNotifyStatus.mockResolvedValue(null) mockSendEmail.mockResolvedValue({ data: { @@ -48,81 +49,6 @@ describe('Send Notification', () => { }) }) - test('should allow sending when existing notification has non-active status', async () => { - const existingNotification = { - notifyResponseId: 'existing-notify-id', - status: 'permanent-failure' - } - mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) - const message = { - id: 'message-id-123', - data: { - notifyTemplateId: 'mock-notify-template-id', - commsAddresses: 'mock-email@test.com', - personalisation: { - reference: 'mock-reference', - agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' - } - } - } - await sendNotification(message) - expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') - expect(mockSendEmail).toHaveBeenCalled() - expect(logRejectedNotification).not.toHaveBeenCalled() - }) - - test('should check for duplicate notifications before sending', async () => { - const message = { - id: 'message-id-123', - data: { - notifyTemplateId: 'mock-notify-template-id', - commsAddresses: 'mock-email@test.com', - personalisation: { - reference: 'mock-reference', - agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' - } - } - } - - await sendNotification(message) - - expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') - expect(mockSendEmail).toHaveBeenCalled() - }) - - test('should not send email if duplicate notification exists and is active', async () => { - const existingNotification = { - notifyResponseId: 'existing-notify-id', - status: 'created' - } - mockfindNotificationByIdAndEmail.mockResolvedValue(existingNotification) - const message = { - id: 'message-id-123', - data: { - notifyTemplateId: 'mock-notify-template-id', - commsAddresses: 'mock-email@test.com', - personalisation: { - reference: 'mock-reference', - agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' - } - } - } - await sendNotification(message) - expect(mockfindNotificationByIdAndEmail).toHaveBeenCalledWith('message-id-123', 'mock-email@test.com') - expect(mockSendEmail).not.toHaveBeenCalled() - expect(logRejectedNotification).toHaveBeenCalledWith( - message, - 'mock-email@test.com', - expect.objectContaining({ - response: { - data: { - status_code: 409 - } - } - }) - ) - }) - test('should send an email with the correct arguments to a single email address', async () => { const uuidSpy = jest.spyOn(crypto, 'randomUUID').mockReturnValue('mock-uuid') diff --git a/test/unit/repos/notification-log.test.js b/test/unit/repos/notification-log.test.js index 3470a56..4aebc76 100644 --- a/test/unit/repos/notification-log.test.js +++ b/test/unit/repos/notification-log.test.js @@ -2,6 +2,7 @@ import { jest } from '@jest/globals' const mockCreate = jest.fn() const mockFindOne = jest.fn() +const mockFailureFindOne = jest.fn() jest.unstable_mockModule('../../../app/data/index.js', () => ({ default: { @@ -10,7 +11,8 @@ jest.unstable_mockModule('../../../app/data/index.js', () => ({ findOne: mockFindOne }, notifyApiRequestFailure: { - create: mockCreate + create: mockCreate, + findOne: mockFailureFindOne } } })) @@ -19,7 +21,8 @@ const { logCreatedNotification, logRejectedNotification, updateNotificationStatus, - findNotificationByIdAndEmail + findSuccessNotificationByIdAndEmail, + findFailNotificationByIdAndEmail } = await import('../../../app/repos/notification-log.js') describe('Notification Log Repository', () => { @@ -98,7 +101,7 @@ describe('Notification Log Repository', () => { } ) - describe('findNotificationByIdAndEmail', () => { + describe('findSuccessNotificationByIdAndEmail', () => { test('should return existing notification when found', async () => { const mockNotification = { notifyResponseId: '123456789', @@ -111,7 +114,7 @@ describe('Notification Log Repository', () => { mockFindOne.mockResolvedValue(mockNotification) - const result = await findNotificationByIdAndEmail('test-message-id', 'test@example.com') + const result = await findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com') expect(mockFindOne).toHaveBeenCalledWith({ where: { @@ -125,7 +128,7 @@ describe('Notification Log Repository', () => { test('should return null when notification is not found', async () => { mockFindOne.mockResolvedValue(null) - const result = await findNotificationByIdAndEmail('test-message-id', 'test@example.com') + const result = await findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com') expect(mockFindOne).toHaveBeenCalledWith({ where: { @@ -140,7 +143,7 @@ describe('Notification Log Repository', () => { const mockError = new Error('Database connection failed') mockFindOne.mockRejectedValue(mockError) - await expect(findNotificationByIdAndEmail('test-message-id', 'test@example.com')) + await expect(findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com')) .rejects.toThrow('Database connection failed') expect(mockFindOne).toHaveBeenCalledWith({ @@ -151,4 +154,57 @@ describe('Notification Log Repository', () => { }) }) }) + + describe('findFailNotificationByIdAndEmail', () => { + test('should return existing failed notification when found', async () => { + const mockNotification = { + message: { + id: 'test-message-id' + }, + recipient: 'test@example.com', + error: 'some error' + } + + mockFailureFindOne.mockResolvedValue(mockNotification) + + const result = await findFailNotificationByIdAndEmail('test-message-id', 'test@example.com') + + expect(mockFailureFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + expect(result).toEqual(mockNotification) + }) + + test('should return null when failed notification is not found', async () => { + mockFailureFindOne.mockResolvedValue(null) + + const result = await findFailNotificationByIdAndEmail('test-message-id', 'test@example.com') + + expect(mockFailureFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + expect(result).toBeNull() + }) + + test('should handle database errors appropriately', async () => { + const mockError = new Error('Database connection failed') + mockFailureFindOne.mockRejectedValue(mockError) + + await expect(findFailNotificationByIdAndEmail('test-message-id', 'test@example.com')) + .rejects.toThrow('Database connection failed') + + expect(mockFailureFindOne).toHaveBeenCalledWith({ + where: { + 'message.id': 'test-message-id', + recipient: 'test@example.com' + } + }) + }) + }) }) diff --git a/test/unit/utils/check-duplicate-notification.test.js b/test/unit/utils/check-duplicate-notification.test.js new file mode 100644 index 0000000..d2b2536 --- /dev/null +++ b/test/unit/utils/check-duplicate-notification.test.js @@ -0,0 +1,139 @@ +import { jest } from '@jest/globals' + +jest.unstable_mockModule('../../../app/repos/notification-log.js', () => ({ + findSuccessNotificationByIdAndEmail: jest.fn(), + findFailNotificationByIdAndEmail: jest.fn() +})) + +jest.unstable_mockModule('../../../app/constants/notify-statuses.js', () => ({ + default: { + SENDING: 'sending', + DELIVERED: 'delivered', + CREATED: 'created', + PERMANENT_FAILURE: 'permanent-failure', + TEMPORARY_FAILURE: 'temporary-failure', + TECHNICAL_FAILURE: 'technical-failure', + INTERNAL_FAILURE: 'internal-failure', + VALIDATION_FAILURE: 'validation-failure' + } +})) + +const { findSuccessNotificationByIdAndEmail, findFailNotificationByIdAndEmail } = await import('../../../app/repos/notification-log.js') +const { checkDuplicateNotification } = await import('../../../app/utils/check-duplicate-notification.js') + +describe('checkDuplicateNotification', () => { + beforeEach(() => { + jest.clearAllMocks() + console.log = jest.fn() + }) + + const mockMessage = { + id: 'test-message-id', + data: { + content: 'test content' + } + } + + describe('single email address', () => { + test('should throw error when notification exists with status "sending"', async () => { + const existingNotification = { + id: 'notification-id', + status: 'sending' + } + findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) + + await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) + .rejects.toThrow('Duplicate notification detected') + + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') + expect(findFailNotificationByIdAndEmail).not.toHaveBeenCalled() + }) + + test('should throw error when notification exists with status "delivered"', async () => { + const existingNotification = { + id: 'notification-id', + status: 'delivered' + } + findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) + + await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) + .rejects.toThrow('Duplicate notification detected') + }) + + test('should throw error when notification exists with status "created"', async () => { + const existingNotification = { + id: 'notification-id', + status: 'created' + } + findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) + + await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) + .rejects.toThrow('Duplicate notification detected') + }) + + test('should check for failed notification when no success notification exists', async () => { + findSuccessNotificationByIdAndEmail.mockResolvedValue(null) + findFailNotificationByIdAndEmail.mockResolvedValue(null) + + await checkDuplicateNotification(mockMessage, 'test@example.com') + + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') + expect(findFailNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') + }) + + test('should allow resend when failed notification exists', async () => { + findSuccessNotificationByIdAndEmail.mockResolvedValue(null) + findFailNotificationByIdAndEmail.mockResolvedValue({ id: 'failed-notification-id' }) + + const result = await checkDuplicateNotification(mockMessage, 'test@example.com') + + expect(result).toBeNull() + expect(console.log).toHaveBeenCalledWith('resending failed notification') + }) + }) + + describe('multiple email addresses', () => { + const emailAddresses = ['test1@example.com', 'test2@example.com'] + + test('should check each email address for duplicates', async () => { + findSuccessNotificationByIdAndEmail.mockResolvedValue(null) + findFailNotificationByIdAndEmail.mockResolvedValue(null) + + await checkDuplicateNotification(mockMessage, emailAddresses) + + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledTimes(2) + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test1@example.com') + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test2@example.com') + }) + + test('should throw error if any email has an active notification', async () => { + findSuccessNotificationByIdAndEmail + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'notification-id', status: 'sending' }) + + await expect(checkDuplicateNotification(mockMessage, emailAddresses)) + .rejects.toThrow('Duplicate notification detected') + + expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledTimes(2) + }) + }) + + describe('error handling', () => { + test('should handle database errors from findSuccessNotificationByIdAndEmail', async () => { + const dbError = new Error('Database error') + findSuccessNotificationByIdAndEmail.mockRejectedValue(dbError) + + await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) + .rejects.toThrow('Database error') + }) + + test('should handle database errors from findFailNotificationByIdAndEmail', async () => { + findSuccessNotificationByIdAndEmail.mockResolvedValue(null) + const dbError = new Error('Database error') + findFailNotificationByIdAndEmail.mockRejectedValue(dbError) + + await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) + .rejects.toThrow('Database error') + }) + }) +}) From d0f4dd34087150d36bfc1b515af6b6457cfef723 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Mon, 10 Feb 2025 13:55:10 +0000 Subject: [PATCH 09/15] removed unused assigments in test-file --- .../messages/inbound/comms-request/send-notification.test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 38cf84a..48ae855 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -2,8 +2,6 @@ import { jest, test } from '@jest/globals' import crypto from 'crypto' const mockSendEmail = jest.fn() -const mockFindSuccessNotificationByIdAndEmail = jest.fn() -const mockFindFailNotificationByIdAndEmail = jest.fn() const mockGetNotifyStatus = jest.fn() jest.unstable_mockModule('../../../../../app/jobs/check-notify-status/get-notify-status.js', () => ({ @@ -39,8 +37,6 @@ console.log = jest.fn() describe('Send Notification', () => { beforeEach(() => { jest.clearAllMocks() - mockFindSuccessNotificationByIdAndEmail.mockResolvedValue(null) - mockFindFailNotificationByIdAndEmail.mockResolvedValue(null) mockGetNotifyStatus.mockResolvedValue(null) mockSendEmail.mockResolvedValue({ data: { From c42c7ba3636545a2a6883ab53a3e6054d4cb0d37 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Mon, 10 Feb 2025 13:58:49 +0000 Subject: [PATCH 10/15] resolved sonar issue --- app/utils/check-duplicate-notification.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/utils/check-duplicate-notification.js b/app/utils/check-duplicate-notification.js index fe71f35..09ff219 100644 --- a/app/utils/check-duplicate-notification.js +++ b/app/utils/check-duplicate-notification.js @@ -26,6 +26,7 @@ const checkDuplicateNotification = async (message, emailAddresses) => { } } } + return null } export { checkDuplicateNotification } From 6b271c91839b61872089aba9d13c385a6fb0eb29 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Thu, 13 Feb 2025 09:15:11 +0000 Subject: [PATCH 11/15] refacotred into single function --- .../check-notify-status/get-notify-status.js | 3 +- app/messages/inbound/comms-request/handler.js | 2 +- .../comms-request/send-notification.js | 9 +- app/repos/notification-log.js | 22 +-- app/utils/check-duplicate-notification.js | 32 ---- .../inbound/comms-request/handler.test.js | 55 ------- .../comms-request/send-notification.test.js | 28 +++- test/unit/repos/notification-log.test.js | 108 ++++++-------- .../check-duplicate-notification.test.js | 139 ------------------ 9 files changed, 86 insertions(+), 312 deletions(-) delete mode 100644 app/utils/check-duplicate-notification.js delete mode 100644 test/unit/utils/check-duplicate-notification.test.js diff --git a/app/jobs/check-notify-status/get-notify-status.js b/app/jobs/check-notify-status/get-notify-status.js index 634a616..c2d4ebe 100644 --- a/app/jobs/check-notify-status/get-notify-status.js +++ b/app/jobs/check-notify-status/get-notify-status.js @@ -4,8 +4,7 @@ const getNotifyStatus = async (id) => { const { data } = await notifyClient.getNotificationById(id) return { id: data.id, - status: data.status, - emailAddress: data.email_address + status: data.status } } diff --git a/app/messages/inbound/comms-request/handler.js b/app/messages/inbound/comms-request/handler.js index 76aa4b5..8a39fca 100644 --- a/app/messages/inbound/comms-request/handler.js +++ b/app/messages/inbound/comms-request/handler.js @@ -4,7 +4,7 @@ import { } from '../../../schemas/comms-request/index.js' import { publishInvalidRequest, publishReceived } from '../../outbound/notification-status/publish.js' import { sendNotification } from './send-notification.js' -import { checkDuplicateNotification } from '../../../utils/check-duplicate-notification.js' +import { checkDuplicateNotification } from '../../../repos/notification-log.js' const handleCommsRequest = async (message, receiver) => { const commsRequest = message.body diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index b796bd3..2faade6 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -1,7 +1,7 @@ import crypto from 'crypto' import notifyClient from '../../../clients/notify-client.js' import notifyStatus from '../../../constants/notify-statuses.js' -import { logCreatedNotification, logRejectedNotification } from '../../../repos/notification-log.js' +import { logCreatedNotification, logRejectedNotification, checkDuplicateNotification } from '../../../repos/notification-log.js' import { publishStatus } from '../../outbound/notification-status/publish.js' const trySendViaNotify = async (message, emailAddress) => { @@ -28,6 +28,13 @@ const sendNotification = async (message) => { : [message.data.commsAddresses] for (const emailAddress of emailAddresses) { + const duplicate = await checkDuplicateNotification(message.id, emailAddress) + + if (duplicate) { + console.warn('Duplicate notification detected:', duplicate) + continue + } + const [response, notifyError] = await trySendViaNotify(message, emailAddress) try { diff --git a/app/repos/notification-log.js b/app/repos/notification-log.js index ac423d3..7277452 100644 --- a/app/repos/notification-log.js +++ b/app/repos/notification-log.js @@ -1,6 +1,8 @@ import db from '../data/index.js' import notifyStatus from '../constants/notify-statuses.js' +const nonFailureStatuses = [notifyStatus.CREATED, notifyStatus.SENDING, notifyStatus.DELIVERED] + const logCreatedNotification = async (message, recipient, notificationId) => { await db.notifyApiRequestSuccess.create({ createdAt: new Date(), @@ -54,24 +56,17 @@ const updateNotificationStatus = async (notificationId, status) => { await notification.save() } -const findSuccessNotificationByIdAndEmail = async (messageId, recipient) => { - const existingNotification = await db.notifyApiRequestSuccess.findOne({ +const checkDuplicateNotification = async (messageId, recipient) => { + const existing = await db.notifyApiRequestSuccess.findOne({ where: { 'message.id': messageId, recipient } }) - return existingNotification -} -const findFailNotificationByIdAndEmail = async (messageId, recipient) => { - const existingNotification = await db.notifyApiRequestFailure.findOne({ - where: { - 'message.id': messageId, - recipient - } - }) - return existingNotification + if (existing && nonFailureStatuses.includes(existing.status)) { + return true + } } export { @@ -79,6 +74,5 @@ export { logRejectedNotification, getPendingNotifications, updateNotificationStatus, - findSuccessNotificationByIdAndEmail, - findFailNotificationByIdAndEmail + checkDuplicateNotification } diff --git a/app/utils/check-duplicate-notification.js b/app/utils/check-duplicate-notification.js deleted file mode 100644 index 09ff219..0000000 --- a/app/utils/check-duplicate-notification.js +++ /dev/null @@ -1,32 +0,0 @@ -import { findSuccessNotificationByIdAndEmail, findFailNotificationByIdAndEmail } from '../repos/notification-log.js' -import notifyStatus from '../constants/notify-statuses.js' - -const checkDuplicateNotification = async (message, emailAddresses) => { - const emailList = Array.isArray(emailAddresses) ? emailAddresses : [emailAddresses] - for (const emailAddress of emailList) { - const existingSuccessNotification = await findSuccessNotificationByIdAndEmail(message.id, emailAddress) - if (existingSuccessNotification) { - if (existingSuccessNotification.status === notifyStatus.SENDING || - existingSuccessNotification.status === notifyStatus.DELIVERED || - existingSuccessNotification.status === notifyStatus.CREATED) { - const error = new Error('Duplicate notification detected') - error.response = { - data: { - status_code: 409, - message: `Duplicate notification for ${existingSuccessNotification.id} detected` - } - } - throw error - } - } else { - const existingFailNotification = await findFailNotificationByIdAndEmail(message.id, emailAddress) - if (existingFailNotification) { - console.log('resending failed notification') - return null - } - } - } - return null -} - -export { checkDuplicateNotification } diff --git a/test/unit/messages/inbound/comms-request/handler.test.js b/test/unit/messages/inbound/comms-request/handler.test.js index c5fe54c..35cea54 100644 --- a/test/unit/messages/inbound/comms-request/handler.test.js +++ b/test/unit/messages/inbound/comms-request/handler.test.js @@ -1,10 +1,6 @@ import { expect, jest, test } from '@jest/globals' import commsMessage from '../../../../mocks/comms-message.js' -jest.unstable_mockModule('../../../../../app/utils/check-duplicate-notification.js', () => ({ - checkDuplicateNotification: jest.fn() -})) - const mockReceiver = { completeMessage: jest.fn(), abandonMessage: jest.fn(), @@ -24,63 +20,14 @@ const { publishReceived } = await import('../../../../../app/messages/outbound/n const { publishInvalidRequest } = await import('../../../../../app/messages/outbound/notification-status/publish.js') const { sendNotification } = await import('../../../../../app/messages/inbound/comms-request/send-notification.js') const { handleCommsRequest } = await import('../../../../../app/messages/inbound/comms-request/handler.js') -const { checkDuplicateNotification } = await import('../../../../../app/utils/check-duplicate-notification.js') describe('Handle Message', () => { beforeEach(() => { jest.clearAllMocks() }) - test('should abandon message when duplicate notification is detected', async () => { - const message = { body: commsMessage } - const duplicateError = new Error('Duplicate notification detected') - duplicateError.response = { - data: { - status_code: 409, - message: 'Duplicate notification detected' - } - } - checkDuplicateNotification.mockRejectedValue(duplicateError) - - await handleCommsRequest(message, mockReceiver) - - expect(mockReceiver.abandonMessage).toHaveBeenCalledWith(message) - expect(sendNotification).not.toHaveBeenCalled() - }) - - test('should continue processing when notification is not a duplicate', async () => { - const message = { body: commsMessage } - checkDuplicateNotification.mockResolvedValue(null) - - await handleCommsRequest(message, mockReceiver) - - expect(checkDuplicateNotification).toHaveBeenCalledWith(message.body, message.body.data.commsAddresses) - expect(publishReceived).toHaveBeenCalled() - expect(sendNotification).toHaveBeenCalled() - expect(mockReceiver.completeMessage).toHaveBeenCalled() - }) - test('should continue processing when notification had previous failure', async () => { - const message = { body: commsMessage } - checkDuplicateNotification.mockResolvedValue(null) - await handleCommsRequest(message, mockReceiver) - expect(checkDuplicateNotification).toHaveBeenCalledWith(message.body, message.body.data.commsAddresses) - expect(publishReceived).toHaveBeenCalled() - expect(sendNotification).toHaveBeenCalled() - expect(mockReceiver.completeMessage).toHaveBeenCalled() - }) - test('should log error when duplicate check fails', async () => { - const message = { body: commsMessage } - const consoleErrorSpy = jest.spyOn(console, 'error') - const error = new Error('Database error') - checkDuplicateNotification.mockRejectedValue(error) - - await handleCommsRequest(message, mockReceiver) - expect(consoleErrorSpy).toHaveBeenCalledWith('Error handling message: ', error) - expect(mockReceiver.abandonMessage).toHaveBeenCalledWith(message) - }) test('should call publishReceived', async () => { const message = { body: commsMessage } - checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) @@ -89,7 +36,6 @@ describe('Handle Message', () => { test('should call sendNotification', async () => { const message = { body: commsMessage } - checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) @@ -98,7 +44,6 @@ describe('Handle Message', () => { test('should call completeMessage', async () => { const message = { body: commsMessage } - checkDuplicateNotification.mockResolvedValue(null) await handleCommsRequest(message, mockReceiver) diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 48ae855..2f93822 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -16,7 +16,8 @@ jest.unstable_mockModule('../../../../../app/clients/notify-client.js', () => ({ jest.unstable_mockModule('../../../../../app/repos/notification-log.js', () => ({ logCreatedNotification: jest.fn(), - logRejectedNotification: jest.fn() + logRejectedNotification: jest.fn(), + checkDuplicateNotification: jest.fn().mockResolvedValue(false) })) jest.unstable_mockModule('../../../../../app/messages/outbound/notification-status/publish.js', () => ({ @@ -25,7 +26,8 @@ jest.unstable_mockModule('../../../../../app/messages/outbound/notification-stat const { logCreatedNotification, - logRejectedNotification + logRejectedNotification, + checkDuplicateNotification } = await import('../../../../../app/repos/notification-log.js') const { publishStatus } = await import('../../../../../app/messages/outbound/notification-status/publish.js') @@ -33,6 +35,7 @@ const { publishStatus } = await import('../../../../../app/messages/outbound/not const { sendNotification } = await import('../../../../../app/messages/inbound/comms-request/send-notification.js') console.log = jest.fn() +console.warn = jest.fn() describe('Send Notification', () => { beforeEach(() => { @@ -44,6 +47,27 @@ describe('Send Notification', () => { } }) }) + test('should skip sending when duplicate notification is detected', async () => { + const message = { + id: 'message-id', + data: { + notifyTemplateId: 'mock-notify-template-id', + commsAddresses: 'mock-email@test.com', + personalisation: { + reference: 'mock-reference', + agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' + } + } + } + + checkDuplicateNotification.mockResolvedValueOnce(true) + + await sendNotification(message) + + expect(checkDuplicateNotification).toHaveBeenCalledWith('message-id', 'mock-email@test.com') + expect(mockSendEmail).not.toHaveBeenCalled() + expect(console.warn).toHaveBeenCalledWith('Duplicate notification detected:', true) + }) test('should send an email with the correct arguments to a single email address', async () => { const uuidSpy = jest.spyOn(crypto, 'randomUUID').mockReturnValue('mock-uuid') diff --git a/test/unit/repos/notification-log.test.js b/test/unit/repos/notification-log.test.js index 4aebc76..7ce77bb 100644 --- a/test/unit/repos/notification-log.test.js +++ b/test/unit/repos/notification-log.test.js @@ -17,12 +17,19 @@ jest.unstable_mockModule('../../../app/data/index.js', () => ({ } })) +jest.unstable_mockModule('../../../app/constants/notify-statuses.js', () => ({ + default: { + CREATED: 'created', + SENDING: 'sending', + DELIVERED: 'delivered' + } +})) + const { logCreatedNotification, logRejectedNotification, updateNotificationStatus, - findSuccessNotificationByIdAndEmail, - findFailNotificationByIdAndEmail + checkDuplicateNotification } = await import('../../../app/repos/notification-log.js') describe('Notification Log Repository', () => { @@ -101,8 +108,8 @@ describe('Notification Log Repository', () => { } ) - describe('findSuccessNotificationByIdAndEmail', () => { - test('should return existing notification when found', async () => { + describe('checkDuplicateNotification', () => { + test('should return true if notification exists with status "created"', async () => { const mockNotification = { notifyResponseId: '123456789', message: { @@ -111,10 +118,9 @@ describe('Notification Log Repository', () => { recipient: 'test@example.com', status: 'created' } - mockFindOne.mockResolvedValue(mockNotification) - const result = await findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com') + const result = await checkDuplicateNotification('test-message-id', 'test@example.com') expect(mockFindOne).toHaveBeenCalledWith({ where: { @@ -122,89 +128,59 @@ describe('Notification Log Repository', () => { recipient: 'test@example.com' } }) - expect(result).toEqual(mockNotification) + expect(result).toBe(true) }) - test('should return null when notification is not found', async () => { - mockFindOne.mockResolvedValue(null) - - const result = await findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com') - - expect(mockFindOne).toHaveBeenCalledWith({ - where: { - 'message.id': 'test-message-id', - recipient: 'test@example.com' - } - }) - expect(result).toBeNull() - }) - - test('should handle database errors appropriately', async () => { - const mockError = new Error('Database connection failed') - mockFindOne.mockRejectedValue(mockError) - - await expect(findSuccessNotificationByIdAndEmail('test-message-id', 'test@example.com')) - .rejects.toThrow('Database connection failed') + test('should return true if notification exists with status "sending"', async () => { + const mockNotification = { + status: 'sending' + } + mockFindOne.mockResolvedValue(mockNotification) - expect(mockFindOne).toHaveBeenCalledWith({ - where: { - 'message.id': 'test-message-id', - recipient: 'test@example.com' - } - }) + const result = await checkDuplicateNotification('test-message-id', 'test@example.com') + expect(result).toBe(true) }) - }) - describe('findFailNotificationByIdAndEmail', () => { - test('should return existing failed notification when found', async () => { + test('should return true if notification exists with status "delivered"', async () => { const mockNotification = { - message: { - id: 'test-message-id' - }, - recipient: 'test@example.com', - error: 'some error' + status: 'delivered' } + mockFindOne.mockResolvedValue(mockNotification) - mockFailureFindOne.mockResolvedValue(mockNotification) - - const result = await findFailNotificationByIdAndEmail('test-message-id', 'test@example.com') - - expect(mockFailureFindOne).toHaveBeenCalledWith({ - where: { - 'message.id': 'test-message-id', - recipient: 'test@example.com' - } - }) - expect(result).toEqual(mockNotification) + const result = await checkDuplicateNotification('test-message-id', 'test@example.com') + expect(result).toBe(true) }) - test('should return null when failed notification is not found', async () => { - mockFailureFindOne.mockResolvedValue(null) - - const result = await findFailNotificationByIdAndEmail('test-message-id', 'test@example.com') + test('should return undefined if notification exists with failure status', async () => { + const mockNotification = { + status: 'permanent-failure' + } + mockFindOne.mockResolvedValue(mockNotification) - expect(mockFailureFindOne).toHaveBeenCalledWith({ - where: { - 'message.id': 'test-message-id', - recipient: 'test@example.com' - } - }) - expect(result).toBeNull() + const result = await checkDuplicateNotification('test-message-id', 'test@example.com') + expect(result).toBeUndefined() }) test('should handle database errors appropriately', async () => { const mockError = new Error('Database connection failed') - mockFailureFindOne.mockRejectedValue(mockError) + mockFindOne.mockRejectedValue(mockError) - await expect(findFailNotificationByIdAndEmail('test-message-id', 'test@example.com')) + await expect(checkDuplicateNotification('test-message-id', 'test@example.com')) .rejects.toThrow('Database connection failed') - expect(mockFailureFindOne).toHaveBeenCalledWith({ + expect(mockFindOne).toHaveBeenCalledWith({ where: { 'message.id': 'test-message-id', recipient: 'test@example.com' } }) }) + + test('should return undefined when notification is not found', async () => { + mockFindOne.mockResolvedValue(null) + + const result = await checkDuplicateNotification('test-message-id', 'test@example.com') + expect(result).toBeUndefined() + }) }) }) diff --git a/test/unit/utils/check-duplicate-notification.test.js b/test/unit/utils/check-duplicate-notification.test.js deleted file mode 100644 index d2b2536..0000000 --- a/test/unit/utils/check-duplicate-notification.test.js +++ /dev/null @@ -1,139 +0,0 @@ -import { jest } from '@jest/globals' - -jest.unstable_mockModule('../../../app/repos/notification-log.js', () => ({ - findSuccessNotificationByIdAndEmail: jest.fn(), - findFailNotificationByIdAndEmail: jest.fn() -})) - -jest.unstable_mockModule('../../../app/constants/notify-statuses.js', () => ({ - default: { - SENDING: 'sending', - DELIVERED: 'delivered', - CREATED: 'created', - PERMANENT_FAILURE: 'permanent-failure', - TEMPORARY_FAILURE: 'temporary-failure', - TECHNICAL_FAILURE: 'technical-failure', - INTERNAL_FAILURE: 'internal-failure', - VALIDATION_FAILURE: 'validation-failure' - } -})) - -const { findSuccessNotificationByIdAndEmail, findFailNotificationByIdAndEmail } = await import('../../../app/repos/notification-log.js') -const { checkDuplicateNotification } = await import('../../../app/utils/check-duplicate-notification.js') - -describe('checkDuplicateNotification', () => { - beforeEach(() => { - jest.clearAllMocks() - console.log = jest.fn() - }) - - const mockMessage = { - id: 'test-message-id', - data: { - content: 'test content' - } - } - - describe('single email address', () => { - test('should throw error when notification exists with status "sending"', async () => { - const existingNotification = { - id: 'notification-id', - status: 'sending' - } - findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) - - await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) - .rejects.toThrow('Duplicate notification detected') - - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') - expect(findFailNotificationByIdAndEmail).not.toHaveBeenCalled() - }) - - test('should throw error when notification exists with status "delivered"', async () => { - const existingNotification = { - id: 'notification-id', - status: 'delivered' - } - findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) - - await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) - .rejects.toThrow('Duplicate notification detected') - }) - - test('should throw error when notification exists with status "created"', async () => { - const existingNotification = { - id: 'notification-id', - status: 'created' - } - findSuccessNotificationByIdAndEmail.mockResolvedValue(existingNotification) - - await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) - .rejects.toThrow('Duplicate notification detected') - }) - - test('should check for failed notification when no success notification exists', async () => { - findSuccessNotificationByIdAndEmail.mockResolvedValue(null) - findFailNotificationByIdAndEmail.mockResolvedValue(null) - - await checkDuplicateNotification(mockMessage, 'test@example.com') - - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') - expect(findFailNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test@example.com') - }) - - test('should allow resend when failed notification exists', async () => { - findSuccessNotificationByIdAndEmail.mockResolvedValue(null) - findFailNotificationByIdAndEmail.mockResolvedValue({ id: 'failed-notification-id' }) - - const result = await checkDuplicateNotification(mockMessage, 'test@example.com') - - expect(result).toBeNull() - expect(console.log).toHaveBeenCalledWith('resending failed notification') - }) - }) - - describe('multiple email addresses', () => { - const emailAddresses = ['test1@example.com', 'test2@example.com'] - - test('should check each email address for duplicates', async () => { - findSuccessNotificationByIdAndEmail.mockResolvedValue(null) - findFailNotificationByIdAndEmail.mockResolvedValue(null) - - await checkDuplicateNotification(mockMessage, emailAddresses) - - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledTimes(2) - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test1@example.com') - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledWith('test-message-id', 'test2@example.com') - }) - - test('should throw error if any email has an active notification', async () => { - findSuccessNotificationByIdAndEmail - .mockResolvedValueOnce(null) - .mockResolvedValueOnce({ id: 'notification-id', status: 'sending' }) - - await expect(checkDuplicateNotification(mockMessage, emailAddresses)) - .rejects.toThrow('Duplicate notification detected') - - expect(findSuccessNotificationByIdAndEmail).toHaveBeenCalledTimes(2) - }) - }) - - describe('error handling', () => { - test('should handle database errors from findSuccessNotificationByIdAndEmail', async () => { - const dbError = new Error('Database error') - findSuccessNotificationByIdAndEmail.mockRejectedValue(dbError) - - await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) - .rejects.toThrow('Database error') - }) - - test('should handle database errors from findFailNotificationByIdAndEmail', async () => { - findSuccessNotificationByIdAndEmail.mockResolvedValue(null) - const dbError = new Error('Database error') - findFailNotificationByIdAndEmail.mockRejectedValue(dbError) - - await expect(checkDuplicateNotification(mockMessage, 'test@example.com')) - .rejects.toThrow('Database error') - }) - }) -}) From b13eb1084c055de8f30a9770e14a129162b11d69 Mon Sep 17 00:00:00 2001 From: jungabunga Date: Thu, 13 Feb 2025 09:47:24 +0000 Subject: [PATCH 12/15] resolved sonar --- app/repos/notification-log.js | 4 +--- test/unit/repos/notification-log.test.js | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/repos/notification-log.js b/app/repos/notification-log.js index 7277452..1b1345a 100644 --- a/app/repos/notification-log.js +++ b/app/repos/notification-log.js @@ -64,9 +64,7 @@ const checkDuplicateNotification = async (messageId, recipient) => { } }) - if (existing && nonFailureStatuses.includes(existing.status)) { - return true - } + return existing && nonFailureStatuses.includes(existing.status) } export { diff --git a/test/unit/repos/notification-log.test.js b/test/unit/repos/notification-log.test.js index 7ce77bb..ebd1923 100644 --- a/test/unit/repos/notification-log.test.js +++ b/test/unit/repos/notification-log.test.js @@ -158,7 +158,7 @@ describe('Notification Log Repository', () => { mockFindOne.mockResolvedValue(mockNotification) const result = await checkDuplicateNotification('test-message-id', 'test@example.com') - expect(result).toBeUndefined() + expect(result).toBe(false) }) test('should handle database errors appropriately', async () => { @@ -180,7 +180,7 @@ describe('Notification Log Repository', () => { mockFindOne.mockResolvedValue(null) const result = await checkDuplicateNotification('test-message-id', 'test@example.com') - expect(result).toBeUndefined() + expect(result).toBeNull() }) }) }) From 8121b043e241ddc48ad06e7c56cf4617036820ea Mon Sep 17 00:00:00 2001 From: jungabunga Date: Thu, 13 Feb 2025 12:06:38 +0000 Subject: [PATCH 13/15] removing artifacts and improving tests --- app/messages/inbound/comms-request/handler.js | 2 -- .../comms-request/send-notification.js | 2 +- app/repos/notification-log.js | 8 +++-- .../comms-request/send-notification.test.js | 18 ++++++++---- test/unit/repos/notification-log.test.js | 29 +++++++------------ 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/app/messages/inbound/comms-request/handler.js b/app/messages/inbound/comms-request/handler.js index 8a39fca..5d154fd 100644 --- a/app/messages/inbound/comms-request/handler.js +++ b/app/messages/inbound/comms-request/handler.js @@ -4,7 +4,6 @@ import { } from '../../../schemas/comms-request/index.js' import { publishInvalidRequest, publishReceived } from '../../outbound/notification-status/publish.js' import { sendNotification } from './send-notification.js' -import { checkDuplicateNotification } from '../../../repos/notification-log.js' const handleCommsRequest = async (message, receiver) => { const commsRequest = message.body @@ -25,7 +24,6 @@ const handleCommsRequest = async (message, receiver) => { return } - await checkDuplicateNotification(validated, validated.data.commsAddresses) await publishReceived(validated) await sendNotification(validated) diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index 2faade6..c87bd53 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -31,7 +31,7 @@ const sendNotification = async (message) => { const duplicate = await checkDuplicateNotification(message.id, emailAddress) if (duplicate) { - console.warn('Duplicate notification detected:', duplicate) + console.warn('Duplicate notification detected') continue } diff --git a/app/repos/notification-log.js b/app/repos/notification-log.js index 1b1345a..f9384cb 100644 --- a/app/repos/notification-log.js +++ b/app/repos/notification-log.js @@ -1,7 +1,11 @@ import db from '../data/index.js' import notifyStatus from '../constants/notify-statuses.js' -const nonFailureStatuses = [notifyStatus.CREATED, notifyStatus.SENDING, notifyStatus.DELIVERED] +const nonFailureStatuses = [ + notifyStatus.CREATED, + notifyStatus.SENDING, + notifyStatus.DELIVERED +] const logCreatedNotification = async (message, recipient, notificationId) => { await db.notifyApiRequestSuccess.create({ @@ -64,7 +68,7 @@ const checkDuplicateNotification = async (messageId, recipient) => { } }) - return existing && nonFailureStatuses.includes(existing.status) + return nonFailureStatuses.includes(existing?.status) } export { diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 2f93822..2461069 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -35,11 +35,15 @@ const { publishStatus } = await import('../../../../../app/messages/outbound/not const { sendNotification } = await import('../../../../../app/messages/inbound/comms-request/send-notification.js') console.log = jest.fn() -console.warn = jest.fn() describe('Send Notification', () => { + let consoleWarnSpy + let consoleErrorSpy + beforeEach(() => { jest.clearAllMocks() + consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation() + consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation() mockGetNotifyStatus.mockResolvedValue(null) mockSendEmail.mockResolvedValue({ data: { @@ -47,6 +51,12 @@ describe('Send Notification', () => { } }) }) + + afterEach(() => { + consoleWarnSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) + test('should skip sending when duplicate notification is detected', async () => { const message = { id: 'message-id', @@ -66,7 +76,7 @@ describe('Send Notification', () => { expect(checkDuplicateNotification).toHaveBeenCalledWith('message-id', 'mock-email@test.com') expect(mockSendEmail).not.toHaveBeenCalled() - expect(console.warn).toHaveBeenCalledWith('Duplicate notification detected:', true) + expect(consoleWarnSpy).toHaveBeenCalledWith('Duplicate notification detected') }) test('should send an email with the correct arguments to a single email address', async () => { @@ -150,7 +160,6 @@ describe('Send Notification', () => { test('should log an error message when sendEmail fails', async () => { const uuidSpy = jest.spyOn(crypto, 'randomUUID').mockReturnValue('mock-uuid') - const consoleSpy = jest.spyOn(console, 'error') const message = { data: { @@ -181,9 +190,8 @@ describe('Send Notification', () => { await sendNotification(message) - expect(consoleSpy).toHaveBeenCalledWith('Error sending email with code:', 400) + expect(consoleErrorSpy).toHaveBeenCalledWith('Error sending email with code:', 400) - consoleSpy.mockRestore() uuidSpy.mockRestore() }) diff --git a/test/unit/repos/notification-log.test.js b/test/unit/repos/notification-log.test.js index ebd1923..47d3c82 100644 --- a/test/unit/repos/notification-log.test.js +++ b/test/unit/repos/notification-log.test.js @@ -1,4 +1,5 @@ import { jest } from '@jest/globals' +import notifyStatus from '../../../app/constants/notify-statuses.js' const mockCreate = jest.fn() const mockFindOne = jest.fn() @@ -17,14 +18,6 @@ jest.unstable_mockModule('../../../app/data/index.js', () => ({ } })) -jest.unstable_mockModule('../../../app/constants/notify-statuses.js', () => ({ - default: { - CREATED: 'created', - SENDING: 'sending', - DELIVERED: 'delivered' - } -})) - const { logCreatedNotification, logRejectedNotification, @@ -50,7 +43,7 @@ describe('Notification Log Repository', () => { createdAt: expect.any(Date), notifyResponseId: '123456789', message, - status: 'created', + status: notifyStatus.CREATED, statusUpdatedAt: expect.any(Date), completed: null, recipient: 'mock-email@test.gov.uk' @@ -75,7 +68,7 @@ describe('Notification Log Repository', () => { async (status) => { const notification = { save: jest.fn(), - status: 'created', + status: notifyStatus.CREATED, statusUpdatedAt: new Date(), completed: null } @@ -95,7 +88,7 @@ describe('Notification Log Repository', () => { const notification = { save: jest.fn(), - status: 'created', + status: notifyStatus.CREATED, statusUpdatedAt: new Date(), completed: null } @@ -116,7 +109,7 @@ describe('Notification Log Repository', () => { id: 'test-message-id' }, recipient: 'test@example.com', - status: 'created' + status: notifyStatus.CREATED } mockFindOne.mockResolvedValue(mockNotification) @@ -133,7 +126,7 @@ describe('Notification Log Repository', () => { test('should return true if notification exists with status "sending"', async () => { const mockNotification = { - status: 'sending' + status: notifyStatus.SENDING } mockFindOne.mockResolvedValue(mockNotification) @@ -143,7 +136,7 @@ describe('Notification Log Repository', () => { test('should return true if notification exists with status "delivered"', async () => { const mockNotification = { - status: 'delivered' + status: notifyStatus.DELIVERED } mockFindOne.mockResolvedValue(mockNotification) @@ -151,9 +144,9 @@ describe('Notification Log Repository', () => { expect(result).toBe(true) }) - test('should return undefined if notification exists with failure status', async () => { + test('should return false if notification exists with failure status', async () => { const mockNotification = { - status: 'permanent-failure' + status: notifyStatus.PERMANENT_FAILURE } mockFindOne.mockResolvedValue(mockNotification) @@ -176,11 +169,11 @@ describe('Notification Log Repository', () => { }) }) - test('should return undefined when notification is not found', async () => { + test('should return false when notification is not found', async () => { mockFindOne.mockResolvedValue(null) const result = await checkDuplicateNotification('test-message-id', 'test@example.com') - expect(result).toBeNull() + expect(result).toBe(false) }) }) }) From 4135e6a76626830a54f7bd00c80f95790f4de158 Mon Sep 17 00:00:00 2001 From: Adam Kay Date: Fri, 14 Feb 2025 15:54:48 +0000 Subject: [PATCH 14/15] WIP: Adds message retry and simulates a 500 error --- app/messages/inbound/comms-request/handler.js | 15 ++++- .../comms-request/send-notification.js | 62 ++++++++++++++----- .../comms-request/send-notification.test.js | 56 ++++++++++++++--- 3 files changed, 106 insertions(+), 27 deletions(-) diff --git a/app/messages/inbound/comms-request/handler.js b/app/messages/inbound/comms-request/handler.js index 5d154fd..86ddd83 100644 --- a/app/messages/inbound/comms-request/handler.js +++ b/app/messages/inbound/comms-request/handler.js @@ -21,13 +21,22 @@ const handleCommsRequest = async (message, receiver) => { } await receiver.deadLetterMessage(message) - return } + await publishReceived(validated) - await sendNotification(validated) - await receiver.completeMessage(message) + try { + await sendNotification(validated, receiver) + await receiver.completeMessage(message) + } catch (error) { + if (error.message === 'NOTIFY_RETRY_ERROR') { + console.log('Abandoning message for retry due to notify 500 error') + await receiver.abandonMessage(message) + return + } + throw error + } } catch (error) { console.error('Error handling message: ', error) await receiver.abandonMessage(message) diff --git a/app/messages/inbound/comms-request/send-notification.js b/app/messages/inbound/comms-request/send-notification.js index c87bd53..57096c5 100644 --- a/app/messages/inbound/comms-request/send-notification.js +++ b/app/messages/inbound/comms-request/send-notification.js @@ -1,28 +1,50 @@ -import crypto from 'crypto' -import notifyClient from '../../../clients/notify-client.js' +// import crypto from 'crypto' +// import notifyClient from '../../../clients/notify-client.js' import notifyStatus from '../../../constants/notify-statuses.js' import { logCreatedNotification, logRejectedNotification, checkDuplicateNotification } from '../../../repos/notification-log.js' import { publishStatus } from '../../outbound/notification-status/publish.js' const trySendViaNotify = async (message, emailAddress) => { + // temporarily removed for testing purposes +// try { +// const response = await notifyClient.sendEmail( +// message.data.notifyTemplateId, +// emailAddress, { +// personalisation: message.data.personalisation, +// reference: crypto.randomUUID() +// } +// ) + + // return [response, null] + // } catch (error) { + // const statusCode = error.response?.data?.status_code + // console.error('Error sending email with code:', statusCode) + // return [null, error] + // } + // } + + // Simulate a 500 error for testing purposes try { - const response = await notifyClient.sendEmail( - message.data.notifyTemplateId, - emailAddress, { - personalisation: message.data.personalisation, - reference: crypto.randomUUID() + const error = new Error('Simulated 500 error') + error.response = { + status: 500, + data: { + errors: [ + { + error: 'simulated-error' + } + ] } - ) - - return [response, null] + } + throw error } catch (error) { - const statusCode = error.response?.data?.status_code + const statusCode = error.response?.status console.error('Error sending email with code:', statusCode) return [null, error] } } -const sendNotification = async (message) => { +const sendNotification = async (message, receiver) => { const emailAddresses = Array.isArray(message.data.commsAddresses) ? message.data.commsAddresses : [message.data.commsAddresses] @@ -42,12 +64,20 @@ const sendNotification = async (message) => { await publishStatus(message, emailAddress, notifyStatus.SENDING) await logCreatedNotification(message, emailAddress, response.data.id) } else { - const status = notifyStatus.INTERNAL_FAILURE - const notifyErrorData = notifyError.response.data - await publishStatus(message, emailAddress, status, notifyErrorData) - await logRejectedNotification(message, emailAddress, notifyError) + if (notifyError.response.status === 500) { + throw new Error('NOTIFY_RETRY_ERROR') + } else { + const status = notifyStatus.INTERNAL_FAILURE + const notifyErrorData = notifyError.response.data + await publishStatus(message, emailAddress, status, notifyErrorData) + await logRejectedNotification(message, emailAddress, notifyError) + } } } catch (error) { + if (error.message === 'NOTIFY_RETRY_ERROR') { + // Re-throw to be caught by handler + throw error + } console.error('Error logging notification:', error) } } diff --git a/test/unit/messages/inbound/comms-request/send-notification.test.js b/test/unit/messages/inbound/comms-request/send-notification.test.js index 2461069..e28cbe6 100644 --- a/test/unit/messages/inbound/comms-request/send-notification.test.js +++ b/test/unit/messages/inbound/comms-request/send-notification.test.js @@ -39,6 +39,7 @@ console.log = jest.fn() describe('Send Notification', () => { let consoleWarnSpy let consoleErrorSpy + let mockReceiver beforeEach(() => { jest.clearAllMocks() @@ -50,6 +51,11 @@ describe('Send Notification', () => { id: 'mock-notify-response-id' } }) + mockReceiver = { + abandonMessage: jest.fn(), + completeMessage: jest.fn(), + deadLetterMessage: jest.fn() + } }) afterEach(() => { @@ -72,7 +78,7 @@ describe('Send Notification', () => { checkDuplicateNotification.mockResolvedValueOnce(true) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(checkDuplicateNotification).toHaveBeenCalledWith('message-id', 'mock-email@test.com') expect(mockSendEmail).not.toHaveBeenCalled() @@ -94,7 +100,7 @@ describe('Send Notification', () => { } } - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(mockSendEmail).toHaveBeenCalled() expect(mockSendEmail).toHaveBeenCalledWith( @@ -127,7 +133,7 @@ describe('Send Notification', () => { } } - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(mockSendEmail).toHaveBeenCalledTimes(2) @@ -188,7 +194,7 @@ describe('Send Notification', () => { mockSendEmail.mockRejectedValue(mockError) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(consoleErrorSpy).toHaveBeenCalledWith('Error sending email with code:', 400) @@ -214,7 +220,7 @@ describe('Send Notification', () => { } }) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(logCreatedNotification).toHaveBeenCalledTimes(1) expect(logCreatedNotification).toHaveBeenCalledWith(message, 'mock-email@test.com', 'mock-notify-response-id') @@ -239,7 +245,7 @@ describe('Send Notification', () => { } }) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(publishStatus).toHaveBeenCalledTimes(1) expect(publishStatus).toHaveBeenCalledWith(message, 'mock-email@test.com', 'sending') @@ -273,7 +279,7 @@ describe('Send Notification', () => { mockSendEmail.mockRejectedValue(mockError) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(logRejectedNotification).toHaveBeenCalledTimes(1) expect(logRejectedNotification).toHaveBeenCalledWith(message, 'mock-email@test.com', mockError) @@ -309,9 +315,43 @@ describe('Send Notification', () => { mockSendEmail.mockRejectedValue(mockError) - await sendNotification(message) + await sendNotification(message, mockReceiver) expect(publishStatus).toHaveBeenCalledTimes(1) expect(publishStatus).toHaveBeenCalledWith(message, 'mock-email@test.com', 'internal-failure', mockError.response.data) }) + + test('should log an internal failure and abandon message when notify returns a 500 status code', async () => { + const message = { + id: 'message-id', + data: { + notifyTemplateId: 'mock-notify-template-id', + commsAddresses: 'mock-email@test.com', + personalisation: { + reference: 'mock-reference', + agreementSummaryLink: 'https://test.com/mock-agreeement-summary-link' + } + } + } + + const mockError = { + response: { + status: 500, + data: { + errors: [ + { + error: 'mock-error' + } + ] + } + } + } + + mockSendEmail.mockRejectedValue(mockError) + + await sendNotification(message, mockReceiver) + + expect(consoleErrorSpy).toHaveBeenCalledWith('Internal failure sending notification:', mockError) + expect(mockReceiver.abandonMessage).toHaveBeenCalledWith(message) + }) }) From a3fc683e058d24cb0b0a684ce8c84a1e0eb6b1ea Mon Sep 17 00:00:00 2001 From: Adam Kay Date: Fri, 14 Feb 2025 16:57:14 +0000 Subject: [PATCH 15/15] WIP: AC4 - Event is pushed to data layer if max attempts exceeded --- app/messages/inbound/comms-request/handler.js | 37 ++++++++++++++++++- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/app/messages/inbound/comms-request/handler.js b/app/messages/inbound/comms-request/handler.js index 86ddd83..d1cddee 100644 --- a/app/messages/inbound/comms-request/handler.js +++ b/app/messages/inbound/comms-request/handler.js @@ -2,8 +2,9 @@ import { validate } from '../../../schemas/validate.js' import { v3 as commsSchema } from '../../../schemas/comms-request/index.js' -import { publishInvalidRequest, publishReceived } from '../../outbound/notification-status/publish.js' +import { publishInvalidRequest, publishReceived, publishStatus } from '../../outbound/notification-status/publish.js' import { sendNotification } from './send-notification.js' +import notifyStatus from '../../../constants/notify-statuses.js' const handleCommsRequest = async (message, receiver) => { const commsRequest = message.body @@ -31,7 +32,39 @@ const handleCommsRequest = async (message, receiver) => { await receiver.completeMessage(message) } catch (error) { if (error.message === 'NOTIFY_RETRY_ERROR') { - console.log('Abandoning message for retry due to notify 500 error') + const deliveryCount = message.deliveryCount || 1 + + console.log(`Current delivery attempt: ${deliveryCount} of 10`) + + // our logic here and the interaction with service bus needs to be tested and checked more thoroughly + if (deliveryCount === 8) { // if i change this to > 9 the message will be moved to the dead letter queue by azure service bus by default so need to understand how to handle this better + console.log('Maximum retry attempts reached') + + try { + console.log('Publishing technical failure status to data layer...') + await publishStatus( + validated, + validated.data.commsAddresses, + notifyStatus.TECHNICAL_FAILURE, + { + error: 'Maximum retry attempts exceeded', + deliveryCount, + messageId: message.messageId + } + ) + console.log('Status published successfully') + + console.log('Moving message to dead letter queue...') + await receiver.deadLetterMessage(message) + console.log('Message moved to dead letter queue') + } catch (statusError) { + console.error('Error during max attempts handling:', statusError) + throw statusError + } + return + } + + console.log(`Abandoning message for retry. Attempt ${deliveryCount} of 10`) await receiver.abandonMessage(message) return }