Fix registered to link in view licence (#1291)

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'.

After looking at the issue, we thought that we had lost the ability to add the user ID to the link at some point.

Having dug into the `registeredToAndLicenceName()` modifier, we see it pulls out the correct name to display.

However, the view is expecting something to set `primaryUser.userId`. After double-checking the existing code and the original PR, we found no other reference to this.

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! 😱🤦

This means this minor fix ended up exploding in size. We started by modifying the users view migration to include the critical field. We then needed to update a series of models to add the missing relationships that take us from licence to user.

After doing that, we realized the existing `registeredToAndLicenceName()` modifier was doing too much and not returning what we needed. So, we replaced it with 2 new modifiers and associated instance methods.

This meant updating the fetch licence service to apply the new modifiers, updating the presenter to use the latest data being returned, and then the service and view that backs the top content in the view licence page.

Each time we touched a module, we found either dead or redundant code or tests that covered things already tested elsewhere. So, we've done a bunch of cleanup as we've worked through the fix.

In short, this change fixes the issue, plus _a lot_ of housekeeping! 😁

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'
+        }
       }
     }
   }

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',

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'
+        }
       }
     }
   }

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')
@@ -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
+   *
+   * 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.
    *
-   * 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.
+   * If set this is visible on the view licence page above the licence reference.
    *
-   * 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.
+   * 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)} `null` if this instance does not have the additional properties needed to determine the
-   * licence name else the licence's custom 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
   }
 }
 

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',

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,40 +11,46 @@ 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
  */
 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) {
@@ -61,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'
   })

app/services/licences/fetch-licence-summary.service.js

@@ -31,13 +31,20 @@ async function _fetch (licenceId) {
       'startDate'
     ])
     .modify('currentVersion')
+    .modify('licenceHolder')
     .withGraphFetched('region')
     .modifyGraph('region', (builder) => {
       builder.select([
         'id',
         '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 = {