Update client apps when user's name changes #2523
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
chrisroos
force-pushed
the
update-client-apps-when-users-name-changes
branch
2 times, most recently
from
d14faa3 to
712ca0f
Compare
chrisroos
changed the title
floehopper
approved these changes
Nov 20, 2023
There was a problem hiding this comment.
Good catch! I think adding the controller tests with expectations on PermissioUpdater.perform_on are particularly useful, because (a) they better document the somewhat hidden behaviour of UserUpdate#call; and (b) they should make it easier to refactor UserUpdate in the future.
I think the reason I missed this in #2497 was (a) because UserUpdate does so much but only a small part of it is relevant to this controller action, so it's difficult to see the wood for the trees; and (b) the name of PermissionUpdater implies that it only affects a user's permissions. These are both things it would be good to rectify as soon as possible to avoid similar confusion for others.
Trello: https://trello.com/c/2t9zx0Aq Prior to #2497 changing a user's name in Signon would queue a job to push that update out to the applications the user has access to. This commit reinstates the previous behaviour by using `UserUpdate` in `Users::NamesController#update`, which in turn uses `PermissionUpdater` to push updates to the user's apps. I can't find any documentation that describes the changes that result in push updates to a user's applications, but as far as I can see, changing a user's name has resulted in a push update since the relevant functionality was added to Signon[1] and gds-sso[2] in 2012 so it seems reasonable that we retain this behaviour. Note that `UserUpdate#record_update`[3] records an `EventLog` event so I've removed this from `Users::NamesController#update`. [1]: f339848 [2]: alphagov/gds-sso@8c0888c [3]: https://github.com/alphagov/signon/blob/16299a28ed46d12a8b0cb61c1cf755166317057c/app/services/user_update.rb#L72
As far as I can tell we've been sending push updates (to the applications a user has access to) when a user's email changes since the functionality was added to Signon[1] and gds-sso[2] in 2012. Although we're still sending those push updates from the new `Users::EmailsController` (added in #2509) we weren't testing it in the controller test. I'm adding a test (copied from the previous commit) to avoid regressions in future. [1]: f339848 [2]: alphagov/gds-sso@8c0888c
chrisroos
force-pushed
the
update-client-apps-when-users-name-changes
branch
from
712ca0f to
6b845d9
Compare
|
Thanks, @floehopper! 👍 |
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.
Trello - https://trello.com/c/2t9zx0Aq
Prior to #2497 changing a user's name in Signon would queue a job to push that update out to the applications the user has access to. This PR reinstates the previous behaviour by using
UserUpdateinUsers::NamesController#update, which in turn usesPermissionUpdaterto push updates to the user's apps.I can't find any documentation that describes the changes that result in push updates to a user's applications, but as far as I can see, changing a user's name has resulted in a push update since the relevant functionality was added to Signon and gds-sso in 2012 so it seems reasonable that we retain this behaviour.
I've also added a test to ensure we continue to push email updates out to a user's applications.