From 04760076a531e4c1963058338aa4b5d5fd23e016 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Tue, 24 May 2022 22:57:01 +0100 Subject: [PATCH] User search fixes (#607) * Update query message * Add clear search link * Set input value * Use gem component * Move to list partial pattern * Partial path * Update spec * Rubocop * Unit test filter module * Rubocop * Add search result to page title if searched * Add missing horizontal rule * Use form_group attributes for search input Co-authored-by: Paul Robert Lloyd --- app/components/search_component.html.erb | 23 +++--- app/components/search_component.rb | 7 +- app/controllers/modules/users_filter.rb | 7 +- app/controllers/organisations_controller.rb | 4 +- app/controllers/users_controller.rb | 4 +- app/views/users/_user_list.html.erb | 53 ++++++++++++++ app/views/users/index.html.erb | 70 ++++--------------- spec/components/search_component_spec.rb | 15 +++- spec/controllers/modules/users_filter_spec.rb | 31 ++++++++ spec/requests/users_controller_spec.rb | 16 ++--- 10 files changed, 141 insertions(+), 89 deletions(-) create mode 100644 app/views/users/_user_list.html.erb create mode 100644 spec/controllers/modules/users_filter_spec.rb diff --git a/app/components/search_component.html.erb b/app/components/search_component.html.erb index cf5da1898..c63039b8c 100644 --- a/app/components/search_component.html.erb +++ b/app/components/search_component.html.erb @@ -1,16 +1,15 @@ <%= form_with model: @user, url: path(current_user), method: "get", local: true do |f| %> -
-
- +
+ <%= f.govuk_text_field :search, + form_group: { + class: "app-search__form-group", + }, + label: { text: search_label }, + type: "search", + value:, + autocomplete: "off", + class: "app-search__input" %> - + <%= f.govuk_submit "Search", classes: "app-search__button" %>
- - - -
<% end %> diff --git a/app/components/search_component.rb b/app/components/search_component.rb index dbab17245..aacc902e1 100644 --- a/app/components/search_component.rb +++ b/app/components/search_component.rb @@ -1,9 +1,10 @@ class SearchComponent < ViewComponent::Base - attr_reader :current_user, :label + attr_reader :current_user, :search_label, :value - def initialize(current_user:, label:) + def initialize(current_user:, search_label:, value: nil) @current_user = current_user - @label = label + @search_label = search_label + @value = value super end diff --git a/app/controllers/modules/users_filter.rb b/app/controllers/modules/users_filter.rb index 1b6ba4c53..1426f24fc 100644 --- a/app/controllers/modules/users_filter.rb +++ b/app/controllers/modules/users_filter.rb @@ -1,8 +1,7 @@ module Modules::UsersFilter - def filtered_users(base_collection) - search_param = params["search-field"] - if search_param.present? - base_collection.search_by(search_param) + def filtered_users(base_collection, search_term = nil) + if search_term.present? + base_collection.search_by(search_term) else base_collection end.filter_by_active.includes(:organisation) diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 17d3b8a3f..4781da5e6 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -18,8 +18,8 @@ class OrganisationsController < ApplicationController end def users - @pagy, @users = pagy(filtered_users(@organisation.users)) - @searched = params["search-field"].present? + @pagy, @users = pagy(filtered_users(@organisation.users, params["search"])) + @searched = params["search"].presence render "users/index" end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8e58f7396..7b2873461 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -10,8 +10,8 @@ class UsersController < ApplicationController def index redirect_to users_organisation_path(current_user.organisation) unless current_user.support? - @pagy, @users = pagy(filtered_users(User.all)) - @searched = params["search-field"].present? + @pagy, @users = pagy(filtered_users(User.all, params["search"])) + @searched = params["search"].presence respond_to do |format| format.html diff --git a/app/views/users/_user_list.html.erb b/app/views/users/_user_list.html.erb new file mode 100644 index 000000000..6c0cc988a --- /dev/null +++ b/app/views/users/_user_list.html.erb @@ -0,0 +1,53 @@ +<%= govuk_table do |table| %> + <%= table.caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do |caption| %> + + <% if searched.present? %> + <%= pagy.count %> <%= item_label %> found matching ‘<%= searched %>’ of <%= total_user_count %> total users. <%= govuk_link_to("Clear search", request.path) %> + <% else %> + <%= pagy.count %> total users. + <% end %> + + <% if current_user.support? %> + <%= govuk_link_to "Download (CSV)", "/users.csv", type: "text/csv" %> + <% end %> + <% end %> + <%= table.head do |head| %> + <%= head.row do |row| %> + <% row.cell(header: true, text: "Name and email adress", html_attributes: { + scope: "col", + }) %> + <% row.cell(header: true, text: "Organisation and role", html_attributes: { + scope: "col", + }) %> + <% row.cell(header: true, text: "Last logged in", html_attributes: { + scope: "col", + }) %> + <% end %> + <% end %> + <% users.each do |user| %> + <%= table.body do |body| %> + <%= body.row do |row| %> + <% row.cell(header: true, html_attributes: { + scope: "row", + }) do %> + <%= simple_format(user_cell(user), {}, wrapper_tag: "span") %> + <% if user.is_data_protection_officer? || user.is_key_contact? %> +
+ <% end %> + <%= user.is_data_protection_officer? ? govuk_tag( + classes: "app-tag--small", + colour: "turquoise", + text: "Data protection officer", + ) : "" %> + <%= user.is_key_contact? ? govuk_tag( + classes: "app-tag--small", + colour: "turquoise", + text: "Key contact", + ) : "" %> + <% end %> + <% row.cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %> + <% row.cell(text: user.last_sign_in_at&.to_formatted_s(:govuk_date)) %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index e9bd62065..ceeb563b9 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -1,4 +1,14 @@ -<% content_for :title, "Your organisation (Users)" %> +<% if @searched.present? %> + <% item_label = @pagy.count > 1 ? "users" : "user" %> + <% total_user_count = User.all.count %> + <% title = "Your organisation (#{@pagy.count} #{item_label} matching ‘#{@searched}’ of #{total_user_count} total users)" %> +<% else %> + <% item_label = "" %> + <% total_user_count = nil %> + <% title = "Your organisation (Users)" %> +<% end %> + +<% content_for :title, title %> <% content_for :tab_title do %> <%= "Users" %> @@ -8,59 +18,9 @@ <%= govuk_button_link_to "Invite user", new_user_path, html: { method: :get } %> <% end %> -<%= render SearchComponent.new(current_user:, label: "Search by name or email address") %> +<%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %> -<%= govuk_table do |table| %> - <%= table.caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do |caption| %> - - <% if @searched %> - Matches <%= @pagy.count %> of <%= User.count %> total users. - <% else %> - <%= @pagy.count %> total users. - <% end %> - - <% if current_user.support? %> - <%= govuk_link_to "Download (CSV)", "/users.csv", type: "text/csv" %> - <% end %> - <% end %> - <%= table.head do |head| %> - <%= head.row do |row| %> - <% row.cell(header: true, text: "Name and email adress", html_attributes: { - scope: "col", - }) %> - <% row.cell(header: true, text: "Organisation and role", html_attributes: { - scope: "col", - }) %> - <% row.cell(header: true, text: "Last logged in", html_attributes: { - scope: "col", - }) %> - <% end %> - <% end %> - <% @users.each do |user| %> - <%= table.body do |body| %> - <%= body.row do |row| %> - <% row.cell(header: true, html_attributes: { - scope: "row", - }) do %> - <%= simple_format(user_cell(user), {}, wrapper_tag: "span") %> - <% if user.is_data_protection_officer? || user.is_key_contact? %> -
- <% end %> - <%= user.is_data_protection_officer? ? govuk_tag( - classes: "app-tag--small", - colour: "turquoise", - text: "Data protection officer", - ) : "" %> - <%= user.is_key_contact? ? govuk_tag( - classes: "app-tag--small", - colour: "turquoise", - text: "Key contact", - ) : "" %> - <% end %> - <% row.cell(text: simple_format(org_cell(user), {}, wrapper_tag: "div")) %> - <% row.cell(text: user.last_sign_in_at&.to_formatted_s(:govuk_date)) %> - <% end %> - <% end %> - <% end %> -<% end %> +
+ +<%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_user_count: } %> <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %> diff --git a/spec/components/search_component_spec.rb b/spec/components/search_component_spec.rb index d79ad53ca..fb68267eb 100644 --- a/spec/components/search_component_spec.rb +++ b/spec/components/search_component_spec.rb @@ -2,11 +2,12 @@ require "rails_helper" RSpec.describe SearchComponent, type: :component do let(:current_user) { FactoryBot.create(:user, :support) } - let(:label) { "Search by name or email address" } + let(:search_label) { "Search by name or email address" } let(:page) { Capybara::Node::Simple.new(rendered_component) } + let(:value) { nil } before do - render_inline(described_class.new(current_user:, label:)) + render_inline(described_class.new(current_user:, search_label:, value:)) end it "renders a search bar" do @@ -14,6 +15,14 @@ RSpec.describe SearchComponent, type: :component do end it "renders the given label" do - expect(page).to have_content(label) + expect(page).to have_content(search_label) + end + + context "when a search term has been entered" do + let(:value) { "search term" } + + it "shows the search term in the input field" do + expect(page).to have_field("search-field", type: "search", with: value) + end end end diff --git a/spec/controllers/modules/users_filter_spec.rb b/spec/controllers/modules/users_filter_spec.rb new file mode 100644 index 000000000..154f1ccb0 --- /dev/null +++ b/spec/controllers/modules/users_filter_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" + +RSpec.describe Modules::UsersFilter do + describe "filtered_users" do + subject(:instance) { Class.new.include(described_class).new } + + before do + FactoryBot.create_list(:user, 5) + FactoryBot.create(:user, name: "Joe Blogg") + FactoryBot.create(:user, name: "Tom Blogg", active: false) + end + + let(:user_list) { User.all } + + context "when given a search term" do + let(:search_term) { "Blogg" } + + it "filters the collection on search term and active users" do + expect(instance.filtered_users(user_list, search_term).count).to eq(1) + end + end + + context "when not given a search term" do + let(:search_term) { nil } + + it "filters the collection on active users" do + expect(instance.filtered_users(user_list, search_term).count).to eq(6) + end + end + end +end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 10821961c..54c4e3526 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -359,7 +359,7 @@ RSpec.describe UsersController, type: :request do it "shows a search bar" do follow_redirect! - expect(page).to have_field("search-field", type: "search") + expect(page).to have_field("search", type: "search") end end @@ -369,7 +369,7 @@ RSpec.describe UsersController, type: :request do let!(:other_org_user) { FactoryBot.create(:user, name: "User 4", email: "joe@other_example.com") } before do - get "/organisations/#{user.organisation.id}/users?search-field=#{search_param}" + get "/organisations/#{user.organisation.id}/users?search=#{search_param}" end context "when our search string matches case" do @@ -381,7 +381,7 @@ RSpec.describe UsersController, type: :request do end it "updates the table caption" do - expect(page).to have_content("Matches 1 of 5 total users.") + expect(page).to have_content("1 user found matching ‘filter’ of 5 total users.") end end @@ -417,7 +417,7 @@ RSpec.describe UsersController, type: :request do end it "updates the table caption" do - expect(page).to have_content("Matches 2 of 5 total users.") + expect(page).to have_content("2 users found matching ‘joe’ of 5 total users.") end end end @@ -822,12 +822,12 @@ RSpec.describe UsersController, type: :request do end it "shows a search bar" do - expect(page).to have_field("search-field", type: "search") + expect(page).to have_field("search", type: "search") end context "when a search parameter is passed" do before do - get "/users?search-field=#{search_param}" + get "/users?search=#{search_param}" end context "when our search term matches a name" do @@ -842,7 +842,7 @@ RSpec.describe UsersController, type: :request do end it "updates the table caption" do - expect(page).to have_content("Matches 1 of 4 total users.") + expect(page).to have_content("1 user found matching ‘Danny’ of 4 total users.") end end @@ -881,7 +881,7 @@ RSpec.describe UsersController, type: :request do end it "updates the table caption" do - expect(page).to have_content("Matches 2 of 4 total users.") + expect(page).to have_content("2 users found matching ‘joe’ of 4 total users.") end end end