Refactor importer connector - extract TableRegistrar and QuotaChecker

app/connectors/importer.rb

@@ -1,5 +1,4 @@
 # encoding: utf-8
-require_relative '../../services/importer/lib/importer'
 
 module CartoDB
   module Connector
@@ -8,12 +7,12 @@ module CartoDB
 
       attr_accessor :table
 
-      def initialize(user, data_source, pg_options, log, table_klass)
-        @user         = user
-        @data_source  = data_source
-        @pg_options   = pg_options
-        @log          = log
-        @table_klass  = table_klass
+      def initialize(runner, downloader, table_registrar, quota_checker, database)
+        @runner           = runner
+        @downloader       = downloader
+        @table_registrar  = table_registrar
+        @quota_checker    = quota_checker
+        @database         = database
       end
 
       def run(tracker)
@@ -28,69 +27,53 @@ module CartoDB
         self
       end
 
+      def register(result)
+        move_to_schema(result, 'public')
+        rename(result.table_name, result.name)
+      end
+
       def success?
-        return false if table_quota_exceeded?
-        runner.success?
+        !table_quota_exceeded? && runner.success?
       end
 
       def drop_all(results)
-        results.each { |result| drop(result) }
+        results.each { |result| drop(result.qualified_table_name) }
       end
 
-      def drop(result)
-        statement = %Q{DROP TABLE #{result.qualified_table_name}}
-        user.in_database.execute(statement)
+      def drop(table_name)
+        database.execute(%Q(DROP TABLE #{table_name}))
       rescue
         self
       end
 
-      def register(result)
-        move_to_schema(result, 'public')
-        rename(result)
-      end
-
       def move_to_schema(result, schema=DEFAULT_SCHEMA)
         return self if schema == result.schema
-        user.in_database(as: :superuser).execute(%Q{
+        database.execute(%Q{
           ALTER TABLE "#{result.schema}"."#{result.table_name}"
           SET SCHEMA public
         })
       end
 
-      def rename(result, rename_attempts=0)
+      def rename(current_name, new_name, rename_attempts=0)
         rename_attempts = rename_attempts + 1
-        name            = name_candidate(result.name)
+        new_name        = table_registrar.get_valid_table_name(new_name)
 
-        user.in_database.execute(%Q{
-          ALTER TABLE "public"."#{result.table_name}"
-          RENAME TO "#{name}"
+        database.execute(%Q{
+          ALTER TABLE "public"."#{current_name}"
+          RENAME TO "#{new_name}"
         })
-        persist_metadata(name)
+        persist_metadata(new_name)
       rescue => exception
         retry unless rename_attempts > 1
       end
 
-      def name_candidate(table_name)
-        table_klass.get_valid_table_name(
-          table_name, name_candidates: user.reload.tables.map(&:name)
-        )
-      end
-
       def table_quota_exceeded?
-        return false unless user.remaining_table_quota
-        results.length > user.remaining_table_quota.to_i
+        quota_checker.over_table_quota?(results.length)
       end
 
       def persist_metadata(name)
-        table                         = table_klass.new
-        table.user_id                 = user.id
-        table.name                    = name
-        table.migrate_existing_table  = name
-        table.save
-        table.optimize
-        table.map.recalculate_bounds!
-
-        self.table = table
+        table_registrar.register(name)
+        self.table = table_registrar.table
         self
       end
 
@@ -105,16 +88,8 @@ module CartoDB
 
       private
 
-      attr_reader :user, :data_source, :pg_options, :log, :table_klass
-
-      def downloader
-        @downloader ||= Importer2::Downloader.new(data_source)
-      end
-
-      def runner
-        @runner ||= Importer2::Runner
-          .new(pg_options, downloader, log, user.remaining_quota)
-      end
+      attr_reader :runner, :downloader, :table_registrar, :quota_checker,
+                  :database
     end # Importer
   end # Connector
 end # CartoDB

app/models/data_import.rb

@@ -10,6 +10,7 @@ require_relative '../../lib/cartodb/github_reporter'
 require_relative '../../lib/cartodb_stats'
 require_relative '../../services/track_record/track_record/log'
 require_relative '../../config/initializers/redis'
+require_relative '../../services/importer/lib/importer'
 
 class DataImport < Sequel::Model
   REDIS_LOG_KEY_PREFIX          = 'importer'
@@ -262,10 +263,19 @@ class DataImport < Sequel::Model
   end #pg_options
 
   def new_importer
-    tracker   = lambda { |state| self.state = state; save }
-    importer  = CartoDB::Connector::Importer.new(
-      current_user, data_source, pg_options, log, Table
-    ).run(tracker)
+    tracker       = lambda { |state| self.state = state; save }
+    downloader    = CartoDB::Importer2::Downloader.new(data_source)
+    runner        = CartoDB::Importer2::Runner.new(
+                      pg_options, downloader, log, current_user.remaining_quota
+                    )
+    registrar     = CartoDB::TableRegistrar.new(current_user, Table)
+    quota_checker = CartoDB::QuotaChecker.new(current_user)
+    importer      = CartoDB::Connector::Importer.new(
+                      runner, downloader, registrar, quota_checker, 
+                      current_user.in_database
+                    )
+    
+    importer.run(tracker)
 
     self.results    = importer.results
     self.error_code = importer.error_code

app/models/quota_checker.rb

@@ -0,0 +1,19 @@
+# encoding: utf-8
+
+module CartoDB
+  class QuotaChecker
+    def initialize(user)
+      @user = user
+    end 
+
+    def over_table_quota?(number_of_new_tables)
+      return false unless user.remaining_table_quota
+      number_of_new_tables.to_i > user.remaining_table_quota.to_i
+    end
+
+    private
+
+    attr_reader :user
+  end # QuotaChecker
+end # CartoDB
+

app/models/table_registrar.rb

@@ -0,0 +1,39 @@
+# encoding: utf-8
+
+module CartoDB
+  class TableRegistrar
+    def initialize(user, table_klass=nil)
+      @user         = user
+      @table_klass  = table_klass
+    end
+
+    def register(table_name)
+      self.table                    = table_klass.new
+      table.user_id                 = user.id
+      table.name                    = table_name
+      table.migrate_existing_table  = table_name
+      table.save
+      table.optimize
+      table.map.recalculate_bounds!
+    end
+
+    def exists?(user, table_name)
+      !table_klass.where(user_id: user.id, name: table_name).empty?
+    end 
+
+    def get_valid_table_name(table_name)
+      table_klass.get_valid_table_name(
+        table_name, 
+        name_candidates: user.reload.tables.map(&:name)
+      )
+    end
+
+    attr_reader :user, :table
+
+    private
+
+    attr_reader :table_klass
+    attr_writer :table
+  end # TableRegistrar
+end # CartoDB
+

services/importer/lib/importer/runner.rb

@@ -32,7 +32,7 @@ module CartoDB
         log.append "Getting file from #{downloader.url}"
         downloader.run(available_quota)
 
-        return self unless modified?
+        return self unless remote_data_updated?
 
         log.append "Starting import for #{downloader.source_file.fullpath}"
         log.append "Unpacking #{downloader.source_file.fullpath}"
@@ -85,7 +85,7 @@ module CartoDB
         }
       end #loader_for
 
-      def modified?
+      def remote_data_updated?
         downloader.modified?
       end
 
@@ -94,6 +94,7 @@ module CartoDB
       end #tracker
 
       def success?
+        return true unless remote_data_updated?
         results.select(&:success?).length > 0
       end
 

spec/models/data_import_spec.rb

@@ -1,6 +1,5 @@
 # encoding: utf-8
 require_relative '../spec_helper'
-require 'ruby-debug'
 
 describe DataImport do
   before(:each) do
@@ -53,7 +52,7 @@ describe DataImport do
     duplicated_table.records[:rows].should have(5).items
   end
 
-  it 'imports a simple file', now: true do
+  it 'imports a simple file' do
     data_import = DataImport.create(
       :user_id       => @user.id,
       :data_source   => '/../db/fake_data/clubbing.csv',