From cd38461927d44e063b1dc4843cc957fd0f98ee25 Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Tue, 13 Jun 2023 16:21:15 +0100 Subject: [PATCH] minor refactors after rebasing onto Nat's work --- app/controllers/delete_logs_controller.rb | 11 +++++-- app/helpers/filters_helper.rb | 17 ++++++----- app/helpers/log_list_helper.rb | 6 ++-- app/models/forms/delete_logs_form.rb | 2 +- app/views/logs/_log_list.html.erb | 3 +- app/views/logs/index.html.erb | 1 + app/views/organisations/logs.html.erb | 1 + spec/helpers/filters_helper_spec.rb | 7 +++-- spec/helpers/log_list_helper_spec.rb | 13 ++++---- spec/requests/delete_logs_controller_spec.rb | 32 ++++++++++---------- spec/views/logs/delete_logs_spec.rb | 6 ++-- 11 files changed, 56 insertions(+), 43 deletions(-) diff --git a/app/controllers/delete_logs_controller.rb b/app/controllers/delete_logs_controller.rb index c2c6a3327..0b4dc3ba5 100644 --- a/app/controllers/delete_logs_controller.rb +++ b/app/controllers/delete_logs_controller.rb @@ -1,6 +1,4 @@ class DeleteLogsController < ApplicationController - include Modules::LogsFilter - rescue_from ActiveRecord::RecordNotFound, with: :render_not_found before_action :session_filters, if: :current_user, except: %i[discard_lettings_logs discard_sales_logs discard_lettings_logs_for_organisation discard_sales_logs_for_organisation] @@ -112,6 +110,15 @@ class DeleteLogsController < ApplicationController private + def session_filters + @session_filters ||= filter_manager.session_filters + end + + def filter_manager + log_type = action_name.include?("lettings") ? "lettings_logs" : "sales_logs" + FilterManager.new(current_user:, session:, params:, filter_type: log_type) + end + def delete_logs_form(log_type:, organisation: false, selected_ids: nil, form_params: {}) paths = case log_type when :lettings diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index 8a242b11e..ca343290b 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -11,14 +11,15 @@ module FiltersHelper selected_filters[filter].include?(value.to_s) end - def any_filter_selected? - return false unless session[:logs_filters] - - selected_filters = JSON.parse(session[:logs_filters]) - filter_selected?("user", "yours") || - selected_filters["organisation"]&.present? || - selected_filters["status"]&.compact_blank&.any? || - selected_filters["years"]&.compact_blank&.any? + def any_filter_selected?(filter_type) + filters_json = session[session_name_for(filter_type)] + return false unless filters_json + + filters = JSON.parse(filters_json) + filters["user"] == "yours" || + filters["organisation"]&.present? || + filters["status"]&.compact_blank&.any? || + filters["years"]&.compact_blank&.any? end def status_filters diff --git a/app/helpers/log_list_helper.rb b/app/helpers/log_list_helper.rb index 54ca4c668..b3e967358 100644 --- a/app/helpers/log_list_helper.rb +++ b/app/helpers/log_list_helper.rb @@ -1,9 +1,9 @@ module LogListHelper - def display_delete_logs?(current_user, search_term) + def display_delete_logs?(current_user, search_term, filter_type) if current_user.data_provider? - filter_selected?("user", "yours") + filter_selected?("user", "yours", filter_type) else - any_filter_selected? || search_term.present? + any_filter_selected?(filter_type) || search_term.present? end end diff --git a/app/models/forms/delete_logs_form.rb b/app/models/forms/delete_logs_form.rb index 220fd15dd..6c4d8eba6 100644 --- a/app/models/forms/delete_logs_form.rb +++ b/app/models/forms/delete_logs_form.rb @@ -11,7 +11,7 @@ module Forms @log_type = attributes[:log_type] @search_term = attributes[:search_term] @current_user = attributes[:current_user] - @logs = FilterService.filter_logs(visible_logs, @search_term, attributes[:log_filters], nil, @current_user) + @logs = FilterManager.filter_logs(visible_logs, @search_term, attributes[:log_filters], nil, @current_user) @selected_ids = attributes[:selected_ids] || @logs.map(&:id) @delete_confirmation_path = attributes[:delete_confirmation_path] @back_to_logs_path = attributes[:back_to_logs_path] diff --git a/app/views/logs/_log_list.html.erb b/app/views/logs/_log_list.html.erb index 73d0ccf9b..d41bdc9a8 100644 --- a/app/views/logs/_log_list.html.erb +++ b/app/views/logs/_log_list.html.erb @@ -10,7 +10,8 @@ <% end %>
- <% if logs&.any? && (display_delete_logs?(@current_user, @searched) || in_organisations_tab?) %> + <% if logs&.any? && (display_delete_logs?(@current_user, searched, filter_type) || in_organisations_tab?) %> + <%# remove @ on current user %> <%= govuk_link_to "Delete logs", delete_logs_path, class: "app-!-colour-red" %> <% end %>
diff --git a/app/views/logs/index.html.erb b/app/views/logs/index.html.erb index 7a9b34962..eac53ed7c 100644 --- a/app/views/logs/index.html.erb +++ b/app/views/logs/index.html.erb @@ -71,6 +71,7 @@ csv_download_url: csv_download_url_for_controller(controller:, search: @searched, codes_only: false), csv_codes_only_download_url: csv_download_url_for_controller(controller:, search: @searched, codes_only: true), delete_logs_path: @delete_logs_path, + filter_type: @filter_type, } %> <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %> diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index a433cebd2..89f54fb1d 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -35,6 +35,7 @@ csv_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: false), csv_codes_only_download_url: csv_download_url_by_log_type(@log_type, @organisation, search: @search_term, codes_only: true), delete_logs_path: @delete_logs_path, + filter_type: @filter_type, } %> <%= render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %> diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb index 84d6030b8..fa803e6fa 100644 --- a/spec/helpers/filters_helper_spec.rb +++ b/spec/helpers/filters_helper_spec.rb @@ -71,15 +71,16 @@ RSpec.describe FiltersHelper do end describe "#any_filter_selected?" do - let(:result) { any_filter_selected? } + let(:filter_type) { "lettings_logs" } + let(:result) { any_filter_selected?(filter_type) } let(:serialised_filters) { filters&.to_json } let(:filters) { nil } before do - session[:logs_filters] = serialised_filters if serialised_filters + session[:lettings_logs_filters] = serialised_filters if serialised_filters end - it "returns false if the session contains no logs filters" do + it "returns false if the session contains no filters" do expect(result).to be_falsey end diff --git a/spec/helpers/log_list_helper_spec.rb b/spec/helpers/log_list_helper_spec.rb index 2a34135b6..22836ec45 100644 --- a/spec/helpers/log_list_helper_spec.rb +++ b/spec/helpers/log_list_helper_spec.rb @@ -5,9 +5,10 @@ RSpec.describe LogListHelper, type: :helper do describe "#display_delete_logs?" do let(:search_term) { nil } + let(:filter_type) { "lettings_logs" } context "when logged in as a data provider" do - let(:result) { display_delete_logs?(user, search_term) } + let(:result) { display_delete_logs?(user, search_term, filter_type) } let(:user) { create(:user) } it "returns false if no filters or search are set" do @@ -16,13 +17,13 @@ RSpec.describe LogListHelper, type: :helper do end it "returns true if the user filter is set to 'yours'" do - allow(self).to receive(:filter_selected?).with("user", "yours").and_return true + allow(self).to receive(:filter_selected?).with("user", "yours", filter_type).and_return true expect(result).to be true end it "returns false if any filters other than the user filter are set" do allow(self).to receive(:filter_selected?).and_return true - allow(self).to receive(:filter_selected?).with("user", "yours").and_return false + allow(self).to receive(:filter_selected?).with("user", "yours", filter_type).and_return false expect(result).to be false end @@ -30,7 +31,7 @@ RSpec.describe LogListHelper, type: :helper do let(:search_term) { "word" } it "still returns false as long as the user filter is not set to yours" do - allow(self).to receive(:filter_selected?).with("user", "yours").and_return false + allow(self).to receive(:filter_selected?).with("user", "yours", filter_type).and_return false expect(result).to be false end end @@ -39,8 +40,8 @@ RSpec.describe LogListHelper, type: :helper do context "when logged in as a support user or data coordinator" do let(:support_user) { create(:user, :support) } let(:data_coordinator) { create(:user, :data_coordinator) } - let(:support_result) { display_delete_logs?(support_user, search_term) } - let(:coordinator_result) { display_delete_logs?(data_coordinator, search_term) } + let(:support_result) { display_delete_logs?(support_user, search_term, filter_type) } + let(:coordinator_result) { display_delete_logs?(data_coordinator, search_term, filter_type) } let(:results) { [support_result, coordinator_result] } it "returns false if no filters or search are set" do diff --git a/spec/requests/delete_logs_controller_spec.rb b/spec/requests/delete_logs_controller_spec.rb index ab017f3ea..123d3d2d0 100644 --- a/spec/requests/delete_logs_controller_spec.rb +++ b/spec/requests/delete_logs_controller_spec.rb @@ -14,7 +14,7 @@ RSpec.describe "DeleteLogs", type: :request do let!(:log_2) { create(:lettings_log, :completed, created_by: user) } before do - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + allow(FilterManager).to receive(:filter_logs).and_return LettingsLog.all end it "calls the filter service with the filters in the session and the search term from the query params" do @@ -26,7 +26,7 @@ RSpec.describe "DeleteLogs", type: :request do } get lettings_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters @@ -59,7 +59,7 @@ RSpec.describe "DeleteLogs", type: :request do let(:selected_ids) { log_1.id } before do - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + allow(FilterManager).to receive(:filter_logs).and_return LettingsLog.all end it "throws an error if selected ids are not provided" do @@ -75,7 +75,7 @@ RSpec.describe "DeleteLogs", type: :request do } get lettings_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters @@ -254,7 +254,7 @@ RSpec.describe "DeleteLogs", type: :request do let!(:log_2) { create(:sales_log, :completed, created_by: user) } before do - allow(FilterService).to receive(:filter_logs).and_return SalesLog.all + allow(FilterManager).to receive(:filter_logs).and_return SalesLog.all end it "calls the filter service with the filters in the session and the search term from the query params" do @@ -266,7 +266,7 @@ RSpec.describe "DeleteLogs", type: :request do } get sales_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters @@ -299,7 +299,7 @@ RSpec.describe "DeleteLogs", type: :request do let(:selected_ids) { log_1.id } before do - allow(FilterService).to receive(:filter_logs).and_return SalesLog.all + allow(FilterManager).to receive(:filter_logs).and_return SalesLog.all end it "throws an error if selected ids are not provided" do @@ -315,7 +315,7 @@ RSpec.describe "DeleteLogs", type: :request do } get sales_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters @@ -498,7 +498,7 @@ RSpec.describe "DeleteLogs", type: :request do let!(:log_2) { create(:lettings_log, :completed, owning_organisation: organisation) } before do - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + allow(FilterManager).to receive(:filter_logs).and_return LettingsLog.all end it "calls the filter service with the filters in the session and the search term from the query params" do @@ -510,7 +510,7 @@ RSpec.describe "DeleteLogs", type: :request do } get lettings_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) @@ -543,7 +543,7 @@ RSpec.describe "DeleteLogs", type: :request do let(:selected_ids) { log_1.id } before do - allow(FilterService).to receive(:filter_logs).and_return LettingsLog.all + allow(FilterManager).to receive(:filter_logs).and_return LettingsLog.all end it "throws an error if selected ids are not provided" do @@ -559,7 +559,7 @@ RSpec.describe "DeleteLogs", type: :request do } get lettings_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) @@ -723,7 +723,7 @@ RSpec.describe "DeleteLogs", type: :request do let!(:log_2) { create(:sales_log, :completed, owning_organisation: organisation) } before do - allow(FilterService).to receive(:filter_logs).and_return SalesLog.all + allow(FilterManager).to receive(:filter_logs).and_return SalesLog.all end it "calls the filter service with the filters in the session and the search term from the query params" do @@ -735,7 +735,7 @@ RSpec.describe "DeleteLogs", type: :request do } get sales_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) @@ -768,7 +768,7 @@ RSpec.describe "DeleteLogs", type: :request do let(:selected_ids) { log_1.id } before do - allow(FilterService).to receive(:filter_logs).and_return SalesLog.all + allow(FilterManager).to receive(:filter_logs).and_return SalesLog.all end it "throws an error if selected ids are not provided" do @@ -784,7 +784,7 @@ RSpec.describe "DeleteLogs", type: :request do } get sales_logs_path(logs_filters) # adds the filters to the session - expect(FilterService).to receive(:filter_logs) { |arg1, arg2, arg3| + expect(FilterManager).to receive(:filter_logs) { |arg1, arg2, arg3| expect(arg1).to contain_exactly(log_1, log_2) expect(arg2).to eq search expect(arg3).to eq logs_filters.merge(organisation: organisation.id.to_s) diff --git a/spec/views/logs/delete_logs_spec.rb b/spec/views/logs/delete_logs_spec.rb index f8a024a86..e8200d0a3 100644 --- a/spec/views/logs/delete_logs_spec.rb +++ b/spec/views/logs/delete_logs_spec.rb @@ -15,7 +15,7 @@ RSpec.describe "logs/delete_logs.html.erb" do before do sign_in user - allow(FilterService).to receive(:filter_logs).and_return lettings_logs + allow(FilterManager).to receive(:filter_logs).and_return lettings_logs assign(:delete_logs_form, delete_logs_form) end @@ -40,7 +40,7 @@ RSpec.describe "logs/delete_logs.html.erb" do before do lettings_logs << lettings_log_2 - allow(FilterService).to receive(:filter_logs).and_return lettings_logs + allow(FilterManager).to receive(:filter_logs).and_return lettings_logs delete_logs_form = Forms::DeleteLogsForm.new(log_type: :lettings, current_user: user, **paths) assign(:delete_logs_form, delete_logs_form) end @@ -76,7 +76,7 @@ RSpec.describe "logs/delete_logs.html.erb" do before do sign_in user - allow(FilterService).to receive(:filter_logs).and_return sales_logs + allow(FilterManager).to receive(:filter_logs).and_return sales_logs assign(:delete_logs_form, delete_logs_form_sales) end