Merge branch 'develop' into feat/contact-import-status

- Track actual imported contacts using database count differences
- Add non_identifiable_records tracking for contacts without email/phone/identifier
- Fix misleading email notifications that showed all contacts as "imported successfully"
- Enhanced import job to distinguish between identifiable, non-identifiable, and failed contacts
- Updated specs to match new accurate counting behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

app/jobs/data_import_job.rb

@@ -1,123 +1,176 @@
 # TODO: logic is written tailored to contact import since its the only import available
-# let's break this logic and clean this up in future
+  # let's break this logic and clean this up in future
 
-class DataImportJob < ApplicationJob
-  queue_as :low
-  retry_on ActiveStorage::FileNotFoundError, wait: 1.minute, attempts: 3
+  class DataImportJob < ApplicationJob
+    queue_as :low
+    retry_on ActiveStorage::FileNotFoundError, wait: 1.minute, attempts: 3
 
-  def perform(data_import)
-    @data_import = data_import
-    @contact_manager = DataImport::ContactManager.new(@data_import.account)
-    begin
-      process_import_file
-      send_import_notification_to_admin
-    rescue CSV::MalformedCSVError => e
-      handle_csv_error(e)
+    def perform(data_import)
+      @data_import = data_import
+      @contact_manager = DataImport::ContactManager.new(@data_import.account)
+      begin
+        process_import_file
+        send_import_notification_to_admin
+      rescue CSV::MalformedCSVError => e
+        handle_csv_error(e)
+      end
     end
-  end
 
-  private
+    private
 
-  def process_import_file
-    @data_import.update!(status: :processing)
-    contacts, rejected_contacts = parse_csv_and_build_contacts
+    def process_import_file
+      @data_import.update!(status: :processing)
+      identifiable_contacts, non_identifiable_contacts, rejected_contacts, total_csv_records = parse_csv_and_build_contacts
 
-    import_contacts(contacts)
-    update_data_import_status(contacts.length, rejected_contacts.length)
-    save_failed_records_csv(rejected_contacts)
-  end
+      all_valid_contacts = identifiable_contacts + non_identifiable_contacts
 
-  def parse_csv_and_build_contacts
-    contacts = []
-    rejected_contacts = []
+      # Count contacts before import
+      contacts_before = @data_import.account.contacts.count
 
-    with_import_file do |file|
-      csv_reader(file).each do |row|
-        current_contact = @contact_manager.build_contact(row.to_h.with_indifferent_access)
-        if current_contact.valid?
-          contacts << current_contact
-        else
-          append_rejected_contact(row, current_contact, rejected_contacts)
+      import_contacts(all_valid_contacts)
+
+      # Count contacts after import to get actual imported count
+      contacts_after = @data_import.account.contacts.count
+      actual_imported_count = contacts_after - contacts_before
+
+      # Calculate actual imported counts based on what we built vs what was imported
+      actual_identifiable_count, actual_non_identifiable_count = calculate_actual_import_counts(
+        identifiable_contacts, non_identifiable_contacts, actual_imported_count
+      )
+
+      update_data_import_status(actual_identifiable_count, actual_non_identifiable_count, rejected_contacts.length, total_csv_records)
+      save_failed_records_csv(rejected_contacts)
+    end
+
+    # rubocop:disable Metrics/MethodLength
+    def parse_csv_and_build_contacts
+      identifiable_contacts = []
+      non_identifiable_contacts = []
+      rejected_contacts = []
+      total_csv_records = 0
+
+      with_import_file do |file|
+        csv_reader(file).each do |row|
+          total_csv_records += 1
+          current_contact = @contact_manager.build_contact(row.to_h.with_indifferent_access)
+          if current_contact.valid?
+            is_identifiable = @contact_manager.contact_identifiable?(current_contact)
+
+            if is_identifiable
+              identifiable_contacts << current_contact
+            else
+              non_identifiable_contacts << current_contact
+            end
+          else
+            append_rejected_contact(row, current_contact, rejected_contacts)
+          end
+        end
+      end
+
+      [identifiable_contacts, non_identifiable_contacts, rejected_contacts, total_csv_records]
+    end
+    # rubocop:enable Metrics/MethodLength
+
+    def append_rejected_contact(row, contact, rejected_contacts)
+      row['errors'] = contact.errors.full_messages.join(', ')
+      rejected_contacts << row
+    end
+
+    def import_contacts(contacts)
+      # Returns ActiveRecord::Import::Result with info about what was actually imported
+      Contact.import(contacts, synchronize: contacts, on_duplicate_key_ignore: true, track_validation_failures: true, validate: true, batch_size: 1000)
+    end
+
+    def calculate_actual_import_counts(identifiable_contacts, non_identifiable_contacts, actual_imported_count)
+      Rails.logger.info "Actual imported: #{actual_imported_count}, expected identifiable: #{identifiable_contacts.length}, " \
+                        "expected non-identifiable: #{non_identifiable_contacts.length}"
+
+      expected_count = identifiable_contacts.length + non_identifiable_contacts.length
+
+      if actual_imported_count <= expected_count
+        # Prioritize identifiable contacts in the count since they're more important
+        actual_identifiable_count = [actual_imported_count, identifiable_contacts.length].min
+        actual_non_identifiable_count = actual_imported_count - actual_identifiable_count
+      else
+        # This shouldn't happen, but fallback to expected counts
+        actual_identifiable_count = identifiable_contacts.length
+        actual_non_identifiable_count = non_identifiable_contacts.length
+      end
+
+      Rails.logger.info "Final counts: identifiable=#{actual_identifiable_count}, non-identifiable=#{actual_non_identifiable_count}"
+
+      [actual_identifiable_count, actual_non_identifiable_count]
+    end
+
+    def update_data_import_status(identifiable_records, non_identifiable_records, _rejected_records, total_csv_records)
+      processed_records = identifiable_records + non_identifiable_records
+      @data_import.update!(
+        status: :completed,
+        processed_records: processed_records,
+        non_identifiable_records: non_identifiable_records,
+        total_records: total_csv_records
+      )
+    end
+
+    def save_failed_records_csv(rejected_contacts)
+      csv_data = generate_csv_data(rejected_contacts)
+      return if csv_data.blank?
+
+      @data_import.failed_records.attach(io: StringIO.new(csv_data), filename: "#{Time.zone.today.strftime('%Y%m%d')}_contacts.csv",
+                                         content_type: 'text/csv')
+      send_import_notification_to_admin
+    end
+
+    def generate_csv_data(rejected_contacts)
+      headers = csv_headers
+      headers << 'errors'
+      return if rejected_contacts.blank?
+
+      CSV.generate do |csv|
+        csv << headers
+        rejected_contacts.each do |record|
+          csv << record
         end
       end
     end
 
-    [contacts, rejected_contacts]
-  end
+    def handle_csv_error(error) # rubocop:disable Lint/UnusedMethodArgument
+      @data_import.update!(status: :failed)
+      send_import_failed_notification_to_admin
+    end
 
-  def append_rejected_contact(row, contact, rejected_contacts)
-    row['errors'] = contact.errors.full_messages.join(', ')
-    rejected_contacts << row
-  end
+    def send_import_notification_to_admin
+      AdministratorNotifications::AccountNotificationMailer.with(account: @data_import.account).contact_import_complete(@data_import).deliver_later
+    end
 
-  def import_contacts(contacts)
-    # <struct ActiveRecord::Import::Result failed_instances=[], num_inserts=1, ids=[444, 445], results=[]>
-    Contact.import(contacts, synchronize: contacts, on_duplicate_key_ignore: true, track_validation_failures: true, validate: true, batch_size: 1000)
-  end
+    def send_import_failed_notification_to_admin
+      AdministratorNotifications::AccountNotificationMailer.with(account: @data_import.account).contact_import_failed.deliver_later
+    end
 
-  def update_data_import_status(processed_records, rejected_records)
-    @data_import.update!(status: :completed, processed_records: processed_records, total_records: processed_records + rejected_records)
-  end
+    def csv_headers
+      header_row = nil
+      with_import_file do |file|
+        header_row = csv_reader(file).first
+      end
+      header_row&.headers || []
+    end
 
-  def save_failed_records_csv(rejected_contacts)
-    csv_data = generate_csv_data(rejected_contacts)
-    return if csv_data.blank?
+    def csv_reader(file)
+      file.rewind
+      raw_data = file.read
+      utf8_data = raw_data.force_encoding('UTF-8')
+      clean_data = utf8_data.valid_encoding? ? utf8_data : utf8_data.encode('UTF-16le', invalid: :replace, replace: '').encode('UTF-8')
 
-    @data_import.failed_records.attach(io: StringIO.new(csv_data), filename: "#{Time.zone.today.strftime('%Y%m%d')}_contacts.csv",
-                                       content_type: 'text/csv')
-    send_import_notification_to_admin
-  end
+      CSV.new(StringIO.new(clean_data), headers: true)
+    end
 
-  def generate_csv_data(rejected_contacts)
-    headers = csv_headers
-    headers << 'errors'
-    return if rejected_contacts.blank?
+    def with_import_file
+      temp_dir = Rails.root.join('tmp/imports')
+      FileUtils.mkdir_p(temp_dir)
 
-    CSV.generate do |csv|
-      csv << headers
-      rejected_contacts.each do |record|
-        csv << record
+      @data_import.import_file.open(tmpdir: temp_dir) do |file|
+        file.binmode
+        yield file
       end
     end
-  end
-
-  def handle_csv_error(error) # rubocop:disable Lint/UnusedMethodArgument
-    @data_import.update!(status: :failed)
-    send_import_failed_notification_to_admin
-  end
-
-  def send_import_notification_to_admin
-    AdministratorNotifications::AccountNotificationMailer.with(account: @data_import.account).contact_import_complete(@data_import).deliver_later
-  end
-
-  def send_import_failed_notification_to_admin
-    AdministratorNotifications::AccountNotificationMailer.with(account: @data_import.account).contact_import_failed.deliver_later
-  end
-
-  def csv_headers
-    header_row = nil
-    with_import_file do |file|
-      header_row = csv_reader(file).first
-    end
-    header_row&.headers || []
-  end
-
-  def csv_reader(file)
-    file.rewind
-    raw_data = file.read
-    utf8_data = raw_data.force_encoding('UTF-8')
-    clean_data = utf8_data.valid_encoding? ? utf8_data : utf8_data.encode('UTF-16le', invalid: :replace, replace: '').encode('UTF-8')
-
-    CSV.new(StringIO.new(clean_data), headers: true)
-  end
-
-  def with_import_file
-    temp_dir = Rails.root.join('tmp/imports')
-    FileUtils.mkdir_p(temp_dir)
-
-    @data_import.import_file.open(tmpdir: temp_dir) do |file|
-      file.binmode
-      yield file
-    end
-  end
-end
+  end

app/mailers/administrator_notifications/account_notification_mailer.rb

@@ -32,9 +32,16 @@ class AdministratorNotifications::AccountNotificationMailer < AdministratorNotif
                    "#{ENV.fetch('FRONTEND_URL', nil)}/app/accounts/#{resource.account.id}/contacts"
                  end
 
+    # Calculate metrics with backward compatibility
+    non_identifiable_count = resource.respond_to?(:non_identifiable_records) ? resource.non_identifiable_records : 0
+    identifiable_count = resource.processed_records - non_identifiable_count
+    failed_count = resource.total_records - resource.processed_records
+
     meta = {
-      'failed_contacts' => resource.total_records - resource.processed_records,
-      'imported_contacts' => resource.processed_records
+      'identifiable_contacts' => identifiable_count,
+      'non_identifiable_contacts' => non_identifiable_count,
+      'failed_contacts' => failed_count,
+      'total_contacts' => resource.total_records
     }
 
     send_notification(subject, action_url: action_url, meta: meta)

app/models/data_import.rb

@@ -2,15 +2,16 @@
 #
 # Table name: data_imports
 #
-#  id                :bigint           not null, primary key
-#  data_type         :string           not null
-#  processed_records :integer
-#  processing_errors :text
-#  status            :integer          default("pending"), not null
-#  total_records     :integer
-#  created_at        :datetime         not null
-#  updated_at        :datetime         not null
-#  account_id        :bigint           not null
+#  id                       :bigint           not null, primary key
+#  data_type                :string           not null
+#  non_identifiable_records :integer          default(0), not null
+#  processed_records        :integer
+#  processing_errors        :text
+#  status                   :integer          default("pending"), not null
+#  total_records            :integer
+#  created_at               :datetime         not null
+#  updated_at               :datetime         not null
+#  account_id               :bigint           not null
 #
 # Indexes
 #

app/services/data_import/contact_manager.rb

@@ -9,6 +9,10 @@ class DataImport::ContactManager
     contact
   end
 
+  def contact_identifiable?(contact)
+    contact.email.present? || contact.phone_number.present? || contact.identifier.present?
+  end
+
   def find_or_initialize_contact(params)
     contact = find_existing_contact(params)
     contact_params = params.slice(:email, :identifier, :phone_number)

app/views/mailers/administrator_notifications/account_notification_mailer/contact_import_complete.liquid

@@ -1,10 +1,21 @@
 <p>Hello,</p>
 
-<p>Your contact import has been completed. Please check the contacts tab to view the imported contacts.</p>
+<p>Your contact import has been completed. Here's a summary of the results:</p>
 
-<p>Number of records imported: {{meta['imported_contacts']}}</p>
+<p><strong>Contacts imported successfully:</strong> {{meta['identifiable_contacts']}}</p>
+<p><em>These contacts are visible in your contacts list and can be used for conversations.</em></p>
 
-<p>Number of records failed: {{meta['failed_contacts']}}</p>
+{% if meta['non_identifiable_contacts'] > 0 %}
+<p><strong>Contacts created but not visible:</strong> {{meta['non_identifiable_contacts']}}</p>
+<p><em>These contacts were created successfully but don't have email, phone number, or identifier, so they won't appear in your contacts list until you add this information.</em></p>
+{% endif %}
+
+{% if meta['failed_contacts'] > 0 %}
+<p><strong>Contacts that failed to import:</strong> {{meta['failed_contacts']}}</p>
+<p><em>These contacts had validation errors and could not be imported.</em></p>
+{% endif %}
+
+<p><strong>Total contacts processed:</strong> {{meta['total_contacts']}}</p>
 
 {% if meta['failed_contacts'] == 0 %}
   <p>
@@ -12,6 +23,6 @@
   </p>
 {% else %}
   <p>
-  Click <a href="{{ action_url }}" target="_blank">here</a> to view failed records.
+    Click <a href="{{ action_url }}" target="_blank">here</a> to download the failed records and see specific error messages.
   </p>
 {% endif %}

db/migrate/20251024000001_add_non_identifiable_records_to_data_imports.rb

@@ -0,0 +1,5 @@
+class AddNonIdentifiableRecordsToDataImports < ActiveRecord::Migration[7.0]
+  def change
+    add_column :data_imports, :non_identifiable_records, :integer, default: 0, null: false
+  end
+end

spec/jobs/data_import_job_spec.rb

@@ -62,7 +62,8 @@ RSpec.describe DataImportJob do
         described_class.perform_now(invalid_data_import)
         expect(invalid_data_import.account.contacts.count).to eq(csv_length - 1)
         expect(invalid_data_import.reload.total_records).to eq(csv_length)
-        expect(invalid_data_import.reload.processed_records).to eq(csv_length)
+        # Only 2 contacts actually imported (duplicate email fails for the 3rd)
+        expect(invalid_data_import.reload.processed_records).to eq(csv_length - 1)
       end
 
       it 'will preserve emojis' do
@@ -150,11 +151,48 @@ RSpec.describe DataImportJob do
           expect(phone_contact.reload.email).to be_nil
           expect(email_contact.reload.phone_number).to be_nil
           expect(existing_data_import.total_records).to eq(csv_length)
+          # Only 2 contacts imported (1 has conflicting phone/email so fails)
           expect(existing_data_import.processed_records).to eq(csv_length - 1)
         end
       end
     end
 
+    context 'with non_identifiable_records tracking' do
+      it 'tracks non-identifiable records correctly' do
+        # Test that the new non_identifiable_records field is properly tracked
+        described_class.perform_now(data_import)
+
+        data_import.reload
+        expect(data_import.non_identifiable_records).to eq(0) # Default CSV has all identifiable contacts
+      end
+    end
+
+    context 'when all contacts have duplicate identifiers' do
+      it 'imports only the first contact and reports failures accurately' do
+        duplicate_data = [
+          %w[id name email identifier phone_number],
+          ['1', 'First Contact', '', 'DUPLICATE-ID', '+918484848484'], # Will succeed
+          ['2', 'Second Contact', '', 'DUPLICATE-ID', '+918484848485'], # Will fail (duplicate identifier)
+          ['3', 'Third Contact', '', 'DUPLICATE-ID', '+918484848486'] # Will fail (duplicate identifier)
+        ]
+
+        duplicate_data_import = create(:data_import, import_file: generate_csv_file(duplicate_data))
+        CSV.parse(duplicate_data_import.import_file.download, headers: true)
+
+        described_class.perform_now(duplicate_data_import)
+
+        # Only first contact should be imported
+        expect(duplicate_data_import.account.contacts.count).to eq(1)
+        expect(duplicate_data_import.account.contacts.first.name).to eq('First Contact')
+
+        # Check metrics
+        duplicate_data_import.reload
+        expect(duplicate_data_import.total_records).to eq(3)
+        expect(duplicate_data_import.processed_records).to eq(1) # Only first contact imported
+        expect(duplicate_data_import.non_identifiable_records).to eq(0) # First contact is identifiable
+      end
+    end
+
     context 'when the CSV file is invalid' do
       let(:invalid_csv_content) do
         "id,name,email,phone_number,company\n1,\"Clarice Uzzell,\"missing_quote,918080808080,Acmecorp\n2,Marieann Creegan,,+918080808081,Acmecorp"

spec/models/data_import_spec.rb

@@ -11,6 +11,19 @@ RSpec.describe DataImport do
     end
   end
 
+  describe 'fields' do
+    let(:data_import) { create(:data_import) }
+
+    it 'has non_identifiable_records field with default value' do
+      expect(data_import.non_identifiable_records).to eq(0)
+    end
+
+    it 'allows setting non_identifiable_records' do
+      data_import.update!(non_identifiable_records: 5)
+      expect(data_import.reload.non_identifiable_records).to eq(5)
+    end
+  end
+
   describe 'callbacks' do
     let(:data_import) { build(:data_import) }