Housekeeping of return requirement routes #647
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4261 This is just a bit of housekeeping of the return requirements setup routes before things get too baked in. Key thing is the ordering of things in the files; we have a convention of doing things alphabetically so they could do with being tidied up. We've also spotted that some routes have a `returns-` prefix and others don't. So, this will make them consistent. Finally, this change proposes renaming the controller handlers that deal with the POST requests to be prefixed with `submit` rather than `save`. Our reasoning is they are handling the form _submission_, and should there be a validation issue no save will take place.
This is the only one of the pages that has retained the adjective which is inconsistent.
Went with the plural because it is consistent with `/no-returns-required`.
This remove a lot of the duplication in the unit tests and is something we have done in other places where we need to create an object where only one or two things change between tests.
We expect to persist the return requirement and delete the session when the 'Approved' button is clicked in the check your answers page. So, we won't have a session record to fetch and use. But we will need to know which licence we approved the return requirement for so we can set the link needed in the page. This updates the route to include that and adds the link to the view so we have the context of why we need it.
We've just used the titles in the views returned as the basis for each description.
Also, as best as we can now ensure the redirects are going to the right places. We also checked the back links to make sure they were going where expected. There are still things to be sorted out - only one start page but 2 journeys - 2 check your answer pages when we should be able to work with one (would fix `/add-note` journey!)
Will at least mean we can get back to the right place for now. In future we'll make a single `/check-your-answers` and which will mean back will be dynamic depending on the journey type.
Cruikshanks
added
the
housekeeping
Adding missing govuk backlink partial
| {% from "govuk/components/button/macro.njk" import govukButton %} | ||
|
|
||
| {% set title = "Check your answers" %} | ||
| {% set rootLink = "/system/return-requirements/" + id %} |
There was a problem hiding this comment.
this page did not have a back button on it in the story and shabbir questioned it so I removed it. I think the point being you should be using the change links on the page to go back to anything you want to change? https://eaflood.atlassian.net/browse/WATER-4255
There was a problem hiding this comment.
Okey-dokey. That's valid and I don't see them in the prototype. I'll whip them out then merge this in when I get back from school run!
robertparkinson
previously approved these changes
Jan 10, 2024
Was reminded in feedback that QA does not expect to see these in the page. Double checked the prototype and confirmed that is the case.
Cruikshanks
dismissed stale reviews from robertparkinson and Demwunz
via
d296fff
Cruikshanks
added a commit
that referenced
this pull request
Jan 10, 2024
https://eaflood.atlassian.net/browse/WATER-4261 Whilst working on some [housekeeping](#647) and [tweaks](#648) we spotted that the titles for all the new returns requirements set up pages were missing. A quick check identified that all the pages are using our standard `layout.njk` (😁) but it expects a `pageTitle` value to be passed in the context and none of the pages are doing this (😱). This changes fixes the issue and adds the missing titles.
Cruikshanks
added a commit
that referenced
this pull request
Jan 11, 2024
https://eaflood.atlassian.net/browse/WATER-4261 Whilst working on some [housekeeping](#647) and [tweaks](#648) we spotted that the titles for all the new returns requirements set up pages were missing. A quick check identified that all the pages are using our standard `layout.njk` (😁) but it expects a `pageTitle` value to be passed in the context and none of the pages are doing this (😱). This change fixes the issue and adds the missing titles. We also ended up doing a bit more housekeeping. - remove the duplicate `/no-returns-check-your-answers` - fix approve button on `check-your-answers` not redirecting properly - remove Nunjucks `set title=` and just use `{{ pageTitle }}` in all templates - fix 'Return to licence setup' link on approve page --------- Co-authored-by: Robert Parkinson <robertparkinson@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4261
This is some housekeeping of the return requirements setup routes before things get too baked in.
Primarily it started as sorting the order of the handlers, routes and tests to be alphabetical. However, once we dug in we spotted several other things that needed sorting.
POSTendpointsPOSThandlersPOSThandlersPOSThandlers fromsavetosubmit(reason being they are handling the form submission, and if there is a validation issue no save will take place)/approvepageoptionsSo, it's a bigger change than we'd normally do. But a number are pretty intrinsically linked so we're bundling them into this single housekeeping change.