Display success banner when adding or removing access to an application #3171
Conversation
Gweaton
force-pushed
the
add-success-banner
branch
2 times, most recently
from
6128128
to
cb7ddb2
Compare
This was a lot more complex than I'd hoped due to the difficulties in cleanly distinguishing between a user having the Signin permission granted vs having all permissions other than the Signin permission removed. I hope this is reasonably clear and duplication-free. I'd rather have avoided passing the `granting_access` flag if possible and opting for something slightly more extendable in future, but it won out over the following alternative approaches: 1. Performing a lot of this logic in the view - harder to test, a bit too much duplication and adding too much logic to a rendering template. 2. Refactoring the `ApplicationPermissionsHelper` to support a new path for granting access - this had a lot of fairly complex branching logic in it and felt fairly geared towards "permissions" rather than "access" so I've opted against extending it further. I've opted against writing controller tests for this behaviour. Although we have some where we test for the presence of `flash[:application_id]`, this feels a bit over the top, and I think it's sufficiently covered with integration tests that are testing the view.
This takes a similar approach to the previous banner. However, I've had to perform a little more logic in the view here to allow us to re-use the success alert component.
Gweaton
force-pushed
the
add-success-banner
branch
from
cb7ddb2
to
fe41de4
Compare
| else | ||
| { | ||
| message: "Permissions updated", | ||
| description: message_for_success(application_id, user), |
There was a problem hiding this comment.
Could we possibly change the name of message_for_success to something like permissions_updated_message to make this more readable without much context (and more in line with the better name you chose: access_granted_message)?
| def access_removed_params(application_id, user: current_user) | ||
| { | ||
| message: "Access removed", | ||
| description: access_removed_message(application_id, user), | ||
| } | ||
| end |
There was a problem hiding this comment.
I'm not sure I see the distinction between the two methods in this file. The first one includes granted in the name but it would also apply if permissions have been removed (or even some granted, some removed), so I'm not sure the granted/removed distinction holds?
Could the method added in the previous commit maybe be rejigged here to accommodate the three options (e.g. adding removing_access: to the method signature), or a clearer distinction made? This would extend the conditional logic in the helper a little but allow the view to stay 'simple'
| end | ||
|
|
||
| context "when the user is granting another access" do | ||
| should "return a message informing them that the other user have access to an application" do |
| @@ -29,6 +29,9 @@ def assert_grant_access(application, grantee, grantee_is_self: false) | |||
|
|
|||
| assert app_with_access_table.has_content?(application.name) | |||
| assert grantee.has_access_to?(application) | |||
| success_banner_caption = grantee_is_self ? "You have been granted access to #{application.name}." : "#{grantee.name} has been granted access to #{application.name}." | |||
There was a problem hiding this comment.
Looks like _caption was copied from table_caption? That's referring to the table caption option. I guess this should be _description (to match the actual option) or just _body?
Likewise in test/support/removing_access_helpers.rb in the next commit
|
|
||
| context "#access_removed_message" do | ||
| context "when the user is removing their own access" do | ||
| should "return a message informing them that they have access to an application" do |
There was a problem hiding this comment.
The should descriptions need updating for this method
|
Closing in favour of #3175 |
Trello card
Changes in this PR
We previously displayed a success alert banner when one's own or another user's permissions were updated, but didn't show anything when granting access or removing it. We now display the same banner with different messaging when a user's access is granted or removed.
This was a little more involved than I'd expected due to the challenge in differentiating cleanly between a user having all their permissions removed and seeing one message (i.e. they just have the "Signin" permission) and a user being granted access to an app (i.e. also just having the "Signin" permission) and seeing another message in the banner. Because of this, I've had to make use of some additional flash variables and flags to method arguments.
The API Users journey didn't need updating here, as we already show flash messages when an API token is created or revoked, as well as when permissions are updated.
Screenshots
Granting own access
Removing own access
Granting another user access
Removing another user's access
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.