From 54e2bf6db4de1e32d31eecf58339b37f721021e8 Mon Sep 17 00:00:00 2001 From: Stuart Adair <43574728+StuAA78@users.noreply.github.com> Date: Tue, 9 May 2023 11:43:47 +0100 Subject: [PATCH] Tidy supplementary billing code - second pass (#208) We've been working quickly to get supplementary billing completed on time. This means we haven't always been able to spend as much time as we'd like on some of the finer details -- documentation, consistency of standards, naming conventions, etc. [Following our first pass](https://github.com/DEFRA/water-abstraction-system/pull/197), this PR is a second pass at spotting these things and amending them. Note that while refactoring `ProcessBillingBatchService` we identified a couple of ways we can restructure the process. This will be handled in a separate PR; for now, we have simply refactored code into helper functions which more accurately describe the flow of the process. --- app/lib/legacy-request.lib.js | 4 +- app/lib/request.lib.js | 6 +- app/services/plugins/filter-routes.service.js | 2 +- ...te-authorised-and-billable-days.service.js | 13 ++- .../determine-charge-period.service.js | 21 +++-- ...h-previous-billing-transactions.service.js | 4 +- .../process-billing-batch.service.js | 80 +++++++++++++------ .../process-billing-transactions.service.js | 4 +- 8 files changed, 79 insertions(+), 55 deletions(-) diff --git a/app/lib/legacy-request.lib.js b/app/lib/legacy-request.lib.js index a54a9e76e5..4ad8ed3dce 100644 --- a/app/lib/legacy-request.lib.js +++ b/app/lib/legacy-request.lib.js @@ -76,7 +76,7 @@ const services = { * @returns {Object} result.response The legacy service's response if successful or the error response if not. */ async function get (serviceName, path, apiRequest = true) { - return await _sendRequest(RequestLib.get, serviceName, path, apiRequest) + return _sendRequest(RequestLib.get, serviceName, path, apiRequest) } /** @@ -93,7 +93,7 @@ async function get (serviceName, path, apiRequest = true) { * @returns {Object} result.response The legacy service's response if successful or the error response if not. */ async function post (serviceName, path, apiRequest = true, body = {}) { - return await _sendRequest(RequestLib.post, serviceName, path, apiRequest, body) + return _sendRequest(RequestLib.post, serviceName, path, apiRequest, body) } /** diff --git a/app/lib/request.lib.js b/app/lib/request.lib.js index cc952054d9..62b63554ce 100644 --- a/app/lib/request.lib.js +++ b/app/lib/request.lib.js @@ -31,15 +31,15 @@ const requestConfig = require('../../config/request.config.js') * @returns {Object} The result of the request; whether it succeeded and the response or error returned */ async function get (url, additionalOptions = {}) { - return await _sendRequest('get', url, additionalOptions) + return _sendRequest('get', url, additionalOptions) } async function patch (url, additionalOptions = {}) { - return await _sendRequest('patch', url, additionalOptions) + return _sendRequest('patch', url, additionalOptions) } async function post (url, additionalOptions = {}) { - return await _sendRequest('post', url, additionalOptions) + return _sendRequest('post', url, additionalOptions) } async function _sendRequest (method, url, additionalOptions) { diff --git a/app/services/plugins/filter-routes.service.js b/app/services/plugins/filter-routes.service.js index 27eb23c7c6..1aa7d858c9 100644 --- a/app/services/plugins/filter-routes.service.js +++ b/app/services/plugins/filter-routes.service.js @@ -42,7 +42,7 @@ function _protectedEnvironment (environment) { } function _filteredRoutes (routes) { - return routes.filter(route => !route?.options?.app?.excludeFromProd) + return routes.filter((route) => !route?.options?.app?.excludeFromProd) } module.exports = { diff --git a/app/services/supplementary-billing/calculate-authorised-and-billable-days.service.js b/app/services/supplementary-billing/calculate-authorised-and-billable-days.service.js index daf83990ef..5dcf96feee 100644 --- a/app/services/supplementary-billing/calculate-authorised-and-billable-days.service.js +++ b/app/services/supplementary-billing/calculate-authorised-and-billable-days.service.js @@ -61,10 +61,10 @@ function go (chargePeriod, billingPeriod, chargeElement) { const authorisedAbstractionPeriods = [] const billableAbstractionPeriods = [] - for (const chargePurpose of chargePurposes) { + chargePurposes.forEach((chargePurpose) => { authorisedAbstractionPeriods.push(..._abstractionPeriods(billingPeriod, chargePurpose)) billableAbstractionPeriods.push(..._abstractionPeriods(chargePeriod, chargePurpose)) - } + }) return { authorisedDays: _consolidateAndCalculate(billingPeriod, authorisedAbstractionPeriods), @@ -207,13 +207,12 @@ function _calculateAbstractionOverlapPeriod (referencePeriod, abstractionPeriod) function _consolidateAndCalculate (referencePeriod, abstractionsPeriods) { const consolidatedAbstractionPeriods = ConsolidateDateRangesService.go(abstractionsPeriods) - let days = 0 - for (const abstractionPeriod of consolidatedAbstractionPeriods) { + const totalDays = consolidatedAbstractionPeriods.reduce((acc, abstractionPeriod) => { const abstractionOverlapPeriod = _calculateAbstractionOverlapPeriod(referencePeriod, abstractionPeriod) - days += _calculateDays(abstractionOverlapPeriod) - } + return acc + _calculateDays(abstractionOverlapPeriod) + }, 0) - return days + return totalDays } /** diff --git a/app/services/supplementary-billing/determine-charge-period.service.js b/app/services/supplementary-billing/determine-charge-period.service.js index 66abf78dca..dbf598012a 100644 --- a/app/services/supplementary-billing/determine-charge-period.service.js +++ b/app/services/supplementary-billing/determine-charge-period.service.js @@ -33,19 +33,16 @@ function go (chargeVersion, financialYearEnding) { chargeVersion.licence.startDate ) - // If the end date is null then use the financial year end date instead as Math.min() will count nulls as 0 - const chargeVersionEndDate = chargeVersion.endDate || financialYearEndDate - const licenceExpiredDate = chargeVersion.licence.expiredDate || financialYearEndDate - const licencelapsedDate = chargeVersion.licence.lapsedDate || financialYearEndDate - const licencerevokedDate = chargeVersion.licence.revokedDate || financialYearEndDate - - const earliestEndDateTimestamp = Math.min( + // We use .filter() to remove any null timestamps, as Math.min() assumes a value of `0` for these + const endDateTimestamps = [ financialYearEndDate, - chargeVersionEndDate, - licenceExpiredDate, - licencelapsedDate, - licencerevokedDate - ) + chargeVersion.endDate, + chargeVersion.licence.expiredDate, + chargeVersion.licence.lapsedDate, + chargeVersion.licence.revokedDate + ].filter((timestamp) => timestamp) + + const earliestEndDateTimestamp = Math.min(...endDateTimestamps) return { startDate: new Date(latestStartDateTimestamp), diff --git a/app/services/supplementary-billing/fetch-previous-billing-transactions.service.js b/app/services/supplementary-billing/fetch-previous-billing-transactions.service.js index 4305e8c3e3..ae92e904b6 100644 --- a/app/services/supplementary-billing/fetch-previous-billing-transactions.service.js +++ b/app/services/supplementary-billing/fetch-previous-billing-transactions.service.js @@ -39,7 +39,7 @@ function _cleanse (billingTransactions) { const credits = billingTransactions.filter((transaction) => transaction.isCredit) const debits = billingTransactions.filter((transaction) => !transaction.isCredit) - for (const credit of credits) { + credits.forEach((credit) => { const debitIndex = debits.findIndex((debit) => { return debit.billableDays === credit.billableDays && debit.chargeType === credit.chargeType }) @@ -47,7 +47,7 @@ function _cleanse (billingTransactions) { if (debitIndex > -1) { debits.splice(debitIndex, 1) } - } + }) return debits } diff --git a/app/services/supplementary-billing/process-billing-batch.service.js b/app/services/supplementary-billing/process-billing-batch.service.js index 77fadcf983..18544b30e2 100644 --- a/app/services/supplementary-billing/process-billing-batch.service.js +++ b/app/services/supplementary-billing/process-billing-batch.service.js @@ -58,30 +58,23 @@ async function go (billingBatch, billingPeriod) { billingPeriod ) - // We need to deal with the very first iteration when currentBillingData is all nulls! So, we check both there is - // a billingInvoiceLicence and that its ID is different - if ( - currentBillingData.billingInvoiceLicence && - currentBillingData.billingInvoiceLicence.billingInvoiceLicenceId !== billingInvoiceLicence.billingInvoiceLicenceId - ) { + // If we've moved on to the next invoice licence then we need to finalise the previous one before we can continue + if (_thisIsADifferentBillingInvoiceLicence(currentBillingData, billingInvoiceLicence)) { await _finaliseCurrentInvoiceLicence(currentBillingData, billingPeriod, billingBatch) currentBillingData.calculatedTransactions = [] } - currentBillingData.licence = chargeVersion.licence - currentBillingData.billingInvoice = billingInvoice - currentBillingData.billingInvoiceLicence = billingInvoiceLicence - - // If the charge version has a status of 'current' (APPROVED) then it is likely to be something we have never - // billed previously so we need to calculate a debit line. - // Else the charge version has been 'superseded' (REPLACED). So, we won't be adding a new debit line to the bill - // for it. But we still need to process it to understand what, if anything, needs to be credited back or if our - // calculated debit line has already been billed. - if (chargeVersion.status === 'current') { - const calculatedTransactions = _generateCalculatedTransactions(billingPeriod, chargeVersion, billingBatchId, billingInvoiceLicence) - currentBillingData.calculatedTransactions.push(...calculatedTransactions) - } + _updateCurrentBillingData(currentBillingData, chargeVersion, billingInvoice, billingInvoiceLicence) + + _generateTransactionsIfStatusIsCurrent( + chargeVersion, + billingPeriod, + billingBatchId, + billingInvoiceLicence, + currentBillingData + ) } + await _finaliseCurrentInvoiceLicence(currentBillingData, billingPeriod, billingBatch) await _finaliseBillingBatch(billingBatch, chargeVersions, currentBillingData.isEmpty) @@ -93,6 +86,39 @@ async function go (billingBatch, billingPeriod) { } } +function _generateTransactionsIfStatusIsCurrent (chargeVersion, billingPeriod, billingBatchId, billingInvoiceLicence, currentBillingData) { + // If the charge version status isn't 'current' then we don't need to add any new debit lines to the bill + if (chargeVersion.status !== 'current') { + return + } + + // Otherwise, it's likely to be something we have never billed previously so we need to calculate debit line(s) + const calculatedTransactions = _generateCalculatedTransactions( + billingPeriod, + chargeVersion, + billingBatchId, + billingInvoiceLicence + ) + currentBillingData.calculatedTransactions.push(...calculatedTransactions) +} + +function _updateCurrentBillingData (currentBillingData, chargeVersion, billingInvoice, billingInvoiceLicence) { + currentBillingData.licence = chargeVersion.licence + currentBillingData.billingInvoice = billingInvoice + currentBillingData.billingInvoiceLicence = billingInvoiceLicence +} + +function _thisIsADifferentBillingInvoiceLicence (currentBillingData, billingInvoiceLicence) { + // If we don't yet have a billing invoice licence (which will be the case the first time we iterate over the charge + // versions) then simply return false straight away + if (!currentBillingData.billingInvoiceLicence) { + return false + } + + // Otherwise we want to return true if the previous and current licence ids don't match, and false if they do match + return currentBillingData.billingInvoiceLicence.billingInvoiceLicenceId !== billingInvoiceLicence.billingInvoiceLicenceId +} + /** * Log the time taken to process the billing batch * @@ -164,7 +190,11 @@ async function _fetchChargeVersions (billingBatch, billingPeriod) { // 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) + const chargeVersions = await FetchChargeVersionsService.go(billingBatch.regionId, billingPeriod) + + // We don't just `return FetchChargeVersionsService.go()` as we need to call HandleErroredBillingBatchService if it + // fails + return chargeVersions } catch (error) { HandleErroredBillingBatchService.go( billingBatch.billingBatchId, @@ -252,18 +282,16 @@ function _generateCalculatedTransactions (billingPeriod, chargeVersion, billingB const isNewLicence = DetermineMinimumChargeService.go(chargeVersion, financialYearEnding) const isWaterUndertaker = chargeVersion.licence.isWaterUndertaker - const transactions = [] - for (const chargeElement of chargeVersion.chargeElements) { - const result = GenerateBillingTransactionsService.go( + // We use flatMap as GenerateBillingTransactionsService returns an array of transactions + const transactions = chargeVersion.chargeElements.flatMap((chargeElement) => { + return GenerateBillingTransactionsService.go( chargeElement, billingPeriod, chargePeriod, isNewLicence, isWaterUndertaker ) - result.billingInvoiceLicenceId = billingInvoiceLicence.billingInvoiceLicenceId - transactions.push(...result) - } + }) return transactions } catch (error) { diff --git a/app/services/supplementary-billing/process-billing-transactions.service.js b/app/services/supplementary-billing/process-billing-transactions.service.js index 8ed6256304..25eaa54b69 100644 --- a/app/services/supplementary-billing/process-billing-transactions.service.js +++ b/app/services/supplementary-billing/process-billing-transactions.service.js @@ -112,11 +112,11 @@ function _cleanseTransactions (calculatedTransactions, reverseTransactions) { // "cancelling pair" with it. If not then add the unpaired calculated transaction to our array of cleansed transaction // lines. Note that `reverseTransactions` will be mutated to remove any reverse transactions which form a cancelling // pair. - for (const calculatedTransactionLine of calculatedTransactions) { + calculatedTransactions.forEach((calculatedTransactionLine) => { if (!_cancelCalculatedTransaction(calculatedTransactionLine, reverseTransactions)) { cleansedTransactionLines.push(calculatedTransactionLine) } - } + }) // Add the remaining reverse transactions (ie. those which didn't form a cancelling pair) cleansedTransactionLines.push(...reverseTransactions)