From 0abcc7ac16cf8adeac5387531a4585d3383b1572 Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Wed, 15 Feb 2023 16:19:24 +0000 Subject: [PATCH] change variable names throughout after info on rauby/rails naming conventions, update tests for change in who can view codes only download link --- app/controllers/lettings_logs_controller.rb | 8 +++--- app/controllers/organisations_controller.rb | 8 +++--- app/jobs/email_csv_job.rb | 4 +-- app/models/lettings_log.rb | 4 +-- app/services/csv/lettings_log_csv_service.rb | 10 +++---- app/views/logs/download_csv.html.erb | 2 +- spec/models/lettings_log_spec.rb | 6 ++-- .../requests/lettings_logs_controller_spec.rb | 28 ++++++++----------- .../requests/organisations_controller_spec.rb | 10 +++---- .../csv/lettings_log_csv_service_spec.rb | 2 +- 10 files changed, 39 insertions(+), 43 deletions(-) diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index bae6cc844..582c0390e 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -80,15 +80,15 @@ class LettingsLogsController < LogsController def download_csv unpaginated_filtered_logs = filtered_logs(current_user.lettings_logs, search_term, @session_filters) - is_codes_only_export = params.require(:codes_only) == "true" + codes_only_export = params.require(:codes_only) == "true" - render "download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: email_csv_lettings_logs_path, is_codes_only_export: } + render "download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: email_csv_lettings_logs_path, codes_only_export: } end def email_csv all_orgs = params["organisation_select"] == "all" - is_codes_only_export = params.require(:is_codes_only_export) == "true" - EmailCsvJob.perform_later(current_user, search_term, @session_filters, all_orgs, nil, is_codes_only_export) + codes_only_export = params.require(:codes_only_export) == "true" + EmailCsvJob.perform_later(current_user, search_term, @session_filters, all_orgs, nil, codes_only_export) redirect_to csv_confirmation_lettings_logs_path end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 1370c7358..9a9fcbe47 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -107,14 +107,14 @@ class OrganisationsController < ApplicationController def download_csv organisation_logs = LettingsLog.all.where(owning_organisation_id: @organisation.id) unpaginated_filtered_logs = filtered_logs(organisation_logs, search_term, @session_filters) - is_codes_only_export = params.require(:codes_only) == "true" + codes_only_export = params.require(:codes_only) == "true" - render "logs/download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: logs_email_csv_organisation_path, is_codes_only_export: } + render "logs/download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: logs_email_csv_organisation_path, codes_only_export: } end def email_csv - is_codes_only_export = params.require(:is_codes_only_export) == "true" - EmailCsvJob.perform_later(current_user, search_term, @session_filters, false, @organisation, is_codes_only_export) + codes_only_export = params.require(:codes_only_export) == "true" + EmailCsvJob.perform_later(current_user, search_term, @session_filters, false, @organisation, codes_only_export) redirect_to logs_csv_confirmation_organisation_path end diff --git a/app/jobs/email_csv_job.rb b/app/jobs/email_csv_job.rb index f67ab1cdc..6e7888f18 100644 --- a/app/jobs/email_csv_job.rb +++ b/app/jobs/email_csv_job.rb @@ -5,14 +5,14 @@ class EmailCsvJob < ApplicationJob EXPIRATION_TIME = 3.hours.to_i - def perform(user, search_term = nil, filters = {}, all_orgs = false, organisation = nil, is_codes_only_export = false) # rubocop:disable Style/OptionalBooleanParameter - sidekiq can't serialise named params + def perform(user, search_term = nil, filters = {}, all_orgs = false, organisation = nil, codes_only_export = false) # rubocop:disable Style/OptionalBooleanParameter - sidekiq can't serialise named params unfiltered_logs = organisation.present? && user.support? ? LettingsLog.where(owning_organisation_id: organisation.id) : user.lettings_logs filtered_logs = FilterService.filter_logs(unfiltered_logs, search_term, filters, all_orgs, user) filename = organisation.present? ? "logs-#{organisation.name}-#{Time.zone.now}.csv" : "logs-#{Time.zone.now}.csv" storage_service = Storage::S3Service.new(Configuration::EnvConfigurationService.new, ENV["CSV_DOWNLOAD_PAAS_INSTANCE"]) - storage_service.write_file(filename, BYTE_ORDER_MARK + filtered_logs.to_csv(user, is_codes_only_export:)) + storage_service.write_file(filename, BYTE_ORDER_MARK + filtered_logs.to_csv(user, codes_only_export:)) url = storage_service.get_presigned_url(filename, EXPIRATION_TIME) diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 86f3231d1..9f8d39612 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -448,8 +448,8 @@ class LettingsLog < Log location&.id end - def self.to_csv(user = nil, is_codes_only_export:) - Csv::LettingsLogCsvService.new(user).to_csv(is_codes_only_export:) + def self.to_csv(user = nil, codes_only_export:) + Csv::LettingsLogCsvService.new(user).to_csv(codes_only_export:) end def beds_for_la_rent_range diff --git a/app/services/csv/lettings_log_csv_service.rb b/app/services/csv/lettings_log_csv_service.rb index c060c4391..daa574dec 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -7,13 +7,13 @@ module Csv set_csv_attributes end - def to_csv(is_codes_only_export:) + def to_csv(codes_only_export:) CSV.generate(headers: true) do |csv| csv << @attributes LettingsLog.all.find_each do |record| csv << @attributes.map do |att| - label_from_value(record, att, is_codes_only_export:) + label_from_value(record, att, codes_only_export:) end end end @@ -21,18 +21,18 @@ module Csv private - def label_from_value(record, att, is_codes_only_export:) + def label_from_value(record, att, codes_only_export:) if %w[la prevloc].include? att record.send(att) elsif %w[mrcdate startdate voiddate].include? att record.send(att)&.to_formatted_s(:govuk_date) - elsif is_codes_only_export && att.start_with?("location_", "scheme_") + elsif codes_only_export && att.start_with?("location_", "scheme_") att += "_before_type_cast" unless %w[location_code scheme_code scheme_owning_organisation_name scheme_created_at location_startdate].include? att record.send(att) else att = att.remove("_label", "_detail") # a couple of csv column headers have suffixes for the user that are not reflected in the app domain field_value = record.send(att) - if is_codes_only_export + if codes_only_export field_value else answer_label = record.form diff --git a/app/views/logs/download_csv.html.erb b/app/views/logs/download_csv.html.erb index 74c077bdf..fff55e933 100644 --- a/app/views/logs/download_csv.html.erb +++ b/app/views/logs/download_csv.html.erb @@ -11,6 +11,6 @@

We'll send a secure download link to your email address <%= @current_user.email %>.

You've selected <%= count %> logs.

- <%= govuk_button_to "Send email", post_path, method: :post, params: { search: search_term, is_codes_only_export: } %> + <%= govuk_button_to "Send email", post_path, method: :post, params: { search: search_term, codes_only_export: } %> diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 9cdc1912d..c6f559aec 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2711,7 +2711,7 @@ RSpec.describe LettingsLog do let(:csv_export_file) { File.open("spec/fixtures/files/lettings_logs_download.csv", "r:UTF-8") } it "generates a correct csv from a lettings log" do - expect(described_class.to_csv(is_codes_only_export: false)).to eq(expected_content) + expect(described_class.to_csv(codes_only_export: false)).to eq(expected_content) end end @@ -2719,7 +2719,7 @@ RSpec.describe LettingsLog do let(:csv_export_file) { File.open("spec/fixtures/files/lettings_logs_download_non_support.csv", "r:UTF-8") } it "generates a correct csv from a lettings log" do - expect(described_class.to_csv(user, is_codes_only_export: false)).to eq(expected_content) + expect(described_class.to_csv(user, codes_only_export: false)).to eq(expected_content) end end end @@ -2745,7 +2745,7 @@ RSpec.describe LettingsLog do let(:csv_export_file) { File.open("spec/fixtures/files/lettings_logs_download_codes_only.csv", "r:UTF-8") } it "generates a correct csv from a lettings log" do - expect(described_class.to_csv(is_codes_only_export: true)).to eq(expected_content) + expect(described_class.to_csv(codes_only_export: true)).to eq(expected_content) end end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 1b89f86c7..3cc45c0bf 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -532,7 +532,7 @@ RSpec.describe LettingsLogsController, type: :request do it "displays CSV download links with the correct paths" do get "/lettings-logs", headers:, params: {} expect(page).to have_link("Download (CSV)", href: "/lettings-logs/csv-download?codes_only=false") - expect(page).to have_link("Download (CSV, codes only)", href: "/lettings-logs/csv-download?codes_only=true") + expect(page).not_to have_link("Download (CSV, codes only)") end it "does not display CSV download links if there are no logs" do @@ -590,10 +590,7 @@ RSpec.describe LettingsLogsController, type: :request do get "/lettings-logs?search=#{search_term}", headers:, params: {} download_link = page.find_link("Download (CSV)") download_link_params = CGI.parse(URI.parse(download_link[:href]).query) - codes_only_download_link = page.find_link("Download (CSV, codes only)") - codes_only_download_link_params = CGI.parse(URI.parse(codes_only_download_link[:href]).query) expect(download_link_params).to include("search" => [search_term]) - expect(codes_only_download_link_params).to include("search" => [search_term]) end context "when more than one results with matching postcode" do @@ -661,7 +658,7 @@ RSpec.describe LettingsLogsController, type: :request do end end - context "when there are less than 20 logs" do + context "when there are fewer than 20 logs" do before do get "/lettings-logs", headers:, params: {} end @@ -704,9 +701,8 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).to have_title("Logs - Submit social housing lettings and sales data (CORE) - GOV.UK") end - it "shows the CSV download links" do + it "shows the CSV download link" do expect(page).to have_link("Download (CSV)", href: "/lettings-logs/csv-download?codes_only=false") - expect(page).to have_link("Download (CSV, codes only)", href: "/lettings-logs/csv-download?codes_only=true") end it "does not show the organisation filter" do @@ -1304,14 +1300,14 @@ RSpec.describe LettingsLogsController, type: :request do it "when codes_only query parameter is false, form contains hidden field with correct value" do codes_only = false get "/lettings-logs/csv-download?codes_only=#{codes_only}", headers:, params: {} - hidden_field = page.find("form.button_to").find_field("is_codes_only_export", type: "hidden") + hidden_field = page.find("form.button_to").find_field("codes_only_export", 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 "/lettings-logs/csv-download?codes_only=#{codes_only}", headers:, params: {} - hidden_field = page.find("form.button_to").find_field("is_codes_only_export", type: "hidden") + hidden_field = page.find("form.button_to").find_field("codes_only_export", type: "hidden") expect(hidden_field.value).to eq(codes_only.to_s) end @@ -1347,33 +1343,33 @@ RSpec.describe LettingsLogsController, type: :request do it "creates an E-mail job" do expect { - post "/lettings-logs/email-csv?is_codes_only_export=true", headers:, params: {} + post "/lettings-logs/email-csv?codes_only_export=true", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, nil, true) end it "redirects to the confirmation page" do - post "/lettings-logs/email-csv?is_codes_only_export=true", headers:, params: {} + post "/lettings-logs/email-csv?codes_only_export=true", headers:, params: {} expect(response).to redirect_to(csv_confirmation_lettings_logs_path) end it "passes the search term" do expect { - post "/lettings-logs/email-csv?search=#{lettings_log.id}&is_codes_only_export=false", headers:, params: {} + post "/lettings-logs/email-csv?search=#{lettings_log.id}&codes_only_export=false", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, lettings_log.id.to_s, {}, false, nil, false) end it "passes filter parameters" do expect { - post "/lettings-logs/email-csv?status[]=completed&is_codes_only_export=true", headers:, params: {} + post "/lettings-logs/email-csv?status[]=completed&codes_only_export=true", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, nil, { "status" => %w[completed] }, false, nil, true) end it "passes export type flag" do expect { - post "/lettings-logs/email-csv?is_codes_only_export=true", headers:, params: {} + post "/lettings-logs/email-csv?codes_only_export=true", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, nil, true) expect { - post "/lettings-logs/email-csv?is_codes_only_export=false", headers:, params: {} + post "/lettings-logs/email-csv?codes_only_export=false", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, nil, false) end @@ -1381,7 +1377,7 @@ RSpec.describe LettingsLogsController, type: :request do postcode = "XX1 1TG" expect { - post "/lettings-logs/email-csv?status[]=completed&search=#{postcode}&is_codes_only_export=false", headers:, params: {} + post "/lettings-logs/email-csv?status[]=completed&search=#{postcode}&codes_only_export=false", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, postcode, { "status" => %w[completed] }, false, nil, false) end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index df70ffb48..ce3b5049e 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -1166,18 +1166,18 @@ RSpec.describe OrganisationsController, type: :request do it "provides the organisation to the mail job" do expect { - post "/organisations/#{organisation.id}/logs/email-csv?status[]=completed&is_codes_only_export=false", headers:, params: {} + post "/organisations/#{organisation.id}/logs/email-csv?status[]=completed&codes_only_export=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?is_codes_only_export=#{codes_only_export_type}", headers:, params: {} + post "/organisations/#{organisation.id}/logs/email-csv?codes_only_export=#{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?is_codes_only_export=#{codes_only_export_type}", headers:, params: {} + post "/organisations/#{organisation.id}/logs/email-csv?codes_only_export=#{codes_only_export_type}", headers:, params: {} }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false, organisation, codes_only_export_type) end end @@ -1201,14 +1201,14 @@ RSpec.describe OrganisationsController, type: :request do 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: {} - hidden_field = page.find("form.button_to").find_field("is_codes_only_export", type: "hidden") + hidden_field = page.find("form.button_to").find_field("codes_only_export", 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: {} - hidden_field = page.find("form.button_to").find_field("is_codes_only_export", type: "hidden") + hidden_field = page.find("form.button_to").find_field("codes_only_export", type: "hidden") expect(hidden_field.value).to eq(codes_only.to_s) end diff --git a/spec/services/csv/lettings_log_csv_service_spec.rb b/spec/services/csv/lettings_log_csv_service_spec.rb index e072b5edc..4e2f0f025 100644 --- a/spec/services/csv/lettings_log_csv_service_spec.rb +++ b/spec/services/csv/lettings_log_csv_service_spec.rb @@ -222,7 +222,7 @@ RSpec.describe Csv::LettingsLogCsvService do location_mobility_type location_admin_district location_startdate] - csv = CSV.parse(described_class.new(user).to_csv(is_codes_only_export: false)) + csv = CSV.parse(described_class.new(user).to_csv(codes_only_export: false)) expect(csv.first).to eq(expected_csv_attributes) end end