Merge pull request #343 from alphagov/fix-encoding-errors-with-consti…

…tuency-api

Refactor constituency api to eliminate encoding errors

app/controllers/local_petitions_controller.rb

@@ -22,9 +22,7 @@ def postcode?
   end
 
   def find_constituency
-    @constituency = ConstituencyApi::Client.constituency(@postcode)
-  rescue ConstituencyApi::Error => e
-    Rails.logger.error("Failed to fetch constituency - #{e}")
+    @constituency = ConstituencyApi.constituency(@postcode)
   end
 
   def constituency?

app/lib/constituency_api.rb

@@ -1,8 +1,8 @@
+require 'faraday'
+require 'nokogiri'
 require 'postcode_sanitizer'
 
 module ConstituencyApi
-  class Error < RuntimeError; end
-
   class Constituency
     attr_reader :id, :name, :mp
 
@@ -18,73 +18,121 @@ def ==(other)
 
   class Mp
     attr_reader :id, :name, :start_date
-    URL = "http://www.parliament.uk/biographies/commons"
+    URL = "http://www.parliament.uk/biographies/commons/%{name}/%{id}"
 
     def initialize(id, name, start_date)
       @id, @name, @start_date = id, name, start_date.to_date
     end
 
     def url
-      "#{URL}/#{name.parameterize}/#{id}"
+      URL % { name: name.parameterize, id: id }
     end
 
     def ==(other)
-      other.is_a?(self.class) && id == other.id && name == other.name && start_date && other.start_date
+      other.is_a?(self.class) && id == other.id && name == other.name && start_date == other.start_date
     end
     alias_method :eql?, :==
   end
 
   class Client
-    include Faraday
-    URL = 'http://data.parliament.uk/membersdataplatform/services/mnis/Constituencies'
+    API_HOST = 'http://data.parliament.uk'
+    API_ENDPOINT = '/membersdataplatform/services/mnis/Constituencies/%{postcode}/'
     TIMEOUT = 5
 
-    def self.constituency(postcode)
-      constituencies(postcode).first
+    def call(postcode)
+      faraday.get(path(postcode)) do |request|
+        request.options[:timeout] = TIMEOUT
+        request.options[:open_timeout] = TIMEOUT
+      end
     end
 
-    def self.constituencies(postcode)
-      response = call_api(postcode)
-      parse_constituencies(response)
+    private
+
+    def faraday
+      Faraday.new(API_HOST) do |f|
+        f.response :follow_redirects
+        f.response :raise_error
+        f.adapter Faraday.default_adapter
+      end
     end
 
-    def self.parse_constituencies(response)
-      return [] unless response["Constituencies"]
-      constituencies = response["Constituencies"]["Constituency"]
-      Array.wrap(constituencies).map { |c| Constituency.new(c["Constituency_Id"], c["Name"], last_mp(c)) }
+    def path(postcode)
+      API_ENDPOINT % { postcode: escape_path(postcode) }
     end
 
-    def self.call_api(postcode)
-      sanitized_postcode = PostcodeSanitizer.call(postcode)
-      response = Faraday.new(URL).get("#{sanitized_postcode}/") do |req|
-        req.options[:timeout] = TIMEOUT
-        req.options[:open_timeout] = TIMEOUT
-      end
-      unless response.status == 200
-        raise Error.new("Unexpected response from API:"\
-                        "status #{response.status}"\
-                        "body #{response.body}"\
-                        "request #{URL}/#{sanitized_postcode}/")
+    def escape_path(value)
+      Rack::Utils.escape_path(value)
+    end
+  end
+
+  class Query
+    CONSTITUENCIES    = '//Constituencies/Constituency'
+    CONSTITUENCY_ID   = './Constituency_Id'
+    CONSTITUENCY_NAME = './Name'
+
+    CURRENT_MP = './RepresentingMembers/RepresentingMember[1]'
+    MP_ID      = './Member_Id'
+    MP_NAME    = './Member'
+    MP_DATE    = './StartDate'
+
+    def initialize(postcode)
+      @postcode = postcode
+    end
+
+    def fetch
+      response = client.call(postcode)
+
+      if response.success?
+        parse(response.body)
+      else
+        []
       end
-      Hash.from_xml(response.body)
-    rescue Faraday::Error::TimeoutError
-      raise Error.new("Timeout after #{TIMEOUT} seconds")
+    rescue Faraday::Error::ResourceNotFound => e
+      return []
     rescue Faraday::Error => e
-      raise Error.new("Network error - #{e}")
+      Appsignal.send_exception(e) if defined?(Appsignal)
+      return []
     end
 
-    def self.last_mp(constituency_hash)
-      mps = parse_mps(constituency_hash)
-      mps.select(&:start_date).sort_by(&:start_date).last
+    private
+
+    def client
+      @client ||= Client.new
     end
 
-    def self.parse_mps(response)
-      return [] unless response["RepresentingMembers"]
-      mps = response["RepresentingMembers"]["RepresentingMember"]
-      Array.wrap(mps).map { |m| Mp.new(m["Member_Id"], m["Member"], m["StartDate"]) }
+    def postcode
+      PostcodeSanitizer.call(@postcode)
     end
 
-    private_class_method :parse_constituencies, :call_api, :parse_mps, :last_mp
+    def parse(body)
+      xml = Nokogiri::XML(body)
+
+      xml.xpath(CONSTITUENCIES).map do |node|
+        id   = node.xpath(CONSTITUENCY_ID).text
+        name = node.xpath(CONSTITUENCY_NAME).text
+
+        if mp = node.at_xpath(CURRENT_MP)
+          Constituency.new(id, name, parse_mp(mp))
+        else
+          Constituency.new(id, name)
+        end
+      end
+    end
+
+    def parse_mp(node)
+      id   = node.xpath(MP_ID).text
+      name = node.xpath(MP_NAME).text
+      date = node.xpath(MP_DATE).text
+
+      Mp.new(id, name, date)
+    end
+  end
+
+  def self.constituency(postcode)
+    constituencies(postcode).first
   end
-end
 
+  def self.constituencies(postcode)
+    Query.new(postcode).fetch
+  end
+end

app/models/signature.rb

@@ -130,10 +130,7 @@ def invalid_unsubscribe_token?
   end
 
   def constituency
-    @constituency ||= ConstituencyApi::Client.constituencies(self.postcode).first
-  rescue ConstituencyApi::Error => e
-    Rails.logger.error("Failed to fetch constituency - #{e}")
-    nil
+    @constituency ||= ConstituencyApi.constituency(postcode)
   end
 
   def set_constituency_id

spec/controllers/local_petitions_controller_spec.rb

@@ -68,7 +68,7 @@
 
     shared_examples_for 'a local petitions controller that does not try to lookup a constituency' do
       it 'does not communicate with the API' do
-        expect(ConstituencyApi::Client).not_to receive(:constituency)
+        expect(ConstituencyApi).not_to receive(:constituency)
         get :index, params
       end
 

spec/lib/constituency_api_spec.rb

@@ -14,13 +14,13 @@
       let(:mp) { ConstituencyApi::Mp.new("1536", "Emily Thornberry MP", Date.new(2015, 5, 7)) }
 
       it "returns the URL for the mp" do
-        expect(mp.url).to eq "#{ConstituencyApi::Mp::URL}/emily-thornberry-mp/1536"
+        expect(mp.url).to eq "http://www.parliament.uk/biographies/commons/emily-thornberry-mp/1536"
       end
     end
   end
 
   describe "query methods" do
-    let(:api) { ConstituencyApi::Client }
+    let(:api) { ConstituencyApi }
     let(:url) { "http://data.parliament.uk" }
     let(:endpoint) { "#{url}/membersdataplatform/services/mnis/Constituencies" }
 
@@ -162,8 +162,8 @@ def constituency(id, name, mp = nil, &block)
           stub_request(:get, /.*data.parliament.uk.*/).to_timeout
         end
 
-        it "raises a ConstituencyApi::Error" do
-          expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error)
+        it "returns an empty array" do
+          expect(api.constituencies("N1")).to eq([])
         end
       end
 
@@ -172,8 +172,8 @@ def constituency(id, name, mp = nil, &block)
           stub_request(:get, /.*data.parliament.uk.*/).to_raise(Faraday::Error::ConnectionFailed)
         end
 
-        it "raises a ConstituencyApi::Error" do
-          expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error)
+        it "returns an empty array" do
+          expect(api.constituencies("N1")).to eq([])
         end
       end
 
@@ -182,8 +182,8 @@ def constituency(id, name, mp = nil, &block)
           stub_request(:get, /.*data.parliament.uk.*/).to_raise(Faraday::Error::ResourceNotFound)
         end
 
-        it "raises a ConstituencyApi::Error" do
-          expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error)
+        it "returns an empty array" do
+          expect(api.constituencies("N1")).to eq([])
         end
       end
 
@@ -192,8 +192,8 @@ def constituency(id, name, mp = nil, &block)
           stub_request(:get, /.*data.parliament.uk.*/).to_return(status: 500, body: "<Constituencies/>")
         end
 
-        it "raises a ConstituencyApi::Error" do
-          expect{ api.constituencies("N1") }.to raise_error(ConstituencyApi::Error)
+        it "returns an empty array" do
+          expect(api.constituencies("N1")).to eq([])
         end
       end
     end

spec/support/constituency_api_helpers.rb

@@ -9,37 +9,48 @@ def stub_constituency(postcode, *constituency_params)
         else
           ConstituencyApi::Constituency.new(*constituency_params)
         end
-      allow(ConstituencyApi::Client).to receive(:constituencies).
-                                        with(PostcodeSanitizer.call(postcode)).
-                                        and_return([constituency])
+      allow(ConstituencyApi).to receive(:constituencies).
+                                  with(PostcodeSanitizer.call(postcode)).
+                                  and_return([constituency])
     end
 
     def stub_constituencies(partial_postcode, *constituencies)
-      allow(ConstituencyApi::Client).to receive(:constituencies).
-                                        with(PostcodeSanitizer.call(partial_postcode)).
-                                        and_return(constituencies)
+      allow(ConstituencyApi).to receive(:constituencies).
+                                  with(PostcodeSanitizer.call(partial_postcode)).
+                                  and_return(constituencies)
 
     end
 
     def stub_no_constituencies(postcode)
-      allow(ConstituencyApi::Client).to receive(:constituencies).
-                                        with(PostcodeSanitizer.call(postcode)).
-                                        and_return([])
+      allow(ConstituencyApi).to receive(:constituencies).
+                                  with(PostcodeSanitizer.call(postcode)).
+                                  and_return([])
     end
 
     def stub_broken_api
-      allow(ConstituencyApi::Client).to receive(:constituencies).
-                                        and_raise(ConstituencyApi::Error)
+      allow(ConstituencyApi).to receive(:constituencies).and_return([])
     end
   end
 
   module NetworkLevel
-    def api_url
-      ConstituencyApi::Client::URL
+    def api_host
+      ConstituencyApi::Client::API_HOST
+    end
+
+    def api_endpoint
+      ConstituencyApi::Client::API_ENDPOINT
+    end
+
+    def api_url(postcode)
+      "#{api_host}#{api_endpoint}" % { postcode: sanitize_and_escape_postcode(postcode) }
+    end
+
+    def sanitize_and_escape_postcode(postcode)
+      Rack::Utils.escape_path(PostcodeSanitizer.call(postcode))
     end
 
     def stub_constituency_from_file(postcode, filename)
-      stub_request(:get, "#{ api_url }/#{PostcodeSanitizer.call(postcode)}/").to_return(status: 200, body: IO.read(filename))
+      stub_request(:get, api_url(postcode)).to_return(status: 200, body: IO.read(filename))
     end
 
     def stub_constituency(postcode, constituency_id, constituency_name, mp_id: '0001', mp_name: 'A. N. Other MP', mp_start_date: '2015-05-07T00:00:00')
@@ -61,15 +72,15 @@ def stub_constituency(postcode, constituency_id, constituency_name, mp_id: '0001
           </Constituencies>
         RESPONSE
 
-      stub_request(:get, "#{ api_url }/#{PostcodeSanitizer.call(postcode)}/").to_return(status: 200, body: api_response)
+      stub_request(:get, api_url(postcode)).to_return(status: 200, body: api_response)
     end
 
     def stub_no_constituencies(postcode)
       stub_constituency_from_file(postcode, Rails.root.join("spec", "fixtures", "constituency_api", "no_results.xml"))
     end
 
     def stub_broken_api
-      stub_request(:get, %r[#{ Regexp.escape(api_url) }/*]).to_return(status: 500)
+      stub_request(:get, %r[#{api_host}/*]).to_return(status: 500)
     end
   end
 end