From 8cee19f73c800e91cf278bdf404444f39eea3278 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 11:22:12 +0100 Subject: [PATCH 01/24] Fix registered to link in view licence https://eaflood.atlassian.net/browse/WATER-4650 The new view licence page went live in the [release today](https://github.com/DEFRA/water-abstraction-system/releases/tag/v0.20.0). But users have found if a licence has a registered user, and you click the link to see their status, it results in a 'page not found'. Having looked at the issue, it appears we have lost adding the user ID to the link. This change fixes the issue. From 604715720581c7122dfd055911f562b362118eef Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 11:31:49 +0100 Subject: [PATCH 02/24] Housekeeping - fix JSDoc missing param --- app/presenters/licences/view-licence.presenter.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/presenters/licences/view-licence.presenter.js b/app/presenters/licences/view-licence.presenter.js index 2216fb8b5f..22d0c6d32a 100644 --- a/app/presenters/licences/view-licence.presenter.js +++ b/app/presenters/licences/view-licence.presenter.js @@ -1,7 +1,7 @@ 'use strict' /** - * Formats data for common licence data `/licences/{id}` page's + * Formats data for common licence data `/licences/{id}` pages * @module ViewLicencePresenter */ @@ -11,6 +11,7 @@ const { formatLongDate } = require('../base.presenter.js') * Formats data for common licence data `/licences/{id}` page's * * @param {module:LicenceModel} licence - The licence where the data will be extracted for from + * @param {object} auth - The auth object taken from `request.auth` containing user details * * @returns {object} The data formatted for the view template */ From 6acf9f9d7cfa16d64b477fee601f5e5fcbbad137 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 12:07:14 +0100 Subject: [PATCH 03/24] Housekeeping - remove redundant withGraphFetched() The instance and data this is pulling is covered by the modifier `registeredToAndLicenceName()` so the withGraphFetched is just duplicating the work. --- app/services/licences/fetch-licence.service.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/services/licences/fetch-licence.service.js b/app/services/licences/fetch-licence.service.js index ea3c752a5d..349a43bfb7 100644 --- a/app/services/licences/fetch-licence.service.js +++ b/app/services/licences/fetch-licence.service.js @@ -47,12 +47,6 @@ async function _fetchLicence (id) { 'revokedDate', 'lapsedDate' ]) - .withGraphFetched('licenceDocumentHeader') - .modifyGraph('licenceDocumentHeader', (builder) => { - builder.select([ - 'licenceDocumentHeaders.id' - ]) - }) .modify('registeredToAndLicenceName') .withGraphFetched('workflows') .modifyGraph('workflows', (builder) => { From f8e287b1d5c2648098e4a9e943c868ee00f36922 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 12:11:57 +0100 Subject: [PATCH 04/24] Hmm ... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having dug into the `registeredToAndLicenceName()` modifier I can see it is pulling out the correct name to display. But the view is expecting something to set `primaryUser.userId`. But double checking the existing code and the original PR there is no other reference made to this. In fact, to pull out the user ID for the registered to user, you need to be able to link `public.licence_entities` to `public.users`. However, this is impossible because we are not including `idm.users.external_id` in our `public.users` view! So, this isn't a bug that's crept in, this has been here since day 1! Doh! 😱🤦 From d0d4839dbbceb26cbf227d336ab14354c760ae93 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 13:29:44 +0100 Subject: [PATCH 05/24] Alter user view to include licence_entity_id In the table it is `external_id`` but having checked it is only ever used to link `crm.entity` records to the user record. --- .../public/20240828111744_alter-users-view.js | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 db/migrations/public/20240828111744_alter-users-view.js diff --git a/db/migrations/public/20240828111744_alter-users-view.js b/db/migrations/public/20240828111744_alter-users-view.js new file mode 100644 index 0000000000..ce12bf33c3 --- /dev/null +++ b/db/migrations/public/20240828111744_alter-users-view.js @@ -0,0 +1,55 @@ +'use strict' + +const viewName = 'users' + +exports.up = function (knex) { + return knex + .schema + .dropViewIfExists(viewName) + .createView(viewName, (view) => { + // NOTE: We have commented out unused columns from the source table + view.as(knex('users').withSchema('idm').select([ + 'users.user_id AS id', + 'users.user_name AS username', + 'users.password', + // 'users.user_data', // inconsistently set and in most cases is {} + 'users.reset_guid', + 'users.reset_required', + 'users.last_login', + 'users.bad_logins', + 'users.application', + // 'users.role', // was made redundant when roles were moved to be stored separately in tables + 'users.external_id AS licence_entity_id', + 'users.reset_guid_date_created AS reset_guid_created_at', + 'users.enabled', + 'users.date_created AS created_at', + 'users.date_updated AS updated_at' + ])) + }) +} + +exports.down = function (knex) { + return knex + .schema + .dropViewIfExists(viewName) + .createView(viewName, (view) => { + // NOTE: We have commented out unused columns from the source table + view.as(knex('users').withSchema('idm').select([ + 'users.user_id AS id', + 'users.user_name AS username', + 'users.password', + // 'users.user_data', // inconsistently set and in most cases is {} + 'users.reset_guid', + 'users.reset_required', + 'users.last_login', + 'users.bad_logins', + 'users.application', + // 'users.role', // was made redundant when roles were moved to be stored separately in tables + // 'users.external_id', // was set during a migration of users from the crm schema but is never used + 'users.reset_guid_date_created AS reset_guid_created_at', + 'users.enabled', + 'users.date_created AS created_at', + 'users.date_updated AS updated_at' + ])) + }) +} From d1eab7c4d7df92c102d51481f2a38349e03fa13c Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 13:31:43 +0100 Subject: [PATCH 06/24] Add the relationship to the LicenceEntityModel --- app/models/licence-entity.model.js | 8 ++++++ test/models/licence-entity.model.test.js | 31 ++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/app/models/licence-entity.model.js b/app/models/licence-entity.model.js index 015b8a3778..13bccc30d5 100644 --- a/app/models/licence-entity.model.js +++ b/app/models/licence-entity.model.js @@ -43,6 +43,14 @@ class LicenceEntityModel extends BaseModel { from: 'licenceEntities.id', to: 'licenceEntityRoles.licenceEntityId' } + }, + user: { + relation: Model.BelongsToOneRelation, + modelClass: 'user.model', + join: { + from: 'licenceEntities.id', + to: 'users.licenceEntityId' + } } } } diff --git a/test/models/licence-entity.model.test.js b/test/models/licence-entity.model.test.js index ccf23fc08b..f34c6bdc6e 100644 --- a/test/models/licence-entity.model.test.js +++ b/test/models/licence-entity.model.test.js @@ -11,6 +11,8 @@ const { expect } = Code const LicenceEntityHelper = require('../support/helpers/licence-entity.helper.js') const LicenceEntityRoleHelper = require('../support/helpers/licence-entity-role.helper.js') const LicenceEntityRoleModel = require('../../app/models/licence-entity-role.model.js') +const UserHelper = require('../support/helpers/user.helper.js') +const UserModel = require('../../app/models/user.model.js') // Thing under test const LicenceEntityModel = require('../../app/models/licence-entity.model.js') @@ -69,5 +71,34 @@ describe('Licence Entity model', () => { expect(result.licenceEntityRoles).to.include(testLicenceEntityRoles[1]) }) }) + + describe('when linking to user', () => { + let testUser + + beforeEach(async () => { + testRecord = await LicenceEntityHelper.add() + + testUser = await UserHelper.add({ licenceEntityId: testRecord.id }) + }) + + it('can successfully run a related query', async () => { + const query = await LicenceEntityModel.query() + .innerJoinRelated('user') + + expect(query).to.exist() + }) + + it('can eager load the user', async () => { + const result = await LicenceEntityModel.query() + .findById(testRecord.id) + .withGraphFetched('user') + + expect(result).to.be.instanceOf(LicenceEntityModel) + expect(result.id).to.equal(testRecord.id) + + expect(result.user).to.be.an.instanceOf(UserModel) + expect(result.user).to.equal(testUser) + }) + }) }) }) From c99f5ff73f6e45bc0d73a80fbf1f49d4e7c103bc Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 13:32:13 +0100 Subject: [PATCH 07/24] Add the relationship to the UserModel --- app/models/user.model.js | 8 +++++++ test/models/user.model.test.js | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/app/models/user.model.js b/app/models/user.model.js index c96fa173fd..1369048b65 100644 --- a/app/models/user.model.js +++ b/app/models/user.model.js @@ -37,6 +37,14 @@ class UserModel extends BaseModel { to: 'groups.id' } }, + licenceEntity: { + relation: Model.HasOneRelation, + modelClass: 'licence-entity.model', + join: { + from: 'users.licenceEntityId', + to: 'licenceEntities.id' + } + }, returnVersions: { relation: Model.HasManyRelation, modelClass: 'return-version.model', diff --git a/test/models/user.model.test.js b/test/models/user.model.test.js index 50ebdf5764..705634ba0a 100644 --- a/test/models/user.model.test.js +++ b/test/models/user.model.test.js @@ -12,6 +12,8 @@ const ChargeVersionNoteHelper = require('../support/helpers/charge-version-note. const ChargeVersionNoteModel = require('../../app/models/charge-version-note.model.js') const GroupHelper = require('../support/helpers/group.helper.js') const GroupModel = require('../../app/models/group.model.js') +const LicenceEntityHelper = require('../support/helpers/licence-entity.helper.js') +const LicenceEntityModel = require('../../app/models/licence-entity.model.js') const ReturnVersionHelper = require('../support/helpers/return-version.helper.js') const ReturnVersionModel = require('../../app/models/return-version.model.js') const RoleHelper = require('../support/helpers/role.helper.js') @@ -108,6 +110,43 @@ describe('User model', () => { }) }) + describe('when linking to licence entity', () => { + let testAddedRecord + let testLicenceEntity + + before(async () => { + testLicenceEntity = await LicenceEntityHelper.add() + + const { id: licenceEntityId } = testLicenceEntity + + // NOTE: The entity ID is held against the user, not the other way round!! Because of this we can't seed a user + // with `licence_entity_id` set because the licence entity record is only created when an external user is + // linked to a licence using the external UI. + // + // So, for this test we have to fall back to generating a user against which we assign the licence entity ID. + testAddedRecord = await UserHelper.add({ licenceEntityId }) + }) + + it('can successfully run a related query', async () => { + const query = await UserModel.query() + .innerJoinRelated('licenceEntity') + + expect(query).to.exist() + }) + + it('can eager load the licence entity', async () => { + const result = await UserModel.query() + .findById(testAddedRecord.id) + .withGraphFetched('licenceEntity') + + expect(result).to.be.instanceOf(UserModel) + expect(result.id).to.equal(testAddedRecord.id) + + expect(result.licenceEntity).to.be.an.instanceOf(LicenceEntityModel) + expect(result.licenceEntity).to.equal(testLicenceEntity) + }) + }) + describe('when linking to return versions', () => { let testReturnVersions From 8c04c71b268fced55588b5558cc624c378679853 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 14:18:52 +0100 Subject: [PATCH 08/24] Housekeeping - reduce DB interactions in test We need to add the relationship between licence document header and licence entity role. But ahead of that we spotted some improvements we could do to the test to reduce the number of database interactions happening. --- .../licence-document-header.model.test.js | 23 ++++----- test/models/licence-entity-role.model.test.js | 49 ++++++------------- test/models/licence-entity.model.test.js | 46 +++++++---------- 3 files changed, 42 insertions(+), 76 deletions(-) diff --git a/test/models/licence-document-header.model.test.js b/test/models/licence-document-header.model.test.js index 1b972574ff..f82c90ea6d 100644 --- a/test/models/licence-document-header.model.test.js +++ b/test/models/licence-document-header.model.test.js @@ -4,7 +4,7 @@ const Lab = require('@hapi/lab') const Code = require('@hapi/code') -const { describe, it, beforeEach } = exports.lab = Lab.script() +const { describe, it, before } = exports.lab = Lab.script() const { expect } = Code // Test helpers @@ -16,13 +16,18 @@ const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document const LicenceDocumentHeaderModel = require('../../app/models/licence-document-header.model.js') describe('Licence Document Header model', () => { + let testLicence let testRecord - describe('Basic query', () => { - beforeEach(async () => { - testRecord = await LicenceDocumentHeaderHelper.add() + before(async () => { + testLicence = await LicenceHelper.add() + + testRecord = await LicenceDocumentHeaderHelper.add({ + licenceRef: testLicence.licenceRef }) + }) + describe('Basic query', () => { it('can successfully run a basic query', async () => { const result = await LicenceDocumentHeaderModel.query().findById(testRecord.id) @@ -33,16 +38,6 @@ describe('Licence Document Header model', () => { 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') diff --git a/test/models/licence-entity-role.model.test.js b/test/models/licence-entity-role.model.test.js index 175f0b1396..f50c9ff9f8 100644 --- a/test/models/licence-entity-role.model.test.js +++ b/test/models/licence-entity-role.model.test.js @@ -4,7 +4,7 @@ const Lab = require('@hapi/lab') const Code = require('@hapi/code') -const { describe, it, beforeEach } = exports.lab = Lab.script() +const { describe, it, before } = exports.lab = Lab.script() const { expect } = Code // Test helpers @@ -16,13 +16,24 @@ const LicenceEntityRoleHelper = require('../support/helpers/licence-entity-role. const LicenceEntityRoleModel = require('../../app/models/licence-entity-role.model.js') describe('Licence Entity Role model', () => { + let testCompanyEntity + let testLicenceEntity let testRecord + let testRegimeEntity - describe('Basic query', () => { - beforeEach(async () => { - testRecord = await LicenceEntityRoleHelper.add() + before(async () => { + testCompanyEntity = await LicenceEntityHelper.add({ type: 'company' }) + testLicenceEntity = await LicenceEntityHelper.add() + testRegimeEntity = await LicenceEntityHelper.add({ type: 'regime' }) + + testRecord = await LicenceEntityRoleHelper.add({ + companyEntityId: testCompanyEntity.id, + licenceEntityId: testLicenceEntity.id, + regimeEntityId: testRegimeEntity.id }) + }) + describe('Basic query', () => { it('can successfully run a basic query', async () => { const result = await LicenceEntityRoleModel.query().findById(testRecord.id) @@ -33,16 +44,6 @@ describe('Licence Entity Role model', () => { describe('Relationships', () => { describe('when linking to company entity', () => { - let testCompanyEntity - - beforeEach(async () => { - testCompanyEntity = await LicenceEntityHelper.add({ type: 'company' }) - - const { id: companyEntityId } = testCompanyEntity - - testRecord = await LicenceEntityRoleHelper.add({ companyEntityId }) - }) - it('can successfully run a related query', async () => { const query = await LicenceEntityRoleModel.query() .innerJoinRelated('companyEntity') @@ -64,16 +65,6 @@ describe('Licence Entity Role model', () => { }) describe('when linking to licence entity', () => { - let testLicenceEntity - - beforeEach(async () => { - testLicenceEntity = await LicenceEntityHelper.add() - - const { id: licenceEntityId } = testLicenceEntity - - testRecord = await LicenceEntityRoleHelper.add({ licenceEntityId }) - }) - it('can successfully run a related query', async () => { const query = await LicenceEntityRoleModel.query() .innerJoinRelated('licenceEntity') @@ -95,16 +86,6 @@ describe('Licence Entity Role model', () => { }) describe('when linking to regime entity', () => { - let testRegimeEntity - - beforeEach(async () => { - testRegimeEntity = await LicenceEntityHelper.add({ type: 'regime' }) - - const { id: regimeEntityId } = testRegimeEntity - - testRecord = await LicenceEntityRoleHelper.add({ regimeEntityId }) - }) - it('can successfully run a related query', async () => { const query = await LicenceEntityRoleModel.query() .innerJoinRelated('regimeEntity') diff --git a/test/models/licence-entity.model.test.js b/test/models/licence-entity.model.test.js index f34c6bdc6e..b1fb27ef7b 100644 --- a/test/models/licence-entity.model.test.js +++ b/test/models/licence-entity.model.test.js @@ -4,7 +4,7 @@ const Lab = require('@hapi/lab') const Code = require('@hapi/code') -const { describe, it, beforeEach } = exports.lab = Lab.script() +const { describe, it, before } = exports.lab = Lab.script() const { expect } = Code // Test helpers @@ -18,13 +18,26 @@ const UserModel = require('../../app/models/user.model.js') const LicenceEntityModel = require('../../app/models/licence-entity.model.js') describe('Licence Entity model', () => { + let testLicenceEntityRoles let testRecord + let testUser - describe('Basic query', () => { - beforeEach(async () => { - testRecord = await LicenceEntityHelper.add() - }) + before(async () => { + testRecord = await LicenceEntityHelper.add() + + const { id: licenceEntityId } = testRecord + + testLicenceEntityRoles = [] + for (let i = 0; i < 2; i++) { + const licenceEntityRole = await LicenceEntityRoleHelper.add({ licenceEntityId }) + + testLicenceEntityRoles.push(licenceEntityRole) + } + testUser = await UserHelper.add({ licenceEntityId }) + }) + + describe('Basic query', () => { it('can successfully run a basic query', async () => { const result = await LicenceEntityModel.query().findById(testRecord.id) @@ -35,21 +48,6 @@ describe('Licence Entity model', () => { describe('Relationships', () => { describe('when linking to licence entity roles', () => { - let testLicenceEntityRoles - - beforeEach(async () => { - testRecord = await LicenceEntityHelper.add() - - const { id: licenceEntityId } = testRecord - - testLicenceEntityRoles = [] - for (let i = 0; i < 2; i++) { - const licenceEntityRole = await LicenceEntityRoleHelper.add({ licenceEntityId }) - - testLicenceEntityRoles.push(licenceEntityRole) - } - }) - it('can successfully run a related query', async () => { const query = await LicenceEntityModel.query() .innerJoinRelated('licenceEntityRoles') @@ -73,14 +71,6 @@ describe('Licence Entity model', () => { }) describe('when linking to user', () => { - let testUser - - beforeEach(async () => { - testRecord = await LicenceEntityHelper.add() - - testUser = await UserHelper.add({ licenceEntityId: testRecord.id }) - }) - it('can successfully run a related query', async () => { const query = await LicenceEntityModel.query() .innerJoinRelated('user') From 2950af5ae78c67da20f16849b38754fcd862f265 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 14:39:36 +0100 Subject: [PATCH 09/24] Link LicenceDocumentHeader to LicenceEntityRole --- app/models/licence-document-header.model.js | 8 ++++++ .../licence-document-header.model.test.js | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/app/models/licence-document-header.model.js b/app/models/licence-document-header.model.js index 7d664035c2..67f9b3d49b 100644 --- a/app/models/licence-document-header.model.js +++ b/app/models/licence-document-header.model.js @@ -52,6 +52,14 @@ class LicenceDocumentHeaderModel extends BaseModel { from: 'licenceDocumentHeaders.licenceRef', to: 'licences.licenceRef' } + }, + licenceEntityRole: { + relation: Model.HasOneRelation, + modelClass: 'licence-entity-role.model', + join: { + from: 'licenceDocumentHeaders.companyEntityId', + to: 'licenceEntityRoles.companyEntityId' + } } } } diff --git a/test/models/licence-document-header.model.test.js b/test/models/licence-document-header.model.test.js index f82c90ea6d..c51dd1b8dd 100644 --- a/test/models/licence-document-header.model.test.js +++ b/test/models/licence-document-header.model.test.js @@ -8,6 +8,8 @@ const { describe, it, before } = exports.lab = Lab.script() const { expect } = Code // Test helpers +const LicenceEntityRoleHelper = require('../support/helpers/licence-entity-role.helper.js') +const LicenceEntityRoleModel = require('../../app/models/licence-entity-role.model.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') @@ -16,13 +18,16 @@ const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document const LicenceDocumentHeaderModel = require('../../app/models/licence-document-header.model.js') describe('Licence Document Header model', () => { + let testLicenceEntityRole let testLicence let testRecord before(async () => { + testLicenceEntityRole = await LicenceEntityRoleHelper.add() testLicence = await LicenceHelper.add() testRecord = await LicenceDocumentHeaderHelper.add({ + companyEntityId: testLicenceEntityRole.companyEntityId, licenceRef: testLicence.licenceRef }) }) @@ -57,5 +62,26 @@ describe('Licence Document Header model', () => { expect(result.licence).to.equal(testLicence) }) }) + + describe('when linking to licence entity role', () => { + it('can successfully run a related query', async () => { + const query = await LicenceDocumentHeaderModel.query() + .innerJoinRelated('licenceEntityRole') + + expect(query).to.exist() + }) + + it('can eager load the licence entity role', async () => { + const result = await LicenceDocumentHeaderModel.query() + .findById(testRecord.id) + .withGraphFetched('licenceEntityRole') + + expect(result).to.be.instanceOf(LicenceDocumentHeaderModel) + expect(result.id).to.equal(testRecord.id) + + expect(result.licenceEntityRole).to.be.an.instanceOf(LicenceEntityRoleModel) + expect(result.licenceEntityRole).to.equal(testLicenceEntityRole) + }) + }) }) }) From cdbe8c945fb853e18b39c73b6d1e5f682bfe852b Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 14:40:02 +0100 Subject: [PATCH 10/24] Link LicenceEntityRole to LicenceDocumentHeader --- app/models/licence-entity-role.model.js | 8 ++++++ test/models/licence-entity-role.model.test.js | 25 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/app/models/licence-entity-role.model.js b/app/models/licence-entity-role.model.js index 188872afed..3f25a44bac 100644 --- a/app/models/licence-entity-role.model.js +++ b/app/models/licence-entity-role.model.js @@ -36,6 +36,14 @@ class LicenceEntityRoleModel extends BaseModel { to: 'licenceEntities.id' } }, + licenceDocumentHeader: { + relation: Model.BelongsToOneRelation, + modelClass: 'licence-document-header.model', + join: { + from: 'licenceEntityRoles.companyEntityId', + to: 'licenceDocumentHeaders.companyEntityId' + } + }, licenceEntity: { relation: Model.BelongsToOneRelation, modelClass: 'licence-entity.model', diff --git a/test/models/licence-entity-role.model.test.js b/test/models/licence-entity-role.model.test.js index f50c9ff9f8..1ce0915a15 100644 --- a/test/models/licence-entity-role.model.test.js +++ b/test/models/licence-entity-role.model.test.js @@ -8,6 +8,8 @@ const { describe, it, before } = exports.lab = Lab.script() const { expect } = Code // Test helpers +const LicenceDocumentHeaderHelper = require('../support/helpers/licence-document-header.helper.js') +const LicenceDocumentHeaderModel = require('../../app/models/licence-document-header.model.js') const LicenceEntityHelper = require('../support/helpers/licence-entity.helper.js') const LicenceEntityModel = require('../../app/models/licence-entity.model.js') const LicenceEntityRoleHelper = require('../support/helpers/licence-entity-role.helper.js') @@ -17,12 +19,14 @@ const LicenceEntityRoleModel = require('../../app/models/licence-entity-role.mod describe('Licence Entity Role model', () => { let testCompanyEntity + let testLicenceDocumentHeader let testLicenceEntity let testRecord let testRegimeEntity before(async () => { testCompanyEntity = await LicenceEntityHelper.add({ type: 'company' }) + testLicenceDocumentHeader = await LicenceDocumentHeaderHelper.add({ companyEntityId: testCompanyEntity.id }) testLicenceEntity = await LicenceEntityHelper.add() testRegimeEntity = await LicenceEntityHelper.add({ type: 'regime' }) @@ -64,6 +68,27 @@ describe('Licence Entity Role model', () => { }) }) + describe('when linking to licence document header', () => { + it('can successfully run a related query', async () => { + const query = await LicenceEntityRoleModel.query() + .innerJoinRelated('licenceDocumentHeader') + + expect(query).to.exist() + }) + + it('can eager load the licence document header', async () => { + const result = await LicenceEntityRoleModel.query() + .findById(testRecord.id) + .withGraphFetched('licenceDocumentHeader') + + expect(result).to.be.instanceOf(LicenceEntityRoleModel) + 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 entity', () => { it('can successfully run a related query', async () => { const query = await LicenceEntityRoleModel.query() From e33a29203b902e33b868b21fe27db344d1c62ae2 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 16:10:40 +0100 Subject: [PATCH 11/24] Replace registeredToAndLicenceName With us having to go even deeper to retrieve the primary user ID in order to make the broken link work we've decided to split up the existing modifier/helper. Having them separate means it is easier to see what is needed to determine each property. Having done so it becomes clear licence name is a doddle but primary user is a hell scape! --- app/models/licence.model.js | 112 ++++++++++++++++++++---------- test/models/licence.model.test.js | 46 ++++++++---- 2 files changed, 109 insertions(+), 49 deletions(-) diff --git a/app/models/licence.model.js b/app/models/licence.model.js index 4cb3f0a4f3..e6ee525589 100644 --- a/app/models/licence.model.js +++ b/app/models/licence.model.js @@ -205,26 +205,67 @@ class LicenceModel extends BaseModel { ]) }) }, - /** - * registeredToAndLicenceName modifier fetches the linked `licenceDocumentHeader` which holds the licence name and - * adds to it the registered user's email address if one is set. - */ - registeredToAndLicenceName (query) { + // When an external user registers themselves as the 'primary user' for a licence (see $primaryUser()) they + // can choose to set a custom name for the licence. If set this is visible on the view licence page above the + // licence reference. + // + // The value itself is stored in `crm.document_header` not `water.licences` which is why we have to pull the + // related record. + licenceName (query) { + query + .withGraphFetched('licenceDocumentHeader') + .modifyGraph('licenceDocumentHeader', (builder) => { + builder.select([ + 'id', + 'licenceName' + ]) + }) + }, + // An external user with an account can register a licence via the external UI. The legacy service will generate a + // letter with a code which gets sent to the licence's address. Once they receive it they can enter the code in + // the UI and they then become the 'primary user' for it. + // + // When they do this the legacy service creates + // + // - creates a `crm.entity` record with the user's email + // - updates the user's `idm.users` record with the entity ID (see `external_id) + // - creates a `crm.entity_role` record with a role of 'primary_user' linked to both the entity, and the company + // entity linked to the licence + // - the `crm.entity_role` is linked to the `crm.document_header` for the licence + // + // We know, this is mad! It explains why even the previous team were trying to move away from this and had created + // `crm_v2`. Unfortunately, this never got sorted so it remains the only means to get from a licence to the user + // record which holds the ID of the primary user. + primaryUser (query) { query .withGraphFetched('licenceDocumentHeader') .modifyGraph('licenceDocumentHeader', (builder) => { builder.select([ - 'licenceDocumentHeaders.id', - 'licenceDocumentHeaders.licenceName', - 'licenceEntityRoles.role', - 'licenceEntities.name AS registeredTo' + 'id' ]) - .leftJoin('licenceEntityRoles', function () { - this - .on('licenceEntityRoles.companyEntityId', '=', 'licenceDocumentHeaders.companyEntityId') - .andOn('licenceEntityRoles.role', '=', Model.raw('?', ['primary_user'])) - }) - .leftJoin('licenceEntities', 'licenceEntities.id', 'licenceEntityRoles.licenceEntityId') + }) + .withGraphFetched('licenceDocumentHeader.licenceEntityRole') + .modifyGraph('licenceDocumentHeader.licenceEntityRole', (builder) => { + builder + .select([ + 'id' + ]) + .where('role', 'primary_user') + }) + .withGraphFetched('licenceDocumentHeader.licenceEntityRole.licenceEntity') + .modifyGraph('licenceDocumentHeader.licenceEntityRole.licenceEntity', (builder) => { + builder + .select([ + 'id' + ]) + }) + .withGraphFetched('licenceDocumentHeader.licenceEntityRole.licenceEntity.user') + .modifyGraph('licenceDocumentHeader.licenceEntityRole.licenceEntity.user', (builder) => { + builder + .select([ + 'id', + 'username' + ]) }) } } @@ -359,17 +400,18 @@ class LicenceModel extends BaseModel { /** * Determine the licence name for the licence * - * > We recommend adding the `registeredToAndLicenceName` modifier to your query to ensure the joined records are - * > available to determine this + * > We recommend adding the `licenceName` modifier to your query to ensure the joined records are available to + * > determine this * - * If set this is visible on the view licence page above the licence reference and on the legacy external view as a - * field in the summary tab. + * When an external user registers themselves as the 'primary user' for a licence (see {@link $primaryUser}) they can + * choose to set a custom name for the licence. * - * The licence name is a custom name the registered user of the licence can set. So, you will only see a licence name - * if the licence is registered to a user and they have chosen to set a name for it via the external UI. + * If set this is visible on the view licence page above the licence reference. * - * @returns {(string|null)} `null` if this instance does not have the additional properties needed to determine the - * licence name else the licence's custom name + * So, you will only see a licence name if the licence is registered to a user and they have chosen to set a name. + * + * @returns {(string|null)} the licence name set by the primary user for the licence if the licence has one and the + * additional properties needed to to determine it have been set, else `null` */ $licenceName () { const licenceName = this?.licenceDocumentHeader?.licenceName @@ -378,24 +420,24 @@ class LicenceModel extends BaseModel { } /** - * Determine who the licence is registered to + * Determine who the primary user for the licence is * - * > We recommend adding the `registeredToAndLicenceName` modifier to your query to ensure the joined records are - * > available to determine this + * > We recommend adding the `primaryUser` modifier to your query to ensure the joined records are available to + * > determine this * - * If set this is visible on the view licence page below the licence reference. + * An external user with an account can register a licence via the external UI. The legacy service will generate a + * letter with a code which gets sent to the licence's address. Once they receive it they can enter the code in the UI + * and they then become the 'primary user' for it. * - * When an external user has an account they can add a licence via the external UI. We'll generate a letter with a - * code which gets sent to the licence's address. Once they receive it they can enter the code in the UI and they - * then become the registered user for it. + * Within the UI this what determines whether you see the "Registered to" link in the view licence page's top section. * - * @returns {(string|null)} `null` if this instance does not have the additional properties needed to determine who - * the licence is registered to else the email of the user + * @returns {(module:UserModel|null)} the primary user if the licence has one and the additional properties needed to + * to determine it have been set, else `null` */ - $registeredTo () { - const registeredUserName = this?.licenceDocumentHeader?.registeredTo + $primaryUser () { + const primaryUser = this?.licenceDocumentHeader?.licenceEntityRole?.licenceEntity?.user - return registeredUserName || null + return primaryUser || null } } diff --git a/test/models/licence.model.test.js b/test/models/licence.model.test.js index 3adec32a1a..e18dcce475 100644 --- a/test/models/licence.model.test.js +++ b/test/models/licence.model.test.js @@ -14,6 +14,7 @@ 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 { generateUUID } = require('../../app/lib/general.lib.js') const LicenceAgreementHelper = require('../support/helpers/licence-agreement.helper.js') const LicenceAgreementModel = require('../../app/models/licence-agreement.model.js') const LicenceHelper = require('../support/helpers/licence.helper.js') @@ -22,6 +23,8 @@ 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 LicenceEntityHelper = require('../support/helpers/licence-entity.helper.js') +const LicenceEntityRoleHelper = require('../support/helpers/licence-entity-role.helper.js') const LicenceGaugingStationHelper = require('../support/helpers/licence-gauging-station.helper.js') const LicenceGaugingStationModel = require('../../app/models/licence-gauging-station.model.js') const LicenceRoleHelper = require('../support/helpers/licence-role.helper.js') @@ -35,9 +38,10 @@ const ReturnLogHelper = require('../support/helpers/return-log.helper.js') const ReturnLogModel = require('../../app/models/return-log.model.js') const ReturnVersionHelper = require('../support/helpers/return-version.helper.js') const ReturnVersionModel = require('../../app/models/return-version.model.js') -const RegisteredToAndLicenceNameSeeder = require('../support/seeders/registered-to-and-licence-name.seeder.js') const ReviewLicenceHelper = require('../support/helpers/review-licence.helper.js') const ReviewLicenceModel = require('../../app/models/review-licence.model.js') +const UserHelper = require('../support/helpers/user.helper.js') +const UserModel = require('../../app/models/user.model.js') const WorkflowHelper = require('../support/helpers/workflow.helper.js') const WorkflowModel = require('../../app/models/workflow.model.js') @@ -859,9 +863,11 @@ describe('Licence model', () => { beforeEach(async () => { const licence = await LicenceHelper.add() - await RegisteredToAndLicenceNameSeeder.seed(licence) + await LicenceDocumentHeaderHelper.add({ + licenceRef: licence.licenceRef, licenceName: 'My custom licence name' + }) - testRecord = await LicenceModel.query().findById(licence.id).modify('registeredToAndLicenceName') + testRecord = await LicenceModel.query().findById(licence.id).modify('licenceName') }) it('returns the licence name', async () => { @@ -872,32 +878,44 @@ describe('Licence model', () => { }) }) - describe('$registeredTo', () => { - beforeEach(async () => { - testRecord = await LicenceHelper.add() - }) - + describe('$primaryUser', () => { describe('when instance has not been set with the additional properties needed', () => { it('returns null', () => { - const result = testRecord.$registeredTo() + const result = testRecord.$primaryUser() expect(result).to.be.null() }) }) describe('when the instance has been set with the additional properties needed', () => { + let primaryUser + beforeEach(async () => { const licence = await LicenceHelper.add() - await RegisteredToAndLicenceNameSeeder.seed(licence) + const companyEntityId = generateUUID() + const username = `${generateUUID()}@wrls.gov.uk` + + const licenceEntity = await LicenceEntityHelper.add({ name: username }) + + primaryUser = await UserHelper.add({ application: 'water_vml', licenceEntityId: licenceEntity.id, username }) + + await LicenceEntityRoleHelper.add({ + companyEntityId, licenceEntityId: licenceEntity.id, role: 'primary_user' + }) + + await LicenceDocumentHeaderHelper.add({ + companyEntityId, licenceRef: licence.licenceRef, licenceName: 'My custom licence name' + }) - testRecord = await LicenceModel.query().findById(licence.id).modify('registeredToAndLicenceName') + testRecord = await LicenceModel.query().findById(licence.id).modify('primaryUser') }) - it('returns who the licence is registered to', async () => { - const result = testRecord.$registeredTo() + it('returns the primary user', async () => { + const result = testRecord.$primaryUser() - expect(result).to.equal('grace.hopper@example.com') + expect(result).to.be.an.instanceOf(UserModel) + expect(result).to.equal({ id: primaryUser.id, username: primaryUser.username }) }) }) }) From 3bb7f792b754f04e39468a2ba1bfb7fb3c207030 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 16:15:09 +0100 Subject: [PATCH 12/24] Housekeeping - fix JSDoc comments --- app/models/licence.model.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/licence.model.js b/app/models/licence.model.js index e6ee525589..42209bc0c8 100644 --- a/app/models/licence.model.js +++ b/app/models/licence.model.js @@ -134,17 +134,19 @@ class LicenceModel extends BaseModel { /** * Modifiers allow us to reuse logic in queries, eg. select the licence and everything to get the licence holder: * + * ```javascript * return LicenceModel.query() * .findById(licenceId) * .modify('licenceHolder') + * ``` * * See {@link https://vincit.github.io/objection.js/recipes/modifiers.html | Modifiers} for more details + * + * @returns {object} */ static get modifiers () { return { - /** - * currentVersion modifier fetches only the current licence version record for this licence - */ + // currentVersion modifier fetches only the current licence version record for this licence currentVersion (query) { query .withGraphFetched('licenceVersions') @@ -160,9 +162,7 @@ class LicenceModel extends BaseModel { .limit(1) }) }, - /** - * licenceHolder modifier fetches all the joined records needed to identify the licence holder - */ + // licenceHolder modifier fetches all the joined records needed to identify the licence holder licenceHolder (query) { query .withGraphFetched('licenceDocument') From 88f438262fa739d8ba4a054707a10c58f99bb37e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 16:39:19 +0100 Subject: [PATCH 13/24] Remove seeder We're not using it any more so we can remove the defunct seeder. --- .../registered-to-and-licence-name.seeder.js | 47 ------------------- 1 file changed, 47 deletions(-) delete mode 100644 test/support/seeders/registered-to-and-licence-name.seeder.js diff --git a/test/support/seeders/registered-to-and-licence-name.seeder.js b/test/support/seeders/registered-to-and-licence-name.seeder.js deleted file mode 100644 index b903c38b77..0000000000 --- a/test/support/seeders/registered-to-and-licence-name.seeder.js +++ /dev/null @@ -1,47 +0,0 @@ -'use strict' - -/** - * Seeder to setup registered to and licence name details for a LicenceModel - * @module RegisteredToAndLicenceNameSeeder - */ - -const LicenceDocumentHeaderHelper = require('../helpers/licence-document-header.helper.js') -const LicenceEntityRoleHelper = require('../helpers/licence-entity-role.helper.js') -const LicenceEntityHelper = require('../helpers/licence-entity.helper.js') - -/** - * Sets up the additional records needed for `$licenceName()` and `%registeredTo()` on the licence to return values - * - * The registered to and licence name details are seen on the view licence page (if a licence has them). Unfortunately, - * we have to link through a number of the legacy tables to extract the data. - * - * This seeder ensures the records needed are created and specifically support the `registeredToAndLicenceName()` - * modifier on the `LicenceModel`. - * - * @param {module:LicenceModel} licence - The licence instance we are setting up the records for - * @param {string} [licenceName] - The custom licence name to use - */ -async function seed (licence, licenceName = 'My custom licence name') { - const { licenceRef } = licence - - // We get from the licence to the registered user via LicenceDocumentHeader and LicenceEntityRole which are linked - // by the same companyEntityId - const companyEntityId = 'c960a4a1-94f9-4c05-9db1-a70ce5d08738' - - // Create a licence document header record - await LicenceDocumentHeaderHelper.add({ - companyEntityId, - licenceName, - licenceRef - }) - - // Create the licence entity record. It's `name` field holds the user email that the licence is registered to - const { id: licenceEntityId } = await LicenceEntityHelper.add() - - // Create the licence entity role record that is the part of the link between the licence and the user email - await LicenceEntityRoleHelper.add({ companyEntityId, licenceEntityId }) -} - -module.exports = { - seed -} From f2c8763961cd83042dc1e8b83e151615ed8bb13a Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 16:43:15 +0100 Subject: [PATCH 14/24] Fix FetchLicenceSummary It was using the `registeredToAndLicenceName()` modifier just to get the `LicenceDocumentHeader` ID which is overkill. --- app/services/licences/fetch-licence-summary.service.js | 9 +++++++-- .../licences/fetch-licence-summary.service.test.js | 5 +---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/services/licences/fetch-licence-summary.service.js b/app/services/licences/fetch-licence-summary.service.js index cba79c9781..3fa32625e5 100644 --- a/app/services/licences/fetch-licence-summary.service.js +++ b/app/services/licences/fetch-licence-summary.service.js @@ -31,6 +31,7 @@ async function _fetch (licenceId) { 'startDate' ]) .modify('currentVersion') + .modify('licenceHolder') .withGraphFetched('region') .modifyGraph('region', (builder) => { builder.select([ @@ -38,6 +39,12 @@ async function _fetch (licenceId) { 'displayName' ]) }) + .withGraphFetched('licenceDocumentHeader') + .modifyGraph('licenceDocumentHeader', (builder) => { + builder.select([ + 'id' + ]) + }) .withGraphFetched('permitLicence') .modifyGraph('permitLicence', (builder) => { builder.select([ @@ -93,8 +100,6 @@ async function _fetch (licenceId) { 'label' ]) }) - .modify('licenceHolder') - .modify('registeredToAndLicenceName') } module.exports = { diff --git a/test/services/licences/fetch-licence-summary.service.test.js b/test/services/licences/fetch-licence-summary.service.test.js index 8e77e677a2..f52de16a54 100644 --- a/test/services/licences/fetch-licence-summary.service.test.js +++ b/test/services/licences/fetch-licence-summary.service.test.js @@ -176,10 +176,7 @@ describe('Fetch Licence Summary service', () => { }] }, licenceDocumentHeader: { - id: licenceDocumentHeader.id, - licenceName: 'Licence Holder Ltd', - registeredTo: 'grace.hopper@example.com', - role: 'primary_user' + id: licenceDocumentHeader.id } }) }) From 0d610c3210828c1760825f58294d29d22d3675bc Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 17:12:57 +0100 Subject: [PATCH 15/24] Fix up FetchLicenceService We remove the data transformation from the service so we can leave that to the presenter and simplify this fetch service. In the tests, the modifiers are already covered by tests in the `LicenceModel` itself so we don't need to re-test them here. We just add enough, i.e. a workflow record, to cover those query elements we're doing in the fetch service itself. We also tidy up field names, JSDoc comments and any sorting. --- .../licences/fetch-licence.service.js | 52 +++++------- .../licences/fetch-licence.service.test.js | 79 ++++++------------- 2 files changed, 45 insertions(+), 86 deletions(-) diff --git a/app/services/licences/fetch-licence.service.js b/app/services/licences/fetch-licence.service.js index 349a43bfb7..b826102524 100644 --- a/app/services/licences/fetch-licence.service.js +++ b/app/services/licences/fetch-licence.service.js @@ -1,61 +1,47 @@ 'use strict' /** - * Fetches data needed for the view '/licences/{id}` page + * Fetches the matching licence and its data needed for the view '/licences/{id}/*` pages * @module FetchLicenceService */ const LicenceModel = require('../../models/licence.model.js') /** - * Fetch the matching licence and return data needed for the view licence page + * Fetches the matching licence and its data needed for the view '/licences/{id}/*` pages * * Was built to provide the data needed for the '/licences/{id}' page * - * @param {string} id - The UUID for the licence to fetch + * @param {string} licenceId - The UUID for the licence to fetch * - * @returns {Promise} the data needed to populate the view licence page and some elements of the summary tab + * @returns {Promise} the matching `LicenceModel` populated with the data needed for the view + * licence page top section (shared by all tab pages) and some elements needed for the summary tab */ -async function go (id) { - const licence = await _fetchLicence(id) - const data = await _data(licence) - - return data -} - -async function _data (licence) { - const registeredTo = licence.$registeredTo() ?? null - const licenceName = registeredTo ? licence.$licenceName() : 'Unregistered licence' - - return { - ...licence, - ends: licence.$ends(), - licenceName, - registeredTo - } +async function go (licenceId) { + return _fetch(licenceId) } -async function _fetchLicence (id) { - const result = await LicenceModel.query() - .findById(id) +async function _fetch (licenceId) { + return LicenceModel.query() + .findById(licenceId) .select([ 'id', - 'include_in_presroc_billing', - 'include_in_sroc_billing', - 'licenceRef', 'expiredDate', - 'revokedDate', - 'lapsedDate' + 'lapsedDate', + 'includeInPresrocBilling', + 'includeInSrocBilling', + 'licenceRef', + 'revokedDate' ]) - .modify('registeredToAndLicenceName') + .modify('licenceName') + .modify('primaryUser') .withGraphFetched('workflows') .modifyGraph('workflows', (builder) => { builder.select([ - 'workflows.status' + 'id', + 'status' ]) }) - - return result } module.exports = { diff --git a/test/services/licences/fetch-licence.service.test.js b/test/services/licences/fetch-licence.service.test.js index ef7f4b1e7f..4ed66d9b16 100644 --- a/test/services/licences/fetch-licence.service.test.js +++ b/test/services/licences/fetch-licence.service.test.js @@ -8,76 +8,49 @@ const { describe, it, beforeEach } = exports.lab = Lab.script() const { expect } = Code // Test helpers -const LicenceEntityHelper = require('../../support/helpers/licence-entity.helper.js') -const LicenceEntityRoleHelper = require('../../support/helpers/licence-entity-role.helper.js') const LicenceHelper = require('../../support/helpers/licence.helper.js') -const LicenceDocumentHeaderHelper = require('../../support/helpers/licence-document-header.helper.js') -const LicenceHolderSeeder = require('../../support/seeders/licence-holder.seeder.js') +const LicenceModel = require('../../../app/models/licence.model.js') +const WorkflowHelper = require('../../support/helpers/workflow.helper.js') // Thing under test const FetchLicenceService = require('../../../app/services/licences/fetch-licence.service.js') -describe('Fetch licence service', () => { +describe('Fetch Licence service', () => { let licence + let workflow - describe('when there is no optional data in the model', () => { + describe('when there is a matching licence', () => { beforeEach(async () => { - licence = await LicenceHelper.add({ - expiredDate: null, - include_in_presroc_billing: 'yes', - include_in_sroc_billing: true, - lapsedDate: null, - revokedDate: null - }) - - // Create a licence holder for the licence with the default name 'Licence Holder Ltd' - await LicenceHolderSeeder.seed(licence.licenceRef) + licence = await LicenceHelper.add() + workflow = await WorkflowHelper.add({ licenceId: licence.id }) }) - it('returns results', async () => { + it('returns the matching licence', async () => { const result = await FetchLicenceService.go(licence.id) - expect(result.id).to.equal(licence.id) - expect(result.ends).to.equal(null) - expect(result.expiredDate).to.equal(null) - expect(result.lapsedDate).to.equal(null) - expect(result.licenceName).to.equal('Unregistered licence') - expect(result.licenceRef).to.equal(licence.licenceRef) - expect(result.revokedDate).to.equal(null) - expect(result.includeInPresrocBilling).to.equal('yes') - expect(result.includeInSrocBilling).to.be.true() - }) - }) - - describe('when there is optional data in the model', () => { - beforeEach(async () => { - licence = await LicenceHelper.add({ - expiredDate: '2020-01-01', + expect(result).to.be.an.instanceOf(LicenceModel) + expect(result).to.equal({ + id: licence.id, + includeInPresrocBilling: 'no', + includeInSrocBilling: false, + licenceRef: licence.licenceRef, + expiredDate: null, + revokedDate: null, lapsedDate: null, - revokedDate: null + licenceDocumentHeader: null, + workflows: [{ + id: workflow.id, + status: workflow.status + }] }) - - const licenceName = 'Test Company Ltd' - const companyEntityId = 'c960a4a1-94f9-4c05-9db1-a70ce5d08738' - const licenceRef = licence.licenceRef - - await LicenceDocumentHeaderHelper.add({ companyEntityId, licenceName, licenceRef }) - const { id: licenceEntityId } = await LicenceEntityHelper.add() - - await LicenceEntityRoleHelper.add({ companyEntityId, licenceEntityId }) }) + }) - it('returns results', async () => { - const result = await FetchLicenceService.go(licence.id) + describe('when there is not a matching licence', () => { + it('returns undefined', async () => { + const result = await FetchLicenceService.go('4ba2066b-4623-4102-801a-c771e39af2f3') - expect(result.registeredTo).to.equal('grace.hopper@example.com') - expect(result.licenceName).to.equal('Test Company Ltd') - expect(result.ends).to.equal({ - date: new Date('2020-01-01'), - priority: 3, - reason: 'expired' - } - ) + expect(result).to.be.undefined() }) }) }) From 0e3361ca2626dc8c6c6714a66fa5f7e74a3e01ec Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 22:25:50 +0100 Subject: [PATCH 16/24] Housekeeping - add basic access user to seed Needed to confirm that the basic access internal user has no roles assigned to it. But to do so we needed to create a new user account because we are not currently seeding one. So, updated the seed whilst about it to ensure we have the ability to access a user with this permission level both in tests and our non-prod environments. --- db/seeds/data/users.js | 12 ++++++++++++ test/support/helpers/user.helper.js | 9 +++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/db/seeds/data/users.js b/db/seeds/data/users.js index a0cadcbc9b..d65d221924 100644 --- a/db/seeds/data/users.js +++ b/db/seeds/data/users.js @@ -120,6 +120,18 @@ const data = [ application: 'water_vml', resetGuidCreatedAt: null, enabled: true + }, + { + id: 100010, + username: 'basic.access@wrls.gov.uk', + password: 'P@55word', + resetGuid: null, + resetRequired: 0, + lastLogin: null, + badLogins: 0, + application: 'water_admin', + resetGuidCreatedAt: null, + enabled: true } ] diff --git a/test/support/helpers/user.helper.js b/test/support/helpers/user.helper.js index f93f49f00d..4505d570c7 100644 --- a/test/support/helpers/user.helper.js +++ b/test/support/helpers/user.helper.js @@ -62,9 +62,14 @@ function defaults (data = {}) { } } +/** + * Generates a random user ID + * + * @returns {number} a random integer between 100011 and 199999 + */ function generateUserId () { - // The last ID in the pre-seeded users is 100009 - return randomInteger(100010, 199999) + // The last ID in the pre-seeded users is 100010 + return randomInteger(100011, 199999) } /** From 99bf7612bcc32d08253f5d15ec5533ac346eab6e Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 22:40:07 +0100 Subject: [PATCH 17/24] Fix up view licence presenter Found we were returning values that either were not used by the view, or that could be simplified. In the tests, we were generating data that the presenter ignored or had tests that covered functionality already covered by other tests. So, in all we were able to simplify both the presenter and the tests. On top of that we brought the naming of the functions et al in line with our conventions plus some other housekeeping. --- .../licences/view-licence.presenter.js | 67 ++-- .../licences/view-licence.presenter.test.js | 333 +++++++++--------- 2 files changed, 193 insertions(+), 207 deletions(-) diff --git a/app/presenters/licences/view-licence.presenter.js b/app/presenters/licences/view-licence.presenter.js index 22d0c6d32a..be13e04fd6 100644 --- a/app/presenters/licences/view-licence.presenter.js +++ b/app/presenters/licences/view-licence.presenter.js @@ -17,35 +17,40 @@ const { formatLongDate } = require('../base.presenter.js') */ function go (licence, auth) { const { - ends, id, - includeInPresrocBilling, - includeInSrocBilling, licenceDocumentHeader, - licenceName, licenceRef, - registeredTo, workflows } = licence + const primaryUser = licence.$primaryUser() + return { - activeNavBar: 'search', documentId: licenceDocumentHeader.id, - ends, - includeInPresrocBilling, licenceId: id, - licenceName, + licenceName: _licenceName(licence), licenceRef, - notification: _determineNotificationBanner(includeInPresrocBilling, includeInSrocBilling), + notification: _notification(licence), pageTitle: `Licence ${licenceRef}`, - registeredTo, - roles: _authRoles(auth), - warning: _generateWarningMessage(ends), - workflowWarning: _generateWorkflowWarningMessage(workflows) + primaryUser, + roles: _roles(auth), + warning: _warning(licence), + workflowWarning: _workflowWarning(workflows) + } +} + +function _licenceName (licence) { + const licenceName = licence.$licenceName() + + if (licenceName) { + return licenceName } + + return 'Unregistered licence' } -function _determineNotificationBanner (includeInPresrocBilling, includeInSrocBilling) { +function _notification (licence) { + const { includeInPresrocBilling, includeInSrocBilling } = licence const baseMessage = 'This licence has been marked for the next supplementary bill run' if (includeInPresrocBilling === 'yes' && includeInSrocBilling === true) { @@ -62,38 +67,34 @@ function _determineNotificationBanner (includeInPresrocBilling, includeInSrocBil return null } -function _authRoles (auth) { - const roles = auth?.credentials?.roles?.map((role) => { - return role?.role +function _roles (auth) { + return auth.credentials.roles.map((role) => { + return role.role }) - - return roles || null } -function _generateWarningMessage (ends) { - if (!ends) { - return null - } - - const { date, reason } = ends +function _warning (licence) { const today = new Date() + const ends = licence.$ends() - if (date > today) { + if (!ends || ends.date > today) { return null } - if (reason === 'revoked') { - return `This licence was revoked on ${formatLongDate(date)}` + const formattedDate = formatLongDate(ends.date) + + if (ends.reason === 'revoked') { + return `This licence was revoked on ${formattedDate}` } - if (reason === 'lapsed') { - return `This licence lapsed on ${formatLongDate(date)}` + if (ends.reason === 'lapsed') { + return `This licence lapsed on ${formattedDate}` } - return `This licence expired on ${formatLongDate(date)}` + return `This licence expired on ${formattedDate}` } -function _generateWorkflowWarningMessage (workflows) { +function _workflowWarning (workflows) { return workflows.some((workflow) => { return workflow.status === 'to_setup' }) diff --git a/test/presenters/licences/view-licence.presenter.test.js b/test/presenters/licences/view-licence.presenter.test.js index ae0953c141..49947dabde 100644 --- a/test/presenters/licences/view-licence.presenter.test.js +++ b/test/presenters/licences/view-licence.presenter.test.js @@ -7,34 +7,37 @@ const Code = require('@hapi/code') const { describe, it, beforeEach } = exports.lab = Lab.script() const { expect } = Code +// Test helpers +const LicenceModel = require('../../../app/models/licence.model.js') + // Thing under test const ViewLicencePresenter = require('../../../app/presenters/licences/view-licence.presenter.js') -describe('View Licence presenter', () => { - let licence +describe.only('View Licence presenter', () => { let auth + let licence beforeEach(() => { + auth = _auth() licence = _licence() - auth = undefined }) describe('when provided with a populated licence', () => { it('correctly presents the data', () => { - const result = ViewLicencePresenter.go(licence) + const result = ViewLicencePresenter.go(licence, auth) expect(result).to.equal({ - activeNavBar: 'search', - documentId: '28665d16-eba3-4c9a-aa55-7ab671b0c4fb', - ends: null, - includeInPresrocBilling: 'no', + documentId: 'e8f491f0-0c60-4083-9d41-d2be69f17a1e', licenceId: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', - licenceName: 'Unregistered licence', + licenceName: 'Between two ferns', licenceRef: '01/123', notification: null, pageTitle: 'Licence 01/123', - registeredTo: null, - roles: null, + primaryUser: { + id: 10036, + username: 'grace.hopper@example.co.uk' + }, + roles: ['billing', 'view_charge_versions'], warning: null, workflowWarning: true }) @@ -42,259 +45,241 @@ describe('View Licence presenter', () => { }) describe('the "licenceName" property', () => { - describe('when there is no licenceName property', () => { - it('returns Unregistered licence', () => { - const result = ViewLicencePresenter.go(licence) + describe('when the primary user has added a custom name for the licence', () => { + it('returns the licence name', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.licenceName).to.equal('Unregistered licence') + expect(result.licenceName).to.equal('Between two ferns') }) }) - describe('when there is a licenceName property', () => { + describe('when the primary user has not added a custom name for the licence', () => { beforeEach(() => { - licence.licenceName = 'example@example.com' + licence.licenceDocumentHeader.licenceName = null }) - it('returns a string with the licence name values', () => { - const result = ViewLicencePresenter.go(licence) + it('returns "Unregistered licence"', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.licenceName).to.equal('example@example.com') + expect(result.licenceName).to.equal('Unregistered licence') }) }) }) - describe('the "registeredTo" property', () => { - describe('when there is no registeredTo property', () => { - it('returns null', () => { - const result = ViewLicencePresenter.go(licence) - expect(result.registeredTo).to.equal(null) + describe('the "notification" property', () => { + describe('when the licence has NOT been flagged for either supplementary bill run', () => { + it('returns "null"', () => { + const result = ViewLicencePresenter.go(licence, auth) + + expect(result.notification).to.be.null() }) }) - describe('when there is a registeredTo property', () => { + describe('when the licence has been flagged just for the next PRESROC supplementary bill run', () => { beforeEach(() => { - licence.registeredTo = 'Company' + licence.includeInPresrocBilling = 'yes' }) - it('returns a string with the registered to name', () => { - const result = ViewLicencePresenter.go(licence) - - expect(result.registeredTo).to.equal('Company') - }) - }) - }) - describe('the "warning" property', () => { - describe('when the licence does not have an end date (expired, lapsed or revoked)', () => { - it('returns NULL', () => { - const result = ViewLicencePresenter.go(licence) + it('returns a notification just for the "old charge scheme"', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.warning).to.be.null() + expect(result.notification).to.equal('This licence has been marked for the next supplementary bill run for the old charge scheme.') }) }) - describe('when the licence does have an end date but it is in the future (expired, lapsed or revoked)', () => { + describe('when the licence has been flagged just for the next SROC supplementary bill run', () => { beforeEach(() => { - licence.expiredDate = new Date('2099-04-01') + licence.includeInSrocBilling = true }) - it('returns NULL', () => { - const result = ViewLicencePresenter.go(licence) + it('returns a notification just for the current charge scheme', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.warning).to.be.null() + expect(result.notification).to.equal('This licence has been marked for the next supplementary bill run.') }) }) - describe('when the licence ends today or in the past (2019-04-01) because it is expired', () => { + describe('when the licence has been flagged for the next PRESROC & SROC supplementary bill runs', () => { beforeEach(() => { - licence.ends = { date: new Date('2019-04-01'), reason: 'expired' } + licence.includeInPresrocBilling = 'yes' + licence.includeInSrocBilling = true }) - it('returns "This licence expired on 1 April 2019"', () => { - const result = ViewLicencePresenter.go(licence) + it('returns a notification just for both charge schemes', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.warning).to.equal('This licence expired on 1 April 2019') + expect(result.notification).to.equal('This licence has been marked for the next supplementary bill runs for the current and old charge schemes.') }) }) + }) - describe('when the licence ends today or in the past (2019-04-01) because it is lapsed', () => { - beforeEach(() => { - licence.ends = { date: new Date('2019-04-01'), reason: 'lapsed' } - }) - - it('returns "This licence lapsed on 1 April 2019"', () => { - const result = ViewLicencePresenter.go(licence) + describe('the "roles" property', () => { + describe('when the authenticated user has roles', () => { + it('returns the roles names as an array', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.warning).to.equal('This licence lapsed on 1 April 2019') + expect(result.roles).to.equal(['billing', 'view_charge_versions']) }) }) - describe('when the licence was ends today or in the past (2019-04-01) because it is revoked', () => { + describe('when the authenticated user has no roles', () => { beforeEach(() => { - licence.ends = { date: new Date('2019-04-01'), reason: 'revoked' } + auth.credentials.roles = [] }) - it('returns "This licence was revoked on 1 April 2019"', () => { - const result = ViewLicencePresenter.go(licence) + it('returns an empty array', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.warning).to.equal('This licence was revoked on 1 April 2019') + expect(result.roles).to.be.empty() }) }) }) - describe('the workflowWarning property', () => { - describe('when the licence does not have a workflow', () => { - beforeEach(() => { - licence.workflows = [] - }) - - it('returns false', () => { - const result = ViewLicencePresenter.go(licence) + describe('the "warning" property', () => { + describe('when the licence does not have an "end" date', () => { + it('returns null', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.workflowWarning).to.equal(false) + expect(result.warning).to.be.null() }) }) - describe('when the licence has a workflow but the status is not `to_setup`', () => { - beforeEach(() => { - licence.workflows[0].status = 'changes_requested' - }) + describe('when the licence does have an "end" date', () => { + describe('but it is in the future', () => { + beforeEach(() => { + licence.expiredDate = new Date('2099-04-01') + }) - it('returns false', () => { - const result = ViewLicencePresenter.go(licence) + it('returns null', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.workflowWarning).to.equal(false) + expect(result.warning).to.be.null() + }) }) - }) - describe('when the licence has a workflow and the status is `to_setup`', () => { - beforeEach(() => { - licence.workflows[0].status = 'to_setup' - }) + describe('because it expired in the past', () => { + beforeEach(() => { + licence.expiredDate = new Date('2019-04-01') + }) - it('returns true', () => { - const result = ViewLicencePresenter.go(licence) + it('returns "This licence expired on 1 April 2019"', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.workflowWarning).to.equal(true) + expect(result.warning).to.equal('This licence expired on 1 April 2019') + }) }) - }) - }) - describe('the "notification" property', () => { - describe('when the licence will not be in the next supplementary bill run', () => { - it('returns NULL', () => { - const result = ViewLicencePresenter.go(licence) + describe('because it lapsed in the past', () => { + beforeEach(() => { + licence.lapsedDate = new Date('2019-04-01') + }) - expect(result.notification).to.be.null() - }) - }) + it('returns "This licence lapsed on 1 April 2019"', () => { + const result = ViewLicencePresenter.go(licence, auth) - describe('when the licence will be in the next supplementary bill run (PRESROC)', () => { - beforeEach(() => { - licence.includeInPresrocBilling = 'yes' + expect(result.warning).to.equal('This licence lapsed on 1 April 2019') + }) }) - it('returns the notification for PRESROC', () => { - const result = ViewLicencePresenter.go(licence) + describe('because it was revoked in the past', () => { + beforeEach(() => { + licence.revokedDate = new Date('2019-04-01') + }) - expect(result.notification).to.equal('This licence has been marked for the next supplementary bill run for the old charge scheme.') - }) - }) + it('returns "This licence was revoked on 1 April 2019"', () => { + const result = ViewLicencePresenter.go(licence, auth) - describe('when the licence will be in the next supplementary bill run (SROC)', () => { - beforeEach(() => { - licence.includeInSrocBilling = true - }) - - it('returns the notification for SROC', () => { - const result = ViewLicencePresenter.go(licence) - - expect(result.notification).to.equal('This licence has been marked for the next supplementary bill run.') + expect(result.warning).to.equal('This licence was revoked on 1 April 2019') + }) }) }) + }) - describe('when the licence will be in the next supplementary bill run (SROC & PRESROC)', () => { + describe('the "workflowWarning" property', () => { + describe('when the licence is not in workflow', () => { beforeEach(() => { - licence.includeInSrocBilling = true - licence.includeInPresrocBilling = 'yes' + licence.workflows = [] }) - it('returns the notification for SROC & PRESROC)', () => { - const result = ViewLicencePresenter.go(licence) + it('returns false', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.notification).to.equal('This licence has been marked for the next supplementary bill runs for the current and old charge schemes.') + expect(result.workflowWarning).to.be.false() }) }) - }) - describe('the "roles" property', () => { - describe('when the authenticated user has roles', () => { - beforeEach(() => { - auth = { - credentials: { - roles: [{ - role: 'role 1' - }, - { - role: 'role 2' - }] - } - } - }) - it('returns the roles in a flat array', () => { - const result = ViewLicencePresenter.go(licence, auth) + describe('when the licence is in workflow', () => { + describe('but the status is not "to_setup"', () => { + beforeEach(() => { + licence.workflows[0].status = 'changes_requested' + }) - expect(result.roles).to.equal(['role 1', 'role 2']) - }) - }) - describe('when the authenticated user has NO roles', () => { - beforeEach(() => { - auth = undefined + it('returns false', () => { + const result = ViewLicencePresenter.go(licence, auth) + + expect(result.workflowWarning).to.be.false() + }) }) - it('returns null for the roles', () => { - const result = ViewLicencePresenter.go(licence, auth) + describe('and the status is "to_setup"', () => { + it('returns true', () => { + const result = ViewLicencePresenter.go(licence, auth) - expect(result.roles).to.be.null() + expect(result.workflowWarning).to.be.true() + }) }) }) }) }) -function _licence () { +function _auth () { return { + credentials: { + roles: [ + { + id: 'b62afe79-d599-4101-b374-729011711462', + role: 'billing', + description: 'Administer billing', + createdAt: new Date('2023-12-14'), + updatedAt: new Date('2024-08-19') + }, + { + id: '02b09477-8c1e-4f9a-956c-ad18f9d4f222', + role: 'view_charge_versions', + description: 'View charge information', + createdAt: new Date('2023-12-14'), + updatedAt: new Date('2024-08-19') + } + ] + } + } +} + +function _licence () { + const licence = LicenceModel.fromJson({ id: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', - ends: null, expiredDate: null, + lapsedDate: null, includeInPresrocBilling: 'no', - licenceDocumentHeader: { id: '28665d16-eba3-4c9a-aa55-7ab671b0c4fb' }, - licenceGaugingStations: [{ - gaugingStationId: 'ac075651-4781-4e24-a684-b943b98607ca', - label: 'MEVAGISSEY FIRE STATION' - }], - licenceHolder: null, - licenceName: 'Unregistered licence', + includeInSrocBilling: false, licenceRef: '01/123', - permitLicence: { - purposes: [{ - ANNUAL_QTY: 'null', - DAILY_QTY: 'null', - HOURLY_QTY: 'null', - INST_QTY: 'null', - purposePoints: [{ - point_detail: { - NGR1_SHEET: 'TL', - NGR1_EAST: '23198', - NGR1_NORTH: '88603' - }, - point_source: { - NAME: 'SURFACE WATER SOURCE OF SUPPLY' + revokedDate: null, + licenceDocumentHeader: { + id: 'e8f491f0-0c60-4083-9d41-d2be69f17a1e', + licenceName: 'Between two ferns', + licenceEntityRole: { + id: 'd7eecfc1-7afa-49f7-8bef-5dc477696a2d', + licenceEntity: { + id: 'ba7702cf-cd87-4419-a04c-8cea4e0cfdc2', + user: { + id: 10036, + username: 'grace.hopper@example.co.uk' } - }] - }] + } + } }, - region: { displayName: 'Narnia' }, - registeredTo: null, - startDate: new Date('2019-04-01'), - workflows: [{ status: 'to_setup' }] - } + workflows: [{ id: 'b6f44c94-25e4-4ca8-a7db-364534157ba7', status: 'to_setup' }] + }) + + return licence } From fed114a24792c1731b052198b96184b79e0c73ac Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 22:42:27 +0100 Subject: [PATCH 18/24] Fix up view licence service A similar story to the presenter. We were able to tidy some things up in the service. The key thing was the tests were testing things already covered by the presenter, which in turn we now know was covering stuff already tested in the model. --- app/services/licences/view-licence.service.js | 6 +- .../licences/view-licence.service.test.js | 204 +++++++----------- 2 files changed, 84 insertions(+), 126 deletions(-) diff --git a/app/services/licences/view-licence.service.js b/app/services/licences/view-licence.service.js index bcf23e1b5b..22b29adcfc 100644 --- a/app/services/licences/view-licence.service.js +++ b/app/services/licences/view-licence.service.js @@ -13,14 +13,16 @@ const ViewLicencePresenter = require('../../presenters/licences/view-licence.pre * * @param {string} licenceId - The UUID of the licence * @param {object} auth - The auth object taken from `request.auth` containing user details + * * @returns {Promise} an object representing the `pageData` needed by the licence summary template. */ async function go (licenceId, auth) { - const licenceData = await FetchLicenceService.go(licenceId) + const licence = await FetchLicenceService.go(licenceId) - const pageData = ViewLicencePresenter.go(licenceData, auth) + const pageData = ViewLicencePresenter.go(licence, auth) return { + activeNavBar: 'search', ...pageData } } diff --git a/test/services/licences/view-licence.service.test.js b/test/services/licences/view-licence.service.test.js index 8bd3080952..947e177741 100644 --- a/test/services/licences/view-licence.service.test.js +++ b/test/services/licences/view-licence.service.test.js @@ -18,146 +18,102 @@ const FetchLicenceService = require('../../../app/services/licences/fetch-licenc const ViewLicenceService = require('../../../app/services/licences/view-licence.service.js') describe('View Licence service', () => { - const testId = '2c80bd22-a005-4cf4-a2a2-73812a9861de' + let auth + let licence - let fetchLicenceResult + beforeEach(() => { + auth = _auth() + licence = _licence() + }) afterEach(() => { Sinon.restore() }) describe('when a licence with a matching ID exists', () => { - describe('and it has no optional fields', () => { - beforeEach(() => { - fetchLicenceResult = _testLicence() - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will return all the mandatory data and default values for use in the licence summary page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result).to.equal({ - activeNavBar: 'search', - documentId: '28665d16-eba3-4c9a-aa55-7ab671b0c4fb', - ends: null, - includeInPresrocBilling: 'no', - licenceId: '2c80bd22-a005-4cf4-a2a2-73812a9861de', - licenceName: 'Unregistered licence', - licenceRef: '01/130/R01', - notification: null, - pageTitle: 'Licence 01/130/R01', - registeredTo: null, - roles: null, - warning: null, - workflowWarning: true - }) - }) + beforeEach(() => { + Sinon.stub(FetchLicenceService, 'go').resolves(licence) }) - describe('and it does not have an expired, lapsed, or revoke date', () => { - beforeEach(() => { - fetchLicenceResult = _testLicence() - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will return the data and format it for use in the licence summary page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result.warning).to.equal(null) + it('returns page data for the view', async () => { + const result = await ViewLicenceService.go(licence.id, auth) + + expect(result).to.equal({ + activeNavBar: 'search', + documentId: 'e8f491f0-0c60-4083-9d41-d2be69f17a1e', + licenceId: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', + licenceName: 'Between two ferns', + licenceRef: '01/123', + notification: null, + pageTitle: 'Licence 01/123', + primaryUser: { + id: 10036, + username: 'grace.hopper@example.co.uk' + }, + roles: ['billing', 'view_charge_versions'], + warning: null, + workflowWarning: true }) }) - describe('and it did "end" in the past', () => { - beforeEach(() => { - fetchLicenceResult = _testLicence() - }) - - describe('because it was revoked', () => { - beforeEach(() => { - fetchLicenceResult.ends = { date: new Date('2023-03-07'), priority: 1, reason: 'revoked' } - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will include the revoked warning message for use in the view licence page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result.warning).to.equal('This licence was revoked on 7 March 2023') - }) - }) - - describe('because it was lapsed', () => { - beforeEach(() => { - fetchLicenceResult.ends = { date: new Date('2023-03-07'), priority: 1, reason: 'lapsed' } - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will include the lapsed warning message for use in the view licence page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result.warning).to.equal('This licence lapsed on 7 March 2023') - }) - }) - - describe('because it was expired', () => { - beforeEach(() => { - fetchLicenceResult.ends = { date: new Date('2023-03-07'), priority: 1, reason: 'expired' } - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will include the expired warning message for use in the view licence page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result.warning).to.equal('This licence expired on 7 March 2023') - }) - }) - }) - - describe('and it did "ends" today', () => { - beforeEach(() => { - fetchLicenceResult = _testLicence() - fetchLicenceResult.ends = { date: new Date(), priority: 1, reason: 'revoked' } - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will include a warning message for use in the view licence page', async () => { - const result = await ViewLicenceService.go(testId) - - expect(result.warning).to.startWith('This licence was revoked on') - }) - }) - - describe('and it did "ends" in the future', () => { - beforeEach(() => { - fetchLicenceResult = _testLicence() - - // Set the 'end' date to tomorrow - const today = new Date() - // 86400000 is one day in milliseconds - const tomorrow = new Date(today.getTime() + 86400000) - - fetchLicenceResult.ends = { date: tomorrow, priority: 1, reason: 'revoked' } - - Sinon.stub(FetchLicenceService, 'go').resolves(fetchLicenceResult) - }) - - it('will include a warning message for use in the view licence page', async () => { - const result = await ViewLicenceService.go(testId) + }) - expect(result.warning).to.be.null() - }) + describe('when a licence with a matching ID does not exist', () => { + it('throws an exception', async () => { + await expect(ViewLicenceService.go('a45341d0-82c7-4a00-975c-e978a6f776eb', auth)) + .to + .reject() }) }) }) -function _testLicence () { - return LicenceModel.fromJson({ - id: '2c80bd22-a005-4cf4-a2a2-73812a9861de', +function _auth () { + return { + credentials: { + roles: [ + { + id: 'b62afe79-d599-4101-b374-729011711462', + role: 'billing', + description: 'Administer billing', + createdAt: new Date('2023-12-14'), + updatedAt: new Date('2024-08-19') + }, + { + id: '02b09477-8c1e-4f9a-956c-ad18f9d4f222', + role: 'view_charge_versions', + description: 'View charge information', + createdAt: new Date('2023-12-14'), + updatedAt: new Date('2024-08-19') + } + ] + } + } +} + +function _licence () { + const licence = LicenceModel.fromJson({ + id: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', + expiredDate: null, + lapsedDate: null, includeInPresrocBilling: 'no', - licenceRef: '01/130/R01', - ends: null, - licenceName: 'Unregistered licence', - registeredTo: null, - startDate: new Date('2013-03-07'), - licenceDocumentHeader: { id: '28665d16-eba3-4c9a-aa55-7ab671b0c4fb' }, - workflows: [{ status: 'to_setup' }] + includeInSrocBilling: false, + licenceRef: '01/123', + revokedDate: null, + licenceDocumentHeader: { + id: 'e8f491f0-0c60-4083-9d41-d2be69f17a1e', + licenceName: 'Between two ferns', + licenceEntityRole: { + id: 'd7eecfc1-7afa-49f7-8bef-5dc477696a2d', + licenceEntity: { + id: 'ba7702cf-cd87-4419-a04c-8cea4e0cfdc2', + user: { + id: 10036, + username: 'grace.hopper@example.co.uk' + } + } + } + }, + workflows: [{ id: 'b6f44c94-25e4-4ca8-a7db-364534157ba7', status: 'to_setup' }] }) + + return licence } From 5934afc43fe7f1ff2c45288e2c0edb1a336d637b Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 22:44:02 +0100 Subject: [PATCH 19/24] Update view licence based on changes --- app/views/licences/view.njk | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/licences/view.njk b/app/views/licences/view.njk index 562b2c351e..fed40068a2 100644 --- a/app/views/licences/view.njk +++ b/app/views/licences/view.njk @@ -19,7 +19,7 @@ }) }} {% endif %} - {% if (workflowWarning === true) and ((roles and 'billing' in roles) or (roles and 'view_charge_versions' in roles))%} + {% if (workflowWarning === true) and (('billing' in roles) or ('view_charge_versions' in roles))%} {{ govukWarningText({ text: "This licence has changed and needs the set up reviewed before it can be billed.", iconFallbackText: "Warning" @@ -37,8 +37,8 @@ {{ licenceName }}

Licence number {{ licenceRef }}

- {% if registeredTo %} -

Registered to {{ registeredTo }}

+ {% if primaryUser %} +

Registered to {{ primaryUser.username }}

{% endif %}
@@ -66,14 +66,14 @@ Communications - {% if roles and 'billing' in roles %} + {% if 'billing' in roles %}
  • Bills
  • {% endif %} - {% if roles and 'view_charge_versions' in roles %} + {% if 'view_charge_versions' in roles %}
  • Licence set up From 70007f117b8d76b75172dff0c9706836d0cb2514 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 22:52:30 +0100 Subject: [PATCH 20/24] Doh! --- test/presenters/licences/view-licence.presenter.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/presenters/licences/view-licence.presenter.test.js b/test/presenters/licences/view-licence.presenter.test.js index 49947dabde..6f430ee231 100644 --- a/test/presenters/licences/view-licence.presenter.test.js +++ b/test/presenters/licences/view-licence.presenter.test.js @@ -13,7 +13,7 @@ const LicenceModel = require('../../../app/models/licence.model.js') // Thing under test const ViewLicencePresenter = require('../../../app/presenters/licences/view-licence.presenter.js') -describe.only('View Licence presenter', () => { +describe('View Licence presenter', () => { let auth let licence From 3f9dc61d9150f5c0cf3bacf1db361fee8a07be00 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 23:04:38 +0100 Subject: [PATCH 21/24] Revert user seed change Its breaking a lot of tests. As it might mean they all need updating we'll do this in a separate PR. It's clearly beyind the scope of a minor tweak. --- db/seeds/data/users.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/db/seeds/data/users.js b/db/seeds/data/users.js index d65d221924..a0cadcbc9b 100644 --- a/db/seeds/data/users.js +++ b/db/seeds/data/users.js @@ -120,18 +120,6 @@ const data = [ application: 'water_vml', resetGuidCreatedAt: null, enabled: true - }, - { - id: 100010, - username: 'basic.access@wrls.gov.uk', - password: 'P@55word', - resetGuid: null, - resetRequired: 0, - lastLogin: null, - badLogins: 0, - application: 'water_admin', - resetGuidCreatedAt: null, - enabled: true } ] From ce2d9be2ab8d2f3f3b591816821783751565ba2f Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 23:33:37 +0100 Subject: [PATCH 22/24] Fix up licence controller tests --- test/controllers/licences.controller.test.js | 594 +++++++++---------- 1 file changed, 265 insertions(+), 329 deletions(-) diff --git a/test/controllers/licences.controller.test.js b/test/controllers/licences.controller.test.js index df02db4231..cc82817310 100644 --- a/test/controllers/licences.controller.test.js +++ b/test/controllers/licences.controller.test.js @@ -17,8 +17,8 @@ const LicenceSupplementaryProcessBillingFlagService = require('../../app/service const ViewLicenceBillsService = require('../../app/services/licences/view-licence-bills.service.js') const ViewLicenceCommunicationsService = require('../../app/services/licences/view-licence-communications.service.js') const ViewLicenceContactDetailsService = require('../../app/services/licences/view-licence-contact-details.service.js') -const ViewLicenceSetUpService = require('../../app/services/licences/view-licence-set-up.service.js') const ViewLicenceReturnsService = require('../../app/services/licences/view-licence-returns.service.js') +const ViewLicenceSetUpService = require('../../app/services/licences/view-licence-set-up.service.js') const ViewLicenceSummaryService = require('../../app/services/licences/view-licence-summary.service.js') // For running our service @@ -44,429 +44,360 @@ describe('Licences controller', () => { Sinon.restore() }) - describe('GET /licences/{id}/no-returns-required', () => { - const session = { id: '1c265420-6a5e-4a4c-94e4-196d7799ed01' } - - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/64924759-8142-4a08-9d1e-1e902cd9d316/no-returns-required', - auth: { - strategy: 'session', - credentials: { scope: ['billing'] } - } - } - }) - - describe('when a request is valid', () => { + describe('/licences/{id}/bills', () => { + describe('GET', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').resolves(session) + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/bills', + auth: { + strategy: 'session', + credentials: { scope: ['billing'] } + } + } }) - it('redirects to select return start date page', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(302) - expect(response.headers.location).to.equal(`/system/return-requirements/${session.id}/start-date`) - }) - }) - - describe('when a request is invalid', () => { - describe('because the licence ID is unrecognised', () => { + describe('when a request is valid', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').rejects(Boom.notFound()) + Sinon.stub(ViewLicenceBillsService, 'go').resolves(_viewLicenceBills()) }) - it('returns a 404 and page not found', async () => { + it('returns the page successfully', async () => { const response = await server.inject(options) - expect(response.statusCode).to.equal(404) - expect(response.payload).to.contain('Page not found') + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Bills') }) }) + }) + }) + + describe('/licences/{id}/communications', () => { + describe('GET', () => { + beforeEach(async () => { + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/communications', + auth: { + strategy: 'session', + credentials: { scope: [] } + } + } + }) - describe('because the initialise session service errors', () => { + describe('when a request is valid', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').rejects() + Sinon.stub(ViewLicenceCommunicationsService, 'go').resolves(_viewLicenceCommunications()) }) - it('returns a 200 and there is a problem with the service page', async () => { + it('returns the page successfully', async () => { const response = await server.inject(options) expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Sorry, there is a problem with the service') + expect(response.payload).to.contain('Communications') }) }) }) }) - describe('GET /licences/{id}/returns-required', () => { - const session = { id: '1c265420-6a5e-4a4c-94e4-196d7799ed01' } - - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/64924759-8142-4a08-9d1e-1e902cd9d316/returns-required', - auth: { - strategy: 'session', - credentials: { scope: ['billing'] } - } - } - }) - - describe('when a request is valid', () => { + describe('/licences/{id}/contact-details', () => { + describe('GET', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').resolves(session) - }) - - it('redirects to select return start date page', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(302) - expect(response.headers.location).to.equal(`/system/return-requirements/${session.id}/start-date`) + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/contact-details', + auth: { + strategy: 'session', + credentials: { scope: [] } + } + } }) - }) - describe('when a request is invalid', () => { - describe('because the licence ID is unrecognised', () => { + describe('when a request is valid and has contacts', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').rejects(Boom.notFound()) + Sinon.stub(ViewLicenceContactDetailsService, 'go').resolves(_viewLicenceContactDetails()) }) - it('returns a 404 and page not found', async () => { + it('returns the page successfully', async () => { const response = await server.inject(options) - expect(response.statusCode).to.equal(404) - expect(response.payload).to.contain('Page not found') + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Contact details') }) }) + }) + }) - describe('because the initialise session service errors', () => { + describe('/licences/{id}/no-returns-required', () => { + describe('GET', () => { + const session = { id: '1c265420-6a5e-4a4c-94e4-196d7799ed01' } + + beforeEach(async () => { + options = { + method: 'GET', + url: '/licences/64924759-8142-4a08-9d1e-1e902cd9d316/no-returns-required', + auth: { + strategy: 'session', + credentials: { scope: ['billing'] } + } + } + }) + + describe('when a request is valid', () => { beforeEach(async () => { - Sinon.stub(InitiateSessionService, 'go').rejects() + Sinon.stub(InitiateSessionService, 'go').resolves(session) }) - it('returns a 200 and there is a problem with the service page', async () => { + it('redirects to select return start date page', async () => { const response = await server.inject(options) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Sorry, there is a problem with the service') + expect(response.statusCode).to.equal(302) + expect(response.headers.location).to.equal(`/system/return-requirements/${session.id}/start-date`) }) }) - }) - }) - describe('GET /licences/{id}/bills', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/bills', - auth: { - strategy: 'session', - credentials: { scope: ['billing'] } - } - } - }) + describe('when a request is invalid', () => { + describe('because the licence ID is unrecognised', () => { + beforeEach(async () => { + Sinon.stub(InitiateSessionService, 'go').rejects(Boom.notFound()) + }) - describe('when a request is valid and has bills', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceBillsService, 'go').resolves(_viewLicenceBills()) - }) + it('returns a 404 and page not found', async () => { + const response = await server.inject(options) - it('returns the page successfully', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Bills') - // Check the table titles - expect(response.payload).to.contain('Bill number') - expect(response.payload).to.contain('Date created') - expect(response.payload).to.contain('Billing account') - expect(response.payload).to.contain('Bill run type') - expect(response.payload).to.contain('Bill total') - }) - }) - describe('when a request is valid and has no bills', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceBillsService, 'go').resolves({ activeTab: 'bills', bills: [] }) - }) + expect(response.statusCode).to.equal(404) + expect(response.payload).to.contain('Page not found') + }) + }) + + describe('because the initialise session service errors', () => { + beforeEach(async () => { + Sinon.stub(InitiateSessionService, 'go').rejects() + }) - it('returns the page successfully', async () => { - const response = await server.inject(options) + it('returns a 200 and there is a problem with the service page', async () => { + const response = await server.inject(options) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Bills') - // Check the table titles - expect(response.payload).to.contain('Bills') - expect(response.payload).to.contain('No bills sent for this licence.') + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Sorry, there is a problem with the service') + }) + }) }) }) }) - describe('GET /licences/{id}/communications', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/communications', - auth: { - strategy: 'session', - credentials: { scope: [] } - } - } - }) + describe('/licences/{id}/returns-required', () => { + describe('GET', () => { + const session = { id: '1c265420-6a5e-4a4c-94e4-196d7799ed01' } - describe('when a request is valid and has communications', () => { beforeEach(async () => { - Sinon.stub(ViewLicenceCommunicationsService, 'go').resolves(_viewLicenceCommunications()) + options = { + method: 'GET', + url: '/licences/64924759-8142-4a08-9d1e-1e902cd9d316/returns-required', + auth: { + strategy: 'session', + credentials: { scope: ['billing'] } + } + } }) - it('returns the page successfully', async () => { - const response = await server.inject(options) + describe('when a request is valid', () => { + beforeEach(async () => { + Sinon.stub(InitiateSessionService, 'go').resolves(session) + }) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Communications') - expect(response.payload).to.contain('Type') - expect(response.payload).to.contain('Sent') - expect(response.payload).to.contain('Sender') - expect(response.payload).to.contain('Method') - }) - }) + it('redirects to select return start date page', async () => { + const response = await server.inject(options) - describe('when a request is valid and does not have communications', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceCommunicationsService, 'go').resolves({ - activeTab: 'communications', - communications: [] + expect(response.statusCode).to.equal(302) + expect(response.headers.location).to.equal(`/system/return-requirements/${session.id}/start-date`) }) }) - it('returns the page successfully', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Communications') - expect(response.payload).to.contain('No communications for this licence.') - }) - }) - }) + describe('when a request is invalid', () => { + describe('because the licence ID is unrecognised', () => { + beforeEach(async () => { + Sinon.stub(InitiateSessionService, 'go').rejects(Boom.notFound()) + }) - describe('GET /licences/{id}/contact-details', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/contact-details', - auth: { - strategy: 'session', - credentials: { scope: [] } - } - } - }) + it('returns a 404 and page not found', async () => { + const response = await server.inject(options) - describe('when a request is valid and has contacts', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceContactDetailsService, 'go').resolves(_viewLicenceContactDetails()) - }) + expect(response.statusCode).to.equal(404) + expect(response.payload).to.contain('Page not found') + }) + }) - it('returns the page successfully', async () => { - const response = await server.inject(options) + describe('because the initialise session service errors', () => { + beforeEach(async () => { + Sinon.stub(InitiateSessionService, 'go').rejects() + }) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Contact details') - // Table row titles - expect(response.payload).to.contain('Name') - expect(response.payload).to.contain('Communication type') - expect(response.payload).to.contain('Send to') - }) - }) + it('returns a 200 and there is a problem with the service page', async () => { + const response = await server.inject(options) - describe('when a request is valid and has no contact details', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceContactDetailsService, 'go').resolves({ - activeTab: 'contact-details', - licenceContacts: [], - customerContacts: [] + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Sorry, there is a problem with the service') + }) }) }) - - it('returns the page successfully', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Contact details') - expect(response.payload).to.contain('No licence contacts found.') - expect(response.payload).to.contain('No customer contacts found.') - }) }) }) - describe('GET /licences/{id}/set-up', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/set-up', - auth: { - strategy: 'session', - credentials: { scope: ['view_charge_versions'] } - } - } - }) - - describe('when a request is valid and has charge versions and charge edit buttons', () => { + describe('/licences/{id}/returns', () => { + describe('GET', () => { beforeEach(async () => { - Sinon.stub(ViewLicenceSetUpService, 'go').resolves(_viewLicenceSetUp()) - }) - - it('returns the page successfully', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Licence set up') - // Returns for requirements - expect(response.payload).to.contain('Requirements for returns') - // Returns for requirements present - expect(response.payload).to.contain('Set up new requirements') - expect(response.payload).to.contain('Mark licence as') - expect(response.payload).to.contain('no returns needed') - // Charge information - expect(response.payload).to.contain('Charge information') - // Charge information table headers - expect(response.payload).to.contain('Start date') - expect(response.payload).to.contain('End date') - expect(response.payload).to.contain('Reason') - expect(response.payload).to.contain('Status') - expect(response.payload).to.contain('Action') - // Charge information buttons present - expect(response.payload).to.contain('Set up a new charge') - expect(response.payload).to.contain('Make licence non-chargeable') - - // Agreements - expect(response.payload).to.contain('Charging agreements') - // Agreements table headers - expect(response.payload).to.contain('Agreement') - expect(response.payload).to.contain('Date signed') - // Charge information buttons present - expect(response.payload).to.contain('Set up a new agreement') + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/returns', + auth: { + strategy: 'session', + credentials: { scope: ['billing'] } + } + } }) - }) - describe('when a request is valid and does not have any data', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceSetUpService, 'go').resolves({ - activeTab: 'set-up', - agreements: [], - chargeInformation: [], - returnVersions: [] + describe('when a request is valid', () => { + beforeEach(async () => { + Sinon.stub(ViewLicenceReturnsService, 'go').resolves(_viewLicenceReturns()) }) - }) - it('returns the page successfully', async () => { - const response = await server.inject(options) - - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Licence set up') - expect(response.payload).to.contain('Requirements for returns') - expect(response.payload).to.contain('No requirements for returns have been set up for this licence.') - expect(response.payload).to.contain('Charge information') - expect(response.payload).to.contain('No charge information for this licence.') - expect(response.payload).to.contain('Charging agreements') - expect(response.payload).to.contain('No agreements for this licence.') + it('returns the page successfully', async () => { + const response = await server.inject(options) + + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Returns') + }) }) }) }) - describe('GET /licences/{id}/summary', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/summary', - auth: { - strategy: 'session', - credentials: { scope: [] } - } - } - }) - - describe('when a request is valid', () => { + describe('/licences/{id}/set-up', () => { + describe('GET', () => { beforeEach(async () => { - Sinon.stub(ViewLicenceSummaryService, 'go').resolves(_viewLicenceSummary()) + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/set-up', + auth: { + strategy: 'session', + credentials: { scope: ['view_charge_versions'] } + } + } }) - it('returns the page successfully', async () => { - const response = await server.inject(options) + describe('when a request is valid', () => { + beforeEach(async () => { + Sinon.stub(ViewLicenceSetUpService, 'go').resolves(_viewLicenceSetUp()) + }) + + it('returns the page successfully', async () => { + const response = await server.inject(options) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Summary') - expect(response.payload).to.contain('Effective from') - expect(response.payload).to.contain('End date') + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Licence set up') + }) }) }) }) - describe('GET /licences/{id}/returns', () => { - beforeEach(async () => { - options = { - method: 'GET', - url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/returns', - auth: { - strategy: 'session', - credentials: { scope: ['billing'] } + describe('/licences/{id}/summary', () => { + describe('GET', () => { + beforeEach(async () => { + options = { + method: 'GET', + url: '/licences/7861814c-ca19-43f2-be11-3c612f0d744b/summary', + auth: { + strategy: 'session', + credentials: { scope: [] } + } } - } - }) + }) - describe('when a request is valid', () => { - beforeEach(async () => { - Sinon.stub(ViewLicenceReturnsService, 'go').resolves({ - activeTab: 'returns', - returns: [ - { id: 'returns-id' } - ], - noReturnsMessage: null + describe('when a request is valid', () => { + beforeEach(async () => { + Sinon.stub(ViewLicenceSummaryService, 'go').resolves(_viewLicenceSummary()) }) - }) - it('returns the page successfully', async () => { - const response = await server.inject(options) + it('returns the page successfully', async () => { + const response = await server.inject(options) - expect(response.statusCode).to.equal(200) - expect(response.payload).to.contain('Returns') - // Check the table titles - expect(response.payload).to.contain('Return reference and dates') - expect(response.payload).to.contain('Purpose and description') - expect(response.payload).to.contain('Due date') - expect(response.payload).to.contain('Status') + expect(response.statusCode).to.equal(200) + expect(response.payload).to.contain('Summary') + }) }) }) }) - describe('POST /licences/supplementary', () => { - beforeEach(() => { - options = { method: 'POST', url: '/licences/supplementary' } - }) - - describe('when the request succeeds', () => { - beforeEach(async () => { - Sinon.stub(LicenceSupplementaryProcessBillingFlagService, 'go').resolves() + describe('/licences/supplementary', () => { + describe('POST', () => { + beforeEach(() => { + options = { method: 'POST', url: '/licences/supplementary' } }) - it('returns a 204 response', async () => { - const response = await server.inject(options) + describe('when the request succeeds', () => { + beforeEach(async () => { + Sinon.stub(LicenceSupplementaryProcessBillingFlagService, 'go').resolves() + }) + + it('returns a 204 response', async () => { + const response = await server.inject(options) - expect(response.statusCode).to.equal(204) + expect(response.statusCode).to.equal(204) + }) }) }) }) }) function _viewLicenceBills () { + const commonLicenceData = _viewLicence() + return { + ...commonLicenceData, activeTab: 'bills', bills: [{ id: 'bills-id' }] } } +function _viewLicenceCommunications () { + const commonLicenceData = _viewLicence() + + return { + ...commonLicenceData, + activeTab: 'communications', + communications: [{ type: 'paper return', sent: 'date', sender: 'admin@email.com', method: 'letter' }] + } +} + +function _viewLicenceContactDetails () { + const commonLicenceData = _viewLicence() + + return { + ...commonLicenceData, + activeTab: 'contact-details', + licenceContacts: [{ name: 'jobo', communicationType: 'Licence Holder' }], + customerContacts: [{ name: 'jimbo', communicationType: 'customer' }] + } +} + +function _viewLicenceReturns () { + const commonLicenceData = _viewLicence() + + return { + ...commonLicenceData, + activeTab: 'returns', + returns: [ + { id: 'returns-id' } + ], + noReturnsMessage: null + } +} + function _viewLicenceSetUp () { + const commonLicenceData = _viewLicence() + return { + ...commonLicenceData, activeTab: 'set-up', agreements: [{}], chargeInformation: [{ }], @@ -488,28 +419,33 @@ function _viewLicenceSetUp () { } } -function _viewLicenceCommunications () { - return { - activeTab: 'communications', - communications: [{ type: 'paper return', sent: 'date', sender: 'admin@email.com', method: 'letter' }] - } -} - -function _viewLicenceContactDetails () { - return { - activeTab: 'contact-details', - licenceContacts: [{ name: 'jobo', communicationType: 'Licence Holder' }], - customerContacts: [{ name: 'jimbo', communicationType: 'customer' }] - } -} - function _viewLicenceSummary () { + const commonLicenceData = _viewLicence() + return { + ...commonLicenceData, id: '7861814c-ca19-43f2-be11-3c612f0d744b', - licenceRef: '01/130/R01', region: 'Southern', startDate: '1 November 2022', endDate: '1 November 2032', activeTab: 'summary' } } + +function _viewLicence () { + return { + documentId: 'e8f491f0-0c60-4083-9d41-d2be69f17a1e', + licenceId: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', + licenceName: 'Between two ferns', + licenceRef: '01/123', + notification: null, + pageTitle: 'Licence 01/123', + primaryUser: { + id: 10036, + username: 'grace.hopper@example.co.uk' + }, + roles: ['billing', 'view_charge_versions'], + warning: null, + workflowWarning: true + } +} From 9c95b77f9dc4815bd8dbe8a8c05645d6d03989f6 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 23:51:19 +0100 Subject: [PATCH 23/24] Fix broken user tests Ahh! It wasn't our new seed breaking the tests, but the new field we'd added to the view that isn't in our seed data. --- test/models/charge-version-note.model.test.js | 2 +- test/models/group.model.test.js | 2 +- test/models/return-version.model.test.js | 2 +- test/models/role.model.test.js | 2 +- test/models/user-group.model.test.js | 2 +- test/models/user-role.model.test.js | 2 +- test/services/idm/fetch-user-roles-and-groups.service.test.js | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/models/charge-version-note.model.test.js b/test/models/charge-version-note.model.test.js index 6b1df9c2a3..9234e792d0 100644 --- a/test/models/charge-version-note.model.test.js +++ b/test/models/charge-version-note.model.test.js @@ -77,7 +77,7 @@ describe('Charge Version Note model', () => { expect(result.id).to.equal(testRecord.id) expect(result.user).to.be.instanceOf(UserModel) - expect(result.user).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.user).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/models/group.model.test.js b/test/models/group.model.test.js index ae031a3914..60ad69076f 100644 --- a/test/models/group.model.test.js +++ b/test/models/group.model.test.js @@ -141,7 +141,7 @@ describe('Group model', () => { expect(result.users).to.be.an.array() expect(result.users).to.have.length(1) expect(result.users[0]).to.be.an.instanceOf(UserModel) - expect(result.users[0]).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.users[0]).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/models/return-version.model.test.js b/test/models/return-version.model.test.js index 63010786bb..7ef83ceeb6 100644 --- a/test/models/return-version.model.test.js +++ b/test/models/return-version.model.test.js @@ -172,7 +172,7 @@ describe('Return Version model', () => { expect(result.id).to.equal(testRecord.id) expect(result.user).to.be.an.instanceOf(UserModel) - expect(result.user).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.user).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/models/role.model.test.js b/test/models/role.model.test.js index 4531d17ff3..46015d8a9d 100644 --- a/test/models/role.model.test.js +++ b/test/models/role.model.test.js @@ -140,7 +140,7 @@ describe('Role model', () => { expect(result.users).to.be.an.array() expect(result.users).to.have.length(1) expect(result.users[0]).to.be.an.instanceOf(UserModel) - expect(result.users[0]).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.users[0]).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/models/user-group.model.test.js b/test/models/user-group.model.test.js index db0dc3a5f1..9718b041f4 100644 --- a/test/models/user-group.model.test.js +++ b/test/models/user-group.model.test.js @@ -81,7 +81,7 @@ describe('User Group model', () => { expect(result.id).to.equal(testRecord.id) expect(result.user).to.be.an.instanceOf(UserModel) - expect(result.user).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.user).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/models/user-role.model.test.js b/test/models/user-role.model.test.js index 8dc448109a..a609d93081 100644 --- a/test/models/user-role.model.test.js +++ b/test/models/user-role.model.test.js @@ -79,7 +79,7 @@ describe('User Role model', () => { expect(result.id).to.equal(testRecord.id) expect(result.user).to.be.an.instanceOf(UserModel) - expect(result.user).to.equal(testUser, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.user).to.equal(testUser, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) }) }) diff --git a/test/services/idm/fetch-user-roles-and-groups.service.test.js b/test/services/idm/fetch-user-roles-and-groups.service.test.js index d4e5b993b8..7534c76c36 100644 --- a/test/services/idm/fetch-user-roles-and-groups.service.test.js +++ b/test/services/idm/fetch-user-roles-and-groups.service.test.js @@ -44,7 +44,7 @@ describe('Fetch User Roles And Groups service', () => { it('returns the user', async () => { const result = await FetchUserRolesAndGroupsService.go(user.id) - expect(result.user).to.equal(user, { skip: ['createdAt', 'password', 'updatedAt'] }) + expect(result.user).to.equal(user, { skip: ['createdAt', 'licenceEntityId', 'password', 'updatedAt'] }) }) it("returns the user's roles", async () => { From 6a96585e72d416514a67b9d21acbd5ae484ce1e9 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 28 Aug 2024 23:59:06 +0100 Subject: [PATCH 24/24] Add back in basic access user to seeds You weren't the problem little fella! Back in you go. --- db/seeds/data/users.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/db/seeds/data/users.js b/db/seeds/data/users.js index a0cadcbc9b..d65d221924 100644 --- a/db/seeds/data/users.js +++ b/db/seeds/data/users.js @@ -120,6 +120,18 @@ const data = [ application: 'water_vml', resetGuidCreatedAt: null, enabled: true + }, + { + id: 100010, + username: 'basic.access@wrls.gov.uk', + password: 'P@55word', + resetGuid: null, + resetRequired: 0, + lastLogin: null, + badLogins: 0, + application: 'water_admin', + resetGuidCreatedAt: null, + enabled: true } ]