Fix missing titles in returns reqs. set up pages #651
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 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.
This is the first page that has had controls and validation added to it. As such it has adopted the pattern we expect the other placeholder pages to follow when they get updated. We were initially going to add the page title to `NoReturnsRequiredPresenter`. But then we realised the concept of presenters is to take some existing data and transform it ready for 'presenting' elsewhere. Returning a value that hasn't been generated from the data passed in breaks that concept. So, we're also tweaking things a little here. We add the title directly to what the service returns. We also move `activeNavBar:` out of the presenter because it too is a break from what how we intend to use presenters.
First, we rename `_radioButtons()` to be `_radioItems()` as that is what it is generating and the property it is setting. We then spotted that `session` wasn't being used in `NoReturnsRequiredPresenter._error()` so removed the argument. For `NoReturnsRequiredPresenter.__radioItems()` we do intend to use it in future because it will tell us which item has previously been selected. So, we don't remove it, but do indicate we are not using it by applying the 'underscore' convention.
We were about to sort out the title for the 'Check your answers page'. But then we remembered we have 2 of them so we'd need to do it in 2 places. We have no intention of maintaining 2 versions of the same page. So, rather than duplicate something again we use this opportunity to get shot of the no returns check your answers page, route and controller handlers.
Because of our work in WATER-4292 we're able to tell who the licence holder is and add that to both the page title and the title in the page!
When we updated the title for 'Check your answers' we spotted that the title was actually be set to a variable in the template and then referred to in the H1. We're not sure what benefit that adds over just putting the title directly into the H1. So, this helps us get rid of some lines of code and complexity from our views.
We broke the test because there is no session object with a licence backing the tests at the moment. In the future fetching the session and formatting the data for check your answers will be done by a service that we can then stub in the controller test. But as we're not ready for that yet (and we still haven't figured out how to stub Objection model queries 😢) we 'fudge' it and use the javascript safe navigation operator.
|
SonarCloud is right about the missing coverage. But that is because we're tinkering or adding logic that will be superseded shortly. So, we're just going to ignore it for now 😁 |
robertparkinson
requested changes
Jan 11, 2024
| @@ -19,7 +17,7 @@ | |||
| {% block content %} | |||
| {# Main heading #} | |||
| <div class="govuk-body"> | |||
| <h1 class="govuk-heading-xl govuk-!-margin-bottom-3">{{ title }}</h1> | |||
| <h1 class="govuk-heading-xl govuk-!-margin-bottom-3">Enter the abstraction period for the return requirement</h1> | |||
There was a problem hiding this comment.
if pageTitle will always match the H1 question we should just reuse the pageTitle here rather than duplicating the text
There was a problem hiding this comment.
Great minds and all that!
It was pointed out during review we'd gone to the effort of removing `set title=` in our templates and then set the text explicitly. But we are already passing in the title for each page; `pageTitle`, the whole point of this PR! Doh!
The test is checking that the correct title is present. And for this handler the title comes from the service we are stubbing.
robertparkinson
approved these changes
Jan 11, 2024
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
Whilst working on some housekeeping and tweaks 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 apageTitlevalue 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.
/no-returns-check-your-answerscheck-your-answersnot redirecting properlyset title=and just use{{ pageTitle }}in all templates