Merge pull request #3019 from alphagov/1184b-always-show-view-permiss…

…ions-link

Show view permissions link even when updatable

app/helpers/application_table_helper.rb

@@ -37,22 +37,23 @@ def users_applications_remove_access_link(application, user)
     end
   end
 
-  def account_applications_permissions_link(application)
-    if policy([:account, application]).edit_permissions?
-      update_permissions_link(application)
-    elsif policy([:account, application]).view_permissions?
-      view_permissions_link(application)
-    else
-      ""
-    end
+  def account_applications_permissions_links(application)
+    links = []
+
+    links << view_permissions_link(application) if policy([:account, application]).view_permissions?
+    links << update_permissions_link(application) if policy([:account, application]).edit_permissions?
+
+    safe_join(links)
   end
 
-  def users_applications_permissions_link(application, user)
+  def users_applications_permissions_links(application, user)
+    links = [view_permissions_link(application, user)]
+
     if policy(UserApplicationPermission.for(user:, supported_permission: application.signin_permission)).edit?
-      update_permissions_link(application, user)
-    else
-      view_permissions_link(application, user)
+      links << update_permissions_link(application, user)
     end
+
+    safe_join(links)
   end
 
   def api_users_applications_permissions_link(application, user)

app/views/account/applications/index.html.erb

@@ -37,7 +37,7 @@
       { text: application.name },
       { text: application.description },
       { text: wrap_links_in_actions_markup([
-        account_applications_permissions_link(application), 
+        account_applications_permissions_links(application), 
         account_applications_remove_access_link(application)
         ])
       },

app/views/users/applications/index.html.erb

@@ -42,7 +42,7 @@
       { text: application.name },
       { text: application.description },
       { text: wrap_links_in_actions_markup([
-          users_applications_permissions_link(application, @user),
+          users_applications_permissions_links(application, @user),
           users_applications_remove_access_link(application, @user)
         ])
       },

docs/access_and_permissions.md

@@ -29,16 +29,16 @@ In this section, the granter and grantee are the same user: this is about managi
 
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
-| None                    | ✅            | ✅             | ✅                | ❌                |
-| `signin`                | ✅            | ✅             | ✅                | ❌                |
-| Another permission      | ✅            | ✅             | ✅                | ❌                |
+| None                    | ✅            | ✅             | ✅                | ✅                |
+| `signin`                | ✅            | ✅             | ✅                | ✅                |
+| Another permission      | ✅            | ✅             | ✅                | ✅                |
 
 #### As a publishing manager
 
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
 | None                    | ❌            | ❌             | ❌                | ✅                |
-| `signin`                | ❌            | ✅             | ✅                | ❌                |
+| `signin`                | ❌            | ✅             | ✅                | ✅                |
 | Another permission      | ❌            | ❌             | ❌                | ✅                |
 
 ### Dependencies by route
@@ -131,17 +131,17 @@ In this section, the granter and grantee are different users: this is about mana
 
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
-| None                    | ✅            | ✅             | ✅                | ❌                |
-| `signin`                | ✅            | ✅             | ✅                | ❌                |
-| Another permission      | ✅            | ✅             | ✅                | ❌                |
+| None                    | ✅            | ✅             | ✅                | ✅                |
+| `signin`                | ✅            | ✅             | ✅                | ✅                |
+| Another permission      | ✅            | ✅             | ✅                | ✅                |
 
 ##### Without access to the app
 
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
-| None                    | ✅            | ✅             | ✅                | ❌                |
-| `signin`                | ✅            | ✅             | ✅                | ❌                |
-| Another permission      | ✅            | ✅             | ✅                | ❌                |
+| None                    | ✅            | ✅             | ✅                | ✅                |
+| `signin`                | ✅            | ✅             | ✅                | ✅                |
+| Another permission      | ✅            | ✅             | ✅                | ✅                |
 
 #### As a publishing manager
 
@@ -150,7 +150,7 @@ In this section, the granter and grantee are different users: this is about mana
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
 | None                    | ❌            | ❌             | ❌                | ✅                |
-| `signin`                | ✅            | ✅             | ✅                | ❌                |
+| `signin`                | ✅            | ✅             | ✅                | ✅                |
 | Another permission      | ❌            | ❌             | ❌                | ✅                |
 
 ##### Without access to the app

test/controllers/account/applications_controller_test.rb

@@ -126,28 +126,48 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
         end
 
         context "editing permissions" do
-          should "not display any permissions links when the app only has the signin permission" do
-            stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true
+          context "when the app only has the signin permission" do
+            should "only display a link to view permissions when authorised to view or edit permissions" do
+              stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true
 
-            get :index
+              get :index
+
+              assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
+              assert_select "a[href='#{account_application_permissions_path(@application)}']"
+            end
+
+            should "not display links to view or edit permissions when not authorised to view permissions" do
+              stub_policy @user, [:account, @application], view_permissions?: false, edit_permissions?: true
+
+              get :index
 
-            assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
-            assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0
+              assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
+              assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0
+            end
           end
 
           context "when the app has non-signin permissions" do
             setup { create(:supported_permission, application: @application) }
 
-            should "display a link to edit permissions when authorised to edit permissions" do
+            should "display links to view and edit permissions when authorised to view and edit permissions" do
               stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true
 
               get :index
 
+              assert_select "a[href='#{edit_account_application_permissions_path(@application)}']"
+              assert_select "a[href='#{account_application_permissions_path(@application)}']"
+            end
+
+            should "only display a link to edit permissions when authorised to edit but not view permissions" do
+              stub_policy @user, [:account, @application], view_permissions?: false, edit_permissions?: true
+
+              get :index
+
               assert_select "a[href='#{edit_account_application_permissions_path(@application)}']"
               assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0
             end
 
-            should "display a link to view permissions when not authorised to view but not edit permissions" do
+            should "only display a link to view permissions when not authorised to edit permissions" do
               stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: false
 
               get :index

test/controllers/users/applications_controller_test.rb

@@ -136,33 +136,34 @@ class Users::ApplicationsControllerTest < ActionController::TestCase
         context "editing permissions" do
           setup { @user_application_permission = stub_user_application_permission(@user, @application) }
 
-          should "not display any permissions links when the app only has the signin permission" do
+          should "only display a link to view permissions when the app only has the signin permission" do
             stub_policy @current_user, @user_application_permission, edit?: true
 
             get :index, params: { user_id: @user }
 
-            assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", count: 0
+            assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name", count: 0
+            assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name"
           end
 
           context "when the app has non-signin permissions" do
             setup { create(:supported_permission, application: @application) }
 
-            should "display a link to edit permissions when authorised to edit permissions" do
+            should "display links to view and edit permissions when authorised to edit permissions" do
               stub_policy @current_user, @user_application_permission, edit?: true
 
               get :index, params: { user_id: @user }
 
               assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name"
-              assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name", count: 0
+              assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name"
             end
 
-            should "display a link to view permissions when not authorised to edit permissions" do
+            should "only display a link to view permissions when not authorised to edit permissions" do
               stub_policy @current_user, @user_application_permission, edit?: false
 
               get :index, params: { user_id: @user }
 
-              assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name"
               assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name", count: 0
+              assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name"
             end
           end
         end

test/helpers/application_table_helper_test.rb

@@ -97,47 +97,70 @@ class ApplicationTableHelperTest < ActionView::TestCase
     end
   end
 
-  context "#account_applications_permissions_link" do
+  context "#account_applications_permissions_links" do
     setup do
       @user = build(:user)
       stubs(:current_user).returns(@user)
       @application = create(:application, with_supported_permissions: %w[permission])
     end
 
-    should "generate an update link when the user can edit permissions" do
-      stub_policy @user, [:account, @application], edit_permissions?: true
-      assert_includes account_applications_permissions_link(@application), "Update permissions"
+    should "generate both view and update links when the user can both view and edit permissions" do
+      stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true
+
+      result = account_applications_permissions_links(@application)
+
+      assert_includes result, "View permissions"
+      assert_includes result, "Update permissions"
     end
 
-    should "generate a view link when the user can view permissions" do
+    should "only generate a view link when the user can only view permissions" do
       stub_policy @user, [:account, @application], view_permissions?: true
-      assert_includes account_applications_permissions_link(@application), "View permissions"
+
+      result = account_applications_permissions_links(@application)
+
+      assert_includes result, "View permissions"
+      assert_not_includes result, "Update permissions"
+    end
+
+    should "only generate an update link when the user can only edit permissions" do
+      stub_policy @user, [:account, @application], edit_permissions?: true
+
+      result = account_applications_permissions_links(@application)
+
+      assert_not_includes result, "View permissions"
+      assert_includes result, "Update permissions"
     end
 
     should "return an empty string when the user can do neither" do
       stub_policy @user, [:account, @application]
-      assert account_applications_permissions_link(@application).empty?
+      assert account_applications_permissions_links(@application).empty?
     end
   end
 
-  context "#users_applications_permissions_link" do
+  context "#users_applications_permissions_links" do
     setup do
       @application = create(:application, with_supported_permissions: %w[permission])
       @grantee = create(:user)
     end
 
-    should "generate an update link when the user can edit permissions" do
+    should "generate both view and update links when the user can edit permissions" do
       granter = create(:superadmin_user)
       stubs(:current_user).returns(granter)
 
-      assert_includes users_applications_permissions_link(@application, @grantee), "Update permissions"
+      result = users_applications_permissions_links(@application, @grantee)
+
+      assert_includes result, "View permissions"
+      assert_includes result, "Update permissions"
     end
 
-    should "generate a view link when the user cannot edit permissions" do
+    should "only generate a view link when the user cannot edit permissions" do
       granter = create(:user)
       stubs(:current_user).returns(granter)
 
-      assert_includes users_applications_permissions_link(@application, @grantee), "View permissions"
+      result = users_applications_permissions_links(@application, @grantee)
+
+      assert_includes result, "View permissions"
+      assert_not_includes result, "Update permissions"
     end
   end