correct various linter complaints and tech review suggestions

app/jobs/email_csv_job.rb

@@ -8,19 +8,16 @@ class EmailCsvJob < ApplicationJob
   def perform(user, search_term = nil, filters = {}, all_orgs = false, organisation = nil, codes_only_export = false, log_type = "lettings") # rubocop:disable Style/OptionalBooleanParameter - sidekiq can't serialise named params
     if log_type == "lettings"
       unfiltered_logs = organisation.present? && user.support? ? LettingsLog.visible.where(owning_organisation_id: organisation.id) : user.lettings_logs.visible
+      filtered_logs = FilterService.filter_logs(unfiltered_logs, search_term, filters, all_orgs, user)
+      csv_string = filtered_logs.to_csv(user, codes_only_export:)
     else
       unfiltered_logs = organisation.present? && user.support? ? SalesLog.visible.where(owning_organisation_id: organisation.id) : user.sales_logs.visible
+      filtered_logs = FilterService.filter_logs(unfiltered_logs, search_term, filters, all_orgs, user)
+      export_type = codes_only_export ? "codes" : "labels"
+      csv_string = Csv::SalesLogCsvService.new(export_type:).prepare_csv(filtered_logs)
     end
-    filtered_logs = FilterService.filter_logs(unfiltered_logs, search_term, filters, all_orgs, user)
 
-    filename = [log_type, "logs", organisation&.name, Time.zone.now].compact.join("-") + ".csv"
-
-    csv_string = if log_type == "sales"
-                   export_type = codes_only_export ? "codes" : "labels"
-                   Csv::SalesLogCsvService.new(export_type:).prepare_csv(filtered_logs)
-                 else
-                   filtered_logs.to_csv(user, codes_only_export:)
-                 end
+    filename = "#{[log_type, 'logs', organisation&.name, Time.zone.now].compact.join('-')}.csv"
 
     storage_service = Storage::S3Service.new(Configuration::EnvConfigurationService.new, ENV["CSV_DOWNLOAD_PAAS_INSTANCE"])
     storage_service.write_file(filename, BYTE_ORDER_MARK + csv_string)

app/models/form_handler.rb

@@ -49,10 +49,10 @@ class FormHandler
   # sales_forms = [2021..2023].each { |year| Form.new(nil, year, SALES_SECTIONS, sales) }
   # sidenote, why do we save a reference to the next years sales log in the FormHandler?
   def ordered_sales_questions_for_all_years
-    sales_forms = forms.filter { |name, _form| name.end_with? 'sales' }.values
-    ordered_questions = sales_forms.pop.questions.uniq { |q| q.id }
+    sales_forms = forms.filter { |name, _form| name.end_with? "sales" }.values
+    ordered_questions = sales_forms.pop.questions.uniq(&:id)
     index_of_last_question = 0
-    sales_forms.flat_map(&:questions).each_with_index do |question, i|
+    sales_forms.flat_map(&:questions).each do |question|
       index = ordered_questions.index { |q| q.id == question.id }
       if index
         index_of_last_question = index

app/services/csv/sales_log_csv_service.rb

@@ -21,7 +21,7 @@ module Csv
       created_by_name: %i[created_by name],
       is_dpo: %i[created_by is_dpo],
       owning_organisation_name: %i[owning_organisation name],
-    }
+    }.freeze
 
     # can this be made so general it can also be extracted? quite possibly
     def value(attribute, log)
@@ -66,7 +66,7 @@ module Csv
       "ppostcode_full" => %w[ppostc1 ppostc2],
       "la" => %w[la la_label],
       "prevloc" => %w[prevloc prevloc_label],
-    }
+    }.freeze
 
     def sales_log_attributes
       ordered_questions = FormHandler.instance.ordered_sales_questions_for_all_years
@@ -81,7 +81,7 @@ module Csv
         end
       end
       non_question_fields = %w[id status created_at updated_at created_by_name is_dpo owning_organisation_name collection_start_year creation_method]
-      attributes = non_question_fields + attributes
+      non_question_fields + attributes
     end
   end
-end
+end

spec/jobs/email_csv_job_spec.rb

@@ -41,7 +41,7 @@ describe EmailCsvJob do
       allow(storage_service).to receive(:get_presigned_url).and_return(test_url)
 
       allow(Csv::SalesLogCsvService).to receive(:new).and_return(sales_log_csv_service)
-      allow(sales_log_csv_service).to receive(:prepare_csv).and_return ("")
+      allow(sales_log_csv_service).to receive(:prepare_csv).and_return("")
 
       allow(CsvDownloadMailer).to receive(:new).and_return(mailer)
       allow(mailer).to receive(:send_csv_download_mail)
@@ -59,7 +59,7 @@ describe EmailCsvJob do
       end
     end
 
-    context "When exporting sales logs" do
+    context "when exporting sales logs" do
       it "uses an appropriate filename in S3" do
         expect(storage_service).to receive(:write_file).with(/sales-logs-.*\.csv/, anything)
         job.perform(user, nil, {}, nil, nil, nil, "sales")

spec/models/form_handler_spec.rb

@@ -221,11 +221,11 @@ RSpec.describe FormHandler do
   end
 
   describe "#ordered_sales_questions_for_all_years" do
-    let(:result) { FormHandler.instance.ordered_sales_questions_for_all_years }
+    let(:result) { described_class.instance.ordered_sales_questions_for_all_years }
     let(:now) { Time.zone.now }
 
     it "returns an array of questions" do
-      expect(result).to satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } }
+      expect(result).to(satisfy { |result| result.all? { |element| element.is_a?(Form::Question) } })
     end
 
     it "does not return multiple questions with the same id" do
@@ -237,7 +237,7 @@ RSpec.describe FormHandler do
       household_situation_question_ids = %w[prevten ppcodenk ppostcode_full previous_la_known prevloc buyers_organisations]
       index_of_household_situation_start = 0
       household_situation_question_ids.each_with_index do |id, i|
-        if i = 0
+        if i == 0
           index_of_household_situation_start = result.index { |q| q.id == id }
         else
           question_index = result.index { |q| q.id == id }
@@ -247,21 +247,21 @@ RSpec.describe FormHandler do
     end
 
     it "returns questions form all years" do
-      current_sales_question_ids = FormHandler.instance.forms["current_sales"].questions.map(&:id).uniq
-      previous_sales_question_ids = FormHandler.instance.forms["previous_sales"].questions.map(&:id).uniq
+      current_sales_question_ids = described_class.instance.forms["current_sales"].questions.map(&:id).uniq
+      previous_sales_question_ids = described_class.instance.forms["previous_sales"].questions.map(&:id).uniq
       expect(result.count).to be > current_sales_question_ids.count
       expect(result.count).to be > previous_sales_question_ids.count
     end
 
     it "inserts questions from previous years that do not appear in more recent years in the correct position" do
-      current_sales_question_ids = FormHandler.instance.forms["current_sales"].questions.map(&:id).uniq
-      previous_sales_question_ids = FormHandler.instance.forms["previous_sales"].questions.map(&:id).uniq
+      current_sales_question_ids = described_class.instance.forms["current_sales"].questions.map(&:id).uniq
+      previous_sales_question_ids = described_class.instance.forms["previous_sales"].questions.map(&:id).uniq
       obsolete_question_ids = previous_sales_question_ids - current_sales_question_ids
       expect(obsolete_question_ids.count).to be_positive
       obsolete_question_ids.each do |obsolete_id|
         index = previous_sales_question_ids.index(obsolete_id)
         previous_question_id = previous_sales_question_ids[index - 1]
-        expect(result).to include { |q| q.id == id }
+        expect(result).to(include { |q| q.id == id })
         index_of_previous_question_in_result = result.index { |q| q.id == previous_question_id }
         index_of_obsolete_question_in_result = result.index { |q| q.id == obsolete_id }
         expect(index_of_obsolete_question_in_result).to be index_of_previous_question_in_result + 1

spec/requests/organisations_controller_spec.rb

@@ -375,7 +375,7 @@ RSpec.describe OrganisationsController, type: :request do
 
         it "prevents CSV download" do
           expect {
-            post "/organisations/#{unauthorised_organisation.id}/logs/email-csv", headers:, params: {}
+            post "/organisations/#{unauthorised_organisation.id}/lettings-logs/email-csv", headers:, params: {}
           }.not_to enqueue_job(EmailCsvJob)
           expect(response).to have_http_status(:unauthorized)
         end
@@ -389,7 +389,7 @@ RSpec.describe OrganisationsController, type: :request do
 
         it "prevents CSV download" do
           expect {
-            post "/organisations/#{organisation.id}/logs/email-csv", headers:, params: {}
+            post "/organisations/#{organisation.id}/lettings-logs/email-csv", headers:, params: {}
           }.not_to enqueue_job(EmailCsvJob)
           expect(response).to have_http_status(:unauthorized)
         end
@@ -552,7 +552,7 @@ RSpec.describe OrganisationsController, type: :request do
 
         it "prevents CSV download" do
           expect {
-            post "/organisations/#{unauthorised_organisation.id}/logs/email-csv", headers:, params: {}
+            post "/organisations/#{unauthorised_organisation.id}/lettings-logs/email-csv", headers:, params: {}
           }.not_to enqueue_job(EmailCsvJob)
           expect(response).to have_http_status(:unauthorized)
         end
@@ -566,7 +566,7 @@ RSpec.describe OrganisationsController, type: :request do
 
         it "prevents CSV download" do
           expect {
-            post "/organisations/#{organisation.id}/logs/email-csv", headers:, params: {}
+            post "/organisations/#{organisation.id}/lettings-logs/email-csv", headers:, params: {}
           }.not_to enqueue_job(EmailCsvJob)
           expect(response).to have_http_status(:unauthorized)
         end
@@ -1183,8 +1183,8 @@ RSpec.describe OrganisationsController, type: :request do
 
       it "has CSV download buttons with the correct paths if at least 1 log exists" do
         get "/organisations/#{organisation.id}/lettings-logs"
-        expect(page).to have_link("Download (CSV)", href: "/organisations/#{organisation.id}/logs/csv-download?codes_only=false")
-        expect(page).to have_link("Download (CSV, codes only)", href: "/organisations/#{organisation.id}/logs/csv-download?codes_only=true")
+        expect(page).to have_link("Download (CSV)", href: "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=false")
+        expect(page).to have_link("Download (CSV, codes only)", href: "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=true")
       end
 
       context "when you download the CSV" do
@@ -1197,25 +1197,25 @@ RSpec.describe OrganisationsController, type: :request do
         end
 
         it "only includes logs from that organisation" do
-          get "/organisations/#{organisation.id}/logs/csv-download?codes_only=false"
+          get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=false"
 
           expect(page).to have_text("You've selected 3 logs.")
         end
 
         it "provides the organisation to the mail job" do
           expect {
-            post "/organisations/#{organisation.id}/logs/email-csv?status[]=completed&codes_only=false", headers:, params: {}
+            post "/organisations/#{organisation.id}/lettings-logs/email-csv?status[]=completed&codes_only=false", headers:, params: {}
           }.to enqueue_job(EmailCsvJob).with(user, nil, { "status" => %w[completed] }, false, organisation, false)
         end
 
         it "provides the export type to the mail job" do
           codes_only_export_type = false
           expect {
-            post "/organisations/#{organisation.id}/logs/email-csv?codes_only=#{codes_only_export_type}", headers:, params: {}
+            post "/organisations/#{organisation.id}/lettings-logs/email-csv?codes_only=#{codes_only_export_type}", headers:, params: {}
           }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, organisation, codes_only_export_type)
           codes_only_export_type = true
           expect {
-            post "/organisations/#{organisation.id}/logs/email-csv?codes_only=#{codes_only_export_type}", headers:, params: {}
+            post "/organisations/#{organisation.id}/lettings-logs/email-csv?codes_only=#{codes_only_export_type}", headers:, params: {}
           }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, organisation, codes_only_export_type)
         end
       end
@@ -1223,36 +1223,36 @@ RSpec.describe OrganisationsController, type: :request do
 
     describe "GET #download_csv" do
       it "renders a page with the correct header" do
-        get "/organisations/#{organisation.id}/logs/csv-download?codes_only=false", headers:, params: {}
+        get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=false", headers:, params: {}
         header = page.find_css("h1")
         expect(header.text).to include("Download CSV")
       end
 
       it "renders a form with the correct target containing a button with the correct text" do
-        get "/organisations/#{organisation.id}/logs/csv-download?codes_only=false", headers:, params: {}
+        get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=false", headers:, params: {}
         form = page.find("form.button_to")
         expect(form[:method]).to eq("post")
-        expect(form[:action]).to eq("/organisations/#{organisation.id}/logs/email-csv")
+        expect(form[:action]).to eq("/organisations/#{organisation.id}/lettings-logs/email-csv")
         expect(form).to have_button("Send email")
       end
 
       it "when codes_only query parameter is false, form contains hidden field with correct value" do
         codes_only = false
-        get "/organisations/#{organisation.id}/logs/csv-download?codes_only=#{codes_only}", headers:, params: {}
+        get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=#{codes_only}", headers:, params: {}
         hidden_field = page.find("form.button_to").find_field("codes_only", type: "hidden")
         expect(hidden_field.value).to eq(codes_only.to_s)
       end
 
       it "when codes_only query parameter is true, form contains hidden field with correct value" do
         codes_only = true
-        get "/organisations/#{organisation.id}/logs/csv-download?codes_only=#{codes_only}", headers:, params: {}
+        get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=#{codes_only}", headers:, params: {}
         hidden_field = page.find("form.button_to").find_field("codes_only", type: "hidden")
         expect(hidden_field.value).to eq(codes_only.to_s)
       end
 
       it "when query string contains search parameter, form contains hidden field with correct value" do
         search_term = "blam"
-        get "/organisations/#{organisation.id}/logs/csv-download?codes_only=true&search=#{search_term}", headers:, params: {}
+        get "/organisations/#{organisation.id}/lettings-logs/csv-download?codes_only=true&search=#{search_term}", headers:, params: {}
         hidden_field = page.find("form.button_to").find_field("search", type: "hidden")
         expect(hidden_field.value).to eq(search_term)
       end

spec/services/csv/sales_log_csv_service_spec.rb

@@ -2,6 +2,9 @@ require "rails_helper"
 
 RSpec.describe Csv::SalesLogCsvService do
   let(:form_handler_mock) { instance_double(FormHandler) }
+  let!(:log) { create(:sales_log, :completed) }
+  let(:service) { described_class.new(export_type: "labels") }
+  let(:csv) { CSV.parse(service.prepare_csv(SalesLog.all)) }
 
   it "calls the form handler to get all questions in order when initialized" do
     allow(FormHandler).to receive(:instance).and_return(form_handler_mock)
@@ -10,10 +13,6 @@ RSpec.describe Csv::SalesLogCsvService do
     expect(form_handler_mock).to have_received(:ordered_sales_questions_for_all_years)
   end
 
-  let!(:log) { create(:sales_log, :completed) }
-  let(:service) { described_class.new(export_type: "labels") }
-  let(:csv) { CSV.parse(service.prepare_csv(SalesLog.all)) }
-
   it "returns a string" do
     result = service.prepare_csv(SalesLog.all)
     expect(result).to be_a String
@@ -125,4 +124,4 @@ RSpec.describe Csv::SalesLogCsvService do
       expect(la_label_value).to eq "Barnet"
     end
   end
-end
+end