diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss
index e72aba116..0644b15f5 100644
--- a/app/assets/stylesheets/application.scss
+++ b/app/assets/stylesheets/application.scss
@@ -121,3 +121,16 @@ $show-button-space: calc($show-button-width + $show-button-left-margin);
padding-right: $show-button-space;
}
}
+
+.applications-table__remove_access_link {
+ width: auto;
+ min-width: 90px;
+}
+
+.applications-table__permissions-action-links a {
+ display: inline-block;
+
+ &:not(:last-child) {
+ margin-right: 1rem;
+ }
+}
diff --git a/app/helpers/application_table_helper.rb b/app/helpers/application_table_helper.rb
index 88b03638d..3dd86483a 100644
--- a/app/helpers/application_table_helper.rb
+++ b/app/helpers/application_table_helper.rb
@@ -1,10 +1,6 @@
module ApplicationTableHelper
include Pundit::Authorization
- def wrap_links_in_actions_markup(links)
- "#{links.join}
".html_safe
- end
-
def account_applications_grant_access_link(application)
if policy([:account, Doorkeeper::Application]).grant_signin_permission?
grant_access_link(application)
@@ -73,7 +69,12 @@ def grant_access_link(application, user = nil)
path,
class: "govuk-button govuk-!-margin-0",
data: { module: "govuk-button" },
- ) { button_or_link_content("Grant access", "to", application.name) }
+ ) do
+ button_or_link_content(
+ visible_text: "Grant",
+ visually_hidden_text: "access to #{application.name}",
+ )
+ end
end
def remove_access_link(application, user = nil)
@@ -85,9 +86,14 @@ def remove_access_link(application, user = nil)
link_to(
path,
- class: "govuk-button govuk-button--warning govuk-!-margin-0",
+ class: "govuk-button govuk-button--warning govuk-!-margin-0 applications-table__remove_access_link",
data: { module: "govuk-button" },
- ) { button_or_link_content("Remove access", "to", application.name) }
+ ) do
+ button_or_link_content(
+ visible_text: "Remove",
+ visually_hidden_text: "access to #{application.name}",
+ )
+ end
end
def view_permissions_link(application, user = nil)
@@ -97,7 +103,12 @@ def view_permissions_link(application, user = nil)
account_application_permissions_path(application)
end
- link_to(path, class: "govuk-link") { button_or_link_content("View permissions", "for", application.name) }
+ link_to(path, class: "govuk-link") do
+ button_or_link_content(
+ visible_text: "View",
+ visually_hidden_text: "permissions for #{application.name}",
+ )
+ end
end
def update_permissions_link(application, user = nil)
@@ -115,13 +126,18 @@ def update_permissions_link(application, user = nil)
edit_user_application_permissions_path(user, application)
end
- link_to(path, class: "govuk-link") { button_or_link_content("Update permissions", "for", application.name) }
+ link_to(path, class: "govuk-link") do
+ button_or_link_content(
+ visible_text: "Update",
+ visually_hidden_text: "permissions for #{application.name}",
+ )
+ end
end
- def button_or_link_content(visible_text, visually_hidden_join_word, application_name)
+ def button_or_link_content(visible_text:, visually_hidden_text:)
safe_join([
visible_text,
- content_tag(:span, " #{visually_hidden_join_word} #{application_name}", class: "govuk-visually-hidden"),
+ content_tag(:span, " #{visually_hidden_text.strip}", class: "govuk-visually-hidden"),
])
end
end
diff --git a/app/helpers/components/table_helper.rb b/app/helpers/components/table_helper.rb
index fe47237ec..8ef1c3c04 100644
--- a/app/helpers/components/table_helper.rb
+++ b/app/helpers/components/table_helper.rb
@@ -61,6 +61,7 @@ def header(str, opt = {})
def cell(str, opt = {}, &block)
classes = %w[govuk-table__cell]
+ classes << opt[:classes] if opt[:classes]
classes << "govuk-table__cell--#{opt[:format]}" if opt[:format]
classes << "govuk-table__cell--empty" unless str || block_given?
str ||= "Not set"
diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb
index 334572e10..5f0bee1ea 100644
--- a/app/views/account/applications/index.html.erb
+++ b/app/views/account/applications/index.html.erb
@@ -29,18 +29,16 @@
caption: "Apps you have access to",
head: [
{ text: "Name", classes: "govuk-!-width-one-quarter" },
- { text: "Description", classes: "govuk-!-width-one-third" },
- { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
+ { text: "Description", classes: "govuk-!-width-one-half" },
+ { text: "Access" },
+ { text: "Permissions" }
],
rows: @applications_with_signin.map do |application|
[
{ text: application.name },
{ text: application.description },
- { text: wrap_links_in_actions_markup([
- account_applications_permissions_links(application),
- account_applications_remove_access_link(application)
- ])
- },
+ { text: account_applications_remove_access_link(application) },
+ { text: account_applications_permissions_links(application), classes: "applications-table__permissions-action-links" }
]
end,
vertical_on_small_screen: true,
@@ -51,14 +49,14 @@
caption: "Apps you don't have access to",
head: [
{ text: "Name", classes: "govuk-!-width-one-quarter" },
- { text: "Description", classes: "govuk-!-width-one-third" },
- { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }
+ { text: "Description", classes: "govuk-!-width-one-half" },
+ { text: "Access" }
],
rows: @applications_without_signin.map do |application|
[
{ text: application.name },
{ text: application.description },
- { text: wrap_links_in_actions_markup([account_applications_grant_access_link(application)]) }
+ { text: account_applications_grant_access_link(application) }
]
end,
vertical_on_small_screen: true,
diff --git a/app/views/api_users/applications/index.html.erb b/app/views/api_users/applications/index.html.erb
index 6f654957a..2f75135b7 100644
--- a/app/views/api_users/applications/index.html.erb
+++ b/app/views/api_users/applications/index.html.erb
@@ -34,14 +34,14 @@
caption: "Apps #{@api_user.name} has access to",
head: [
{ text: "Name", classes: "govuk-!-width-one-quarter" },
- { text: "Description", classes: "govuk-!-width-one-third" },
- { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
+ { text: "Description", classes: "govuk-!-width-one-half" },
+ { text: "Permissions" }
],
rows: @applications.map do |application|
[
{ text: application.name },
{ text: application.description },
- { text: wrap_links_in_actions_markup([api_users_applications_permissions_link(application, @api_user)]) }
+ { text: api_users_applications_permissions_link(application, @api_user) }
]
end,
vertical_on_small_screen: true,
diff --git a/app/views/components/_table.html.erb b/app/views/components/_table.html.erb
index b95a85abc..e012a6c81 100644
--- a/app/views/components/_table.html.erb
+++ b/app/views/components/_table.html.erb
@@ -45,10 +45,11 @@
<% if cellindex == 0 && first_cell_is_header %>
<%= t.header cell[:text], {
scope: "row",
+ classes: cell[:classes],
format: cell[:format]
} %>
<% elsif vertical_on_small_screen && head.any? %>
- <%= t.cell nil, { format: cell[:format] } do %>
+ <%= t.cell nil, { classes: cell[:classes], format: cell[:format] } do %>
" aria-hidden="true">
<%= head[cellindex][:text] %>
@@ -56,6 +57,7 @@
<% end %>
<% else %>
<%= t.cell cell[:text], {
+ classes: cell[:classes],
format: cell[:format]
} %>
<% end %>
diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb
index d230a79c0..3ddf9d81c 100644
--- a/app/views/users/applications/index.html.erb
+++ b/app/views/users/applications/index.html.erb
@@ -34,18 +34,16 @@
caption: "Apps #{@user.name} has access to",
head: [
{ text: "Name", classes: "govuk-!-width-one-quarter" },
- { text: "Description", classes: "govuk-!-width-one-third" },
- { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
+ { text: "Description", classes: "govuk-!-width-one-half" },
+ { text: "Access" },
+ { text: "Permissions" }
],
rows: @applications_with_signin.map do |application|
[
{ text: application.name },
{ text: application.description },
- { text: wrap_links_in_actions_markup([
- users_applications_permissions_links(application, @user),
- users_applications_remove_access_link(application, @user)
- ])
- },
+ { text: users_applications_remove_access_link(application, @user) },
+ { text: users_applications_permissions_links(application, @user), classes: "applications-table__permissions-action-links" }
]
end,
vertical_on_small_screen: true,
@@ -55,14 +53,14 @@
caption: "Apps #{@user.name} does not have access to",
head: [
{ text: "Name", classes: "govuk-!-width-one-quarter" },
- { text: "Description", classes: "govuk-!-width-one-third" },
- { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }
+ { text: "Description", classes: "govuk-!-width-one-half" },
+ { text: "Access" }
],
rows: @applications_without_signin.map do |application|
[
{ text: application.name },
{ text: application.description },
- { text: wrap_links_in_actions_markup([users_applications_grant_access_link(application, @user)]) }
+ { text: users_applications_grant_access_link(application, @user) }
]
end,
vertical_on_small_screen: true,
diff --git a/docs/access_and_permissions.md b/docs/access_and_permissions.md
index ffc820486..c00f9b902 100644
--- a/docs/access_and_permissions.md
+++ b/docs/access_and_permissions.md
@@ -68,14 +68,13 @@ These dependencies determine whether a user can:
```mermaid
flowchart TD
A(Account::ApplicationsController#index) --authorize [:account, Doorkeeper::Application]--> B(Account::ApplicationPolicy#index?)
- C(app/views/account/applications/index.html.erb) --wrap_links_in_actions_markup--> D(ApplicationTableHelper#wrap_links_in_actions_markup)
- C --account_applications_permissions_links--> E(ApplicationTableHelper#account_applications_permissions_links)
- E --policy([:account, application]).view_permissions?--> F(Account::ApplicationPolicy#view_permissions?)
- E --policy([:account, application]).edit_permissions?--> G(Account::ApplicationPolicy#edit_permissions?)
- C --account_applications_remove_access_link--> H(ApplicationTableHelper#account_applications_remove_access_link)
- H --policy([:account, application]).remove_signin_permission?--> I(Account::ApplicationPolicy#remove_signin_permission?)
- C --account_applications_grant_access_link--> J(ApplicationTableHelper#account_applications_grant_access_link)
- J --policy([:account, Doorkeeper::Application]).grant_signin_permission?--> K(Account::ApplicationPolicy#grant_signin_permission?)
+ C(app/views/account/applications/index.html.erb) --account_applications_permissions_links--> D(ApplicationTableHelper#account_applications_permissions_links)
+ D --policy([:account, application]).view_permissions?--> E(Account::ApplicationPolicy#view_permissions?)
+ D --policy([:account, application]).edit_permissions?--> F(Account::ApplicationPolicy#edit_permissions?)
+ C --account_applications_remove_access_link--> G(ApplicationTableHelper#account_applications_remove_access_link)
+ G --policy([:account, application]).remove_signin_permission?--> H(Account::ApplicationPolicy#remove_signin_permission?)
+ C --account_applications_grant_access_link--> I(ApplicationTableHelper#account_applications_grant_access_link)
+ I --policy([:account, Doorkeeper::Application]).grant_signin_permission?--> J(Account::ApplicationPolicy#grant_signin_permission?)
```
#### Account permissions show
@@ -239,18 +238,17 @@ These dependencies determine whether a user can:
flowchart TD
A(Users::ApplicationsController#index) --authorize [{ user: }], policy_class: Users::ApplicationPolicy--> B(Users::ApplicationPolicy#index?)
B --Pundit.policy(current_user, user).edit?--> C(UserPolicy#edit?)
- D(app/views/users/applications/index.html.erb) --wrap_links_in_actions_markup--> E(ApplicationTableHelper#wrap_links_in_actions_markup)
- D --users_applications_permissions_links--> F(ApplicationTableHelper#users_applications_permissions_links)
- F --Users::ApplicationPolicy.new(current_user, { application:, user: }).view_permissions?--> G(Users::ApplicationPolicy#view_permissions?)
- G --Pundit.policy(current_user, user).edit?--> H(UserPolicy#edit?)
- F --Users::ApplicationPolicy.new(current_user, { application:, user: }).edit_permissions?--> I(Users::ApplicationPolicy#edit_permissions?)
- I --Pundit.policy(current_user, user).edit?--> H
- D --users_applications_remove_access_link--> J(ApplicationTableHelper#users_applications_remove_access_link)
- J --Users::ApplicationPolicy.new(current_user, { application:, user: }).remove_signin_permission?--> K(Users::ApplicationPolicy#remove_signin_permission?)
- K --Pundit.policy(current_user, user).edit?--> H
- D --users_applications_grant_access_link--> L(ApplicationTableHelper#users_applications_grant_access_link)
- L --Users::ApplicationPolicy.new(current_user, { application:, user: }).grant_signin_permission?--> M(Users::ApplicationPolicy#grant_signin_permission?)
- M --Pundit.policy(current_user, user).edit?--> H
+ D(app/views/users/applications/index.html.erb) --users_applications_permissions_links--> E(ApplicationTableHelper#users_applications_permissions_links)
+ E --Users::ApplicationPolicy.new(current_user, { application:, user: }).view_permissions?--> F(Users::ApplicationPolicy#view_permissions?)
+ F --Pundit.policy(current_user, user).edit?--> G(UserPolicy#edit?)
+ E --Users::ApplicationPolicy.new(current_user, { application:, user: }).edit_permissions?--> H(Users::ApplicationPolicy#edit_permissions?)
+ H --Pundit.policy(current_user, user).edit?--> G
+ D --users_applications_remove_access_link--> I(ApplicationTableHelper#users_applications_remove_access_link)
+ I --Users::ApplicationPolicy.new(current_user, { application:, user: }).remove_signin_permission?--> J(Users::ApplicationPolicy#remove_signin_permission?)
+ J --Pundit.policy(current_user, user).edit?--> G
+ D --users_applications_grant_access_link--> K(ApplicationTableHelper#users_applications_grant_access_link)
+ K --Users::ApplicationPolicy.new(current_user, { application:, user: }).grant_signin_permission?--> L(Users::ApplicationPolicy#grant_signin_permission?)
+ L --Pundit.policy(current_user, user).edit?--> G
```
#### Users permissions show
diff --git a/test/helpers/application_table_helper_test.rb b/test/helpers/application_table_helper_test.rb
index 78261dc7e..70d4bac14 100644
--- a/test/helpers/application_table_helper_test.rb
+++ b/test/helpers/application_table_helper_test.rb
@@ -3,22 +3,6 @@
class ApplicationTableHelperTest < ActionView::TestCase
include PunditHelpers
- context "#wrap_links_in_actions_markup" do
- should "wrap an array of links in a container with a class to apply actions styles" do
- links = [
- "Destination one",
- "Destination two",
- ]
-
- expected_output = "".gsub(/>\s+<")
-
- assert_equal expected_output, wrap_links_in_actions_markup(links)
- end
- end
-
context "#account_applications_grant_access_link" do
setup do
@user = build(:user)
@@ -28,7 +12,7 @@ class ApplicationTableHelperTest < ActionView::TestCase
should "generate a grant access button when the current user can grant the signin permission" do
stub_policy @user, [:account, Doorkeeper::Application], grant_signin_permission?: true
- assert_includes account_applications_grant_access_link(@application), "Grant access"
+ assert_includes account_applications_grant_access_link(@application), "Grant"
end
should "return an empty string when the current user cannot grant the signin permission" do
@@ -54,7 +38,7 @@ class ApplicationTableHelperTest < ActionView::TestCase
grant_signin_permission?: true,
)
- assert_includes users_applications_grant_access_link(@application, @grantee), "Grant access"
+ assert_includes users_applications_grant_access_link(@application, @grantee), "Grant"
end
should "return an empty string when the current user cannot grant the signin permission" do
@@ -78,7 +62,7 @@ class ApplicationTableHelperTest < ActionView::TestCase
should "generate a remove access link when the current user can remove the signin permission" do
stub_policy @user, [:account, @application], remove_signin_permission?: true
- assert_includes account_applications_remove_access_link(@application), "Remove access"
+ assert_includes account_applications_remove_access_link(@application), "Remove"
end
should "return an empty string when the current user cannot remove the signin permission" do
@@ -104,7 +88,7 @@ class ApplicationTableHelperTest < ActionView::TestCase
remove_signin_permission?: true,
)
- assert_includes users_applications_remove_access_link(@application, @grantee), "Remove access"
+ assert_includes users_applications_remove_access_link(@application, @grantee), "Remove"
end
should "return an empty string when the current user cannot remove the signin permission" do
@@ -130,8 +114,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = account_applications_permissions_links(@application)
- assert_includes result, "View permissions"
- assert_includes result, "Update permissions"
+ assert_includes result, "View"
+ assert_includes result, "Update"
end
should "only generate a view link when the user can only view permissions" do
@@ -139,8 +123,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = account_applications_permissions_links(@application)
- assert_includes result, "View permissions"
- assert_not_includes result, "Update permissions"
+ assert_includes result, "View"
+ assert_not_includes result, "Update"
end
should "only generate an update link when the user can only edit permissions" do
@@ -148,8 +132,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = account_applications_permissions_links(@application)
- assert_not_includes result, "View permissions"
- assert_includes result, "Update permissions"
+ assert_not_includes result, "View"
+ assert_includes result, "Update"
end
should "return an empty string when the user can do neither" do
@@ -178,8 +162,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = users_applications_permissions_links(@application, @grantee)
- assert_includes result, "View permissions"
- assert_includes result, "Update permissions"
+ assert_includes result, "View"
+ assert_includes result, "Update"
end
should "only generate a view link when the user can only view permissions" do
@@ -193,8 +177,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = users_applications_permissions_links(@application, @grantee)
- assert_includes result, "View permissions"
- assert_not_includes result, "Update permissions"
+ assert_includes result, "View"
+ assert_not_includes result, "Update"
end
should "only generate an edit link when the user can only edit permissions" do
@@ -208,8 +192,8 @@ class ApplicationTableHelperTest < ActionView::TestCase
result = users_applications_permissions_links(@application, @grantee)
- assert_not_includes result, "View permissions"
- assert_includes result, "Update permissions"
+ assert_not_includes result, "View"
+ assert_includes result, "Update"
end
should "return an empty string when the user can do neither" do
@@ -234,7 +218,7 @@ class ApplicationTableHelperTest < ActionView::TestCase
grantee = create(:api_user)
stubs(:current_user).returns(granter)
- assert_includes api_users_applications_permissions_link(application, grantee), "Update permissions"
+ assert_includes api_users_applications_permissions_link(application, grantee), "Update"
end
end