Split out edit user organisation page from user edit page #2540
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
Since this commit [1], the `name` param hasn't been included in the permitted params and so it's pointless including it in the request. The behaviour under test doesn't rely on any permitted user params being supplied, so it's simplest to remove it. [1]: 899a8a1
These should have been added in this commit [1]. [1]: 81ac2c0
Trello: https://trello.com/c/fA6dRVSm 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/organisations/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::OrganisationsController` 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::OrganisationsController`, because that still seems to make sense. However, note that we also make use of the existing unconventional `UserPolicy#assign_organisations?` 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::OrganisationsControllerTest` 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`. Most of these tests are in the context of a Admin user doing the editing, because only Admins & Superadmins can change a user's organisation (see `UserPolicy#assign_organisations?`). Rather than creating a combinatorial explosion of tests in `Users::OrganisationsControllerTest` 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. [1]: 899a8a1
Trello: https://trello.com/c/fA6dRVSm c.f. this commit [1] where we did the same for user name. This changes the edit user organisation 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. Only Admins & Superadmins can change another user's organisation and so the "Change (organisation)" link is only displayed alongside the user's organisation name on the "edit user" page for users with those roles. I've based the design on the similar functionality in the `app/views/account/organisations/edit.html.erb` & `app/views/devise/invitations/new.html.erb` templates which have already been converted to use the GOV.UK Design System. 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 `organisation_id` field and the field itself is highlighted in red as per this Design System guidance [2]. I've enhanced a test in `Users::OrganisationsControllerTest` 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 remove the organisation from an Organisation Admin. The button text has changed from "Update User" -> "Change organisation" 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
In this 6-year old commit [1] which was part of #555, a change was made to prevent Organisation Admins & Super Organisation Admins from changing another user's organisation. This behaviour was enforced by a custom guard clause [2] on `UsersController#update` which called `UserPolicy#assign_organisations?`. I think this commit should have also removed `:organisation_id` from the array returned by `Roles::OrganisationAdmin.permitted_user_params` & `Roles::SuperOrganisationAdmin.permitted_user_params` to make everything consistent. And that's what I've done in this commit. Note that this won't affect `Account::OrganisationsController#update`, because it doesn't use any of the `.permitted_user_params` methods. [1]: 645b6c5 [2]: https://github.com/alphagov/signon/blob/bc818ecd5515721002e75b3544a89e4c91a2736e/app/controllers/users_controller.rb#L39
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` & `Roles::Admin.permitted_user_params` return an array including `:organisation_id` 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
To make it consistent with `UserPolicy#assign_role?`. I think the singular form of these predicate methods is more appropriate, because you don't assign multiple organisations or multiple roles to a user.
floehopper
force-pushed
the
split-out-edit-user-organisation-page-from-user-edit-page
branch
from
1f741a0 to
65792aa
Compare
chrisroos
approved these changes
Nov 23, 2023
|
Thanks, @chrisroos! |
floehopper
deleted the
split-out-edit-user-organisation-page-from-user-edit-page
branch
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/fA6dRVSm
This splits out the editing of another user's organisation 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 organisation page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497, #2509 & #2537.
The new "Change (organisation)" link on the "edit user" page only shows up if the current user is an Admin or a Superadmin, i.e. they have permission to change another user's organisation.
In my use of the
error_summarycomponent I have also brought the display of validation errors more into line with this Design System guidance. The most obvious validation error that can occur is if you attempt to change an Organisation Admin or Super Organisation Admin's organisation to None. Although I do wonder if this would be better tackled by excluding the "None" option from the select box in that scenario. However, I suggest we tackle that separately.New "Change" link on "edit user" page
New "edit user organisation" page
New "edit user organisation" page with validation error
Planned design for the "edit user" page (not part of this PR)