Fix registered to link in view licence #1291
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-4650 The new view licence page went live in the [release today](https://github.com/DEFRA/water-abstraction-system/releases/tag/v0.20.0). But users have found if a licence has a registered user, and you click the link to see their status, it results in a 'page not found'. Having looked at the issue, it appears we have lost adding the user ID to the link. This change fixes the issue.
The instance and data this is pulling is covered by the modifier `registeredToAndLicenceName()` so the withGraphFetched is just duplicating the work.
Having dug into the `registeredToAndLicenceName()` modifier I can see it is pulling out the correct name to display. But the view is expecting something to set `primaryUser.userId`. But double checking the existing code and the original PR there is no other reference made to this. In fact, to pull out the user ID for the registered to user, you need to be able to link `public.licence_entities` to `public.users`. However, this is impossible because we are not including `idm.users.external_id` in our `public.users` view! So, this isn't a bug that's crept in, this has been here since day 1! Doh! 😱🤦
In the table it is `external_id`` but having checked it is only ever used to link `crm.entity` records to the user record.
We need to add the relationship between licence document header and licence entity role. But ahead of that we spotted some improvements we could do to the test to reduce the number of database interactions happening.
With us having to go even deeper to retrieve the primary user ID in order to make the broken link work we've decided to split up the existing modifier/helper. Having them separate means it is easier to see what is needed to determine each property. Having done so it becomes clear licence name is a doddle but primary user is a hell scape!
We're not using it any more so we can remove the defunct seeder.
It was using the `registeredToAndLicenceName()` modifier just to get the `LicenceDocumentHeader` ID which is overkill.
We remove the data transformation from the service so we can leave that to the presenter and simplify this fetch service. In the tests, the modifiers are already covered by tests in the `LicenceModel` itself so we don't need to re-test them here. We just add enough, i.e. a workflow record, to cover those query elements we're doing in the fetch service itself. We also tidy up field names, JSDoc comments and any sorting.
Needed to confirm that the basic access internal user has no roles assigned to it. But to do so we needed to create a new user account because we are not currently seeding one. So, updated the seed whilst about it to ensure we have the ability to access a user with this permission level both in tests and our non-prod environments.
Found we were returning values that either were not used by the view, or that could be simplified. In the tests, we were generating data that the presenter ignored or had tests that covered functionality already covered by other tests. So, in all we were able to simplify both the presenter and the tests. On top of that we brought the naming of the functions et al in line with our conventions plus some other housekeeping.
A similar story to the presenter. We were able to tidy some things up in the service. The key thing was the tests were testing things already covered by the presenter, which in turn we now know was covering stuff already tested in the model.
Its breaking a lot of tests. As it might mean they all need updating we'll do this in a separate PR. It's clearly beyind the scope of a minor tweak.
Ahh! It wasn't our new seed breaking the tests, but the new field we'd added to the view that isn't in our seed data.
You weren't the problem little fella! Back in you go.
Cruikshanks
added a commit
to DEFRA/water-abstraction-ui
that referenced
this pull request
Aug 29, 2024
https://eaflood.atlassian.net/browse/WATER-4558 > Part of the work to replace the legacy view licence page Spotted whilst [fixing an issue in the water-abstraction-system](DEFRA/water-abstraction-system#1291) version of the view licence page. The links in the `/user/{id}/status` page were still pointing to the old view. After taking a look we realised - feature flags were never actually being passed to the view - the flags were not being passed through the many (_many_ 😩) layers of macros to where it was being used - the link we would have generated would have been broken This change fixes all those issues and gets the link working.
Cruikshanks
added a commit
to DEFRA/water-abstraction-ui
that referenced
this pull request
Aug 29, 2024
https://eaflood.atlassian.net/browse/WATER-4558 > Part of the work to replace the legacy view licence page Spotted whilst [fixing an issue in the water-abstraction-system](DEFRA/water-abstraction-system#1291) version of the view licence page. The links in the `/user/{id}/status` page still point to the old view. After taking a look, we realised - feature flags were never actually being passed to the view - the flags were not being passed through the many (_many_ 😩) layers of macros to where it was being used - the link we would have generated would have been broken This change fixes all those issues and gets the link working.
Beckyrose200
added a commit
that referenced
this pull request
Sep 2, 2024
#1291 During a recent bug fix for the view-licence presenter, we removed some data from the presenter that we thought was redundant to it. This data is actually needed, so this PR is adding it back in thus fixing some broken functionality that was created by removing it (mainly the recalculate bills link showing for a licence agreement).
Beckyrose200
added a commit
that referenced
this pull request
Sep 2, 2024
#1291 During a recent bug fix for the view-licence presenter, we removed some data from the presenter that we thought was redundant to it. This data is needed, so this PR is adding it back in thus fixing some broken functionality that was created by removing it (mainly the recalculate bills link showing for a licence agreement).
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-4650
The new view licence page went live in the release today. But users have found if a licence has a registered user, and you click the link to see their status, it results in a 'page not found'.
After looking at the issue, we thought that we had lost the ability to add the user ID to the link at some point.
Having dug into the
registeredToAndLicenceName()modifier, we see it pulls out the correct name to display.However, the view is expecting something to set
primaryUser.userId. After double-checking the existing code and the original PR, we found no other reference to this.To pull out the user ID for the 'registered to' user, you need to be able to link
public.licence_entitiestopublic.users. However, this is impossible because we are not includingidm.users.external_idin ourpublic.usersview!So, this isn't a bug that's crept in; this has been here since day 1!
Doh! 😱🤦
This means this minor fix ended up exploding in size. We started by modifying the users view migration to include the critical field. We then needed to update a series of models to add the missing relationships that take us from licence to user.
After doing that, we realized the existing
registeredToAndLicenceName()modifier was doing too much and not returning what we needed. So, we replaced it with 2 new modifiers and associated instance methods.This meant updating the fetch licence service to apply the new modifiers, updating the presenter to use the latest data being returned, and then the service and view that backs the top content in the view licence page.
Each time we touched a module, we found either dead or redundant code or tests that covered things already tested elsewhere. So, we've done a bunch of cleanup as we've worked through the fix.
In short, this change fixes the issue, plus a lot of housekeeping! 😁