From c61edff1898f4ee3602d76ed3c43ff76f78bfbfc Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 3 Feb 2021 19:24:51 +0530 Subject: [PATCH] feat: Add bulk imports API for contacts (#1724) --- Gemfile | 2 + Gemfile.lock | 3 ++ .../contacts/contact_inboxes_controller.rb | 9 +--- .../api/v1/accounts/contacts_controller.rb | 8 ++++ app/jobs/data_import_job.rb | 46 +++++++++++++++++++ app/models/account.rb | 1 + app/models/contact_inbox.rb | 21 +++++++++ app/models/data_import.rb | 37 +++++++++++++++ app/policies/contact_policy.rb | 4 ++ config/routes.rb | 1 + .../20210201150037_create_data_imports.rb | 14 ++++++ db/schema.rb | 15 +++++- spec/assets/contacts.csv | 26 +++++++++++ .../contact_inboxes_controller_spec.rb | 6 +-- .../v1/accounts/contacts_controller_spec.rb | 37 +++++++++++++++ spec/factories/channel/twilio_sms.rb | 4 +- spec/factories/contact_inbox.rb | 14 +++++- spec/factories/data_import.rb | 7 +++ spec/jobs/data_import_job_spec.rb | 24 ++++++++++ spec/models/data_import_spec.rb | 13 ++++++ 20 files changed, 278 insertions(+), 14 deletions(-) create mode 100644 app/jobs/data_import_job.rb create mode 100644 app/models/data_import.rb create mode 100644 db/migrate/20210201150037_create_data_imports.rb create mode 100644 spec/assets/contacts.csv create mode 100644 spec/factories/data_import.rb create mode 100644 spec/jobs/data_import_job_spec.rb create mode 100644 spec/models/data_import_spec.rb diff --git a/Gemfile b/Gemfile index b719156ef..611c2b933 100644 --- a/Gemfile +++ b/Gemfile @@ -44,6 +44,8 @@ gem 'pg' gem 'redis' gem 'redis-namespace' gem 'redis-rack-cache' +# super fast record imports in bulk +gem 'activerecord-import' ##--- gems for server & infra configuration ---## gem 'dotenv-rails' diff --git a/Gemfile.lock b/Gemfile.lock index d9da65980..7875e6c2a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -62,6 +62,8 @@ GEM activerecord (6.0.3.4) activemodel (= 6.0.3.4) activesupport (= 6.0.3.4) + activerecord-import (1.0.7) + activerecord (>= 3.2) activestorage (6.0.3.4) actionpack (= 6.0.3.4) activejob (= 6.0.3.4) @@ -582,6 +584,7 @@ PLATFORMS DEPENDENCIES action-cable-testing + activerecord-import acts-as-taggable-on administrate annotate diff --git a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb index 7875b0f10..16686dbfe 100644 --- a/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb +++ b/app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb @@ -1,21 +1,14 @@ class Api::V1::Accounts::Contacts::ContactInboxesController < Api::V1::Accounts::BaseController before_action :ensure_contact before_action :ensure_inbox, only: [:create] - before_action :validate_channel_type def create source_id = params[:source_id] || SecureRandom.uuid - @contact_inbox = ContactInbox.create(contact: @contact, inbox: @inbox, source_id: source_id) + @contact_inbox = ContactInbox.create!(contact: @contact, inbox: @inbox, source_id: source_id) end private - def validate_channel_type - return if @inbox.channel_type == 'Channel::Api' - - render json: { error: 'Contact Inbox creation is only allowed in API inboxes' }, status: :unprocessable_entity - end - def ensure_inbox @inbox = Current.account.inboxes.find(params[:inbox_id]) end diff --git a/app/controllers/api/v1/accounts/contacts_controller.rb b/app/controllers/api/v1/accounts/contacts_controller.rb index 72b343880..37a583f05 100644 --- a/app/controllers/api/v1/accounts/contacts_controller.rb +++ b/app/controllers/api/v1/accounts/contacts_controller.rb @@ -22,6 +22,14 @@ class Api::V1::Accounts::ContactsController < Api::V1::Accounts::BaseController @contacts = fetch_contact_last_seen_at(contacts) end + def import + ActiveRecord::Base.transaction do + import = Current.account.data_imports.create!(data_type: 'contacts') + import.import_file.attach(params[:import_file]) + end + head :ok + end + # returns online contacts def active contacts = Current.account.contacts.where(id: ::OnlineStatusTracker diff --git a/app/jobs/data_import_job.rb b/app/jobs/data_import_job.rb new file mode 100644 index 000000000..7c285eb8c --- /dev/null +++ b/app/jobs/data_import_job.rb @@ -0,0 +1,46 @@ +# 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 + +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) + 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) + + contact.name = params[:name] if params[:name].present? + contact.assign_attributes(custom_attributes: contact.custom_attributes.merge(params.except(:identifier, :email, :name))) + + # since callbacks aren't triggered lets ensure a pubsub token + contact.pubsub_token ||= SecureRandom.base58(24) + 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] + [identifier_contact, email_contact] + end + + def init_contact(params, account) + identifier_contact, email_contact = get_identified_contacts(params, account) + + # intiating the new contact / contact attributes only by ensuring the identifier or email duplication errors won't occur + contact = identifier_contact + contact&.email = params[:email] if params[:email].present? && email_contact.blank? + contact ||= email_contact + contact ||= account.contacts.new(params.slice(:email, :identifier)) + contact + end +end diff --git a/app/models/account.rb b/app/models/account.rb index 89ca8a8be..1bb9f1a19 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -34,6 +34,7 @@ class Account < ApplicationRecord has_many :account_users, dependent: :destroy has_many :agent_bot_inboxes, dependent: :destroy + has_many :data_imports, dependent: :destroy has_many :users, through: :account_users has_many :inboxes, dependent: :destroy has_many :conversations, dependent: :destroy diff --git a/app/models/contact_inbox.rb b/app/models/contact_inbox.rb index 2fe1a76c5..9d7d3d5ad 100644 --- a/app/models/contact_inbox.rb +++ b/app/models/contact_inbox.rb @@ -27,6 +27,7 @@ class ContactInbox < ApplicationRecord validates :inbox_id, presence: true validates :contact_id, presence: true validates :source_id, presence: true + validate :valid_source_id_format? belongs_to :contact belongs_to :inbox @@ -47,4 +48,24 @@ class ContactInbox < ApplicationRecord def current_conversation conversations.last end + + private + + def validate_twilio_source_id + # https://www.twilio.com/docs/glossary/what-e164#regex-matching-for-e164 + if inbox.channel.medium == 'sms' && !/^\+[1-9]\d{1,14}$/.match?(source_id) + errors.add(:source_id, 'invalid source id for twilio sms inbox. valid Regex /^\+[1-9]\d{1,14}$/') + elsif inbox.channel.medium == 'whatsapp' && !/^whatsapp:\+[1-9]\d{1,14}$/.match?(source_id) + errors.add(:source_id, 'invalid source id for twilio whatsapp inbox. valid Regex /^whatsapp:\+[1-9]\d{1,14}$/') + end + end + + def validate_email_source_id + errors.add(:source_id, "invalid source id for Email inbox. valid Regex #{Device.email_regexp}") unless Devise.email_regexp.match?(source_id) + end + + def valid_source_id_format? + validate_twilio_source_id if inbox.channel_type == 'Channel::TwilioSms' + validate_email_source_id if inbox.channel_type == 'Channel::Email' + end end diff --git a/app/models/data_import.rb b/app/models/data_import.rb new file mode 100644 index 000000000..6b3db0c06 --- /dev/null +++ b/app/models/data_import.rb @@ -0,0 +1,37 @@ +# == Schema Information +# +# 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 +# +# Indexes +# +# index_data_imports_on_account_id (account_id) +# +# Foreign Keys +# +# fk_rails_... (account_id => accounts.id) +# +class DataImport < ApplicationRecord + belongs_to :account + validates :data_type, inclusion: { in: ['contacts'], message: '%s is an invalid data type' } + enum status: { pending: 0, processing: 1, completed: 2, failed: 3 } + + has_one_attached :import_file + + after_create_commit :process_data_import + + private + + def process_data_import + DataImportJob.perform_later(self) + end +end diff --git a/app/policies/contact_policy.rb b/app/policies/contact_policy.rb index a6e1590fb..b5ec2f78c 100644 --- a/app/policies/contact_policy.rb +++ b/app/policies/contact_policy.rb @@ -7,6 +7,10 @@ class ContactPolicy < ApplicationPolicy true end + def import? + @account_user.administrator? + end + def search? true end diff --git a/config/routes.rb b/config/routes.rb index 55e864bbf..f78073eed 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,6 +67,7 @@ Rails.application.routes.draw do collection do get :active get :search + post :import end scope module: :contacts do resources :conversations, only: [:index] diff --git a/db/migrate/20210201150037_create_data_imports.rb b/db/migrate/20210201150037_create_data_imports.rb new file mode 100644 index 000000000..349453a8d --- /dev/null +++ b/db/migrate/20210201150037_create_data_imports.rb @@ -0,0 +1,14 @@ +class CreateDataImports < ActiveRecord::Migration[6.0] + def change + create_table :data_imports do |t| + t.references :account, null: false, foreign_key: true + t.string :data_type, null: false + t.integer :status, null: false, default: 0 + t.text :processing_errors + t.integer :total_records + t.integer :processed_records + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 3f84e27c3..aa7cc393e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_01_26_121313) do +ActiveRecord::Schema.define(version: 2021_02_01_150037) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" @@ -241,6 +241,18 @@ ActiveRecord::Schema.define(version: 2021_01_26_121313) do t.index ["team_id"], name: "index_conversations_on_team_id" end + create_table "data_imports", force: :cascade do |t| + t.bigint "account_id", null: false + t.string "data_type", null: false + t.integer "status", default: 0, null: false + t.text "processing_errors" + t.integer "total_records" + t.integer "processed_records" + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["account_id"], name: "index_data_imports_on_account_id" + end + create_table "email_templates", force: :cascade do |t| t.string "name", null: false t.text "body", null: false @@ -585,6 +597,7 @@ ActiveRecord::Schema.define(version: 2021_01_26_121313) do add_foreign_key "contact_inboxes", "inboxes" add_foreign_key "conversations", "contact_inboxes" add_foreign_key "conversations", "teams" + add_foreign_key "data_imports", "accounts" add_foreign_key "team_members", "teams" add_foreign_key "team_members", "users" add_foreign_key "teams", "accounts" diff --git a/spec/assets/contacts.csv b/spec/assets/contacts.csv new file mode 100644 index 000000000..250ea1572 --- /dev/null +++ b/spec/assets/contacts.csv @@ -0,0 +1,26 @@ +id,first_name,last_name,email,gender,ip_address,identifier +1,Clarice,Uzzell,cuzzell0@mozilla.org,Genderfluid,70.61.11.201,bb4e11cd-0f23-49da-a123-dcc1fec6852c +2,Marieann,Creegan,mcreegan1@cornell.edu,Genderfluid,168.186.4.241,e60bab4c-9fbb-47eb-8f75-42025b789c47 +3,Nancey,Windibank,nwindibank2@bluehost.com,Agender,73.44.41.59,f793e813-4210-4bf3-a812-711418de25d2 +4,Sibel,Stennine,sstennine3@yellowbook.com,Genderqueer,115.249.27.155,d6e35a2d-d093-4437-a577-7df76316b937 +5,Tina,O'Lunney,tolunney4@si.edu,Bigender,219.181.212.8,3540d40a-5567-4f28-af98-5583a7ddbc56 +6,Quinn,Neve,qneve5@army.mil,Genderfluid,231.210.115.166,ba0e1bf0-c74b-41ce-8a2d-0b08fa0e5aa5 +7,Karylin,Gaunson,kgaunson6@tripod.com,Polygender,160.189.41.11,d24cac79-c81b-4b84-a33e-0441b7c6a981 +8,Jamison,Shenton,jshenton7@upenn.edu,Agender,53.94.18.201,29a7a8c0-c7f7-4af9-852f-761b1a784a7a +9,Gavan,Threlfall,gthrelfall8@spotify.com,Genderfluid,18.87.247.249,847d4943-ddb5-47cc-8008-ed5092c675c5 +10,Katina,Hemmingway,khemmingway9@ameblo.jp,Non-binary,25.191.96.124,8f0b5efd-b6a8-4f1e-a1e3-b0ea8c9e3048 +11,Jillian,Deinhard,jdeinharda@canalblog.com,Female,11.211.174.93,bd952787-1b05-411f-9975-b916ec0950cc +12,Blake,Finden,bfindenb@wsj.com,Female,47.26.205.153,12c95613-e49d-4fa2-86fb-deabb6ebe600 +13,Liane,Maxworthy,lmaxworthyc@un.org,Non-binary,157.196.34.166,36b68e4c-40d6-4e09-bf59-7db3b27b18f0 +14,Martynne,Ledley,mledleyd@sourceforge.net,Polygender,109.231.152.148,1856bceb-cb36-415c-8ffc-0527f3f750d8 +15,Katharina,Ruffli,krufflie@huffingtonpost.com,Genderfluid,20.43.146.179,604de5c9-b154-4279-8978-41fb71f0f773 +16,Tucker,Simmance,tsimmancef@bbc.co.uk,Bigender,179.76.226.171,0a8fc3a7-4986-4a51-a503-6c7f974c90ad +17,Wenona,Martinson,wmartinsong@census.gov,Genderqueer,92.243.194.160,0e5ea6e3-6824-4e78-a6f5-672847eafa17 +18,Gretna,Vedyasov,gvedyasovh@lycos.com,Female,25.22.86.101,6becf55b-a7b5-48f6-8788-b89cae85b066 +19,Lurline,Abdon,labdoni@archive.org,Genderqueer,150.249.116.118,afa9429f-9034-4b06-9efa-980e01906ebf +20,Fiann,Norcliff,fnorcliffj@istockphoto.com,Female,237.167.197.197,59f72dec-14ba-4d6e-b17c-0d962e69ffac +21,Zed,Linn,zlinnk@phoca.cz,Genderqueer,88.102.64.113,95f7bc56-be92-4c9c-ad58-eff3e63c7bea +22,Averyl,Simyson,asimysonl@livejournal.com,Agender,141.248.89.29,bde1fe59-c9bd-440c-bb39-79fe61dac1d1 +23,Camella,Blackadder,cblackadderm@nifty.com,Polygender,118.123.138.115,0c981752-5857-487c-b9b5-5d0253df740a +24,Aurie,Spatig,aspatign@printfriendly.com,Polygender,157.45.102.235,4cf22bfb-2c3f-41d1-9993-6e3758e457ba +25,Adrienne,Bellard,abellardo@cnn.com,Male,170.73.198.47,f10f9b8d-38ac-4e17-8a7d-d2e6a055f944 diff --git a/spec/controllers/api/v1/accounts/contacts/contact_inboxes_controller_spec.rb b/spec/controllers/api/v1/accounts/contacts/contact_inboxes_controller_spec.rb index 5d2288aeb..6bf741347 100644 --- a/spec/controllers/api/v1/accounts/contacts/contact_inboxes_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/contacts/contact_inboxes_controller_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe '/api/v1/accounts/{account.id}/contacts/:id/contact_inboxes', type: :request do let(:account) { create(:account) } let(:contact) { create(:contact, account: account) } - let(:inbox_1) { create(:inbox, account: account) } + let(:channel_twilio_sms) { create(:channel_twilio_sms, account: account) } let(:channel_api) { create(:channel_api, account: account) } let(:user) { create(:user, account: account) } @@ -28,10 +28,10 @@ RSpec.describe '/api/v1/accounts/{account.id}/contacts/:id/contact_inboxes', typ expect(contact.reload.contact_inboxes.map(&:inbox_id)).to include(channel_api.inbox.id) end - it 'throws error when its not an api inbox' do + it 'throws error for invalid source id' do expect do post "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/contact_inboxes", - params: { inbox_id: inbox_1.id }, + params: { inbox_id: channel_twilio_sms.inbox.id }, headers: user.create_new_auth_token, as: :json end.to change(ContactInbox, :count).by(0) diff --git a/spec/controllers/api/v1/accounts/contacts_controller_spec.rb b/spec/controllers/api/v1/accounts/contacts_controller_spec.rb index e419656cb..549614b26 100644 --- a/spec/controllers/api/v1/accounts/contacts_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/contacts_controller_spec.rb @@ -43,6 +43,43 @@ RSpec.describe 'Contacts API', type: :request do end end + describe 'POST /api/v1/accounts/{account.id}/contacts/import' do + context 'when it is an unauthenticated user' do + it 'returns unauthorized' do + post "/api/v1/accounts/#{account.id}/contacts/import" + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated user with out permission' do + let(:agent) { create(:user, account: account, role: :agent) } + + it 'returns unauthorized' do + post "/api/v1/accounts/#{account.id}/contacts/import", + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated user' do + let(:admin) { create(:user, account: account, role: :administrator) } + + it 'creates a data import' do + file = fixture_file_upload(Rails.root.join('spec/assets/contacts.csv'), 'text/csv') + post "/api/v1/accounts/#{account.id}/contacts/import", + headers: admin.create_new_auth_token, + params: { import_file: file } + + expect(response).to have_http_status(:success) + expect(account.data_imports.count).to eq(1) + expect(account.data_imports.first.import_file.attached?).to eq(true) + end + end + end + describe 'GET /api/v1/accounts/{account.id}/contacts/active' do context 'when it is an unauthenticated user' do it 'returns unauthorized' do diff --git a/spec/factories/channel/twilio_sms.rb b/spec/factories/channel/twilio_sms.rb index fce3f9fce..e166058d4 100644 --- a/spec/factories/channel/twilio_sms.rb +++ b/spec/factories/channel/twilio_sms.rb @@ -4,7 +4,9 @@ FactoryBot.define do account_sid { SecureRandom.uuid } sequence(:phone_number) { |n| "+123456789#{n}1" } medium { :sms } - inbox account + after(:build) do |channel| + channel.inbox ||= create(:inbox, account: channel.account) + end end end diff --git a/spec/factories/contact_inbox.rb b/spec/factories/contact_inbox.rb index 24b39cba4..23abde2ca 100644 --- a/spec/factories/contact_inbox.rb +++ b/spec/factories/contact_inbox.rb @@ -4,6 +4,18 @@ FactoryBot.define do factory :contact_inbox do contact inbox - source_id { SecureRandom.uuid } + + after(:build) { |contact_inbox| contact_inbox.source_id ||= generate_source_id(contact_inbox) } + end +end + +def generate_source_id(contact_inbox) + case contact_inbox.inbox.channel_type + when 'Channel::TwilioSms' + contact_inbox.inbox.channel.medium == 'sms' ? Faker::PhoneNumber.cell_phone_in_e164 : "whatsapp:#{Faker::PhoneNumber.cell_phone_in_e164}" + when 'Channel::Email' + "#{SecureRandom.uuid}@acme.inc" + else + SecureRandom.uuid end end diff --git a/spec/factories/data_import.rb b/spec/factories/data_import.rb new file mode 100644 index 000000000..ff7561f31 --- /dev/null +++ b/spec/factories/data_import.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :data_import do + data_type { 'contacts' } + import_file { Rack::Test::UploadedFile.new(Rails.root.join('spec/assets/contacts.csv'), 'text/csv') } + account + end +end diff --git a/spec/jobs/data_import_job_spec.rb b/spec/jobs/data_import_job_spec.rb new file mode 100644 index 000000000..ad5e7b597 --- /dev/null +++ b/spec/jobs/data_import_job_spec.rb @@ -0,0 +1,24 @@ +require 'rails_helper' + +RSpec.describe DataImportJob, type: :job do + subject(:job) { described_class.perform_later(data_import) } + + 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') + 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) + + # should generate pubsub tokens for contacts + expect(data_import.account.contacts.last.pubsub_token).present? + end +end diff --git a/spec/models/data_import_spec.rb b/spec/models/data_import_spec.rb new file mode 100644 index 000000000..df37c9211 --- /dev/null +++ b/spec/models/data_import_spec.rb @@ -0,0 +1,13 @@ +require 'rails_helper' + +RSpec.describe DataImport, type: :model do + describe 'associations' do + it { is_expected.to belong_to(:account) } + end + + describe 'validations' do + it 'returns false for invalid data type' do + expect(build(:data_import, data_type: 'Xyc').valid?).to eq false + end + end +end