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](#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

@@ -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)
 }
 
 /**

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) {

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 = {

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
 }
 
 /**

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),

app/services/supplementary-billing/fetch-previous-billing-transactions.service.js

@@ -39,15 +39,15 @@ 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
     })
 
     if (debitIndex > -1) {
       debits.splice(debitIndex, 1)
     }
-  }
+  })
 
   return debits
 }

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) {

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)