Move licence end date logic to model #681
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-4325 https://eaflood.atlassian.net/browse/WATER-4263 It just so happens we have had two tickets come through that both need to determine if and when a licence 'ends'. A licence can 'end' for 3 reasons: - because it is _revoked_ - because it is _lapsed_ - because it is _expired_ The previous delivery team chose to encode these as 3 separate date fields on the licence record. So, if a field is populated it means the licence 'ends' for that reason on that day. But it's not that simple! 😁 More than one of these fields may be populated. For example, a licence was due to expire on 2023-08-10 but was then revoked on 2022-04-27. So, to determine the reason you need to select the _earliest_ date. But there's more! 😲 We have examples where 2 of the fields might be populated with the same date (and 1 licence where all 3 have the same date!) So, how do you determine the 'end' reason then? If more than one date field is populated and they hold the earliest date value then we select based on priority; revoked -> lapsed -> expired. Both tickets are connected to PR's that have to handle all or some of this logic. - [Returns required journey - Select start date page iteration 2 (1 of 7)](#646) - [Add warning text to view licence page](#670) Both PR's will also have to write numerous tests to check the right date is being used, and we have a strong suspicion we are already doing something similar in supplementary billing and that we'll need the 'end' in future tickets. So, enough is enough! This refactoring change moves the logic to the model. Specifically, as a [custom Objection.js instance method](https://vincit.github.io/objection.js/api/model/instance-methods.html). This is something we've done in a [previous project](https://github.com/DEFRA/sroc-charging-module-api) and in this project to determine a `ContactModel`s name. We can then remove the duplication both in logic and unit tests. This change updates the model, adds the new tests, then refactors existing usages to use the model's `$ends()` method.
The end date should now be available in the session when retrieved as part of the journey.
We've just tweaked the tests at this point to confirm they get the same result following the change.
We don't need tests for all the different scenarios because they are now handled by the model tests. But we also add coverage for end dates in the future as these should not generate a warning message.
Cruikshanks
added
the
housekeeping
Cruikshanks
requested review from
robertparkinson,
Demwunz,
Jozzey and
Beckyrose200
|
Ignore SonarCloud! Fake news, fake news! (I'm dealing with it 😩 😬 ) |
robertparkinson
requested changes
Jan 23, 2024
app/models/licence.model.js
Outdated
| } | ||
|
|
||
| filteredDates.sort((firstDate, secondDate) => { | ||
| if (firstDate.date !== secondDate.date) { |
There was a problem hiding this comment.
weird that this is working for you. when I implemented it I found when doing !== or === you need to use date.getTime() as can be explained here: https://stackoverflow.com/questions/492994/compare-two-dates-with-javascript
Or perhaps it is failing and just defaulting to the priority check below? which is still giving you the same result but possibly not for the reason you think? 🤔
There was a problem hiding this comment.
Spot on insight! Should have known not to trust JavaScript 
I've implemented your suggestion, with a couple of commits along the way to confirm how I managed to get away with it!
@robertparkinson has correctly flagged that using `!==` to compare dates does not work as expected. Here is an example of what you actually get ```javascript const firstDate = new Date('2023-02-01') const secondDate = new Date('2023-03-01') const thirdDate = new Date('2023-03-01') console.log(firstDate !== secondDate) // true console.log(secondDate !== thirdDate) // true (!!) console.log(firstDate < secondDate) // true console.log(secondDate < thirdDate) // false ``` The current logic expects `secondDate !== thirdDate` to be false but clearly that is not the case. Our tests are passing though because - they weren't passing in all the dates (Doh!) - because of how we initialised the `endDates` array revoked was always coming top because nothing was really being resorted So, we've fixed the test and re-jigged the logic. Pushing this to confirm that the test will fail now before implementing the suggested fix.
Thanks again to @robertparkinson for pointing us to the explanation and the suggested fix. This is in draft because we have retained the console.log() calls and we intend to switch back how `endDates` is instantiated (nothing wrong with having it 'just work' as a fall back!)
On discussing what was needed to help the return requirements setup journey it was agreed we don't care about the reason, we just need the date value. So, rather than adding the result as is from `myLicence.$ends()` instead we should just add the resulting date to the session.
Demwunz
reviewed
Jan 23, 2024
|
|
||
| return { | ||
| licence: { | ||
| id, | ||
| endDate: ends ? ends.date : null, |
robertparkinson
approved these changes
Jan 23, 2024
Demwunz
added a commit
that referenced
this pull request
Jan 23, 2024
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.
Cruikshanks
added a commit
that referenced
this pull request
Jan 23, 2024
https://eaflood.atlassian.net/browse/WATER-4188 https://eaflood.atlassian.net/browse/WATER-4328 Much like in [Move licence end date logic to model](#681) we have another two tickets that need to identify the licence holder for a licence. We've already figured out how to do that in [Add licence ref. and holder to rtn. req. session](#639). But that implementation was specific to return requirements and unlike `myContact.$name()` and `myLicence.$ends()` to determine the licence holder you need to have pulled through a series of related models. So, for the same reasons we want to move this piece of logic to the model to make it easier to reuse. However, their will be differences in implementation. This change adds the logic to the model and then refactors `InitiateReturnRequirementSessionService` to demonstrate how to use it.
Cruikshanks
added a commit
that referenced
this pull request
Jan 24, 2024
https://eaflood.atlassian.net/browse/WATER-4188 https://eaflood.atlassian.net/browse/WATER-4328 Much like in [Move licence end date logic to model](#681) we have another two tickets that need to identify the licence holder for a licence. We've already figured out how to do that in [Add licence ref. and holder to rtn. req. session](#639). But that implementation was specific to return requirements and unlike `myContact.$name()` and `myLicence.$ends()` to determine the licence holder you need to have pulled through a series of related models. So, for the same reasons we want to move this piece of logic to the model to make it easier to reuse. However, there will be differences in implementation. This change adds the logic to the model and then refactors `InitiateReturnRequirementSessionService` to demonstrate how to use it.
robertparkinson
pushed a commit
that referenced
this pull request
Jan 24, 2024
https://eaflood.atlassian.net/browse/WATER-4325 https://eaflood.atlassian.net/browse/WATER-4263 We have had two tickets come through that both need to determine if and when a licence 'ends'. A licence can 'end' for 3 reasons: - because it is _revoked_ - because it is _lapsed_ - because it is _expired_ The previous delivery team chose to encode these as 3 separate date fields on the licence record. So, if a field is populated it means the licence 'ends' for that reason on that day. But it's not that simple! 😁 More than one of these fields may be populated. For example, a licence was due to expire on 2023-08-10 but was then revoked on 2022-04-27. So, to determine the reason you need to select the _earliest_ date. But there's more! 😲 We have examples where 2 of the fields might be populated with the same date (and 1 licence where all 3 have the same date!) So, how do you determine the 'end' reason then? If more than one date field is populated and they hold the earliest date value then we select based on priority; revoked -> lapsed -> expired. Both tickets are connected to PRs that have to handle all or some of this logic. - [Returns required journey - Select start date page iteration 2 (1 of 7)](#646) - [Add warning text to view licence page](#670) Both PRs will also have to write numerous tests to check the right date is being used, and we have a strong suspicion we are already doing something similar in supplementary billing and that we'll need the 'end' in future tickets. So, enough is enough! This refactoring change moves the logic to the model, specifically, as a [custom Objection.js instance method](https://vincit.github.io/objection.js/api/model/instance-methods.html). This is something we've done in a [previous project](https://github.com/DEFRA/sroc-charging-module-api) and in this project to determine a `ContactModel`s name. We can then remove the duplication both in logic and unit tests. This change updates the model, adds the new tests, and then refactors existing usages to use the model's `$ends()` method.
robertparkinson
pushed a commit
that referenced
this pull request
Jan 24, 2024
https://eaflood.atlassian.net/browse/WATER-4188 https://eaflood.atlassian.net/browse/WATER-4328 Much like in [Move licence end date logic to model](#681) we have another two tickets that need to identify the licence holder for a licence. We've already figured out how to do that in [Add licence ref. and holder to rtn. req. session](#639). But that implementation was specific to return requirements and unlike `myContact.$name()` and `myLicence.$ends()` to determine the licence holder you need to have pulled through a series of related models. So, for the same reasons we want to move this piece of logic to the model to make it easier to reuse. However, there will be differences in implementation. This change adds the logic to the model and then refactors `InitiateReturnRequirementSessionService` to demonstrate how to use it.
Demwunz
added a commit
that referenced
this pull request
Jan 29, 2024
#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]>
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-4325
https://eaflood.atlassian.net/browse/WATER-4263
We have had two tickets come through that both need to determine if and when a licence 'ends'.
A licence can 'end' for 3 reasons:
The previous delivery team chose to encode these as 3 separate date fields on the licence record. So, if a field is populated it means the licence 'ends' for that reason on that day. But it's not that simple! 😁
More than one of these fields may be populated. For example, a licence was due to expire on 2023-08-10 but was then revoked on 2022-04-27. So, to determine the reason you need to select the earliest date.
But there's more! 😲
We have examples where 2 of the fields might be populated with the same date (and 1 licence where all 3 have the same date!) So, how do you determine the 'end' reason then? If more than one date field is populated and they hold the earliest date value then we select based on priority; revoked -> lapsed -> expired.
Both tickets are connected to PRs that have to handle all or some of this logic.
Both PRs will also have to write numerous tests to check the right date is being used, and we have a strong suspicion we are already doing something similar in supplementary billing and that we'll need the 'end' in future tickets.
So, enough is enough! This refactoring change moves the logic to the model, specifically, as a custom Objection.js instance method. This is something we've done in a previous project and in this project to determine a
ContactModels name.We can then remove the duplication both in logic and unit tests.
This change updates the model, adds the new tests, and then refactors existing usages to use the model's
$ends()method.