Split "edit user" page and move top to GOV.UK Design System #2567
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
floehopper
force-pushed
the
split-edit-user-page-and-move-top-to-design-system
branch
4 times, most recently
from
2522093 to
9526b45
Compare
floehopper
force-pushed
the
split-edit-user-page-and-move-top-to-design-system
branch
from
b8a49ce to
b7b7244
Compare
chrislo
approved these changes
Dec 5, 2023
floehopper
added a commit
that referenced
this pull request
Dec 5, 2023
As suggested by @chrislo [1] [1]: #2567 (comment)
This is more consistent with what we're doing on other similar pages, particularly ones that have been changed to use the GOV.UK Design System, so I think this will help with subsequent changes.
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
I'm doing some reorganisation of this page as part of the move to use the GOV.UK Design System. Doing this first will make that easier.
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
Trello: https://trello.com/c/uZD2I9dj This is (yet!) another step on the way to moving the edit user page to use the GOV.UK Design System. Work on moving the permission management functionality is ongoing [1], but we are now in a position to move the rest of the edit user page to use the Design System. Splitting the manage permissions form out means we can e.g. change the layout for the edit user page. I did consider moving the non-permission-related functionality into a new `UsersController#show` action template and leaving the permission-related functionality in the existing `UsersController#edit` action template. This made some sense, because we no longer need a form for the non-permission-related functionality. However, it would have meant a much more pervasive change, because the path to the edit user page is used in many more places. Also even though there isn't a form, it feels as if admins are most likely to visit the edit user page in order to *edit* the user! I've introduced a new policy predicate method, `UserPolicy#manage_permissions?`, for the new action which is just aliased to `UserPolicy#edit?`. In `UsersControllerTest` I created a new context for the new `manage_permissions` action and then either moved or copied tests from the existing `edit` action as appropriate. One minor fly in the ointment is that I've had to change the `UsersController#update` action to render the new `UsersController#manage_permissions` action template in the case of a validation error. This is fine for when an admin is managing a user's permissions, but it might be confusing when they are mandating 2SV for a new user via the `UsersController#require_2sv` action template which also submits to `UsersController#update`. However, it feels extremely unlikely that there would ever be a validation error in the latter case and even if there was, they user would at least see the error even if they ended up on a surprising looking page! All this just makes me think it's worth combining `UsersController#require_2sv` into `Users::TwoStepVerificationMandationsController#edit` as I previously suggested in #2547, but I'm going to leave that for now. [1]: https://trello.com/c/baXtt5Dg
c.f. 62609d2 The `SuspensionsController` requires the current user to have the `UserPolicy#suspension?` permission and so it makes sense that we only display the link on the edit user page if the current user has that permission. Since `UserPolicy#suspension?` is currently aliased to `UserPolicy#edit?` this doesn't make any practical difference, but since we've defined `UserPolicy#suspension?` I think it makes sense to add this condition in case the policy implementation changes in the future.
c.f. 62609d2 The `Users::TwoStepVerificationResetsController` requires the current user to have the `UserPolicy#reset_2sv?` permission and so it makes sense that we only display the link on the edit user page if the current user has that permission. Since `UserPolicy#reset_2sv?` is currently aliased to `UserPolicy#edit?` this doesn't make any practical difference, but since we've defined `UserPolicy#reset_2sv?` I think it makes sense to add this condition in case the policy implementation changes in the future.
I found the interpolated ternary expression hard to read. While it's not as terse I think that avoiding the interpolation makes the code more readable.
It's OK to display information about the user's invitation to an admin without the `UserPolicy#resend_invitation?` permission; it's just the link to resend the invitation that needs to be hidden in that case.
It's OK to display the user's locked status to an admin without the `UserPolicy#unlock?` permission; it's just the link to unlock their account that needs to be hidden in that case.
I'm not sure why these two checks were previously passing the `User` class rather than the `user` instance to the `policy` helper method. I suspect it might have been because neither of the `UserPolicy` predicate methods actually depend on the user being edited; only on the current user which is provided implicitly. Anyway, I think it's clearer and more robust to consistently pass in the `user` instance in case the policy implementation changes.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier.
I plan to use this in a subsequent commit, but in any case this removes a bit of duplication and is consistent with `UsersHelper#two_step_status`.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. I've used an HTML description list element, because this most closely resembles the Summary List component [1] that we're planning to use. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. Note that I've changed the "Change name" link text to "Change Name", because the Summary List component [1] uses the same capitalization as the Description Term element text. Changing it here will make things simpler later. I've also made the controller test assertions robust against the markup generated by the Summary List component. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. Note that I've changed the "Change email" link text to "Change Email", because the Summary List component [1] uses the same capitalization as the Description Term element text. Changing it here will make things simpler later. I've also made the controller test assertions robust against the markup generated by the Summary List component. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. Note that I've changed the "Change organisation" link text to "Change Organisation", because the Summary List component [1] uses the same capitalization as the Description Term element text. Changing it here will make things simpler later. I've also made the controller test assertions robust against the markup generated by the Summary List component. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. Note that I've changed the "Change role" link text to "Change Role", because the Summary List component [1] uses the same capitalization as the Description Term element text. Changing it here will make things simpler later. I've also made the controller test assertions robust against the markup generated by the Summary List component. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing.
I'm planning to use this in a subsequent commit.
I'm planning to use this in a subsequent commit.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. We're no longer displaying the 2SV exemption reason if an exemption exists. Joseph has confirmed he's happy with this given the reason is visible (on another page) if you edit the exemption. The logic for the 2SV status is very slightly different compared with that in `User#two_step_status` in that `User#has_2sv?` is now checked *before* `User#exempt_from_2sv?`. I don't think this should make a material difference and in any case I think it's better than we use the shared `User#two_step_status` method rather than implementing our own custom version. If there's a problem with the latter then that needs fixing separately.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. We're no longer displaying the suspension reason if a user is suspended since the reason is visible (on another page) if you edit the suspension.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. This text has already moved to `app/views/users/invitation_resends/edit.html.erb` and it doesn't appear on the edit user page in the new design.
Trello: https://trello.com/c/uZD2I9dj This is a small step towards moving this page to the latest design which will use the GOV.UK Design System. I'm trying to make small changes to the page which will make the transition easier while retaining all the functionality and keeping all the tests passing. This text has already moved to `app/views/users/unlockings/edit.html.erb` and it doesn't appear on the edit user page in the new design.
This removes some duplication and I think it makes the code more readable. I also contemplated using it in `UserExportPresenter#row` instead of `user.organisation.try(:name)`, but that would have changed the format of the CSV, so I've left it alone for now. Ideally I think we'd create a null Organisation object to further simplify the code, but that can wait for another day.
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to suspend/unsuspend, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. I decided to remove a bunch of combinatorial tests in `UsersControllerTest`, because the new tests in `UsersHelperTest` for `UsersHelper#link_to_suspension` provide better coverage of the same logic. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to resend the signup email or if the user being edited has accepted their invitation, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. I decided to remove a bunch of combinatorial tests in `UsersControllerTest`, because the new tests in `UsersHelperTest` for `UsersHelper#link_to_resend_invitation` provide better coverage of the same logic. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to unlock the user or if the user being edited is not locked, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. I decided to remove a bunch of combinatorial tests in `UsersControllerTest`, because the new tests in `UsersHelperTest` for `UsersHelper#link_to_unlock` provide better coverage of the same logic. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
This link is only displayed if the current user is an admin in the GDS organisation.
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to exempt the user being edited, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to reset 2SV or if the user being edited does not have 2SV setup, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. I decided to remove a bunch of combinatorial tests in `UsersControllerTest`, because the new tests in `UsersHelperTest` for `UsersHelper#link_to_reset_2sv` provide better coverage of the same logic. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
Note that this changes the behaviour very slightly, because if the current user doesn't have permission to mandate 2SV or if the user being edited is already required to have 2SV, then an *empty* list item will be rendered. However, I have a plan on how to tackle that in a subsequent commit when we start using the Summary List component [1]. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list
This introduces the Summary List component [1] that we've been working towards for displaying the attributes of the user along with links to change them. I've extracted methods into `UsersHelper` to generate the appropriate options for each of the items. I decided to replace a bunch of combinatorial tests in `UsersControllerTest` with unit tests for these new helper methods. The links in the "Actions" section are displayed using a List component [2] and its items are obtained from a bunch of `UsersHelper#link_to_*` methods. As I mentioned when I added the latter, these return `nil` if the link is not to be displayed and we handle that here by compacting the array of items before passing it to the component. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: https://components.publishing.service.gov.uk/component-guide/list
The links in the "Actions" section should all start with a verb as recommended by Etain.
As suggested by Joseph.
As recommended by Etain and to match the title & heading of the page to which it links.
As suggested by Joseph.
To make it more consistent with other similar pages.
To make it more consistent with other similar pages.
As suggested by @chrislo [1] [1]: #2567 (comment)
floehopper
force-pushed
the
split-edit-user-page-and-move-top-to-design-system
branch
from
3dfa35b to
1e98cb9
Compare
floehopper
deleted the
split-edit-user-page-and-move-top-to-design-system
branch
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
This splits out the permissions-related functionality from the "edit user" page into a separate "manage permissions" page and then changes the "edit user" page to use the GOV.UK Design System. This is a culmination of a bunch of previous PRs that split out separate pages for various actions an admin can perform on another user, e.g. #2497 & #2509. This means that the "edit user" page no longer contains any forms, but it's now more like a "show user" page which displays various attributes of the user along with a bunch of links to pages for specific actions.
Although there are a lot of commits in this PR, almost all of them are small/mechanical preparatory refactorings intended to make the actual changeover relatively straightforward. The key commits are probably:
And then there are a few commits at the end which make small copy improvements suggested by Etain, Joseph et al, and some tweaks to make things a bit more consistent. In a bid to keep this PR as small (!) as possible, I haven't implemented the copy changes which would've meant slightly more pervasive changes to keep everything consistent.
Old "edit user" page
New "edit user" page
New "manage permissions" page
Old "suspend user" page
New "suspend user" page
Old "access log" page
New "access log" page