Fix licence issues failing to insert for review (#867)

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

In recent testing of triggering an SROC two-part tariff bill run, we've seen errors thrown by the `PersistAllocatedLicenceToResultsService`.

After investigating it is not the issue. It was being asked to persist every issue for every element and return found by `DetermineLicenceIssuesService` in its `issues` field.

No de-duping of the issues is taking place so it's blowing past the 255-char limit for the DB field for some licences.

This causes the bill run to fail and the system app to crash (but we'll deal with that in another fix!)

This change will de-dupe the issues found _before_ they are persisted to keep the entry to a minimum. But should a licence have every single issue the resulting value would still be more than 255 characters. 

So, we also add a migration to alter the `issues` column in `review_licences`, `review_charge_elements` and `review_returns` from `varchar(255)` to `text` which has no limit (in practical terms).

app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js

@@ -1,16 +1,29 @@
 'use strict'
 
 /**
- * Determines the issues on a licences for a two-part tariff bill run
+ * Determines the issues on a licence for a two-part tariff bill run
  * @module DetermineLicenceIssuesService
  */
 
 const { twoPartTariffReviewIssues } = require('../../../lib/static-lookups.lib.js')
 
 /**
- * Determines the issues on a licence charge elements and return logs and sets the status based on them
+ * Determines the issues on a licence for a two-part tariff bill run
  *
- * @param {module:LicenceModel} licence - the two-part tariff licence included in the bill run
+ * The issues we determine are not related directly to the licence. They are with either the returns or the charge
+ * elements linked to the licence.
+ *
+ * But the review screens in the UI work from the context of a licence and the main page needs to show whether a licence
+ * has any issues and whether it is in 'review'. Only when a user clicks through to look at a the results for a licence
+ * will they see which of its returns and charge elements have the issues.
+ *
+ * If only one issue is found we still assign it to the licence (an the charge element or return in question) for later
+ * persisting in a `ReviewLicence` record. We also check the issue against a list of those that will flag the licence
+ * for review. Should a licence have multiple issues we flag it for review regardless.
+ *
+ * > No result is returned. The issues are directly assigned to the licence and its relevant properties
+ *
+ * @param {module:LicenceModel} licence - The two-part tariff licence to determine issues for
  */
 async function go (licence) {
   const { returnLogs: licenceReturnLogs, chargeVersions } = licence
@@ -19,7 +32,7 @@ async function go (licence) {
   const allElementIssues = _determineChargeElementsIssues(chargeVersions, licenceReturnLogs)
 
   licence.status = _determineLicenceStatus(allElementIssues, allReturnIssues)
-  licence.issues = [...allElementIssues, ...allReturnIssues]
+  licence.issues = _licenceIssues(allElementIssues, allReturnIssues)
 }
 
 function _determineChargeElementsIssues (chargeVersions, licenceReturnLogs) {
@@ -155,6 +168,13 @@ function _getReviewStatuses () {
   return reviewStatuses
 }
 
+function _licenceIssues (allElementIssues, allReturnIssues) {
+  const allIssues = [...allElementIssues, ...allReturnIssues]
+  const uniqueIssues = new Set(allIssues)
+
+  return [...uniqueIssues].sort()
+}
+
 function _returnLogIssues (returnLog, licence) {
   const returnLogIssues = []
 

db/migrations/public/20240327200101_alter-review-issues-columns.js

@@ -0,0 +1,41 @@
+'use strict'
+
+exports.up = async function (knex) {
+  await knex
+    .schema
+    .alterTable('review_charge_elements', (table) => {
+      table.text('issues').alter()
+    })
+
+  await knex
+    .schema
+    .alterTable('review_licences', (table) => {
+      table.text('issues').alter()
+    })
+
+  return knex
+    .schema
+    .alterTable('review_returns', (table) => {
+      table.text('issues').alter()
+    })
+}
+
+exports.down = async function (knex) {
+  await knex
+    .schema
+    .alterTable('review_charge_elements', (table) => {
+      table.string('issues').alter()
+    })
+
+  await knex
+    .schema
+    .alterTable('review_licences', (table) => {
+      table.string('issues').alter()
+    })
+
+  return knex
+    .schema
+    .alterTable('review_returns', (table) => {
+      table.string('issues').alter()
+    })
+}

test/services/bill-runs/two-part-tariff/determine-licence-issues.service.test.js

@@ -19,13 +19,23 @@ describe('Determine Licence Issues Service', () => {
         licence = _generateMultipleIssuesLicenceData()
       })
 
-      describe('on the returns', () => {
-        it('sets all the issues on the returns object', () => {
+      describe('on the licence', () => {
+        it("sets 'issues' to a comma separated unique list in alphabetical order of the issues found", () => {
           DetermineLicenceIssuesService.go(licence)
 
-          expect(licence.returnLogs[0].issues).to.equal(['Abstraction outside period', 'Checking query', 'Over abstraction', 'Returns received late', 'Return split over charge references'])
-          expect(licence.returnLogs[1].issues).to.equal(['No returns received'])
-          expect(licence.returnLogs[2].issues).to.equal(['Returns received but not processed'])
+          expect(licence.issues).to.equal([
+            'Abstraction outside period',
+            'Aggregate',
+            'Checking query',
+            'No returns received',
+            'Over abstraction',
+            'Overlap of charge dates',
+            'Return split over charge references',
+            'Returns received but not processed',
+            'Returns received late',
+            'Some returns not received',
+            'Unable to match return'
+          ])
         })
 
         it("sets the status of the licence to 'review'", () => {
@@ -35,18 +45,22 @@ describe('Determine Licence Issues Service', () => {
         })
       })
 
-      describe('on the charge elements', () => {
-        it('sets all the issues on the element object', () => {
+      describe('on the returns', () => {
+        it('sets all the issues on the returns object', () => {
           DetermineLicenceIssuesService.go(licence)
 
-          expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].issues).to.equal(['Aggregate', 'Overlap of charge dates', 'Some returns not received'])
-          expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[1].issues).to.equal(['Aggregate', 'Unable to match return'])
+          expect(licence.returnLogs[0].issues).to.equal(['Abstraction outside period', 'Checking query', 'Over abstraction', 'Returns received late', 'Return split over charge references'])
+          expect(licence.returnLogs[1].issues).to.equal(['No returns received'])
+          expect(licence.returnLogs[2].issues).to.equal(['Returns received but not processed'])
         })
+      })
 
-        it("sets the status of the licence to 'review'", () => {
+      describe('on the charge elements', () => {
+        it('sets all the issues on the element object', () => {
           DetermineLicenceIssuesService.go(licence)
 
-          expect(licence.status).to.equal('review')
+          expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].issues).to.equal(['Aggregate', 'Overlap of charge dates', 'Some returns not received'])
+          expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[1].issues).to.equal(['Aggregate', 'Unable to match return'])
         })
 
         it("sets the status of the charge element to 'review'", () => {