Skip to content

Commit

Permalink
Merge pull request #3175 from alphagov/success-banner-2
Browse files Browse the repository at this point in the history
Display success banner when adding or removing access to an application (take 2)
  • Loading branch information
Gweaton authored Sep 23, 2024
2 parents f672b08 + 658aa6d commit bd6cf2d
Show file tree
Hide file tree
Showing 18 changed files with 237 additions and 28 deletions.
5 changes: 4 additions & 1 deletion app/controllers/account/permissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ def update
flash[:new_permission_name] = SupportedPermission.find(update_params[:new_permission_id]).name
redirect_to edit_account_application_permissions_path(@application)
else
flash[:application_id] = @application.id
flash[:success_alert] = {
message: "Permissions updated",
description: permissions_updated_description(@application.id),
}
redirect_to account_applications_path
end
end
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/account/signin_permissions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
class Account::SigninPermissionsController < ApplicationController
before_action :authenticate_user!

include ApplicationAccessHelper

def create
authorize [:account, Doorkeeper::Application], :grant_signin_permission?

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[:success_alert] = { message: "Access granted", description: access_granted_description(application.id) }
redirect_to account_applications_path
end

Expand All @@ -20,6 +23,7 @@ def destroy
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[:success_alert] = { message: "Access removed", description: access_removed_description(application.id) }
redirect_to account_applications_path
end

Expand Down
5 changes: 4 additions & 1 deletion app/controllers/users/permissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ def update
flash[:new_permission_name] = SupportedPermission.find(update_params[:new_permission_id]).name
redirect_to edit_user_application_permissions_path(@user, @application)
else
flash[:application_id] = @application.id
flash[:success_alert] = {
message: "Permissions updated",
description: permissions_updated_description(@application.id, @user),
}
redirect_to user_applications_path(@user)
end
end
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/users/signin_permissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ class Users::SigninPermissionsController < ApplicationController
before_action :set_user
before_action :set_application, except: [:create]

include ApplicationAccessHelper

def create
application = Doorkeeper::Application.not_api_only.find(params[:application_id])
authorize [{ application:, user: @user }], :grant_signin_permission?, policy_class: Users::ApplicationPolicy

params = { supported_permission_ids: @user.supported_permissions.map(&:id) + [application.signin_permission.id] }
UserUpdate.new(@user, params, current_user, user_ip_address).call

flash[:success_alert] = { message: "Access granted", description: access_granted_description(application.id, @user) }
redirect_to user_applications_path(@user)
end

Expand All @@ -23,6 +26,7 @@ def destroy
params = { supported_permission_ids: @user.supported_permissions.map(&:id) - [@application.signin_permission.id] }
UserUpdate.new(@user, params, current_user, user_ip_address).call

flash[:success_alert] = { message: "Access removed", description: access_removed_description(@application.id, @user) }
redirect_to user_applications_path(@user)
end

Expand Down
19 changes: 19 additions & 0 deletions app/helpers/application_access_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module ApplicationAccessHelper
def access_granted_description(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

def access_removed_description(application_id, user = current_user)
application = Doorkeeper::Application.find_by(id: application_id)
return nil unless application

return "Your access to #{application.name} has been removed." if user == current_user

"#{user.name}'s access to #{application.name} has been removed."
end
end
19 changes: 12 additions & 7 deletions app/helpers/application_permissions_helper.rb
Original file line number Diff line number Diff line change
@@ -1,26 +1,31 @@
module ApplicationPermissionsHelper
def message_for_success(application_id, user = current_user)
include ActionView::Helpers::TagHelper

def permissions_updated_description(application_id, user = current_user)
application = Doorkeeper::Application.find_by(id: application_id)
return nil unless application

additional_permissions = user.permissions_for(application).reject { |permission| permission == SupportedPermission::SIGNIN_NAME }

if additional_permissions.any?
prefix = user == current_user ? "You now have" : "#{user.name} now has"
paragraph = tag.p("#{prefix} the following permissions for #{application.name}:", class: "govuk-body")
paragraph = tag.p(
(user == current_user ? "You now have" : "#{user.name} now has") + " the following permissions for #{application.name}:",
class: "govuk-body",
)

list = tag.ul(class: "govuk-list govuk-list--bullet")
additional_permissions.map { |permission| list << tag.li(permission) }

paragraph + list
else
string = if user == current_user
"You can access #{application.name} but you do not have any additional permissions."
else
"#{user.name} can access #{application.name} but does not have any additional permissions."
end
paragraph = tag.p(string, class: "govuk-body")
list = nil
end

paragraph + list
tag.p(string, class: "govuk-body")
end
end

def notice_about_non_delegatable_permissions(current_user, application, other_grantee = nil)
Expand Down
6 changes: 3 additions & 3 deletions app/views/account/applications/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
})
%>
<% if flash[:application_id] %>
<% if flash[:success_alert] %>
<% content_for(:custom_alerts) do %>
<%= render "govuk_publishing_components/components/success_alert", {
message: "Permissions updated",
description: message_for_success(flash[:application_id]),
message: flash[:success_alert]["message"],
description: sanitize(flash[:success_alert]["description"]),
} %>
<% end %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/api_users/applications/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
<% content_for(:custom_alerts) do %>
<%= render "govuk_publishing_components/components/success_alert", {
message: "Permissions updated",
description: message_for_success(flash[:application_id], @api_user),
description: permissions_updated_description(flash[:application_id], @api_user),
} %>
<% end %>
<% end %>
Expand Down
6 changes: 3 additions & 3 deletions app/views/users/applications/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
})
%>
<% if flash[:application_id] %>
<% if flash[:success_alert] %>
<% content_for(:custom_alerts) do %>
<%= render "govuk_publishing_components/components/success_alert", {
message: "Permissions updated",
description: message_for_success(flash[:application_id], @user),
message: flash[:success_alert]["message"],
description: sanitize(flash[:success_alert]["description"]),
} %>
<% end %>
<% end %>
Expand Down
10 changes: 8 additions & 2 deletions test/controllers/account/permissions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class Account::PermissionsControllerTest < ActionController::TestCase
assert_same_elements [application.signin_permission, new_permission], current_user.supported_permissions
end

should "assign the application id to the application_id flash" do
should "assign the success alert hash to flash" do
application = create(:application)
old_permission = create(:supported_permission, application:)
new_permission = create(:supported_permission, application:)
Expand All @@ -281,9 +281,15 @@ class Account::PermissionsControllerTest < ActionController::TestCase
)
sign_in user

Account::PermissionsController
.any_instance
.expects(:permissions_updated_description)
.with(application.id).returns("Updated some permissions for myself")

patch :update, params: { application_id: application, application: { supported_permission_ids: [new_permission.id] } }

assert_equal application.id, flash[:application_id]
expected = { message: "Permissions updated", description: "Updated some permissions for myself" }
assert_equal expected, flash[:success_alert]
end

context "when current_permission_ids and new_permission_id are provided instead of supported_permission_ids" do
Expand Down
49 changes: 49 additions & 0 deletions test/controllers/account/signin_permissions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,30 @@ class Account::SigninPermissionsControllerTest < ActionController::TestCase
post :create, params: { application_id: application.id }
end
end

should "assign the success alert hash to flash" do
current_user = create(:admin_user)
sign_in current_user

application = create(:application)

stub_policy(
current_user,
Doorkeeper::Application,
policy_class: Account::ApplicationPolicy,
grant_signin_permission?: true,
)

Account::SigninPermissionsController
.any_instance
.expects(:access_granted_description)
.with(application.id).returns("Granted access to myself")

post :create, params: { application_id: application.id }

expected = { message: "Access granted", description: "Granted access to myself" }
assert_equal expected, flash[:success_alert]
end
end

context "#delete" do
Expand All @@ -42,6 +66,31 @@ class Account::SigninPermissionsControllerTest < ActionController::TestCase
end

context "#destroy" do
should "assign the success alert hash to flash" do
current_user = create(:admin_user)
sign_in current_user

application = create(:application)
current_user.grant_application_signin_permission(application)

stub_policy(
current_user,
application,
policy_class: Account::ApplicationPolicy,
remove_signin_permission?: true,
)

Account::SigninPermissionsController
.any_instance
.expects(:access_removed_description)
.with(application.id).returns("Removed access from myself")

delete :destroy, params: { application_id: application.id }

expected = { message: "Access removed", description: "Removed access from myself" }
assert_equal expected, flash[:success_alert]
end

should "prevent unauthenticated users" do
application = create(:application)

Expand Down
10 changes: 8 additions & 2 deletions test/controllers/users/permissions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ class Users::PermissionsControllerTest < ActionController::TestCase
assert_same_elements [new_permission, application.signin_permission], user.supported_permissions
end

should "assign the application id to the application_id flash" do
should "assign the success alert message to flash" do
application = create(:application)
old_permission = create(:supported_permission, application:)
new_permission = create(:supported_permission, application:)
Expand All @@ -385,9 +385,15 @@ class Users::PermissionsControllerTest < ActionController::TestCase
edit_permissions?: true,
)

Users::PermissionsController
.any_instance
.expects(:permissions_updated_description)
.with(application.id, user).returns("Updated some permissions for another user")

patch :update, params: { user_id: user, application_id: application, application: { supported_permission_ids: [new_permission.id] } }

assert_equal application.id, flash[:application_id]
expected = { message: "Permissions updated", description: "Updated some permissions for another user" }
assert_equal expected, flash[:success_alert]
end

context "when current_permission_ids and new_permission_id are provided instead of supported_permission_ids" do
Expand Down
51 changes: 51 additions & 0 deletions test/controllers/users/signin_permissions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,31 @@ class Users::SigninPermissionsControllerTest < ActionController::TestCase
assert_redirected_to user_applications_path(user)
end

should "assign the success alert hash to flash" do
current_user = create(:admin_user)
sign_in current_user

user = create(:user)
application = create(:application)

stub_policy(
current_user,
{ application:, user: },
policy_class: Users::ApplicationPolicy,
grant_signin_permission?: true,
)

Users::SigninPermissionsController
.any_instance
.expects(:access_granted_description)
.with(application.id, user).returns("Granted access to another user")

post :create, params: { user_id: user, application_id: application.id }

expected = { message: "Access granted", description: "Granted access to another user" }
assert_equal expected, flash[:success_alert]
end

should "prevent unauthenticated users" do
user = create(:user)
application = create(:application)
Expand Down Expand Up @@ -249,6 +274,32 @@ class Users::SigninPermissionsControllerTest < ActionController::TestCase
assert_redirected_to user_applications_path(user)
end

should "assign the success alert hash to flash" do
current_user = create(:admin_user)
sign_in current_user

user = create(:user)
application = create(:application)
user.grant_application_signin_permission(application)

stub_policy(
current_user,
{ application:, user: },
policy_class: Users::ApplicationPolicy,
remove_signin_permission?: true,
)

Users::SigninPermissionsController
.any_instance
.expects(:access_removed_description)
.with(application.id, user).returns("Removed access from another user")

delete :destroy, params: { user_id: user, application_id: application.id }

expected = { message: "Access removed", description: "Removed access from another user" }
assert_equal expected, flash[:success_alert]
end

should "prevent unauthenticated users" do
user = create(:user)
application = create(:application)
Expand Down
Loading

0 comments on commit bd6cf2d

Please sign in to comment.