Update the 'points' data solution #1352
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-4600 https://eaflood.atlassian.net/browse/WATER-4645 > Part of the work to migrate return versions from NALD to WRLS In [Add Licence Version Purpose Point model](#1288) we added our second model that represented a table in the `water` schema that hold point information. When we added `ReturnRequirementPointModel` it was to represent points linked to a return requirement. In the setup journey for new return requirements you select the points to associate from those against the licence. We were extracting this information from the `permit.licence` table until we realised that [water-abstraction-import](https://github.com/DEFRA/water-abstraction-import) does not populate it when a licence is 'dead'. We need to be able to create new return requirements even for dead licences in order to correct historic information. So, this source being defunct we made changes to the import to populate a new `water.licence_version_purpose_points` from which we could then derive a licence's points. That worked great for returns setup, so we moved to updating the view licence summary page to use the new source. It also was effected by the issue that point information was no longer available if the licence was dead. The page includes information about the water source for a licence. It was then we realised that the water source is derived from a licence's points! What we'd built for return requirements and licence version purposes was missing this information. We investigated the NALD data properly and came away with some realisations - NALD maintains a central list of 'points' (we knew this, but like addresses felt this was unnecessary) - Each point holds a lot more data than just the National Grid References for its location - Each NALD point is linked to a NALD source, and these too hold extra information There was extra information users were maintaining against each point that we were missing. This information explained why points were centrally managed and other records were linked to them. This realisation meant we needed to completely re-think how we should maintain points in WRLS going forward. So, in [the water-abstraction-service](DEFRA/water-abstraction-service#2638) we've created a migration to add new `sources` and `points` tables. We'also amended the existing `licence_version_purpose_points` and `return_requirement_points` to make it possible to link them to `points`. (Their `ngr_*` fields plus things like `description` are now defunct but to avoid breaking changes we'll need to drop them in a future change.) In [water-abstraction-import](DEFRA/water-abstraction-import#1028) we've added a new job to ensure `points` gets populated as part of the import. We've also incorporated populating and linking `licence_version_purpose_points` and `return_requirement_points` as part of the new job. With those changes in place, this change updates everything around the existing point models, as well as adding the new ones. The water sources are reference data, so as part of adding `SourceModel` we include a seeder. The existing models and their tests are updated to represent the fact they are now just `many-to-many` models. This means we make `points` a top level property of `ReturnRequirementModel` and `LicenceVersionPurposeModel`. The logic for describing a point is copied to `PointModel`. We'll delete `BasePointModel` in the next change, where we update the existing functionality to use the new structure.
Cruikshanks
added
the
housekeeping
Some fields have their NOT NULL constraint removed plus we add the new `point_id` to the tables.
We need to make the new `point_id` field available in the views. But we also drop the columns now we intend to drop in the future.
This includes its helper, plus the seeder as it will be treated as reference data and will be populated by the migration change we made to water-abstraction-service.
This includes copying the describe instance method from BasePoint as we'll no longer need `BasePointModel` in the future.
This converts the existing model to be a 'many-to-many' resource for linking requirements to points.
Same changes made to return requirement points applies here.
They break too many tests so will need to do them as part of the next phase of work.
TBH, not sure how it was working in the first place!
The logic does not appear to have anything to do with points, but the test is failing because it is expecting the recalculate bills link to be present.
Breaks even more tests not having them there!
I was hoping to separate changes at the data layer with those at the actual services and features. However, no matter which way I go it looks like I'll be breaking existing tests. I could fix them, but I'm only going to then change them again in phase 2. So, contrary to what I preach, this is going to have to be a big PR doing multiple things in order to get return requirements working again in a timely manner. 😳😬
This is pulling data from the new data structure.
THis is the final test related to the backing services we've updated to get view return version working again.
That excludes check answers because you need all 3 journeys working for that. But you can now get through the manual journey's select a point page.
The `SubmitPointsService` already handles dealing with the issue that where a single option is selected we don't get an array in the request payload. So, what the validator was doing was unnecessary and never called. And as the calling service has already formatted the points on the payload, it might as well directly pass them in rather than the whole payload. We've then updated the tests to reflect our changes but also to 'test' what the current tests claim to. In this commit the second test is failing not because it is mistakenly validating the data, but because we are getting a different error message (we had never covered the value not being a string).
Fix the invalid data test. We just needed to include the key that tells Joi to use our error message if the validation fails because the value is not a string.
We actually know we are using the point ID as our value so we should be expecting and validating that we are getting a UUID. So, the second test is failing again because we've changed the point to be the old NALD point ID which we have moved away from. As it is a string, no validation error is raised when now we expect one.
Update the validator to check specifically for UUID values and ensure the expected error message is returned if not.
At this point we know it is correctly assigning the point ID's from the selected return version's return requirements to the session. We've made the fixes necessary to confirm this in the `CheckService`. But we'll commit those when all journeys are working.
There is now just the PointModel.
The wrong feature flag was being stubbed.
Cruikshanks
added a commit
that referenced
this pull request
Sep 26, 2024
https://eaflood.atlassian.net/browse/WATER-4600 https://eaflood.atlassian.net/browse/WATER-4645 > Part of the work to migrate return versions from NALD to WRLS In [Update the 'points' data solution](#1352) we updated this project to source point information (where water is extracted) from new tables we'd created instead of extracting it from the `permit.licence` JSONB blob. At the same time we included it in our data loader, which is used by [water-abstraction-acceptance-tests](https://github.com/DEFRA/water-abstraction-acceptance-tests). But we forgot to include it in the data tear down, which is also used by the acceptance tests to avoid orphaned test data causing other issues.
This was referenced Sep 26, 2024
Cruikshanks
added a commit
that referenced
this pull request
Sep 26, 2024
https://eaflood.atlassian.net/browse/WATER-4600 https://eaflood.atlassian.net/browse/WATER-4645 > Part of the work to migrate return versions from NALD to WRLS In [Update the 'points' data solution](#1352), we updated this project to source point information (where water is extracted) from new tables we'd created instead of extracting it from the `permit.licence` JSONB blob. At the same time we included it in our data loader, which is used by [water-abstraction-acceptance-tests](https://github.com/DEFRA/water-abstraction-acceptance-tests). But we forgot to include it in the data tear down, which is also used by the acceptance tests to avoid orphaned test data causing other issues.
Cruikshanks
added a commit
that referenced
this pull request
Sep 27, 2024
https://eaflood.atlassian.net/browse/WATER-4600 https://eaflood.atlassian.net/browse/WATER-4645 We've been extending and amending the import of return versions from NALD to WRLS as part of our work to switch from NALD to WRLS to manage them. In [Update Return req set up journey to use new points](#1301) we updated the return requirement setup journey to use the new `water.licence_version_purpose_points` table we've created and now populate thanks to [a change to the import](#1301). The one other place we are referencing the `permit.licence` table to retrieve point information is the view licence summary page. This change updates its logic to grab the points data using our new [points solution](#1352): `water.licence_version_purposes` linked to `water.points` via `water.licence_version_purpose_points`.
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-4600
https://eaflood.atlassian.net/browse/WATER-4645
In Add Licence Version Purpose Point model, we added our second model, which represented a table in the
waterschema that holds point information. When we addedReturnRequirementPointModel,it was to represent points linked to a return requirement.In the setup journey for new return requirements, you select the points to associate with those against the licence. We were extracting this information from the
permit.licencetable until we realised that water-abstraction-import does not populate it when a licence is 'dead'.We need to be able to create new return requirements, even for dead licences, to correct historical information. So, this source being defunct, we made changes to the import to populate a new
water.licence_version_purpose_pointsfrom which we could then derive a licence's points.That worked great for returns setup, so we also updated the view licence summary page to use
water.licence_version_purpose_points. The issue that point information was not available if the licence was dead affected it. The page also includes information about a licence's water source. It was then that we realised that the water source is derived from a licence's points!What we'd built for return requirements and licence version purposes was missing this information.
We investigated the NALD data properly and came up with some new realisations.
There was extra information users were maintaining against each point that we were missing. This information explained why points were centrally managed and linked to other records. This realisation meant we needed to completely re-think how we should maintain points in WRLS going forward.
So, in the water-abstraction-service, we've created a migration to add new
sourcesandpointstables. We also amended the existinglicence_version_purpose_pointsandreturn_requirement_pointsto make it possible to link them topoints. (Theirngr_*fields plus things likedescriptionare now defunct, but to avoid breaking changes, we'll need to drop them in a future change.)In water-abstraction-import, we've added a new job to ensure
pointsget populated as part of the import. We've also incorporated populating and linkinglicence_version_purpose_pointsandreturn_requirement_pointsinto the new job.With those changes in place, this change updates everything around the existing point models and adds new ones.
The water sources are reference data, so we include a seeder when adding
SourceModel. We update the existing models and their tests to reflect the fact that they are now justmany-to-manymodels. This means we makepointsa top-level property ofReturnRequirementModelandLicenceVersionPurposeModel.We then go through all existing logic and update it to use the new points data structure.