Make test data helpers more 'unique' (#452)

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

We've been investigating changing our unit test framework recently (see [Investigate switching to jest](#430) and [water-abstraction-ava](https://github.com/DEFRA/water-abstraction-ava)).

A key issue we'll have if we switch is that most other frameworks run tests asynchronously. This means our pattern of clean-setup-assert won't work. At best we can clean before any tests run but then the test data we set up must not clash.

To achieve that we need to make the test data the helpers generate as unique as possible, which means moving away from hard-coded values.

We're still playing with frameworks but there is nothing stopping us from making this change now ahead of a decision.

So, this change covers making the helpers generate more unique and fixing any tests that break.

** Notes

- Add randomInteger() to new GeneralHelper

Where a helper is hard-coding a UUID for a field is going to be easy to make it unique; we just call our `GeneralLib.generateUUID()` function.

But many helpers also need to generate references, licence numbers being a good example. If we are going to make these unique we're going to need a means to generate random numbers. Often they need to be within a certain range, for example, 100 to 999 because the number must be a certain number of digits long.

So, we're adding a new `randomInteger()` function which can handle this requirement. Doing so forced us to make a decision as to where to put it. We decided to create a `GeneralHelper` module that can be used going forward for any shared functionality in the helpers.

app/services/billing/supplementary/fetch-charge-versions.service.js

@@ -130,9 +130,10 @@ async function _fetch (regionId, billingPeriod) {
  * We are interested in the associated licences to ensure that their supplementary billing flag is unset.
  */
 function _extractLicenceIdsThenRemoveNonChargeableChargeVersions (allChargeVersions) {
-  const licenceIdsForPeriod = []
   const chargeVersions = []
 
+  let licenceIdsForPeriod = []
+
   for (const chargeVersion of allChargeVersions) {
     licenceIdsForPeriod.push(chargeVersion.licence.licenceId)
 
@@ -141,6 +142,11 @@ function _extractLicenceIdsThenRemoveNonChargeableChargeVersions (allChargeVersi
     }
   }
 
+  // NOTE: If a licence appears multiple times in the results it will be pushed multiple times into the array. We have
+  // found a handy way to de-dupe an array of values is to create a new Set and then destructure it back to an array
+  // of values.
+  licenceIdsForPeriod = [...new Set(licenceIdsForPeriod)]
+
   return { chargeVersions, licenceIdsForPeriod }
 }
 

test/presenters/charging-module/create-transaction.presenter.test.js

@@ -27,14 +27,15 @@ describe('Charging Module Create Transaction presenter', () => {
 
   let transaction
   let licence
+  let region
 
   beforeEach(async () => {
     await DatabaseHelper.clean()
   })
 
   describe('when provided with a Transaction and Licence instance', () => {
     beforeEach(async () => {
-      const region = await RegionHelper.add()
+      region = await RegionHelper.add()
 
       // NOTE: In the context the presenter is used it is from a Licence instance returned by
       // FetchChargeVersionsService. We recreate how that instance is formed here, including extracting some of the
@@ -65,6 +66,7 @@ describe('Charging Module Create Transaction presenter', () => {
       // standard transaction instance and amend some of the properties to match what FormatSrocTransactionLineService
       // does.
       transaction = await TransactionHelper.add()
+      transaction.chargeCategoryCode = '4.5.6'
       transaction.section127Agreement = false
       transaction.section130Agreement = false
     })
@@ -90,10 +92,10 @@ describe('Charging Module Create Transaction presenter', () => {
       expect(result.chargePeriod).to.equal('01-APR-2022 - 31-MAR-2023')
       expect(result.compensationCharge).to.equal(false)
       expect(result.customerReference).to.equal(invoiceAccountNumber)
-      expect(result.licenceNumber).to.equal('01/123')
+      expect(result.licenceNumber).to.equal(licence.licenceRef)
       expect(result.lineDescription).to.equal('Water abstraction charge: Agriculture other than spray irrigation at East Rudham')
       expect(result.loss).to.equal('medium')
-      expect(result.region).to.equal('S')
+      expect(result.region).to.equal(region.chargeRegionId)
       expect(result.regionalChargingArea).to.equal('Southern')
       expect(result.section127Agreement).to.equal(false)
       expect(result.section130Agreement).to.equal(false)

test/services/billing-accounts/change-address.service.test.js

@@ -27,13 +27,20 @@ const ChangeAddressService = require('../../../app/services/billing-accounts/cha
 
 describe('Change address service', () => {
   const addressFromLookup = {
-    addressLine1: 'NATURAL ENGLAND', addressLine2: 'HORIZON HOUSE', town: 'BRISTOL', postcode: 'BS1 5AH', uprn: 340116
+    addressLine1: 'NATURAL ENGLAND',
+    addressLine2: 'HORIZON HOUSE',
+    town: 'BRISTOL',
+    postcode: 'BS1 5AH',
+    uprn: AddressHelper.generateUprn()
   }
   const addressFromManual = {
     addressLine1: '62 High St', town: 'Harpenden', postcode: 'AL5 2SP'
   }
   const companyCompaniesHouse = {
-    type: 'organisation', name: 'SCP Foundation', companyNumber: '04296934', organisationType: 'limitedCompany'
+    type: 'organisation',
+    name: 'SCP Foundation',
+    companyNumber: CompanyHelper.generateCompanyNumber(),
+    organisationType: 'limitedCompany'
   }
   const contactDepartment = {
     type: 'department', department: 'Humanoid Risk Assessment'
@@ -90,7 +97,8 @@ describe('Change address service', () => {
           let existingAddress
 
           beforeEach(async () => {
-            existingAddress = await AddressHelper.add()
+            const { uprn } = address
+            existingAddress = await AddressHelper.add({ uprn })
           })
 
           it('overwrites the existing address with the latest OS Places details', async () => {
@@ -100,7 +108,7 @@ describe('Change address service', () => {
 
             expect(reFetchedExistingAddress.addressId).to.equal(existingAddress.addressId)
             expect(reFetchedExistingAddress.createdAt).to.equal(existingAddress.createdAt)
-            expect(reFetchedExistingAddress.address1).to.equal('NATURAL ENGLAND')
+            expect(reFetchedExistingAddress.address1).to.equal(address.addressLine1)
             expect(reFetchedExistingAddress.updatedAt).not.to.equal(existingAddress.updatedAt)
           })
 
@@ -118,8 +126,8 @@ describe('Change address service', () => {
             const result = await AddressModel.query()
 
             expect(result.length).to.equal(1)
-            expect(result[0].address1).to.equal('NATURAL ENGLAND')
-            expect(result[0].uprn).to.equal(340116)
+            expect(result[0].address1).to.equal(address.addressLine1)
+            expect(result[0].uprn).to.equal(address.uprn)
           })
 
           it('links the invoice account address record to the new address', async () => {
@@ -193,7 +201,8 @@ describe('Change address service', () => {
         let existingCompany
 
         beforeEach(async () => {
-          existingCompany = await CompanyHelper.add()
+          const { companyNumber } = agentCompany
+          existingCompany = await CompanyHelper.add({ companyNumber })
         })
 
         it('overwrites the existing company with the latest Companies House details', async () => {
@@ -203,7 +212,7 @@ describe('Change address service', () => {
 
           expect(reFetchedExistingCompany.companyId).to.equal(existingCompany.companyId)
           expect(reFetchedExistingCompany.createdAt).to.equal(existingCompany.createdAt)
-          expect(reFetchedExistingCompany.name).to.equal('SCP Foundation')
+          expect(reFetchedExistingCompany.name).to.equal(agentCompany.name)
           expect(reFetchedExistingCompany.updatedAt).not.to.equal(existingCompany.updatedAt)
         })
 
@@ -221,8 +230,8 @@ describe('Change address service', () => {
           const result = await CompanyModel.query()
 
           expect(result.length).to.equal(1)
-          expect(result[0].name).to.equal('SCP Foundation')
-          expect(result[0].companyNumber).to.equal('04296934')
+          expect(result[0].name).to.equal(agentCompany.name)
+          expect(result[0].companyNumber).to.equal(agentCompany.companyNumber)
         })
 
         it('links the invoice account address record to the new company', async () => {

test/services/billing/fetch-region.service.test.js

@@ -24,7 +24,7 @@ describe('Fetch Region service', () => {
 
   describe('when there is a region with a matching NALD region id', () => {
     beforeEach(async () => {
-      testRegion = await RegionHelper.add()
+      testRegion = await RegionHelper.add({ naldRegionId })
     })
 
     it('returns results', async () => {
@@ -36,7 +36,7 @@ describe('Fetch Region service', () => {
 
   describe('when there is no region with a matching NALD region id', () => {
     beforeEach(async () => {
-      RegionHelper.add({ naldRegionId: 99 })
+      testRegion = await RegionHelper.add({ naldRegionId: 99 })
     })
 
     it('returns no results', async () => {

test/services/billing/supplementary/fetch-charge-versions.service.test.js

@@ -52,39 +52,41 @@ describe('Fetch Charge Versions service', () => {
         startDate: new Date('2023-04-01'),
         endDate: new Date('2024-03-31')
       }
-
       licence = await LicenceHelper.add({
         regionId,
         isWaterUndertaker: true,
         includeInSrocSupplementaryBilling: true,
         includeInSupplementaryBilling: 'yes'
       })
-      const { licenceId } = licence
       changeReason = await ChangeReasonHelper.add({ triggersMinimumCharge: true })
 
+      const { licenceId, licenceRef } = licence
+      const { changeReasonId } = changeReason
+      const invoiceAccountId = '77483323-daec-443e-912f-b87e1e9d0721'
+
       // This creates a 'current' SROC charge version which covers only FYE 2024
       const sroc2024ChargeVersion = await ChargeVersionHelper.add(
-        { startDate: new Date('2023-11-01'), changeReasonId: changeReason.changeReasonId, licenceId }
+        { startDate: new Date('2023-11-01'), changeReasonId, invoiceAccountId, licenceId, licenceRef }
       )
 
       // This creates a 'current' SROC charge version which covers both FYE 2023 and 2024
       const sroc2023And24ChargeVersion = await ChargeVersionHelper.add(
-        { endDate: new Date('2023-10-31'), changeReasonId: changeReason.changeReasonId, licenceId }
+        { endDate: new Date('2023-10-31'), changeReasonId, invoiceAccountId, licenceId, licenceRef }
       )
 
       // This creates a 'current' SROC charge version which covers only FYE 2023
       const sroc2023ChargeVersion = await ChargeVersionHelper.add(
-        { endDate: new Date('2022-10-31'), changeReasonId: changeReason.changeReasonId, licenceId }
+        { endDate: new Date('2022-10-31'), changeReasonId, invoiceAccountId, licenceId, licenceRef }
       )
 
       // This creates a 'superseded' SROC charge version
       const srocSupersededChargeVersion = await ChargeVersionHelper.add(
-        { changeReasonId: changeReason.changeReasonId, status: 'superseded', licenceId }
+        { changeReasonId, status: 'superseded', invoiceAccountId, licenceId, licenceRef }
       )
 
       // This creates an ALCS (presroc) charge version
       const alcsChargeVersion = await ChargeVersionHelper.add(
-        { scheme: 'alcs', licenceId }
+        { scheme: 'alcs', invoiceAccountId, licenceId, licenceRef }
       )
 
       testRecords = [
@@ -136,37 +138,44 @@ describe('Fetch Charge Versions service', () => {
         const result = await FetchChargeVersionsService.go(regionId, billingPeriod)
 
         expect(result.chargeVersions).to.have.length(4)
-        expect(result.chargeVersions[0].chargeVersionId).to.equal(testRecords[0].chargeVersionId)
-        expect(result.chargeVersions[1].chargeVersionId).to.equal(testRecords[1].chargeVersionId)
-        expect(result.chargeVersions[2].chargeVersionId).to.equal(testRecords[2].chargeVersionId)
-        expect(result.chargeVersions[3].chargeVersionId).to.equal(testRecords[3].chargeVersionId)
+
+        const chargeVersionIds = result.chargeVersions.map((chargeVersion) => {
+          return chargeVersion.chargeVersionId
+        })
+
+        expect(chargeVersionIds).to.include(testRecords[0].chargeVersionId)
+        expect(chargeVersionIds).to.include(testRecords[1].chargeVersionId)
+        expect(chargeVersionIds).to.include(testRecords[2].chargeVersionId)
+        expect(chargeVersionIds).to.include(testRecords[3].chargeVersionId)
       })
 
-      it('returns the licenceIds from SROC charge versions that are applicable', async () => {
+      it('returns a unique list of licenceIds from SROC charge versions that are applicable', async () => {
         const result = await FetchChargeVersionsService.go(regionId, billingPeriod)
 
-        expect(result.licenceIdsForPeriod).to.have.length(4)
+        expect(result.licenceIdsForPeriod).to.have.length(1)
         expect(result.licenceIdsForPeriod[0]).to.equal(licence.licenceId)
-        expect(result.licenceIdsForPeriod[1]).to.equal(licence.licenceId)
-        expect(result.licenceIdsForPeriod[2]).to.equal(licence.licenceId)
-        expect(result.licenceIdsForPeriod[3]).to.equal(licence.licenceId)
       })
     })
 
     it("returns both 'current' and 'superseded' SROC charge version that are applicable", async () => {
       const result = await FetchChargeVersionsService.go(regionId, billingPeriod)
 
       expect(result.chargeVersions).to.have.length(4)
-      expect(result.chargeVersions[0].chargeVersionId).to.equal(testRecords[0].chargeVersionId)
-      expect(result.chargeVersions[1].chargeVersionId).to.equal(testRecords[1].chargeVersionId)
-      expect(result.chargeVersions[2].chargeVersionId).to.equal(testRecords[2].chargeVersionId)
-      expect(result.chargeVersions[3].chargeVersionId).to.equal(testRecords[3].chargeVersionId)
+
+      const chargeVersionIds = result.chargeVersions.map((chargeVersion) => {
+        return chargeVersion.chargeVersionId
+      })
+
+      expect(chargeVersionIds).to.include(testRecords[0].chargeVersionId)
+      expect(chargeVersionIds).to.include(testRecords[1].chargeVersionId)
+      expect(chargeVersionIds).to.include(testRecords[2].chargeVersionId)
+      expect(chargeVersionIds).to.include(testRecords[3].chargeVersionId)
     })
 
     it('includes the related licence and region', async () => {
       const result = await FetchChargeVersionsService.go(regionId, billingPeriod)
 
-      expect(result.chargeVersions[0].licence.licenceRef).to.equal(licenceDefaults.licenceRef)
+      expect(result.chargeVersions[0].licence.licenceRef).to.equal(licence.licenceRef)
       expect(result.chargeVersions[0].licence.isWaterUndertaker).to.equal(true)
       expect(result.chargeVersions[0].licence.historicalAreaCode).to.equal(licenceDefaults.regions.historicalAreaCode)
       expect(result.chargeVersions[0].licence.regionalChargeArea).to.equal(licenceDefaults.regions.regionalChargeArea)