Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Returns required journey - Select start date page iteration 2 (1 of 7) (
#646) * Returns required journey - Select start date page iteration 2 (1 of 7) https://eaflood.atlassian.net/browse/WATER-4266 This page will contain date controls for a start date for the return requirement. The page will have specific validation rules for the date which adhere to business requiurements. This is step 1 of a 7 step journey through return requirements. * feat: added start date view * fix: removed data dump * fix: put correct startdate value into view * chore: added fields for enddate * chore: added end date logic * chore: refactored form validation * chore: allow users to see correct values * fix: fixed back button and field values * fix: remove only test command * fix: updated failing test * fix: updated failing tests * fix: fixed controller test * chore: simplify field values * refactor(app): WATER-4266 integrated end date amend Reused the endDate from PR #681 which removes calculating the date from this branch. Also refactored adding an error class so that all error types are captured correctly. * chore: fix sonarcloud warnings * chore: fix more sonarcloud warnings * chore: removed rogue empty line * Update app/controllers/return-requirements.controller.js Co-authored-by: Alan Cruikshanks <[email protected]> * Update app/presenters/return-requirements/start-date.presenter.js Co-authored-by: Alan Cruikshanks <[email protected]> * Update app/services/return-requirements/start-date.service.js Co-authored-by: Alan Cruikshanks <[email protected]> * Update app/services/return-requirements/start-date.service.js Co-authored-by: Alan Cruikshanks <[email protected]> * Update app/presenters/return-requirements/start-date.presenter.js Co-authored-by: Alan Cruikshanks <[email protected]> * Update app/validators/return-requirements/start-date.validator.js Co-authored-by: Alan Cruikshanks <[email protected]> * Thoughts and opinions - @Cruikshanks contributions Having deep dived on what we have so far we've in the main managed to solve the problem of validating a date both generically (is this date valid) and from a business perspective (is this date acceptable). There is an issue of handling no selection being made (it looks like this got broken with later changes). But IMHO I think there are also some simplifications and optimisations that could be made. In the subsequent changes I'll try and make the change and explain why I'm doing it. I'll deal with all the tests I break at the end! * Move submit logic to dedicated service This deals with 2 problems I foresaw. Optimisation The first was the current logic in the controller depended on calling the `StartDateService` twice in the event of a validation error. From what I could see this was purely to access the presenter to generate the page data with the error and payload included. But it meant a second query request was going out to get the 'session', which we already had. Simplification Our convention of testing controller's via 'controller' tests (integration in other words) is intended. The more complex the controller, the more 'controller' tests we'll have to add that will be slower and more complex to setup. To get fewer, easier to write 'controller' tests we need to make to our controllers as 'dumb' as possible. This is why we follow a pattern of doing all the work in a 'service' and I felt this controller would benefit from one. But rather than reuse the `StartDateService` if we created a `SubmitStartDateService` it would allow us to also handle our optimisation issue. --- We've not made any changes to how the thing is validated, the formatting of the data or the view template in this change. We've just moved what was in the controller to a new service. * Simplify StartDateService Because the service is no longer trying to do two things we can simplify it. * Consistency - update quotes in view template To stop anyone second guessing about what is the 'right' way to do something we try and focus on being as consistent as possible. We haven't been, and views being a recent addition to the project is where we're most at fault. But trying to be an exemplar this updates the quotes usage in the template to match what StandardJS would shout at us about in our real code; single quotes as the default, doubles when we need them. We also fix some inconsistent indentation. * Separation & Simplification - move items to view Separation Again, we have the excuse that we are still working out our conventions for where things sit when it comes to building pages. We know we want as little logic in the views as possible. Nothing beyond some looping, string concatenation and if/else should be in the view. But what is emerging is that anything to do with the structure of the page should sit in the view. For example, we have a page with a table. We could have the presenter combine both the layout, data and element options in one go. But that mixes concerns; the presenter is now responsible for both the data and the layout. So, if we can we should leave layout and structure duties to the view and data handling to the code/presenters. Simplification One benefit of this is it has made our presenter much simpler. There is some increase in complexity in the view. But we think this is nullified by most of the changes just being structural. We have only added one new conditional to the view, but removed a great many from the presenter. Admittedly, some of this simplification comes from the fact we are no longer trying to style individual elements. But we still think separating the concerns has made things clearer in both the view and the code. * Housekeeping - remove unused radio button option * Housekeeping - put form inside body div Plus some whitespacing * Simplification - use static value for start date We don't need the licence version start date value to be passed into the form and then returned to us. Whether it is the GET or the POST we'll have this information in the session object. So, we can simplify and be consistent by just using a static, set value. * Fix - broken year styling Realised our simplification of the setting the classes had overlooked that the year field needs a width-4 rather than width-2. * Simplify - remove formatting of start and end date Now we are not passing the date through to the view for use in the form. So, that is one less reason to format it. Secondly, the Joi date validators accept a string. So, what is in the session can be passed straight through. So, rather than having the presenter format the dates, we can pass them straight through to the validator from the session object. It also removes the condition of checking for an error and calling the presenter for a second time. Now we can just call it once. * Simplification - remove need for validation object * Convention & consistency - use existing function Mainly to be consistent we replace the direct use of `pad()` with our own helper function. We also rename the function. Our convention generally sways to a 'ruby' style. We actively avoid prefixing anything with `get` and `set`. But also where a function is returning a named value, we err on the side of naming the function after the named value. The fact we're calling a function is an indication we're doing something more than popping it off an object. But we make our lives a little easier by not stressing what to name that 'something'! * Simplification - remove date field validation Being able to highlight a specific field being incorrect I believe is a 'nice to have'. To do so is going to add lots more complexity to the code and open a series of other scenarios we have to cater for. Which do we highlight if someone enters 'Z 13 [null]'? Do we highlight all 3 or just the first one? Add to that the guidance in the design system is clear that if highlighting specific fields the error message needs to include this information. > If you're highlighting just one field - either the day, month or year - only style the field that has an error. The error message must say which field has an error, [..] The ACs for the story only require us to check the date is valid and meets certain business rules. Where the date is invalid we have been given only one message to display, 'Enter a real start date'. So, as we are not providing distinct error messages and we want to make maintenance of the service as simple as possible we remove all the functionality related to checking individual date fields. * Fix - getting Joi date format error With the last change we made we are now passing in an invalid date format to Joi to validate. Because it wasn't covered when we enter a partial date or one including invalid values we see `"fullDate" must be in ISO 8601 date format`. Adding this message gets it displaying `Enter a real start date` again. * Simplification - remove custom messages We had custom messages outside of the validation because it was needed where we were creating our own custom Joi error. Now we are not we can simplify things by just putting the messages directly into the schema. * Fix - getting joi startDate required error Moving this message means we now see `Select the start date for the return requirement` rather than `"startDate" is required` when no option is selected. * Simplification - use schema directly Rather then create the schema object from a function and then call `validate()` on it we can convert our `_createSchema()` function into the validator. > This takes a different approach to what I originally submitted where I had broken the schema up. In retrospect unless the schema got really crazy it feels cleaner to have a single schema that we use rather than 2 separate ones. * Documentation - add JSDoc for the validator As a minimum we include a JSDoc comment for all our public methods. This can be a great help when using a tool like VSCode as it will display the comment in the ID when you hover over references to it elsewhere in the code. It also allows us to give some context to why the function exists. We have more guidance about this in https://github.com/DEFRA/water-abstraction-team/blob/main/coding_conventions.md#jsdoc-comments * Fix - select anotherStartDate on nothing selected This change fixes one of the original issues found when reviewing the change. If no option was selected then the page would reload with 'Another date' automatically selected. By including the selected options as properties in our page data we can more reliably tell the input controls which radio button to select, or none at all. We did have to go back on a previous decision though. We wanted to keep the schema as a single thing. However, we found we could not get the different error states to fire properly. Splitting the schema and only validating against each got the screen behaving as expected. * Convention - order functions alphabetically * Fix - ensure form is highlighted on nothing In previous changes we solved the issue of the 'Another date' radio button being selected when nothing was selected and just a Joi error message being shown. The final piece of the puzzle is to ensure that the whole control gets highlighted when no option is selected which this change does. * Housekeeping - add documentation and tidy * Fix - using wrong start date for validation The acceptance criteria in the ticket is > I enter a date that is earlier than the original licence effective (start date) date The original licence effective date is the start date recorded against the licence and not what is against the current licence version record. When you search for a licence it is the **Effective from** date you see on the Summary tab. We were not capturing that in the session. So, this change updates the `InitiateReturnRequirementService` to fetch and set it against the session as `startDate`. We move the licence version start date to `currentVersionStartDate` and update the presenter and the view. * Fix - broken journey We'd overwritten the logic that was in the controller used to determine which page to go to next. Start date is the first page you go to for both journeys. But once submitted we have to redirect to different pages depending on whether the user clicked 'Set up new returns requirements' or 'No returns required'. * Fix InitiateReturnRequirementSessionService tests * Fix StartDateService tests * Fix StartDatePresenter tests * Fix - date validation using joi-date extension When fixing the tests for the `StartDateValidator` we wrote one to confirm that a this payload would cause a validation error. ```javascript { 'start-date-options': 'anotherStartDate', 'start-date-day': '29', 'start-date-month': '2', 'start-date-year': '2023' } ``` There was no leap year in 2023 so this is not a real date. But the test failed; Joi was letting it through. When we checked the value it was generating was '2023-03-01'. This then triggered a memory of an issue we'd had with [date validation on the sroc-charging-module-api](DEFRA/sroc-charging-module-api#170). That was more about Joi mixing up US and UK date formats but it reminded us about this comment in the Joi docs > If the validation convert option is on (enabled by default), a string or number will be converted to a Date if specified. Note that some invalid date strings will be accepted if they can be adjusted to valid dates (e.g. '2/31/2019' will be converted to '3/3/2019') by the internal JS Date.parse() implementation. > > https://joi.dev/api/?v=17.12.0#date This explains exactly what we were seeing in the test. JavaScript's [Date.parse()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse) is 'helpfully' converting it to a valid date. We clearly don't want this to happen. So, we've come to the same conclusion. The only way to prevent this is to add the [joi-date extension](https://joi.dev/module/joi-date/) and be explicit about the format we are validating. * Fix StartDateValidator tests * Convention - add missing test comments * Add tests for SubmitStartDateService --------- Co-authored-by: Alan Cruikshanks <[email protected]>
- Loading branch information
1 parent
959d7de
commit f6942ee