From d1ed4208170b74ce5ae7140905d2b26e866b3e0e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 Mar 2023 10:22:02 +0000 Subject: [PATCH 1/2] Fix cached data in ProcessBillingBatchService https://eaflood.atlassian.net/browse/WATER-3906 As part of implementing [Handle errors in ProcessBillingBatchService](https://github.com/DEFRA/water-abstraction-system/pull/161) we refactored the `ProcessBillingBatchService` to make the code cleaner and easier to read. One of the changes was moving the `generatedInvoices` and `generatedInvoiceLicences` to the module scope rather than just in the method scope. It saved us having to pass them around the place which removed a bunch of 'cruft'. What we didn't take account of is they would be cached between calls to the service. Now it's at module scope the data from previous runs is being cached and still exists when a new one starts. So, we're getting to the finalise stage and we're trying to persist old data from a previous run! This change resolves the issue. From 5d2148b2df024122d7c4bdd622838fba45d9b0ec Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 Mar 2023 11:27:35 +0000 Subject: [PATCH 2/2] Implement fix We move the information into the scope of the `go()` function removing the issue of the data being cached at the global scope. We then take a half-way house approach; we pass the information into our methods but we accept having to mutate the the data rather than pass back a modified copy. --- .../process-billing-batch.service.js | 50 ++++++++++++------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/app/services/supplementary-billing/process-billing-batch.service.js b/app/services/supplementary-billing/process-billing-batch.service.js index 035be53658..19ca05b29f 100644 --- a/app/services/supplementary-billing/process-billing-batch.service.js +++ b/app/services/supplementary-billing/process-billing-batch.service.js @@ -21,9 +21,6 @@ const GenerateBillingInvoiceLicenceService = require('./generate-billing-invoice 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 * @@ -35,6 +32,14 @@ let generatedInvoiceLicences = [] async function go (billingBatch, billingPeriod) { const { billingBatchId } = billingBatch + // NOTE: This is where we store generated data that _might_ be persisted when we finalise the billing batch. We need + // to generate invoice and invoice licence information so we can persist those transactions we want to bill. When we + // persist a transaction we flag the accompanying invoice and invoice licence as needing persisting. + // This means a number of our methods are mutating this information as the billing batch is processed. + const generatedData = { + invoices: [], + invoiceLicences: [] + } try { // Mark the start time for later logging const startTime = process.hrtime.bigint() @@ -46,8 +51,8 @@ async function go (billingBatch, billingPeriod) { for (const chargeVersion of chargeVersions) { const { licence } = chargeVersion - const billingInvoice = await _generateBillingInvoice(chargeVersion, billingBatchId, billingPeriod) - const billingInvoiceLicence = _generateBillingInvoiceLicence(billingInvoice, licence) + const billingInvoice = await _generateBillingInvoice(generatedData, chargeVersion, billingBatchId, billingPeriod) + const billingInvoiceLicence = _generateBillingInvoiceLicence(generatedData, billingInvoice, licence) const transactionLines = _generateTransactionLines(billingPeriod, chargeVersion, billingBatchId) @@ -61,7 +66,7 @@ async function go (billingBatch, billingPeriod) { ) } - await _finaliseBillingBatch(billingBatch) + await _finaliseBillingBatch(generatedData, billingBatch) // Log how log the process took _calculateAndLogTime(billingBatchId, startTime) @@ -88,15 +93,15 @@ function _calculateAndLogTime (billingBatchId, startTime) { global.GlobalNotifier.omg(`Time taken to process billing batch ${billingBatchId}: ${timeTakenMs}ms`) } -async function _generateBillingInvoice (chargeVersion, billingBatchId, billingPeriod) { +async function _generateBillingInvoice (generatedData, chargeVersion, billingBatchId, billingPeriod) { try { const billingInvoiceData = await GenerateBillingInvoiceService.go( - generatedInvoices, + generatedData.invoices, chargeVersion.invoiceAccountId, billingBatchId, billingPeriod.endDate.getFullYear() ) - generatedInvoices = billingInvoiceData.billingInvoices + generatedData.invoices = billingInvoiceData.billingInvoices return billingInvoiceData.billingInvoice } catch (error) { @@ -106,14 +111,14 @@ async function _generateBillingInvoice (chargeVersion, billingBatchId, billingPe } } -function _generateBillingInvoiceLicence (billingInvoice, licence) { +function _generateBillingInvoiceLicence (generatedData, billingInvoice, licence) { try { const billingInvoiceLicenceData = GenerateBillingInvoiceLicenceService.go( - generatedInvoiceLicences, + generatedData.invoiceLicences, billingInvoice.billingInvoiceId, licence ) - generatedInvoiceLicences = billingInvoiceLicenceData.billingInvoiceLicences + generatedData.invoiceLicences = billingInvoiceLicenceData.billingInvoiceLicences return billingInvoiceLicenceData.billingInvoiceLicence } catch (error) { @@ -182,11 +187,12 @@ async function _createTransactionLines ( } } -async function _finaliseBillingBatch (billingBatch) { - try { - const { billingBatchId, externalId } = billingBatch +async function _finaliseBillingBatch (generatedData, billingBatch) { + const { invoices, invoiceLicences } = generatedData + const { billingBatchId, externalId } = billingBatch - const billingInvoicesToInsert = generatedInvoices.filter((billingInvoice) => billingInvoice.persist) + try { + const billingInvoicesToInsert = invoices.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) { @@ -196,7 +202,7 @@ async function _finaliseBillingBatch (billingBatch) { } // We need to persist the billing invoice and billing invoice licence records - const billingInvoiceLicencesToInsert = generatedInvoiceLicences.filter((invoiceLicence) => invoiceLicence.persist) + const billingInvoiceLicencesToInsert = invoiceLicences.filter((invoiceLicence) => invoiceLicence.persist) await _persistBillingInvoices(billingInvoicesToInsert) await _persistBillingInvoiceLicences(billingInvoiceLicencesToInsert) @@ -210,7 +216,7 @@ async function _finaliseBillingBatch (billingBatch) { // 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) + HandleErroredBillingBatchService.go(billingBatchId) throw error } @@ -225,7 +231,13 @@ function _generateTransactionLines (billingPeriod, chargeVersion, billingBatchId const transactionLines = [] for (const chargeElement of chargeVersion.chargeElements) { - const result = GenerateBillingTransactionsService.go(chargeElement, billingPeriod, chargePeriod, isNewLicence, isWaterUndertaker) + const result = GenerateBillingTransactionsService.go( + chargeElement, + billingPeriod, + chargePeriod, + isNewLicence, + isWaterUndertaker + ) transactionLines.push(...result) }