Fix display of credit transactions in bill views (#522)

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

In testing, we've spotted that the display of credit values on certain pages has gone awry!

The first issue is how credit values are displayed.

In most cases, we'll display `£150.25 credit` which is correct. But there are times, mainly in tables, where we need to show the signed version, for example `-£150.25`. A change to clean up our money formatters in `BasePresenter` broke this. So now negative values in the licence summaries table and the transaction table's total row are always displayed as positive.

In this change, we update the formatter to allow callers to specify whether a sign should be returned. It also fixes all the JSDoc comments we forgot to update when we made the original change. Doh!

The second issue is about _when_ we show credit values. During the initial build of the screens, we encountered some errors locally. We realised that bills sourced from NALD were missing data. When we checked the legacy code we found it only displayed credits when the bill run type was `supplementary` _and_ its source was `wrls`. When we did the same the errors went away.

During QA it's been spotted, rightly, that this means the views can appear inconsistent sometimes showing the credits column and sometimes not. Without knowing why we're doing it, it just looks 'broken'.

So, we dug deeper and realised that the key difference between bills from NALD and those created in WRLS is that the `invoiceValue` and `creditNoteValue` columns are empty for NALD ones. All we get is the `netAmount`. Knowing that we can change how we determine the credit and debit total values and not cause an error. So, the second change is to _always_ show the credits column for supplementary bills but to add a little 'dev magic' to handle how we calculate the totals!

app/presenters/base.presenter.js

@@ -118,22 +118,32 @@ function formatLongDateTime (date) {
 /**
  * Formats a value in pence as a money string with commas, for example, 12776805 as '£127,768.05'
  *
- * As this is intended for showing values in the UI if the value is a negative it will be made a positive before then
- * being formatted. This is because the UI shows credits as '£127,768.05 credit' rather than '£-127,768.05'.
+ * This is intended for use when formatting values to be displayed in the UI. In most cases if the value is a negative
+ * it will be made a positive before then being formatted. This is because the UI generally shows credits as
+ * '£127,768.05 credit' rather than '-£127,768.05'.
+ *
+ * But there are times when we need to show credits with a sign instead. In those cases you can pass the optional
+ * `signed` flag to have the value returned with a sign.
  *
  * > Credit to https://stackoverflow.com/a/32154217/6117745 for showing numbers with commas
  *
- * @param {Number} value The value to display as currency. Assumed to be in pounds
- * @param {Boolean} includeSymbol Whether to add the £ symbol to the start of the returned string
+ * @param {Number} valueInPence The value (in pence) to display as money
+ * @param {Boolean} signed Whether to add the - sign to the start of the returned string if valueInPence is negative
  *
- * @returns {string} The value formatted as a money string with commas with optional currency symbol
+ * @returns {string} The value formatted as a money string with commas and currency symbol plus optional sign
  */
-function formatMoney (valueInPence) {
-  // Even though we store signed values (which you should never do!) we don't display them in the UI. Argh!!!
+function formatMoney (valueInPence, signed = false) {
+  // NOTE: The legacy DB stores values signed (which you should never do!) We always abs the valueInPence for 2 reasons
+  //
+  // - in most cases we display credits as £127,768.05 credit
+  // - if we call `toLocaleString()` on a negative number we'll get a signed number back resulting in £-127,768.05
   const positiveValueInPence = Math.abs(valueInPence)
   const positiveValueInPounds = convertPenceToPounds(positiveValueInPence)
 
-  return `£${positiveValueInPounds.toLocaleString('en-GB', { minimumFractionDigits: 2, maximumFractionDigits: 2 })}`
+  // Determine if we should add a sign (the caller said they want one and the original value in pence was negative)
+  const sign = signed && valueInPence < 0 ? '-' : ''
+
+  return `${sign}£${positiveValueInPounds.toLocaleString('en-GB', { minimumFractionDigits: 2, maximumFractionDigits: 2 })}`
 }
 
 /**

app/presenters/bill-licences/view-bill-licence.presenter.js

@@ -42,9 +42,9 @@ function go (billLicence) {
 }
 
 function _displayCreditDebitTotals (billRun) {
-  const { batchType, source } = billRun
+  const { batchType } = billRun
 
-  return batchType === 'supplementary' && source === 'wrls'
+  return batchType === 'supplementary'
 }
 
 function _tableCaption (transactions) {
@@ -76,7 +76,7 @@ function _totals (transactions) {
   return {
     creditTotal: formatMoney(creditTotal),
     debitTotal: formatMoney(debitTotal),
-    total: formatMoney(total)
+    total: formatMoney(total, true)
   }
 }
 

app/presenters/bills/bill.presenter.js

@@ -28,9 +28,9 @@ function go (bill, billingAccount) {
     chargeScheme: _scheme(billRun),
     contactName: _contactName(billingAccount),
     credit: bill.isCredit,
-    creditsTotal: formatMoney(bill.creditNoteValue),
+    creditsTotal: _creditsTotal(bill, billRun),
     dateCreated: formatLongDate(bill.createdAt),
-    debitsTotal: formatMoney(bill.invoiceValue),
+    debitsTotal: _debitsTotal(bill, billRun),
     deminimis: bill.isDeMinimis,
     displayCreditDebitTotals: _displayCreditDebitTotals(billRun),
     financialYear: _financialYear(billRun),
@@ -98,10 +98,40 @@ function _contactName (billingAccount) {
   return null
 }
 
+function _creditsTotal (bill, billRun) {
+  const { creditNoteValue, netAmount } = bill
+  const { source } = billRun
+
+  if (source === 'wrls') {
+    return formatMoney(creditNoteValue)
+  }
+
+  if (netAmount < 0) {
+    return formatMoney(netAmount)
+  }
+
+  return '£0.00'
+}
+
+function _debitsTotal (bill, billRun) {
+  const { invoiceValue, netAmount } = bill
+  const { source } = billRun
+
+  if (source === 'wrls') {
+    return formatMoney(invoiceValue)
+  }
+
+  if (netAmount > 0) {
+    return formatMoney(netAmount)
+  }
+
+  return '£0.00'
+}
+
 function _displayCreditDebitTotals (billRun) {
-  const { batchType, source } = billRun
+  const { batchType } = billRun
 
-  return batchType === 'supplementary' && source === 'wrls'
+  return batchType === 'supplementary'
 }
 
 function _financialYear (billRun) {

app/presenters/bills/licence-summaries.presenter.js

@@ -25,7 +25,7 @@ function _billLicences (licenceSummaries) {
     return {
       id,
       reference,
-      total: formatMoney(total)
+      total: formatMoney(total, true)
     }
   })
 }

client/sass/application.scss

@@ -115,5 +115,6 @@ $govuk-global-styles: true;
   @media (min-width: 768px) {
     border-top: solid 2px #0b0c0c !important;
     border-bottom: none !important;
+    white-space: nowrap;
   }
 }

test/presenters/base.presenter.test.js

@@ -192,10 +192,24 @@ describe('Base presenter', () => {
     })
 
     describe('when the value in pence is negative', () => {
-      it('correctly returns the value as a positive money string with commas and a symbol, for example, £1,149.50', async () => {
-        const result = BasePresenter.formatMoney(valueInPence, true)
+      beforeEach(() => {
+        valueInPence = -114950
+      })
 
-        expect(result).to.equal('£1,149.50')
+      describe("and we do not override the default parameter 'signed'", () => {
+        it('correctly returns the value as a positive money string with commas and a symbol, for example, £1,149.50', async () => {
+          const result = BasePresenter.formatMoney(valueInPence)
+
+          expect(result).to.equal('£1,149.50')
+        })
+      })
+
+      describe("and we override the default parameter 'signed'", () => {
+        it('correctly returns the value as a positive money string with commas and a symbol, for example, -£1,149.50', async () => {
+          const result = BasePresenter.formatMoney(valueInPence, true)
+
+          expect(result).to.equal('-£1,149.50')
+        })
       })
     })
   })

test/presenters/bill-licences/view-bill-licence.presenter.test.js

@@ -46,29 +46,10 @@ describe('View Bill Licence presenter', () => {
       })
 
       describe('when the bill run is supplementary', () => {
-        describe('and was created in WRLS', () => {
-          beforeEach(() => {
-            billLicence.bill.billRun.batchType = 'supplementary'
-          })
-
-          it('returns true', () => {
-            const result = ViewBillLicencePresenter.go(billLicence)
-
-            expect(result.displayCreditDebitTotals).to.be.true()
-          })
-        })
-
-        describe('but was created in NALD', () => {
-          beforeEach(() => {
-            billLicence.bill.billRun.batchType = 'supplementary'
-            billLicence.bill.billRun.source = 'nald'
-          })
-
-          it('returns false', () => {
-            const result = ViewBillLicencePresenter.go(billLicence)
+        it('returns true', () => {
+          const result = ViewBillLicencePresenter.go(billLicence)
 
-            expect(result.displayCreditDebitTotals).to.be.false()
-          })
+          expect(result.displayCreditDebitTotals).to.be.true()
         })
       })
     })
@@ -95,30 +76,68 @@ describe('View Bill Licence presenter', () => {
       })
     })
 
-    it('correctly presents the data', () => {
-      const result = ViewBillLicencePresenter.go(billLicence)
-
-      // NOTE: The transaction details we pass in and what we get back is not what would actually happen. Our
-      // transaction presenter tests exhaust what we expect back for all scenarios. What we are confirming though is
-      // that depending on a transaction's charge type ViewBillLicencePresenter will call the relevant transaction
-      // presenter. Plus, that things like the totals and the table caption is returned as expected.
-      expect(result).to.equal({
-        accountNumber: 'W88898987A',
-        billingInvoiceId: '5a5b313b-e707-490a-a693-799339941e4f',
-        creditTotal: '£10.00',
-        debitTotal: '£298.37',
-        displayCreditDebitTotals: true,
-        licenceId: '2eaa831d-7bd6-4b0a-aaf1-3aacafec6bf2',
-        licenceRef: 'WA/055/0017/013',
-        scheme: 'alcs',
-        tableCaption: '4 transactions',
-        total: '£288.37',
-        transactions: [
-          { chargeType: 'standard' },
-          { chargeType: 'standard' },
-          { chargeType: 'compensation' },
-          { chargeType: 'minimum_charge' }
-        ]
+    describe('and the total for the transactions is a debit', () => {
+      it('correctly presents the data', () => {
+        const result = ViewBillLicencePresenter.go(billLicence)
+
+        // NOTE: The transaction details we pass in and what we get back is not what would actually happen. Our
+        // transaction presenter tests exhaust what we expect back for all scenarios. What we are confirming though is
+        // that depending on a transaction's charge type ViewBillLicencePresenter will call the relevant transaction
+        // presenter. Plus, that things like the totals and the table caption is returned as expected.
+        expect(result).to.equal({
+          accountNumber: 'W88898987A',
+          billingInvoiceId: '5a5b313b-e707-490a-a693-799339941e4f',
+          creditTotal: '£10.00',
+          debitTotal: '£298.37',
+          displayCreditDebitTotals: true,
+          licenceId: '2eaa831d-7bd6-4b0a-aaf1-3aacafec6bf2',
+          licenceRef: 'WA/055/0017/013',
+          scheme: 'alcs',
+          tableCaption: '4 transactions',
+          total: '£288.37',
+          transactions: [
+            { chargeType: 'standard' },
+            { chargeType: 'standard' },
+            { chargeType: 'compensation' },
+            { chargeType: 'minimum_charge' }
+          ]
+        })
+      })
+    })
+
+    describe('and the total for the transactions is a credit', () => {
+      beforeEach(() => {
+        billLicence.transactions[0].isCredit = true
+        billLicence.transactions[0].netAmount = -29837
+        billLicence.transactions[1].isCredit = false
+        billLicence.transactions[1].netAmount = 1000
+      })
+
+      it('correctly presents the data', () => {
+        const result = ViewBillLicencePresenter.go(billLicence)
+
+        // NOTE: The transaction details we pass in and what we get back is not what would actually happen. Our
+        // transaction presenter tests exhaust what we expect back for all scenarios. What we are confirming though is
+        // that depending on a transaction's charge type ViewBillLicencePresenter will call the relevant transaction
+        // presenter. Plus, that things like the totals and the table caption is returned as expected.
+        expect(result).to.equal({
+          accountNumber: 'W88898987A',
+          billingInvoiceId: '5a5b313b-e707-490a-a693-799339941e4f',
+          creditTotal: '£298.37',
+          debitTotal: '£10.00',
+          displayCreditDebitTotals: true,
+          licenceId: '2eaa831d-7bd6-4b0a-aaf1-3aacafec6bf2',
+          licenceRef: 'WA/055/0017/013',
+          scheme: 'alcs',
+          tableCaption: '4 transactions',
+          total: '-£288.37',
+          transactions: [
+            { chargeType: 'standard' },
+            { chargeType: 'standard' },
+            { chargeType: 'compensation' },
+            { chargeType: 'minimum_charge' }
+          ]
+        })
       })
     })
   })