diff --git a/app/presenters/base.presenter.js b/app/presenters/base.presenter.js index 659e35b7c3..eb8471824b 100644 --- a/app/presenters/base.presenter.js +++ b/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 })}` } /** diff --git a/app/presenters/bill-licences/view-bill-licence.presenter.js b/app/presenters/bill-licences/view-bill-licence.presenter.js index 30c85abfac..2003193d92 100644 --- a/app/presenters/bill-licences/view-bill-licence.presenter.js +++ b/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) } } diff --git a/app/presenters/bills/bill.presenter.js b/app/presenters/bills/bill.presenter.js index 0e045fffd0..aa701bc4fd 100644 --- a/app/presenters/bills/bill.presenter.js +++ b/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) { diff --git a/app/presenters/bills/licence-summaries.presenter.js b/app/presenters/bills/licence-summaries.presenter.js index 5af58187d2..a492a47a5c 100644 --- a/app/presenters/bills/licence-summaries.presenter.js +++ b/app/presenters/bills/licence-summaries.presenter.js @@ -25,7 +25,7 @@ function _billLicences (licenceSummaries) { return { id, reference, - total: formatMoney(total) + total: formatMoney(total, true) } }) } diff --git a/client/sass/application.scss b/client/sass/application.scss index 5922404a2c..a74475c3a1 100644 --- a/client/sass/application.scss +++ b/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; } } diff --git a/test/presenters/base.presenter.test.js b/test/presenters/base.presenter.test.js index 598434e14f..cb5caf3f6e 100644 --- a/test/presenters/base.presenter.test.js +++ b/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') + }) }) }) }) diff --git a/test/presenters/bill-licences/view-bill-licence.presenter.test.js b/test/presenters/bill-licences/view-bill-licence.presenter.test.js index c346f96385..5ce56d1927 100644 --- a/test/presenters/bill-licences/view-bill-licence.presenter.test.js +++ b/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' } + ] + }) }) }) }) diff --git a/test/presenters/bills/bill.presenter.test.js b/test/presenters/bills/bill.presenter.test.js index be17c8458f..245a427abd 100644 --- a/test/presenters/bills/bill.presenter.test.js +++ b/test/presenters/bills/bill.presenter.test.js @@ -208,43 +208,124 @@ describe('Bill presenter', () => { }) }) - describe("the 'displayCreditDebitTotals' property", () => { - describe('when the bill run is not supplementary', () => { - it('returns false', () => { + describe("the 'creditsTotal' property", () => { + describe('when the bill run was created in WRLS', () => { + it("returns the 'creditNoteValue' of the bill (£0.00)", () => { const result = BillPresenter.go(bill, billingAccount) - expect(result.displayCreditDebitTotals).to.be.false() + expect(result.creditsTotal).to.equal('£0.00') }) }) - describe('when the bill run is supplementary', () => { - describe('and was created in WRLS', () => { + describe('when the bill run was created in NALD', () => { + beforeEach(() => { + bill.billRun.source = 'nald' + }) + + describe("and 'netAmount' on the bill is 21317800", () => { + it('returns £0.00', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.creditsTotal).to.equal('£0.00') + }) + }) + + describe("and 'netAmount' on the bill is -21317800", () => { + beforeEach(() => { + bill.netAmount = -21317800 + }) + + it('returns £213,178.00', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.creditsTotal).to.equal('£213,178.00') + }) + }) + + describe("and 'netAmount' on the bill is 0", () => { + beforeEach(() => { + bill.netAmount = 0 + }) + + it('returns £0.00', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.creditsTotal).to.equal('£0.00') + }) + }) + }) + }) + + describe("the 'debitsTotal' property", () => { + describe('when the bill run was created in WRLS', () => { + it("returns the 'invoiceValue' of the bill (£213,178.00)", () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.debitsTotal).to.equal('£213,178.00') + }) + }) + + describe('when the bill run was created in NALD', () => { + beforeEach(() => { + bill.billRun.source = 'nald' + }) + + describe("and 'netAmount' on the bill is 21317800", () => { + it('returns £213,178.00', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.debitsTotal).to.equal('£213,178.00') + }) + }) + + describe("and 'netAmount' on the bill is -21317800", () => { beforeEach(() => { - bill.billRun.batchType = 'supplementary' + bill.netAmount = -21317800 }) - it('returns true', () => { + it('returns £0.00', () => { const result = BillPresenter.go(bill, billingAccount) - expect(result.displayCreditDebitTotals).to.be.true() + expect(result.debitsTotal).to.equal('£0.00') }) }) - describe('but was created in NALD', () => { + describe("and 'netAmount' on the bill is 0", () => { beforeEach(() => { - bill.billRun.batchType = 'supplementary' - bill.billRun.source = 'nald' + bill.netAmount = 0 }) - it('returns false', () => { + it('returns £0.00', () => { const result = BillPresenter.go(bill, billingAccount) - expect(result.displayCreditDebitTotals).to.be.false() + expect(result.debitsTotal).to.equal('£0.00') }) }) }) }) + describe("the 'displayCreditDebitTotals' property", () => { + describe('when the bill run is not supplementary', () => { + it('returns false', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.displayCreditDebitTotals).to.be.false() + }) + }) + + describe('when the bill run is supplementary', () => { + beforeEach(() => { + bill.billRun.batchType = 'supplementary' + }) + + it('returns true', () => { + const result = BillPresenter.go(bill, billingAccount) + + expect(result.displayCreditDebitTotals).to.be.true() + }) + }) + }) + describe("the 'financialYear' property", () => { it('returns the bill run start and end financial year', () => { const result = BillPresenter.go(bill, billingAccount) diff --git a/test/presenters/bills/licence-summaries.presenter.test.js b/test/presenters/bills/licence-summaries.presenter.test.js index 01a57ac031..fec74dc1d8 100644 --- a/test/presenters/bills/licence-summaries.presenter.test.js +++ b/test/presenters/bills/licence-summaries.presenter.test.js @@ -26,7 +26,7 @@ describe('Licence Summaries presenter', () => { { id: 'e37320ba-10c8-4954-8bc4-6982e56ded41', reference: '01/735', - total: '£6,222.18' + total: '-£6,222.18' }, { id: '127377ea-24ea-4578-8b96-ef9a8625a313', @@ -74,7 +74,7 @@ function _testLicenceSummaries () { { billingInvoiceLicenceId: 'e37320ba-10c8-4954-8bc4-6982e56ded41', licenceRef: '01/735', - total: 622218 + total: -622218 }, { billingInvoiceLicenceId: '127377ea-24ea-4578-8b96-ef9a8625a313',