From 3f9bde8e6f41df4708ec032ec887c600e13c5f27 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 12:29:28 +0100 Subject: [PATCH 01/10] Fix test name in InvitationsControllerTest This should have been changed in this earlier commit [1]. [1]: https://github.com/alphagov/signon/commit/735fd59fdc2c3f86ac777c3a8081d2899124bd3f --- test/controllers/invitations_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index 34b2575eb..f1406fc57 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -48,7 +48,7 @@ class InvitationsControllerTest < ActionController::TestCase end end - should "render form checkbox input for signin permission & select for other permissions" do + should "render form checkbox input for permission" do create(:application) get :new From 87d4892ecf503edc1b553194106d354c01269981 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 12:46:07 +0100 Subject: [PATCH 02/10] Improve test for permissions checkboxes in InvitationsController#new --- test/controllers/invitations_controller_test.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index f1406fc57..2fb31384d 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -48,12 +48,17 @@ class InvitationsControllerTest < ActionController::TestCase end end - should "render form checkbox input for permission" do - create(:application) + should "render form checkbox inputs for permissions" do + application = create(:application) + signin_permission = application.signin_permission + other_permission = create(:supported_permission) get :new - assert_select "input[type='checkbox'][name='user[supported_permission_ids][]']" + assert_select "form" do + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{signin_permission.to_param}']" + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{other_permission.to_param}']" + end end end From 52472e3938a54946c99fe70f8a56675a16b49270 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 12:53:22 +0100 Subject: [PATCH 03/10] Add test for permissions checkboxes in BatchInvitationPermissionsController#new --- .../batch_invitation_permissions_controller_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index ecf058638..e61c4a001 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -41,6 +41,19 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase assert_select "label", "Has access to Profound Publisher?" assert_select "label", "reader" end + + should "render form checkbox inputs for permissions" do + application = create(:application) + signin_permission = application.signin_permission + other_permission = create(:supported_permission) + + get :new, params: { batch_invitation_id: @batch_invitation.id } + + assert_select "form" do + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{signin_permission.to_param}']" + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{other_permission.to_param}']" + end + end end context "POST create" do From 685d3f24a7e597a607eb0cf46571011e8f643067 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 13:03:53 +0100 Subject: [PATCH 04/10] Use option-select vs checkboxes for granting permissions Trello: https://trello.com/c/SfIc6Q2q The main motivation for these changes is the Licensing app which has a large number of permissions. The recent changes [1,2] which moved the UI for granting permissions to the GOV.UK Design System have made managing the permissions for the Licensing unmanageable. This replaces the accordion component [3] where each item of the accordion was a checkboxes component [4] (i.e. a set of checkboxes) with a list of option-select components [5] (each of which has a set of checkboxes within it). We've made this change to *both* the new invitation ("Create new user") page and the new batch invitation permissions page. The option-select component gives us something similar to the show/hide toggle which was previously provided by the accordion which is why it no longer makes sense to use the accordion. Also it makes the permissions section more compact which is no bad thing given how long it is! Most importantly the option-select component gives us the option to enable a search filter for the permissions for a given app. I plan to make use of this in a subsequent commit. This will address the primary motivation which is the unmanagability of the Licensing app permissions. Note that I've set the `key` attribute of the option-select components to "not-used" for now, because it's a required attribute but the name of the checkboxes is being derived from each individual option hash. I plan to address this in a subsequent commit. The use of the option-select component isn't really what it was intended for in that it doesn't *control* a list of things. However, I don't think it's too far away and it gives us a much better UI. [1]: https://github.com/alphagov/signon/pull/2361 [2]: https://github.com/alphagov/signon/pull/2412 [3]: https://components.publishing.service.gov.uk/component-guide/accordion [4]: https://components.publishing.service.gov.uk/component-guide/checkboxes [5]: https://components.publishing.service.gov.uk/component-guide/option_select --- app/helpers/users_helper.rb | 2 +- .../batch_invitation_permissions/new.html.erb | 24 +++++++------------ app/views/devise/invitations/new.html.erb | 24 +++++++------------ test/helpers/users_helper_test.rb | 10 ++++---- test/integration/inviting_users_test.rb | 8 +++---- 5 files changed, 26 insertions(+), 42 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index f86b8623c..72904c6c9 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -75,7 +75,7 @@ def options_for_organisation_select(selected: nil) end end - def items_for_permission_checkboxes(application:, user: nil) + def options_for_permission_option_select(application:, user: nil) application.sorted_supported_permissions_grantable_from_ui.map do |permission| { id: supported_permission_checkbox_id(application, permission), diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 888f32238..14dc3025a 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -19,22 +19,14 @@ %> <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - <%= render "govuk_publishing_components/components/accordion", { - items: policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| - { - heading: { - text: application.name - }, - content: { - html: render("govuk_publishing_components/components/checkboxes", { - name: "permissions_for_#{application.id}", - heading: "Permissions for #{application.name}", - items: items_for_permission_checkboxes(application:), - }) - }, - } - end - } %> + <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <%= render("govuk_publishing_components/components/option_select", { + title: application.name, + key: "not-used", + options_container_id: "user_application_#{application.id}_supported_permissions", + options: options_for_permission_option_select(application:), + }) %> + <% end %> <%= render "govuk_publishing_components/components/button", { text: "Create users and send emails" } %> <% end %> diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index ed39efed5..7d61d9088 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -86,22 +86,14 @@ heading_size: "l", } do %> - <%= render "govuk_publishing_components/components/accordion", { - items: policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| - { - heading: { - text: application.name - }, - content: { - html: render("govuk_publishing_components/components/checkboxes", { - name: "permissions_for_#{application.id}", - heading: "Permissions for #{application.name}", - items: items_for_permission_checkboxes(application:, user: f.object), - }) - }, - } - end - } %> + <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <%= render("govuk_publishing_components/components/option_select", { + title: application.name, + key: "not-used", + options_container_id: "user_application_#{application.id}_supported_permissions", + options: options_for_permission_option_select(application:, user: f.object), + }) %> + <% end %> <% end %> diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index fdaee2d2e..0d37a4e10 100644 --- a/test/helpers/users_helper_test.rb +++ b/test/helpers/users_helper_test.rb @@ -76,8 +76,8 @@ class UsersHelperTest < ActionView::TestCase end end - context "#items_for_permission_checkboxes" do - should "return permission options suitable for checkboxes component" do + context "#options_for_permission_option_select" do + should "return permission options suitable for option-select component" do application = create(:application) signin_permission = application.signin_permission permission1 = create(:supported_permission, application:, name: "permission1") @@ -85,9 +85,9 @@ class UsersHelperTest < ActionView::TestCase user = create(:user, supported_permissions: [signin_permission, permission1]) - items = items_for_permission_checkboxes(application:, user:) + options = options_for_permission_option_select(application:, user:) - expected_items = [ + expected_options = [ { id: supported_permission_checkbox_id(application, signin_permission), name: "user[supported_permission_ids][]", @@ -111,7 +111,7 @@ class UsersHelperTest < ActionView::TestCase }, ] - assert_equal expected_items, items + assert_equal expected_options, options end end diff --git a/test/integration/inviting_users_test.rb b/test/integration/inviting_users_test.rb index 8784fd4ad..6634042ae 100644 --- a/test/integration/inviting_users_test.rb +++ b/test/integration/inviting_users_test.rb @@ -151,11 +151,11 @@ class InvitingUsersTest < ActionDispatch::IntegrationTest fill_in "Name", with: "Alicia Smith" fill_in "Email", with: "alicia@example.com" - within_fieldset "Permissions for #{application_one.name}" do + within_fieldset application_one.name do uncheck "Has access to #{application_one.name}?" check "editor", allow_label_click: true end - within_fieldset "Permissions for #{application_two.name}" do + within_fieldset application_two.name do check "Has access to #{application_two.name}?" uncheck "gds-admin", allow_label_click: true end @@ -322,11 +322,11 @@ class InvitingUsersTest < ActionDispatch::IntegrationTest fill_in "Name", with: "Alicia Smith" fill_in "Email", with: "alicia@example.com" - within_fieldset "Permissions for #{application_one.name}" do + within_fieldset application_one.name do uncheck "Has access to #{application_one.name}?" check "editor" end - within_fieldset "Permissions for #{application_two.name}" do + within_fieldset application_two.name do check "Has access to #{application_two.name}?" uncheck "gds-admin" end From 1fd0421f33ad3427f0b187116bcb1b08a90b5f37 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 13:07:06 +0100 Subject: [PATCH 05/10] Use auto-generated ids for permission checkboxes We don't really need to set the `id` attribute for each checkbox in `UsersHelper#options_for_permission_option_select` - the checkboxes component within the option-select component will generate unique `id` values automatically and we don't need to reference them anywhere. This simplifies the code somewhat. --- app/helpers/users_helper.rb | 5 ----- test/helpers/users_helper_test.rb | 3 --- 2 files changed, 8 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 72904c6c9..a7c6157fd 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -78,7 +78,6 @@ def options_for_organisation_select(selected: nil) def options_for_permission_option_select(application:, user: nil) application.sorted_supported_permissions_grantable_from_ui.map do |permission| { - id: supported_permission_checkbox_id(application, permission), name: "user[supported_permission_ids][]", label: formatted_permission_name(application.name, permission.name), value: permission.id, @@ -87,10 +86,6 @@ def options_for_permission_option_select(application:, user: nil) end end - def supported_permission_checkbox_id(application, permission) - "user_application_#{application.id}_supported_permission_#{permission.id}" - end - def formatted_permission_name(application_name, permission_name) if permission_name == SupportedPermission::SIGNIN_NAME "Has access to #{application_name}?" diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index 0d37a4e10..ae2298be0 100644 --- a/test/helpers/users_helper_test.rb +++ b/test/helpers/users_helper_test.rb @@ -89,21 +89,18 @@ class UsersHelperTest < ActionView::TestCase expected_options = [ { - id: supported_permission_checkbox_id(application, signin_permission), name: "user[supported_permission_ids][]", label: "Has access to #{application.name}?", value: signin_permission.id, checked: true, }, { - id: supported_permission_checkbox_id(application, permission1), name: "user[supported_permission_ids][]", label: "permission1", value: permission1.id, checked: true, }, { - id: supported_permission_checkbox_id(application, permission2), name: "user[supported_permission_ids][]", label: "permission2", value: permission2.id, From 307f0215fbfbaf50cedd3e478cb9a9e67108ab49 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 13:09:27 +0100 Subject: [PATCH 06/10] Set name of permission checkboxes at option-select level Set the name of the permission checkboxes at the option-select component level (via its `key` attribute) rather than having to set it for each of its options in `UsersHelper#options_for_permission_option_select`. This simplifies the code somewhat. --- app/helpers/users_helper.rb | 1 - app/views/batch_invitation_permissions/new.html.erb | 2 +- app/views/devise/invitations/new.html.erb | 2 +- test/helpers/users_helper_test.rb | 3 --- 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index a7c6157fd..874474763 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -78,7 +78,6 @@ def options_for_organisation_select(selected: nil) def options_for_permission_option_select(application:, user: nil) application.sorted_supported_permissions_grantable_from_ui.map do |permission| { - name: "user[supported_permission_ids][]", label: formatted_permission_name(application.name, permission.name), value: permission.id, checked: user&.has_permission?(permission), diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 14dc3025a..6b0ccbec4 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -22,7 +22,7 @@ <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, - key: "not-used", + key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", options: options_for_permission_option_select(application:), }) %> diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index 7d61d9088..eae9243a0 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -89,7 +89,7 @@ <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, - key: "not-used", + key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", options: options_for_permission_option_select(application:, user: f.object), }) %> diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index ae2298be0..628240d68 100644 --- a/test/helpers/users_helper_test.rb +++ b/test/helpers/users_helper_test.rb @@ -89,19 +89,16 @@ class UsersHelperTest < ActionView::TestCase expected_options = [ { - name: "user[supported_permission_ids][]", label: "Has access to #{application.name}?", value: signin_permission.id, checked: true, }, { - name: "user[supported_permission_ids][]", label: "permission1", value: permission1.id, checked: true, }, { - name: "user[supported_permission_ids][]", label: "permission2", value: permission2.id, checked: false, From 2992f719d3e22850ccb1d61fb2f3bfbe90f8dcb6 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 13:11:14 +0100 Subject: [PATCH 07/10] Enable option-select filter for app with > 4 permissions Enable the search filter for the option-select component for applications which have more than 4 permissions which is roughly the point that a scrollbar appears. This addresses the primary motivation of this PR which is to improve the managability of the Licensing app permissions. --- app/views/batch_invitation_permissions/new.html.erb | 4 +++- app/views/devise/invitations/new.html.erb | 4 +++- .../batch_invitation_permissions_controller_test.rb | 12 ++++++++++++ test/controllers/invitations_controller_test.rb | 12 ++++++++++++ 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 6b0ccbec4..8180f0ee8 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -20,11 +20,13 @@ <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <% options = options_for_permission_option_select(application:) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", - options: options_for_permission_option_select(application:), + show_filter: options.length > 4, + options:, }) %> <% end %> diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index eae9243a0..406ec9eba 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -87,11 +87,13 @@ } do %> <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <% options = options_for_permission_option_select(application:, user: f.object) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", - options: options_for_permission_option_select(application:, user: f.object), + show_filter: options.length > 4, + options:, }) %> <% end %> diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index e61c4a001..ced6fa070 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -54,6 +54,18 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{other_permission.to_param}']" end end + + should "render filter for option-select component when app has more than 4 permissions" do + application = create(:application) + 4.times { create(:supported_permission, application:) } + assert application.supported_permissions.count > 4 + + get :new, params: { batch_invitation_id: @batch_invitation.id } + + assert_select "form" do + assert_select ".gem-c-option-select[data-filter-element]" + end + end end context "POST create" do diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index 2fb31384d..019383e47 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -60,6 +60,18 @@ class InvitationsControllerTest < ActionController::TestCase assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{other_permission.to_param}']" end end + + should "render filter for option-select component when app has more than 4 permissions" do + application = create(:application) + 4.times { create(:supported_permission, application:) } + assert application.supported_permissions.count > 4 + + get :new + + assert_select "form" do + assert_select ".gem-c-option-select[data-filter-element]" + end + end end context "when inviter is signed in as a superadmin" do From 735ddbe3cdfa2beb1046a6df87498cc35a4bf0a6 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 14:37:28 +0100 Subject: [PATCH 08/10] Only open option-selects when app has checked permissions Set each option-select component to open on load only if any of its permissions are selected. At the moment this is only relevant to the new invitation page when validation errors have occurred and the form has been re-rendered. The new batch invitation permissions page currently never has any validation errors, so we can just hard-code the value of `closed_on_load` to `true`. This behaviour is very similar to what we did on the users index page. Since this behaviour is implemented in JS, it's not possible to test it in the controller test and it doesn't seem important enough to add an integration test for it. --- app/views/batch_invitation_permissions/new.html.erb | 1 + app/views/devise/invitations/new.html.erb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 8180f0ee8..927dce4dc 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -26,6 +26,7 @@ key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", show_filter: options.length > 4, + closed_on_load: true, options:, }) %> <% end %> diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index 406ec9eba..4025aa927 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -93,6 +93,7 @@ key: "user[supported_permission_ids]", options_container_id: "user_application_#{application.id}_supported_permissions", show_filter: options.length > 4, + closed_on_load: options.none? { |o| o[:checked] }, options:, }) %> <% end %> From 040fef799be7aaaa86450abafe28871299592d64 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 15:05:48 +0100 Subject: [PATCH 09/10] Move UsersFilter.with_checked_at_top -> ApplicationHelper I want to be able to use this in the new invitation page and the new batch invitation permissions page. Moving it to a more generic namespace will make that easier. --- app/helpers/application_helper.rb | 4 ++++ app/models/users_filter.rb | 4 ---- app/views/users/_users_filter.html.erb | 4 ++-- test/helpers/application_helper_test.rb | 20 ++++++++++++++++++++ test/models/users_filter_test.rb | 20 -------------------- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 10a212870..b5db6a6a4 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -26,4 +26,8 @@ def sanitised_fullpath uri.query_values = uri.query_values.reject { |key, _value| SENSITIVE_QUERY_PARAMETERS.include?(key) } uri.to_s end + + def with_checked_options_at_top(options) + options.sort_by { |o| o[:checked] ? 0 : 1 } + end end diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index 299a17a15..fe805e436 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -4,10 +4,6 @@ class UsersFilter attr_reader :options - def self.with_checked_at_top(options) - options.sort_by { |o| o[:checked] ? 0 : 1 } - end - def initialize(users, current_user, options = {}) @users = users @current_user = current_user diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 8a24f4454..11a674199 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -48,7 +48,7 @@ options_container_id: "organisations_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:organisations), - options: UsersFilter.with_checked_at_top( + options: with_checked_options_at_top( @filter.organisation_option_select_options(aria_controls_id: filtered_users_id) ), } %> @@ -60,7 +60,7 @@ options_container_id: "permissions_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:permissions), - options: UsersFilter.with_checked_at_top( + options: with_checked_options_at_top( @filter.permission_option_select_options(aria_controls_id: filtered_users_id) ), } %> diff --git a/test/helpers/application_helper_test.rb b/test/helpers/application_helper_test.rb index 5a5cf5427..326b1d55b 100644 --- a/test/helpers/application_helper_test.rb +++ b/test/helpers/application_helper_test.rb @@ -25,4 +25,24 @@ class ApplicationHelperTest < ActionView::TestCase self.request = ActionDispatch::Request.new(Rack::MockRequest.env_for("/secret-squirrel?invitation_token=w1&reset_password_token=d1&sharing=ok")) assert_equal "/secret-squirrel?sharing=ok", sanitised_fullpath end + + context "#with_checked_options_at_top" do + should "put all checked options before all unchecked options" do + options = [ + { label: "A", checked: false }, + { label: "B", checked: true }, + { label: "C", checked: false }, + { label: "D", checked: true }, + ] + + expected_options = [ + { label: "B", checked: true }, + { label: "D", checked: true }, + { label: "A", checked: false }, + { label: "C", checked: false }, + ] + + assert_equal expected_options, with_checked_options_at_top(options) + end + end end diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index 6d1b4768f..e6bbb7668 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -5,26 +5,6 @@ class UsersFilterTest < ActiveSupport::TestCase @current_user = User.new end - context ".with_checked_at_top" do - should "put all checked options before all unchecked options" do - options = [ - { label: "A", checked: false }, - { label: "B", checked: true }, - { label: "C", checked: false }, - { label: "D", checked: true }, - ] - - expected_options = [ - { label: "B", checked: true }, - { label: "D", checked: true }, - { label: "A", checked: false }, - { label: "C", checked: false }, - ] - - assert_equal expected_options, UsersFilter.with_checked_at_top(options) - end - end - should "return all users in alphabetical name order" do create(:user, name: "beta") create(:user, name: "alpha") From 7eb14ee2a6fdfb045ff9e8ef7c434f8076c907b2 Mon Sep 17 00:00:00 2001 From: James Mead <james.mead@digital.cabinet-office.gov.uk> Date: Wed, 11 Oct 2023 15:31:44 +0100 Subject: [PATCH 10/10] Put checked permissions at top on new invitations page This change ensures any checked permission checkboxes are displayed at the top of the list of checkboxes for a given application option-select. At the moment this is only relevant to the new invitation page when validation errors have occurred and the form has been re-rendered. Currently the new batch invitation permissions page can never have any validation errors. This behaviour is very similar to what we did on the users index page. --- app/views/devise/invitations/new.html.erb | 2 +- test/controllers/invitations_controller_test.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index 4025aa927..503cb935f 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -94,7 +94,7 @@ options_container_id: "user_application_#{application.id}_supported_permissions", show_filter: options.length > 4, closed_on_load: options.none? { |o| o[:checked] }, - options:, + options: with_checked_options_at_top(options), }) %> <% end %> diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index 019383e47..c05083c1d 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -348,6 +348,22 @@ class InvitationsControllerTest < ActionController::TestCase end end + should "put selected permissions at the top if there are validation errors" do + application = create(:application) + signin_permission = application.signin_permission + other_permission = create(:supported_permission, application:) + selected_permissions = [other_permission] + + post :create, params: { user: { supported_permission_ids: selected_permissions.map(&:to_param) } } + + assert_select "form #user_application_#{application.id}_supported_permissions" do + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]']" do |checkboxes| + assert_equal other_permission.to_param, checkboxes.first["value"] + assert_equal signin_permission.to_param, checkboxes.last["value"] + end + end + end + should "record account invitation in event log when invitation sent" do EventLog.expects(:record_account_invitation).with(instance_of(User), @inviter)