Fix broken send bill run functionality (#2446)

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

We have been working on replacing the legacy SROC annual billing engine with one in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system). That engine is based on the one we built for supplementary billing, also in **water-abstraction-system**.

In a previous change, we updated this project to [Stop marking bill runs as ready when they are not](#2442).

The refresh job was marking the bill run as **ready** and _then_ kicking off the process to refresh WRLS with data from the Charging Module API. That process entails making a request for every bill in the bill run to the CHA and then updating our local invoice and transactions with the result.

You can't **SEND** a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as **READY**. Users familiar with the service may know they need to leave it a while for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process.

Ideally, they should wait for this process to complete before starting a new bill run. But there is no way to know that. So, the next bill run will take longer to complete and you increase the risk of an error.

Anyway, that's why we made the change. But when we did we had no way of knowing when you send a bill run this legacy service _calls the refresh invoices job again_!!

When you confirm and send a bill run it tells the [charging-module-api (CHA)](https://github.com/DEFRA/sroc-charging-module-api) to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. That's the only new piece of information we need to obtain and you can get it from a single request to `GET`` the bill run summary from the CHA.

So, the fact we are reusing the refresh invoices job again is quite simply 🦇💩!

Anyway, we're going to stop this insanity once and for all in [Migrate legacy send bill run functionality](DEFRA/water-abstraction-system#771).

But till then we need to get it working again in this project. We broke it by having the job set the bill run status to `ready` at the end of the process. Before our change when the refresh job was kicked off, it would immediately set the status to `ready` or `sent` then get on with the process.

Our change means the bill run is getting marked as `sent`, then reset back to `ready` after the update is complete.

This ~hack~ change ensures a sent bill run stays sent!

src/lib/constants.js

@@ -3,5 +3,6 @@ exports.jobNames = {
   findUpdatedInvoiceAccounts: 'billing.find-update-invoice-accounts',
   updateInvoice: 'billing.update-invoice',
   updateInvoices: 'billing.update-invoices',
+  updateInvoiceReferences: 'billing.update-invoice-references',
   updateWithCMSummary: 'billing.update-with-cm-summary'
 }

src/lib/queue-manager/job-registration-service.js

@@ -16,6 +16,7 @@ const billingRefreshTotals = require('../../modules/billing/jobs/refresh-totals'
 const billingTwoPartTariffMatching = require('../../modules/billing/jobs/two-part-tariff-matching')
 const billingUpdateCustomerAccount = require('../../modules/billing/jobs/update-customer')
 const billingUpdateInvoices = require('../../modules/billing/jobs/update-invoices')
+const billingUpdateInvoiceReferences = require('../../modules/billing/jobs/update-invoice-references.js')
 const chargeInformationUploadStart = require('../../modules/charge-versions-upload/jobs/update-charge-information-start')
 const chargeInformationUploadToJson = require('../../modules/charge-versions-upload/jobs/update-charge-information-to-json')
 const gaugingStationsCopyLicenceGaugingStationsFromDigitise = require('../../modules/gauging-stations/jobs/sync-licence-gauging-stations-from-digitise')
@@ -88,6 +89,7 @@ class JobRegistrationService {
       billingTwoPartTariffMatching,
       billingUpdateCustomerAccount,
       billingUpdateInvoices,
+      billingUpdateInvoiceReferences,
       chargeInformationUploadStart,
       chargeInformationUploadToJson,
       gaugingStationsCopyLicenceGaugingStationsFromDigitise,

src/modules/billing/jobs/refresh-totals.js

@@ -23,12 +23,7 @@ const createMessage = batchId => ([
     batchId
   },
   {
-    jobId: `${JOB_NAME}.${batchId}.${uuid()}`,
-    attempts: 10,
-    backoff: {
-      type: 'exponential',
-      delay: 5000
-    }
+    jobId: `${JOB_NAME}.${batchId}.${uuid()}`
   }
 ])
 
@@ -64,8 +59,15 @@ const handler = async job => {
     if (!['generated', 'billed', 'billing_not_required'].includes(cmBatch.status)) {
       throw new StateError(`CM bill run summary not ready for batch ${batchId}`)
     }
-    // Update batch with totals/bill run ID from charge module
-    const isSuccess = await cmRefreshService.updateBatch(batchId)
+
+    // Default to update the invoices and transactions after generating a bill run
+    let nextJobName = 'billing.update-invoices'
+    if (cmBatch.status !== 'generated') {
+      // Else we need to update the invoices with their transactions references
+      nextJobName = 'billing.update-invoice-references'
+    }
+
+    const isSuccess = await cmRefreshService.updateBatch(batchId, nextJobName)
 
     if (!isSuccess) {
       throw new StateError(`CM bill run summary not ready for batch ${batchId}`)

src/modules/billing/jobs/update-invoice-references.js

@@ -0,0 +1,51 @@
+'use strict'
+
+const { v4: uuid } = require('uuid')
+
+// Utils
+const batchService = require('../services/batch-service.js')
+const { logger } = require('../../../logger.js')
+const UpdateInvoicesWorker = require('./lib/update-invoices-worker.js')
+
+// Constants
+const { BATCH_ERROR_CODE } = require('../../../lib/models/batch.js')
+const JOB_NAME = 'billing.update-invoice-references'
+
+const createMessage = data => ([
+  JOB_NAME,
+  data,
+  {
+    jobId: `${JOB_NAME}.${data.batch.id}.${uuid()}`
+  }
+])
+
+const handler = async job => {
+  const { id: batchId } = job.data.batch
+
+  try {
+    await UpdateInvoicesWorker.updateInvoices(job)
+    await batchService.setStatus(batchId, 'sent')
+  } catch (error) {
+    await batchService.setErrorStatus(batchId, BATCH_ERROR_CODE.failedToGetChargeModuleBillRunSummary)
+  }
+}
+
+const onComplete = async (job) => {
+  logger.info(`onComplete: ${job.id}`)
+}
+
+const onFailed = async (job, err) => {
+  logger.error(`onFailed: ${job.id} - ${err.message}`, err.stack)
+}
+
+module.exports = {
+  jobName: JOB_NAME,
+  createMessage,
+  handler,
+  onFailed,
+  onComplete,
+  workerOptions: {
+    lockDuration: 3600000,
+    lockRenewTime: 3600000 / 2
+  }
+}

src/modules/billing/services/batch-service.js

@@ -481,7 +481,7 @@ const updateWithCMSummary = async (batchId, cmResponse) => {
   const cmCompletedStatuses = ['billed', 'billing_not_required']
 
   const batch = await newRepos.billingBatches.findOne(batchId)
-  const status = cmCompletedStatuses.includes(cmStatus) && batch.status !== 'cancel' ? Batch.BATCH_STATUS.sent : Batch.BATCH_STATUS.processing
+  const status = cmCompletedStatuses.includes(cmStatus) && batch.status !== 'cancel' ? Batch.BATCH_STATUS.sending : Batch.BATCH_STATUS.processing
 
   // Get transaction count in local DB
   // if 0, the batch will be set to "empty" status

src/modules/billing/services/cm-refresh-service.js

@@ -7,7 +7,6 @@
 const errors = require('../../../lib/errors')
 
 const queueManager = require('../../../lib/queue-manager')
-const { jobNames } = require('../../../lib/constants')
 
 const chargeModuleBillRunConnector = require('../../../lib/connectors/charge-module/bill-runs')
 
@@ -16,13 +15,7 @@ const batchService = require('./batch-service')
 
 const isCMGeneratingSummary = cmResponse => ['pending', 'sending'].includes(cmResponse.billRun.status)
 
-/**
- * Updates the batch with the given batch ID
- * with data retrieved from the CM
- *
- * @param {String} batchId
- */
-const updateBatch = async batchId => {
+const updateBatch = async (batchId, nextJobName) => {
   // Fetch WRLS batch
   const batch = await batchService.getBatchById(batchId)
   if (!batch) {
@@ -36,12 +29,7 @@ const updateBatch = async batchId => {
     return false
   }
 
-  // Update invoices in batch
-  // It is important to update the invoices first so that
-  // for a batch containing only re-billing, there are >0 transactions
-  // in the batch before calculating the new batch status
-
-  queueManager.getQueueManager().add(jobNames.updateInvoices, { batch, cmResponse })
+  queueManager.getQueueManager().add(nextJobName, { batch, cmResponse })
 
   await batchService.updateWithCMSummary(batch.id, cmResponse)
 

test/modules/billing/jobs/refresh-totals.test.js

@@ -74,11 +74,6 @@ experiment('modules/billing/jobs/refresh-totals', () => {
       expect(data).to.equal({ batchId: BATCH_ID })
 
       expect(options.jobId).to.startWith(`billing.refresh-totals.${BATCH_ID}.`)
-      expect(options.attempts).to.equal(10)
-      expect(options.backoff).to.equal({
-        type: 'exponential',
-        delay: 5000
-      })
     })
   })
 

test/modules/billing/services/batch-service.test.js

@@ -869,9 +869,9 @@ experiment('modules/billing/services/batch-service', () => {
         await batchService.updateWithCMSummary(BATCH_ID, cmResponse)
       })
 
-      test('the batch is updated correctly with "sent" status', async () => {
+      test('the batch is updated correctly with "sending" status', async () => {
         expect(newRepos.billingBatches.update.calledWith(BATCH_ID, {
-          status: Batch.BATCH_STATUS.sent,
+          status: Batch.BATCH_STATUS.sending,
           invoiceCount,
           creditNoteCount,
           invoiceValue,

test/modules/billing/services/cm-refresh-service.test.js

@@ -226,7 +226,7 @@ experiment('modules/billing/services/cm-refresh-service', () => {
       })
 
       test('a NotFoundError is thrown', async () => {
-        const func = () => cmRefreshService.updateBatch(batchId)
+        const func = () => cmRefreshService.updateBatch(batchId, 'billing.update-invoices')
         const err = await expect(func()).to.reject()
         expect(err.name).to.equal('NotFoundError')
       })
@@ -237,7 +237,7 @@ experiment('modules/billing/services/cm-refresh-service', () => {
         batch = createBatch()
         batchService.getBatchById.resolves(batch)
         cmBillRunsConnector.get.resolves(cmResponses.batchSummary.generating)
-        result = await cmRefreshService.updateBatch(batchId)
+        result = await cmRefreshService.updateBatch(batchId, 'billing.update-invoices')
       })
 
       test('the correct batch is retrieved from the batch service', async () => {
@@ -274,7 +274,7 @@ experiment('modules/billing/services/cm-refresh-service', () => {
           invoiceService.getInvoicesForBatch.resolves(invoices)
 
           cmBillRunsConnector.get.resolves(cmResponses.batchSummary.ready)
-          result = await cmRefreshService.updateBatch(batchId)
+          result = await cmRefreshService.updateBatch(batchId, 'billing.update-invoices')
         })
 
         test('the correct batch is retrieved from the batch service', async () => {
@@ -298,7 +298,7 @@ experiment('modules/billing/services/cm-refresh-service', () => {
           batchSummary.billRun.invoices[0].rebilledType = 'C'
           batchSummary.billRun.invoices[1].rebilledType = 'R'
           cmBillRunsConnector.get.resolves(batchSummary)
-          result = await cmRefreshService.updateBatch(batchId)
+          result = await cmRefreshService.updateBatch(batchId, 'billing.update-invoices')
         })
 
         test('the correct batch is retrieved from the batch service', async () => {