chore: Contact import improvements (#7787)

- Ensure existing contact information is updated on data import - Refactor the existing job to make it more readable - Fixes issues with import files in the wrong encoding fixes: #7307
chatwoot · Aug 24, 2023 · acb7deb · acb7deb
1 parent 44f6a9e
commit acb7deb
Show file tree

Hide file tree

Showing 11 changed files with 174 additions and 100 deletions.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -598,7 +598,7 @@ GEM
       ffi (~> 1.0)
     redis (5.0.6)
       redis-client (>= 0.9.0)
-    redis-client (0.14.1)
+    redis-client (0.16.0)
       connection_pool
     redis-namespace (1.10.0)
       redis (>= 4)
@@ -713,7 +713,7 @@ GEM
       connection_pool (>= 2.3.0)
       rack (>= 2.2.4)
       redis-client (>= 0.14.0)
-    sidekiq-cron (1.10.0)
+    sidekiq-cron (1.10.1)
       fugit (~> 1.8)
       globalid (>= 1.0.1)
       sidekiq (>= 6)

diff --git a/app/javascript/dashboard/i18n/locale/en/contact.json b/app/javascript/dashboard/i18n/locale/en/contact.json
@@ -71,7 +71,7 @@
       "SUBMIT": "Import",
       "CANCEL": "Cancel"
     },
-    "SUCCESS_MESSAGE": "Contacts saved successfully",
+    "SUCCESS_MESSAGE": "You will be notified via email when the import is complete.",
     "ERROR_MESSAGE": "There was an error, please try again"
   },
   "EXPORT_CONTACTS": {

diff --git a/app/jobs/data_import_job.rb b/app/jobs/data_import_job.rb
@@ -6,15 +6,15 @@ class DataImportJob < ApplicationJob
 
   def perform(data_import)
     @data_import = data_import
+    @contact_manager = DataImport::ContactManager.new(@data_import.account)
     process_import_file
-    send_failed_records_to_admin
+    send_import_notification_to_admin
   end
 
   private
 
   def process_import_file
     @data_import.update!(status: :processing)
-
     contacts, rejected_contacts = parse_csv_and_build_contacts
 
     import_contacts(contacts)
@@ -25,21 +25,28 @@ def process_import_file
   def parse_csv_and_build_contacts
     contacts = []
     rejected_contacts = []
-    csv = CSV.parse(@data_import.import_file.download, headers: true)
+    # Ensuring that importing non utf-8 characters will not throw error
+    data = @data_import.import_file.download
+    clean_data = data.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '')
+    csv = CSV.parse(clean_data, headers: true)
 
     csv.each do |row|
-      current_contact = build_contact(row.to_h.with_indifferent_access, @data_import.account)
+      current_contact = @contact_manager.build_contact(row.to_h.with_indifferent_access)
       if current_contact.valid?
         contacts << current_contact
       else
-        row['errors'] = current_contact.errors.full_messages.join(', ')
-        rejected_contacts << row
+        append_rejected_contact(row, current_contact, rejected_contacts)
       end
     end
 
     [contacts, rejected_contacts]
   end
 
+  def append_rejected_contact(row, contact, rejected_contacts)
+    row['errors'] = contact.errors.full_messages.join(', ')
+    rejected_contacts << row
+  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)
@@ -49,63 +56,13 @@ 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 build_contact(params, account)
-    contact = find_or_initialize_contact(params, account)
-    contact.name = params[:name] if params[:name].present?
-    contact.additional_attributes ||= {}
-    contact.additional_attributes[:company] = params[:company] if params[:company].present?
-    contact.additional_attributes[:city] = params[:city] if params[:city].present?
-    contact.assign_attributes(custom_attributes: contact.custom_attributes.merge(params.except(:identifier, :email, :name, :phone_number)))
-    contact
-  end
-
-  def find_or_initialize_contact(params, account)
-    contact = find_existing_contact(params, account)
-    contact ||= account.contacts.new(params.slice(:email, :identifier, :phone_number))
-    contact
-  end
-
-  def find_existing_contact(params, account)
-    contact = find_contact_by_identifier(params, account)
-    contact ||= find_contact_by_email(params, account)
-    contact ||= find_contact_by_phone_number(params, account)
-
-    update_contact_with_merged_attributes(params, contact) if contact.present? && contact.valid?
-    contact
-  end
-
-  def find_contact_by_identifier(params, account)
-    return unless params[:identifier]
-
-    account.contacts.find_by(identifier: params[:identifier])
-  end
-
-  def find_contact_by_email(params, account)
-    return unless params[:email]
-
-    account.contacts.find_by(email: params[:email])
-  end
-
-  def find_contact_by_phone_number(params, account)
-    return unless params[:phone_number]
-
-    account.contacts.find_by(phone_number: params[:phone_number])
-  end
-
-  def update_contact_with_merged_attributes(params, contact)
-    contact.email = params[:email] if params[:email].present?
-    contact.phone_number = params[:phone_number] if params[:phone_number].present?
-    contact.save
-  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_failed_records_to_admin
+    send_import_notification_to_admin
   end
 
   def generate_csv_data(rejected_contacts)
@@ -121,7 +78,7 @@ def generate_csv_data(rejected_contacts)
     end
   end
 
-  def send_failed_records_to_admin
-    AdministratorNotifications::ChannelNotificationsMailer.with(account: @data_import.account).failed_records(@data_import).deliver_later
+  def send_import_notification_to_admin
+    AdministratorNotifications::ChannelNotificationsMailer.with(account: @data_import.account).contact_import_complete(@data_import).deliver_later
   end
 end
diff --git a/app/mailers/administrator_notifications/channel_notifications_mailer.rb b/app/mailers/administrator_notifications/channel_notifications_mailer.rb
@@ -1,16 +1,4 @@
 class AdministratorNotifications::ChannelNotificationsMailer < ApplicationMailer
-  def failed_records(resource)
-    return unless smtp_config_set_or_development?
-
-    subject = 'Contact Import Completed'
-
-    @attachment_url = Rails.application.routes.url_helpers.rails_blob_url(resource.failed_records) if resource.failed_records.attached?
-    @action_url = "#{ENV.fetch('FRONTEND_URL', nil)}/app/accounts/#{resource.account.id}/contacts"
-    @failed_contacts = resource.total_records - resource.processed_records
-    @imported_contacts = resource.processed_records
-    send_mail_with_liquid(to: admin_emails, subject: subject) and return
-  end
-
   def slack_disconnect
     return unless smtp_config_set_or_development?
 
@@ -43,6 +31,19 @@ def email_disconnect(inbox)
     send_mail_with_liquid(to: admin_emails, subject: subject) and return
   end
 
+  def contact_import_complete(resource)
+    return unless smtp_config_set_or_development?
+
+    subject = 'Contact Import Completed'
+
+    @action_url = Rails.application.routes.url_helpers.rails_blob_url(resource.failed_records) if resource.failed_records.attached?
+    @action_url ||= "#{ENV.fetch('FRONTEND_URL', nil)}/app/accounts/#{resource.account.id}/contacts"
+    @meta = {}
+    @meta['failed_contacts'] = resource.total_records - resource.processed_records
+    @meta['imported_contacts'] = resource.processed_records
+    send_mail_with_liquid(to: admin_emails, subject: subject) and return
+  end
+
   def contact_export_complete(file_url)
     return unless smtp_config_set_or_development?
 
@@ -56,4 +57,8 @@ def contact_export_complete(file_url)
   def admin_emails
     Current.account.administrators.pluck(:email)
   end
+
+  def liquid_locals
+    super.merge({ meta: @meta })
+  end
 end
diff --git a/app/services/data_import/contact_manager.rb b/app/services/data_import/contact_manager.rb
@@ -0,0 +1,68 @@
+class DataImport::ContactManager
+  def initialize(account)
+    @account = account
+  end
+
+  def build_contact(params)
+    contact = find_or_initialize_contact(params)
+    update_contact_attributes(params, contact)
+    contact
+  end
+
+  def find_or_initialize_contact(params)
+    contact = find_existing_contact(params)
+    contact_params = params.slice(:email, :identifier, :phone_number)
+    contact_params[:phone_number] = format_phone_number(contact_params[:phone_number]) if contact_params[:phone_number].present?
+    contact ||= @account.contacts.new(contact_params)
+    contact
+  end
+
+  def find_existing_contact(params)
+    contact = find_contact_by_identifier(params)
+    contact ||= find_contact_by_email(params)
+    contact ||= find_contact_by_phone_number(params)
+
+    update_contact_with_merged_attributes(params, contact) if contact.present? && contact.valid?
+    contact
+  end
+
+  def find_contact_by_identifier(params)
+    return unless params[:identifier]
+
+    @account.contacts.find_by(identifier: params[:identifier])
+  end
+
+  def find_contact_by_email(params)
+    return unless params[:email]
+
+    @account.contacts.find_by(email: params[:email])
+  end
+
+  def find_contact_by_phone_number(params)
+    return unless params[:phone_number]
+
+    @account.contacts.find_by(phone_number: format_phone_number(params[:phone_number]))
+  end
+
+  def format_phone_number(phone_number)
+    phone_number.start_with?('+') ? phone_number : "+#{phone_number}"
+  end
+
+  def update_contact_with_merged_attributes(params, contact)
+    contact.identifier = params[:identifier] if params[:identifier].present?
+    contact.email = params[:email] if params[:email].present?
+    contact.phone_number = format_phone_number(params[:phone_number]) if params[:phone_number].present?
+    update_contact_attributes(params, contact)
+    contact.save
+  end
+
+  private
+
+  def update_contact_attributes(params, contact)
+    contact.name = params[:name] if params[:name].present?
+    contact.additional_attributes ||= {}
+    contact.additional_attributes[:company] = params[:company] if params[:company].present?
+    contact.additional_attributes[:city] = params[:city] if params[:city].present?
+    contact.assign_attributes(custom_attributes: contact.custom_attributes.merge(params.except(:identifier, :email, :name, :phone_number)))
+  end
+end
diff --git a/...s/administrator_notifications/channel_notifications_mailer/contact_import_complete.liquid b/...s/administrator_notifications/channel_notifications_mailer/contact_import_complete.liquid
@@ -0,0 +1,17 @@
+<p>Hello,</p>
+
+<p>Your contact import has been completed. Please check the contacts tab to view the imported contacts.</p>
+
+<p>Number of records imported: {{meta['imported_contacts']}}</p>
+
+<p>Number of records failed: {{meta['failed_contacts']}}</p>
+
+{% if meta['failed_contacts'] == 0 %}
+  <p>
+    Click <a href="{{action_url}}">here</a> to view the imported contacts.
+  </p>
+{% else %}
+  <p>
+  Click <a href="{{ action_url }}" target="_blank">here</a> to view failed records.
+  </p>
+{% endif %}
diff --git a/...ws/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid b/...ws/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid
diff --git a/spec/assets/contacts.csv b/spec/assets/contacts.csv
@@ -1,18 +1,18 @@
 id,first_name,last_name,email,gender,ip_address,identifier,phone_number,company
-1,Clarice,Uzzell,cuzzell0@mozilla.org,Genderfluid,70.61.11.201,bb4e11cd-0f23-49da-a123-dcc1fec6852c,+918080808080,My Company Name
+1,Clarice,Uzzell,cuzzell0@mozilla.org,Genderfluid,70.61.11.201,bb4e11cd-0f23-49da-a123-dcc1fec6852c,918080808080,My Company Name
 2,Marieann,Creegan,mcreegan1@cornell.edu,Genderfluid,168.186.4.241,e60bab4c-9fbb-47eb-8f75-42025b789c47,+918080808081
 3,Nancey,Windibank,nwindibank2@bluehost.com,Agender,73.44.41.59,f793e813-4210-4bf3-a812-711418de25d2,+918080808082
 4,Sibel,Stennine,sstennine3@yellowbook.com,Genderqueer,115.249.27.155,d6e35a2d-d093-4437-a577-7df76316b937,+918080808083
 5,Tina,O'Lunney,tolunney4@si.edu,Bigender,219.181.212.8,3540d40a-5567-4f28-af98-5583a7ddbc56,+918080808084
-6,Quinn,Neve,qneve5@army.mil,Genderfluid,231.210.115.166,ba0e1bf0-c74b-41ce-8a2d-0b08fa0e5aa5,+918080808085
+6,Quinn,Neve,qneve5@army.mil,Genderfluid,231.210.115.166,ba0e1bf0-c74b-41ce-8a2d-0b08fa0e5aa5,918080808085
 7,Karylin,Gaunson,kgaunson6@tripod.com,Polygender,160.189.41.11,d24cac79-c81b-4b84-a33e-0441b7c6a981,+918080808086
 8,Jamison,Shenton,jshenton7@upenn.edu,Agender,53.94.18.201,29a7a8c0-c7f7-4af9-852f-761b1a784a7a,+918080808087
 9,Gavan,Threlfall,gthrelfall8@spotify.com,Genderfluid,18.87.247.249,847d4943-ddb5-47cc-8008-ed5092c675c5,+918080808088
 10,Katina,Hemmingway,khemmingway9@ameblo.jp,Non-binary,25.191.96.124,8f0b5efd-b6a8-4f1e-a1e3-b0ea8c9e3048,+918080808089
 11,Jillian,Deinhard,jdeinharda@canalblog.com,Female,11.211.174.93,bd952787-1b05-411f-9975-b916ec0950cc,+918080808090
 12,Blake,Finden,bfindenb@wsj.com,Female,47.26.205.153,12c95613-e49d-4fa2-86fb-deabb6ebe600,+918080808091
-13,Liane,Maxworthy,lmaxworthyc@un.org,Non-binary,157.196.34.166,36b68e4c-40d6-4e09-bf59-7db3b27b18f0,+918080808092
-14,Martynne,Ledley,mledleyd@sourceforge.net,Polygender,109.231.152.148,1856bceb-cb36-415c-8ffc-0527f3f750d8,+918080808093
+13,Liane,Maxworthy,lmaxworthyc@un.org,Non-binary,157.196.34.166,36b68e4c-40d6-4e09-bf59-7db3b27b18f0,918080808092
+14,Martynne,Ledley,mledleyd@sourceforge.net,Polygender,109.231.152.148,1856bceb-cb36-415c-8ffc-0527f3f750d8,918080808093
 15,Katharina,Ruffli,krufflie@huffingtonpost.com,Genderfluid,20.43.146.179,604de5c9-b154-4279-8978-41fb71f0f773,+918080808094
 16,Tucker,Simmance,tsimmancef@bbc.co.uk,Bigender,179.76.226.171,0a8fc3a7-4986-4a51-a503-6c7f974c90ad,+918080808095
 17,Wenona,Martinson,wmartinsong@census.gov,Genderqueer,92.243.194.160,0e5ea6e3-6824-4e78-a6f5-672847eafa17,+918080808096

diff --git a/spec/fixtures/data_import/invalid_bytes.csv b/spec/fixtures/data_import/invalid_bytes.csv
@@ -0,0 +1,3 @@
+name,email
+J�hn D�e,john.doe@example.com
+Jane Smith,jane.smith@example.com
diff --git a/spec/jobs/data_import_job_spec.rb b/spec/jobs/data_import_job_spec.rb
@@ -47,15 +47,29 @@
         expect(invalid_data_import.reload.total_records).to eq(csv_length)
         expect(invalid_data_import.reload.processed_records).to eq(csv_length)
       end
+
+      it 'will not throw error for non utf-8 characters' do
+        invalid_data_import = create(:data_import,
+                                     import_file: Rack::Test::UploadedFile.new(Rails.root.join('spec/fixtures/data_import/invalid_bytes.csv'),
+                                                                               'text/csv'))
+        csv_data = CSV.parse(invalid_data_import.import_file.download, headers: true)
+        csv_length = csv_data.length
+
+        described_class.perform_now(invalid_data_import)
+        expect(invalid_data_import.account.contacts.count).to eq(csv_length)
+
+        expect(invalid_data_import.account.contacts.first.name).to eq(csv_data[0]['name'].encode('UTF-8', 'binary', invalid: :replace,
+                                                                                                                    undef: :replace, replace: ''))
+      end
     end
 
     context 'when the data contains existing records' do
       let(:existing_data) do
         [
-          %w[id first_name last_name email phone_number],
-          ['1', 'Clarice', 'Uzzell', 'cuzzell0@mozilla.org', '+918080808080'],
-          ['2', 'Marieann', 'Creegan', 'mcreegan1@cornell.edu', '+918080808081'],
-          ['3', 'Nancey', 'Windibank', 'nwindibank2@bluehost.com', '+918080808082']
+          %w[id name email phone_number company],
+          ['1', 'Clarice Uzzell', 'cuzzell0@mozilla.org', '918080808080', 'Acmecorp'],
+          ['2', 'Marieann Creegan', 'mcreegan1@cornell.edu', '+918080808081', 'Acmecorp'],
+          ['3', 'Nancey Windibank', 'nwindibank2@bluehost.com', '+918080808082', 'Acmecorp']
         ]
       end
       let(:existing_data_import) { create(:data_import, import_file: generate_csv_file(existing_data)) }
@@ -70,8 +84,11 @@
 
           described_class.perform_now(existing_data_import)
           expect(existing_data_import.account.contacts.count).to eq(csv_length)
-          expect(Contact.find_by(email: csv_data[0]['email']).phone_number).to eq(csv_data[0]['phone_number'])
-          expect(Contact.where(email: csv_data[0]['email']).count).to eq(1)
+          contact = Contact.find_by(email: csv_data[0]['email'])
+          expect(contact).to be_present
+          expect(contact.phone_number).to eq("+#{csv_data[0]['phone_number']}")
+          expect(contact.name).to eq((csv_data[0]['name']).to_s)
+          expect(contact.additional_attributes['company']).to eq((csv_data[0]['company']).to_s)
         end
       end
 
@@ -84,8 +101,11 @@
           described_class.perform_now(existing_data_import)
           expect(existing_data_import.account.contacts.count).to eq(csv_length)
 
-          expect(Contact.find_by(phone_number: csv_data[1]['phone_number']).email).to eq(csv_data[1]['email'])
-          expect(Contact.where(phone_number: csv_data[1]['phone_number']).count).to eq(1)
+          contact = Contact.find_by(phone_number: "+#{csv_data[0]['phone_number']}")
+          expect(contact).to be_present
+          expect(contact.email).to eq(csv_data[0]['email'])
+          expect(contact.name).to eq((csv_data[0]['name']).to_s)
+          expect(contact.additional_attributes['company']).to eq((csv_data[0]['company']).to_s)
         end
       end