From 0b4e2fb25549ec4d18a47a6483f6052a51060f3f Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 10:42:43 +0100 Subject: [PATCH 01/25] Remove redundant assign_organisation_from_csv? implementation This method only returns true if the user is a `govuk_admin`, which is the same logic as the `new?` method in this policy. --- app/policies/batch_invitation_policy.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/policies/batch_invitation_policy.rb b/app/policies/batch_invitation_policy.rb index dc8e90d69..a2e726fb1 100644 --- a/app/policies/batch_invitation_policy.rb +++ b/app/policies/batch_invitation_policy.rb @@ -7,8 +7,5 @@ def new? alias_method :create?, :new? alias_method :show?, :new? alias_method :manage_permissions?, :new? - - def assign_organisation_from_csv? - current_user.govuk_admin? - end + alias_method :assign_organisation_from_csv?, :new? end From afea2a87258229a41adeeaa6c8564e8c79899c30 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 10:53:44 +0100 Subject: [PATCH 02/25] Remove unreachable code in batch_invitations/new.html.erb The `BatchInvitationPolicy#assign_organisation_from_csv?` method is an alias for `BatchInvitationPolicy#new` which means that only people who can view this page can assign organisations from the CSV file. So the second branch (where `assign_organisation_from_csv` is `false`) is not reachable in this view template and can therefore be removed. --- app/views/batch_invitations/new.html.erb | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/app/views/batch_invitations/new.html.erb b/app/views/batch_invitations/new.html.erb index 6efdc0db3..a34bcdc94 100644 --- a/app/views/batch_invitations/new.html.erb +++ b/app/views/batch_invitations/new.html.erb @@ -38,20 +38,12 @@

The format of the CSV should be as follows:

- <% if policy(f.object).assign_organisation_from_csv? %> 
 Name,Email,Organisation
 Jane Smith,jane@example.com,government-digital-service
 Winston Churchill,winston@example.com,cabinet-office
-

The values in the Organisation column should be the slug of the organisation the user will be assigned to. If the value is blank, the user will be assigned to the Organisation selected in the drop-down below. If the value is provided, but is not a valid slug, the user will not be invited. You can find the slug for an organisation on <%= link_to 'the list of organisations', organisations_path %>.

- <% else %> - 
-Name,Email
-Jane Smith,jane@example.com
-Winston Churchill,winston@example.com
-
- <% end %> +

The values in the Organisation column should be the slug of the organisation the user will be assigned to. If the value is blank, the user will be assigned to the Organisation selected in the drop-down below. If the value is provided, but is not a valid slug, the user will not be invited. You can find the slug for an organisation on <%= link_to 'the list of organisations', organisations_path %>.

Any fields in the CSV other than those shown above will be ignored.

@@ -59,11 +51,7 @@ Winston Churchill,winston@example.com
<%= f.label :organisation_id, "Organisation" %> <%= f.select :organisation_id, organisation_options(f), organisation_select_options, { class: "chosen-select", 'data-module' => 'chosen' } %> - <% if policy(f.object).assign_organisation_from_csv? %> -

If the uploaded CSV doesn't contain an Organisation column, or the value is blank for a row, the user will be assigned to this organisation instead.

- <% else %> -

All users in the CSV will be assigned to the organisation selected here.

- <% end %> +

If the uploaded CSV doesn't contain an Organisation column, or the value is blank for a row, the user will be assigned to this organisation instead.

<%= f.submit "Manage permissions for new users", :class => 'btn btn-success' %> From 104895794f88fde8230ebfaa2d82ceae01598fe4 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 10:57:24 +0100 Subject: [PATCH 03/25] Remove unneccesary condition from BatchInvitationsController#create `BatchInvitationPolicy#assign_organisation_from_csv?` is an alias for `BatchInvitationPolicy#create` so this method always returns `true` in this controller action. We can therefore remove the conditional and inline it into the argument hash. --- app/controllers/batch_invitations_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb index b51a54d87..13f11fae0 100644 --- a/app/controllers/batch_invitations_controller.rb +++ b/app/controllers/batch_invitations_controller.rb @@ -44,10 +44,8 @@ def create batch_invitation: @batch_invitation, name: row["Name"], email: row["Email"], + organisation_slug: row["Organisation"], } - if policy(@batch_invitation).assign_organisation_from_csv? - batch_user_args[:organisation_slug] = row["Organisation"] - end batch_user = BatchInvitationUser.new(batch_user_args) unless batch_user.valid? From 458cbe1094bc42a08301f254925d992de76ca990 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 11:01:49 +0100 Subject: [PATCH 04/25] Remove unused method assign_organisation_from_csv? With the previous commits this method is no longer used so can be removed. --- app/policies/batch_invitation_policy.rb | 1 - test/policies/batch_invitation_policy_test.rb | 11 ----------- 2 files changed, 12 deletions(-) diff --git a/app/policies/batch_invitation_policy.rb b/app/policies/batch_invitation_policy.rb index a2e726fb1..463b9c5ca 100644 --- a/app/policies/batch_invitation_policy.rb +++ b/app/policies/batch_invitation_policy.rb @@ -7,5 +7,4 @@ def new? alias_method :create?, :new? alias_method :show?, :new? alias_method :manage_permissions?, :new? - alias_method :assign_organisation_from_csv?, :new? end diff --git a/test/policies/batch_invitation_policy_test.rb b/test/policies/batch_invitation_policy_test.rb index 4d3d17c6a..8e78c276d 100644 --- a/test/policies/batch_invitation_policy_test.rb +++ b/test/policies/batch_invitation_policy_test.rb @@ -22,15 +22,4 @@ class BatchInvitationPolicyTest < ActiveSupport::TestCase forbid?(create(:user), BatchInvitation.new, :new) end end - - context "assign_organisation_from_csv" do - should "allow only for superadmins and admins" do - assert permit?(create(:superadmin_user), User, :assign_organisation_from_csv) - assert permit?(create(:admin_user), User, :assign_organisation_from_csv) - - assert forbid?(create(:super_organisation_admin_user), User, :assign_organisation_from_csv) - assert forbid?(create(:organisation_admin_user), User, :assign_organisation_from_csv) - assert forbid?(create(:user), User, :assign_organisation_from_csv) - end - end end From 9e85b6ad9f03a248189059a2d5d06ba7036c69cb Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 11:18:36 +0100 Subject: [PATCH 05/25] Remove table of recent batches from batch_invitations/new.html.erb The new (design-system) designs for this page don't include the table of recent batches, so in preparation for migrating this page to the design system, I'm removing them. With the removal of the table the `batch_invite_status_link` helper method is no longer used so can also be removed. --- .../batch_invitations_controller.rb | 6 ----- app/helpers/batch_invitations_helper.rb | 8 ------- app/views/batch_invitations/new.html.erb | 24 ------------------- .../batch_invitations_controller_test.rb | 14 ----------- test/helpers/batch_invitations_helper_test.rb | 14 ----------- 5 files changed, 66 deletions(-) diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb index 13f11fae0..1c32f98e8 100644 --- a/app/controllers/batch_invitations_controller.rb +++ b/app/controllers/batch_invitations_controller.rb @@ -3,8 +3,6 @@ class BatchInvitationsController < ApplicationController before_action :authenticate_user! - helper_method :recent_batch_invitations - layout "admin_layout", only: %w[show] def new @@ -68,10 +66,6 @@ def show private - def recent_batch_invitations - @recent_batch_invitations ||= BatchInvitation.where("created_at > ?", 3.days.ago).order("created_at desc") - end - def file_uploaded? if params[:batch_invitation].nil? || params[:batch_invitation][:user_names_and_emails].nil? false diff --git a/app/helpers/batch_invitations_helper.rb b/app/helpers/batch_invitations_helper.rb index 30778c38a..90cfd7c3b 100644 --- a/app/helpers/batch_invitations_helper.rb +++ b/app/helpers/batch_invitations_helper.rb @@ -1,12 +1,4 @@ module BatchInvitationsHelper - def batch_invite_status_link(batch_invitation, &block) - if !batch_invitation.has_permissions? - link_to(new_batch_invitation_permissions_path(batch_invitation), alt: "Edit this batch's permissions", &block) - else - link_to(batch_invitation_path(batch_invitation), alt: "View this batch", &block) - end - end - def batch_invite_status_message(batch_invitation) if batch_invitation.in_progress? "In progress. " \ diff --git a/app/views/batch_invitations/new.html.erb b/app/views/batch_invitations/new.html.erb index a34bcdc94..89fece92f 100644 --- a/app/views/batch_invitations/new.html.erb +++ b/app/views/batch_invitations/new.html.erb @@ -4,30 +4,6 @@

Upload a batch of users

-<% if recent_batch_invitations.any? %> -

Recent batches

- - - - - - - - - <% recent_batch_invitations.each do |batch_invitation| %> - - - - - <% end %> - -
SummaryStatus
- <%= batch_invite_status_link(batch_invitation) do %> - <%= batch_invitation.batch_invitation_users.count %> users by <%= batch_invitation.user.name %> at <%= batch_invitation.created_at.to_fs(:govuk_date) %> - <% end %> - <%= batch_invite_status_message(batch_invitation) %>
-<% end %> -
<%= form_for @batch_invitation do |f| %>

Upload a CSV

diff --git a/test/controllers/batch_invitations_controller_test.rb b/test/controllers/batch_invitations_controller_test.rb index d516c80d8..81f9b41e6 100644 --- a/test/controllers/batch_invitations_controller_test.rb +++ b/test/controllers/batch_invitations_controller_test.rb @@ -19,20 +19,6 @@ def users_csv(filename = "users.csv") assert_select "input[type=file]" end - context "some batches created recently" do - setup do - @bi = create(:batch_invitation, :in_progress) - create(:batch_invitation_user, batch_invitation: @bi) - end - - should "show a table summarising them" do - get :new - assert_select "table.recent-batches tbody tr", count: 1 - assert_select "table.recent-batches tbody td", "1 users by #{@bi.user.name} at #{@bi.created_at.to_fs(:govuk_date)}" - assert_select "table.recent-batches tbody td", "In progress. 0 of 1 users processed." - end - end - should "allow selection of an organisation to invite users to" do organisation = create(:organisation) get :new diff --git a/test/helpers/batch_invitations_helper_test.rb b/test/helpers/batch_invitations_helper_test.rb index 9c9b5b132..f7190cde5 100644 --- a/test/helpers/batch_invitations_helper_test.rb +++ b/test/helpers/batch_invitations_helper_test.rb @@ -93,18 +93,4 @@ class BatchInvitationsHelperTest < ActionView::TestCase end end end - - context "#batch_invite_status_link" do - should "link to show the batch when it has permissions" do - batch_invitation = create(:batch_invitation, :has_permissions, outcome: "success") - - assert_includes batch_invite_status_link(batch_invitation) {}, batch_invitation_path(batch_invitation) - end - - should "link to show edit the permissions when it has no permissions" do - batch_invitation = create(:batch_invitation) - - assert_includes batch_invite_status_link(batch_invitation) {}, new_batch_invitation_permissions_path(batch_invitation) - end - end end From 079dd398580755cfc34af8f9136e7499a6350908 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Fri, 15 Sep 2023 12:42:00 +0100 Subject: [PATCH 06/25] Migrate BatchInvitationsController#new to design system This commit switches the first "page" of the batch creation of users workflow to the design system. All of the actions in BatchInvitationsController now use the `admin_layout` so I've removed the `only` array rather than add `new` to it. The `form_for` helper needs `multipart: true` for file uploads to work. This confused me for a while - I think rails default form builder knows to set `multipart: true` implicitly if you use the `file_field` helper, but as we've removed that in order to use the `file_upload` component we now have to set it explicitly. I've inlined the relevant `policy_scope` line from `UserHelpers#organisation_options` to build the list of `Organisations` in the select. This helper method is still used elsewhere though, so I can't remove it yet. --- .../batch_invitations_controller.rb | 2 +- app/views/batch_invitations/new.html.erb | 110 +++++++++++++----- test/integration/batch_inviting_users_test.rb | 4 +- 3 files changed, 83 insertions(+), 33 deletions(-) diff --git a/app/controllers/batch_invitations_controller.rb b/app/controllers/batch_invitations_controller.rb index 1c32f98e8..ac3f275b5 100644 --- a/app/controllers/batch_invitations_controller.rb +++ b/app/controllers/batch_invitations_controller.rb @@ -3,7 +3,7 @@ class BatchInvitationsController < ApplicationController before_action :authenticate_user! - layout "admin_layout", only: %w[show] + layout "admin_layout" def new @batch_invitation = BatchInvitation.new(organisation_id: current_user.organisation_id) diff --git a/app/views/batch_invitations/new.html.erb b/app/views/batch_invitations/new.html.erb index 89fece92f..80224afd6 100644 --- a/app/views/batch_invitations/new.html.erb +++ b/app/views/batch_invitations/new.html.erb @@ -1,35 +1,85 @@ <% content_for :title, "Upload a batch of users" %> +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Home", + url: root_path, + }, + { + title: "Users", + url: users_path, + }, + { + title: "Upload a batch of users", + } + ] + }) +%> -
-

Upload a batch of users

-
+
+ <%= form_for @batch_invitation, multipart: true do |f| %> + <%= render "govuk_publishing_components/components/fieldset", { + legend_text: "Upload a CSV", + heading_level: 2, + heading_size: "s" + } do %> + + <%= render "govuk_publishing_components/components/hint", { text: + "The format of the CSV should be as follows:" } %> + + 
+        Name,Email,Organisation
+        Jane Smith,jane@example.com,government-digital-service
+        Winston Churchill,winston@example.com,cabinet-office
+
+ +

The values in the Organisation + column should be the slug of the organisation the user will be + assigned to. If the value is blank, the user will be assigned + to the Organisation selected in the drop-down below. If the + value is provided, but is not a valid slug, the user will not + be invited. You can find the slug for an organisation on + <%= link_to 'the list of organisations', organisations_path + %>.

+ + <%= render "govuk_publishing_components/components/hint", { text: + "Any fields in the CSV other than those shown above will be + ignored." } %> + + <%= render "govuk_publishing_components/components/file_upload", { + label: { + text: "Upload a CSV file" + }, + name: "batch_invitation[user_names_and_emails]", + id: "batch_invitation_user_names_and_emails", + accept: "text/csv", + } %> + <% end %> + + <%= render "govuk_publishing_components/components/fieldset", { + legend_text: "Organisation", + heading_level: 2, + heading_size: "s" + } do %> + + <%= render "govuk_publishing_components/components/hint", { + text: "If the uploaded CSV doesn't contain an Organisation + column, or the value is blank for a row, the user will be + assigned to this organisation instead." } %> + + <%= render "govuk_publishing_components/components/select", { + id: "batch_invitation_organisation_id", + name: "batch_invitation[organisation_id]", + label: "Organisation", + options: policy_scope(Organisation).map { |organisation| { text: organisation.name_with_abbreviation, value: organisation.id } } + } %> + + <% end %> -
- <%= form_for @batch_invitation do |f| %> -

Upload a CSV

-
-
- <%= f.label :user_names_and_emails, "Choose a CSV file of users with names and email addresses" %> - <%= f.file_field(:user_names_and_emails, :accept => 'text/csv', required: true) %> -
-
-

The format of the CSV should be as follows:

- 
-Name,Email,Organisation
-Jane Smith,jane@example.com,government-digital-service
-Winston Churchill,winston@example.com,cabinet-office
-
-

The values in the Organisation column should be the slug of the organisation the user will be assigned to. If the value is blank, the user will be assigned to the Organisation selected in the drop-down below. If the value is provided, but is not a valid slug, the user will not be invited. You can find the slug for an organisation on <%= link_to 'the list of organisations', organisations_path %>.

-

Any fields in the CSV other than those shown above will be ignored.

-
-
- -
- <%= f.label :organisation_id, "Organisation" %> - <%= f.select :organisation_id, organisation_options(f), organisation_select_options, { class: "chosen-select", 'data-module' => 'chosen' } %> -

If the uploaded CSV doesn't contain an Organisation column, or the value is blank for a row, the user will be assigned to this organisation instead.

-
- - <%= f.submit "Manage permissions for new users", :class => 'btn btn-success' %> + <%= render "govuk_publishing_components/components/button", { + text: "Manage permissions for new users" + } %> <% end %>
diff --git a/test/integration/batch_inviting_users_test.rb b/test/integration/batch_inviting_users_test.rb index 518d1be28..1812fd843 100644 --- a/test/integration/batch_inviting_users_test.rb +++ b/test/integration/batch_inviting_users_test.rb @@ -87,7 +87,7 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest perform_enqueued_jobs do visit new_batch_invitation_path path = Rails.root.join("test/fixtures/users.csv") - attach_file("Choose a CSV file of users with names and email addresses", path) + attach_file("Upload a CSV file", path) click_button "Manage permissions for new users" uncheck "Has access to #{support_app.name}?" @@ -125,7 +125,7 @@ def perform_batch_invite_with_user(user, application, organisation:, fixture_fil visit new_batch_invitation_path path = Rails.root.join("test/fixtures", fixture_file) - attach_file("Choose a CSV file of users with names and email addresses", path) + attach_file("Upload a CSV file", path) if organisation select organisation.name, from: "Organisation" end From f068f711d12da6f1e9bb22bb54e910ce92288400 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:33:54 +0100 Subject: [PATCH 07/25] Extract a local variable I want to inline the shared template, doing this will make that change more obvious. --- app/views/batch_invitation_permissions/new.html.erb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 346f0a3e1..a8961ec95 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -6,7 +6,8 @@
<%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - <%= render partial: "shared/user_permissions", locals: { user_object: User.new } %> + <% user_object = User.new %> + <%= render partial: "shared/user_permissions", locals: { user_object: user_object } %> <%= f.submit "Create users and send emails", :class => 'btn btn-success' %> <% end %> From 62c59417f1a8bc9f80c68c04b82c178ebe885809 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:35:59 +0100 Subject: [PATCH 08/25] Inline shared/user_permissions template We want to make fairly extensive changes to how the permissions are selected for this page of the batch user creation process and convert it to the design system. Inlining the shared form will let me make some changes to the logic first and then hopefully make it easier to see the design-system changes that follow. --- .../batch_invitation_permissions/new.html.erb | 73 ++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index a8961ec95..25a2217a4 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -7,7 +7,78 @@
<%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> <% user_object = User.new %> - <%= render partial: "shared/user_permissions", locals: { user_object: user_object } %> + + + + + <% unless user_object.api_user? %> + + <% end %> + + <% if user_object.persisted? %> + + <% end %> + + + + <% applications_and_permissions(user_object).each do |(application, permissions)| + attribute_name = user_object.api_user? ? 'api_user' : 'user' + supported_permission_field_name = "#{attribute_name}[supported_permission_ids][]" + supported_permission_field_prefix = "#{attribute_name}_application_#{application.id}_supported_permission" %> + + + <% if user_object.api_user? %> + <% + # Emulate form.check_box helper: + # http://api.rubyonrails.org/v3.1.3/classes/ActionView/Helpers/FormHelper.html#method-i-check_box + # API Users will always have a "signin" permission for apps for which they have access token. + # The hidden field ensures it is not lost. + %> + <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> + <% else %> + + <% end %> + + <% if user_object.persisted? %> + + <% end %> + + <% end %> + +
ApplicationHas access?<%= "Other" unless user_object.api_user? %> PermissionsLast synced at
+ <% if application.retired? %> + <%= application.name %> + <% else %> + <%= application.name %> + <% end %> + + <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> + <%= check_box_tag supported_permission_field_name, application.signin_permission.id, user_object.has_access_to?(application), + id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> + + <%= label_tag "#{supported_permission_field_prefix}_ids", "Permissions for #{application.name}", class: "rm" %> + <% supported_permissions_options = application.supported_permissions.grantable_from_ui + .inject({}) {|h, per| h.merge(per.name => per.id) } + supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> + <%= select_tag supported_permission_field_name, + options_for_select(supported_permissions_options, + user_object.permission_ids_for(application) - [application.signin_permission.id]), + multiple: true, + class: "chosen-select", + id: "#{supported_permission_field_prefix}_ids", + 'data-module' => 'chosen', + 'data-placeholder' => 'Start typing to search for permissions' + %> + + <% synced_permissions = permissions.select { |p| p.last_synced_at.present? } %> + <% if synced_permissions.any? %> + "> + <%= time_ago_in_words(synced_permissions.map(&:last_synced_at).max) %> ago + + <% else %> + Never + <% end %> +
<%= f.submit "Create users and send emails", :class => 'btn btn-success' %> <% end %> From 19b174f3bc75c5d99699bd8c37cbf6001205a1b4 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:40:59 +0100 Subject: [PATCH 09/25] Simplify view code checking for api_user? `user_object` in this view is always `User.new` and `User.new.api_user?` is `false`, so the predicate conditions on this method can be simplified. --- .../batch_invitation_permissions/new.html.erb | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 25a2217a4..4918cd000 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -11,10 +11,8 @@ Application - <% unless user_object.api_user? %> Has access? - <% end %> - <%= "Other" unless user_object.api_user? %> Permissions + Other Permissions <% if user_object.persisted? %> Last synced at <% end %> @@ -22,7 +20,7 @@ <% applications_and_permissions(user_object).each do |(application, permissions)| - attribute_name = user_object.api_user? ? 'api_user' : 'user' + attribute_name = 'user' supported_permission_field_name = "#{attribute_name}[supported_permission_ids][]" supported_permission_field_prefix = "#{attribute_name}_application_#{application.id}_supported_permission" %> @@ -33,21 +31,11 @@ <%= application.name %> <% end %> - <% if user_object.api_user? %> - <% - # Emulate form.check_box helper: - # http://api.rubyonrails.org/v3.1.3/classes/ActionView/Helpers/FormHelper.html#method-i-check_box - # API Users will always have a "signin" permission for apps for which they have access token. - # The hidden field ensures it is not lost. - %> - <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> - <% else %> <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> <%= check_box_tag supported_permission_field_name, application.signin_permission.id, user_object.has_access_to?(application), id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> - <% end %> <%= label_tag "#{supported_permission_field_prefix}_ids", "Permissions for #{application.name}", class: "rm" %> <% supported_permissions_options = application.supported_permissions.grantable_from_ui From bd979b85c12b87c30b32093ced77e16850150d47 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:43:38 +0100 Subject: [PATCH 10/25] Simplify view code checking for persisted? `user_object` in this view is always `User.new` and `User.new.persisted?` is `false`, so the predicate conditions on this method can be removed. --- .../batch_invitation_permissions/new.html.erb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 4918cd000..7385f3043 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -13,9 +13,6 @@ Application Has access? Other Permissions - <% if user_object.persisted? %> - Last synced at - <% end %> @@ -51,18 +48,6 @@ 'data-placeholder' => 'Start typing to search for permissions' %> - <% if user_object.persisted? %> - - <% synced_permissions = permissions.select { |p| p.last_synced_at.present? } %> - <% if synced_permissions.any? %> - "> - <%= time_ago_in_words(synced_permissions.map(&:last_synced_at).max) %> ago - - <% else %> - Never - <% end %> - - <% end %> <% end %> From 0dfcd8aba71b1e95faaffbf19d005c2dac4dd381 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:45:19 +0100 Subject: [PATCH 11/25] Inline attribute_name --- app/views/batch_invitation_permissions/new.html.erb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 7385f3043..6d48a11b3 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -17,9 +17,8 @@ <% applications_and_permissions(user_object).each do |(application, permissions)| - attribute_name = 'user' - supported_permission_field_name = "#{attribute_name}[supported_permission_ids][]" - supported_permission_field_prefix = "#{attribute_name}_application_#{application.id}_supported_permission" %> + supported_permission_field_name = "user[supported_permission_ids][]" + supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> <% if application.retired? %> From ba0a7112b6fe1e678f1b61bef541e312d3c00b32 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 15:57:25 +0100 Subject: [PATCH 12/25] Don't show retired applications in bulk user create We don't think it makes sense for users to be granted permissions to applications that have been retired, certainly in this scenario of bulk creating users. So rather than stricking-through the application name, simply skip it altogether from the table. --- app/views/batch_invitation_permissions/new.html.erb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 6d48a11b3..87e5f664e 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -19,13 +19,10 @@ <% applications_and_permissions(user_object).each do |(application, permissions)| supported_permission_field_name = "user[supported_permission_ids][]" supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> + <% next if application.retired? %> - <% if application.retired? %> - <%= application.name %> - <% else %> <%= application.name %> - <% end %> <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> From 5cc6bd5248235c20f4d27ea8483196590f3ef423 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:07:17 +0100 Subject: [PATCH 13/25] Remove confusing assignment to `permissions` This block variable is not actually used inside the block, so I'm replacing it with a dummy variable to avoid confusion. --- app/views/batch_invitation_permissions/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 87e5f664e..166f830ef 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -16,7 +16,7 @@ - <% applications_and_permissions(user_object).each do |(application, permissions)| + <% applications_and_permissions(user_object).each do |(application, _)| supported_permission_field_name = "user[supported_permission_ids][]" supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> <% next if application.retired? %> From f42f61ad7e6fc835f85bc26862595fe43f747332 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:16:01 +0100 Subject: [PATCH 14/25] Inline `applications_and_permissions` The second block variable yielded by this helper method is not used, instead we only need the applications. This list of applications was previously obtained from `UserPermissionsControllerMethods#applications_and_permissions` which in turn called `UserPermissionsControllerMethods#visible_applications` with the `user_object` which in this view is always `User.new`. Because `User.new.api_user? == false` the implementation of `UserPermissionsControllerMethods#visible_applications` fell through to return `policy_scope(:user_permission_manageable_application)`. So we can inline this in the view instead of the more convoluted chain of methods. --- app/controllers/batch_invitation_permissions_controller.rb | 2 -- app/views/batch_invitation_permissions/new.html.erb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/batch_invitation_permissions_controller.rb b/app/controllers/batch_invitation_permissions_controller.rb index af73635c1..9143c3b2a 100644 --- a/app/controllers/batch_invitation_permissions_controller.rb +++ b/app/controllers/batch_invitation_permissions_controller.rb @@ -5,8 +5,6 @@ class BatchInvitationPermissionsController < ApplicationController before_action :authorise_to_manage_permissions before_action :prevent_updating - helper_method :applications_and_permissions - def new; end def create diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 166f830ef..2c2e9b1d1 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -16,7 +16,7 @@ - <% applications_and_permissions(user_object).each do |(application, _)| + <% policy_scope(:user_permission_manageable_application).each do |application| supported_permission_field_name = "user[supported_permission_ids][]" supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> <% next if application.retired? %> From bdda4fdbdfacc9ff2da1e70376acbd05669ca1f6 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:22:39 +0100 Subject: [PATCH 15/25] Reject retired applications before iteration rather than inside it --- app/views/batch_invitation_permissions/new.html.erb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 2c2e9b1d1..e7c1064e9 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -16,10 +16,9 @@ - <% policy_scope(:user_permission_manageable_application).each do |application| + <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each do |application| supported_permission_field_name = "user[supported_permission_ids][]" supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> - <% next if application.retired? %> <%= application.name %> From cf42aecfb62e2ae4565aa1a0ba1ecbbac628f537 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:34:51 +0100 Subject: [PATCH 16/25] Remove unnecessary call to `has_access_to?` The `user_object` in this template is always `User.new` and `User.new.has_access_to?(application)` always returns `false` since `User.new` has no associated `application_permissions`. So we can replace this method call with the literal `false` indicating that this checkbox is not selected. --- app/views/batch_invitation_permissions/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index e7c1064e9..daf7358c8 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -25,7 +25,7 @@ <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> - <%= check_box_tag supported_permission_field_name, application.signin_permission.id, user_object.has_access_to?(application), + <%= check_box_tag supported_permission_field_name, application.signin_permission.id, false, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> From 20eecf4f470beff311dca82c1a53bb558d03bad4 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:43:47 +0100 Subject: [PATCH 17/25] Remove unnecessary argument to `options_for_select` In this `user_object` is always `User.new` and `User.new.permission_ids_for(application)` will always return `[]` since a new User does not have any associated `application_permissions`. This means that no applications need to be pre-selected in this call to `options_for_select` and we can rely on the default value for the second argument (which is `nil`). --- app/views/batch_invitation_permissions/new.html.erb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index daf7358c8..9854b4148 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -34,8 +34,7 @@ .inject({}) {|h, per| h.merge(per.name => per.id) } supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> <%= select_tag supported_permission_field_name, - options_for_select(supported_permissions_options, - user_object.permission_ids_for(application) - [application.signin_permission.id]), + options_for_select(supported_permissions_options), multiple: true, class: "chosen-select", id: "#{supported_permission_field_prefix}_ids", From 51743ad066f66a6ceac5ee03dbd54515af2fee3f Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:46:08 +0100 Subject: [PATCH 18/25] Remove unused user_object variable assignment This variable is no longer used in this template, so there's no need to assign it. --- app/views/batch_invitation_permissions/new.html.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 9854b4148..6e6f6aacf 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -6,7 +6,6 @@
<%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - <% user_object = User.new %> From ce2bb4a60d8b939eb9ff84d058fb1c3f803bbf5a Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 16:52:33 +0100 Subject: [PATCH 19/25] Inline supported_permission_field_name variable in template --- app/views/batch_invitation_permissions/new.html.erb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 6e6f6aacf..4d28fb8e0 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -16,7 +16,6 @@ <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each do |application| - supported_permission_field_name = "user[supported_permission_ids][]" supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> - <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each do |application| - supported_permission_field_prefix = "user_application_#{application.id}_supported_permission" %> + <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each do |application| %> <% end %> diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index 59e82e838..b00c8e015 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -7,7 +7,7 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase @user = create(:admin_user) sign_in @user - @app = create(:application, name: "Profound Publisher") + @app = create(:application, name: "Profound Publisher", with_supported_permissions: %w[reader]) @batch_invitation = create(:batch_invitation, user: @user) create( @@ -40,7 +40,7 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase assert_select "table#editable-permissions" do assert_select "td", "Has access to Profound Publisher?" - assert_select "td", "Permissions for Profound Publisher" + assert_select "label", "reader" end end end diff --git a/test/integration/batch_inviting_users_test.rb b/test/integration/batch_inviting_users_test.rb index 1812fd843..dcbe1cc79 100644 --- a/test/integration/batch_inviting_users_test.rb +++ b/test/integration/batch_inviting_users_test.rb @@ -92,7 +92,7 @@ class BatchInvitingUsersTest < ActionDispatch::IntegrationTest uncheck "Has access to #{support_app.name}?" check "Has access to #{@application.name}?" - unselect "reader", from: "Permissions for #{@application.name}" + uncheck "reader" click_button "Create users and send emails" invited_user = User.find_by(email: "fred@example.com") From 7238f87e4112690a30947798d12fe49655e25578 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 19 Sep 2023 12:12:25 +0100 Subject: [PATCH 22/25] Combine the signin permission checkbox with the other checkboxes In the new designs we intend to present these checkboxes together in a single list, so as a step towards that this commit changes the existing design to group all of the checkboxes together. --- app/helpers/batch_invitation_permissions_helper.rb | 9 +++++++++ app/views/batch_invitation_permissions/new.html.erb | 12 +++--------- .../batch_invitation_permissions_controller_test.rb | 2 +- .../batch_invitation_permissions_helper_test.rb | 13 +++++++++++++ 4 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 app/helpers/batch_invitation_permissions_helper.rb create mode 100644 test/helpers/batch_invitation_permissions_helper_test.rb diff --git a/app/helpers/batch_invitation_permissions_helper.rb b/app/helpers/batch_invitation_permissions_helper.rb new file mode 100644 index 000000000..55235c0b6 --- /dev/null +++ b/app/helpers/batch_invitation_permissions_helper.rb @@ -0,0 +1,9 @@ +module BatchInvitationPermissionsHelper + def formatted_permission_name(application_name, permission_name) + if permission_name == SupportedPermission::SIGNIN_NAME + "Has access to #{application_name}?" + else + permission_name + end + end +end diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 83cf5a0da..82f9a6160 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -10,8 +10,7 @@ - - + @@ -21,13 +20,8 @@ <%= application.name %> -
@@ -24,7 +23,7 @@ <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> - <%= check_box_tag supported_permission_field_name, application.signin_permission.id, false, + <%= check_box_tag "user[supported_permission_ids][]", application.signin_permission.id, false, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> @@ -32,7 +31,7 @@ <% supported_permissions_options = application.supported_permissions.grantable_from_ui .inject({}) {|h, per| h.merge(per.name => per.id) } supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> - <%= select_tag supported_permission_field_name, + <%= select_tag "user[supported_permission_ids][]", options_for_select(supported_permissions_options), multiple: true, class: "chosen-select", From d585054dc0892399b275e407a303a015f8da313d Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Mon, 18 Sep 2023 17:09:47 +0100 Subject: [PATCH 20/25] Inline supported_permission_field_prefix variable in template --- app/views/batch_invitation_permissions/new.html.erb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 4d28fb8e0..bc9eb20e0 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -15,19 +15,18 @@
<%= application.name %> - <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> + <%= label_tag "user_application_#{application.id}_supported_permission_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> <%= check_box_tag "user[supported_permission_ids][]", application.signin_permission.id, false, - id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %> + id: "user_application_#{application.id}_supported_permission_#{SupportedPermission::SIGNIN_NAME}" %> - <%= label_tag "#{supported_permission_field_prefix}_ids", "Permissions for #{application.name}", class: "rm" %> + <%= label_tag "user_application_#{application.id}_supported_permission_ids", "Permissions for #{application.name}", class: "rm" %> <% supported_permissions_options = application.supported_permissions.grantable_from_ui .inject({}) {|h, per| h.merge(per.name => per.id) } supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> @@ -35,7 +34,7 @@ options_for_select(supported_permissions_options), multiple: true, class: "chosen-select", - id: "#{supported_permission_field_prefix}_ids", + id: "user_application_#{application.id}_supported_permission_ids", 'data-module' => 'chosen', 'data-placeholder' => 'Start typing to search for permissions' %> From 89f050df6dc17da558f7e95f4cdabcc26c895f88 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 19 Sep 2023 11:54:15 +0100 Subject: [PATCH 21/25] Replace permission select box with checkboxes In the new designs for this page we want to have a list of checkboxes rather than the current `chosen`-powered select box. As a step towards that this commit replaces the select box with checkboxes. I haven't made any attempt to style these new checkboxes since they will shortly be replaced by design-system ones. With this commit it is less obvious that we need to have a distinction between the sign-on permission and the others, but I'm going to live with the rather ugly chained `reject` method here and tidy that up next. --- .../batch_invitation_permissions/new.html.erb | 17 +++++------------ ...ch_invitation_permissions_controller_test.rb | 4 ++-- test/integration/batch_inviting_users_test.rb | 2 +- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index bc9eb20e0..83cf5a0da 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -26,18 +26,11 @@ id: "user_application_#{application.id}_supported_permission_#{SupportedPermission::SIGNIN_NAME}" %> - <%= label_tag "user_application_#{application.id}_supported_permission_ids", "Permissions for #{application.name}", class: "rm" %> - <% supported_permissions_options = application.supported_permissions.grantable_from_ui - .inject({}) {|h, per| h.merge(per.name => per.id) } - supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %> - <%= select_tag "user[supported_permission_ids][]", - options_for_select(supported_permissions_options), - multiple: true, - class: "chosen-select", - id: "user_application_#{application.id}_supported_permission_ids", - 'data-module' => 'chosen', - 'data-placeholder' => 'Start typing to search for permissions' - %> + <% application.supported_permissions.grantable_from_ui.reject { |p| p.name == SupportedPermission::SIGNIN_NAME }.each do |permission| %> + <%= label_tag "user_application_#{application.id}_supported_permission_#{permission.id}", permission.name %> + <%= check_box_tag "user[supported_permission_ids][]", permission.id, false, + id: "user_application_#{application.id}_supported_permission_#{permission.id}" %> + <% end %>
ApplicationHas access?Other PermissionsPermissions
- <%= label_tag "user_application_#{application.id}_supported_permission_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %> - <%= check_box_tag "user[supported_permission_ids][]", application.signin_permission.id, false, - id: "user_application_#{application.id}_supported_permission_#{SupportedPermission::SIGNIN_NAME}" %> - - <% application.supported_permissions.grantable_from_ui.reject { |p| p.name == SupportedPermission::SIGNIN_NAME }.each do |permission| %> - <%= label_tag "user_application_#{application.id}_supported_permission_#{permission.id}", permission.name %> + <% application.supported_permissions.grantable_from_ui.each do |permission| %> + <%= label_tag "user_application_#{application.id}_supported_permission_#{permission.id}", formatted_permission_name(application.name, permission.name) %> <%= check_box_tag "user[supported_permission_ids][]", permission.id, false, id: "user_application_#{application.id}_supported_permission_#{permission.id}" %> <% end %> diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index b00c8e015..24b0760e5 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -39,7 +39,7 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase get :new, params: { batch_invitation_id: @batch_invitation.id } assert_select "table#editable-permissions" do - assert_select "td", "Has access to Profound Publisher?" + assert_select "label", "Has access to Profound Publisher?" assert_select "label", "reader" end end diff --git a/test/helpers/batch_invitation_permissions_helper_test.rb b/test/helpers/batch_invitation_permissions_helper_test.rb new file mode 100644 index 000000000..80760db76 --- /dev/null +++ b/test/helpers/batch_invitation_permissions_helper_test.rb @@ -0,0 +1,13 @@ +require "test_helper" + +class BatchInvitationPermissionsHelperTest < ActionView::TestCase + context "#formatted_permission_name" do + should "return the permission name if permission is not the signin permission" do + assert_equal "Editor", formatted_permission_name("Whitehall", "Editor") + end + + should "include the application name if permission is the signin permission" do + assert_equal "Has access to Whitehall?", formatted_permission_name("Whitehall", SupportedPermission::SIGNIN_NAME) + end + end +end From 119de00487cb53d90480368132dcaceb22fdf81d Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 19 Sep 2023 12:22:20 +0100 Subject: [PATCH 23/25] Extract a `permissions_for(application)` helper method I want to control the sort order of the permissions that appear in the list of permissions so I'm first extracting a helper method and writing a test to make that change easier. --- .../batch_invitation_permissions_helper.rb | 4 ++++ .../batch_invitation_permissions/new.html.erb | 2 +- .../batch_invitation_permissions_helper_test.rb | 15 +++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/helpers/batch_invitation_permissions_helper.rb b/app/helpers/batch_invitation_permissions_helper.rb index 55235c0b6..047b860a7 100644 --- a/app/helpers/batch_invitation_permissions_helper.rb +++ b/app/helpers/batch_invitation_permissions_helper.rb @@ -6,4 +6,8 @@ def formatted_permission_name(application_name, permission_name) permission_name end end + + def permissions_for(application) + application.supported_permissions.grantable_from_ui + end end diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 82f9a6160..1e7b8274d 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -20,7 +20,7 @@ <%= application.name %> - <% application.supported_permissions.grantable_from_ui.each do |permission| %> + <% permissions_for(application).each do |permission| %> <%= label_tag "user_application_#{application.id}_supported_permission_#{permission.id}", formatted_permission_name(application.name, permission.name) %> <%= check_box_tag "user[supported_permission_ids][]", permission.id, false, id: "user_application_#{application.id}_supported_permission_#{permission.id}" %> diff --git a/test/helpers/batch_invitation_permissions_helper_test.rb b/test/helpers/batch_invitation_permissions_helper_test.rb index 80760db76..4317c2bda 100644 --- a/test/helpers/batch_invitation_permissions_helper_test.rb +++ b/test/helpers/batch_invitation_permissions_helper_test.rb @@ -10,4 +10,19 @@ class BatchInvitationPermissionsHelperTest < ActionView::TestCase assert_equal "Has access to Whitehall?", formatted_permission_name("Whitehall", SupportedPermission::SIGNIN_NAME) end end + + context "#permissions_for" do + should "return all of the supported permissions that are grantable from the ui" do + application = create(:application, + name: "Whitehall", + with_supported_permissions: ["Editor", SupportedPermission::SIGNIN_NAME], + with_supported_permissions_not_grantable_from_ui: ["Not grantable"]) + + permission_names = permissions_for(application).map(&:name) + + assert permission_names.include?("Editor") + assert permission_names.include?(SupportedPermission::SIGNIN_NAME) + assert_not permission_names.include?("Not grantable") + end + end end From f1070efb5b85531c044cc68d231b9680e98b956c Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 19 Sep 2023 12:37:00 +0100 Subject: [PATCH 24/25] Sort permissions by name but have signin permission first The most commonly assigned permission is the signin permission so we want to have that at the top of the list of selectable permissions. The others are sorted alphabetically to make them easier to find. --- app/helpers/batch_invitation_permissions_helper.rb | 5 ++++- .../batch_invitation_permissions_helper_test.rb | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/helpers/batch_invitation_permissions_helper.rb b/app/helpers/batch_invitation_permissions_helper.rb index 047b860a7..b863dc48b 100644 --- a/app/helpers/batch_invitation_permissions_helper.rb +++ b/app/helpers/batch_invitation_permissions_helper.rb @@ -8,6 +8,9 @@ def formatted_permission_name(application_name, permission_name) end def permissions_for(application) - application.supported_permissions.grantable_from_ui + all_permissions = application.supported_permissions.grantable_from_ui + signin, others = all_permissions.partition(&:signin?) + + signin + others.sort_by(&:name) end end diff --git a/test/helpers/batch_invitation_permissions_helper_test.rb b/test/helpers/batch_invitation_permissions_helper_test.rb index 4317c2bda..ae2207f8c 100644 --- a/test/helpers/batch_invitation_permissions_helper_test.rb +++ b/test/helpers/batch_invitation_permissions_helper_test.rb @@ -24,5 +24,15 @@ class BatchInvitationPermissionsHelperTest < ActionView::TestCase assert permission_names.include?(SupportedPermission::SIGNIN_NAME) assert_not permission_names.include?("Not grantable") end + + should "sort the permissions alphabetically by name, but with the signin permission first" do + application = create(:application, + name: "Whitehall", + with_supported_permissions: ["Writer", "Editor", SupportedPermission::SIGNIN_NAME]) + + permission_names = permissions_for(application).map(&:name) + + assert_equal [SupportedPermission::SIGNIN_NAME, "Editor", "Writer"], permission_names + end end end From 539a70bfefd80511808bed60518f669d33248e06 Mon Sep 17 00:00:00 2001 From: Chris Lowis Date: Tue, 19 Sep 2023 13:21:33 +0100 Subject: [PATCH 25/25] Migrate BatchInvitationPermissionsController#new to design system This commit migrates the BatchInvitationPermissionsController#new page to the design system. Hopefully with the preparatory commits this diff isn't too hard to follow. I've had to use the "raw" HTML version of the accordian component[1] rather than the one in the publishing component library[2] since the latter doesn't support passing a block of arbitrary content into each section. I think it's a small enough amount of HTML to take this route rather than try to extend the component. The breadcrumb trail could be improved - ideally it would link to the previous "step" in this flow which would involve editing the CSV file and organisation - but we don't have an edit route for this yet and I'm concerned already about the size of this change. I think it's one we could revisit in a subsequent enhancement. [1] https://design-system.service.gov.uk/components/accordion/ [2] https://components.publishing.service.gov.uk/component-guide/accordion --- ...batch_invitation_permissions_controller.rb | 2 + .../batch_invitation_permissions/new.html.erb | 76 +++++++++++-------- ..._invitation_permissions_controller_test.rb | 6 +- 3 files changed, 48 insertions(+), 36 deletions(-) diff --git a/app/controllers/batch_invitation_permissions_controller.rb b/app/controllers/batch_invitation_permissions_controller.rb index 9143c3b2a..57b573d04 100644 --- a/app/controllers/batch_invitation_permissions_controller.rb +++ b/app/controllers/batch_invitation_permissions_controller.rb @@ -5,6 +5,8 @@ class BatchInvitationPermissionsController < ApplicationController before_action :authorise_to_manage_permissions before_action :prevent_updating + layout "admin_layout" + def new; end def create diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 1e7b8274d..f0f89049e 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -1,36 +1,48 @@ <% content_for :title, "Manage permissions for new users" %> +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Home", + url: root_path, + }, + { + title: "Users", + url: users_path, + }, + { + title: "Manage permissions for new users", + } + ] + }) +%> -
-

Manage permissions for new users

-
- -
- <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - - - - - - - - - <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each do |application| %> - - - - - <% end %> - -
ApplicationPermissions
- <%= application.name %> - - <% permissions_for(application).each do |permission| %> - <%= label_tag "user_application_#{application.id}_supported_permission_#{permission.id}", formatted_permission_name(application.name, permission.name) %> - <%= check_box_tag "user[supported_permission_ids][]", permission.id, false, - id: "user_application_#{application.id}_supported_permission_#{permission.id}" %> - <% end %> -
- - <%= f.submit "Create users and send emails", :class => 'btn btn-success' %> +<%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> +
+ <% policy_scope(:user_permission_manageable_application).reject(&:retired?).each_with_index do |application, idx| %> +
+
+

+ + <%= application.name %> + +

+
+
+ <%= render "govuk_publishing_components/components/checkboxes", { + name: "permissions_for_#{application.id}", + heading: "Permissions for #{application.name}", + items: permissions_for(application).map do |permission| + { label: formatted_permission_name(application.name, permission.name), + value: permission.id, + id: "user_application_#{application.id}_supported_permission_#{permission.id}", + name: "user[supported_permission_ids][]" } + end + } %> +
+
<% end %>
+<%= render "govuk_publishing_components/components/button", { text: "Create users and send emails" } %> +<% end %> diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index 24b0760e5..ecf058638 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -38,10 +38,8 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase should "allow selection of application permissions to grant to users" do get :new, params: { batch_invitation_id: @batch_invitation.id } - assert_select "table#editable-permissions" do - assert_select "label", "Has access to Profound Publisher?" - assert_select "label", "reader" - end + assert_select "label", "Has access to Profound Publisher?" + assert_select "label", "reader" end end