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

Persist and retrieve rtn-req set up session values #814

Merged

Conversation

Demwunz
Copy link
Contributor

@Demwunz Demwunz commented Mar 13, 2024

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

This PR will allow the application to persist data through the 'return-requirements' journey.

The data will persist in the session until it is cleared by another part of the application.

The data will be used to populate controls and fields so that the user can edit or review, and also for UI updates.

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

This PR will allow the application to persist data through the 'return-requirements' journey.

The data will persist in the session until it is cleared by another part of the application.

The data will be used to populate controls and fields so that the user can edit or review, and also for UI updates.
@Demwunz Demwunz self-assigned this Mar 13, 2024
@Demwunz Demwunz added the enhancement New feature or request label Mar 13, 2024
@Demwunz Demwunz force-pushed the WATER-4384-persist-and-retrieve-rtn-req-set-up-session-values branch from 2c8d44a to ced8c2f Compare March 22, 2024 16:51
@Demwunz Demwunz force-pushed the WATER-4384-persist-and-retrieve-rtn-req-set-up-session-values branch from 19f9014 to 2d238fc Compare March 22, 2024 17:10
@Demwunz Demwunz marked this pull request as ready for review March 26, 2024 17:25
@Demwunz
Copy link
Contributor Author

Demwunz commented Mar 26, 2024

This PR also includes minor amends to pages that were worked on in this PR, namely changing UI control names from snake_case to kebab-case.

Also where appropriate, the tests are also split into the respective journeys, either 'no-returns-required' or 'returns-required'. This is because the same view/presenter/service is used, but the output is different when presented on the "Check your Answers" page or the next page in the respective journey.

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.

Cleary, I've just got a head full of NITs!

Awesome work. Ping me if any issues or when updated and I'll jump straight back on and ✅ !

@@ -2,7 +2,7 @@

/**
* Formats data for the `/return-requirements/{sessionId}/start-date` page
* @module StartDatedPresenter
* @module StartDatePresenter
Copy link
Member

Choose a reason for hiding this comment

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

🔍 👍

}
})
})

describe('when called', () => {
describe('with a valid payload', () => {
describe('with a valid payload for currentStartDate', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Did we mean currentStartDate? Or should this be licenceStartDate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, I changed the test but forgot to update that to licenceStartDate

@@ -47,29 +69,21 @@ describe('Submit Start Date service', () => {
}
})

it('fetches the current setup session record', async () => {
const result = await SubmitStartDateService.go(session.id, payload)
it('saves the submitted value', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

NIT

Suggested change
it('saves the submitted value', async () => {
it('saves the submitted values', async () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽

}
})

it('saves the submitted value', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

NIT

Suggested change
it('saves the submitted value', async () => {
it('saves the selected option', async () => {

Copy link
Contributor Author

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 March 28, 2024 14:14
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.

Having properly clocked the context where journey rather than view was being used ( 🤦 ) I think the only thing I have left is about splitting the check your answers tests at the top level; we can get the same result without splitting things at the very top level for the presenter.

Copy link
Member

Choose a reason for hiding this comment

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

I think this comment still applies here

I 💯 agree it's going to make things easier to manage if we test 'check your answers' separately depending on the journey. But I believe we can do this with the same tools and not break a convention. For example

describe('Check Your Answers presenter', () => {
  describe("when the 'no returns required' journey was selected", () => {
    // ...
  })

  describe("when the 'returns required' journey was selected", () => {
    // ...
  })
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended this

@Demwunz Demwunz force-pushed the WATER-4384-persist-and-retrieve-rtn-req-set-up-session-values branch from 6d5bdd5 to 52d4c20 Compare April 3, 2024 12:17
@Demwunz Demwunz requested a review from Cruikshanks April 3, 2024 13:27
@@ -12,17 +12,17 @@ const CheckYourAnswersPresenter = require('../../../app/presenters/return-requir

describe('Check Your Answers presenter - No Returns Required', () => {
let session

Copy link
Member

Choose a reason for hiding this comment

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

This is whitespace we would normally leave in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

restored the whitespace

Comment on lines 34 to 35
await _save(session, payload)
return {
Copy link
Member

Choose a reason for hiding this comment

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

This one still doesn't look to be resolved.

@Cruikshanks
Copy link
Member

Happy to ignore the SonarCloud duplication issue for now. Once we have the journeys complete we can take a more informed view as to what refactoring needs doing.

@Demwunz Demwunz requested a review from Cruikshanks April 4, 2024 08:01
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 merged commit ab8b439 into main Apr 4, 2024
5 of 6 checks passed
@Demwunz Demwunz deleted the WATER-4384-persist-and-retrieve-rtn-req-set-up-session-values branch April 4, 2024 09:39
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.

3 participants