Merge pull request #3175 from alphagov/success-banner-2

Display success banner when adding or removing access to an application (take 2)

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

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
 

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

app/controllers/users/signin_permissions_controller.rb

@@ -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
 
@@ -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
 

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

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)

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 %>

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 %>

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 %>

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

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)
 

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

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)