Fix view licence abs amounts issue plus refactor #1132
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
Cruikshanks
added
bug
Cruikshanks
added a commit
that referenced
this pull request
Jun 21, 2024
https://eaflood.atlassian.net/browse/WATER-4322 https://eaflood.atlassian.net/browse/WATER-4442 Whilst working on [Fix view licence abs amounts issue plus refactor](#1132) we started making a change to `app/services/licences/fetch-licence-summary.service.js` to bring how it fetches the current licence to be inline with `app/services/return-requirements/setup/fetch-abstraction-data.service.js`. In both cases, the services that call these are then doing `licence.licenceVersions[0]` in order to access the 'current' one. We could ensure consistency by adding an [Objection.js modifier](https://vincit.github.io/objection.js/api/model/static-properties.html#static-modifiers) to `LicenceModel`. And then we could make it explicit we only care about the current licence (and make everyone's lives easier) if we added a custom [instance method](https://vincit.github.io/objection.js/api/model/instance-methods.html)! This change does both then updates anything currently handling a current licence version to use them.
Cruikshanks
added a commit
that referenced
this pull request
Jun 24, 2024
https://eaflood.atlassian.net/browse/WATER-4322 https://eaflood.atlassian.net/browse/WATER-4442 Whilst working on [Fix view licence abs amounts issue plus refactor](#1132) we started making a change to `app/services/licences/fetch-licence-summary.service.js` to bring how it fetches the current licence to be in line with `app/services/return-requirements/setup/fetch-abstraction-data.service.js`. In both cases, the services that call these are then doing `licence.licenceVersions[0]` in order to access the 'current' one. We could ensure consistency by adding an [Objection.js modifier](https://vincit.github.io/objection.js/api/model/static-properties.html#static-modifiers) to `LicenceModel`. And then we could make it explicit that we only care about the current licence (and make everyone's lives easier) if we added a custom [instance method](https://vincit.github.io/objection.js/api/model/instance-methods.html)! This change does both then updates anything currently handling a current licence version to use them.
Cruikshanks
force-pushed
the
simplify-licence-summary
branch
from
1130a92 to
042d92b
Compare
https://eaflood.atlassian.net/browse/WATER-4322 > Part of our work to replace the legacy view licence page We found our new licence was crashing when we tried to view a specific licence. When we dug into it the problem was that even though it had a current version in the `PermitLicenceModel`s `licenceDataValue` field, and that version had purposes, those purposes didn't have abstraction amounts. It's not common for licences to be in this state, but there is not much we can do as the data comes from NALD. It would have to be corrected there. So, we need to update the `ViewLicenceSummaryPresenter` code to handle. But it has been some time since we built the summary tab so we have more experience with the licence abstraction data. We now know these values can be taken from somewhere other than the JSON blob the previous team were using. This means along with fixing the issue, we intend to do a little 'housekeeping' along the way to try and simplify how things are working and to use normalised data wherever we can.
Now matches the fact we expect multiple values back and the variable name.
There is no real need for these to be determined inside the abstraction wrapper. They can handled like other properties and the methods can be called directly.
We can take these off the licence version purposes which means we can drop having to extract them from the permit licence first. We also set them directly in function and remove the element from the `_abstractionWrapper()`.
Cruikshanks
force-pushed
the
simplify-licence-summary
branch
from
20f57ec to
e6ca170
Compare
Cruikshanks
added a commit
that referenced
this pull request
Jul 1, 2024
https://eaflood.atlassian.net/browse/WATER-4331 > Part of the work to replace the legacy view licence page After attempting to [simplify the logic that builds the view licence page's summary tab](#1132) we've spotted that we may have introduced a bug. We have found a licence that has been linked to the same gauging station multiple times. This results in seeing the same station listed multiple times in the summary. Like the other sections, where something is duplicated we should see only one entry. This change fixes the logic and ensures the list is distinct.
Cruikshanks
added a commit
that referenced
this pull request
Jul 2, 2024
https://eaflood.atlassian.net/browse/WATER-4331 > Part of the work to replace the legacy view licence page After attempting to [simplify the logic that builds the view licence page's summary tab](#1132) we've spotted that we may have introduced a bug. We have found a licence that has been linked to the same gauging station multiple times. This results in seeing the same station listed multiple times in the summary. Like the other sections, where something is duplicated, we should see only one entry. This change fixes the logic and ensures the list is distinct.
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-4322
We found our new licence was crashing when we tried to view a specific licence. When we dug into it, the problem was that even though it had a current version in the
PermitLicenceModelslicenceDataValuefield, and that version had purposes, those purposes didn't have abstraction amounts.It's not common for licences to be in this state, but there is not much we can do as the data comes from NALD. It would have to be corrected there.
So, we need to update the
ViewLicenceSummaryPresentercode to handle this. But it has been some time since we built the summary tab so we have more experience with the licence abstraction data. For example, we now know these values can be taken from somewhere other than the JSON blob the previous team was using.This means, along with fixing the issue, we intend to do a little 'housekeeping' along the way to try and simplify how things are working, and to use normalised data wherever we can.