diff --git a/app/controllers/account/permissions_controller.rb b/app/controllers/account/permissions_controller.rb index 1bf11d95c..207541c16 100644 --- a/app/controllers/account/permissions_controller.rb +++ b/app/controllers/account/permissions_controller.rb @@ -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 diff --git a/app/controllers/account/signin_permissions_controller.rb b/app/controllers/account/signin_permissions_controller.rb index 968f7af87..47f78dd7a 100644 --- a/app/controllers/account/signin_permissions_controller.rb +++ b/app/controllers/account/signin_permissions_controller.rb @@ -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 @@ -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 diff --git a/app/controllers/users/permissions_controller.rb b/app/controllers/users/permissions_controller.rb index 562b9607a..509b47061 100644 --- a/app/controllers/users/permissions_controller.rb +++ b/app/controllers/users/permissions_controller.rb @@ -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 diff --git a/app/controllers/users/signin_permissions_controller.rb b/app/controllers/users/signin_permissions_controller.rb index 2bca2d42b..39fc9a2ea 100644 --- a/app/controllers/users/signin_permissions_controller.rb +++ b/app/controllers/users/signin_permissions_controller.rb @@ -3,6 +3,8 @@ 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 @@ -10,6 +12,7 @@ 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[:success_alert] = { message: "Access granted", description: access_granted_description(application.id, @user) } redirect_to user_applications_path(@user) end @@ -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 diff --git a/app/helpers/application_access_helper.rb b/app/helpers/application_access_helper.rb new file mode 100644 index 000000000..1dc1b7d42 --- /dev/null +++ b/app/helpers/application_access_helper.rb @@ -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 diff --git a/app/helpers/application_permissions_helper.rb b/app/helpers/application_permissions_helper.rb index 4be9e8351..b8d75625d 100644 --- a/app/helpers/application_permissions_helper.rb +++ b/app/helpers/application_permissions_helper.rb @@ -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) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index e8623ec64..334572e10 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -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 %> diff --git a/app/views/api_users/applications/index.html.erb b/app/views/api_users/applications/index.html.erb index 44d36ae89..6f654957a 100644 --- a/app/views/api_users/applications/index.html.erb +++ b/app/views/api_users/applications/index.html.erb @@ -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 %> diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb index 5f63b19d9..d230a79c0 100644 --- a/app/views/users/applications/index.html.erb +++ b/app/views/users/applications/index.html.erb @@ -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 %> diff --git a/test/controllers/account/permissions_controller_test.rb b/test/controllers/account/permissions_controller_test.rb index 8ae4a9fa9..9c3076a3b 100644 --- a/test/controllers/account/permissions_controller_test.rb +++ b/test/controllers/account/permissions_controller_test.rb @@ -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:) @@ -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 diff --git a/test/controllers/account/signin_permissions_controller_test.rb b/test/controllers/account/signin_permissions_controller_test.rb index 91c7e893c..eef99fd50 100644 --- a/test/controllers/account/signin_permissions_controller_test.rb +++ b/test/controllers/account/signin_permissions_controller_test.rb @@ -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 @@ -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) diff --git a/test/controllers/users/permissions_controller_test.rb b/test/controllers/users/permissions_controller_test.rb index 387df6538..f3f59d93b 100644 --- a/test/controllers/users/permissions_controller_test.rb +++ b/test/controllers/users/permissions_controller_test.rb @@ -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:) @@ -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 diff --git a/test/controllers/users/signin_permissions_controller_test.rb b/test/controllers/users/signin_permissions_controller_test.rb index f4c1b4989..3c7eb140d 100644 --- a/test/controllers/users/signin_permissions_controller_test.rb +++ b/test/controllers/users/signin_permissions_controller_test.rb @@ -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) @@ -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) diff --git a/test/helpers/application_access_helper_test.rb b/test/helpers/application_access_helper_test.rb new file mode 100644 index 000000000..0b1356d46 --- /dev/null +++ b/test/helpers/application_access_helper_test.rb @@ -0,0 +1,50 @@ +require "test_helper" + +class ApplicationAccessHelperTest < ActionView::TestCase + setup do + @application = create(:application, name: "Whitehall") + stubs(:current_user).returns(create(:user)) + end + + context "#access_granted_description" do + context "when the user is granting themself access" 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_description(@application) + end + end + + context "when the user is granting another access" do + should "return a message informing them that the other user has access to an application" do + user = create(:user, name: "Gerald") + assert_equal "Gerald has been granted access to Whitehall.", access_granted_description(@application, user) + end + end + + context "when the application does not exist" do + should "return nil" do + assert_nil access_granted_description(:made_up_id) + end + end + end + + context "#access_removed_description" do + context "when the user is removing their own access" do + should "return a message informing them that they no longer have access to the application" do + assert_equal "Your access to Whitehall has been removed.", access_removed_description(@application) + end + end + + context "when the user is removing another's access" do + should "return a message informing them that the other user no longer has access to the application" do + user = create(:user, name: "Gerald") + assert_equal "Gerald's access to Whitehall has been removed.", access_removed_description(@application, user) + end + end + + context "when the application does not exist" do + should "return nil" do + assert_nil access_removed_description(:made_up_id) + end + end + end +end diff --git a/test/helpers/application_permissions_helper_test.rb b/test/helpers/application_permissions_helper_test.rb index fc0cdc8d4..2fdb3f5fe 100644 --- a/test/helpers/application_permissions_helper_test.rb +++ b/test/helpers/application_permissions_helper_test.rb @@ -1,7 +1,7 @@ require "test_helper" class ApplicationPermissionsHelperTest < ActionView::TestCase - context "#message_for_success" do + context "#permissions_updated_description" do setup do @application = create(:application, name: "Whitehall", with_non_delegatable_supported_permissions: ["Permission 1"]) user = create(:user, with_permissions: { @application => ["Permission 1", SupportedPermission::SIGNIN_NAME] }) @@ -9,20 +9,20 @@ class ApplicationPermissionsHelperTest < ActionView::TestCase end should "include the application name in the message" do - assert_includes message_for_success(@application.id), "You now have the following permissions for Whitehall" + assert_includes permissions_updated_description(@application.id), "You now have the following permissions for Whitehall" end should "include the users permissions in the message" do - assert_includes message_for_success(@application.id), "Permission 1" + assert_includes permissions_updated_description(@application.id), "Permission 1" end should "not include the signin permission in the message" do - assert_not_includes message_for_success(@application.id), "signin" + assert_not_includes permissions_updated_description(@application.id), "signin" end context "when the application does not exist" do should "return nil" do - assert_nil message_for_success(:made_up_id) + assert_nil permissions_updated_description(:made_up_id) end end @@ -33,7 +33,7 @@ class ApplicationPermissionsHelperTest < ActionView::TestCase end should "indicate that the user has no additional permissions" do - assert_includes message_for_success(@application.id), "You can access Whitehall but you do not have any additional permissions." + assert_includes permissions_updated_description(@application.id), "You can access Whitehall but you do not have any additional permissions." end end @@ -43,7 +43,7 @@ class ApplicationPermissionsHelperTest < ActionView::TestCase end should "include the application name in the message" do - assert_includes message_for_success(@application.id, @user), "user-name now has the following permissions for Whitehall" + assert_includes permissions_updated_description(@application.id, @user), "user-name now has the following permissions for Whitehall" end end @@ -53,7 +53,7 @@ class ApplicationPermissionsHelperTest < ActionView::TestCase end should "indicate that the user has no additional permissions" do - assert_includes message_for_success(@application.id, @user), "user-name can access Whitehall but does not have any additional permissions." + assert_includes permissions_updated_description(@application.id, @user), "user-name can access Whitehall but does not have any additional permissions." end end end diff --git a/test/support/granting_access_helpers.rb b/test/support/granting_access_helpers.rb index 063446fb3..c55a6473f 100644 --- a/test/support/granting_access_helpers.rb +++ b/test/support/granting_access_helpers.rb @@ -29,6 +29,10 @@ 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_alert_description = 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_alert_description) end def refute_grant_access(application) diff --git a/test/support/removing_access_helpers.rb b/test/support/removing_access_helpers.rb index a8b674788..228d2fa3b 100644 --- a/test/support/removing_access_helpers.rb +++ b/test/support/removing_access_helpers.rb @@ -30,6 +30,10 @@ def assert_remove_access(application, grantee, grantee_is_self: false) assert apps_without_access_table.has_content?(application.name) assert_not grantee.has_access_to?(application) + + success_alert_description = grantee_is_self ? "Your access to #{application.name} has been removed." : "#{grantee.name}'s access to #{application.name} has been removed." + assert_flash_content("Access removed") + assert_flash_content(success_alert_description) end def refute_remove_access(application) diff --git a/test/support/updating_permissions_helpers.rb b/test/support/updating_permissions_helpers.rb index 4052a94bf..29bc1994b 100644 --- a/test/support/updating_permissions_helpers.rb +++ b/test/support/updating_permissions_helpers.rb @@ -35,6 +35,7 @@ def assert_update_permissions(application, grantee, grant: [], revoke: []) click_button "Update permissions" + assert_flash_content("Permissions updated") assert_flash_content(grant.map(&:name)) grant.each { |new_permission| assert grantee.has_permission?(new_permission) }