From 2971028ab0af47c82529b7691606fb086aeeca0e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Thu, 11 Jul 2024 08:52:33 +0100 Subject: [PATCH] Fix error in generate 2PT bill run transactions (#1188) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://eaflood.atlassian.net/browse/WATER-4196 > Part of the two-part tariff annual billing work We recently completed [implementation of the annual two-part tariff billing engine](https://github.com/DEFRA/water-abstraction-system/pull/1172). But we've immediately spotted an error with the results. We are not populating the correct field in order to see the amended billing volume being sent to the Charging Module and persisting against the transaction. However, that uncovered something else we'd overlooked. Now that we were using the review allocated amount, some transactions were being returned from `GenerateTransactionService` with a 0 volume, which caused the Charging Module to error. Of course! If a return was submitted as a 'Nil return' then we should not be trying to generate a charge for those charge elements linked to it. 🤦 So, this change corrects which value we are assigning to the generated transaction and therefore passing to the charging module. But it also updates the engine to handle ignoring transactions with no volume rather than attempting to send them to the Charging Module API. --- .../generate-transaction.service.js | 6 +- .../process-billing-period.service.js | 8 +- .../generate-transaction.service.test.js | 113 ++++++++++-------- .../process-billing-period.service.test.js | 15 +-- 4 files changed, 83 insertions(+), 59 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/generate-transaction.service.js b/app/services/bill-runs/two-part-tariff/generate-transaction.service.js index 05a8002583..f07f1a3a6b 100644 --- a/app/services/bill-runs/two-part-tariff/generate-transaction.service.js +++ b/app/services/bill-runs/two-part-tariff/generate-transaction.service.js @@ -35,6 +35,10 @@ const { generateUUID } = require('../../../lib/general.lib.js') function go (billLicenceId, chargeReference, chargePeriod, newLicence, waterUndertaker) { const billableQuantity = _billableQuantity(chargeReference.chargeElements) + if (billableQuantity === 0) { + return null + } + return _standardTransaction( billLicenceId, billableQuantity, @@ -105,7 +109,7 @@ function _standardTransaction ( billableQuantity, status: 'candidate', description: _description(chargeReference), - volume: chargeReference.volume, + volume: billableQuantity, section126Factor: Number(chargeReference.adjustments.s126) || 1, section127Agreement: !!chargeReference.adjustments.s127, section130Agreement: !!chargeReference.adjustments.s130, diff --git a/app/services/bill-runs/two-part-tariff/process-billing-period.service.js b/app/services/bill-runs/two-part-tariff/process-billing-period.service.js index 45b5d272e9..528b90f69e 100644 --- a/app/services/bill-runs/two-part-tariff/process-billing-period.service.js +++ b/app/services/bill-runs/two-part-tariff/process-billing-period.service.js @@ -127,6 +127,10 @@ async function _createTransactions (billLicenceId, billingPeriod, chargeVersion, const generatedTransactions = _generateTransactionData(billLicenceId, chargePeriod, chargeVersion) + if (generatedTransactions.length === 0) { + return [] + } + return SendTransactionsService.go(generatedTransactions, billRunExternalId, accountNumber, chargeVersion.licence) } @@ -221,7 +225,9 @@ function _generateTransactionData (billLicenceId, chargePeriod, chargeVersion) { chargeVersion.licence.waterUndertaker ) - transactions.push(transaction) + if (transaction) { + transactions.push(transaction) + } }) return transactions diff --git a/test/services/bill-runs/two-part-tariff/generate-transaction.service.test.js b/test/services/bill-runs/two-part-tariff/generate-transaction.service.test.js index d3a6d94b3e..91e0c83718 100644 --- a/test/services/bill-runs/two-part-tariff/generate-transaction.service.test.js +++ b/test/services/bill-runs/two-part-tariff/generate-transaction.service.test.js @@ -30,63 +30,80 @@ describe('Generate Transaction service', () => { }) describe('when called', () => { - it('returns a two-part tariff transaction ready to be persisted', () => { - const result = GenerateTransactionService.go( - billLicenceId, chargeReference, chargePeriod, newLicence, waterUndertaker - ) - - expect(result).to.equal({ - billLicenceId, - authorisedDays: 0, - billableDays: 0, - newLicence, - waterUndertaker, - chargeReferenceId: '89450220-0f7f-4280-a946-1fdbe9b789c1', - startDate: chargePeriod.startDate, - endDate: chargePeriod.endDate, - source: 'non-tidal', - season: 'all year', - loss: 'low', - credit: false, - chargeType: 'standard', - authorisedQuantity: 20, - billableQuantity: 15, - status: 'candidate', - description: 'Water abstraction charge: Lower Queenstown - Pittisham', - volume: 15, - section126Factor: 1, - section127Agreement: false, - section130Agreement: false, - secondPartCharge: true, - scheme: 'sroc', - aggregateFactor: 0.75, - adjustmentFactor: 0.6, - chargeCategoryCode: '4.4.5', - chargeCategoryDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model', - supportedSource: false, - supportedSourceName: null, - waterCompanyCharge: false, - winterOnly: false - }, - { - // We skip the ID because it is a UUID generated by the service. We skip purposes because it is the result of - // calling `toJSON()` on the charge references child ChargeElementModel instances. Including it would just be us - // testing Objection - skip: ['id', 'purposes'] + describe('with a charge reference that has volume to be billed', () => { + it('returns a two-part tariff transaction ready to be persisted', () => { + const result = GenerateTransactionService.go( + billLicenceId, chargeReference, chargePeriod, newLicence, waterUndertaker + ) + + expect(result).to.equal({ + billLicenceId, + authorisedDays: 0, + billableDays: 0, + newLicence, + waterUndertaker, + chargeReferenceId: '89450220-0f7f-4280-a946-1fdbe9b789c1', + startDate: chargePeriod.startDate, + endDate: chargePeriod.endDate, + source: 'non-tidal', + season: 'all year', + loss: 'low', + credit: false, + chargeType: 'standard', + authorisedQuantity: 20, + billableQuantity: 15, + status: 'candidate', + description: 'Water abstraction charge: Lower Queenstown - Pittisham', + volume: 15, + section126Factor: 1, + section127Agreement: false, + section130Agreement: false, + secondPartCharge: true, + scheme: 'sroc', + aggregateFactor: 0.75, + adjustmentFactor: 0.6, + chargeCategoryCode: '4.4.5', + chargeCategoryDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model', + supportedSource: false, + supportedSourceName: null, + waterCompanyCharge: false, + winterOnly: false + }, + { + // We skip the ID because it is a UUID generated by the service. We skip purposes because it is the result of + // calling `toJSON()` on the charge references child ChargeElementModel instances. Including it would just be + // us testing Objection + skip: ['id', 'purposes'] + }) + }) + + describe('and the charge reference has a two-part tariff agreement (section 127)', () => { + beforeEach(() => { + chargeReference.adjustments.s127 = true + }) + + it('returns the two-part tariff prefixed description', () => { + const result = GenerateTransactionService.go( + billLicenceId, chargeReference, chargePeriod, newLicence, waterUndertaker + ) + + expect(result.description).to.equal('Two-part tariff basic water abstraction charge: Lower Queenstown - Pittisham') + }) }) }) - describe('and the charge reference has a two-part tariff agreement (section 127)', () => { + describe('with a charge reference that has no volume to be billed', () => { beforeEach(() => { - chargeReference.adjustments.s127 = true + chargeReference.chargeElements[0].reviewChargeElements[0].amendedAllocated = 0 + chargeReference.chargeElements[1].reviewChargeElements[0].amendedAllocated = 0 }) - it('returns the two-part tariff prefixed description', () => { + it('returns null', () => { const result = GenerateTransactionService.go( billLicenceId, chargeReference, chargePeriod, newLicence, waterUndertaker ) - expect(result.description).to.equal('Two-part tariff basic water abstraction charge: Lower Queenstown - Pittisham') + expect(result).to.be.null() }) }) }) @@ -137,6 +154,6 @@ function _chargeReference () { amendedChargeAdjustment: 0.6 }], source: 'non-tidal', - volume: 15 + volume: 20 } } diff --git a/test/services/bill-runs/two-part-tariff/process-billing-period.service.test.js b/test/services/bill-runs/two-part-tariff/process-billing-period.service.test.js index 2dfbd6781a..767ba01219 100644 --- a/test/services/bill-runs/two-part-tariff/process-billing-period.service.test.js +++ b/test/services/bill-runs/two-part-tariff/process-billing-period.service.test.js @@ -156,15 +156,12 @@ describe('Two-part Tariff Process Billing Period service', () => { describe('but they are not billable', () => { beforeEach(() => { - // Create a licence that has a revoked date before the billing period and then link it to a charge version - // that is also linked to our billing account. The engine will determine that the charge period for the charge - // version is invalid so won't attempt to generate a transaction. If we did try, the Charging Module would - // only reject it. - const unbillableLicence = _licence() - - unbillableLicence.revokedDate = new Date('2019-01-01') + // This time we update the charge version so that nothing is allocated in the charge references. This means + // the service will not generate any transactions and therefore no bills leading to bills being empty + const unbillableChargeVersion = _chargeVersion(billingAccount.id, licence) - const unbillableChargeVersion = _chargeVersion(billingAccount.id, unbillableLicence) + unbillableChargeVersion.chargeReferences[0].chargeElements[0].reviewChargeElements[0].amendedAllocated = 0 + unbillableChargeVersion.chargeReferences[0].chargeElements[1].reviewChargeElements[0].amendedAllocated = 0 billingAccount.chargeVersions = [unbillableChargeVersion] }) @@ -289,7 +286,7 @@ function _chargeVersion (billingAccountId, licence) { amendedChargeAdjustment: 0.6 }], source: 'non-tidal', - volume: 15 + volume: 20 } ] }