Skip to content

Commit

Permalink
Add success banner when users are granted access to an app
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Gweaton committed Sep 16, 2024
1 parent 8e2ccfa commit adb7c9c
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 9 deletions.
2 changes: 2 additions & 0 deletions app/controllers/account/signin_permissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ def create
params = { supported_permission_ids: current_user.supported_permissions.map(&:id) + [application.signin_permission.id] }
UserUpdate.new(current_user, params, current_user, user_ip_address).call

flash[:application_id] = application.id
flash[:granting_access] = true
redirect_to account_applications_path
end

Expand Down
2 changes: 2 additions & 0 deletions app/controllers/users/signin_permissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ def create
params = { supported_permission_ids: @user.supported_permissions.map(&:id) + [application.signin_permission.id] }
UserUpdate.new(@user, params, current_user, user_ip_address).call

flash[:application_id] = application.id
flash[:granting_access] = true
redirect_to user_applications_path(@user)
end

Expand Down
10 changes: 10 additions & 0 deletions app/helpers/application_access_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module ApplicationAccessHelper
def access_granted_message(application_id, user = current_user)
application = Doorkeeper::Application.find_by(id: application_id)
return nil unless application

return "You have been granted access to #{application.name}." if user == current_user

"#{user.name} has been granted access to #{application.name}."
end
end
15 changes: 15 additions & 0 deletions app/helpers/success_alert_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module SuccessAlertHelper
def access_and_permissions_granted_params(application_id, granting_access:, user: current_user)
if granting_access
{
message: "Access granted",
description: access_granted_message(application_id, user),
}
else
{
message: "Permissions updated",
description: message_for_success(application_id, user),
}
end
end
end
7 changes: 3 additions & 4 deletions app/views/account/applications/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@
<% if flash[:application_id] %>
<% content_for(:custom_alerts) do %>
<%= render "govuk_publishing_components/components/success_alert", {
message: "Permissions updated",
description: message_for_success(flash[:application_id]),
} %>
<%= render "govuk_publishing_components/components/success_alert",
access_and_permissions_granted_params(flash[:application_id], granting_access: flash[:granting_access])
%>
<% end %>
<% end %>
Expand Down
8 changes: 3 additions & 5 deletions app/views/users/applications/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@
<% if flash[:application_id] %>
<% content_for(:custom_alerts) do %>
<%= render "govuk_publishing_components/components/success_alert", {
message: "Permissions updated",
description: message_for_success(flash[:application_id], @user),
} %>
<% end %>
<%= render "govuk_publishing_components/components/success_alert",
access_and_permissions_granted_params(flash[:application_id], granting_access: flash[:granting_access], user: @user)
%> <% end %>
<% end %>
<%= render "components/table", {
Expand Down
29 changes: 29 additions & 0 deletions test/helpers/application_access_helper_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
require "test_helper"

class ApplicationAccessHelperTest < ActionView::TestCase
setup do
@application = create(:application, name: "Whitehall")
stubs(:current_user).returns(create(:user))
end

context "#access_granted_message" do
context "when the user is setting their own permissions" do
should "return a message informing them that they have access to an application" do
assert_equal "You have been granted access to Whitehall.", access_granted_message(@application)
end
end

context "when the user is setting another's permissions" do
should "return a message informing them that the other user have access to an application" do
user = create(:user, name: "Gerald")
assert_equal "Gerald has been granted access to Whitehall.", access_granted_message(@application, user)
end
end

context "when the application does not exist" do
should "return nil" do
assert_nil access_granted_message(:made_up_id)
end
end
end
end
28 changes: 28 additions & 0 deletions test/helpers/success_alert_helper_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
require "test_helper"

class SuccessAlertHelperTest < ActionView::TestCase
context "#access_and_permissions_granted_params" do
setup do
@application = create(:application)
stubs(:current_user).returns(create(:user))
end

context "when granting access" do
should "return success alert params with the `access_granted_message` text" do
stubs(:access_granted_message).returns("Granted access")

expected = { message: "Access granted", description: "Granted access" }
assert_equal expected, access_and_permissions_granted_params(@application.id, granting_access: true)
end
end

context "when updating permissions" do
should "return success alert params with the `message_for_success` text" do
stubs(:message_for_success).returns("Added permissions")

expected = { message: "Permissions updated", description: "Added permissions" }
assert_equal expected, access_and_permissions_granted_params(@application.id, granting_access: false)
end
end
end
end
3 changes: 3 additions & 0 deletions test/support/granting_access_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}."
assert_flash_content("Access granted")
assert_flash_content(success_banner_caption)
end

def refute_grant_access(application)
Expand Down

0 comments on commit adb7c9c

Please sign in to comment.