Display monitoring stations in view licence page summary tab #773
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
View licence summary monitoring stations
Cruikshanks
requested changes
Mar 5, 2024
db/migrations/public/20240229142857_create-water-gauging-stations-view.js
Outdated
Show resolved
Hide resolved
db/migrations/public/20240229150626_create-water-licence-gauging-stations-view.js
Outdated
Show resolved
Hide resolved
Comment on lines
+114
to
+115
| const jsonArray = stations.map(JSON.stringify) | ||
| monitoringStations = Array.from(new Set(jsonArray)).map(JSON.parse) |
There was a problem hiding this comment.
Can we not just pass stations directly into new Set() and therefore get our results directly from it?
Suggested change
| const jsonArray = stations.map(JSON.stringify) | |
| monitoringStations = Array.from(new Set(jsonArray)).map(JSON.parse) | |
| const monitoringStations = [...new Set(stations)] |
Worth a try?
There was a problem hiding this comment.
I tried that and sadly didn't work. I don't think it's built around doing sets of objects. or at least not that I could get working
There was a problem hiding this comment.
You are 💯 right! 🤦 It's that JavaScript thing of 2 objects even with the same properties and values are not equal, but 'foo' and 'foo' are.
Reminded myself of where we are using Set() and it is always with strings.
Cruikshanks
added a commit
that referenced
this pull request
Jun 13, 2024
https://eaflood.atlassian.net/browse/WATER-4331 The `LicenceGaugingStationModel` was in [Display monitoring stations in view licence page summary tab](#773) as part of our work to replace the legacy view licence page. `water.licence_gauging_station` is simply the many-to-many table that links licences to gauging stations. When we get those we'll typically set the relationship up, for example, that `licences` links to `gauging_stations` _through_ `licence_gauging_stations`. That was what we did in this case. Only, we named the relationship `licenceGaugingStations`. This is incorrect because if we did do an [Objection.js withGraphFetched()](https://vincit.github.io/objection.js/api/query-builder/eager-methods.html#withgraphfetched) we'd get `GaugingStationModel`s back, _not_ `LicenceGaugingStationModel`s. Also, we never added any tests for the relationship! This change fixes the name of the relationship and adds the missing tests.
Cruikshanks
added a commit
that referenced
this pull request
Jun 14, 2024
https://eaflood.atlassian.net/browse/WATER-4331 ** Where we started! The `LicenceGaugingStationModel` was added in [Display monitoring stations in view licence page summary tab](#773) as part of our work to replace the legacy view licence page. `water.licence_gauging_stations` links licences to gauging stations. When we get these, we'll typically setup the relationship, for example, as `licences` links to `gauging_stations` _through_ `licence_gauging_stations`. That was what we did in this case. Only we named the relationship `licenceGaugingStations`. This is incorrect because if we did do an [Objection.js withGraphFetched()](https://vincit.github.io/objection.js/api/query-builder/eager-methods.html#withgraphfetched) we'd get `GaugingStationModel`s back, _not_ `LicenceGaugingStationModel`s. But in this case, `water.licence_gauging_stations` also contains details about the abstraction periods that apply and the certain metrics (though, to be honest, we don't know why we record them!) We'll want that information as well at some point, so a _through_ relationship is not really appropriate. Also, we never added any tests for the relationship! ** Where we finished! Digging into this relationship and correcting it then meant needing to make changes to `FetchLicenceSummaryService`. But that brought to light that the fetch query was doing things in a way that could be simplified with knock-on effects for the presenter. Then in the presenter tests, we saw that there wasn't a test that covered a full response, and we'd split them based on the properties passed in and not what we get out. We also found that a number of these models didn't have unit tests themselves. Finally, the migration scripts didn't match up to what is actually in the real DB. SO, this became a major tidy up of fetching and presenting a licence for the view summary. There is still more we think can be done. But we've drawn a line at this point 😁.
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-4331
View licence summary monitoring stations