Background and rate-limit geocoding

Gemfile

@@ -49,6 +49,7 @@ gem 'uglifier'
 gem 'whenever'
 gem 'will_paginate'
 gem 'will_paginate-bootstrap'
+gem 'with_advisory_lock'
 gem 'zip-zip'
 
 # this needs to be down here due to load order weirdness

Gemfile.lock

@@ -429,6 +429,9 @@ GEM
     will_paginate (3.0.7)
     will_paginate-bootstrap (1.0.1)
       will_paginate (>= 3.0.3)
+    with_advisory_lock (3.0.0)
+      activerecord (>= 3.2)
+      thread_safe
     zip-zip (0.3)
       rubyzip (>= 1.0.0)
 
@@ -501,6 +504,7 @@ DEPENDENCIES
   whenever
   will_paginate
   will_paginate-bootstrap
+  with_advisory_lock
   zip-zip
 
 BUNDLED WITH

app/jobs/geocoder_job.rb

@@ -0,0 +1,66 @@
+class GeocoderJob < ActiveJob::Base
+  queue_as :geocode
+
+  class GeocodingError < StandardError; end
+
+  SLEEP_MULTIPLIER = 3
+  MAX_SLEEP_TIME = 12.hours
+  MAX_FAILURE_COUNT = 2
+
+  def perform(site, model_type, model_id)
+    @delay = 1
+    @error_count = 0
+    ActiveRecord::Base.connection_pool.with_connection do
+      Site.with_current(site) do
+        klass = model_type.constantize
+        klass.with_advisory_lock('geocode') do # only one at a time
+          model = klass.find(model_id)
+          geocode_model(model)
+        end
+      end
+    end
+    sleep @delay unless Rails.env.test? # delay next GeocoderJob as a simple rate limit
+  end
+
+  def geocode_model(model)
+    model.geocode
+  rescue Geocoder::OverQueryLimitError
+    over_query_limit_error(model)
+    retry
+  rescue Geocoder::Error, Timeout::Error => e
+    general_error(model, e.message)
+    retry
+  else
+    model.dont_geocode = true
+    model.save(validate: false)
+  end
+
+  def over_query_limit_error(model)
+    @delay *= SLEEP_MULTIPLIER
+    if @delay > MAX_SLEEP_TIME
+      fail GeocodingError,
+           'Over geocoder rate limit for #{model.class.name} #{model.id}. Giving up.'
+    else
+      Rails.logger.warn(
+        "Over geocoder rate limit for #{model.class.name} #{model.id}. " \
+          "Sleeping for #{@delay} second(s)"
+      )
+      sleep @delay
+    end
+  end
+
+  def general_error(model, message)
+    @error_count += 1
+    if @error_count > MAX_FAILURE_COUNT
+      fail GeocodingError,
+           "Error geocoding for #{model.class.name} #{model.id}: #{message}. " \
+           "This is error number #{@error_count}. Giving up."
+    else
+      Rails.logger.warn(
+        "Error geocoding for #{model.class.name} #{model.id}: #{message}. " \
+          "This is error number #{@error_count}. Sleeping for 5 seconds."
+      )
+      sleep 5
+    end
+  end
+end

app/models/concerns/geocode.rb

@@ -6,30 +6,44 @@ module Geocode
 
     included do
       attr_accessor :dont_geocode
-      after_validation :geocode, if: :should_geocode?
-      after_validation :clear_lat_lon, if: :blank_address?
+      after_save :geocode_later, if: :should_geocode?
     end
 
-    module ClassMethods
-      def geocode_with(location)
-        proc = method(:process_geocode_result).to_proc
-        geocoded_by location, &proc
-      end
+    def geocode_later
+      GeocoderJob.perform_later(site, self.class.name, id)
+    end
 
-      def process_geocode_result(model, results=nil)
-        if (geocoding_data = results.first) && geocoding_data.try(:precision) != 'APPROXIMATE'
-          model.latitude = geocoding_data.latitude
-          model.longitude = geocoding_data.longitude
+    def geocode
+      if blank_address?
+        self.latitude = nil
+        self.longitude = nil
+      else
+        results = Geocoder.search(geocoding_address)
+        if (result = results.first)
+          self.latitude = result.latitude
+          self.longitude = result.longitude
         else
-          model.latitude = nil
-          model.longitude = nil
+          self.latitude = nil
+          self.longitude = nil
         end
       end
     end
 
-    def clear_lat_lon
-      self.latitude = nil
-      self.longitude = nil
+    class_methods do
+      def geocode_with(*attrs)
+        define_method :geocoding_address do
+          attrs.map { |attr| send(attr) }.reject(&:blank?).join(', ')
+        end
+
+        define_method :blank_address? do
+          attrs.any? { |attr| send(attr).blank? }
+        end
+
+        define_method :should_geocode? do
+          return false if dont_geocode
+          attrs.any? { |attr| changed.include?(attr.to_s) }
+        end
+      end
     end
   end
 end

app/models/family.rb

@@ -68,21 +68,7 @@ def mapable?
   end
 
   include Concerns::Geocode
-  geocode_with :geocoding_address
-
-  def geocoding_address
-    [address, city, state, zip, country].map(&:presence).compact.join(', ')
-  end
-
-  def should_geocode?
-    return false if dont_geocode
-    changes = address1_changed? || address2_changed? || city_changed? || state_changed? || zip_changed?
-    changes && !blank_address?
-  end
-
-  def blank_address?
-    address1.blank? || city.blank? || state.blank?
-  end
+  geocode_with :address, :city, :state, :zip, :country
 
   # not HTML-escaped!
   def pretty_address

app/models/group.rb

@@ -94,27 +94,14 @@ def parents_of?
   end
 
   include Concerns::Geocode
-  geocode_with :location_with_country
+  geocode_with :location, :country
 
-  blank_to_nil :address
-
-  alias_attribute :pretty_address, :location
-
-  def location_with_country
-    [location, Setting.get(:system, :default_country)].join(", ")
-  end
-
-  def should_geocode?
-    return false if dont_geocode
-    location_changed? && !blank_address?
-  end
-
-  def blank_address?
-    location.blank?
+  def country
+    Setting.get(:system, :default_country)
   end
 
   def mapable?
-    latitude.to_f != 0.0 and longitude.to_f != 0.0
+    latitude.to_f != 0.0 && longitude.to_f != 0.0
   end
 
   def get_options_for(person)

config/initializers/geocoder.rb

@@ -1,3 +1,4 @@
 Geocoder.configure(
-  lookup: :google
+  lookup: :google,
+  always_raise: :all
 )

config/initializers/sucker_punch.rb

@@ -0,0 +1 @@
+require Rails.root.join('lib/sucker_punch/enqueue_at_monkey_patch')

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150905135659) do
+ActiveRecord::Schema.define(version: 20150906004322) do
 
   create_table "admins", force: :cascade do |t|
     t.datetime "created_at"
@@ -234,6 +234,17 @@
     t.string   "job_id",            limit: 50
   end
 
+  create_table "geocoding_jobs", force: :cascade do |t|
+    t.integer  "site_id",         limit: 4
+    t.integer  "geocodable_id",   limit: 4
+    t.string   "geocodable_type", limit: 255
+    t.datetime "created_at",                              null: false
+    t.datetime "updated_at",                              null: false
+    t.integer  "error_count",     limit: 4,   default: 0
+  end
+
+  add_index "geocoding_jobs", ["site_id", "created_at"], name: "index_geocoding_jobs_on_site_id_and_created_at", using: :btree
+
   create_table "group_times", force: :cascade do |t|
     t.integer  "group_id",            limit: 4
     t.integer  "checkin_time_id",     limit: 4

lib/sucker_punch/enqueue_at_monkey_patch.rb

@@ -0,0 +1,17 @@
+module ActiveJob
+  module QueueAdapters
+    class SuckerPunchAdapter
+      # TODO in Rails 5 this is an INSTANCE method -- not a class method
+      def self.enqueue_at(job, timestamp)
+        wait = timestamp.to_i - Time.now.to_i
+        JobWrapper.new.async.later(wait, job.serialize)
+      end
+
+      class JobWrapper
+        def later(sec, data)
+          after(sec) { perform(data) }
+        end
+      end
+    end
+  end
+end

spec/jobs/geocoder_job_spec.rb

@@ -0,0 +1,124 @@
+require_relative '../rails_helper'
+
+describe GeocoderJob do
+  let(:family) do
+    FactoryGirl.create(
+      :family,
+      address1: '650 S. Peoria',
+      city: 'Tulsa',
+      state: 'OK',
+      zip: '74120',
+      country: 'US',
+      dont_geocode: true
+    )
+  end
+
+  describe '#perform' do
+    context 'given a proper response from the geocoding service' do
+      before do
+        Geocoder::Lookup::Test.add_stub(
+          '650 S. Peoria, Tulsa, OK, 74120, US', [
+            {
+              'latitude'     => 36.151305,
+              'longitude'    => -95.975393,
+              'address'      => 'Tulsa, OK, USA',
+              'state'        => 'Oklahoma',
+              'state_code'   => 'OK',
+              'country'      => 'United States',
+              'country_code' => 'US'
+            }
+          ]
+        )
+        subject.perform(Site.current, 'Family', family.id)
+      end
+
+      it 'updates the latitude and longitude' do
+        expect(family.reload.attributes).to include(
+          'latitude'  => within(0.00001).of(36.151305),
+          'longitude' => within(0.00001).of(-95.975393)
+        )
+      end
+    end
+
+    context 'given a timeout error from the geocoding service' do
+      before do
+        allow(subject).to receive(:sleep)
+        call_count = 0
+        allow(Geocoder).to receive(:search) do
+          call_count += 1
+          if call_count < 3
+            fail Geocoder::OverQueryLimitError
+          else
+            [double('result', latitude: 36.151305, longitude: -95.975393)]
+          end
+        end
+        subject.perform(Site.current, 'Family', family.id)
+      end
+
+      it 'sleeps for a time before trying again' do
+        expect(subject).to have_received(:sleep).with(3)
+        expect(subject).to have_received(:sleep).with(9)
+        expect(Geocoder).to have_received(:search).exactly(3).times
+      end
+
+      it 'updates the latitude and longitude' do
+        expect(family.reload.attributes).to include(
+          'latitude'  => within(0.00001).of(36.151305),
+          'longitude' => within(0.00001).of(-95.975393)
+        )
+      end
+    end
+
+    context 'given a general error from the geocoding service' do
+      before do
+        allow(subject).to receive(:sleep)
+        call_count = 0
+        allow(Geocoder).to receive(:search) do
+          call_count += 1
+          if call_count <= error_count
+            fail Geocoder::RequestDenied
+          else
+            [double('result', latitude: 36.151305, longitude: -95.975393)]
+          end
+        end
+      end
+
+      context 'the error happens twice' do
+        let(:error_count) { 2 }
+
+        before do
+          subject.perform(Site.current, 'Family', family.id)
+        end
+
+        it 'sleeps twice before trying again' do
+          expect(subject).to have_received(:sleep).with(5).twice
+          expect(Geocoder).to have_received(:search).exactly(3).times
+        end
+
+        it 'updates the latitude and longitude' do
+          expect(family.reload.attributes).to include(
+            'latitude'  => within(0.00001).of(36.151305),
+            'longitude' => within(0.00001).of(-95.975393)
+          )
+        end
+      end
+
+      context 'the error happens three times' do
+        let(:error_count) { 3 }
+
+        it 'fails' do
+          expect {
+            subject.perform(Site.current, 'Family', family.id)
+          }.to raise_error(GeocoderJob::GeocodingError)
+        end
+
+        it 'does not update the latitude and longitude' do
+          expect(family.reload.attributes).to include(
+            'latitude'  => nil,
+            'longitude' => nil
+          )
+        end
+      end
+    end
+  end
+end