From 6ce785e5d1bb7dacfeeeec2704958d6b822bab2e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 13 Mar 2023 16:19:18 +0000 Subject: [PATCH] Handle errors in ProcessBillingBatchService (#161) https://eaflood.atlassian.net/browse/WATER-3924 Should an error occur during the supplementary bill run process it brings down the service. We're not handling the exception and because we moved the process to the background in our bill runs controller, there is nothing to deal with it when thrown. So, we need to protect the service from _any_ errors during the process, to prevent the service from going down. But we also need to mimic the legacy service and use an 'error code' to provide extra information about the error. We've already used this concept with [Create error bill run when CM fails](https://github.com/DEFRA/water-abstraction-system/pull/104) where we apply a code to the `billing_batch` record which the UI then uses to indicate what happened to the user. This change deals with both; ensuring we handle any exceptions raised and where possible, setting a relevant error code when updating the `billing_batch` status to 'errored'. --- .../handle-errored-billing-batch.service.js | 41 ++++ .../process-billing-batch.service.js | 230 ++++++++++++------ ...ndle-errored-billing-batch.service.test.js | 84 +++++++ .../process-billing-batch.service.test.js | 114 ++++++++- 4 files changed, 383 insertions(+), 86 deletions(-) create mode 100644 app/services/supplementary-billing/handle-errored-billing-batch.service.js create mode 100644 test/services/supplementary-billing/handle-errored-billing-batch.service.test.js diff --git a/app/services/supplementary-billing/handle-errored-billing-batch.service.js b/app/services/supplementary-billing/handle-errored-billing-batch.service.js new file mode 100644 index 0000000000..dd394d3165 --- /dev/null +++ b/app/services/supplementary-billing/handle-errored-billing-batch.service.js @@ -0,0 +1,41 @@ +'use strict' + +/** + * Handles an errored billing batch (setting status etc.) + * @module HandleErroredBillingBatchService + */ + +const BillingBatchModel = require('../../models/water/billing-batch.model.js') + +/** + * Sets the status of the specified billing batch to `error`, and logs an error if this can't be done. + * + * We keep this in a separate service so we don't need to worry about multiple/nested try-catch blocks in cases where a + * billing batch fails and setting its status to error also fails. + * + * Note that although this is async we would generally not call it asyncronously as the intent is you can call it and + * continue with whatever error logging is required + * + * @param {string} billingBatchId UUID of the billing batch to be marked with `error` status + * @param {number} [errorCode] Numeric error code as defined in BillingBatchModel. Defaults to `null` + */ +async function go (billingBatchId, errorCode = null) { + try { + await _updateBillingBatch(billingBatchId, errorCode) + } catch (error) { + global.GlobalNotifier.omfg('Failed to set error status on billing batch', { error, billingBatchId, errorCode }) + } +} + +async function _updateBillingBatch (billingBatchId, errorCode) { + await BillingBatchModel.query() + .findById(billingBatchId) + .patch({ + status: 'error', + errorCode + }) +} + +module.exports = { + go +} diff --git a/app/services/supplementary-billing/process-billing-batch.service.js b/app/services/supplementary-billing/process-billing-batch.service.js index 6050cc4dc4..143db79c18 100644 --- a/app/services/supplementary-billing/process-billing-batch.service.js +++ b/app/services/supplementary-billing/process-billing-batch.service.js @@ -18,8 +18,12 @@ const FetchChargeVersionsService = require('./fetch-charge-versions.service.js') const GenerateBillingTransactionsService = require('./generate-billing-transactions.service.js') const GenerateBillingInvoiceService = require('./generate-billing-invoice.service.js') const GenerateBillingInvoiceLicenceService = require('./generate-billing-invoice-licence.service.js') +const HandleErroredBillingBatchService = require('./handle-errored-billing-batch.service.js') const LegacyRequestLib = require('../../lib/legacy-request.lib.js') +let generatedInvoices = [] +let generatedInvoiceLicences = [] + /** * Creates the invoices and transactions in both WRLS and the Charging Module API * @@ -31,20 +35,37 @@ const LegacyRequestLib = require('../../lib/legacy-request.lib.js') async function go (billingBatch, billingPeriod) { const { billingBatchId } = billingBatch - await _updateStatus(billingBatchId, 'processing') + try { + await _updateStatus(billingBatchId, 'processing') + + const chargeVersions = await _fetchChargeVersions(billingBatch, billingPeriod) + + for (const chargeVersion of chargeVersions) { + const { licence } = chargeVersion - // We know in the future we will be calculating multiple billing periods and so will have to iterate through each, - // generating bill runs and reviewing if there is anything to bill. For now, whilst our knowledge of the process - // is low we are focusing on just the current financial year, and intending to ship a working version for just it. - // This is why we are only passing through the first billing period; we know there is only one! - const chargeVersions = await FetchChargeVersionsService.go(billingBatch.regionId, billingPeriod) + const billingInvoice = await _generateBillingInvoice(chargeVersion, billingBatchId, billingPeriod) + const billingInvoiceLicence = _generateBillingInvoiceLicence(billingInvoice, licence) - let generatedInvoices = [] - let generatedInvoiceLicences = [] + const transactionLines = _generateTransactionLines(billingPeriod, chargeVersion, billingBatchId) + + await _createTransactionLines( + transactionLines, + billingPeriod, + billingInvoice, + billingInvoiceLicence, + chargeVersion, + billingBatch + ) + } - for (const chargeVersion of chargeVersions) { - const { chargeElements, licence } = chargeVersion + await _finaliseBillingBatch(billingBatch) + } catch (error) { + global.GlobalNotifier.omfg('Billing Batch process errored', { billingBatch, error }) + } +} +async function _generateBillingInvoice (chargeVersion, billingBatchId, billingPeriod) { + try { const billingInvoiceData = await GenerateBillingInvoiceService.go( generatedInvoices, chargeVersion.invoiceAccountId, @@ -52,106 +73,147 @@ async function go (billingBatch, billingPeriod) { billingPeriod.endDate.getFullYear() ) generatedInvoices = billingInvoiceData.billingInvoices - const { billingInvoice } = billingInvoiceData + return billingInvoiceData.billingInvoice + } catch (error) { + HandleErroredBillingBatchService.go(billingBatchId) + + throw error + } +} + +function _generateBillingInvoiceLicence (billingInvoice, licence) { + try { const billingInvoiceLicenceData = GenerateBillingInvoiceLicenceService.go( generatedInvoiceLicences, - billingInvoiceData.billingInvoice.billingInvoiceId, + billingInvoice.billingInvoiceId, licence ) generatedInvoiceLicences = billingInvoiceLicenceData.billingInvoiceLicences - const { billingInvoiceLicence } = billingInvoiceLicenceData - - if (chargeElements) { - const transactionLines = _generateTransactionLines(billingPeriod, chargeVersion) - - if (transactionLines.length > 0) { - await _createTransactionLines( - transactionLines, - billingPeriod, - billingInvoice.invoiceAccountNumber, - billingInvoiceLicence.billingInvoiceLicenceId, - chargeVersion, - billingBatch.externalId - ) - - billingInvoice.persist = true - billingInvoiceLicence.persist = true - } - } + + return billingInvoiceLicenceData.billingInvoiceLicence + } catch (error) { + HandleErroredBillingBatchService.go(billingInvoice.billingBatchId) + + throw error } +} - await _finaliseBillingBatch(billingBatch, generatedInvoices, generatedInvoiceLicences) +async function _fetchChargeVersions (billingBatch, billingPeriod) { + try { + // We know in the future we will be calculating multiple billing periods and so will have to iterate through each, + // generating bill runs and reviewing if there is anything to bill. For now, whilst our knowledge of the process + // is low we are focusing on just the current financial year, and intending to ship a working version for just it. + // This is why we are only passing through the first billing period; we know there is only one! + return await FetchChargeVersionsService.go(billingBatch.regionId, billingPeriod) + } catch (error) { + HandleErroredBillingBatchService.go( + billingBatch.billingBatchId, + BillingBatchModel.errorCodes.failedToProcessChargeVersions + ) + + throw error + } } async function _createTransactionLines ( transactionLines, billingPeriod, - invoiceAccountNumber, - billingInvoiceLicenceId, + billingInvoice, + billingInvoiceLicence, chargeVersion, - chargingModuleBillRunId + billingBatch ) { - for (const transaction of transactionLines) { - const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go( - transaction, - billingPeriod, - invoiceAccountNumber, - chargeVersion.licence - ) + if (transactionLines.length === 0) { + return + } + + try { + for (const transaction of transactionLines) { + const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go( + transaction, + billingPeriod, + billingInvoice.invoiceAccountNumber, + chargeVersion.licence + ) - const chargingModuleResponse = await ChargingModuleCreateTransactionService.go(chargingModuleBillRunId, chargingModuleRequest) + const chargingModuleResponse = await ChargingModuleCreateTransactionService.go(billingBatch.externalId, chargingModuleRequest) - // TODO: Handle a failed request - transaction.status = 'charge_created' - transaction.externalId = chargingModuleResponse.response.body.transaction.id - transaction.billingInvoiceLicenceId = billingInvoiceLicenceId + transaction.status = 'charge_created' + transaction.externalId = chargingModuleResponse.response.body.transaction.id + transaction.billingInvoiceLicenceId = billingInvoiceLicence.billingInvoiceLicenceId + + await CreateBillingTransactionService.go(transaction) + } - await CreateBillingTransactionService.go(transaction) + billingInvoice.persist = true + billingInvoiceLicence.persist = true + } catch (error) { + HandleErroredBillingBatchService.go( + billingBatch.billingBatchId, + BillingBatchModel.errorCodes.failedToCreateCharge + ) + + throw error } } -async function _finaliseBillingBatch (billingBatch, generatedInvoices, generatedInvoiceLicences) { - const { billingBatchId, externalId } = billingBatch +async function _finaliseBillingBatch (billingBatch) { + try { + const { billingBatchId, externalId } = billingBatch - const billingInvoicesToInsert = generatedInvoices.filter((billingInvoice) => billingInvoice.persist) + const billingInvoicesToInsert = generatedInvoices.filter((billingInvoice) => billingInvoice.persist) - // The bill run is considered empty. We just need to set the status to indicate this in the UI - if (billingInvoicesToInsert.length === 0) { - await _updateStatus(billingBatchId, 'empty') + // The bill run is considered empty. We just need to set the status to indicate this in the UI + if (billingInvoicesToInsert.length === 0) { + await _updateStatus(billingBatchId, 'empty') - return - } + return + } + + // We need to persist the billing invoice and billing invoice licence records + const billingInvoiceLicencesToInsert = generatedInvoiceLicences.filter((invoiceLicence) => invoiceLicence.persist) - // We need to persist the billing invoice and billing invoice licence records - const billingInvoiceLicencesToInsert = generatedInvoiceLicences.filter((invoiceLicence) => invoiceLicence.persist) + await _persistBillingInvoices(billingInvoicesToInsert) + await _persistBillingInvoiceLicences(billingInvoiceLicencesToInsert) - await _persistBillingInvoices(billingInvoicesToInsert) - await _persistBillingInvoiceLicences(billingInvoiceLicencesToInsert) + // We then need to tell the Charging Module to run its generate process. This is where the Charging module finalises + // the debit and credit amounts, and adds any additional transactions needed, for example, minimum charge + await ChargingModuleGenerateService.go(externalId) - // We then need to tell the Charging Module to run its generate process. This is where the Charging module finalises - // the debit and credit amounts, and adds any additional transactions needed, for example, minimum charge - await ChargingModuleGenerateService.go(externalId) + // We then tell our legacy service to queue up its refresh totals job. This requests the finalised bill run and + // invoice detail from the Charging Module and updates our data with it. The good news is the legacy code handles + // all that within this job. We just need to queue it up 😁 + await LegacyRequestLib.post('water', `billing/batches/${billingBatchId}/refresh`) + } catch (error) { + HandleErroredBillingBatchService.go(billingBatch.billingBatchId) - // We then tell our legacy service to queue up its refresh totals job. This requests the finalised bill run and - // invoice detail from the Charging Module and updates our data with it. The good news is the legacy code handles - // all that within this job. We just need to queue it up 😁 - await LegacyRequestLib.post('water', `billing/batches/${billingBatchId}/refresh`) + throw error + } } -function _generateTransactionLines (billingPeriod, chargeVersion) { - const financialYearEnding = billingPeriod.endDate.getFullYear() - const chargePeriod = DetermineChargePeriodService.go(chargeVersion, financialYearEnding) - const isNewLicence = DetermineMinimumChargeService.go(chargeVersion, financialYearEnding) - const isWaterUndertaker = chargeVersion.licence.isWaterUndertaker +function _generateTransactionLines (billingPeriod, chargeVersion, billingBatchId) { + try { + const financialYearEnding = billingPeriod.endDate.getFullYear() + const chargePeriod = DetermineChargePeriodService.go(chargeVersion, financialYearEnding) + const isNewLicence = DetermineMinimumChargeService.go(chargeVersion, financialYearEnding) + const isWaterUndertaker = chargeVersion.licence.isWaterUndertaker + + const transactionLines = [] + for (const chargeElement of chargeVersion.chargeElements) { + const result = GenerateBillingTransactionsService.go(chargeElement, billingPeriod, chargePeriod, isNewLicence, isWaterUndertaker) + transactionLines.push(...result) + } - const transactionLines = [] - for (const chargeElement of chargeVersion.chargeElements) { - const result = GenerateBillingTransactionsService.go(chargeElement, billingPeriod, chargePeriod, isNewLicence, isWaterUndertaker) - transactionLines.push(...result) - } + return transactionLines + } catch (error) { + HandleErroredBillingBatchService.go( + billingBatchId, + BillingBatchModel.errorCodes.failedToPrepareTransactions + ) - return transactionLines + throw error + } } async function _persistBillingInvoiceLicences (billingInvoiceLicences) { @@ -181,9 +243,15 @@ async function _persistBillingInvoices (billingInvoices) { } async function _updateStatus (billingBatchId, status) { - await BillingBatchModel.query() - .findById(billingBatchId) - .patch({ status }) + try { + await BillingBatchModel.query() + .findById(billingBatchId) + .patch({ status }) + } catch (error) { + HandleErroredBillingBatchService.go(billingBatchId) + + throw error + } } module.exports = { diff --git a/test/services/supplementary-billing/handle-errored-billing-batch.service.test.js b/test/services/supplementary-billing/handle-errored-billing-batch.service.test.js new file mode 100644 index 0000000000..268b7cc3a3 --- /dev/null +++ b/test/services/supplementary-billing/handle-errored-billing-batch.service.test.js @@ -0,0 +1,84 @@ +'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 BillingBatchHelper = require('../../support/helpers/water/billing-batch.helper.js') + +// Thing under test +const HandleErroredBillingBatchService = require('../../../app/services/supplementary-billing/handle-errored-billing-batch.service.js') + +describe('Handle Errored Billing Batch service', () => { + let billingBatch + let notifierStub + + beforeEach(async () => { + // RequestLib depends on the 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 + + billingBatch = await BillingBatchHelper.add() + }) + + afterEach(() => { + Sinon.restore() + }) + + describe('when the service is called successfully', () => { + it('sets the billing batch status to `error`', async () => { + await HandleErroredBillingBatchService.go(billingBatch.billingBatchId) + + const result = await billingBatch.$query() + + expect(result.status).to.equal('error') + }) + + describe('when no error code is passed', () => { + it('doesn\'t set an error code', async () => { + await HandleErroredBillingBatchService.go(billingBatch.billingBatchId) + + const result = await billingBatch.$query() + + expect(result.errorCode).to.be.null() + }) + }) + + describe('when an error code is passed', () => { + it('does set an error code', async () => { + await HandleErroredBillingBatchService.go(billingBatch.billingBatchId, 40) + + const result = await billingBatch.$query() + + expect(result.errorCode).to.equal(40) + }) + }) + }) + + describe('when the service is called unsuccessfully', () => { + describe('because patching the billing batch fails', () => { + it('handles the error', async () => { + await expect(HandleErroredBillingBatchService.go(billingBatch.billingBatchId, 'INVALID_ERROR_CODE')).not.to.reject() + }) + + it('logs an error', async () => { + // Note that we would not normally pass a string as an error code but we do this here to force the patch to fail + // in lieu of a working method of stubbing Objection + await HandleErroredBillingBatchService.go(billingBatch.billingBatchId, 'INVALID_ERROR_CODE') + + const logDataArg = notifierStub.omfg.firstCall.args[1] + + expect(notifierStub.omfg.calledWith('Failed to set error status on billing batch')).to.be.true() + expect(logDataArg.billingBatchId).to.equal(billingBatch.billingBatchId) + expect(logDataArg.errorCode).to.equal('INVALID_ERROR_CODE') + }) + }) + }) +}) diff --git a/test/services/supplementary-billing/process-billing-batch.service.test.js b/test/services/supplementary-billing/process-billing-batch.service.test.js index 8d767a5f84..1ad42e1fda 100644 --- a/test/services/supplementary-billing/process-billing-batch.service.test.js +++ b/test/services/supplementary-billing/process-billing-batch.service.test.js @@ -10,28 +10,132 @@ const { expect } = Code // Test helpers const BillingBatchHelper = require('../../support/helpers/water/billing-batch.helper.js') +const BillingBatchModel = require('../../../app/models/water/billing-batch.model.js') +const BillingChargeCategoryHelper = require('../../support/helpers/water/billing-charge-category.helper.js') +const ChangeReasonHelper = require('../../support/helpers/water/change-reason.helper.js') +const ChargeElementHelper = require('../../support/helpers/water/charge-element.helper.js') +const ChargePurposeHelper = require('../../support/helpers/water/charge-purpose.helper.js') +const ChargeVersionHelper = require('../../support/helpers/water/charge-version.helper.js') +const InvoiceAccountHelper = require('../../support/helpers/crm-v2/invoice-account.helper.js') +const LicenceHelper = require('../../support/helpers/water/licence.helper.js') +const DatabaseHelper = require('../../support/helpers/database.helper.js') +const RegionHelper = require('../../support/helpers/water/region.helper.js') + +// Things we need to stub +const ChargingModuleCreateTransactionService = require('../../../app/services/charging-module/create-transaction.service.js') +const FetchChargeVersionsService = require('../../../app/services/supplementary-billing/fetch-charge-versions.service.js') +const GenerateBillingTransactionsService = require('../../../app/services/supplementary-billing/generate-billing-transactions.service.js') +const HandleErroredBillingBatchService = require('../../../app/services/supplementary-billing/handle-errored-billing-batch.service.js') // Thing under test const ProcessBillingBatchService = require('../../../app/services/supplementary-billing/process-billing-batch.service.js') -describe.skip('Process billing batch service', () => { +describe('Process billing batch service', () => { const billingPeriod = { startDate: new Date('2022-04-01'), endDate: new Date('2023-03-31') } + let billingBatch + let handleErroredBillingBatchStub + let notifierStub + + beforeEach(async () => { + await DatabaseHelper.clean() + + const { regionId } = await RegionHelper.add() + const { licenceId } = await LicenceHelper.add({ includeInSupplementaryBilling: 'yes', regionId }) + const { changeReasonId } = await ChangeReasonHelper.add() + const { invoiceAccountId } = await InvoiceAccountHelper.add() + const { chargeVersionId } = await ChargeVersionHelper.add({ changeReasonId, invoiceAccountId }, { licenceId }) + const { billingChargeCategoryId } = await BillingChargeCategoryHelper.add() + const { chargeElementId } = await ChargeElementHelper.add({ billingChargeCategoryId, chargeVersionId }) + await ChargePurposeHelper.add({ chargeElementId }) + + billingBatch = await BillingBatchHelper.add({ regionId }) + + // RequestLib depends on the 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 + + handleErroredBillingBatchStub = Sinon.stub(HandleErroredBillingBatchService, 'go') + }) afterEach(() => { Sinon.restore() }) - describe('when the service is called', () => { - beforeEach(async () => { - billingBatch = await BillingBatchHelper.add() + describe('when the service errors', () => { + const expectedError = new Error('ERROR') + + beforeEach(() => { + Sinon.stub(FetchChargeVersionsService, 'go').rejects(expectedError) }) - it('does something temporarily as it is just a placeholder at the moment', async () => { + it('handles the error', async () => { await expect(ProcessBillingBatchService.go(billingBatch, billingPeriod)).not.to.reject() }) + + it('calls HandleErroredBillingBatchService with the billing batch id', async () => { + await ProcessBillingBatchService.go(billingBatch, billingPeriod) + + const handlerArgs = handleErroredBillingBatchStub.firstCall.args + + expect(handlerArgs[0]).to.equal(billingBatch.billingBatchId) + }) + + it('logs the error', async () => { + await ProcessBillingBatchService.go(billingBatch, billingPeriod) + + const logDataArg = notifierStub.omfg.firstCall.args[1] + + expect(notifierStub.omfg.calledWith('Billing Batch process errored')).to.be.true() + expect(logDataArg.billingBatch).to.equal(billingBatch) + expect(logDataArg.error).to.equal(expectedError) + }) + }) + + describe('when attempting to fetch the charge versions fails', () => { + beforeEach(() => { + Sinon.stub(FetchChargeVersionsService, 'go').rejects() + }) + + it('sets the appropriate error code', async () => { + await ProcessBillingBatchService.go(billingBatch, billingPeriod) + + const handlerArgs = handleErroredBillingBatchStub.firstCall.args + + expect(handlerArgs[1]).to.equal(BillingBatchModel.errorCodes.failedToProcessChargeVersions) + }) + }) + + describe('when attempting to generate the transaction lines fails', () => { + beforeEach(() => { + Sinon.stub(GenerateBillingTransactionsService, 'go').throws() + }) + + it('sets the appropriate error code', async () => { + await ProcessBillingBatchService.go(billingBatch, billingPeriod) + + const handlerArgs = handleErroredBillingBatchStub.firstCall.args + + expect(handlerArgs[1]).to.equal(BillingBatchModel.errorCodes.failedToPrepareTransactions) + }) + }) + + describe('when attempting to create the transaction lines fails', () => { + beforeEach(() => { + Sinon.stub(ChargingModuleCreateTransactionService, 'go').rejects() + }) + + it('sets the appropriate error code', async () => { + await ProcessBillingBatchService.go(billingBatch, billingPeriod) + + const handlerArgs = handleErroredBillingBatchStub.firstCall.args + + expect(handlerArgs[1]).to.equal(BillingBatchModel.errorCodes.failedToCreateCharge) + }) }) })