Clear Include In SROC supp. billing flag on send (#2080)

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

We have an issue that the current `include_in_supplementary_billing` was added at a time there was only 1 charge scheme. A licence gets flagged irrespective of whether the change relates to PRESROC or SROC.

Where that has impacted us is when sending a billing batch. When it gets sent it clears the flag for _all_ licences included. But if we cancel one, for example, the PRESROC bill run, and send the SROC one we lose which licences still need to be processed on the PRESROC one.

Our solution was to add a new `include_in_sroc_supplementary_billing` flag to the `licences` table in [Add SROC include in supplementary billing flag](#2077). That also covered setting the new flag when a new charge version is approved. We now need to handle clearing the flag when the SROC bill run is 'sent'.

src/lib/connectors/repos/licences.js

@@ -116,6 +116,24 @@ const updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = (regionId,
     .raw(queries.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate, params)
 }
 
+/**
+ * For all licences where the created date is less than the batch created date update the
+ * include_in_sroc_supplementary_billing value to a new value where the current
+ * value is equal to the 'from' parameter value.
+ *
+ * @param {String} regionId
+ * @param {Date} batchCreatedDate
+ * @param {String} from The status value to move from (enum water.include_in_supplementary_billing)
+ * @param {String} to The status value to move to (enum water.include_in_supplementary_billing)
+ */
+const updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = (regionId, batchCreatedDate, from, to) => {
+  const params = { batchCreatedDate: batchCreatedDate.toISOString(), from, to, regionId }
+
+  return bookshelf
+    .knex
+    .raw(queries.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate, params)
+}
+
 /**
  * Finds licences which have a 'current' charge version linked to the specified
  * invoice account ID
@@ -137,3 +155,4 @@ exports.update = update
 exports.updateIncludeLicenceInSupplementaryBilling = updateIncludeLicenceInSupplementaryBilling
 exports.updateIncludeInSupplementaryBillingStatusForBatch = updateIncludeInSupplementaryBillingStatusForBatch
 exports.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSupplementaryBillingStatusForBatchCreatedDate
+exports.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate

src/lib/connectors/repos/queries/licences.js

@@ -30,6 +30,26 @@ const updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = `
     and l.include_in_supplementary_billing = :from
 `
 
+/**
+ * Updates the `include_in_sroc_supplementary_billing` flag for licences marked for SROC supplementary billing
+ *
+ * When a supplementary bill run gets approved (sent in the Charging Module) the legacy service updates the flag
+ * on all licences that were marked for processing. It needs to do it by date because you can have licences flagged
+ * that don't end up in the bill run, and with no other mechanism for knowing they were looked at this is what you
+ * are left with.
+ *
+ * We've only seen this and its partner version above used when setting the field from false to true. But we've kept
+ * the current pattern of supporting going the over way as we can't be 100% sure!
+ */
+const updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = `
+  update water.licences l
+  set include_in_sroc_supplementary_billing = :to
+  where l.date_updated <= :batchCreatedDate
+    and l.region_id = :regionId
+    and l.licence_id not in (select licence_id from water.charge_version_workflows where date_deleted is null)
+    and l.include_in_supplementary_billing = :from
+`
+
 /**
  * Updates the include_in_supplementary_billing value in the
  * water licences table from a value to a value for a given batch
@@ -42,19 +62,19 @@ const updateIncludeInSupplementaryBillingStatusForBatch = `
       join water.billing_batch_charge_version_years y
         on b.billing_batch_id = y.billing_batch_id
       join water.charge_versions cv
-        on y.charge_version_id = cv.charge_version_id      
+        on y.charge_version_id = cv.charge_version_id
   where l.licence_ref = cv.licence_ref
   and b.billing_batch_id = :batchId
   and b.batch_type = 'supplementary'
   and l.include_in_supplementary_billing = :from
   and l.licence_id in (
     select il.licence_id
-    from 
-      water.billing_invoice_licences il 
+    from
+      water.billing_invoice_licences il
         join water.billing_invoices i
-          on il.billing_invoice_id = i.billing_invoice_id 
+          on il.billing_invoice_id = i.billing_invoice_id
     where i.billing_batch_id = :batchId
-    ); 
+    );
 `
 
 const getLicencesByInvoiceAccount = `
@@ -65,6 +85,7 @@ and cv.status='current'
 `
 
 exports.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSupplementaryBillingStatusForBatchCreatedDate
+exports.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate
 exports.updateIncludeInSupplementaryBillingStatusForBatch = updateIncludeInSupplementaryBillingStatusForBatch
 exports.findByBatchIdForTwoPartTariffReview = findByBatchIdForTwoPartTariffReview
 exports.getLicencesByInvoiceAccount = getLicencesByInvoiceAccount

src/lib/services/licences.js

@@ -90,6 +90,13 @@ const updateIncludeInSupplementaryBillingStatusForSentBatch = async batchId => {
  * Sets the water.licences.include_in_supplementary_billing value
  * from yes to no based on the region and batch created date
  *
+ * NOTE: Logically, if you know the batch you would update the flag for those licences linked to it. The problem is a
+ * licence might be flagged for inclusion but end up not being included. The billing process might calculate the
+ * billable days as 0, or the debit line gets cancelled out by a credit in a supplementary bill run.
+ *
+ * Least, that is the issue we think the previous team were trying to tackle with this way of resetting the flags
+ * when a bill run gets approved (only place this is called).
+ *
  * @param {String} regionId
  * @param {Date} dateCreated
  */
@@ -100,6 +107,13 @@ const updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = async (regi
   INCLUDE_IN_SUPPLEMENTARY_BILLING.no
 )
 
+const updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = async (regionId, dateCreated) => repos.licences.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate(
+  regionId,
+  dateCreated,
+  true,
+  false
+)
+
 /**
  * Fetches the invoice accounts associated with a licence using the licence ref and date as input.
  * @todo move to invoice accounts service
@@ -226,6 +240,7 @@ exports.getLicenceAccountsByRefAndDate = getLicenceAccountsByRefAndDate
 exports.updateIncludeInSupplementaryBillingStatus = updateIncludeInSupplementaryBillingStatus
 exports.updateIncludeInSupplementaryBillingStatusForSentBatch = updateIncludeInSupplementaryBillingStatusForSentBatch
 exports.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSupplementaryBillingStatusForBatchCreatedDate
+exports.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate = updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate
 exports.flagForSupplementaryBilling = flagForSupplementaryBilling
 exports.getLicenceInvoices = getLicenceInvoices
 exports.getLicencesByInvoiceAccountId = getLicencesByInvoiceAccountId

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

@@ -192,12 +192,22 @@ const approveBatch = async (batch, internalCallingUser) => {
     await saveEvent('billing-batch:approve', 'sent', internalCallingUser, batch)
     await invoiceService.resetIsFlaggedForRebilling(batch.id)
 
-    // if it is a supplementary batch mark all the
-    // old transactions in previous batches that was credited back in new invoices sent in this batch
-    // for the relevant region.
     if (batch.type === BATCH_TYPE.supplementary) {
+      // if it is a supplementary batch mark all the old transactions in previous batches that was credited back in new
+      // invoices sent in this batch for the relevant region.
       await transactionsService.updateIsCredited(batch.region.id)
-      await licencesService.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate(batch.region.id, batch.dateCreated)
+
+      if (batch.scheme === 'alcs') {
+        await licencesService.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate(
+          batch.region.id,
+          batch.dateCreated
+        )
+      } else {
+        await licencesService.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate(
+          batch.region.id,
+          batch.dateCreated
+        )
+      }
     }
 
     return batch

test/lib/connectors/repos/licences.test.js

@@ -232,6 +232,37 @@ experiment('lib/connectors/repos/licences.js', () => {
     })
   })
 
+  experiment('.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate', () => {
+    let batchCreatedDate
+    let regionId
+
+    beforeEach(async () => {
+      batchCreatedDate = moment()
+      regionId = uuid()
+      await licencesRepo.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate(
+        regionId,
+        batchCreatedDate,
+        'from-value',
+        'to-value'
+      )
+    })
+
+    test('uses the expected SQL query', async () => {
+      const [query] = bookshelf.knex.raw.lastCall.args
+      expect(query).to.equal(licenceQueries.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate)
+    })
+
+    test('passes the expected params to the query', async () => {
+      const [, params] = bookshelf.knex.raw.lastCall.args
+      expect(params).to.equal({
+        regionId,
+        batchCreatedDate: batchCreatedDate.toISOString(),
+        from: 'from-value',
+        to: 'to-value'
+      })
+    })
+  })
+
   experiment('.findByBatchIdForTwoPartTariffReview', () => {
     beforeEach(async () => {
       await licencesRepo.findByBatchIdForTwoPartTariffReview('batch-id')

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

@@ -125,6 +125,7 @@ experiment('modules/billing/services/batch-service', () => {
     sandbox.stub(licencesService, 'updateIncludeInSupplementaryBillingStatus').resolves()
     sandbox.stub(licencesService, 'updateIncludeInSupplementaryBillingStatusForSentBatch').resolves()
     sandbox.stub(licencesService, 'updateIncludeInSupplementaryBillingStatusForBatchCreatedDate').resolves()
+    sandbox.stub(licencesService, 'updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate').resolves()
     sandbox.stub(licencesService, 'flagForSupplementaryBilling').resolves()
 
     sandbox.stub(chargeModuleBillRunConnector, 'create').resolves()
@@ -484,7 +485,8 @@ experiment('modules/billing/services/batch-service', () => {
         externalId: uuid(),
         dateCreated: '2020-01-09T16:23:24.753Z',
         type: 'supplementary',
-        region: { id: 'test-regioin-id' }
+        region: { id: 'test-regioin-id' },
+        scheme: 'alcs'
       }
 
       internalCallingUser = {
@@ -494,21 +496,23 @@ experiment('modules/billing/services/batch-service', () => {
     })
 
     experiment('when the approval succeeds', () => {
-      beforeEach(async () => {
+      test('approves the batch at the charge module', async () => {
         await batchService.approveBatch(batch, internalCallingUser)
-      })
 
-      test('approves the batch at the charge module', async () => {
         const [externalId] = chargeModuleBillRunConnector.approve.lastCall.args
         expect(externalId).to.equal(batch.externalId)
       })
 
       test('sends the batch at the charge module', async () => {
+        await batchService.approveBatch(batch, internalCallingUser)
+
         const [externalId] = chargeModuleBillRunConnector.send.lastCall.args
         expect(externalId).to.equal(batch.externalId)
       })
 
       test('creates an event showing the calling user successfully approved the batch', async () => {
+        await batchService.approveBatch(batch, internalCallingUser)
+
         const [savedEvent] = eventService.create.lastCall.args
         expect(savedEvent.issuer).to.equal(internalCallingUser.email)
         expect(savedEvent.type).to.equal('billing-batch:approve')
@@ -517,17 +521,37 @@ experiment('modules/billing/services/batch-service', () => {
         expect(savedEvent.metadata.batch).to.equal(batch)
       })
 
-      test('updates the include in supplementary billing status for the licences with an updated date <= batch created date', async () => {
-        const [regionId, batchCreatedDate] = licencesService.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate.lastCall.args
-        expect(regionId).to.equal(batch.region.id)
-        expect(batchCreatedDate).to.equal(batch.dateCreated)
-      })
-
       test('resets the invoice rebilling flags', async () => {
+        await batchService.approveBatch(batch, internalCallingUser)
+
         expect(invoiceService.resetIsFlaggedForRebilling.calledWith(
           batch.id
         )).to.be.true()
       })
+
+      experiment('when the billing batch is for SROC', () => {
+        beforeEach(() => {
+          batch.scheme = 'sroc'
+        })
+
+        test('updates the include in SROC supplementary billing status for the licences with an updated date <= batch created date', async () => {
+          await batchService.approveBatch(batch, internalCallingUser)
+
+          const [regionId, batchCreatedDate] = licencesService.updateIncludeInSrocSupplementaryBillingStatusForBatchCreatedDate.lastCall.args
+          expect(regionId).to.equal(batch.region.id)
+          expect(batchCreatedDate).to.equal(batch.dateCreated)
+        })
+      })
+
+      experiment('when the billing batch is for PRESROC', () => {
+        test('updates the include in supplementary billing status for the licences with an updated date <= batch created date', async () => {
+          await batchService.approveBatch(batch, internalCallingUser)
+
+          const [regionId, batchCreatedDate] = licencesService.updateIncludeInSupplementaryBillingStatusForBatchCreatedDate.lastCall.args
+          expect(regionId).to.equal(batch.region.id)
+          expect(batchCreatedDate).to.equal(batch.dateCreated)
+        })
+      })
     })
 
     experiment('when the batch does not approve', () => {