From 6dabcfae6b1709cfbf9e8c9f3b865adde1b1048d Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 14:51:46 +0000 Subject: [PATCH] Add missing SubmitSendBillRunService unit tests (#851) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://eaflood.atlassian.net/browse/WATER-4390 In [Migrate legacy send bill run functionality](https://github.com/DEFRA/water-abstraction-system/pull/771) we replaced the mad-as-a-box-of-frogs legacy send functionality with our own! Only our supposed leader of tech and keeper of quality @Cruikshanks completely missed adding unit tests for the primary `SubmitSendBillRunService`! 🤦 This change adds the missing tests. --- Note the change in the `SubmitSendService`. We have been working under the impression that we can return early from an async function by not awaiting the final function. Whilst in theory this does appear to work it's probably not how we should be doing it. This is because for this service (`SubmitCancelService` is the other place we are following this pattern) we are purposefully throwing an error assuming the `try/catch` in the controller will see it and log it. When it came to trying to write a unit test for it we found we just kept blowing up [Hapi/Lab](https://hapi.dev/module/lab/). So, we then tried it for real, forcing `ChargingModuleViewBillRunRequest` to throw an error. We confirmed the controller's try/catch doesn't see the error and we caused 'system' to crash. The only way to catch the error is to chain `.catch()` directly onto the call. We were only intending to log the error so what we're doing is now fine. But it is worth noting and we may need to revisit our foreground/background pattern. --- .../submit-cancel-bill-run.service.js | 9 +- .../bill-runs/submit-send-bill-run.service.js | 18 +- .../bill-runs/submit-send.service.test.js | 177 ++++++++++++++++++ 3 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 test/services/bill-runs/submit-send.service.test.js diff --git a/app/services/bill-runs/submit-cancel-bill-run.service.js b/app/services/bill-runs/submit-cancel-bill-run.service.js index b5b40b6af1..a16e38bd8a 100644 --- a/app/services/bill-runs/submit-cancel-bill-run.service.js +++ b/app/services/bill-runs/submit-cancel-bill-run.service.js @@ -48,7 +48,14 @@ async function go (billRunId) { await _updateStatusToCancel(billRunId) - _cancelBillRun(billRun) + // NOTE: We originally believed that should anything error in _cancelBillRun() the try/catch in the controller would + // catch it. However, when testing this theory we found it crashed the test framework. So, we tried it for real and + // again confirmed we'd crash the service. The controller would never see the error. It is because we are not awaiting + // this call that any errors thrown are considered uncaught. However, if we instead use the ES6 variant the error _is_ + // caught. All we can do at this point is log it. + _cancelBillRun(billRun).catch((error) => { + global.GlobalNotifier.omfg(error.message, { billRunId }, error) + }) } async function _cancelBillRun (billRun) { diff --git a/app/services/bill-runs/submit-send-bill-run.service.js b/app/services/bill-runs/submit-send-bill-run.service.js index 66dca2bb7e..e2ddaefa09 100644 --- a/app/services/bill-runs/submit-send-bill-run.service.js +++ b/app/services/bill-runs/submit-send-bill-run.service.js @@ -35,12 +35,19 @@ async function go (billRunId) { const billRun = await _fetchBillRun(billRunId) if (billRun.status !== 'ready') { - return + throw new ExpandedError('Cannot send a bill run that is not ready', { billRunId }) } await _updateStatus(billRunId, 'sending') - _sendBillRun(billRun) + // NOTE: We originally believed that should anything error in _sendBillRun() the try/catch in the controller would + // catch it. However, when testing this theory we found it crashed the test framework. So, we tried it for real and + // again confirmed we'd crash the service. The controller would never see the error. It is because we are not awaiting + // this call that any errors thrown are considered uncaught. However, if we instead use the ES6 variant the error _is_ + // caught. All we can do at this point is log it. + _sendBillRun(billRun).catch((error) => { + global.GlobalNotifier.omfg(error.message, { billRunId }, error) + }) } /** @@ -68,12 +75,7 @@ async function _fetchChargingModuleBillRun (externalId) { const result = await ChargingModuleViewBillRunRequest.send(externalId) if (!result.succeeded) { - const error = new ExpandedError( - 'Charging Module view bill run request failed', - { billRunExternalId: externalId, responseBody: result.response.body } - ) - - throw error + throw new ExpandedError('Charging Module view bill run request failed', { billRunExternalId: externalId }) } return result.response.body.billRun diff --git a/test/services/bill-runs/submit-send.service.test.js b/test/services/bill-runs/submit-send.service.test.js new file mode 100644 index 0000000000..15c93b5bb5 --- /dev/null +++ b/test/services/bill-runs/submit-send.service.test.js @@ -0,0 +1,177 @@ +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') +const Sinon = require('sinon') + +const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script() +const { expect } = Code + +// Test helpers +const { setTimeout } = require('timers/promises') + +const BillHelper = require('../../support/helpers/bill.helper.js') +const BillRunHelper = require('../../support/helpers/bill-run.helper.js') +const DatabaseSupport = require('../../support/database.js') +const ExpandedError = require('../../../app/errors/expanded.error.js') + +// Things we need to stub +const ChargingModuleSendBillRunRequest = require('../../../app/requests/charging-module/send-bill-run.request.js') +const ChargingModuleViewBillRunRequest = require('../../../app/requests/charging-module/view-bill-run.request.js') + +// Thing under test +const SubmitSendBillBunService = require('../../../app/services/bill-runs/submit-send-bill-run.service.js') + +describe('Submit Cancel Bill Run service', () => { + // NOTE: introducing a delay in the tests is not ideal. But the service is written such that the send happens in the + // background and is not awaited. We want to confirm things like the records have been updated. But the only way to do + // so is to give the background process time to complete. + const delay = 500 + + let chargingModuleSendBillRunRequestStub + let chargingModuleViewBillRunRequestStub + let notifierStub + + beforeEach(async () => { + await DatabaseSupport.clean() + + chargingModuleSendBillRunRequestStub = Sinon.stub(ChargingModuleSendBillRunRequest, 'send').resolves() + chargingModuleViewBillRunRequestStub = Sinon.stub(ChargingModuleViewBillRunRequest, 'send') + + // The service depends on GlobalNotifier to have been set. This happens in app/plugins/global-notifier.plugin.js + // when the app starts up and the plugin is registered. As we're not creating an instance of Hapi server in this + // test we recreate the condition by setting it directly with our own stub + notifierStub = { omg: Sinon.stub(), omfg: Sinon.stub() } + global.GlobalNotifier = notifierStub + }) + + afterEach(() => { + Sinon.restore() + }) + + describe('when the bill run exists', () => { + let firstBill + let secondBill + let billRun + + describe("and its status is 'ready'", () => { + beforeEach(async () => { + billRun = await BillRunHelper.add({ externalId: '8212003e-059a-4cf2-94c2-da9dfae90a80', status: 'ready' }) + }) + + describe('and the /send request to the Charging Module API succeeds', () => { + beforeEach(async () => { + firstBill = await BillHelper.add({ externalId: 'fe7b0de1-e8c0-42f0-ba66-42f816e67c62' }) + secondBill = await BillHelper.add({ externalId: '7b15c45e-9166-40a8-9b3f-1ee394f773dd' }) + + chargingModuleViewBillRunRequestStub.resolves({ + succeeded: true, + response: { + body: { + billRun: { + invoices: [ + { id: firstBill.externalId, transactionReference: 'WAI1000429' }, + { id: secondBill.externalId, transactionReference: 'WAI1000428' } + ], + transactionFileReference: 'nalwi50031' + } + } + } + }) + }) + + it('sends a request to the Charging Module API to send its copy', async () => { + await SubmitSendBillBunService.go(billRun.id) + + expect(chargingModuleSendBillRunRequestStub.called).to.be.true() + }) + + it('updates the bill run with the Charging Module transaction file reference', async () => { + await SubmitSendBillBunService.go(billRun.id) + + await setTimeout(delay) + + const refreshedBillRun = await billRun.$query() + + expect(refreshedBillRun.transactionFileReference).to.equal('nalwi50031') + }) + + it('updates the bills with the Charging Module invoice numbers', async () => { + await SubmitSendBillBunService.go(billRun.id) + + await setTimeout(delay) + + const refreshedFirstBill = await firstBill.$query() + expect(refreshedFirstBill.invoiceNumber).to.equal('WAI1000429') + + const refreshedSecondBill = await secondBill.$query() + expect(refreshedSecondBill.invoiceNumber).to.equal('WAI1000428') + }) + + it('logs the time taken in milliseconds and seconds', async () => { + await SubmitSendBillBunService.go(billRun.id) + + await setTimeout(delay) + + const logDataArg = notifierStub.omg.firstCall.args[1] + + expect( + notifierStub.omg.calledWith('Send bill run complete') + ).to.be.true() + expect(logDataArg.timeTakenMs).to.exist() + expect(logDataArg.timeTakenSs).to.exist() + expect(logDataArg.billRunId).to.exist() + }) + }) + + describe('but the request to view the Charging Module bill run fails', () => { + beforeEach(() => { + chargingModuleViewBillRunRequestStub.resolves({ + succeeded: false, + response: new Error('CHA go pop') + }) + }) + + it('logs an error', async () => { + await SubmitSendBillBunService.go(billRun.id) + + await setTimeout(delay) + + const errorLogArgs = notifierStub.omfg.firstCall.args + + expect( + notifierStub.omfg.calledWith('Charging Module view bill run request failed') + ).to.be.true() + expect(errorLogArgs[1].billRunId).to.exist() + expect(errorLogArgs[2]).to.be.instanceOf(ExpandedError) + expect(errorLogArgs[2].billRunExternalId).to.equal('8212003e-059a-4cf2-94c2-da9dfae90a80') + }) + }) + }) + + describe("but its status is not 'ready'", () => { + beforeEach(async () => { + billRun = await BillRunHelper.add({ status: 'sent' }) + }) + + it('throws as error', async () => { + const result = await expect(SubmitSendBillBunService.go(billRun.id)) + .to + .reject() + + expect(result).to.be.instanceOf(ExpandedError) + expect(result.message).to.equal('Cannot send a bill run that is not ready') + expect(result.billRunId).to.equal(billRun.id) + }) + }) + }) + + describe('when the bill run does not exist', () => { + it('throws an error', async () => { + await expect(SubmitSendBillBunService.go('testId')) + .to + .reject() + }) + }) +})