Add separate page for editing another user's email #2509
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
suggested changes
Nov 15, 2023
There was a problem hiding this comment.
I've added some comments that I intend to address tomorrow in @floehopper's absence.
I think this makes the tests more intention-revealing and the use of Devise.confirm_within makes the tests robust against configuration changes.
Trello: https://trello.com/c/SsVuShZm c.f. this commit [1] where we did the same for user name. This is a step on the journey to move the edit user page to use the GOV.UK Design System. The new design calls for separate pages for editing the different user attribute and this is the next one. The new `app/views/users/emails/edit.html.erb` template in this commit is closely based on the relevant bits of `app/views/users/edit.html.erb` & `app/views/users/_form_fields.html.erb`. I haven't yet moved the "Resend confirmation email" or "Cancel change" functionality and I haven't yet changed the template to use the Design System. I plan to do that in subsequent commits. The new `Users::EmailsController` is closely based on the relevant bits of code from `UsersController`, even though some of it is probably overkill in the new context, e.g. the use of `UserUpdate` & `UserParameterSanitiser`. However, I thought it was worth keeping this step as small as possible. I'm reusing `UserPolicy#edit?` & `UserPolicy#update?` for the authorization in the new `Users::EmailsController`, because that still seems to make sense. I've tried to add tests in `Users::EmailsControllerTest` to capture the behaviour implied by how things worked when they were in `UsersController`. It's not obvious that all of this was previously captured in `UsersControllerTest` or `UserUpdateTest`. Rather than creating a combinatorial explosion of tests in `Users::EmailsControllerTest` relating to whether a user with all the different roles can edit another user with all the different roles, I've resorted to stubbing `UserPolicy.new` and relevant predicate methods on the `UserPolicy` instance. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. The are already some integration tests in `EmailChangeTest` so I've just changed them to make them work with the new page. [1]: 899a8a1
Trello: https://trello.com/c/SsVuShZm This moves the functionality associated with the `UsersController#resend_email_change` & `#cancel_email_change` into the recently added edit user email page served by `Users::NamesController#edit`. This is another step on the journey to move the edit user page to use the GOV.UK Design System. The new design calls for separate pages for editing the different user attributes. The new bits of the `app/views/users/emails/edit.html.erb` template in this commit are closely based on the relevant bits of `app/views/users/_form_fields.html.erb`. I haven't yet changed the template to use the Design System. I plan to do that in a subsequent commit. I'm reusing `UserPolicy#resend_email_change?` & `UserPolicy#cancel_email_change?` for the authorization in the new `Users::EmailsController` actions, because that still seems to make sense. I've tried to add tests in `Users::EmailsControllerTest` to capture the behaviour implied by how things worked when they were in `UsersController`. It's not obvious that all of this was previously captured in `UsersControllerTest`. As before, rather than creating a combinatorial explosion of tests in `Users::EmailsControllerTest` relating to whether a user with all the different roles can inoke these actions on another user with all the different roles, I've resorted to stubbing `UserPolicy.new` and relevant predicate methods on the `UserPolicy` instance. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. The are already some integration tests in `EmailChangeTest` so I've just changed them to make them work with the new page.
Trello: https://trello.com/c/SsVuShZm c.f. this commit [1] where we did the same for user name. This changes the edit user email page to use the Design System and makes a few other tweaks to bring it into line with the latest designs. The page title & heading are slightly different to match other similar pages. I've added breadcrumbs to make this page consistent with other pages even though they're not shown in the latest design for this type of page. The template for this controller is a bit more complicated than the one for editing a user's name, because it includes some functionality for seeing a user's unconfirmed email, for resending an email change email, and for cancelling an email change. I've based the design on the similar functionality in the `app/views/account/emails/edit.html.erb` template, although I've reworded the text to make it clear that in this case it's an admin user making changes to a user rather than a user making changes to their own user. Also note that this unconfirmed email functionality is only activated when a user changes their own email (through the relevant account page) and not when an admin changes another users email - in that case the change is effectively confirmed immediately and automatically. I'm using the `error_summary` component in combination with identical code we've used elsewhere so that the errors in the summary link to the relevant form field. Similarly I'm using the `input` component with `error_items` set appropriately, so that any relevant errors are displayed alongside the `email` field and the field itself is highlighted in red as per this Design System guidance [2]. I've enhanced a test in `Users::EmailsControllerTest` to cover this new behaviour. The button text has changed from "Update User" -> "Change email" to match other similar pages. [1]: f71a128 [2]: https://design-system.service.gov.uk/patterns/validation/#how-to-tell-the-user-about-validation-errors
c.f. `redirect_to_account_page_if_acting_on_own_user` before action in `UsersController`. In this case it makes sense to redirect to the specific account page for editing email address.
c.f. `redirect_to_account_page_if_acting_on_own_user` before action in `UsersController`. In this case it makes sense to redirect to the specific account page for editing name. I missed this in #2497.
This is orthogonal to the rest of this PR, but I thought I'd make the change when I noticed it. This test became redundant in this commit [1] or one of its associated commits in #2484. This redirect is no longer relevant to a normal user, because they are not authorized to view the page at all and so they will be redirected to the dashboard page with a message saying they don't have permission to access the page. There is a separate test under the "for the currently logged in user" context which checks that an authorized (admin) user is redirected to the account page. [1]: baf42ed
c.f. this commit [1] where we did the same for `Users::NamesController`. I found the `UserParameterSanitiser` and associated logic that we brought from the `UsersController` quite confusing somewhat overkill for this controller, especially as the `.permitted_user_params` methods on the `Roles::Base` subclasses all return an array including `:email` which is the only parameter we're interested in in this controller. I did consider simplifying `Users::EmailsController#user_params` further, i.e. to just the idiomatic `params.require(:user).permit(:email)` which is actually all we need. However, since it's possible one or more of the `.permitted_user_params` methods on the `Roles::Base` subclasses might change and stop including `:email`, I thought I ought to handle that eventuality. I think ideally we'd make use of Pundit's strong parameters functionality [2] and move this logic out of the `Roles::Base` subclasses and `UserParameterSanitiser` into the relevant policy class. However, that's a job for another day! [1]: 2c8010c [2]: https://github.com/varvet/pundit/blob/4d8cdf1c10058c12f2c175f30b408f5d5532a00b/README.md#strong-parameters
To further simplify the method, c.f. this commit [1] where User#permitted_params was introduced. [1]: 09ff60d
To reduce duplication. Note that I left a couple of occurrences of the raw assertion, because they seemed to be more explicitly testing the target path of the redirect.
This reverts commit e2d86a2. I'm reinstating this test because it's testing some otherwise untested behaviour, notably that normal users are redirected to their account page. This is because we're calling `redirect_to_account_page_if_acting_on_own_user` before calling `authorize_user`[1]. [1]: https://github.com/alphagov/signon/blob/cece01e98035c35a8e834565ba1d3e0b353b27ae/app/controllers/users_controller.rb#L10
We don't yet have a way for users to edit their own names so we're redirecting to the account page.
chrisroos
force-pushed
the
split-out-edit-user-email-page-from-user-edit-page
branch
from
fac1ca8 to
ea7a390
Compare
chrisroos
approved these changes
Nov 16, 2023
chrisroos
deleted the
split-out-edit-user-email-page-from-user-edit-page
branch
|
This has now been deployed. Good work, @floehopper! 👍 |
chrisroos
added a commit
that referenced
this pull request
Nov 16, 2023
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
added a commit
that referenced
this pull request
Nov 16, 2023
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
added a commit
that referenced
this pull request
Nov 16, 2023
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
added a commit
that referenced
this pull request
Nov 20, 2023
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
|
Thanks for fixing this up and getting it merged, @chrisroos! |
floehopper
added a commit
that referenced
this pull request
Nov 21, 2023
The `#user_name_path` & `#user_email_path` route helper methods should require a `user` argument. However, the argument was not provided in either of the PRs where these forms were added (#2497 & #2509). The assertions in `Users::NamesControllerTest` & `Users::EmailsControllerTest` were already comparing with the correct values and *somehow* the calls to `#user_name_path` & `#user_email_path` without any arguments were magically returning the correct value, even though when I try them in a Rails console, I get an `ActionController::UrlGenerationError`. So this mistake wasn't actually causing any problems.
This was referenced Nov 21, 2023
This was referenced Nov 27, 2023
floehopper
added a commit
that referenced
this pull request
Nov 30, 2023
In recent PRs we've been moving all actions into separate pages, e.g. #2497, #2509, #2537 & #2540. This means that a bunch of form elements have been replaced by links to other pages. There's no need for these links to be rendered within the form. I'm about to restructure this page as a step on the way to moving it to use the GOV.UK Design System. Making this change first will make that easier. This diff is best viewed with the `--ignore-all-space` option [1]. [1]: https://git-scm.com/docs/diff-options#Documentation/diff-options.txt---ignore-all-space
floehopper
added a commit
that referenced
this pull request
Dec 4, 2023
In recent PRs we've been moving all actions into separate pages, e.g. #2497, #2509, #2537 & #2540. This means that a bunch of form elements have been replaced by links to other pages. There's no need for these links to be rendered within the form. I'm about to restructure this page as a step on the way to moving it to use the GOV.UK Design System. Making this change first will make that easier. This diff is best viewed with the `--ignore-all-space` option [1]. [1]: https://git-scm.com/docs/diff-options#Documentation/diff-options.txt---ignore-all-space
floehopper
added a commit
that referenced
this pull request
Dec 5, 2023
In recent PRs we've been moving all actions into separate pages, e.g. #2497, #2509, #2537 & #2540. This means that a bunch of form elements have been replaced by links to other pages. There's no need for these links to be rendered within the form. I'm about to restructure this page as a step on the way to moving it to use the GOV.UK Design System. Making this change first will make that easier. This diff is best viewed with the `--ignore-all-space` option [1]. [1]: https://git-scm.com/docs/diff-options#Documentation/diff-options.txt---ignore-all-space
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/SsVuShZm
This splits out the editing of another user's email into a separate page. The latest design calls for the splitting out the editing of a bunch of user fields into separate pages like this. The new "edit user email page uses the GOV.UK Design System, but the "edit user" page still does not.
These changes are closely based on #2497 which added a separate page for editing another user's name. However, the new email-related page is more complex, because it also includes functionality related to the scenario when a user has requested a change to their own email address and the change is pending until they click a link in a confirmation email. An admin can click a button to resend the confirmation email or click a link to cancel the change in email address. I've based the design for this on the equivalent design of the account edit email page, i.e. the page where a user can request a change to their own email, however I have adapted the text to make it clearer that in this case it's an admin carrying out actions for another user.
I do slightly wonder whether it makes sense to include the pending email change functionality on this new page - it might not be obvious that it's where to find it. However, for now I think it makes sense to follow what we've done in the account version of the page. There might also be a question of whether admins need this functionality at all!
In making the changes relating to the pending email change scenario, I noticed that we are relying on
rails-ujsvia themethod: :put&method: :deleteoptions passed tolink_to(both in the new page and in the account version of the page). The use ofrails-ujshas been deprecated in Rails v7.0, so we're going to need to address that before we can upgrade to Rails v7.1. I plan to add a note to this Trello card to make sure we don't miss that.Unlike in #2497, I decided not to inline
UserUpdate#callin this PR. I would like to do it eventually, but there's a lot more going on when a user'semailchanges. I found it particularly confusing that the (apparently important) call toUser#skip_reconfirmation!is inUserUpdate#callitself; whereas the rest of the relevant functionality is inUserUpdate#record_email_change_and_notify. I have at least added test coverage for all the behaviour inUserUpdaterelevant to this controller which should make any future refactoring a bit safer.In my use of the
error_summarycomponent I have also brought the display of validation errors more into line with this Design System guidance.New "Change" link on "edit user" page
New "edit user email" page
New "edit user email page with validation error
New "edit user email page with pending email change
Planned design for the "edit user" page (not part of this PR)