diff --git a/app/jobs/data_import_job.rb b/app/jobs/data_import_job.rb index dff5faab2..caed3d52e 100644 --- a/app/jobs/data_import_job.rb +++ b/app/jobs/data_import_job.rb @@ -5,53 +5,123 @@ class DataImportJob < ApplicationJob queue_as :low def perform(data_import) - contacts = [] - data_import.update!(status: :processing) - csv = CSV.parse(data_import.import_file.download, headers: true) - csv.each { |row| contacts << build_contact(row.to_h.with_indifferent_access, data_import.account) } - result = Contact.import contacts, on_duplicate_key_update: :all, batch_size: 1000 - data_import.update!(status: :completed, processed_records: csv.length - result.failed_instances.length, total_records: csv.length) + @data_import = data_import + process_import_file + send_failed_records_to_admin end private - def build_contact(params, account) - # TODO: rather than doing the find or initialize individually lets fetch objects in bulk and update them in memory - contact = init_contact(params, account) + def process_import_file + @data_import.update!(status: :processing) + contacts, rejected_contacts = parse_csv_and_build_contacts + + import_contacts(contacts) + update_data_import_status(contacts.length, rejected_contacts.length) + save_failed_records_csv(rejected_contacts) + end + + def parse_csv_and_build_contacts + contacts = [] + rejected_contacts = [] + csv = CSV.parse(@data_import.import_file.download, headers: true) + + csv.each do |row| + current_contact = build_contact(row.to_h.with_indifferent_access, @data_import.account) + if current_contact.valid? + contacts << current_contact + else + row['errors'] = current_contact.errors.full_messages.join(', ') + rejected_contacts << row + end + end + + [contacts, rejected_contacts] + end + + def import_contacts(contacts) + # + Contact.import(contacts, synchronize: contacts, on_duplicate_key_ignore: true, track_validation_failures: true, validate: true, batch_size: 1000) + 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 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 get_identified_contacts(params, account) - identifier_contact = account.contacts.find_by(identifier: params[:identifier]) if params[:identifier] - email_contact = account.contacts.find_by(email: params[:email]) if params[:email] - phone_number_contact = account.contacts.find_by(phone_number: params[:phone_number]) if params[:phone_number] - contact = merge_identified_contact_attributes(params, [identifier_contact, email_contact, phone_number_contact]) - # intiating the new contact / contact attributes only by ensuring the identifier, email or phone_number duplication errors won't occur - contact ||= merge_contact(email_contact, phone_number_contact) - contact - end - - def merge_contact(email_contact, phone_number_contact) - contact ||= email_contact - contact ||= phone_number_contact - contact - end - - def merge_identified_contact_attributes(params, available_contacts) - identifier_contact, email_contact, phone_number_contact = available_contacts - - contact = identifier_contact - contact&.email = params[:email] if params[:email].present? && email_contact.blank? - contact&.phone_number = params[:phone_number] if params[:phone_number].present? && phone_number_contact.blank? - contact - end - - def init_contact(params, account) - contact = get_identified_contacts(params, account) + 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 + end + + def generate_csv_data(rejected_contacts) + headers = CSV.parse(@data_import.import_file.download, headers: true).headers + headers << 'errors' + return if rejected_contacts.blank? + + CSV.generate do |csv| + csv << headers + rejected_contacts.each do |record| + csv << record + end + end + end + + def send_failed_records_to_admin + AdministratorNotifications::ChannelNotificationsMailer.with(account: @data_import.account).failed_records(@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 daeb44ece..c0b3e1697 100644 --- a/app/mailers/administrator_notifications/channel_notifications_mailer.rb +++ b/app/mailers/administrator_notifications/channel_notifications_mailer.rb @@ -1,4 +1,16 @@ 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? diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 18fd2b83e..a3c6a8016 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -52,10 +52,14 @@ class ApplicationMailer < ActionMailer::Base def liquid_locals # expose variables you want to be exposed in liquid - { + locals = { global_config: GlobalConfig.get('BRAND_NAME', 'BRAND_URL'), action_url: @action_url } + + locals.merge({ attachment_url: @attachment_url }) if @attachment_url + locals.merge({ failed_contacts: @failed_contacts, imported_contacts: @imported_contacts }) + locals end def locale_from_account(account) diff --git a/app/models/data_import.rb b/app/models/data_import.rb index b86ebac23..874eb75f8 100644 --- a/app/models/data_import.rb +++ b/app/models/data_import.rb @@ -22,6 +22,7 @@ class DataImport < ApplicationRecord enum status: { pending: 0, processing: 1, completed: 2, failed: 3 } has_one_attached :import_file + has_one_attached :failed_records after_create_commit :process_data_import 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 new file mode 100644 index 000000000..c9a0721b6 --- /dev/null +++ b/app/views/mailers/administrator_notifications/channel_notifications_mailer/failed_records.liquid @@ -0,0 +1,14 @@ +

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 3f7cd8cb4..1e3c270b6 100644 --- a/spec/assets/contacts.csv +++ b/spec/assets/contacts.csv @@ -1,5 +1,5 @@ -id,first_name,last_name,email,gender,ip_address,identifier,phone_number -1,Clarice,Uzzell,cuzzell0@mozilla.org,Genderfluid,70.61.11.201,bb4e11cd-0f23-49da-a123-dcc1fec6852c,+918080808080 +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 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 diff --git a/spec/jobs/data_import_job_spec.rb b/spec/jobs/data_import_job_spec.rb index 7939cdc0e..d48f2ca3c 100644 --- a/spec/jobs/data_import_job_spec.rb +++ b/spec/jobs/data_import_job_spec.rb @@ -5,18 +5,104 @@ RSpec.describe DataImportJob, type: :job do let!(:data_import) { create(:data_import) } - it 'queues the job' do - expect { job }.to have_enqueued_job(described_class) - .with(data_import) - .on_queue('low') + describe 'enqueueing the job' do + it 'queues the job on the low priority queue' do + expect { job }.to have_enqueued_job(described_class) + .with(data_import) + .on_queue('low') + end end - it 'imports data into the account' do - csv_length = CSV.parse(data_import.import_file.download, headers: true).length - described_class.perform_now(data_import) - expect(data_import.account.contacts.count).to eq(csv_length) - expect(data_import.reload.total_records).to eq(csv_length) - expect(data_import.reload.processed_records).to eq(csv_length) - expect(Contact.find_by(phone_number: '+918080808080')).to be_truthy + describe 'importing data' do + context 'when the data is valid' do + it 'imports data into the account' do + csv_length = CSV.parse(data_import.import_file.download, headers: true).length + + described_class.perform_now(data_import) + expect(data_import.account.contacts.count).to eq(csv_length) + expect(data_import.reload.total_records).to eq(csv_length) + expect(data_import.reload.processed_records).to eq(csv_length) + contact = Contact.find_by(phone_number: '+918080808080') + expect(contact).to be_truthy + expect(contact['additional_attributes']['company']).to eq('My Company Name') + end + end + + context 'when the data contains errors' do + it 'imports erroneous data into the account, skipping invalid records' do + # Last record is invalid because of duplicate email + invalid_data = [ + %w[id first_name last_name email phone_number], + ['1', 'Clarice', 'Uzzell', 'cuzzell0@mozilla.org', '+918484848484'], + ['2', 'Marieann', 'Creegan', 'mcreegan1@cornell.edu', '+918484848485'], + ['3', 'Nancey', 'Windibank', 'cuzzell0@mozilla.org', '+91848484848'] + ] + + invalid_data_import = create(:data_import, import_file: generate_csv_file(invalid_data)) + 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 - 1) + expect(invalid_data_import.reload.total_records).to eq(csv_length) + expect(invalid_data_import.reload.processed_records).to eq(csv_length) + 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'] + ] + end + let(:existing_data_import) { create(:data_import, import_file: generate_csv_file(existing_data)) } + let(:csv_data) { CSV.parse(existing_data_import.import_file.download, headers: true) } + + context 'when the existing record has an email in import data' do + it 'updates the existing record with new data' do + contact = Contact.create!(email: csv_data[0]['email'], account_id: existing_data_import.account_id) + expect(contact.reload.phone_number).to be_nil + + csv_length = csv_data.length + + 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) + end + end + + context 'when the existing record has a phone_number in import data' do + it 'updates the existing record with new data' do + contact = Contact.create!(account_id: existing_data_import.account_id, phone_number: csv_data[1]['phone_number']) + expect(contact.reload.email).to be_nil + csv_length = csv_data.length + + 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) + end + end + + context 'when the existing record has both email and phone_number in import data' do + it 'skips importing the records' do + phone_contact = Contact.create!(account_id: existing_data_import.account_id, phone_number: csv_data[1]['phone_number']) + email_contact = Contact.create!(account_id: existing_data_import.account_id, email: csv_data[1]['email']) + + csv_length = csv_data.length + + described_class.perform_now(existing_data_import) + 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) + expect(existing_data_import.processed_records).to eq(csv_length - 1) + end + end + end end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 2e34ab601..1c78cb93c 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -69,6 +69,7 @@ RSpec.configure do |config| # config.filter_gems_from_backtrace("gem name") config.include SlackStubs config.include FileUploadHelpers + config.include CsvSpecHelpers config.include Devise::Test::IntegrationHelpers, type: :request config.include ActiveSupport::Testing::TimeHelpers config.include ActionCable::TestHelper diff --git a/spec/support/csv_spec_helpers.rb b/spec/support/csv_spec_helpers.rb new file mode 100644 index 000000000..4f3c691c4 --- /dev/null +++ b/spec/support/csv_spec_helpers.rb @@ -0,0 +1,18 @@ +module CsvSpecHelpers + # Generates a Rack::Test::UploadedFile object from an array of arrays + # data: Accepts an array of arrays as the only argument + def generate_csv_file(data) + # Create a temporary file + temp_file = Tempfile.new(['data', '.csv']) + + # Write the array of arrays to the temporary file as CSV + CSV.open(temp_file.path, 'wb') do |csv| + data.each do |row| + csv << row + end + end + + # Create and return a Rack::Test::UploadedFile object + Rack::Test::UploadedFile.new(temp_file.path, 'text/csv') + end +end