Refactor ProcessBillingBatchService - first pass

app/services/supplementary-billing/fetch-invoice-account-numbers.service.js

@@ -0,0 +1,52 @@
+'use strict'
+
+/**
+ * Fetches all invoice account numbers for the supplied charge versions
+ * @module FetchInvoiceAccountNumbersService
+ */
+
+const InvoiceAccountModel = require('../../models/crm-v2/invoice-account.model.js')
+
+/**
+ * Fetch all invoice account numbers for the supplied charge versions
+ *
+ * @param {module:ChargeVersionModel[]} chargeVersions An array of charge versions
+ *
+ * @returns {Object[]} Array of objects in the format { invoiceAccountId: '...', invoiceAccountNumber: '...' }
+ */
+async function go (chargeVersions) {
+  const uniqueInvoiceAccountIds = _extractUniqueInvoiceAccountIds(chargeVersions)
+  const invoiceAccountModels = await _fetch(uniqueInvoiceAccountIds)
+
+  // The results come back from Objection as InvoiceAccountModels. Since we want to be clear that these are not
+  // full-blown models, we turn them into plain objects using Objection's .toJSON() method
+  const invoiceAccountObjects = _makeObjects(invoiceAccountModels)
+
+  return invoiceAccountObjects
+}
+
+function _extractUniqueInvoiceAccountIds (chargeVersions) {
+  const allInvoiceAccountIds = chargeVersions.map((chargeVersion) => {
+    return chargeVersion.invoiceAccountId
+  })
+
+  // Creating a new set from allInvoiceAccountIds gives us just the unique ids. Objection does not accept sets in
+  // .findByIds() so we spread it into an array
+  return [...new Set(allInvoiceAccountIds)]
+}
+
+function _fetch (uniqueInvoiceAccountIds) {
+  return InvoiceAccountModel.query()
+    .select('invoiceAccountId', 'invoiceAccountNumber')
+    .findByIds([...uniqueInvoiceAccountIds])
+}
+
+function _makeObjects (models) {
+  return models.map(model => {
+    return model.toJSON()
+  })
+}
+
+module.exports = {
+  go
+}

app/services/supplementary-billing/generate-billing-invoice.service.js

@@ -7,44 +7,20 @@
 
 const { randomUUID } = require('crypto')
 
-const InvoiceAccountModel = require('../../models/crm-v2/invoice-account.model.js')
-
 /**
- * Return either a new billing invoice object ready for persisting or an existing one if it exists
- *
- * This first checks whether the invoice account ID of `currentBillingInvoice` matches the one passed to this service.
- * If it does, we return that instance.
- *
- * If it doesn't, we generate a new instance and return it.
- *
- * For context, this is all to avoid creating `billing_invoice` and `billing_invoice_licence` records unnecessarily.
- * The legacy service will create them first, then determine if there are any transactions to be billed. If there
- * aren't, it then has to go back and delete the records it created.
+ * Return a billing invoice object ready for persisting
  *
- * Our intent is to only call the DB when we have records that need persisting. So, we start at the transaction level
- * and only persist `billing_invoice` and `billing_invoice_licence` records that are linked to billable transactions.
- * But to persist the billing transactions we need the foreign keys. So, we generate our billing invoice and billing
- * licence data in memory along with ID's, and use this service to provide the right record when persisting the
- * transaction.
- *
- * @param {module:BillingInvoiceModel} currentBillingInvoice A billing invoice object
- * @param {String} invoiceAccountId UUID of the invoice account this billing invoice will be linked to if persisted
- * @param {String} billingBatchId UUID of the billing batch this billing invoice will be linked to if persisted
+ * @param {module:InvoiceAccountModel} invoiceAccount The invoice account this billing invoice will be linked to
+ * @param {String} billingBatchId UUID of the billing batch this billing invoice will be linked to
  * @param {Number} financialYearEnding A value that must exist in the persisted record
  *
- * @returns {Object} The current or newly-generated billing invoice object
+ * @returns {Object} The billing invoice object ready to be persisted
  */
-async function go (currentBillingInvoice, invoiceAccountId, billingBatchId, financialYearEnding) {
-  if (currentBillingInvoice?.invoiceAccountId === invoiceAccountId) {
-    return currentBillingInvoice
-  }
-
-  const invoiceAccount = await InvoiceAccountModel.query().findById(invoiceAccountId)
-
+function go (invoiceAccount, billingBatchId, financialYearEnding) {
   const billingInvoice = {
     billingBatchId,
     financialYearEnding,
-    invoiceAccountId,
+    invoiceAccountId: invoiceAccount.invoiceAccountId,
     billingInvoiceId: randomUUID({ disableEntropyCache: true }),
     address: {}, // Address is set to an empty object for SROC billing invoices
     invoiceAccountNumber: invoiceAccount.invoiceAccountNumber,

app/services/supplementary-billing/process-billing-batch.service.js

@@ -15,6 +15,7 @@ const CreateBillingTransactionService = require('./create-billing-transaction.se
 const DetermineChargePeriodService = require('./determine-charge-period.service.js')
 const DetermineMinimumChargeService = require('./determine-minimum-charge.service.js')
 const FetchChargeVersionsService = require('./fetch-charge-versions.service.js')
+const FetchInvoiceAccountNumbersService = require('./fetch-invoice-account-numbers.service.js')
 const GenerateBillingTransactionsService = require('./generate-billing-transactions.service.js')
 const GenerateBillingInvoiceService = require('./generate-billing-invoice.service.js')
 const GenerateBillingInvoiceLicenceService = require('./generate-billing-invoice-licence.service.js')
@@ -49,13 +50,16 @@ async function go (billingBatch, billingPeriod) {
     await _updateStatus(billingBatchId, 'processing')
 
     const chargeVersions = await _fetchChargeVersions(billingBatch, billingPeriod)
+    const invoiceAccounts = await _fetchInvoiceData(chargeVersions, billingBatch.billingBatchId)
 
     for (const chargeVersion of chargeVersions) {
-      const { billingInvoice, billingInvoiceLicence } = await _generateInvoiceData(
+      const billingInvoice = _generateInvoiceData(invoiceAccounts, chargeVersion, billingBatchId, billingPeriod)
+
+      const billingInvoiceLicence = _generateInvoiceLicenceData(
         currentBillingData,
         billingBatch,
         chargeVersion,
-        billingPeriod
+        billingInvoice
       )
 
       // If we've moved on to the next invoice licence then we need to finalise the previous one before we can continue
@@ -86,6 +90,44 @@ async function go (billingBatch, billingPeriod) {
   }
 }
 
+function _generateInvoiceData (invoiceAccounts, chargeVersion, billingBatchId, billingPeriod) {
+  // Pull the invoice account from our previously-fetched accounts
+  const invoiceAccount = invoiceAccounts[chargeVersion.invoiceAccountId]
+
+  const billingInvoice = GenerateBillingInvoiceService.go(
+    invoiceAccount,
+    billingBatchId,
+    billingPeriod.endDate.getFullYear()
+  )
+
+  return billingInvoice
+}
+
+async function _fetchInvoiceData (chargeVersions, billingBatchId) {
+  try {
+    const invoiceAccountsArray = await FetchInvoiceAccountNumbersService.go(chargeVersions)
+
+    // We create a keyed object from the array so we can quickly retrieve the required invoice account later. This will
+    // be in the format:
+    // {
+    //   'uuid-1': { invoiceAccountId: 'uuid-1', ... },
+    //   'uuid-2': { invoiceAccountId: 'uuid-2', ... }
+    // }
+    const invoiceAccountsObject = invoiceAccountsArray.reduce((acc, item) => {
+      return {
+        ...acc,
+        [item.invoiceAccountId]: item
+      }
+    }, {})
+
+    return invoiceAccountsObject
+  } catch (error) {
+    HandleErroredBillingBatchService.go(billingBatchId)
+
+    throw error
+  }
+}
+
 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') {
@@ -255,19 +297,15 @@ async function _finaliseCurrentInvoiceLicence (currentBillingData, billingPeriod
   }
 }
 
-async function _generateInvoiceData (currentBillingData, billingBatch, chargeVersion, billingPeriod) {
+function _generateInvoiceLicenceData (currentBillingData, billingBatch, chargeVersion, billingInvoice) {
   try {
-    const { invoiceAccountId, licence } = chargeVersion
-    const { billingBatchId } = billingBatch
-    const financialYearEnding = billingPeriod.endDate.getFullYear()
-
-    const billingInvoice = await GenerateBillingInvoiceService.go(currentBillingData.billingInvoice, invoiceAccountId, billingBatchId, financialYearEnding)
-    const billingInvoiceLicence = GenerateBillingInvoiceLicenceService.go(currentBillingData.billingInvoiceLicence, billingInvoice.billingInvoiceId, licence)
+    const billingInvoiceLicence = GenerateBillingInvoiceLicenceService.go(
+      currentBillingData.billingInvoiceLicence,
+      billingInvoice.billingInvoiceId,
+      chargeVersion.licence
+    )
 
-    return {
-      billingInvoice,
-      billingInvoiceLicence
-    }
+    return billingInvoiceLicence
   } catch (error) {
     HandleErroredBillingBatchService.go(billingBatch.billingBatchId)
 

test/services/supplementary-billing/fetch-invoice-account-numbers.service.test.js

@@ -0,0 +1,55 @@
+'use strict'
+
+// Test framework dependencies
+const Lab = require('@hapi/lab')
+const Code = require('@hapi/code')
+
+const { describe, it, beforeEach } = exports.lab = Lab.script()
+const { expect } = Code
+
+// Test helpers
+const DatabaseHelper = require('../../support/helpers/database.helper.js')
+const InvoiceAccountHelper = require('../../support/helpers/crm-v2/invoice-account.helper.js')
+
+// Thing under test
+const FetchInvoiceAccountNumbersService = require('../../../app/services/supplementary-billing/fetch-invoice-account-numbers.service.js')
+
+describe('Fetch Invoice Account Numbers service', () => {
+  beforeEach(async () => {
+    await DatabaseHelper.clean()
+  })
+
+  describe('when the service is called with an array of charge versions', () => {
+    let expectedResult
+    let invoiceAccounts
+
+    beforeEach(async () => {
+      // We create three invoice accounts but we will only be fetching the first two
+      invoiceAccounts = await Promise.all([
+        await InvoiceAccountHelper.add(),
+        await InvoiceAccountHelper.add(),
+        await InvoiceAccountHelper.add()
+      ])
+
+      expectedResult = [
+        {
+          invoiceAccountId: invoiceAccounts[0].invoiceAccountId,
+          invoiceAccountNumber: invoiceAccounts[0].invoiceAccountNumber
+        },
+        {
+          invoiceAccountId: invoiceAccounts[1].invoiceAccountId,
+          invoiceAccountNumber: invoiceAccounts[1].invoiceAccountNumber
+        }
+      ]
+    })
+
+    it('fetches the invoice accounts that the charge versions link to', async () => {
+      const result = await FetchInvoiceAccountNumbersService.go([
+        { invoiceAccountId: invoiceAccounts[0].invoiceAccountId },
+        { invoiceAccountId: invoiceAccounts[1].invoiceAccountId }
+      ])
+
+      expect(result).to.have.length(2).and.contain(expectedResult)
+    })
+  })
+})

test/services/supplementary-billing/generate-billing-invoice.service.test.js

@@ -18,82 +18,35 @@ describe('Generate billing invoice service', () => {
   const billingBatchId = 'f4fb6257-c50f-46ea-80b0-7533423d6efd'
   const financialYearEnding = 2023
 
-  let currentBillingInvoice
   let expectedResult
   let invoiceAccount
 
   beforeEach(async () => {
     await DatabaseHelper.clean()
 
     invoiceAccount = await InvoiceAccountHelper.add()
-  })
 
-  describe('when `currentBillingInvoice` is null', () => {
-    beforeEach(async () => {
-      currentBillingInvoice = null
-      expectedResult = _billingInvoiceGenerator(invoiceAccount, billingBatchId, financialYearEnding)
-    })
+    expectedResult = {
+      invoiceAccountId: invoiceAccount.invoiceAccountId,
+      address: {},
+      invoiceAccountNumber: invoiceAccount.invoiceAccountNumber,
+      billingBatchId,
+      financialYearEnding,
+      isCredit: false
+    }
+  })
 
-    it('returns a new billing invoice with the provided values', async () => {
-      const result = await GenerateBillingInvoiceService.go(
-        currentBillingInvoice,
-        invoiceAccount.invoiceAccountId,
+  describe('when called', () => {
+    it('returns a new billing invoice with the provided values', () => {
+      const result = GenerateBillingInvoiceService.go(
+        invoiceAccount,
         billingBatchId,
         financialYearEnding
       )
 
       expect(result).to.equal(expectedResult, { skip: 'billingInvoiceId' })
-    })
-  })
-
-  describe('when `currentBillingInvoice` is set', () => {
-    beforeEach(async () => {
-      currentBillingInvoice = _billingInvoiceGenerator(invoiceAccount, billingBatchId, financialYearEnding)
-    })
-
-    describe('and the invoice account ID matches', () => {
-      it('returns the `currentBillingInvoice`', async () => {
-        const result = await GenerateBillingInvoiceService.go(
-          currentBillingInvoice,
-          currentBillingInvoice.invoiceAccountId,
-          billingBatchId,
-          financialYearEnding
-        )
-
-        expect(result).to.equal(currentBillingInvoice)
-      })
-    })
-
-    describe('and the invoice account ID does not match', () => {
-      let otherInvoiceAccount
-
-      beforeEach(async () => {
-        otherInvoiceAccount = await InvoiceAccountHelper.add()
-      })
-
-      it('returns a new billing invoice with the provided values', async () => {
-        const result = await GenerateBillingInvoiceService.go(
-          currentBillingInvoice,
-          otherInvoiceAccount.invoiceAccountId,
-          billingBatchId,
-          financialYearEnding
-        )
-
-        expect(result).not.to.equal(currentBillingInvoice)
-        expect(result.invoiceAccountId).to.equal(otherInvoiceAccount.invoiceAccountId)
-      })
+      // Separate check for billingInvoiceId as it will be a random UUID
+      expect(result.billingInvoiceId).to.be.a.string().and.to.have.length(36)
     })
   })
 })
-
-function _billingInvoiceGenerator (invoiceAccount, billingBatchId, financialYearEnding) {
-  return {
-    billingInvoiceId: 'fa0c763e-3976-42df-ae2c-e93a954701dd',
-    invoiceAccountId: invoiceAccount.invoiceAccountId,
-    address: {},
-    invoiceAccountNumber: invoiceAccount.invoiceAccountNumber,
-    billingBatchId,
-    financialYearEnding,
-    isCredit: false
-  }
-}