From 072bc8bbbca7d2bb500f9e647b40fddeec386a1c Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Thu, 20 Apr 2023 17:31:12 +0100 Subject: [PATCH] correct various linter complaints and tech review suggestions --- app/jobs/email_csv_job.rb | 15 ++++----- app/models/form_handler.rb | 6 ++-- app/services/csv/sales_log_csv_service.rb | 8 ++--- spec/jobs/email_csv_job_spec.rb | 4 +-- spec/models/form_handler_spec.rb | 16 +++++----- .../requests/organisations_controller_spec.rb | 32 +++++++++---------- .../csv/sales_log_csv_service_spec.rb | 9 +++--- 7 files changed, 43 insertions(+), 47 deletions(-) diff --git a/app/jobs/email_csv_job.rb b/app/jobs/email_csv_job.rb index 3134478cd..62a0ac89a 100644 --- a/app/jobs/email_csv_job.rb +++ b/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) diff --git a/app/models/form_handler.rb b/app/models/form_handler.rb index aa56fc587..b327529fc 100644 --- a/app/models/form_handler.rb +++ b/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 diff --git a/app/services/csv/sales_log_csv_service.rb b/app/services/csv/sales_log_csv_service.rb index 977121b7d..d942aa5d7 100644 --- a/app/services/csv/sales_log_csv_service.rb +++ b/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 \ No newline at end of file +end diff --git a/spec/jobs/email_csv_job_spec.rb b/spec/jobs/email_csv_job_spec.rb index 40b486ea3..40f13e022 100644 --- a/spec/jobs/email_csv_job_spec.rb +++ b/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") diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index 2a5e8be59..d32cd4066 100644 --- a/spec/models/form_handler_spec.rb +++ b/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 diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index d526f218f..9c6103f9a 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/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 diff --git a/spec/services/csv/sales_log_csv_service_spec.rb b/spec/services/csv/sales_log_csv_service_spec.rb index a89bf544c..c13c7357d 100644 --- a/spec/services/csv/sales_log_csv_service_spec.rb +++ b/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 \ No newline at end of file +end