Skip to content

Commit

Permalink
Add base point model with a describe helper (#1289)
Browse files Browse the repository at this point in the history
https://eaflood.atlassian.net/browse/WATER-4645

> Part of the work to migrate return versions from NALD to WRLS

**Context**

We've been extending and amending the import of return versions from NALD to WRLS as part of our work to switch from NALD to WRLS to manage them. To support this, we [Created a return-requirement-points table](DEFRA/water-abstraction-service#2540) and [updated the import](DEFRA/water-abstraction-import#933) to import them point details.

Users select these points as part of the return requirements setup journey we've built. We currently extract them from the JSON blob stored in the `permit.licence` table. However, the import service only populates the points for licences.

- That have not ended
- That have a current licence version

Otherwise, `permit.licence` is not populated with the points data our journey relies on, causing it to throw an error. Places like the view licence page are also affected by this.

For example, it is perfectly valid that we have an 'ended' licence that we need to correct the historic return versions. And no matter the state, we can see what points the licence was linked to.

Thanks to [this change in water-abstraction-import we are now importing them](DEFRA/water-abstraction-import#1009) to a [new table in the `water` schema](DEFRA/water-abstraction-service#2626). And with [Add Licence Version Purpose Point model](#1288) we enabled access to the data via [Objection.js](https://vincit.github.io/objection.js/).

**Change**

Depending on the detail in a 'point', it needs to be described in 3 different ways.

- when a single grid reference as a _point_
- when two grid references as a _reach_
- when four grid references as an _area_

> A point can only have 1, 2 or 4 grid references, never 3!

We already have so many places in this repo where we need to work this out that we have a general helper method (`generateAbstractionPointDetail()` in `app/lib/general.lib.js`). This will become defunct, however, when we start pulling the data using our new `LicenceVersionPurposePointModel`.

We already have a pattern for models that contain data that needs to be interpreted in a certain way, such as the contact name in `ContactModel` and the licence holder in `LicenceModel`.

The slight difference is that we now have _two_ models that need the same ability. So, this change will introduce a new `BasePointModel` that the existing `ReturnRequirementPointModel` and `LicenceVersionPurposePointModel` will extend. On the new `BasePointModel`, we'll add the Objection.js instance function `$describe()`, which will interpret the point data and determine what description to use.

> In later changes, we'll start updating the repo to use the new helper and model rather than the old one.
  • Loading branch information
Cruikshanks authored Aug 29, 2024
1 parent b8fc0aa commit c28f58e
Show file tree
Hide file tree
Showing 6 changed files with 150 additions and 47 deletions.
40 changes: 0 additions & 40 deletions app/lib/general.lib.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,45 +152,6 @@ function generateAbstractionPointDetail (pointDetail) {
return abstractionPoint
}

/**
* Generate a string that represents an abstraction point based on the assumption the points have already been merged
*
* When abstracting water the point at which this is done can be described in several ways depending on the number of
* Nation Grid References are saved against the abstraction point. This function checks for these references and builds
* a string that defines the details of the abstraction point.
*
* This follows the same out put as generateAbstractionPointDetail but the points have already been merged
*
* @param {object} pointDetail - Object containing all the details for the point
*
* @returns {string} a description of the abstraction point
*/
function generatePointDetail (pointDetail) {
let abstractionPoint = null

if (pointDetail.ngr4) {
const point1 = pointDetail.ngr1
const point2 = pointDetail.ngr2
const point3 = pointDetail.ngr3
const point4 = pointDetail.ngr4

abstractionPoint = `Within the area formed by the straight lines running between National Grid References ${point1} ${point2} ${point3} and ${point4}`
} else if (pointDetail.ngr2) {
const point1 = pointDetail.ngr1
const point2 = pointDetail.ngr2

abstractionPoint = `Between National Grid References ${point1} and ${point2}`
} else {
const point1 = pointDetail.ngr1

abstractionPoint = `At National Grid Reference ${point1}`
}

abstractionPoint += pointDetail.description !== undefined ? ` (${pointDetail.description})` : ''

return abstractionPoint
}

/**
* Generate a Universally Unique Identifier (UUID)
*
Expand Down Expand Up @@ -334,7 +295,6 @@ module.exports = {
determineCurrentFinancialYear,
flashNotification,
generateAbstractionPointDetail,
generatePointDetail,
generateUUID,
periodsOverlap,
timestampForPostgres,
Expand Down
48 changes: 48 additions & 0 deletions app/models/base-point.model.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
'use strict'

/**
* @module BasePointModel
*/

const BaseModel = require('./base.model.js')

/**
* Base class for all 'point' based models, for example, `ReturnRequirementPointModel`
*/
class BasePointModel extends BaseModel {
/**

Check warning on line 13 in app/models/base-point.model.js

View workflow job for this annotation

GitHub Actions / build

Required 1 line(s) before JSDoc block
* Generate a string that describes this abstraction point
*
* When abstracting water the point at which this is done can be described in several ways depending on the number of
* Nation Grid References saved against the abstraction point.
*
* - 'point' - A single point grid reference
* - 'reach' - Two grid references defining a 'reach' or line along which water will be abstracted
* - 'area' - Four grid references defining the area water will be abstracted
*
* Finally, when the point is added to the system the user can add a supplementary description. If this exists the
* description returned combines the point description and the supplementary one.
*
* @returns {string} the description of this abstraction point
*/
$describe () {
let abstractionPoint

// If ng4 is populated then we know this point is an 'area'
if (this.ngr4) {
abstractionPoint = `Within the area formed by the straight lines running between National Grid References ${this.ngr1} ${this.ngr2} ${this.ngr3} and ${this.ngr4}`
} else if (this.ngr2) {
// If ng2 is populated then we know this point is a 'reach'
abstractionPoint = `Between National Grid References ${this.ngr1} and ${this.ngr2}`
} else {
// Else this point must be ... a point!
abstractionPoint = `At National Grid Reference ${this.ngr1}`
}

abstractionPoint = this.description ? `${abstractionPoint} (${this.description})` : abstractionPoint

return abstractionPoint
}
}

module.exports = BasePointModel
4 changes: 2 additions & 2 deletions app/models/licence-version-purpose-point.model.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@

const { Model } = require('objection')

const BaseModel = require('./base.model.js')
const BasePointModel = require('./base-point.model.js')

class LicenceVersionPurposePointModel extends BaseModel {
class LicenceVersionPurposePointModel extends BasePointModel {
static get tableName () {
return 'licenceVersionPurposePoints'
}
Expand Down
4 changes: 2 additions & 2 deletions app/models/return-requirement-point.model.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@

const { Model } = require('objection')

const BaseModel = require('./base.model.js')
const BasePointModel = require('./base-point.model.js')

class ReturnRequirementPointModel extends BaseModel {
class ReturnRequirementPointModel extends BasePointModel {
static get tableName () {
return 'returnRequirementPoints'
}
Expand Down
4 changes: 1 addition & 3 deletions app/presenters/return-requirements/view.presenter.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

const { formatAbstractionDate } = require('../base.presenter.js')
const { formatLongDate } = require('../base.presenter.js')
const { generatePointDetail } = require('../../lib/general.lib.js')
const { returnRequirementReasons, returnRequirementFrequencies } = require('../../lib/static-lookups.lib.js')

/**
Expand All @@ -18,7 +17,6 @@ const { returnRequirementReasons, returnRequirementFrequencies } = require('../.
*
* @returns {object} requirements for returns data needed by the view template
*/

function go (returnVersion) {
const { licence, multipleUpload, returnRequirements, startDate, status } =
returnVersion
Expand Down Expand Up @@ -133,7 +131,7 @@ function _purposes (returnRequirementPurposes) {

function _points (returnRequirementPoints) {
return returnRequirementPoints.map((returnRequirementPoint) => {
return generatePointDetail(returnRequirementPoint)
return returnRequirementPoint.$describe()
})
}

Expand Down
97 changes: 97 additions & 0 deletions test/models/base-point.model.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
'use strict'

// Test framework dependencies
const Lab = require('@hapi/lab')
const Code = require('@hapi/code')

const { describe, it, beforeEach } = exports.lab = Lab.script()
const { expect } = Code

// Thing under test
const BasePointModel = require('../../app/models/base-point.model.js')

describe('Base Point model', () => {
let testRecord

describe('$describe', () => {
describe('when the instance represents a "point" (1 grid reference)', () => {
beforeEach(() => {
testRecord = BasePointModel.fromJson({ ngr1: 'ST 58212 72697' })
})

describe('and it has a supplementary description', () => {
beforeEach(() => {
testRecord.description = 'Head office'
})

it('returns a "point" description including the supplementary', () => {
const result = testRecord.$describe()

expect(result).to.equal('At National Grid Reference ST 58212 72697 (Head office)')
})
})

describe('and it does not have a supplementary description', () => {
it('returns just the "point" description', () => {
const result = testRecord.$describe()

expect(result).to.equal('At National Grid Reference ST 58212 72697')
})
})
})

describe('when the instance represents a "reach" (2 grid references)', () => {
beforeEach(() => {
testRecord = BasePointModel.fromJson({ ngr1: 'ST 58212 72697', ngr2: 'ST 58151 72683' })
})

describe('and it has a supplementary description', () => {
beforeEach(() => {
testRecord.description = 'Head office'
})

it('returns a "reach" description including the supplementary', () => {
const result = testRecord.$describe()

expect(result).to.equal('Between National Grid References ST 58212 72697 and ST 58151 72683 (Head office)')
})
})

describe('and it does not have a supplementary description', () => {
it('returns just the "reach" description', () => {
const result = testRecord.$describe()

expect(result).to.equal('Between National Grid References ST 58212 72697 and ST 58151 72683')
})
})
})

describe('when the instance represents an "area" (4 grid references)', () => {
beforeEach(() => {
testRecord = BasePointModel.fromJson({
ngr1: 'ST 58212 72697', ngr2: 'ST 58151 72683', ngr3: 'ST 58145 72727', ngr4: 'ST 58222 72744'
})
})

describe('and it has a supplementary description', () => {
beforeEach(() => {
testRecord.description = 'Head office'
})

it('returns an "area" description including the supplementary', () => {
const result = testRecord.$describe()

expect(result).to.equal('Within the area formed by the straight lines running between National Grid References ST 58212 72697 ST 58151 72683 ST 58145 72727 and ST 58222 72744 (Head office)')
})
})

describe('and it does not have a supplementary description', () => {
it('returns just the "area" description', () => {
const result = testRecord.$describe()

expect(result).to.equal('Within the area formed by the straight lines running between National Grid References ST 58212 72697 ST 58151 72683 ST 58145 72727 and ST 58222 72744')
})
})
})
})
})

0 comments on commit c28f58e

Please sign in to comment.