Merge pull request #2424 from alphagov/use-option-select-vs-checkboxe…

…s-for-granting-permissions

Use option-select vs checkboxes for granting permissions

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

app/helpers/users_helper.rb

@@ -75,22 +75,16 @@ 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),
-        name: "user[supported_permission_ids][]",
         label: formatted_permission_name(application.name, permission.name),
         value: permission.id,
         checked: user&.has_permission?(permission),
       }
     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}?"

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

app/views/batch_invitation_permissions/new.html.erb

@@ -19,22 +19,17 @@
 %>
 
 <%= 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| %>
+    <% 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",
+      show_filter: options.length > 4,
+      closed_on_load: true,
+      options:,
+    }) %>
+  <% end %>
 
   <%= render "govuk_publishing_components/components/button", { text: "Create users and send emails" } %>
 <% end %>

app/views/devise/invitations/new.html.erb

@@ -86,22 +86,17 @@
         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| %>
+          <% 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",
+            show_filter: options.length > 4,
+            closed_on_load: options.none? { |o| o[:checked] },
+            options: with_checked_options_at_top(options),
+          }) %>
+        <% end %>
 
       <% end %>
 

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)
       ),
     } %>

test/controllers/batch_invitation_permissions_controller_test.rb

@@ -41,6 +41,31 @@ 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
+
+    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

test/controllers/invitations_controller_test.rb

@@ -48,12 +48,29 @@ class InvitationsControllerTest < ActionController::TestCase
         end
       end
 
-      should "render form checkbox input for signin permission & select for other permissions" 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 "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
+
+      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 "input[type='checkbox'][name='user[supported_permission_ids][]']"
+        assert_select "form" do
+          assert_select ".gem-c-option-select[data-filter-element]"
+        end
       end
     end
 
@@ -331,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)
 

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

test/helpers/users_helper_test.rb

@@ -76,42 +76,36 @@ 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")
       permission2 = create(:supported_permission, application:, name: "permission2")
 
       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][]",
           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,
           checked: false,
         },
       ]
 
-      assert_equal expected_items, items
+      assert_equal expected_options, options
     end
   end
 

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

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")