Add separate page for editing another user's role #2537
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
I want to be able to combine this trait with the `two_step_exempted_user` in a test I'm about to add.
floehopper
changed the title
floehopper
changed the title
floehopper
force-pushed
the
split-out-edit-user-role-page-from-user-email-page
branch
from
ed122c6 to
0276b5b
Compare
chrisroos
approved these changes
Nov 22, 2023
There was a problem hiding this comment.
Looks good to me, @floehopper.
I've asked a couple of questions but I don't feel strongly enough that the answers should block merging so I'll approve this and let you decide what to do with the questions.
Trello: https://trello.com/c/shaJpu8F 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/roles/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 changed the template to use the Design System. I plan to do that in subsequent commits. The new `Users::RolesController` 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::RolesController`, because that still seems to make sense. However, note that we also make use of the existing unconventional `UserPolicy#assign_role?` predicate method which does not correspond to a controller action. I suspect there's some simplification we could do here to make this more idiomatic, but I've left that for now. I've tried to add tests in `Users::RolesControllerTest` 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`. Also note that, unlike in `Users::NamesControllerTest` & `Users::EmailsControllerTest`, most of the tests are in the context of a Superadmin user doing the editing rather than just an Admin user. This is because only Superadmins can change a user's role (see `UserPolicy#assign_role?`). It took me a while to understand that the `User#exempt_from_2sv?` condition in the template is there in order to avoid the validation error that's added by `User#user_can_be_exempted_from_2sv` when an attempt is made to upgrade a normal user exempted from 2SV to any admin role. I have added tests for this scenario for both `Users::RolesController#edit` and `Users::RolesController#update`, even though it's unlikely that a user would ever see the relevant validation error unless they hacked the form. Rather than creating a combinatorial explosion of tests in `Users::RolesControllerTest` 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 `ChangeUserRoleTest` so I've just changed them to make them work with the new page. [1]: 899a8a1
Trello: https://trello.com/c/shaJpu8F c.f. this commit [1] where we did the same for user name. This changes the edit user role 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 has to handle the case where the user being edited is exempt from 2SV which means they cannot be upgraded from the normal role to any other (admin) role. In this case we just display some inset text to explain why the user's role cannot be changed. I'm not sure this is a great user experience, but it's somewhate similar to what happens in the account change role page. Note that I've humanized the role in the inset text, because I think that's more consistent with how we're displaying role names to users in the rest of the app. I did contemplate hiding the "Change (role)" link on the edit user page in this scenario. However, this link is already hidden when the current user does not have permission to change a user's role and I thought it would be more confusing if there were two reasons why it might not show up. I think the behaviour in this commit is good enough for now and we can iterate on the UX/design later. I've based the design on the similar functionality in the `app/views/account/roles/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. I've also included the same hint HTML as used for a very similar `role` select component in `app/views/devise/invitations/new.html.erb` which replaces the "help-block" `span` element. I plan to extract this duplication in a subsequent commit. 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 `select` component with `error_message` set appropriately, so that any relevant errors are displayed alongside the `role` field and the field itself is highlighted in red as per this Design System guidance [2]. I've enhanced a test in `Users::RolesControllerTest` to cover this new behaviour. Note that in normal operation there are unlikely to be validation errors on this page. However, it is possible if someone hacks the form or e.g. you try to upgrade a user without an organisation to Organisation Admin. The button text has changed from "Update User" -> "Change role" and I've introduced a "Cancel" link to match other similar pages. [1]: f71a128 [2]: https://design-system.service.gov.uk/patterns/validation/#how-to-tell-the-user-about-validation-errors
To remove duplication.
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 and somewhat overkill for this controller. This commit removes the use of `UserParameterSanitiser` but retains the use of `.permitted_user_params` on the `Role::Base` subclasses. Note that only `Roles::Superadmin.permitted_user_params` returns an array including `:role` which is the only parameter we're interested in in this controller. I think ideally we'd make use of Pundit's strong parameters functionality [2] and move this logic out of the `Roles::Base` subclasses 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
floehopper
force-pushed
the
split-out-edit-user-role-page-from-user-email-page
branch
from
a0e540a to
1633d09
Compare
|
Applied fixup commits and force-pushed in preparation for merging |
floehopper
deleted the
split-out-edit-user-role-page-from-user-email-page
branch
This was referenced Nov 23, 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/uZD2I9dj & https://trello.com/c/shaJpu8F
This splits out the editing of another user's role 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 role" page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497 & #2509.
One complication is that there are at least two scenarios in which it may not be possible to change a user's role:
I did contemplate hiding the new "Change (role)" link on the "edit user" page in both scenarios, but I thought it might be confusing if we lost the text explaining the problem in scenario 2. I'm sure we can improve on the user experience in scenario 2, but this seems like a reasonable first step, especially since I'm guessing scenario 2 isn't very common.
In my use of the
error_summarycomponent I have also brought the display of validation errors more into line with this Design System guidance. Having said that, it's quite hard (but not impossible) to get a validation error on this page!New "Change" link on "edit user" page
New "edit user role" page
New "edit user role" page with validation error (by hacking the form)
New "edit user role" page with validation error (missing organisation)
New "edit user role" page with explaination for users exempt from 2SV
Planned design for the "edit user" page (not part of this PR)