minor refactors after rebasing onto Nat's work

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

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

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
 

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]

app/views/logs/_log_list.html.erb

@@ -10,7 +10,8 @@
   <% end %>
 </div>
 <div class="govuk-grid-column-one-quarter govuk-!-text-align-right">
-  <% 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 %>
 </div>

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" } %>
   </div>

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" } %>
   </div>

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
 

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

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)

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