From 9dcf668b66f1806bf430cc2caa420edcdc11230f Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 12:40:15 +0100 Subject: [PATCH 1/8] Fix unsent bills displaying in licence bills tab https://eaflood.atlassian.net/browse/WATER-4316 In [Add View Licence Bills page](https://github.com/DEFRA/water-abstraction-system/pull/986) we added support for displaying the bills linked to a licence in the view licence page's 'Bills' tab. Only we didn't spot that it should only be 'sent' bills. This change updates the relevant fetch service to ensure the bills we get back are only those with a status of 'sent'. From 30ecea4249ee7a9780edf2153022f1eb05fce0cf Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 12:45:14 +0100 Subject: [PATCH 2/8] Housekeeping - fix existing eslint infraction --- app/services/licences/fetch-licence-bills.service.js | 4 ++-- .../services/licences/fetch-licence-bills.service.test.js | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/services/licences/fetch-licence-bills.service.js b/app/services/licences/fetch-licence-bills.service.js index 1564df56ec..1096011889 100644 --- a/app/services/licences/fetch-licence-bills.service.js +++ b/app/services/licences/fetch-licence-bills.service.js @@ -5,9 +5,9 @@ * @module FetchLicenceBillsService */ -const BillModel = require('../../models/bill.model') +const BillModel = require('../../models/bill.model.js') -const DatabaseConfig = require('../../../config/database.config') +const DatabaseConfig = require('../../../config/database.config.js') /** * Fetches all bills for a licence which is needed for the view '/licences/{id}/bills` page diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index bd3a495a41..094e18bc54 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -9,12 +9,12 @@ const { expect } = Code // Test helpers const DatabaseSupport = require('../../support/database.js') -const BillLicenceHelper = require('../../support/helpers/bill-licence.helper') -const BillHelper = require('../../support/helpers/bill.helper') -const BillRunHelper = require('../../support/helpers/bill-run.helper') +const BillLicenceHelper = require('../../support/helpers/bill-licence.helper.js') +const BillHelper = require('../../support/helpers/bill.helper.js') +const BillRunHelper = require('../../support/helpers/bill-run.helper.js') // Thing under test -const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service') +const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service.js') describe('Fetch licence bills service', () => { beforeEach(async () => { From 36ebef6df3da95d8d18b7e34dcefbc0775cc4c17 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 12:47:12 +0100 Subject: [PATCH 3/8] Housekeeping - correct test title case --- test/services/licences/fetch-licence-bills.service.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index 094e18bc54..76d362a96b 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -16,7 +16,7 @@ const BillRunHelper = require('../../support/helpers/bill-run.helper.js') // Thing under test const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service.js') -describe('Fetch licence bills service', () => { +describe('Fetch Licence Bills service', () => { beforeEach(async () => { await DatabaseSupport.clean() }) From 50e9e339bf22fbdd751d45cfc13a47bc89d75c39 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 12:51:58 +0100 Subject: [PATCH 4/8] Highlight existing test does not link with bill run The service has a withGraphFetched that we are assuming is finding the matching bill run as we know the presenter this data gets passed to is expecting it. However, coming in to add a test case for our currently failing scenario we spotted that the existing test is not actually checking for the bill run. Nor was the bill run created actually linked to the bill. So, we remove everything to do with the bill run from this test to highlight the point we'll need to do some refactoring to ensure the existing scenarios are covered before we make our changes. --- test/services/licences/fetch-licence-bills.service.test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index 76d362a96b..76bc4eb5a1 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -11,7 +11,6 @@ const { expect } = Code const DatabaseSupport = require('../../support/database.js') const BillLicenceHelper = require('../../support/helpers/bill-licence.helper.js') const BillHelper = require('../../support/helpers/bill.helper.js') -const BillRunHelper = require('../../support/helpers/bill-run.helper.js') // Thing under test const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service.js') @@ -34,8 +33,6 @@ describe('Fetch Licence Bills service', () => { billId }) - await BillRunHelper.add({ batchType: 'annual' }) - await BillHelper.add( { id: billId, From 222387e4851518e52ffb42a1f41d31dc390e3326 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 13:01:18 +0100 Subject: [PATCH 5/8] Update existing tests to cover the bill run Now we have something we can add our scenario around 'sent' bill runs to. --- .../licences/fetch-licence-bills.service.test.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index 76bc4eb5a1..add3d286b1 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -11,6 +11,7 @@ const { expect } = Code const DatabaseSupport = require('../../support/database.js') const BillLicenceHelper = require('../../support/helpers/bill-licence.helper.js') const BillHelper = require('../../support/helpers/bill.helper.js') +const BillRunHelper = require('../../support/helpers/bill-run.helper.js') // Thing under test const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service.js') @@ -26,6 +27,7 @@ describe('Fetch Licence Bills service', () => { const licenceId = '96d97293-1a62-4ad0-bcb6-24f68a203e6b' const billId = '72988ec1-9fb2-4b87-b0a0-3c0be628a72c' const billingAccountId = '0ba3b707-72ee-4296-b177-a19afff10688' + const billRunId = 'd40004d5-a99b-40a7-adf2-22d36d5b20b5' beforeEach(async () => { await BillLicenceHelper.add({ @@ -33,17 +35,21 @@ describe('Fetch Licence Bills service', () => { billId }) + await BillRunHelper.add({ id: billRunId }) + await BillHelper.add( { id: billId, + billRunId, invoiceNumber: '123', accountNumber: 'T21404193A', netAmount: 12345, billingAccountId, createdAt: createdDate }) - // Add an extra record to ensure the bill is retrieved by the license id - await BillHelper.add() + + // Add an extra bill linked to the same bill run to test only bills for the licence are retrieved + await BillHelper.add({ billRunId }) }) it('returns results', async () => { @@ -56,7 +62,9 @@ describe('Fetch Licence Bills service', () => { expect(result.bills).to.equal( [{ accountNumber: 'T21404193A', - billRun: null, + billRun: { + batchType: 'supplementary' + }, billingAccountId, createdAt: createdDate, credit: null, From cfc2cae9a185e1bdc4a1fbbfbc37386b4c5b1069 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 13:08:05 +0100 Subject: [PATCH 6/8] Add failing test scenario --- .../fetch-licence-bills.service.test.js | 102 ++++++++++++------ 1 file changed, 68 insertions(+), 34 deletions(-) diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index add3d286b1..52f84e2cf5 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -34,48 +34,82 @@ describe('Fetch Licence Bills service', () => { licenceId, billId }) + }) + + describe("and they are linked to a 'sent' bill run", () => { + beforeEach(async () => { + await BillRunHelper.add({ id: billRunId, status: 'sent' }) + + await BillHelper.add( + { + id: billId, + billRunId, + invoiceNumber: '123', + accountNumber: 'T21404193A', + netAmount: 12345, + billingAccountId, + createdAt: createdDate + }) + + // Add an extra bill linked to the same bill run to test only bills for the licence are retrieved + await BillHelper.add({ billRunId }) + }) + + it('returns results', async () => { + const result = await FetchLicenceBillService.go(licenceId, 1) - await BillRunHelper.add({ id: billRunId }) - - await BillHelper.add( - { - id: billId, - billRunId, - invoiceNumber: '123', - accountNumber: 'T21404193A', - netAmount: 12345, - billingAccountId, - createdAt: createdDate + expect(result.pagination).to.equal({ + total: 1 }) - // Add an extra bill linked to the same bill run to test only bills for the licence are retrieved - await BillHelper.add({ billRunId }) + expect(result.bills).to.equal( + [{ + accountNumber: 'T21404193A', + billRun: { + batchType: 'supplementary' + }, + billingAccountId, + createdAt: createdDate, + credit: null, + deminimis: false, + financialYearEnding: 2023, + id: billId, + invoiceNumber: '123', + legacyId: null, + netAmount: 12345 + }] + ) + }) }) - it('returns results', async () => { - const result = await FetchLicenceBillService.go(licenceId, 1) + describe("but they are not linked to a 'sent' bill run", () => { + beforeEach(async () => { + await BillRunHelper.add({ id: billRunId }) + + await BillHelper.add( + { + id: billId, + billRunId, + invoiceNumber: '123', + accountNumber: 'T21404193A', + netAmount: 12345, + billingAccountId, + createdAt: createdDate + }) - expect(result.pagination).to.equal({ - total: 1 + // Add an extra bill linked to the same bill run to test only bills for the licence are retrieved + await BillHelper.add({ billRunId }) }) - expect(result.bills).to.equal( - [{ - accountNumber: 'T21404193A', - billRun: { - batchType: 'supplementary' - }, - billingAccountId, - createdAt: createdDate, - credit: null, - deminimis: false, - financialYearEnding: 2023, - id: billId, - invoiceNumber: '123', - legacyId: null, - netAmount: 12345 - }] - ) + it('returns no results', async () => { + const result = await FetchLicenceBillService.go(licenceId, 1) + + expect(result.pagination).to.equal({ + total: 0 + }) + + expect(result.bills).to.be.empty() + }) }) }) }) From c33fb7905dd452f5aac0074a27362282b04d1584 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 13:12:14 +0100 Subject: [PATCH 7/8] Implement fix to fetch query --- app/services/licences/fetch-licence-bills.service.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/services/licences/fetch-licence-bills.service.js b/app/services/licences/fetch-licence-bills.service.js index 1096011889..f9d815a090 100644 --- a/app/services/licences/fetch-licence-bills.service.js +++ b/app/services/licences/fetch-licence-bills.service.js @@ -37,7 +37,9 @@ async function _fetch (licenceId, page) { 'bills.netAmount' ]) .innerJoinRelated('billLicences') + .innerJoin('billRuns', 'billRuns.id', 'bills.billRunId') .where('billLicences.licence_id', licenceId) + .where('billRuns.status', 'sent') .withGraphFetched('billRun') .modifyGraph('billRun', (builder) => { builder.select(['batchType']) From b243b9bd0c5712f133036ee56a62243ca2f927e9 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 14 May 2024 13:16:05 +0100 Subject: [PATCH 8/8] Housekeeping - Add additional test scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a test that describes what happens when a licence has bills. If only for documentation purposes, when we make a statement of behaviour like that it can be handy to show what happens when that is not the case. ``` Fetch Licence Bills service when the licence has bills and they are linked to a 'sent' bill run ✔ 1871) returns results (14 ms) but they are not linked to a 'sent' bill run ✔ 1872) returns no results (3 ms) when the licence has no bills ✔ 1873) returns no results (4 ms) ```` --- .../fetch-licence-bills.service.test.js | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/test/services/licences/fetch-licence-bills.service.test.js b/test/services/licences/fetch-licence-bills.service.test.js index 52f84e2cf5..5c28244150 100644 --- a/test/services/licences/fetch-licence-bills.service.test.js +++ b/test/services/licences/fetch-licence-bills.service.test.js @@ -17,18 +17,17 @@ const BillRunHelper = require('../../support/helpers/bill-run.helper.js') const FetchLicenceBillService = require('../../../app/services/licences/fetch-licence-bills.service.js') describe('Fetch Licence Bills service', () => { + const billId = '72988ec1-9fb2-4b87-b0a0-3c0be628a72c' + const billingAccountId = '0ba3b707-72ee-4296-b177-a19afff10688' + const billRunId = 'd40004d5-a99b-40a7-adf2-22d36d5b20b5' + const createdDate = new Date('2022-01-01') + const licenceId = '96d97293-1a62-4ad0-bcb6-24f68a203e6b' + beforeEach(async () => { await DatabaseSupport.clean() }) describe('when the licence has bills', () => { - const createdDate = new Date('2022-01-01') - - const licenceId = '96d97293-1a62-4ad0-bcb6-24f68a203e6b' - const billId = '72988ec1-9fb2-4b87-b0a0-3c0be628a72c' - const billingAccountId = '0ba3b707-72ee-4296-b177-a19afff10688' - const billRunId = 'd40004d5-a99b-40a7-adf2-22d36d5b20b5' - beforeEach(async () => { await BillLicenceHelper.add({ licenceId, @@ -112,4 +111,22 @@ describe('Fetch Licence Bills service', () => { }) }) }) + + describe('when the licence has no bills', () => { + beforeEach(async () => { + await BillRunHelper.add({ id: billRunId, status: 'sent' }) + await BillHelper.add({ id: billId, billRunId }) + await BillLicenceHelper.add({ billId }) + }) + + it('returns no results', async () => { + const result = await FetchLicenceBillService.go(licenceId, 1) + + expect(result.pagination).to.equal({ + total: 0 + }) + + expect(result.bills).to.be.empty() + }) + }) })