diff --git a/Gemfile b/Gemfile index 4dc93aaf7..a5c7dd01f 100644 --- a/Gemfile +++ b/Gemfile @@ -39,7 +39,6 @@ gem "devise_two_factor_authentication" # UK postcode parsing and validation gem "uk_postcode" # Get rich data from postcode lookups. Wraps postcodes.io -gem "postcodes_io" # Use Ruby objects to build reusable markup. A React inspired evolution of the presenter pattern gem "view_component", "~> 3.9" # Use the AWS S3 SDK as storage mechanism @@ -112,3 +111,5 @@ end # Windows does not include zoneinfo files, so bundle the tzinfo-data gem gem "cssbundling-rails" gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw jruby] + +gem "excon", "~> 0.111.0" diff --git a/Gemfile.lock b/Gemfile.lock index 7cc2addda..cdf923ae0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -183,7 +183,7 @@ GEM et-orbi (1.2.11) tzinfo event_stream_parser (1.0.0) - excon (0.109.0) + excon (0.111.0) factory_bot (6.4.6) activesupport (>= 5.0.0) factory_bot_rails (6.4.3) @@ -302,8 +302,6 @@ GEM racc pg (1.5.5) possessive (1.0.1) - postcodes_io (0.4.0) - excon (~> 0.39) propshaft (0.8.0) actionpack (>= 7.0.0) activesupport (>= 7.0.0) @@ -533,6 +531,7 @@ DEPENDENCIES devise_two_factor_authentication dotenv-rails erb_lint + excon (~> 0.111.0) factory_bot_rails faker govuk-components (~> 5.1) @@ -549,7 +548,6 @@ DEPENDENCIES parallel_tests pg (~> 1.1) possessive - postcodes_io propshaft pry-byebug puma (~> 5.6) diff --git a/app/services/postcode_service.rb b/app/services/postcode_service.rb index c3c6dcfdd..8cfe9c43c 100644 --- a/app/services/postcode_service.rb +++ b/app/services/postcode_service.rb @@ -1,26 +1,24 @@ class PostcodeService - def initialize - @pio = Postcodes::IO.new - end - def lookup(postcode) # Avoid network calls when postcode is invalid return unless postcode.match(POSTCODE_REGEXP) - postcode_lookup = nil + result = nil begin # URI encoding only supports ASCII characters ascii_postcode = self.class.clean(postcode) - Timeout.timeout(30) { 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? - { - location_code: postcode_lookup.codes["admin_district"], - location_admin_district: postcode_lookup&.admin_district, + response = Excon.get("https://api.postcodes.io/postcodes/#{ascii_postcode}", idempotent: true, timeout: 30, expects: [200]) + parsed_response = JSON.parse(response.body) + result = { + location_code: parsed_response["result"]["codes"]["admin_district"], + location_admin_district: parsed_response["result"]["admin_district"], } + rescue Excon::Error => e + Rails.logger.warn("Postcode lookup request was not successful: #{e} #{e.response.body}") + rescue StandardError => e + Rails.logger.error("Unexpected error with postcode lookup request: #{e}") end + result end def self.clean(postcode) diff --git a/spec/lib/tasks/update_schemes_and_locations_from_csv_spec.rb b/spec/lib/tasks/update_schemes_and_locations_from_csv_spec.rb index 0d61b08f1..818a2b589 100644 --- a/spec/lib/tasks/update_schemes_and_locations_from_csv_spec.rb +++ b/spec/lib/tasks/update_schemes_and_locations_from_csv_spec.rb @@ -25,7 +25,7 @@ RSpec.describe "bulk_update" do allow(ENV).to receive(:[]).with("BULK_UPLOAD_BUCKET").and_return(instance_name) WebMock.stub_request(:get, /api\.postcodes\.io/) - .to_return(status: 200, body: "{\"status\":404,\"error\":\"Postcode not found\"}", headers: {}) + .to_return(status: 404, body: "{\"status\":404,\"error\":\"Postcode not found\"}", headers: {}) WebMock.stub_request(:get, /api\.postcodes\.io\/postcodes\/B11BB/) .to_return(status: 200, body: '{"status":200,"result":{"admin_district":"Westminster","codes":{"admin_district":"E09000033"}}}', headers: {}) end diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 2001c0bfa..92452f197 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -329,17 +329,6 @@ RSpec.describe LettingsLog do .to raise_error(ActiveRecord::RecordInvalid, /#{I18n.t("validations.postcode")}/) end - context "when the local authority lookup times out" do - before do - allow(Timeout).to receive(:timeout).and_raise(Timeout::Error) - end - - it "logs a warning" do - expect(Rails.logger).to receive(:warn).with("Postcodes.io lookup timed out") - address_lettings_log.update!({ postcode_known: 1, postcode_full: "M1 1AD" }) - end - end - it "correctly resets all fields if property postcode not known" do address_lettings_log.update!({ postcode_known: 0 }) diff --git a/spec/models/sales_log_spec.rb b/spec/models/sales_log_spec.rb index ddc696be4..a568ca330 100644 --- a/spec/models/sales_log_spec.rb +++ b/spec/models/sales_log_spec.rb @@ -628,17 +628,6 @@ RSpec.describe SalesLog, type: :model do .to raise_error(ActiveRecord::RecordInvalid, /#{I18n.t("validations.postcode")}/) end - context "when the local authority lookup times out" do - before do - allow(Timeout).to receive(:timeout).and_raise(Timeout::Error) - end - - it "logs a warning" do - expect(Rails.logger).to receive(:warn).with("Postcodes.io lookup timed out") - address_sales_log.update!({ pcodenk: 1, postcode_full: "M1 1AD" }) - end - end - it "correctly resets all fields if property postcode not known" do address_sales_log.update!({ pcodenk: 1 }) diff --git a/spec/request_helper.rb b/spec/request_helper.rb index 15d178218..f1f208ec6 100644 --- a/spec/request_helper.rb +++ b/spec/request_helper.rb @@ -4,7 +4,7 @@ module RequestHelper def self.stub_http_requests WebMock.disable_net_connect!(allow_localhost: true) WebMock.stub_request(:get, /api\.postcodes\.io/) - .to_return(status: 200, body: "{\"status\":404,\"error\":\"Postcode not found\"}", headers: {}) + .to_return(status: 404, body: "{\"status\":404,\"error\":\"Postcode not found\"}", headers: {}) WebMock.stub_request(:get, "https://api.postcodes.io/postcodes/AA11AA") .to_return(status: 200, body: "{\"status\":200,\"result\":{\"postcode\":\"AA1 1AA\",\"admin_district\":\"Westminster\",\"codes\":{\"admin_district\":\"E09000033\"}}}", headers: {}) diff --git a/spec/services/postcode_service_spec.rb b/spec/services/postcode_service_spec.rb index ecf20fdac..abc61129f 100644 --- a/spec/services/postcode_service_spec.rb +++ b/spec/services/postcode_service_spec.rb @@ -1,9 +1,80 @@ require "rails_helper" describe PostcodeService do - let(:postcode) { "s r81LS\u00A0" } + let(:service) { described_class.new } - it "returns clean postcode" do - expect(described_class.clean(postcode)).to eq "SR81LS" + describe "clean" do + let(:postcode) { "s r81LS\u00A0" } + + it "returns clean postcode" do + expect(described_class.clean(postcode)).to eq "SR81LS" + end + end + + describe "lookup" do + before do + Excon.defaults[:mock] = true + Excon.defaults[:stubs] = :local + end + + context "when the request returns a success response" do + before do + Excon.stub({}, { body: '{"result": { "admin_district": "District", "codes": { "admin_district": "123" } } }', status: 200 }) + end + + it "returns the admin district and admin district code" do + result = service.lookup("A00 0AA") + expect(result[:location_code]).to eq("123") + expect(result[:location_admin_district]).to eq("District") + end + end + + context "when the request returns a not found response" do + before do + Excon.stub({}, { status: 404 }) + end + + it "returns nil" do + result = service.lookup("A00 0AA") + expect(result).to be_nil + end + + it "logs the error at warning level" do + expect(Rails.logger).to receive(:warn).with(match "404 Not Found") + service.lookup("A00 0AA") + end + end + + context "when the request returns an error response" do + before do + Excon.stub({}, { body: "This is an error message that is not valid json", status: 500 }) + end + + it "returns nil" do + result = service.lookup("A00 0AA") + expect(result).to be_nil + end + + it "logs the error at warning level" do + expect(Rails.logger).to receive(:warn).with(match "This is an error message that is not valid json") + service.lookup("A00 0AA") + end + end + + context "when the request returns a success response that causes later errors" do + before do + Excon.stub({}, { body: '{"result": { "admin_district": "District" } }', status: 200 }) + end + + it "returns nil" do + result = service.lookup("A00 0AA") + expect(result).to be_nil + end + + it "logs the error at error level" do + expect(Rails.logger).to receive(:error).with(match "Unexpected error with postcode lookup request") + service.lookup("A00 0AA") + end + end end end