From 12a73fee5e1a812553682930390f54a0049be50f Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 26 Jul 2023 09:12:42 +0100 Subject: [PATCH] CLDC-2245 Add user status filter (#1797) * Add user status filters * Add filters to the pages * only show unconfirmed and deactivated logs as deactivated * Look at last signed in at to determine unconfirmed users --- app/controllers/organisations_controller.rb | 7 +- app/controllers/users_controller.rb | 13 ++- app/helpers/filters_helper.rb | 8 ++ app/models/user.rb | 21 ++++ app/services/filter_manager.rb | 29 +++++- app/views/organisations/users.html.erb | 13 ++- app/views/users/_user_filters.html.erb | 29 ++++++ app/views/users/index.html.erb | 14 ++- spec/features/user_spec.rb | 33 +++++++ spec/models/user_spec.rb | 46 ++++++++- spec/requests/users_controller_spec.rb | 102 ++++++++++++++++++++ 11 files changed, 296 insertions(+), 19 deletions(-) create mode 100644 app/views/users/_user_filters.html.erb diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 0b1f1995e..8ca6d5bb8 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -6,7 +6,9 @@ class OrganisationsController < ApplicationController before_action :find_resource, except: %i[index new create] before_action :authenticate_scope!, except: [:index] before_action :session_filters, if: -> { current_user.support? || current_user.organisation.has_managing_agents? }, only: %i[lettings_logs sales_logs email_lettings_csv download_lettings_csv email_sales_csv download_sales_csv] + before_action :session_filters, only: %i[users] before_action -> { filter_manager.serialize_filters_to_session }, if: -> { current_user.support? || current_user.organisation.has_managing_agents? }, only: %i[lettings_logs sales_logs email_lettings_csv download_lettings_csv email_sales_csv download_sales_csv] + before_action -> { filter_manager.serialize_filters_to_session }, only: %i[users] def index redirect_to organisation_path(current_user.organisation) unless current_user.support? @@ -31,13 +33,14 @@ class OrganisationsController < ApplicationController def users organisation_users = @organisation.users.sorted_by_organisation_and_role - unpaginated_filtered_users = filtered_collection(organisation_users, search_term) + unpaginated_filtered_users = filter_manager.filtered_users(organisation_users, search_term, session_filters) respond_to do |format| format.html do @pagy, @users = pagy(unpaginated_filtered_users) @searched = search_term.presence @total_count = @organisation.users.size + @filter_type = "users" if current_user.support? render "users", layout: "application" @@ -204,6 +207,8 @@ private "lettings_logs" elsif params[:action].include?("sales") "sales_logs" + elsif params[:action].include?("users") + "users" end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 31df38da7..5f619ad36 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -7,15 +7,18 @@ class UsersController < ApplicationController before_action :authenticate_user! before_action :find_resource, except: %i[new create] before_action :authenticate_scope!, except: %i[new] + before_action :session_filters, if: :current_user, only: %i[index] + before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user, only: %i[index] def index redirect_to users_organisation_path(current_user.organisation) unless current_user.support? all_users = User.sorted_by_organisation_and_role - filtered_users = filtered_users(all_users, search_term) + filtered_users = filter_manager.filtered_users(all_users, search_term, session_filters) @pagy, @users = pagy(filtered_users) @searched = search_term.presence @total_count = all_users.size + @filter_type = "users" respond_to do |format| format.html @@ -194,4 +197,12 @@ private current_user.data_coordinator? || current_user.support? || current_user == @user end end + + def filter_manager + FilterManager.new(current_user:, session:, params:, filter_type: "users") + end + + def session_filters + filter_manager.session_filters + end end diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index f3d03cc01..7ac1277bd 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -38,6 +38,14 @@ module FiltersHelper }.freeze end + def user_status_filters + { + "active" => "Active", + "deactivated" => "Deactivated", + "unconfirmed" => "Unconfirmed", + }.freeze + end + def selected_option(filter, filter_type) return false unless session[session_name_for(filter_type)] diff --git a/app/models/user.rb b/app/models/user.rb index 4e762c9a1..2ca5a3db6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -56,6 +56,27 @@ class User < ApplicationRecord scope :filter_by_active, -> { where(active: true) } scope :search_by, ->(param) { search_by_name(param).or(search_by_email(param)) } scope :sorted_by_organisation_and_role, -> { joins(:organisation).order("organisations.name", role: :desc, name: :asc) } + scope :filter_by_status, lambda { |statuses, _user = nil| + filtered_records = all + scopes = [] + + statuses.each do |status| + status = status == "active" ? "active_status" : status + status = status == "unconfirmed" ? "not_signed_in" : status + if respond_to?(status, true) + scopes << send(status) + end + end + + if scopes.any? + filtered_records = filtered_records.merge(scopes.reduce(&:or)) + end + + filtered_records + } + scope :not_signed_in, -> { where(last_sign_in_at: nil, active: true) } + scope :deactivated, -> { where(active: false) } + scope :active_status, -> { where(active: true).where.not(last_sign_in_at: nil) } def lettings_logs if support? diff --git a/app/services/filter_manager.rb b/app/services/filter_manager.rb index 20b6a09d1..9de3fcdbb 100644 --- a/app/services/filter_manager.rb +++ b/app/services/filter_manager.rb @@ -39,6 +39,17 @@ class FilterManager end end + def self.filter_users(users, search_term, filters, user) + users = filter_by_search(users, search_term) + + filters.each do |category, values| + next if Array(values).reject(&:empty?).blank? + + users = users.public_send("filter_by_#{category}", values, user) + end + users + end + def serialize_filters_to_session(specific_org: false) session[session_name_for(filter_type)] = session_filters(specific_org:).to_json end @@ -54,13 +65,17 @@ class FilterManager current_user.logs_filters(specific_org:).each do |filter| new_filters[filter] = params[filter] if params[filter].present? end - end - new_filters = new_filters.except("owning_organisation") if params["owning_organisation_select"] == "all" - new_filters = new_filters.except("managing_organisation") if params["managing_organisation_select"] == "all" + new_filters = new_filters.except("owning_organisation") if params["owning_organisation_select"] == "all" + new_filters = new_filters.except("managing_organisation") if params["managing_organisation_select"] == "all" + + new_filters = new_filters.except("user") if params["assigned_to"] == "all" + new_filters["user"] = current_user.id.to_s if params["assigned_to"] == "you" + end - new_filters = new_filters.except("user") if params["assigned_to"] == "all" - new_filters["user"] = current_user.id.to_s if params["assigned_to"] == "you" + if @filter_type.include?("users") && params["status"].present? + new_filters["status"] = params["status"] + end new_filters end @@ -71,6 +86,10 @@ class FilterManager FilterManager.filter_logs(logs, search_term, filters, all_orgs, current_user) end + def filtered_users(users, search_term, filters) + FilterManager.filter_users(users, search_term, filters, current_user) + end + def bulk_upload id = (logs_filters["bulk_upload_id"] || []).reject(&:blank?)[0] @bulk_upload ||= current_user.bulk_uploads.find_by(id:) diff --git a/app/views/organisations/users.html.erb b/app/views/organisations/users.html.erb index c604a1c9f..f1a6336ce 100644 --- a/app/views/organisations/users.html.erb +++ b/app/views/organisations/users.html.erb @@ -15,10 +15,15 @@ <% if current_user.data_coordinator? || current_user.support? %> <%= govuk_button_link_to "Invite user", new_user_path(organisation_id: @organisation.id), html: { method: :get } %> <% end %> +
+ <%= render partial: "users/user_filters" %> +
-<%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %> + <%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %> -<%= govuk_section_break(visible: true, size: "m") %> + <%= govuk_section_break(visible: true, size: "m") %> -<%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> -<%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %> + <%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %> +
+
diff --git a/app/views/users/_user_filters.html.erb b/app/views/users/_user_filters.html.erb new file mode 100644 index 000000000..41280b3ae --- /dev/null +++ b/app/views/users/_user_filters.html.erb @@ -0,0 +1,29 @@ +
+
+
+

Filters

+
+ +
+ <%= form_with url: users_path, html: { method: :get } do |f| %> +
+

+ <%= filters_applied_text(@filter_type) %> +

+

+ <%= reset_filters_link(@filter_type) %> +

+
+ + <%= render partial: "filters/checkbox_filter", + locals: { + f:, + options: user_status_filters, + label: "Status", + category: "status", + } %> + <%= f.govuk_submit "Apply filters", class: "govuk-!-margin-bottom-0" %> + <% end %> +
+
+
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index b9b9975be..7da3deda1 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -8,10 +8,14 @@ <% if current_user.data_coordinator? || current_user.support? %> <%= govuk_button_link_to "Invite user", new_user_path, html: { method: :get } %> <% end %> +
+ <%= render partial: "users/user_filters" %> +
+ <%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %> -<%= render SearchComponent.new(current_user:, search_label: "Search by name or email address", value: @searched) %> + <%= govuk_section_break(visible: true, size: "m") %> -<%= govuk_section_break(visible: true, size: "m") %> - -<%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> -<%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %> + <%= render partial: "users/user_list", locals: { users: @users, title:, pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <%= render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "users" } %> +
+
diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index dcf33ac46..f568ef96b 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -228,6 +228,39 @@ RSpec.describe "User Features" do expect(page).not_to have_css('[aria-current="page"]', text: "About your organisation") expect(page).not_to have_css('[aria-current="page"]', text: "Logs") end + + context "when filtering users" do + context "when no filters are selected" do + it "displays the filters component with no clear button" do + expect(page).to have_content("No filters applied") + expect(page).not_to have_content("Clear") + end + end + + context "when I have selected filters" do + before do + check("Active") + check("Deactivated") + click_button("Apply filters") + end + + it "displays the filters component with a correct count and clear button" do + expect(page).to have_content("2 filters applied") + expect(page).to have_content("Clear") + end + + context "when clearing the filters" do + before do + click_link("Clear") + end + + it "clears the filters and displays the filter component as before" do + expect(page).to have_content("No filters applied") + expect(page).not_to have_content("Clear") + end + end + end + end end context "when viewing your organisation details" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2bd862a90..034d37015 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -258,11 +258,11 @@ RSpec.describe User, type: :model do describe "scopes" do let(:organisation_1) { create(:organisation, :without_dpc, name: "A") } let(:organisation_2) { create(:organisation, :without_dpc, name: "B") } - let!(:user_1) { create(:user, name: "Joe Bloggs", email: "joe@example.com", organisation: organisation_1, role: "support") } - let!(:user_3) { create(:user, name: "Tom Smith", email: "tom@example.com", organisation: organisation_1, role: "data_provider") } + let!(:user_1) { create(:user, name: "Joe Bloggs", email: "joe@example.com", organisation: organisation_1, role: "support", last_sign_in_at: Time.zone.now) } let!(:user_2) { create(:user, name: "Jenny Ford", email: "jenny@smith.com", organisation: organisation_1, role: "data_coordinator") } + let!(:user_3) { create(:user, name: "Tom Smith", email: "tom@example.com", organisation: organisation_1, role: "data_provider") } let!(:user_4) { create(:user, name: "Greg Thomas", email: "greg@org2.com", organisation: organisation_2, role: "data_coordinator") } - let!(:user_5) { create(:user, name: "Adam Thomas", email: "adam@org2.com", organisation: organisation_2, role: "data_coordinator") } + let!(:user_5) { create(:user, name: "Adam Thomas", email: "adam@org2.com", organisation: organisation_2, role: "data_coordinator", last_sign_in_at: Time.zone.now) } context "when searching by name" do it "returns case insensitive matching records" do @@ -290,6 +290,46 @@ RSpec.describe User, type: :model do expect(described_class.sorted_by_organisation_and_role.to_a).to eq([user_1, user_2, user_3, user_5, user_4]) end end + + context "when filtering by status" do + before do + user_2.update!(active: false) + user_3.update!(active: false, last_sign_in_at: nil) + user_4.update!(last_sign_in_at: nil) + end + + context "when filtering by active status" do + it "returns only active users" do + expect(described_class.filter_by_status(%w[active]).count).to eq(2) + expect(described_class.filter_by_status(%w[active])).to include(user_1) + expect(described_class.filter_by_status(%w[active])).to include(user_5) + end + end + + context "when filtering by deactivated status" do + it "returns only deactivated users" do + expect(described_class.filter_by_status(%w[deactivated]).count).to eq(2) + expect(described_class.filter_by_status(%w[deactivated])).to include(user_2) + expect(described_class.filter_by_status(%w[deactivated])).to include(user_3) + end + end + + context "when filtering by unconfirmed status" do + it "returns only unconfirmed users" do + expect(described_class.filter_by_status(%w[unconfirmed]).count).to eq(1) + expect(described_class.filter_by_status(%w[unconfirmed])).to include(user_4) + end + end + + context "when filtering by multiple statuses" do + it "returns relevant users" do + expect(described_class.filter_by_status(%w[active unconfirmed]).count).to eq(3) + expect(described_class.filter_by_status(%w[active])).to include(user_1) + expect(described_class.filter_by_status(%w[active])).to include(user_5) + expect(described_class.filter_by_status(%w[unconfirmed])).to include(user_4) + end + end + end end describe "validate" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 92e575cec..984090863 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -471,6 +471,60 @@ RSpec.describe UsersController, type: :request do end end end + + context "when filtering" do + context "with status filter" do + let!(:active_user) { create(:user, name: "active name", active: true, organisation: user.organisation, last_sign_in_at: Time.zone.now) } + let!(:deactivated_user) { create(:user, active: false, name: "deactivated name", organisation: user.organisation, last_sign_in_at: Time.zone.now) } + let!(:unconfirmed_user) { create(:user, last_sign_in_at: nil, name: "unconfirmed name", organisation: user.organisation) } + + it "shows users for multiple selected statuses" do + get "/users?status[]=active&status[]=deactivated", headers:, params: {} + follow_redirect! + expect(page).to have_link(active_user.name) + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered active users" do + get "/users?status[]=active", headers:, params: {} + follow_redirect! + expect(page).to have_link(active_user.name) + expect(page).not_to have_link(deactivated_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered deactivated users" do + get "/users?status[]=deactivated", headers:, params: {} + follow_redirect! + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered unconfirmed users" do + get "/users?status[]=unconfirmed", headers:, params: {} + follow_redirect! + expect(page).to have_link(unconfirmed_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(deactivated_user.name) + end + + it "does not reset the filters" do + get "/users?status[]=deactivated", headers:, params: {} + follow_redirect! + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + + get "/users", headers:, params: {} + follow_redirect! + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + end + end end describe "CSV download" do @@ -1158,6 +1212,54 @@ RSpec.describe UsersController, type: :request do end end end + + context "when filtering" do + context "with status filter" do + let!(:active_user) { create(:user, name: "active name", active: true, last_sign_in_at: Time.zone.now) } + let!(:deactivated_user) { create(:user, active: false, name: "deactivated name", last_sign_in_at: Time.zone.now) } + let!(:unconfirmed_user) { create(:user, last_sign_in_at: nil, name: "unconfirmed name") } + + it "shows users for multiple selected statuses" do + get "/users?status[]=active&status[]=deactivated", headers:, params: {} + expect(page).to have_link(active_user.name) + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered active users" do + get "/users?status[]=active", headers:, params: {} + expect(page).to have_link(active_user.name) + expect(page).not_to have_link(deactivated_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered deactivated users" do + get "/users?status[]=deactivated", headers:, params: {} + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + + it "shows filtered unconfirmed users" do + get "/users?status[]=unconfirmed", headers:, params: {} + expect(page).to have_link(unconfirmed_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(deactivated_user.name) + end + + it "does not reset the filters" do + get "/users?status[]=deactivated", headers:, params: {} + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + + get "/users", headers:, params: {} + expect(page).to have_link(deactivated_user.name) + expect(page).not_to have_link(active_user.name) + expect(page).not_to have_link(unconfirmed_user.name) + end + end + end end describe "CSV download" do