chore: Refactor Contact Inbox Builders (#5617)

- Remove duplicate code and move everything to builders
- fixes: #4680
This commit is contained in:
Sojan Jose
2022-10-13 15:12:04 -07:00
committed by GitHub
parent 1f271356ca
commit e310230f62
21 changed files with 234 additions and 210 deletions

View File

@@ -1,13 +1,12 @@
# This Builder will create a contact inbox with specified attributes. If the contact inbox already exists, it will be returned.
# For Specific Channels like whatsapp, email etc . it smartly generated appropriate the source id when none is provided.
class ContactInboxBuilder
pattr_initialize [:contact_id!, :inbox_id!, :source_id]
pattr_initialize [:contact, :inbox, :source_id, { hmac_verified: false }]
def perform
@contact = Contact.find(contact_id)
@inbox = @contact.account.inboxes.find(inbox_id)
return unless ['Channel::TwilioSms', 'Channel::Sms', 'Channel::Email', 'Channel::Api', 'Channel::Whatsapp'].include? @inbox.channel_type
source_id = @source_id || generate_source_id
create_contact_inbox(source_id) if source_id.present?
@source_id ||= generate_source_id
create_contact_inbox if source_id.present?
end
private
@@ -19,23 +18,37 @@ class ContactInboxBuilder
when 'Channel::Whatsapp'
wa_source_id
when 'Channel::Email'
@contact.email
email_source_id
when 'Channel::Sms'
@contact.phone_number
when 'Channel::Api'
phone_source_id
when 'Channel::Api', 'Channel::WebWidget'
SecureRandom.uuid
else
raise "Unsupported operation for this channel: #{@inbox.channel_type}"
end
end
def email_source_id
raise ActionController::ParameterMissing, 'contact email' unless @contact.email
@contact.email
end
def phone_source_id
raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number
@contact.phone_number
end
def wa_source_id
return unless @contact.phone_number
raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number
# whatsapp doesn't want the + in e164 format
@contact.phone_number.delete('+').to_s
end
def twilio_source_id
return unless @contact.phone_number
raise ActionController::ParameterMissing, 'contact phone number' unless @contact.phone_number
case @inbox.channel.medium
when 'sms'
@@ -45,11 +58,11 @@ class ContactInboxBuilder
end
end
def create_contact_inbox(source_id)
::ContactInbox.find_or_create_by!(
def create_contact_inbox
::ContactInbox.create_with(hmac_verified: hmac_verified || false).find_or_create_by!(
contact_id: @contact.id,
inbox_id: @inbox.id,
source_id: source_id
source_id: @source_id
)
end
end

View File

@@ -1,25 +1,47 @@
class ContactBuilder
pattr_initialize [:source_id!, :inbox!, :contact_attributes!, :hmac_verified]
# This Builder will create a contact and contact inbox with specified attributes.
# If an existing identified contact exisits, it will be returned.
# for contact inbox logic it uses the contact inbox builder
class ContactInboxWithContactBuilder
pattr_initialize [:inbox!, :contact_attributes!, :source_id, :hmac_verified]
def perform
contact_inbox = inbox.contact_inboxes.find_by(source_id: source_id)
return contact_inbox if contact_inbox
find_or_create_contact_and_contact_inbox
# in case of race conditions where contact is created by another thread
# we will try to find the contact and create a contact inbox
rescue ActiveRecord::RecordNotUnique
find_or_create_contact_and_contact_inbox
end
build_contact_inbox
def find_or_create_contact_and_contact_inbox
@contact_inbox = inbox.contact_inboxes.find_by(source_id: source_id) if source_id.present?
return @contact_inbox if @contact_inbox
ActiveRecord::Base.transaction(requires_new: true) do
build_contact_with_contact_inbox
update_contact_avatar(@contact) unless @contact.avatar.attached?
@contact_inbox
end
end
private
def build_contact_with_contact_inbox
@contact = find_contact || create_contact
@contact_inbox = create_contact_inbox
end
def account
@account ||= inbox.account
end
def create_contact_inbox(contact)
::ContactInbox.create_with(hmac_verified: hmac_verified || false).find_or_create_by!(
contact_id: contact.id,
inbox_id: inbox.id,
source_id: source_id
)
def create_contact_inbox
ContactInboxBuilder.new(
contact: @contact,
inbox: @inbox,
source_id: @source_id,
hmac_verified: hmac_verified
).perform
end
def update_contact_avatar(contact)
@@ -61,16 +83,4 @@ class ContactBuilder
account.contacts.find_by(phone_number: phone_number)
end
def build_contact_inbox
ActiveRecord::Base.transaction do
contact = find_contact || create_contact
contact_inbox = create_contact_inbox(contact)
update_contact_avatar(contact)
contact_inbox
rescue StandardError => e
Rails.logger.error e
raise e
end
end
end

View File

@@ -22,10 +22,9 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
return if @inbox.channel.reauthorization_required?
ActiveRecord::Base.transaction do
build_contact
build_contact_inbox
build_message
end
ensure_contact_avatar
rescue Koala::Facebook::AuthenticationError
@inbox.channel.authorization_error!
rescue StandardError => e
@@ -35,15 +34,12 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
private
def contact
@contact ||= @inbox.contact_inboxes.find_by(source_id: @sender_id)&.contact
end
def build_contact
return if contact.present?
@contact = Contact.create!(contact_params.except(:remote_avatar_url))
@contact_inbox = ContactInbox.find_or_create_by!(contact: contact, inbox: @inbox, source_id: @sender_id)
def build_contact_inbox
@contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: @sender_id,
inbox: @inbox,
contact_attributes: contact_params
).perform
end
def build_message
@@ -54,19 +50,11 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
end
end
def ensure_contact_avatar
return if contact_params[:remote_avatar_url].blank?
return if @contact.avatar.attached?
Avatar::AvatarFromUrlJob.perform_later(@contact, contact_params[:remote_avatar_url])
end
def conversation
@conversation ||= Conversation.find_by(conversation_params) || build_conversation
end
def build_conversation
@contact_inbox ||= contact.contact_inboxes.find_by!(source_id: @sender_id)
Conversation.create!(conversation_params.merge(
contact_inbox_id: @contact_inbox.id
))
@@ -94,7 +82,7 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
{
account_id: @inbox.account_id,
inbox_id: @inbox.id,
contact_id: contact.id
contact_id: @contact_inbox.contact_id
}
end
@@ -105,7 +93,7 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
message_type: @message_type,
content: response.content,
source_id: response.identifier,
sender: @outgoing_echo ? nil : contact
sender: @outgoing_echo ? nil : @contact_inbox.contact
}
end
@@ -113,7 +101,7 @@ class Messages::Facebook::MessageBuilder < Messages::Messenger::MessageBuilder
{
name: "#{result['first_name'] || 'John'} #{result['last_name'] || 'Doe'}",
account_id: @inbox.account_id,
remote_avatar_url: result['profile_pic'] || ''
avatar_url: result['profile_pic']
}
end

View File

@@ -2,8 +2,11 @@ class Api::V1::Accounts::Contacts::ContactInboxesController < Api::V1::Accounts:
before_action :ensure_inbox, only: [:create]
def create
source_id = params[:source_id] || SecureRandom.uuid
@contact_inbox = ContactInbox.create!(contact: @contact, inbox: @inbox, source_id: source_id)
@contact_inbox = ContactInboxBuilder.new(
contact: @contact,
inbox: @inbox,
source_id: params[:source_id]
).perform
end
private

View File

@@ -134,8 +134,11 @@ class Api::V1::Accounts::ContactsController < Api::V1::Accounts::BaseController
return if params[:inbox_id].blank?
inbox = Current.account.inboxes.find(params[:inbox_id])
source_id = params[:source_id] || SecureRandom.uuid
ContactInbox.create!(contact: @contact, inbox: inbox, source_id: source_id)
ContactInboxBuilder.new(
contact: @contact,
inbox: inbox,
source_id: params[:source_id]
).perform
end
def permitted_params

View File

@@ -3,7 +3,7 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro
include DateRangeHelper
before_action :conversation, except: [:index, :meta, :search, :create, :filter]
before_action :contact_inbox, only: [:create]
before_action :inbox, :contact, :contact_inbox, only: [:create]
def index
result = conversation_finder.perform
@@ -109,22 +109,35 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro
authorize @conversation.inbox, :show?
end
def inbox
return if params[:inbox_id].blank?
@inbox = Current.account.inboxes.find(params[:inbox_id])
authorize @inbox, :show?
end
def contact
return if params[:contact_id].blank?
@contact = Current.account.contacts.find(params[:contact_id])
end
def contact_inbox
@contact_inbox = build_contact_inbox
# fallback for the old case where we do look up only using source id
# In future we need to change this and make sure we do look up on combination of inbox_id and source_id
# and deprecate the support of passing only source_id as the param
@contact_inbox ||= ::ContactInbox.find_by!(source_id: params[:source_id])
authorize @contact_inbox.inbox, :show?
end
def build_contact_inbox
return if params[:contact_id].blank? || params[:inbox_id].blank?
inbox = Current.account.inboxes.find(params[:inbox_id])
authorize inbox, :show?
return if @inbox.blank? || @contact.blank?
ContactInboxBuilder.new(
contact_id: params[:contact_id],
inbox_id: inbox.id,
contact: @contact,
inbox: @inbox,
source_id: params[:source_id]
).perform
end

View File

@@ -13,6 +13,8 @@ module RequestExceptionHandler
render_not_found_error('Resource could not be found')
rescue Pundit::NotAuthorizedError
render_unauthorized('You are not authorized to do this action')
rescue ActionController::ParameterMissing => e
render_could_not_create_error(e.message)
ensure
# to address the thread variable leak issues in Puma/Thin webserver
Current.reset

View File

@@ -4,7 +4,7 @@ class Public::Api::V1::Inboxes::ContactsController < Public::Api::V1::InboxesCon
def create
source_id = params[:source_id] || SecureRandom.uuid
@contact_inbox = ::ContactBuilder.new(
@contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: source_id,
inbox: @inbox_channel.inbox,
contact_attributes: permitted_params.except(:identifier, :identifier_hash)

View File

@@ -34,7 +34,7 @@ module MailboxHelper
end
def create_contact
@contact_inbox = ::ContactBuilder.new(
@contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: processed_mail.original_sender,
inbox: @inbox,
contact_attributes: {

View File

@@ -37,16 +37,11 @@ class Channel::FacebookPage < ApplicationRecord
end
def create_contact_inbox(instagram_id, name)
ActiveRecord::Base.transaction do
contact = inbox.account.contacts.create!(name: name)
::ContactInbox.create!(
contact_id: contact.id,
inbox_id: inbox.id,
source_id: instagram_id
)
rescue StandardError => e
Rails.logger.error e
end
@contact_inbox = ::ContactInboxWithContactBuilder.new({
source_id: instagram_id,
inbox: inbox,
contact_attributes: { name: name }
}).perform
end
def subscribe

View File

@@ -32,16 +32,11 @@ class Channel::TwitterProfile < ApplicationRecord
end
def create_contact_inbox(profile_id, name, additional_attributes)
ActiveRecord::Base.transaction do
contact = inbox.account.contacts.create!(additional_attributes: additional_attributes, name: name)
::ContactInbox.create!(
contact_id: contact.id,
inbox_id: inbox.id,
source_id: profile_id
)
rescue StandardError => e
Rails.logger.error e
end
::ContactInboxWithContactBuilder.new({
source_id: profile_id,
inbox: inbox,
contact_attributes: { name: name, additional_attributes: additional_attributes }
}).perform
end
def twitter_client

View File

@@ -98,19 +98,9 @@ class Channel::WebWidget < ApplicationRecord
end
def create_contact_inbox(additional_attributes = {})
ActiveRecord::Base.transaction do
contact = inbox.account.contacts.create!(
name: ::Haikunator.haikunate(1000),
additional_attributes: additional_attributes
)
contact_inbox = ::ContactInbox.create!(
contact_id: contact.id,
inbox_id: inbox.id,
source_id: SecureRandom.uuid
)
contact_inbox
rescue StandardError => e
Rails.logger.error e
end
::ContactInboxWithContactBuilder.new({
inbox: inbox,
contact_attributes: { additional_attributes: additional_attributes }
}).perform
end
end

View File

@@ -81,7 +81,7 @@ class Line::IncomingMessageService
end
def set_contact
contact_inbox = ::ContactBuilder.new(
contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: line_contact_info['userId'],
inbox: inbox,
contact_attributes: contact_attributes

View File

@@ -37,7 +37,7 @@ class Sms::IncomingMessageService
end
def set_contact
contact_inbox = ::ContactBuilder.new(
contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: params[:from],
inbox: @inbox,
contact_attributes: contact_attributes

View File

@@ -31,7 +31,7 @@ class Telegram::IncomingMessageService
end
def set_contact
contact_inbox = ::ContactBuilder.new(
contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: params[:message][:from][:id],
inbox: inbox,
contact_attributes: contact_attributes

View File

@@ -47,7 +47,7 @@ class Twilio::IncomingMessageService
end
def set_contact
contact_inbox = ::ContactBuilder.new(
contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: params[:From],
inbox: inbox,
contact_attributes: contact_attributes

View File

@@ -48,7 +48,7 @@ class Whatsapp::IncomingMessageBaseService
contact_params = @processed_params[:contacts]&.first
return if contact_params.blank?
contact_inbox = ::ContactBuilder.new(
contact_inbox = ::ContactInboxWithContactBuilder.new(
source_id: contact_params[:wa_id],
inbox: inbox,
contact_attributes: { name: contact_params.dig(:profile, :name), phone_number: "+#{@processed_params[:messages].first[:from]}" }