From ad9a0398cc51eae8bce57d2fda1b03a2bf08b939 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:01 +0100 Subject: [PATCH 01/41] Move UserFilterHelper#two_step_abbr_tag -> UsersHelper This method is nothing to do with filtering and it's only used in the users index page, so the UsersHelper seems like a better home. --- app/helpers/user_filter_helper.rb | 4 ---- app/helpers/users_helper.rb | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/helpers/user_filter_helper.rb b/app/helpers/user_filter_helper.rb index 9ad1777bc..ee41eea56 100644 --- a/app/helpers/user_filter_helper.rb +++ b/app/helpers/user_filter_helper.rb @@ -10,10 +10,6 @@ def user_role_text "#{params[:role]} users".strip.humanize.capitalize end - def two_step_abbr_tag - tag.abbr("2SV", title: "Two step verification") - end - def title_from(filter_type) if filter_type == :two_step_status safe_join([two_step_abbr_tag, "Status"], " ") diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 2623f963f..a87e0ee7c 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -1,4 +1,8 @@ module UsersHelper + def two_step_abbr_tag + tag.abbr("2SV", title: "Two step verification") + end + def two_step_status(user) if user.has_2sv? "Enabled" From 2722ff27b41abbe54cada0aa70d6be6a7679061d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:02 +0100 Subject: [PATCH 02/41] Rename UserFilterHelper#filtered_user_roles -> assignable_user_roles This method is nothing to do with the filtering that the rest of this helper is concerned with. This method is only used in the app/views/users/_form_fields.html.erb partial template which allows setting or updating a user's role; whereas all the others are used in the filtering of users in the users index page. --- app/helpers/user_filter_helper.rb | 4 ++-- app/views/users/_form_fields.html.erb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/user_filter_helper.rb b/app/helpers/user_filter_helper.rb index ee41eea56..219701b9f 100644 --- a/app/helpers/user_filter_helper.rb +++ b/app/helpers/user_filter_helper.rb @@ -21,7 +21,7 @@ def title_from(filter_type) def user_filter_list_items(filter_type) items = case filter_type when :role - filtered_user_roles + assignable_user_roles when :permission Doorkeeper::Application .joins(:supported_permissions) @@ -64,7 +64,7 @@ def user_filter_list_items(filter_type) list_items.join("\n").html_safe end - def filtered_user_roles + def assignable_user_roles current_user.manageable_roles end diff --git a/app/views/users/_form_fields.html.erb b/app/views/users/_form_fields.html.erb index 5dc3484c1..90fb69452 100644 --- a/app/views/users/_form_fields.html.erb +++ b/app/views/users/_form_fields.html.erb @@ -28,7 +28,7 @@ <% if policy(User).assign_role? && @user.reason_for_2sv_exemption.blank? %>

<%= f.label :role %>
- <%= f.select :role, options_for_select(filtered_user_roles.map(&:humanize).zip(filtered_user_roles), f.object.role), {}, class: "chosen-select form-control", 'data-module' => 'chosen' %> + <%= f.select :role, options_for_select(assignable_user_roles.map(&:humanize).zip(assignable_user_roles), f.object.role), {}, class: "chosen-select form-control", 'data-module' => 'chosen' %> Superadmins can create and edit all user types and edit applications.
Admins can create and edit normal users.
From 9a2e75f922f420659790ab8159a48954041e20c5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:03 +0100 Subject: [PATCH 03/41] Move UserFilterHelper#assignable_user_roles -> UsersHelper This method is nothing to do with the filtering that the rest of this helper is concerned with. This method is only used in the app/views/users/_form_fields.html.erb partial template which allows setting or updating a user's role; whereas all the others are used in the filtering of users in the users index page. --- app/helpers/user_filter_helper.rb | 4 ---- app/helpers/users_helper.rb | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/helpers/user_filter_helper.rb b/app/helpers/user_filter_helper.rb index 219701b9f..409c5ce03 100644 --- a/app/helpers/user_filter_helper.rb +++ b/app/helpers/user_filter_helper.rb @@ -64,10 +64,6 @@ def user_filter_list_items(filter_type) list_items.join("\n").html_safe end - def assignable_user_roles - current_user.manageable_roles - end - def value_from(filter_type) value = params[filter_type] return nil if value.blank? diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index a87e0ee7c..636cbd8b4 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -53,4 +53,8 @@ def link_to_users_csv(text, params, options = {}) def formatted_number_of_users(users) pluralize(number_with_delimiter(users.total_count), "user") end + + def assignable_user_roles + current_user.manageable_roles + end end From 64c8ba946096bb78d35983f69a141662b5daca4c Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:04 +0100 Subject: [PATCH 04/41] Remove filter functionality from users index page Trello: https://trello.com/c/2gvEJKw9 This filtering functionality doesn't comform to the Design Sytem so we're going to have to re-implement it so that it does. As a first step I'm removing the old implementation which should hopefully make the addition of the new functionality easier to follow. I've inlined UserFilterHelper#user_role_text to a hard-coded string, because it no longer varies according to the filtered role. --- .../legacy/modules/dropdown_filter.js | 49 ------ app/assets/stylesheets/legacy/_filters.scss | 102 ----------- app/assets/stylesheets/legacy_layout.scss | 1 - app/controllers/users_controller.rb | 26 +-- app/helpers/user_filter_helper.rb | 106 ------------ app/helpers/users_helper.rb | 5 - app/models/user.rb | 33 ---- app/views/users/_user_filter.html.erb | 45 ----- app/views/users/_user_filter_group.html.erb | 40 ----- .../users/_user_filter_group_no_js.html.erb | 17 -- app/views/users/index.html.erb | 8 +- .../legacy/modules/dropdown_filter.spec.js | 81 --------- test/controllers/users_controller_test.rb | 95 +---------- test/integration/admin_user_index_test.rb | 161 ------------------ test/models/user_test.rb | 80 +-------- 15 files changed, 8 insertions(+), 841 deletions(-) delete mode 100644 app/assets/javascripts/legacy/modules/dropdown_filter.js delete mode 100644 app/assets/stylesheets/legacy/_filters.scss delete mode 100644 app/helpers/user_filter_helper.rb delete mode 100644 app/views/users/_user_filter.html.erb delete mode 100644 app/views/users/_user_filter_group.html.erb delete mode 100644 app/views/users/_user_filter_group_no_js.html.erb delete mode 100644 spec/javascripts/legacy/modules/dropdown_filter.spec.js diff --git a/app/assets/javascripts/legacy/modules/dropdown_filter.js b/app/assets/javascripts/legacy/modules/dropdown_filter.js deleted file mode 100644 index 79572bbb4..000000000 --- a/app/assets/javascripts/legacy/modules/dropdown_filter.js +++ /dev/null @@ -1,49 +0,0 @@ -(function (Modules) { - 'use strict' - Modules.DropdownFilter = function () { - var that = this - that.start = function (element) { - var list = element.find('.js-filter-list') - var listItems = list.find('li:not(:first)') - var listInput = element.find('.js-filter-list-input') - - // Prevent dropdowns with text inputs from closing when - // interacting with them - element.on('click', '.js-filter-list-input', function (event) { event.stopPropagation() }) - - element.on('shown.bs.dropdown', focusInput) - element.on('keyup change', '.js-filter-list-input', filterListBasedOnInput) - element.on('submit', 'form', openFirstVisibleLink) - - // Set explicit width inline, so filtering doesn't change dropdown size - list.width(list.width()) - - function filterListBasedOnInput (event) { - var searchString = $.trim(listInput.val()) - var regExp = new RegExp(searchString, 'i') - - listItems.each(function () { - var item = $(this) - if (item.text().search(regExp) > -1) { - item.show() - } else { - item.hide() - } - }) - } - - function openFirstVisibleLink (evt) { - evt.preventDefault() - var link = list.find('a:visible').first() - GOVUKAdmin.redirect(link.attr('href')) - } - - function focusInput (event) { - var container = $(event.target) - setTimeout(function () { - container.find('input[type="text"]').focus() - }, 50) - } - } - } -})(window.GOVUKAdmin.Modules) diff --git a/app/assets/stylesheets/legacy/_filters.scss b/app/assets/stylesheets/legacy/_filters.scss deleted file mode 100644 index ef4e4b290..000000000 --- a/app/assets/stylesheets/legacy/_filters.scss +++ /dev/null @@ -1,102 +0,0 @@ -// stylelint-disable selector-no-qualifying-type -- There are a large number -// of violations of this rule in this file that should be fixed. - -.filters .panel-heading { - background: transparent; -} - -a.filter-option, -a.filter-option:visited, -.filter-option { - color: inherit !important; // stylelint-disable-line declaration-no-important - - .glyphicon { - @extend %glyphicon-smaller-than-text; - } - - &.filter-selected { - font-weight: bold; - background: #eeeeee; - - &:hover, - &:focus, - &:active { - background: #dddddd; - } - } - - .selected-and-available & { - float: left; - - &:first-child { - border-top-right-radius: 0; - border-bottom-right-radius: 0; - } - - &:first-child + .filter-option { - border-top-left-radius: 0; - border-bottom-left-radius: 0; - border-left: 1px solid #dddddd; - } - } -} - -.remove-filters { - float: right; - margin: 5px; -} - -.filter-by-name-field { - width: 290px; - - &::placeholder { - color: #757575; - } -} - -// Nav compact - -.nav-compact > li > a { - padding: 5px 10px; -} - -// Nav pills - -.nav-pill-text { - position: relative; - display: block; - padding: 10px 15px; -} - -.nav-compact .nav-pill-text { - padding: 5px 10px; -} - -.nav-compact .nav-pill-text:first-child { - padding-left: 0; -} - -// Dropdown filter for Organisation & Permission - -.filter-by-organisation-menu, -.filter-by-permission-menu { - .dropdown-menu { - max-height: 300px; - overflow-y: scroll; - } - - abbr { - cursor: pointer; - } -} - -// List filter - -.list-filter { - background: #eeeeee; - padding: 10px $default-horizontal-margin; - margin-top: -5px; - margin-bottom: $default-vertical-margin / 2; - border-radius: $border-radius-base $border-radius-base 0 0; - border-bottom: 1px solid $dropdown-border; -} diff --git a/app/assets/stylesheets/legacy_layout.scss b/app/assets/stylesheets/legacy_layout.scss index 9ec297016..8c34c62b8 100644 --- a/app/assets/stylesheets/legacy_layout.scss +++ b/app/assets/stylesheets/legacy_layout.scss @@ -2,6 +2,5 @@ @import "chosen"; @import "legacy/bootstrap_chosen"; -@import "legacy/filters"; @import "legacy/admin"; @import "legacy/thin_form"; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9e8cdba12..a0621d45f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -8,7 +8,7 @@ class UsersController < ApplicationController before_action :authenticate_user!, except: :show before_action :load_and_authorize_user, except: %i[index show] before_action :allow_no_application_access, only: [:update] - helper_method :applications_and_permissions, :any_filter? + helper_method :applications_and_permissions respond_to :html before_action :doorkeeper_authorize!, only: :show @@ -29,7 +29,6 @@ def index authorize User @users = policy_scope(User).includes(:organisation).order(:name) - filter_users if any_filter? respond_to do |format| format.html do paginate_users @@ -133,20 +132,6 @@ def load_and_authorize_user authorize @user end - def filter_users - @users = @users.filter_by_name(params[:filter]) if params[:filter].present? - @users = @users.with_role(params[:role]) if can_filter_role? - @users = @users.with_permission(params[:permission]) if params[:permission].present? - @users = @users.with_organisation(params[:organisation]) if params[:organisation].present? - @users = @users.with_status(params[:status]) if params[:status].present? - @users = @users.with_2sv_enabled(params[:two_step_status]) if params[:two_step_status].present? - end - - def can_filter_role? - params[:role].present? && - current_user.manageable_roles.include?(params[:role]) - end - def should_include_permissions? params[:format] == "csv" end @@ -155,15 +140,6 @@ def paginate_users @users = @users.page(params[:page]).per(25) end - def any_filter? - params[:filter].present? || - params[:role].present? || - params[:permission].present? || - params[:status].present? || - params[:organisation].present? || - params[:two_step_status].present? - end - def validate_token_matches_client_id # FIXME: Once gds-sso is updated everywhere, this should always validate # the client_id param. It should 401 if no client_id is given. diff --git a/app/helpers/user_filter_helper.rb b/app/helpers/user_filter_helper.rb deleted file mode 100644 index 409c5ce03..000000000 --- a/app/helpers/user_filter_helper.rb +++ /dev/null @@ -1,106 +0,0 @@ -module UserFilterHelper - def current_path_with_filter(filter_type, filter_value) - query_parameters = (request.query_parameters.clone || {}) - filter_value.nil? ? query_parameters.delete(filter_type) : query_parameters[filter_type] = filter_value - query_string = query_parameters.map { |k, v| "#{k}=#{v}" }.join("&") - "#{request.path_info}?#{query_string}" - end - - def user_role_text - "#{params[:role]} users".strip.humanize.capitalize - end - - def title_from(filter_type) - if filter_type == :two_step_status - safe_join([two_step_abbr_tag, "Status"], " ") - else - filter_type.to_s.humanize.capitalize - end - end - - def user_filter_list_items(filter_type) - items = case filter_type - when :role - assignable_user_roles - when :permission - Doorkeeper::Application - .joins(:supported_permissions) - .order("oauth_applications.name", "supported_permissions.name") - .pluck("supported_permissions.id", "oauth_applications.name", "supported_permissions.name") - .map { |e| [e[0], "#{e[1]} #{e[2]}"] } - when :status - User::USER_STATUSES - when :organisation - if current_user.super_organisation_admin? - current_user.organisation.subtree.order(:name).joins(:users).uniq.map { |org| [org.id, org.name_with_abbreviation] } - else - Organisation.order(:name).joins(:users).uniq.map { |org| [org.id, org.name_with_abbreviation] } - end - when :two_step_status - [["true", "Enabled"], ["false", "Not set up"], ["exempt", "Exempted"]] - end - - list_items = items.map do |item| - if item.is_a? String - item_id = item - item_name = item.humanize - else - item_id = item[0].to_s - item_name = item[1] - end - tag.li( - link_to(item_name, current_path_with_filter(filter_type, item_id)), - class: params[filter_type] == item_id ? "active" : "", - ) - end - - list_items << tag.li( - link_to( - "All #{title_from(filter_type).pluralize}".html_safe, - current_path_with_filter(filter_type, nil), - ), - ) - - list_items.join("\n").html_safe - end - - def value_from(filter_type) - value = params[filter_type] - return nil if value.blank? - - case filter_type - when :organisation - org = Organisation.find(value) - if org.abbreviation.presence - tag.abbr(org.abbreviation, title: org.name) - else - org.name - end - when :permission - Doorkeeper::Application - .joins(:supported_permissions) - .where("supported_permissions.id = ?", value) - .pick("oauth_applications.name", "supported_permissions.name") - .join(" ") - when :two_step_status - case value - when "true" - "Enabled" - when "exempt" - "Exempted" - else - "Not set up" - end - else - value.humanize.capitalize - end - end - - def any_filter? - params[:filter].present? || - params[:role].present? || - params[:permission].present? || - params[:status].present? || - params[:organisation].present? - end -end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 636cbd8b4..e1fb6d965 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -45,11 +45,6 @@ def sync_needed?(permissions) max_updated_at.present? && max_last_synced_at.present? ? max_updated_at > max_last_synced_at : false end - def link_to_users_csv(text, params, options = {}) - merged_params = params.permit(:filter, :role, :permission, :status, :organisation, :two_step_status).merge(format: "csv") - link_to text, merged_params, options - end - def formatted_number_of_users(users) pluralize(number_with_delimiter(users.total_count), "user") end diff --git a/app/models/user.rb b/app/models/user.rb index 063bbcec9..01ee13d2c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -61,10 +61,6 @@ class User < ApplicationRecord scope :web_users, -> { where(api_user: false) } scope :not_suspended, -> { where(suspended_at: nil) } - scope :with_role, ->(role_name) { where(role: role_name) } - scope :with_permission, ->(permission_id) { joins(:supported_permissions).where("supported_permissions.id = ?", permission_id) } - scope :with_organisation, ->(org_id) { where(organisation_id: org_id) } - scope :filter_by_name, ->(filter_param) { where("users.email like ? OR users.name like ?", "%#{filter_param.strip}%", "%#{filter_param.strip}%") } scope :last_signed_in_on, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) = date(?)", date) } scope :last_signed_in_before, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) < date(?)", date) } scope :last_signed_in_after, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) >= date(?)", date) } @@ -72,35 +68,6 @@ class User < ApplicationRecord scope :expired_never_signed_in, -> { never_signed_in.where("invitation_sent_at < ?", NEVER_SIGNED_IN_EXPIRY_PERIOD.ago) } scope :not_recently_unsuspended, -> { where(["unsuspended_at IS NULL OR unsuspended_at < ?", UNSUSPENSION_GRACE_PERIOD.ago]) } scope :with_access_to_application, ->(application) { UsersWithAccess.new(self, application).users } - scope :with_2sv_enabled, - lambda { |enabled| - case enabled - when "exempt" - where("reason_for_2sv_exemption IS NOT NULL") - when "true" - where("otp_secret_key IS NOT NULL") - else - where("otp_secret_key IS NULL AND reason_for_2sv_exemption IS NULL") - end - } - - scope :with_status, - lambda { |status| - case status - when USER_STATUS_SUSPENDED - where.not(suspended_at: nil) - when USER_STATUS_INVITED - where.not(invitation_sent_at: nil).where(invitation_accepted_at: nil) - when USER_STATUS_LOCKED - where.not(locked_at: nil) - when USER_STATUS_ACTIVE - where(suspended_at: nil, locked_at: nil) - .where(arel_table[:invitation_sent_at].eq(nil) - .or(arel_table[:invitation_accepted_at].not_eq(nil))) - else - raise NotImplementedError, "Filtering by status '#{status}' not implemented." - end - } def require_2sv? return require_2sv unless organisation diff --git a/app/views/users/_user_filter.html.erb b/app/views/users/_user_filter.html.erb deleted file mode 100644 index 92520bf14..000000000 --- a/app/views/users/_user_filter.html.erb +++ /dev/null @@ -1,45 +0,0 @@ -

- -<%= render partial: 'user_filter_group_no_js', locals: {filter_type: :role} %> -<%= render partial: 'user_filter_group_no_js', locals: {filter_type: :permission} %> -<%= render partial: 'user_filter_group_no_js', locals: {filter_type: :status} %> -<%= render partial: 'user_filter_group_no_js', locals: {filter_type: :organisation} %> -<%= render partial: 'user_filter_group_no_js', locals: {filter_type: :two_step_status} %> diff --git a/app/views/users/_user_filter_group.html.erb b/app/views/users/_user_filter_group.html.erb deleted file mode 100644 index 93f69ba84..000000000 --- a/app/views/users/_user_filter_group.html.erb +++ /dev/null @@ -1,40 +0,0 @@ -<% - filter_title = title_from(filter_type) - filter_value = value_from(filter_type) -%> -
  • <% if params[filter_type] %>selected-and-available<% end %>" <% if filter_type.in?([:organisation, :permission]) %>data-module="dropdown-filter"<% end %>> - <% if params[filter_type] %> - <%= link_to current_path_with_filter(filter_type, nil), - class: 'filter-option filter-selected', - title: 'Remove filter' do %> - Remove - <% end %> - <% end %> - <%= link_to current_path_with_filter(filter_type, ''), - class: "filter-option #{params[filter_type] ? 'filter-selected' : ''}", - data: { toggle: 'dropdown' }, - role: 'button' do %> - <%= filter_value || filter_title %> - <%= filter_title %> - - <% end %> - -
    • diff --git a/app/views/users/_user_filter_group_no_js.html.erb b/app/views/users/_user_filter_group_no_js.html.erb deleted file mode 100644 index 4c216a274..000000000 --- a/app/views/users/_user_filter_group_no_js.html.erb +++ /dev/null @@ -1,17 +0,0 @@ -<% if params[filter_type] %> - <% filter_title = title_from(filter_type) %> -
    - -
    -<% end %> diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index c5a92234e..708c990ca 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -1,7 +1,7 @@ -<% content_for :title, user_role_text %> +<% content_for :title, "Users" %>
    -

    <%= user_role_text %>

    +

    Users

    <% if policy(User).new? %> <%= link_to "Create user", new_user_invitation_path, class: "btn btn-success add-right-margin" %> @@ -15,11 +15,9 @@

    <%= formatted_number_of_users(@users) %>

    - <%= link_to_users_csv "Export #{formatted_number_of_users(@users)} as CSV", params, class: "btn btn-default pull-right" %> + <%= link_to "Export #{formatted_number_of_users(@users)} as CSV", url_for(format: "csv"), class: "btn btn-default pull-right" %>
    -<%= render partial: "user_filter" %> - diff --git a/spec/javascripts/legacy/modules/dropdown_filter.spec.js b/spec/javascripts/legacy/modules/dropdown_filter.spec.js deleted file mode 100644 index 02561019f..000000000 --- a/spec/javascripts/legacy/modules/dropdown_filter.spec.js +++ /dev/null @@ -1,81 +0,0 @@ -describe('A dropdown filter module', function () { - 'use strict' - - var dropdownFilter - var listElement - - beforeEach(function () { - listElement = $('
    ' + - '' + - '
    ') - - $('body').append(listElement) - dropdownFilter = new GOVUKAdmin.Modules.DropdownFilter() - dropdownFilter.start(listElement) - }) - - afterEach(function () { - listElement.remove() - }) - - it('filters the dropdown list based on input', function () { - filterBy('another') - expect(listElement.find('.first').is(':visible')).toBe(false) - expect(listElement.find('.second').is(':visible')).toBe(true) - - filterBy('something') - expect(listElement.find('.first').is(':visible')).toBe(true) - expect(listElement.find('.second').is(':visible')).toBe(false) - - filterBy('thing') - expect(listElement.find('.first').is(':visible')).toBe(true) - expect(listElement.find('.second').is(':visible')).toBe(true) - - filterBy('not a thing') - expect(listElement.find('.first').is(':visible')).toBe(false) - expect(listElement.find('.second').is(':visible')).toBe(false) - }) - - it('keeps the first list item visible, the filter input', function () { - filterBy('another') - expect(listElement.find('li:first').is(':visible')).toBe(true) - - filterBy('') - expect(listElement.find('li:first').is(':visible')).toBe(true) - - filterBy('not a thing') - expect(listElement.find('li:first').is(':visible')).toBe(true) - }) - - it('shows all items when no input is entered', function () { - filterBy('another') - filterBy('') - expect(listElement.find('.first').is(':visible')).toBe(true) - expect(listElement.find('.second').is(':visible')).toBe(true) - }) - - describe('when the form is submitted', function () { - it('opens the first visible link', function () { - spyOn(GOVUKAdmin, 'redirect') - filterBy('another') - listElement.find('form').trigger('submit') - expect(GOVUKAdmin.redirect).toHaveBeenCalledWith('/second-link') - }) - }) - - function filterBy (value) { - listElement.find('input').val(value).trigger('change') - } -}) diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 82f45e744..125fc5718 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -318,28 +318,7 @@ def change_user_password(user_factory, new_password) assert_equal "text/csv", @response.media_type end - should "export filtered users by role" do - create(:user) - get :index, params: { role: "normal", format: :csv } - lines = @response.body.lines - assert_equal(2, lines.length) - end - - should "export filtered users by all fields" do - organisation = create(:organisation, name: "Cookies Of Other Lands", abbreviation: "COOL") - - create(:suspended_user, name: "Special cookie", email: "specialcookie@email.com", role: "normal", organisation:) - create(:suspended_user, name: "Normal cookie", email: "normalcookie@email.com", role: "normal", organisation:) - - assert_equal(3, User.count) - - get :index, params: { role: "normal", status: "suspended", organisation:, two_step_status: false, filter: "special", format: :csv } - - lines = @response.body.lines - assert_equal(2, lines.length) - end - - should "export all users when no filter selected" do + should "export all users" do create(:user) get :index, params: { format: :csv } lines = @response.body.lines @@ -347,78 +326,6 @@ def change_user_password(user_factory, new_password) end end - context "filter" do - setup do - create(:user, email: "not_admin@gov.uk") - end - - should "filter results to users where their name or email contains the string" do - create(:user, email: "special@gov.uk") - create(:user, name: "Someone special", email: "someone@gov.uk") - - get :index, params: { filter: "special" } - - assert_select "tbody tr", count: 2 - assert_select "td.email", /special@gov.uk/ - assert_select "td.email", /someone@gov.uk/ - end - - should "scope list of users by role" do - get :index, params: { role: "normal" } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /admin@gov.uk/ - end - - should "scope filtered list of users by role" do - create(:organisation_admin_user, email: "xyz@gov.uk") - - get :index, params: { filter: "admin", role: Roles::Admin.role_name } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /admin@gov.uk/ - end - - should "scope list of users by permission" do - user_application_permissions = create_list(:user_application_permission, 2) - - user_application_permissions.each do |uap| - get :index, params: { permission: uap.supported_permission_id } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /#{uap.user.email}/ - end - end - end - - should "scope list of users by status" do - create(:suspended_user, email: "suspended_user@gov.uk") - - get :index, params: { status: "suspended" } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /suspended_user@gov.uk/ - end - - should "scope list of users by status and role" do - create(:suspended_user, email: "suspended_user@gov.uk", role: Roles::Admin.role_name) - create(:suspended_user, email: "normal_suspended_user@gov.uk") - - get :index, params: { status: "suspended", role: Roles::Admin.role_name } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /suspended_user@gov.uk/ - end - - should "scope list of users by organisation" do - user = create(:user_in_organisation, email: "orgmember@gov.uk") - - get :index, params: { organisation: user.organisation.id } - - assert_select "tbody tr", count: 1 - assert_select "td.email", /orgmember@gov.uk/ - end - context "as superadmin" do should "not list api users" do @user.update_column(:role, Roles::Superadmin.role_name) diff --git a/test/integration/admin_user_index_test.rb b/test/integration/admin_user_index_test.rb index 33f310539..e1e49425d 100644 --- a/test/integration/admin_user_index_test.rb +++ b/test/integration/admin_user_index_test.rb @@ -44,166 +44,5 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest actual_last_sign_in_strings = page.all("table tr td.last-sign-in").map(&:text).map(&:strip)[0..1] assert_equal ["5 minutes ago", "never signed in"], actual_last_sign_in_strings end - - should "be able to filter users" do - visit "/users" - - fill_in "Name or email", with: "bb" - click_on "Filter" - - assert page.has_content?("Abbey abbey@example.com") - assert page.has_content?("Abbot mr_ab@example.com") - assert page.has_content?("Bert bbbert@example.com") - assert page.has_content?("Eddie eddie_bb@example.com") - - assert_not page.has_content?("Aardvark aardvark@example.com") - assert_not page.has_content?("Ernie ernie@example.com") - - click_on "Users" - - assert page.has_content?("Aardvark aardvark@example.com") - end - - should "filter users by role" do - visit "/users" - - assert_role_not_present("Superadmin") - - select_role("Normal") - - assert_equal User.with_role(:normal).count, page.all("table tbody tr").count - assert_not page.has_content?("Admin User admin@example.com") - User.with_role(:normal).each do |normal_user| - assert page.has_content?(normal_user.email) - end - - select_role("All Roles") - - %w[Aardvark Abbot Abbey Admin].each do |user_name| - assert page.has_content?(user_name) - end - end - - should "filter users by permission" do - uap = create(:user_application_permission, user: User.find_by(name: "Ernie")) - visit "/users" - - select_permission("#{uap.application.name} #{uap.supported_permission.name}") - - assert_equal 1, page.all("table tbody tr").count - within ".table" do - assert page.has_content?("Ernie") - (User.pluck(:name) - %w[Ernie]).each do |name| - assert_not page.has_content?(name) - end - end - - select_permission("All Permissions") - - %w[Aardvark Abbot Abbey Admin].each do |user_name| - assert page.has_content?(user_name) - end - end - - should "filter users by status" do - visit "/users" - - select_status("Suspended") - - assert_equal 1, page.all("table tbody tr").count - assert_not page.has_content?("Aardvark") - assert page.has_content?("Suspended McFee") - - select_status("All Statuses") - - %w[Aardvark Abbot Abbey Admin Suspended].each do |user_name| - assert page.has_content?(user_name) - end - end - - should "filter users by organisation" do - visit "/users" - - select_organisation("Org 1") - assert_equal 1, page.all("table tbody tr").count - assert_not page.has_content?("Aardvark") - assert page.has_content?("Ed") - - select_organisation("All Organisations") - - %w[Aardvark Abbot Abbey Admin Suspended].each do |user_name| - assert page.has_content?(user_name) - end - end - - should "filter users by 2SV status" do - visit "/users" - total_enabled = 1 - total_disabled = 7 - total_exempted = 1 - - within ".filter-by-two_step_status-menu .dropdown-menu" do - click_on "Enabled" - end - - assert has_css?("td", text: "Enabled", count: total_enabled) - assert has_css?(".filter-by-two_step_status-menu .filter-selected span", text: "Enabled") - assert has_no_css?("td", text: "Not set up") - assert has_no_css?("td", text: "Exempted") - - within ".filter-by-two_step_status-menu .dropdown-menu" do - click_on "Not set up" - end - - assert has_no_css?("td", text: "Enabled") - assert has_css?("td", text: "Not set up", count: total_disabled) - assert has_css?(".filter-by-two_step_status-menu .filter-selected span", text: "Not set up") - assert has_no_css?("td", text: "Exempted") - - within ".filter-by-two_step_status-menu .dropdown-menu" do - click_on "Exempted" - end - - assert has_css?("td", text: "Exempted", count: total_exempted) - assert has_css?(".filter-by-two_step_status-menu .filter-selected span", text: "Exempted") - assert has_no_css?("td", text: "Not set up") - assert has_no_css?("td", text: "Enabled") - end - end - - def select_organisation(organisation_name) - within ".filter-by-organisation-menu .dropdown-menu" do - click_on organisation_name - end - end - - def select_status(status_name) - within ".filter-by-status-menu .dropdown-menu" do - click_on status_name - end - end - - def assert_role_not_present(role_name) - within ".filter-by-role-menu" do - click_on "Role", match: :prefer_exact - within ".dropdown-menu" do - assert page.has_no_content? role_name - end - end - end - - def select_role(role_name) - within ".filter-by-role-menu" do - click_on "Role", match: :prefer_exact - within ".dropdown-menu" do - click_on role_name - end - end - end - - def select_permission(permission_name) - within ".filter-by-permission-menu" do - click_on permission_name - end end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 8670684f3..ab332636c 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -229,37 +229,6 @@ def setup assert_equal [never_signed_in_ages_ago], User.expired_never_signed_in end - context ".with_role" do - setup do - @admin = create(:admin_user) - @normal = create(:user) - end - - should "return users with specified role" do - assert_includes User.with_role(:admin), @admin - end - - should "not return users with a role other than the specified role" do - assert_not_includes User.with_role(:admin), @normal - end - end - - context ".with_permission" do - should "only return users with the specified permission" do - joe = create(:user) - amy = create(:user) - app = create(:application, with_supported_permissions: %w[manage]) - signin_perm = app.signin_permission # created in a callback in the Application model - manage_perm = app.supported_permissions.find_by(name: "manage") - create(:user_application_permission, user: joe, application: app, supported_permission: signin_perm) - create(:user_application_permission, user: amy, application: app, supported_permission: signin_perm) - create(:user_application_permission, user: amy, application: app, supported_permission: manage_perm) - - assert_equal User.with_permission(signin_perm.id), [joe, amy] - assert_equal User.with_permission(manage_perm.id), [amy] - end - end - context ".not_recently_unsuspended" do should "return users who have never been unsuspended" do assert_includes User.not_recently_unsuspended, @user @@ -280,28 +249,9 @@ def setup end end - context ".with_2sv_enabled" do - should "return users with 2SV enabled when true" do - enabled_user = create(:two_step_enabled_user) - exempt_user = create(:two_step_exempted_user) - - enabled_users = User.with_2sv_enabled("true") - assert_equal 1, enabled_users.count - assert_equal enabled_user, enabled_users.first - - disabled_users = User.with_2sv_enabled("false") - assert_equal 1, disabled_users.count - assert_equal @user, disabled_users.first - - exempt_users = User.with_2sv_enabled("exempt") - assert_equal 1, exempt_users.count - assert_equal exempt_user, exempt_users.first - end - - should "encrypt otp_secret_key" do - enabled_user = create(:two_step_enabled_user) - assert enabled_user.encrypted_attribute?(:otp_secret_key) - end + should "encrypt otp_secret_key" do + enabled_user = create(:two_step_enabled_user) + assert enabled_user.encrypted_attribute?(:otp_secret_key) end context "email validation" do @@ -645,30 +595,6 @@ def setup @invited = User.invite!(name: "Oberyn Martell", email: "redviper@dorne.com") end - context "filtering" do - should "be able to filter by all statuses" do - User::USER_STATUSES.each do |status| - assert_not_empty User.with_status(status) - end - end - - should "filter suspended" do - assert_equal [@suspended], User.with_status(User::USER_STATUS_SUSPENDED).all - end - - should "filter invited" do - assert_equal [@invited], User.with_status(User::USER_STATUS_INVITED).all - end - - should "filter locked" do - assert_equal [@locked], User.with_status(User::USER_STATUS_LOCKED).all - end - - should "filter active" do - assert_equal [@user], User.with_status(User::USER_STATUS_ACTIVE).all - end - end - context "detecting" do should "detect suspended" do assert_equal "suspended", @suspended.status From 62eb2f65c9655dcc2a5098f7e3ea277f65dece8d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:06 +0100 Subject: [PATCH 05/41] Remove "Unlock account" button from users index page Trello: https://trello.com/c/2gvEJKw9 The new design does not include this button and I've confirmed that its removal is intentional. It is still possible to unlock accounts from the user edit page. --- app/views/users/index.html.erb | 5 ----- test/integration/event_log_creation_test.rb | 5 ++--- test/integration/user_locking_test.rb | 15 --------------- 3 files changed, 2 insertions(+), 23 deletions(-) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 708c990ca..9d0251999 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -59,11 +59,6 @@ <%= submit_tag "Resend signup email", :class => 'btn btn-sm btn-default' %> <% end %> <% end %> - <% if user.access_locked? %> - <%= form_tag unlock_user_path(user) do %> - <%= submit_tag "Unlock account", :class => 'btn btn-default' %> - <% end %> - <% end %> <% end %> diff --git a/test/integration/event_log_creation_test.rb b/test/integration/event_log_creation_test.rb index d709cfe34..b3206a970 100644 --- a/test/integration/event_log_creation_test.rb +++ b/test/integration/event_log_creation_test.rb @@ -129,9 +129,8 @@ class EventLogCreationIntegrationTest < ActionDispatch::IntegrationTest visit root_path signin_with(@admin) - first_letter_of_name = @user.name[0] - visit users_path(letter: first_letter_of_name) - click_on "Unlock" + visit user_path(@user) + click_on "Unlock account" visit event_logs_user_path(@user) assert page.has_content?("#{EventLog::MANUAL_ACCOUNT_UNLOCK.description} by #{@admin.name}") diff --git a/test/integration/user_locking_test.rb b/test/integration/user_locking_test.rb index a70c83f8e..52416e154 100644 --- a/test/integration/user_locking_test.rb +++ b/test/integration/user_locking_test.rb @@ -34,21 +34,6 @@ class UserLockingTest < ActionDispatch::IntegrationTest end end - should "be reversible by admins" do - admin = create(:admin_user) - user = create(:user) - user.lock_access! - - visit root_path - signin_with(admin) - first_letter_of_name = user.name[0] - visit users_path(letter: first_letter_of_name) - click_button "Unlock account" - - user.reload - assert_not user.access_locked? - end - should "be reversible from the user edit page" do admin = create(:admin_user) user = create(:user) From 21970503e2294d5ee89b71d2c37f1a18b3cfae6b Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:07 +0100 Subject: [PATCH 06/41] Remove "Resend signup email" button from users index page Trello: https://trello.com/c/2gvEJKw9 The new design does not include this button and I've confirmed that its removal is intentional. It is still possible to resend a signup email from the user edit page. --- app/views/users/index.html.erb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 9d0251999..1c043ebe4 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -54,11 +54,6 @@ <% end %> From 12c780e2344e2fc8ec549617b0a18d39c94e0013 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:08 +0100 Subject: [PATCH 07/41] Move users index page to use the Design System Trello: https://trello.com/c/2gvEJKw9 The layout is a bit noddy, but my aim was to port as much of the content & functionality of the page as possible to use the GOV.UK Design System before then iterating towards the proposed design for the page. --- app/controllers/users_controller.rb | 2 +- app/helpers/users_helper.rb | 5 + app/views/users/index.html.erb | 135 +++++++++++++--------- test/controllers/users_controller_test.rb | 19 ++- test/integration/admin_user_index_test.rb | 2 +- 5 files changed, 96 insertions(+), 67 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a0621d45f..0fdb91aaf 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController include UserPermissionsControllerMethods - layout "admin_layout", only: %w[edit_email_or_password event_logs require_2sv] + layout "admin_layout", only: %w[index edit_email_or_password event_logs require_2sv] before_action :authenticate_user!, except: :show before_action :load_and_authorize_user, except: %i[index show] diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index e1fb6d965..da9f9fce6 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -52,4 +52,9 @@ def formatted_number_of_users(users) def assignable_user_roles current_user.manageable_roles end + + def user_name(user) + anchor_tag = link_to(user.name, edit_user_path(user), class: "govuk-link") + user.suspended? ? content_tag(:del, anchor_tag) : anchor_tag + end end diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 1c043ebe4..827a8dac6 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -1,63 +1,88 @@ <% content_for :title, "Users" %> +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Users", + } + ] + }) +%> -
    -

    Users

    -
    - <% if policy(User).new? %> - <%= link_to "Create user", new_user_invitation_path, class: "btn btn-success add-right-margin" %> - <%= link_to "Upload a batch of users", new_batch_invitation_path, class: "btn btn-default add-right-margin" %> - <% end %> - <% if policy(BulkGrantPermissionSet).new? %> - <%= link_to "Grant access to all users", new_bulk_grant_permission_set_path, class: "btn btn-default" %> - <% end %> +
    +
    +

    <%= formatted_number_of_users(@users) %>

    -
    -

    <%= formatted_number_of_users(@users) %>

    - <%= link_to "Export #{formatted_number_of_users(@users)} as CSV", url_for(format: "csv"), class: "btn btn-default pull-right" %> +
    +
    +
    + <% if policy(User).new? %> + <%= render "govuk_publishing_components/components/button", { + text: "Create user", + href: new_user_invitation_path, + margin_bottom: 4, + } %> + <%= render "govuk_publishing_components/components/button", { + text: "Upload a batch of users", + href: new_batch_invitation_path, + margin_bottom: 4, + } %> + <% end %> + <% if policy(BulkGrantPermissionSet).new? %> + <%= render "govuk_publishing_components/components/button", { + text: "Grant access to all users", + href: new_bulk_grant_permission_set_path, + margin_bottom: 4, + } %> + <% end %> +
    +
    +
    + +
    +
    + <%= render "govuk_publishing_components/components/button", { + text: "Export #{formatted_number_of_users(@users)} as CSV", + href: url_for(format: "csv"), + margin_bottom: 4, + } %> +
    -
    <%= user.status.humanize %> <%= two_step_status(user) %> - <% if user.invited_but_not_accepted %> - <%= form_tag resend_user_invitation_path(user) do %> - <%= submit_tag "Resend signup email", :class => 'btn btn-sm btn-default' %> - <% end %> - <% end %>
    - - - - - - - - - - - - - - <% @users.each do |user| %> - - - - - - - - - - - <% end %> - -
    NameEmailRoleOrganisationLast sign-inStatus<%= two_step_abbr_tag %> StatusActions
    - <%= user.unusable_account? ? "".html_safe : "" %> - - <%= link_to "#{user.name}", edit_user_path(user) %> - - <%= user.unusable_account? ? "".html_safe : "" %> - <%= user.role.humanize %><%= user.organisation.try(:name) %><%= user.status.humanize %><%= two_step_status(user) %> -
    +
    +
    + <%= render "govuk_publishing_components/components/table", { + caption: "Users", + caption_classes: "govuk-visually-hidden", + head: [ + { text: "Name" }, + { text: "Email" }, + { text: "Role" }, + { text: "Organisation" }, + { text: "Last sign-in" }, + { text: "Status" }, + { text: "#{two_step_abbr_tag} Status".html_safe }, + ], + rows: @users.map do |user| + [ + { text: user_name(user) }, + { text: user.email }, + { text: user.role.humanize }, + { text: user.organisation.try(:name) }, + { text: user.current_sign_in_at ? "#{time_ago_in_words(user.current_sign_in_at)} ago" : "never signed in" }, + { text: user.status.humanize }, + { text: two_step_status(user) }, + ] + end, + } %> -<%= paginate(@users, theme: 'twitter-bootstrap-3') %> + <%= paginate(@users, theme: "gds") %> +
    +
    diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 125fc5718..95791586b 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -271,13 +271,13 @@ def change_user_password(user_factory, new_password) should "list users" do create(:user, email: "another_user@email.com") get :index - assert_select "td.email", /another_user@email.com/ + assert_select "tr td:nth-child(2)", /another_user@email.com/ end should "not list api users" do create(:api_user, email: "api_user@email.com") get :index - assert_select "td.email", count: 0, text: /api_user@email.com/ + assert_select "tr td:nth-child(2)", count: 0, text: /api_user@email.com/ end should "not show superadmin users" do @@ -286,7 +286,7 @@ def change_user_password(user_factory, new_password) get :index assert_select "tbody tr", count: 1 - assert_select "td.email", /#{@user.email}/ + assert_select "tr td:nth-child(2)", /#{@user.email}/ end should "show user roles" do @@ -296,11 +296,10 @@ def change_user_password(user_factory, new_password) get :index - assert_select "td.role", "Normal" - assert_select "td.role", "Organisation admin" - assert_select "td.role", "Super organisation admin" - assert_select "td.role", "Admin" - assert_select "td.role", count: 4 + assert_select "tr td:nth-child(3)", "Normal" + assert_select "tr td:nth-child(3)", "Organisation admin" + assert_select "tr td:nth-child(3)", "Super organisation admin" + assert_select "tr td:nth-child(3)", "Admin" end should "show user organisation" do @@ -308,7 +307,7 @@ def change_user_password(user_factory, new_password) get :index - assert_select "td.organisation", user.organisation.name + assert_select "tr td:nth-child(4)", user.organisation.name end context "CSV export" do @@ -333,7 +332,7 @@ def change_user_password(user_factory, new_password) get :index - assert_select "td.email", count: 0, text: /api_user@email.com/ + assert_select "tr td:nth-child(2)", count: 0, text: /api_user@email.com/ end end end diff --git a/test/integration/admin_user_index_test.rb b/test/integration/admin_user_index_test.rb index e1e49425d..16904200d 100644 --- a/test/integration/admin_user_index_test.rb +++ b/test/integration/admin_user_index_test.rb @@ -41,7 +41,7 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest assert page.has_content?("Last sign-in") - actual_last_sign_in_strings = page.all("table tr td.last-sign-in").map(&:text).map(&:strip)[0..1] + actual_last_sign_in_strings = page.all("table tr td:nth-child(5)").map(&:text).map(&:strip)[0..1] assert_equal ["5 minutes ago", "never signed in"], actual_last_sign_in_strings end end From 6bb4e92324405e1d70a2b582c8e5d6a556212a3d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:09 +0100 Subject: [PATCH 08/41] Remove redundant includes from UsersController#export This method is using the @users relation assigned in UsersController#index which already has includes(:organisation) on it. --- app/controllers/users_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0fdb91aaf..b5ac29e34 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -153,7 +153,7 @@ def export CSV.generate do |csv| presenter = UserExportPresenter.new(applications) csv << presenter.header_row - @users.includes(:organisation).find_each do |user| + @users.find_each do |user| csv << presenter.row(user) end end From a5df6188f03a817b70592c00f183800d913b30ac Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:10 +0100 Subject: [PATCH 09/41] Introduce UsersFilter on users index page Trello: https://trello.com/c/2gvEJKw9 This is a first step towards implementing the sidebar filtering in the new proposed design. It's loosely based on Whitehall's Admin::EditionsFilter [1]. I haven't actually implemented any filtering in this commit - I've just moved the ordering and pagination into the new UsersFilter in preparation for implementing filtering in the UsersFilter#users method based on the options set from the params. [1]: https://github.com/alphagov/whitehall/blob/8ddae783224de0c4889dbea0e3d049649287ee75/app/models/admin/edition_filter.rb --- app/controllers/users_controller.rb | 16 +++++++++------- app/models/users_filter.rb | 18 ++++++++++++++++++ app/views/users/_users_filter.html.erb | 7 +++++++ app/views/users/index.html.erb | 5 ++++- test/models/users_filter_test.rb | 22 ++++++++++++++++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 app/models/users_filter.rb create mode 100644 app/views/users/_users_filter.html.erb create mode 100644 test/models/users_filter_test.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b5ac29e34..802ee985f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -8,7 +8,7 @@ class UsersController < ApplicationController before_action :authenticate_user!, except: :show before_action :load_and_authorize_user, except: %i[index show] before_action :allow_no_application_access, only: [:update] - helper_method :applications_and_permissions + helper_method :applications_and_permissions, :filter_params respond_to :html before_action :doorkeeper_authorize!, only: :show @@ -28,12 +28,14 @@ def show def index authorize User - @users = policy_scope(User).includes(:organisation).order(:name) + @filter = UsersFilter.new(policy_scope(User), filter_params) + respond_to do |format| format.html do - paginate_users + @users = @filter.paginated_users end format.csv do + @users = @filter.users headers["Content-Disposition"] = 'attachment; filename="signon_users.csv"' render plain: export, content_type: "text/csv" end @@ -136,10 +138,6 @@ def should_include_permissions? params[:format] == "csv" end - def paginate_users - @users = @users.page(params[:page]).per(25) - end - def validate_token_matches_client_id # FIXME: Once gds-sso is updated everywhere, this should always validate # the client_id param. It should 401 if no client_id is given. @@ -188,4 +186,8 @@ def password_params :password_confirmation, ) end + + def filter_params + params.permit(:page, :format) + end end diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb new file mode 100644 index 000000000..fc051fa99 --- /dev/null +++ b/app/models/users_filter.rb @@ -0,0 +1,18 @@ +class UsersFilter + attr_reader :options + + def initialize(users, options = {}) + @users = users + @options = options + @options[:per_page] ||= 25 + end + + def users + filtered_users = @users + filtered_users.includes(:organisation).order(:name) + end + + def paginated_users + users.page(options[:page]).per(options[:per_page]) + end +end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb new file mode 100644 index 000000000..c11186a86 --- /dev/null +++ b/app/views/users/_users_filter.html.erb @@ -0,0 +1,7 @@ +
    + <%= form_with url: users_path, method: :get do |form| %> + <%= render "govuk_publishing_components/components/button", { + text: "Update results", + } %> + <% end %> +
    diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 827a8dac6..883807a86 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -50,13 +50,16 @@
    <%= render "govuk_publishing_components/components/button", { text: "Export #{formatted_number_of_users(@users)} as CSV", - href: url_for(format: "csv"), + href: url_for(filter_params.merge(format: "csv")), margin_bottom: 4, } %>
    +
    + <%= render "users_filter", filter: @filter %> +
    <%= render "govuk_publishing_components/components/table", { caption: "Users", diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb new file mode 100644 index 000000000..b633427a5 --- /dev/null +++ b/test/models/users_filter_test.rb @@ -0,0 +1,22 @@ +require "test_helper" + +class UsersFilterTest < ActiveSupport::TestCase + should "return all users in alphabetical name order" do + create(:user, name: "beta") + create(:user, name: "alpha") + + filter = UsersFilter.new(User.all) + + assert_equal %w[alpha beta], filter.users.map(&:name) + end + + should "return pages of users" do + 3.times { create(:user) } + + filter = UsersFilter.new(User.all, page: 1, per_page: 2) + assert_equal 2, filter.paginated_users.count + + filter = UsersFilter.new(User.all, page: 2, per_page: 2) + assert_equal 1, filter.paginated_users.count + end +end From 3c299f5a37617b1faf12ae09f5e359adef32418b Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:11 +0100 Subject: [PATCH 10/41] Improve test for exporting CSV of users Previously the magic number, 3, in the assertion related to (a) the admin user created in the setup; (b) the user created in the test; and (c) the header row. Now we parse the CSV so we don't have to consider the header row and we use User.count to make it clear that we're including all users, i.e. in this case both the admin user and the normal user. --- test/controllers/users_controller_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 95791586b..2bee43a8d 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -317,11 +317,11 @@ def change_user_password(user_factory, new_password) assert_equal "text/csv", @response.media_type end - should "export all users" do + should "include all users" do create(:user) get :index, params: { format: :csv } - lines = @response.body.lines - assert_equal(3, lines.length) + number_of_users = CSV.parse(@response.body, headers: true).length + assert_equal User.count, number_of_users end end From d1e01258ad9a6735eef507020fbf104aa92434f6 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:12 +0100 Subject: [PATCH 11/41] Add name/email filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds a search input field to the filter sidebar. When the user enters a value in that input field and clicks either the integrated search button or the form "Update results" button, the list of users is restricted to only those which have a name or email partially matching the search input value. I've tried to give the new User scopes more intention-revealing names than the previous implementation. And I've tried to make the new implementation of the scopes more idiomatic Rails. Unlike the previous implementation of the scopes, I'm not stripping the supplied string, because I think this it's more appropriate for the caller to handle this, i.e. UsersFilter#users in this case. I considered wrapping the supplied string in a call to ActiveRecord::Sanitization::ClassMethods#sanitize_sql_like to escape underscores & percent signs. However, in the end I decided against doing this because it would be a change from the previous behaviour and in any case I can imagine it might be useful to use these wildcard characters in searches. I've used the slightly confusing "filter" param name, because that's what was used in the previous implementation and I didn't want to unnecessarily break any bookmarked URLs. --- app/controllers/users_controller.rb | 2 +- app/models/user.rb | 3 +++ app/models/users_filter.rb | 1 + app/views/users/_users_filter.html.erb | 9 +++++++ test/controllers/users_controller_test.rb | 32 ++++++++++++++++++++++- test/models/user_test.rb | 30 +++++++++++++++++++++ test/models/users_filter_test.rb | 20 ++++++++++++++ 7 files changed, 95 insertions(+), 2 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 802ee985f..f15c79560 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:page, :format) + params.permit(:filter, :page, :format) end end diff --git a/app/models/user.rb b/app/models/user.rb index 01ee13d2c..70608c0d9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -61,6 +61,9 @@ class User < ApplicationRecord scope :web_users, -> { where(api_user: false) } scope :not_suspended, -> { where(suspended_at: nil) } + scope :with_partially_matching_name, ->(name) { where("name LIKE ?", "%#{name}%") } + scope :with_partially_matching_email, ->(email) { where("email LIKE ?", "%#{email}%") } + scope :with_partially_matching_name_or_email, ->(value) { with_partially_matching_name(value).or(with_partially_matching_email(value)) } scope :last_signed_in_on, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) = date(?)", date) } scope :last_signed_in_before, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) < date(?)", date) } scope :last_signed_in_after, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) >= date(?)", date) } diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index fc051fa99..11408be05 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -9,6 +9,7 @@ def initialize(users, options = {}) def users filtered_users = @users + filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] filtered_users.includes(:organisation).order(:name) end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index c11186a86..c1255c1c1 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -1,5 +1,14 @@
    <%= form_with url: users_path, method: :get do |form| %> + <%= render "govuk_publishing_components/components/search", { + id: "name_or_email_filter", + name: "filter", + label_text: "Search by name or email", + label_size: "s", + value: filter.options[:filter], + margin_bottom: 4, + } %> + <%= render "govuk_publishing_components/components/button", { text: "Update results", } %> diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 2bee43a8d..b82dd8f18 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -310,6 +310,28 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(4)", user.organisation.name end + context "filter" do + should "filter by partially matching name" do + create(:user, name: "does-match1") + create(:user, name: "does-match2") + create(:user, name: "does-not-match") + + get :index, params: { filter: "does-match" } + + assert_select "tr td:nth-child(1)", text: /does-match/, count: 2 + end + + should "filter by partially matching email" do + create(:user, email: "does-match1@example.com") + create(:user, email: "does-match2@example.com") + create(:user, email: "does-not-match@example.com") + + get :index, params: { filter: "does-match" } + + assert_select "tr td:nth-child(2)", text: /does-match/, count: 2 + end + end + context "CSV export" do should "respond to CSV format" do get :index, params: { format: :csv } @@ -317,7 +339,15 @@ def change_user_password(user_factory, new_password) assert_equal "text/csv", @response.media_type end - should "include all users" do + should "only include filtered users" do + create(:user, name: "does-match") + create(:user, name: "does-not-match") + get :index, params: { filter: "does-match", format: :csv } + number_of_users = CSV.parse(@response.body, headers: true).length + assert_equal 1, number_of_users + end + + should "include all users when no filter selected" do create(:user) get :index, params: { format: :csv } number_of_users = CSV.parse(@response.body, headers: true).length diff --git a/test/models/user_test.rb b/test/models/user_test.rb index ab332636c..8e5aa73f2 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -698,6 +698,36 @@ def setup end end + context ".with_partially_matching_name" do + should "only return users with a name that includes the value" do + user1 = create(:user, name: "does-match1") + user2 = create(:user, name: "does-match2") + create(:user, name: "does-not-match") + + assert_equal [user1, user2], User.with_partially_matching_name("does-match") + end + end + + context ".with_partially_matching_email" do + should "only return users with an email that includes the value" do + user1 = create(:user, email: "does-match1@example.com") + user2 = create(:user, email: "does-match2@example.com") + create(:user, email: "does-not-match@example.com") + + assert_equal [user1, user2], User.with_partially_matching_email("does-match") + end + end + + context ".with_partially_matching_name_or_email" do + should "only return users with a name OR email that includes the value" do + user1 = create(:user, name: "does-match", email: "does-not-match@example.com") + user2 = create(:user, name: "does-not-match", email: "does-match@example.com") + create(:user, name: "does-not-match", email: "does-not-match-either@example.com") + + assert_equal [user1, user2], User.with_partially_matching_name_or_email("does-match") + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index b633427a5..a0ce3f802 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -19,4 +19,24 @@ class UsersFilterTest < ActiveSupport::TestCase filter = UsersFilter.new(User.all, page: 2, per_page: 2) assert_equal 1, filter.paginated_users.count end + + context "when filtering by name or email" do + should "return users with partially matching name or email" do + create(:user, name: "does-not-match") + create(:user, name: "does-match") + + filter = UsersFilter.new(User.all, filter: "does-match") + + assert_equal %w[does-match], filter.users.map(&:name) + end + + should "ignore leading/trailing spaces in search term" do + create(:user, name: "does-not-match") + create(:user, name: "does-match") + + filter = UsersFilter.new(User.all, filter: " does-match ") + + assert_equal %w[does-match], filter.users.map(&:name) + end + end end From 95dc4b6eaf5e53859351588f65e5dbe1074ac869 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:14 +0100 Subject: [PATCH 12/41] Add a "Clear all filters" link Trello: https://trello.com/c/2gvEJKw9 I've wrapped this in a button group with the "Update results" button so that the link appears alongside the button in wider screen widths, but below the button in narrower screen widths. I've added the govuk-link--no-visited-state CSS class to the link, because that's what we've done elsewhere - it's not obvious it's useful to highlight that the user has clicked on this link previously. --- app/views/users/_users_filter.html.erb | 10 +++++++--- test/controllers/users_controller_test.rb | 6 ++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index c1255c1c1..9f70363c7 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -9,8 +9,12 @@ margin_bottom: 4, } %> - <%= render "govuk_publishing_components/components/button", { - text: "Update results", - } %> +
    + <%= render "govuk_publishing_components/components/button", { + text: "Update results", + } %> + + <%= link_to "Clear all filters", users_path, class: "govuk-link govuk-link--no-visited-state" %> +
    <% end %>
    diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index b82dd8f18..5f2115cc0 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -330,6 +330,12 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(2)", text: /does-match/, count: 2 end + + should "display link to clear all filters" do + get :index + + assert_select "a", text: "Clear all filters", href: users_path + end end context "CSV export" do From f38214b17c74b817597a2e699cad88d4def8e20d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:15 +0100 Subject: [PATCH 13/41] Copy option-select component from finder-frontend The copy was made from the finder-frontend repo at this commit [1]. We're planning to use this component on the users index page for the filter functionality in the proposed design. I haven't copied any of the specs, because I would've had to translate the RSpec spec into Minitest and I wasn't sure the Jasmine test would just work. Since the plan is to extract this component into the govuk_publishing_components gem, it didn't seem worth the effort. [1]: https://github.com/alphagov/finder-frontend/tree/38758e110bf8397c64b2143771794318af9d25b5 --- app/assets/config/manifest.js | 2 + .../images/option-select/input-icon.svg | 3 + app/assets/javascripts/application.js | 4 +- .../javascripts/components/option-select.js | 304 ++++++++++++++++ .../components/_option-select.scss | 168 +++++++++ app/views/components/_option_select.html.erb | 69 ++++ app/views/components/docs/option_select.yml | 343 ++++++++++++++++++ config/locales/components/option_select.yml | 6 + 8 files changed, 896 insertions(+), 3 deletions(-) create mode 100644 app/assets/images/option-select/input-icon.svg create mode 100644 app/assets/javascripts/components/option-select.js create mode 100644 app/assets/stylesheets/components/_option-select.scss create mode 100644 app/views/components/_option_select.html.erb create mode 100644 app/views/components/docs/option_select.yml create mode 100644 config/locales/components/option_select.yml diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js index e1d6e089d..f2c77e945 100644 --- a/app/assets/config/manifest.js +++ b/app/assets/config/manifest.js @@ -3,3 +3,5 @@ //= link legacy_layout.css //= link application.js //= link legacy_layout.js + +//= link components/_option-select.css diff --git a/app/assets/images/option-select/input-icon.svg b/app/assets/images/option-select/input-icon.svg new file mode 100644 index 000000000..ad3de76f1 --- /dev/null +++ b/app/assets/images/option-select/input-icon.svg @@ -0,0 +1,3 @@ + + + diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 5bd8106c0..7de4925fa 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -1,7 +1,5 @@ //= require govuk_publishing_components/dependencies //= require govuk_publishing_components/all_components //= require_tree ./modules -//= require rails-ujs - -// Components from this application //= require_tree ./components +//= require rails-ujs diff --git a/app/assets/javascripts/components/option-select.js b/app/assets/javascripts/components/option-select.js new file mode 100644 index 000000000..7fd99fbc0 --- /dev/null +++ b/app/assets/javascripts/components/option-select.js @@ -0,0 +1,304 @@ +window.GOVUK = window.GOVUK || {} +window.GOVUK.Modules = window.GOVUK.Modules || {}; + +(function (Modules) { + /* This JavaScript provides two functional enhancements to option-select components: + 1) A count that shows how many results have been checked in the option-container + 2) Open/closing of the list of checkboxes + */ + function OptionSelect ($module) { + this.$optionSelect = $module + this.$options = this.$optionSelect.querySelectorAll("input[type='checkbox']") + this.$optionsContainer = this.$optionSelect.querySelector('.js-options-container') + this.$optionList = this.$optionsContainer.querySelector('.js-auto-height-inner') + this.$allCheckboxes = this.$optionsContainer.querySelectorAll('.govuk-checkboxes__item') + this.hasFilter = this.$optionSelect.getAttribute('data-filter-element') || '' + + this.checkedCheckboxes = [] + } + + OptionSelect.prototype.init = function () { + if (this.hasFilter.length) { + var filterEl = document.createElement('div') + filterEl.innerHTML = this.hasFilter + + var optionSelectFilter = document.createElement('div') + optionSelectFilter.classList.add('app-c-option-select__filter') + optionSelectFilter.innerHTML = filterEl.childNodes[0].nodeValue + + this.$optionsContainer.parentNode.insertBefore(optionSelectFilter, this.$optionsContainer) + + this.$filter = this.$optionSelect.querySelector('input[name="option-select-filter"]') + this.$filterCount = document.getElementById(this.$filter.getAttribute('aria-describedby')) + this.filterTextSingle = ' ' + this.$filterCount.getAttribute('data-single') + this.filterTextMultiple = ' ' + this.$filterCount.getAttribute('data-multiple') + this.filterTextSelected = ' ' + this.$filterCount.getAttribute('data-selected') + this.checkboxLabels = [] + this.filterTimeout = 0 + + this.getAllCheckedCheckboxes() + for (var i = 0; i < this.$allCheckboxes.length; i++) { + this.checkboxLabels.push(this.cleanString(this.$allCheckboxes[i].textContent)) + } + + this.$filter.addEventListener('keyup', this.typeFilterText.bind(this)) + } + + // Attach listener to update checked count + this.$optionsContainer.querySelector('.gem-c-checkboxes__list').addEventListener('change', this.updateCheckedCount.bind(this)) + + // Replace div.container-head with a button + this.replaceHeadingSpanWithButton() + + // Add js-collapsible class to parent for CSS + this.$optionSelect.classList.add('js-collapsible') + + // Add open/close listeners + var button = this.$optionSelect.querySelector('.js-container-button') + button.addEventListener('click', this.toggleOptionSelect.bind(this)) + + var closedOnLoad = this.$optionSelect.getAttribute('data-closed-on-load') + var closedOnLoadMobile = this.$optionSelect.getAttribute('data-closed-on-load-mobile') + + // By default the .filter-content container is hidden on mobile + // By checking if .filter-content is hidden, we are in mobile view given the current implementation + var isFacetsContentHidden = this.isFacetsContainerHidden() + + // Check if the option select should be closed for mobile screen sizes + var closedForMobile = closedOnLoadMobile === 'true' && isFacetsContentHidden + + // Always set the contain height to 200px for mobile screen sizes + if (closedForMobile) { + this.setContainerHeight(200) + } + + if (closedOnLoad === 'true' || closedForMobile) { + this.close() + } else { + this.setupHeight() + } + + var checkedString = this.checkedString() + if (checkedString) { + this.attachCheckedCounter(checkedString) + } + } + + OptionSelect.prototype.typeFilterText = function (event) { + event.stopPropagation() + var ENTER_KEY = 13 + + if (event.keyCode !== ENTER_KEY) { + clearTimeout(this.filterTimeout) + this.filterTimeout = setTimeout( + function () { this.doFilter(this) }.bind(this), + 300 + ) + } else { + event.preventDefault() // prevents finder forms from being submitted when user presses ENTER + } + } + + OptionSelect.prototype.cleanString = function cleanString (text) { + text = text.replace(/&/g, 'and') + text = text.replace(/[’',:–-]/g, '') // remove punctuation characters + text = text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') // escape special characters + return text.trim().replace(/\s\s+/g, ' ').toLowerCase() // replace multiple spaces with one + } + + OptionSelect.prototype.getAllCheckedCheckboxes = function getAllCheckedCheckboxes () { + this.checkedCheckboxes = [] + + for (var i = 0; i < this.$options.length; i++) { + if (this.$options[i].checked) { + this.checkedCheckboxes.push(i) + } + } + } + + OptionSelect.prototype.doFilter = function doFilter (obj) { + var filterBy = obj.cleanString(obj.$filter.value) + var showCheckboxes = obj.checkedCheckboxes.slice() + var i = 0 + + for (i = 0; i < obj.$allCheckboxes.length; i++) { + if (showCheckboxes.indexOf(i) === -1 && obj.checkboxLabels[i].search(filterBy) !== -1) { + showCheckboxes.push(i) + } + } + + for (i = 0; i < obj.$allCheckboxes.length; i++) { + obj.$allCheckboxes[i].style.display = 'none' + } + + for (i = 0; i < showCheckboxes.length; i++) { + obj.$allCheckboxes[showCheckboxes[i]].style.display = 'block' + } + + var lenChecked = obj.$optionsContainer.querySelectorAll('.govuk-checkboxes__input:checked').length + var len = showCheckboxes.length + lenChecked + var html = len + (len === 1 ? obj.filterTextSingle : obj.filterTextMultiple) + ', ' + lenChecked + obj.filterTextSelected + obj.$filterCount.innerHTML = html + } + + OptionSelect.prototype.replaceHeadingSpanWithButton = function replaceHeadingSpanWithButton () { + /* Replace the span within the heading with a button element. This is based on feedback from Léonie Watson. + * The button has all of the accessibility hooks that are used by screen readers and etc. + * We do this in the JavaScript because if the JavaScript is not active then the button shouldn't + * be there as there is no JS to handle the click event. + */ + var containerHead = this.$optionSelect.querySelector('.js-container-button') + var jsContainerHeadHTML = containerHead.innerHTML + + // Create button and replace the preexisting html with the button. + var button = document.createElement('button') + button.setAttribute('class', 'js-container-button app-c-option-select__title app-c-option-select__button') + // Add type button to override default type submit when this component is used within a form + button.setAttribute('type', 'button') + button.setAttribute('aria-expanded', true) + button.setAttribute('id', containerHead.getAttribute('id')) + button.setAttribute('aria-controls', this.$optionsContainer.getAttribute('id')) + button.innerHTML = jsContainerHeadHTML + containerHead.parentNode.replaceChild(button, containerHead) + + // GA4 Accordion tracking. Relies on the ga4-finder-tracker setting the index first, so we wrap this in a custom event. + window.addEventListener('ga4-filter-indexes-added', function () { + if (window.GOVUK.analyticsGa4) { + if (window.GOVUK.analyticsGa4.Ga4FinderTracker) { + window.GOVUK.analyticsGa4.Ga4FinderTracker.addFilterButtonTracking(button, button.innerHTML) + } + } + }) + } + + OptionSelect.prototype.attachCheckedCounter = function attachCheckedCounter (checkedString) { + var element = document.createElement('div') + element.setAttribute('class', 'app-c-option-select__selected-counter js-selected-counter') + element.innerHTML = checkedString + this.$optionSelect.querySelector('.js-container-button').insertAdjacentElement('afterend', element) + } + + OptionSelect.prototype.updateCheckedCount = function updateCheckedCount () { + var checkedString = this.checkedString() + var checkedStringElement = this.$optionSelect.querySelector('.js-selected-counter') + + if (checkedString) { + if (checkedStringElement === null) { + this.attachCheckedCounter(checkedString) + } else { + checkedStringElement.textContent = checkedString + } + } else if (checkedStringElement) { + checkedStringElement.parentNode.removeChild(checkedStringElement) + } + } + + OptionSelect.prototype.checkedString = function checkedString () { + this.getAllCheckedCheckboxes() + var count = this.checkedCheckboxes.length + var checkedString = false + if (count > 0) { + checkedString = count + ' selected' + } + + return checkedString + } + + OptionSelect.prototype.toggleOptionSelect = function toggleOptionSelect (e) { + if (this.isClosed()) { + this.open() + } else { + this.close() + } + e.preventDefault() + } + + OptionSelect.prototype.open = function open () { + if (this.isClosed()) { + this.$optionSelect.querySelector('.js-container-button').setAttribute('aria-expanded', true) + this.$optionSelect.classList.remove('js-closed') + this.$optionSelect.classList.add('js-opened') + if (!this.$optionsContainer.style.height) { + this.setupHeight() + } + } + } + + OptionSelect.prototype.close = function close () { + this.$optionSelect.classList.remove('js-opened') + this.$optionSelect.classList.add('js-closed') + this.$optionSelect.querySelector('.js-container-button').setAttribute('aria-expanded', false) + } + + OptionSelect.prototype.isClosed = function isClosed () { + return this.$optionSelect.classList.contains('js-closed') + } + + OptionSelect.prototype.setContainerHeight = function setContainerHeight (height) { + this.$optionsContainer.style.height = height + 'px' + } + + OptionSelect.prototype.isCheckboxVisible = function isCheckboxVisible (option) { + var initialOptionContainerHeight = this.$optionsContainer.clientHeight + var optionListOffsetTop = this.$optionList.getBoundingClientRect().top + var distanceFromTopOfContainer = option.getBoundingClientRect().top - optionListOffsetTop + return distanceFromTopOfContainer < initialOptionContainerHeight + } + + OptionSelect.prototype.getVisibleCheckboxes = function getVisibleCheckboxes () { + var visibleCheckboxes = [] + for (var i = 0; i < this.$options.length; i++) { + if (this.isCheckboxVisible(this.$options[i])) { + visibleCheckboxes.push(this.$options[i]) + } + } + + // add an extra checkbox, if the label of the first is too long it collapses onto itself + if (this.$options[visibleCheckboxes.length]) { + visibleCheckboxes.push(this.$options[visibleCheckboxes.length]) + } + return visibleCheckboxes + } + + OptionSelect.prototype.isFacetsContainerHidden = function isFacetsContainerHidden () { + var facetsContent = this.$optionSelect.parentElement + var isFacetsContentHidden = false + // check whether this is hidden by progressive disclosure, + // because height calculations won't work + // would use offsetParent === null but for IE10+ + if (facetsContent) { + isFacetsContentHidden = !(facetsContent.offsetWidth || facetsContent.offsetHeight || facetsContent.getClientRects().length) + } + + return isFacetsContentHidden + } + + OptionSelect.prototype.setupHeight = function setupHeight () { + var initialOptionContainerHeight = this.$optionsContainer.clientHeight + var height = this.$optionList.offsetHeight + + var isFacetsContainerHidden = this.isFacetsContainerHidden() + + if (isFacetsContainerHidden) { + initialOptionContainerHeight = 200 + height = 200 + } + + // Resize if the list is only slightly bigger than its container + // If isFacetsContainerHidden is true, then 200 < 250 + // And the container height is always set to 201px + if (height < initialOptionContainerHeight + 50) { + this.setContainerHeight(height + 1) + return + } + + // Resize to cut last item cleanly in half + var visibleCheckboxes = this.getVisibleCheckboxes() + + var lastVisibleCheckbox = visibleCheckboxes[visibleCheckboxes.length - 1] + var position = lastVisibleCheckbox.parentNode.offsetTop // parent element is relative + this.setContainerHeight(position + (lastVisibleCheckbox.clientHeight / 1.5)) + } + + Modules.OptionSelect = OptionSelect +})(window.GOVUK.Modules) diff --git a/app/assets/stylesheets/components/_option-select.scss b/app/assets/stylesheets/components/_option-select.scss new file mode 100644 index 000000000..2470abbdb --- /dev/null +++ b/app/assets/stylesheets/components/_option-select.scss @@ -0,0 +1,168 @@ +@import "govuk_publishing_components/individual_component_support"; + +.app-c-option-select { + position: relative; + padding: 0 0 govuk-spacing(2); + margin-bottom: govuk-spacing(2); + border-bottom: 1px solid $govuk-border-colour; + + @include govuk-media-query($from: desktop) { + // Redefine scrollbars on desktop where these lists are scrollable + // so they are always visible in option lists + ::-webkit-scrollbar { + -webkit-appearance: none; + width: 7px; + } + + ::-webkit-scrollbar-thumb { + border-radius: 4px; + + background-color: rgba(0, 0, 0, .5); + -webkit-box-shadow: 0 0 1px rgba(255, 255, 255, .87); + } + } + + .gem-c-checkboxes { + margin: 0; + } +} + +.app-c-option-select__title { + @include govuk-font(19); + margin: 0; +} + +.app-c-option-select__button { + z-index: 1; + background: none; + border: 0; + text-align: left; + padding: 0; + cursor: pointer; + color: $govuk-link-colour; + + &:hover { + text-decoration: underline; + text-underline-offset: .1em; + @include govuk-link-hover-decoration; + } + + &::-moz-focus-inner { + border: 0; + } + + &:focus { + outline: none; + text-decoration: none; + @include govuk-focused-text; + } + + &[disabled] { + background-image: none; + color: inherit; + } + + // Extend the touch area of the button to span the heading + &::after { + content: ""; + position: absolute; + top: 0; + right: 0; + bottom: 0; + left: 0; + z-index: 2; + } +} + +.app-c-option-select__icon { + display: none; + position: absolute; + top: 0; + left: 9px; + width: 30px; + height: 40px; + fill: govuk-colour("black"); +} + +.app-c-option-select__container { + position: relative; + max-height: 200px; + overflow-y: auto; + overflow-x: hidden; + background-color: govuk-colour("white"); + + &:focus { + outline: 0; + } +} + +.app-c-option-select__container--large { + max-height: 600px; +} + +.app-c-option-select__container-inner { + padding: govuk-spacing(1) 13px; +} + +.app-c-option-select__filter { + position: relative; + background: govuk-colour("white"); + padding: 13px 13px govuk-spacing(2) 13px; +} + +.app-c-option-select__filter-input { + @include govuk-font(19); + padding-left: 33px; + background: image-url("option-select/input-icon.svg") govuk-colour("white") no-repeat -5px -3px; + + @include govuk-media-query($from: tablet) { + @include govuk-font(16); + } +} + +.js-enabled { + .app-c-option-select__heading { + position: relative; + padding: 10px 8px 5px 43px; + } + + [aria-expanded="true"] ~ .app-c-option-select__icon--up { + display: block; + } + + [aria-expanded="false"] ~ .app-c-option-select__icon--down { + display: block; + } + + .app-c-option-select__container { + height: 200px; + } + + .app-c-option-select__container--large { + height: 600px; + } + + [data-closed-on-load="true"] .app-c-option-select__container { + display: none; + } +} + +.app-c-option-select__selected-counter { + @include govuk-font($size: 14); + color: $govuk-text-colour; + margin-top: 3px; +} + +.app-c-option-select.js-closed { + .app-c-option-select__filter, + .app-c-option-select__container { + display: none; + } +} + +.app-c-option-select.js-opened { + .app-c-option-select__filter, + .app-c-option-select__container { + display: block; + } +} diff --git a/app/views/components/_option_select.html.erb b/app/views/components/_option_select.html.erb new file mode 100644 index 000000000..144b3a9e3 --- /dev/null +++ b/app/views/components/_option_select.html.erb @@ -0,0 +1,69 @@ +<% add_app_component_stylesheet("option-select") %> +<% + title_id = "option-select-title-#{title.parameterize}" + checkboxes_id = "checkboxes-#{SecureRandom.hex(4)}" + checkboxes_count_id = checkboxes_id + "-count" + show_filter ||= false + large ||= false + + classes = %w[app-c-option-select__container js-options-container] + classes << "app-c-option-select__container--large" if large +%> + +<% if show_filter %> + <% + filter_id ||= "input-#{SecureRandom.hex(4)}" + %> + <% filter = capture do %> + <%= tag.label for: filter_id, class: "govuk-label govuk-visually-hidden" do %> + Filter <%= title %> + <% end %> + + <%= tag.input name: "option-select-filter", + id: filter_id, + class: "app-c-option-select__filter-input govuk-input", + type: "text", + aria: { + describedby: checkboxes_count_id, + controls: checkboxes_id + } + %> + <% end %> + <% filter_element = CGI::escapeHTML(filter) %> +<% end %> + +
    data-closed-on-load="true"<% end %> + <% if local_assigns.include?(:closed_on_load_mobile) && closed_on_load_mobile %>data-closed-on-load-mobile="true"<% end %> + <% if local_assigns.include?(:aria_controls_id) %>data-input-aria-controls="<%= aria_controls_id %>"<% end %> + <% if show_filter %>data-filter-element="<%= filter_element %>"<% end %> +> +

    + <%= title %> + + +

    + + <%= content_tag(:div, role: "group", aria: { labelledby: title_id }, class: classes, id: options_container_id, tabindex: "-1") do %> +
    + <% if show_filter %> + + <% end %> + <%= render "govuk_publishing_components/components/checkboxes", { + name: "#{key}[]", + id: checkboxes_id, + heading: title, + small: true, + visually_hide_heading: true, + no_hint_text: true, + items: options + } %> +
    + <% end %> +
    diff --git a/app/views/components/docs/option_select.yml b/app/views/components/docs/option_select.yml new file mode 100644 index 000000000..4ffc68686 --- /dev/null +++ b/app/views/components/docs/option_select.yml @@ -0,0 +1,343 @@ +name: Option select +description: A scrollable list of checkboxes to be displayed on a form where one might + otherwise use a multi-select +body: +accessibility_criteria: | + The option select must: + + - indicate that the option select is expandable/collapsible + - indicate the initial state of expandable content + - indicate where the state of expandable content has changed + + The option select inputs must: + + - have a margin to the right when the box is scrollable so that users can interact with a scrollbar without accidentally clicking an option + - only include an `aria-controls` attribute if an element with the ID specified exists on the page + - be [grouped with a label](https://www.w3.org/WAI/GL/wiki/Using_grouping_roles_to_identify_related_form_controls) + + The option select filter must: + + - be focusable with a keyboard + - indicate when it has keyboard focus + - inform the user that is it an editable field + - inform the user when there are matches, or if there are no matches + - inform the user as the number of matches changes +examples: + default: + data: + key: market_sector + title: Market sector + options_container_id: list_of_sectors + options: + - value: aerospace + label: Aerospace + id: aerospace + - value: agriculture-environment-and-natural-resources + label: Agriculture, environment and natural resources + id: agriculture-environment-and-natural-resources + - value: building-and-construction + label: Building and construction + id: building-and-construction + - value: chemicals + label: Chemicals + id: chemicals + - value: clothing-footwear-and-fashion + label: Clothing, footwear and fashion + id: clothing-footwear-and-fashion + - value: defence + label: Defence + id: defence + - value: distribution-and-service-industries + label: Distribution and Service Industries + id: distribution-and-service-industries + - value: electronics-industry + label: Electronics Industry + id: electronics-industry + - value: energy + label: Energy + id: energy + - value: engineering + label: Engineering + id: engineering + - value: financial-services + label: Financial services + id: financial-services + - value: fire-police-and-security + label: Fire, police, and security + id: fire-police-and-security + - value: food-manufacturing + label: Food manufacturing + id: food-manufacturing + - value: giftware-jewellery-and-tableware + label: Giftware, jewellery and tableware + id: giftware-jewellery-and-tableware + with_option_pre_checked: + data: + key: with_checked_value_set + title: List of options + options_container_id: list_of_vegetables + options: + - value: potatoes + label: Potatoes + checked: true + - value: carrots + label: Carrots + id: carrots + - value: mash + label: Mash + id: mash + with_aria_controls: + description: | + aria_controls_id adds an [`aria-controls` attribute](https://tink.uk/using-the-aria-controls-attribute/) to each checkbox input. This makes it easier for users of assitive tech to jump from them to the part of the page they’re updating. + + The aria_controls_id must be set to the ID of an element that’s on the page or it won’t be included. + data: + key: with_aria-control_set + title: List of options (with aria controls) + aria_controls_id: wrapper + options_container_id: list_of_countries + options: + - value: britain + label: Britain + id: britain + - value: france + label: France + id: france + - value: spain + label: Spain + id: spain + closed_on_load: + data: + key: closed_on_load + title: List of hats + closed_on_load: true + options_container_id: list_of_hats + options: + - value: bobble_hat + label: Bobble hat + id: bobble_hat + - value: fez + label: Fez + id: fez + - value: sombrero + label: Sombrero + id: sombrero + with_tracking: + data: + key: list_of_shoes + title: List of shoes + options_container_id: list_of_shoes + options: + - value: trainers + label: Trainers + id: trainers + data_attributes: + track_category: "filterClicked" + track_action: "List of shoes" + track_label: "trainers" + track_options: + dimension28: 1 + - value: plimsolls + label: Plimsolls + id: plimsolls + data_attributes: + track_category: "filterClicked" + track_action: "List of shoes" + track_label: "plimsolls" + track_options: + dimension28: 1 + - value: high_heels + label: High heels + id: high_heels + data_attributes: + track_category: "filterClicked" + track_action: "List of shoes" + track_label: "high_heels" + track_options: + dimension28: 1 + with_filter: + description: Adds a filter to allow users to narrow the checkboxes down. Checkboxes will only show if they match what the user has typed, or if they are already checked. The filter is case insensitive and strips out punctuation characters and duplicate whitespace, and sees '&' and 'and' as the same, to make filtering easier. + data: + key: filter_demo + title: Countries + options_container_id: list_of_countries_to_filter + show_filter: true + options: + - value: afghanistan + label: Afghanistan + id: afghanistan + - value: albania + label: Albania + id: albania + - value: algeria + label: Algeria + id: algeria + checked: true + - value: american_samoa + label: American Samoa + id: american_samoa + - value: andorra + label: Andorra + id: andorra + checked: true + - value: angola + label: Angola + id: angola + - value: anguilla + label: Anguilla + id: anguilla + - value: antigua_and_barbuda + label: Antigua and Barbuda + id: antigua_and_barbuda + - value: argentina + label: Argentina + id: argentina + - value: armenia + label: Armenia + id: armenia + - value: aruba + label: Aruba + id: aruba + - value: australia + label: Australia + id: australia + - value: austria + label: Austria + id: austria + - value: azerbaijan + label: Azerbaijan + id: azerbaijan + - value: bahamas + label: Bahamas + id: bahamas + - value: bahrain + label: Bahrain + id: bahrain + - value: bangladesh + label: Bangladesh + id: bangladesh + - value: barbados + label: Barbados + id: barbados + - value: belarus + label: Belarus + id: belarus + - value: belgium + label: Belgium + id: belgium + - value: belize + label: Belize + id: belize + - value: benin + label: Benin + id: benin + - value: bermuda + label: Bermuda + id: bermuda + - value: bhutan + label: Bhutan + id: bhutan + - value: bolivia + label: Bolivia + id: bolivia + - value: cote + label: Cote d’Ivoire + id: cote + - value: sthelena + label: St Helena, Ascension and Tristan da Cunha + id: sthelena + - value: trinidad + label: Trinidad & Tobago + id: trinidad + large: + description: When `large` is set to `true`, the option-select container has an increased max-height of 600px, allowing more checkbox items to be visible. + data: + key: large_demo + title: Countries + options_container_id: list_of_countries_to_filter_large + large: true + options: + - value: afghanistan + label: Afghanistan + id: afghanistanLargeExample + - value: albania + label: Albania + id: albaniaLargeExample + - value: algeria + label: Algeria + id: algeriaLargeExample + - value: american_samoa + label: American Samoa + id: american_samoaLargeExample + - value: andorra + label: Andorra + id: andorraLargeExample + - value: angola + label: Angola + id: angolaLargeExample + - value: anguilla + label: Anguilla + id: anguillaLargeExample + - value: antigua_and_barbuda + label: Antigua and Barbuda + id: antigua_and_barbudaLargeExample + - value: argentina + label: Argentina + id: argentinaLargeExample + - value: armenia + label: Armenia + id: armeniaLargeExample + - value: aruba + label: Aruba + id: arubaLargeExample + - value: australia + label: Australia + id: australiaLargeExample + - value: austria + label: Austria + id: austriaLargeExample + - value: azerbaijan + label: Azerbaijan + id: azerbaijanLargeExample + - value: bahamas + label: Bahamas + id: bahamasLargeExample + - value: bahrain + label: Bahrain + id: bahrainLargeExample + - value: bangladesh + label: Bangladesh + id: bangladeshLargeExample + - value: barbados + label: Barbados + id: barbadosLargeExample + - value: belarus + label: Belarus + id: belarusLargeExample + - value: belgium + label: Belgium + id: belgiumLargeExample + - value: belize + label: Belize + id: belizeLargeExample + - value: benin + label: Benin + id: beninLargeExample + - value: bermuda + label: Bermuda + id: bermudaLargeExample + - value: bhutan + label: Bhutan + id: bhutanLargeExample + - value: bolivia + label: Bolivia + id: boliviaLargeExample + - value: cote + label: Cote d'Ivoire + id: coteLargeExample + - value: sthelena + label: St Helena, Ascension and Tristan da Cunha + id: sthelenaLargeExample + - value: trinidad + label: Trinidad & Tobago + id: trinidadLargeExample diff --git a/config/locales/components/option_select.yml b/config/locales/components/option_select.yml new file mode 100644 index 000000000..1eb1060a0 --- /dev/null +++ b/config/locales/components/option_select.yml @@ -0,0 +1,6 @@ +en: + components: + option_select: + found_single: option found + found_multiple: options found + selected: selected From a7a30d273be07cb17c055e884fb0835288f8c192 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:16 +0100 Subject: [PATCH 14/41] Extract User#role_class method I preferred the constant lookup approach used in UserPolicy#can_manage? over that used in User#manageable_roles, so I've used that in the new method. --- app/models/user.rb | 6 ++++-- app/policies/user_policy.rb | 2 +- test/models/user_test.rb | 20 ++++++++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 70608c0d9..cf4822bb2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -33,6 +33,8 @@ class User < ApplicationRecord :confirmable, :password_archivable # in signon/lib/devise/models/password_archivable.rb + delegate :manageable_roles, to: :role_class + encrypts :otp_secret_key validates :name, presence: true @@ -229,8 +231,8 @@ def status USER_STATUS_ACTIVE end - def manageable_roles - "Roles::#{role.camelize}".constantize.manageable_roles + def role_class + Roles.const_get(role.classify) end # Make devise send all emails using ActiveJob diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 2cac400f9..466264879 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -71,7 +71,7 @@ def allow_self_only end def can_manage? - Roles.const_get(current_user.role.classify).can_manage?(record.role) + current_user.role_class.can_manage?(record.role) end class Scope < ::BasePolicy::Scope diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 8e5aa73f2..9db5ffdc9 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -728,6 +728,26 @@ def setup end end + context "#role_class" do + should "return the role class" do + assert_equal Roles::Normal, build(:user).role_class + assert_equal Roles::OrganisationAdmin, build(:organisation_admin_user).role_class + assert_equal Roles::SuperOrganisationAdmin, build(:super_organisation_admin_user).role_class + assert_equal Roles::Admin, build(:admin_user).role_class + assert_equal Roles::Superadmin, build(:superadmin_user).role_class + end + end + + context "#manageable_roles" do + should "return names of roles that the user is allowed to manage" do + assert_equal %w[], build(:user).manageable_roles + assert_equal %w[normal organisation_admin], build(:organisation_admin_user).manageable_roles + assert_equal %w[normal organisation_admin super_organisation_admin], build(:super_organisation_admin_user).manageable_roles + assert_equal %w[normal organisation_admin super_organisation_admin admin], build(:admin_user).manageable_roles + assert_equal User.roles, build(:superadmin_user).manageable_roles + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end From f1b3c83813f1aaebb8866afc300516c2b5ad8214 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:17 +0100 Subject: [PATCH 15/41] Encapsulate more of UserPolicy#can_manage? in User#can_manage? --- app/models/user.rb | 4 ++++ app/policies/user_policy.rb | 2 +- test/models/user_test.rb | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index cf4822bb2..c14bc954f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -235,6 +235,10 @@ def role_class Roles.const_get(role.classify) end + def can_manage?(other_user) + role_class.can_manage?(other_user.role) + end + # Make devise send all emails using ActiveJob def send_devise_notification(notification, *args) devise_mailer.send(notification, self, *args).deliver_later diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 466264879..6d26412b7 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -71,7 +71,7 @@ def allow_self_only end def can_manage? - current_user.role_class.can_manage?(record.role) + current_user.can_manage?(record) end class Scope < ::BasePolicy::Scope diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 9db5ffdc9..a9e7f086c 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -748,6 +748,40 @@ def setup end end + context "#can_manage?" do + should "indicate whether user is allowed to manage another user" do + assert_not build(:user).can_manage?(build(:user)) + assert_not build(:user).can_manage?(build(:organisation_admin_user)) + assert_not build(:user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:user).can_manage?(build(:admin_user)) + assert_not build(:user).can_manage?(build(:superadmin_user)) + + assert build(:organisation_admin_user).can_manage?(build(:user)) + assert build(:organisation_admin_user).can_manage?(build(:organisation_admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:superadmin_user)) + + assert build(:super_organisation_admin_user).can_manage?(build(:user)) + assert build(:super_organisation_admin_user).can_manage?(build(:organisation_admin_user)) + assert build(:super_organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:super_organisation_admin_user).can_manage?(build(:admin_user)) + assert_not build(:super_organisation_admin_user).can_manage?(build(:superadmin_user)) + + assert build(:admin_user).can_manage?(build(:user)) + assert build(:admin_user).can_manage?(build(:organisation_admin_user)) + assert build(:admin_user).can_manage?(build(:super_organisation_admin_user)) + assert build(:admin_user).can_manage?(build(:admin_user)) + assert_not build(:admin_user).can_manage?(build(:superadmin_user)) + + assert build(:superadmin_user).can_manage?(build(:user)) + assert build(:superadmin_user).can_manage?(build(:organisation_admin_user)) + assert build(:superadmin_user).can_manage?(build(:super_organisation_admin_user)) + assert build(:superadmin_user).can_manage?(build(:admin_user)) + assert build(:superadmin_user).can_manage?(build(:superadmin_user)) + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end From 10964526a5ce7d2f048eaa1ff874001704029a26 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:18 +0100 Subject: [PATCH 16/41] Inline Role::Base#can_manage? into User#can_manage? It's not obvious that the former was adding much value. --- app/models/user.rb | 2 +- lib/roles/base.rb | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index c14bc954f..e8369b4cf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -236,7 +236,7 @@ def role_class end def can_manage?(other_user) - role_class.can_manage?(other_user.role) + manageable_roles.include?(other_user.role) end # Make devise send all emails using ActiveJob diff --git a/lib/roles/base.rb b/lib/roles/base.rb index 9e36e1dbd..5b0a5cd86 100644 --- a/lib/roles/base.rb +++ b/lib/roles/base.rb @@ -1,7 +1,3 @@ module Roles - class Base - def self.can_manage?(role) - manageable_roles.include?(role) - end - end + class Base; end end From ff6a198730bd39aa035bc0e208416527d01de36b Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:19 +0100 Subject: [PATCH 17/41] Add role filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected roles. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single role. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `roles` as the parameter name to make it clear that more than one role can be selected and to distinguish it from the previous implementation which used `role`. We might want to consider redirecting URLs using `role` to use `roles` if we want bookmarked URLs to continue to work as expected. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 --- app/controllers/users_controller.rb | 4 +- app/models/user.rb | 1 + app/models/users_filter.rb | 14 +++++- app/views/users/_users_filter.html.erb | 8 +++ test/controllers/users_controller_test.rb | 12 +++++ test/models/user_test.rb | 10 ++++ test/models/users_filter_test.rb | 59 +++++++++++++++++++++-- 7 files changed, 100 insertions(+), 8 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f15c79560..383ed50ea 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -28,7 +28,7 @@ def show def index authorize User - @filter = UsersFilter.new(policy_scope(User), filter_params) + @filter = UsersFilter.new(policy_scope(User), current_user, filter_params) respond_to do |format| format.html do @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:filter, :page, :format) + params.permit(:filter, :page, :format, roles: []) end end diff --git a/app/models/user.rb b/app/models/user.rb index e8369b4cf..f1a739d37 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -63,6 +63,7 @@ class User < ApplicationRecord scope :web_users, -> { where(api_user: false) } scope :not_suspended, -> { where(suspended_at: nil) } + scope :with_role, ->(role) { where(role:) } scope :with_partially_matching_name, ->(name) { where("name LIKE ?", "%#{name}%") } scope :with_partially_matching_email, ->(email) { where("email LIKE ?", "%#{email}%") } scope :with_partially_matching_name_or_email, ->(value) { with_partially_matching_name(value).or(with_partially_matching_email(value)) } diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index 11408be05..90330c3a3 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -1,8 +1,9 @@ class UsersFilter attr_reader :options - def initialize(users, options = {}) + def initialize(users, current_user, options = {}) @users = users + @current_user = current_user @options = options @options[:per_page] ||= 25 end @@ -10,10 +11,21 @@ def initialize(users, options = {}) def users filtered_users = @users filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] + filtered_users = filtered_users.with_role(options[:roles]) if options[:roles] filtered_users.includes(:organisation).order(:name) end def paginated_users users.page(options[:page]).per(options[:per_page]) end + + def role_option_select_options + @current_user.manageable_roles.map do |role| + { + label: role.humanize.capitalize, + value: role, + checked: Array(options[:roles]).include?(role), + } + end + end end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 9f70363c7..d7cad41b8 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -9,6 +9,14 @@ margin_bottom: 4, } %> + <%= render "components/option_select", { + key: "roles", + title: "Roles", + options_container_id: "roles_filter", + closed_on_load: true, + options: @filter.role_option_select_options, + } %> +
    <%= render "govuk_publishing_components/components/button", { text: "Update results", diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 5f2115cc0..8fa3000b1 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -331,6 +331,18 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(2)", text: /does-match/, count: 2 end + should "filter by roles" do + create(:admin_user, name: "admin-user") + create(:organisation_admin_user, name: "organisation-admin-user") + create(:user, name: "normal-user") + + get :index, params: { roles: %w[admin normal] } + + assert_select "tr td:nth-child(1)", text: "admin-user" + assert_select "tr td:nth-child(1)", text: "organisation-admin-user", count: 0 + assert_select "tr td:nth-child(1)", text: "normal-user" + end + should "display link to clear all filters" do get :index diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a9e7f086c..6c555d714 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -782,6 +782,16 @@ def setup end end + context ".with_role" do + should "only return users with specified role(s)" do + admin_user = create(:admin_user) + organisation_admin = create(:organisation_admin_user) + create(:user) + + assert_equal [admin_user, organisation_admin], User.with_role(%w[admin organisation_admin]) + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index a0ce3f802..394ac8dce 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -1,11 +1,15 @@ require "test_helper" class UsersFilterTest < ActiveSupport::TestCase + setup do + @current_user = User.new + end + should "return all users in alphabetical name order" do create(:user, name: "beta") create(:user, name: "alpha") - filter = UsersFilter.new(User.all) + filter = UsersFilter.new(User.all, @current_user) assert_equal %w[alpha beta], filter.users.map(&:name) end @@ -13,10 +17,10 @@ class UsersFilterTest < ActiveSupport::TestCase should "return pages of users" do 3.times { create(:user) } - filter = UsersFilter.new(User.all, page: 1, per_page: 2) + filter = UsersFilter.new(User.all, @current_user, page: 1, per_page: 2) assert_equal 2, filter.paginated_users.count - filter = UsersFilter.new(User.all, page: 2, per_page: 2) + filter = UsersFilter.new(User.all, @current_user, page: 2, per_page: 2) assert_equal 1, filter.paginated_users.count end @@ -25,7 +29,7 @@ class UsersFilterTest < ActiveSupport::TestCase create(:user, name: "does-not-match") create(:user, name: "does-match") - filter = UsersFilter.new(User.all, filter: "does-match") + filter = UsersFilter.new(User.all, @current_user, filter: "does-match") assert_equal %w[does-match], filter.users.map(&:name) end @@ -34,9 +38,54 @@ class UsersFilterTest < ActiveSupport::TestCase create(:user, name: "does-not-match") create(:user, name: "does-match") - filter = UsersFilter.new(User.all, filter: " does-match ") + filter = UsersFilter.new(User.all, @current_user, filter: " does-match ") assert_equal %w[does-match], filter.users.map(&:name) end end + + context "when filtering by role" do + should "return users matching any of the specified roles" do + create(:admin_user, name: "admin-user") + create(:organisation_admin_user, name: "organisation-admin-user") + create(:user, name: "normal-user") + + filter = UsersFilter.new(User.all, @current_user, roles: %w[admin normal]) + + assert_equal %w[admin-user normal-user], filter.users.map(&:name) + end + end + + context "#role_option_select_options" do + context "when no roles are selected" do + should "return options for roles manageable by the current user with none checked" do + @current_user.stubs(:manageable_roles).returns(%w[normal organisation_admin]) + + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.role_option_select_options + + expected_options = [ + { label: "Normal", value: "normal", checked: false }, + { label: "Organisation admin", value: "organisation_admin", checked: false }, + ] + assert_equal expected_options, options + end + end + + context "when some roles are selected" do + should "return options for roles manageable by the current user with relevant options checked" do + @current_user.stubs(:manageable_roles).returns(%w[normal organisation_admin super_organisation_admin]) + + filter = UsersFilter.new(User.all, @current_user, roles: %w[normal super_organisation_admin]) + options = filter.role_option_select_options + + expected_options = [ + { label: "Normal", value: "normal", checked: true }, + { label: "Organisation admin", value: "organisation_admin", checked: false }, + { label: "Super organisation admin", value: "super_organisation_admin", checked: true }, + ] + assert_equal expected_options, options + end + end + end end From 5ebfbce28dce9a980939e328800727590fb19e03 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:21 +0100 Subject: [PATCH 18/41] Introduce User#manageable_organisations method This is similar to the User#manageable_roles method. I plan to use it in subsequent commits to make it clearer what a user with a given role is allowed to do. --- app/models/user.rb | 4 ++++ lib/roles/admin.rb | 4 ++++ lib/roles/normal.rb | 4 ++++ lib/roles/organisation_admin.rb | 4 ++++ lib/roles/super_organisation_admin.rb | 4 ++++ lib/roles/superadmin.rb | 4 ++++ test/models/user_test.rb | 14 ++++++++++++++ 7 files changed, 38 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index f1a739d37..0bdf056fb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -240,6 +240,10 @@ def can_manage?(other_user) manageable_roles.include?(other_user.role) end + def manageable_organisations + role_class.manageable_organisations_for(self).order(:name) + end + # Make devise send all emails using ActiveJob def send_devise_notification(notification, *args) devise_mailer.send(notification, self, *args).deliver_later diff --git a/lib/roles/admin.rb b/lib/roles/admin.rb index c3cd2d207..c7ec2edc3 100644 --- a/lib/roles/admin.rb +++ b/lib/roles/admin.rb @@ -26,5 +26,9 @@ def self.level def self.manageable_roles %w[normal organisation_admin super_organisation_admin admin] end + + def self.manageable_organisations_for(_) + Organisation.all + end end end diff --git a/lib/roles/normal.rb b/lib/roles/normal.rb index cd6000b3c..8fd67cfff 100644 --- a/lib/roles/normal.rb +++ b/lib/roles/normal.rb @@ -15,5 +15,9 @@ def self.level def self.manageable_roles [] end + + def self.manageable_organisations_for(_) + Organisation.where("false") + end end end diff --git a/lib/roles/organisation_admin.rb b/lib/roles/organisation_admin.rb index 4a106eb12..9f2500fb7 100644 --- a/lib/roles/organisation_admin.rb +++ b/lib/roles/organisation_admin.rb @@ -26,5 +26,9 @@ def self.level def self.manageable_roles %w[normal organisation_admin] end + + def self.manageable_organisations_for(user) + Organisation.where(id: user.organisation) + end end end diff --git a/lib/roles/super_organisation_admin.rb b/lib/roles/super_organisation_admin.rb index 38adba77a..0e64c410a 100644 --- a/lib/roles/super_organisation_admin.rb +++ b/lib/roles/super_organisation_admin.rb @@ -26,5 +26,9 @@ def self.level def self.manageable_roles %w[normal organisation_admin super_organisation_admin] end + + def self.manageable_organisations_for(user) + Organisation.where(id: user.organisation.subtree) + end end end diff --git a/lib/roles/superadmin.rb b/lib/roles/superadmin.rb index 78d8a202b..44a5def39 100644 --- a/lib/roles/superadmin.rb +++ b/lib/roles/superadmin.rb @@ -27,5 +27,9 @@ def self.level def self.manageable_roles User.roles end + + def self.manageable_organisations_for(_) + Organisation.all + end end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 6c555d714..c2c1ea45d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -792,6 +792,20 @@ def setup end end + context "#manageable_organisations" do + should "return relation for organisations that the user is allowed to manage" do + organisation = create(:organisation, name: "Org1") + child_organisation = create(:organisation, parent: organisation, name: "Org2") + create(:organisation, name: "Org3") + + assert_equal [], create(:user, organisation:).manageable_organisations + assert_equal [organisation], create(:organisation_admin_user, organisation:).manageable_organisations + assert_equal [organisation, child_organisation], create(:super_organisation_admin_user, organisation:).manageable_organisations + assert_equal Organisation.order(:name), create(:admin_user, organisation:).manageable_organisations + assert_equal Organisation.order(:name), create(:superadmin_user, organisation:).manageable_organisations + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end From 1e4750d3b932822f5057978bf54b7a7fcf370e74 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:22 +0100 Subject: [PATCH 19/41] Add organisation filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which belong to one of the selected organisations. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single organisation. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `organisations` as the parameter name to make it clear that more than one organisation can be selected and to distinguish it from the previous implementation which used `organisation`. We might want to consider redirecting URLs using `organisation` to use `organisations` if we want bookmarked URLs to continue to work as expected. The option-select component is only displayed if the current user is allowed to manage more than one organisation. I've made this more explicit than the original implementation by using User#manageable_organisations in UsersFilter#organisation_option_select_options. If the current user is allowed to manage exactly one organisation (i.e. they are an organisation admin) then the filtered users heading is qualified with the relevant organisation, e.g. "399 users in Government Digital Service". This is because the organisation option select will not be displayed in this scenario so we need to make it clear which organisation the users in the table belong to. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 --- app/controllers/users_controller.rb | 2 +- app/helpers/users_helper.rb | 9 +++ app/models/user.rb | 1 + app/models/users_filter.rb | 12 +++ app/views/users/_users_filter.html.erb | 11 +++ app/views/users/index.html.erb | 2 +- test/controllers/users_controller_test.rb | 24 ++++++ test/helpers/users_helper_test.rb | 23 ++++++ test/models/user_test.rb | 14 ++++ test/models/users_filter_test.rb | 96 +++++++++++++++++++++++ 10 files changed, 192 insertions(+), 2 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 383ed50ea..9e8c77946 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:filter, :page, :format, roles: []) + params.permit(:filter, :page, :format, roles: [], organisations: []) end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index da9f9fce6..566952afa 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -49,6 +49,15 @@ def formatted_number_of_users(users) pluralize(number_with_delimiter(users.total_count), "user") end + def filtered_users_heading(users) + count = formatted_number_of_users(users) + if current_user.manageable_organisations.one? + "#{count} in #{current_user.manageable_organisations.first.name}" + else + count + end + end + def assignable_user_roles current_user.manageable_roles end diff --git a/app/models/user.rb b/app/models/user.rb index 0bdf056fb..c38550744 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -64,6 +64,7 @@ class User < ApplicationRecord scope :web_users, -> { where(api_user: false) } scope :not_suspended, -> { where(suspended_at: nil) } scope :with_role, ->(role) { where(role:) } + scope :with_organisation, ->(organisation) { where(organisation:) } scope :with_partially_matching_name, ->(name) { where("name LIKE ?", "%#{name}%") } scope :with_partially_matching_email, ->(email) { where("email LIKE ?", "%#{email}%") } scope :with_partially_matching_name_or_email, ->(value) { with_partially_matching_name(value).or(with_partially_matching_email(value)) } diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index 90330c3a3..2101a43e3 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -12,6 +12,7 @@ def users filtered_users = @users filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] filtered_users = filtered_users.with_role(options[:roles]) if options[:roles] + filtered_users = filtered_users.with_organisation(options[:organisations]) if options[:organisations] filtered_users.includes(:organisation).order(:name) end @@ -28,4 +29,15 @@ def role_option_select_options } end end + + def organisation_option_select_options + scope = @current_user.manageable_organisations + scope.order(:name).joins(:users).uniq.map do |organisation| + { + label: organisation.name_with_abbreviation, + value: organisation.to_param, + checked: Array(options[:organisations]).include?(organisation.to_param), + } + end + end end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index d7cad41b8..b1dce0936 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -17,6 +17,17 @@ options: @filter.role_option_select_options, } %> + <% if current_user.manageable_organisations.many? %> + <%= render "components/option_select", { + key: "organisations", + title: "Organisations", + options_container_id: "organisations_filter", + show_filter: true, + closed_on_load: true, + options: @filter.organisation_option_select_options, + } %> + <% end %> +
    <%= render "govuk_publishing_components/components/button", { text: "Update results", diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 883807a86..bee467fd3 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -16,7 +16,7 @@
    -

    <%= formatted_number_of_users(@users) %>

    +

    <%= filtered_users_heading(@users) %>

    diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 8fa3000b1..269cd576c 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -343,6 +343,24 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(1)", text: "normal-user" end + should "filter by organisations" do + organisation1 = create(:organisation, name: "Organisation 1") + organisation2 = create(:organisation, name: "Organisation 2") + organisation3 = create(:organisation, name: "Organisation 3") + + create(:user, name: "user1-in-organisation1", organisation: organisation1) + create(:user, name: "user2-in-organisation1", organisation: organisation1) + create(:user, name: "user3-in-organisation2", organisation: organisation2) + create(:user, name: "user4-in-organisation3", organisation: organisation3) + + get :index, params: { organisations: [organisation1, organisation3] } + + assert_select "tr td:nth-child(1)", text: "user1-in-organisation1" + assert_select "tr td:nth-child(1)", text: "user2-in-organisation1" + assert_select "tr td:nth-child(1)", text: "user3-in-organisation2", count: 0 + assert_select "tr td:nth-child(1)", text: "user4-in-organisation3" + end + should "display link to clear all filters" do get :index @@ -894,6 +912,12 @@ def change_user_password(user_factory, new_password) assert_select "a", text: "Grant access to all users", count: 0 end + + should "not display organisations filter" do + get :index + + assert_select "#organisations_filter", count: 0 + end end end end diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index 7036071ed..cfdcc7e48 100644 --- a/test/helpers/users_helper_test.rb +++ b/test/helpers/users_helper_test.rb @@ -20,4 +20,27 @@ class UsersHelperTest < ActionView::TestCase test "two_step_status should reflect the user's status accurately when the user does not have 2sv set up" do assert_equal "Not set up", two_step_status(create(:user)) end + + context "#filtered_users_heading" do + setup do + @users = [build(:user), build(:user)] + @current_user = build(:user) + stubs(:formatted_number_of_users).with(@users).returns("2 users") + stubs(:current_user).returns(@current_user) + @organisation = build(:organisation) + end + + should "return formatted number of users" do + another_organisation = build(:organisation) + @current_user.stubs(:manageable_organisations).returns([@organisation, another_organisation]) + + assert_equal "2 users", filtered_users_heading(@users) + end + + should "return formatted number of users in specified organisation" do + @current_user.stubs(:manageable_organisations).returns([@organisation]) + + assert_equal "2 users in #{@organisation.name}", filtered_users_heading(@users) + end + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c2c1ea45d..54e9240d8 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -806,6 +806,20 @@ def setup end end + context ".with_organisation" do + should "only return users with specified organisation(s)" do + org1 = create(:organisation) + org2 = create(:organisation) + org3 = create(:organisation) + + user_in_org1 = create(:user, organisation: org1) + create(:user, organisation: org2) + user_in_org3 = create(:user, organisation: org3) + + assert_equal [user_in_org1, user_in_org3], User.with_organisation([org1, org3].map(&:to_param)) + end + end + def authenticate_access(user, app) ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index 394ac8dce..08a41009f 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -88,4 +88,100 @@ class UsersFilterTest < ActiveSupport::TestCase end end end + + context "when filtering by organisation" do + should "return users matching any of the specified organisations" do + organisation1 = create(:organisation, name: "Organisation 1") + organisation2 = create(:organisation, name: "Organisation 2") + organisation3 = create(:organisation, name: "Organisation 3") + + create(:user, name: "user1-in-organisation1", organisation: organisation1) + create(:user, name: "user2-in-organisation1", organisation: organisation1) + create(:user, name: "user3-in-organisation2", organisation: organisation2) + create(:user, name: "user4-in-organisation3", organisation: organisation3) + + filter = UsersFilter.new(User.all, @current_user, organisations: [organisation1, organisation3].map(&:to_param)) + + assert_equal %w[user1-in-organisation1 user2-in-organisation1 user4-in-organisation3], filter.users.map(&:name) + end + end + + context "#organisation_option_select_options" do + context "when current user is an admin" do + setup do + @organisation = create(:organisation, name: "Org1") + @current_user = create(:admin_user, organisation: @organisation) + end + + should "return select options for organisations that have any users" do + another_organisation = create(:organisation, name: "Org2") + create(:user, organisation: another_organisation) + + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.organisation_option_select_options + + expected_options = [ + { label: @organisation.name, value: @organisation.to_param, checked: false }, + { label: another_organisation.name, value: another_organisation.to_param, checked: false }, + ] + assert_equal expected_options, options + end + + should "return select options with `selected` set appropriately" do + another_organisation = create(:organisation, name: "Org2") + create(:user, organisation: another_organisation) + + filter = UsersFilter.new(User.all, @current_user, organisations: [another_organisation.to_param]) + options = filter.organisation_option_select_options + + expected_options = [ + { label: @organisation.name, value: @organisation.to_param, checked: false }, + { label: another_organisation.name, value: another_organisation.to_param, checked: true }, + ] + assert_equal expected_options, options + end + end + + context "when current user is a super organisation admin" do + setup do + @organisation = create(:organisation, name: "Org1") + @current_user = create(:super_organisation_admin_user, organisation: @organisation) + end + + should "return select options for organisation and sub-organisations that have any users" do + sub_organisation = create(:organisation, parent: @organisation, name: "Org2") + create(:organisation, parent: @organisation, name: "Org3") + create(:user, organisation: sub_organisation) + + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.organisation_option_select_options + + expected_options = [ + { label: @organisation.name, value: @organisation.to_param, checked: false }, + { label: sub_organisation.name, value: sub_organisation.to_param, checked: false }, + ] + assert_equal expected_options, options + end + end + + context "when current user is an organisation admin" do + setup do + @organisation = create(:organisation, name: "Org1") + @current_user = create(:organisation_admin_user, organisation: @organisation) + end + + should "return select options for only the user's organisation" do + another_organisation = create(:organisation, name: "Org2") + create(:user, organisation: another_organisation) + + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.organisation_option_select_options + + expected_options = [ + { label: @organisation.name, value: @organisation.to_param, checked: false }, + ] + assert_equal expected_options, options + end + end + end end From 603c7d0b5b1fcbc0312c16f2808da64cf06a2966 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:23 +0100 Subject: [PATCH 20/41] Add permission filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected permissions. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single permission. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `permissions` as the parameter name to make it clear that more than one permission can be selected and to distinguish it from the previous implementation which used `permission`. We might want to consider redirecting URLs using `permission` to use `permissions` if we want bookmarked URLs to continue to work as expected. I've used arel_table to generate the column name in the existing User.with_partially_matching_name & User.with_partially_matching_email scopes so that they work in conjunction with the new User.with_permission scope, i.e. to avoid ambiguity in column names. I've tried to make the implementation of UsersHelper#permission_option_select_options a bit more idioomatic than the original version in UserFilterHelper#user_filter_list_items: * The new implementation doesn't specify an order, because we can rely on the default scope order of Doorkeeper::Application & SupportedPermission. * I didn't feel as if plucking values made the query significantly more efficient and it cluttered up the code. * Using a map loop within a flat_map loop allowed me to use instances of Doorkeeper::Application & SupportedPermission which made the code a bit simpler / more readable. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 --- app/controllers/users_controller.rb | 2 +- app/models/user.rb | 5 +- app/models/users_filter.rb | 13 ++++++ app/views/users/_users_filter.html.erb | 9 ++++ test/controllers/users_controller_test.rb | 17 +++++++ test/models/user_test.rb | 16 +++++++ test/models/users_filter_test.rb | 57 +++++++++++++++++++++++ 7 files changed, 116 insertions(+), 3 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9e8c77946..2693b4099 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:filter, :page, :format, roles: [], organisations: []) + params.permit(:filter, :page, :format, roles: [], permissions: [], organisations: []) end end diff --git a/app/models/user.rb b/app/models/user.rb index c38550744..e7f72fa86 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -64,9 +64,10 @@ class User < ApplicationRecord scope :web_users, -> { where(api_user: false) } scope :not_suspended, -> { where(suspended_at: nil) } scope :with_role, ->(role) { where(role:) } + scope :with_permission, ->(permission) { joins(:supported_permissions).merge(SupportedPermission.where(id: permission)) } scope :with_organisation, ->(organisation) { where(organisation:) } - scope :with_partially_matching_name, ->(name) { where("name LIKE ?", "%#{name}%") } - scope :with_partially_matching_email, ->(email) { where("email LIKE ?", "%#{email}%") } + scope :with_partially_matching_name, ->(name) { where(arel_table[:name].matches("%#{name}%")) } + scope :with_partially_matching_email, ->(email) { where(arel_table[:email].matches("%#{email}%")) } scope :with_partially_matching_name_or_email, ->(value) { with_partially_matching_name(value).or(with_partially_matching_email(value)) } scope :last_signed_in_on, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) = date(?)", date) } scope :last_signed_in_before, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) < date(?)", date) } diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index 2101a43e3..f6ff9e885 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -12,6 +12,7 @@ def users filtered_users = @users filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] filtered_users = filtered_users.with_role(options[:roles]) if options[:roles] + filtered_users = filtered_users.with_permission(options[:permissions]) if options[:permissions] filtered_users = filtered_users.with_organisation(options[:organisations]) if options[:organisations] filtered_users.includes(:organisation).order(:name) end @@ -30,6 +31,18 @@ def role_option_select_options end end + def permission_option_select_options + Doorkeeper::Application.includes(:supported_permissions).flat_map do |application| + application.supported_permissions.map do |permission| + { + label: "#{application.name} #{permission.name}", + value: permission.to_param, + checked: Array(options[:permissions]).include?(permission.to_param), + } + end + end + end + def organisation_option_select_options scope = @current_user.manageable_organisations scope.order(:name).joins(:users).uniq.map do |organisation| diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index b1dce0936..a0393f7e9 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -28,6 +28,15 @@ } %> <% end %> + <%= render "components/option_select", { + key: "permissions", + title: "Permissions", + options_container_id: "permissions_filter", + show_filter: true, + closed_on_load: true, + options: @filter.permission_option_select_options, + } %> +
    <%= render "govuk_publishing_components/components/button", { text: "Update results", diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 269cd576c..059259b13 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -343,6 +343,23 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(1)", text: "normal-user" end + should "filter by permissions" do + app1 = create(:application, name: "App 1") + app2 = create(:application, name: "App 2") + + permission1 = create(:supported_permission, application: app1, name: "Permission 1") + + create(:user, name: "user1", supported_permissions: [app1.signin_permission, permission1]) + create(:user, name: "user2", supported_permissions: []) + create(:user, name: "user3", supported_permissions: [app2.signin_permission, permission1]) + + get :index, params: { permissions: [app1.signin_permission, app2.signin_permission] } + + assert_select "tr td:nth-child(1)", text: "user1" + assert_select "tr td:nth-child(1)", text: "user2", count: 0 + assert_select "tr td:nth-child(1)", text: "user3" + end + should "filter by organisations" do organisation1 = create(:organisation, name: "Organisation 1") organisation2 = create(:organisation, name: "Organisation 2") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 54e9240d8..e44daf0eb 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -806,6 +806,22 @@ def setup end end + context ".with_permission" do + should "only return users with specified permission(s)" do + app1 = create(:application) + app2 = create(:application) + + permission1 = create(:supported_permission, application: app1) + + user1 = create(:user, supported_permissions: [app1.signin_permission, permission1]) + create(:user, supported_permissions: []) + user2 = create(:user, supported_permissions: [app2.signin_permission, permission1]) + + specified_permissions = [app1.signin_permission, app2.signin_permission] + assert_equal [user1, user2], User.with_permission(specified_permissions.map(&:to_param)) + end + end + context ".with_organisation" do should "only return users with specified organisation(s)" do org1 = create(:organisation) diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index 08a41009f..d6f032488 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -89,6 +89,63 @@ class UsersFilterTest < ActiveSupport::TestCase end end + context "when filtering by permission" do + should "return users matching any of the specified permissions" do + app1 = create(:application, name: "App 1") + app2 = create(:application, name: "App 2") + + permission1 = create(:supported_permission, application: app1, name: "Permission 1") + + create(:user, name: "user1", supported_permissions: [app1.signin_permission, permission1]) + create(:user, name: "user2", supported_permissions: []) + create(:user, name: "user3", supported_permissions: [app2.signin_permission, permission1]) + + filter = UsersFilter.new(User.all, @current_user, permissions: [permission1].map(&:to_param)) + + assert_equal %w[user1 user3], filter.users.map(&:name) + end + end + + context "#permission_option_select_options" do + setup do + @app1 = create(:application, name: "App 1") + @app2 = create(:application, name: "App 2") + + @permission1 = create(:supported_permission, application: @app1, name: "Permission 1") + end + + context "when no permissions are selected" do + should "return options for application permissions in alphabetical order with none checked" do + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.permission_option_select_options + + expected_options = [ + { label: "App 1 Permission 1", value: @permission1.to_param, checked: false }, + { label: "App 1 signin", value: @app1.signin_permission.to_param, checked: false }, + { label: "App 2 signin", value: @app2.signin_permission.to_param, checked: false }, + ] + + assert_equal expected_options, options + end + end + + context "when some permissions are selected" do + should "return options for application permissions with relevant options checked" do + selected_permissions = [@app2.signin_permission, @permission1] + filter = UsersFilter.new(User.all, @current_user, permissions: selected_permissions.map(&:to_param)) + options = filter.permission_option_select_options + + expected_options = [ + { label: "App 1 Permission 1", value: @permission1.to_param, checked: true }, + { label: "App 1 signin", value: @app1.signin_permission.to_param, checked: false }, + { label: "App 2 signin", value: @app2.signin_permission.to_param, checked: true }, + ] + + assert_equal expected_options, options + end + end + end + context "when filtering by organisation" do should "return users matching any of the specified organisations" do organisation1 = create(:organisation, name: "Organisation 1") From 91d5f8b88b70459d6cd5af9d1457b259e6fbd8cc Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:24 +0100 Subject: [PATCH 21/41] Add status filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected statuses. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single status. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `statuses` as the parameter name to make it clear that more than one status can be selected and to distinguish it from the previous implementation which used `status`. We might want to consider redirecting URLs using `status` to use `statuses` if we want bookmarked URLs to continue to work as expected. Unlike for the previous implementation of User.with_status which used a case statement to return the relevant un-named scope, for User.with_statuses I've implemented a bunch of named scopes which make it easier to combine them programmatically using ActiveRecord::QueryMethods#or. This makes the code easier to test and I think it makes it easier to read. I'm hoping we might be able to re-use the named scopes elsewhere in the code. I've also introduced some new user factories for the various user "states". Note that the new User.locked scope perpetuates the issue where we don't consider Devise's time-based unlock strategy, i.e. where User#access_locked? may return false when the user is still included in the User.locked scope because User#locked_at is not nil. This problem is captured in this Trello card [2]. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 [2]: https://trello.com/c/88jMyKuf --- app/controllers/users_controller.rb | 2 +- app/models/user.rb | 19 ++++ app/models/users_filter.rb | 11 +++ app/views/users/_users_filter.html.erb | 7 ++ test/controllers/users_controller_test.rb | 14 +++ test/factories/users.rb | 13 +++ test/models/user_test.rb | 102 ++++++++++++++++++++++ test/models/users_filter_test.rb | 45 ++++++++++ 8 files changed, 212 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2693b4099..f3ad97a2a 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:filter, :page, :format, roles: [], permissions: [], organisations: []) + params.permit(:filter, :page, :format, statuses: [], roles: [], permissions: [], organisations: []) end end diff --git a/app/models/user.rb b/app/models/user.rb index e7f72fa86..1016fc08b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -62,13 +62,22 @@ class User < ApplicationRecord before_save :strip_whitespace_from_name scope :web_users, -> { where(api_user: false) } + + scope :suspended, -> { where.not(suspended_at: nil) } scope :not_suspended, -> { where(suspended_at: nil) } + scope :invited, -> { where.not(invitation_sent_at: nil).where(invitation_accepted_at: nil) } + scope :not_invited, -> { where(invitation_sent_at: nil).or(where.not(invitation_accepted_at: nil)) } + scope :locked, -> { where.not(locked_at: nil) } + scope :not_locked, -> { where(locked_at: nil) } + scope :active, -> { not_suspended.not_invited.not_locked } + scope :with_role, ->(role) { where(role:) } scope :with_permission, ->(permission) { joins(:supported_permissions).merge(SupportedPermission.where(id: permission)) } scope :with_organisation, ->(organisation) { where(organisation:) } scope :with_partially_matching_name, ->(name) { where(arel_table[:name].matches("%#{name}%")) } scope :with_partially_matching_email, ->(email) { where(arel_table[:email].matches("%#{email}%")) } scope :with_partially_matching_name_or_email, ->(value) { with_partially_matching_name(value).or(with_partially_matching_email(value)) } + scope :last_signed_in_on, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) = date(?)", date) } scope :last_signed_in_before, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) < date(?)", date) } scope :last_signed_in_after, ->(date) { web_users.not_suspended.where("date(current_sign_in_at) >= date(?)", date) } @@ -77,6 +86,16 @@ class User < ApplicationRecord scope :not_recently_unsuspended, -> { where(["unsuspended_at IS NULL OR unsuspended_at < ?", UNSUSPENSION_GRACE_PERIOD.ago]) } scope :with_access_to_application, ->(application) { UsersWithAccess.new(self, application).users } + def self.with_statuses(statuses) + permitted_statuses = statuses.intersection(USER_STATUSES) + relations = permitted_statuses.map { |s| public_send(s) } + relation = relations.pop || all + while (next_relation = relations.pop) + relation = relation.or(next_relation) + end + relation + end + def require_2sv? return require_2sv unless organisation diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index f6ff9e885..a413d5bdf 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -11,6 +11,7 @@ def initialize(users, current_user, options = {}) def users filtered_users = @users filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] + filtered_users = filtered_users.with_statuses(options[:statuses]) if options[:statuses] filtered_users = filtered_users.with_role(options[:roles]) if options[:roles] filtered_users = filtered_users.with_permission(options[:permissions]) if options[:permissions] filtered_users = filtered_users.with_organisation(options[:organisations]) if options[:organisations] @@ -21,6 +22,16 @@ def paginated_users users.page(options[:page]).per(options[:per_page]) end + def status_option_select_options + User::USER_STATUSES.map do |status| + { + label: status.humanize.capitalize, + value: status, + checked: Array(options[:statuses]).include?(status), + } + end + end + def role_option_select_options @current_user.manageable_roles.map do |role| { diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index a0393f7e9..67de92022 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -9,6 +9,13 @@ margin_bottom: 4, } %> + <%= render "components/option_select", { + key: "statuses", + title: "Status", + options_container_id: "statuses_filter", + options: @filter.status_option_select_options, + } %> + <%= render "components/option_select", { key: "roles", title: "Roles", diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 059259b13..7e636956a 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -331,6 +331,20 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(2)", text: /does-match/, count: 2 end + should "filter by statuses" do + create(:active_user, name: "active-user") + create(:suspended_user, name: "suspended-user") + create(:invited_user, name: "invited-user") + create(:locked_user, name: "locked-user") + + get :index, params: { statuses: %w[locked suspended] } + + assert_select "tr td:nth-child(1)", text: "active-user", count: 0 + assert_select "tr td:nth-child(1)", text: "suspended-user" + assert_select "tr td:nth-child(1)", text: "invited-user", count: 0 + assert_select "tr td:nth-child(1)", text: "locked-user" + end + should "filter by roles" do create(:admin_user, name: "admin-user") create(:organisation_admin_user, name: "organisation-admin-user") diff --git a/test/factories/users.rb b/test/factories/users.rb index c78a3d7df..94965ae90 100644 --- a/test/factories/users.rb +++ b/test/factories/users.rb @@ -83,11 +83,24 @@ role { Roles::OrganisationAdmin.role_name } end + factory :invited_user, parent: :user do + invitation_sent_at { 1.minute.ago } + invitation_accepted_at { nil } + end + + factory :active_user, parent: :invited_user do + invitation_accepted_at { Time.zone.now } + end + factory :suspended_user, parent: :user do suspended_at { Time.zone.now } reason_for_suspension { "Testing" } end + factory :locked_user, parent: :user do + locked_at { Time.zone.now } + end + factory :user_in_organisation, parent: :user do association :organisation, factory: :organisation end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index e44daf0eb..221f103fc 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -698,6 +698,70 @@ def setup end end + context ".suspended" do + should "only return suspended users" do + suspended_user = create(:suspended_user) + + assert_equal [suspended_user], User.suspended + end + end + + context ".not_suspended" do + should "only return users that have not been suspended" do + create(:suspended_user) + active_user = create(:active_user) + + assert_equal [@user, active_user], User.not_suspended + end + end + + context ".invited" do + should "only return users that have been invited but have not accepted" do + invited_user = create(:invited_user) + create(:active_user) + + assert_equal [invited_user], User.invited + end + end + + context ".not_invited" do + should "only return users that have not been invited or have accepted" do + create(:invited_user) + active_user = create(:active_user) + + assert_equal [@user, active_user], User.not_invited + end + end + + context ".locked" do + should "only return users that have been locked" do + locked_user = create(:locked_user) + create(:active_user) + + assert_equal [locked_user], User.locked + end + end + + context ".not_locked" do + should "only return users that have not been locked" do + create(:locked_user) + active_user = create(:active_user) + + assert_equal [@user, active_user], User.not_locked + end + end + + context ".active" do + should "only return users that are considered active" do + create(:invited_user) + create(:locked_user) + create(:suspended_user) + active_user = create(:active_user) + + assert_equal [@user, active_user], User.active + end + end + context ".with_partially_matching_name" do should "only return users with a name that includes the value" do user1 = create(:user, name: "does-match1") @@ -782,6 +846,44 @@ def setup end end + context ".with_statuses" do + should "only return suspended or invited users" do + suspended_user = create(:suspended_user) + invited_user = create(:invited_user) + create(:locked_user) + create(:active_user) + + assert_equal [suspended_user, invited_user], User.with_statuses(%w[suspended invited]) + end + + should "only return active or locked users" do + create(:suspended_user) + create(:invited_user) + locked_user = create(:locked_user) + active_user = create(:active_user) + + assert_equal [@user, locked_user, active_user], User.with_statuses(%w[active locked]) + end + + should "return all users if no statuses specified" do + suspended_user = create(:suspended_user) + invited_user = create(:invited_user) + locked_user = create(:locked_user) + active_user = create(:active_user) + + assert_equal [@user, suspended_user, invited_user, locked_user, active_user], User.with_statuses([]) + end + + should "ignore any non-existent statuses" do + suspended_user = create(:suspended_user) + invited_user = create(:invited_user) + locked_user = create(:locked_user) + active_user = create(:active_user) + + assert_equal [@user, suspended_user, invited_user, locked_user, active_user], User.with_statuses(%w[non-existent]) + end + end + context ".with_role" do should "only return users with specified role(s)" do admin_user = create(:admin_user) diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index d6f032488..e1e63120e 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -44,6 +44,51 @@ class UsersFilterTest < ActiveSupport::TestCase end end + context "when filtering by status" do + should "return users matching any of the specified statuses" do + create(:suspended_user, name: "suspended-user") + create(:invited_user, name: "invited-user") + create(:locked_user, name: "locked-user") + create(:active_user, name: "active-user") + + filter = UsersFilter.new(User.all, @current_user, statuses: %w[suspended locked]) + + assert_equal %w[locked-user suspended-user], filter.users.map(&:name) + end + end + + context "#status_option_select_options" do + context "when no statuses are selected" do + should "return options for statuses with none checked" do + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.status_option_select_options + + expected_options = [ + { label: "Suspended", value: "suspended", checked: false }, + { label: "Invited", value: "invited", checked: false }, + { label: "Locked", value: "locked", checked: false }, + { label: "Active", value: "active", checked: false }, + ] + assert_equal expected_options, options + end + end + + context "when some statuses are selected" do + should "return options for statuses with relevant options checked" do + filter = UsersFilter.new(User.all, @current_user, statuses: %w[invited active]) + options = filter.status_option_select_options + + expected_options = [ + { label: "Suspended", value: "suspended", checked: false }, + { label: "Invited", value: "invited", checked: true }, + { label: "Locked", value: "locked", checked: false }, + { label: "Active", value: "active", checked: true }, + ] + assert_equal expected_options, options + end + end + end + context "when filtering by role" do should "return users matching any of the specified roles" do create(:admin_user, name: "admin-user") From 688e49ec2cfbbac30cf6d818a61b6819cfcbcc15 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:25 +0100 Subject: [PATCH 22/41] Extract User#two_step_status & status constants This is more consistent with how User#status works and it will allow me to reuse the new User::TWO_STEP_STATUS_*** constants in a subsequent commit to avoid duplication. --- app/helpers/users_helper.rb | 8 +------- app/models/user.rb | 14 ++++++++++++++ test/models/user_test.rb | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 566952afa..fd5f5c2f0 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -4,13 +4,7 @@ def two_step_abbr_tag end def two_step_status(user) - if user.has_2sv? - "Enabled" - elsif user.exempt_from_2sv? - "Exempted" - else - "Not set up" - end + user.two_step_status.humanize.capitalize end def organisation_options(form_builder) diff --git a/app/models/user.rb b/app/models/user.rb index 1016fc08b..95541becc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,6 +20,10 @@ class User < ApplicationRecord USER_STATUS_LOCKED, USER_STATUS_ACTIVE].freeze + TWO_STEP_STATUS_ENABLED = "enabled".freeze + TWO_STEP_STATUS_NOT_SET_UP = "not_set_up".freeze + TWO_STEP_STATUS_EXEMPTED = "exempted".freeze + devise :database_authenticatable, :recoverable, :trackable, @@ -253,6 +257,16 @@ def status USER_STATUS_ACTIVE end + def two_step_status + if has_2sv? + TWO_STEP_STATUS_ENABLED + elsif exempt_from_2sv? + TWO_STEP_STATUS_EXEMPTED + else + TWO_STEP_STATUS_NOT_SET_UP + end + end + def role_class Roles.const_get(role.classify) end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 221f103fc..3d5f9542d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -641,6 +641,23 @@ def setup end end + context "#two_step_status" do + should "return 'enabled' when user has 2SV" do + user = build(:two_step_enabled_user) + assert_equal User::TWO_STEP_STATUS_ENABLED, user.two_step_status + end + + should "return 'exempt' when user has been exempted from 2SV" do + user = build(:two_step_exempted_user) + assert_equal User::TWO_STEP_STATUS_EXEMPTED, user.two_step_status + end + + should "return 'not_set_up' when user does not have 2SV and has not been exempted" do + user = build(:user) + assert_equal User::TWO_STEP_STATUS_NOT_SET_UP, user.two_step_status + end + end + context "authorised applications" do setup do @user = create(:user) From 0c4697ace0be79a174dba98efee216a7d19f17ad Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:26 +0100 Subject: [PATCH 23/41] Add 2SV status filtering to users page Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected 2SV statuses. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single 2SV status. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `two_step_statuses` as the parameter name to make it clear that more than one 2SV status can be selected and to distinguish it from the previous implementation which used `two_step_status`. We might want to consider redirecting URLs using `two_step_status` to use `two_step_statuses` if we want bookmarked URLs to continue to work as expected. Unlike for the previous implementation of User.with_2sv_enabled which used a case statement to return the relevant un-named scope, for User.with_2sv_statuses I've implemented a bunch of named scopes which make it easier to combine them programmatically using ActiveRecord::QueryMethods#or. This makes the code easier to test and I think it makes it easier to read. I'm hoping we might be able to re-use the named scopes elsewhere in the code. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 --- app/controllers/users_controller.rb | 2 +- app/models/user.rb | 22 +++++++ app/models/users_filter.rb | 11 ++++ app/views/users/_users_filter.html.erb | 8 +++ test/controllers/users_controller_test.rb | 12 ++++ test/models/user_test.rb | 80 +++++++++++++++++++++++ test/models/users_filter_test.rb | 42 ++++++++++++ 7 files changed, 176 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f3ad97a2a..34b684a2f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,6 @@ def password_params end def filter_params - params.permit(:filter, :page, :format, statuses: [], roles: [], permissions: [], organisations: []) + params.permit(:filter, :page, :format, statuses: [], two_step_statuses: [], roles: [], permissions: [], organisations: []) end end diff --git a/app/models/user.rb b/app/models/user.rb index 95541becc..3937790bf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -24,6 +24,12 @@ class User < ApplicationRecord TWO_STEP_STATUS_NOT_SET_UP = "not_set_up".freeze TWO_STEP_STATUS_EXEMPTED = "exempted".freeze + TWO_STEP_STATUSES_VS_NAMED_SCOPES = { + TWO_STEP_STATUS_ENABLED => "has_2sv", + TWO_STEP_STATUS_NOT_SET_UP => "not_setup_2sv", + TWO_STEP_STATUS_EXEMPTED => "exempt_from_2sv", + }.freeze + devise :database_authenticatable, :recoverable, :trackable, @@ -75,6 +81,12 @@ class User < ApplicationRecord scope :not_locked, -> { where(locked_at: nil) } scope :active, -> { not_suspended.not_invited.not_locked } + scope :exempt_from_2sv, -> { where.not(reason_for_2sv_exemption: nil) } + scope :not_exempt_from_2sv, -> { where(reason_for_2sv_exemption: nil) } + scope :has_2sv, -> { where.not(otp_secret_key: nil) } + scope :does_not_have_2sv, -> { where(otp_secret_key: nil) } + scope :not_setup_2sv, -> { not_exempt_from_2sv.does_not_have_2sv } + scope :with_role, ->(role) { where(role:) } scope :with_permission, ->(permission) { joins(:supported_permissions).merge(SupportedPermission.where(id: permission)) } scope :with_organisation, ->(organisation) { where(organisation:) } @@ -100,6 +112,16 @@ def self.with_statuses(statuses) relation end + def self.with_2sv_statuses(scope_names) + permitted_scopes = scope_names.intersection(TWO_STEP_STATUSES_VS_NAMED_SCOPES.values) + relations = permitted_scopes.map { |s| public_send(s) } + relation = relations.pop || all + while (next_relation = relations.pop) + relation = relation.or(next_relation) + end + relation + end + def require_2sv? return require_2sv unless organisation diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index a413d5bdf..c1cefe27e 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -12,6 +12,7 @@ def users filtered_users = @users filtered_users = filtered_users.with_partially_matching_name_or_email(options[:filter].strip) if options[:filter] filtered_users = filtered_users.with_statuses(options[:statuses]) if options[:statuses] + filtered_users = filtered_users.with_2sv_statuses(options[:two_step_statuses]) if options[:two_step_statuses] filtered_users = filtered_users.with_role(options[:roles]) if options[:roles] filtered_users = filtered_users.with_permission(options[:permissions]) if options[:permissions] filtered_users = filtered_users.with_organisation(options[:organisations]) if options[:organisations] @@ -32,6 +33,16 @@ def status_option_select_options end end + def two_step_status_option_select_options + User::TWO_STEP_STATUSES_VS_NAMED_SCOPES.map do |status, scope_name| + { + label: status.humanize.capitalize, + value: scope_name, + checked: Array(options[:two_step_statuses]).include?(scope_name), + } + end + end + def role_option_select_options @current_user.manageable_roles.map do |role| { diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 67de92022..1f6e08bd1 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -16,6 +16,14 @@ options: @filter.status_option_select_options, } %> + <%= render "components/option_select", { + key: "two_step_statuses", + title: "2SV Status", + options_container_id: "two_step_statuses_filter", + closed_on_load: true, + options: @filter.two_step_status_option_select_options, + } %> + <%= render "components/option_select", { key: "roles", title: "Roles", diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 7e636956a..0d232f810 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -345,6 +345,18 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(1)", text: "locked-user" end + should "filter by 2SV statuses" do + create(:user, name: "not-set-up-user") + create(:two_step_exempted_user, name: "exempted-user") + create(:two_step_enabled_user, name: "enabled-user") + + get :index, params: { two_step_statuses: %w[not_setup_2sv exempt_from_2sv] } + + assert_select "tr td:nth-child(1)", text: "not-set-up-user" + assert_select "tr td:nth-child(1)", text: "exempted-user" + assert_select "tr td:nth-child(1)", text: "enabled-user", count: 0 + end + should "filter by roles" do create(:admin_user, name: "admin-user") create(:organisation_admin_user, name: "organisation-admin-user") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 3d5f9542d..eb2274990 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -779,6 +779,52 @@ def setup end end + context ".exempt_from_2sv" do + should "only return users that have been exempted from 2SV" do + exempted_user = create(:two_step_exempted_user) + create(:two_step_enabled_user) + + assert_equal [exempted_user], User.exempt_from_2sv + end + end + + context ".not_exempt_from_2sv" do + should "only return users that have not been exempted from 2SV" do + create(:two_step_exempted_user) + enabled_user = create(:two_step_enabled_user) + + assert_equal [@user, enabled_user], User.not_exempt_from_2sv + end + end + + context ".has_2sv" do + should "only return users that have 2SV" do + create(:two_step_exempted_user) + enabled_user = create(:two_step_enabled_user) + + assert_equal [enabled_user], User.has_2sv + end + end + + context ".does_not_have_2sv" do + should "only return users that do not have 2SV" do + exempted_user = create(:two_step_exempted_user) + create(:two_step_enabled_user) + + assert_equal [@user, exempted_user], User.does_not_have_2sv + end + end + + context ".not_setup_2sv" do + should "only return users that have not been exempted from 2SV and do not have 2SV" do + not_set_up_user = create(:user) + create(:two_step_exempted_user) + create(:two_step_enabled_user) + + assert_equal [@user, not_set_up_user], User.not_setup_2sv + end + end + context ".with_partially_matching_name" do should "only return users with a name that includes the value" do user1 = create(:user, name: "does-match1") @@ -901,6 +947,40 @@ def setup end end + context ".with_2sv_statuses" do + should "only return not_set_up and exempted users" do + not_set_up_user = create(:user) + exempted_user = create(:two_step_exempted_user) + create(:two_step_enabled_user) + + assert_equal [@user, not_set_up_user, exempted_user], User.with_2sv_statuses(%w[not_setup_2sv exempt_from_2sv]) + end + + should "only return enabled users" do + create(:user) + create(:two_step_exempted_user) + enabled_user = create(:two_step_enabled_user) + + assert_equal [enabled_user], User.with_2sv_statuses(%w[has_2sv]) + end + + should "return all users if no statuses specified" do + not_set_up_user = create(:user) + exempted_user = create(:two_step_exempted_user) + enabled_user = create(:two_step_enabled_user) + + assert_equal [@user, not_set_up_user, exempted_user, enabled_user], User.with_2sv_statuses([]) + end + + should "ignore any non-existent statuses" do + not_set_up_user = create(:user) + exempted_user = create(:two_step_exempted_user) + enabled_user = create(:two_step_enabled_user) + + assert_equal [@user, not_set_up_user, exempted_user, enabled_user], User.with_2sv_statuses(%w[non-existent]) + end + end + context ".with_role" do should "only return users with specified role(s)" do admin_user = create(:admin_user) diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index e1e63120e..b4501d969 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -89,6 +89,48 @@ class UsersFilterTest < ActiveSupport::TestCase end end + context "when filtering by 2SV status" do + should "return users matching any of the specified 2SV statuses" do + create(:user, name: "not-set-up-user") + create(:two_step_exempted_user, name: "exempted-user") + create(:two_step_enabled_user, name: "enabled-user") + + filter = UsersFilter.new(User.all, @current_user, two_step_statuses: %w[not_setup_2sv exempt_from_2sv]) + + assert_equal %w[exempted-user not-set-up-user], filter.users.map(&:name) + end + end + + context "#two_step_status_option_select_options" do + context "when no 2SV statuses are selected" do + should "return options for 2SV statuses with none checked" do + filter = UsersFilter.new(User.all, @current_user, {}) + options = filter.two_step_status_option_select_options + + expected_options = [ + { label: "Enabled", value: "has_2sv", checked: false }, + { label: "Not set up", value: "not_setup_2sv", checked: false }, + { label: "Exempted", value: "exempt_from_2sv", checked: false }, + ] + assert_equal expected_options, options + end + end + + context "when some 2SV statuses are selected" do + should "return options for statuses with relevant options checked" do + filter = UsersFilter.new(User.all, @current_user, two_step_statuses: %w[not_setup_2sv exempt_from_2sv]) + options = filter.two_step_status_option_select_options + + expected_options = [ + { label: "Enabled", value: "has_2sv", checked: false }, + { label: "Not set up", value: "not_setup_2sv", checked: true }, + { label: "Exempted", value: "exempt_from_2sv", checked: true }, + ] + assert_equal expected_options, options + end + end + end + context "when filtering by role" do should "return users matching any of the specified roles" do create(:admin_user, name: "admin-user") From 782840bc2f62b49e87407c67e96429eec23eacd4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:28 +0100 Subject: [PATCH 24/41] Move CSV export link below filter & style as link Trello: https://trello.com/c/2gvEJKw9 As per the propposed design. --- app/views/users/_users_filter.html.erb | 4 ++++ app/views/users/index.html.erb | 10 ---------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 1f6e08bd1..3fe6c727f 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -60,4 +60,8 @@ <%= link_to "Clear all filters", users_path, class: "govuk-link govuk-link--no-visited-state" %>
    <% end %> + +
    + <%= link_to "Export #{formatted_number_of_users(@users)} as CSV", url_for(filter_params.merge(format: "csv")), class: "govuk-link govuk-link--no-visited-state" %> +
    diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index bee467fd3..e30845c8e 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -46,16 +46,6 @@
    -
    -
    - <%= render "govuk_publishing_components/components/button", { - text: "Export #{formatted_number_of_users(@users)} as CSV", - href: url_for(filter_params.merge(format: "csv")), - margin_bottom: 4, - } %> -
    -
    -
    <%= render "users_filter", filter: @filter %> From 8c221778f92318e5051160265c0c6b94f7e644a4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:29 +0100 Subject: [PATCH 25/41] Move number of users into table caption Trello: https://trello.com/c/2gvEJKw9 As per the propposed design. --- app/views/users/index.html.erb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index e30845c8e..fab0d9737 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -14,12 +14,6 @@ }) %> -
    -
    -

    <%= filtered_users_heading(@users) %>

    -
    -
    -
    @@ -52,8 +46,7 @@
    <%= render "govuk_publishing_components/components/table", { - caption: "Users", - caption_classes: "govuk-visually-hidden", + caption: filtered_users_heading(@users), head: [ { text: "Name" }, { text: "Email" }, From d0d7b816bece628b132d2b72f079f86503f628dd Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:30 +0100 Subject: [PATCH 26/41] Add horizontal lines to separate filters on users page Trello: https://trello.com/c/2gvEJKw9 As per the propposed design. I couldn't see a way to style the search component directly without targetting *all* instances of the component, so I've wrapped it in a div element with a CSS class which adds the bottom border. I've used the same CSS class to add a bottom border to the button group which was already in a containing div. --- app/assets/stylesheets/application.scss | 4 ++++ app/views/users/_users_filter.html.erb | 20 +++++++++++--------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 58486e4f8..b8deeb796 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -42,6 +42,10 @@ padding: 7px 10px; } +.app-bottom-separator { + border-bottom: 1px solid $govuk-border-colour; +} + .govuk-error-message p { margin: 0; } diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 3fe6c727f..177fc513b 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -1,13 +1,15 @@
    <%= form_with url: users_path, method: :get do |form| %> - <%= render "govuk_publishing_components/components/search", { - id: "name_or_email_filter", - name: "filter", - label_text: "Search by name or email", - label_size: "s", - value: filter.options[:filter], - margin_bottom: 4, - } %> +
    + <%= render "govuk_publishing_components/components/search", { + id: "name_or_email_filter", + name: "filter", + label_text: "Search by name or email", + label_size: "s", + value: filter.options[:filter], + margin_bottom: 4, + } %> +
    <%= render "components/option_select", { key: "statuses", @@ -52,7 +54,7 @@ options: @filter.permission_option_select_options, } %> -
    +
    <%= render "govuk_publishing_components/components/button", { text: "Update results", } %> From b02684200271f8a7c792f46f9edb07e243c96fdf Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:31 +0100 Subject: [PATCH 27/41] Add "Filter results" heading to sidebar on users page Trello: https://trello.com/c/2gvEJKw9 As per the propposed design. --- app/views/users/_users_filter.html.erb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 177fc513b..8a8b6c40b 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -11,6 +11,11 @@ } %>
    + <%= render "govuk_publishing_components/components/heading", { + text: "Filter results", + padding: true, + } %> + <%= render "components/option_select", { key: "statuses", title: "Status", From 301bba5af0bc0a89c8bc8cb635336d713505b243 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:32 +0100 Subject: [PATCH 28/41] Avoid unpermitted parameter warnings in log I was seeing the following warning in the log when updating the filters on the users page: Unpermitted parameter: :option-select-filter It seems as if the options-select component adds an input with this name, but we don't need it in our implementation, so I've chosen to accept it and then immediately discard it. --- app/controllers/users_controller.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 34b684a2f..f66fb2005 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -188,6 +188,9 @@ def password_params end def filter_params - params.permit(:filter, :page, :format, statuses: [], two_step_statuses: [], roles: [], permissions: [], organisations: []) + params.permit( + :filter, :page, :format, :"option-select-filter", + statuses: [], two_step_statuses: [], roles: [], permissions: [], organisations: [] + ).except(:"option-select-filter") end end From 66e402267628cb683c26dbfdcb5a2528434507c4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:33 +0100 Subject: [PATCH 29/41] Open option-select components if any options selected This makes the option-select components on the users page "open" if any of its options are selected. The option-select for statuses will also "open" if none of the option-select components have any options selected - this is to make it clearer to users what's hidden away in the "closed". I think this will also make things less confusing when we add JS to submit the form when any of the checkboxes changes state. --- app/controllers/users_controller.rb | 2 +- app/models/users_filter.rb | 11 +++++++++++ app/views/users/_users_filter.html.erb | 9 +++++---- test/models/users_filter_test.rb | 24 ++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f66fb2005..38a7b0a39 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -190,7 +190,7 @@ def password_params def filter_params params.permit( :filter, :page, :format, :"option-select-filter", - statuses: [], two_step_statuses: [], roles: [], permissions: [], organisations: [] + **UsersFilter::PERMITTED_CHECKBOX_FILTER_PARAMS ).except(:"option-select-filter") end end diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index c1cefe27e..249767b34 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -1,4 +1,7 @@ class UsersFilter + CHECKBOX_FILTER_KEYS = %i[statuses two_step_statuses roles permissions organisations].freeze + PERMITTED_CHECKBOX_FILTER_PARAMS = CHECKBOX_FILTER_KEYS.each.with_object({}) { |k, h| h[k] = [] }.freeze + attr_reader :options def initialize(users, current_user, options = {}) @@ -75,4 +78,12 @@ def organisation_option_select_options } end end + + def no_options_selected_for?(key) + Array(options[key]).none? + end + + def any_options_selected? + options.slice(*CHECKBOX_FILTER_KEYS).values.any? + end end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 8a8b6c40b..630a94ada 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -20,6 +20,7 @@ key: "statuses", title: "Status", options_container_id: "statuses_filter", + closed_on_load: @filter.no_options_selected_for?(:statuses) && @filter.any_options_selected?, options: @filter.status_option_select_options, } %> @@ -27,7 +28,7 @@ key: "two_step_statuses", title: "2SV Status", options_container_id: "two_step_statuses_filter", - closed_on_load: true, + closed_on_load: @filter.no_options_selected_for?(:two_step_statuses), options: @filter.two_step_status_option_select_options, } %> @@ -35,7 +36,7 @@ key: "roles", title: "Roles", options_container_id: "roles_filter", - closed_on_load: true, + closed_on_load: @filter.no_options_selected_for?(:roles), options: @filter.role_option_select_options, } %> @@ -45,7 +46,7 @@ title: "Organisations", options_container_id: "organisations_filter", show_filter: true, - closed_on_load: true, + closed_on_load: @filter.no_options_selected_for?(:organisations), options: @filter.organisation_option_select_options, } %> <% end %> @@ -55,7 +56,7 @@ title: "Permissions", options_container_id: "permissions_filter", show_filter: true, - closed_on_load: true, + closed_on_load: @filter.no_options_selected_for?(:permissions), options: @filter.permission_option_select_options, } %> diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index b4501d969..412d7bf26 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -328,4 +328,28 @@ class UsersFilterTest < ActiveSupport::TestCase end end end + + context "#no_options_selected_for?" do + should "return true when there are no options selected for the specified key" do + filter = UsersFilter.new(User.all, @current_user, {}) + assert filter.no_options_selected_for?(:statuses) + end + + should "return false when there is at least one option selected for the specified key" do + filter = UsersFilter.new(User.all, @current_user, { statuses: %w[active] }) + assert_not filter.no_options_selected_for?(:statuses) + end + end + + context "#any_options_selected?" do + should "return true when this is at least one checkbox filter option selected" do + filter = UsersFilter.new(User.all, @current_user, { statuses: %w[active] }) + assert filter.any_options_selected? + end + + should "return false when there are no checkbox filter options selected" do + filter = UsersFilter.new(User.all, @current_user, { filter: "Jill", page: 1, per_page: 25 }) + assert_not filter.any_options_selected? + end + end end From deb4739647a45739522cfcf23cde1fa6f82da1e5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:34 +0100 Subject: [PATCH 30/41] Put selected options first when there are a lot of options There are a lot of options in the option-select component for organisations & permissions on the users index page. While it's easy enough to find a specific option using the search box (displayed because show_filter is set to true), it's not so easy to find the options that have already been selected unless you can remember the option label. Putting the selected options above all the unselected options should make things much easier. --- app/models/users_filter.rb | 4 ++++ app/views/users/_users_filter.html.erb | 4 ++-- test/models/users_filter_test.rb | 20 ++++++++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index 249767b34..ab16cdaef 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -4,6 +4,10 @@ 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 630a94ada..30872fc83 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -47,7 +47,7 @@ options_container_id: "organisations_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:organisations), - options: @filter.organisation_option_select_options, + options: UsersFilter.with_checked_at_top(@filter.organisation_option_select_options), } %> <% end %> @@ -57,7 +57,7 @@ options_container_id: "permissions_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:permissions), - options: @filter.permission_option_select_options, + options: UsersFilter.with_checked_at_top(@filter.permission_option_select_options), } %>
    diff --git a/test/models/users_filter_test.rb b/test/models/users_filter_test.rb index 412d7bf26..55b1c05b5 100644 --- a/test/models/users_filter_test.rb +++ b/test/models/users_filter_test.rb @@ -5,6 +5,26 @@ 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 50e22bd186e047ff5a4c3f611b0a9afbd2c1d6e3 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:36 +0100 Subject: [PATCH 31/41] Redirect legacy users filters on users index page The filters on the users index page now allow multiple options to be selected and the param keys have changed from singular to plural. Also the values for the two-step status filter have changed to match the relevant User scopes. This change redirects requests to the users index page that use the legacy filter params. This is in case anyone has bookmarked some of these URLs. Note that the redirection won't handle all URLs containing a mixture of legacy and non-legacy params, but this seems like a real edge case that's not worth worrying about. It might be worth adding a flash message to instruct users to update their bookmark so that we can safely remove this functionality in the not too distant future, but I haven't done that here. --- app/controllers/users_controller.rb | 9 ++++ app/models/legacy_users_filter.rb | 32 ++++++++++++ test/controllers/users_controller_test.rb | 8 +++ test/models/legacy_users_filter_test.rb | 63 +++++++++++++++++++++++ 4 files changed, 112 insertions(+) create mode 100644 app/models/legacy_users_filter.rb create mode 100644 test/models/legacy_users_filter_test.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 38a7b0a39..a81e219e7 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -8,6 +8,7 @@ class UsersController < ApplicationController before_action :authenticate_user!, except: :show before_action :load_and_authorize_user, except: %i[index show] before_action :allow_no_application_access, only: [:update] + before_action :redirect_legacy_filters, only: [:index] helper_method :applications_and_permissions, :filter_params respond_to :html @@ -190,7 +191,15 @@ def password_params def filter_params params.permit( :filter, :page, :format, :"option-select-filter", + *LegacyUsersFilter::PARAM_KEYS, **UsersFilter::PERMITTED_CHECKBOX_FILTER_PARAMS ).except(:"option-select-filter") end + + def redirect_legacy_filters + filter = LegacyUsersFilter.new(filter_params) + if filter.redirect? + redirect_to users_path(filter.options) + end + end end diff --git a/app/models/legacy_users_filter.rb b/app/models/legacy_users_filter.rb new file mode 100644 index 000000000..257855f35 --- /dev/null +++ b/app/models/legacy_users_filter.rb @@ -0,0 +1,32 @@ +class LegacyUsersFilter + PARAM_KEYS = %i[status two_step_status role permission organisation].freeze + LEGACY_TWO_STEP_STATUS_VS_TWO_STEP_STATUS = { + "true" => User::TWO_STEP_STATUS_ENABLED, + "false" => User::TWO_STEP_STATUS_NOT_SET_UP, + "exempt" => User::TWO_STEP_STATUS_EXEMPTED, + }.freeze + + def initialize(options = {}) + @options = options + end + + def redirect? + !@options.slice(*PARAM_KEYS).empty? + end + + def options + @options.except(*PARAM_KEYS).tap do |o| + o[:statuses] = [@options[:status]] if @options[:status].present? + o[:two_step_statuses] = [two_step_status_from(@options[:two_step_status])] if @options[:two_step_status].present? + o[:roles] = [@options[:role]] if @options[:role].present? + o[:organisations] = [@options[:organisation]] if @options[:organisation].present? + o[:permissions] = [@options[:permission]] if @options[:permission].present? + end + end + +private + + def two_step_status_from(legacy_two_step_status) + LEGACY_TWO_STEP_STATUS_VS_TWO_STEP_STATUS[legacy_two_step_status] + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 0d232f810..7ed92d45f 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -409,6 +409,14 @@ def change_user_password(user_factory, new_password) assert_select "a", text: "Clear all filters", href: users_path end + + should "redirect legacy filters" do + organisation1 = create(:organisation, name: "Organisation 1") + + get :index, params: { organisation: organisation1 } + + assert_redirected_to users_path(organisations: [organisation1]) + end end context "CSV export" do diff --git a/test/models/legacy_users_filter_test.rb b/test/models/legacy_users_filter_test.rb new file mode 100644 index 000000000..1b42267a4 --- /dev/null +++ b/test/models/legacy_users_filter_test.rb @@ -0,0 +1,63 @@ +require "test_helper" + +class LegacyUsersFilterTest < ActiveSupport::TestCase + context "#redirect?" do + should "return true if there are any legacy filter params" do + filter = LegacyUsersFilter.new({ status: User::USER_STATUS_ACTIVE }) + + assert filter.redirect? + end + + should "return false if there are no legacy filter params" do + filter = LegacyUsersFilter.new({ non_legacy_filter_key: 456 }) + + assert_not filter.redirect? + end + end + + context "#options" do + should "transform status legacy filter -> statuses array" do + filter = LegacyUsersFilter.new({ status: User::USER_STATUS_ACTIVE }) + + assert_equal({ statuses: [User::USER_STATUS_ACTIVE] }, filter.options) + end + + should "transform two_step_status legacy filter -> two_step_statuses array when value is 'true'" do + filter = LegacyUsersFilter.new({ two_step_status: "true" }) + + assert_equal({ two_step_statuses: [User::TWO_STEP_STATUS_ENABLED] }, filter.options) + end + + should "transform two_step_status legacy filter -> two_step_statuses array when value is 'false'" do + filter = LegacyUsersFilter.new({ two_step_status: "false" }) + + assert_equal({ two_step_statuses: [User::TWO_STEP_STATUS_NOT_SET_UP] }, filter.options) + end + + should "transform two_step_status legacy filter -> two_step_statuses array when value is 'exempt'" do + filter = LegacyUsersFilter.new({ two_step_status: "exempt" }) + + assert_equal({ two_step_statuses: [User::TWO_STEP_STATUS_EXEMPTED] }, filter.options) + end + + should "transform role legacy filter -> roles array" do + filter = LegacyUsersFilter.new({ role: Roles::Admin.role_name }) + + assert_equal({ roles: [Roles::Admin.role_name] }, filter.options) + end + + should "transform organisation legacy filter -> organisations array" do + organisation = create(:organisation) + filter = LegacyUsersFilter.new({ organisation: organisation.to_param }) + + assert_equal({ organisations: [organisation.to_param] }, filter.options) + end + + should "transform permission legacy filter -> permissions array" do + permission = create(:supported_permission) + filter = LegacyUsersFilter.new({ permission: permission.to_param }) + + assert_equal({ permissions: [permission.to_param] }, filter.options) + end + end +end From 0df53af54a48aa58e914ac1ef17fb7e499b7c280 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:37 +0100 Subject: [PATCH 32/41] Automatically submit users filter form This introduces a new AutoSubmitForm JS module which is activated for the form on the users index page. This JS automatically submits the form whenever the form's change event fires, i.e. whenever one of the filter checkboxes is checked/unchecked. The "auto-submit-ignore" data attribute should be a comma-separated list of input names for which to ignore the form's change event. We use this for the "option-select-filter" text inputs which are dynamically added to the option-select component when `show_filter` is set to `true`. The values of those text inputs are not used on the server-side and so we don't want to auto-submit the form when they change. --- .../javascripts/modules/auto-submit-form.js | 21 ++++++++ app/models/users_filter.rb | 25 +++++---- app/views/users/_users_filter.html.erb | 17 +++--- app/views/users/index.html.erb | 52 ++++++++++--------- 4 files changed, 75 insertions(+), 40 deletions(-) create mode 100644 app/assets/javascripts/modules/auto-submit-form.js diff --git a/app/assets/javascripts/modules/auto-submit-form.js b/app/assets/javascripts/modules/auto-submit-form.js new file mode 100644 index 000000000..a400db3d2 --- /dev/null +++ b/app/assets/javascripts/modules/auto-submit-form.js @@ -0,0 +1,21 @@ +window.GOVUK = window.GOVUK || {} +window.GOVUK.Modules = window.GOVUK.Modules || {}; + +(function (Modules) { + 'use strict' + + function AutoSubmitForm (module) { + this.module = module + this.module.ignore = this.module.getAttribute('data-auto-submit-ignore').split(',') + } + + AutoSubmitForm.prototype.init = function () { + this.module.addEventListener('change', function (e) { + if (!this.module.ignore.includes(e.target.getAttribute('name'))) { + this.module.submit() + } + }.bind(this)) + } + + Modules.AutoSubmitForm = AutoSubmitForm +})(window.GOVUK.Modules) diff --git a/app/models/users_filter.rb b/app/models/users_filter.rb index ab16cdaef..299a17a15 100644 --- a/app/models/users_filter.rb +++ b/app/models/users_filter.rb @@ -30,56 +30,61 @@ def paginated_users users.page(options[:page]).per(options[:per_page]) end - def status_option_select_options + def status_option_select_options(aria_controls_id: nil) User::USER_STATUSES.map do |status| { label: status.humanize.capitalize, + controls: aria_controls_id, value: status, checked: Array(options[:statuses]).include?(status), - } + }.compact end end - def two_step_status_option_select_options + def two_step_status_option_select_options(aria_controls_id: nil) User::TWO_STEP_STATUSES_VS_NAMED_SCOPES.map do |status, scope_name| { label: status.humanize.capitalize, + controls: aria_controls_id, value: scope_name, checked: Array(options[:two_step_statuses]).include?(scope_name), - } + }.compact end end - def role_option_select_options + def role_option_select_options(aria_controls_id: nil) @current_user.manageable_roles.map do |role| { label: role.humanize.capitalize, + controls: aria_controls_id, value: role, checked: Array(options[:roles]).include?(role), - } + }.compact end end - def permission_option_select_options + def permission_option_select_options(aria_controls_id: nil) Doorkeeper::Application.includes(:supported_permissions).flat_map do |application| application.supported_permissions.map do |permission| { label: "#{application.name} #{permission.name}", + controls: aria_controls_id, value: permission.to_param, checked: Array(options[:permissions]).include?(permission.to_param), - } + }.compact end end end - def organisation_option_select_options + def organisation_option_select_options(aria_controls_id: nil) scope = @current_user.manageable_organisations scope.order(:name).joins(:users).uniq.map do |organisation| { label: organisation.name_with_abbreviation, + controls: aria_controls_id, value: organisation.to_param, checked: Array(options[:organisations]).include?(organisation.to_param), - } + }.compact end end diff --git a/app/views/users/_users_filter.html.erb b/app/views/users/_users_filter.html.erb index 30872fc83..e8164b974 100644 --- a/app/views/users/_users_filter.html.erb +++ b/app/views/users/_users_filter.html.erb @@ -1,11 +1,12 @@
    - <%= form_with url: users_path, method: :get do |form| %> + <%= form_with url: users_path, method: :get, data: { module: "auto-submit-form", "auto-submit-ignore": "option-select-filter" } do |form| %>
    <%= render "govuk_publishing_components/components/search", { id: "name_or_email_filter", name: "filter", label_text: "Search by name or email", label_size: "s", + aria_controls: filtered_users_id, value: filter.options[:filter], margin_bottom: 4, } %> @@ -21,7 +22,7 @@ title: "Status", options_container_id: "statuses_filter", closed_on_load: @filter.no_options_selected_for?(:statuses) && @filter.any_options_selected?, - options: @filter.status_option_select_options, + options: @filter.status_option_select_options(aria_controls_id: filtered_users_id), } %> <%= render "components/option_select", { @@ -29,7 +30,7 @@ title: "2SV Status", options_container_id: "two_step_statuses_filter", closed_on_load: @filter.no_options_selected_for?(:two_step_statuses), - options: @filter.two_step_status_option_select_options, + options: @filter.two_step_status_option_select_options(aria_controls_id: filtered_users_id), } %> <%= render "components/option_select", { @@ -37,7 +38,7 @@ title: "Roles", options_container_id: "roles_filter", closed_on_load: @filter.no_options_selected_for?(:roles), - options: @filter.role_option_select_options, + options: @filter.role_option_select_options(aria_controls_id: filtered_users_id), } %> <% if current_user.manageable_organisations.many? %> @@ -47,7 +48,9 @@ options_container_id: "organisations_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:organisations), - options: UsersFilter.with_checked_at_top(@filter.organisation_option_select_options), + options: UsersFilter.with_checked_at_top( + @filter.organisation_option_select_options(aria_controls_id: filtered_users_id) + ), } %> <% end %> @@ -57,7 +60,9 @@ options_container_id: "permissions_filter", show_filter: true, closed_on_load: @filter.no_options_selected_for?(:permissions), - options: UsersFilter.with_checked_at_top(@filter.permission_option_select_options), + options: UsersFilter.with_checked_at_top( + @filter.permission_option_select_options(aria_controls_id: filtered_users_id) + ), } %>
    diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index fab0d9737..1cd697477 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -40,34 +40,38 @@
    +<% filtered_users_id = "filtered-users" %> +
    - <%= render "users_filter", filter: @filter %> + <%= render "users_filter", filter: @filter, filtered_users_id: %>
    - <%= render "govuk_publishing_components/components/table", { - caption: filtered_users_heading(@users), - head: [ - { text: "Name" }, - { text: "Email" }, - { text: "Role" }, - { text: "Organisation" }, - { text: "Last sign-in" }, - { text: "Status" }, - { text: "#{two_step_abbr_tag} Status".html_safe }, - ], - rows: @users.map do |user| - [ - { text: user_name(user) }, - { text: user.email }, - { text: user.role.humanize }, - { text: user.organisation.try(:name) }, - { text: user.current_sign_in_at ? "#{time_ago_in_words(user.current_sign_in_at)} ago" : "never signed in" }, - { text: user.status.humanize }, - { text: two_step_status(user) }, - ] - end, - } %> + <%= content_tag :div, id: filtered_users_id do %> + <%= render "govuk_publishing_components/components/table", { + caption: filtered_users_heading(@users), + head: [ + { text: "Name" }, + { text: "Email" }, + { text: "Role" }, + { text: "Organisation" }, + { text: "Last sign-in" }, + { text: "Status" }, + { text: "#{two_step_abbr_tag} Status".html_safe }, + ], + rows: @users.map do |user| + [ + { text: user_name(user) }, + { text: user.email }, + { text: user.role.humanize }, + { text: user.organisation.try(:name) }, + { text: user.current_sign_in_at ? "#{time_ago_in_words(user.current_sign_in_at)} ago" : "never signed in" }, + { text: user.status.humanize }, + { text: two_step_status(user) }, + ] + end, + } %> + <% end %> <%= paginate(@users, theme: "gds") %>
    From 6de8f5807f83f13f1e74f8f0ec215aca3cc5f50b Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:38 +0100 Subject: [PATCH 33/41] Remove unused context span in admin layout This was added in the commit [1] where the layout was originally added. It sounds as if it was all just copied from Content Publisher, so I don't think there's a strong reason to add it until we actually need it. [1]: 5173710f3f91fe35a26944b5ed3ca52a5cbeb938 --- app/views/layouts/admin_layout.html.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/layouts/admin_layout.html.erb b/app/views/layouts/admin_layout.html.erb index 10bb75669..4907569e1 100644 --- a/app/views/layouts/admin_layout.html.erb +++ b/app/views/layouts/admin_layout.html.erb @@ -37,7 +37,6 @@
    - <%= yield(:context) %>

    <% if yield(:page_heading).present? %> <%= yield(:page_heading) %> From c73b2b683b826798199c6e366caca2890628b49d Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:39 +0100 Subject: [PATCH 34/41] Move 3 buttons on users index page to top-right As per the proposed design [1]. I'm not particularly happy with the changes to the admin_layout. I think the real problem is that we've got too much of the page in the layout and we can probably come up with a better way to avoid duplication. However, I haven't attempted to address that here, because it would be a significant chunk of work and this PR is already big enough! Since the container isn't using a flex layout as suggested by the prototype, I had to use `float: right` to position the button group to the right of the page on wider screens. I'm not 100% sure whether I've used the correct naming conventions in the naming of the "users-index-button-group" CSS class. [1]: https://trello.com/c/bXsVUK6c --- app/assets/stylesheets/application.scss | 6 ++++ app/views/layouts/admin_layout.html.erb | 7 +++- app/views/users/index.html.erb | 46 ++++++++++++------------- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index b8deeb796..a49ffb159 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -77,3 +77,9 @@ .govuk-table__cell--line-through { text-decoration: line-through; } + +.users-index-button-group { + @include govuk-media-query($from: tablet) { + float: right; + } +} diff --git a/app/views/layouts/admin_layout.html.erb b/app/views/layouts/admin_layout.html.erb index 4907569e1..a994b1de5 100644 --- a/app/views/layouts/admin_layout.html.erb +++ b/app/views/layouts/admin_layout.html.erb @@ -36,7 +36,7 @@ <%= yield(:error_summary) %>
    -
    +

    <% if yield(:page_heading).present? %> <%= yield(:page_heading) %> @@ -45,6 +45,11 @@ <% end %>

    + <% if yield(:top_right) %> +
    + <%= yield(:top_right) %> +
    + <% end %>
    <%= yield %> diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 1cd697477..d619fab09 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -14,31 +14,29 @@ }) %> -
    -
    -
    - <% if policy(User).new? %> - <%= render "govuk_publishing_components/components/button", { - text: "Create user", - href: new_user_invitation_path, - margin_bottom: 4, - } %> - <%= render "govuk_publishing_components/components/button", { - text: "Upload a batch of users", - href: new_batch_invitation_path, - margin_bottom: 4, - } %> - <% end %> - <% if policy(BulkGrantPermissionSet).new? %> - <%= render "govuk_publishing_components/components/button", { - text: "Grant access to all users", - href: new_bulk_grant_permission_set_path, - margin_bottom: 4, - } %> - <% end %> -
    +<% content_for :top_right do %> +
    + <% if policy(User).new? %> + <%= render "govuk_publishing_components/components/button", { + text: "Create user", + href: new_user_invitation_path, + margin_bottom: 4, + } %> + <%= render "govuk_publishing_components/components/button", { + text: "Upload a batch of users", + href: new_batch_invitation_path, + margin_bottom: 4, + } %> + <% end %> + <% if policy(BulkGrantPermissionSet).new? %> + <%= render "govuk_publishing_components/components/button", { + text: "Grant access to all users", + href: new_bulk_grant_permission_set_path, + margin_bottom: 4, + } %> + <% end %>
    -
    +<% end %> <% filtered_users_id = "filtered-users" %> From 8fa8fafb5e1b5fb57d01bb1f87279b28e3aa2f18 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:40 +0100 Subject: [PATCH 35/41] Make some buttons secondary on users index page As per the proposed design [1]. [1]: https://trello.com/c/bXsVUK6c --- app/views/users/index.html.erb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index d619fab09..95ad52671 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -25,6 +25,7 @@ <%= render "govuk_publishing_components/components/button", { text: "Upload a batch of users", href: new_batch_invitation_path, + secondary: true, margin_bottom: 4, } %> <% end %> @@ -32,6 +33,7 @@ <%= render "govuk_publishing_components/components/button", { text: "Grant access to all users", href: new_bulk_grant_permission_set_path, + secondary: true, margin_bottom: 4, } %> <% end %> From 14d33ad4421b69df2dd613c5f0e76839d5ddf7fa Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 08:59:41 +0100 Subject: [PATCH 36/41] Use new table component on users index page This means the table is now much more "responsive" i.e. it looks a lot better on mobile devices. I've fixed the UsersControllerTest assertions against table cells by using regular expressions c.f. the equivalent commit for the API users index page [1]. [1]: f0a8f53f820aeb7542e217fc468038f29bdd8e03 --- app/views/users/index.html.erb | 3 +- test/controllers/users_controller_test.rb | 44 +++++++++++------------ 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 95ad52671..b8dcdb17e 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -48,8 +48,9 @@
    <%= content_tag :div, id: filtered_users_id do %> - <%= render "govuk_publishing_components/components/table", { + <%= render "components/table", { caption: filtered_users_heading(@users), + vertical_on_small_screen: true, head: [ { text: "Name" }, { text: "Email" }, diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 7ed92d45f..6b49010c1 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -296,10 +296,10 @@ def change_user_password(user_factory, new_password) get :index - assert_select "tr td:nth-child(3)", "Normal" - assert_select "tr td:nth-child(3)", "Organisation admin" - assert_select "tr td:nth-child(3)", "Super organisation admin" - assert_select "tr td:nth-child(3)", "Admin" + assert_select "tr td:nth-child(3)", /Normal/ + assert_select "tr td:nth-child(3)", /Organisation admin/ + assert_select "tr td:nth-child(3)", /Super organisation admin/ + assert_select "tr td:nth-child(3)", /Admin/ end should "show user organisation" do @@ -307,7 +307,7 @@ def change_user_password(user_factory, new_password) get :index - assert_select "tr td:nth-child(4)", user.organisation.name + assert_select "tr td:nth-child(4)", Regexp.new(user.organisation.name) end context "filter" do @@ -339,10 +339,10 @@ def change_user_password(user_factory, new_password) get :index, params: { statuses: %w[locked suspended] } - assert_select "tr td:nth-child(1)", text: "active-user", count: 0 - assert_select "tr td:nth-child(1)", text: "suspended-user" - assert_select "tr td:nth-child(1)", text: "invited-user", count: 0 - assert_select "tr td:nth-child(1)", text: "locked-user" + assert_select "tr td:nth-child(1)", text: /active-user/, count: 0 + assert_select "tr td:nth-child(1)", text: /suspended-user/ + assert_select "tr td:nth-child(1)", text: /invited-user/, count: 0 + assert_select "tr td:nth-child(1)", text: /locked-user/ end should "filter by 2SV statuses" do @@ -352,9 +352,9 @@ def change_user_password(user_factory, new_password) get :index, params: { two_step_statuses: %w[not_setup_2sv exempt_from_2sv] } - assert_select "tr td:nth-child(1)", text: "not-set-up-user" - assert_select "tr td:nth-child(1)", text: "exempted-user" - assert_select "tr td:nth-child(1)", text: "enabled-user", count: 0 + assert_select "tr td:nth-child(1)", text: /not-set-up-user/ + assert_select "tr td:nth-child(1)", text: /exempted-user/ + assert_select "tr td:nth-child(1)", text: /enabled-user/, count: 0 end should "filter by roles" do @@ -364,9 +364,9 @@ def change_user_password(user_factory, new_password) get :index, params: { roles: %w[admin normal] } - assert_select "tr td:nth-child(1)", text: "admin-user" - assert_select "tr td:nth-child(1)", text: "organisation-admin-user", count: 0 - assert_select "tr td:nth-child(1)", text: "normal-user" + assert_select "tr td:nth-child(1)", text: /admin-user/ + assert_select "tr td:nth-child(1)", text: /organisation-admin-user/, count: 0 + assert_select "tr td:nth-child(1)", text: /normal-user/ end should "filter by permissions" do @@ -381,9 +381,9 @@ def change_user_password(user_factory, new_password) get :index, params: { permissions: [app1.signin_permission, app2.signin_permission] } - assert_select "tr td:nth-child(1)", text: "user1" - assert_select "tr td:nth-child(1)", text: "user2", count: 0 - assert_select "tr td:nth-child(1)", text: "user3" + assert_select "tr td:nth-child(1)", text: /user1/ + assert_select "tr td:nth-child(1)", text: /user2/, count: 0 + assert_select "tr td:nth-child(1)", text: /user3/ end should "filter by organisations" do @@ -398,10 +398,10 @@ def change_user_password(user_factory, new_password) get :index, params: { organisations: [organisation1, organisation3] } - assert_select "tr td:nth-child(1)", text: "user1-in-organisation1" - assert_select "tr td:nth-child(1)", text: "user2-in-organisation1" - assert_select "tr td:nth-child(1)", text: "user3-in-organisation2", count: 0 - assert_select "tr td:nth-child(1)", text: "user4-in-organisation3" + assert_select "tr td:nth-child(1)", text: /user1-in-organisation1/ + assert_select "tr td:nth-child(1)", text: /user2-in-organisation1/ + assert_select "tr td:nth-child(1)", text: /user3-in-organisation2/, count: 0 + assert_select "tr td:nth-child(1)", text: /user4-in-organisation3/ end should "display link to clear all filters" do From a6452a93bf17853fb765ec7e4895d6ef68f70259 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 17:36:55 +0100 Subject: [PATCH 37/41] Remove "Organisation" & "Last sign-in" cols from users page As agreed in the collaboration session on Wed 06 Sep [1], we're removing these columns in order to reduce the chances of the table overflowing the right-hand side of the page in screens wider than the "tablet" breakpoint, i.e. when the table is still in "horizontal" mode. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821 --- app/views/users/index.html.erb | 4 ---- test/controllers/users_controller_test.rb | 8 -------- test/integration/admin_user_index_test.rb | 9 --------- 3 files changed, 21 deletions(-) diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index b8dcdb17e..c50858579 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -55,8 +55,6 @@ { text: "Name" }, { text: "Email" }, { text: "Role" }, - { text: "Organisation" }, - { text: "Last sign-in" }, { text: "Status" }, { text: "#{two_step_abbr_tag} Status".html_safe }, ], @@ -65,8 +63,6 @@ { text: user_name(user) }, { text: user.email }, { text: user.role.humanize }, - { text: user.organisation.try(:name) }, - { text: user.current_sign_in_at ? "#{time_ago_in_words(user.current_sign_in_at)} ago" : "never signed in" }, { text: user.status.humanize }, { text: two_step_status(user) }, ] diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 6b49010c1..07b482526 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -302,14 +302,6 @@ def change_user_password(user_factory, new_password) assert_select "tr td:nth-child(3)", /Admin/ end - should "show user organisation" do - user = create(:user_in_organisation) - - get :index - - assert_select "tr td:nth-child(4)", Regexp.new(user.organisation.name) - end - context "filter" do should "filter by partially matching name" do create(:user, name: "does-match1") diff --git a/test/integration/admin_user_index_test.rb b/test/integration/admin_user_index_test.rb index 16904200d..c0e386401 100644 --- a/test/integration/admin_user_index_test.rb +++ b/test/integration/admin_user_index_test.rb @@ -35,14 +35,5 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest assert has_css?("td", text: "Not set up", count: 7) end end - - should "see when the user last logged in" do - visit "/users" - - assert page.has_content?("Last sign-in") - - actual_last_sign_in_strings = page.all("table tr td:nth-child(5)").map(&:text).map(&:strip)[0..1] - assert_equal ["5 minutes ago", "never signed in"], actual_last_sign_in_strings - end end end From a4a7062e7719a9338ff367e361aa7ccab369c42a Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 17:41:13 +0100 Subject: [PATCH 38/41] Increase max-width of govuk-width-container for all pages As agreed in the collaboration session on Wed 06 Sep [1], we're increasing the value of `$govuk-page-width` from the default of 960px to 1140px which is the value used in Whitehall [2]. We're making this change in order to give more room to the table on the new version of the users index page which now has a filter sidebar occupying valuable screen wdith. This will reduce the chances of the table overflowing the right-hand side of the page in screens wider than the "tablet" breakpoint, i.e. when the table is still in "horizontal" mode. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821 [2]: https://github.com/alphagov/whitehall/blob/b355ef2caf4b414487d3687495d133b66f7f7846/app/assets/stylesheets/application.scss#L1 --- app/assets/stylesheets/application.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index a49ffb159..a3d8c3f3d 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -1,3 +1,5 @@ +$govuk-page-width: 1140px; + @import "govuk_publishing_components/all_components"; @import "user_research_recruitment_banner"; From b62d5afd110dd060e80ec1c3e2dffb30180ec157 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 17:55:29 +0100 Subject: [PATCH 39/41] Use custom breakpoint for table on users page As agreed in the collaboration session on Wed 06 Sep [1], we're increasing the value of this breakpoint from the standard "tablet" breakpoint of 641px to a custom breakpoint of 940px. We chose this value so that the table changes into "vertical" mode before the table content overflows the right-hand side of the container and before a scrollbar appears at the bottom of the page. This change also affects the API users index page, but seems to work OK there too. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821 --- app/assets/stylesheets/components/_table.scss | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/components/_table.scss b/app/assets/stylesheets/components/_table.scss index 3390b5345..b44c60574 100644 --- a/app/assets/stylesheets/components/_table.scss +++ b/app/assets/stylesheets/components/_table.scss @@ -6,6 +6,7 @@ $table-border-colour: govuk-colour("mid-grey", $legacy: "grey-2"); $table-header-border-width: 2px; $table-header-background-colour: govuk-colour("light-grey", $legacy: "grey-3"); $vertical-row-bottom-border-width: 3px; +$vertical-on-smallscreen-breakpoint: 940px; $sort-link-active-colour: govuk-colour("white"); $sort-link-arrow-size: 14px; $sort-link-arrow-size-small: 8px; @@ -157,7 +158,7 @@ $table-row-even-background-colour: govuk-colour("light-grey", $legacy: "grey-4") text-align: right; } - @include govuk-media-query($until: tablet) { + @include govuk-media-query($until: $vertical-on-smallscreen-breakpoint) { .govuk-table__cell { padding-right: 0; } @@ -178,7 +179,7 @@ $table-row-even-background-colour: govuk-colour("light-grey", $legacy: "grey-4") word-break: initial; } - @include govuk-media-query($from: tablet) { + @include govuk-media-query($from: $vertical-on-smallscreen-breakpoint) { .govuk-table__head { clip: auto; -webkit-clip-path: none; From e22212cffe68cafd5b5ddd328dd46d9be5d9b3ef Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 6 Sep 2023 18:55:04 +0100 Subject: [PATCH 40/41] Group tests in UserTest more logically Ideally we'd re-write the git history, but I'm out of time! --- test/models/user_test.rb | 136 +++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 68 deletions(-) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index eb2274990..710a53a75 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -658,6 +658,74 @@ def setup end end + context "#role_class" do + should "return the role class" do + assert_equal Roles::Normal, build(:user).role_class + assert_equal Roles::OrganisationAdmin, build(:organisation_admin_user).role_class + assert_equal Roles::SuperOrganisationAdmin, build(:super_organisation_admin_user).role_class + assert_equal Roles::Admin, build(:admin_user).role_class + assert_equal Roles::Superadmin, build(:superadmin_user).role_class + end + end + + context "#manageable_roles" do + should "return names of roles that the user is allowed to manage" do + assert_equal %w[], build(:user).manageable_roles + assert_equal %w[normal organisation_admin], build(:organisation_admin_user).manageable_roles + assert_equal %w[normal organisation_admin super_organisation_admin], build(:super_organisation_admin_user).manageable_roles + assert_equal %w[normal organisation_admin super_organisation_admin admin], build(:admin_user).manageable_roles + assert_equal User.roles, build(:superadmin_user).manageable_roles + end + end + + context "#can_manage?" do + should "indicate whether user is allowed to manage another user" do + assert_not build(:user).can_manage?(build(:user)) + assert_not build(:user).can_manage?(build(:organisation_admin_user)) + assert_not build(:user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:user).can_manage?(build(:admin_user)) + assert_not build(:user).can_manage?(build(:superadmin_user)) + + assert build(:organisation_admin_user).can_manage?(build(:user)) + assert build(:organisation_admin_user).can_manage?(build(:organisation_admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:admin_user)) + assert_not build(:organisation_admin_user).can_manage?(build(:superadmin_user)) + + assert build(:super_organisation_admin_user).can_manage?(build(:user)) + assert build(:super_organisation_admin_user).can_manage?(build(:organisation_admin_user)) + assert build(:super_organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) + assert_not build(:super_organisation_admin_user).can_manage?(build(:admin_user)) + assert_not build(:super_organisation_admin_user).can_manage?(build(:superadmin_user)) + + assert build(:admin_user).can_manage?(build(:user)) + assert build(:admin_user).can_manage?(build(:organisation_admin_user)) + assert build(:admin_user).can_manage?(build(:super_organisation_admin_user)) + assert build(:admin_user).can_manage?(build(:admin_user)) + assert_not build(:admin_user).can_manage?(build(:superadmin_user)) + + assert build(:superadmin_user).can_manage?(build(:user)) + assert build(:superadmin_user).can_manage?(build(:organisation_admin_user)) + assert build(:superadmin_user).can_manage?(build(:super_organisation_admin_user)) + assert build(:superadmin_user).can_manage?(build(:admin_user)) + assert build(:superadmin_user).can_manage?(build(:superadmin_user)) + end + end + + context "#manageable_organisations" do + should "return relation for organisations that the user is allowed to manage" do + organisation = create(:organisation, name: "Org1") + child_organisation = create(:organisation, parent: organisation, name: "Org2") + create(:organisation, name: "Org3") + + assert_equal [], create(:user, organisation:).manageable_organisations + assert_equal [organisation], create(:organisation_admin_user, organisation:).manageable_organisations + assert_equal [organisation, child_organisation], create(:super_organisation_admin_user, organisation:).manageable_organisations + assert_equal Organisation.order(:name), create(:admin_user, organisation:).manageable_organisations + assert_equal Organisation.order(:name), create(:superadmin_user, organisation:).manageable_organisations + end + end + context "authorised applications" do setup do @user = create(:user) @@ -855,60 +923,6 @@ def setup end end - context "#role_class" do - should "return the role class" do - assert_equal Roles::Normal, build(:user).role_class - assert_equal Roles::OrganisationAdmin, build(:organisation_admin_user).role_class - assert_equal Roles::SuperOrganisationAdmin, build(:super_organisation_admin_user).role_class - assert_equal Roles::Admin, build(:admin_user).role_class - assert_equal Roles::Superadmin, build(:superadmin_user).role_class - end - end - - context "#manageable_roles" do - should "return names of roles that the user is allowed to manage" do - assert_equal %w[], build(:user).manageable_roles - assert_equal %w[normal organisation_admin], build(:organisation_admin_user).manageable_roles - assert_equal %w[normal organisation_admin super_organisation_admin], build(:super_organisation_admin_user).manageable_roles - assert_equal %w[normal organisation_admin super_organisation_admin admin], build(:admin_user).manageable_roles - assert_equal User.roles, build(:superadmin_user).manageable_roles - end - end - - context "#can_manage?" do - should "indicate whether user is allowed to manage another user" do - assert_not build(:user).can_manage?(build(:user)) - assert_not build(:user).can_manage?(build(:organisation_admin_user)) - assert_not build(:user).can_manage?(build(:super_organisation_admin_user)) - assert_not build(:user).can_manage?(build(:admin_user)) - assert_not build(:user).can_manage?(build(:superadmin_user)) - - assert build(:organisation_admin_user).can_manage?(build(:user)) - assert build(:organisation_admin_user).can_manage?(build(:organisation_admin_user)) - assert_not build(:organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) - assert_not build(:organisation_admin_user).can_manage?(build(:admin_user)) - assert_not build(:organisation_admin_user).can_manage?(build(:superadmin_user)) - - assert build(:super_organisation_admin_user).can_manage?(build(:user)) - assert build(:super_organisation_admin_user).can_manage?(build(:organisation_admin_user)) - assert build(:super_organisation_admin_user).can_manage?(build(:super_organisation_admin_user)) - assert_not build(:super_organisation_admin_user).can_manage?(build(:admin_user)) - assert_not build(:super_organisation_admin_user).can_manage?(build(:superadmin_user)) - - assert build(:admin_user).can_manage?(build(:user)) - assert build(:admin_user).can_manage?(build(:organisation_admin_user)) - assert build(:admin_user).can_manage?(build(:super_organisation_admin_user)) - assert build(:admin_user).can_manage?(build(:admin_user)) - assert_not build(:admin_user).can_manage?(build(:superadmin_user)) - - assert build(:superadmin_user).can_manage?(build(:user)) - assert build(:superadmin_user).can_manage?(build(:organisation_admin_user)) - assert build(:superadmin_user).can_manage?(build(:super_organisation_admin_user)) - assert build(:superadmin_user).can_manage?(build(:admin_user)) - assert build(:superadmin_user).can_manage?(build(:superadmin_user)) - end - end - context ".with_statuses" do should "only return suspended or invited users" do suspended_user = create(:suspended_user) @@ -991,20 +1005,6 @@ def setup end end - context "#manageable_organisations" do - should "return relation for organisations that the user is allowed to manage" do - organisation = create(:organisation, name: "Org1") - child_organisation = create(:organisation, parent: organisation, name: "Org2") - create(:organisation, name: "Org3") - - assert_equal [], create(:user, organisation:).manageable_organisations - assert_equal [organisation], create(:organisation_admin_user, organisation:).manageable_organisations - assert_equal [organisation, child_organisation], create(:super_organisation_admin_user, organisation:).manageable_organisations - assert_equal Organisation.order(:name), create(:admin_user, organisation:).manageable_organisations - assert_equal Organisation.order(:name), create(:superadmin_user, organisation:).manageable_organisations - end - end - context ".with_permission" do should "only return users with specified permission(s)" do app1 = create(:application) From 201e2b76b1e518da12a0143993c7761a2f7b1c76 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 12 Sep 2023 12:00:47 +0100 Subject: [PATCH 41/41] Add JS-enabled integration tests for /users page The equivalent non-JS functionality is already being tested in controller and unit tests. These additional happy path tests should give us confidence that the JavaScript functionality continues to work as expected. --- test/integration/admin_user_index_test.rb | 94 ++++++++++++++++++++--- 1 file changed, 84 insertions(+), 10 deletions(-) diff --git a/test/integration/admin_user_index_test.rb b/test/integration/admin_user_index_test.rb index c0e386401..aab86e893 100644 --- a/test/integration/admin_user_index_test.rb +++ b/test/integration/admin_user_index_test.rb @@ -3,6 +3,8 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest context "logged in as an admin" do setup do + use_javascript_driver + current_time = Time.zone.now Timecop.freeze(current_time) @@ -13,14 +15,21 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest org1 = create(:organisation, name: "Org 1") org2 = create(:organisation, name: "Org 2") - create(:user, name: "Aardvark", email: "aardvark@example.com", current_sign_in_at: current_time - 5.minutes) - create(:two_step_enabled_user, name: "Abbey", email: "abbey@example.com") - create(:user, name: "Abbot", email: "mr_ab@example.com") - create(:user, name: "Bert", email: "bbbert@example.com") - create(:user, name: "Ed", email: "ed@example.com", organisation: org1) - create(:user, name: "Eddie", email: "eddie_bb@example.com") - create(:two_step_exempted_user, name: "Ernie", email: "ernie@example.com", organisation: org2) - create(:suspended_user, name: "Suspended McFee", email: "suspenders@example.com") + @aardvark = create(:user, name: "Aardvark", email: "aardvark@example.com", current_sign_in_at: current_time - 5.minutes) + @abbey = create(:two_step_enabled_user, name: "Abbey", email: "abbey@example.com") + @abbot = create(:user, name: "Abbot", email: "mr_ab@example.com") + @bert = create(:user, name: "Bert", email: "bbbert@example.com") + @ed = create(:user, name: "Ed", email: "ed@example.com", organisation: org1) + @eddie = create(:user, name: "Eddie", email: "eddie_bb@example.com") + @ernie = create(:two_step_exempted_user, name: "Ernie", email: "ernie@example.com", organisation: org2) + @suspended_mcfee = create(:suspended_user, name: "Suspended McFee", email: "suspenders@example.com") + + application = create(:application, name: "App name") + create(:supported_permission, application:, name: "App permission") + @aardvark.grant_application_signin_permission(application) + @bert.grant_application_permission(application, "App permission") + + visit "/users" end teardown do @@ -28,12 +37,77 @@ class AdminUserIndexTest < ActionDispatch::IntegrationTest end should "display the 2SV enrollment status for users" do - visit "/users" - within "table" do assert has_css?("td", text: "Enabled", count: 1) assert has_css?("td", text: "Not set up", count: 7) end end + + should "list all users" do + assert_results @admin, @aardvark, @abbey, @abbot, @bert, @ed, @eddie, @ernie, @suspended_mcfee + end + + should "filter users by search string" do + fill_in "Search", with: "bb" + click_on "Search" + + assert_results @abbey, @abbot, @bert, @eddie + end + + should "filter users by status" do + check "Suspended", allow_label_click: true + + assert_results @suspended_mcfee + end + + should "filter users by 2sv status" do + click_on "2SV Status" + + check "Exempted", allow_label_click: true + assert_results @ernie + + check "Enable", allow_label_click: true + assert_results @ernie, @abbey + end + + should "filter users by role" do + click_on "Role" + + check "Admin", allow_label_click: true + assert_results @admin + end + + should "filter users by organisation" do + click_on "Organisation" + + check "Org 1", allow_label_click: true + assert_results @ed + + check "Org 2", allow_label_click: true + assert_results @ed, @ernie + end + + should "filter users by permission" do + click_on "Permissions" + + check "App name signin", allow_label_click: true + assert_results @aardvark + + check "App name App permission", allow_label_click: true + assert_results @aardvark, @bert + end + end + +private + + def assert_results(*users) + expected_table_caption = [users.count, "user".pluralize(users.count)].join(" ") + + table = find("table caption", text: expected_table_caption).ancestor(:table) + assert table.has_css?("tbody tr", count: users.count) + + users.each do |user| + assert table.has_content?(user.name) + end end end