From 6750aedcf033718bb4663b1dff533854d1fbe28f Mon Sep 17 00:00:00 2001 From: Arthur Campbell Date: Thu, 16 Feb 2023 11:41:31 +0000 Subject: [PATCH] rework csv service for readability, remove delegating methods from lettings log to keep all code to do with mapping between our domain and desired export format in one place --- app/models/lettings_log.rb | 22 +-- app/services/csv/lettings_log_csv_service.rb | 127 +++++++++++++++--- .../lettings_logs_download_codes_only.csv | 2 +- .../csv/lettings_log_csv_service_spec.rb | 2 +- 4 files changed, 111 insertions(+), 42 deletions(-) diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 9f8d39612..58e2bd772 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -425,31 +425,13 @@ class LettingsLog < Log created_by&.is_dpo end - delegate :service_name, :sensitive, :registered_under_care_act, :primary_client_group, :has_other_client_group, :secondary_client_group, :owning_organisation, :managing_organisation, :support_type, :intended_stay, :created_at, prefix: "scheme", to: :scheme, allow_nil: true - delegate :service_name_before_type_cast, :sensitive_before_type_cast, :registered_under_care_act_before_type_cast, :primary_client_group_before_type_cast, :has_other_client_group_before_type_cast, :secondary_client_group_before_type_cast, :owning_organisation_before_type_cast, :managing_organisation_before_type_cast, :support_type_before_type_cast, :intended_stay_before_type_cast, :created_at_before_type_cast, prefix: "scheme", to: :scheme, allow_nil: true - delegate :scheme_type, to: :scheme, allow_nil: true - delegate :scheme_type_before_type_cast, to: :scheme, allow_nil: true - def scheme_code scheme&.id ? "S#{scheme.id}" : nil end - def scheme_owning_organisation_name - scheme_owning_organisation&.name - end - - delegate :postcode, :name, :units, :type_of_unit, :mobility_type, :startdate, prefix: "location", to: :location, allow_nil: true - delegate :postcode_before_type_cast, :name_before_type_cast, :units_before_type_cast, :type_of_unit_before_type_cast, :mobility_type_before_type_cast, :startdate_before_type_cast, prefix: "location", to: :location, allow_nil: true - delegate :location_admin_district, to: :location, allow_nil: true - delegate :location_admin_district_before_type_cast, to: :location, allow_nil: true - - # This is not the location_code in the db, location.id is just called code in the UI - def location_code - location&.id - end - def self.to_csv(user = nil, codes_only_export:) - Csv::LettingsLogCsvService.new(user).to_csv(codes_only_export:) + export_type = codes_only_export ? "codes" : "labels" + Csv::LettingsLogCsvService.new(user, export_type:).to_csv 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 daa574dec..09d373b01 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -2,47 +2,134 @@ module Csv class LettingsLogCsvService CSV_FIELDS_TO_OMIT = %w[hhmemb net_income_value_check first_time_property_let_as_social_housing renttype needstype postcode_known is_la_inferred totchild totelder totadult net_income_known is_carehome previous_la_known is_previous_la_inferred age1_known age2_known age3_known age4_known age5_known age6_known age7_known age8_known letting_allocation_unknown details_known_2 details_known_3 details_known_4 details_known_5 details_known_6 details_known_7 details_known_8 rent_type_detail wrent wscharge wpschrge wsupchrg wtcharge wtshortfall rent_value_check old_form_id old_id retirement_value_check tshortfall_known pregnancy_value_check hhtype new_old vacdays la prevloc unresolved updated_by_id bulk_upload_id].freeze - def initialize(user) + def initialize(user, export_type:) @user = user + @export_type = export_type set_csv_attributes end - def to_csv(codes_only_export:) + def to_csv CSV.generate(headers: true) do |csv| csv << @attributes LettingsLog.all.find_each do |record| - csv << @attributes.map do |att| - label_from_value(record, att, codes_only_export:) - end + csv << @attributes.map { |attribute| get_value(attribute, record) } end end end private - 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 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) + ATTRIBUTES_OF_RELATED_OBJECTS = { + location_code: { + labels: %i[location id], + codes: %i[location id], + }, + location_postcode: { + labels: %i[location postcode], + codes: %i[location postcode], + }, + location_name: { + labels: %i[location name], + codes: %i[location name], + }, + location_units: { + labels: %i[location units], + codes: %i[location units], + }, + location_type_of_unit: { + labels: %i[location type_of_unit], + codes: %i[location type_of_unit_before_type_cast], + }, + location_mobility_type: { + labels: %i[location mobility_type], + codes: %i[location mobility_type_before_type_cast], + }, + location_admin_district: { + labels: %i[location location_admin_district], + codes: %i[location location_admin_district], + }, + location_startdate: { + labels: %i[location startdate], + codes: %i[location startdate], + }, + scheme_service_name: { + labels: %i[scheme service_name], + codes: %i[scheme service_name], + }, + scheme_sensitive: { + labels: %i[scheme sensitive], + codes: %i[scheme sensitive_before_type_cast], + }, + scheme_type: { + labels: %i[scheme scheme_type], + codes: %i[scheme scheme_type_before_type_cast], + }, + scheme_registered_under_care_act: { + labels: %i[scheme registered_under_care_act], + codes: %i[scheme registered_under_care_act_before_type_cast], + }, + scheme_owning_organisation_name: { + labels: %i[scheme owning_organisation name], + codes: %i[scheme owning_organisation name], + }, + scheme_primary_client_group: { + labels: %i[scheme primary_client_group], + codes: %i[scheme primary_client_group_before_type_cast], + }, + scheme_has_other_client_group: { + labels: %i[scheme has_other_client_group], + codes: %i[scheme has_other_client_group_before_type_cast], + }, + scheme_secondary_client_group: { + labels: %i[scheme secondary_client_group], + codes: %i[scheme secondary_client_group_before_type_cast], + }, + scheme_support_type: { + labels: %i[scheme support_type], + codes: %i[scheme support_type_before_type_cast], + }, + scheme_intended_stay: { + labels: %i[scheme intended_stay], + codes: %i[scheme intended_stay_before_type_cast], + }, + scheme_created_at: { + labels: %i[scheme created_at], + codes: %i[scheme created_at], + }, + }.freeze + + def get_value(attribute, record) + attribute = "rent_type" if attribute == "rent_type_detail" # rent_type_detail is the requested column header for rent_type, so as not to confuse with renttype + if ATTRIBUTES_OF_RELATED_OBJECTS.key? attribute.to_sym + call_chain = ATTRIBUTES_OF_RELATED_OBJECTS[attribute.to_sym][@export_type.to_sym] + call_chain.reduce(record) { |object, next_call| object&.send(next_call) } + elsif %w[la prevloc].include? attribute # for all exports we output both the codes and labels for these location attributes + record.send(attribute) + elsif %w[la_label prevloc_label].include? attribute # as above + attribute = attribute.remove("_label") + field_value = record.send(attribute) + get_label(field_value, attribute, record) + elsif %w[mrcdate startdate voiddate].include? attribute + record.send(attribute)&.to_formatted_s(:govuk_date) 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 codes_only_export + field_value = record.send(attribute) + case @export_type + when "codes" field_value - else - answer_label = record.form - .get_question(att, record) - &.label_from_value(field_value) + when "labels" + answer_label = get_label(field_value, attribute, record) answer_label || label_if_boolean_value(field_value) || field_value end end end + def get_label(value, attribute, record) + record.form + .get_question(attribute, record) + &.label_from_value(value) + end + def label_if_boolean_value(value) return "Yes" if value == true return "No" if value == false diff --git a/spec/fixtures/files/lettings_logs_download_codes_only.csv b/spec/fixtures/files/lettings_logs_download_codes_only.csv index 456c4f20d..b0bd8be1a 100644 --- a/spec/fixtures/files/lettings_logs_download_codes_only.csv +++ b/spec/fixtures/files/lettings_logs_download_codes_only.csv @@ -1,2 +1,2 @@ id,status,created_at,updated_at,created_by_name,is_dpo,owning_organisation_name,managing_organisation_name,collection_start_year,needstype,renewal,startdate,rent_type_detail,irproduct_other,tenancycode,propcode,age1,sex1,ecstat1,hhmemb,relat2,age2,sex2,retirement_value_check,ecstat2,armedforces,leftreg,illness,housingneeds_a,housingneeds_b,housingneeds_c,housingneeds_h,is_previous_la_inferred,prevloc_label,prevloc,illness_type_1,illness_type_2,is_la_inferred,la_label,la,postcode_known,postcode_full,previous_la_known,wchair,preg_occ,cbl,earnings,incfreq,net_income_value_check,benefits,hb,period,brent,scharge,pscharge,supcharg,tcharge,offered,layear,ppostcode_full,mrcdate,declaration,ethnic,national,prevten,age3,sex3,ecstat3,age4,sex4,ecstat4,age5,sex5,ecstat5,age6,sex6,ecstat6,age7,sex7,ecstat7,age8,sex8,ecstat8,homeless,underoccupation_benefitcap,reservist,startertenancy,tenancylength,tenancy,rsnvac,unittype_gn,beds,waityear,reasonpref,chr,cap,reasonother,housingneeds_f,housingneeds_g,illness_type_3,illness_type_4,illness_type_8,illness_type_5,illness_type_6,illness_type_7,illness_type_9,illness_type_10,rp_homeless,rp_insan_unsat,rp_medwel,rp_hardship,rp_dontknow,tenancyother,property_owner_organisation,property_manager_organisation,purchaser_code,reason,majorrepairs,hbrentshortfall,property_relet,incref,first_time_property_let_as_social_housing,unitletas,builtype,voiddate,renttype,lettype,totchild,totelder,totadult,net_income_known,nocharge,is_carehome,household_charge,referral,tshortfall,chcharge,ppcodenk,age1_known,age2_known,age3_known,age4_known,age5_known,age6_known,age7_known,age8_known,ethnic_group,letting_allocation_unknown,details_known_2,details_known_3,details_known_4,details_known_5,details_known_6,details_known_7,details_known_8,has_benefits,wrent,wscharge,wpschrge,wsupchrg,wtcharge,wtshortfall,refused,housingneeds,wchchrg,newprop,relat3,relat4,relat5,relat6,relat7,relat8,rent_value_check,old_form_id,lar,irproduct,old_id,joint,tshortfall_known,sheltered,pregnancy_value_check,hhtype,new_old,vacdays,major_repairs_date_value_check,void_date_value_check,housingneeds_type,housingneeds_other,unresolved,updated_by_id,unittype_sh,scheme_code,scheme_service_name,scheme_sensitive,scheme_type,scheme_registered_under_care_act,scheme_owning_organisation_name,scheme_primary_client_group,scheme_has_other_client_group,scheme_secondary_client_group,scheme_support_type,scheme_intended_stay,scheme_created_at,location_code,location_postcode,location_name,location_units,location_type_of_unit,location_mobility_type,location_admin_district,location_startdate -{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,false,DLUHC,DLUHC,2021,2,,2 October 2021,2,,,,,,,,,,,,,,,,,,,,false,,,,,false,E09000033,E09000033,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,8,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,9,1,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},0,1,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2021-04-01 00:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,6,A,Westminster,{location_startdate} +{id},in_progress,2022-02-08 16:52:15 +0000,2022-02-08 16:52:15 +0000,Danny Rojas,false,DLUHC,DLUHC,2021,2,,2 October 2021,2,,,,,,,,,,,,,,,,,,,,false,,,,,false,Westminster,E09000033,,SE1 1TE,,2,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,8,0,0,0,,0,,,,,,,,,,,,,,,,,,,,,,,,0,,,,,,,0,,,,,,,,,,,,,,,,,,,9,1,,,,,,,,6,{scheme_code},{scheme_service_name},{scheme_sensitive},0,1,DLUHC,{scheme_primary_client_group},,{scheme_secondary_client_group},{scheme_support_type},{scheme_intended_stay},2021-04-01 00:00:00 +0100,{location_code},SE1 1TE,Downing Street,20,6,A,Westminster,{location_startdate} diff --git a/spec/services/csv/lettings_log_csv_service_spec.rb b/spec/services/csv/lettings_log_csv_service_spec.rb index 4e2f0f025..00955bc37 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(codes_only_export: false)) + csv = CSV.parse(described_class.new(user, export_type: "labels").to_csv) expect(csv.first).to eq(expected_csv_attributes) end end