chore: Refactor Contact Inbox Builders (#5617)

- Remove duplicate code and move everything to builders
- fixes: #4680

.rubocop.yml

@@ -16,6 +16,7 @@ Metrics/ClassLength:
     - 'app/models/message.rb'
     - 'app/builders/messages/facebook/message_builder.rb'
     - 'app/controllers/api/v1/accounts/contacts_controller.rb'
+    - 'app/controllers/api/v1/accounts/conversations_controller.rb'
     - 'app/listeners/action_cable_listener.rb'
     - 'app/models/conversation.rb'
 RSpec/ExampleLength:

app/builders/contact_inbox_builder.rb

@@ -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 @@ def generate_source_id
     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 @@ def twilio_source_id
     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

app/builders/contact_builder.rb → app/builders/contact_inbox_with_contact_builder.rb

@@ -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
+
+  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
 
-    build_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 @@ def find_contact_by_phone_number(phone_number)
 
     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

app/builders/messages/facebook/message_builder.rb

@@ -22,10 +22,9 @@ def perform
     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 @@ def perform
 
   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 @@ def build_message
     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 @@ def conversation_params
     {
       account_id: @inbox.account_id,
       inbox_id: @inbox.id,
-      contact_id: contact.id
+      contact_id: @contact_inbox.contact_id
     }
   end
 
@@ -105,15 +93,15 @@ def message_params
       message_type: @message_type,
       content: response.content,
       source_id: response.identifier,
-      sender: @outgoing_echo ? nil : contact
+      sender: @outgoing_echo ? nil : @contact_inbox.contact
     }
   end
 
   def process_contact_params_result(result)
     {
       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
 

app/controllers/api/v1/accounts/contacts/contact_inboxes_controller.rb

@@ -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

app/controllers/api/v1/accounts/contacts_controller.rb

@@ -134,8 +134,11 @@ def build_contact_inbox
     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

app/controllers/api/v1/accounts/conversations_controller.rb

@@ -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 @@ def conversation
     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

app/controllers/concerns/request_exception_handler.rb

@@ -13,6 +13,8 @@ def handle_with_exception
     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

app/controllers/public/api/v1/inboxes/contacts_controller.rb

@@ -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)

app/mailboxes/mailbox_helper.rb

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

app/models/channel/facebook_page.rb

@@ -37,16 +37,11 @@ def messaging_window_enabled?
   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

app/models/channel/twitter_profile.rb

@@ -32,16 +32,11 @@ def name
   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