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

Conversation

Demwunz
Copy link
Contributor

@Demwunz Demwunz commented Apr 18, 2024

https://eaflood.atlassian.net/browse/WATER-4291

This feature allows the user to see notifications on the page when a note is added, changed or deleted.

A delete-note service has been added which is accessed via a simple GET request. This will delete any note in the session attached to the requirement.

Notifications are shown using hapi/yar and are immediately removed upon first view on the page.

Screenshots image image image

https://eaflood.atlassian.net/browse/WATER-4291

This feature will allow the user to see notifications on the page when a note is added, changed or deleted.
@Demwunz Demwunz added the enhancement New feature or request label Apr 18, 2024
@Demwunz Demwunz self-assigned this Apr 18, 2024
Demwunz and others added 3 commits April 23, 2024 13:33
@Demwunz Demwunz force-pushed the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch from 6954abd to 4bacf6f Compare April 24, 2024 13:36
@Demwunz Demwunz force-pushed the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch from 9bafb5c to 04314cf Compare April 25, 2024 12:11
@Demwunz Demwunz force-pushed the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch from ab94f1f to b53b887 Compare April 25, 2024 16:39
@Demwunz Demwunz force-pushed the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch 2 times, most recently from 5b51a6a to 9228ff3 Compare May 1, 2024 16:09
@Demwunz Demwunz force-pushed the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch from 9228ff3 to 8fb7589 Compare May 1, 2024 16:12
…ur-answers-page-with-a-cs-for-notes-add-change-delete
@Demwunz Demwunz marked this pull request as ready for review May 1, 2024 16:18
@Demwunz Demwunz requested a review from Cruikshanks May 1, 2024 16:19
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

A few changes needed and then this will be ship-shape and bristol fashion!

app/services/return-requirements/delete-note.service.js Outdated Show resolved Hide resolved
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()
  })

Comment on lines 52 to 53
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

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

@Demwunz Demwunz requested a review from Cruikshanks May 2, 2024 08:24
Demwunz and others added 4 commits May 2, 2024 09:47
…ur-answers-page-with-a-cs-for-notes-add-change-delete
…ur-answers-page-with-a-cs-for-notes-add-change-delete
…ur-answers-page-with-a-cs-for-notes-add-change-delete
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

@Demwunz Demwunz requested a review from Cruikshanks May 2, 2024 18:48
…ur-answers-page-with-a-cs-for-notes-add-change-delete
…ur-answers-page-with-a-cs-for-notes-add-change-delete
@Demwunz Demwunz merged commit 8050ccb into main May 3, 2024
6 checks passed
@Demwunz Demwunz deleted the WATER-4291-returns-required-journey-check-your-answers-page-with-a-cs-for-notes-add-change-delete branch May 3, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants