Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check your answers page with notes for additions, changes and deletions #923

Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4cf9d4b
Check your answers page with notes for additions, changes and deletions
Demwunz Apr 18, 2024
baf2882
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz Apr 23, 2024
ee24073
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz Apr 23, 2024
4bacf6f
feat(app): added note notification banner
Demwunz Apr 24, 2024
91d523c
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz Apr 24, 2024
04314cf
feat(app): use yar for notifications
Demwunz Apr 25, 2024
e327b60
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz Apr 25, 2024
b53b887
feat(app): delete note and notification
Demwunz Apr 25, 2024
3c81be0
feat(app): updated yar flash messages
Demwunz May 1, 2024
74a0aa4
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 1, 2024
f633c6e
fix(app): removed redundant test
Demwunz May 1, 2024
eb69746
fix(app): updated controller test
Demwunz May 1, 2024
b7c1a38
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 1, 2024
8fb7589
fix(app): address sonarcloud issues
Demwunz May 1, 2024
b841327
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 1, 2024
31c88d5
fix(app): minor amends
Demwunz May 1, 2024
bd3ea69
Update app/services/return-requirements/submit-add-note.service.js
Demwunz May 2, 2024
6647099
Update test/controllers/return-requirements.controller.test.js
Demwunz May 2, 2024
93c4286
Update app/services/return-requirements/delete-note.service.js
Demwunz May 2, 2024
f45d357
Update app/views/return-requirements/check-your-answers.njk
Demwunz May 2, 2024
6c5ad16
Update test/services/return-requirements/delete-note.service.test.js
Demwunz May 2, 2024
3afead9
Update test/services/return-requirements/delete-note.service.test.js
Demwunz May 2, 2024
264238b
Update test/services/return-requirements/delete-note.service.test.js
Demwunz May 2, 2024
b5fd806
fix(app): fix test
Demwunz May 2, 2024
e685bce
fix(app): fix textarea content
Demwunz May 2, 2024
d2986d1
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 2, 2024
ffb38e1
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 2, 2024
f1f4d5f
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 2, 2024
fa36f46
Update test/services/return-requirements/submit-add-note.service.test.js
Demwunz May 2, 2024
75a3cb8
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 3, 2024
b92d2be
Merge branch 'main' into WATER-4291-returns-required-journey-check-yo…
Demwunz May 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions app/controllers/return-requirements.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const AddNoteService = require('../services/return-requirements/add-note.service
const AbstractionPeriodService = require('../services/return-requirements/abstraction-period.service.js')
const AgreementsExceptionsService = require('../services/return-requirements/agreements-exceptions.service.js')
const CheckYourAnswersService = require('../services/return-requirements/check-your-answers.service.js')
const DeleteNoteService = require('../services/return-requirements/delete-note.service.js')
const FrequencyCollectedService = require('../services/return-requirements/frequency-collected.service.js')
const FrequencyReportedService = require('../services/return-requirements/frequency-reported.service.js')
const NoReturnsRequiredService = require('../services/return-requirements/no-returns-required.service.js')
Expand Down Expand Up @@ -77,13 +78,21 @@ async function approved (request, h) {

async function checkYourAnswers (request, h) {
const { sessionId } = request.params
const pageData = await CheckYourAnswersService.go(sessionId)
const pageData = await CheckYourAnswersService.go(sessionId, request.yar)

return h.view('return-requirements/check-your-answers.njk', {
...pageData
})
}

async function deleteNote (request, h) {
const { sessionId } = request.params

await DeleteNoteService.go(sessionId, request.yar)

return h.redirect(`/system/return-requirements/${sessionId}/check-your-answers`)
}

async function existing (request, h) {
const { sessionId } = request.params

Expand Down Expand Up @@ -215,7 +224,7 @@ async function submitAddNote (request, h) {
const { sessionId } = request.params
const { user } = request.auth.credentials

const pageData = await SubmitAddNoteService.go(sessionId, request.payload, user)
const pageData = await SubmitAddNoteService.go(sessionId, request.payload, user, request.yar)

if (pageData.error) {
return h.view('return-requirements/add-note.njk', pageData)
Expand Down Expand Up @@ -415,6 +424,7 @@ module.exports = {
agreementsExceptions,
approved,
checkYourAnswers,
deleteNote,
existing,
frequencyCollected,
frequencyReported,
Expand Down
13 changes: 13 additions & 0 deletions app/routes/return-requirement.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,19 @@ const routes = [
description: 'Submit check your answers'
}
},
{
method: 'GET',
path: '/return-requirements/{sessionId}/delete-note',
handler: ReturnRequirementsController.deleteNote,
options: {
auth: {
access: {
scope: ['billing']
}
},
description: 'Delete a note'
}
},
{
method: 'GET',
path: '/return-requirements/{sessionId}/existing',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,21 @@ const SessionModel = require('../../models/session.model.js')
* Orchestrates fetching and presenting the data for `/return-requirements/{sessionId}/check-your-answers` page
*
* @param {string} sessionId - The UUID for return requirement setup session record
* @param {Object} yar - The Hapi `request.yar` session manager passed on by the controller
*
* @returns {Promise<Object>} page data needed by the view template
*/
async function go (sessionId) {
async function go (sessionId, yar) {
const session = await SessionModel.query().findById(sessionId)

const notification = yar.flash('notification')[0]
const formattedData = CheckYourAnswersPresenter.go(session)

await _checkYourAnswersVisited(session)

return {
activeNavBar: 'search',
notification,
licenceRef: session.data.licence.licenceRef,
pageTitle: `Check the return requirements for ${session.data.licence.licenceHolder}`,
...formattedData
Expand Down
45 changes: 45 additions & 0 deletions app/services/return-requirements/delete-note.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict'

/**
* Orchestrates deleting the notes data for `/return-requirements/{sessionId}/check-your-answers` page
* @module DeleteNoteService
*/

const SessionModel = require('../../models/session.model.js')

/**
*
* It first retrieves the session instance for the returns requirements journey in progress.
*
* Then it removes the notes data from the session.
*
* @param {string} sessionId - The id of the current session
* @param {Object} yar - The Hapi `request.yar` session manager passed on by the controller
*
* @returns {Promise<Object>} The page data for the check-your-answers page
*/
async function go (sessionId, yar) {
const session = await SessionModel.query().findById(sessionId)
const notification = {
title: 'Removed',
text: 'Note removed'
}

await _save(session)

yar.flash('notification', notification)

return notification
Demwunz marked this conversation as resolved.
Show resolved Hide resolved
}

async function _save (session) {
const currentData = session.data

currentData.note = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to delete the note we ideally want to put it back to the state it was before we added one in the first place. If that is the case I think you need to call this instead.

Suggested change
currentData.note = ''
delete currentData.note

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this should have been the approach. updated


return session.$query().patch({ data: currentData })
}

module.exports = {
go
}
31 changes: 29 additions & 2 deletions app/services/return-requirements/submit-add-note.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,22 @@ const SessionModel = require('../../models/session.model.js')
* @param {string} sessionId - The id of the current session
* @param {Object} payload - The submitted form data
* @param {Object} user - The logged in user details
* @param {Object} yar - The Hapi `request.yar` session manager passed on by the controller
*
* @returns {Promise<Object>} The page data for the no returns required page
* @returns {Promise<Object>} The page data for the check-your-answers page
*/
async function go (sessionId, payload, user) {
async function go (sessionId, payload, user, yar) {
const session = await SessionModel.query().findById(sessionId)
const validationResult = _validate(payload)

if (!validationResult) {
const notification = _notification(session, payload.note)
await _save(session, payload, user)

if (notification) {
yar.flash('notification', notification)
}

return {
journey: session.data.journey
}
Expand All @@ -45,6 +51,27 @@ async function go (sessionId, payload, user) {
}
}

function _notification (session, newNote) {
const { data: { note } } = session
let notification = ''

if (!note && newNote) {
notification = {
title: 'Added',
text: 'Changes made'
}
}

if (note && note.content !== newNote) {
notification = {
title: 'Updated',
text: 'Changes made'
}
}

return notification
}
Demwunz marked this conversation as resolved.
Show resolved Hide resolved

async function _save (session, payload, user) {
const currentData = session.data

Expand Down
13 changes: 12 additions & 1 deletion app/views/return-requirements/check-your-answers.njk
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{% extends 'layout.njk' %}
{% from "govuk/components/button/macro.njk" import govukButton %}
{% from "govuk/components/notification-banner/macro.njk" import govukNotificationBanner %}
{% from "govuk/components/summary-list/macro.njk" import govukSummaryList %}

{% set baseURL = "/system/return-requirements/" + id %}
Expand Down Expand Up @@ -45,6 +46,12 @@
{% endset %}

{% block content %}
{% if notification %}
{{ govukNotificationBanner({
titleText: notification.title,
text: notification.text
}) }}
{% endif %}
Demwunz marked this conversation as resolved.
Show resolved Hide resolved
{# Main heading #}
<div class="govuk-body">
<h1 class="govuk-heading-xl govuk-!-margin-bottom-3">
Expand Down Expand Up @@ -120,7 +127,11 @@
{
href: "add-note",
text: "Change" if note else "Add a note"
}
},
{
href: "delete-note",
text: "Delete"
} if note
]
}
}
Expand Down
19 changes: 18 additions & 1 deletion test/controllers/return-requirements.controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const AbstractionPeriodService = require('../../app/services/return-requirements
const AddNoteService = require('../../app/services/return-requirements/add-note.service.js')
const AgreementsExceptionService = require('../../app/services/return-requirements/agreements-exceptions.service.js')
const CheckYourAnswersService = require('../../app/services/return-requirements/check-your-answers.service.js')
const DeleteNoteService = require('../../app/services/return-requirements/delete-note.service.js')
const FrequencyCollectedService = require('../../app/services/return-requirements/frequency-collected.service.js')
const FrequencyReportedService = require('../../app/services/return-requirements/frequency-reported.service.js')
const NoReturnsRequiredService = require('../../app/services/return-requirements/no-returns-required.service.js')
Expand All @@ -26,6 +27,7 @@ const StartDateService = require('../../app/services/return-requirements/start-d

// For running our service
const { init } = require('../../app/server.js')
const sessionId = '64924759-8142-4a08-9d1e-1e902cd9d316'

describe('Return requirements controller', () => {
let server
Expand Down Expand Up @@ -127,6 +129,21 @@ describe('Return requirements controller', () => {
})
})

describe('GET /return-requirements/{sessionId}/delete-note', () => {
beforeEach(async () => {
Sinon.stub(DeleteNoteService, 'go').resolves({
title: 'Removed',
text: 'Note removed'
})
})
it('redirects on success', async () => {
Demwunz marked this conversation as resolved.
Show resolved Hide resolved
const result = await server.inject(_options('delete-note'))

expect(result.statusCode).to.equal(302)
expect(result.headers.location).to.equal(`/system/return-requirements/${sessionId}/check-your-answers`)
})
})

describe('GET /return-requirements/{sessionId}/existing', () => {
describe('when the request succeeds', () => {
it('returns the page successfully', async () => {
Expand Down Expand Up @@ -306,7 +323,7 @@ describe('Return requirements controller', () => {
function _options (path) {
return {
method: 'GET',
url: `/return-requirements/64924759-8142-4a08-9d1e-1e902cd9d316/${path}`,
url: `/return-requirements/${sessionId}/${path}`,
auth: {
strategy: 'session',
credentials: { scope: ['billing'] }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('Check Your Answers presenter', () => {
journey: 'no-returns-required',
note: {
content: 'Note attached to requirement',
status: 'Added',
userEmail: 'carol.shaw@atari.com'
},
reason: 'returns-exception',
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever we use Sinon we need to add a restore() at the tope level in an afterEach() block.

  afterEach(() => {
    Sinon.restore()
  })

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// Test framework dependencies
const Lab = require('@hapi/lab')
const Code = require('@hapi/code')
const Sinon = require('sinon')

const { describe, it, beforeEach } = exports.lab = Lab.script()
const { expect } = Code
Expand Down Expand Up @@ -37,23 +38,25 @@ const sessionData = {

describe('Check Your Answers service', () => {
let session
let yarStub

beforeEach(async () => {
await DatabaseSupport.clean()
session = await SessionHelper.add({
...sessionData
})
yarStub = { flash: Sinon.stub().returns([]) }
})

describe('when called', () => {
it('fetches the current setup session record', async () => {
const result = await CheckYourAnswersService.go(session.id)
const result = await CheckYourAnswersService.go(session.id, yarStub)

expect(result.id).to.equal(session.id)
})

it('returns page data for the view', async () => {
const result = await CheckYourAnswersService.go(session.id)
const result = await CheckYourAnswersService.go(session.id, yarStub)

expect(result).to.equal({
activeNavBar: 'search',
Expand All @@ -62,14 +65,15 @@ describe('Check Your Answers service', () => {
journey: 'no-returns-required',
licenceRef: '01/ABC',
note: 'Note attached to requirement',
notification: undefined,
userEmail: 'carol.shaw@atari.com',
reason: 'abstraction-below-100-cubic-metres-per-day',
startDate: '8 February 2023'
}, { skip: ['id'] })
})

it('updates the session record to indicate user has visited check-your-answers', async () => {
await CheckYourAnswersService.go(session.id)
await CheckYourAnswersService.go(session.id, yarStub)
const updatedSession = await SessionModel.query().findById(session.id)

expect(updatedSession.data.checkYourAnswersVisited).to.be.true()
Expand Down
64 changes: 64 additions & 0 deletions test/services/return-requirements/delete-note.service.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// write test in hapi/yar for delete-note.service.js
Demwunz marked this conversation as resolved.
Show resolved Hide resolved
// Test framework dependencies
const Lab = require('@hapi/lab')
const Code = require('@hapi/code')
const Sinon = require('sinon')
const { describe, it, beforeEach } = exports.lab = Lab.script()
Demwunz marked this conversation as resolved.
Show resolved Hide resolved
const { expect } = Code

// Test helpers
const DatabaseSupport = require('../../support/database.js')
const SessionHelper = require('../../support/helpers/session.helper.js')

// Thing under test
const DeleteNoteService = require('../../../app/services/return-requirements/delete-note.service.js')

const sessionData = {
data: {
id: 'f1288f6c-8503-4dc1-b114-75c408a14bd0',
checkYourAnswersVisited: true,
licence: {
endDate: null,
licenceRef: '01/ABC',
licenceHolder: 'Astro Boy',
currentVersionStartDate: '2023-02-08T00:00:00.000Z'
},
reason: 'abstraction-below-100-cubic-metres-per-day',
journey: 'no-returns-required',
note: {
content: 'Note attached to requirement',
userEmail: 'carol.shaw@atari.com'
},
startDateOptions: 'licenceStartDate'
}
}
describe('Delete Note service', () => {
let session
let yarStub
Demwunz marked this conversation as resolved.
Show resolved Hide resolved

beforeEach(async () => {
await DatabaseSupport.clean()

session = await SessionHelper.add({
...sessionData
})

yarStub = {
flash: Sinon.stub()
}
})

it('deletes the note', async () => {
await DeleteNoteService.go(session.id, yarStub)
expect(session.note).to.be.undefined()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, a NIT, adding some whitespace. But also this test is incorrect.

It is currently passing because we never set the property session.note but do set the property session.data.note. If you update the test to check for that it will fail because session.data.note is defined.

You need to do this to fix the test and properly check the note has been deleted.

Suggested change
await DeleteNoteService.go(session.id, yarStub)
expect(session.note).to.be.undefined()
await DeleteNoteService.go(session.id, yarStub)
const refreshedSession = await session.$query()
expect(refreshedSession.data.note).to.be.undefined()

However, this still won't pass unless the DeleteNoteService is updated to drop the note property instead of setting it to an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, updated

})

it("sets the notification message to 'Removed'", async () => {
await DeleteNoteService.go(session.id, yarStub)

const [flashType, notification] = yarStub.flash.args[0]

expect(flashType).to.equal('notification')
expect(notification).to.equal({ title: 'Removed', text: 'Note removed' })
})
})
Loading
Loading