Fixes at import notifications #11269

app/mailers/import_mailer.rb

@@ -11,8 +11,13 @@ class ImportMailer < ActionMailer::Base
     @subject = set_subject
     @first_table = first_imported_table.nil? ? first_table : first_imported_table
     @username = user.username
-    @files = filenames
-    @link = first_imported_table.nil? ? "#{user.public_url}#{CartoDB.path(self, 'tables_index')}" : "#{user.public_url}#{CartoDB.path(self, 'public_tables_show', { id: @first_table['name'] })}"
+    @files = filenames || []
+    @dataset_name = @first_table && @first_table['name'].present? ? @first_table['name'] : @files.first
+    @link = if first_imported_table.nil?
+              "#{user.public_url}#{CartoDB.path(self, 'tables_index')}"
+            else
+              "#{user.public_url}#{CartoDB.path(self, 'public_tables_show', { id: @dataset_name })}"
+            end
 
     mail :to => user.email,
          :subject => @subject

app/models/data_import.rb

@@ -401,6 +401,10 @@ class DataImport < Sequel::Model
     errors.add(:user, "Viewer users can't create data imports") if user && user.viewer
   end
 
+  def final_state?
+    [STATE_COMPLETE, STATE_FAILURE, STATE_STUCK].include? state
+  end
+
   private
 
   def dispatch

app/views/import_mailer/data_import_finished.html.erb

@@ -1,6 +1,6 @@
 <% if @total_tables == 1 %>
   <% if @errors.nil? %>
-    <%- message = "<p>Your dataset <strong>#{@first_table['name']}</strong> was successfully imported</p>" %>
+    <%- message = "<p>Your dataset <strong>#{@dataset_name}</strong> was successfully imported</p>" %>
   <% else %>
      <%- message = "<p>There was some error while importing your dataset <strong>#{@files.first}</strong></p>" %>
   <% end %>

services/importer/lib/importer/mail_notifier.rb

@@ -19,7 +19,7 @@ module CartoDB
       end
 
       def should_notify?
-        import_time >= MIN_IMPORT_TIME_TO_NOTIFY && @data_import.synchronization_id.nil?
+        import_time >= MIN_IMPORT_TIME_TO_NOTIFY && @data_import.synchronization_id.nil? && @data_import.final_state?
       end
 
       def import_time
@@ -33,7 +33,8 @@ module CartoDB
         total_tables = @results.length
         first_imported_table = imported_tables == 0 ? nil : @results.select {|r| r.success }.first
         first_table = @results.first
-        errors = imported_tables == total_tables ? nil : @data_import.get_error_text
+        error_text = @data_import.get_error_text
+        errors = error_text.present? ? error_text : nil
         @mail_sent = @resque.enqueue(::Resque::UserJobs::Mail::DataImportFinished,
                                      user_id, imported_tables, total_tables, first_imported_table,
                                      first_table, errors, filenames)

services/importer/spec/unit/mail_notifier_spec.rb

@@ -1,5 +1,6 @@
 # encoding: UTF-8
 
+require 'spec_helper_min'
 require_relative '../../lib/importer/mail_notifier'
 require_relative '../../../../spec/rspec_configuration.rb'
 require          'active_support/core_ext' # Needed for string.blank?
@@ -9,7 +10,7 @@ describe CartoDB::Importer2::MailNotifier do
   START_TIME = 0
 
   before(:each) do
-    @data_import = mock
+    @data_import = FactoryGirl.build(:data_import)
     @resque = mock
     @result = mock
     results = [@result]
@@ -25,12 +26,15 @@ describe CartoDB::Importer2::MailNotifier do
 
     it 'should send a mail if the import took more than MIN_IMPORT_TIME_TO_NOTIFY' do
       @data_import.stubs(:synchronization_id).once.returns(nil)
+      error_text = { title: 'error stubbing' }
+      @data_import.expects(:get_error_text).once.returns(error_text)
       set_import_duration(CartoDB::Importer2::MailNotifier::MIN_IMPORT_TIME_TO_NOTIFY + 1)
       @data_import.stubs(:user_id).once.returns(:any_user_id)
       @data_import.stubs(:stats).returns('[]')
       @data_import.stubs(:service_item_id).returns('filename.txt')
       @result.stubs(:success).returns(true)
-      @resque.expects(:enqueue).with(::Resque::UserJobs::Mail::DataImportFinished, :any_user_id, 1, 1, @result, @result, nil, ['filename.txt']).returns(true)
+      enqueue_params = :any_user_id, 1, 1, @result, @result, error_text, ['filename.txt']
+      @resque.expects(:enqueue).with(::Resque::UserJobs::Mail::DataImportFinished, *enqueue_params).returns(true)
 
       @mail_notifier.notify_if_needed
 
@@ -39,15 +43,26 @@ describe CartoDB::Importer2::MailNotifier do
   end
 
   describe '#should_notify?' do
-    it 'should return true if the import took more than MIN_IMPORT_TIME_TO_NOTIFY and was not a sync' do
+    def stub_notifiable_data_import
       set_import_duration(CartoDB::Importer2::MailNotifier::MIN_IMPORT_TIME_TO_NOTIFY + 1)
       @data_import.stubs(:synchronization_id).once.returns(nil)
       @data_import.stubs(:stats).returns('[]')
       @data_import.stubs(:service_item_id).returns('filename.txt')
+    end
+
+    it 'should return true if the import took more than MIN_IMPORT_TIME_TO_NOTIFY and was not a sync' do
+      stub_notifiable_data_import
 
       @mail_notifier.should_notify?.should == true
     end
 
+    it 'should return false if the import state is not finished' do
+      stub_notifiable_data_import
+      @data_import.stubs(:state).returns('pending')
+
+      @mail_notifier.should_notify?.should == false
+    end
+
     it 'should return false if the import took less than MIN_IMPORT_TIME_TO_NOTIFY' do
       set_import_duration(CartoDB::Importer2::MailNotifier::MIN_IMPORT_TIME_TO_NOTIFY - 1)
 
@@ -68,7 +83,10 @@ describe CartoDB::Importer2::MailNotifier do
       @data_import.stubs(:user_id).once.returns(:any_user_id)
       @data_import.stubs(:stats).returns('[]')
       @data_import.stubs(:service_item_id).returns('filename.txt')
-      @resque.expects(:enqueue).with(::Resque::UserJobs::Mail::DataImportFinished, :any_user_id, 1, 1, @result, @result, nil, ['filename.txt']).returns(true)
+      error_text = { title: 'error stubbing' }
+      @data_import.expects(:get_error_text).once.returns(error_text)
+      enqueue_params = :any_user_id, 1, 1, @result, @result, error_text, ['filename.txt']
+      @resque.expects(:enqueue).with(::Resque::UserJobs::Mail::DataImportFinished, *enqueue_params).returns(true)
       @result.stubs(:success).returns(true)
       @mail_notifier.send!
       @mail_notifier.mail_sent?.should == true