Add separate page for mandating 2SV for another user #2547
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
floehopper
force-pushed
the
split-out-change-2sv-requirement-page-from-user-edit-page
branch
from
cd29976 to
434430c
Compare
floehopper
changed the title
chrislo
approved these changes
Nov 27, 2023
The calls to `use_javascript_driver` should have been removed in this commit [1] when the reset 2SV functionality was changed to work without UJS. [1]: 1baf685
The assertions in these tests were passing even if all of the UI interactions were removed. This was because they were all operating on either the `user` or `user_in_child_organisation` instance variables both of which already had `User#require_2sv` set to `false`, so unsetting it is a no-op. Furthermore, I couldn't fix the tests by setting `User#require_2sv` to `true` in the setup, because then the checkbox for `User#require_2sv` in `app/views/users/_form_fields.html.erb` did not show up and so the call to `uncheck "Mandate 2-step verification for this user"` failed because it couldn't find the checkbox. In summary, these tests weren't testing what they were intended to test and the behaviour they were intended to test wasn't available via the user interface. Note that since this commit [1], the only way to use the user interface to unset `User#require_2sv` is to add an exemption for the user and this is already tested elsewhere in `ManagingTwoStepVerificationTest`. [1]: a5bff44
Trello: https://trello.com/c/caETStGa 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 different operations on a user and this is the next one. The `require_2sv` checkbox was previously only displayed on the "edit user" page (from `app/views/users/_form_fields.html.erb`) when `User#require_2sv?` was `false` and thus it was only ever possible to use this checkbox to *mandate* 2SV; it was not possible to use it to *unset* the `User#require_2sv` attribute. Unlike in commits like this one [1] where we first split out the new page and then separatly moved the new page to use the Design System, here I've created the new page using the Design System in one go, because there wasn't much markup in the "edit user" page to extract. The new `app/views/users/two_step_verification_mandations/edit.html.erb` template is based on other similar pages which we've recently extracted, e.g. `app/views/users/two_step_verification_resets/edit.html.erb`. I decided not to include a checkbox on the new page, because as mentioned above, this page should only ever be needed to *mandate* 2SV. The new `Users::TwoStepVerificationMandationsController` is closely based on the relevant bits of code from `UsersController`, even though some of it is probably overkill in the new context, i.e. the use of `UserUpdate`. However, I thought it was worth keeping this step as small as possible. I've hard-coded `user_params` to `{ require_2sv: true }`, since there's no checkbox in the form. I'm using `UserPolicy#mandate_2sv?` for the authorization in the new `Users::TwoStepVerificationRequirementsController`, because that seems to make the most sense. You could argue that I should also check `UserPolicy#edit?` & `UserPolicy#update?` like I did in `Users::OrganisationsController` in this commit [2], but I've gone off that idea (sorry, @chrisroos!). Instead I've matched what I did in `Users::TwoStepVerificationResetsController` in this commit [3]. There weren't *any* tests in `UsersControllerTest` for this behaviour, so I had to write `Users::TwoStepVerificationMandationsControllerTest` from scratch, but I based it closely on other similar controller tests. Luckily there was an integration test (`test/integration/managing_two_step_verification_test.rb`) covering the behaviour which I've been able to fixup to work with the new UI. Rather than creating a combinatorial explosion of tests in `Users::TwoStepVerificationRequirementsControllerTest` 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 `UserPolicy#mandate_2sv?`. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. I'm using the `error_summary` component in combination with identical code we've used elsewhere on the off-chance there's a validation error, because the controller action does re-render the form in that case. Note that in normal operation there are unlikely to be validation errors on this page, but since it's theoretically possible I thought it was worth adding the error summary. We might want to consider changing this to work more like `Users::TwoStepVerificationResetsController` which raises an exception if there is a validation error. This makes sense particularly given that the only validation on `User#require_2sv` only applied on record creation. [1]: 899a8a1 [2]: bf207ef [3]: 1baf685
As requested by Etain. Note that as discussed, I haven't made similar changes to the "edit user" page, because that's about to be changed significantly when we convert it to use the GOV.UK Design System.
floehopper
force-pushed
the
split-out-change-2sv-requirement-page-from-user-edit-page
branch
from
434430c to
d618833
Compare
|
I've made the copy changes requested by Etain in d618833. 
Force-pushed in preparation for merging. |
floehopper
deleted the
split-out-change-2sv-requirement-page-from-user-edit-page
branch
This was referenced Nov 28, 2023
floehopper
added a commit
that referenced
this pull request
Nov 30, 2023
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
floehopper
added a commit
that referenced
this pull request
Dec 4, 2023
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
floehopper
added a commit
that referenced
this pull request
Dec 5, 2023
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
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/caETStGa
This splits out mandating 2SV for another user into a separate page. The latest design calls for the splitting out of a bunch of user operations into separate pages like this. The new "mandate 2SV" page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497, #2509, #2537 & #2545.
Previously this functionality was implemented via a checkbox on the "edit user" page. This checkbox was only displayed when 2SV was not already mandated for the user either via an organisation mandate or an individual mandate, i.e. when
User#require_2sv?wasfalse. I've implemented the new page as just a button (no checkbox), because we only ever need to setUser#require_2svtotrue. If the user has a 2SV exemption, then some additional text is displayed in both the link and the form to explain that the exemption will be removed. Also in this case thedestructiveoption on the button is set totrueto highlight that the exemption data will be lost.Notes
User#require_2svcan be set tofalseby adding a 2SV exemption for the user.UsersController#require_2svpage which is part of the inviting a new user workflow.UsersController#require_2svpage with the page in this PR (i.e.Users::TwoStepVerificationMandations#edit), but there were enough differences that I decided it was out-of-scope in this PR.New "Mandate 2SV" link on "edit user" page
New "mandate 2SV" page
New "Mandate 2SV" link on "edit user" page (when user has 2SV exemption)
New "mandate 2SV" page (when user has 2SV exemption)