From 292e1bdc37de8af07dd53a00a8cceecd245cc1f4 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 7 Jan 2024 22:25:41 +0000 Subject: [PATCH 1/9] Add DocumentHeaderModel from crm.document_headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://eaflood.atlassian.net/browse/WATER-4292 > This is part of a series of changes to add the ability to identify the licence holder for a licence Arghhhhh! 😱🤬😩 We've not long ago added 3 new models to this project - [LicenceDocumentModel](https://github.com/DEFRA/water-abstraction-system/pull/618) - [LicenceRoleModel](https://github.com/DEFRA/water-abstraction-system/pull/629) - [LicenceDocumentRoleModel](https://github.com/DEFRA/water-abstraction-system/pull/630) We then [linked the LicenceModel to LicenceDocumentModel](https://github.com/DEFRA/water-abstraction-system/pull/632). All this was based on a first pass of what we thought the [water-abstraction-ui](https://github.com/DEFRA/water-abstraction-ui), [water-abstraction-service](https://github.com/DEFRA/water-abstraction-service) and [water-abstraction-tactical-crm](https://github.com/DEFRA/water-abstraction-tactical-crm) apps were doing to determine the licence holder. It reflected what we found in the tables behind the 3 models we added as well. We'd just [coded up a service](https://github.com/DEFRA/water-abstraction-system/pull/639) to set the return requirement setup session with the name of the licence holder and begun testing when we found an issue in testing. We came across a licence where neither the company or contact reflected what we saw in the UI for the licence holder! In the UI beneath the licence holder name is a 'View licence contact details' link. We were able to figure out that the ID it uses is for a `crm.document_headers` record. And in there we found (surprise-surprise!) another JSONB field holding a bunch of data, including the licence holder name we were seeing on screen. Another dive into the legacy code revealed we'd taken a wrong turning. When the page is requested there is logic that grabs records from the tables we created models for. But they are not a used. In fact, not even the `crm.document_headers` record is used, other than to provide an ID for the link. What seems to happen (we could be wrong again!) is that the `permit.licence` table is queried and its `licence_data_value` field is extracted. If you haven't guessed already, this is another JSON field. Only this one seems to be a complete 'dump' of everything imported from NALD for the licence. This data gets passed to `src/lib/licence-transformer/nald-transformer.js` which transforms it from the NALD structure into a 'licence'. This includes a `licenceHolderFullName` property which is eventually what appears in the UI. We have absolutely _zero_ intent to start importing and transforming raw NALD data just to get the licence holder name. Thankfully, in the follow up testing we've been doing the information held in `crm.document_headers` has always matched what we see in the UI. So, this is the start of our second attempt to crack licence holder name starting with adding the `DocumentHeader` model. From 60e5fadfeacadafe04ad659c8cf84451fb3eda39 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 7 Jan 2024 23:11:59 +0000 Subject: [PATCH 2/9] Add migration to create the schema Also ensure it is included in our database helper clean up. --- .../legacy/20240107231129_create-crm-schema.js | 13 +++++++++++++ test/support/helpers/database.helper.js | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 db/migrations/legacy/20240107231129_create-crm-schema.js diff --git a/db/migrations/legacy/20240107231129_create-crm-schema.js b/db/migrations/legacy/20240107231129_create-crm-schema.js new file mode 100644 index 0000000000..f34e3a05de --- /dev/null +++ b/db/migrations/legacy/20240107231129_create-crm-schema.js @@ -0,0 +1,13 @@ +'use strict' + +exports.up = function (knex) { + return knex.raw(` + CREATE SCHEMA IF NOT EXISTS "crm"; + `) +} + +exports.down = function (knex) { + return knex.raw(` + DROP SCHEMA IF EXISTS "crm"; + `) +} diff --git a/test/support/helpers/database.helper.js b/test/support/helpers/database.helper.js index d73751ead6..08bf425b0b 100644 --- a/test/support/helpers/database.helper.js +++ b/test/support/helpers/database.helper.js @@ -19,7 +19,7 @@ const { db } = require('../../../db/db.js') * identity columns. For example, if a table relies on an incrementing ID the query will reset that to 1. */ async function clean () { - const schemas = ['crm_v2', 'idm', 'water', 'returns'] + const schemas = ['crm', 'crm_v2', 'idm', 'water', 'returns'] for (const schema of schemas) { const tables = await _tableNames(schema) From b564488d5c1d65be38ffa8801dc80ae15e7e943b Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 7 Jan 2024 23:29:46 +0000 Subject: [PATCH 3/9] Create LicenceDocumentHeader legacy migration --- ...240107231603_create-crm-document-header.js | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 db/migrations/legacy/20240107231603_create-crm-document-header.js diff --git a/db/migrations/legacy/20240107231603_create-crm-document-header.js b/db/migrations/legacy/20240107231603_create-crm-document-header.js new file mode 100644 index 0000000000..bb6be6d055 --- /dev/null +++ b/db/migrations/legacy/20240107231603_create-crm-document-header.js @@ -0,0 +1,40 @@ +'use strict' + +const tableName = 'document_header' + +exports.up = function (knex) { + return knex + .schema + .withSchema('crm') + .createTable(tableName, (table) => { + // Primary Key + table.uuid('document_id').primary().defaultTo(knex.raw('gen_random_uuid()')) + + // Data + table.string('regime_entity_id').notNullable() + table.string('system_id').notNullable().defaultTo('permit-repo') + table.string('system_internal_id').notNullable() + table.string('system_external_id').notNullable() + table.jsonb('metadata') + table.string('company_entity_id') + table.string('verification_id') + table.string('document_name') + table.date('date_deleted') + + // Legacy timestamps + table.timestamp('date_created').notNullable().defaultTo(knex.fn.now()) + table.timestamp('date_updated').notNullable().defaultTo(knex.fn.now()) + + table.unique( + ['system_id', 'system_internal_id', 'regime_entity_id'], + { useConstraint: true, indexName: 'external_key' } + ) + }) +} + +exports.down = function (knex) { + return knex + .schema + .withSchema('crm') + .dropTableIfExists(tableName) +} From 597ff5a802c50b4be218ae12a16d077ee55f423d Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 7 Jan 2024 23:37:25 +0000 Subject: [PATCH 4/9] Create LicenceDocumentHeader view migration --- ...7233126_create-licence-document-headers.js | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 db/migrations/public/20240107233126_create-licence-document-headers.js diff --git a/db/migrations/public/20240107233126_create-licence-document-headers.js b/db/migrations/public/20240107233126_create-licence-document-headers.js new file mode 100644 index 0000000000..ceb9da1df8 --- /dev/null +++ b/db/migrations/public/20240107233126_create-licence-document-headers.js @@ -0,0 +1,34 @@ +'use strict' + +const viewName = 'licence_document_headers' + +exports.up = function (knex) { + return knex + .schema + .createView(viewName, (view) => { + // NOTE: We have commented out unused columns from the source table + view.as(knex('document_header').withSchema('crm').select([ + 'document_id AS id', + // This could be ignored as it is always set to the same ID. But that id comes from a single record in the + // crm.entity table which has the `entity_type` regime. So, for the purposes of testing we just have to live + // with always populating it even though we don't care about it. + 'regime_entity_id', + // 'system_id', + 'system_internal_id AS nald_id', + 'system_external_id AS licence_ref', + 'metadata', + // 'company_entity_id', + // 'verification_id', + // 'document_name', + 'date_created AS created_at', + 'date_updated AS updated_at', + 'date_deleted AS deleted_at' + ])) + }) +} + +exports.down = function (knex) { + return knex + .schema + .dropViewIfExists(viewName) +} From a7d64a72a75f5ca9de3aa50286537f93bfbdbfc0 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 7 Jan 2024 23:50:27 +0000 Subject: [PATCH 5/9] Create LicenceDocumentHeader model --- app/models/licence-document-header.model.js | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 app/models/licence-document-header.model.js diff --git a/app/models/licence-document-header.model.js b/app/models/licence-document-header.model.js new file mode 100644 index 0000000000..0c65c889b3 --- /dev/null +++ b/app/models/licence-document-header.model.js @@ -0,0 +1,45 @@ +'use strict' + +/** + * Model for licence_document_headers (crm.document_header) + * @module LicenceDocumentHeaderModel + */ + +const BaseModel = require('./base.model.js') + +/** + * Represents an instance of a licence document header record + * + * For reference, the licence document header record is a 'nothing' record! It doesn't hold anything not already stored + * in other licence tables. We only need to obtain the licence holder name which matches what the legacy UI displays. + * + * We think the reason for the table being there is because of the original ambition to have a generic permit + * repository that could be used for all types of permits. Along with that a 'tactical' CRM that interfaced with the + * permit repository was built. Though the permit repository referred to them as licences, the CRM chose to refer to + * them as 'documents' hence the `crm.document_header` table. + * + * The previous team then tried to refactor the CRM schema but never completed it. Hence we have the `crm_v2` schema + * and more licence duplication. Finally, at a later date the previous team then opted to create a `licences` table in + * the `water` schema we think to support charging. + * + * So, if you see the phrase 'Document' you can assume the instance is one of these older copies of a licence. + * `water.licences` is the primary licence record. But we often have to dip into this older tables for other bits of + * information, for example, the licence holder name currently displayed in the legacy UI. This is why we have models + * like this one. + * + * Welcome to dealing with the legacy database schema! ¯\_(ツ)_/¯ + */ +class LicenceDocumentHeaderModel extends BaseModel { + static get tableName () { + return 'licenceDocumentHeaders' + } + + // Defining which fields contain json allows us to insert an object without needing to stringify it first + static get jsonAttributes () { + return [ + 'metadata' + ] + } +} + +module.exports = LicenceDocumentHeaderModel From 88c7af721a3f3d3c77b4268613cd22e6743534fd Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 00:30:37 +0000 Subject: [PATCH 6/9] Create LicenceDocumentHeader helper --- .../helpers/licence-document-header.helper.js | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 test/support/helpers/licence-document-header.helper.js diff --git a/test/support/helpers/licence-document-header.helper.js b/test/support/helpers/licence-document-header.helper.js new file mode 100644 index 0000000000..059e92a0ef --- /dev/null +++ b/test/support/helpers/licence-document-header.helper.js @@ -0,0 +1,97 @@ +'use strict' + +/** + * @module LicenceDocumentHelper + */ + +const { randomInteger } = require('../helpers/general.helper.js') +const { generateUUID } = require('../../../app/lib/general.lib.js') +const { generateLicenceRef } = require('./licence.helper.js') +const LicenceDocumentHeaderModel = require('../../../app/models/licence-document-header.model.js') + +/** + * Add a new licence document header + * + * If no `data` is provided, default values will be used. These are + * + * - `regimeEntityId` - [random UUID] + * - `naldId` - [randomly generated - 105175] + * - `licenceRef` - [randomly generated - 01/123] + * - `metadata` - [object intended to be persisted] as JSON] + * + * @param {Object} [data] Any data you want to use instead of the defaults used here or in the database + * + * @returns {module:LicenceDocumentHeaderModel} The instance of the newly created record + */ +async function add (data = {}) { + const insertData = defaults(data) + + return LicenceDocumentHeaderModel.query() + .insert({ ...insertData }) + .returning('*') +} + +/** + * Returns the defaults used + * + * 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 = { + regimeEntityId: generateUUID(), + naldId: randomInteger(1000, 199999), + licenceRef: generateLicenceRef(), + metadata: _metadata() + } + + return { + ...defaults, + ...data + } +} + +function _metadata () { + return { + Town: 'BRISTOL', + County: 'AVON', + Name: 'GUPTA', + Country: '', + Expires: null, + Forename: 'AMARA', + Initials: 'A', + Modified: '20080327', + Postcode: 'BS1 5AH', + contacts: [ + { + name: 'GUPTA', + role: 'Licence holder', + town: 'BRISTOL', + type: 'Person', + county: 'AVON', + country: null, + forename: 'AMARA', + initials: 'A', + postcode: 'BS1 5AH', + salutation: null, + addressLine1: 'ENVIRONMENT AGENCY', + addressLine2: 'HORIZON HOUSE', + addressLine3: 'DEANERY ROAD', + addressLine4: null + } + ], + IsCurrent: true, + Salutation: '', + AddressLine1: 'ENVIRONMENT AGENCY', + AddressLine2: 'HORIZON HOUSE', + AddressLine3: 'DEANERY ROAD', + AddressLine4: '' + } +} + +module.exports = { + add, + defaults +} From ebe2e5dfd009488790ba97bdae4712528e40e2e9 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 00:31:57 +0000 Subject: [PATCH 7/9] Create LicenceDocumentHeader unit test --- .../licence-document-header.model.test.js | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 test/models/licence-document-header.model.test.js diff --git a/test/models/licence-document-header.model.test.js b/test/models/licence-document-header.model.test.js new file mode 100644 index 0000000000..6b237ee81f --- /dev/null +++ b/test/models/licence-document-header.model.test.js @@ -0,0 +1,36 @@ +'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 + +// Test helpers +const DatabaseHelper = require('../support/helpers/database.helper.js') +const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document-header.helper.js') + +// Thing under test +const LicenceDocumentHeaderModel = require('../../app/models/licence-document-header.model.js') + +describe('Licence Document Header model', () => { + let testRecord + + beforeEach(async () => { + await DatabaseHelper.clean() + }) + + describe('Basic query', () => { + beforeEach(async () => { + testRecord = await LicenceDocumentHeaderHelper.add() + }) + + it('can successfully run a basic query', async () => { + const result = await LicenceDocumentHeaderModel.query().findById(testRecord.id) + + expect(result).to.be.an.instanceOf(LicenceDocumentHeaderModel) + expect(result.id).to.equal(testRecord.id) + }) + }) +}) From 243b1e9eaa876f39a93bb2d7cba36f90ce09308f Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 00:37:06 +0000 Subject: [PATCH 8/9] Link LicenceDocumentHeader to Licence --- app/models/licence-document-header.model.js | 15 ++++++++ .../licence-document-header.model.test.js | 34 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/app/models/licence-document-header.model.js b/app/models/licence-document-header.model.js index 0c65c889b3..7d664035c2 100644 --- a/app/models/licence-document-header.model.js +++ b/app/models/licence-document-header.model.js @@ -5,6 +5,8 @@ * @module LicenceDocumentHeaderModel */ +const { Model } = require('objection') + const BaseModel = require('./base.model.js') /** @@ -40,6 +42,19 @@ class LicenceDocumentHeaderModel extends BaseModel { 'metadata' ] } + + static get relationMappings () { + return { + licence: { + relation: Model.BelongsToOneRelation, + modelClass: 'licence.model', + join: { + from: 'licenceDocumentHeaders.licenceRef', + to: 'licences.licenceRef' + } + } + } + } } module.exports = LicenceDocumentHeaderModel diff --git a/test/models/licence-document-header.model.test.js b/test/models/licence-document-header.model.test.js index 6b237ee81f..14c59aeaf4 100644 --- a/test/models/licence-document-header.model.test.js +++ b/test/models/licence-document-header.model.test.js @@ -9,6 +9,8 @@ const { expect } = Code // Test helpers const DatabaseHelper = require('../support/helpers/database.helper.js') +const LicenceHelper = require('../support/helpers/licence.helper.js') +const LicenceModel = require('../../app/models/licence.model.js') const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document-header.helper.js') // Thing under test @@ -33,4 +35,36 @@ describe('Licence Document Header model', () => { expect(result.id).to.equal(testRecord.id) }) }) + + describe('Relationships', () => { + describe('when linking to licence', () => { + let testLicence + + beforeEach(async () => { + testLicence = await LicenceHelper.add() + + const { licenceRef } = testLicence + testRecord = await LicenceDocumentHeaderHelper.add({ licenceRef }) + }) + + it('can successfully run a related query', async () => { + const query = await LicenceDocumentHeaderModel.query() + .innerJoinRelated('licence') + + expect(query).to.exist() + }) + + it('can eager load the licence', async () => { + const result = await LicenceDocumentHeaderModel.query() + .findById(testRecord.id) + .withGraphFetched('licence') + + expect(result).to.be.instanceOf(LicenceDocumentHeaderModel) + expect(result.id).to.equal(testRecord.id) + + expect(result.licence).to.be.an.instanceOf(LicenceModel) + expect(result.licence).to.equal(testLicence) + }) + }) + }) }) From 256580b8e62d537716c4a69a57b843537feb6fb6 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 00:40:11 +0000 Subject: [PATCH 9/9] Link Licence to LicenceDocumentHeader --- app/models/licence.model.js | 8 ++++++++ test/models/licence.model.test.js | 32 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/app/models/licence.model.js b/app/models/licence.model.js index 76a989caab..4be2dc0f6e 100644 --- a/app/models/licence.model.js +++ b/app/models/licence.model.js @@ -40,6 +40,14 @@ class LicenceModel extends BaseModel { to: 'licenceDocuments.licenceRef' } }, + licenceDocumentHeader: { + relation: Model.BelongsToOneRelation, + modelClass: 'licence-document-header.model', + join: { + from: 'licences.licenceRef', + to: 'licenceDocumentHeaders.licenceRef' + } + }, licenceVersions: { relation: Model.HasManyRelation, modelClass: 'licence-version.model', diff --git a/test/models/licence.model.test.js b/test/models/licence.model.test.js index 713e426b8a..b0b0e095cb 100644 --- a/test/models/licence.model.test.js +++ b/test/models/licence.model.test.js @@ -16,6 +16,8 @@ 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 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') @@ -145,6 +147,36 @@ describe('Licence model', () => { }) }) + describe('when linking to licence document header', () => { + let testLicenceDocumentHeader + + beforeEach(async () => { + testLicenceDocumentHeader = await LicenceDocumentHeaderHelper.add() + + const { licenceRef } = testLicenceDocumentHeader + testRecord = await LicenceHelper.add({ licenceRef }) + }) + + it('can successfully run a related query', async () => { + const query = await LicenceModel.query() + .innerJoinRelated('licenceDocumentHeader') + + expect(query).to.exist() + }) + + it('can eager load the licence document header', async () => { + const result = await LicenceModel.query() + .findById(testRecord.id) + .withGraphFetched('licenceDocumentHeader') + + expect(result).to.be.instanceOf(LicenceModel) + expect(result.id).to.equal(testRecord.id) + + expect(result.licenceDocumentHeader).to.be.an.instanceOf(LicenceDocumentHeaderModel) + expect(result.licenceDocumentHeader).to.equal(testLicenceDocumentHeader) + }) + }) + describe('when linking to licence versions', () => { let testLicenceVersions