From 895b0711ebdb2ec94e325d7180df4fd5e3a738a4 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 23 Nov 2022 15:22:38 +0000 Subject: [PATCH] Refactor Supplementary output to test presenter (#32) https://eaflood.atlassian.net/browse/WATER-3787 https://eaflood.atlassian.net/browse/WATER-3802 https://eaflood.atlassian.net/browse/WATER-3826 We're trying to make it easy to test that we are selecting the right values from the DB when generating the SROC supplementary bill run. Our test endpoint currently lists the charge versions, which includes the licence reference. In [Add new SROC Billing Period service](https://github.com/DEFRA/water-abstraction-system/pull/30) we add the financial years. But it would be easier to validate the licences that have been selected if the response included a unique list of them rather than having to search through the charge version results. This gives us a prime opportunity to split out the presentation logic from what the service is doing, especially as it is likely to be discarded when we are happy the process is working. So, in this change, we move formatting the response to a new 'presenter'. As part of the change, we'll add a unique list of the Licences being used. Example response following this change ```json { "billingPeriods": [ { "startDate": "2022-04-01T00:00:00.000Z", "endDate": "2023-03-31T00:00:00.000Z" } ], "licences": [ { "licenceId": "2627a306-23a3-432f-9c71-a71663888285", "licenceRef": "AT/SROC/SUPB/01" } ], "chargeVersions": [ { "chargeVersionId": "4b5cbe04-a0e2-468c-909e-1e2d93810ba8", "licenceRef": "AT/SROC/SUPB/01", "licenceId": "2627a306-23a3-432f-9c71-a71663888285", "scheme": "sroc", "endDate": null }, { "chargeVersionId": "732fde85-fd3b-44e8-811f-8e6f4eb8cf6f", "licenceRef": "AT/SROC/SUPB/01", "licenceId": "2627a306-23a3-432f-9c71-a71663888285", "scheme": "sroc", "endDate": null } ] } ``` --- app/presenters/supplementary.presenter.js | 72 ++++++++++++++++++ .../fetch_charge_versions.service.js | 11 ++- .../supplementary.service.js | 14 +++- .../supplementary.presenter.test.js | 76 +++++++++++++++++++ .../supplementary.service.test.js | 10 ++- 5 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 app/presenters/supplementary.presenter.js create mode 100644 test/presenters/supplementary.presenter.test.js diff --git a/app/presenters/supplementary.presenter.js b/app/presenters/supplementary.presenter.js new file mode 100644 index 0000000000..cef58aacb5 --- /dev/null +++ b/app/presenters/supplementary.presenter.js @@ -0,0 +1,72 @@ +'use strict' + +/** + * @module SupplementaryPresenter + */ + +class SupplementaryPresenter { + constructor (data) { + this._data = data + } + + go () { + return this._presentation(this._data) + } + + /** + * Creates an array of unique licence objects from the combined data received + * + * We know the main query in `SupplementaryService` is based on charge versions. As a licence may be linked to + * multiple charge versions, it is possible that the licence ID and reference will feature multiple times in the + * results. + * + * To make it easier to confirm we are including the right licences in the Supplementary bill run process we want to + * extract a unique list of licence ID's and references. + * + * @param {Object[]} chargeVersions Results of a call to SupplementaryService and the charge version info it returns + * + * @returns {Object[]} Array of objects representing the unique licence details in the charge versions passed in + */ + _licences (chargeVersions) { + // Use map to generate an array of just licence IDs + const justLicenceIds = chargeVersions.map((chargeVersion) => chargeVersion.licenceId) + // Set is used to store unique values. So, if you what you pass in contains duplicates Set will return just the + // unique ones + const uniqueLicenceIds = new Set(justLicenceIds) + + const licences = [] + for (const id of uniqueLicenceIds) { + // Iterate through our unique Licence Id's and use them to find the index for the first matching entry in the + // charge versions data + const index = chargeVersions.findIndex((chargeVersion) => chargeVersion.licenceId === id) + // Destructure the licenceId and licenceRef from the matching object + const { licenceId, licenceRef } = chargeVersions[index] + + // Push a new object into our array of licences + licences.push({ licenceId, licenceRef }) + } + + return licences + } + + _presentation (data) { + const licences = this._licences(data.chargeVersions) + const chargeVersions = data.chargeVersions.map((chargeVersion) => { + return { + chargeVersionId: chargeVersion.chargeVersionId, + licenceRef: chargeVersion.licenceRef, + licenceId: chargeVersion.licenceId, + scheme: chargeVersion.scheme, + endDate: chargeVersion.endDate + } + }) + + return { + billingPeriods: data.billingPeriods, + licences, + chargeVersions + } + } +} + +module.exports = SupplementaryPresenter diff --git a/app/services/supplementary_billing/fetch_charge_versions.service.js b/app/services/supplementary_billing/fetch_charge_versions.service.js index 529f52d87f..f0afe1796c 100644 --- a/app/services/supplementary_billing/fetch_charge_versions.service.js +++ b/app/services/supplementary_billing/fetch_charge_versions.service.js @@ -2,7 +2,6 @@ /** * @module FetchChargeVersionsService - * */ const { db } = require('../../../db/db') @@ -16,16 +15,16 @@ class FetchChargeVersionsService { static async _fetch (regionId) { const chargeVersions = db - .select('chargeVersionId', 'licences.licenceRef') - .from('water.charge_versions') - .innerJoin('water.licences', 'charge_versions.licence_id', 'licences.licence_id') + .select('chv.chargeVersionId', 'chv.scheme', 'chv.endDate', 'lic.licenceId', 'lic.licenceRef') + .from({ chv: 'water.charge_versions' }) + .innerJoin({ lic: 'water.licences' }, 'chv.licence_id', 'lic.licence_id') .where({ scheme: 'sroc', end_date: null }) .andWhere({ - 'licences.include_in_supplementary_billing': 'yes', - 'licences.region_id': regionId + 'lic.include_in_supplementary_billing': 'yes', + 'lic.region_id': regionId }) return chargeVersions diff --git a/app/services/supplementary_billing/supplementary.service.js b/app/services/supplementary_billing/supplementary.service.js index 3bacdb6a16..6e0991fc69 100644 --- a/app/services/supplementary_billing/supplementary.service.js +++ b/app/services/supplementary_billing/supplementary.service.js @@ -4,16 +4,22 @@ * @module SupplementaryService */ -const FetchChargeVersionsService = require('./fetch_charge_versions.service.js') const BillingPeriodService = require('./billing_period.service') +const FetchChargeVersionsService = require('./fetch_charge_versions.service.js') +const SupplementaryPresenter = require('../../presenters/supplementary.presenter.js') class SupplementaryService { static async go (regionId) { const chargeVersions = await FetchChargeVersionsService.go(regionId) - const financialYears = BillingPeriodService.go() - const response = { chargeVersions, financialYears } + const billingPeriods = BillingPeriodService.go() + + return this._response(chargeVersions, billingPeriods) + } + + static _response (chargeVersions, billingPeriods) { + const presenter = new SupplementaryPresenter({ chargeVersions, billingPeriods }) - return response + return presenter.go() } } diff --git a/test/presenters/supplementary.presenter.test.js b/test/presenters/supplementary.presenter.test.js new file mode 100644 index 0000000000..502b504262 --- /dev/null +++ b/test/presenters/supplementary.presenter.test.js @@ -0,0 +1,76 @@ +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') + +const { describe, it, beforeEach } = exports.lab = Lab.script() +const { expect } = Code + +// Thing under test +const SupplementaryPresenter = require('../../app/presenters/supplementary.presenter.js') + +describe('Supplementary presenter', () => { + let data + + describe('when there are results', () => { + beforeEach(() => { + data = { + billingPeriods: [ + { startDate: new Date(2022, 3, 1), endDate: new Date(2023, 2, 31) } + ], + chargeVersions: [ + { + chargeVersionId: '4b5cbe04-a0e2-468c-909e-1e2d93810ba8', + scheme: 'sroc', + endDate: null, + licenceRef: 'AT/SROC/SUPB/01', + licenceId: '0000579f-0f8f-4e21-b63a-063384ad32c8' + }, + { + chargeVersionId: '732fde85-fd3b-44e8-811f-8e6f4eb8cf6f', + scheme: 'sroc', + endDate: null, + licenceRef: 'AT/SROC/SUPB/01', + licenceId: '0000579f-0f8f-4e21-b63a-063384ad32c8' + } + ] + } + }) + + it('correctly presents the data', () => { + const presenter = new SupplementaryPresenter(data) + const result = presenter.go() + + expect(result.billingPeriods).to.have.length(1) + expect(result.billingPeriods[0]).to.equal(data.billingPeriods[0]) + + expect(result.licences).to.have.length(1) + expect(result.licences[0]).to.equal({ + licenceId: data.chargeVersions[0].licenceId, + licenceRef: data.chargeVersions[0].licenceRef + }) + + expect(result.chargeVersions).to.have.length(2) + expect(result.chargeVersions[0]).to.equal(data.chargeVersions[0]) + }) + }) + + describe('when there are no results', () => { + beforeEach(() => { + data = { + billingPeriods: [], + chargeVersions: [] + } + }) + + it('correctly presents the data', () => { + const presenter = new SupplementaryPresenter(data) + const result = presenter.go() + + expect(result.billingPeriods).to.be.empty() + expect(result.licences).to.be.empty() + expect(result.chargeVersions).to.be.empty() + }) + }) +}) diff --git a/test/services/supplementary_billing/supplementary.service.test.js b/test/services/supplementary_billing/supplementary.service.test.js index c1e7ee21d0..049a2cff1d 100644 --- a/test/services/supplementary_billing/supplementary.service.test.js +++ b/test/services/supplementary_billing/supplementary.service.test.js @@ -20,7 +20,13 @@ describe('Supplementary service', () => { }) describe('when there are licences to be included in supplementary billing', () => { - const testRecords = [{ charge_version_id: '878bc903-836d-4549-83f5-4e20ccf87d2f' }] + const testRecords = [{ + chargeVersionId: '4b5cbe04-a0e2-468c-909e-1e2d93810ba8', + scheme: 'sroc', + endDate: null, + licenceId: '2627a306-23a3-432f-9c71-a71663888285', + licenceRef: 'AT/SROC/SUPB/01' + }] beforeEach(async () => { Sinon.stub(FetchChargeVersionsService, 'go').resolves(testRecords) @@ -30,7 +36,7 @@ describe('Supplementary service', () => { const result = await SupplementaryService.go('regionId') expect(result.chargeVersions.length).to.equal(1) - expect(result.chargeVersions[0].charge_version_id).to.equal(testRecords[0].charge_version_id) + expect(result.chargeVersions[0].chargeVersionId).to.equal(testRecords[0].chargeVersionId) }) })