From 8598109fe0544f7cfb920f540506844ef6939760 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 08:51:15 +0000
Subject: [PATCH 01/11] Move SendTransactions to root with refactoring

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

> For context this came out of us working on re-implementing the SROC annual bill run using what we've learnt and components from our supplementary billing engine.

As part of looking at re-implementing the SROC annual billing engine in this project our spike (WATER-4348 ) confirmed we'd need to reuse some of the services currently sitting in `app/services/bill-runs/supplementary`.

We moved most of these in [Move shared billing services to bill-runs root](https://github.com/DEFRA/water-abstraction-system/pull/720) but left `app/services/bill-runs/supplementary/send-transactions.service.js` out even though we also need to reuse it.

When working on the spike we found there is some tidy up we can do in the service. We also spotted we were passing in `BillLicence` purely to set the `billLicenceId`. We now feel this should be done elsewhere.

So, we're doing those changes here as they are a little more involved than simply moving the service.

From 8b3be1f322d4701ca6f9b4ba7b4548a04999f420 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 15:46:47 +0000
Subject: [PATCH 02/11] Refactor where billLicenceId is applied

Previously, we passed the billLicence to `SendTransactionsService`, extracted the ID and applied it to the transaction along with the values returned from the Charging Module.

We don't rightly know why we chose to do this there (most likely the result of a hack we never refactored) but it feels out of place.

Better to have it applied to the transaction at the point we generate the transaction.
---
 .../generate-transactions.service.js          | 16 +++----
 .../process-billing-period.service.js         |  5 ++-
 .../generate-transactions.service.test.js     | 42 ++++++++++++++-----
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/app/services/bill-runs/generate-transactions.service.js b/app/services/bill-runs/generate-transactions.service.js
index 6a69006774..bc7b8fc712 100644
--- a/app/services/bill-runs/generate-transactions.service.js
+++ b/app/services/bill-runs/generate-transactions.service.js
@@ -24,6 +24,7 @@ const CalculateAuthorisedAndBillableDaysServiceService = require('./calculate-au
  * They will then be returned in an array for further processing before being persisted to the DB as
  * `billing_transactions`.
  *
+ * @param {String} billLicenceId The UUID of the bill licence the transaction will be linked to
  * @param {Object} chargeReference The charge reference the transaction generated from
  * @param {Object} billingPeriod A start and end date representing the billing period for the bill run
  * @param {Object} chargePeriod A start and end date representing the charge period for the charge version
@@ -32,7 +33,7 @@ const CalculateAuthorisedAndBillableDaysServiceService = require('./calculate-au
  *
  * @returns {Object[]} an array of 0, 1 or 2 transaction objects
  */
-function go (chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker) {
+function go (billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker) {
   const { authorisedDays, billableDays } = CalculateAuthorisedAndBillableDaysServiceService.go(
     chargePeriod,
     billingPeriod,
@@ -46,7 +47,7 @@ function go (chargeReference, billingPeriod, chargePeriod, newLicence, waterUnde
   }
 
   const standardTransaction = _standardTransaction(
-    generateUUID(),
+    billLicenceId,
     authorisedDays,
     billableDays,
     chargeReference,
@@ -58,7 +59,7 @@ function go (chargeReference, billingPeriod, chargePeriod, newLicence, waterUnde
   transactions.push(standardTransaction)
 
   if (!waterUndertaker) {
-    const compensationTransaction = _compensationTransaction(generateUUID(), standardTransaction)
+    const compensationTransaction = _compensationTransaction(standardTransaction)
     transactions.push(compensationTransaction)
   }
 
@@ -69,10 +70,10 @@ function go (chargeReference, billingPeriod, chargePeriod, newLicence, waterUnde
  * Generates a compensation transaction by taking a standard transaction and overwriting it with the supplied billing id
  * and the correct charge type and description for a compensation charge.
  */
-function _compensationTransaction (transactionId, standardTransaction) {
+function _compensationTransaction (standardTransaction) {
   return {
     ...standardTransaction,
-    id: transactionId,
+    id: generateUUID(),
     chargeType: 'compensation',
     description: 'Compensation charge: calculated from the charge reference, activity description and regional environmental improvement charge; excludes any supported source additional charge and two-part tariff charge agreement'
   }
@@ -102,7 +103,7 @@ function _generateElements (chargeReference) {
  * Generates a standard transaction based on the supplied data, along with some default fields (eg. status)
  */
 function _standardTransaction (
-  transactionId,
+  billLicenceId,
   authorisedDays,
   billableDays,
   chargeReference,
@@ -111,7 +112,8 @@ function _standardTransaction (
   waterUndertaker
 ) {
   return {
-    id: transactionId,
+    id: generateUUID(),
+    billLicenceId,
     authorisedDays,
     billableDays,
     newLicence,
diff --git a/app/services/bill-runs/supplementary/process-billing-period.service.js b/app/services/bill-runs/supplementary/process-billing-period.service.js
index 5221efc9b8..300f3b97ca 100644
--- a/app/services/bill-runs/supplementary/process-billing-period.service.js
+++ b/app/services/bill-runs/supplementary/process-billing-period.service.js
@@ -119,7 +119,7 @@ function _buildBillingDataWithTransactions (chargeVersions, preGeneratedData, bi
     // We only need to calculate the transactions for charge versions with a status of `current` (APPROVED).
     // We fetch the previous transactions for `superseded` (REPLACED) charge versions later in the process
     if (chargeVersion.status === 'current') {
-      const calculatedTransactions = _generateCalculatedTransactions(billingPeriod, chargeVersion)
+      const calculatedTransactions = _generateCalculatedTransactions(billLicenceId, billingPeriod, chargeVersion)
       acc[billLicenceId].calculatedTransactions.push(...calculatedTransactions)
     }
 
@@ -185,7 +185,7 @@ async function _cleanseTransactions (currentBillingData, billingPeriod) {
   return cleansedTransactions
 }
 
-function _generateCalculatedTransactions (billingPeriod, chargeVersion) {
+function _generateCalculatedTransactions (billLicenceId, billingPeriod, chargeVersion) {
   try {
     const chargePeriod = DetermineChargePeriodService.go(chargeVersion, billingPeriod)
 
@@ -199,6 +199,7 @@ function _generateCalculatedTransactions (billingPeriod, chargeVersion) {
     // We use flatMap as GenerateTransactionsService returns an array of transactions
     const transactions = chargeVersion.chargeReferences.flatMap((chargeReference) => {
       return GenerateTransactionsService.go(
+        billLicenceId,
         chargeReference,
         billingPeriod,
         chargePeriod,
diff --git a/test/services/bill-runs/generate-transactions.service.test.js b/test/services/bill-runs/generate-transactions.service.test.js
index 7e0d329047..72dd2e2edb 100644
--- a/test/services/bill-runs/generate-transactions.service.test.js
+++ b/test/services/bill-runs/generate-transactions.service.test.js
@@ -21,6 +21,7 @@ const CalculateAuthorisedAndBillableDaysService = require('../../../app/services
 const GenerateTransactionsService = require('../../../app/services/bill-runs/generate-transactions.service.js')
 
 describe('Generate Transactions service', () => {
+  const billLicenceId = '5e2afb53-ca92-4515-ad71-36a7cefbcebb'
   const reference = '4.4.5'
 
   let chargePeriod
@@ -63,6 +64,7 @@ describe('Generate Transactions service', () => {
       newLicence = false
 
       expectedStandardChargeResult = {
+        billLicenceId,
         chargeReferenceId: chargeReference.id,
         startDate: chargePeriod.startDate,
         endDate: chargePeriod.endDate,
@@ -103,7 +105,9 @@ describe('Generate Transactions service', () => {
       })
 
       it('returns an array of one transaction containing the expected data', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         // Should only return the 'standard' charge transaction line
         expect(results).to.have.length(1)
@@ -118,7 +122,9 @@ describe('Generate Transactions service', () => {
       })
 
       it('returns the charge element as JSON in the transaction line `purposes` property', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         const parsedElements = JSON.parse(results[0].purposes)
 
@@ -128,7 +134,9 @@ describe('Generate Transactions service', () => {
 
     describe('and is not a water undertaker', () => {
       it('returns an array of two transactions containing the expected data', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         // Should return both a 'standard' charge and 'compensation' charge transaction line
         expect(results).to.have.length(2)
@@ -143,7 +151,9 @@ describe('Generate Transactions service', () => {
       })
 
       it('returns a second compensation charge transaction', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         expect(results[1]).to.equal(
           {
@@ -157,7 +167,9 @@ describe('Generate Transactions service', () => {
       })
 
       it('returns the charge element as JSON in both transaction lines `purposes` property', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         const parsedStandardElements = JSON.parse(results[0].purposes)
         const parsedCompensationElements = JSON.parse(results[1].purposes)
@@ -176,7 +188,9 @@ describe('Generate Transactions service', () => {
       })
 
       it('returns `newLicence` as true in the results', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         expect(results[0].newLicence).to.be.true()
       })
@@ -184,7 +198,9 @@ describe('Generate Transactions service', () => {
 
     describe('and is not a new licence', () => {
       it('returns `newLicence` as false in the results', () => {
-        const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+        const results = GenerateTransactionsService.go(
+          billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+        )
 
         expect(results[0].newLicence).to.be.false()
       })
@@ -193,7 +209,9 @@ describe('Generate Transactions service', () => {
     describe('and a two-part tariff agreement (section 127)', () => {
       describe('has not applied', () => {
         it('returns the standard description', () => {
-          const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+          const results = GenerateTransactionsService.go(
+            billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+          )
 
           expect(results[0].description).to.equal(`Water abstraction charge: ${chargeReference.description}`)
         })
@@ -205,7 +223,9 @@ describe('Generate Transactions service', () => {
         })
 
         it('returns the two-part tariff prefixed description', () => {
-          const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+          const results = GenerateTransactionsService.go(
+            billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+          )
 
           expect(results[0].description).to.equal(`Two-part tariff basic water abstraction charge: ${chargeReference.description}`)
         })
@@ -224,7 +244,9 @@ describe('Generate Transactions service', () => {
     })
 
     it('returns an empty array', () => {
-      const results = GenerateTransactionsService.go(chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker)
+      const results = GenerateTransactionsService.go(
+        billLicenceId, chargeReference, billingPeriod, chargePeriod, newLicence, waterUndertaker
+      )
 
       expect(results).to.be.empty()
     })

From de8095ae6f6e88ac676b233171d2d4ddb6c85ce6 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 15:52:34 +0000
Subject: [PATCH 03/11] Remove billLicence from SendTransactionsService

Now we can start to refactor SendTransactionsService. The first step is to remove the need to set the billLicenceId.
---
 .../process-billing-period.service.js             |  1 -
 .../supplementary/send-transactions.service.js    | 15 +++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/app/services/bill-runs/supplementary/process-billing-period.service.js b/app/services/bill-runs/supplementary/process-billing-period.service.js
index 300f3b97ca..3e38ab6854 100644
--- a/app/services/bill-runs/supplementary/process-billing-period.service.js
+++ b/app/services/bill-runs/supplementary/process-billing-period.service.js
@@ -64,7 +64,6 @@ async function _buildDataToPersist (billingData, billingPeriod, billRunExternalI
       const transactions = await SendTransactionsService.go(
         currentBillingData.licence,
         currentBillingData.bill,
-        currentBillingData.billLicence,
         billRunExternalId,
         cleansedTransactions
       )
diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/supplementary/send-transactions.service.js
index 0185dc7950..6f5dbd3dc3 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/supplementary/send-transactions.service.js
@@ -11,20 +11,20 @@ const ChargingModuleCreateTransactionService = require('../../charging-module/cr
 const ChargingModuleCreateTransactionPresenter = require('../../../presenters/charging-module/create-transaction.presenter.js')
 
 /**
- * Sends the provided transactions to the Charging Module and returns an array of the sent transactions, each updated
- * with a status of `charge_created`; the external id returned by the Charging Module; and the appropriate bill licence
- * id
+ * Sends the provided transactions to the Charging Module and returns an array of the sent transactions
+ *
+ * Each sent transaction is updated with a status of `charge_created` and the external id returned by the
+ * Charging Module.
  *
  * @param {module:LicenceModel} licence The licence that each transaction is linked to
  * @param {module:BillModel} bill The bill each transaction is to be linked to
- * @param {module:BillLicenceModel} billLicence The bill licence each transaction is to be linked to
  * @param {string} billRunExternalId The Charging Module bill run id that the transactions are to be created on
  * @param {Object[]} transactions The transactions to be sent to the Charging Module
  * @param {Object} billingPeriod The billing period of the transactions
  *
  * @returns {Promise<Object[]>} Array of transactions which have been sent to the Charging Module
  */
-async function go (licence, bill, billLicence, billRunExternalId, transactions) {
+async function go (licence, bill, billRunExternalId, transactions) {
   try {
     const sentTransactions = []
 
@@ -36,7 +36,7 @@ async function go (licence, bill, billLicence, billRunExternalId, transactions)
         billRunExternalId
       )
 
-      _updateTransaction(transaction, chargingModuleResponse, billLicence)
+      _updateTransaction(transaction, chargingModuleResponse)
 
       sentTransactions.push(transaction)
     }
@@ -57,10 +57,9 @@ async function _sendTransactionToChargingModule (transaction, bill, licence, bil
   return ChargingModuleCreateTransactionService.go(billRunExternalId, chargingModuleRequest)
 }
 
-function _updateTransaction (transaction, chargingModuleResponse, billLicence) {
+function _updateTransaction (transaction, chargingModuleResponse) {
   transaction.status = 'charge_created'
   transaction.externalId = chargingModuleResponse.response.body.transaction.id
-  transaction.billLicenceId = billLicence.id
 }
 
 module.exports = {

From 027bfdc2eadc09d6a6ae4f67f66bf1c49507e8c6 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 15:54:20 +0000
Subject: [PATCH 04/11] Oops! Forgot to update the tests

---
 .../bill-runs/supplementary/send-transactions.service.test.js | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/test/services/bill-runs/supplementary/send-transactions.service.test.js b/test/services/bill-runs/supplementary/send-transactions.service.test.js
index eebc4856bd..7eabccd0a4 100644
--- a/test/services/bill-runs/supplementary/send-transactions.service.test.js
+++ b/test/services/bill-runs/supplementary/send-transactions.service.test.js
@@ -21,7 +21,6 @@ const SendTransactionsService = require('../../../../app/services/bill-runs/supp
 describe('Send Transactions service', () => {
   const billRunExternalId = '4f3710ca-75b1-4828-8fe9-f7c1edecbbf3'
   const bill = { accountNumber: 'ABC123' }
-  const billLicence = { id: '594fc25e-99c1-440a-8b88-b507ee17738a' }
   const billingPeriod = {
     startDate: new Date('2022-04-01'),
     endDate: new Date('2023-03-31')
@@ -90,7 +89,6 @@ describe('Send Transactions service', () => {
       const results = await SendTransactionsService.go(
         licence,
         bill,
-        billLicence,
         billRunExternalId,
         transactions,
         billingPeriod
@@ -99,7 +97,6 @@ describe('Send Transactions service', () => {
       expect(results).length(1)
       expect(results[0].status).to.equal('charge_created')
       expect(results[0].externalId).to.equal('7e752fa6-a19c-4779-b28c-6e536f028795')
-      expect(results[0].billLicenceId).to.equal(billLicence.id)
     })
   })
 
@@ -113,7 +110,6 @@ describe('Send Transactions service', () => {
         SendTransactionsService.go(
           licence,
           bill,
-          billLicence,
           billRunExternalId,
           transactions,
           billingPeriod

From 4daf78cc500282a44cedcd3ab849e1543947f708 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 15:55:30 +0000
Subject: [PATCH 05/11] Remove _updateTransaction()

Now we have removed one of the things it was doing it's not really adding any value any more. We can make these changes directly.
---
 .../bill-runs/supplementary/send-transactions.service.js  | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/supplementary/send-transactions.service.js
index 6f5dbd3dc3..c2e074f4e4 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/supplementary/send-transactions.service.js
@@ -36,7 +36,8 @@ async function go (licence, bill, billRunExternalId, transactions) {
         billRunExternalId
       )
 
-      _updateTransaction(transaction, chargingModuleResponse)
+      transaction.status = 'charge_created'
+      transaction.externalId = chargingModuleResponse.response.body.transaction.id
 
       sentTransactions.push(transaction)
     }
@@ -57,11 +58,6 @@ async function _sendTransactionToChargingModule (transaction, bill, licence, bil
   return ChargingModuleCreateTransactionService.go(billRunExternalId, chargingModuleRequest)
 }
 
-function _updateTransaction (transaction, chargingModuleResponse) {
-  transaction.status = 'charge_created'
-  transaction.externalId = chargingModuleResponse.response.body.transaction.id
-}
-
 module.exports = {
   go
 }

From fb968f3da6b09adf3cd1d4d4c9755091050cdc09 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 17:52:04 +0000
Subject: [PATCH 06/11] Expand unit tests

Part of the refactoring will be related to firing all the requests rather than waiting for each one to complete.

Therefore to be sure everything is still working as expected we expand the unit tests to deal with multiple transactions rather than just one.
---
 .../send-transactions.service.test.js         | 105 +++++++++++-------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/test/services/bill-runs/supplementary/send-transactions.service.test.js b/test/services/bill-runs/supplementary/send-transactions.service.test.js
index 7eabccd0a4..fefc93d6cf 100644
--- a/test/services/bill-runs/supplementary/send-transactions.service.test.js
+++ b/test/services/bill-runs/supplementary/send-transactions.service.test.js
@@ -31,44 +31,17 @@ describe('Send Transactions service', () => {
     regionalChargeArea: 'Yorkshire',
     region: { chargeRegionId: 'N' }
   }
-  const transaction = {
-    id: '9b092372-1a26-436a-bf1f-b5eb3f9aca44',
-    chargeReferenceId: '32058a19-4813-4ee7-808b-a0559deb8469',
-    startDate: new Date('2022-04-01'),
-    endDate: new Date('2022-10-31'),
-    source: 'non-tidal',
-    season: 'all year',
-    loss: 'low',
-    credit: false,
-    chargeType: 'standard',
-    authorisedQuantity: 6.82,
-    billableQuantity: 6.82,
-    authorisedDays: 365,
-    billableDays: 214,
-    status: 'candidate',
-    description: 'Water abstraction charge: Mineral washing',
-    volume: 6.82,
-    section126Factor: 1,
-    section127Agreement: false,
-    section130Agreement: false,
-    newLicence: false,
-    twoPartSecondPartCharge: false,
-    scheme: 'sroc',
-    aggregateFactor: 0.562114443,
-    adjustmentFactor: 1,
-    chargeCategoryCode: '4.4.5',
-    chargeCategoryDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model',
-    supportedSource: false,
-    supportedSourceName: null,
-    waterCompanyCharge: true,
-    winterOnly: false,
-    waterUndertaker: false
-  }
 
+  let chargingModuleCreateTransactionServiceStub
   let transactions
 
   beforeEach(() => {
-    transactions = [transaction]
+    chargingModuleCreateTransactionServiceStub = Sinon.stub(ChargingModuleCreateTransactionService, 'go')
+
+    transactions = [
+      _transaction('fca14c7e-c895-4991-9651-9a76f45b971d'),
+      _transaction('e98ad67f-5a26-43d7-bdba-39eba49ab62c')
+    ]
   })
 
   afterEach(() => {
@@ -77,11 +50,11 @@ describe('Send Transactions service', () => {
 
   describe('when calling the Charging Module API is successful', () => {
     beforeEach(async () => {
-      Sinon.stub(ChargingModuleCreateTransactionService, 'go').resolves({
-        succeeded: true,
-        response: {
-          body: { transaction: { id: '7e752fa6-a19c-4779-b28c-6e536f028795' } }
-        }
+      chargingModuleCreateTransactionServiceStub.onFirstCall().resolves({
+        ..._chargingModuleResponse('7e752fa6-a19c-4779-b28c-6e536f028795')
+      })
+      chargingModuleCreateTransactionServiceStub.onSecondCall().resolves({
+        ..._chargingModuleResponse('a2086da4-e3b6-4b83-afe1-0e2e5255efaf')
       })
     })
 
@@ -94,15 +67,20 @@ describe('Send Transactions service', () => {
         billingPeriod
       )
 
-      expect(results).length(1)
+      expect(results).length(2)
       expect(results[0].status).to.equal('charge_created')
       expect(results[0].externalId).to.equal('7e752fa6-a19c-4779-b28c-6e536f028795')
+      expect(results[1].status).to.equal('charge_created')
+      expect(results[1].externalId).to.equal('a2086da4-e3b6-4b83-afe1-0e2e5255efaf')
     })
   })
 
   describe('when calling the Charging Module API is unsuccessful', () => {
     beforeEach(async () => {
-      Sinon.stub(ChargingModuleCreateTransactionService, 'go').rejects()
+      chargingModuleCreateTransactionServiceStub.onFirstCall().resolves({
+        ..._chargingModuleResponse('7e752fa6-a19c-4779-b28c-6e536f028795')
+      })
+      chargingModuleCreateTransactionServiceStub.onSecondCall().rejects()
     })
 
     it('throws a BillRunError with the correct code', async () => {
@@ -123,3 +101,48 @@ describe('Send Transactions service', () => {
     })
   })
 })
+
+function _transaction (transactionId) {
+  return {
+    id: transactionId,
+    chargeReferenceId: '32058a19-4813-4ee7-808b-a0559deb8469',
+    startDate: new Date('2022-04-01'),
+    endDate: new Date('2022-10-31'),
+    source: 'non-tidal',
+    season: 'all year',
+    loss: 'low',
+    credit: false,
+    chargeType: 'standard',
+    authorisedQuantity: 6.82,
+    billableQuantity: 6.82,
+    authorisedDays: 365,
+    billableDays: 214,
+    status: 'candidate',
+    description: 'Water abstraction charge: Mineral washing',
+    volume: 6.82,
+    section126Factor: 1,
+    section127Agreement: false,
+    section130Agreement: false,
+    newLicence: false,
+    twoPartSecondPartCharge: false,
+    scheme: 'sroc',
+    aggregateFactor: 0.562114443,
+    adjustmentFactor: 1,
+    chargeCategoryCode: '4.4.5',
+    chargeCategoryDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model',
+    supportedSource: false,
+    supportedSourceName: null,
+    waterCompanyCharge: true,
+    winterOnly: false,
+    waterUndertaker: false
+  }
+}
+
+function _chargingModuleResponse (transactionId) {
+  return {
+    succeeded: true,
+    response: {
+      body: { transaction: { id: transactionId } }
+    }
+  }
+}

From ebe7a58517ce0888e1cea70236e47da59853ec4e Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 17:55:54 +0000
Subject: [PATCH 07/11] Refactor SendTransactionsService to send all

One of the performance improvements we identified when working on the annual bill run spike was that we can reduce the time it takes to generate the bill run if we allow the service to send them all together rather than awaiting each one.
---
 .../send-transactions.service.js              | 60 +++++++++++--------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/supplementary/send-transactions.service.js
index c2e074f4e4..29f9342cf9 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/supplementary/send-transactions.service.js
@@ -22,40 +22,50 @@ const ChargingModuleCreateTransactionPresenter = require('../../../presenters/ch
  * @param {Object[]} transactions The transactions to be sent to the Charging Module
  * @param {Object} billingPeriod The billing period of the transactions
  *
- * @returns {Promise<Object[]>} Array of transactions which have been sent to the Charging Module
+ * @returns {Promise<Object[]>} Array of transactions which have been sent to the Charging Module and updated with its
+ * response
  */
 async function go (licence, bill, billRunExternalId, transactions) {
-  try {
-    const sentTransactions = []
-
-    for (const transaction of transactions) {
-      const chargingModuleResponse = await _sendTransactionToChargingModule(
-        transaction,
-        bill,
-        licence,
-        billRunExternalId
-      )
-
-      transaction.status = 'charge_created'
-      transaction.externalId = chargingModuleResponse.response.body.transaction.id
+  const sendRequests = []
 
-      sentTransactions.push(transaction)
-    }
+  for (const transaction of transactions) {
+    // NOTE: we purposefully loop through all the transactions to send without awaiting them. This is for performance
+    // purposes. If for example we have 3 transactions to send we'll send the requests 1 straight after the other. We
+    // then wait for all 3 to complete. The overall process time will only be that of the one that takes the longest. If
+    // we await instead the overall time will be the sum of the time to complete each one.
+    const sendRequest = _sendTransactionToChargingModule(
+      transaction,
+      bill,
+      licence,
+      billRunExternalId
+    )
 
-    return sentTransactions
-  } catch (error) {
-    throw new BillRunError(error, BillRunModel.errorCodes.failedToCreateCharge)
+    sendRequests.push(sendRequest)
   }
+
+  // We use Promise.all() to ensure we wait for all the send requests to resolve. The service that awaits the call to
+  // SendTransactionsService.go() will still get the updated transactions as Promise.all() returns what each promise
+  // resolves to as an array.
+  return Promise.all(sendRequests)
 }
 
 async function _sendTransactionToChargingModule (transaction, bill, licence, billRunExternalId) {
-  const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go(
-    transaction,
-    bill.accountNumber,
-    licence
-  )
+  try {
+    const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go(
+      transaction,
+      bill.accountNumber,
+      licence
+    )
+
+    const chargingModuleResponse = await ChargingModuleCreateTransactionService.go(billRunExternalId, chargingModuleRequest)
+
+    transaction.status = 'charge_created'
+    transaction.externalId = chargingModuleResponse.response.body.transaction.id
 
-  return ChargingModuleCreateTransactionService.go(billRunExternalId, chargingModuleRequest)
+    return transaction
+  } catch (error) {
+    throw new BillRunError(error, BillRunModel.errorCodes.failedToCreateCharge)
+  }
 }
 
 module.exports = {

From b7d17e36a315096e64a4232e8545d6de22e6510e Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 18:03:20 +0000
Subject: [PATCH 08/11] Refactor to use map()

Just makes things a little cleaner and is more idiomatic.
---
 .../send-transactions.service.js              | 23 ++++++-------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/supplementary/send-transactions.service.js
index 29f9342cf9..f0567e85d7 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/supplementary/send-transactions.service.js
@@ -26,22 +26,13 @@ const ChargingModuleCreateTransactionPresenter = require('../../../presenters/ch
  * response
  */
 async function go (licence, bill, billRunExternalId, transactions) {
-  const sendRequests = []
-
-  for (const transaction of transactions) {
-    // NOTE: we purposefully loop through all the transactions to send without awaiting them. This is for performance
-    // purposes. If for example we have 3 transactions to send we'll send the requests 1 straight after the other. We
-    // then wait for all 3 to complete. The overall process time will only be that of the one that takes the longest. If
-    // we await instead the overall time will be the sum of the time to complete each one.
-    const sendRequest = _sendTransactionToChargingModule(
-      transaction,
-      bill,
-      licence,
-      billRunExternalId
-    )
-
-    sendRequests.push(sendRequest)
-  }
+  // NOTE: we purposefully loop through all the transactions to send without awaiting them. This is for performance
+  // purposes. If for example we have 3 transactions to send we'll send the requests 1 straight after the other. We
+  // then wait for all 3 to complete. The overall process time will only be that of the one that takes the longest. If
+  // we await instead the overall time will be the sum of the time to complete each one.
+  const sendRequests = transactions.map((transaction) => {
+    return _sendTransactionToChargingModule(transaction, bill, licence, billRunExternalId)
+  })
 
   // We use Promise.all() to ensure we wait for all the send requests to resolve. The service that awaits the call to
   // SendTransactionsService.go() will still get the updated transactions as Promise.all() returns what each promise

From 3974afce6c22b438e796635b46b1069543e5f28f Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 18:24:45 +0000
Subject: [PATCH 09/11] Tidy up arguments to the service

Spotted the docs and tests were sending through a billing period which is no longer referred to. Plus as transactions is the critical param we move that to first position. We also just pass in the account number rather than the whole bill as that is all we need.
---
 .../process-billing-period.service.js         |  6 ++---
 .../send-transactions.service.js              | 26 +++++++++----------
 .../send-transactions.service.test.js         | 22 +++-------------
 3 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/app/services/bill-runs/supplementary/process-billing-period.service.js b/app/services/bill-runs/supplementary/process-billing-period.service.js
index 3e38ab6854..72d5d748ee 100644
--- a/app/services/bill-runs/supplementary/process-billing-period.service.js
+++ b/app/services/bill-runs/supplementary/process-billing-period.service.js
@@ -62,10 +62,10 @@ async function _buildDataToPersist (billingData, billingPeriod, billRunExternalI
 
     if (cleansedTransactions.length !== 0) {
       const transactions = await SendTransactionsService.go(
-        currentBillingData.licence,
-        currentBillingData.bill,
+        cleansedTransactions,
         billRunExternalId,
-        cleansedTransactions
+        currentBillingData.bill.accountNumber,
+        currentBillingData.licence
       )
 
       dataToPersist.transactions.push(...transactions)
diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/supplementary/send-transactions.service.js
index f0567e85d7..cb03fcbc91 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/supplementary/send-transactions.service.js
@@ -16,22 +16,21 @@ const ChargingModuleCreateTransactionPresenter = require('../../../presenters/ch
  * Each sent transaction is updated with a status of `charge_created` and the external id returned by the
  * Charging Module.
  *
- * @param {module:LicenceModel} licence The licence that each transaction is linked to
- * @param {module:BillModel} bill The bill each transaction is to be linked to
- * @param {string} billRunExternalId The Charging Module bill run id that the transactions are to be created on
- * @param {Object[]} transactions The transactions to be sent to the Charging Module
- * @param {Object} billingPeriod The billing period of the transactions
+ * @param {Object[]} transactions - The transactions to be sent to the Charging Module
+ * @param {string} billRunExternalId - The Charging Module bill run id that the transactions are to be created on
+ * @param {string} accountNumber - The billing account number for the transactions
+ * @param {module:LicenceModel} licence - The licence that each transaction is linked to
  *
  * @returns {Promise<Object[]>} Array of transactions which have been sent to the Charging Module and updated with its
  * response
  */
-async function go (licence, bill, billRunExternalId, transactions) {
+async function go (transactions, billRunExternalId, accountNumber, licence) {
   // NOTE: we purposefully loop through all the transactions to send without awaiting them. This is for performance
   // purposes. If for example we have 3 transactions to send we'll send the requests 1 straight after the other. We
   // then wait for all 3 to complete. The overall process time will only be that of the one that takes the longest. If
   // we await instead the overall time will be the sum of the time to complete each one.
   const sendRequests = transactions.map((transaction) => {
-    return _sendTransactionToChargingModule(transaction, bill, licence, billRunExternalId)
+    return _sendTransactionToChargingModule(transaction, billRunExternalId, accountNumber, licence)
   })
 
   // We use Promise.all() to ensure we wait for all the send requests to resolve. The service that awaits the call to
@@ -40,15 +39,14 @@ async function go (licence, bill, billRunExternalId, transactions) {
   return Promise.all(sendRequests)
 }
 
-async function _sendTransactionToChargingModule (transaction, bill, licence, billRunExternalId) {
+async function _sendTransactionToChargingModule (transaction, billRunExternalId, accountNumber, licence) {
   try {
-    const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go(
-      transaction,
-      bill.accountNumber,
-      licence
-    )
+    const chargingModuleRequest = ChargingModuleCreateTransactionPresenter.go(transaction, accountNumber, licence)
 
-    const chargingModuleResponse = await ChargingModuleCreateTransactionService.go(billRunExternalId, chargingModuleRequest)
+    const chargingModuleResponse = await ChargingModuleCreateTransactionService.go(
+      billRunExternalId,
+      chargingModuleRequest
+    )
 
     transaction.status = 'charge_created'
     transaction.externalId = chargingModuleResponse.response.body.transaction.id
diff --git a/test/services/bill-runs/supplementary/send-transactions.service.test.js b/test/services/bill-runs/supplementary/send-transactions.service.test.js
index fefc93d6cf..b08c12be13 100644
--- a/test/services/bill-runs/supplementary/send-transactions.service.test.js
+++ b/test/services/bill-runs/supplementary/send-transactions.service.test.js
@@ -19,12 +19,8 @@ const ChargingModuleCreateTransactionService = require('../../../../app/services
 const SendTransactionsService = require('../../../../app/services/bill-runs/supplementary/send-transactions.service.js')
 
 describe('Send Transactions service', () => {
+  const accountNumber = 'ABC123'
   const billRunExternalId = '4f3710ca-75b1-4828-8fe9-f7c1edecbbf3'
-  const bill = { accountNumber: 'ABC123' }
-  const billingPeriod = {
-    startDate: new Date('2022-04-01'),
-    endDate: new Date('2023-03-31')
-  }
   const licence = {
     historicalAreaCode: 'DALES',
     licenceRef: 'AT/CURR/MONTHLY/02',
@@ -59,13 +55,7 @@ describe('Send Transactions service', () => {
     })
 
     it('updates the transactions with the responses from the Charging Module API', async () => {
-      const results = await SendTransactionsService.go(
-        licence,
-        bill,
-        billRunExternalId,
-        transactions,
-        billingPeriod
-      )
+      const results = await SendTransactionsService.go(transactions, billRunExternalId, accountNumber, licence)
 
       expect(results).length(2)
       expect(results[0].status).to.equal('charge_created')
@@ -85,13 +75,7 @@ describe('Send Transactions service', () => {
 
     it('throws a BillRunError with the correct code', async () => {
       const error = await expect(
-        SendTransactionsService.go(
-          licence,
-          bill,
-          billRunExternalId,
-          transactions,
-          billingPeriod
-        )
+        SendTransactionsService.go(transactions, billRunExternalId, accountNumber, licence)
       )
         .to
         .reject()

From 965b156532846b2b3ddcb6c7ce5b66e52105b0e1 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 18:29:17 +0000
Subject: [PATCH 10/11] Move SendTransactionsService to /bill-runs root

---
 .../{supplementary => }/send-transactions.service.js      | 8 ++++----
 .../supplementary/process-billing-period.service.js       | 2 +-
 .../{supplementary => }/send-transactions.service.test.js | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)
 rename app/services/bill-runs/{supplementary => }/send-transactions.service.js (89%)
 rename test/services/bill-runs/{supplementary => }/send-transactions.service.test.js (92%)

diff --git a/app/services/bill-runs/supplementary/send-transactions.service.js b/app/services/bill-runs/send-transactions.service.js
similarity index 89%
rename from app/services/bill-runs/supplementary/send-transactions.service.js
rename to app/services/bill-runs/send-transactions.service.js
index cb03fcbc91..8c4fb3f960 100644
--- a/app/services/bill-runs/supplementary/send-transactions.service.js
+++ b/app/services/bill-runs/send-transactions.service.js
@@ -5,10 +5,10 @@
  * @module SendTransactionsService
  */
 
-const BillRunError = require('../../../errors/bill-run.error.js')
-const BillRunModel = require('../../../models/bill-run.model.js')
-const ChargingModuleCreateTransactionService = require('../../charging-module/create-transaction.service.js')
-const ChargingModuleCreateTransactionPresenter = require('../../../presenters/charging-module/create-transaction.presenter.js')
+const BillRunError = require('../../errors/bill-run.error.js')
+const BillRunModel = require('../../models/bill-run.model.js')
+const ChargingModuleCreateTransactionService = require('../charging-module/create-transaction.service.js')
+const ChargingModuleCreateTransactionPresenter = require('../../presenters/charging-module/create-transaction.presenter.js')
 
 /**
  * Sends the provided transactions to the Charging Module and returns an array of the sent transactions
diff --git a/app/services/bill-runs/supplementary/process-billing-period.service.js b/app/services/bill-runs/supplementary/process-billing-period.service.js
index 72d5d748ee..819eff7dd0 100644
--- a/app/services/bill-runs/supplementary/process-billing-period.service.js
+++ b/app/services/bill-runs/supplementary/process-billing-period.service.js
@@ -14,7 +14,7 @@ const DetermineMinimumChargeService = require('../determine-minimum-charge.servi
 const GenerateTransactionsService = require('../generate-transactions.service.js')
 const PreGenerateBillingDataService = require('./pre-generate-billing-data.service.js')
 const ProcessTransactionsService = require('./process-transactions.service.js')
-const SendTransactionsService = require('./send-transactions.service.js')
+const SendTransactionsService = require('../send-transactions.service.js')
 const TransactionModel = require('../../../models/transaction.model.js')
 
 /**
diff --git a/test/services/bill-runs/supplementary/send-transactions.service.test.js b/test/services/bill-runs/send-transactions.service.test.js
similarity index 92%
rename from test/services/bill-runs/supplementary/send-transactions.service.test.js
rename to test/services/bill-runs/send-transactions.service.test.js
index b08c12be13..de27ad2d37 100644
--- a/test/services/bill-runs/supplementary/send-transactions.service.test.js
+++ b/test/services/bill-runs/send-transactions.service.test.js
@@ -9,14 +9,14 @@ const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script()
 const { expect } = Code
 
 // Test helpers
-const BillRunError = require('../../../../app/errors/bill-run.error.js')
-const BillRunModel = require('../../../../app/models/bill-run.model.js')
+const BillRunError = require('../../../app/errors/bill-run.error.js')
+const BillRunModel = require('../../../app/models/bill-run.model.js')
 
 // Things we need to stub
-const ChargingModuleCreateTransactionService = require('../../../../app/services/charging-module/create-transaction.service.js')
+const ChargingModuleCreateTransactionService = require('../../../app/services/charging-module/create-transaction.service.js')
 
 // Thing under test
-const SendTransactionsService = require('../../../../app/services/bill-runs/supplementary/send-transactions.service.js')
+const SendTransactionsService = require('../../../app/services/bill-runs/send-transactions.service.js')
 
 describe('Send Transactions service', () => {
   const accountNumber = 'ABC123'

From 8b017a755c6312bd32d96ea45b128dc1a7b29b56 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 20 Feb 2024 18:52:53 +0000
Subject: [PATCH 11/11] Fix broken test

---
 .../supplementary/process-billing-period.service.test.js        | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/services/bill-runs/supplementary/process-billing-period.service.test.js b/test/services/bill-runs/supplementary/process-billing-period.service.test.js
index c3422d42d8..ca976e4ffd 100644
--- a/test/services/bill-runs/supplementary/process-billing-period.service.test.js
+++ b/test/services/bill-runs/supplementary/process-billing-period.service.test.js
@@ -26,7 +26,7 @@ const RegionHelper = require('../../../support/helpers/region.helper.js')
 // Things we need to stub
 const ChargingModuleGenerateService = require('../../../../app/services/charging-module/generate-bill-run.service.js')
 const GenerateTransactionsService = require('../../../../app/services/bill-runs/generate-transactions.service.js')
-const SendTransactionsService = require('../../../../app/services/bill-runs/supplementary/send-transactions.service.js')
+const SendTransactionsService = require('../../../../app/services/bill-runs/send-transactions.service.js')
 
 // Thing under test
 const ProcessBillingPeriodService = require('../../../../app/services/bill-runs/supplementary/process-billing-period.service.js')