CLDC-3244: Replace postcodes_io gem to improve error handling (#2634)

* CLDC-3244: Replace postcodes_io gem to improve error handling

* Fix status code in default postcodes api mocking

* Fix more mocking

* Remove blank line

* Handle non-Excon errors, in case of unexpected api behaviour

* Remove outdated test

* Remove another outdated test

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"

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)

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)

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

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 })
 

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 })
 

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: {})

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