Move "edit API user" page to use GOV.UK Design System #2593
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
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.
I'm planning to change the "edit user" page to remove the form field for the user name and instead provide a link to the change user name page provided by `Users::NamesController#edit`. This will mean we no longer want to permit the `api_user[:name]` param for the `ApiUsersController#update` action, but we will want to preserve it for the `#create` action. Extracting this local variable will make that simpler. Note that I've also renamed the `params` argument to `ApiUsersController#permitted_user_params` to avoid it shadowing the raw params, because I'm going to need the latter to detect which is the current action.
I'm about to make some changes in this area and so having some coverage in place beforehand will give me more confidence to make the changes.
The tests don't particularly depend on the role of the signed-in user - we stub `UserPolicy.new` explicitly to avoid this. I'm about to change `Users::NamesController` to support operating on API users and making this change first will make that easier.
floehopper
force-pushed
the
split-edit-api-user-page-and-move-top-to-design-system
branch
from
774737c
to
a70cdae
Compare
I'm planning to reuse this controller by linking to its edit page from the edit API user page. However, it needs some tweaks to be able to support this use case. We're not actually linking to it yet; just preparing if for that change. I'm confident that `Users::NamesController#update` calling `UserUpdate#call` does everything related to changing `ApiUser#name` that `ApiUsersController#update` currently does: * The call to `ApiUser#skip_reconfirmation!` in `ApiUsersController#update` is only relevant when updating `ApiUser#email`, because it prevents the sending of confirmation emails, but `UserUpdate#call` does it anyway. * There's no need to call `reload` on `ApiUser#application_permissions`, because the permissions are not being changed, but `UserUpdate#call` does it anyway. * The call to `PermissionUpdater.perform_on` (which confusingly doesn't only need to be called when permissions are changed) is done in `UserUpdate#perform_permissions_update`. * Although an extra call to `EventLog.record_event` will now be triggered in `UserUpdate#record_update`, I think it makes complete sense and it may even have been an oversight that it wasn't called when the event logging was originally added. Other notes: * The logic to determine whether the `name` param is permitted in `Users::NamesController#user_params` was originally based on similar code to that in `ApiUsersController#api_user_params` i.e. using `UserParameterSanitiser` and it takes the various `.permitted_user_params` methods on the `Roles::Base` subclasses into account. * The `title_caption` is now different depending on whether the user is an API user or not. * The breadcrumb & cancel links back to the edit user page are now different depending on whether the user is an API user or not. * The successful redirect path is now different depending on whether the user is an API user or not.
This ensures that we use predicate methods on an instance of the correct policy class, i.e. `UserPolicy` or `ApiUserPolicy`. Previously it was always using `UserPolicy` whether the user was an API user or not.
This is another small step towards moving the "edit API user" page to use the GOV.UK Design System. Having adapted `Users::NamesController` to work with API users in a previous commit, we can now safely link to it from the "edit API user" page. As I explained in that previous commit, I'm confident that `Users::NamesController#update` calling `UserUpdate#call` does everything related to changing `ApiUser#name` that `ApiUsersController#update` used to do. In `ApiUsersController`, I've removed `api_user[:name]` from the permitted params for all actions other than the `create` action. I've changed various tests for `ApiUsersController#update` to use the `email` param rather than `name`. I've changed the `ManageApiUsersTest` integration test to work with the new UI design.
I want to use this for a pathological controller test where an `ApiUser` is invited. I don't think an `ApiUser` should be able to get into this state which is why I've only extracted the trait and not also created an `invited_api_user` factory.
The tests don't particularly depend on the role of the signed-in user - we stub `UserPolicy.new` explicitly to avoid this. I'm about to change `Users::EmailsController` to support operating on API users and making this change first will make that easier.
floehopper
force-pushed
the
split-edit-api-user-page-and-move-top-to-design-system
branch
3 times, most recently
from
9aba67e
to
56f8b5c
Compare
chrisroos
approved these changes
Dec 13, 2023
I'm planning to reuse this controller by linking to its edit page from the edit API user page. However, it needs some tweaks to be able to support this use case. We're not actually linking to it yet; just preparing it for that change. I'm confident that `Users::EmailsController#update` calling the modified `UserUpdate#call` does everything related to changing `ApiUser#email` that `ApiUsersController#update` currently does: * The call to `ApiUser#skip_reconfirmation!` is now made by `UserUpdate#call`. * There's no need to call `reload` on `ApiUser#application_permissions`, because the permissions are not being changed, but `UserUpdate#call` does it anyway. * The call to `PermissionUpdater.perform_on` (which confusingly doesn't only need to be called when permissions are changed) is done in `UserUpdate#perform_permissions_update`. * Although extra calls to `EventLog.record_event` & `EventLog.record_email_change` will now be triggered in `UserUpdate#record_update` & `UserUpdate#record_email_change_and_notify` respectively, I think it makes complete sense and it may even have been an oversight that they weren't called when the event logging was originally added. Other notes: * The links to "Resend confirmation email" or "Cancel change", or any other information relating to `User#unconfirmed_email` are not displayed when the user is an API user, because API users don't go through the email confirmation process. * The logic to determine whether the `email` param is permitted in `Users::EmailsController#user_params` was originally based on similar code to that in `ApiUsersController#api_user_params` i.e. using `UserParameterSanitiser` and it takes the various `.permitted_user_params` methods on the `Roles::Base` subclasses into account. * The `title_caption` is now different depending on whether the user is an API user or not. * The breadcrumb & cancel links back to the edit user page are now different depending on whether the user is an API user or not. * The successful redirect path is now different depending on whether the user is an API user or not.
This ensures that we use predicate methods on an instance of the correct policy class, i.e. `UserPolicy` or `ApiUserPolicy`. Previously it was always using `UserPolicy` whether the user was an API user or not. Also although it shouldn't be possible to trigger requests to `Users::EmailsController#resend_email_change` or `#cancel_email_change` through the UI, this adds predicate methods to `ApiUserPolicy` for these actions to make them forbidden. I do slightly wonder whether the latter behaviour is truly an "authorization" concern, because really it's just that these operations should not exist for an `ApiUser`. However, I think that's a bit philosophical at this point.
This is another small step towards moving the "edit API user" page to use the GOV.UK Design System. Having adapted `Users::EmailsController` to work with API users in a previous commit, we can now safely link to it from the "edit API user" page. As I explained in that previous commit, I'm confident that `Users::EmailsController#update` calling `UserUpdate#call` does everything related to changing `ApiUser#name` that `ApiUsersController#update` used to do. In `ApiUsersController`, I've removed `api_user[:email]` from the permitted params for all actions other than the `create` action. I've changed various tests for `ApiUsersController#update` to use stubbing, because it's no longer possible to use either the `name` or `email` params. This is a bit clunky, but I felt as if it was a sensible step given that the action is still used by at least the `app/views/api_users/manage_permissions.html.erb` template and it _might_ still be possible for the user object to end up with validation errors. These tests and the displaying of validation errors in `app/views/api_users/edit.html.erb` can almost certainly be removed once we move the "Manage permissions" page to the new UI design. I've updated the `ManageApiUsersTest` integration test to work with the new UI design.
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 link is unchanged and it takes you to the existing access log page which is implemented by `UsersController#event_logs` and works for both users and API users. I've made a slight tweak to the `title_caption` to distinguish the context from when you are viewing log for a non-API user. c.f. 2afff6e
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 link is unchanged and it takes you to the existing suspend/unsuspend user page which is implemented by `SuspensionsController` and works for both users and API users. I've made a slight tweak to the `title_caption` to distinguish the context from when you are acting on a non-API user. c.f. e1c82a3
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. Note that I'm using `UsersHelper#status` to format the user's status to match the design. c.f. this commit [2]. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: 1094739
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 made the controller test assertions robust against the markup generated by the Summary List component [1]. c.f. this commit [2]. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: c8fa23a
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 made the controller test assertions robust against the markup generated by the Summary List component [1]. c.f. this commit [2]. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: 9cd3dcc
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. c.f. this commit [1]. [1]: 9e98c89
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. The new design doesn't include this timestamp.
I think this is more idiomatic; or at least more consistent with what we're doing in other controllers.
Now that the `app/views/api_users/edit.html.erb` page no longer contains a form submitting to `ApiUsersController#update`, it makes more sense to change that action to re-render the `app/views/api_users/manage_permissions.html.erb` template in the case of a validation error and display the validation errors there. I did something similar for non-API users in this commit [1]. [1]: 3d26433
c.f. this similar commit [1] for non-API users. [1]: 24fa57f
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]. c.f. this similar commit [2] for non-API users. [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: 1941d80
Trello: https://trello.com/c/bhPUU9qo 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 reused some of the `UsersHelper` methods that I extracted in the equivalent commit [2] for the edit API user page to generate the appropriate options for each of the items. 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. I've had to tweak a few of the assertions in the `ManageApiUsersTest` integration test: * The link text for the access log page has changed * Flash alert messages are rendered differently in the new layout [1]: https://components.publishing.service.gov.uk/component-guide/summary_list [2]: 8d16e6f [3]: https://components.publishing.service.gov.uk/component-guide/list
The `href` option on these `assert_select` calls wasn't having any effect, so they were only checking that a link with the given text was present not where it was linking to! Also there was a copy/paste error in the assertion for the "Change Email" link - it was looking for the wrong URL path.
This means we can make `Users::NamesController#load_user` a bit more idiomatic and it means the URL paths are a little more logical/hackable. I haven't changed the form field name, i.e. it's always "user[name]" rather than having it be "user[name]" for non-API users and "api_user[name]" for API users, because I thought that was an unnecessary complication, although obviously it's not so idiomatic. Having the controller support two different sets of routes does make the controller a bit more complicated, but I think it's probably worth it. If it gets any more complicated, it might be worth splitting it up into two controllers and removing the duplication some other way.
This means we can make `Users::EmailsController#load_user` a bit more idiomatic and it means the URL paths are a little more logical/hackable. I haven't changed the form field name, i.e. it's always "user[email]" rather than having it be "user[email]" for non-API users and "api_user[email]" for API users, because I thought that was an unnecessary complication, although obviously it's not so idiomatic. Having the controller support two different sets of routes does make the controller a bit more complicated, but I think it's probably worth it, especially because in this case it means we don't expose the `resend_email_change` & `cancel_email_change` actions for API users. If it gets any more complicated, it might be worth splitting it up into two controllers and removing the duplication some other way.
Ideally I'd have added most of these at the beginning of this PR and then added the ones relating to the different behaviour for API users at the point where the behaviour changed, but I think it's more effort than it's worth.
I'm planning to extract separate `api_user_params` methods for the `create` and `update` actions. Doing this first will make that easier.
As suggested by @chrisroos. I agree that this makes it clearer what's going on. Also it has highlighted a potential security issue which is that the form for the `create` action doesn't include permissions, but the `create` action itself allows the `supported_permission_ids` param to be supplied. I plan to address this in a separate commit.
This was possibly a minor a security issue, because previously it would've been possible to edit the "new API user" form in the browser (or to manually craft a POST request) to add permissions even though the form only includes fields for name & email.
floehopper
force-pushed
the
split-edit-api-user-page-and-move-top-to-design-system
branch
from
33bffd1
to
aae753f
Compare
floehopper
deleted the
split-edit-api-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/bhPUU9qo
This moves what was (prior to #2587) the top of the "edit API user" page to use the GOV.UK Design System along similar lines to what we did for the "edit user" page in #2567.
The new "edit API user" page is in three sections:
I've reused the "change name" & "change user" pages, although they had to be adapted for use with API users rather than web users. At the suggestion of @chrisroos, I've made the relevant controllers (
Users::NamesController&Users::EmailsController) available via routes under/api_usersas well as under/users. As we discussed at the time, I think we should probably use this approach more widely for web-user- & API-user-specific pages, but in an effort to keep control of the scope of this PR, I've only tackled the controllers directly related to this PR.I've also been able to reuse a bunch of methods in
UsersHelper.Although there are a lot of commits in this PR, many of them are small/mechanical preparatory refactorings intended to make the actual changeover relatively straightforward. The actual changeover happens in this commit.
Old "edit API user" page
New "edit API user" page
Adapted "change name" page
Adapted "change email" page