diff --git a/app/controllers/api/json/geocodings_controller.rb b/app/controllers/api/json/geocodings_controller.rb index 35c49a7a5b..63a5892ae8 100644 --- a/app/controllers/api/json/geocodings_controller.rb +++ b/app/controllers/api/json/geocodings_controller.rb @@ -60,9 +60,8 @@ class Api::Json::GeocodingsController < Api::ApplicationController end def estimation_for - dataset = current_user.in_database.select.from(params[:table_name]) - dataset = dataset.where(cartodb_georef_status: nil) if dataset.columns.include?(:cartodb_georef_status) - total_rows = dataset.count + table = current_user.tables.where(name: params[:table_name]).first + total_rows = Geocoding.processable_rows(table) remaining_blocks = (current_user.geocoding_quota - current_user.get_geocoding_calls) / User::GEOCODING_BLOCK_SIZE remaining_blocks = (remaining_blocks > 0 ? remaining_blocks : 0) needed_blocks = (total_rows / User::GEOCODING_BLOCK_SIZE) - remaining_blocks diff --git a/app/models/geocoding.rb b/app/models/geocoding.rb index ec8f080a2e..258f96d20e 100644 --- a/app/models/geocoding.rb +++ b/app/models/geocoding.rb @@ -8,7 +8,7 @@ class Geocoding < Sequel::Model ALLOWED_KINDS = %w(admin0 admin1 namedplace postalcode high-resolution ipaddress) PUBLIC_ATTRIBUTES = [:id, :table_id, :state, :kind, :country_code, :formatter, :geometry_type, - :error, :processed_rows, :cache_hits] + :error, :processed_rows, :cache_hits, :processable_rows, :real_rows] many_to_one :user many_to_one :table @@ -68,28 +68,37 @@ class Geocoding < Sequel::Model end # cancel def run! - self.update(state: 'started') + self.update state: 'started', processable_rows: self.class.processable_rows(table) + rows_geocoded_before = table.owner.in_database.select.from(table.name).where(cartodb_georef_status: true).count rescue 0 table_geocoder.run self.update remote_id: table_geocoder.remote_id started = Time.now begin self.update(table_geocoder.update_geocoding_status) puts "Processed: #{processed_rows}" - raise "Geocoding timeout" if Time.now - started > run_timeout and ['started', 'submitted', 'accepted'].include? state + raise 'Geocoding timeout' if Time.now - started > run_timeout and ['started', 'submitted', 'accepted'].include? state + raise 'Geocoding failed' if state == 'failed' sleep(2) - end until ['completed', 'cancelled', 'failed'].include? state + end until ['completed', 'cancelled'].include? state return false if state == 'cancelled' self.update(cache_hits: table_geocoder.cache.hits) if table_geocoder.respond_to?(:cache) Statsd.gauge("geocodings.requests", "+#{self.processed_rows}") rescue nil Statsd.gauge("geocodings.cache_hits", "+#{self.cache_hits}") rescue nil - table_geocoder.process_results + table_geocoder.process_results if state == 'completed' create_automatic_geocoding if automatic_geocoding_id.blank? - self.update(state: 'finished') + rows_geocoded_after = table.owner.in_database.select.from(table.name).where(cartodb_georef_status: true).count rescue 0 + self.update(state: 'finished', real_rows: rows_geocoded_after - rows_geocoded_before) rescue => e self.update(state: 'failed', processed_rows: 0, cache_hits: 0) CartoDB::notify_exception(e, user: user) end # run! + def self.processable_rows(table) + dataset = table.owner.in_database.select.from(table.name) + dataset = dataset.where(cartodb_georef_status: nil) if dataset.columns.include?(:cartodb_georef_status) + dataset.count + end # self.processable_rows + def create_automatic_geocoding # Disabled until we stop sending previously failed rows # best way to do this: use append mode on synchronizations diff --git a/db/migrate/20140410145007_add_processable_rows_to_geocodings.rb b/db/migrate/20140410145007_add_processable_rows_to_geocodings.rb new file mode 100644 index 0000000000..17a083e51c --- /dev/null +++ b/db/migrate/20140410145007_add_processable_rows_to_geocodings.rb @@ -0,0 +1,7 @@ +Sequel.migration do + change do + alter_table :geocodings do + add_column :processable_rows, :bigint + end + end +end diff --git a/db/migrate/20140410145024_add_real_rows_to_geocodings.rb b/db/migrate/20140410145024_add_real_rows_to_geocodings.rb new file mode 100644 index 0000000000..35d7b7f9c0 --- /dev/null +++ b/db/migrate/20140410145024_add_real_rows_to_geocodings.rb @@ -0,0 +1,7 @@ +Sequel.migration do + change do + alter_table :geocodings do + add_column :real_rows, :bigint + end + end +end diff --git a/spec/models/geocoding_spec.rb b/spec/models/geocoding_spec.rb index 181df22610..a7a0b55672 100644 --- a/spec/models/geocoding_spec.rb +++ b/spec/models/geocoding_spec.rb @@ -31,12 +31,12 @@ describe Geocoding do end it 'returns an instance of InternalGeocoder when kind is not high-resolution' do - geocoding = FactoryGirl.build(:geocoding, user: @user, table: @table, kind: 'admin0') + geocoding = FactoryGirl.build(:geocoding, user: @user, table: @table, kind: 'admin0', geometry_type: 'point') geocoding.table_geocoder.should be_kind_of(CartoDB::InternalGeocoder) end it 'memoizes' do - geocoding = FactoryGirl.build(:geocoding, user: @user, table: @table, kind: 'admin0') + geocoding = FactoryGirl.build(:geocoding, user: @user, table: @table, kind: 'admin0', geometry_type: 'point') geocoder = geocoding.table_geocoder geocoder.should be_kind_of(CartoDB::InternalGeocoder) geocoder.should eq geocoding.table_geocoder @@ -47,11 +47,13 @@ describe Geocoding do let(:geocoding) { FactoryGirl.build(:geocoding, user: @user, table: @table) } it 'validates formatter' do + geocoding.raise_on_save_failure = true expect { geocoding.save }.to raise_error(Sequel::ValidationFailed) geocoding.errors[:formatter].join(',').should match /is not present/ end it 'validates kind' do + geocoding.raise_on_save_failure = true geocoding.kind = 'nonsense' expect { geocoding.save }.to raise_error(Sequel::ValidationFailed) geocoding.errors[:kind].join(',').should match /is not in range or set/ @@ -83,7 +85,7 @@ describe Geocoding do end describe '#run!' do - it 'updates processed_rows, cache_hits and state' do + it 'updates geocoding stats' do geocoding = FactoryGirl.create(:geocoding, user: @user, table: @table, formatter: 'b') geocoding.table_geocoder.stubs(:run).returns true geocoding.table_geocoder.stubs(:cache).returns OpenStruct.new(hits: 5) @@ -137,7 +139,7 @@ describe Geocoding do @user.stubs('hard_geocoding_limit?').returns(true) delete_user_data @user geocoding.max_geocodable_rows.should eq 200 - FactoryGirl.create(:geocoding, user: @user, processed_rows: 100) + FactoryGirl.create(:geocoding, user: @user, processed_rows: 100, remote_id: 'wadus') geocoding.max_geocodable_rows.should eq 100 end diff --git a/spec/requests/api/geocodings_spec.rb b/spec/requests/api/geocodings_spec.rb index aab2f86029..c6bcbe91f7 100644 --- a/spec/requests/api/geocodings_spec.rb +++ b/spec/requests/api/geocodings_spec.rb @@ -64,7 +64,7 @@ describe "Geocodings API" do describe 'GET /api/v1/geocodings' do it 'returns every geocoding belonging to current_user' do FactoryGirl.create(:geocoding, table_name: 'a', formatter: 'b', user: @user, state: 'wadus') - FactoryGirl.create(:geocoding, table_name: 'a', formatter: 'b', user_id: @user.id+1) + FactoryGirl.create(:geocoding, table_name: 'a', formatter: 'b', user_id: UUIDTools::UUID.timestamp_create.to_s) get_json v1_geocodings_url(params) do |response| response.status.should be_success response.body[:geocodings].size.should == 1 @@ -74,8 +74,8 @@ describe "Geocodings API" do describe 'GET /api/v1/geocodings/:id' do it 'returns a geocoding' do - geocoding = FactoryGirl.create(:geocoding, table_id: 1, formatter: 'b', user: @user) - FactoryGirl.create(:geocoding, table_id: 2, formatter: 'b', user_id: @user.id+1) + geocoding = FactoryGirl.create(:geocoding, table_id: UUIDTools::UUID.timestamp_create.to_s, formatter: 'b', user: @user) + FactoryGirl.create(:geocoding, table_id: UUIDTools::UUID.timestamp_create.to_s, formatter: 'b', user_id: UUIDTools::UUID.timestamp_create.to_s) get_json v1_geocoding_url(params.merge(id: geocoding.id)) do |response| response.status.should be_success @@ -84,7 +84,7 @@ describe "Geocodings API" do end it 'does not return a geocoding owned by another user' do - geocoding = FactoryGirl.create(:geocoding, table_id: 1, formatter: 'b', user_id: @user.id + 1) + geocoding = FactoryGirl.create(:geocoding, table_id: UUIDTools::UUID.timestamp_create.to_s, formatter: 'b', user_id: UUIDTools::UUID.timestamp_create.to_s) get_json v1_geocoding_url(params.merge(id: geocoding.id)) do |response| response.status.should eq 404 @@ -94,7 +94,7 @@ describe "Geocodings API" do describe 'PUT /api/v1/geocodings/:id' do it 'cancels a geocoding job' do - geocoding = FactoryGirl.create(:geocoding, table_id: 2, formatter: 'b', user: @user) + geocoding = FactoryGirl.create(:geocoding, table_id: UUIDTools::UUID.timestamp_create.to_s, formatter: 'b', user: @user) Geocoding.any_instance.stubs(:cancel).returns(true) put_json v1_geocoding_url(params.merge(id: geocoding.id)), { state: 'cancelled' } do |response| @@ -105,7 +105,7 @@ describe "Geocodings API" do end it 'fails gracefully on job cancel failure' do - geocoding = FactoryGirl.create(:geocoding, table_id: 1, formatter: 'b', user: @user) + geocoding = FactoryGirl.create(:geocoding, table_id: UUIDTools::UUID.timestamp_create.to_s, formatter: 'b', user: @user) Geocoding.any_instance.stubs(:cancel).raises('wadus') put_json v1_geocoding_url(params.merge(id: geocoding.id)), { state: 'cancelled' } do |response|