From 51be0702e7279e7a43c6752906e7e782ccdcbe42 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Thu, 8 Dec 2022 23:41:18 +0000 Subject: [PATCH 1/6] Replacing classes with modules and functions > Classes are a template for creating objects. That is the first sentance of the Mozilla documentation for **Classes**. It is what those of us from other languages understand classes to be for. Some of us though (we're looking at you @Cruikshanks) classes are _all things_! Because of that thinking, we've allowed Classes to creap in where they shouldn't really be used. Most noticeably, things like controllers, presenters and services, which are formed of purely static methods. There is no real reason to wrap their functionality in a class. They could just be expressed as modules and export their relevant functions. That's what those experienced with JavaScript are more likely to expect. By not doing things that way we're breaking the [Principle of least astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment). We're believe classes are right for our Models, as it follows the convention of [Objection](https://vincit.github.io/objection.js/guide/models.html) and we do create instances of them. Also, if we have need to create objects in the future, classes will be our go-to. But before this repo gets to far down the road we're taking this opportunity to refactor things from classes into modules. From 669a3f94db782a48f6ca5403079d7485cd44c4c6 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Fri, 9 Dec 2022 00:00:03 +0000 Subject: [PATCH 2/6] Refactor controllers --- app/controllers/health/airbrake.controller.js | 30 ++++++++--------- app/controllers/health/database.controller.js | 12 +++---- app/controllers/root.controller.js | 33 ++++++++----------- .../test/supplementary.controller.js | 12 +++---- app/routes/root.routes.js | 8 ----- 5 files changed, 40 insertions(+), 55 deletions(-) diff --git a/app/controllers/health/airbrake.controller.js b/app/controllers/health/airbrake.controller.js index 6ce7bba67b..b4c9d3e08e 100644 --- a/app/controllers/health/airbrake.controller.js +++ b/app/controllers/health/airbrake.controller.js @@ -5,22 +5,22 @@ * @module AirbrakeController */ -class AirbrakeController { - static async index (request, _h) { - // First section tests connecting to Airbrake through a manual notification - request.server.app.airbrake.notify({ - message: 'Airbrake manual health check', - error: new Error('Airbrake manual health check error'), - session: { - req: { - id: request.info.id - } +async function index (request, _h) { + // First section tests connecting to Airbrake through a manual notification + request.server.app.airbrake.notify({ + message: 'Airbrake manual health check', + error: new Error('Airbrake manual health check error'), + session: { + req: { + id: request.info.id } - }) + } + }) - // Second section throws an error and checks that we automatically capture it and then connect to Airbrake - throw new Error('Airbrake automatic health check error') - } + // Second section throws an error and checks that we automatically capture it and then connect to Airbrake + throw new Error('Airbrake automatic health check error') } -module.exports = AirbrakeController +module.exports = { + index +} diff --git a/app/controllers/health/database.controller.js b/app/controllers/health/database.controller.js index 68d986a3d7..dc59b2c61f 100644 --- a/app/controllers/health/database.controller.js +++ b/app/controllers/health/database.controller.js @@ -7,12 +7,12 @@ const DatabaseHealthCheckService = require('../../services/database-health-check.service.js') -class DatabaseController { - static async index (_request, h) { - const result = await DatabaseHealthCheckService.go() +async function index (_request, h) { + const result = await DatabaseHealthCheckService.go() - return h.response(result).code(200) - } + return h.response(result).code(200) } -module.exports = DatabaseController +module.exports = { + index +} diff --git a/app/controllers/root.controller.js b/app/controllers/root.controller.js index 24b7e6a234..71730f966c 100644 --- a/app/controllers/root.controller.js +++ b/app/controllers/root.controller.js @@ -7,27 +7,20 @@ const ServiceStatusService = require('../services/service-status.service.js') -class RootController { - static async index (_request, _h) { - return { status: 'alive' } - } - - static helloWorld (_request, h) { - return h.view('home.njk', { - title: 'Hello', - message: 'World', - pageTitle: 'Hello World!' - }) - } +async function index (_request, _h) { + return { status: 'alive' } +} - static async serviceStatus (_request, h) { - const pageData = await ServiceStatusService.go() +async function serviceStatus (_request, h) { + const pageData = await ServiceStatusService.go() - return h.view('service_status.njk', { - pageTitle: 'Service Status', - ...pageData - }) - } + return h.view('service_status.njk', { + pageTitle: 'Service Status', + ...pageData + }) } -module.exports = RootController +module.exports = { + index, + serviceStatus +} diff --git a/app/controllers/test/supplementary.controller.js b/app/controllers/test/supplementary.controller.js index 90084db350..09757cd84b 100644 --- a/app/controllers/test/supplementary.controller.js +++ b/app/controllers/test/supplementary.controller.js @@ -7,12 +7,12 @@ const SupplementaryService = require('../../services/supplementary-billing/supplementary.service.js') -class SupplementaryController { - static async index (request, h) { - const result = await SupplementaryService.go(request.query.region) +async function index (request, h) { + const result = await SupplementaryService.go(request.query.region) - return h.response(result).code(200) - } + return h.response(result).code(200) } -module.exports = SupplementaryController +module.exports = { + index +} diff --git a/app/routes/root.routes.js b/app/routes/root.routes.js index 5296f054ef..f42a50f973 100644 --- a/app/routes/root.routes.js +++ b/app/routes/root.routes.js @@ -19,14 +19,6 @@ const routes = [ auth: false } }, - { - method: 'GET', - path: '/hello-world', - handler: RootController.helloWorld, - options: { - auth: false - } - }, { method: 'GET', path: '/service-status', From 0969c228eaf0abf5808ee63fcf5368a570b65577 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Fri, 9 Dec 2022 08:43:01 +0000 Subject: [PATCH 3/6] Refactor presenters --- app/presenters/supplementary.presenter.js | 54 ++++++++----------- .../supplementary.service.js | 4 +- .../supplementary.presenter.test.js | 6 +-- 3 files changed, 26 insertions(+), 38 deletions(-) diff --git a/app/presenters/supplementary.presenter.js b/app/presenters/supplementary.presenter.js index e318099a4e..1c6d45bc06 100644 --- a/app/presenters/supplementary.presenter.js +++ b/app/presenters/supplementary.presenter.js @@ -5,39 +5,31 @@ * @module SupplementaryPresenter */ -class SupplementaryPresenter { - constructor (data) { - this._data = data - } - - go () { - return this._presentation(this._data) - } - - _presentation (data) { - const licences = data.licences.map((licence) => { - return { - licenceId: licence.licenceId, - licenceRef: licence.licenceRef - } - }) - const chargeVersions = data.chargeVersions.map((chargeVersion) => { - return { - chargeVersionId: chargeVersion.chargeVersionId, - licenceRef: chargeVersion.licenceRef, - licenceId: chargeVersion.licenceId, - scheme: chargeVersion.scheme, - startDate: chargeVersion.startDate, - endDate: chargeVersion.endDate - } - }) - +function go (data) { + const licences = data.licences.map((licence) => { return { - billingPeriods: data.billingPeriods, - licences, - chargeVersions + licenceId: licence.licenceId, + licenceRef: licence.licenceRef } + }) + const chargeVersions = data.chargeVersions.map((chargeVersion) => { + return { + chargeVersionId: chargeVersion.chargeVersionId, + licenceRef: chargeVersion.licenceRef, + licenceId: chargeVersion.licenceId, + scheme: chargeVersion.scheme, + startDate: chargeVersion.startDate, + endDate: chargeVersion.endDate + } + }) + + return { + billingPeriods: data.billingPeriods, + licences, + chargeVersions } } -module.exports = SupplementaryPresenter +module.exports = { + go +} diff --git a/app/services/supplementary-billing/supplementary.service.js b/app/services/supplementary-billing/supplementary.service.js index 5aba11a7bf..7a982a7a47 100644 --- a/app/services/supplementary-billing/supplementary.service.js +++ b/app/services/supplementary-billing/supplementary.service.js @@ -31,9 +31,7 @@ class SupplementaryService { } static _response (data) { - const presenter = new SupplementaryPresenter(data) - - return presenter.go() + return SupplementaryPresenter.go(data) } } diff --git a/test/presenters/supplementary.presenter.test.js b/test/presenters/supplementary.presenter.test.js index dc5dcfb55c..80847d2e9d 100644 --- a/test/presenters/supplementary.presenter.test.js +++ b/test/presenters/supplementary.presenter.test.js @@ -44,8 +44,7 @@ describe('Supplementary presenter', () => { }) it('correctly presents the data', () => { - const presenter = new SupplementaryPresenter(data) - const result = presenter.go() + const result = SupplementaryPresenter.go(data) expect(result.billingPeriods).to.have.length(1) expect(result.billingPeriods[0]).to.equal(data.billingPeriods[0]) @@ -71,8 +70,7 @@ describe('Supplementary presenter', () => { }) it('correctly presents the data', () => { - const presenter = new SupplementaryPresenter(data) - const result = presenter.go() + const result = SupplementaryPresenter.go(data) expect(result.billingPeriods).to.be.empty() expect(result.licences).to.be.empty() From 405092077ea4c90db49132c5f06539045642bcbf Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Fri, 9 Dec 2022 09:04:00 +0000 Subject: [PATCH 4/6] Update services Note - this forced us to review how we'd handled 'current date' in the BillingPeriodService. We found Sinon's [useFakeTimers()](https://sinonjs.org/releases/latest/fake-timers/) which is the more appropriate mechanism for controlling what `Date` returns during our tests. --- app/services/database-health-check.service.js | 16 +- app/services/service-status.service.js | 230 +++++++++--------- .../billing-period.service.js | 90 ++++--- .../fetch-charge-versions.service.js | 64 ++--- .../fetch-licences.service.js | 38 ++- .../fetch-region.service.js | 46 ++-- .../supplementary.service.js | 39 ++- .../billing-period.service.test.js | 55 ++--- 8 files changed, 294 insertions(+), 284 deletions(-) diff --git a/app/services/database-health-check.service.js b/app/services/database-health-check.service.js index af98ba6be3..c9559702e4 100644 --- a/app/services/database-health-check.service.js +++ b/app/services/database-health-check.service.js @@ -8,20 +8,22 @@ const { db } = require('../../db/db.js') /** - * Generates an array of stats for each table in the database when `go()` is called + * Generates an array of stats for each table in the database * * This is a dump of running `SELECT * FROM pg_stat_user_tables` for the database. It's part of the database * healthcheck and we use it for 2 reasons * * - confirm we can connect * - get some basic stats, for example number of records, for each table without needing to connect to the db + * + * @returns an array of stats for each table found in the db */ -class DatabaseHealthCheckService { - static async go () { - const stats = db.select().table('pg_stat_user_tables') +async function go () { + const stats = db.select().table('pg_stat_user_tables') - return stats - } + return stats } -module.exports = DatabaseHealthCheckService +module.exports = { + go +} diff --git a/app/services/service-status.service.js b/app/services/service-status.service.js index 0f7f60b86b..8e51cc310c 100644 --- a/app/services/service-status.service.js +++ b/app/services/service-status.service.js @@ -19,140 +19,140 @@ const servicesConfig = require('../../config/services.config.js') * Each data set is returned in the format needed to populate the gov.uk table elements ie. an array containing one * array per row, where each row array contains multiple `{ text: '...' }` elements, one for each cell in the row. */ -class ServiceStatusService { - static async go () { - const virusScannerData = await this._getVirusScannerData() - const redisConnectivityData = await this._getRedisConnectivityData() - - const addressFacadeData = await this._getAddressFacadeData() - const chargingModuleData = await this._getChargingModuleData() - const appData = await this._getAppData() - - return { - virusScannerData, - redisConnectivityData, - addressFacadeData, - chargingModuleData, - appData - } +async function go () { + const virusScannerData = await _getVirusScannerData() + const redisConnectivityData = await _getRedisConnectivityData() + + const addressFacadeData = await _getAddressFacadeData() + const chargingModuleData = await _getChargingModuleData() + const appData = await _getAppData() + + return { + virusScannerData, + redisConnectivityData, + addressFacadeData, + chargingModuleData, + appData } +} - /** - * Receives an array and returns it in the format required by the nunjucks template in the view. - */ - static _mapArrayToTextCells (rows) { - return rows.map(row => { - return row.map(cell => { - return { text: cell } - }) +/** + * Receives an array and returns it in the format required by the nunjucks template in the view. + */ +function _mapArrayToTextCells (rows) { + return rows.map(row => { + return row.map(cell => { + return { text: cell } }) - } - - static async _getVirusScannerData () { - try { - const { stdout, stderr } = await exec('clamdscan --version') - return stderr ? `ERROR: ${stderr}` : stdout - } catch (error) { - return `ERROR: ${error.message}` - } - } + }) +} - static async _getRedisConnectivityData () { - try { - const { stdout, stderr } = await exec('redis-server --version') - return stderr ? `ERROR: ${stderr}` : stdout - } catch (error) { - return `ERROR: ${error.message}` - } +async function _getVirusScannerData () { + try { + const { stdout, stderr } = await exec('clamdscan --version') + return stderr ? `ERROR: ${stderr}` : stdout + } catch (error) { + return `ERROR: ${error.message}` } +} - static async _getAddressFacadeData () { - const statusUrl = new URL('/address-service/hola', servicesConfig.addressFacade.url) - const result = await this._requestData(statusUrl) - - return result.succeeded ? result.response.body : result.response +async function _getRedisConnectivityData () { + try { + const { stdout, stderr } = await exec('redis-server --version') + return stderr ? `ERROR: ${stderr}` : stdout + } catch (error) { + return `ERROR: ${error.message}` } +} - static async _getChargingModuleData () { - const statusUrl = new URL('/status', servicesConfig.chargingModule.url) - const result = await this._requestData(statusUrl) +async function _getAddressFacadeData () { + const statusUrl = new URL('/address-service/hola', servicesConfig.addressFacade.url) + const result = await _requestData(statusUrl) - return result.succeeded ? result.response.headers['x-cma-docker-tag'] : result.response - } + return result.succeeded ? result.response.body : result.response +} - static async _requestData (url) { - // As of v12, the got dependency no longer supports CJS modules. This causes us a problem as we are locked into - // using these for the time being. Some workarounds are provided here: https://github.com/sindresorhus/got/issues/1789 - // We have gone the route of using await import('got'). We cannot do this at the top level as Node doesn't support - // top level in CJS so we do it here instead. - const { got } = await import('got') - const result = { - succeeded: true, - response: null - } +async function _getChargingModuleData () { + const statusUrl = new URL('/status', servicesConfig.chargingModule.url) + const result = await _requestData(statusUrl) - try { - result.response = await got.get(url, { - retry: { - // We ensure that the only network errors Got retries are timeout errors - errorCodes: ['ETIMEDOUT'], - // We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried - statusCodes: [] - } - }) - } catch (error) { - const statusCode = error.response ? error.response.statusCode : 'N/A' - result.response = `ERROR: ${statusCode} - ${error.name} - ${error.message}` - result.succeeded = false - } + return result.succeeded ? result.response.headers['x-cma-docker-tag'] : result.response +} - return result +async function _requestData (url) { + // As of v12, the got dependency no longer supports CJS modules. This causes us a problem as we are locked into + // using these for the time being. Some workarounds are provided here: https://github.com/sindresorhus/got/issues/1789 + // We have gone the route of using await import('got'). We cannot do this at the top level as Node doesn't support + // top level in CJS so we do it here instead. + const { got } = await import('got') + const result = { + succeeded: true, + response: null } - static _getImportJobsData () { - return this._mapArrayToTextCells([ - [ - 'Cell 1.1', - 'Cell 1.2' - ], - [ - 'Cell 2.1', - 'Cell 2.2' - ] - ]) + try { + result.response = await got.get(url, { + retry: { + // We ensure that the only network errors Got retries are timeout errors + errorCodes: ['ETIMEDOUT'], + // We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried + statusCodes: [] + } + }) + } catch (error) { + const statusCode = error.response ? error.response.statusCode : 'N/A' + result.response = `ERROR: ${statusCode} - ${error.name} - ${error.message}` + result.succeeded = false } - static async _getAppData () { - const healthInfoPath = '/health/info' - const services = [ - { name: 'Service - foreground', url: new URL(healthInfoPath, servicesConfig.serviceForeground.url) }, - { name: 'Service - background', url: new URL(healthInfoPath, servicesConfig.serviceBackground.url) }, - { name: 'Reporting', url: new URL(healthInfoPath, servicesConfig.reporting.url) }, - { name: 'Import', url: new URL(healthInfoPath, servicesConfig.import.url) }, - { name: 'Tactical CRM', url: new URL(healthInfoPath, servicesConfig.tacticalCrm.url) }, - { name: 'External UI', url: new URL(healthInfoPath, servicesConfig.externalUi.url) }, - { name: 'Internal UI', url: new URL(healthInfoPath, servicesConfig.internalUi.url) }, - { name: 'Tactical IDM', url: new URL(healthInfoPath, servicesConfig.tacticalIdm.url) }, - { name: 'Permit repository', url: new URL(healthInfoPath, servicesConfig.permitRepository.url) }, - { name: 'Returns', url: new URL(healthInfoPath, servicesConfig.returns.url) } + return result +} + +function _getImportJobsData () { + return _mapArrayToTextCells([ + [ + 'Cell 1.1', + 'Cell 1.2' + ], + [ + 'Cell 2.1', + 'Cell 2.2' ] + ]) +} - for (const service of services) { - const result = await this._requestData(service.url) - - if (result.succeeded) { - const data = JSON.parse(result.response.body) - service.version = data.version - service.commit = data.commit - service.jobs = service.name === 'Import' ? this._getImportJobsData() : [] - } else { - service.version = result.response - service.commit = '' - } +async function _getAppData () { + const healthInfoPath = '/health/info' + const services = [ + { name: 'Service - foreground', url: new URL(healthInfoPath, servicesConfig.serviceForeground.url) }, + { name: 'Service - background', url: new URL(healthInfoPath, servicesConfig.serviceBackground.url) }, + { name: 'Reporting', url: new URL(healthInfoPath, servicesConfig.reporting.url) }, + { name: 'Import', url: new URL(healthInfoPath, servicesConfig.import.url) }, + { name: 'Tactical CRM', url: new URL(healthInfoPath, servicesConfig.tacticalCrm.url) }, + { name: 'External UI', url: new URL(healthInfoPath, servicesConfig.externalUi.url) }, + { name: 'Internal UI', url: new URL(healthInfoPath, servicesConfig.internalUi.url) }, + { name: 'Tactical IDM', url: new URL(healthInfoPath, servicesConfig.tacticalIdm.url) }, + { name: 'Permit repository', url: new URL(healthInfoPath, servicesConfig.permitRepository.url) }, + { name: 'Returns', url: new URL(healthInfoPath, servicesConfig.returns.url) } + ] + + for (const service of services) { + const result = await _requestData(service.url) + + if (result.succeeded) { + const data = JSON.parse(result.response.body) + service.version = data.version + service.commit = data.commit + service.jobs = service.name === 'Import' ? _getImportJobsData() : [] + } else { + service.version = result.response + service.commit = '' } - - return services } + + return services } -module.exports = ServiceStatusService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/billing-period.service.js b/app/services/supplementary-billing/billing-period.service.js index 6fc8f20662..f668c7ef7b 100644 --- a/app/services/supplementary-billing/billing-period.service.js +++ b/app/services/supplementary-billing/billing-period.service.js @@ -5,55 +5,51 @@ * @module BillingPeriodService */ -class BillingPeriodService { - /** - * Returns the billing periods needed when generating a supplementary bill run - * - * **IMPORANT!** This service currently only handles SROC billing periods and only the 'current year' - * - * Using the current date at the time the service is called, it calculates the billing periods to use. We permit - * changes to charge versions to be retroactively applied up to 5 years. So, a bill run generated in 2022 would need - * to consider every financial year back to 2017. - * - * The exception to that is the change in charge scheme that happened in 2022, when we moved from ALCS (or PRESROC) to - * SROC. Changes prior to 2022 would only apply to a ALCS bill run and vice versa. - * - * @returns {Object[]} an array of billing periods each containing a `startDate` and `endDate`. - */ - static go () { - const currentDate = this._currentDate() - const currentYear = currentDate.getFullYear() - - // 01-APR to 31-MAR - const financialPeriod = { - start: { day: 1, month: 3 }, - end: { day: 31, month: 2 } - } - - let startYear - let endYear - - // IMPORANT! getMonth returns an integer (0-11). So, January is represented as 0 and December as 11. This is why - // financialPeriod.end.month is 2 rather than 3, even though we mean March - if (currentDate.getMonth() <= financialPeriod.end.month) { - // For example, if currentDate was 2022-02-15 it would fall in financial year 2021-04-01 to 2022-03-31 - startYear = currentYear - 1 - endYear = currentYear - } else { - // For example, if currentDate was 2022-06-15 it would fall in financial year 2022-04-01 to 2023-03-31 - startYear = currentYear - endYear = currentYear + 1 - } - - return [{ - startDate: new Date(startYear, financialPeriod.start.month, financialPeriod.start.day), - endDate: new Date(endYear, financialPeriod.end.month, financialPeriod.end.day) - }] +/** + * Returns the billing periods needed when generating a supplementary bill run + * + * **IMPORANT!** This service currently only handles SROC billing periods and only the 'current year' + * + * Using the current date at the time the service is called, it calculates the billing periods to use. We permit + * changes to charge versions to be retroactively applied up to 5 years. So, a bill run generated in 2022 would need + * to consider every financial year back to 2017. + * + * The exception to that is the change in charge scheme that happened in 2022, when we moved from ALCS (or PRESROC) to + * SROC. Changes prior to 2022 would only apply to a ALCS bill run and vice versa. + * + * @returns {Object[]} an array of billing periods each containing a `startDate` and `endDate`. + */ +function go () { + const currentDate = new Date() + const currentYear = currentDate.getFullYear() + + // 01-APR to 31-MAR + const financialPeriod = { + start: { day: 1, month: 3 }, + end: { day: 31, month: 2 } } - static _currentDate () { - return new Date() + let startYear + let endYear + + // IMPORANT! getMonth returns an integer (0-11). So, January is represented as 0 and December as 11. This is why + // financialPeriod.end.month is 2 rather than 3, even though we mean March + if (currentDate.getMonth() <= financialPeriod.end.month) { + // For example, if currentDate was 2022-02-15 it would fall in financial year 2021-04-01 to 2022-03-31 + startYear = currentYear - 1 + endYear = currentYear + } else { + // For example, if currentDate was 2022-06-15 it would fall in financial year 2022-04-01 to 2023-03-31 + startYear = currentYear + endYear = currentYear + 1 } + + return [{ + startDate: new Date(startYear, financialPeriod.start.month, financialPeriod.start.day), + endDate: new Date(endYear, financialPeriod.end.month, financialPeriod.end.day) + }] } -module.exports = BillingPeriodService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/fetch-charge-versions.service.js b/app/services/supplementary-billing/fetch-charge-versions.service.js index 416be60887..a1df9fe4d1 100644 --- a/app/services/supplementary-billing/fetch-charge-versions.service.js +++ b/app/services/supplementary-billing/fetch-charge-versions.service.js @@ -7,40 +7,40 @@ const { db } = require('../../../db/db.js') -class FetchChargeVersionsService { - /** - * Fetch all SROC charge versions linked to licences flagged for supplementary billing that are in the period being - * billed - * - * > This is not the final form of the service. It is a 'work in progress' as we implement tickets that gradually - * > build up our understanding of SROC supplementary billing - * - * @param {string} regionId GUID of the region which the licences will be linked to - * @param {Object} billingPeriod Object with a `startDate` and `endDate` property representing the period being billed - * - * @returns an array of Objects containing the relevant charge versions - */ - static async go (regionId, billingPeriod) { - const chargeVersions = await this._fetch(regionId, billingPeriod) +/** + * Fetch all SROC charge versions linked to licences flagged for supplementary billing that are in the period being + * billed + * + * > This is not the final form of the service. It is a 'work in progress' as we implement tickets that gradually + * > build up our understanding of SROC supplementary billing + * + * @param {string} regionId GUID of the region which the licences will be linked to + * @param {Object} billingPeriod Object with a `startDate` and `endDate` property representing the period being billed + * + * @returns an array of Objects containing the relevant charge versions + */ +async function go (regionId, billingPeriod) { + const chargeVersions = await _fetch(regionId, billingPeriod) - return chargeVersions - } + return chargeVersions +} - static async _fetch (regionId, billingPeriod) { - const chargeVersions = db - .select('chv.chargeVersionId', 'chv.scheme', 'chv.startDate', 'chv.endDate', 'lic.licenceId', 'lic.licenceRef') - .from({ chv: 'water.charge_versions' }) - .innerJoin({ lic: 'water.licences' }, 'chv.licence_id', 'lic.licence_id') - .where({ - scheme: 'sroc', - 'lic.include_in_supplementary_billing': 'yes', - 'lic.region_id': regionId - }) - .andWhere('chv.start_date', '>=', billingPeriod.startDate) - .andWhere('chv.start_date', '<=', billingPeriod.endDate) +async function _fetch (regionId, billingPeriod) { + const chargeVersions = db + .select('chv.chargeVersionId', 'chv.scheme', 'chv.startDate', 'chv.endDate', 'lic.licenceId', 'lic.licenceRef') + .from({ chv: 'water.charge_versions' }) + .innerJoin({ lic: 'water.licences' }, 'chv.licence_id', 'lic.licence_id') + .where({ + scheme: 'sroc', + 'lic.include_in_supplementary_billing': 'yes', + 'lic.region_id': regionId + }) + .andWhere('chv.start_date', '>=', billingPeriod.startDate) + .andWhere('chv.start_date', '<=', billingPeriod.endDate) - return chargeVersions - } + return chargeVersions } -module.exports = FetchChargeVersionsService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/fetch-licences.service.js b/app/services/supplementary-billing/fetch-licences.service.js index 583bf8b400..0354914894 100644 --- a/app/services/supplementary-billing/fetch-licences.service.js +++ b/app/services/supplementary-billing/fetch-licences.service.js @@ -7,6 +7,7 @@ const LicenceModel = require('../../models/licence.model.js') +<<<<<<< HEAD class FetchLicencesService { /** * Fetches licences flagged for supplementary billing that are linked to the selected region @@ -20,19 +21,34 @@ class FetchLicencesService { */ static async go (region) { const licences = await this._fetch(region) +======= +/** + * Fetches licences flagged for supplementary billing that are linked to the selected region + * + * This is a temporary service to help us confirm we are selecting the correct data to use when creating a + * supplementary bill run. Its primary aim is to meet the acceptance criteria defined in WATER-3787. + * + * @param {Objecy} region Instance of `RegionModel` for the selected region + * + * @returns {Object[]} Array of matching `LicenceModel` + */ +async function go (region) { + const licences = await _fetch(region) +>>>>>>> 901b57f (Update services) - return licences - } + return licences +} - static async _fetch (region) { - const result = await LicenceModel.query() - .innerJoinRelated('chargeVersions') - .where('region_id', region.regionId) - .where('include_in_supplementary_billing', 'yes') - .where('chargeVersions.scheme', 'sroc') +async function _fetch (region) { + const result = await LicenceModel.query() + .innerJoinRelated('chargeVersions') + .where('region_id', region.regionId) + .where('include_in_supplementary_billing', 'yes') + .where('chargeVersions.scheme', 'sroc') - return result - } + return result } -module.exports = FetchLicencesService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/fetch-region.service.js b/app/services/supplementary-billing/fetch-region.service.js index c6b926626a..0705534cd6 100644 --- a/app/services/supplementary-billing/fetch-region.service.js +++ b/app/services/supplementary-billing/fetch-region.service.js @@ -7,31 +7,31 @@ const RegionModel = require('../../models/region.model.js') -class FetchRegionService { - /** - * Fetches the region with the matching NALD Region ID - * - * This is a temporary service to help us confirm we are selecting the correct data to use when creating a - * supplementary bill run. Its primary aim is to meet the acceptance criteria defined in WATER-3787. - * - * @param {string} naldRegionId The NALD region ID (a number between 1 to 9, 9 being the test region) for the region - * to find - * - * @returns {Object} Instance of `RegionModel` with the matching NALD Region ID - */ - static async go (naldRegionId) { - const region = await this._fetch(naldRegionId) +/** + * Fetches the region with the matching NALD Region ID + * + * This is a temporary service to help us confirm we are selecting the correct data to use when creating a + * supplementary bill run. Its primary aim is to meet the acceptance criteria defined in WATER-3787. + * + * @param {string} naldRegionId The NALD region ID (a number between 1 to 9, 9 being the test region) for the region + * to find + * + * @returns {Object} Instance of `RegionModel` with the matching NALD Region ID + */ +async function go (naldRegionId) { + const region = await _fetch(naldRegionId) - return region - } + return region +} - static async _fetch (naldRegionId) { - const result = await RegionModel.query() - .where('nald_region_id', naldRegionId) - .first() +async function _fetch (naldRegionId) { + const result = await RegionModel.query() + .where('nald_region_id', naldRegionId) + .first() - return result - } + return result } -module.exports = FetchRegionService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/supplementary.service.js b/app/services/supplementary-billing/supplementary.service.js index 7a982a7a47..5208fda9f4 100644 --- a/app/services/supplementary-billing/supplementary.service.js +++ b/app/services/supplementary-billing/supplementary.service.js @@ -2,6 +2,9 @@ /** * Determines the billing periods, licences and charge versions used to generate an SROC supplementary bill run + * + * WIP: This is currently being used to generate testing data to confirm we are understanding SROC supplementary + * billing. We intend to refactor things so that the service starts representing what is actually required. * @module SupplementaryService */ @@ -11,28 +14,24 @@ const FetchLicencesService = require('./fetch-licences.service.js') const FetchRegionService = require('./fetch-region.service.js') const SupplementaryPresenter = require('../../presenters/supplementary.presenter.js') -/** - * WIP: This is currently being used to generate testing data to confirm we are understanding SROC supplementary - * billing. We intend to refactor things so that the service starts representing what is actually required. - */ -class SupplementaryService { - static async go (naldRegionId) { - const region = await FetchRegionService.go(naldRegionId) - const billingPeriods = BillingPeriodService.go() - const licences = await FetchLicencesService.go(region) +async function go (naldRegionId) { + const region = await FetchRegionService.go(naldRegionId) + const billingPeriods = BillingPeriodService.go() + const licences = await FetchLicencesService.go(region) - // We know in the future we will be calculating multiple billing periods and so will have to iterate through each, - // generating bill runs and reviewing if there is anything to bill. For now, whilst our knowledge of the process - // is low we are focusing on just the current financial year, and intending to ship a working version for just it. - // This is why we are only passing through the first billing period; we know there is only one! - const chargeVersions = await FetchChargeVersionsService.go(region.regionId, billingPeriods[0]) + // We know in the future we will be calculating multiple billing periods and so will have to iterate through each, + // generating bill runs and reviewing if there is anything to bill. For now, whilst our knowledge of the process + // is low we are focusing on just the current financial year, and intending to ship a working version for just it. + // This is why we are only passing through the first billing period; we know there is only one! + const chargeVersions = await FetchChargeVersionsService.go(region.regionId, billingPeriods[0]) - return this._response({ billingPeriods, licences, chargeVersions }) - } + return _response({ billingPeriods, licences, chargeVersions }) +} - static _response (data) { - return SupplementaryPresenter.go(data) - } +function _response (data) { + return SupplementaryPresenter.go(data) } -module.exports = SupplementaryService +module.exports = { + go +} diff --git a/test/services/supplementary-billing/billing-period.service.test.js b/test/services/supplementary-billing/billing-period.service.test.js index fe74618301..563171a44f 100644 --- a/test/services/supplementary-billing/billing-period.service.test.js +++ b/test/services/supplementary-billing/billing-period.service.test.js @@ -12,19 +12,24 @@ const { expect } = Code const BillingPeriodService = require('../../../app/services/supplementary-billing/billing-period.service.js') describe('Billing Period service', () => { + let clock + let expectedResult + let testDate + afterEach(() => { + clock.restore() Sinon.restore() }) describe('when the date is in 2022 and falls within the 2022 financial year', () => { - const testDate = new Date('2022-04-01') - const expectedResult = { - startDate: new Date('2022-04-01'), - endDate: new Date('2023-03-31') - } - beforeEach(async () => { - Sinon.stub(BillingPeriodService, '_currentDate').returns(testDate) + testDate = new Date('2022-04-01') + expectedResult = { + startDate: new Date('2022-04-01'), + endDate: new Date('2023-03-31') + } + + clock = Sinon.useFakeTimers(testDate) }) it('returns the expected date range', () => { @@ -36,14 +41,14 @@ describe('Billing Period service', () => { }) describe('when the date is in 2023 and falls within the 2022 financial year', () => { - const testDate = new Date('2023-03-01') - const expectedResult = { - startDate: new Date('2022-04-01'), - endDate: new Date('2023-03-31') - } - beforeEach(async () => { - Sinon.stub(BillingPeriodService, '_currentDate').returns(testDate) + testDate = new Date('2023-03-01') + expectedResult = { + startDate: new Date('2022-04-01'), + endDate: new Date('2023-03-31') + } + + clock = Sinon.useFakeTimers(testDate) }) it('returns the expected date range', () => { @@ -55,14 +60,14 @@ describe('Billing Period service', () => { }) describe('when the date is in 2023 and falls within the 2023 financial year', () => { - const testDate = new Date('2023-10-10') - const expectedResult = { - startDate: new Date('2023-04-01'), - endDate: new Date('2024-03-31') - } - beforeEach(async () => { - Sinon.stub(BillingPeriodService, '_currentDate').returns(testDate) + testDate = new Date('2023-10-10') + expectedResult = { + startDate: new Date('2023-04-01'), + endDate: new Date('2024-03-31') + } + + clock = Sinon.useFakeTimers(testDate) }) it('returns the expected date range', () => { @@ -72,12 +77,4 @@ describe('Billing Period service', () => { expect(result[0]).to.equal(expectedResult) }) }) - - describe('when the _currentDate function is not stubbed', () => { - it('returns a date range', () => { - const result = BillingPeriodService.go() - - expect(result.length).to.equal(1) - }) - }) }) From b3b8f78c47902e87005aa942973a345cf153376f Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Fri, 9 Dec 2022 09:12:03 +0000 Subject: [PATCH 5/6] Refactor test helpers --- test/support/helpers/charge-version.helper.js | 107 +++++++++--------- test/support/helpers/database.helper.js | 48 ++++---- test/support/helpers/licence.helper.js | 81 ++++++------- test/support/helpers/region.helper.js | 81 ++++++------- 4 files changed, 159 insertions(+), 158 deletions(-) diff --git a/test/support/helpers/charge-version.helper.js b/test/support/helpers/charge-version.helper.js index 9df8cb4b9d..55cfc6924b 100644 --- a/test/support/helpers/charge-version.helper.js +++ b/test/support/helpers/charge-version.helper.js @@ -8,66 +8,67 @@ const { db } = require('../../../db/db.js') const LicenceHelper = require('./licence.helper.js') -class ChargeVersionHelper { - /** - * Add a new charge version - * - * A charge version is always linked to a licence. So, creating a charge version will automatically create a new - * licence and handle linking the two together by `licence_id`. - * - * If no `data` is provided, default values will be used. These are - * - * - `scheme` - sroc - * - `licence_ref` - 01/123 - * - * See `LicenceHelper` for the licence defaults - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - * @param {Object} [licence] Any licence data you want to use instead of the defaults used here or in the database - * - * @returns {string} The ID of the newly created record - */ - static async add (data = {}, licence = {}) { - let licenceId = licence?.licenceId - if (!licenceId) { - licenceId = await this._addLicence(licence) - } +/** + * Add a new charge version + * + * A charge version is always linked to a licence. So, creating a charge version will automatically create a new + * licence and handle linking the two together by `licence_id`. + * + * If no `data` is provided, default values will be used. These are + * + * - `scheme` - sroc + * - `licence_ref` - 01/123 + * + * See `LicenceHelper` for the licence defaults + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + * @param {Object} [licence] Any licence data you want to use instead of the defaults used here or in the database + * + * @returns {string} The ID of the newly created record + */ +async function add (data = {}, licence = {}) { + let licenceId = licence?.licenceId + if (!licenceId) { + licenceId = await _addLicence(licence) + } - const insertData = this.defaults({ ...data, licence_id: licenceId }) + const insertData = defaults({ ...data, licence_id: licenceId }) - const result = await db.table('water.charge_versions') - .insert(insertData) - .returning('charge_version_id') + const result = await db.table('water.charge_versions') + .insert(insertData) + .returning('charge_version_id') - return result - } + return result +} - /** - * Returns the defaults used when creating a new charge version - * - * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available - * for use in tests to avoid having to duplicate values. - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - */ - static defaults (data = {}) { - const defaults = { - licence_ref: '01/123', - scheme: 'sroc', - start_date: new Date('2022-04-01') - } +/** + * Returns the defaults used when creating a new charge version + * + * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available + * for use in tests to avoid having to duplicate values. + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + */ +function defaults (data = {}) { + const defaults = { + licence_ref: '01/123', + scheme: 'sroc', + start_date: new Date('2022-04-01') + } - return { - ...defaults, - ...data - } + return { + ...defaults, + ...data } +} - static async _addLicence (licence) { - const result = await LicenceHelper.add(licence) +async function _addLicence (licence) { + const result = await LicenceHelper.add(licence) - return result[0].licenceId - } + return result[0].licenceId } -module.exports = ChargeVersionHelper +module.exports = { + add, + defaults +} diff --git a/test/support/helpers/database.helper.js b/test/support/helpers/database.helper.js index 051c0d4dee..c79a26db07 100644 --- a/test/support/helpers/database.helper.js +++ b/test/support/helpers/database.helper.js @@ -1,42 +1,40 @@ 'use strict' /** + * Use to help with cleaning the database between tests + * + * It's good practise to ensure the database is in a 'clean' state between tests to avoid any side effects caused by + * data from one test being present in another. * @module DatabaseHelper */ const { db } = require('../../../db/db.js') /** - * Use to help with cleaning the database between tests + * Call to clean the database of all data * - * It's good practise to ensure the database is in a 'clean' state between tests to avoid any side effects caused by - * data from one test being present in another. + * It works by identifying all the tables in each schema which we use. + * + * Once it has that info it creates a query that tells PostgreSQL to TRUNCATE all the tables and restart their + * identity columns. For example, if a table relies on an incrementing ID the query will reset that to 1. */ -class DatabaseHelper { - /** - * Call to clean the database of all data - * - * It works by identifying all the tables in each schema which we use. - * - * Once it has that info it creates a query that tells PostgreSQL to TRUNCATE all the tables and restart their - * identity columns. For example, if a table relies on an incrementing ID the query will reset that to 1. - */ - static async clean () { - const schemas = ['water'] +async function clean () { + const schemas = ['water'] - for (const schema of schemas) { - const tables = await this._tableNames(schema) - await db.raw(`TRUNCATE TABLE ${tables.join(',')} RESTART IDENTITY`) - } + for (const schema of schemas) { + const tables = await _tableNames(schema) + await db.raw(`TRUNCATE TABLE ${tables.join(',')} RESTART IDENTITY`) } +} - static async _tableNames (schema) { - const result = await db('pg_tables') - .select('tablename') - .where('schemaname', schema) +async function _tableNames (schema) { + const result = await db('pg_tables') + .select('tablename') + .where('schemaname', schema) - return result.map((table) => `${schema}.${table.tablename}`) - } + return result.map((table) => `${schema}.${table.tablename}`) } -module.exports = DatabaseHelper +module.exports = { + clean +} diff --git a/test/support/helpers/licence.helper.js b/test/support/helpers/licence.helper.js index 461996440d..072c8b078f 100644 --- a/test/support/helpers/licence.helper.js +++ b/test/support/helpers/licence.helper.js @@ -6,48 +6,49 @@ const { db } = require('../../../db/db.js') -class LicenceHelper { - /** - * Add a new licence - * - * If no `data` is provided, default values will be used. These are - * - * - `licence_ref` - 01/123 - * - `region_id` - bd114474-790f-4470-8ba4-7b0cc9c225d7 - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - * - * @returns {string} The ID of the newly created record - */ - static async add (data = {}) { - const insertData = this.defaults(data) - - const result = await db.table('water.licences') - .insert(insertData) - .returning('licence_id') - - return result +/** + * Add a new licence + * + * If no `data` is provided, default values will be used. These are + * + * - `licence_ref` - 01/123 + * - `region_id` - bd114474-790f-4470-8ba4-7b0cc9c225d7 + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + * + * @returns {string} The ID of the newly created record + */ +async function add (data = {}) { + const insertData = defaults(data) + + const result = await db.table('water.licences') + .insert(insertData) + .returning('licence_id') + + return result +} + +/** + * Returns the defaults used when creating a new licence + * + * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available + * for use in tests to avoid having to duplicate values. + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + */ +function defaults (data = {}) { + const defaults = { + licence_ref: '01/123', + region_id: 'bd114474-790f-4470-8ba4-7b0cc9c225d7' } - /** - * Returns the defaults used when creating a new licence - * - * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available - * for use in tests to avoid having to duplicate values. - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - */ - static defaults (data = {}) { - const defaults = { - licence_ref: '01/123', - region_id: 'bd114474-790f-4470-8ba4-7b0cc9c225d7' - } - - return { - ...defaults, - ...data - } + return { + ...defaults, + ...data } } -module.exports = LicenceHelper +module.exports = { + add, + defaults +} diff --git a/test/support/helpers/region.helper.js b/test/support/helpers/region.helper.js index da89cb9c42..1434eda4bd 100644 --- a/test/support/helpers/region.helper.js +++ b/test/support/helpers/region.helper.js @@ -6,48 +6,49 @@ const { db } = require('../../../db/db.js') -class RegionHelper { - /** - * Add a new region - * - * If no `data` is provided, default values will be used. These are - * - * - `charge_region_id` - S - * - `nald_region_id` - 9 - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - * - * @returns {string} The ID of the newly created record - */ - static async add (data = {}) { - const insertData = this.defaults(data) - - const result = await db.table('water.regions') - .insert(insertData) - .returning('region_id') - - return result +/** + * Add a new region + * + * If no `data` is provided, default values will be used. These are + * + * - `charge_region_id` - S + * - `nald_region_id` - 9 + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + * + * @returns {string} The ID of the newly created record + */ +async function add (data = {}) { + const insertData = defaults(data) + + const result = await db.table('water.regions') + .insert(insertData) + .returning('region_id') + + return result +} + +/** + * Returns the defaults used when creating a new region + * + * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available + * for use in tests to avoid having to duplicate values. + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + */ +function defaults (data = {}) { + const defaults = { + charge_region_id: 'S', + nald_region_id: 9 } - /** - * Returns the defaults used when creating a new region - * - * It will override or append to them any data provided. Mainly used by the `add()` method, we make it available - * for use in tests to avoid having to duplicate values. - * - * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database - */ - static defaults (data = {}) { - const defaults = { - charge_region_id: 'S', - nald_region_id: 9 - } - - return { - ...defaults, - ...data - } + return { + ...defaults, + ...data } } -module.exports = RegionHelper +module.exports = { + add, + defaults +} From 6d3d1a399600de8c3c064e2139a289226e922721 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Fri, 9 Dec 2022 10:41:35 +0000 Subject: [PATCH 6/6] Final tweaks and fixes post merge --- .../create-billing-batch.service.js | 46 +++++++++---------- .../fetch-licences.service.js | 18 +------- 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/app/services/supplementary-billing/create-billing-batch.service.js b/app/services/supplementary-billing/create-billing-batch.service.js index bb7c39a061..6a0db47031 100644 --- a/app/services/supplementary-billing/create-billing-batch.service.js +++ b/app/services/supplementary-billing/create-billing-batch.service.js @@ -7,29 +7,29 @@ const BillingBatchModel = require('../../models/billing-batch.model.js') -class CreateBillingBatchService { - /** - * Create a new billing batch - * - * @param {Object} [regionId] The region_id for the selected region - * @param {Object} [billingPeriod] The billing period in the format { startDate: 2022-04-01, endDate: 2023-03-31 } - * - * @returns {Object} The newly created billing batch record - */ - static async go (regionId, billingPeriod) { - const billingBatch = await BillingBatchModel.query() - .insert({ - region_id: regionId, - batch_type: 'supplementary', - from_financial_year_ending: billingPeriod.endDate.getFullYear(), - to_financial_year_ending: billingPeriod.endDate.getFullYear(), - status: 'processing', - scheme: 'sroc' - }) - .returning('*') +/** + * Create a new billing batch + * + * @param {Object} [regionId] The region_id for the selected region + * @param {Object} [billingPeriod] The billing period in the format { startDate: 2022-04-01, endDate: 2023-03-31 } + * + * @returns {Object} The newly created billing batch record + */ +async function go (regionId, billingPeriod) { + const billingBatch = await BillingBatchModel.query() + .insert({ + region_id: regionId, + batch_type: 'supplementary', + from_financial_year_ending: billingPeriod.endDate.getFullYear(), + to_financial_year_ending: billingPeriod.endDate.getFullYear(), + status: 'processing', + scheme: 'sroc' + }) + .returning('*') - return billingBatch - } + return billingBatch } -module.exports = CreateBillingBatchService +module.exports = { + go +} diff --git a/app/services/supplementary-billing/fetch-licences.service.js b/app/services/supplementary-billing/fetch-licences.service.js index 0354914894..d07cc8df2f 100644 --- a/app/services/supplementary-billing/fetch-licences.service.js +++ b/app/services/supplementary-billing/fetch-licences.service.js @@ -7,34 +7,18 @@ const LicenceModel = require('../../models/licence.model.js') -<<<<<<< HEAD -class FetchLicencesService { - /** - * Fetches licences flagged for supplementary billing that are linked to the selected region - * - * This is a temporary service to help us confirm we are selecting the correct data to use when creating a - * supplementary bill run. Its primary aim is to meet the acceptance criteria defined in WATER-3787. - * - * @param {Object} region Instance of `RegionModel` for the selected region - * - * @returns {Object[]} Array of matching `LicenceModel` - */ - static async go (region) { - const licences = await this._fetch(region) -======= /** * Fetches licences flagged for supplementary billing that are linked to the selected region * * This is a temporary service to help us confirm we are selecting the correct data to use when creating a * supplementary bill run. Its primary aim is to meet the acceptance criteria defined in WATER-3787. * - * @param {Objecy} region Instance of `RegionModel` for the selected region + * @param {Object} region Instance of `RegionModel` for the selected region * * @returns {Object[]} Array of matching `LicenceModel` */ async function go (region) { const licences = await _fetch(region) ->>>>>>> 901b57f (Update services) return licences }