From d88e7d704b89988dfc78be48b065759287075e44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Meny?= Date: Fri, 15 Jul 2022 14:51:01 +0100 Subject: [PATCH] Changes following code review --- app/models/case_log.rb | 33 +++++++++-------- .../derived_variables/case_log_variables.rb | 1 - app/models/location.rb | 8 +++++ .../imports/case_logs_import_service.rb | 9 ----- .../imports/scheme_location_import_service.rb | 10 +++--- app/services/postcode_service.rb | 21 +++++++++++ ...20715133937_remove_county_from_location.rb | 5 +++ db/schema.rb | 7 ++-- spec/factories/location.rb | 2 -- spec/models/case_log_spec.rb | 15 +++++--- spec/models/location_spec.rb | 11 ++++++ .../imports/case_logs_import_service_spec.rb | 36 ++++++++++--------- .../scheme_location_import_service_spec.rb | 1 + 13 files changed, 103 insertions(+), 56 deletions(-) create mode 100644 db/migrate/20220715133937_remove_county_from_location.rb diff --git a/app/models/case_log.rb b/app/models/case_log.rb index ddf728329..a8250056d 100644 --- a/app/models/case_log.rb +++ b/app/models/case_log.rb @@ -102,6 +102,22 @@ class CaseLog < ApplicationRecord attribute_names - AUTOGENERATED_FIELDS end + def la + if location + location.location_code + else + super + end + end + + def postcode_full + if location + location.postcode + else + super + end + end + def completed? status == "completed" end @@ -465,7 +481,7 @@ class CaseLog < ApplicationRecord private - PIO = Postcodes::IO.new + PIO = PostcodeService.new def update_status! self.status = if all_fields_completed? && errors.empty? @@ -587,20 +603,7 @@ private end def get_inferred_la(postcode) - # Avoid network calls when postcode is invalid - return unless postcode.match(POSTCODE_REGEXP) - - postcode_lookup = nil - begin - # URI encoding only supports ASCII characters - ascii_postcode = PostcodeService.clean(postcode) - Timeout.timeout(5) { postcode_lookup = PIO.lookup(ascii_postcode) } - rescue Timeout::Error - Rails.logger.warn("Postcodes.io lookup timed out") - end - if postcode_lookup && postcode_lookup.info.present? - postcode_lookup.codes["admin_district"] - end + PIO.infer_la(postcode) end def get_has_benefits diff --git a/app/models/derived_variables/case_log_variables.rb b/app/models/derived_variables/case_log_variables.rb index e679d9058..312c5a719 100644 --- a/app/models/derived_variables/case_log_variables.rb +++ b/app/models/derived_variables/case_log_variables.rb @@ -73,7 +73,6 @@ module DerivedVariables::CaseLogVariables self.location = scheme.locations.first end if location - self.postcode_full = location.postcode # TODO: Remove and replace with mobility type self.wchair = location.wheelchair_adaptation_before_type_cast end diff --git a/app/models/location.rb b/app/models/location.rb index 0eb929938..a44640889 100644 --- a/app/models/location.rb +++ b/app/models/location.rb @@ -2,6 +2,8 @@ class Location < ApplicationRecord validate :validate_postcode belongs_to :scheme + before_save :infer_la!, if: :postcode_changed? + attr_accessor :add_another_location WHEELCHAIR_ADAPTATIONS = { @@ -44,10 +46,16 @@ class Location < ApplicationRecord private + PIO = PostcodeService.new + def validate_postcode if postcode.nil? || !postcode&.match(POSTCODE_REGEXP) error_message = I18n.t("validations.postcode") errors.add :postcode, error_message end end + + def infer_la! + self.location_code = PIO.infer_la(postcode) + end end diff --git a/app/services/imports/case_logs_import_service.rb b/app/services/imports/case_logs_import_service.rb index 8b7aa80dc..942a88bb2 100644 --- a/app/services/imports/case_logs_import_service.rb +++ b/app/services/imports/case_logs_import_service.rb @@ -627,15 +627,6 @@ module Imports end end - def clear_household_charges(attributes) - attributes["period"] = nil - attributes["brent"] = nil - attributes["scharge"] = nil - attributes["pscharge"] = nil - attributes["supcharg"] = nil - attributes["tcharge"] = nil - end - def is_carehome(scheme) return nil unless scheme diff --git a/app/services/imports/scheme_location_import_service.rb b/app/services/imports/scheme_location_import_service.rb index 698c715b5..9c29f67e2 100644 --- a/app/services/imports/scheme_location_import_service.rb +++ b/app/services/imports/scheme_location_import_service.rb @@ -72,7 +72,8 @@ module Imports attributes["secondary_client_group"] = string_or_nil(xml_doc, "client-group-2") attributes["secondary_client_group"] = nil if attributes["primary_client_group"] == attributes["secondary_client_group"] attributes["sensitive"] = sensitive(xml_doc) - attributes["end_date"] = parse_end_date(xml_doc) + attributes["start_date"] = parse_date(xml_doc, "start-date") + attributes["end_date"] = parse_date(xml_doc, "end-date") attributes["location_name"] = string_or_nil(xml_doc, "name") attributes["postcode"] = string_or_nil(xml_doc, "postcode") attributes["units"] = safe_string_as_integer(xml_doc, "total-units") @@ -94,6 +95,7 @@ module Imports type_of_unit: attributes["type_of_unit"], old_visible_id: attributes["location_old_visible_id"], old_id: attributes["location_old_id"], + startdate: attributes["start_date"], scheme:, ) rescue ActiveRecord::RecordNotUnique @@ -186,9 +188,9 @@ module Imports end end - def parse_end_date(xml_doc) - end_date = string_or_nil(xml_doc, "end-date") - Time.zone.parse(end_date) if end_date + def parse_date(xml_doc, attribute) + date = string_or_nil(xml_doc, attribute) + Time.zone.parse(date) if date end end end diff --git a/app/services/postcode_service.rb b/app/services/postcode_service.rb index 737e3fd9c..a82dcc2aa 100644 --- a/app/services/postcode_service.rb +++ b/app/services/postcode_service.rb @@ -1,4 +1,25 @@ class PostcodeService + def initialize + @pio = Postcodes::IO.new + end + + def infer_la(postcode) + # Avoid network calls when postcode is invalid + return unless postcode.match(POSTCODE_REGEXP) + + postcode_lookup = nil + begin + # URI encoding only supports ASCII characters + ascii_postcode = self.class.clean(postcode) + Timeout.timeout(5) { postcode_lookup = @pio.lookup(ascii_postcode) } + rescue Timeout::Error + Rails.logger.warn("Postcodes.io lookup timed out") + end + if postcode_lookup && postcode_lookup.info.present? + postcode_lookup.codes["admin_district"] + end + end + def self.clean(postcode) postcode.encode("ASCII", "UTF-8", invalid: :replace, undef: :replace, replace: "").delete(" ").upcase end diff --git a/db/migrate/20220715133937_remove_county_from_location.rb b/db/migrate/20220715133937_remove_county_from_location.rb new file mode 100644 index 000000000..02fddb658 --- /dev/null +++ b/db/migrate/20220715133937_remove_county_from_location.rb @@ -0,0 +1,5 @@ +class RemoveCountyFromLocation < ActiveRecord::Migration[7.0] + def change + remove_column :locations, :county, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 5433ddfcc..20e4f4bf9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_07_14_080044) do +ActiveRecord::Schema[7.0].define(version: 2022_07_15_133937) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -191,9 +191,9 @@ ActiveRecord::Schema[7.0].define(version: 2022_07_14_080044) do t.integer "joint" t.bigint "created_by_id" t.integer "illness_type_0" - t.integer "retirement_value_check" t.integer "tshortfall_known" t.integer "sheltered" + t.integer "retirement_value_check" t.integer "pregnancy_value_check" t.integer "hhtype" t.integer "new_old" @@ -240,16 +240,15 @@ ActiveRecord::Schema[7.0].define(version: 2022_07_14_080044) do t.string "location_code" t.string "postcode" t.string "type_of_building" - t.integer "wheelchair_adaptation" t.bigint "scheme_id", null: false t.string "name" - t.string "county" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "units" t.integer "type_of_unit" t.string "old_id" t.integer "old_visible_id" + t.integer "wheelchair_adaptation" t.string "mobility_type" t.datetime "startdate", precision: nil t.index ["old_id"], name: "index_locations_on_old_id", unique: true diff --git a/spec/factories/location.rb b/spec/factories/location.rb index 21663a86a..daef3d2ce 100644 --- a/spec/factories/location.rb +++ b/spec/factories/location.rb @@ -1,13 +1,11 @@ FactoryBot.define do factory :location do - location_code { Faker::Name.initials(number: 10) } postcode { Faker::Address.postcode.delete(" ") } name { Faker::Address.street_name } type_of_unit { [1, 2, 3, 4, 6, 7].sample } type_of_building { "Purpose built" } mobility_type { %w[A M N W X].sample } wheelchair_adaptation { 2 } - county { Faker::Address.state } scheme end end diff --git a/spec/models/case_log_spec.rb b/spec/models/case_log_spec.rb index 2cf69e23d..fd2056577 100644 --- a/spec/models/case_log_spec.rb +++ b/spec/models/case_log_spec.rb @@ -1702,9 +1702,9 @@ RSpec.describe CaseLog do context "and not renewal" do let(:scheme) { FactoryBot.create(:scheme) } - let(:location) { FactoryBot.create(:location, scheme:, county: "E07000041", type_of_unit: 1, type_of_building: "Purpose built") } + let(:location) { FactoryBot.create(:location, scheme:, postcode: "M11AE", type_of_unit: 1, type_of_building: "Purpose built") } - let!(:supported_housing_case_log) do + let(:supported_housing_case_log) do described_class.create!({ managing_organisation: owning_organisation, owning_organisation:, @@ -1716,14 +1716,21 @@ RSpec.describe CaseLog do }) end + before do + stub_request(:get, /api.postcodes.io/) + .to_return(status: 200, body: "{\"status\":200,\"result\":{\"admin_district\":\"Manchester\",\"codes\":{\"admin_district\": \"E08000003\"}}}", headers: {}) + end + it "correctly infers and saves la" do record_from_db = ActiveRecord::Base.connection.execute("SELECT la from case_logs WHERE id=#{supported_housing_case_log.id}").to_a[0] - expect(record_from_db["la"]).to eq(location.county) + expect(record_from_db["la"]).to be_nil + expect(supported_housing_case_log.la).to eq("E08000003") end it "correctly infers and saves postcode" do record_from_db = ActiveRecord::Base.connection.execute("SELECT postcode_full from case_logs WHERE id=#{supported_housing_case_log.id}").to_a[0] - expect(record_from_db["postcode_full"]).to eq(location.postcode) + expect(record_from_db["postcode_full"]).to eq(nil) + expect(supported_housing_case_log.postcode_full).to eq("M11AE") end it "unittype_sh method returns the type_of_unit of the location" do diff --git a/spec/models/location_spec.rb b/spec/models/location_spec.rb index a886c6034..ae78df6c5 100644 --- a/spec/models/location_spec.rb +++ b/spec/models/location_spec.rb @@ -4,9 +4,20 @@ RSpec.describe Location, type: :model do describe "#new" do let(:location) { FactoryBot.build(:location) } + before do + stub_request(:get, /api.postcodes.io/) + .to_return(status: 200, body: "{\"status\":200,\"result\":{\"admin_district\":\"Manchester\",\"codes\":{\"admin_district\": \"E08000003\"}}}", headers: {}) + end + it "belongs to an organisation" do expect(location.scheme).to be_a(Scheme) end + + it "infers the local authority" do + location.postcode = "M1 1AE" + location.save! + expect(location.location_code).to eq("E08000003") + end end describe "#validate_postcode" do diff --git a/spec/services/imports/case_logs_import_service_spec.rb b/spec/services/imports/case_logs_import_service_spec.rb index b67f92481..27b6b975c 100644 --- a/spec/services/imports/case_logs_import_service_spec.rb +++ b/spec/services/imports/case_logs_import_service_spec.rb @@ -16,6 +16,9 @@ RSpec.describe Imports::CaseLogsImportService do end before do + WebMock.stub_request(:get, /api.postcodes.io\/postcodes\/LS166FT/) + .to_return(status: 200, body: '{"status":200,"result":{"codes":{"admin_district":"E08000035"}}}', headers: {}) + allow(Organisation).to receive(:find_by).and_return(nil) allow(Organisation).to receive(:find_by).with(old_visible_id: organisation.old_visible_id.to_i).and_return(organisation) @@ -34,9 +37,6 @@ RSpec.describe Imports::CaseLogsImportService do # 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) allow(FormHandler.instance).to receive(:get_form).with("2022_2023").and_return(real_2022_2023_form) - - WebMock.stub_request(:get, /api.postcodes.io\/postcodes\/LS166FT/) - .to_return(status: 200, body: '{"status":200,"result":{"codes":{"admin_district":"E08000035"}}}', headers: {}) end context "when importing case logs" do @@ -69,45 +69,45 @@ RSpec.describe Imports::CaseLogsImportService do expect(logger).not_to receive(:warn) expect(logger).not_to receive(:info) expect { case_log_service.create_logs(remote_folder) } - .to change(CaseLog, :count).by(3) + .to change(CaseLog, :count).by(4) end it "only updates existing case logs" do expect(logger).not_to receive(:error) expect(logger).not_to receive(:warn) - expect(logger).to receive(:info).with(/Updating case log/).exactly(3).times + expect(logger).to receive(:info).with(/Updating case log/).exactly(4).times expect { 2.times { case_log_service.create_logs(remote_folder) } } - .to change(CaseLog, :count).by(3) + .to change(CaseLog, :count).by(4) end context "when there are status discrepancies" do - let(:case_log_id4) { "893ufj2s-lq77-42m4-rty6-ej09gh585uy1" } - let(:case_log_id5) { "5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd" } - let(:case_log_file) { open_file(fixture_directory, case_log_id4) } + let(:case_log_id5) { "893ufj2s-lq77-42m4-rty6-ej09gh585uy1" } + let(:case_log_id6) { "5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd" } + let(:case_log_file) { open_file(fixture_directory, case_log_id5) } let(:case_log_xml) { Nokogiri::XML(case_log_file) } before do - allow(storage_service).to receive(:get_file_io) - .with("#{remote_folder}/#{case_log_id4}.xml") - .and_return(open_file(fixture_directory, case_log_id4), open_file(fixture_directory, case_log_id4)) allow(storage_service).to receive(:get_file_io) .with("#{remote_folder}/#{case_log_id5}.xml") .and_return(open_file(fixture_directory, case_log_id5), open_file(fixture_directory, case_log_id5)) + allow(storage_service).to receive(:get_file_io) + .with("#{remote_folder}/#{case_log_id6}.xml") + .and_return(open_file(fixture_directory, case_log_id6), open_file(fixture_directory, case_log_id6)) end it "the logger logs a warning with the case log's old id/filename" do expect(logger).to receive(:warn).with(/is not completed/).once - expect(logger).to receive(:warn).with(/Case log with old id:#{case_log_id4} is incomplete but status should be complete/).once + expect(logger).to receive(:warn).with(/Case log with old id:#{case_log_id5} is incomplete but status should be complete/).once case_log_service.send(:create_log, case_log_xml) end it "on completion the ids of all logs with status discrepancies are logged in a warning" do allow(storage_service).to receive(:list_files) - .and_return(%W[#{remote_folder}/#{case_log_id4}.xml #{remote_folder}/#{case_log_id5}.xml]) - allow(logger).to receive(:warn).with(/is not completed/) - allow(logger).to receive(:warn).with(/is incomplete but status should be complete/) - expect(logger).to receive(:warn).with(/The following case logs had status discrepancies: \[893ufj2s-lq77-42m4-rty6-ej09gh585uy1, 5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd\]/).once + .and_return(%W[#{remote_folder}/#{case_log_id5}.xml #{remote_folder}/#{case_log_id6}.xml]) + expect(logger).to receive(:warn).with(/is not completed/).twice + expect(logger).to receive(:warn).with(/is incomplete but status should be complete/).twice + expect(logger).to receive(:warn).with(/The following case logs had status discrepancies: \[893ufj2s-lq77-42m4-rty6-ej09gh585uy1, 5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd\]/) case_log_service.create_logs(remote_folder) end @@ -220,11 +220,13 @@ RSpec.describe Imports::CaseLogsImportService do let(:case_log_id) { "0b4a68df-30cc-474a-93c0-a56ce8fdad3b" } 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 diff --git a/spec/services/imports/scheme_location_import_service_spec.rb b/spec/services/imports/scheme_location_import_service_spec.rb index 6d276ef37..199424f54 100644 --- a/spec/services/imports/scheme_location_import_service_spec.rb +++ b/spec/services/imports/scheme_location_import_service_spec.rb @@ -138,6 +138,7 @@ RSpec.describe Imports::SchemeLocationImportService do expect(location.type_of_unit).to eq("Bungalow") expect(location.old_id).to eq(first_location_id) expect(location.old_visible_id).to eq(10) + expect(location.startdate).to eq("1900-01-01") expect(location.scheme).to eq(scheme) end