From 1f069151e64ed2e331e4e217ef92b9ab35a94752 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 5 Feb 2024 00:05:46 +0000 Subject: [PATCH] Update no-returns-required to use submit service (#708) 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, but we also 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. --- .../return-requirements.controller.js | 8 +- app/lib/static-lookups.lib.js | 7 -- .../no-returns-required.presenter.js | 42 +------- .../no-returns-required.service.js | 11 +-- .../submit-no-returns-required.service.js | 57 +++++++++++ .../no-returns-required.validator.js | 4 +- .../no-returns-required.njk | 44 ++++++--- .../no-returns-required.presenter.test.js | 31 +----- .../no-returns-required.service.test.js | 46 ++++----- ...submit-no-returns-required.service.test.js | 97 +++++++++++++++++++ .../no-returns-required.validator.test.js | 25 +++-- 11 files changed, 230 insertions(+), 142 deletions(-) create mode 100644 app/services/return-requirements/submit-no-returns-required.service.js create mode 100644 test/services/return-requirements/submit-no-returns-required.service.test.js 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) } 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 98b8ab44a1..bbd681f0a0 100644 --- a/app/presenters/return-requirements/no-returns-required.presenter.js +++ b/app/presenters/return-requirements/no-returns-required.presenter.js @@ -5,53 +5,15 @@ * @module NoReturnsRequiredPresenter */ -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) + licenceRef: session.data.licence.licenceRef } return data } -function _error (error) { - if (!error) { - return null - } - - const errorMessage = { - text: error.message - } - - return errorMessage -} - -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/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', 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..7a7727fc5a --- /dev/null +++ b/app/services/return-requirements/submit-no-returns-required.service.js @@ -0,0 +1,57 @@ +'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, + 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 +} diff --git a/app/validators/return-requirements/no-returns-required.validator.js b/app/validators/return-requirements/no-returns-required.validator.js index 3feb8cfef6..159c1be249 100644 --- a/app/validators/return-requirements/no-returns-required.validator.js +++ b/app/validators/return-requirements/no-returns-required.validator.js @@ -9,10 +9,12 @@ const Joi = require('joi') function go (data) { const schema = Joi.object({ - reasonNewRequirements: Joi.string() + '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/app/views/return-requirements/no-returns-required.njk b/app/views/return-requirements/no-returns-required.njk index 8b7a969416..b32d29a8b5 100644 --- a/app/views/return-requirements/no-returns-required.njk +++ b/app/views/return-requirements/no-returns-required.njk @@ -4,35 +4,34 @@ {% 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 %} {% block content %} - {% if errorMessage %} + {% if error %} {{ govukErrorSummary({ titleText: "There is a problem", errorList: [ { - text: errorMessage.text, - href: "#reasonNewRequirements-error" + text: error.text, + href: "#no-returns-required-error" } ] }) }} {% endif %} -
- {{ govukRadios({ - idPrefix: "reasonNewRequirements", - name: "reasonNewRequirements", - errorMessage: errorMessage, +
+ + {{ govukRadios({ + name: "no-returns-required", + errorMessage: error, fieldset: { legend: { html: 'Licence ' + licenceRef + '' + pageTitle, @@ -40,12 +39,27 @@ classes: "govuk-fieldset__legend--l govuk-!-margin-bottom-6" } }, - items: radioItems - }) }} + 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 %} 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/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' + }) + }) + }) + }) + }) +}) 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..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 @@ -7,28 +7,33 @@ 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', () => { describe('when valid data is provided', () => { - reasonNewRequirementsFields.forEach(reason => { - it(`confirms the data is valid (reason ${reason})`, () => { - const result = NoReturnsRequiredValidator.go({ reasonNewRequirements: 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() }) }) 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() + 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()