Add licence ref. and holder to rtn. req. session #639
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 In WATER-4292 we added a much of models (`LicenceDocument`, `LicenceRole` and `LicenceDocumentRole`) in preparation for identifying the current licence holder for a licence. We need the licence reference in every page of the return requirement setup journey because the page design has it as a caption alongside the page title. Rather than the number on each page it would be better to just grab the information once, and add it to the 'session' record the journey is using. But as we also need the licence holder ready for the check answers page so we may as well grab that at the same time. So, this change refactors the `LicencesController` to use a new `InitiateReturnRequirementSessionService` that will handle fetching this licence data and instantiating the initial `SessionModel` instance. Subsequent controller handlers will then be able to access this data when they fetch the session record.
We don't expect this, but just to make for a cleaner experience if someone happens to ping the service with a licence ID we cannot find we want it to redirect to our 404 page i.e. we couldn't find that thing.
Should our new server error for a reason other than not finding the licence we ad a test to cover what behaviour we expect.
This covers what the service is doing now, which is exactly what the controller was doing before we moved the code to a new service. Next, we'll start working with the licence for real.
We don't just want them as direct properties of the session's `data` property. Instead we want them grouped within a `licence:`.
Now we can fetch the matching licence from the DB and extract the licence reference. We can then test that the session we are creating contains the same details as fetched.
Based on the controller test we added towards the start we amend the service to throw a Boom not found error if fetching the licence returns no result.
We've had to make quite a jump in complexity but there is not way round it thanks to how the previous team chose to store licence details. We make use of the new Licence document, role and document-role models we recently created to get to the company and/or contact record that is identified as the licence holder. We then have to apply a bit of logic to determine which 'party' to use; the company or the contact. Contact takes precedence. We've already had to deal with the madness of displaying a contact name, so can take advantage of the existing model instance method `$name()` to sort that out for us.
In initial testing we found that there are often multiple licence document roles with the same created date. the only thing that differentiates them is the start date. So, we update our logic and tests to order by that instead of `created_at`.
Cruikshanks
added a commit
that referenced
this pull request
Jan 7, 2024
https://eaflood.atlassian.net/browse/WATER-4292 > This is part of a series of changes to add the ability to identify the licence holder for a licence Arghhhhh! 😱🤬😩 We've not long ago added 3 new models to this project - [LicenceDocumentModel](#618) - [LicenceRoleModel](#629) - [LicenceDocumentRoleModel](#630) We then [linked the LicenceModel to LicenceDocumentModel](#632). All this was based on a first pass of what we thought the [water-abstraction-ui](https://github.com/DEFRA/water-abstraction-ui), [water-abstraction-service](https://github.com/DEFRA/water-abstraction-service) and [water-abstraction-tactical-crm](https://github.com/DEFRA/water-abstraction-tactical-crm) apps were doing to determine the licence holder. It reflected what we found in the tables behind the 3 models we added as well. We'd just [coded up a service](#639) to set the return requirement setup session with the name of the licence holder and begun testing when we found an issue in testing. We came across a licence where neither the company or contact reflected what we saw in the UI for the licence holder! In the UI beneath the licence holder name is a 'View licence contact details' link. We were able to figure out that the ID it uses is for a `crm.document_headers` record. And in there we found (surprise-surprise!) another JSONB field holding a bunch of data, including the licence holder name we were seeing on screen. Another dive into the legacy code revealed we'd taken a wrong turning. When the page is requested there is logic that grabs records from the tables we created models for. But they are not a used. In fact, not even the `crm.document_headers` record is used, other than to provide an ID for the link. What seems to happen (we could be wrong again!) is that the `permit.licence` table is queried and its `licence_data_value` field is extracted. If you haven't guessed already, this is another JSON field. Only this one seems to be a complete 'dump' of everything imported from NALD for the licence. This data gets passed to `src/lib/licence-transformer/nald-transformer.js` which transforms it from the NALD structure into a 'licence'. This includes a `licenceHolderFullName` property which is eventually what appears in the UI. We have absolutely _zero_ intent to start importing and transforming raw NALD data just to get the licence holder name. Thankfully, in the follow up testing we've been doing the information held in `crm.document_headers` has always matched what we see in the UI. So, this is the start of our second attempt to crack licence holder name starting with adding the `DocumentHeader` model.
|
Do not merge this until it has been refactored to use #640 instead of the models it is currently using. |
No reason not to just return the whole session object. Plus it makes the tests simpler.
Cruikshanks
requested review from
Demwunz,
robertparkinson,
Jozzey and
Beckyrose200
robertparkinson
approved these changes
Jan 8, 2024
Cruikshanks
deleted the
add-licence-details-to-return-requirements-session
branch
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-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.
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
In WATER-4292 we added a much of models (
LicenceDocument,LicenceRoleandLicenceDocumentRole) in preparation for identifying the current licence holder for a licence. We need the licence reference on every page of the return requirement setup journey because the page design has it as a caption alongside the page title.Rather than the number on each page, it would be better to just grab the information once, and add it to the 'session' record the journey is using. But as we also need the licence holder ready for the check answers page we may as well grab that at the same time.
So, this change refactors the
LicencesControllerto use a newInitiateReturnRequirementSessionServicethat will handle fetching this licence data and instantiating the initialSessionModelinstance. Subsequent controller handlers will then be able to access this data when they fetch the session record.