Changes following code review

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
+    PIO.infer_la(postcode)
-    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
   end
 
   def get_has_benefits

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

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

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
 

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)
+    def parse_date(xml_doc, attribute)
-      end_date = string_or_nil(xml_doc, "end-date")
+      date = string_or_nil(xml_doc, attribute)
-      Time.zone.parse(end_date) if end_date
+      Time.zone.parse(date) if date
     end
   end
 end

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

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

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

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

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

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

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) { "893ufj2s-lq77-42m4-rty6-ej09gh585uy1" }
-      let(:case_log_id5) { "5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd" }
+      let(:case_log_id6) { "5ybz29dj-l33t-k1l0-hj86-n4k4ma77xkcd" }
-      let(:case_log_file) { open_file(fixture_directory, case_log_id4) }
+      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])
+                                  .and_return(%W[#{remote_folder}/#{case_log_id5}.xml #{remote_folder}/#{case_log_id6}.xml])
-        allow(logger).to receive(:warn).with(/is not completed/)
+        expect(logger).to receive(:warn).with(/is not completed/).twice
-        allow(logger).to receive(:warn).with(/is incomplete but status should be complete/)
+        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\]/).once
+        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

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