Account page using design system #2377
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
mike29736
commented
Sep 26, 2023
chrisroos
reviewed
Sep 27, 2023
There was a problem hiding this comment.
This all looks good to me, @mike29736!
I'm not approving it yet because I know you're making other changes.
chrisroos
reviewed
Sep 28, 2023
chrisroos
reviewed
Sep 28, 2023
mike29736
force-pushed
the
account-page-design-system
branch
2 times, most recently
from
cfbe5e3 to
e5d4d39
Compare
chrisroos
force-pushed
the
account-page-design-system
branch
4 times, most recently
from
c0fe369 to
b7566ea
Compare
This partial was being shared between two pages that I want to decouple. In the version inlined into `Users#edit_email_or_password`, I've swapped references to `@user` for `current_user`, because in that view a user can only ever change their own email and password.
Adding some IDs to make the anchor links work
I'm planning on moving this view to another controller. I don't believe we're currently aiming to localise Signon so it seems cleaner to just remove locales entries instead of updating them
I kept reading these aliases of `#edit?` as aliases of `#edit_email_or_password?`, purely based on their location. Maybe it's just me!
We're making a new Account page. The new page will link to a handful of sub-pages that each handle a specific account-related task. This existing page will be used for changing your email address or password, so I've moved it into the /account namespace to hopefully make the user journey feel more cohesive.
The ("Change your email or password") page that uses this endpoint has
already moved to the /account namespace because it's becoming a sub-page
of the new Account page.
I'm undecided on the naming/RESTful design of this resource, so I'm
waiting to take some cues from the upcoming design review in this area.
For the time being I'm keeping it as I found it, but moving it into the
same controller as the page that uses it.
The ("Change your email or password") page that uses this endpoint has
already moved to the /account namespace because it's becoming a sub-page
of the new Account page.
I'm undecided on the naming/RESTful design of this resource, so I'm
waiting to take some cues from the upcoming design review in this area.
For the time being I'm keeping it as I found it, but moving it into the
same controller as the page that uses it.
Until now, Users#resend_email_change was used in two distinct scenarios: 1. Admins performing the action for other users' accounts 2. Users (of all roles) performing the action for their own account It seems as though the `if @user.normal?` conditional was actually breaking this distinction: in the case of admin users in scenario 2, they'd only ever see the flash notice intended for scenario 1. Scenario 2 is now exclusively performed from the "Change your email or password" page. That page has already moved to the /account namespace because it's becoming a sub-page of the new Account page. That page now has its own dedicated `#resend_email_change` endpoint. Scenario 1 is still performed from Users#edit. Similarly to the approach with the actions that I've just lifted-and-shifted from UsersController to Account::EmailPasswordsController, for the time being I'm keeping the naming/RESTful design as I found it and planning to rethink that after the upcoming design review.
Until now, Users#cancel_email_change was used in two distinct scenarios: 1. Admins performing the action for other users' accounts 2. Users (of all roles) performing the action for their own account Scenario 2 is now exclusively performed from the "Change your email or password" page. That page has already moved to the /account namespace because it's becoming a sub-page of the new Account page. (Scenario 1 is still performed from Users#edit.) Similarly to the approach with the actions that I've just lifted-and-shifted from UsersController to Account::EmailPasswordsController, for the time being I'm keeping the naming/RESTful design as I found it and planning to rethink that after the upcoming design review. I'm struggling to fully understand the UserPolicy (!) but I think it was safe to remove the remaining references to `allow_self_only`, since Users#cancel_email_change was the last UsersController action left that was used in the ways described by both of the scenarios listed above ^
We were only doing this for one of the actions in this controller, which was resulting in an inconsistent experience for users. Since there are soon going to be multiple routes into the "Change your email or password" page (i.e. the page that uses this endpoint), it might make sense to redirect back to the page that the user visited *before* that one. That's not what `redirect_back_or_to` was doing in this case, though, it was redirecting back to the "Change your email or password" page itself. Either way, it should be consistent.
I've included tests to confirm that this is working (most useful for `#update_email` where we've switched from using `new_email` to using `current_user.unconfirmed_email`)
That "sub-page" is the "Change your email or password" page, which is linked to from a card component. I'm planning to add 4 more cards to this page in later commits. Like this first one, they'll link to a standalone sub-page each allowing users to make changes to a different aspect of their account. In this commit I've also removed an integration test instead of updating it to reflect my changes, because the important stuff to cover is now covered by the AccountTest.
This "Edit your account" title might need a rethink, but it's consistent with the pre-existing page title and heading
Again, I've taken the breadcrumb name from the page's title/heading. I'm no breadcrumb theorist... we also link to some of these pages directly from the dashboard. It's OK for breadcrumbs to not reflect the actual route that the user's taken, right?
Instead of redirecting to the Dashboard, Account page-related actions now redirect to the Account page on success. The page that these actions are performed from can be accessed from either the Dashboard or the Account page, but: 1. (I think) we can't tell which route the user took to get here 2. the Account page is now their canonical home I can't say I really know where these actions would be best redirecting back to in terms of the user experience. How they fit with a typical user's expectations might depend on the specific action they're performing, too.
This is going to be linked to from the new Account page (for users who are allowed to changing their own permissions). There's an entire new permissions management area being developed as we speak, so it doesn't make sense to put a lot of additional effort into this version of it. As a working placeholder, I've copied the permissions management section of `Users#edit` and I've tried to keep the changes to a minimum. Some things just didn't make sense in the new (current_user's account) context. For example, I've tried to remove confusing references to the user that's having their permissions changed -- in this case that's just the current_user, changing their own permissions.
As with the other Account sub-page's endpoints, I'm not 100% sure on the ideal place to redirect to but the Account page now makes more sense than the Dashboard
I'm planning on moving the parent :two_step_verification resource and
want to make small, incremental changes in case we need to pinpoint the
introduction of anything unexpected!
Because the `path` and `controller` are explicitly set on this resource
declaration, I think the only side effect this change could have caused
is to the naming of the Rails-generated URL helper methods (e.g.
`two_step_verification_session_path`). I've prevented that from
happening by prefixing the resource name ("two_step_verification_" +
"session").
I'm planning to make this page another sub-page of the new Account page. For consistency with the Account page and the other Account sub-pages, I've changed this page's URL to live under /account instead of /users. Since this page is already only ever used by the current_user to configure their own account and since the controller already lives in the fairly neutral `Devise` namespace, it's only the URL that needs changing to make this an "account" page instead of a "user" one. And since the affected part of the URL isn't factored into its Rails-generated URL helper method names, no redirects or links need updating to complete this change, either.
I'm planning to make the 2SV setup page an Account sub-page. Without any better ideas, I'd like the wording of the link from the Account page to be consistent with the page's own existing title and heading. The wording of the link on the Dashboard and the (title and heading of the) 2SV page weren't exactly identical before this change, but the difference seemed arbitrary enough not to worry about.
Give the link a different name depending on whether the user has 2SV setup already or not
Consistent with the other Account sub-pages, instead of redirecting to the Dashboard, redirect to the Account page on success.
This provides some functionality that was lost in the switch from Users#edit to the new Account page. For Admin and Superadmin users it allows making changes, but for users with other roles it's read-only. Since this is another Account sub-page and another mixed bag of a page, for consistency I've mostly followed the approaches taken by the "Change your email or password" page. --- I've let one of the new helper methods be tested through the integration test only, because I didn't manage to figure out the correct test setup required for a helper method that's calling a Pundit method.
This provides another bit of functionality that was lost in the switch from Users#edit to the new Account page. It's pretty much identical to the Users#event_logs page except for living under /account
This matches the behaviour of the users edit page, where only Super Admin users can update their and other users' roles (implemented in `UserPolicy#assign_role?`[1]). [1]: https://github.com/alphagov/signon/blob/1faf579e720605006b693ad6ca56d6aa29fe2852/app/policies/user_policy.rb#L56
So that users don't have two different ways of editing their own details (account page and users edit page). I've implemented this redirect in `#load_and_authorize_user` (instead of in the action itself) because in a future commit I want to update the `UserPolicy` to prevent users editing their own accounts through this controller.
chrisroos
force-pushed
the
account-page-design-system
branch
2 times, most recently
from
af342f0 to
a743532
Compare
Users should edit their own details using their account page functionality. This commit ensures that no one can continue using the /users pages to edit their own account. In the UserPolicy test for superadmins and admins, I have to skip the `create` action because these roles should continue to be able to create new users. This includes a fix for a bug in the _form_fields partial. The partial was previously checking whether the signed in user can mandate_2sv for themselves instead of checking whether the signed in user can mandate_2sv for the user they're editing. The change to UserPolicy in this commit highlighted the bug because users can no longer edit their details (including mandate_2sv) through the users controller. I've had to update two tests in users_controller_test so that we're mimicking a user editing another user, instead of the user editing themselves.
We're already using the `policy` helper method elsewhere so let's use that consistently. This also avoids us having to pass the `current_user` in.
chrisroos
force-pushed
the
account-page-design-system
branch
from
a743532 to
22c3626
Compare
chrisroos
approved these changes
Oct 3, 2023
There was a problem hiding this comment.
This all looks good to me, @mike29736!
I've added a couple of small commits to the branch:
- Ensure only superadmins can edit their role
- Redirect users to
/accountif they attempt to edit their own account using/users/<id>/edit
floehopper
added a commit
that referenced
this pull request
Nov 9, 2023
It's a bit hard to follow the git history of this controller, because it seems to have begun its life as a non-admin controller with actions allowing users to change their own user, but then in this commit [1] a bunch of functionality from an admin controller was merged into it. When we moved the users index page to use the Design System, we removed some actions from that page [2,3]. More recently we've split out a lot (all?) of the non-admin functionality into controllers under the `Account` namespace [4]. I'm pretty sure the redirects to `root_path` date back to a time when the action was called in the context of a user updating their own user. However, these three actions (`unlock_access`, `resend_email_change` & `cancel_email_change`) are now only every called in the context of the page rendered by `UsersController#edit`, so it doesn't make any sense to me to redirect to the `root_path`. Instead I've changed them to redirect to the `users_path` which seems consistent with the `update` action. The redirects using `redirect_back_or_to` seem to relate to a time when the action was called in multiple contexts: either non-admin vs admin; or "user index" page vs "edit user" page. The idea is to redirect back to the page the user came from so the the user interaction makes sense in both contexts. Note that the path passed to this `redirect_back_or_to` method is only a fallback if the previous page cannot be determined. I've tried to standardize on using the "users index" page (i.e. `users_path`) as the success path and the "edit user" page (i.e. `edit_user_path`) as the failure path (i.e. the page where the form errors are typically displayed). This feels like it gives a better user experience and makes the code less suprising. [1]: b730700 [2]: 62eb2f6 [3]: 2197050 [4]: #2377
floehopper
added a commit
that referenced
this pull request
Nov 9, 2023
It's a bit hard to follow the git history of this controller, because it seems to have begun its life as a non-admin controller with actions allowing users to change their own user, but then in this commit [1] a bunch of functionality from an admin controller was merged into it. When we moved the users index page to use the Design System, we removed some actions from that page [2,3]. More recently we've split out a lot (all?) of the non-admin functionality into controllers under the `Account` namespace [4]. I'm pretty sure the redirects to `root_path` date back to a time when the action was called in the context of a user updating their own user. However, these three actions (`unlock_access`, `resend_email_change` & `cancel_email_change`) are now only every called in the context of the page rendered by `UsersController#edit`, so it doesn't make any sense to me to redirect to the `root_path`. Instead I've changed them to redirect to the `users_path` which seems consistent with the `update` action. The redirects using `redirect_back_or_to` seem to relate to a time when the action was called in multiple contexts: either non-admin vs admin; or "user index" page vs "edit user" page. The idea is to redirect back to the page the user came from so the the user interaction makes sense in both contexts. Note that the path passed to this `redirect_back_or_to` method is only a fallback if the previous page cannot be determined. I've tried to standardize on using the "users index" page (i.e. `users_path`) as the success path and the "edit user" page (i.e. `edit_user_path`) as the failure path (i.e. the page where the form errors are typically displayed). This feels like it gives a better user experience and makes the code less suprising. [1]: b730700 [2]: 62eb2f6 [3]: 2197050 [4]: #2377
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.
https://trello.com/c/CTftWKa2/259-update-the-own-account-page-to-use-the-design-system
What
This pull request introduces the new Account page at /account. It includes 4 or 5 cards (depending on user role) that link to various account-related "sub-pages".
Summary (in commit-order)
/users/<id>/editand instead redirecting them/accountRoute changes
/users/<id>/edit_email_or_passwordto/account/email_password/users/<id>/update_emailto/account/email_password/update_email/users/<id>/update_passwordto/account/email_password/update_password/users/two_step_verificationto/account/two_step_verification/users/<id>/resend_email_changeto/account/email_password/resend_email_change/users/<id>/cancel_email_changeto/account/email_passwords/cancel_email_change/users/<id>/event_logsto/account/activity/account/manage_permissions/account/role_organisation,/account/role_organisation/update_organisationand/account/role_organisation/update_roleThings I haven't done:
Users#reset_two_step_verification-like action (or probably more possible, for a link to a new page dedicated to that action), as Joseph suggested when we spoke earlier in the week, but I have let them knowAccount page (view for a GOV.UK Admin who doesn't have 2SV setup yet)
Account page (view for a normal user who already has 2SV setup)
Breadcrumbs
Manage permissions page (view for a GOV.UK Admin)
Manage permissions page (view for a publishing manager)
Role and organisation page (view for a GOV.UK Admin)
Role and organisation page (view for a user who isn't a GOV.UK Admin)