From 0009d535fe819b3e1e3981c743b816e9b823bab7 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Mon, 23 Jan 2023 17:23:54 +0000 Subject: [PATCH 01/10] CLDC-1896 Bulk upload line endings (#1208) * add class to create logs from bulk upload * create logs when processing bulk uploads * remove bulk_upload_id from csv output * create bulk upload logs only if all valid - this will be changed later to allow for partial logs - and only to create logs when a threshold has been met * add method to blank invalid non setup fields * bulk upload log creation blanks invalid fields * fix incorrect logic for bulk upload renewal * fix linting * bulk upload log creation fail logs to sentry * fix bulk upload line ending parsing * extract bulk uploading csv parsing to class * use csv parser in log creator * change handle line endings mechanism - we now strip all windows line endings for unix based line endings - this normalises things making it simpler --- app/models/bulk_upload.rb | 3 + app/models/log.rb | 12 + .../bulk_upload/lettings/csv_parser.rb | 54 ++++ .../bulk_upload/lettings/log_creator.rb | 57 ++++ .../bulk_upload/lettings/row_parser.rb | 2 +- .../bulk_upload/lettings/validator.rb | 47 ++-- app/services/bulk_upload/processor.rb | 19 ++ app/services/csv/lettings_log_csv_service.rb | 4 +- .../20230113154518_add_bulk_upload_to_logs.rb | 6 + db/schema.rb | 4 + .../files/2022_23_lettings_bulk_upload.csv | 2 + spec/models/lettings_log_spec.rb | 20 ++ spec/rails_helper.rb | 4 +- .../bulk_upload/lettings/log_creator_spec.rb | 68 +++++ .../bulk_upload/lettings/row_parser_spec.rb | 4 +- .../bulk_upload/lettings/validator_spec.rb | 81 +++++- spec/services/bulk_upload/processor_spec.rb | 43 ++- spec/support/bulk_upload/log_to_csv.rb | 255 ++++++++++++++++++ 18 files changed, 638 insertions(+), 47 deletions(-) create mode 100644 app/services/bulk_upload/lettings/csv_parser.rb create mode 100644 app/services/bulk_upload/lettings/log_creator.rb create mode 100644 db/migrate/20230113154518_add_bulk_upload_to_logs.rb create mode 100644 spec/services/bulk_upload/lettings/log_creator_spec.rb create mode 100644 spec/support/bulk_upload/log_to_csv.rb diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb index 3cab1fe23..7933ac204 100644 --- a/app/models/bulk_upload.rb +++ b/app/models/bulk_upload.rb @@ -2,7 +2,10 @@ class BulkUpload < ApplicationRecord enum log_type: { lettings: "lettings", sales: "sales" } belongs_to :user + has_many :bulk_upload_errors + has_many :lettings_logs + has_many :sales_logs after_initialize :generate_identifier, unless: :identifier diff --git a/app/models/log.rb b/app/models/log.rb index 0d0df6451..ea8a637af 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -5,6 +5,8 @@ class Log < ApplicationRecord belongs_to :managing_organisation, class_name: "Organisation", optional: true belongs_to :created_by, class_name: "User", optional: true belongs_to :updated_by, class_name: "User", optional: true + belongs_to :bulk_upload, optional: true + before_save :update_status! STATUS = { "not_started" => 0, "in_progress" => 1, "completed" => 2 }.freeze @@ -50,6 +52,16 @@ class Log < ApplicationRecord form.end_date > Time.zone.today end + def blank_invalid_non_setup_fields! + setup_ids = form.setup_sections.flat_map(&:subsections).flat_map(&:questions).map(&:id) + + errors.each do |error| + next if setup_ids.include?(error.attribute.to_s) + + public_send("#{error.attribute}=", nil) + end + end + private def update_status! diff --git a/app/services/bulk_upload/lettings/csv_parser.rb b/app/services/bulk_upload/lettings/csv_parser.rb new file mode 100644 index 000000000..dde079840 --- /dev/null +++ b/app/services/bulk_upload/lettings/csv_parser.rb @@ -0,0 +1,54 @@ +require "csv" + +class BulkUpload::Lettings::CsvParser + attr_reader :path + + def initialize(path:) + @path = path + end + + def row_offset + 5 + end + + def col_offset + 1 + end + + def cols + @cols ||= ("A".."EE").to_a + end + + def row_parsers + @row_parsers ||= body_rows.map do |row| + stripped_row = row[1..] + headers = ("field_1".."field_134").to_a + hash = Hash[headers.zip(stripped_row)] + + BulkUpload::Lettings::RowParser.new(hash) + end + end + + def body_rows + rows[row_offset..] + end + + def rows + @rows ||= CSV.parse(normalised_string, row_sep:) + end + +private + + def row_sep + "\n" + end + + def normalised_string + return @normalised_string if @normalised_string + + @normalised_string = File.read(path) + @normalised_string.gsub!("\r\n", "\n") + + @normalised_string + end +end diff --git a/app/services/bulk_upload/lettings/log_creator.rb b/app/services/bulk_upload/lettings/log_creator.rb new file mode 100644 index 000000000..9dd4a2f66 --- /dev/null +++ b/app/services/bulk_upload/lettings/log_creator.rb @@ -0,0 +1,57 @@ +class BulkUpload::Lettings::LogCreator + attr_reader :bulk_upload, :path + + def initialize(bulk_upload:, path:) + @bulk_upload = bulk_upload + @path = path + end + + def call + row_parsers.each do |row_parser| + row_parser.valid? + + row_parser.log.blank_invalid_non_setup_fields! + row_parser.log.bulk_upload = bulk_upload + + begin + row_parser.log.save! + rescue StandardError => e + Sentry.capture_exception(e) + end + end + end + +private + + def csv_parser + @csv_parser ||= BulkUpload::Lettings::CsvParser.new(path:) + end + + def row_offset + csv_parser.row_offset + end + + def col_offset + csv_parser.col_offset + end + + def row_parsers + return @row_parsers if @row_parsers + + @row_parsers = csv_parser.row_parsers + + @row_parsers.each do |row_parser| + row_parser.bulk_upload = bulk_upload + end + + @row_parsers + end + + def body_rows + csv_parser.body_rows + end + + def rows + csv_parser.rows + end +end diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index a6c999fe6..47a53c103 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -663,7 +663,7 @@ private when 2 0 when nil - field_116 == 14 ? 1 : 0 + rsnvac == 14 ? 1 : 0 end end diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index 43fd17b7d..8cfdee684 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -169,18 +169,26 @@ class BulkUpload::Lettings::Validator end end + def create_logs? + row_parsers.all?(&:valid?) + end + def self.question_for_field(field) QUESTIONS[field] end private + def csv_parser + @csv_parser ||= BulkUpload::Lettings::CsvParser.new(path:) + end + def row_offset - 5 + csv_parser.row_offset end def col_offset - 1 + csv_parser.col_offset end def field_number_for_attribute(attribute) @@ -188,46 +196,27 @@ private end def cols - @cols ||= ("A".."EE").to_a + csv_parser.cols end def row_parsers - @row_parsers ||= body_rows.map do |row| - stripped_row = row[1..] - headers = ("field_1".."field_134").to_a - hash = Hash[headers.zip(stripped_row)] - hash[:bulk_upload] = bulk_upload + return @row_parsers if @row_parsers - BulkUpload::Lettings::RowParser.new(hash) - end - end - - # determine the row seperator from CSV - # Windows will use \r\n - def row_sep - contents = "" + @row_parsers = csv_parser.row_parsers - File.open(path, "r") do |f| - f.seek(9900) - contents = f.read + @row_parsers.each do |row_parser| + row_parser.bulk_upload = bulk_upload end - rn_count = contents.scan("\r\n").count - n_count = contents.scan(/[^\r]\n/).count - - if rn_count > n_count - "\r\n" - else - "\n" - end + @row_parsers end def rows - @rows ||= CSV.read(path, row_sep:) + csv_parser.rows end def body_rows - rows[row_offset..] + csv_parser.body_rows end def validate_file_not_empty diff --git a/app/services/bulk_upload/processor.rb b/app/services/bulk_upload/processor.rb index 93d3fd3e7..c8c8398c2 100644 --- a/app/services/bulk_upload/processor.rb +++ b/app/services/bulk_upload/processor.rb @@ -8,12 +8,31 @@ class BulkUpload::Processor def call download validator.call + create_logs if validator.create_logs? ensure downloader.delete_local_file! end private + def create_logs + log_creator_class.new( + bulk_upload:, + path: downloader.path, + ).call + end + + def log_creator_class + case bulk_upload.log_type + when "lettings" + BulkUpload::Lettings::LogCreator + when "sales" + BulkUpload::Sales::LogCreator + else + raise "Log creator not found for #{bulk_upload.log_type}" + end + end + def downloader @downloader ||= BulkUpload::Downloader.new(bulk_upload:) end diff --git a/app/services/csv/lettings_log_csv_service.rb b/app/services/csv/lettings_log_csv_service.rb index cd4ab0041..c8a2cf41e 100644 --- a/app/services/csv/lettings_log_csv_service.rb +++ b/app/services/csv/lettings_log_csv_service.rb @@ -1,6 +1,6 @@ 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].freeze + 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) @user = user @@ -40,7 +40,7 @@ module Csv def set_csv_attributes metadata_fields = %w[id status created_at updated_at created_by_name is_dpo owning_organisation_name managing_organisation_name collection_start_year] - metadata_id_fields = %w[managing_organisation_id owning_organisation_id created_by_id] + metadata_id_fields = %w[managing_organisation_id owning_organisation_id created_by_id bulk_upload_id] scheme_and_location_ids = %w[scheme_id location_id] scheme_attributes = %w[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_attributes = %w[location_code location_postcode location_name location_units location_type_of_unit location_mobility_type location_admin_district location_startdate] diff --git a/db/migrate/20230113154518_add_bulk_upload_to_logs.rb b/db/migrate/20230113154518_add_bulk_upload_to_logs.rb new file mode 100644 index 000000000..ceff40fc9 --- /dev/null +++ b/db/migrate/20230113154518_add_bulk_upload_to_logs.rb @@ -0,0 +1,6 @@ +class AddBulkUploadToLogs < ActiveRecord::Migration[7.0] + def change + add_reference :lettings_logs, :bulk_upload + add_reference :sales_logs, :bulk_upload + end +end diff --git a/db/schema.rb b/db/schema.rb index 9e02d77e4..f79d25275 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -265,6 +265,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_16_151942) do t.integer "housingneeds_other" t.boolean "unresolved" t.bigint "updated_by_id" + t.bigint "bulk_upload_id" + t.index ["bulk_upload_id"], name: "index_lettings_logs_on_bulk_upload_id" t.index ["created_by_id"], name: "index_lettings_logs_on_created_by_id" t.index ["location_id"], name: "index_lettings_logs_on_location_id" t.index ["managing_organisation_id"], name: "index_lettings_logs_on_managing_organisation_id" @@ -499,6 +501,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_16_151942) do t.string "postcode_full" t.boolean "is_la_inferred" t.integer "hodate_check" + t.bigint "bulk_upload_id" + t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id" t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id" t.index ["managing_organisation_id"], name: "index_sales_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id" diff --git a/spec/fixtures/files/2022_23_lettings_bulk_upload.csv b/spec/fixtures/files/2022_23_lettings_bulk_upload.csv index b767b6e64..c20057489 100644 --- a/spec/fixtures/files/2022_23_lettings_bulk_upload.csv +++ b/spec/fixtures/files/2022_23_lettings_bulk_upload.csv @@ -69,4 +69,6 @@ if 87 = 1, then a value must be entered",No,,,Yes,,,,No,,,"If the property is be or 106 = 15 - 17",No,"Only if 1 = 2, 4, 6, 8, 10 or 12",,,,No,,No,"Yes, if 45 = 2, 3 or 6",,"Yes, if 50 = 1","Only if 1 = 1, 3, 5, 7, 9 or 11",No,Yes,,,,,,,,,,Only if 1 = 1 - 4 or 9 - 12.,Only if 1 = 1 - 8.,Only if 130 is not 3,No,No,Yes, Bulk upload format and duplicate check,All lettings,Question removed from 22/23 onwards,,Supported housing only,,Question Removed from 2020/21,,,,,,Duplicate check field,,,,,,,,Duplicate check field,,,,,,,,,,,,,,,Duplicate check field,,,,,,,,,,,,,,,,,,,Question removed from 22/23 onwards,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,Duplicate check field,,,,,,,,,,,Question removed from 22/23 onwards,Duplicate check fields,,,,Duplicate check field,General Needs lettings only,,,All lettings,General Needs lettings only,All lettings,General Needs lettings only,,,Question removed from 2020/21,Duplicate check field, “Username does not exist”. ,,,Question removed from 21/22 onwards,,Supported Housing lettings only.,,,,,,,,,,,,Affordable Rent Lettings only,Intermediate Rent Lettings only,,All lettings,All lettings,, Bulk upload field number,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120,121,122,123,124,125,126,127,128,129,130,131,132,133,134, +,1,,,,,,123,1,4,,2,55,54,,,,,,,F,,,,,,,,,,,,,,,1,,,,,,,,17,13,2,,2,3,4,,2,7,,,,,,,,,3,,,,2,1,2,1,3,,,,,,,,,16,4,1000,100,100,100,1300,,,,,,,,,,,,13,1,23,,,4,1,1,2,,,,EC1N,2TD,,3,,3,,,,,2,,,,,,,,,,,,,,1,2,2 ,1,,,,,,123,1,2,,6,55,54,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, +,1,,,,,,123,1,2,,,55,54,,,,,,,"A",,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,13,1,23,,,,,,,,,,,,,123,,123,,,,,,,,,,,,,,,,,,,,,2, diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 13f8eafd2..8a4325880 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2536,4 +2536,24 @@ RSpec.describe LettingsLog do end end end + + describe "#blank_invalid_non_setup_fields!" do + context "when a setup field is invalid" do + subject(:model) { described_class.new(needstype: 404) } + + it "does not blank it" do + model.valid? + expect { model.blank_invalid_non_setup_fields! }.not_to change(model, :needstype) + end + end + + context "when a non setup field is invalid" do + subject(:model) { described_class.new(beds: 404) } + + it "blanks it" do + model.valid? + expect { model.blank_invalid_non_setup_fields! }.to change(model, :beds) + end + end + end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e25631df7..7ae06f7e4 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -32,8 +32,8 @@ Capybara.javascript_driver = :headless # of increasing the boot-up time by auto-requiring all files in the support # directory. Alternatively, in the individual `*_spec.rb` files, manually # require only the support files necessary. -# -# Dir[Rails.root.join('spec', 'support', '**', '*.rb')].sort.each { |f| require f } + +Dir[Rails.root.join("spec/support/**/*.rb")].sort.each { |f| require f } # Checks for pending migrations and applies them before tests are run. # If you are not using ActiveRecord, you can remove these lines. diff --git a/spec/services/bulk_upload/lettings/log_creator_spec.rb b/spec/services/bulk_upload/lettings/log_creator_spec.rb new file mode 100644 index 000000000..acf297dcf --- /dev/null +++ b/spec/services/bulk_upload/lettings/log_creator_spec.rb @@ -0,0 +1,68 @@ +require "rails_helper" + +RSpec.describe BulkUpload::Lettings::LogCreator do + subject(:service) { described_class.new(bulk_upload:, path:) } + + let(:owning_org) { create(:organisation, old_visible_id: 123) } + let(:user) { create(:user, organisation: owning_org) } + + let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } + let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + + describe "#call" do + context "when a valid csv with new log" do + it "creates a new log" do + expect { service.call }.to change(LettingsLog, :count) + end + + it "associates log with bulk upload" do + service.call + + log = LettingsLog.last + expect(log.bulk_upload).to eql(bulk_upload) + expect(bulk_upload.lettings_logs).to include(log) + end + end + + context "when a valid csv with row with one invalid non setup field" do + let(:file) { Tempfile.new } + let(:path) { file.path } + let(:log) do + build( + :lettings_log, + :completed, + renttype: 3, + age1: 5, + owning_organisation: owning_org, + managing_organisation: owning_org, + national: 18, + waityear: 9, + joint: 2, + tenancy: 9, + ppcodenk: 0, + ) + end + + before do + 5.times { file.write "\n" } + file.write(BulkUpload::LogToCsv.new(log:).to_csv_row) + file.rewind + end + + it "creates the log" do + expect { service.call }.to change(LettingsLog, :count).by(1) + end + + it "blanks invalid field" do + service.call + + record = LettingsLog.last + expect(record.age1).to be_blank + end + end + + context "when valid csv with existing log" do + xit "what should happen?" + end + end +end diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb index 4b4795348..8b9ff5102 100644 --- a/spec/services/bulk_upload/lettings/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/row_parser_spec.rb @@ -377,8 +377,8 @@ RSpec.describe BulkUpload::Lettings::RowParser do end end - context "when field_134 is null but rsnvac/field_116 is 14" do - let(:attributes) { { bulk_upload:, field_134: "", field_116: "14" } } + context "when field_134 is null but rsnvac/field_106 is 14" do + let(:attributes) { { bulk_upload:, field_134: "", field_106: "14" } } it "sets renewal to 1" do expect(parser.log.renewal).to eq(1) diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 1543774e9..624345aac 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -3,7 +3,9 @@ require "rails_helper" RSpec.describe BulkUpload::Lettings::Validator do subject(:validator) { described_class.new(bulk_upload:, path:) } - let(:bulk_upload) { create(:bulk_upload) } + let(:organisation) { create(:organisation, old_visible_id: "3") } + let(:user) { create(:user, organisation:) } + let(:bulk_upload) { create(:bulk_upload, user:) } let(:path) { file.path } let(:file) { Tempfile.new } @@ -29,18 +31,79 @@ RSpec.describe BulkUpload::Lettings::Validator do context "when incorrect headers" end - context "when a valid csv" do - let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + describe "#call" do + context "when a valid csv" do + let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } - it "creates validation errors" do - expect { validator.call }.to change(BulkUploadError, :count) + it "creates validation errors" do + expect { validator.call }.to change(BulkUploadError, :count) + end + + it "create validation error with correct values" do + validator.call + + error = BulkUploadError.first + expect(error.row).to eq("7") + end + end + + context "with unix line endings" do + let(:fixture_path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + let(:file) { Tempfile.new } + let(:path) { file.path } + + before do + string = File.read(fixture_path) + string.gsub!("\r\n", "\n") + file.write(string) + file.rewind + end + + it "creates validation errors" do + expect { validator.call }.to change(BulkUploadError, :count) + end + end + + context "without headers" do + let(:log) { build(:lettings_log, :completed) } + let(:file) { Tempfile.new } + let(:path) { file.path } + + before do + file.write("\r\n" * 5) + file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n").to_csv_row) + file.rewind + end + + it "creates validation errors" do + expect { validator.call }.to change(BulkUploadError, :count) + end end + end - it "create validation error with correct values" do - validator.call + describe "#should_create_logs?" do + context "when all logs are valid" do + let(:target_path) { file_fixture("2022_23_lettings_bulk_upload.csv") } - error = BulkUploadError.first - expect(error.row).to eq("6") + before do + target_array = File.open(target_path).readlines + target_array[0..71].each do |line| + file.write line + end + file.rewind + end + + it "returns truthy" do + expect(validator).to be_create_logs + end + end + + context "when there is an invalid log" do + let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + + it "returns falsey" do + expect(validator).not_to be_create_logs + end end end end diff --git a/spec/services/bulk_upload/processor_spec.rb b/spec/services/bulk_upload/processor_spec.rb index fce40b998..aaff12678 100644 --- a/spec/services/bulk_upload/processor_spec.rb +++ b/spec/services/bulk_upload/processor_spec.rb @@ -5,8 +5,8 @@ RSpec.describe BulkUpload::Processor do let(:bulk_upload) { create(:bulk_upload, :lettings) } - context "when processing a bulk upload with errors" do - describe "#call" do + describe "#call" do + context "when processing a bulk upload with errors" do let(:mock_downloader) do instance_double( BulkUpload::Downloader, @@ -30,5 +30,44 @@ RSpec.describe BulkUpload::Processor do expect(mock_downloader).to have_received(:delete_local_file!) end end + + context "when processing a bulk with perfect data" do + let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + + let(:mock_downloader) do + instance_double( + BulkUpload::Downloader, + call: nil, + path:, + delete_local_file!: nil, + ) + end + + let(:mock_validator) do + instance_double( + BulkUpload::Lettings::Validator, + call: nil, + create_logs?: true, + ) + end + + let(:mock_creator) do + instance_double( + BulkUpload::Lettings::LogCreator, + call: nil, + path:, + ) + end + + it "creates logs" do + allow(BulkUpload::Downloader).to receive(:new).with(bulk_upload:).and_return(mock_downloader) + allow(BulkUpload::Lettings::Validator).to receive(:new).and_return(mock_validator) + allow(BulkUpload::Lettings::LogCreator).to receive(:new).with(bulk_upload:, path:).and_return(mock_creator) + + processor.call + + expect(mock_creator).to have_received(:call) + end + end end end diff --git a/spec/support/bulk_upload/log_to_csv.rb b/spec/support/bulk_upload/log_to_csv.rb new file mode 100644 index 000000000..e7d1273d8 --- /dev/null +++ b/spec/support/bulk_upload/log_to_csv.rb @@ -0,0 +1,255 @@ +class BulkUpload::LogToCsv + attr_reader :log, :line_ending + + def initialize(log:, line_ending: "\n") + @log = log + @line_ending = line_ending + end + + def to_csv_row + [ + nil, # 0 + log.renttype, # 1 + nil, + nil, + log.scheme&.old_visible_id, + log.location&.old_visible_id, + nil, + log.tenancycode, + log.startertenancy, + log.tenancy, + log.tenancyother, # 10 + log.tenancylength, + log.age1, + log.age2, + log.age3, + log.age4, + log.age5, + log.age6, + log.age7, + log.age8, + + log.sex1, # 20 + log.sex2, + log.sex3, + log.sex4, + log.sex5, + log.sex6, + log.sex7, + log.sex8, + + log.relat2, + log.relat3, + log.relat4, # 30 + log.relat5, + log.relat6, + log.relat7, + log.relat8, + + log.ecstat1, + log.ecstat2, + log.ecstat3, + log.ecstat4, + log.ecstat5, + log.ecstat6, # 40 + log.ecstat7, + log.ecstat8, + + log.ethnic, + log.national, + log.armedforces, + log.reservist, + log.preg_occ, + log.hb, + log.benefits, + log.earnings, # 50 + net_income_known, + nil, + log.reasonother, + nil, + nil, + nil, + nil, + nil, + nil, + nil, # 60 + log.prevten, + log.prevloc, + ((log.ppostcode_full || "").split(" ") || [""]).first, + ((log.ppostcode_full || "").split(" ") || [""]).last, + previous_postcode_known, + log.layear, + log.waityear, + homeless, + log.reasonpref, + log.rp_homeless, # 70 + log.rp_insan_unsat, + log.rp_medwel, + log.rp_hardship, + log.rp_dontknow, + cbl, + chr, + cap, + log.referral, + log.period, + + log.brent, # 80 + log.scharge, + log.pscharge, + log.supcharg, + log.tcharge, + log.chcharge, + log.household_charge, + log.hbrentshortfall, + log.tshortfall, + log.voiddate&.day, + + log.voiddate&.month, # 90 + log.voiddate&.strftime("%y"), + log.mrcdate&.day, + log.mrcdate&.month, + log.mrcdate&.strftime("%y"), + nil, + log.startdate&.day, + log.startdate&.month, + log.startdate&.strftime("%y"), + log.offered, + + log.propcode, # 100 + log.beds, + log.unittype_gn, + log.builtype, + log.wchair, + log.unitletas, + log.rsnvac, + log.la, + ((log.postcode_full || "").split(" ") || [""]).first, + ((log.postcode_full || "").split(" ") || [""]).last, + + nil, # 110 + log.owning_organisation&.old_visible_id, + nil, + log.managing_organisation&.old_visible_id, + leftreg, + nil, + log.incfreq, + log.sheltered, + log.illness, + log.illness_type_1, + + log.illness_type_2, # 120 + log.illness_type_3, + log.illness_type_4, + log.illness_type_5, + log.illness_type_6, + log.illness_type_7, + log.illness_type_8, + log.illness_type_9, + log.illness_type_10, + london_affordable_rent, + + intermediate_rent_type, # 130 + log.irproduct_other, + log.declaration, + log.joint, + renewal, + line_ending, + ].join(",") + end + + def renewal + case log.renewal + when 1 + 1 + when 0 + 2 + end + end + + def london_affordable_rent + case log.renttype + when Imports::LettingsLogsImportService::RENT_TYPE[:london_affordable_rent] + 1 + end + end + + def intermediate_rent_type + case log.renttype + when Imports::LettingsLogsImportService::RENT_TYPE[:rent_to_buy] + 1 + when Imports::LettingsLogsImportService::RENT_TYPE[:london_living_rent] + 2 + when Imports::LettingsLogsImportService::RENT_TYPE[:other_intermediate_rent_product] + 3 + end + end + + def leftreg + case log.leftreg + when 3 + 3 + when 1 + 4 + when 2 + 5 + when 0 + 6 + end + end + + def net_income_known + case log.net_income_known + when 0 + 1 + when 1 + 2 + when 2 + 4 + end + end + + def previous_postcode_known + case log.ppcodenk + when 1 + 1 + when 0 + 2 + end + end + + def homeless + case log.homeless + when 1 + 1 + when 11 + 12 + end + end + + def cbl + case log.cbl + when 0 + 2 + when 1 + 1 + end + end + + def chr + case log.chr + when 0 + 2 + when 1 + 1 + end + end + + def cap + case log.cap + when 0 + 2 + when 1 + 1 + end + end +end From f0868b81730374f962916da3d45d3c492b3088a5 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 24 Jan 2023 09:34:48 +0000 Subject: [PATCH 02/10] CLDC-1898 Bulk upload headers (#1210) * fix bulk upload age data types * bulk upload handles both with and without headers - headers are from the spreadsheet template - otherwise assume cell A1 is start of the dataset --- .../bulk_upload/lettings/csv_parser.rb | 10 +++-- .../bulk_upload/lettings/row_parser.rb | 16 ++++---- .../bulk_upload/lettings/csv_parser_spec.rb | 38 +++++++++++++++++++ .../bulk_upload/lettings/log_creator_spec.rb | 3 +- .../bulk_upload/lettings/validator_spec.rb | 3 +- spec/support/bulk_upload/log_to_csv.rb | 9 +++-- 6 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 spec/services/bulk_upload/lettings/csv_parser_spec.rb diff --git a/app/services/bulk_upload/lettings/csv_parser.rb b/app/services/bulk_upload/lettings/csv_parser.rb index dde079840..b8e66678b 100644 --- a/app/services/bulk_upload/lettings/csv_parser.rb +++ b/app/services/bulk_upload/lettings/csv_parser.rb @@ -8,11 +8,11 @@ class BulkUpload::Lettings::CsvParser end def row_offset - 5 + with_headers? ? 5 : 0 end def col_offset - 1 + with_headers? ? 1 : 0 end def cols @@ -21,7 +21,7 @@ class BulkUpload::Lettings::CsvParser def row_parsers @row_parsers ||= body_rows.map do |row| - stripped_row = row[1..] + stripped_row = row[col_offset..] headers = ("field_1".."field_134").to_a hash = Hash[headers.zip(stripped_row)] @@ -39,6 +39,10 @@ class BulkUpload::Lettings::CsvParser private + def with_headers? + rows[0][0]&.match?(/\D+/) + end + def row_sep "\n" end diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index 47a53c103..d9a4a3e95 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -15,14 +15,14 @@ class BulkUpload::Lettings::RowParser attribute :field_9, :integer attribute :field_10, :string attribute :field_11, :integer - attribute :field_12, :string - attribute :field_13, :string - attribute :field_14, :string - attribute :field_15, :string - attribute :field_16, :string - attribute :field_17, :string - attribute :field_18, :string - attribute :field_19, :string + attribute :field_12, :integer + attribute :field_13, :integer + attribute :field_14, :integer + attribute :field_15, :integer + attribute :field_16, :integer + attribute :field_17, :integer + attribute :field_18, :integer + attribute :field_19, :integer attribute :field_20, :string attribute :field_21, :string attribute :field_22, :string diff --git a/spec/services/bulk_upload/lettings/csv_parser_spec.rb b/spec/services/bulk_upload/lettings/csv_parser_spec.rb new file mode 100644 index 000000000..6711d82cd --- /dev/null +++ b/spec/services/bulk_upload/lettings/csv_parser_spec.rb @@ -0,0 +1,38 @@ +require "rails_helper" + +RSpec.describe BulkUpload::Lettings::CsvParser do + subject(:service) { described_class.new(path:) } + + let(:path) { file_fixture("2022_23_lettings_bulk_upload.csv") } + + context "when parsing csv with headers" do + it "returns correct offsets" do + expect(service.row_offset).to eq(5) + expect(service.col_offset).to eq(1) + end + + it "parses csv correctly" do + expect(service.row_parsers[0].field_12).to eq(55) + end + end + + context "when parsing csv without headers" do + let(:file) { Tempfile.new } + let(:path) { file.path } + let(:log) { build(:lettings_log, :completed) } + + before do + file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_csv_row) + file.rewind + end + + it "returns correct offsets" do + expect(service.row_offset).to eq(0) + expect(service.col_offset).to eq(0) + end + + it "parses csv correctly" do + expect(service.row_parsers[0].field_12).to eql(log.age1) + end + end +end diff --git a/spec/services/bulk_upload/lettings/log_creator_spec.rb b/spec/services/bulk_upload/lettings/log_creator_spec.rb index acf297dcf..fc7d1cdc0 100644 --- a/spec/services/bulk_upload/lettings/log_creator_spec.rb +++ b/spec/services/bulk_upload/lettings/log_creator_spec.rb @@ -44,8 +44,7 @@ RSpec.describe BulkUpload::Lettings::LogCreator do end before do - 5.times { file.write "\n" } - file.write(BulkUpload::LogToCsv.new(log:).to_csv_row) + file.write(BulkUpload::LogToCsv.new(log:, col_offset: 0).to_csv_row) file.rewind end diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 624345aac..e242858bf 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -70,8 +70,7 @@ RSpec.describe BulkUpload::Lettings::Validator do let(:path) { file.path } before do - file.write("\r\n" * 5) - file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n").to_csv_row) + file.write(BulkUpload::LogToCsv.new(log:, line_ending: "\r\n", col_offset: 0).to_csv_row) file.rewind end diff --git a/spec/support/bulk_upload/log_to_csv.rb b/spec/support/bulk_upload/log_to_csv.rb index e7d1273d8..3b49f0b86 100644 --- a/spec/support/bulk_upload/log_to_csv.rb +++ b/spec/support/bulk_upload/log_to_csv.rb @@ -1,14 +1,15 @@ class BulkUpload::LogToCsv - attr_reader :log, :line_ending + attr_reader :log, :line_ending, :col_offset - def initialize(log:, line_ending: "\n") + def initialize(log:, line_ending: "\n", col_offset: 1) @log = log @line_ending = line_ending + @col_offset = col_offset end def to_csv_row [ - nil, # 0 + [nil] * col_offset, # 0 log.renttype, # 1 nil, nil, @@ -154,7 +155,7 @@ class BulkUpload::LogToCsv log.joint, renewal, line_ending, - ].join(",") + ].flatten.join(",") end def renewal From 1bf8373a74af4612098fc7d9a8e1f65dc4fdf8c0 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 24 Jan 2023 09:37:34 +0000 Subject: [PATCH 03/10] CLDC-867 Add previous bedrooms validations (#1219) * Add previous property bedsit validations * Add min and max for frombeds --- .../form/sales/questions/previous_bedrooms.rb | 3 ++- .../sales/sale_information_validations.rb | 9 ++++++++ config/locales/en.yml | 5 +++++ .../sales/questions/previous_bedrooms_spec.rb | 6 ++++- .../sale_information_validations_spec.rb | 22 +++++++++++++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/app/models/form/sales/questions/previous_bedrooms.rb b/app/models/form/sales/questions/previous_bedrooms.rb index e63a1b3cf..1bea6538c 100644 --- a/app/models/form/sales/questions/previous_bedrooms.rb +++ b/app/models/form/sales/questions/previous_bedrooms.rb @@ -6,7 +6,8 @@ class Form::Sales::Questions::PreviousBedrooms < ::Form::Question @header = "How many bedrooms did the property have?" @type = "numeric" @width = 5 - @min = 0 + @min = 1 + @max = 6 @hint_text = "For bedsits enter 1" end end diff --git a/app/models/validations/sales/sale_information_validations.rb b/app/models/validations/sales/sale_information_validations.rb index 11049e8ff..49e39d052 100644 --- a/app/models/validations/sales/sale_information_validations.rb +++ b/app/models/validations/sales/sale_information_validations.rb @@ -14,4 +14,13 @@ module Validations::Sales::SaleInformationValidations record.errors.add :hodate, "Practical completion or handover date must be before exchange date" end end + + def validate_previous_property_unit_type(record) + return unless record.fromprop && record.frombeds + + if record.frombeds != 1 && record.fromprop == 2 + record.errors.add :frombeds, I18n.t("validations.sale_information.previous_property_beds.property_type_bedsit") + record.errors.add :fromprop, I18n.t("validations.sale_information.previous_property_type.property_type_bedsit") + end + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index fbfd0c8e0..73bd8bdf7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -408,6 +408,11 @@ en: before_deactivation: "This location was deactivated on %{date}. The reactivation date must be on or after deactivation date" deactivation: during_deactivated_period: "The location is already deactivated during this date, please enter a different date" + sale_information: + previous_property_beds: + property_type_bedsit: "Bedsit bedroom maximum 1" + previous_property_type: + property_type_bedsit: "A bedsit can not have more than 1 bedroom" soft_validations: net_income: diff --git a/spec/models/form/sales/questions/previous_bedrooms_spec.rb b/spec/models/form/sales/questions/previous_bedrooms_spec.rb index bbfef7fab..1798e23fa 100644 --- a/spec/models/form/sales/questions/previous_bedrooms_spec.rb +++ b/spec/models/form/sales/questions/previous_bedrooms_spec.rb @@ -40,6 +40,10 @@ RSpec.describe Form::Sales::Questions::PreviousBedrooms, type: :model do end it "has correct min" do - expect(question.min).to eq(0) + expect(question.min).to eq(1) + end + + it "has correct max" do + expect(question.max).to eq(6) end end diff --git a/spec/models/validations/sales/sale_information_validations_spec.rb b/spec/models/validations/sales/sale_information_validations_spec.rb index a808d2ba8..fafe2786a 100644 --- a/spec/models/validations/sales/sale_information_validations_spec.rb +++ b/spec/models/validations/sales/sale_information_validations_spec.rb @@ -108,4 +108,26 @@ RSpec.describe Validations::Sales::SaleInformationValidations do end end end + + describe "#validate_previous_property_unit_type" do + context "when number of bedrooms is <= 1" do + let(:record) { FactoryBot.build(:sales_log, frombeds: 1, fromprop: 2) } + + it "does not add an error if it's a bedsit" do + sale_information_validator.validate_previous_property_unit_type(record) + + expect(record.errors).to be_empty + end + end + + context "when number of bedrooms is > 1" do + let(:record) { FactoryBot.build(:sales_log, frombeds: 2, fromprop: 2) } + + it "does add an error if it's a bedsit" do + sale_information_validator.validate_previous_property_unit_type(record) + expect(record.errors["fromprop"]).to include(I18n.t("validations.sale_information.previous_property_type.property_type_bedsit")) + expect(record.errors["frombeds"]).to include(I18n.t("validations.sale_information.previous_property_beds.property_type_bedsit")) + end + end + end end From 92a00e43054245d4b415bc3c99c39a3cf918abe1 Mon Sep 17 00:00:00 2001 From: SamSeed-Softwire <63662292+SamSeed-Softwire@users.noreply.github.com> Date: Tue, 24 Jan 2023 10:11:46 +0000 Subject: [PATCH 04/10] CLDC-1723 Overhaul letting log owning & managing org questions & tests (#1140) * test: check managing org not gone from answer opts when relationship deleted * feat: add current managing org to answer opts * feat: check if managing org exists before trying to show it * wip * test: improve managing orgs opts test when not support * test: improve managing orgs opts tests when support * test: make relationship deletion test consistent with other tests * test: add "(with hint)" to managing org opts test descriptions * test: refactor managing orgs opts tests for support user case * fix: don't call user in get_answer_label in CYA component * style: reorder instance vars and remove old comments in managing_organisation.rb * refactor: ensure label_from_value always accepts log & nil as args * lint * test: pass in log and user in housing provider opts test for support user * test: update housing provider opts tests for non-support user * feat: update housing provider answer opts to include current HP in db * style: add space after user definition * test: make context definition more human-readable * test: refactor housing providers opts tests (not support user) * test: check housing prov. still selectable after deleting relationship * fix: define log and current_user instance vars in label_from_value (housing prov.) * lint * test: update lettings log feature tests to differentiate between different numbers of stock owners when acting as a data coordinator * test: check owning & managing orgs set correctly when a log is created * test: add line breaks and start context descriptions with and (not if) * test: artificially reference org_rel2 to avoid lint offense * feat: don't set log owning org as user's org if that org doesn't hold stock * test: improve test context descriptions in lettings_log_spec * test: finish overhauling owning and managing org tests in lettings_log_spec * test: change let! to let where possible in spec/features/lettings_log_spec.rb * test: change let! to let where possible in spec/models/form/lettings/questions/managing_organisation_spec.rb * test: change let! to let where possible in spec/models/form/lettings/questions/stock_owner_spec.rb * test: remove if statement from "coordinator user's org doesn't hold stock" managing org test * test: remove if statement from "coordinator user's org does hold stock" no managing orgs managing org test * test: remove if statement from "coordinator user's org does hold stock" >=1 managing orgs managing org test plus refactor previous test * test: explicitly reference org rels in "coordinator user's org doesn't hold stock" managing org test * test: don't create vars inside other vars (for tests edited/created in this branch) * chore: save schema changes after migration Co-authored-by: Phil Lee --- ...eck_answers_summary_list_card_component.rb | 2 +- app/controllers/logs_controller.rb | 3 +- app/helpers/check_answers_helper.rb | 2 +- .../form/common/questions/created_by_id.rb | 2 +- .../questions/owning_organisation_id.rb | 2 +- .../form/lettings/questions/created_by_id.rb | 2 +- .../questions/managing_organisation.rb | 10 +- .../form/lettings/questions/stock_owner.rb | 11 +- app/models/form/question.rb | 6 +- db/schema.rb | 10 +- spec/features/lettings_log_spec.rb | 239 +++++++++++++++++- .../questions/managing_organisation_spec.rb | 117 +++++---- .../lettings/questions/stock_owner_spec.rb | 74 ++++-- .../requests/lettings_logs_controller_spec.rb | 58 +++++ 14 files changed, 439 insertions(+), 99 deletions(-) diff --git a/app/components/check_answers_summary_list_card_component.rb b/app/components/check_answers_summary_list_card_component.rb index 246173f48..113303ba0 100644 --- a/app/components/check_answers_summary_list_card_component.rb +++ b/app/components/check_answers_summary_list_card_component.rb @@ -13,7 +13,7 @@ class CheckAnswersSummaryListCardComponent < ViewComponent::Base end def get_answer_label(question) - question.answer_label(log).presence || "You didn’t answer this question".html_safe + question.answer_label(log, user).presence || "You didn’t answer this question".html_safe end def check_answers_card_title(question) diff --git a/app/controllers/logs_controller.rb b/app/controllers/logs_controller.rb index 3982d4a54..083d40178 100644 --- a/app/controllers/logs_controller.rb +++ b/app/controllers/logs_controller.rb @@ -60,8 +60,9 @@ private end def org_params + owning_organisation_id = current_user.organisation.holds_own_stock? ? current_user.organisation.id : nil { - "owning_organisation_id" => current_user.organisation.id, + "owning_organisation_id" => owning_organisation_id, "managing_organisation_id" => current_user.organisation.id, "created_by_id" => current_user.id, } diff --git a/app/helpers/check_answers_helper.rb b/app/helpers/check_answers_helper.rb index 709bbacd6..66a833859 100644 --- a/app/helpers/check_answers_helper.rb +++ b/app/helpers/check_answers_helper.rb @@ -43,6 +43,6 @@ private end def get_answer_label(question, lettings_log) - question.answer_label(lettings_log).presence || "You didn’t answer this question".html_safe + question.answer_label(lettings_log, current_user).presence || "You didn’t answer this question".html_safe end end diff --git a/app/models/form/common/questions/created_by_id.rb b/app/models/form/common/questions/created_by_id.rb index 5b6631868..ee2767a4f 100644 --- a/app/models/form/common/questions/created_by_id.rb +++ b/app/models/form/common/questions/created_by_id.rb @@ -24,7 +24,7 @@ class Form::Common::Questions::CreatedById < ::Form::Question answer_options.select { |k, _v| user_ids.include?(k) } end - def label_from_value(value) + def label_from_value(value, _log = nil, _user = nil) return unless value answer_options[value] diff --git a/app/models/form/common/questions/owning_organisation_id.rb b/app/models/form/common/questions/owning_organisation_id.rb index c09968ef9..14e262f58 100644 --- a/app/models/form/common/questions/owning_organisation_id.rb +++ b/app/models/form/common/questions/owning_organisation_id.rb @@ -21,7 +21,7 @@ class Form::Common::Questions::OwningOrganisationId < ::Form::Question answer_options end - def label_from_value(value) + def label_from_value(value, _log = nil, _user = nil) return unless value answer_options[value] diff --git a/app/models/form/lettings/questions/created_by_id.rb b/app/models/form/lettings/questions/created_by_id.rb index d61be8882..6402f14e0 100644 --- a/app/models/form/lettings/questions/created_by_id.rb +++ b/app/models/form/lettings/questions/created_by_id.rb @@ -27,7 +27,7 @@ class Form::Lettings::Questions::CreatedById < ::Form::Question answer_options.select { |k, _v| user_ids.include?(k) } end - def label_from_value(value) + def label_from_value(value, _log = nil, _user = nil) return unless value answer_options[value] diff --git a/app/models/form/lettings/questions/managing_organisation.rb b/app/models/form/lettings/questions/managing_organisation.rb index bd4f57fe8..e2e3b3e3d 100644 --- a/app/models/form/lettings/questions/managing_organisation.rb +++ b/app/models/form/lettings/questions/managing_organisation.rb @@ -12,10 +12,15 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question def answer_options opts = { "" => "Select an option" } + return opts unless ActiveRecord::Base.connected? return opts unless current_user return opts unless log + if log.managing_organisation.present? + opts = opts.merge({ log.managing_organisation.id => log.managing_organisation.name }) + end + if current_user.support? if log.owning_organisation.holds_own_stock? opts[log.owning_organisation.id] = "#{log.owning_organisation.name} (Owning organisation)" @@ -34,7 +39,10 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question answer_options end - def label_from_value(value) + def label_from_value(value, log = nil, user = nil) + @log = log + @current_user = user + return unless value answer_options[value] diff --git a/app/models/form/lettings/questions/stock_owner.rb b/app/models/form/lettings/questions/stock_owner.rb index de82258a6..e2d2a633d 100644 --- a/app/models/form/lettings/questions/stock_owner.rb +++ b/app/models/form/lettings/questions/stock_owner.rb @@ -11,8 +11,14 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question def answer_options answer_opts = { "" => "Select an option" } + return answer_opts unless ActiveRecord::Base.connected? return answer_opts unless current_user + return answer_opts unless log + + if log.owning_organisation_id.present? + answer_opts = answer_opts.merge({ log.owning_organisation.id => log.owning_organisation.name }) + end if !current_user.support? && current_user.organisation.holds_own_stock? answer_opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)" @@ -28,7 +34,10 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question answer_options end - def label_from_value(value) + def label_from_value(value, log = nil, user = nil) + @log = log + @current_user = user + return unless value answer_options[value] diff --git a/app/models/form/question.rb b/app/models/form/question.rb index ebc7b1fc6..cbfc6aae2 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -46,11 +46,11 @@ class Form::Question delegate :subsection, to: :page delegate :form, to: :subsection - def answer_label(log) + def answer_label(log, user = nil) return checkbox_answer_label(log) if type == "checkbox" return log[id]&.to_formatted_s(:govuk_date).to_s if type == "date" - answer = label_from_value(log[id]) if log[id].present? + answer = label_from_value(log[id], log, user) if log[id].present? answer_label = [prefix, format_value(answer), suffix_label(log)].join("") if answer inferred_answer_value(log) || answer_label @@ -151,7 +151,7 @@ class Form::Question end end - def label_from_value(value) + def label_from_value(value, _log = nil, _user = nil) return unless value label = case type diff --git a/db/schema.rb b/db/schema.rb index f79d25275..e05334946 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -490,16 +490,16 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_16_151942) do t.integer "mortgagelender" t.string "mortgagelenderother" t.integer "mortlen" - t.integer "extrabor" - t.integer "hhmemb" - t.integer "totadult" - t.integer "totchild" - t.integer "hhtype" t.string "pcode1" t.string "pcode2" t.integer "pcodenk" t.string "postcode_full" t.boolean "is_la_inferred" + t.integer "extrabor" + t.integer "hhmemb" + t.integer "totadult" + t.integer "totchild" + t.integer "hhtype" t.integer "hodate_check" t.bigint "bulk_upload_id" t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id" diff --git a/spec/features/lettings_log_spec.rb b/spec/features/lettings_log_spec.rb index 725be4744..f145f9e7c 100644 --- a/spec/features/lettings_log_spec.rb +++ b/spec/features/lettings_log_spec.rb @@ -58,7 +58,8 @@ RSpec.describe "Lettings Log Features" do end context "when the signed is user is a Support user" do - let(:support_user) { create(:user, :support, last_sign_in_at: Time.zone.now) } + let(:organisation) { create(:organisation, name: "User org") } + let(:support_user) { create(:user, :support, last_sign_in_at: Time.zone.now, organisation:) } let(:devise_notify_mailer) { DeviseNotifyMailer.new } let(:notify_client) { instance_double(Notifications::Client) } let(:mfa_template_id) { User::MFA_TEMPLATE_ID } @@ -78,7 +79,7 @@ RSpec.describe "Lettings Log Features" do end context "when completing the setup lettings log section", :aggregate_failure do - it "includes the organisation and created by questions" do + it "includes the owning organisation and created by questions" do visit("/lettings-logs") click_button("Create a new lettings log") click_link("Set up this lettings log") @@ -88,14 +89,116 @@ RSpec.describe "Lettings Log Features" do click_button("Save and continue") log_id = page.current_path.scan(/\d/).join visit("lettings-logs/#{log_id}/setup/check-answers") - expect(page).to have_content("Stock owner #{support_user.organisation.name}") + expect(page).to have_content("Stock owner User org") expect(page).to have_content("You have answered 2 of 8 questions") end end + + context "when the owning organisation question isn't answered" do + it "doesn't show the managing agent question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + click_link("Skip for now") + expect(page).not_to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + end + end + + context "when the owning organisation question is answered" do + context "and the owning organisation doesn't hold stock" do + let(:managing_org) { create(:organisation, name: "Managing org") } + let!(:org_rel) { create(:organisation_relationship, parent_organisation: support_user.organisation, child_organisation: managing_org) } + + before do + support_user.organisation.update!(holds_own_stock: false) + end + + it "shows the managing organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + select(managing_org.name, from: "lettings-log-managing-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent Managing org") + expect(support_user.organisation.managing_agents).to eq([org_rel.child_organisation]) + end + end + + context "and the owning organisation does hold stock" do + before do + support_user.organisation.update!(holds_own_stock: true) + end + + context "and the owning organisation has no managing agents" do + it "doesn't show the managing organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).not_to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).not_to have_content("Managing agent ") + end + end + + context "and the owning organisation has 1 or more managing agents" do + let(:managing_org1) { create(:organisation, name: "Managing org 1") } + let!(:org_rel1) { create(:organisation_relationship, parent_organisation: support_user.organisation, child_organisation: managing_org1) } + + it "does show the managing organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + select(managing_org1.name, from: "lettings-log-managing-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent Managing org 1") + end + + context "and the owning organisation has 2 or more managing agents" do + let(:managing_org2) { create(:organisation, name: "Managing org 2") } + let!(:org_rel2) { create(:organisation_relationship, parent_organisation: support_user.organisation, child_organisation: managing_org2) } + + context "and the organisation relationship for the selected managing agent is deleted" do + it "doesn't change the CYA page text to be 'You didn't answer this question'" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + select(support_user.organisation.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + select(managing_org1.name, from: "lettings-log-managing-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent Managing org 1") + org_rel1.destroy! + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent Managing org 1") + expect(support_user.organisation.managing_agents).to eq([org_rel2.child_organisation]) + end + end + end + end + end + end end context "when the signed is user is not a Support user" do - let(:user) { create(:user) } + let(:organisation) { create(:organisation, name: "User org") } + let(:user) { create(:user, :data_coordinator, name: "User name", organisation:) } let(:devise_notify_mailer) { DeviseNotifyMailer.new } let(:notify_client) { instance_double(Notifications::Client) } @@ -110,15 +213,125 @@ RSpec.describe "Lettings Log Features" do end context "when completing the setup log section" do - it "does not include the organisation and created by questions" do - visit("/lettings-logs") - click_button("Create a new lettings log") - click_link("Set up this lettings log") - log_id = page.current_path.scan(/\d/).join - expect(page).to have_current_path("/lettings-logs/#{log_id}/needs-type") - visit("lettings-logs/#{log_id}/setup/check-answers") - expect(page).not_to have_content("Owning organisation #{user.organisation.name}") - expect(page).not_to have_content("Log owner #{user.name}") + context "and there is at most 1 potential stock owner" do + it "does not include the owning organisation and created by questions" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/needs-type") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).not_to have_content("Stock owner ") + expect(page).not_to have_content("Log owner ") + end + end + + context "and there are 2 or more potential stock owners" do + let(:owning_org1) { create(:organisation, name: "Owning org 1") } + let!(:org_rel1) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org1) } + + it "does include the owning organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/stock-owner") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Stock owner User org") + end + + context "and there are 3 or more potential stock owners" do + let(:owning_org2) { create(:organisation, name: "Owning org 2") } + let!(:org_rel2) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org2) } + + context "and the organisation relationship for the selected stock owner is deleted" do + it "doesn't change the CYA page text to be 'You didn't answer this question'" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/stock-owner") + select(owning_org1.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Stock owner Owning org 1") + org_rel1.destroy! + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Stock owner Owning org 1") + expect(user.organisation.stock_owners).to eq([org_rel2.parent_organisation]) + end + end + end + end + + context "when the current user's organisation doesn't hold stock" do + let(:owning_org1) { create(:organisation, name: "Owning org 1") } + let(:owning_org2) { create(:organisation, name: "Owning org 2") } + let!(:org_rel1) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org1) } + let!(:org_rel2) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org2) } + + it "shows the managing organisation question" do + user.organisation.update!(holds_own_stock: false) + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/stock-owner") + select(owning_org1.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + select(user.organisation.name, from: "lettings-log-managing-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent User org") + expect(user.organisation.stock_owners).to eq([org_rel1.parent_organisation, org_rel2.parent_organisation]) + end + end + + context "when the current user's organisation does hold stock" do + let!(:owning_org) { create(:organisation, name: "Owning org") } + let!(:org_rel1) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org) } + + before do + user.organisation.update!(holds_own_stock: true) + end + + context "and the user's organisation has no managing agents" do + it "doesn't show the managing organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/stock-owner") + select(owning_org.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).not_to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).not_to have_content("Managing agent ") + expect(user.organisation.stock_owners).to eq([org_rel1.parent_organisation]) + end + end + + context "and the user's organisation has 1 or more managing agents" do + let(:managing_org) { create(:organisation, name: "Managing org") } + let!(:org_rel2) { create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: managing_org) } + + it "does show the managing organisation question" do + visit("/lettings-logs") + click_button("Create a new lettings log") + click_link("Set up this lettings log") + log_id = page.current_path.scan(/\d/).join + expect(page).to have_current_path("/lettings-logs/#{log_id}/stock-owner") + select(user.organisation.name, from: "lettings-log-owning-organisation-id-field") + click_button("Save and continue") + expect(page).to have_current_path("/lettings-logs/#{log_id}/managing-organisation") + select(managing_org.name, from: "lettings-log-managing-organisation-id-field") + click_button("Save and continue") + visit("lettings-logs/#{log_id}/setup/check-answers") + expect(page).to have_content("Managing agent Managing org") + expect(user.organisation.managing_agents).to eq([org_rel2.child_organisation]) + end + end end end diff --git a/spec/models/form/lettings/questions/managing_organisation_spec.rb b/spec/models/form/lettings/questions/managing_organisation_spec.rb index 9977f07be..9d289d455 100644 --- a/spec/models/form/lettings/questions/managing_organisation_spec.rb +++ b/spec/models/form/lettings/questions/managing_organisation_spec.rb @@ -52,86 +52,97 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do end end - context "when user not support and owns own stock" do - let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: true)) } + context "when user is not support" do + let(:user_org) { create(:organisation, name: "User org") } + let(:user) { create(:user, :data_coordinator, organisation: user_org) } - let(:log) { create(:lettings_log) } - let!(:org_rel1) { create(:organisation_relationship, parent_organisation: user.organisation) } - let!(:org_rel2) { create(:organisation_relationship, parent_organisation: user.organisation) } + let(:managing_org1) { create(:organisation, name: "Managing org 1") } + let(:managing_org2) { create(:organisation, name: "Managing org 2") } + let(:managing_org3) { create(:organisation, name: "Managing org 3") } - let(:options) do - { - "" => "Select an option", - user.organisation.id => "#{user.organisation.name} (Your organisation)", - org_rel1.child_organisation.id => org_rel1.child_organisation.name, - org_rel2.child_organisation.id => org_rel2.child_organisation.name, - } - end - - it "shows managing agents with own org at the top" do - expect(question.displayed_answer_options(log, user)).to eq(options) - end - end - - context "when user not support and does not own stock" do - let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: false)) } - - let(:log) { create(:lettings_log) } - let!(:org_rel1) { create(:organisation_relationship, parent_organisation: user.organisation) } - let!(:org_rel2) { create(:organisation_relationship, parent_organisation: user.organisation) } + let(:log) { create(:lettings_log, managing_organisation: managing_org1) } + let!(:org_rel1) { create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: managing_org2) } + let!(:org_rel2) { create(:organisation_relationship, parent_organisation: user.organisation, child_organisation: managing_org3) } let(:options) do { "" => "Select an option", - user.organisation.id => "#{user.organisation.name} (Your organisation)", - org_rel1.child_organisation.id => org_rel1.child_organisation.name, - org_rel2.child_organisation.id => org_rel2.child_organisation.name, + log.managing_organisation.id => "Managing org 1", + user.organisation.id => "User org (Your organisation)", + org_rel1.child_organisation.id => "Managing org 2", + org_rel2.child_organisation.id => "Managing org 3", } end - it "shows managing agents with own org at the top" do + it "shows current managing agent at top, followed by user's org (with hint), followed by the managing agents of the user's org" do expect(question.displayed_answer_options(log, user)).to eq(options) end end - context "when support user and org does not own own stock" do + context "when user is support" do let(:user) { create(:user, :support) } - let(:log_owning_org) { create(:organisation, holds_own_stock: false) } - let(:log) { create(:lettings_log, owning_organisation: log_owning_org) } - let!(:org_rel1) { create(:organisation_relationship, parent_organisation: log_owning_org) } - let!(:org_rel2) { create(:organisation_relationship, parent_organisation: log_owning_org) } - - let(:options) do - { - "" => "Select an option", - org_rel1.child_organisation.id => org_rel1.child_organisation.name, - org_rel2.child_organisation.id => org_rel2.child_organisation.name, - } + let(:log_owning_org) { create(:organisation, name: "Owning org") } + + let(:managing_org1) { create(:organisation, name: "Managing org 1") } + let(:managing_org2) { create(:organisation, name: "Managing org 2") } + let(:managing_org3) { create(:organisation, name: "Managing org 3") } + + let(:log) { create(:lettings_log, owning_organisation: log_owning_org, managing_organisation: managing_org1, created_by: nil) } + let!(:org_rel1) { create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: managing_org2) } + let!(:org_rel2) { create(:organisation_relationship, parent_organisation: log_owning_org, child_organisation: managing_org3) } + + context "when org owns stock" do + let(:options) do + { + "" => "Select an option", + log.managing_organisation.id => "Managing org 1", + log_owning_org.id => "Owning org (Owning organisation)", + org_rel1.child_organisation.id => "Managing org 2", + org_rel2.child_organisation.id => "Managing org 3", + } + end + + it "shows current managing agent at top, followed by the current owning organisation (with hint), followed by the managing agents of the current owning organisation" do + log_owning_org.update!(holds_own_stock: true) + expect(question.displayed_answer_options(log, user)).to eq(options) + end end - it "shows owning org managing agents with hint" do - expect(question.displayed_answer_options(log, user)).to eq(options) + context "when org does not own stock" do + let(:options) do + { + "" => "Select an option", + log.managing_organisation.id => "Managing org 1", + org_rel1.child_organisation.id => "Managing org 2", + org_rel2.child_organisation.id => "Managing org 3", + } + end + + it "shows current managing agent at top, followed by the managing agents of the current owning organisation" do + log_owning_org.update!(holds_own_stock: false) + expect(question.displayed_answer_options(log, user)).to eq(options) + end end end - context "when support user and org does own stock" do + context "when the owning-managing organisation relationship is deleted" do let(:user) { create(:user, :support) } - let(:log_owning_org) { create(:organisation, holds_own_stock: true) } - let(:log) { create(:lettings_log, owning_organisation: log_owning_org) } - let!(:org_rel1) { create(:organisation_relationship, parent_organisation: log_owning_org) } - let!(:org_rel2) { create(:organisation_relationship, parent_organisation: log_owning_org) } + + let(:owning_org) { create(:organisation, name: "Owning org", holds_own_stock: true) } + let(:managing_org) { create(:organisation, name: "Managing org", holds_own_stock: false) } + let(:org_rel) { create(:organisation_relationship, parent_organisation: owning_org, child_organisation: managing_org) } + let(:log) { create(:lettings_log, owning_organisation: owning_org, managing_organisation: managing_org, created_by: nil) } let(:options) do { "" => "Select an option", - log_owning_org.id => "#{log_owning_org.name} (Owning organisation)", - org_rel1.child_organisation.id => org_rel1.child_organisation.name, - org_rel2.child_organisation.id => org_rel2.child_organisation.name, + owning_org.id => "Owning org (Owning organisation)", + managing_org.id => "Managing org", } end - it "shows owning org managing agents - " do + it "doesn't remove the managing org from the list of allowed managing orgs" do + org_rel.destroy! expect(question.displayed_answer_options(log, user)).to eq(options) end end diff --git a/spec/models/form/lettings/questions/stock_owner_spec.rb b/spec/models/form/lettings/questions/stock_owner_spec.rb index f08318f02..41aebed89 100644 --- a/spec/models/form/lettings/questions/stock_owner_spec.rb +++ b/spec/models/form/lettings/questions/stock_owner_spec.rb @@ -42,28 +42,68 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do end end - context "when user not support and owns own stock" do - let(:user) { create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: true)) } - let(:options) do - { - "" => "Select an option", - user.organisation.id => "#{user.organisation.name} (Your organisation)", - } - end + context "when user is not support" do + let(:user_org) { create(:organisation, name: "User org") } + let(:user) { create(:user, :data_coordinator, organisation: user_org) } + + let(:owning_org_1) { create(:organisation, name: "Owning org 1") } + let(:owning_org_2) { create(:organisation, name: "Owning org 2") } + let!(:org_rel) { create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org_2) } + let(:log) { create(:lettings_log, owning_organisation: owning_org_1) } + + context "when user's org owns stock" do + let(:options) do + { + "" => "Select an option", + owning_org_1.id => "Owning org 1", + user.organisation.id => "User org (Your organisation)", + owning_org_2.id => "Owning org 2", + } + end - before do - question.current_user = user + it "shows current stock owner at top, followed by user's org (with hint), followed by the stock owners of the user's org" do + user.organisation.update!(holds_own_stock: true) + expect(question.displayed_answer_options(log, user)).to eq(options) + end + + context "when the owning-managing organisation relationship is deleted" do + let(:options) do + { + "" => "Select an option", + user.organisation.id => "User org (Your organisation)", + owning_org_2.id => "Owning org 2", + } + end + + it "doesn't remove the housing provider from the list of allowed housing providers" do + log.update!(owning_organisation: owning_org_2) + expect(question.displayed_answer_options(log, user)).to eq(options) + org_rel.destroy! + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end end - it "shows stock owners with own org at the top" do - expect(question.answer_options).to eq(options) + context "when user's org doesn't own stock" do + let(:options) do + { + "" => "Select an option", + owning_org_1.id => "Owning org 1", + owning_org_2.id => "Owning org 2", + } + end + + it "shows current stock owner at top, followed by the stock owners of the user's org" do + user.organisation.update!(holds_own_stock: false) + expect(question.displayed_answer_options(log, user)).to eq(options) + end end end - context "when user support" do - before do - question.current_user = create(:user, :support) - end + context "when user is support" do + let(:user) { create(:user, :support) } + + let(:log) { create(:lettings_log) } let(:expected_opts) do Organisation.all.each_with_object(options) do |organisation, hsh| @@ -73,7 +113,7 @@ RSpec.describe Form::Lettings::Questions::StockOwner, type: :model do end it "shows all orgs" do - expect(question.answer_options).to eq(expected_opts) + expect(question.displayed_answer_options(log, user)).to eq(expected_opts) end end end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 862f82fa1..dc009a16d 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -143,6 +143,64 @@ RSpec.describe LettingsLogsController, type: :request do expect(whodunnit_actor).to be_a(User) expect(whodunnit_actor.id).to eq(user.id) end + + context "when creating a new log" do + context "when the user is support" do + let(:organisation) { FactoryBot.create(:organisation) } + let(:support_user) { FactoryBot.create(:user, :support, organisation:) } + + before do + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in support_user + post "/lettings-logs", headers: + end + + it "sets the managing org and stock-owning org as nil" do + created_id = response.location.match(/[0-9]+/)[0] + lettings_log = LettingsLog.find_by(id: created_id) + expect(lettings_log.owning_organisation).to eq(nil) + expect(lettings_log.managing_organisation).to eq(nil) + end + end + + context "when the user is not support" do + context "when the user's org holds stock" do + let(:organisation) { FactoryBot.create(:organisation, name: "User org", holds_own_stock: true) } + let(:user) { FactoryBot.create(:user, :data_coordinator, organisation:) } + + before do + RequestHelper.stub_http_requests + sign_in user + post "/lettings-logs", headers: + end + + it "sets the managing org and stock-owning org as the user's org" do + created_id = response.location.match(/[0-9]+/)[0] + lettings_log = LettingsLog.find_by(id: created_id) + expect(lettings_log.owning_organisation.name).to eq("User org") + expect(lettings_log.managing_organisation.name).to eq("User org") + end + end + + context "when the user's org doesn't hold stock" do + let(:organisation) { FactoryBot.create(:organisation, name: "User org", holds_own_stock: false) } + let(:user) { FactoryBot.create(:user, :data_coordinator, organisation:) } + + before do + RequestHelper.stub_http_requests + sign_in user + post "/lettings-logs", headers: + end + + it "sets the managing org as the user's org but the stock-owning org as nil" do + created_id = response.location.match(/[0-9]+/)[0] + lettings_log = LettingsLog.find_by(id: created_id) + expect(lettings_log.owning_organisation).to eq(nil) + expect(lettings_log.managing_organisation.name).to eq("User org") + end + end + end + end end end From 7e176bba34d15220f18f5561ffac203b0ce78e4b Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 24 Jan 2023 10:17:28 +0000 Subject: [PATCH 05/10] CLDC-845 Add sales household details validations (#1191) * Add min and max for age 1 validation * Add retirement value check column to the sales table * Add retirement value check question and page * Add retirement value check pages to household characteristics * Reuse soft validations for retirement in sales * Update person_indexes * Add old_persons_shared_ownership validation * Display the retirement value check in correct places depending on the joinpur value * Update page IDs * It's always joint purchase if age 2 is displayed * lint * lint --- .../sales/pages/retirement_value_check.rb | 43 ++ app/models/form/sales/questions/age1.rb | 2 + .../sales/questions/retirement_value_check.rb | 24 + .../subsections/household_characteristics.rb | 33 + app/models/lettings_log.rb | 21 - app/models/log.rb | 21 + app/models/sales_log.rb | 21 + .../sales/household_validations.rb | 18 + app/models/validations/soft_validations.rb | 4 +- config/locales/en.yml | 3 +- ...402_add_retirement_value_check_to_sales.rb | 7 + db/schema.rb | 1 + .../pages/retirement_value_check_spec.rb | 603 ++++++++++++++++++ spec/models/form/sales/questions/age1_spec.rb | 8 + .../questions/retirement_value_check_spec.rb | 61 ++ .../household_characteristics_spec.rb | 33 + spec/models/form_handler_spec.rb | 4 +- .../sales/household_validations_spec.rb | 115 ++++ 18 files changed, 996 insertions(+), 26 deletions(-) create mode 100644 app/models/form/sales/pages/retirement_value_check.rb create mode 100644 app/models/form/sales/questions/retirement_value_check.rb create mode 100644 db/migrate/20230116124402_add_retirement_value_check_to_sales.rb create mode 100644 spec/models/form/sales/pages/retirement_value_check_spec.rb create mode 100644 spec/models/form/sales/questions/retirement_value_check_spec.rb diff --git a/app/models/form/sales/pages/retirement_value_check.rb b/app/models/form/sales/pages/retirement_value_check.rb new file mode 100644 index 000000000..ae8389acf --- /dev/null +++ b/app/models/form/sales/pages/retirement_value_check.rb @@ -0,0 +1,43 @@ +class Form::Sales::Pages::RetirementValueCheck < Form::Sales::Pages::Person + def initialize(id, hsh, subsection, person_index:) + super + @depends_on = [ + { + "person_#{person_index}_retired_under_soft_min_age?" => true, + "jointpur" => joint_purchase? ? 1 : 2, + }, + ] + @person_index = person_index + @title_text = { + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_#{person_index}", + "label" => false, + "i18n_template" => "age", + }, + ], + } + @informative_text = { + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_#{person_index}", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_#{person_index}", + "label" => false, + "i18n_template" => "age", + }, + ], + } + end + + def questions + @questions ||= [ + Form::Sales::Questions::RetirementValueCheck.new(nil, nil, self, person_index: @person_index), + ] + end +end diff --git a/app/models/form/sales/questions/age1.rb b/app/models/form/sales/questions/age1.rb index 2ac7a9e3f..94d40100f 100644 --- a/app/models/form/sales/questions/age1.rb +++ b/app/models/form/sales/questions/age1.rb @@ -17,5 +17,7 @@ class Form::Sales::Questions::Age1 < ::Form::Question }, ] @check_answers_card_number = 1 + @min = 16 + @max = 110 end end diff --git a/app/models/form/sales/questions/retirement_value_check.rb b/app/models/form/sales/questions/retirement_value_check.rb new file mode 100644 index 000000000..56573490e --- /dev/null +++ b/app/models/form/sales/questions/retirement_value_check.rb @@ -0,0 +1,24 @@ +class Form::Sales::Questions::RetirementValueCheck < ::Form::Question + def initialize(id, hsh, page, person_index:) + super(id, hsh, page) + @id = "retirement_value_check" + @check_answer_label = "Retirement confirmation" + @type = "interruption_screen" + @answer_options = { + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + } + @hidden_in_check_answers = { + "depends_on" => [ + { + "retirement_value_check" => 0, + }, + { + "retirement_value_check" => 1, + }, + ], + } + @check_answers_card_number = person_index + @header = "Are you sure this person is retired?" + end +end diff --git a/app/models/form/sales/subsections/household_characteristics.rb b/app/models/form/sales/subsections/household_characteristics.rb index 38a71cf41..b4c8badcf 100644 --- a/app/models/form/sales/subsections/household_characteristics.rb +++ b/app/models/form/sales/subsections/household_characteristics.rb @@ -11,7 +11,11 @@ class Form::Sales::Subsections::HouseholdCharacteristics < ::Form::Subsection Form::Sales::Pages::BuyerInterview.new(nil, nil, self), Form::Sales::Pages::PrivacyNotice.new(nil, nil, self), Form::Sales::Pages::Age1.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("age_1_retirement_value_check", nil, self, person_index: 1), + Form::Sales::Pages::RetirementValueCheck.new("age_1_retirement_value_check_joint_purchase", nil, self, person_index: 1), Form::Sales::Pages::GenderIdentity1.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("gender_1_retirement_value_check", nil, self, person_index: 1), + Form::Sales::Pages::RetirementValueCheck.new("gender_1_retirement_value_check_joint_purchase", nil, self, person_index: 1), Form::Sales::Pages::Buyer1EthnicGroup.new(nil, nil, self), Form::Sales::Pages::Buyer1EthnicBackgroundBlack.new(nil, nil, self), Form::Sales::Pages::Buyer1EthnicBackgroundAsian.new(nil, nil, self), @@ -20,12 +24,17 @@ class Form::Sales::Subsections::HouseholdCharacteristics < ::Form::Subsection Form::Sales::Pages::Buyer1EthnicBackgroundWhite.new(nil, nil, self), Form::Sales::Pages::Nationality1.new(nil, nil, self), Form::Sales::Pages::Buyer1WorkingSituation.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_1_retirement_value_check", nil, self, person_index: 1), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_1_retirement_value_check_joint_purchase", nil, self, person_index: 1), Form::Sales::Pages::Buyer1IncomeValueCheck.new("working_situation_buyer_1_income_value_check", nil, self), Form::Sales::Pages::Buyer1LiveInProperty.new(nil, nil, self), Form::Sales::Pages::Buyer2RelationshipToBuyer1.new(nil, nil, self), Form::Sales::Pages::Age2.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("age_2_retirement_value_check_joint_purchase", nil, self, person_index: 2), Form::Sales::Pages::GenderIdentity2.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("gender_2_retirement_value_check_joint_purchase", nil, self, person_index: 2), Form::Sales::Pages::Buyer2WorkingSituation.new(nil, nil, self), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_2_retirement_value_check_joint_purchase", nil, self, person_index: 2), Form::Sales::Pages::Buyer2LiveInProperty.new(nil, nil, self), Form::Sales::Pages::NumberOfOthersInProperty.new(nil, nil, self), Form::Sales::Pages::PersonKnown.new("person_1_known", nil, self, person_index: 2), @@ -33,41 +42,65 @@ class Form::Sales::Subsections::HouseholdCharacteristics < ::Form::Subsection Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_1_relationship_to_buyer_1", nil, self, person_index: 2), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_1_relationship_to_buyer_1_joint_purchase", nil, self, person_index: 3), Form::Sales::Pages::PersonAge.new("person_1_age", nil, self, person_index: 2), + Form::Sales::Pages::RetirementValueCheck.new("age_2_retirement_value_check", nil, self, person_index: 2), Form::Sales::Pages::PersonAge.new("person_1_age_joint_purchase", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("age_3_retirement_value_check_joint_purchase", nil, self, person_index: 3), Form::Sales::Pages::PersonGenderIdentity.new("person_1_gender_identity", nil, self, person_index: 2), + Form::Sales::Pages::RetirementValueCheck.new("gender_2_retirement_value_check", nil, self, person_index: 2), Form::Sales::Pages::PersonGenderIdentity.new("person_1_gender_identity_joint_purchase", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("gender_3_retirement_value_check_joint_purchase", nil, self, person_index: 3), Form::Sales::Pages::PersonWorkingSituation.new("person_1_working_situation", nil, self, person_index: 2), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_2_retirement_value_check", nil, self, person_index: 2), Form::Sales::Pages::PersonWorkingSituation.new("person_1_working_situation_joint_purchase", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_3_retirement_value_check_joint_purchase", nil, self, person_index: 3), Form::Sales::Pages::PersonKnown.new("person_2_known", nil, self, person_index: 3), Form::Sales::Pages::PersonKnown.new("person_2_known_joint_purchase", nil, self, person_index: 4), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_2_relationship_to_buyer_1", nil, self, person_index: 3), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_2_relationship_to_buyer_1_joint_purchase", nil, self, person_index: 4), Form::Sales::Pages::PersonAge.new("person_2_age", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("age_3_retirement_value_check", nil, self, person_index: 3), Form::Sales::Pages::PersonAge.new("person_2_age_joint_purchase", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("age_4_retirement_value_check_joint_purchase", nil, self, person_index: 4), Form::Sales::Pages::PersonGenderIdentity.new("person_2_gender_identity", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("gender_3_retirement_value_check", nil, self, person_index: 3), Form::Sales::Pages::PersonGenderIdentity.new("person_2_gender_identity_joint_purchase", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("gender_4_retirement_value_check_joint_purchase", nil, self, person_index: 4), Form::Sales::Pages::PersonWorkingSituation.new("person_2_working_situation", nil, self, person_index: 3), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_3_retirement_value_check", nil, self, person_index: 3), Form::Sales::Pages::PersonWorkingSituation.new("person_2_working_situation_joint_purchase", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_4_retirement_value_check_joint_purchase", nil, self, person_index: 4), Form::Sales::Pages::PersonKnown.new("person_3_known", nil, self, person_index: 4), Form::Sales::Pages::PersonKnown.new("person_3_known_joint_purchase", nil, self, person_index: 5), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_3_relationship_to_buyer_1", nil, self, person_index: 4), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_3_relationship_to_buyer_1_joint_purchase", nil, self, person_index: 5), Form::Sales::Pages::PersonAge.new("person_3_age", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("age_4_retirement_value_check", nil, self, person_index: 4), Form::Sales::Pages::PersonAge.new("person_3_age_joint_purchase", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("age_5_retirement_value_check_joint_purchase", nil, self, person_index: 5), Form::Sales::Pages::PersonGenderIdentity.new("person_3_gender_identity", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("gender_4_retirement_value_check", nil, self, person_index: 4), Form::Sales::Pages::PersonGenderIdentity.new("person_3_gender_identity_joint_purchase", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("gender_5_retirement_value_check_joint_purchase", nil, self, person_index: 5), Form::Sales::Pages::PersonWorkingSituation.new("person_3_working_situation", nil, self, person_index: 4), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_4_retirement_value_check", nil, self, person_index: 4), Form::Sales::Pages::PersonWorkingSituation.new("person_3_working_situation_joint_purchase", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_5_retirement_value_check_joint_purchase", nil, self, person_index: 5), Form::Sales::Pages::PersonKnown.new("person_4_known", nil, self, person_index: 5), Form::Sales::Pages::PersonKnown.new("person_4_known_joint_purchase", nil, self, person_index: 6), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_4_relationship_to_buyer_1", nil, self, person_index: 5), Form::Sales::Pages::PersonRelationshipToBuyer1.new("person_4_relationship_to_buyer_1_joint_purchase", nil, self, person_index: 6), Form::Sales::Pages::PersonAge.new("person_4_age", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("age_5_retirement_value_check", nil, self, person_index: 5), Form::Sales::Pages::PersonAge.new("person_4_age_joint_purchase", nil, self, person_index: 6), + Form::Sales::Pages::RetirementValueCheck.new("age_6_retirement_value_check_joint_purchase", nil, self, person_index: 6), Form::Sales::Pages::PersonGenderIdentity.new("person_4_gender_identity", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("gender_5_retirement_value_check", nil, self, person_index: 5), Form::Sales::Pages::PersonGenderIdentity.new("person_4_gender_identity_joint_purchase", nil, self, person_index: 6), + Form::Sales::Pages::RetirementValueCheck.new("gender_6_retirement_value_check_joint_purchase", nil, self, person_index: 6), Form::Sales::Pages::PersonWorkingSituation.new("person_4_working_situation", nil, self, person_index: 5), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_5_retirement_value_check", nil, self, person_index: 5), Form::Sales::Pages::PersonWorkingSituation.new("person_4_working_situation_joint_purchase", nil, self, person_index: 6), + Form::Sales::Pages::RetirementValueCheck.new("working_situation_6_retirement_value_check_joint_purchase", nil, self, person_index: 6), ] end end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index d78495016..04b4b4ec9 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -456,16 +456,6 @@ class LettingsLog < Log OPTIONAL_FIELDS + dynamically_not_required end - (1..8).each do |person_num| - define_method("retirement_age_for_person_#{person_num}") do - retirement_age_for_person(person_num) - end - - define_method("plural_gender_for_person_#{person_num}") do - plural_gender_for_person(person_num) - end - end - def retirement_age_for_person(person_num) gender = public_send("sex#{person_num}".to_sym) return unless gender @@ -473,17 +463,6 @@ class LettingsLog < Log RETIREMENT_AGES[gender] end - def plural_gender_for_person(person_num) - gender = public_send("sex#{person_num}".to_sym) - return unless gender - - if %w[M X].include?(gender) - "male and non-binary people" - elsif gender == "F" - "females" - end - end - def age_known?(person_num) return false unless person_num.is_a?(Integer) diff --git a/app/models/log.rb b/app/models/log.rb index ea8a637af..d10211577 100644 --- a/app/models/log.rb +++ b/app/models/log.rb @@ -62,8 +62,29 @@ class Log < ApplicationRecord end end + (1..8).each do |person_num| + define_method("retirement_age_for_person_#{person_num}") do + retirement_age_for_person(person_num) + end + + define_method("plural_gender_for_person_#{person_num}") do + plural_gender_for_person(person_num) + end + end + private + def plural_gender_for_person(person_num) + gender = public_send("sex#{person_num}".to_sym) + return unless gender + + if %w[M X].include?(gender) + "male and non-binary people" + elsif gender == "F" + "females" + end + end + def update_status! self.status = if all_fields_completed? && errors.empty? "completed" diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index cdf2964f1..45bd9f729 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -16,6 +16,7 @@ end class SalesLog < Log include DerivedVariables::SalesLogVariables include Validations::Sales::SoftValidations + include Validations::SoftValidations self.inheritance_column = :_type_disabled @@ -33,6 +34,7 @@ class SalesLog < Log scope :search_by, ->(param) { filter_by_id(param) } OPTIONAL_FIELDS = %w[purchid].freeze + RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze def startdate saledate @@ -181,4 +183,23 @@ class SalesLog < Log self.postcode_full = upcase_and_remove_whitespace(postcode_full) process_postcode(postcode_full, "pcodenk", "is_la_inferred", "la") end + + def retirement_age_for_person(person_num) + gender = public_send("sex#{person_num}".to_sym) + return unless gender + + RETIREMENT_AGES[gender] + end + + def joint_purchase? + jointpur == 1 + end + + def not_joint_purchase? + jointpur == 2 + end + + def old_persons_shared_ownership? + type == 24 + end end diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index 6e16a0476..8ee00fdfd 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -18,6 +18,20 @@ module Validations::Sales::HouseholdValidations shared_validate_partner_count(record, 6) end + def validate_buyers_age_for_old_persons_shared_ownership(record) + if record.old_persons_shared_ownership? + if record.joint_purchase? && ages_unknown_or_under_64?(record, [1, 2]) + record.errors.add :age1, I18n.t("validations.household.old_persons_shared_ownership") + record.errors.add :age2, I18n.t("validations.household.old_persons_shared_ownership") + record.errors.add :type, I18n.t("validations.household.old_persons_shared_ownership") + end + if record.not_joint_purchase? && ages_unknown_or_under_64?(record, [1]) + record.errors.add :age1, I18n.t("validations.household.old_persons_shared_ownership") + record.errors.add :type, I18n.t("validations.household.old_persons_shared_ownership") + end + end + end + private def validate_person_age_matches_relationship(record, person_num) @@ -97,4 +111,8 @@ private def tenant_is_economic_child?(economic_status) economic_status == 9 end + + def ages_unknown_or_under_64?(record, person_indexes) + person_indexes.all? { |person_num| record["age#{person_num}"].present? && record["age#{person_num}"] < 64 || record["age#{person_num}_known"] == 1 } + end end diff --git a/app/models/validations/soft_validations.rb b/app/models/validations/soft_validations.rb index 944fff022..fc2c469ca 100644 --- a/app/models/validations/soft_validations.rb +++ b/app/models/validations/soft_validations.rb @@ -111,7 +111,7 @@ private gender = public_send("sex#{person_num}") return unless age && economic_status && gender - %w[M X].include?(gender) && tenant_is_retired?(economic_status) && age < 67 || + %w[M X].include?(gender) && tenant_is_retired?(economic_status) && age < retirement_age_for_person(person_num) || gender == "F" && tenant_is_retired?(economic_status) && age < 60 end @@ -122,7 +122,7 @@ private tenant_retired_or_prefers_not_say = tenant_is_retired?(economic_status) || tenant_prefers_not_to_say?(economic_status) return unless age && economic_status && gender - %w[M X].include?(gender) && !tenant_retired_or_prefers_not_say && age > 67 || + %w[M X].include?(gender) && !tenant_retired_or_prefers_not_say && age > retirement_age_for_person(person_num) || gender == "F" && !tenant_retired_or_prefers_not_say && age > 60 end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 73bd8bdf7..ac6a2fa7f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -367,7 +367,8 @@ en: not_internal_transfer: "Answer cannot be ‘permanently decanted from another property owned by this landlord’ as you told us the source of referral for this tenancy was not an internal transfer" condition_effects: no_choices: "You cannot answer this question as you told us nobody in the household has a physical or mental health condition (or other illness) expected to last 12 months or more" - + old_persons_shared_ownership: "Are you sure? At least one buyer should be aged over 64 for Older persons‘ shared ownership scheme" + tenancy: length: fixed_term_not_required: "You must only answer the length of the tenancy if it's fixed-term" diff --git a/db/migrate/20230116124402_add_retirement_value_check_to_sales.rb b/db/migrate/20230116124402_add_retirement_value_check_to_sales.rb new file mode 100644 index 000000000..714c5f6b3 --- /dev/null +++ b/db/migrate/20230116124402_add_retirement_value_check_to_sales.rb @@ -0,0 +1,7 @@ +class AddRetirementValueCheckToSales < ActiveRecord::Migration[7.0] + def change + change_table :sales_logs, bulk: true do |t| + t.column :retirement_value_check, :integer + end + end +end diff --git a/db/schema.rb b/db/schema.rb index e05334946..91c6bed37 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -503,6 +503,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_01_16_151942) do t.integer "hodate_check" t.bigint "bulk_upload_id" t.index ["bulk_upload_id"], name: "index_sales_logs_on_bulk_upload_id" + t.integer "retirement_value_check" t.index ["created_by_id"], name: "index_sales_logs_on_created_by_id" t.index ["managing_organisation_id"], name: "index_sales_logs_on_managing_organisation_id" t.index ["owning_organisation_id"], name: "index_sales_logs_on_owning_organisation_id" diff --git a/spec/models/form/sales/pages/retirement_value_check_spec.rb b/spec/models/form/sales/pages/retirement_value_check_spec.rb new file mode 100644 index 000000000..9c04ef25c --- /dev/null +++ b/spec/models/form/sales/pages/retirement_value_check_spec.rb @@ -0,0 +1,603 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Pages::RetirementValueCheck, type: :model do + subject(:page) { described_class.new(page_id, page_definition, subsection, person_index:) } + + let(:page_definition) { nil } + let(:subsection) { instance_double(Form::Subsection) } + let(:person_index) { 1 } + + let(:page_id) { "person_1_retirement_value_check" } + + it "has correct subsection" do + expect(page.subsection).to eq(subsection) + end + + it "has the correct header" do + expect(page.header).to be_nil + end + + it "has the correct description" do + expect(page.description).to be_nil + end + + context "with joint purchase" do + context "with person 1" do + let(:person_index) { 1 } + let(:page_id) { "person_1_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_1_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_1_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_1", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_1", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_1", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 2" do + let(:person_index) { 2 } + let(:page_id) { "person_2_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_2_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_2_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_2", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 3" do + let(:person_index) { 3 } + let(:page_id) { "person_3_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_3_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_3_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_3", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_3", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_3", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 4" do + let(:person_index) { 4 } + let(:page_id) { "person_4_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_4_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_4_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_4", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_4", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_4", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 5" do + let(:person_index) { 5 } + let(:page_id) { "person_5_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_5_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_5_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_5", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_5", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_5", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 6" do + let(:person_index) { 6 } + let(:page_id) { "person_6_retirement_value_check_joint_purchase" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_6_retirement_value_check_joint_purchase") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_6_retired_under_soft_min_age?" => true, "jointpur" => 1 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_6", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_6", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_6", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + end + + context "without joint purchase" do + context "with person 1" do + let(:person_index) { 1 } + let(:page_id) { "person_1_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_1_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_1_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_1", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_1", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_1", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 2" do + let(:person_index) { 2 } + let(:page_id) { "person_2_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_2_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_2_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_2", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 3" do + let(:person_index) { 2 } + let(:page_id) { "person_3_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_3_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_2_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_2", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_2", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 4" do + let(:person_index) { 3 } + let(:page_id) { "person_4_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_4_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_3_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_3", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_3", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_3", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 5" do + let(:person_index) { 4 } + let(:page_id) { "person_5_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_5_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_4_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_4", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_4", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_4", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + + context "with person 6" do + let(:person_index) { 5 } + let(:page_id) { "person_6_retirement_value_check" } + + it "has correct questions" do + expect(page.questions.map(&:id)).to eq(%w[retirement_value_check]) + end + + it "has the correct id" do + expect(page.id).to eq("person_6_retirement_value_check") + end + + it "has correct depends_on" do + expect(page.depends_on).to eq([{ "person_5_retired_under_soft_min_age?" => true, "jointpur" => 2 }]) + end + + it "has correct title_text" do + expect(page.title_text).to eq({ + "translation" => "soft_validations.retirement.min.title", + "arguments" => [ + { + "key" => "retirement_age_for_person_5", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + + it "has correct informative_text" do + expect(page.informative_text).to eq({ + "translation" => "soft_validations.retirement.min.hint_text", + "arguments" => [ + { + "key" => "plural_gender_for_person_5", + "label" => false, + "i18n_template" => "gender", + }, + { + "key" => "retirement_age_for_person_5", + "label" => false, + "i18n_template" => "age", + }, + ], + }) + end + end + end +end diff --git a/spec/models/form/sales/questions/age1_spec.rb b/spec/models/form/sales/questions/age1_spec.rb index b5ab5d44b..33fe91c82 100644 --- a/spec/models/form/sales/questions/age1_spec.rb +++ b/spec/models/form/sales/questions/age1_spec.rb @@ -55,4 +55,12 @@ RSpec.describe Form::Sales::Questions::Age1, type: :model do it "has the correct check_answers_card_number" do expect(question.check_answers_card_number).to eq(1) end + + it "has the correct min" do + expect(question.min).to eq(16) + end + + it "has the correct max" do + expect(question.max).to eq(110) + end end diff --git a/spec/models/form/sales/questions/retirement_value_check_spec.rb b/spec/models/form/sales/questions/retirement_value_check_spec.rb new file mode 100644 index 000000000..318faa5d9 --- /dev/null +++ b/spec/models/form/sales/questions/retirement_value_check_spec.rb @@ -0,0 +1,61 @@ +require "rails_helper" + +RSpec.describe Form::Sales::Questions::RetirementValueCheck, type: :model do + subject(:question) { described_class.new(question_id, question_definition, page, person_index: 1) } + + let(:question_id) { nil } + let(:question_definition) { nil } + let(:page) { instance_double(Form::Page) } + + it "has correct page" do + expect(question.page).to eq(page) + end + + it "has the correct id" do + expect(question.id).to eq("retirement_value_check") + end + + it "has the correct header" do + expect(question.header).to eq("Are you sure this person is retired?") + end + + it "has the correct check_answer_label" do + expect(question.check_answer_label).to eq("Retirement confirmation") + end + + it "has the correct type" do + expect(question.type).to eq("interruption_screen") + end + + it "is not marked as derived" do + expect(question.derived?).to be false + end + + it "has the correct hint" do + expect(question.hint_text).to be_nil + end + + it "has a correct check_answers_card_number" do + expect(question.check_answers_card_number).to eq(1) + end + + it "has the correct answer_options" do + expect(question.answer_options).to eq({ + "0" => { "value" => "Yes" }, + "1" => { "value" => "No" }, + }) + end + + it "has the correct hidden_in_check_answers" do + expect(question.hidden_in_check_answers).to eq({ + "depends_on" => [ + { + "retirement_value_check" => 0, + }, + { + "retirement_value_check" => 1, + }, + ], + }) + end +end diff --git a/spec/models/form/sales/subsections/household_characteristics_spec.rb b/spec/models/form/sales/subsections/household_characteristics_spec.rb index 15c8be02f..34b1a7536 100644 --- a/spec/models/form/sales/subsections/household_characteristics_spec.rb +++ b/spec/models/form/sales/subsections/household_characteristics_spec.rb @@ -17,7 +17,11 @@ RSpec.describe Form::Sales::Subsections::HouseholdCharacteristics, type: :model buyer_interview privacy_notice buyer_1_age + age_1_retirement_value_check + age_1_retirement_value_check_joint_purchase buyer_1_gender_identity + gender_1_retirement_value_check + gender_1_retirement_value_check_joint_purchase buyer_1_ethnic_group buyer_1_ethnic_background_black buyer_1_ethnic_background_asian @@ -26,12 +30,17 @@ RSpec.describe Form::Sales::Subsections::HouseholdCharacteristics, type: :model buyer_1_ethnic_background_white buyer_1_nationality buyer_1_working_situation + working_situation_1_retirement_value_check + working_situation_1_retirement_value_check_joint_purchase working_situation_buyer_1_income_value_check buyer_1_live_in_property buyer_2_relationship_to_buyer_1 buyer_2_age + age_2_retirement_value_check_joint_purchase buyer_2_gender_identity + gender_2_retirement_value_check_joint_purchase buyer_2_working_situation + working_situation_2_retirement_value_check_joint_purchase buyer_2_live_in_property number_of_others_in_property person_1_known @@ -39,41 +48,65 @@ RSpec.describe Form::Sales::Subsections::HouseholdCharacteristics, type: :model person_1_relationship_to_buyer_1 person_1_relationship_to_buyer_1_joint_purchase person_1_age + age_2_retirement_value_check person_1_age_joint_purchase + age_3_retirement_value_check_joint_purchase person_1_gender_identity + gender_2_retirement_value_check person_1_gender_identity_joint_purchase + gender_3_retirement_value_check_joint_purchase person_1_working_situation + working_situation_2_retirement_value_check person_1_working_situation_joint_purchase + working_situation_3_retirement_value_check_joint_purchase person_2_known person_2_known_joint_purchase person_2_relationship_to_buyer_1 person_2_relationship_to_buyer_1_joint_purchase person_2_age + age_3_retirement_value_check person_2_age_joint_purchase + age_4_retirement_value_check_joint_purchase person_2_gender_identity + gender_3_retirement_value_check person_2_gender_identity_joint_purchase + gender_4_retirement_value_check_joint_purchase person_2_working_situation + working_situation_3_retirement_value_check person_2_working_situation_joint_purchase + working_situation_4_retirement_value_check_joint_purchase person_3_known person_3_known_joint_purchase person_3_relationship_to_buyer_1 person_3_relationship_to_buyer_1_joint_purchase person_3_age + age_4_retirement_value_check person_3_age_joint_purchase + age_5_retirement_value_check_joint_purchase person_3_gender_identity + gender_4_retirement_value_check person_3_gender_identity_joint_purchase + gender_5_retirement_value_check_joint_purchase person_3_working_situation + working_situation_4_retirement_value_check person_3_working_situation_joint_purchase + working_situation_5_retirement_value_check_joint_purchase person_4_known person_4_known_joint_purchase person_4_relationship_to_buyer_1 person_4_relationship_to_buyer_1_joint_purchase person_4_age + age_5_retirement_value_check person_4_age_joint_purchase + age_6_retirement_value_check_joint_purchase person_4_gender_identity + gender_5_retirement_value_check person_4_gender_identity_joint_purchase + gender_6_retirement_value_check_joint_purchase person_4_working_situation + working_situation_5_retirement_value_check person_4_working_situation_joint_purchase + working_situation_6_retirement_value_check_joint_purchase ], ) end diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index 1e0483fbc..c272ac4a9 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -52,14 +52,14 @@ RSpec.describe FormHandler do it "is able to load a current sales form" do form = form_handler.get_form("current_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(149) + expect(form.pages.count).to eq(182) expect(form.name).to eq("2022_2023_sales") end it "is able to load a previous sales form" do form = form_handler.get_form("previous_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(149) + expect(form.pages.count).to eq(182) expect(form.name).to eq("2021_2022_sales") end end diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index bdfb01ea4..61a204878 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -164,5 +164,120 @@ RSpec.describe Validations::Sales::HouseholdValidations do expect(record.errors["ecstat2"]) .to include(match I18n.t("validations.household.ecstat.student_16_19.cannot_be_student.child_not_16_19")) end + + context "when it is a joint purchase and both buyers are over 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1: 65, age2: 66, type: 24) } + + it "does not add an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors).not_to be_present + end + end + + context "when it is a joint purchase and first buyer is over 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1: 65, age2: 40, type: 24) } + + it "does not add an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors).not_to be_present + end + end + + context "when it is a joint purchase and second buyer is over 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1: 43, age2: 64, type: 24) } + + it "does not add an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors).not_to be_present + end + end + + context "when it is a joint purchase and neither of the buyers are over 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1: 43, age2: 33, type: 24) } + + it "adds an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors["age1"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["age2"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["type"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + end + end + + context "when it is a joint purchase and first buyer is under 64 and the second buyers' age is unknown" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1: 43, age2_known: 1, type: 24) } + + it "adds an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors["age1"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["age2"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["type"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + end + end + + context "when it is a joint purchase and neither of the buyers ages are known" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 1, age1_known: 1, age2_known: 1, type: 24) } + + it "adds an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors["age1"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["age2"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["type"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + end + end + + context "when it is not a joint purchase and the buyer is over 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 2, age1: 70, type: 24) } + + it "does not add an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors).not_to be_present + end + end + + context "when it is not a joint purchase and the buyer is under 64" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 2, age1: 20, type: 24) } + + it "adds an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors["age1"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["age2"]) + .to be_empty + expect(record.errors["type"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + end + end + + context "when it is not a joint purchase and the buyers age is not known" do + let(:record) { FactoryBot.build(:sales_log, jointpur: 2, age1_known: 1, type: 24) } + + it "adds an error" do + household_validator.validate_buyers_age_for_old_persons_shared_ownership(record) + + expect(record.errors["age1"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + expect(record.errors["age2"]) + .to be_empty + expect(record.errors["type"]) + .to include(match I18n.t("validations.household.old_persons_shared_ownership")) + end + end end end From 1cced31e2708bfcf01b86a7cab3ecc6aa376dde0 Mon Sep 17 00:00:00 2001 From: Arthur Campbell <51094020+arfacamble@users.noreply.github.com> Date: Tue, 24 Jan 2023 11:50:18 +0000 Subject: [PATCH 06/10] hard validation on value of cash discount (#1221) --- .../form/sales/questions/deposit_discount.rb | 1 + .../sales/financial_validations.rb | 8 +++++++ config/locales/en.yml | 1 + .../sales/financial_validations_spec.rb | 22 +++++++++++++++++++ 4 files changed, 32 insertions(+) diff --git a/app/models/form/sales/questions/deposit_discount.rb b/app/models/form/sales/questions/deposit_discount.rb index dd3f98939..b23a3e1b4 100644 --- a/app/models/form/sales/questions/deposit_discount.rb +++ b/app/models/form/sales/questions/deposit_discount.rb @@ -6,6 +6,7 @@ class Form::Sales::Questions::DepositDiscount < ::Form::Question @header = "How much cash discount was given through Social HomeBuy?" @type = "numeric" @min = 0 + @max = 999_999 @width = 5 @prefix = "£" @hint_text = "Enter the total cash discount given on the property being purchased through the Social HomeBuy scheme" diff --git a/app/models/validations/sales/financial_validations.rb b/app/models/validations/sales/financial_validations.rb index 04f6a1d06..116ecf301 100644 --- a/app/models/validations/sales/financial_validations.rb +++ b/app/models/validations/sales/financial_validations.rb @@ -11,4 +11,12 @@ module Validations::Sales::FinancialValidations end end end + + def validate_cash_discount(record) + return unless record.cashdis + + unless record.cashdis.between?(0, 999_999) + record.errors.add :cashdis, I18n.t("validations.financial.cash_discount_invalid") + end + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index ac6a2fa7f..d84429637 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -274,6 +274,7 @@ en: carehome: out_of_range: "Household rent and other charges must be between %{min_chcharge} and %{max_chcharge} if paying %{period}" not_provided: "Enter how much rent and other charges the household pays %{period}" + cash_discount_invalid: "Cash discount must be £0 - £999,999" household: reasonpref: not_homeless: "Answer cannot be ‘homeless or about to lose their home’ as the tenant was not homeless immediately prior to this letting" diff --git a/spec/models/validations/sales/financial_validations_spec.rb b/spec/models/validations/sales/financial_validations_spec.rb index b736878dc..7b5b8868b 100644 --- a/spec/models/validations/sales/financial_validations_spec.rb +++ b/spec/models/validations/sales/financial_validations_spec.rb @@ -53,4 +53,26 @@ RSpec.describe Validations::Sales::FinancialValidations do end end end + + describe "#validate_cash_discount" do + let(:record) { FactoryBot.create(:sales_log) } + + it "adds an error if the cash discount is below zero" do + record.cashdis = -1 + financial_validator.validate_cash_discount(record) + expect(record.errors["cashdis"]).to include(match I18n.t("validations.financial.cash_discount_invalid")) + end + + it "adds an error if the cash discount is one million or more" do + record.cashdis = 1_000_000 + financial_validator.validate_cash_discount(record) + expect(record.errors["cashdis"]).to include(match I18n.t("validations.financial.cash_discount_invalid")) + end + + it "does not add an error if the cash discount is in the expected range" do + record.cashdis = 10_000 + financial_validator.validate_cash_discount(record) + expect(record.errors["cashdis"]).to be_empty + end + end end From 45b098ad4b55f5e5520a8330aa7613a0a2267a70 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 24 Jan 2023 13:27:44 +0000 Subject: [PATCH 07/10] CLDC-1457 Update checkbox validation message (#1207) --- app/models/form/sales/questions/buyers_organisations.rb | 4 ++++ spec/models/form/sales/questions/buyers_organisations_spec.rb | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/app/models/form/sales/questions/buyers_organisations.rb b/app/models/form/sales/questions/buyers_organisations.rb index 6457b6866..735a1563d 100644 --- a/app/models/form/sales/questions/buyers_organisations.rb +++ b/app/models/form/sales/questions/buyers_organisations.rb @@ -15,4 +15,8 @@ class Form::Sales::Questions::BuyersOrganisations < ::Form::Question "pregla" => { "value" => "Local Authority" }, "pregghb" => { "value" => "Help to Buy Agent" }, }.freeze + + def unanswered_error_message + "At least one option must be selected of these four" + end end diff --git a/spec/models/form/sales/questions/buyers_organisations_spec.rb b/spec/models/form/sales/questions/buyers_organisations_spec.rb index 14bb944f4..d9f61df5f 100644 --- a/spec/models/form/sales/questions/buyers_organisations_spec.rb +++ b/spec/models/form/sales/questions/buyers_organisations_spec.rb @@ -35,6 +35,10 @@ RSpec.describe Form::Sales::Questions::BuyersOrganisations, type: :model do expect(question.hint_text).to eq("Select all that apply") end + it "has the correct unanswered_error_message" do + expect(question.unanswered_error_message).to eq("At least one option must be selected of these four") + end + it "has the correct answer_options" do expect(question.answer_options).to eq( { From cf92c9559c6d1c4c474b349f06ffe9a8c8533631 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 24 Jan 2023 13:28:50 +0000 Subject: [PATCH 08/10] Add disability and wheelchair questions validation (#1215) * Add Page#interruption_screen? method * Use form helper method * Validate household disability questions * Update CYA --- app/controllers/form_controller.rb | 30 ++++++++----- app/models/form/page.rb | 4 ++ .../questions/household_wheelchair_check.rb | 1 + .../form/sales/subsections/household_needs.rb | 1 + .../validations/sales/soft_validations.rb | 4 +- spec/fixtures/forms/2021_2022.json | 2 +- spec/models/form/page_spec.rb | 16 +++++++ .../sales/subsections/household_needs_spec.rb | 1 + spec/models/form_handler_spec.rb | 4 +- .../sales/soft_validations_spec.rb | 45 +++++++++++++++++-- 10 files changed, 87 insertions(+), 21 deletions(-) diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index b0468c875..24e3d1b32 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -6,7 +6,7 @@ class FormController < ApplicationController def submit_form if @log - @page = @log.form.get_page(params[@log.model_name.param_key][:page]) + @page = form.get_page(params[@log.model_name.param_key][:page]) responses_for_page = responses_for_page(@page) mandatory_questions_with_no_response = mandatory_questions_with_no_response(responses_for_page) @@ -30,7 +30,7 @@ class FormController < ApplicationController def check_answers if @log current_url = request.env["PATH_INFO"] - subsection = @log.form.get_subsection(current_url.split("/")[-2]) + subsection = form.get_subsection(current_url.split("/")[-2]) render "form/check_answers", locals: { subsection:, current_user: } else render_not_found @@ -49,8 +49,8 @@ class FormController < ApplicationController if @log restore_error_field_values page_id = request.path.split("/")[-1].underscore - @page = @log.form.get_page(page_id) - @subsection = @log.form.subsection_for_page(@page) + @page = form.get_page(page_id) + @subsection = form.subsection_for_page(@page) if @page.routed_to?(@log, current_user) render "form/page" else @@ -71,7 +71,7 @@ private end if session["fields"] session["fields"].each do |field, value| - if @log.form.get_question(field, @log)&.type != "date" && @log.respond_to?(field) + if form.get_question(field, @log)&.type != "date" && @log.respond_to?(field) @log[field] = value end end @@ -129,20 +129,26 @@ private def successful_redirect_path if is_referrer_check_answers? - page_ids = @log.form.subsection_for_page(@page).pages.map(&:id) + page_ids = form.subsection_for_page(@page).pages.map(&:id) page_index = page_ids.index(@page.id) - next_page = @log.form.next_page(@page, @log, current_user) - previous_page = @log.form.previous_page(page_ids, page_index, @log, current_user) - if next_page.to_s.include?("value_check") || next_page == previous_page - return send("#{@log.class.name.underscore}_#{next_page}_path", @log, { referrer: "check_answers" }) + next_page_id = form.next_page(@page, @log, current_user) + next_page = form.get_page(next_page_id) + previous_page = form.previous_page(page_ids, page_index, @log, current_user) + + if next_page&.interruption_screen? || next_page_id == previous_page + return send("#{@log.class.name.underscore}_#{next_page_id}_path", @log, { referrer: "check_answers" }) else - return send("#{@log.model_name.param_key}_#{@log.form.subsection_for_page(@page).id}_check_answers_path", @log) + return send("#{@log.model_name.param_key}_#{form.subsection_for_page(@page).id}_check_answers_path", @log) end end - redirect_path = @log.form.next_page_redirect_path(@page, @log, current_user) + redirect_path = form.next_page_redirect_path(@page, @log, current_user) send(redirect_path, @log) end + def form + @log&.form + end + def mandatory_questions_with_no_response(responses_for_page) session["fields"] = {} calc_questions = @page.questions.map(&:result_field) diff --git a/app/models/form/page.rb b/app/models/form/page.rb index 33e944eed..ea57bac66 100644 --- a/app/models/form/page.rb +++ b/app/models/form/page.rb @@ -32,6 +32,10 @@ class Form::Page end end + def interruption_screen? + questions.all? { |question| question.type == "interruption_screen" } + end + private def conditional_question_ids diff --git a/app/models/form/sales/questions/household_wheelchair_check.rb b/app/models/form/sales/questions/household_wheelchair_check.rb index 1a9db3f3a..f2c15b9ac 100644 --- a/app/models/form/sales/questions/household_wheelchair_check.rb +++ b/app/models/form/sales/questions/household_wheelchair_check.rb @@ -19,5 +19,6 @@ class Form::Sales::Questions::HouseholdWheelchairCheck < ::Form::Question }, ], } + @page = page end end diff --git a/app/models/form/sales/subsections/household_needs.rb b/app/models/form/sales/subsections/household_needs.rb index d411d5101..b5d4d528e 100644 --- a/app/models/form/sales/subsections/household_needs.rb +++ b/app/models/form/sales/subsections/household_needs.rb @@ -12,6 +12,7 @@ class Form::Sales::Subsections::HouseholdNeeds < ::Form::Subsection Form::Sales::Pages::BuyerStillServing.new(nil, nil, self), Form::Sales::Pages::ArmedForcesSpouse.new(nil, nil, self), Form::Sales::Pages::HouseholdDisability.new(nil, nil, self), + Form::Sales::Pages::HouseholdWheelchairCheck.new("disability_wheelchair_check", nil, self), Form::Sales::Pages::HouseholdWheelchair.new(nil, nil, self), Form::Sales::Pages::HouseholdWheelchairCheck.new("wheelchair_check", nil, self), ] diff --git a/app/models/validations/sales/soft_validations.rb b/app/models/validations/sales/soft_validations.rb index 3deb92aaa..19c2a13ec 100644 --- a/app/models/validations/sales/soft_validations.rb +++ b/app/models/validations/sales/soft_validations.rb @@ -22,9 +22,9 @@ module Validations::Sales::SoftValidations end def wheelchair_when_not_disabled? - return false unless disabled == 2 + return unless disabled && wheel - wheel == 1 + wheel == 1 && disabled == 2 end def savings_over_soft_max? diff --git a/spec/fixtures/forms/2021_2022.json b/spec/fixtures/forms/2021_2022.json index 486bc126c..ec13bdabb 100644 --- a/spec/fixtures/forms/2021_2022.json +++ b/spec/fixtures/forms/2021_2022.json @@ -208,7 +208,7 @@ "check_answer_label": "Retirement age soft validation", "hidden_in_check_answers": true, "header": "Are you sure this person is retired?", - "type": "radio", + "type": "interruption_screen", "answer_options": { "0": { "value": "Yes" diff --git a/spec/models/form/page_spec.rb b/spec/models/form/page_spec.rb index bb448aca6..4a870fe46 100644 --- a/spec/models/form/page_spec.rb +++ b/spec/models/form/page_spec.rb @@ -47,6 +47,22 @@ RSpec.describe Form::Page, type: :model do end end + describe "#interruption_screen?" do + context "when it has regular questions" do + it "returns false" do + expect(page.interruption_screen?).to be false + end + end + + context "when it has interruption_screen question" do + let(:page) { form.get_page("retirement_value_check") } + + it "returns true" do + expect(page.interruption_screen?).to be true + end + end + end + context "with a lettings log" do let(:lettings_log) { FactoryBot.build(:lettings_log, :in_progress) } diff --git a/spec/models/form/sales/subsections/household_needs_spec.rb b/spec/models/form/sales/subsections/household_needs_spec.rb index 5220f2d05..5395da095 100644 --- a/spec/models/form/sales/subsections/household_needs_spec.rb +++ b/spec/models/form/sales/subsections/household_needs_spec.rb @@ -18,6 +18,7 @@ RSpec.describe Form::Sales::Subsections::HouseholdNeeds, type: :model do buyer_still_serving armed_forces_spouse household_disability + disability_wheelchair_check household_wheelchair wheelchair_check ], diff --git a/spec/models/form_handler_spec.rb b/spec/models/form_handler_spec.rb index c272ac4a9..965e79690 100644 --- a/spec/models/form_handler_spec.rb +++ b/spec/models/form_handler_spec.rb @@ -52,14 +52,14 @@ RSpec.describe FormHandler do it "is able to load a current sales form" do form = form_handler.get_form("current_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(182) + expect(form.pages.count).to eq(183) expect(form.name).to eq("2022_2023_sales") end it "is able to load a previous sales form" do form = form_handler.get_form("previous_sales") expect(form).to be_a(Form) - expect(form.pages.count).to eq(182) + expect(form.pages.count).to eq(183) expect(form.name).to eq("2021_2022_sales") end end diff --git a/spec/models/validations/sales/soft_validations_spec.rb b/spec/models/validations/sales/soft_validations_spec.rb index 902790009..34f49d847 100644 --- a/spec/models/validations/sales/soft_validations_spec.rb +++ b/spec/models/validations/sales/soft_validations_spec.rb @@ -7,14 +7,14 @@ RSpec.describe Validations::Sales::SoftValidations do context "when validating soft min" do it "returns false if no income1 is given" do record.income1 = nil - expect(record) - .not_to be_income1_under_soft_min + + expect(record).not_to be_income1_under_soft_min end it "returns false if no ecstat1 is given" do record.ecstat1 = nil - expect(record) - .not_to be_income1_under_soft_min + + expect(record).not_to be_income1_under_soft_min end [ @@ -292,4 +292,41 @@ RSpec.describe Validations::Sales::SoftValidations do expect(record).not_to be_hodate_3_years_or_more_saledate end end + + describe "wheelchair_when_not_disabled" do + it "when hodate not set" do + record.disabled = 2 + record.wheel = nil + + expect(record).not_to be_wheelchair_when_not_disabled + end + + it "when disabled not set" do + record.disabled = nil + record.wheel = 1 + + expect(record).not_to be_wheelchair_when_not_disabled + end + + it "when disabled and wheel not set" do + record.disabled = nil + record.wheel = nil + + expect(record).not_to be_wheelchair_when_not_disabled + end + + it "when disabled == 2 && wheel == 1" do + record.disabled = 2 + record.wheel = 1 + + expect(record).to be_wheelchair_when_not_disabled + end + + it "when disabled == 2 && wheel != 1" do + record.disabled = 2 + record.wheel = 2 + + expect(record).not_to be_wheelchair_when_not_disabled + end + end end From 3308d86a106b2bde9fe1dae41af687b98c46526d Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 24 Jan 2023 15:21:09 +0000 Subject: [PATCH 09/10] Last settled accommodation and discounted ownership property postcodes must match (#1213) --- .../sales/household_validations.rb | 9 +++ config/locales/en.yml | 4 +- .../sales/household_validations_spec.rb | 58 +++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/app/models/validations/sales/household_validations.rb b/app/models/validations/sales/household_validations.rb index 8ee00fdfd..42846273c 100644 --- a/app/models/validations/sales/household_validations.rb +++ b/app/models/validations/sales/household_validations.rb @@ -32,6 +32,15 @@ module Validations::Sales::HouseholdValidations end end + def validate_previous_postcode(record) + return unless record.postcode_full && record.ppostcode_full && record.discounted_ownership_sale? + + unless record.postcode_full == record.ppostcode_full + record.errors.add :postcode_full, I18n.t("validations.household.postcode.discounted_ownership") + record.errors.add :ppostcode_full, I18n.t("validations.household.postcode.discounted_ownership") + end + end + private def validate_person_age_matches_relationship(record, person_num) diff --git a/config/locales/en.yml b/config/locales/en.yml index d84429637..68c7199de 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -369,7 +369,9 @@ en: condition_effects: no_choices: "You cannot answer this question as you told us nobody in the household has a physical or mental health condition (or other illness) expected to last 12 months or more" old_persons_shared_ownership: "Are you sure? At least one buyer should be aged over 64 for Older persons‘ shared ownership scheme" - + postcode: + discounted_ownership: "Last settled accommodation and discounted ownership property postcodes must match" + tenancy: length: fixed_term_not_required: "You must only answer the length of the tenancy if it's fixed-term" diff --git a/spec/models/validations/sales/household_validations_spec.rb b/spec/models/validations/sales/household_validations_spec.rb index 61a204878..5b227c36d 100644 --- a/spec/models/validations/sales/household_validations_spec.rb +++ b/spec/models/validations/sales/household_validations_spec.rb @@ -280,4 +280,62 @@ RSpec.describe Validations::Sales::HouseholdValidations do end end end + + describe "previous postcode validations" do + let(:record) { build(:sales_log) } + + context "with a discounted sale" do + before do + record.ownershipsch = 2 + end + + it "adds an error when previous and current postcodes are not the same" do + record.postcode_full = "SO32 3PT" + record.ppostcode_full = "DN6 7FB" + household_validator.validate_previous_postcode(record) + expect(record.errors["postcode_full"]) + .to include(match I18n.t("validations.household.postcode.discounted_ownership")) + expect(record.errors["ppostcode_full"]) + .to include(match I18n.t("validations.household.postcode.discounted_ownership")) + end + + it "allows same postcodes" do + record.postcode_full = "SO32 3PT" + record.ppostcode_full = "SO32 3PT" + household_validator.validate_previous_postcode(record) + expect(record.errors["postcode_full"]).to be_empty + expect(record.errors["ppostcode_full"]).to be_empty + end + + it "does not add an error when postcode is missing" do + record.postcode_full = nil + record.ppostcode_full = "SO32 3PT" + household_validator.validate_previous_postcode(record) + expect(record.errors["postcode_full"]).to be_empty + expect(record.errors["ppostcode_full"]).to be_empty + end + + it "does not add an error when previous postcode is missing" do + record.postcode_full = "SO32 3PT" + record.ppostcode_full = nil + household_validator.validate_previous_postcode(record) + expect(record.errors["postcode_full"]).to be_empty + expect(record.errors["ppostcode_full"]).to be_empty + end + end + + context "without a discounted sale" do + before do + record.ownershipsch = 1 + end + + it "allows different postcodes" do + record.postcode_full = "SO32 3PT" + record.ppostcode_full = "DN6 7FB" + household_validator.validate_previous_postcode(record) + expect(record.errors["postcode_full"]).to be_empty + expect(record.errors["ppostcode_full"]).to be_empty + end + end + end end From 10536ae13fb7536d491d261a0ba4281762e131de Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 24 Jan 2023 16:45:33 +0000 Subject: [PATCH 10/10] Update reson answer options (#1209) --- config/forms/2021_2022.json | 6 +++--- config/forms/2022_2023.json | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config/forms/2021_2022.json b/config/forms/2021_2022.json index f586b3069..e47191f6e 100644 --- a/config/forms/2021_2022.json +++ b/config/forms/2021_2022.json @@ -6166,10 +6166,10 @@ "1": { "value": "Permanently decanted from another property owned by this landlord" }, - "45": { + "46": { "value": "Discharged from long-stay hospital or similar institution" }, - "44": { + "45": { "value": "Discharged from prison" }, "2": { @@ -6181,7 +6181,7 @@ "9": { "value": "Asked to leave by family or friends" }, - "46": { + "44": { "value": "Death of household member in last settled accommodation" }, "8": { diff --git a/config/forms/2022_2023.json b/config/forms/2022_2023.json index a5048b958..7c54b67d4 100644 --- a/config/forms/2022_2023.json +++ b/config/forms/2022_2023.json @@ -6165,10 +6165,10 @@ "1": { "value": "Permanently decanted from another property owned by this landlord" }, - "45": { + "46": { "value": "Discharged from long-stay hospital or similar institution" }, - "44": { + "45": { "value": "Discharged from prison" }, "2": { @@ -6180,7 +6180,7 @@ "9": { "value": "Asked to leave by family or friends" }, - "46": { + "44": { "value": "Death of household member in last settled accommodation" }, "8": {