Silo rebilling from supplementary (#854)

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

When we initially delivered SROC supplementary billing we were required to continue to support handling reissuing bills.

But the feature was put behind a feature flag so we could ship supplementary and then worry about reissuing.

At the time we pointed out that in the life of WRLS, only 8 bills had ever been re-issued. Yet it was one of the most complex processes to support.

It turns out the billing & data team had also come to the same conclusion. There are things they have to be aware of and features in the UI they need to enable reissuing to happen. Alternatively, they can just call SSCL and request they reissue the bill from SOP.

Because of this, they've held off asking us to enable the feature.

So, it has been more than 6 months and reissuing still hasn't been enabled in production. In that time we've made many other changes where we've disregarded reissuing to avoid needless complexity and our belief a decision will be made never to enable it.

Should that assumption be wrong we'd need to make a suite of changes to support it, for example, in how we display things in the billing views. We are making a series of updates to the supplementary billing engine to implement the improvements we implemented in SROC annual billing. That refactoring is a prime opportunity to silo the reissuing from the engine and the complexity it adds.

app/services/bill-runs/reissue/README.md

@@ -0,0 +1,33 @@
+# Reissue
+
+> Sometimes folks will talk about rebilling. Technically there is a difference but from a system point of view they are the same thing.
+
+## Context
+
+WRLS supports going back and changing the charging information for customers up to 6 years back from the current date. When this happens we have to recalculate bills, working out what debits and credits cancel each other out and what remains to be owed or credited.
+
+Sometimes the change is purely a billing contact. For example, an invoice is unpaid but it turns out it is because the previous contact has moved on and the invoices need to go to a different contact and address. For accounting reasons we can't just send a new invoice and cancel the previous one. A credit has to be raised to the previous billing account and then a new invoice sent.
+
+For these specific cases it was requested that a rebilling feature (later to be called reissue) was implemented. This would allow a user to flag a bill in the service for reissue. Then, when a supplementary bill run was created the engine would first process these flagged bills. For each one a request is sent to the [Charging Module API to rebill the bill](https://defra.github.io/sroc-charging-module-api-docs/#/bill-run/RebillBillRunInvoice).
+
+The CHA would then automatically generate a new credit and debit bill. On our side along with setting flags and other info on the source bill, we import the data for the new bills into WRLS and include them in the bill run.
+
+When we first implemented the SROC supplementary billing engine this was seen as 'must have' feature.
+
+## Issues
+
+On the face of it it seems simple enough. But it complicates what is already a very complex process. Plus, it leads to a proliferation of logic throughout the service about whether a bill is the source of a reissue, or one of the results and if so to amend the data displayed. There are estimated to more than a thousand lines of code, not including unit tests, throughout the legacy code base supporting this one feature.
+
+All this to support, after we checked, 8 instances of reissuing a bill. Added to that, those were done early in the feature being shipped. No bill has been reissued for a number of years now.
+
+In fact, the whole feature was placed behind a feature flag to allow us to ship the rest of supplementary billing. We could then focus on the testing of reissuing bills later. Supplementary billing was being used in `production` from September 2023. In that time we've never enabled the flag nor completed the testing.
+
+Because of this as we have migrated other legacy views and functionality we have disregarded anything related to reissuing.
+
+## Current state
+
+The alternative to using the service is simply to call SSCL and request they reissue a bill. We've held off enabling the feature pending the business making a firm decision that this will be the process they follow in future.
+
+During a refactor of the supplementary billing engine to incorporate improvements we learnt building the annual billing engine we decided to silo off the reissue logic. Doing so brings the supplementary process even more inline with annual which should help maintaining them in future.
+
+We're not quite brave enough to delete the services we built but we're 99% certain that one day we'll be given the green light to do so. Till then they can live here.

app/services/bill-runs/supplementary/reissue-bill.service.js → app/services/bill-runs/reissue/reissue-bill.service.js

@@ -11,8 +11,8 @@ const ChargingModuleReissueBillRequest = require('../../../requests/charging-mod
 const ChargingModuleViewBillRequest = require('../../../requests/charging-module/view-bill.request.js')
 const ChargingModuleViewBillRunStatusRequest = require('../../../requests/charging-module/view-bill-run-status.request.js')
 const ExpandedError = require('../../../errors/expanded.error.js')
-const GenerateBillLicenceService = require('./generate-bill-licence.service.js')
-const GenerateBillService = require('./generate-bill.service.js')
+const GenerateBillLicenceService = require('../supplementary/generate-bill-licence.service.js')
+const GenerateBillService = require('../supplementary/generate-bill.service.js')
 
 /**
  * Handles the reissuing of a single bill

app/services/bill-runs/supplementary/process-bill-run.service.js

@@ -13,11 +13,8 @@ const { calculateAndLogTimeTaken, currentTimeInNanoseconds } = require('../../..
 const HandleErroredBillRunService = require('../handle-errored-bill-run.service.js')
 const LegacyRefreshBillRunRequest = require('../../../requests/legacy/refresh-bill-run.request.js')
 const ProcessBillingPeriodService = require('./process-billing-period.service.js')
-const ReissueBillsService = require('./reissue-bills.service.js')
 const UnflagUnbilledLicencesService = require('./unflag-unbilled-licences.service.js')
 
-const FeatureFlagsConfig = require('../../../../config/feature-flags.config.js')
-
 /**
  * Process a given bill run for the given billing periods. In this case, "process" means that we create the
  * required bills and transactions for it in both this service and the Charging Module.
@@ -33,9 +30,7 @@ async function go (billRun, billingPeriods) {
 
     await _updateStatus(billRunId, 'processing')
 
-    const resultOfReissuing = await _reissueBills(billRun)
-
-    await _processBillingPeriods(billingPeriods, billRun, resultOfReissuing)
+    await _processBillingPeriods(billingPeriods, billRun)
 
     calculateAndLogTimeTaken(startTime, 'Process bill run complete', { billRunId, type: 'supplementary' })
   } catch (error) {
@@ -44,13 +39,12 @@ async function go (billRun, billingPeriods) {
   }
 }
 
-async function _processBillingPeriods (billingPeriods, billRun, resultOfReissuing) {
+async function _processBillingPeriods (billingPeriods, billRun) {
   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]
+  // and returning `true`).
+  const results = []
 
   for (const billingPeriod of billingPeriods) {
     const { chargeVersions, licenceIdsForPeriod } = await _fetchChargeVersions(billRun, billingPeriod)
@@ -63,21 +57,6 @@ async function _processBillingPeriods (billingPeriods, billRun, resultOfReissuin
   await _finaliseBillRun(billRun, accumulatedLicenceIds, results)
 }
 
-/**
- * Call `ReissueBillsService` and log the time taken. We return `true` if any bills have been reissued (which will
- * have resulted in db changes), otherwise we return `false` (to indicate there have been no db changes)
- */
-async function _reissueBills (billRun) {
-  // 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 ReissueBillsService.go(billRun)
-
-  return result
-}
-
 async function _fetchChargeVersions (billRun, billingPeriod) {
   try {
     const chargeVersionData = await FetchChargeVersionsService.go(billRun.regionId, billingPeriod)

test/services/bill-runs/supplementary/fetch-bills-to-be-reissued.service.test.js → test/services/bill-runs/reissue/fetch-bills-to-be-reissued.service.test.js

@@ -17,7 +17,7 @@ const DatabaseSupport = require('../../../support/database.js')
 const TransactionHelper = require('../../../support/helpers/transaction.helper.js')
 
 // Thing under test
-const FetchBillsToBeReissuedService = require('../../../../app/services/bill-runs/supplementary/fetch-bills-to-be-reissued.service.js')
+const FetchBillsToBeReissuedService = require('../../../../app/services/bill-runs/reissue/fetch-bills-to-be-reissued.service.js')
 
 describe('Fetch Bills To Be Reissued service', () => {
   let billRun

test/services/bill-runs/supplementary/reissue-bill.service.test.js → test/services/bill-runs/reissue/reissue-bill.service.test.js

@@ -21,7 +21,7 @@ const ChargingModuleViewBillRequest = require('../../../../app/requests/charging
 const ChargingModuleViewBillRunStatusRequest = require('../../../../app/requests/charging-module/view-bill-run-status.request.js')
 
 // Thing under test
-const ReissueBillService = require('../../../../app/services/bill-runs/supplementary/reissue-bill.service.js')
+const ReissueBillService = require('../../../../app/services/bill-runs/reissue/reissue-bill.service.js')
 
 const ORIGINAL_BILLING_BATCH_EXTERNAL_ID = generateUUID()
 const INVOICE_EXTERNAL_ID = generateUUID()

test/services/bill-runs/supplementary/reissue-bills.service.test.js → test/services/bill-runs/reissue/reissue-bills.service.test.js

@@ -19,11 +19,11 @@ const TransactionHelper = require('../../../support/helpers/transaction.helper.j
 const TransactionModel = require('../../../../app/models/transaction.model.js')
 
 // Things we need to stub
-const FetchBillsToBeReissuedService = require('../../../../app/services/bill-runs/supplementary/fetch-bills-to-be-reissued.service.js')
-const ReissueBillService = require('../../../../app/services/bill-runs/supplementary/reissue-bill.service.js')
+const FetchBillsToBeReissuedService = require('../../../../app/services/bill-runs/reissue/fetch-bills-to-be-reissued.service.js')
+const ReissueBillService = require('../../../../app/services/bill-runs/reissue/reissue-bill.service.js')
 
 // Thing under test
-const ReissueBillsService = require('../../../../app/services/bill-runs/supplementary/reissue-bills.service.js')
+const ReissueBillsService = require('../../../../app/services/bill-runs/reissue/reissue-bills.service.js')
 
 describe('Reissue Bills service', () => {
   const reissueBillRun = { regionId: generateUUID() }

test/services/bill-runs/supplementary/process-bill-run.service.test.js

@@ -21,7 +21,6 @@ const FetchChargeVersionsService = require('../../../../app/services/bill-runs/s
 const HandleErroredBillRunService = require('../../../../app/services/bill-runs/handle-errored-bill-run.service.js')
 const LegacyRefreshBillRunRequest = require('../../../../app/requests/legacy/refresh-bill-run.request.js')
 const ProcessBillingPeriodService = require('../../../../app/services/bill-runs/supplementary/process-billing-period.service.js')
-const ReissueBillsService = require('../../../../app/services/bill-runs/supplementary/reissue-bills.service.js')
 const UnflagUnbilledLicencesService = require('../../../../app/services/bill-runs/supplementary/unflag-unbilled-licences.service.js')
 
 // Thing under test
@@ -74,39 +73,18 @@ describe('Supplementary Process Bill Run service', () => {
         Sinon.stub(ProcessBillingPeriodService, 'go').resolves(false)
       })
 
-      describe('and there are no invoices to reissue', () => {
-        beforeEach(() => {
-          Sinon.stub(ReissueBillsService, 'go').resolves(false)
-        })
-
-        it('sets the Bill Run status to empty', async () => {
-          await SupplementaryProcessBillRunService.go(billRun, billingPeriods)
-
-          const result = await BillRunModel.query().findById(billRun.id)
-
-          expect(result.status).to.equal('empty')
-        })
-      })
-
-      describe('and there are invoices to reissue', () => {
-        beforeEach(() => {
-          Sinon.stub(ReissueBillsService, 'go').resolves(true)
-        })
-
-        it('sets the Bill Run status to processing', async () => {
-          await SupplementaryProcessBillRunService.go(billRun, billingPeriods)
+      it('sets the Bill Run status to empty', async () => {
+        await SupplementaryProcessBillRunService.go(billRun, billingPeriods)
 
-          const result = await BillRunModel.query().findById(billRun.id)
+        const result = await BillRunModel.query().findById(billRun.id)
 
-          expect(result.status).to.equal('processing')
-        })
+        expect(result.status).to.equal('empty')
       })
     })
 
     describe('and some charge versions are billed', () => {
       beforeEach(() => {
         Sinon.stub(ProcessBillingPeriodService, 'go').resolves(true)
-        Sinon.stub(ReissueBillsService, 'go').resolves(true)
       })
 
       it('sets the Bill Run status to processing', async () => {
@@ -144,10 +122,6 @@ describe('Supplementary Process Bill Run service', () => {
   describe('when the service errors', () => {
     let thrownError
 
-    beforeEach(() => {
-      Sinon.stub(ReissueBillsService, 'go')
-    })
-
     describe('because fetching the charge versions fails', () => {
       beforeEach(() => {
         thrownError = new Error('ERROR')