From 2570c08fe7ee5933c107d0482e70ef5e759fb673 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Thu, 13 Jul 2023 16:43:25 +0100 Subject: [PATCH] CLDC-2241 Add filter count and clear button (#1768) * feat: initial commit * feat: update designs * feat: actually clear filters * refactor: don't use underscore in route * refactor: make filters_count more readable * feat: add session controller tests * feat: add more tests * refactor: simplification * feat: add bulk upload id to filter count, make nil safe * feat: add tests (and add other missing ones to sales log controller spec) * Update flaky test (#1773) * feat: revert db changes * refactor: lint --------- Co-authored-by: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> --- app/controllers/sessions_controller.rb | 13 +++ app/frontend/styles/_filter-layout.scss | 2 +- app/helpers/filters_helper.rb | 30 +++++ app/views/logs/_log_filters.html.erb | 8 ++ config/routes.rb | 2 + spec/controllers/sessions_controller_spec.rb | 33 ++++++ spec/features/lettings_log_spec.rb | 45 ++++++++ spec/features/sales_log_spec.rb | 45 ++++++++ spec/helpers/filters_helper_spec.rb | 43 ++++++- .../requests/lettings_logs_controller_spec.rb | 6 + spec/requests/sales_logs_controller_spec.rb | 109 ++++++++++++++++++ 11 files changed, 334 insertions(+), 2 deletions(-) create mode 100644 app/controllers/sessions_controller.rb create mode 100644 spec/controllers/sessions_controller_spec.rb diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb new file mode 100644 index 000000000..cdcd05c9f --- /dev/null +++ b/app/controllers/sessions_controller.rb @@ -0,0 +1,13 @@ +class SessionsController < ApplicationController + def clear_filters + session[session_name_for(params[:filter_type])] = "{}" + + redirect_to send("#{params[:filter_type]}_path") + end + +private + + def session_name_for(filter_type) + "#{filter_type}_filters" + end +end diff --git a/app/frontend/styles/_filter-layout.scss b/app/frontend/styles/_filter-layout.scss index b9fb838ef..53f1a7d5a 100644 --- a/app/frontend/styles/_filter-layout.scss +++ b/app/frontend/styles/_filter-layout.scss @@ -5,7 +5,7 @@ .app-filter-layout__filter { @include govuk-media-query(wide) { float: left; - min-width: govuk-grid-width("one-quarter"); + width: govuk-grid-width("one-quarter"); } } diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index 34e768603..f54e91fc9 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -46,9 +46,39 @@ module FiltersHelper { "2023": "2023/24", "2022": "2022/23", "2021": "2021/22" } end + def filters_applied_text(filter_type) + applied_filters_count(filter_type).zero? ? "No filters applied" : "#{pluralize(applied_filters_count(filter_type), 'filter')} applied" + end + + def reset_filters_link(filter_type) + if applied_filters_count(filter_type).positive? + govuk_link_to "Clear", clear_filters_path(filter_type:) + end + end + private + def applied_filters_count(filter_type) + filters_count(applied_filters(filter_type)) + end + + def applied_filters(filter_type) + JSON.parse(session[session_name_for(filter_type)]) + end + def session_name_for(filter_type) "#{filter_type}_filters" end + + def filters_count(filters) + filters.each.sum do |category, category_filters| + if %w[status years bulk_upload_id].include?(category) + category_filters.count(&:present?) + elsif %w[user organisation].include?(category) + category_filters != "all" ? 1 : 0 + else + 0 + end + end + end end diff --git a/app/views/logs/_log_filters.html.erb b/app/views/logs/_log_filters.html.erb index d8235d350..d52e25409 100644 --- a/app/views/logs/_log_filters.html.erb +++ b/app/views/logs/_log_filters.html.erb @@ -8,6 +8,14 @@ <%= form_with html: { method: :get } do |f| %> <% all_or_yours = { "all": { label: "All" }, "yours": { label: "Yours" } } %> +
+

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

+

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

+
<% if bulk_upload_options(@bulk_upload).present? %> <%= render partial: "filters/checkbox_filter", locals: { diff --git a/config/routes.rb b/config/routes.rb index 707e8e942..4da2e007e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -54,6 +54,8 @@ Rails.application.routes.draw do get "/download-22-23-sales-bulk-upload-template", to: "start#download_22_23_sales_bulk_upload_template" get "/download-22-23-sales-bulk-upload-specification", to: "start#download_22_23_sales_bulk_upload_specification" + get "clear-filters", to: "sessions#clear_filters" + resource :account, only: %i[show edit], controller: "users" do get "edit/password", to: "users#edit_password" end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb new file mode 100644 index 000000000..4562e9a57 --- /dev/null +++ b/spec/controllers/sessions_controller_spec.rb @@ -0,0 +1,33 @@ +require "rails_helper" + +RSpec.describe SessionsController do + describe "#clear_filters" do + context "when filter_type is lettings_logs" do + let(:filter_type) { "lettings_logs" } + + it "clears only lettings filters" do + session[:lettings_logs_filters] = "{'some_category':'some_filter'}" + session[:sales_logs_filters] = "{'some_other_category':'some_other_filter'}" + + get :clear_filters, params: { filter_type: } + + expect(session[:lettings_logs_filters]).to eq("{}") + expect(session[:sales_logs_filters]).to eq("{'some_other_category':'some_other_filter'}") + end + end + + context "when filter_type is sales_logs" do + let(:filter_type) { "sales_logs" } + + it "clears only sales filters" do + session[:lettings_logs_filters] = "{'some_category':'some_filter'}" + session[:sales_logs_filters] = "{'some_other_category':'some_other_filter'}" + + get :clear_filters, params: { filter_type: } + + expect(session[:lettings_logs_filters]).to eq("{'some_category':'some_filter'}") + expect(session[:sales_logs_filters]).to eq("{}") + end + end + end +end diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 993c7e82c..f74091f5b 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -57,6 +57,51 @@ RSpec.describe "Lettings Log Features" do end end + context "when filtering logs" do + let(:user) { create(:user, last_sign_in_at: Time.zone.now) } + + context "when I am signed in" do + before do + visit("/lettings-logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: user.password) + click_button("Sign in") + end + + 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("Not started") + check("In progress") + choose("Yours") + click_button("Apply filters") + end + + it "displays the filters component with a correct count and clear button" do + expect(page).to have_content("3 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 the signed is user is a Support user" do let(:organisation) { create(:organisation, name: "User org") } let(:support_user) { create(:user, :support, last_sign_in_at: Time.zone.now, organisation:) } diff --git a/spec/features/sales_log_spec.rb b/spec/features/sales_log_spec.rb index 890c82786..9a2fec455 100644 --- a/spec/features/sales_log_spec.rb +++ b/spec/features/sales_log_spec.rb @@ -94,6 +94,51 @@ RSpec.describe "Sales Log Features" do end end + context "when filtering logs" do + let(:user) { create(:user, last_sign_in_at: Time.zone.now) } + + context "when I am signed in" do + before do + visit("/sales-logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: user.password) + click_button("Sign in") + end + + 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("Not started") + check("In progress") + choose("Yours") + click_button("Apply filters") + end + + it "displays the filters component with a correct count and clear button" do + expect(page).to have_content("3 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 signed in as a support user" do let(:devise_notify_mailer) { DeviseNotifyMailer.new } let(:notify_client) { instance_double(Notifications::Client) } diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb index 622fd71f8..65d0c2c6b 100644 --- a/spec/helpers/filters_helper_spec.rb +++ b/spec/helpers/filters_helper_spec.rb @@ -132,7 +132,7 @@ RSpec.describe FiltersHelper do end end - context "when a range of filters are applied" do + context "when a range of filters is applied" do let(:filters) do { "user" => "all", @@ -204,4 +204,45 @@ RSpec.describe FiltersHelper do ) end end + + describe "#filters_applied_text" do + let(:filter_type) { "lettings_logs" } + let(:result) { filters_applied_text(filter_type) } + let(:serialised_filters) { filters&.to_json } + let(:filters) { nil } + + before do + session[:lettings_logs_filters] = serialised_filters if serialised_filters + end + + context "when no filters are applied" do + let(:filters) do + { + "user" => "all", + "status" => [""], + "years" => [""], + "organisation" => "all", + } + end + + it "returns the correct filters count" do + expect(result).to eq "No filters applied" + end + end + + context "when a range of filters is applied" do + let(:filters) do + { + "user" => "all", + "status" => %w[in_progress completed], + "years" => [""], + "organisation" => 2, + } + end + + it "returns the correct filters count" do + expect(result).to eq "3 filters applied" + end + end + end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index bcf1210ce..46f4027a9 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -498,6 +498,12 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).not_to have_content("Status") end + it "has correct filter count and clear button" do + get "/lettings-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content("1 filter applied") + expect(page).to have_content("Clear") + end + it "hides button to create a new log" do get "/lettings-logs?bulk_upload_id[]=#{bulk_upload.id}" expect(page).not_to have_content("Create a new lettings log") diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb index 5d24e6b01..9583cf197 100644 --- a/spec/requests/sales_logs_controller_spec.rb +++ b/spec/requests/sales_logs_controller_spec.rb @@ -350,6 +350,115 @@ RSpec.describe SalesLogsController, type: :request do expect(page).to have_link(sales_log_2023.id.to_s) end end + + context "with bulk_upload_id filter" do + context "with bulk upload that belongs to current user" do + let(:organisation) { create(:organisation) } + + let(:user) { create(:user, organisation:) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + let!(:included_log) { create(:sales_log, :in_progress, bulk_upload:, owning_organisation: organisation) } + let!(:excluded_log) { create(:sales_log, :in_progress, owning_organisation: organisation) } + + it "returns logs only associated with the bulk upload" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + + expect(page).to have_content(included_log.id) + expect(page).not_to have_content(excluded_log.id) + end + + it "dislays how many logs remaining to fix" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content("You need to fix 1 log") + end + + it "displays filter" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content("With logs from bulk upload") + end + + it "hides collection year filter" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).not_to have_content("Collection year") + end + + it "hides status filter" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).not_to have_content("Status") + end + + it "has correct filter count and clear button" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content("1 filter applied") + expect(page).to have_content("Clear") + end + + it "hides button to create a new log" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).not_to have_content("Create a new sales log") + end + + it "displays card with help info" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content("The following logs are from your recent bulk upload") + end + + it "displays meta info about the bulk upload" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + expect(page).to have_content(bulk_upload.filename) + expect(page).to have_content(bulk_upload.created_at.to_fs(:govuk_date_and_time)) + end + end + + context "with bulk upload that belongs to another user" do + let(:organisation) { create(:organisation) } + + let(:user) { create(:user, organisation:) } + let(:other_user) { create(:user, organisation:) } + let(:bulk_upload) { create(:bulk_upload, :sales, user: other_user) } + + let!(:excluded_log) { create(:sales_log, bulk_upload:, owning_organisation: organisation) } + let!(:also_excluded_log) { create(:sales_log, owning_organisation: organisation) } + + it "does not return any logs" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + + expect(page).not_to have_content(excluded_log.id) + expect(page).not_to have_content(also_excluded_log.id) + end + end + + context "when bulk upload has been resolved" do + let(:organisation) { create(:organisation) } + + let(:user) { create(:user, organisation:) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + it "redirects to resume the bulk upload" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + + expect(response).to redirect_to(resume_bulk_upload_sales_result_path(bulk_upload)) + end + end + end + + context "without bulk_upload_id" do + it "does not display filter" do + get "/sales-logs" + expect(page).not_to have_content("With logs from bulk upload") + end + + it "displays button to create a new log" do + get "/sales-logs" + expect(page).to have_content("Create a new sales log") + end + + it "does not display card with help info" do + get "/sales-logs" + expect(page).not_to have_content("The following logs are from your recent bulk upload") + end + end end end