From 321daefb962004bad6286e39b46c1eb35c3faff2 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 11 Nov 2022 15:02:52 +0000
Subject: [PATCH 01/13] Select SROC supplementary charge versions for real

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

In [Fetching the charge versions data](https://github.com/DEFRA/water-abstraction-system/pull/15) we added a new service to start selecting scharge versions to be included in the Supplementary Bill run.

But at the moment, it's only filter is all charge versions with the `scheme` SROC. We have test data and scenarios to work with so we can now start implementing the filter 'for real'.

This is the start of the process. We expect it's likely we'll be finessing the filter in subsequent PR's. But this gets the ball rolling.

From 7e1a2b75df869c46121f77af03f8feaca53816ca Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 11 Nov 2022 18:19:13 +0000
Subject: [PATCH 02/13] Add migration for 'water.licences' table

It turns out the oft-mentioned `include_in_supplementary_billing` flag is in the `licences` table and not the `charge_versions` table. This means we can't just rely on checking the charge versions table and will need to build a licences table for our testing.

We also add a migration to add `licence_id` and `end_date` fields to the charge versions table. `licence_id` is how the 2 tables are linked and `end_date` is what we are having to rely on to determine the 'current' charge version.
---
 .../20221111155222_create_licences.js         | 35 +++++++++++++++++++
 .../20221111184905_alter_charge_versions.js   | 22 ++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 db/migrations/20221111155222_create_licences.js
 create mode 100644 db/migrations/20221111184905_alter_charge_versions.js

diff --git a/db/migrations/20221111155222_create_licences.js b/db/migrations/20221111155222_create_licences.js
new file mode 100644
index 0000000000..404ef3fc8b
--- /dev/null
+++ b/db/migrations/20221111155222_create_licences.js
@@ -0,0 +1,35 @@
+'use strict'
+
+const tableName = 'licences'
+
+exports.up = async function (knex) {
+  await knex
+    .schema
+    .withSchema('water')
+    .createTable(tableName, table => {
+      // Primary Key
+      table.uuid('licence_id').primary().defaultTo(knex.raw('gen_random_uuid()'))
+
+      // Data
+      table.string('licence_ref')
+      table.string('include_in_supplementary_billing')
+
+      // Automatic timestamps
+      table.timestamps(false, true)
+    })
+
+  await knex.raw(`
+    CREATE TRIGGER update_timestamp
+    BEFORE UPDATE
+    ON water.${tableName}
+    FOR EACH ROW
+    EXECUTE PROCEDURE update_timestamp();
+  `)
+}
+
+exports.down = function (knex) {
+  return knex
+    .schema
+    .withSchema('water')
+    .dropTableIfExists(tableName)
+}
diff --git a/db/migrations/20221111184905_alter_charge_versions.js b/db/migrations/20221111184905_alter_charge_versions.js
new file mode 100644
index 0000000000..5c26177c12
--- /dev/null
+++ b/db/migrations/20221111184905_alter_charge_versions.js
@@ -0,0 +1,22 @@
+'use strict'
+
+const tableName = 'charge_versions'
+
+exports.up = async function (knex) {
+  await knex
+    .schema
+    .withSchema('water')
+    .alterTable(tableName, table => {
+      table.uuid('licence_id')
+      table.date('end_date')
+    })
+}
+
+exports.down = function (knex) {
+  return knex
+    .schema
+    .withSchema('water')
+    .alterTable(tableName, table => {
+      table.dropColumns('licence_id', 'end_date')
+    })
+}

From c5e034de530cc43f738f584305e4758f41226726 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 11 Nov 2022 18:38:58 +0000
Subject: [PATCH 03/13] Update knex query in SupplementaryService
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two changes here. First me re-ordered the method calls and swapped some out for other aliases in order for the query to read more like traditional SQL.

The second is that we have updated it to take into account the `include_in_supplementary_billing` flag on the licences table. This is the main driver of whether to include something in supplementary billing.

“Simple”, was our next thought. Charge versions have a `scheme` and a `status` column; we'll just grab the one where `scheme=sroc && status=current` for each licence flagged for supplementary billing. That was until our initial tests showed that `status` is not getting set correctly. This means we have multiple SROC charge versions both with a status of `current`.

Fortunately, it looks like `end_date` is getting set as expected, which means we have a way of selecting them (for now!)
---
 app/services/test/supplementary.service.js | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/app/services/test/supplementary.service.js b/app/services/test/supplementary.service.js
index 937cd1c666..49e50c7727 100644
--- a/app/services/test/supplementary.service.js
+++ b/app/services/test/supplementary.service.js
@@ -23,10 +23,17 @@ class SupplementaryService {
   }
 
   static async _fetchChargeVersions () {
-    const chargeVersions = db.table('water.charge_versions')
-      .where('scheme', 'sroc')
-      .select('chargeVersionId')
-      .select('licenceRef')
+    const chargeVersions = db
+      .select('chargeVersionId', 'licences.licenceRef')
+      .from('water.charge_versions')
+      .innerJoin('water.licences', 'charge_versions.licence_id', 'licences.licence_id')
+      .where({
+        scheme: 'sroc',
+        end_date: null
+      })
+      .andWhere({
+        'licences.include_in_supplementary_billing': 'yes'
+      })
 
     return chargeVersions
   }

From 31a4ce2c1c5dfba2b56bec73502d458fd1574db5 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Sun, 13 Nov 2022 22:08:27 +0000
Subject: [PATCH 04/13] Expand the test data helper support

We add a new `LicenceHelper` as our query now needs these linked to charge versions. Rather than have our test have to repeatedly set up this up we expand our data helpers.

First part of this is having the charge version helper automatically call the licence helper and manage linking the 2 records. Adding the licence also means we need to start passing a bunch of data around now. So, instead we go with some defaults. We'll add some documentation to make this more visible in subsequent commits.
---
 test/support/helpers/charge_version.helper.js | 27 ++++++++++++++--
 test/support/helpers/licence.helper.js        | 31 +++++++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 test/support/helpers/licence.helper.js

diff --git a/test/support/helpers/charge_version.helper.js b/test/support/helpers/charge_version.helper.js
index 3fdd2cc5bf..dd31238cea 100644
--- a/test/support/helpers/charge_version.helper.js
+++ b/test/support/helpers/charge_version.helper.js
@@ -6,14 +6,37 @@
 
 const { db } = require('../../../db/db')
 
+const LicenceHelper = require('./licence.helper.js')
+
 class ChargeVersionHelper {
-  static async add (data) {
+  static async add (data, licence = {}) {
+    const licenceId = await this._addLicence(licence)
+    const insertData = this._defaults({ ...data, licence_id: licenceId })
+
     const result = await db.table('water.charge_versions')
-      .insert(data)
+      .insert(insertData)
       .returning('charge_version_id')
 
     return result
   }
+
+  static async _addLicence (licence) {
+    const result = await LicenceHelper.add(licence)
+
+    return result[0].licenceId
+  }
+
+  static _defaults (data) {
+    const defaults = {
+      licence_ref: '01/123',
+      scheme: 'sroc'
+    }
+
+    return {
+      ...defaults,
+      ...data
+    }
+  }
 }
 
 module.exports = ChargeVersionHelper
diff --git a/test/support/helpers/licence.helper.js b/test/support/helpers/licence.helper.js
new file mode 100644
index 0000000000..6712afe500
--- /dev/null
+++ b/test/support/helpers/licence.helper.js
@@ -0,0 +1,31 @@
+'use strict'
+
+/**
+ * @module LicenceHelper
+ */
+
+const { db } = require('../../../db/db')
+
+class LicenceHelper {
+  static async add (data) {
+    const insertData = this._defaults(data)
+    const result = await db.table('water.licences')
+      .insert(insertData)
+      .returning('licence_id')
+
+    return result
+  }
+
+  static _defaults (data) {
+    const defaults = {
+      licence_ref: '01/123'
+    }
+
+    return {
+      ...defaults,
+      ...data
+    }
+  }
+}
+
+module.exports = LicenceHelper

From b26c828ab4904e596f1363e350785080014daf01 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Sun, 13 Nov 2022 22:13:00 +0000
Subject: [PATCH 05/13] Update Supplementary service test

With the updated query in the service we needed to update our tests to setup the appropriate environments. We've expanded the functionality of our data helpers. So, we take advantage of them to update and expand the tests we have for this service.
---
 .../test/supplementary.service.test.js        | 73 +++++++++++++++----
 1 file changed, 60 insertions(+), 13 deletions(-)

diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index 34248fb71b..a02d336461 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -15,22 +15,30 @@ const DatabaseHelper = require('../../support/helpers/database.helper')
 const SupplementaryService = require('../../../app/services/test/supplementary.service.js')
 
 describe('Supplementary service', () => {
-  const testData = {
-    sroc: { licence_ref: '01/123', scheme: 'sroc' },
-    alcs: { licence_ref: '01/456', scheme: 'alcs' }
-  }
   let testRecords
 
   beforeEach(async () => {
     await DatabaseHelper.clean()
   })
 
-  describe('when there are charge versions to be included in supplimentery billing', () => {
+  describe('when there are licences to be included in supplimentery billing', () => {
     beforeEach(async () => {
-      testRecords = await ChargeVersionHelper.add([testData.sroc, testData.alcs])
+      // This creates an SROC charge version linked to a licence marked for supplementary billing
+      const srocChargeVersion = await ChargeVersionHelper.add(
+        {},
+        { include_in_supplementary_billing: 'yes' }
+      )
+
+      // This creates an ALCS (presroc) charge version linked to a licence marked for supplementary billing
+      const alcsChargeVersion = await ChargeVersionHelper.add(
+        { scheme: 'alcs' },
+        { include_in_supplementary_billing: 'yes' }
+      )
+
+      testRecords = [srocChargeVersion, alcsChargeVersion]
     })
 
-    it('returns only those that match', async () => {
+    it('returns only the current SROC charge versions that are applicable', async () => {
       const result = await SupplementaryService.go()
 
       expect(result.chargeVersions.length).to.equal(1)
@@ -38,15 +46,54 @@ describe('Supplementary service', () => {
     })
   })
 
-  describe('when there are no charge versions to be included in supplimentery billing', () => {
-    beforeEach(async () => {
-      testRecords = await ChargeVersionHelper.add(testData.alcs)
+  describe('when there are no licences to be included in supplimentery billing', () => {
+    describe("because none of them are marked 'include_in_supplimentary_billing'", () => {
+      beforeEach(async () => {
+        // This creates an SROC charge version linked to a licence. But the licence won't be marked for supplementary
+        // billing
+        const srocChargeVersion = await ChargeVersionHelper.add()
+        testRecords = [srocChargeVersion]
+      })
+
+      it('returns no applicable charge versions', async () => {
+        const result = await SupplementaryService.go()
+
+        expect(result.chargeVersions.length).to.equal(0)
+      })
     })
 
-    it('returns no matches', async () => {
-      const result = await SupplementaryService.go()
+    describe("because all the applicable charge versions are 'alcs' (presroc)", () => {
+      beforeEach(async () => {
+        // This creates an ALCS (presroc) charge version linked to a licence marked for supplementary billing
+        const alcsChargeVersion = await ChargeVersionHelper.add(
+          { scheme: 'alcs' },
+          { include_in_supplementary_billing: 'yes' }
+        )
+        testRecords = [alcsChargeVersion]
+      })
+
+      it('returns no applicable charge versions', async () => {
+        const result = await SupplementaryService.go()
+
+        expect(result.chargeVersions.length).to.equal(0)
+      })
+    })
+
+    describe('because there are no current charge versions (they all have end dates)', () => {
+      beforeEach(async () => {
+        // This creates an SROC charge version with an edn date linked to a licence marked for supplementary billing
+        const alcsChargeVersion = await ChargeVersionHelper.add(
+          { end_date: new Date(2022, 2, 1) }, // 2022-03-01 - Months are zero indexed :-)
+          { include_in_supplementary_billing: 'yes' }
+        )
+        testRecords = [alcsChargeVersion]
+      })
+
+      it('returns no applicable charge versions', async () => {
+        const result = await SupplementaryService.go()
 
-      expect(result.chargeVersions.length).to.equal(0)
+        expect(result.chargeVersions.length).to.equal(0)
+      })
     })
   })
 })

From d749826e8431042af792b746363d9ee4f7767d81 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Sun, 13 Nov 2022 22:38:27 +0000
Subject: [PATCH 06/13] Add some documentation to our helpers

---
 test/support/helpers/charge_version.helper.js | 20 ++++++++++++++++++-
 test/support/helpers/licence.helper.js        | 11 ++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/test/support/helpers/charge_version.helper.js b/test/support/helpers/charge_version.helper.js
index dd31238cea..07eaf4f1e6 100644
--- a/test/support/helpers/charge_version.helper.js
+++ b/test/support/helpers/charge_version.helper.js
@@ -9,7 +9,25 @@ const { db } = require('../../../db/db')
 const LicenceHelper = require('./licence.helper.js')
 
 class ChargeVersionHelper {
-  static async add (data, licence = {}) {
+  /**
+   * Add a new charge version
+   *
+   * A charge version is always linked to a licence. So, creating a charge version will automatically create a new
+   * licence and handle linking the two together by `licence_id`.
+   *
+   * If no `data` is provided, default values will be used. These are
+   *
+   * - `scheme` - sroc
+   * - `licence_ref` - 01/123
+   *
+   * See `LicenceHelper` for the licence defaults
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   * @param {Object} [licence] Any licence data you want to use instead of the defaults used here or in the database
+   *
+   * @returns {string} The ID of the newly created record
+   */
+  static async add (data = {}, licence = {}) {
     const licenceId = await this._addLicence(licence)
     const insertData = this._defaults({ ...data, licence_id: licenceId })
 
diff --git a/test/support/helpers/licence.helper.js b/test/support/helpers/licence.helper.js
index 6712afe500..8cd1b0fd0c 100644
--- a/test/support/helpers/licence.helper.js
+++ b/test/support/helpers/licence.helper.js
@@ -7,6 +7,17 @@
 const { db } = require('../../../db/db')
 
 class LicenceHelper {
+  /**
+   * Add a new licence
+   *
+   * If no `data` is provided, default values will be used. These are
+   *
+   * - `licence_ref` - 01/123
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   *
+   * @returns {string} The ID of the newly created record
+   */
   static async add (data) {
     const insertData = this._defaults(data)
     const result = await db.table('water.licences')

From bfbf86bd85b0149d12493af4e8cdf107105376ab Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Sun, 13 Nov 2022 22:43:56 +0000
Subject: [PATCH 07/13] Remove outdated comment

---
 app/services/test/supplementary.service.js | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/services/test/supplementary.service.js b/app/services/test/supplementary.service.js
index 49e50c7727..26e0921b43 100644
--- a/app/services/test/supplementary.service.js
+++ b/app/services/test/supplementary.service.js
@@ -7,11 +7,9 @@
 const { db } = require('../../../db/db')
 
 /**
- * Returns charge versions selected for supplementary billing
- * At present this returns a set response until further development
-*/
+ * @module SupplementaryService
+ */
 
-// Format into the response and return the data
 class SupplementaryService {
   static async go () {
     const chargeVersions = await this._fetchChargeVersions()

From 01d804ed6b49cbe17a858cb5e71b626264ae0ed8 Mon Sep 17 00:00:00 2001
From: Jason Claxton <30830544+Jozzey@users.noreply.github.com>
Date: Mon, 14 Nov 2022 16:46:33 +0000
Subject: [PATCH 08/13] add region to test data and supplementary selection
 criteria

---
 .../test/supplementary.controller.js          |  7 ++-
 app/services/test/find_region.service.js      | 33 ++++++++++++++
 app/services/test/supplementary.service.js    |  9 ++--
 .../20221114104700_create_regions.js          | 35 +++++++++++++++
 .../20221114155444_alter_licences.js          | 21 +++++++++
 .../test/supplementary.controller.test.js     |  4 +-
 .../test/supplementary.service.test.js        | 34 +++++++++++---
 test/support/helpers/licence.helper.js        |  9 ++--
 test/support/helpers/region.helper.js         | 44 +++++++++++++++++++
 9 files changed, 179 insertions(+), 17 deletions(-)
 create mode 100644 app/services/test/find_region.service.js
 create mode 100644 db/migrations/20221114104700_create_regions.js
 create mode 100644 db/migrations/20221114155444_alter_licences.js
 create mode 100644 test/support/helpers/region.helper.js

diff --git a/app/controllers/test/supplementary.controller.js b/app/controllers/test/supplementary.controller.js
index ce00a5aceb..0c23826a1e 100644
--- a/app/controllers/test/supplementary.controller.js
+++ b/app/controllers/test/supplementary.controller.js
@@ -1,10 +1,13 @@
 'use strict'
 
+const FindRegionService = require('../../services/test/find_region.service')
 const SupplementaryService = require('../../services/test/supplementary.service.js')
 
 class SupplementaryController {
-  static async index (_request, h) {
-    const result = await SupplementaryService.go()
+  static async index (request, h) {
+    const region = await FindRegionService.go(request.query.region)
+
+    const result = await SupplementaryService.go(region.regionId)
 
     return h.response(result).code(200)
   }
diff --git a/app/services/test/find_region.service.js b/app/services/test/find_region.service.js
new file mode 100644
index 0000000000..4831d0d987
--- /dev/null
+++ b/app/services/test/find_region.service.js
@@ -0,0 +1,33 @@
+'use strict'
+
+/**
+ * @module FindRegionService
+ */
+
+const { db } = require('../../../db/db')
+
+/**
+ * @module FindRegionService
+ */
+
+class FindRegionService {
+  static async go (regionId) {
+    const region = await this._fetchRegion(regionId)
+
+    return region
+  }
+
+  static async _fetchRegion (regionId) {
+    const result = await db
+      .select('region_id')
+      .from('water.regions')
+      .where({
+        nald_region_id: regionId
+      })
+      .first()
+
+    return result
+  }
+}
+
+module.exports = FindRegionService
diff --git a/app/services/test/supplementary.service.js b/app/services/test/supplementary.service.js
index 26e0921b43..5b87a07919 100644
--- a/app/services/test/supplementary.service.js
+++ b/app/services/test/supplementary.service.js
@@ -11,8 +11,8 @@ const { db } = require('../../../db/db')
  */
 
 class SupplementaryService {
-  static async go () {
-    const chargeVersions = await this._fetchChargeVersions()
+  static async go (regionId) {
+    const chargeVersions = await this._fetchChargeVersions(regionId)
     const response = {
       chargeVersions
     }
@@ -20,7 +20,7 @@ class SupplementaryService {
     return response
   }
 
-  static async _fetchChargeVersions () {
+  static async _fetchChargeVersions (regionId) {
     const chargeVersions = db
       .select('chargeVersionId', 'licences.licenceRef')
       .from('water.charge_versions')
@@ -30,7 +30,8 @@ class SupplementaryService {
         end_date: null
       })
       .andWhere({
-        'licences.include_in_supplementary_billing': 'yes'
+        'licences.include_in_supplementary_billing': 'yes',
+        'licences.region_id': regionId
       })
 
     return chargeVersions
diff --git a/db/migrations/20221114104700_create_regions.js b/db/migrations/20221114104700_create_regions.js
new file mode 100644
index 0000000000..b54c888845
--- /dev/null
+++ b/db/migrations/20221114104700_create_regions.js
@@ -0,0 +1,35 @@
+'use strict'
+
+const tableName = 'regions'
+
+exports.up = async function (knex) {
+  await knex
+    .schema
+    .withSchema('water')
+    .createTable(tableName, table => {
+      // Primary Key
+      table.uuid('region_id').primary().defaultTo(knex.raw('gen_random_uuid()'))
+
+      // Data
+      table.string('charge_region_id')
+      table.integer('nald_region_id')
+
+      // Automatic timestamps
+      table.timestamps(false, true)
+    })
+
+  await knex.raw(`
+    CREATE TRIGGER update_timestamp
+    BEFORE UPDATE
+    ON water.${tableName}
+    FOR EACH ROW
+    EXECUTE PROCEDURE update_timestamp();
+  `)
+}
+
+exports.down = function (knex) {
+  return knex
+    .schema
+    .withSchema('water')
+    .dropTableIfExists(tableName)
+}
diff --git a/db/migrations/20221114155444_alter_licences.js b/db/migrations/20221114155444_alter_licences.js
new file mode 100644
index 0000000000..85872907b7
--- /dev/null
+++ b/db/migrations/20221114155444_alter_licences.js
@@ -0,0 +1,21 @@
+'use strict'
+
+const tableName = 'licences'
+
+exports.up = async function (knex) {
+  await knex
+    .schema
+    .withSchema('water')
+    .alterTable(tableName, table => {
+      table.uuid('region_id')
+    })
+}
+
+exports.down = function (knex) {
+  return knex
+    .schema
+    .withSchema('water')
+    .alterTable(tableName, table => {
+      table.dropColumns('region_id')
+    })
+}
diff --git a/test/controllers/test/supplementary.controller.test.js b/test/controllers/test/supplementary.controller.test.js
index 9627fabb12..4a74cbe8f0 100644
--- a/test/controllers/test/supplementary.controller.test.js
+++ b/test/controllers/test/supplementary.controller.test.js
@@ -9,6 +9,7 @@ const { describe, it, beforeEach, after } = exports.lab = Lab.script()
 const { expect } = Code
 
 // Things we need to stub
+const FindRegionService = require('../../../app/services/test/find_region.service')
 const SupplementaryService = require('../../../app/services/test/supplementary.service.js')
 
 // For running our service
@@ -28,12 +29,13 @@ describe('Supplementary controller', () => {
   describe('GET /test/supplementary', () => {
     const options = {
       method: 'GET',
-      url: '/test/supplementary'
+      url: '/test/supplementary?region=9'
     }
 
     let response
 
     beforeEach(async () => {
+      Sinon.stub(FindRegionService, 'go').resolves({ regionId: 'bd114474-790f-4470-8ba4-7b0cc9c225d7' })
       Sinon.stub(SupplementaryService, 'go').resolves({ chargeVersions: [] })
 
       response = await server.inject(options)
diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index a02d336461..57cb1258ad 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -8,13 +8,15 @@ const { describe, it, beforeEach } = exports.lab = Lab.script()
 const { expect } = Code
 
 // Test helpers
-const ChargeVersionHelper = require('../../support/helpers/charge_version.helper')
-const DatabaseHelper = require('../../support/helpers/database.helper')
+const ChargeVersionHelper = require('../../support/helpers/charge_version.helper.js')
+const DatabaseHelper = require('../../support/helpers/database.helper.js')
+const LicenceHelper = require('../../support/helpers/licence.helper.js')
 
 // Thing under test
 const SupplementaryService = require('../../../app/services/test/supplementary.service.js')
 
 describe('Supplementary service', () => {
+  const regionId = LicenceHelper.defaults().region_id
   let testRecords
 
   beforeEach(async () => {
@@ -39,7 +41,7 @@ describe('Supplementary service', () => {
     })
 
     it('returns only the current SROC charge versions that are applicable', async () => {
-      const result = await SupplementaryService.go()
+      const result = await SupplementaryService.go(regionId)
 
       expect(result.chargeVersions.length).to.equal(1)
       expect(result.chargeVersions[0].charge_version_id).to.equal(testRecords[0].charge_version_id)
@@ -56,7 +58,7 @@ describe('Supplementary service', () => {
       })
 
       it('returns no applicable charge versions', async () => {
-        const result = await SupplementaryService.go()
+        const result = await SupplementaryService.go(regionId)
 
         expect(result.chargeVersions.length).to.equal(0)
       })
@@ -73,7 +75,7 @@ describe('Supplementary service', () => {
       })
 
       it('returns no applicable charge versions', async () => {
-        const result = await SupplementaryService.go()
+        const result = await SupplementaryService.go(regionId)
 
         expect(result.chargeVersions.length).to.equal(0)
       })
@@ -90,7 +92,27 @@ describe('Supplementary service', () => {
       })
 
       it('returns no applicable charge versions', async () => {
-        const result = await SupplementaryService.go()
+        const result = await SupplementaryService.go(regionId)
+
+        expect(result.chargeVersions.length).to.equal(0)
+      })
+    })
+
+    describe('because there are no licences linked to the selected region', () => {
+      beforeEach(async () => {
+        // This creates an SROC charge version linked to an invalid region
+        const otherRegionChargeVersion = await ChargeVersionHelper.add(
+          {},
+          {
+            include_in_supplementary_billing: 'yes',
+            region_id: 'e117b501-e3c1-4337-ad35-21c60ed9ad73'
+          }
+        )
+        testRecords = [otherRegionChargeVersion]
+      })
+
+      it('returns no applicable charge versions', async () => {
+        const result = await SupplementaryService.go(regionId)
 
         expect(result.chargeVersions.length).to.equal(0)
       })
diff --git a/test/support/helpers/licence.helper.js b/test/support/helpers/licence.helper.js
index 8cd1b0fd0c..3993379630 100644
--- a/test/support/helpers/licence.helper.js
+++ b/test/support/helpers/licence.helper.js
@@ -18,8 +18,8 @@ class LicenceHelper {
    *
    * @returns {string} The ID of the newly created record
    */
-  static async add (data) {
-    const insertData = this._defaults(data)
+  static async add (data = {}) {
+    const insertData = this.defaults(data)
     const result = await db.table('water.licences')
       .insert(insertData)
       .returning('licence_id')
@@ -27,9 +27,10 @@ class LicenceHelper {
     return result
   }
 
-  static _defaults (data) {
+  static defaults (data = {}) {
     const defaults = {
-      licence_ref: '01/123'
+      licence_ref: '01/123',
+      region_id: 'bd114474-790f-4470-8ba4-7b0cc9c225d7'
     }
 
     return {
diff --git a/test/support/helpers/region.helper.js b/test/support/helpers/region.helper.js
new file mode 100644
index 0000000000..82ef857d2f
--- /dev/null
+++ b/test/support/helpers/region.helper.js
@@ -0,0 +1,44 @@
+'use strict'
+
+/**
+ * @module RegionHelper
+ */
+
+const { db } = require('../../../db/db')
+
+class RegionHelper {
+  /**
+   * Add a region
+   *
+   * If no `data` is provided, default values will be used. These are
+   *
+   * - `charge_region_id` - S
+   * - `nald_region_id` - 9
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   *
+   * @returns {string} The ID of the newly created record
+   */
+  static async add (data) {
+    const insertData = this._defaults(data)
+    const result = await db.table('water.regions')
+      .insert(insertData)
+      .returning('region_id')
+
+    return result
+  }
+
+  static _defaults (data) {
+    const defaults = {
+      charge_region_id: 'S',
+      nald_region_id: 9
+    }
+
+    return {
+      ...defaults,
+      ...data
+    }
+  }
+}
+
+module.exports = RegionHelper

From 979b805a9edbd215dfa914915b64ed517db47f75 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 15 Nov 2022 09:00:25 +0000
Subject: [PATCH 09/13] =?UTF-8?q?Adding=20Alan's=20sparkle=20=E2=9C=A8?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 app/services/test/find_region.service.js      | 19 +++++++++++----
 .../test/supplementary.service.test.js        |  4 ++--
 test/support/helpers/charge_version.helper.js | 24 ++++++++++++-------
 test/support/helpers/database.helper.js       |  4 ++++
 test/support/helpers/licence.helper.js        | 10 ++++++++
 test/support/helpers/region.helper.js         | 17 +++++++++----
 6 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/app/services/test/find_region.service.js b/app/services/test/find_region.service.js
index 4831d0d987..36bbbbd891 100644
--- a/app/services/test/find_region.service.js
+++ b/app/services/test/find_region.service.js
@@ -11,18 +11,29 @@ const { db } = require('../../../db/db')
  */
 
 class FindRegionService {
-  static async go (regionId) {
-    const region = await this._fetchRegion(regionId)
+  /**
+   * Returns the `region_id` for the matching record in `water.regions`
+   *
+   * > This is a temporary service added whilst developing the new SROC supplementary bill run functionality. We expect
+   * > the region ID to be provided by the UI as part of the normal workflow
+   *
+   * @param {string} naldRegionId The NALD region ID (a number between 1 to 9, 9 being the test region) for the region
+   * to find
+   *
+   * @returns {string} The region_id (a GUID) for the matching region
+   */
+  static async go (naldRegionId) {
+    const region = await this._fetchRegion(naldRegionId)
 
     return region
   }
 
-  static async _fetchRegion (regionId) {
+  static async _fetchRegion (naldRegionId) {
     const result = await db
       .select('region_id')
       .from('water.regions')
       .where({
-        nald_region_id: regionId
+        nald_region_id: naldRegionId
       })
       .first()
 
diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index 57cb1258ad..424f0f8de8 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -83,7 +83,7 @@ describe('Supplementary service', () => {
 
     describe('because there are no current charge versions (they all have end dates)', () => {
       beforeEach(async () => {
-        // This creates an SROC charge version with an edn date linked to a licence marked for supplementary billing
+        // This creates an SROC charge version with an end date linked to a licence marked for supplementary billing
         const alcsChargeVersion = await ChargeVersionHelper.add(
           { end_date: new Date(2022, 2, 1) }, // 2022-03-01 - Months are zero indexed :-)
           { include_in_supplementary_billing: 'yes' }
@@ -100,7 +100,7 @@ describe('Supplementary service', () => {
 
     describe('because there are no licences linked to the selected region', () => {
       beforeEach(async () => {
-        // This creates an SROC charge version linked to an invalid region
+        // This creates an SROC charge version linked to a licence with an different region than selected
         const otherRegionChargeVersion = await ChargeVersionHelper.add(
           {},
           {
diff --git a/test/support/helpers/charge_version.helper.js b/test/support/helpers/charge_version.helper.js
index 07eaf4f1e6..b66fb9cd48 100644
--- a/test/support/helpers/charge_version.helper.js
+++ b/test/support/helpers/charge_version.helper.js
@@ -29,7 +29,7 @@ class ChargeVersionHelper {
    */
   static async add (data = {}, licence = {}) {
     const licenceId = await this._addLicence(licence)
-    const insertData = this._defaults({ ...data, licence_id: licenceId })
+    const insertData = this.defaults({ ...data, licence_id: licenceId })
 
     const result = await db.table('water.charge_versions')
       .insert(insertData)
@@ -38,13 +38,15 @@ class ChargeVersionHelper {
     return result
   }
 
-  static async _addLicence (licence) {
-    const result = await LicenceHelper.add(licence)
-
-    return result[0].licenceId
-  }
-
-  static _defaults (data) {
+  /**
+   * Returns the defaults used when creating a new charge version
+   *
+   * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available
+   * for use in tests to avoid having to duplicate values.
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   */
+  static defaults (data = {}) {
     const defaults = {
       licence_ref: '01/123',
       scheme: 'sroc'
@@ -55,6 +57,12 @@ class ChargeVersionHelper {
       ...data
     }
   }
+
+  static async _addLicence (licence) {
+    const result = await LicenceHelper.add(licence)
+
+    return result[0].licenceId
+  }
 }
 
 module.exports = ChargeVersionHelper
diff --git a/test/support/helpers/database.helper.js b/test/support/helpers/database.helper.js
index d5f036d013..051c0d4dee 100644
--- a/test/support/helpers/database.helper.js
+++ b/test/support/helpers/database.helper.js
@@ -1,5 +1,9 @@
 'use strict'
 
+/**
+ * @module DatabaseHelper
+ */
+
 const { db } = require('../../../db/db.js')
 
 /**
diff --git a/test/support/helpers/licence.helper.js b/test/support/helpers/licence.helper.js
index 3993379630..577b0e178e 100644
--- a/test/support/helpers/licence.helper.js
+++ b/test/support/helpers/licence.helper.js
@@ -13,6 +13,7 @@ class LicenceHelper {
    * If no `data` is provided, default values will be used. These are
    *
    * - `licence_ref` - 01/123
+   * - `region_id` - bd114474-790f-4470-8ba4-7b0cc9c225d7
    *
    * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
    *
@@ -20,6 +21,7 @@ class LicenceHelper {
    */
   static async add (data = {}) {
     const insertData = this.defaults(data)
+
     const result = await db.table('water.licences')
       .insert(insertData)
       .returning('licence_id')
@@ -27,6 +29,14 @@ class LicenceHelper {
     return result
   }
 
+  /**
+   * Returns the defaults used when creating a new licence
+   *
+   * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available
+   * for use in tests to avoid having to duplicate values.
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   */
   static defaults (data = {}) {
     const defaults = {
       licence_ref: '01/123',
diff --git a/test/support/helpers/region.helper.js b/test/support/helpers/region.helper.js
index 82ef857d2f..c3282a7af5 100644
--- a/test/support/helpers/region.helper.js
+++ b/test/support/helpers/region.helper.js
@@ -8,7 +8,7 @@ const { db } = require('../../../db/db')
 
 class RegionHelper {
   /**
-   * Add a region
+   * Add a new region
    *
    * If no `data` is provided, default values will be used. These are
    *
@@ -19,8 +19,9 @@ class RegionHelper {
    *
    * @returns {string} The ID of the newly created record
    */
-  static async add (data) {
-    const insertData = this._defaults(data)
+  static async add (data = {}) {
+    const insertData = this.defaults(data)
+
     const result = await db.table('water.regions')
       .insert(insertData)
       .returning('region_id')
@@ -28,7 +29,15 @@ class RegionHelper {
     return result
   }
 
-  static _defaults (data) {
+  /**
+   * Returns the defaults used when creating a new region
+   *
+   * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available
+   * for use in tests to avoid having to duplicate values.
+   *
+   * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database
+   */
+  static defaults (data = {}) {
     const defaults = {
       charge_region_id: 'S',
       nald_region_id: 9

From 1e725790d3542c2088ab47361a4806dae9e2d3e5 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 15 Nov 2022 11:31:45 +0000
Subject: [PATCH 10/13] Tidy up of some of the code

---
 app/controllers/test/supplementary.controller.js       | 4 ++--
 app/services/test/supplementary.service.js             | 4 +---
 test/controllers/test/supplementary.controller.test.js | 5 ++++-
 test/services/test/supplementary.service.test.js       | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/app/controllers/test/supplementary.controller.js b/app/controllers/test/supplementary.controller.js
index 0c23826a1e..7cb54fca09 100644
--- a/app/controllers/test/supplementary.controller.js
+++ b/app/controllers/test/supplementary.controller.js
@@ -5,9 +5,9 @@ const SupplementaryService = require('../../services/test/supplementary.service.
 
 class SupplementaryController {
   static async index (request, h) {
-    const region = await FindRegionService.go(request.query.region)
+    const { regionId } = await FindRegionService.go(request.query.region)
 
-    const result = await SupplementaryService.go(region.regionId)
+    const result = await SupplementaryService.go(regionId)
 
     return h.response(result).code(200)
   }
diff --git a/app/services/test/supplementary.service.js b/app/services/test/supplementary.service.js
index 5b87a07919..934ca92af4 100644
--- a/app/services/test/supplementary.service.js
+++ b/app/services/test/supplementary.service.js
@@ -13,9 +13,7 @@ const { db } = require('../../../db/db')
 class SupplementaryService {
   static async go (regionId) {
     const chargeVersions = await this._fetchChargeVersions(regionId)
-    const response = {
-      chargeVersions
-    }
+    const response = { chargeVersions }
 
     return response
   }
diff --git a/test/controllers/test/supplementary.controller.test.js b/test/controllers/test/supplementary.controller.test.js
index 4a74cbe8f0..8016c55ab7 100644
--- a/test/controllers/test/supplementary.controller.test.js
+++ b/test/controllers/test/supplementary.controller.test.js
@@ -8,6 +8,9 @@ const Sinon = require('sinon')
 const { describe, it, beforeEach, after } = exports.lab = Lab.script()
 const { expect } = Code
 
+// Test helpers
+const LicenceHelper = require('../../support/helpers/licence.helper.js')
+
 // Things we need to stub
 const FindRegionService = require('../../../app/services/test/find_region.service')
 const SupplementaryService = require('../../../app/services/test/supplementary.service.js')
@@ -35,7 +38,7 @@ describe('Supplementary controller', () => {
     let response
 
     beforeEach(async () => {
-      Sinon.stub(FindRegionService, 'go').resolves({ regionId: 'bd114474-790f-4470-8ba4-7b0cc9c225d7' })
+      Sinon.stub(FindRegionService, 'go').resolves({ regionId: LicenceHelper.defaults().region_id })
       Sinon.stub(SupplementaryService, 'go').resolves({ chargeVersions: [] })
 
       response = await server.inject(options)
diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index 424f0f8de8..25490b4618 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -16,7 +16,7 @@ const LicenceHelper = require('../../support/helpers/licence.helper.js')
 const SupplementaryService = require('../../../app/services/test/supplementary.service.js')
 
 describe('Supplementary service', () => {
-  const regionId = LicenceHelper.defaults().region_id
+  const { region_id: regionId } = LicenceHelper.defaults()
   let testRecords
 
   beforeEach(async () => {

From 622b2c3e31d7b3a4cbbbd3ce83addd90ff271430 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 15 Nov 2022 11:32:54 +0000
Subject: [PATCH 11/13] Typo test/services/test/supplementary.service.test.js

Co-authored-by: Stuart Adair <43574728+StuAA78@users.noreply.github.com>
---
 test/services/test/supplementary.service.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index 25490b4618..f61c17d083 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -48,7 +48,7 @@ describe('Supplementary service', () => {
     })
   })
 
-  describe('when there are no licences to be included in supplimentery billing', () => {
+  describe('when there are no licences to be included in supplementary billing', () => {
     describe("because none of them are marked 'include_in_supplimentary_billing'", () => {
       beforeEach(async () => {
         // This creates an SROC charge version linked to a licence. But the licence won't be marked for supplementary

From 6c2ff0b832eff6548182dd39d3a84fa005f318a9 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 15 Nov 2022 11:33:06 +0000
Subject: [PATCH 12/13] Typo test/services/test/supplementary.service.test.js

Co-authored-by: Stuart Adair <43574728+StuAA78@users.noreply.github.com>
---
 test/services/test/supplementary.service.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index f61c17d083..d07e4ab07f 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -23,7 +23,7 @@ describe('Supplementary service', () => {
     await DatabaseHelper.clean()
   })
 
-  describe('when there are licences to be included in supplimentery billing', () => {
+  describe('when there are licences to be included in supplementary billing', () => {
     beforeEach(async () => {
       // This creates an SROC charge version linked to a licence marked for supplementary billing
       const srocChargeVersion = await ChargeVersionHelper.add(

From a64d5673ef1c4652a7e556467500b39705926783 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Tue, 15 Nov 2022 11:33:23 +0000
Subject: [PATCH 13/13] Typo test/services/test/supplementary.service.test.js

Co-authored-by: Stuart Adair <43574728+StuAA78@users.noreply.github.com>
---
 test/services/test/supplementary.service.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/services/test/supplementary.service.test.js b/test/services/test/supplementary.service.test.js
index d07e4ab07f..6aefd91321 100644
--- a/test/services/test/supplementary.service.test.js
+++ b/test/services/test/supplementary.service.test.js
@@ -49,7 +49,7 @@ describe('Supplementary service', () => {
   })
 
   describe('when there are no licences to be included in supplementary billing', () => {
-    describe("because none of them are marked 'include_in_supplimentary_billing'", () => {
+    describe("because none of them are marked 'include_in_supplementary_billing'", () => {
       beforeEach(async () => {
         // This creates an SROC charge version linked to a licence. But the licence won't be marked for supplementary
         // billing