From 31fe625c4e119c24409744777b22e0066a86f267 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 24 Jan 2024 09:37:52 +0000 Subject: [PATCH] Move getting the licence holder to the model (#683) https://eaflood.atlassian.net/browse/WATER-4188 https://eaflood.atlassian.net/browse/WATER-4328 Much like in [Move licence end date logic to model](https://github.com/DEFRA/water-abstraction-system/pull/681) we have another two tickets that need to identify the licence holder for a licence. We've already figured out how to do that in [Add licence ref. and holder to rtn. req. session](https://github.com/DEFRA/water-abstraction-system/pull/639). But that implementation was specific to return requirements and unlike `myContact.$name()` and `myLicence.$ends()` to determine the licence holder you need to have pulled through a series of related models. So, for the same reasons we want to move this piece of logic to the model to make it easier to reuse. However, there will be differences in implementation. This change adds the logic to the model and then refactors `InitiateReturnRequirementSessionService` to demonstrate how to use it. --- app/models/licence.model.js | 98 ++++++++++++ ...iate-return-requirement-session.service.js | 57 +------ test/models/licence.model.test.js | 100 ++++++++++++ ...return-requirement-session.service.test.js | 142 ++++-------------- 4 files changed, 232 insertions(+), 165 deletions(-) diff --git a/app/models/licence.model.js b/app/models/licence.model.js index b5694e79f1..c68b12cb0a 100644 --- a/app/models/licence.model.js +++ b/app/models/licence.model.js @@ -75,6 +75,65 @@ class LicenceModel extends BaseModel { } } + /** + * Modifiers allow us to reuse logic in queries, eg. select the licence and everything to get the licence holder: + * + * return LicenceModel.query() + * .findById(licenceId) + * .modify('licenceHolder') + * + * See {@link https://vincit.github.io/objection.js/recipes/modifiers.html | Modifiers} for more details + */ + static get modifiers () { + return { + /** + * licenceHolder modifier fetches all the joined records needed to identify the licence holder + */ + licenceHolder (query) { + query + .withGraphFetched('licenceDocument') + .modifyGraph('licenceDocument', (builder) => { + builder.select([ + 'id' + ]) + }) + .withGraphFetched('licenceDocument.licenceDocumentRoles') + .modifyGraph('licenceDocument.licenceDocumentRoles', (builder) => { + builder + .select([ + 'licenceDocumentRoles.id' + ]) + .innerJoinRelated('licenceRole') + .where('licenceRole.name', 'licenceHolder') + .orderBy('licenceDocumentRoles.startDate', 'desc') + }) + .withGraphFetched('licenceDocument.licenceDocumentRoles.company') + .modifyGraph('licenceDocument.licenceDocumentRoles.company', (builder) => { + builder.select([ + 'id', + 'name', + 'type' + ]) + }) + .withGraphFetched('licenceDocument.licenceDocumentRoles.contact') + .modifyGraph('licenceDocument.licenceDocumentRoles.contact', (builder) => { + builder.select([ + 'id', + 'contactType', + 'dataSource', + 'department', + 'firstName', + 'initials', + 'lastName', + 'middleInitials', + 'salutation', + 'suffix' + ]) + }) + } + } + } + /** * Determine the 'end' date for the licence * @@ -132,6 +191,45 @@ class LicenceModel extends BaseModel { return filteredDates[0] } + + /** + * Determine the name of the licence holder for the licence + * + * > We recommend adding the `licenceHolder` modifier to your query to ensure the joined records are available to + * > determine this + * + * Every licence has a licence holder. They may be a company or a person (held as a 'contact' record). This + * information is stored in 'licence document roles' and because the licence holder can change, there may be more + * than one record. + * + * To get to the 'licence document roles' we have to go via the linked 'licence document' and ensure we sort by their + * start date so that we have the 'current' licence holder. Thankfully, the `licenceHolder` query modifier deals + * with this for us. + * + * Every licence is always linked to a 'company' record. But if they are also linked to a 'contact' it takes + * precedence when determining the licence holder name. + * + * @returns {(string|null)} `null` if this instance does not have the additional properties needed to determine the + * licence holder else the licence holder's name + */ + $licenceHolder () { + // Extract the company and contact from the last licenceDocumentRole created. It is assumed that the + // `licenceHolder` modifier has been used to get the additional records needed for this. It also ensures in the case + // that there is more than one that they are ordered by their start date (DESC) + const latestLicenceDocumentRole = this?.licenceDocument?.licenceDocumentRoles[0] + + if (!latestLicenceDocumentRole) { + return null + } + + const { company, contact } = latestLicenceDocumentRole + + if (contact) { + return contact.$name() + } + + return company.name + } } module.exports = LicenceModel diff --git a/app/services/return-requirements/initiate-return-requirement-session.service.js b/app/services/return-requirements/initiate-return-requirement-session.service.js index ae7fd15419..955cbd779a 100644 --- a/app/services/return-requirements/initiate-return-requirement-session.service.js +++ b/app/services/return-requirements/initiate-return-requirement-session.service.js @@ -44,7 +44,7 @@ async function _createSession (data) { } function _data (licence, journey) { - const { id, licenceDocument, licenceRef, licenceVersions } = licence + const { id, licenceRef, licenceVersions } = licence const ends = licence.$ends() return { @@ -52,7 +52,7 @@ function _data (licence, journey) { id, endDate: ends ? ends.date : null, licenceRef, - licenceHolder: _licenceHolder(licenceDocument), + licenceHolder: licence.$licenceHolder(), startDate: _startDate(licenceVersions) }, journey @@ -79,45 +79,8 @@ async function _fetchLicence (licenceId) { .where('status', 'current') .orderBy('startDate', 'desc') }) - .withGraphFetched('licenceDocument') - .modifyGraph('licenceDocument', (builder) => { - builder.select([ - 'id' - ]) - }) - .withGraphFetched('licenceDocument.licenceDocumentRoles') - .modifyGraph('licenceDocument.licenceDocumentRoles', (builder) => { - builder - .select([ - 'licenceDocumentRoles.id' - ]) - .innerJoinRelated('licenceRole') - .where('licenceRole.name', 'licenceHolder') - .orderBy('licenceDocumentRoles.startDate', 'desc') - }) - .withGraphFetched('licenceDocument.licenceDocumentRoles.company') - .modifyGraph('licenceDocument.licenceDocumentRoles.company', (builder) => { - builder.select([ - 'id', - 'name', - 'type' - ]) - }) - .withGraphFetched('licenceDocument.licenceDocumentRoles.contact') - .modifyGraph('licenceDocument.licenceDocumentRoles.contact', (builder) => { - builder.select([ - 'id', - 'contactType', - 'dataSource', - 'department', - 'firstName', - 'initials', - 'lastName', - 'middleInitials', - 'salutation', - 'suffix' - ]) - }) + // See licence.model.js `static get modifiers` if you are unsure about what this is doing + .modify('licenceHolder') if (!licence) { throw Boom.notFound('Licence for new return requirement not found', { id: licenceId }) @@ -126,18 +89,6 @@ async function _fetchLicence (licenceId) { return licence } -function _licenceHolder (licenceDocument) { - // Extract the company and contact from the last licenceDocumentRole created. _fetchLicence() ensures in the case - // that there is more than one that they are ordered by their start date (DESC) - const { company, contact } = licenceDocument.licenceDocumentRoles[0] - - if (contact) { - return contact.$name() - } - - return company.name -} - function _startDate (licenceVersions) { // Extract the start date from the most 'current' licence version. _fetchLicence() ensures in the case // that there is more than one that they are ordered by their start date (DESC) diff --git a/test/models/licence.model.test.js b/test/models/licence.model.test.js index 732c56f200..7a0df1e6f8 100644 --- a/test/models/licence.model.test.js +++ b/test/models/licence.model.test.js @@ -12,12 +12,16 @@ const BillLicenceHelper = require('../support/helpers/bill-licence.helper.js') const BillLicenceModel = require('../../app/models/bill-licence.model.js') const ChargeVersionHelper = require('../support/helpers/charge-version.helper.js') const ChargeVersionModel = require('../../app/models/charge-version.model.js') +const CompanyHelper = require('../support/helpers/company.helper.js') +const ContactHelper = require('../support/helpers/contact.helper.js') const DatabaseHelper = require('../support/helpers/database.helper.js') const LicenceHelper = require('../support/helpers/licence.helper.js') const LicenceDocumentHelper = require('../support/helpers/licence-document.helper.js') const LicenceDocumentModel = require('../../app/models/licence-document.model.js') const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document-header.helper.js') const LicenceDocumentHeaderModel = require('../../app/models/licence-document-header.model.js') +const LicenceDocumentRoleHelper = require('../support/helpers/licence-document-role.helper.js') +const LicenceRoleHelper = require('../support/helpers/licence-role.helper.js') const LicenceVersionHelper = require('../support/helpers/licence-version.helper.js') const LicenceVersionModel = require('../../app/models/licence-version.model.js') const RegionHelper = require('../support/helpers/region.helper.js') @@ -462,4 +466,100 @@ describe('Licence model', () => { }) }) }) + + describe('$licenceHolder', () => { + describe('when instance has not been set with the additional properties needed', () => { + it('returns null', () => { + const result = testRecord.$licenceHolder() + + expect(result).to.be.null() + }) + }) + + describe('when the instance has been set with the additional properties needed', () => { + const licenceRoles = {} + + let licence + let company + let contact + let licenceDocument + + beforeEach(async () => { + licence = await LicenceHelper.add() + + // Create 2 licence roles so we can test the service only gets the licence document role record that is for + // 'licence holder' + licenceRoles.billing = await LicenceRoleHelper.add({ name: 'billing', label: 'Billing' }) + licenceRoles.holder = await LicenceRoleHelper.add() + + // Create company and contact records. We create an additional company so we can create 2 licence document role + // records for our licence to test the one with the latest start date is used. + company = await CompanyHelper.add({ name: 'Licence Holder Ltd' }) + contact = await ContactHelper.add({ firstName: 'Luce', lastName: 'Holder' }) + const oldCompany = await CompanyHelper.add({ name: 'Old Licence Holder Ltd' }) + + // We have to create a licence document to link our licence record to (eventually!) the company or contact + // record that is the 'licence holder' + licenceDocument = await LicenceDocumentHelper.add({ licenceRef: licence.licenceRef }) + + // Create two licence document role records. This one is linked to the billing role so should be ignored by the + // service + await LicenceDocumentRoleHelper.add({ + licenceDocumentId: licenceDocument.id, + licenceRoleId: licenceRoles.billing.id + }) + + // This one is linked to the old company record so should not be used to provide the licence holder name + await LicenceDocumentRoleHelper.add({ + licenceDocumentId: licenceDocument.id, + licenceRoleId: licenceRoles.holder.id, + company: oldCompany.id, + startDate: new Date('2022-01-01') + }) + }) + + describe('and the licence holder is a company', () => { + beforeEach(async () => { + // Create the licence document role record that _is_ linked to the correct licence holder record + await LicenceDocumentRoleHelper.add({ + licenceDocumentId: licenceDocument.id, + licenceRoleId: licenceRoles.holder.id, + companyId: company.id, + startDate: new Date('2022-08-01') + }) + + testRecord = await LicenceModel.query().findById(licence.id).modify('licenceHolder') + }) + + it('returns the company name as the licence holder', async () => { + const result = testRecord.$licenceHolder() + + expect(result).to.equal('Licence Holder Ltd') + }) + }) + + describe('and the licence holder is a contact', () => { + beforeEach(async () => { + // Create the licence document role record that _is_ linked to the correct licence holder record. + // NOTE: We create this against both the company and contact to also confirm that the contact name has + // precedence over the company name + await LicenceDocumentRoleHelper.add({ + licenceDocumentId: licenceDocument.id, + licenceRoleId: licenceRoles.holder.id, + companyId: company.id, + contactId: contact.id, + startDate: new Date('2022-08-01') + }) + + testRecord = await LicenceModel.query().findById(licence.id).modify('licenceHolder') + }) + + it('returns the contact name as the licence holder', async () => { + const result = testRecord.$licenceHolder() + + expect(result).to.equal('Luce Holder') + }) + }) + }) + }) }) diff --git a/test/services/return-requirements/initiate-return-requirement-session.service.test.js b/test/services/return-requirements/initiate-return-requirement-session.service.test.js index 5ba1f77c4f..9d2903fdf5 100644 --- a/test/services/return-requirements/initiate-return-requirement-session.service.test.js +++ b/test/services/return-requirements/initiate-return-requirement-session.service.test.js @@ -9,7 +9,6 @@ const { expect } = Code // Test helpers const CompanyHelper = require('../../support/helpers/company.helper.js') -const ContactHelper = require('../../support/helpers/contact.helper.js') const DatabaseHelper = require('../../support/helpers/database.helper.js') const LicenceHelper = require('../../support/helpers/licence.helper.js') const LicenceDocumentHelper = require('../../support/helpers/licence-document.helper.js') @@ -30,12 +29,6 @@ describe('Initiate Return Requirement Session service', () => { describe('when called', () => { describe('and the licence exists', () => { - const licenceRoles = {} - - let company - let contact - let licenceDocument - beforeEach(async () => { // Create the licence record with an 'end' date so we can confirm the session gets populated with the licence's // 'ends' information @@ -49,134 +42,59 @@ describe('Initiate Return Requirement Session service', () => { licenceId: licence.id, startDate: new Date('2022-05-01') }) - // Create 2 licence roles so we can test the service only gets the licence document role record that is for - // 'licence holder' - licenceRoles.billing = await LicenceRoleHelper.add({ name: 'billing', label: 'Billing' }) - licenceRoles.holder = await LicenceRoleHelper.add() + // Create a licence role (the default is licenceHolder) + const licenceRole = await LicenceRoleHelper.add() - // Create company and contact records. We create an additional company so we can create 2 licence document role - // records for our licence to test the one with the latest start date is used. - company = await CompanyHelper.add({ name: 'Licence Holder Ltd' }) - contact = await ContactHelper.add({ firstName: 'Luce', lastName: 'Holder' }) - const oldCompany = await CompanyHelper.add({ name: 'Old Licence Holder Ltd' }) + // Create a company record + const company = await CompanyHelper.add({ name: 'Licence Holder Ltd' }) // We have to create a licence document to link our licence record to (eventually!) the company or contact // record that is the 'licence holder' - licenceDocument = await LicenceDocumentHelper.add({ licenceRef: licence.licenceRef }) + const licenceDocument = await LicenceDocumentHelper.add({ licenceRef: licence.licenceRef }) - // Create two licence document role records. This one is linked to the billing role so should be ignored by the - // service + // Create the licence document role record that _is_ linked to the correct licence holder record await LicenceDocumentRoleHelper.add({ licenceDocumentId: licenceDocument.id, - licenceRoleId: licenceRoles.billing.id + licenceRoleId: licenceRole.id, + companyId: company.id, + startDate: new Date('2022-08-01') }) - // This one is linked to the old company record so should not be used to provide the licence holder name - await LicenceDocumentRoleHelper.add({ - licenceDocumentId: licenceDocument.id, - licenceRoleId: licenceRoles.holder.id, - company: oldCompany.id, - startDate: new Date('2022-01-01') - }) + journey = 'returns-required' }) - describe('and the licence holder is a company', () => { - beforeEach(async () => { - // Create the licence document role record that _is_ linked to the correct licence holder record - await LicenceDocumentRoleHelper.add({ - licenceDocumentId: licenceDocument.id, - licenceRoleId: licenceRoles.holder.id, - companyId: company.id, - startDate: new Date('2022-08-01') - }) - - journey = 'returns-required' - }) - - it('creates a new session record containing details of the licence and licence holder (company)', async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - - const { data } = result + it('creates a new session record containing details of the licence and licence holder', async () => { + const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - expect(data.licence.id).to.equal(licence.id) - expect(data.licence.licenceRef).to.equal(licence.licenceRef) - expect(data.licence.licenceHolder).to.equal('Licence Holder Ltd') - }) - - it("creates a new session record containing the licence's 'current' start date", async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - - const { data } = result - - expect(data.licence.startDate).to.equal(new Date('2022-05-01')) - }) - - it("creates a new session record containing the licence's end date", async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) + const { data } = result - const { data } = result - - expect(data.licence.endDate).to.equal(new Date('2024-08-10')) - }) - - it('creates a new session record containing the journey passed in', async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - - const { data } = result - - expect(data.journey).to.equal(journey) - }) + expect(data.licence.id).to.equal(licence.id) + expect(data.licence.licenceRef).to.equal(licence.licenceRef) + expect(data.licence.licenceHolder).to.equal('Licence Holder Ltd') }) - describe('and the licence holder is a contact', () => { - beforeEach(async () => { - // Create the licence document role record that _is_ linked to the correct licence holder record. - // NOTE: We create this against both the company and contact to also confirm that the contact name has - // precedence over the company name - await LicenceDocumentRoleHelper.add({ - licenceDocumentId: licenceDocument.id, - licenceRoleId: licenceRoles.holder.id, - companyId: company.id, - contactId: contact.id, - startDate: new Date('2022-08-01') - }) - - journey = 'no-returns-required' - }) - - it('creates a new session record containing details of the licence and licence holder (contact)', async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) + it("creates a new session record containing the licence's 'current' start date", async () => { + const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - const { data } = result - - expect(data.licence.id).to.equal(licence.id) - expect(data.licence.licenceRef).to.equal(licence.licenceRef) - expect(data.licence.licenceHolder).to.equal('Luce Holder') - }) + const { data } = result - it("creates a new session record containing the licence's 'current' start date", async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - - const { data } = result - - expect(data.licence.startDate).to.equal(new Date('2022-05-01')) - }) + expect(data.licence.startDate).to.equal(new Date('2022-05-01')) + }) - it("creates a new session record containing the licence's end date", async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) + it("creates a new session record containing the licence's end date", async () => { + const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - const { data } = result + const { data } = result - expect(data.licence.endDate).to.equal(new Date('2024-08-10')) - }) + expect(data.licence.endDate).to.equal(new Date('2024-08-10')) + }) - it('creates a new session record containing the journey passed in', async () => { - const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) + it('creates a new session record containing the journey passed in', async () => { + const result = await InitiateReturnRequirementSessionService.go(licence.id, journey) - const { data } = result + const { data } = result - expect(data.journey).to.equal(journey) - }) + expect(data.journey).to.equal(journey) }) })