From 5119b373b5dd61fc0f4650420a9d5a8ba144bb3a Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:16:58 +0000 Subject: [PATCH 01/12] Update no-returns-required to use submit service https://eaflood.atlassian.net/browse/WATER-4262 When we added validation and controls to the 'no returns required' page of the returns requirements setup journey it was the first page that we did so. It just has 3 radio buttons and no business logic so validation of the submitted form was quite simple. Therefore, so was our solution. The [Select a start date page](https://github.com/DEFRA/water-abstraction-system/pull/646) on the overhand was a different story. Not only did we have to cover date validation for the first time, we had to combine the date entry with radio buttons and business logic. It was at this point we decided a whole new `Submit[Page]Service` was needed to manage handling the validation. In both cases we will also need to handle persisting valid results into the session so a service separate from what we use during the `GET` request starts to make a lot of sense. This change revisits the 'no returns required' page to update it to use a `SubmitNoReturnsRequiredService`. Hopefully, this will make clear our patterns and conventions going forward. From ef3f29225749f3458f4aaaf034c12e3df38f8c74 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:48:27 +0000 Subject: [PATCH 02/12] Add the new submit service --- .../submit-no-returns-required.service.js | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 app/services/return-requirements/submit-no-returns-required.service.js diff --git a/app/services/return-requirements/submit-no-returns-required.service.js b/app/services/return-requirements/submit-no-returns-required.service.js new file mode 100644 index 0000000000..5692fbf6f5 --- /dev/null +++ b/app/services/return-requirements/submit-no-returns-required.service.js @@ -0,0 +1,58 @@ +'use strict' + +/** + * Orchestrates validating the data for `/return-requirements/{sessionId}/no-returns-required` page + * @module StartDateService + */ + +const NoReturnsRequiredPresenter = require('../../presenters/return-requirements/no-returns-required.presenter.js') +const NoReturnsRequiredValidator = require('../../validators/return-requirements/no-returns-required.validator.js') +const SessionModel = require('../../models/session.model.js') + +/** + * Orchestrates validating the data for `/return-requirements/{sessionId}/no-returns-required` page + * + * It first retrieves the session instance for the returns requirements journey in progress. + * + * The validation result is then combined with the output of the presenter to generate the page data needed by the view. + * If there was a validation error the controller will re-render the page so needs this information. If all is well the + * controller will redirect to the next page in the journey. + * + * @param {string} sessionId - The id of the current session + * @param {Object} payload - The submitted form data + * + * @returns {Promise} The page data for the start date page + */ +async function go (sessionId, payload) { + const session = await SessionModel.query().findById(sessionId) + + const validationResult = _validate(payload) + + const formattedData = NoReturnsRequiredPresenter.go(session, payload) + + return { + activeNavBar: 'search', + error: validationResult, + journey: session.data.journey, + pageTitle: 'Why are no returns required?', + ...formattedData + } +} + +function _validate (payload) { + const validation = NoReturnsRequiredValidator.go(payload) + + if (!validation.error) { + return null + } + + const { message } = validation.error.details[0] + + return { + text: message + } +} + +module.exports = { + go +} From 8b23de06829002d4c9f89f143490ff851b5f28bf Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:48:42 +0000 Subject: [PATCH 03/12] Remove error handling from the presenter The submit service is doing this for us now. --- .../no-returns-required.presenter.js | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/app/presenters/return-requirements/no-returns-required.presenter.js b/app/presenters/return-requirements/no-returns-required.presenter.js index 98b8ab44a1..c6a1221b80 100644 --- a/app/presenters/return-requirements/no-returns-required.presenter.js +++ b/app/presenters/return-requirements/no-returns-required.presenter.js @@ -7,10 +7,9 @@ const { reasonNewRequirementsFields } = require('../../lib/static-lookups.lib.js') -function go (session, error = null) { +function go (session) { const data = { id: session.id, - errorMessage: _error(error), licenceRef: session.data.licence.licenceRef, radioItems: _radioItems(session) } @@ -18,18 +17,6 @@ function go (session, error = null) { return data } -function _error (error) { - if (!error) { - return null - } - - const errorMessage = { - text: error.message - } - - return errorMessage -} - function _radioItems (_session) { const radioItems = [ { From 3be9811f53cd17bfc366ee3405312711e3a00c37 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:49:25 +0000 Subject: [PATCH 04/12] Remove optional error from the GET service We'll never pass an error in now because we'll never call this in the POST handler. --- .../no-returns-required.service.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/app/services/return-requirements/no-returns-required.service.js b/app/services/return-requirements/no-returns-required.service.js index 2cef85a989..f8e023a89a 100644 --- a/app/services/return-requirements/no-returns-required.service.js +++ b/app/services/return-requirements/no-returns-required.service.js @@ -13,18 +13,13 @@ const SessionModel = require('../../models/session.model.js') * Supports generating the data needed for the no returns required page in the return requirements setup journey. It * fetches the current session record and combines it with the radio buttons and other information needed for the form. * - * If a validation issue is found when the form is submitted, it will be called from the POST handler with the Joi - * validation error passed in. This extra information will be used to ensure the right error message is included in the - * data needed by the view. - * * @param {string} id - The UUID for return requirement setup session record - * @param {Object} [error] - A Joi validation error if an issue was found with the submitted form data * - * @returns {Object} page data needed by the view template + * @returns {Promise} The view data for the no returns required page */ -async function go (sessionId, error = null) { +async function go (sessionId) { const session = await SessionModel.query().findById(sessionId) - const formattedData = NoReturnsRequiredPresenter.go(session, error) + const formattedData = NoReturnsRequiredPresenter.go(session) return { activeNavBar: 'search', From 96cc3207bad69b024c931948c743a12b5d81505b Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:50:14 +0000 Subject: [PATCH 05/12] Update view to reference error We adding an error property to the page data to be consistent across the pages. As we learnt on the 'Select a start date page' we might need something more complex than just the text message. Having an `error:` object gives some consistency to the page data and the flexibility to add whatever need (or not). --- app/views/return-requirements/no-returns-required.njk | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/return-requirements/no-returns-required.njk b/app/views/return-requirements/no-returns-required.njk index 8b7a969416..e20ac7bf9a 100644 --- a/app/views/return-requirements/no-returns-required.njk +++ b/app/views/return-requirements/no-returns-required.njk @@ -16,12 +16,12 @@ {% endblock %} {% block content %} - {% if errorMessage %} + {% if error %} {{ govukErrorSummary({ titleText: "There is a problem", errorList: [ { - text: errorMessage.text, + text: error.text, href: "#reasonNewRequirements-error" } ] @@ -32,7 +32,7 @@ {{ govukRadios({ idPrefix: "reasonNewRequirements", name: "reasonNewRequirements", - errorMessage: errorMessage, + errorMessage: error, fieldset: { legend: { html: 'Licence ' + licenceRef + '' + pageTitle, From 0d21a2558949640f952b0ef128c5c31b1aae4244 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 22:52:09 +0000 Subject: [PATCH 06/12] Update controller to use new service --- app/controllers/return-requirements.controller.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/return-requirements.controller.js b/app/controllers/return-requirements.controller.js index fc99a35cab..5707bd23d5 100644 --- a/app/controllers/return-requirements.controller.js +++ b/app/controllers/return-requirements.controller.js @@ -6,9 +6,9 @@ */ const NoReturnsRequiredService = require('../services/return-requirements/no-returns-required.service.js') -const NoReturnsRequiredValidator = require('../validators/return-requirements/no-returns-required.validator.js') const SessionModel = require('../models/session.model.js') const StartDateService = require('../services/return-requirements/start-date.service.js') +const SubmitNoReturnsRequiredService = require('../services/return-requirements/submit-no-returns-required.service.js') const SubmitStartDateService = require('../services/return-requirements/submit-start-date.service.js') async function abstractionPeriod (request, h) { @@ -227,10 +227,10 @@ async function submitFrequencyReported (request, h) { async function submitNoReturnsRequired (request, h) { const { sessionId } = request.params - const validation = NoReturnsRequiredValidator.go(request.payload) - if (validation.error) { - const pageData = await NoReturnsRequiredService.go(sessionId, validation.error) + const pageData = await SubmitNoReturnsRequiredService.go(sessionId, request.payload) + + if (pageData.error) { return h.view('return-requirements/no-returns-required.njk', pageData) } From 009320563a2aeeebd41042458288963e17b64a4b Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:01:04 +0000 Subject: [PATCH 07/12] Move layout back into the view We're trying to keep a clean separation of concerns. The view templates manage everything to do with the layout and how things are presented. The 'presenters' are responsible for formatting the data the views need, whether it is to directly to display or make decisions on what to display. --- app/lib/static-lookups.lib.js | 7 ---- .../no-returns-required.presenter.js | 27 +------------- .../no-returns-required.njk | 37 +++++++++++++------ 3 files changed, 27 insertions(+), 44 deletions(-) diff --git a/app/lib/static-lookups.lib.js b/app/lib/static-lookups.lib.js index df69811e0a..d79b1980bb 100644 --- a/app/lib/static-lookups.lib.js +++ b/app/lib/static-lookups.lib.js @@ -28,17 +28,10 @@ const sources = [ 'wrls' ] -const reasonNewRequirementsFields = [ - 'abstraction_below_100_cubic_metres_per_day', - 'returns_exception', - 'transfer_licence' -] - module.exports = { billRunTypes, companyTypes, contactTypes, organisationTypes, - reasonNewRequirementsFields, sources } diff --git a/app/presenters/return-requirements/no-returns-required.presenter.js b/app/presenters/return-requirements/no-returns-required.presenter.js index c6a1221b80..bbd681f0a0 100644 --- a/app/presenters/return-requirements/no-returns-required.presenter.js +++ b/app/presenters/return-requirements/no-returns-required.presenter.js @@ -5,40 +5,15 @@ * @module NoReturnsRequiredPresenter */ -const { reasonNewRequirementsFields } = require('../../lib/static-lookups.lib.js') - function go (session) { const data = { id: session.id, - licenceRef: session.data.licence.licenceRef, - radioItems: _radioItems(session) + licenceRef: session.data.licence.licenceRef } return data } -function _radioItems (_session) { - const radioItems = [ - { - value: reasonNewRequirementsFields[0], - text: 'Abstraction amount below 100 cubic metres per day', - checked: false - }, - { - value: reasonNewRequirementsFields[1], - text: 'Returns exception', - checked: false - }, - { - value: reasonNewRequirementsFields[2], - text: 'Transfer licence', - checked: false - } - ] - - return radioItems -} - module.exports = { go } diff --git a/app/views/return-requirements/no-returns-required.njk b/app/views/return-requirements/no-returns-required.njk index e20ac7bf9a..288a916945 100644 --- a/app/views/return-requirements/no-returns-required.njk +++ b/app/views/return-requirements/no-returns-required.njk @@ -30,18 +30,33 @@
{{ govukRadios({ - idPrefix: "reasonNewRequirements", - name: "reasonNewRequirements", - errorMessage: error, - fieldset: { - legend: { - html: 'Licence ' + licenceRef + '' + pageTitle, - isPageHeading: true, - classes: "govuk-fieldset__legend--l govuk-!-margin-bottom-6" - } + name: "reasonNewRequirements", + errorMessage: error, + fieldset: { + legend: { + html: 'Licence ' + licenceRef + '' + pageTitle, + isPageHeading: true, + classes: "govuk-fieldset__legend--l govuk-!-margin-bottom-6" + } + }, + items: [ + { + text: 'Abstraction amount below 100 cubic metres per day', + value: 'abstraction_below_100_cubic_metres_per_day', + checked: false + }, + { + text: 'Returns exception', + value: 'returns_exception', + checked: false }, - items: radioItems - }) }} + { + text: 'Transfer licence', + value: 'transfer_licence', + checked: false + } + ] + }) }}
{{ govukButton({ text: "Continue" }) }} From 628147b0e6e5ec6caeb97c4c89ed626e18712cd4 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:04:17 +0000 Subject: [PATCH 08/12] Correct the name of the form element --- .../return-requirements/no-returns-required.validator.js | 2 +- app/views/return-requirements/no-returns-required.njk | 4 ++-- .../no-returns-required/no-returns-required.validator.test.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/validators/return-requirements/no-returns-required.validator.js b/app/validators/return-requirements/no-returns-required.validator.js index 3feb8cfef6..2dc961c401 100644 --- a/app/validators/return-requirements/no-returns-required.validator.js +++ b/app/validators/return-requirements/no-returns-required.validator.js @@ -9,7 +9,7 @@ const Joi = require('joi') function go (data) { const schema = Joi.object({ - reasonNewRequirements: Joi.string() + 'no-returns-required': Joi.string() .required() .messages({ 'any.required': 'Select the reason for the return requirement', diff --git a/app/views/return-requirements/no-returns-required.njk b/app/views/return-requirements/no-returns-required.njk index 288a916945..adc1008bce 100644 --- a/app/views/return-requirements/no-returns-required.njk +++ b/app/views/return-requirements/no-returns-required.njk @@ -22,7 +22,7 @@ errorList: [ { text: error.text, - href: "#reasonNewRequirements-error" + href: "#no-returns-required-error" } ] }) }} @@ -30,7 +30,7 @@ {{ govukRadios({ - name: "reasonNewRequirements", + name: "no-returns-required", errorMessage: error, fieldset: { legend: { diff --git a/test/validators/no-returns-required/no-returns-required.validator.test.js b/test/validators/no-returns-required/no-returns-required.validator.test.js index 9336c03cc7..db95d878f1 100644 --- a/test/validators/no-returns-required/no-returns-required.validator.test.js +++ b/test/validators/no-returns-required/no-returns-required.validator.test.js @@ -17,7 +17,7 @@ describe('No Returns Required validator', () => { describe('when valid data is provided', () => { reasonNewRequirementsFields.forEach(reason => { it(`confirms the data is valid (reason ${reason})`, () => { - const result = NoReturnsRequiredValidator.go({ reasonNewRequirements: reason }) + const result = NoReturnsRequiredValidator.go({ 'no-returns-required': reason }) expect(result.value).to.exist() expect(result.error).not.to.exist() @@ -28,7 +28,7 @@ describe('No Returns Required validator', () => { describe('when valid data is provided', () => { describe("because no 'reason' is given", () => { it('fails validation', () => { - const result = NoReturnsRequiredValidator.go({ reasonNewRequirements: '' }) + const result = NoReturnsRequiredValidator.go({ 'no-returns-required': '' }) expect(result.value).to.exist() expect(result.error).to.exist() From eaaaa4fbf84a2a15480287740fce45500bf6e3b7 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:29:09 +0000 Subject: [PATCH 09/12] Fix the tests --- .../no-returns-required.presenter.test.js | 31 +------------ .../no-returns-required.service.test.js | 46 ++++++++----------- .../no-returns-required.validator.test.js | 7 ++- 3 files changed, 23 insertions(+), 61 deletions(-) diff --git a/test/presenters/return-requirements/no-returns-required.presenter.test.js b/test/presenters/return-requirements/no-returns-required.presenter.test.js index b3ec444fd2..1ec2501336 100644 --- a/test/presenters/return-requirements/no-returns-required.presenter.test.js +++ b/test/presenters/return-requirements/no-returns-required.presenter.test.js @@ -31,38 +31,9 @@ describe('No Returns Required presenter', () => { const result = NoReturnsRequiredPresenter.go(session) expect(result).to.equal({ - errorMessage: null, id: 'f1288f6c-8503-4dc1-b114-75c408a14bd0', - licenceRef: '01/123', - radioItems: [ - { - checked: false, - text: 'Abstraction amount below 100 cubic metres per day', - value: 'abstraction_below_100_cubic_metres_per_day' - }, - { - checked: false, - text: 'Returns exception', - value: 'returns_exception' - }, - { - checked: false, - text: 'Transfer licence', - value: 'transfer_licence' - } - ] + licenceRef: '01/123' }) }) }) - - describe('when provided with an error', () => { - const error = new Error('Test error message') - - it('includes the error message in the presented data', () => { - const result = NoReturnsRequiredPresenter.go(session, error) - - expect(result.errorMessage).to.exist() - expect(result.errorMessage.text).to.equal(error.message) - }) - }) }) diff --git a/test/services/return-requirements/no-returns-required.service.test.js b/test/services/return-requirements/no-returns-required.service.test.js index 4bfb15d227..3b81b8c655 100644 --- a/test/services/return-requirements/no-returns-required.service.test.js +++ b/test/services/return-requirements/no-returns-required.service.test.js @@ -19,7 +19,18 @@ describe('No Returns Required service', () => { beforeEach(async () => { await DatabaseHelper.clean() - session = await SessionHelper.add({ data: { licence: { licenceRef: '01/123' } } }) + session = await SessionHelper.add({ + data: { + licence: { + id: '8b7f78ba-f3ad-4cb6-a058-78abc4d1383d', + currentVersionStartDate: '2023-01-01T00:00:00.000Z', + endDate: null, + licenceRef: '01/ABC', + licenceHolder: 'Turbo Kid', + startDate: '2022-04-01T00:00:00.000Z' + } + } + }) }) describe('when called', () => { @@ -29,33 +40,14 @@ describe('No Returns Required service', () => { expect(result.id).to.equal(session.id) }) - describe('without the optional error param', () => { - it('returns page data for the view', async () => { - const result = await NoReturnsRequiredService.go(session.id) - - expect(result.activeNavBar).to.exist() - expect(result.pageTitle).to.exist() - expect(result.licenceRef).to.exist() - expect(result.radioItems).to.exist() - - expect(result.errorMessage).to.be.null() - }) - }) - - describe('with the optional error param', () => { - const error = new Error('Test error message') - - it('returns page data for the view including the error message', async () => { - const result = await NoReturnsRequiredService.go(session.id, error) - - expect(result.activeNavBar).to.exist() - expect(result.pageTitle).to.exist() - expect(result.licenceRef).to.exist() - expect(result.radioItems).to.exist() + it('returns page data for the view', async () => { + const result = await NoReturnsRequiredService.go(session.id) - expect(result.errorMessage).to.exist() - expect(result.errorMessage.text).to.equal(error.message) - }) + expect(result).to.equal({ + activeNavBar: 'search', + pageTitle: 'Why are no returns required?', + licenceRef: '01/ABC' + }, { skip: ['id'] }) }) }) }) diff --git a/test/validators/no-returns-required/no-returns-required.validator.test.js b/test/validators/no-returns-required/no-returns-required.validator.test.js index db95d878f1..6273941e44 100644 --- a/test/validators/no-returns-required/no-returns-required.validator.test.js +++ b/test/validators/no-returns-required/no-returns-required.validator.test.js @@ -7,15 +7,14 @@ const Code = require('@hapi/code') const { describe, it } = exports.lab = Lab.script() const { expect } = Code -// Test helpers -const { reasonNewRequirementsFields } = require('../../../app/lib/static-lookups.lib.js') - // Thing under test const NoReturnsRequiredValidator = require('../../../app/validators/return-requirements/no-returns-required.validator.js') describe('No Returns Required validator', () => { + const values = ['bajor', 'risa', 'ferenginar'] + describe('when valid data is provided', () => { - reasonNewRequirementsFields.forEach(reason => { + values.forEach(reason => { it(`confirms the data is valid (reason ${reason})`, () => { const result = NoReturnsRequiredValidator.go({ 'no-returns-required': reason }) From d945322d462a96638583b6d985a0033e02f3c59c Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:43:28 +0000 Subject: [PATCH 10/12] Add test for new submit service And remove unused journey property from page data. --- .../submit-no-returns-required.service.js | 1 - ...submit-no-returns-required.service.test.js | 97 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 test/services/return-requirements/submit-no-returns-required.service.test.js diff --git a/app/services/return-requirements/submit-no-returns-required.service.js b/app/services/return-requirements/submit-no-returns-required.service.js index 5692fbf6f5..7a7727fc5a 100644 --- a/app/services/return-requirements/submit-no-returns-required.service.js +++ b/app/services/return-requirements/submit-no-returns-required.service.js @@ -33,7 +33,6 @@ async function go (sessionId, payload) { return { activeNavBar: 'search', error: validationResult, - journey: session.data.journey, pageTitle: 'Why are no returns required?', ...formattedData } diff --git a/test/services/return-requirements/submit-no-returns-required.service.test.js b/test/services/return-requirements/submit-no-returns-required.service.test.js new file mode 100644 index 0000000000..ac3d463f90 --- /dev/null +++ b/test/services/return-requirements/submit-no-returns-required.service.test.js @@ -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 + +// Test helpers +const DatabaseHelper = require('../../support/helpers/database.helper.js') +const SessionHelper = require('../../support/helpers/session.helper.js') + +// Thing under test +const SubmitNoReturnsRequiredService = require('../../../app/services/return-requirements/submit-no-returns-required.service.js') + +describe('Submit No Returns Required service', () => { + let payload + let session + + beforeEach(async () => { + await DatabaseHelper.clean() + + session = await SessionHelper.add({ + data: { + licence: { + id: '8b7f78ba-f3ad-4cb6-a058-78abc4d1383d', + currentVersionStartDate: '2023-01-01T00:00:00.000Z', + endDate: null, + licenceRef: '01/ABC', + licenceHolder: 'Turbo Kid', + startDate: '2022-04-01T00:00:00.000Z' + }, + journey: 'no-returns-required' + } + }) + }) + + describe('when called', () => { + describe('with a valid payload', () => { + beforeEach(() => { + payload = { + 'no-returns-required': 'abstraction_below_100_cubic_metres_per_day' + } + }) + + it('fetches the current setup session record', async () => { + const result = await SubmitNoReturnsRequiredService.go(session.id, payload) + + expect(result.id).to.equal(session.id) + }) + + it('returns page data for the view', async () => { + const result = await SubmitNoReturnsRequiredService.go(session.id, payload) + + expect(result).to.equal({ + activeNavBar: 'search', + error: null, + pageTitle: 'Why are no returns required?', + licenceRef: '01/ABC' + }, { skip: ['id'] }) + }) + }) + + describe('with an invalid payload', () => { + describe('because the user has not selected anything', () => { + beforeEach(() => { + payload = {} + }) + + it('fetches the current setup session record', async () => { + const result = await SubmitNoReturnsRequiredService.go(session.id, payload) + + expect(result.id).to.equal(session.id) + }) + + it('returns page data for the view', async () => { + const result = await SubmitNoReturnsRequiredService.go(session.id, payload) + + expect(result).to.equal({ + activeNavBar: 'search', + pageTitle: 'Why are no returns required?', + licenceRef: '01/ABC' + }, { skip: ['id', 'error'] }) + }) + + it('returns page data with an error', async () => { + const result = await SubmitNoReturnsRequiredService.go(session.id, payload) + + expect(result.error).to.equal({ + text: 'Select the reason for the return requirement' + }) + }) + }) + }) + }) +}) From 0c0ae715c4875ab11fd684f7eeb47c2266e08e42 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:55:12 +0000 Subject: [PATCH 11/12] Make the validator more robust As our unit test confirmed it was checking for a value but would accept any value. Now it will only accept certain values. --- .../no-returns-required.validator.js | 2 ++ .../no-returns-required.validator.test.js | 22 ++++++++++++------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/app/validators/return-requirements/no-returns-required.validator.js b/app/validators/return-requirements/no-returns-required.validator.js index 2dc961c401..159c1be249 100644 --- a/app/validators/return-requirements/no-returns-required.validator.js +++ b/app/validators/return-requirements/no-returns-required.validator.js @@ -11,8 +11,10 @@ function go (data) { const schema = Joi.object({ 'no-returns-required': Joi.string() .required() + .valid('abstraction_below_100_cubic_metres_per_day', 'returns_exception', 'transfer_licence') .messages({ 'any.required': 'Select the reason for the return requirement', + 'any.only': 'Select the reason for the return requirement', 'string.empty': 'Select the reason for the return requirement' }) }) diff --git a/test/validators/no-returns-required/no-returns-required.validator.test.js b/test/validators/no-returns-required/no-returns-required.validator.test.js index 6273941e44..028a09948f 100644 --- a/test/validators/no-returns-required/no-returns-required.validator.test.js +++ b/test/validators/no-returns-required/no-returns-required.validator.test.js @@ -11,16 +11,12 @@ const { expect } = Code const NoReturnsRequiredValidator = require('../../../app/validators/return-requirements/no-returns-required.validator.js') describe('No Returns Required validator', () => { - const values = ['bajor', 'risa', 'ferenginar'] - describe('when valid data is provided', () => { - values.forEach(reason => { - it(`confirms the data is valid (reason ${reason})`, () => { - const result = NoReturnsRequiredValidator.go({ 'no-returns-required': reason }) + it('confirms the data is valid', () => { + const result = NoReturnsRequiredValidator.go({ 'no-returns-required': 'transfer_licence' }) - expect(result.value).to.exist() - expect(result.error).not.to.exist() - }) + expect(result.value).to.exist() + expect(result.error).not.to.exist() }) }) @@ -34,5 +30,15 @@ describe('No Returns Required validator', () => { expect(result.error.details[0].message).to.equal('Select the reason for the return requirement') }) }) + + describe("because an unknown 'reason' is given", () => { + it('fails validation', () => { + const result = NoReturnsRequiredValidator.go({ 'no-returns-required': 'no-water' }) + + expect(result.value).to.exist() + expect(result.error).to.exist() + expect(result.error.details[0].message).to.equal('Select the reason for the return requirement') + }) + }) }) }) From 7bf8ba92e6dc28f5cb5079ca8a38e5b2b1a16dbb Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 4 Feb 2024 23:59:21 +0000 Subject: [PATCH 12/12] Update view to be consistent with start date view --- .../no-returns-required.njk | 65 +++++++++---------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/app/views/return-requirements/no-returns-required.njk b/app/views/return-requirements/no-returns-required.njk index adc1008bce..b32d29a8b5 100644 --- a/app/views/return-requirements/no-returns-required.njk +++ b/app/views/return-requirements/no-returns-required.njk @@ -4,13 +4,12 @@ {% from "govuk/components/error-summary/macro.njk" import govukErrorSummary %} {% from "govuk/components/radios/macro.njk" import govukRadios %} -{% set rootLink = "/system/return-requirements/" + id %} {% block breadcrumbs %} {# Back link #} {{ govukBackLink({ text: 'Back', - href: rootLink + '/start-date' + href: "/system/return-requirements/" + id + '/start-date' }) }} {% endblock %} @@ -28,39 +27,39 @@ }) }} {% endif %} - - {{ govukRadios({ - name: "no-returns-required", - errorMessage: error, - fieldset: { - legend: { - html: 'Licence ' + licenceRef + '' + pageTitle, - isPageHeading: true, - classes: "govuk-fieldset__legend--l govuk-!-margin-bottom-6" - } - }, - items: [ - { - text: 'Abstraction amount below 100 cubic metres per day', - value: 'abstraction_below_100_cubic_metres_per_day', - checked: false - }, - { - text: 'Returns exception', - value: 'returns_exception', - checked: false +
+ + {{ govukRadios({ + name: "no-returns-required", + errorMessage: error, + fieldset: { + legend: { + html: 'Licence ' + licenceRef + '' + pageTitle, + isPageHeading: true, + classes: "govuk-fieldset__legend--l govuk-!-margin-bottom-6" + } }, - { - text: 'Transfer licence', - value: 'transfer_licence', - checked: false - } - ] - }) }} + items: [ + { + text: 'Abstraction amount below 100 cubic metres per day', + value: 'abstraction_below_100_cubic_metres_per_day', + checked: false + }, + { + text: 'Returns exception', + value: 'returns_exception', + checked: false + }, + { + text: 'Transfer licence', + value: 'transfer_licence', + checked: false + } + ] + }) }} -
{{ govukButton({ text: "Continue" }) }} -
- + +
{% endblock %}