From 2f794b6f67129119b3d59584f2bb4c09f6223996 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Tue, 13 Feb 2024 20:07:15 +0000 Subject: [PATCH 1/4] Show user status as tag instead of strikethrough The strikethrough is not part of the design system, so we've decided to use the govuk-tag style instead. The design on the card showed green for active users, grey for suspended users, but there were no examples of locked or invited users. I've chosen to only use green for active users, and grey for all other statuses. At James' suggestion, I created a new method, `status_with_tag`, to avoid changing the display in other places where `status` is used, such as the CSV export of the users, and the summary view for user edit pages --- app/helpers/users_helper.rb | 13 +++++++++++-- app/views/users/index.html.erb | 2 +- test/helpers/users_helper_test.rb | 9 +++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index fe9e36719..8f2fc8d0c 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -5,6 +5,16 @@ def status(user) user.status.humanize end + def status_with_tag(user) + css_classes = if user.status == User::USER_STATUS_ACTIVE + "govuk-tag--green" + else + "govuk-tag--grey" + end + + govuk_tag(status(user), css_classes) + end + def two_step_status(user) user.two_step_status.humanize.capitalize end @@ -63,8 +73,7 @@ def filtered_users_heading(users) end def user_name(user) - anchor_tag = link_to(user.name, edit_user_path(user), class: "govuk-link") - user.suspended? ? content_tag(:del, anchor_tag) : anchor_tag + link_to(user.name, edit_user_path(user), class: "govuk-link") end def options_for_role_select(selected: nil) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 178c653d6..dc33cf595 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -40,7 +40,7 @@ { text: user_name(user) }, { text: user.email }, { text: user.role_display_name }, - { text: status(user) }, + { text: status_with_tag(user) }, { text: two_step_status(user) }, ] end, diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index 46b7b7de4..3afe0c739 100644 --- a/test/helpers/users_helper_test.rb +++ b/test/helpers/users_helper_test.rb @@ -1,6 +1,8 @@ require "test_helper" class UsersHelperTest < ActionView::TestCase + include ApplicationHelper + test "sync_needed? should work with user permissions not synced yet" do application = create(:application) user = create(:user) @@ -16,6 +18,13 @@ class UsersHelperTest < ActionView::TestCase assert_equal "Suspended", status(build(:suspended_user)) end + test "status_with_tag should enclose the status in a govuk tag" do + assert_equal "Invited", status_with_tag(build(:invited_user)) + assert_equal "Active", status_with_tag(build(:active_user)) + assert_equal "Locked", status_with_tag(build(:locked_user)) + assert_equal "Suspended", status_with_tag(build(:suspended_user)) + end + test "two_step_status should reflect the user's status accurately when the user is exempted from 2sv" do assert_equal "Exempted", two_step_status(create(:two_step_exempted_user)) assert_equal "Exempted", two_step_status_with_requirement(create(:two_step_exempted_user)) From 35d09cb17854dc7616b0c4368723fde0caa4d1ed Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Wed, 14 Feb 2024 17:00:07 +0000 Subject: [PATCH 2/4] Replace strikethrough and use tag pattern --- app/helpers/api_users_helper.rb | 3 +-- app/views/api_users/index.html.erb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/helpers/api_users_helper.rb b/app/helpers/api_users_helper.rb index 65d3d78f4..b9884efd7 100644 --- a/app/helpers/api_users_helper.rb +++ b/app/helpers/api_users_helper.rb @@ -4,8 +4,7 @@ def truncate_access_token(token) end def api_user_name(user) - anchor_tag = link_to(user.name, edit_api_user_path(user), class: "govuk-link") - user.suspended? ? content_tag(:del, anchor_tag) : anchor_tag + link_to(user.name, edit_api_user_path(user), class: "govuk-link") end def application_list(user) diff --git a/app/views/api_users/index.html.erb b/app/views/api_users/index.html.erb index 4070766db..e49af765e 100644 --- a/app/views/api_users/index.html.erb +++ b/app/views/api_users/index.html.erb @@ -39,7 +39,7 @@ text: application_list(user), }, { - text: user.suspended? ? "Yes" : "No", + text: user.suspended? ? govuk_tag("Yes", "govuk-tag--grey") : govuk_tag("No", "govuk-tag--green"), }, ] end, From 0b4c7c53610b0906c321932a73dff1bd1c76dedc Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Thu, 15 Feb 2024 12:36:44 +0000 Subject: [PATCH 3/4] Show Status for API users rather than yes/no Calum suggested it would be preferable to show API users' "Status" (instead of "Suspended?") on the API users table, similar to the Users table. This allows us to show the status tag as "Active" or "Suspended", in a similar way to the Users table, to create consistent expectations for users of the interface. --- app/views/api_users/index.html.erb | 4 ++-- test/integration/manage_api_users_test.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/api_users/index.html.erb b/app/views/api_users/index.html.erb index e49af765e..17529eb3a 100644 --- a/app/views/api_users/index.html.erb +++ b/app/views/api_users/index.html.erb @@ -24,7 +24,7 @@ text: "Apps", }, { - text: "Suspended?", + text: "Status", }, ], rows: @api_users.map do |user| @@ -39,7 +39,7 @@ text: application_list(user), }, { - text: user.suspended? ? govuk_tag("Yes", "govuk-tag--grey") : govuk_tag("No", "govuk-tag--green"), + text: user.suspended? ? govuk_tag("Suspended", "govuk-tag--grey") : govuk_tag("Active", "govuk-tag--green"), }, ] end, diff --git a/test/integration/manage_api_users_test.rb b/test/integration/manage_api_users_test.rb index 3a60478fb..f468c21fc 100644 --- a/test/integration/manage_api_users_test.rb +++ b/test/integration/manage_api_users_test.rb @@ -20,7 +20,7 @@ class ManageApiUsersTest < ActionDispatch::IntegrationTest assert page.has_selector?("td", text: @api_user.email) assert page.has_selector?("td", text: @application.name) - assert page.has_selector?("td:last-child", text: "No") # suspended? + assert page.has_selector?("td:last-child", text: "Active") # status end should "be able to create and edit an API user" do From 722561bc1e593b1fdf7921ef618b7f7972dfc345 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Thu, 15 Feb 2024 12:39:00 +0000 Subject: [PATCH 4/4] Use helper method for API users table Now that we are showing the API users' status similar to the users' status, we can use the same helper method in the view. (There is precedent for this, as the edit view uses the same method, `summary_list_item_for_status`, for users and API users.) --- app/views/api_users/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/api_users/index.html.erb b/app/views/api_users/index.html.erb index 17529eb3a..af04541d5 100644 --- a/app/views/api_users/index.html.erb +++ b/app/views/api_users/index.html.erb @@ -39,7 +39,7 @@ text: application_list(user), }, { - text: user.suspended? ? govuk_tag("Suspended", "govuk-tag--grey") : govuk_tag("Active", "govuk-tag--green"), + text: status_with_tag(user), }, ] end,