From df02d208067e599e2252a3eb04c0e8e77c91efcc Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 2 May 2023 14:05:14 +0100 Subject: [PATCH] Inc. changes to agreements & charges in supp bill (#207) https://eaflood.atlassian.net/browse/WATER-3989 When a new charge version is added to a licence, users can change agreements and charges linked to the licence via the charge version. So, for example, when the SROC charge version is first applied the licence does **not** have a two-part tariff agreement (2PT). But later in the year, one is added. To do this the business will add a new charge version and apply the 2PT agreement. Though the number of billable days calculated won't change, the charge that the [charging-module-api](https://gothub.com/DEFRA/sroc-charging-module-api) returns will. So, we need to credit the previous transaction and calculate a new debit. At the moment, our engine does not look at changes to agreements and charges. So, it will compare the billable days, see no change and cancel both the previous and calculated transaction lines before we get a value from the charging module. This change updates the logic for comparing previous transactions to those we've calculated to include agreements and charges. If there has been a change, the lines won't match and get cancelled so will then progress to the next step; sending them to the charging module to calculate their charge value. --- .../process-billing-transactions.service.js | 44 +- ...ocess-billing-transactions.service.test.js | 498 ++++++++++++++---- 2 files changed, 439 insertions(+), 103 deletions(-) diff --git a/app/services/supplementary-billing/process-billing-transactions.service.js b/app/services/supplementary-billing/process-billing-transactions.service.js index ef0db3ad20..8ed6256304 100644 --- a/app/services/supplementary-billing/process-billing-transactions.service.js +++ b/app/services/supplementary-billing/process-billing-transactions.service.js @@ -39,21 +39,55 @@ async function go (calculatedTransactions, billingInvoice, billingInvoiceLicence } /** + * Compares a calculated transaction to the transactions from previous bill runs + * * Takes a single calculated debit transaction and checks to see if the provided array of reversed (credit) transactions - * contains a transaction that will cancel it out, returning `true` or `false` to indicate if it does or doesn't. Since - * the calculated debit transactions have not yet been sent to the Charging Module, we look at `chargeType`, - * `chargeCategoryCode` and `billableDays` as any given combination of these will always result in the same value coming - * back from the Charging Module. + * contains a transaction that will cancel it out, returning `true` or `false` to indicate if it does or doesn't. + * + * We compare those properties which determine the charge value calculated by the charging module. If the calculated + * transaction's properties matches one in reversedTransactions we return true. This will tell the calling method + * to not include the calculated transaction in the bill run. We also remove the matched transaction from + * reversedTransactions. + * + * The key properties are charge type, category code, and billable days. But we also need to compare agreements and + * additional charges because if those have changed, we'll need to credit the previous transaction and calculate the + * new debit value. Because what we are checking does not match up to what you see in the UI we have this reference + * + * - Abatement agreement - section126Factor + * - Two-part tariff agreement - section127Agreement + * - Canal and River Trust agreement - section130Agreement + * - Aggregate - aggregateFactor + * - Charge Adjustment - adjustmentFactor + * - Winter discount - isWinterOnly + * + * - Additional charges - isSupportedSource + * - Additional charges - supportedSourceName + * - Additional charges - isWaterCompanyCharge * * NOTE: This function will mutate the provided array of reversed transactions if one of the transactions in it will * cancel the calculated transaction; in this case, we remove the reversed transaction from the array as it can only * cancel one calculated transaction. */ function _cancelCalculatedTransaction (calculatedTransaction, reversedTransactions) { + // When we put together this matching logic our instincts were to try and do something 'better' than this long, + // chained && statement. But whatever we came up with was + // - more complex + // - less performant + // We found this easy to see what properties are being compared. Plus the moment something doesn't match we bail. So, + // much as it feels 'wrong', we are sticking with it! const result = reversedTransactions.findIndex((reversedTransaction) => { return reversedTransaction.chargeType === calculatedTransaction.chargeType && reversedTransaction.chargeCategoryCode === calculatedTransaction.chargeCategoryCode && - reversedTransaction.billableDays === calculatedTransaction.billableDays + reversedTransaction.billableDays === calculatedTransaction.billableDays && + reversedTransaction.section126Factor === calculatedTransaction.section126Factor && + reversedTransaction.section127Agreement === calculatedTransaction.section127Agreement && + reversedTransaction.section130Agreement === calculatedTransaction.section130Agreement && + reversedTransaction.aggregateFactor === calculatedTransaction.aggregateFactor && + reversedTransaction.adjustmentFactor === calculatedTransaction.adjustmentFactor && + reversedTransaction.isWinterOnly === calculatedTransaction.isWinterOnly && + reversedTransaction.isSupportedSource === calculatedTransaction.isSupportedSource && + reversedTransaction.supportedSourceName === calculatedTransaction.supportedSourceName && + reversedTransaction.isWaterCompanyCharge === calculatedTransaction.isWaterCompanyCharge }) if (result === -1) { diff --git a/test/services/supplementary-billing/process-billing-transactions.service.test.js b/test/services/supplementary-billing/process-billing-transactions.service.test.js index 8dce07ba24..b929c67802 100644 --- a/test/services/supplementary-billing/process-billing-transactions.service.test.js +++ b/test/services/supplementary-billing/process-billing-transactions.service.test.js @@ -23,99 +23,36 @@ describe('Process billing batch service', () => { endDate: new Date('2023-03-31') } - const calculatedTransactions = [ - { - billingTransactionId: '61abdc15-7859-4783-9622-6cb8de7f2461', - billingInvoiceLicenceId: billingInvoiceLicence.billingInvoiceLicenceId, - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '4.10.1', - billableDays: 365, - purposes: 'CALCULATED_TRANSACTION_1' - }, - { - billingTransactionId: 'a903cdd3-1804-4237-aeb9-70ef9008469d', - billingInvoiceLicenceId: billingInvoiceLicence.billingInvoiceLicenceId, - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '5.11.2', - billableDays: 265, - purposes: 'CALCULATED_TRANSACTION_2' - }, - { - billingTransactionId: '34453414-0ecb-49ce-8442-619d22c882f0', - billingInvoiceLicenceId: billingInvoiceLicence.billingInvoiceLicenceId, - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '6.12.3', - billableDays: 100, - purposes: 'CALCULATED_TRANSACTION_3' - } - ] - - const allPreviousTransactions = [ - { - billingTransactionId: '8d68eb26-d054-47a7-aee8-cd93a24fa860', - billingInvoiceLicenceId: '110ab2e2-6076-4d5a-a56f-b17a048eb269', - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '4.10.1', - billableDays: 365, - purposes: ['I_WILL_BE_REMOVED_1'] - }, - { - billingTransactionId: '63742bee-cb1b-44f1-86f6-c7d546f59c88', - billingInvoiceLicenceId: '110ab2e2-6076-4d5a-a56f-b17a048eb269', - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '5.11.2', - billableDays: 265, - purposes: ['I_WILL_BE_REMOVED_2'] - }, - { - billingTransactionId: '4f254a70-d36e-46eb-9e4b-59503c3d5994', - billingInvoiceLicenceId: '110ab2e2-6076-4d5a-a56f-b17a048eb269', - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '6.12.3', - billableDays: 100, - purposes: ['I_WILL_BE_REMOVED_3'] - }, - { - billingTransactionId: '733bdb55-5386-4fdb-a581-1e98f9a35bed', - billingInvoiceLicenceId: '110ab2e2-6076-4d5a-a56f-b17a048eb269', - isCredit: false, - status: 'candidate', - chargeType: 'standard', - chargeCategoryCode: '9.9.9', - billableDays: 180, - purposes: ['I_WILL_NOT_BE_REMOVED'] - } - ] - afterEach(() => { Sinon.restore() }) describe('when the billing invoice, licence and period', () => { + let calculatedTransactions + + beforeEach(() => { + calculatedTransactions = [ + _generateCalculatedTransaction('4.10.1', 365, 'CALCULATED_TRANSACTION_1'), + _generateCalculatedTransaction('5.11.2', 265, 'CALCULATED_TRANSACTION_2'), + _generateCalculatedTransaction('6.12.3', 100, 'CALCULATED_TRANSACTION_3') + ] + }) + describe('match to transactions on a previous billing batch', () => { describe('and the calculated transactions provided', () => { let previousTransactions - describe('completely cancel out the previous transactions from the last billing batch', () => { + describe('match all the previous transactions from the last billing batch', () => { beforeEach(() => { - previousTransactions = [allPreviousTransactions[0], allPreviousTransactions[1]] + previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'I_WILL_BE_REMOVED_1'), + _generatePreviousTransaction('5.11.2', 265, 'I_WILL_BE_REMOVED_2') + ] Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) }) - it('returns the uncanceled calculated transactions', async () => { + it('returns the matched calculated transactions', async () => { const result = await ProcessBillingTransactionsService.go( calculatedTransactions, billingInvoice, @@ -128,14 +65,18 @@ describe('Process billing batch service', () => { }) }) - describe('partially cancel out the previous transactions from the last billing batch', () => { + describe('are cancelled out by the previous transactions from the last billing batch', () => { beforeEach(() => { - previousTransactions = [allPreviousTransactions[0], allPreviousTransactions[1], allPreviousTransactions[3]] + previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'I_WILL_BE_REMOVED_1'), + _generatePreviousTransaction('5.11.2', 265, 'I_WILL_BE_REMOVED_2'), + _generatePreviousTransaction('6.12.3', 100, 'I_WILL_BE_REMOVED_3') + ] Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) }) - it('returns the uncanceled calculated and reversed transactions', async () => { + it('returns no transactions', async () => { const result = await ProcessBillingTransactionsService.go( calculatedTransactions, billingInvoice, @@ -143,51 +84,57 @@ describe('Process billing batch service', () => { billingPeriod ) - expect(result).to.have.length(2) - expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION_3') - expect(result[1].purposes).to.equal('I_WILL_NOT_BE_REMOVED') + expect(result).to.be.empty() }) }) - describe('are cancelled out by the previous transactions from the last billing batch', () => { + describe('are empty', () => { beforeEach(() => { - previousTransactions = [allPreviousTransactions[0], allPreviousTransactions[1], allPreviousTransactions[2]] + previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'I_WILL_NOT_BE_REMOVED_1'), + _generatePreviousTransaction('5.11.2', 265, 'I_WILL_NOT_BE_REMOVED_2') + ] Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) }) - it('returns no transactions', async () => { + it('returns only the previous transactions', async () => { const result = await ProcessBillingTransactionsService.go( - calculatedTransactions, + [], billingInvoice, billingInvoiceLicence, billingPeriod ) - expect(result).to.be.empty() + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('I_WILL_NOT_BE_REMOVED_1') + expect(result[1].purposes).to.equal('I_WILL_NOT_BE_REMOVED_2') }) }) - describe('are empty', () => { + describe('partially match the previous transactions from the last billing batch', () => { beforeEach(() => { - previousTransactions = [allPreviousTransactions[0], allPreviousTransactions[1]] + previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'I_WILL_BE_REMOVED_1'), + _generatePreviousTransaction('5.11.2', 265, 'I_WILL_BE_REMOVED_2'), + _generatePreviousTransaction('9.9.9', 180, 'I_WILL_NOT_BE_REMOVED') + ] Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) }) - it('returns only the previous transactions', async () => { + it('returns the unmatched calculated transactions and previous transactions (reversed)', async () => { const result = await ProcessBillingTransactionsService.go( - [], + calculatedTransactions, billingInvoice, billingInvoiceLicence, billingPeriod ) expect(result).to.have.length(2) - - // NOTE: We know the text says 'I_WILL_BE_REMOVED' but in this scenario they won't be! - expect(result[0].purposes).to.equal('I_WILL_BE_REMOVED_1') - expect(result[1].purposes).to.equal('I_WILL_BE_REMOVED_2') + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION_3') + expect(result[1].purposes).to.equal('I_WILL_NOT_BE_REMOVED') + expect(result[1].isCredit).to.be.true() }) }) }) @@ -213,4 +160,359 @@ describe('Process billing batch service', () => { }) }) }) + + describe('the service matches calculated to previous transactions', () => { + let calculatedTransactions + + beforeEach(() => { + calculatedTransactions = [_generateCalculatedTransaction('4.10.1', 365, 'CALCULATED_TRANSACTION')] + }) + + describe('when the charge type differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction( + '4.10.1', 365, 'PREVIOUS_TRANSACTION', { chargeType: 'compensation' } + ) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the charge category code differs', () => { + beforeEach(() => { + const previousTransactions = [_generatePreviousTransaction('5.10.1', 365, 'PREVIOUS_TRANSACTION')] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the billable days differ', () => { + beforeEach(() => { + const previousTransactions = [_generatePreviousTransaction('4.10.1', 5, 'PREVIOUS_TRANSACTION')] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the abatement agreement (section 126) differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { section126Factor: 0.5 }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the two-part tariff agreement (section 127) differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { section127Agreement: true }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the canal and river trust agreement (section 130) differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { section130Agreement: true }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the aggregate differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { aggregateFactor: 0.5 }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the charge adjustment differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { adjustmentFactor: 0.5 }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the winter discount differs', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { isWinterOnly: true }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when if it is a supported source differs (additional charge)', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { isSupportedSource: true }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the supported source name differs (additional charge)', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { supportedSourceName: 'source name' }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when the water company flag differs (additional charge)', () => { + beforeEach(() => { + const previousTransactions = [ + _generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION', { isWaterCompanyCharge: true }) + ] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('does not match the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.have.length(2) + expect(result[0].purposes).to.equal('CALCULATED_TRANSACTION') + expect(result[1].purposes).to.equal('PREVIOUS_TRANSACTION') + }) + }) + + describe('when nothing differs', () => { + beforeEach(() => { + const previousTransactions = [_generatePreviousTransaction('4.10.1', 365, 'PREVIOUS_TRANSACTION')] + + Sinon.stub(FetchPreviousBillingTransactionsService, 'go').resolves(previousTransactions) + }) + + it('matches the transactions', async () => { + const result = await ProcessBillingTransactionsService.go( + calculatedTransactions, + billingInvoice, + billingInvoiceLicence, + billingPeriod + ) + + expect(result).to.be.empty() + }) + }) + }) }) + +function _generateCalculatedTransaction (chargeCategoryCode, billableDays, testReference, changes = {}) { + const defaultProperties = { + billingTransactionId: '61abdc15-7859-4783-9622-6cb8de7f2461', + billingInvoiceLicenceId: '110ab2e2-6076-4d5a-a56f-b17a048eb269', + isCredit: false, + status: 'candidate', + chargeType: 'standard', + chargeCategoryCode, + billableDays, + section126Factor: 1, + section127Agreement: false, + section130Agreement: false, + aggregateFactor: 1, + adjustmentFactor: 1, + isWinterOnly: false, + isSupportedSource: false, + supportedSourceName: null, + isWaterCompanyCharge: false, + purposes: testReference + } + + return { + ...defaultProperties, + ...changes + } +} + +function _generatePreviousTransaction (chargeCategoryCode, billableDays, testReference, changes = {}) { + const defaultProperties = { + billingTransactionId: '8d68eb26-d054-47a7-aee8-cd93a24fa860', + billingInvoiceLicenceId: 'a76b3ab3-d70d-4fb0-8d72-2e2cdd334729', + isCredit: false, + status: 'candidate', + chargeType: 'standard', + chargeCategoryCode, + billableDays, + section126Factor: 1, + section127Agreement: false, + section130Agreement: false, + aggregateFactor: 1, + adjustmentFactor: 1, + isWinterOnly: false, + isSupportedSource: false, + supportedSourceName: null, + isWaterCompanyCharge: false, + purposes: [testReference] + } + + return { + ...defaultProperties, + ...changes + } +}