From 4c42d13ecfc7511cb64292f41e9ba176006492ac Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 18 Mar 2024 11:27:30 +0000 Subject: [PATCH 1/7] Refactor supplementary billing engine https://eaflood.atlassian.net/browse/WATER-4403 > Part of changes to exclude the current financial year from supplementary billing if the annual bill run has not been completed We have recently migrated the SROC annual billing engine into our project ( WATER-4345 ). The primary drive to do this was to enable them run annual bill runs in April at the same time as licensees are submitting their return data. This is when the service is under its greatest load so we wanted to ensure annual billing could take advantage of the improvements we made as part of the supplementary billing we implemented. Re-visiting supplementary billing as part of that migration meant we spotted further improvements we could make. For example, thanks to our models now being [view](https://www.postgresql.org/docs/current/sql-createview.html) based means tables being in different schemas is no longer a blocker to us creating relationships in the models. In our annual billing engine we are able to fetch everything at the billing account level removing the need for additional queries and the need to pre-generate billing objects. We were also able to implement a batch process, whereby multiple bills can be processed at the same time. We want to bring these improvements to our supplementary billing engine before we start making changes to support excluding the current financial year. From a4565aa7661cc96b186f4c190a355c9e36ca5081 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 22:03:23 +0000 Subject: [PATCH 2/7] Housekeeping - Should have used lib function --- app/services/bill-runs/annual/process-bill-run.service.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/bill-runs/annual/process-bill-run.service.js b/app/services/bill-runs/annual/process-bill-run.service.js index 149ebc06d1..6980637a9a 100644 --- a/app/services/bill-runs/annual/process-bill-run.service.js +++ b/app/services/bill-runs/annual/process-bill-run.service.js @@ -9,7 +9,7 @@ const BillRunModel = require('../../../models/bill-run.model.js') const BillRunError = require('../../../errors/bill-run.error.js') const ChargingModuleGenerateBillRunRequest = require('../../../requests/charging-module/generate-bill-run.request.js') const FetchBillingAccountsService = require('./fetch-billing-accounts.service.js') -const { calculateAndLogTimeTaken } = require('../../../lib/general.lib.js') +const { calculateAndLogTimeTaken, currentTimeInNanoseconds } = require('../../../lib/general.lib.js') const LegacyRefreshBillRunRequest = require('../../../requests/legacy/refresh-bill-run.request.js') const ProcessBillingPeriodService = require('./process-billing-period.service.js') const HandleErroredBillRunService = require('../handle-errored-bill-run.service.js') @@ -41,7 +41,7 @@ async function go (billRun, billingPeriods) { const billingPeriod = billingPeriods[0] try { - const startTime = process.hrtime.bigint() + const startTime = currentTimeInNanoseconds() await _updateStatus(billRunId, 'processing') From b53e900fe1fd6c868910ff72455a48d5c03de203 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 22:06:05 +0000 Subject: [PATCH 3/7] Housekeeping - Overlooked when module moved --- .../handle-errored-bill-run.service.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename test/services/bill-runs/{supplementary => }/handle-errored-bill-run.service.test.js (93%) diff --git a/test/services/bill-runs/supplementary/handle-errored-bill-run.service.test.js b/test/services/bill-runs/handle-errored-bill-run.service.test.js similarity index 93% rename from test/services/bill-runs/supplementary/handle-errored-bill-run.service.test.js rename to test/services/bill-runs/handle-errored-bill-run.service.test.js index 3ad287019c..1a2368bc54 100644 --- a/test/services/bill-runs/supplementary/handle-errored-bill-run.service.test.js +++ b/test/services/bill-runs/handle-errored-bill-run.service.test.js @@ -9,10 +9,10 @@ const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script() const { expect } = Code // Test helpers -const BillRunHelper = require('../../../support/helpers/bill-run.helper.js') +const BillRunHelper = require('../../support/helpers/bill-run.helper.js') // Thing under test -const HandleErroredBillRunService = require('../../../../app/services/bill-runs/handle-errored-bill-run.service.js') +const HandleErroredBillRunService = require('../../../app/services/bill-runs/handle-errored-bill-run.service.js') describe('Handle Errored Bill Run service', () => { let billRun From 8094fdcd302f0c34c434459416b8e524f304fad7 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 22:25:32 +0000 Subject: [PATCH 4/7] Housekeeping - Just pass ID to ReverseTrans We never needed the whole BillLicence, just its ID. --- .../process-transactions.service.js | 3 ++- .../reverse-transactions.service.js | 19 +++++++++---------- .../reverse-transactions.service.test.js | 8 +++----- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/app/services/bill-runs/supplementary/process-transactions.service.js b/app/services/bill-runs/supplementary/process-transactions.service.js index 203653ac69..713cbea027 100644 --- a/app/services/bill-runs/supplementary/process-transactions.service.js +++ b/app/services/bill-runs/supplementary/process-transactions.service.js @@ -28,13 +28,14 @@ const ReverseTransactionsService = require('./reverse-transactions.service.js') * previous matching credit) */ async function go (calculatedTransactions, bill, billLicence, billingPeriod) { + const { id: billLicenceId } = billLicence const previousTransactions = await _fetchPreviousTransactions(bill, billLicence, billingPeriod) if (previousTransactions.length === 0) { return calculatedTransactions } - const reversedTransactions = ReverseTransactionsService.go(previousTransactions, billLicence) + const reversedTransactions = ReverseTransactionsService.go(previousTransactions, billLicenceId) return _cleanseTransactions(calculatedTransactions, reversedTransactions) } diff --git a/app/services/bill-runs/supplementary/reverse-transactions.service.js b/app/services/bill-runs/supplementary/reverse-transactions.service.js index bfd0e37a5a..ecc3514e47 100644 --- a/app/services/bill-runs/supplementary/reverse-transactions.service.js +++ b/app/services/bill-runs/supplementary/reverse-transactions.service.js @@ -11,17 +11,16 @@ const { generateUUID } = require('../../../lib/general.lib.js') * Takes an array of transactions and returns an array of transactions which will reverse them. * * In some situations we need to "reverse" transactions; this is done by issuing new transactions which cancel them out. - * This service takes an array of transactions and a bill licence, and returns an array of transactions which - * will reverse the original transactions, with their bill licence id set to the id of the supplied billing licence. + * This service takes an array of transactions and a bill licence, and returns an array of transactions which will + * reverse the original transactions, with their bill licence id set to the ID of the supplied billing licence. * - * @param {module:TransactionModel[]} transactions Array of transactions to be reversed - * @param {module:BillLicenceModel} billLicence The bill licence these transactions are intended to be added to + * @param {module:TransactionModel[]} transactions - Array of transactions to be reversed + * @param {string} billLicenceId - The bill licence UUID these transactions are to be added to * - * @returns {Object[]} Array of reversing transactions with `billLicenceId` set to the id of the supplied - * `billLicence` + * @returns {Object[]} Array of reversed transactions with `billLicenceId` set to the id of the supplied `billLicence` */ -function go (transactions, billLicence) { - return _reverseTransactions(transactions, billLicence) +function go (transactions, billLicenceId) { + return _reverseTransactions(transactions, billLicenceId) } /** @@ -29,7 +28,7 @@ function go (transactions, billLicence) { * identical except the `credit` flag is set to 'true', the status is set to `candidate`, the `billLicenceId` is set * to the id of the supplied bill licence, and a new transaction ID is generated. */ -function _reverseTransactions (transactions, billLicence) { +function _reverseTransactions (transactions, billLicenceId) { return transactions.map((transaction) => { // TODO: The FetchTransactionsService which we use to get the transactions to reverse adds the billing account ID // and number to each transaction returned. This is a performance measure to avoid an extra query to the DB. But if @@ -42,7 +41,7 @@ function _reverseTransactions (transactions, billLicence) { return { ...propertiesToKeep, id: generateUUID(), - billLicenceId: billLicence.id, + billLicenceId, credit: true, status: 'candidate', // TODO: Our query result seems to return the transaction's `purposes:` property as [Object]. Clearly, we need diff --git a/test/services/bill-runs/supplementary/reverse-transactions.service.test.js b/test/services/bill-runs/supplementary/reverse-transactions.service.test.js index 755422a5a4..46c6a5d376 100644 --- a/test/services/bill-runs/supplementary/reverse-transactions.service.test.js +++ b/test/services/bill-runs/supplementary/reverse-transactions.service.test.js @@ -22,13 +22,11 @@ describe('Reverse Transactions service', () => { } ] - const billLicence = { - id: '8affaa71-c185-4b6c-9814-4c615c235611' - } + const billLicenceId = '8affaa71-c185-4b6c-9814-4c615c235611' describe('when the service is called', () => { it('returns reversing transactions', () => { - const result = ReverseTransactionsService.go(transactions, billLicence) + const result = ReverseTransactionsService.go(transactions, billLicenceId) expect(result).to.have.length(transactions.length) @@ -38,7 +36,7 @@ describe('Reverse Transactions service', () => { expect(result[0].name).to.equal('DEBIT') expect(result[0].credit).to.be.true() expect(result[0].status).to.equal('candidate') - expect(result[0].billLicenceId).to.equal(billLicence.id) + expect(result[0].billLicenceId).to.equal('8affaa71-c185-4b6c-9814-4c615c235611') expect(result[0].id).to.exist().and.to.be.a.string() expect(result[0].purposes).to.equal('foo') }) From b76aab1eee0f0479cd8912269b14d9a52fd3027e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 22:43:37 +0000 Subject: [PATCH 5/7] Housekeeping - stop passing object around We seemed to have been passing whole objects around we think 'just in case'. We can see now we only ever needed their ID's or specific properties so we can stop doing that! --- .../fetch-previous-transactions.service.js | 19 ++++---- .../process-billing-period.service.js | 2 +- .../process-transactions.service.js | 26 +++++----- ...etch-previous-transactions.service.test.js | 48 ++++--------------- .../process-transactions.service.test.js | 17 ++++--- 5 files changed, 40 insertions(+), 72 deletions(-) diff --git a/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js b/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js index 73d138fffa..43916ff25e 100644 --- a/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js +++ b/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js @@ -13,18 +13,17 @@ const { transactionsMatch } = require('../../../lib/general.lib.js') * Fetches the previously billed transactions that match the bill, licence and year provided, removing any debits * which are cancelled out by previous credits. * - * @param {Object} bill A generated bill that identifies the billing account ID we need to match - * against - * @param {Object} billLicence A generated bill licence that identifies the licence we need to - * match against - * @param {Number} financialYearEnding The year the financial billing period ends that we need to match against + * @param {string} billingAccountId - The UUID that identifies the billing account we need to fetch transactions for + * @param {string} licenceId - The UUID that identifies the licence we need to fetch transactions for + * @param {Number} financialYearEnding - The year the financial billing period ends that we need to fetch transactions + * for * - * @returns {Promise} The resulting matched transactions + * @returns {Promise} The resulting matched transactions */ -async function go (bill, billLicence, financialYearEnding) { +async function go (billingAccountId, licenceId, financialYearEnding) { const transactions = await _fetch( - billLicence.licenceId, - bill.billingAccountId, + billingAccountId, + licenceId, financialYearEnding ) @@ -54,7 +53,7 @@ function _cleanse (transactions) { return debits } -async function _fetch (licenceId, billingAccountId, financialYearEnding) { +async function _fetch (billingAccountId, licenceId, financialYearEnding) { return db .select( 't.authorisedDays', diff --git a/app/services/bill-runs/supplementary/process-billing-period.service.js b/app/services/bill-runs/supplementary/process-billing-period.service.js index 819eff7dd0..b7b148c4e7 100644 --- a/app/services/bill-runs/supplementary/process-billing-period.service.js +++ b/app/services/bill-runs/supplementary/process-billing-period.service.js @@ -176,7 +176,7 @@ async function _cleanseTransactions (currentBillingData, billingPeriod) { const cleansedTransactions = await ProcessTransactionsService.go( currentBillingData.calculatedTransactions, - currentBillingData.bill, + currentBillingData.bill.billingAccountId, currentBillingData.billLicence, billingPeriod ) diff --git a/app/services/bill-runs/supplementary/process-transactions.service.js b/app/services/bill-runs/supplementary/process-transactions.service.js index 713cbea027..730e68ad8a 100644 --- a/app/services/bill-runs/supplementary/process-transactions.service.js +++ b/app/services/bill-runs/supplementary/process-transactions.service.js @@ -1,9 +1,7 @@ 'use strict' /** - * Fetches the matching debit transactions from a previous bill run and reverses them as credits; removes any which - * would be cancelled out by the supplied calculated debit transactions; combines the remaining transactions and returns - * them all + * Fetches the matching debit transactions from a previous bill run and reverses them as credits * @module ProcessTransactionsService */ @@ -19,17 +17,17 @@ const ReverseTransactionsService = require('./reverse-transactions.service.js') * sent to the Charging Module) and any matching pairs of transactions which would cancel each other out are removed. * Any remaining reversed credits and calculated debits are returned. * - * @param {Object[]} calculatedTransactions The calculated transactions to be processed - * @param {Object} bill A generated bill that identifies the invoice account ID we need to match against - * @param {Object} billLicence A generated bill licence that identifies the licence we need to match against - * @param {Object} billingPeriod Object with a `startDate` and `endDate` property representing the period being billed + * @param {Object[]} calculatedTransactions - The calculated transactions to be processed + * @param {string} billingAccountId - The UUID that identifies the billing account we are processing transactions for + * @param {Object} billLicence - A generated bill licence that identifies the licence we need to match against + * @param {Object} billingPeriod - Object with a `startDate` and `endDate` property representing the period being billed * - * @returns {Promise} An array of the remaining calculated transactions (ie. those which were not cancelled out by a - * previous matching credit) + * @returns {Promise} An array of the remaining calculated transactions (ie. those which were not cancelled + * out by a previous matching credit) */ -async function go (calculatedTransactions, bill, billLicence, billingPeriod) { - const { id: billLicenceId } = billLicence - const previousTransactions = await _fetchPreviousTransactions(bill, billLicence, billingPeriod) +async function go (calculatedTransactions, billingAccountId, billLicence, billingPeriod) { + const { id: billLicenceId, licenceId } = billLicence + const previousTransactions = await _fetchPreviousTransactions(billingAccountId, licenceId, billingPeriod) if (previousTransactions.length === 0) { return calculatedTransactions @@ -89,10 +87,10 @@ function _cleanseTransactions (calculatedTransactions, reverseTransactions) { return cleansedTransactionLines } -async function _fetchPreviousTransactions (bill, billLicence, billingPeriod) { +async function _fetchPreviousTransactions (billingAccountId, licenceId, billingPeriod) { const financialYearEnding = billingPeriod.endDate.getFullYear() - const transactions = await FetchPreviousTransactionsService.go(bill, billLicence, financialYearEnding) + const transactions = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) return transactions } diff --git a/test/services/bill-runs/supplementary/fetch-previous-transactions.service.test.js b/test/services/bill-runs/supplementary/fetch-previous-transactions.service.test.js index 2d90261599..31208a9f0d 100644 --- a/test/services/bill-runs/supplementary/fetch-previous-transactions.service.test.js +++ b/test/services/bill-runs/supplementary/fetch-previous-transactions.service.test.js @@ -35,11 +35,7 @@ describe('Fetch Previous Transactions service', () => { describe('when there are no transactions', () => { it('returns no results', async () => { - const result = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const result = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(result).to.be.empty() }) @@ -53,11 +49,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns results', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.have.length(1) expect(results[0].credit).to.be.false() @@ -81,11 +73,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns no results', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.be.empty() }) @@ -102,11 +90,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns the debits', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.have.length(1) expect(results[0].credit).to.be.false() @@ -132,11 +116,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns only the follow up debit', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.have.length(1) expect(results[0].credit).to.be.false() @@ -155,11 +135,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns both debits', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.have.length(2) expect(results.every((transaction) => !transaction.credit)).to.be.true() @@ -181,11 +157,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns no results', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.be.empty() }) @@ -202,11 +174,7 @@ describe('Fetch Previous Transactions service', () => { }) it('returns no results', async () => { - const results = await FetchPreviousTransactionsService.go( - { billingAccountId }, - { licenceId }, - financialYearEnding - ) + const results = await FetchPreviousTransactionsService.go(billingAccountId, licenceId, financialYearEnding) expect(results).to.be.empty() }) diff --git a/test/services/bill-runs/supplementary/process-transactions.service.test.js b/test/services/bill-runs/supplementary/process-transactions.service.test.js index 8cc7a5bdd0..960c3425f4 100644 --- a/test/services/bill-runs/supplementary/process-transactions.service.test.js +++ b/test/services/bill-runs/supplementary/process-transactions.service.test.js @@ -15,8 +15,11 @@ const FetchPreviousTransactionsService = require('../../../../app/services/bill- const ProcessTransactionsService = require('../../../../app/services/bill-runs/supplementary/process-transactions.service.js') describe('Process Transactions service', () => { - const bill = { id: 'a56ef6d9-370a-4224-b6ec-0fca8bfa4d1f' } - const billLicence = { id: '110ab2e2-6076-4d5a-a56f-b17a048eb269' } + const billingAccountId = 'a56ef6d9-370a-4224-b6ec-0fca8bfa4d1f' + const billLicence = { + id: '110ab2e2-6076-4d5a-a56f-b17a048eb269', + licenceId: '9d587a65-aa00-4be6-969e-5bbb9fc6c885' + } const billingPeriod = { startDate: new Date('2022-04-01'), @@ -55,7 +58,7 @@ describe('Process Transactions service', () => { it('returns the unmatched calculated transactions', async () => { const result = await ProcessTransactionsService.go( calculatedTransactions, - bill, + billingAccountId, billLicence, billingPeriod ) @@ -79,7 +82,7 @@ describe('Process Transactions service', () => { it('returns no transactions', async () => { const result = await ProcessTransactionsService.go( calculatedTransactions, - bill, + billingAccountId, billLicence, billingPeriod ) @@ -101,7 +104,7 @@ describe('Process Transactions service', () => { it('returns only the previous transactions', async () => { const result = await ProcessTransactionsService.go( [], - bill, + billingAccountId, billLicence, billingPeriod ) @@ -126,7 +129,7 @@ describe('Process Transactions service', () => { it('returns the unmatched calculated transactions and previous transactions (reversed)', async () => { const result = await ProcessTransactionsService.go( calculatedTransactions, - bill, + billingAccountId, billLicence, billingPeriod ) @@ -148,7 +151,7 @@ describe('Process Transactions service', () => { it('returns the calculated transactions unchanged', async () => { const result = await ProcessTransactionsService.go( calculatedTransactions, - bill, + billingAccountId, billLicence, billingPeriod ) From e4e949934ceba9a9e781d21687940faed469a7bb Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 23:16:34 +0000 Subject: [PATCH 6/7] Housekeeping - Switch to objection for query The original query was built when we thought we needed to get billing account details as part of the query. This was because we thought we'd be matching as we iterated the charge versions we fetch and their charge elements. The version of the supplementary billing engine that finally switched had opted to pre-generate billing data and then iterate it. The need to access billing account information on the transaction had become redundant. We should have noticed at the time but it's never too late! Because we can drop those elements we can switch to using Objection which tidies up the query some, for example, the `purposes` field now comes through as proper JSON. These changes to the what `FetchPreviousTransactionsService` returns also means we can remove some of the logic in `ReverseTransactionsService`. --- .../fetch-previous-transactions.service.js | 112 ++++++++---------- .../reverse-transactions.service.js | 21 +--- .../reverse-transactions.service.test.js | 18 ++- 3 files changed, 65 insertions(+), 86 deletions(-) diff --git a/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js b/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js index 43916ff25e..c7fc2db59c 100644 --- a/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js +++ b/app/services/bill-runs/supplementary/fetch-previous-transactions.service.js @@ -1,17 +1,16 @@ 'use strict' /** - * Fetches the previously billed transactions that match the bill, licence and year provided, removing any debits - * which are cancelled out by previous credits. + * Fetches the previously billed transactions that match, removing any debits which cancelled out by previous credits * @module FetchPreviousTransactionsService */ const { db } = require('../../../../db/db.js') const { transactionsMatch } = require('../../../lib/general.lib.js') +const TransactionModel = require('../../../models/transaction.model.js') /** - * Fetches the previously billed transactions that match the bill, licence and year provided, removing any debits - * which are cancelled out by previous credits. + * Fetches the previously billed transactions that match, removing any debits which cancelled out by previous credits * * @param {string} billingAccountId - The UUID that identifies the billing account we need to fetch transactions for * @param {string} licenceId - The UUID that identifies the licence we need to fetch transactions for @@ -21,11 +20,7 @@ const { transactionsMatch } = require('../../../lib/general.lib.js') * @returns {Promise} The resulting matched transactions */ async function go (billingAccountId, licenceId, financialYearEnding) { - const transactions = await _fetch( - billingAccountId, - licenceId, - financialYearEnding - ) + const transactions = await _fetch(billingAccountId, licenceId, financialYearEnding) return _cleanse(transactions) } @@ -54,67 +49,54 @@ function _cleanse (transactions) { } async function _fetch (billingAccountId, licenceId, financialYearEnding) { - return db - .select( - 't.authorisedDays', - 't.billableDays', - 't.waterUndertaker', - 't.chargeReferenceId', - 't.startDate', - 't.endDate', - 't.source', - 't.season', - 't.loss', - 't.credit', - 't.chargeType', - 't.authorisedQuantity', - 't.billableQuantity', - 't.description', - 't.volume', - 't.section126Factor', - 't.section127Agreement', + return TransactionModel.query() + .select([ + 'transactions.authorisedDays', + 'transactions.billableDays', + 'transactions.waterUndertaker', + 'transactions.chargeReferenceId', + 'transactions.startDate', + 'transactions.endDate', + 'transactions.source', + 'transactions.season', + 'transactions.loss', + 'transactions.credit', + 'transactions.chargeType', + 'transactions.authorisedQuantity', + 'transactions.billableQuantity', + 'transactions.description', + 'transactions.volume', + 'transactions.section126Factor', + 'transactions.section127Agreement', + 'transactions.secondPartCharge', + 'transactions.scheme', + 'transactions.aggregateFactor', + 'transactions.adjustmentFactor', + 'transactions.chargeCategoryCode', + 'transactions.chargeCategoryDescription', + 'transactions.supportedSource', + 'transactions.supportedSourceName', + 'transactions.newLicence', + 'transactions.waterCompanyCharge', + 'transactions.winterOnly', + 'transactions.purposes', // NOTE: The section130Agreement field is a varchar in the DB for historic reasons. It seems some early PRESROC // transactions recorded values other than 'true' or 'false'. For SROC though, it will only ever be true/false. We // generate our calculated billing transaction lines based on the Section130 flag against charge_elements which is // always a boolean. So, to avoid issues when we need to compare the values we cast this to a boolean when // fetching the data. - db.raw('t.section_130_agreement::boolean'), - 't.secondPartCharge', - 't.scheme', - 't.aggregateFactor', - 't.adjustmentFactor', - 't.chargeCategoryCode', - 't.chargeCategoryDescription', - 't.supportedSource', - 't.supportedSourceName', - 't.newLicence', - 't.waterCompanyCharge', - 't.winterOnly', - 't.purposes', - 'validBills.billingAccountId', - 'validBills.accountNumber' - ) - .from('transactions as t') - .innerJoin( - db - .select( - 'bl.id', - 'b.billingAccountId', - 'b.accountNumber' - ) - .from('billLicences as bl') - .innerJoin('bills as b', 'bl.billId', 'b.id') - .innerJoin('billRuns as br', 'br.id', 'b.billRunId') - .where({ - 'bl.licenceId': licenceId, - 'b.billingAccountId': billingAccountId, - 'b.financialYearEnding': financialYearEnding, - 'br.status': 'sent', - 'br.scheme': 'sroc' - }) - .as('validBills'), - 't.billLicenceId', 'validBills.id' - ) + db.raw('transactions.section_130_agreement::boolean') + ]) + .innerJoin('billLicences', 'transactions.billLicenceId', 'billLicences.id') + .innerJoin('bills', 'billLicences.billId', 'bills.id') + .innerJoin('billRuns', 'bills.billRunId', 'billRuns.id') + .where({ + 'billLicences.licenceId': licenceId, + 'bills.billingAccountId': billingAccountId, + 'bills.financialYearEnding': financialYearEnding, + 'billRuns.status': 'sent', + 'billRuns.scheme': 'sroc' + }) } module.exports = { diff --git a/app/services/bill-runs/supplementary/reverse-transactions.service.js b/app/services/bill-runs/supplementary/reverse-transactions.service.js index ecc3514e47..f7dbc3a5aa 100644 --- a/app/services/bill-runs/supplementary/reverse-transactions.service.js +++ b/app/services/bill-runs/supplementary/reverse-transactions.service.js @@ -25,30 +25,17 @@ function go (transactions, billLicenceId) { /** * Receives an array of debit transactions and returns transactions that will reverse them. These transactions are - * identical except the `credit` flag is set to 'true', the status is set to `candidate`, the `billLicenceId` is set - * to the id of the supplied bill licence, and a new transaction ID is generated. + * identical except the `credit` flag is set to 'true', the status is set to `candidate`, the `billLicenceId` is for the + * bill licence we are creating, and a new transaction ID is generated. */ function _reverseTransactions (transactions, billLicenceId) { return transactions.map((transaction) => { - // TODO: The FetchTransactionsService which we use to get the transactions to reverse adds the billing account ID - // and number to each transaction returned. This is a performance measure to avoid an extra query to the DB. But if - // we don't strip them from the result when we try to persist our reversed versions, they fail because the - // transactions table doesn't have these fields. We do the stripping here to avoid iterating through the - // collection multiple times. Ideally, we'd look to return a result from FetchTransactionsService that avoids us - // having to do this. - const { billingAccountId, accountNumber, ...propertiesToKeep } = transaction - return { - ...propertiesToKeep, + ...transaction, id: generateUUID(), billLicenceId, credit: true, - status: 'candidate', - // TODO: Our query result seems to return the transaction's `purposes:` property as [Object]. Clearly, we need - // to re-jig something or give Knex some more instructions on dealing with this JSONB field. But just to prove - // the process is working we use this service to deal with the issue and extract the JSON from the array we've - // been provided. - purposes: transaction.purposes[0] + status: 'candidate' } }) } diff --git a/test/services/bill-runs/supplementary/reverse-transactions.service.test.js b/test/services/bill-runs/supplementary/reverse-transactions.service.test.js index 46c6a5d376..ec7fa8cc98 100644 --- a/test/services/bill-runs/supplementary/reverse-transactions.service.test.js +++ b/test/services/bill-runs/supplementary/reverse-transactions.service.test.js @@ -13,12 +13,16 @@ const ReverseTransactionsService = require('../../../../app/services/bill-runs/s describe('Reverse Transactions service', () => { const transactions = [ { - billingAccountId: '7190937e-e176-4d50-ae4f-c00c5e76938a', - accountNumber: 'B12345678A', name: 'DEBIT', credit: false, status: 'TO_BE_OVERWRITTEN', - purposes: ['foo'] + purposes: [{ + id: '04cbede8-45cf-433e-b4f5-f33dc911ced0', + abstractionPeriodStartDay: 1, + abstractionPeriodStartMonth: 4, + abstractionPeriodEndDay: 31, + abstractionPeriodEndMonth: 3 + }] } ] @@ -38,7 +42,13 @@ describe('Reverse Transactions service', () => { expect(result[0].status).to.equal('candidate') expect(result[0].billLicenceId).to.equal('8affaa71-c185-4b6c-9814-4c615c235611') expect(result[0].id).to.exist().and.to.be.a.string() - expect(result[0].purposes).to.equal('foo') + expect(result[0].purposes).to.equal([{ + id: '04cbede8-45cf-433e-b4f5-f33dc911ced0', + abstractionPeriodStartDay: 1, + abstractionPeriodStartMonth: 4, + abstractionPeriodEndDay: 31, + abstractionPeriodEndMonth: 3 + }]) }) }) }) From 1d7de73d15e701a996a4b25e604312cf702e9ead Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 24 Mar 2024 23:36:13 +0000 Subject: [PATCH 7/7] Fix broken tests --- .../supplementary/process-transactions.service.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/services/bill-runs/supplementary/process-transactions.service.test.js b/test/services/bill-runs/supplementary/process-transactions.service.test.js index 960c3425f4..baea512a63 100644 --- a/test/services/bill-runs/supplementary/process-transactions.service.test.js +++ b/test/services/bill-runs/supplementary/process-transactions.service.test.js @@ -110,8 +110,8 @@ describe('Process Transactions service', () => { ) 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') + expect(result[0].purposes).to.equal(['I_WILL_NOT_BE_REMOVED_1']) + expect(result[1].purposes).to.equal(['I_WILL_NOT_BE_REMOVED_2']) }) }) @@ -136,7 +136,7 @@ describe('Process Transactions service', () => { 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[1].purposes).to.equal(['I_WILL_NOT_BE_REMOVED']) expect(result[1].credit).to.be.true() }) })