Add reissuing to ProcessBillingBatchService (#290)

https://eaflood.atlassian.net/browse/WATER-4036

We update `ProcessBillingBatchService` to call our new `ReissueInvoicesService`. This has involved a small refactor to `ProcessBillingBatchService` so that each stage is called in its own function, leaving the main `go` function clean.

We also hide the reissuing behind a feature flag, which is set by `ENABLE_REISSUING_BILLING_BATCHES` in `.env`. This enables us to selectively enable reissuing in environments so that it isn't prematurely deployed.

.env.example

@@ -50,3 +50,6 @@ AWS_REGION=eu-west-1
 AWS_ACCESS_KEY_ID=uploadaccesskey
 AWS_SECRET_ACCESS_KEY=uploadsecretkey
 AWS_MAINTENANCE_BUCKET=upload-bucket-gov-uk
+
+# Feature flags
+ENABLE_REISSUING_BILLING_BATCHES=false

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

@@ -1,51 +1,84 @@
 'use strict'
 
+/**
+ * Process a given billing batch for the given billing periods
+ * @module ProcessBillingBatchService
+ */
+
 const BillingBatchError = require('../../errors/billing-batch.error.js')
 const BillingBatchModel = require('../../models/water/billing-batch.model.js')
 const ChargingModuleGenerateService = require('../charging-module/generate-bill-run.service.js')
 const FetchChargeVersionsService = require('./fetch-charge-versions.service.js')
 const HandleErroredBillingBatchService = require('./handle-errored-billing-batch.service.js')
 const LegacyRequestLib = require('../../lib/legacy-request.lib.js')
 const ProcessBillingPeriodService = require('./process-billing-period.service.js')
+const ReissueInvoicesService = require('./reissue-invoices.service.js')
 const UnflagUnbilledLicencesService = require('./unflag-unbilled-licences.service.js')
 
+const FeatureFlagsConfig = require('../../../config/feature-flags.config.js')
+
+/**
+ * Process a given billing batch for the given billing periods. In this case, "process" means that we create the
+ * required invoices and transactions for it in both this service and the Charging Module.
+ *
+ * TODO: flesh out these docs
+ *
+ * @param {module:BillingBatchModel} billingBatch
+ * @param {Object[]} billingPeriods An array of billing periods each containing a `startDate` and `endDate`
+ */
 async function go (billingBatch, billingPeriods) {
   const { billingBatchId } = billingBatch
 
   try {
-    // Mark the start time for later logging
     const startTime = process.hrtime.bigint()
-    const accumulatedLicenceIds = []
-
-    let isBatchPopulated = false
 
     await _updateStatus(billingBatchId, 'processing')
 
-    for (const billingPeriod of billingPeriods) {
-      const { chargeVersions, licenceIdsForPeriod } = await _fetchChargeVersions(billingBatch, billingPeriod)
-      const isPeriodPopulated = await ProcessBillingPeriodService.go(billingBatch, billingPeriod, chargeVersions)
+    const resultOfReissuing = await _reissueInvoices(billingBatch)
 
-      accumulatedLicenceIds.push(...licenceIdsForPeriod)
+    await _processBillingPeriods(billingPeriods, billingBatch, billingBatchId, resultOfReissuing)
 
-      if (isPeriodPopulated) {
-        isBatchPopulated = true
-      }
-    }
-
-    // Creating a new set from accumulatedLicenceIds gives us just the unique ids. Objection does not accept sets in
-    // .findByIds() so we spread it into an array
-    const allLicenceIds = [...new Set(accumulatedLicenceIds)]
-
-    await _finaliseBillingBatch(billingBatch, allLicenceIds, isBatchPopulated)
-
-    // Log how long the process took
     _calculateAndLogTime(billingBatchId, startTime)
   } catch (error) {
     await HandleErroredBillingBatchService.go(billingBatchId, error.code)
     _logError(billingBatch, error)
   }
 }
 
+async function _processBillingPeriods (billingPeriods, billingBatch, billingBatchId, resultOfReissuing) {
+  const accumulatedLicenceIds = []
+
+  // We use `results` to check if any db changes have been made (which is indicated by a billing period being processed
+  // and returning `true`). We populate it with the result of reissuing as this also indicates whether db changes have
+  // been made.
+  const results = [resultOfReissuing]
+
+  for (const billingPeriod of billingPeriods) {
+    const { chargeVersions, licenceIdsForPeriod } = await _fetchChargeVersions(billingBatch, billingPeriod)
+    const isPeriodPopulated = await ProcessBillingPeriodService.go(billingBatch, billingPeriod, chargeVersions)
+
+    accumulatedLicenceIds.push(...licenceIdsForPeriod)
+    results.push(isPeriodPopulated)
+  }
+
+  await _finaliseBillingBatch(billingBatch, accumulatedLicenceIds, results)
+}
+
+/**
+ * Call `ReissueInvoicesService` and log the time taken. We return `true` if any invoices have been reissued (which will
+ * have resulted in db changes), otherwise we return `false` (to indicate there have been no db changes)
+ */
+async function _reissueInvoices (billingBatch) {
+  // If reissuing isn't enabled then simply return `false` to indicate no db change has been made
+  if (!FeatureFlagsConfig.enableReissuingBillingBatches) {
+    return false
+  }
+
+  const result = await ReissueInvoicesService.go(billingBatch)
+
+  return result
+}
+
 /**
   * Log the time taken to process the billing batch
   *
@@ -81,16 +114,24 @@ async function _fetchChargeVersions (billingBatch, billingPeriod) {
  * process, and refreshes the billing batch locally. However if there were no resulting invoice licences then we simply
  * unflag the unbilled licences and mark the billing batch with `empty` status
  */
-async function _finaliseBillingBatch (billingBatch, allLicenceIds, isPopulated) {
+async function _finaliseBillingBatch (billingBatch, accumulatedLicenceIds, resultsOfProcessing) {
+  // Creating a new set from accumulatedLicenceIds gives us just the unique ids. Objection does not accept sets in
+  // .findByIds() so we spread it into an array
+  const allLicenceIds = [...new Set(accumulatedLicenceIds)]
+
   await UnflagUnbilledLicencesService.go(billingBatch.billingBatchId, allLicenceIds)
 
+  // We set `isPopulated` to `true` if at least one processing result was truthy
+  const isPopulated = resultsOfProcessing.some(result => result)
+
   // If there are no billing invoice licences then the bill run is considered empty. We just need to set the status to
   // indicate this in the UI
   if (!isPopulated) {
-    return _updateStatus(billingBatch.billingBatchId, 'empty')
+    await _updateStatus(billingBatch.billingBatchId, 'empty')
+    return
   }
 
-  // We then need to tell the Charging Module to run its generate process. This is where the Charging module finalises
+  // We now need to tell the Charging Module to run its generate process. This is where the Charging module finalises
   // the debit and credit amounts, and adds any additional transactions needed, for example, minimum charge
   await ChargingModuleGenerateService.go(billingBatch.externalId)
 

app/services/supplementary-billing/unflag-unbilled-licences.service.js

@@ -26,19 +26,21 @@ const LicenceModel = require('../../models/water/licence.model.js')
  * The query we run during the **SEND** process unflags only those which are linked to the bill run being sent. We can
  * do this because we know this service has handled anything that was unbilled and not represented.
  *
- * @param {*} billingBatchId The ID of the bill run (billing batch) being processed
+ * @param {String} billingBatchId The ID of the bill run (billing batch) being processed
  * @param {String[]} allLicenceIds All licence IDs being processed in the bill run
  * @returns {Number} count of records updated
  */
 async function go (billingBatchId, allLicenceIds) {
-  return LicenceModel.query()
+  const result = await LicenceModel.query()
     .patch({ includeInSrocSupplementaryBilling: false })
     .whereIn('licenceId', allLicenceIds)
     .whereNotExists(
       LicenceModel.relatedQuery('billingInvoiceLicences')
         .join('billingInvoices', 'billingInvoices.billingInvoiceId', '=', 'billingInvoiceLicences.billingInvoiceId')
         .where('billingInvoices.billingBatchId', '=', billingBatchId)
     )
+
+  return result
 }
 
 module.exports = {

config/feature-flags.config.js

@@ -0,0 +1,18 @@
+'use strict'
+
+/**
+ * Config values used to enable feature flags
+ * @module FeatureFlagsConfig
+ */
+
+// We require dotenv directly in each config file to support unit tests that depend on this this subset of config.
+// Requiring dotenv in multiple places has no effect on the app when running for real.
+require('dotenv').config()
+
+const config = {
+  // Credit to https://stackoverflow.com/a/323546/6117745 for how to handle
+  // converting the env var to a boolean
+  enableReissuingBillingBatches: (String(process.env.ENABLE_REISSUING_BILLING_BATCHES) === 'true') || false
+}
+
+module.exports = config

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

@@ -16,10 +16,12 @@ const DatabaseHelper = require('../../support/helpers/database.helper.js')
 
 // Things we need to stub
 const ChargingModuleGenerateService = require('../../../app/services/charging-module/generate-bill-run.service.js')
+const FeatureFlagsConfig = require('../../../config/feature-flags.config.js')
 const FetchChargeVersionsService = require('../../../app/services/supplementary-billing/fetch-charge-versions.service.js')
 const HandleErroredBillingBatchService = require('../../../app/services/supplementary-billing/handle-errored-billing-batch.service.js')
 const LegacyRequestLib = require('../../../app/lib/legacy-request.lib.js')
 const ProcessBillingPeriodService = require('../../../app/services/supplementary-billing/process-billing-period.service.js')
+const ReissueInvoicesService = require('../../../app/services/supplementary-billing/reissue-invoices.service.js')
 const UnflagUnbilledLicencesService = require('../../../app/services/supplementary-billing/unflag-unbilled-licences.service.js')
 
 // Thing under test
@@ -51,6 +53,9 @@ describe('Process billing batch service', () => {
     // test we recreate the condition by setting it directly with our own stub
     notifierStub = { omg: Sinon.stub(), omfg: Sinon.stub() }
     global.GlobalNotifier = notifierStub
+
+    // We set the `enableReissuingBillingBatches` feature flag to `true` to ensure that we always perform reissuing
+    Sinon.replace(FeatureFlagsConfig, 'enableReissuingBillingBatches', true)
   })
 
   afterEach(() => {
@@ -69,18 +74,39 @@ describe('Process billing batch service', () => {
         Sinon.stub(ProcessBillingPeriodService, 'go').resolves(false)
       })
 
-      it('sets the Billing Batch status to empty', async () => {
-        await ProcessBillingBatchService.go(billingBatch, billingPeriods)
+      describe('and there are no invoices to reissue', () => {
+        beforeEach(() => {
+          Sinon.stub(ReissueInvoicesService, 'go').resolves(false)
+        })
 
-        const result = await BillingBatchModel.query().findById(billingBatch.billingBatchId)
+        it('sets the Billing Batch status to empty', async () => {
+          await ProcessBillingBatchService.go(billingBatch, billingPeriods)
 
-        expect(result.status).to.equal('empty')
+          const result = await BillingBatchModel.query().findById(billingBatch.billingBatchId)
+
+          expect(result.status).to.equal('empty')
+        })
+      })
+
+      describe('and there are invoices to reissue', () => {
+        beforeEach(() => {
+          Sinon.stub(ReissueInvoicesService, 'go').resolves(true)
+        })
+
+        it('sets the Billing Batch status to processing', async () => {
+          await ProcessBillingBatchService.go(billingBatch, billingPeriods)
+
+          const result = await BillingBatchModel.query().findById(billingBatch.billingBatchId)
+
+          expect(result.status).to.equal('processing')
+        })
       })
     })
 
     describe('and some charge versions are billed', () => {
       beforeEach(() => {
         Sinon.stub(ProcessBillingPeriodService, 'go').resolves(true)
+        Sinon.stub(ReissueInvoicesService, 'go').resolves(true)
       })
 
       it('sets the Billing Batch status to processing', async () => {
@@ -102,22 +128,26 @@ describe('Process billing batch service', () => {
 
         expect(legacyRequestLibStub.called).to.be.true()
       })
-    })
 
-    it('logs the time taken to process the billing batch', async () => {
-      await ProcessBillingBatchService.go(billingBatch, billingPeriods)
+      it('it logs the time taken', async () => {
+        await ProcessBillingBatchService.go(billingBatch, billingPeriods)
 
-      const args = notifierStub.omg.firstCall.args
+        const args = notifierStub.omg.firstCall.args
 
-      expect(args[0]).to.equal('Process billing batch complete')
-      expect(args[1].timeTakenMs).to.exist()
-      expect(args[1].billingBatchId).to.equal(billingBatch.billingBatchId)
+        expect(args[0]).to.equal('Process billing batch complete')
+        expect(args[1].timeTakenMs).to.exist()
+        expect(args[1].billingBatchId).to.equal(billingBatch.billingBatchId)
+      })
     })
   })
 
   describe('when the service errors', () => {
     let thrownError
 
+    beforeEach(() => {
+      Sinon.stub(ReissueInvoicesService, 'go')
+    })
+
     describe('because fetching the charge versions fails', () => {
       beforeEach(() => {
         thrownError = new Error('ERROR')