diff --git a/Gemfile.lock b/Gemfile.lock index 5d3691b9a..8bf50d6d6 100644 --- 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 index 184e461de..8944def30 100644 --- 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 index caed3d52e..6d0de7c89 100644 --- 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 @@ class DataImportJob < ApplicationJob 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) # Contact.import(contacts, synchronize: contacts, on_duplicate_key_ignore: true, track_validation_failures: true, validate: true, batch_size: 1000) @@ -49,63 +56,13 @@ class DataImportJob < ApplicationJob @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 @@ class DataImportJob < ApplicationJob 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 index c1f7bd81c..00a3830b1 100644 --- 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 @@ class AdministratorNotifications::ChannelNotificationsMailer < ApplicationMailer 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 @@ class AdministratorNotifications::ChannelNotificationsMailer < ApplicationMailer 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 new file mode 100644 index 000000000..ab9a1ee70 --- /dev/null +++ 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/app/views/mailers/administrator_notifications/channel_notifications_mailer/contact_import_complete.liquid b/app/views/mailers/administrator_notifications/channel_notifications_mailer/contact_import_complete.liquid new file mode 100644 index 000000000..776077214 --- /dev/null +++ b/app/views/mailers/administrator_notifications/channel_notifications_mailer/contact_import_complete.liquid @@ -0,0 +1,17 @@ +

Hello,

+ +

Your contact import has been completed. Please check the contacts tab to view the imported contacts.

+ +

Number of records imported: {{meta['imported_contacts']}}

+ +

Number of records failed: {{meta['failed_contacts']}}

+ +{% if meta['failed_contacts'] == 0 %} +

+ Click here to view the imported contacts. +

+{% else %} +

+ Click here to view failed records. +

+{% endif %} diff --git a/app/views/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid b/app/views/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid deleted file mode 100644 index c9a0721b6..000000000 --- a/app/views/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid +++ /dev/null @@ -1,14 +0,0 @@ -

Hello,

- -

Your contact import has been completed. Please check the contacts tab to view the imported contacts.

- -

Number of records imported. : {{imported_contacts}}

- -

Number of records failed. : {{failed_contacts}}

- -

-Attachment [Click here to view] -

-

-Click here -

diff --git a/spec/assets/contacts.csv b/spec/assets/contacts.csv index 1e3c270b6..8df75a37c 100644 --- 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 new file mode 100644 index 000000000..c80b35b26 --- /dev/null +++ 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 index 2f2783eb8..032858029 100644 --- a/spec/jobs/data_import_job_spec.rb +++ b/spec/jobs/data_import_job_spec.rb @@ -47,15 +47,29 @@ RSpec.describe DataImportJob do 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 @@ RSpec.describe DataImportJob do 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 @@ RSpec.describe DataImportJob do 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 diff --git a/spec/mailers/administrator_notifications/channel_notifications_mailer_spec.rb b/spec/mailers/administrator_notifications/channel_notifications_mailer_spec.rb index 2bbb25b84..676d2afaf 100644 --- a/spec/mailers/administrator_notifications/channel_notifications_mailer_spec.rb +++ b/spec/mailers/administrator_notifications/channel_notifications_mailer_spec.rb @@ -57,6 +57,24 @@ RSpec.describe AdministratorNotifications::ChannelNotificationsMailer do end end + describe 'contact_import_complete' do + let!(:data_import) { build(:data_import, total_records: 10, processed_records: 10) } + let(:mail) { described_class.with(account: account).contact_import_complete(data_import).deliver_now } + + it 'renders the subject' do + expect(mail.subject).to eq('Contact Import Completed') + end + + it 'renders the processed records' do + expect(mail.body.encoded).to match('Number of records imported: 10') + expect(mail.body.encoded).to match('Number of records failed: 0') + end + + it 'renders the receiver email' do + expect(mail.to).to eq([administrator.email]) + end + end + describe 'contact_export_complete' do let!(:file_url) { Rails.application.routes.url_helpers.rails_blob_url(account.contacts_export) } let(:mail) { described_class.with(account: account).contact_export_complete(file_url).deliver_now }