Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add separate page for editing another user's name #2497

Merged
merged 8 commits into from
Nov 8, 2023

Conversation

floehopper
Copy link
Contributor

@floehopper floehopper commented Nov 8, 2023

Trello: https://trello.com/c/uZD2I9dj & https://trello.com/c/DjdHEqxG

This splits out the editing of another user's name 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 name" page uses the GOV.UK Design System, but the "edit user" page still does not.

In making this change I have fixed a small bug that was originally present in the "edit user" page but had moved to the new "edit user name" page.

In my use of the error_summary component I have also brought the display of validation errors more into line with this Design System guidance.

New "Change" link on "edit user" page

Screenshot 2023-11-08 at 11 19 43

New "edit user name" page

Screenshot 2023-11-08 at 12 11 34

New "edit user name" page with validation error

Screenshot 2023-11-08 at 12 12 51

Planned design for the "edit user" page (not part of this PR)

Screenshot 2023-11-08 at 12 08 47

This was missed in #2494 & #2495. The commits are best viewed with the
`--ignore-all-space` git diff option.
Trello: https://trello.com/c/uZD2I9dj

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 first of them.

The new `app/views/users/names/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` and I haven't yet changed it to
use the Design System. I plan to do that in a subsequent commit.

The new `Users::NamesController` 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::NamesController`, because that still
seems to make sense.

I've tried to add tests in `Users::NamesControllerTest` 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`.

Rather than creating a combinatorial explosion of tests in
`Users::NamesControllerTest` 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.

I've added a single integration test just to cover an obvious "happy
path" usage of the functionality.
@floehopper floehopper force-pushed the add-edit-user-name-page branch from 58b45be to 051a4e0 Compare November 8, 2023 10:27
@floehopper floehopper changed the title Add edit user name page Add separate page for editing user's name Nov 8, 2023
@floehopper floehopper force-pushed the add-edit-user-name-page branch from 051a4e0 to 0a84c70 Compare November 8, 2023 11:57
@floehopper floehopper marked this pull request as ready for review November 8, 2023 12:09
Trello: https://trello.com/c/uZD2I9dj

This changes the edit user name page to use the Design System and makes
a few other tweaks to bring it into line with the latest designs.

I've introduced a new `title_caption` `content_for` block in the
`app/views/layouts/admin_layout.html.erb` layout template to support the
new "Manage other users" caption which appears in this type of page in
the latest designs. It feels as if this area of the layout is getting a
bit complicated and it might be better to push a chunk of the relevant
markup down into the individual templates even at the cost of some
duplication. The latter could probably be better avoided by other means,
e.g. helper methods, presenter objects, etc.

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.

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 `input` component with
`error_items` set appropriately, so that any relevant errors are
displayed alongside the `name` field and the field itself is highlighted
in red as per this Design System guidance [1]. I've enhanced a test in
`Users::NamesControllerTest` to cover this new behaviour.

The button text has changed from "Update User" -> "Change name" to match
other similar pages.

[1]: https://design-system.service.gov.uk/patterns/validation/#how-to-tell-the-user-about-validation-errors
I found the `UserParameterSanitiser` and associated logic that we
brought from the `UsersController` quite confusing somewhat overkill for
this controller, especially as the `.permitted_user_params` methods on
the `Roles::Base` subclasses all return an array including `:name` which
is the only parameter we're interested in in this controller.

I did consider simplifying `Users::NamesController#user_params` furhter,
i.e. to just the idiomatic `params.require(:user).permit(:name)` which
is actually all we need. However, since it's possible one or more of the
`.permitted_user_params` methods on the `Roles::Base` subclasses might
change and stop including `:name`, I thought I ought to handle that
eventuality.

I think ideally we'd make use of Pundit's strong parameters
functionality [1] and move this logic out of the `Roles::Base`
subclasses and `UserParameterSanitiser` into the relevant policy class.
However, that's a job for another day!

[1]: https://github.com/varvet/pundit/blob/4d8cdf1c10058c12f2c175f30b408f5d5532a00b/README.md#strong-parameters
To futher simplify `Users::NamesController#user_params`.
Only including the few bits we need.

We were hardly using any of the logic in `UserUpdate#call` and so I
think it's simpler and more idiomatic to include the logic directly in
the controller action.

In general I think there are better ways to avoid the kind of
duplication that `UserUpdate` was intended to avoid.
Trello: https://trello.com/c/uwgUF9hF

This fixes a bug that was originally present on the edit user page, but
made its way into the recently added edit user name page. The bug is no
longer a problem on the user edit page, because you can no longer change
the user's name on that page.

The problem is best described by the steps to reproduce it when it was
on the user edit page:

* As an admin of some sort edit a user by clicking on a link from the
  table on the users index page
* See that the page title & heading is something like: "Edit “James
  Mead”"
* Change the name field to be blank
* Click the "Update user" button at the bottom of the page
* See that the page title and heading changes to "Edit “”", i.e. it uses
  the new name value and not the old one.
@floehopper floehopper force-pushed the add-edit-user-name-page branch from 0a84c70 to c1a211b Compare November 8, 2023 12:13
@chrislo chrislo self-assigned this Nov 8, 2023
Copy link
Contributor

@chrislo chrislo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me and I think moving a field at a time is a useful way of keeping the changes manageable. I checked out the branch locally and tested the error states and navigation on a couple of browsers and screen widths.

@floehopper
Copy link
Contributor Author

Thanks, @chrislo! 🙏

@floehopper floehopper merged commit bd20483 into main Nov 8, 2023
10 checks passed
@floehopper floehopper deleted the add-edit-user-name-page branch November 8, 2023 15:05
floehopper added a commit that referenced this pull request Nov 9, 2023
Trello: https://trello.com/c/DjdHEqxG

This was a mistake in #2497.

When the update completes successfully, the admin should be taken back
to the edit user page for the user in question.

Previously they were redirected to the *non-existent* path for a
non-existent `UsersController#show` action which was somehow (🤷)
being handled by `UsersController#edit` as I intended!
@floehopper
Copy link
Contributor Author

@chrisroos

I just noticed that I missed this test:

should "push changes out to apps" do
another_user = create(:user, name: "Old Name")
PermissionUpdater.expects(:perform_on).with(another_user).once
put :update, params: { id: another_user.id, user: { name: "New Name" } }
end

And hence this behaviour:

def perform_permissions_update
PermissionUpdater.perform_on(user)
end

When I inlined UserUpdate into Users:NamesController#update in this PR. I don't really understand why we'd want to send a permission update out to the users apps just because we've changed their name - probably just because this has got lumped into the UserUpdate mess. Anyway, I don't think it's a big deal, but thought I'd highlight it just in case something comes up about it next week.

floehopper added a commit that referenced this pull request Nov 9, 2023
This was a copy & paste error in #2497. I thought it was worth fixing
it, because it's pretty much saying the opposite of what the assertion
is testing.
floehopper added a commit that referenced this pull request Nov 9, 2023
This was a copy & paste error in #2497. I thought it was worth fixing
it, because it's pretty much saying the opposite of what the assertion
is testing.
floehopper added a commit that referenced this pull request Nov 10, 2023
This was a mistake in #2497. There is no such input type as "name" - it
was a copy & paste error. In this case there's no need to specifiy a
type at all; we can just rely on the default "text" type for the `input`
component [1].

[1]: https://components.publishing.service.gov.uk/component-guide/input#specific_input_type
floehopper added a commit that referenced this pull request Nov 10, 2023
c.f. `redirect_to_account_page_if_acting_on_own_user` before action in
`UsersController`.

In this case it makes sense to redirect to the specific account page for
editing name.

I missed this in #2497.
@floehopper floehopper changed the title Add separate page for editing user's name Add separate page for editing another user's name Nov 10, 2023
@chrisroos
Copy link
Contributor

@chrisroos

I just noticed that I missed this test:

should "push changes out to apps" do
another_user = create(:user, name: "Old Name")
PermissionUpdater.expects(:perform_on).with(another_user).once
put :update, params: { id: another_user.id, user: { name: "New Name" } }
end

And hence this behaviour:

def perform_permissions_update
PermissionUpdater.perform_on(user)
end

When I inlined UserUpdate into Users:NamesController#update in this PR. I don't really understand why we'd want to send a permission update out to the users apps just because we've changed their name - probably just because this has got lumped into the UserUpdate mess. Anyway, I don't think it's a big deal, but thought I'd highlight it just in case something comes up about it next week.

I've been doing some digging and I think we might want to update the client app when the name changes, in order to retain the previous behaviour.

Prior to this change, when a user's name was updated in Signon it would also be updated in the client app (e.g. Whitehall) very soon after (triggered by the PermissionUpdater). After this change, I think the user's name will only be updated in the client app when they sign in again, or when the user stored in the session expires (default expiry is 20 hours) and the user is authenticated again by gds-sso.

It looks as though gds-sso has supported the updating of name and email, along with permissions, since it was first introduced in 2012. Signon was updated to use this new user update endpoint a few days later, and has also been sending name and email along with permissions, since then.

It feels as though the name of PermissionsUpdater is incorrect and that it should be something more like UserUpdater to indicate that it pushes user updates out to client apps.

I've captured the requirement for updating the name in client apps in this Trello card.

chrisroos pushed a commit that referenced this pull request Nov 16, 2023
c.f. `redirect_to_account_page_if_acting_on_own_user` before action in
`UsersController`.

In this case it makes sense to redirect to the specific account page for
editing name.

I missed this in #2497.
chrisroos added a commit that referenced this pull request Nov 16, 2023
Trello: https://trello.com/c/2t9zx0Aq

Prior to #2497 changing a user's name in Signon would queue a job to
push that update out to the applications the user has access to. This
commit reinstates the previous behaviour by using `UserUpdate` in
`Users::NamesController#update`, which in turn uses `PermissionUpdater`
to push updates to the user's apps.

I can't find any documentation that describes the changes that result in push
updates to a user's applications, but as far as I can see, changing a
user's name has resulted in a push update since the relevant
functionality was added to Signon[1] and gds-sso[2] in 2012 so it seems
reasonable that we retain this behaviour.

[1]: f339848
[2]: alphagov/gds-sso@8c0888c
chrisroos added a commit that referenced this pull request Nov 16, 2023
Trello: https://trello.com/c/2t9zx0Aq

Prior to #2497 changing a user's name in Signon would queue a job to
push that update out to the applications the user has access to. This
commit reinstates the previous behaviour by using `UserUpdate` in
`Users::NamesController#update`, which in turn uses `PermissionUpdater`
to push updates to the user's apps.

I can't find any documentation that describes the changes that result in push
updates to a user's applications, but as far as I can see, changing a
user's name has resulted in a push update since the relevant
functionality was added to Signon[1] and gds-sso[2] in 2012 so it seems
reasonable that we retain this behaviour.

[1]: f339848
[2]: alphagov/gds-sso@8c0888c
chrisroos added a commit that referenced this pull request Nov 16, 2023
Trello: https://trello.com/c/2t9zx0Aq

Prior to #2497 changing a user's name in Signon would queue a job to
push that update out to the applications the user has access to. This
commit reinstates the previous behaviour by using `UserUpdate` in
`Users::NamesController#update`, which in turn uses `PermissionUpdater`
to push updates to the user's apps.

I can't find any documentation that describes the changes that result in push
updates to a user's applications, but as far as I can see, changing a
user's name has resulted in a push update since the relevant
functionality was added to Signon[1] and gds-sso[2] in 2012 so it seems
reasonable that we retain this behaviour.

Note that `UserUpdate#record_update`[3] records an `EventLog` event so
I've removed this from `Users::NamesController#update`.

[1]: f339848
[2]: alphagov/gds-sso@8c0888c
[3]: https://github.com/alphagov/signon/blob/16299a28ed46d12a8b0cb61c1cf755166317057c/app/services/user_update.rb#L72
chrisroos added a commit that referenced this pull request Nov 20, 2023
Trello: https://trello.com/c/2t9zx0Aq

Prior to #2497 changing a user's name in Signon would queue a job to
push that update out to the applications the user has access to. This
commit reinstates the previous behaviour by using `UserUpdate` in
`Users::NamesController#update`, which in turn uses `PermissionUpdater`
to push updates to the user's apps.

I can't find any documentation that describes the changes that result in push
updates to a user's applications, but as far as I can see, changing a
user's name has resulted in a push update since the relevant
functionality was added to Signon[1] and gds-sso[2] in 2012 so it seems
reasonable that we retain this behaviour.

Note that `UserUpdate#record_update`[3] records an `EventLog` event so
I've removed this from `Users::NamesController#update`.

[1]: f339848
[2]: alphagov/gds-sso@8c0888c
[3]: https://github.com/alphagov/signon/blob/16299a28ed46d12a8b0cb61c1cf755166317057c/app/services/user_update.rb#L72
floehopper added a commit that referenced this pull request Nov 21, 2023
The `#user_name_path` & `#user_email_path` route helper methods should
require a `user` argument. However, the argument was not provided in
either of the PRs where these forms were added (#2497 & #2509).

The assertions in `Users::NamesControllerTest` &
`Users::EmailsControllerTest` were already comparing with the correct
values and *somehow* the calls to `#user_name_path` & `#user_email_path`
without any arguments were magically returning the correct value, even
though when I try them in a Rails console, I get an
`ActionController::UrlGenerationError`. So this mistake wasn't actually
causing any problems.
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants