From 68bd511cf2da8e0edcb4fa0907796897bac6aeed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Meny?= Date: Mon, 18 Jul 2022 16:24:38 +0100 Subject: [PATCH] CLDC-1228: Fixes 4 (#748) * Prevents location from being cleared when it has been changed alongside the scheme * Handles visible IDs not being system unique * Hides the location correctly --- app/models/case_log.rb | 6 +--- .../derived_variables/case_log_variables.rb | 12 ++++--- app/models/form.rb | 6 ++-- .../form/setup/questions/location_id.rb | 1 - .../imports/case_logs_import_service.rb | 16 ++++++---- spec/models/form_spec.rb | 5 +-- .../imports/case_logs_import_service_spec.rb | 32 ++++++++++++++----- 7 files changed, 47 insertions(+), 31 deletions(-) diff --git a/app/models/case_log.rb b/app/models/case_log.rb index ef77ea5a5..83e7b6550 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -23,7 +23,7 @@ class CaseLog < ApplicationRecord validates_with CaseLogValidator before_validation :recalculate_start_year!, if: :startdate_changed? - before_validation :reset_scheme_location!, if: :scheme_changed? + before_validation :reset_scheme_location!, if: :scheme_changed?, unless: :location_changed? before_validation :process_postcode_changes!, if: :postcode_full_changed? before_validation :process_previous_postcode_changes!, if: :ppostcode_full_changed? before_validation :reset_invalidated_dependent_fields! @@ -696,8 +696,4 @@ private def upcase_and_remove_whitespace(string) string.present? ? string.upcase.gsub(/\s+/, "") : string end - - def reset_scheme_location! - self.location = nil - end end diff --git a/app/models/derived_variables/case_log_variables.rb b/app/models/derived_variables/case_log_variables.rb index 312c5a719..31209dd50 100644 --- a/app/models/derived_variables/case_log_variables.rb +++ b/app/models/derived_variables/case_log_variables.rb @@ -68,10 +68,7 @@ module DerivedVariables::CaseLogVariables self.new_old = new_or_existing_tenant self.vacdays = property_vacant_days - if is_supported_housing? && scheme - if scheme.locations.size == 1 - self.location = scheme.locations.first - end + if is_supported_housing? if location # TODO: Remove and replace with mobility type self.wchair = location.wheelchair_adaptation_before_type_cast @@ -189,4 +186,11 @@ private (startdate - voiddate).to_i / 1.day end end + + def reset_scheme_location! + self.location = nil + if scheme && scheme.locations.size == 1 + self.location = scheme.locations.first + end + end end diff --git a/app/models/form.rb b/app/models/form.rb index 6176f8850..57b529f10 100644 --- a/app/models/form.rb +++ b/app/models/form.rb @@ -127,9 +127,9 @@ class Form end def invalidated_page_questions(case_log, current_user = nil) - # we're already treating address fields as a special case and reset their values upon saving a case_log - address_questions = %w[postcode_known la ppcodenk previous_la_known prevloc postcode_full ppostcode_full] - questions.reject { |q| q.page.routed_to?(case_log, current_user) || q.derived? || address_questions.include?(q.id) } || [] + # we're already treating these fields as a special case and reset their values upon saving a case_log + callback_questions = %w[postcode_known la ppcodenk previous_la_known prevloc postcode_full ppostcode_full location_id] + questions.reject { |q| q.page.routed_to?(case_log, current_user) || q.derived? || callback_questions.include?(q.id) } || [] end def enabled_page_questions(case_log) diff --git a/app/models/form/setup/questions/location_id.rb b/app/models/form/setup/questions/location_id.rb index 9e6fb6038..b3ddfdb0c 100644 --- a/app/models/form/setup/questions/location_id.rb +++ b/app/models/form/setup/questions/location_id.rb @@ -5,7 +5,6 @@ class Form::Setup::Questions::LocationId < ::Form::Question @header = "Which location is this log for?" @hint_text = "" @type = "radio" - @derived = true unless FeatureToggle.supported_housing_schemes_enabled? @answer_options = answer_options end diff --git a/app/services/imports/case_logs_import_service.rb b/app/services/imports/case_logs_import_service.rb index 942a88bb2..440eecb13 100644 --- a/app/services/imports/case_logs_import_service.rb +++ b/app/services/imports/case_logs_import_service.rb @@ -185,16 +185,20 @@ module Imports # Supported Housing fields if attributes["needstype"] == GN_SH[:supported_housing] - old_visible_id = safe_string_as_integer(xml_doc, "_1cschemecode") - location = Location.find_by(old_visible_id:) - scheme = location.scheme - # Set the scheme via location, because the scheme old visible ID is not unique + location_old_visible_id = safe_string_as_integer(xml_doc, "_1cschemecode") + scheme_old_visible_id = safe_string_as_integer(xml_doc, "_1cmangroupcode") + + schemes = Scheme.where(old_visible_id: scheme_old_visible_id, owning_organisation_id: attributes["owning_organisation_id"]) + location = Location.find_by(old_visible_id: location_old_visible_id, scheme: schemes) + raise "No matching location for scheme #{scheme_old_visible_id} and location #{location_old_visible_id} (visible IDs)" if location.nil? + + # Set the scheme via location, because the scheme old visible ID can be duplicated at import attributes["location_id"] = location.id - attributes["scheme_id"] = scheme.id + attributes["scheme_id"] = location.scheme.id attributes["sheltered"] = unsafe_string_as_integer(xml_doc, "Q1e") attributes["chcharge"] = safe_string_as_decimal(xml_doc, "Q18b") attributes["household_charge"] = household_charge(xml_doc) - attributes["is_carehome"] = is_carehome(scheme) + attributes["is_carehome"] = is_carehome(location.scheme) end # Handles confidential schemes diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb index b1870d709..503900ab0 100644 --- a/spec/models/form_spec.rb +++ b/spec/models/form_spec.rb @@ -178,18 +178,15 @@ RSpec.describe Form, type: :model do describe "invalidated_page_questions" do let(:case_log) { FactoryBot.create(:case_log, :in_progress, needstype: 1) } + let(:expected_invalid) { %w[scheme_id condition_effects cbl conditional_question_no_second_question net_income_value_check dependent_question offered layear declaration] } context "when dependencies are not met" do - let(:expected_invalid) { %w[scheme_id location_id condition_effects cbl conditional_question_no_second_question net_income_value_check dependent_question offered layear declaration] } - it "returns an array of question keys whose pages conditions are not met" do expect(form.invalidated_page_questions(case_log).map(&:id).uniq).to eq(expected_invalid) end end context "with two pages having the same question and only one has dependencies met" do - let(:expected_invalid) { %w[scheme_id location_id condition_effects cbl conditional_question_no_second_question net_income_value_check dependent_question offered layear declaration] } - it "returns an array of question keys whose pages conditions are not met" do case_log["preg_occ"] = "No" expect(form.invalidated_page_questions(case_log).map(&:id).uniq).to eq(expected_invalid) diff --git a/spec/services/imports/case_logs_import_service_spec.rb b/spec/services/imports/case_logs_import_service_spec.rb index 27b6b975c..1e6c28739 100644 --- a/spec/services/imports/case_logs_import_service_spec.rb +++ b/spec/services/imports/case_logs_import_service_spec.rb @@ -9,7 +9,10 @@ RSpec.describe Imports::CaseLogsImportService do let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json", "2021_2022") } let(:real_2022_2023_form) { Form.new("config/forms/2022_2023.json", "2022_2023") } let(:fixture_directory) { "spec/fixtures/imports/case_logs" } + let(:organisation) { FactoryBot.create(:organisation, old_visible_id: "1", provider_type: "PRP") } + let(:scheme1) { FactoryBot.create(:scheme, old_visible_id: 123, owning_organisation: organisation) } + let(:scheme2) { FactoryBot.create(:scheme, old_visible_id: 456, owning_organisation: organisation) } def open_file(directory, filename) File.open("#{directory}/#{filename}.xml") @@ -26,13 +29,10 @@ RSpec.describe Imports::CaseLogsImportService do FactoryBot.create(:user, old_user_id: "c3061a2e6ea0b702e6f6210d5c52d2a92612d2aa", organisation:) FactoryBot.create(:user, old_user_id: "e29c492473446dca4d50224f2bb7cf965a261d6f", organisation:) - # Scheme and Location - scheme = FactoryBot.create(:scheme, old_visible_id: 123) - FactoryBot.create(:location, - old_visible_id: 10, - scheme_id: scheme.id, - wheelchair_adaptation: 1, - postcode: "LS166FT") + # Location setup + FactoryBot.create(:location, old_visible_id: 10, wheelchair_adaptation: 1, postcode: "LS166FT", scheme_id: scheme1.id) + FactoryBot.create(:location, scheme_id: scheme1.id) + FactoryBot.create(:location, old_visible_id: 10, wheelchair_adaptation: 1, postcode: "LS166FT", scheme_id: scheme2.id) # Stub the form handler to use the real form allow(FormHandler.instance).to receive(:get_form).with("2021_2022").and_return(real_2021_2022_form) @@ -216,7 +216,7 @@ RSpec.describe Imports::CaseLogsImportService do end end - context "and this is a supported housing log" do + context "and this is a supported housing log with multiple locations under a scheme" do let(:case_log_id) { "0b4a68df-30cc-474a-93c0-a56ce8fdad3b" } it "sets the scheme and location values" do @@ -229,5 +229,21 @@ RSpec.describe Imports::CaseLogsImportService do expect(case_log.status).to eq("completed") end end + + context "and this is a supported housing log with a single location under a scheme" do + let(:case_log_id) { "0b4a68df-30cc-474a-93c0-a56ce8fdad3b" } + + before { case_log_xml.at_xpath("//xmlns:_1cmangroupcode").content = scheme2.old_visible_id } + + it "sets the scheme and location values" do + expect(logger).not_to receive(:warn) + case_log_service.send(:create_log, case_log_xml) + case_log = CaseLog.find_by(old_id: case_log_id) + + expect(case_log.scheme_id).not_to be_nil + expect(case_log.location_id).not_to be_nil + expect(case_log.status).to eq("completed") + end + end end end