From cc026110073267e3c0910982faf0d1d9bbda8ac3 Mon Sep 17 00:00:00 2001
From: Sojan Jose <sojan@pepalo.com>
Date: Sat, 27 Jun 2020 21:34:53 +0530
Subject: [PATCH] Chore: Convert Message Sender to polymorphic (#740)

Fixes #680
---
 app/actions/contact_merge_action.rb           |  2 +-
 app/builders/messages/message_builder.rb      |  3 +-
 .../messages/outgoing/normal_builder.rb       |  2 +-
 .../conversations/messages_controller.rb      |  3 +-
 .../api/v1/widget/messages_controller.rb      |  2 +-
 app/mailboxes/conversation_mailbox.rb         |  2 +-
 app/models/agent_bot.rb                       |  5 ++--
 app/models/contact.rb                         |  2 +-
 app/models/message.rb                         | 28 +++++++++----------
 app/models/user.rb                            |  2 +-
 .../twilio/incoming_message_service.rb        |  2 +-
 .../twitter/direct_message_parser_service.rb  |  2 +-
 app/services/twitter/tweet_parser_service.rb  |  2 +-
 .../messages/create.json.jbuilder             |  1 +
 .../messages/index.json.jbuilder              |  2 +-
 .../v1/widget/messages/create.json.jbuilder   |  2 +-
 .../v1/widget/messages/index.json.jbuilder    |  2 +-
 .../20200418124534_add_sender_to_messages.rb  | 25 +++++++++++++++++
 db/schema.rb                                  |  8 ++----
 .../slack/incoming_message_builder.rb         |  2 +-
 .../slack/send_on_slack_service.rb            |  6 +---
 .../widget/incoming_message_builder.rb        |  3 +-
 .../widget/outgoing_message_builder.rb        |  2 +-
 spec/actions/contact_merge_action_spec.rb     |  2 +-
 spec/factories/messages.rb                    |  2 +-
 .../slack/send_on_slack_service_spec.rb       |  2 +-
 spec/models/conversation_spec.rb              |  4 +--
 27 files changed, 71 insertions(+), 49 deletions(-)
 create mode 100644 db/migrate/20200418124534_add_sender_to_messages.rb

diff --git a/app/actions/contact_merge_action.rb b/app/actions/contact_merge_action.rb
index 343e78032..f2d9b6d06 100644
--- a/app/actions/contact_merge_action.rb
+++ b/app/actions/contact_merge_action.rb
@@ -29,7 +29,7 @@ class ContactMergeAction
   end
 
   def merge_messages
-    Message.where(contact_id: @mergee_contact.id).update(contact_id: @base_contact.id)
+    Message.where(sender: @mergee_contact).update(sender: @base_contact)
   end
 
   def merge_contact_inboxes
diff --git a/app/builders/messages/message_builder.rb b/app/builders/messages/message_builder.rb
index 0eeba6b95..a8251edb4 100644
--- a/app/builders/messages/message_builder.rb
+++ b/app/builders/messages/message_builder.rb
@@ -117,7 +117,8 @@ class Messages::MessageBuilder
       inbox_id: conversation.inbox_id,
       message_type: @message_type,
       content: response.content,
-      source_id: response.identifier
+      source_id: response.identifier,
+      sender: contact
     }
   end
 
diff --git a/app/builders/messages/outgoing/normal_builder.rb b/app/builders/messages/outgoing/normal_builder.rb
index 1b15f24c0..78572b0ee 100644
--- a/app/builders/messages/outgoing/normal_builder.rb
+++ b/app/builders/messages/outgoing/normal_builder.rb
@@ -37,7 +37,7 @@ class Messages::Outgoing::NormalBuilder
       message_type: :outgoing,
       content: @content,
       private: @private,
-      user_id: @user&.id,
+      sender: @user,
       source_id: @fb_id,
       content_type: @content_type,
       items: @items
diff --git a/app/controllers/api/v1/accounts/conversations/messages_controller.rb b/app/controllers/api/v1/accounts/conversations/messages_controller.rb
index 601db872f..c4d595771 100644
--- a/app/controllers/api/v1/accounts/conversations/messages_controller.rb
+++ b/app/controllers/api/v1/accounts/conversations/messages_controller.rb
@@ -4,7 +4,8 @@ class Api::V1::Accounts::Conversations::MessagesController < Api::V1::Accounts::
   end
 
   def create
-    mb = Messages::Outgoing::NormalBuilder.new(current_user, @conversation, params)
+    user = current_user || @resource
+    mb = Messages::Outgoing::NormalBuilder.new(user, @conversation, params)
     @message = mb.perform
   end
 
diff --git a/app/controllers/api/v1/widget/messages_controller.rb b/app/controllers/api/v1/widget/messages_controller.rb
index 366aa3912..42cd313e2 100644
--- a/app/controllers/api/v1/widget/messages_controller.rb
+++ b/app/controllers/api/v1/widget/messages_controller.rb
@@ -45,7 +45,7 @@ class Api::V1::Widget::MessagesController < Api::V1::Widget::BaseController
   def message_params
     {
       account_id: conversation.account_id,
-      contact_id: @contact.id,
+      sender: @contact,
       content: permitted_params[:message][:content],
       inbox_id: conversation.inbox_id,
       message_type: :incoming
diff --git a/app/mailboxes/conversation_mailbox.rb b/app/mailboxes/conversation_mailbox.rb
index b838ef585..da8afd519 100644
--- a/app/mailboxes/conversation_mailbox.rb
+++ b/app/mailboxes/conversation_mailbox.rb
@@ -20,7 +20,7 @@ class ConversationMailbox < ApplicationMailbox
   def create_message
     @message = @conversation.messages.create(
       account_id: @conversation.account_id,
-      contact_id: @conversation.contact_id,
+      sender: @conversation.contact,
       content: processed_mail.text_content[:reply],
       inbox_id: @conversation.inbox_id,
       message_type: 'incoming',
diff --git a/app/models/agent_bot.rb b/app/models/agent_bot.rb
index 8b27f0fcc..493c0ed0b 100644
--- a/app/models/agent_bot.rb
+++ b/app/models/agent_bot.rb
@@ -17,12 +17,13 @@ class AgentBot < ApplicationRecord
 
   has_many :agent_bot_inboxes, dependent: :destroy
   has_many :inboxes, through: :agent_bot_inboxes
+  has_many :messages, as: :sender, dependent: :restrict_with_exception
 
-  def push_event_data
+  def push_event_data(inbox = nil)
     {
       id: id,
       name: name,
-      avatar_url: avatar_url,
+      avatar_url: avatar_url || inbox&.avatar_url,
       type: 'agent_bot'
     }
   end
diff --git a/app/models/contact.rb b/app/models/contact.rb
index 9ea82ba4c..6433ffcfa 100644
--- a/app/models/contact.rb
+++ b/app/models/contact.rb
@@ -35,7 +35,7 @@ class Contact < ApplicationRecord
   has_many :conversations, dependent: :destroy
   has_many :contact_inboxes, dependent: :destroy
   has_many :inboxes, through: :contact_inboxes
-  has_many :messages, dependent: :destroy
+  has_many :messages, as: :sender, dependent: :destroy
 
   before_validation :downcase_email
   after_create_commit :dispatch_create_event
diff --git a/app/models/message.rb b/app/models/message.rb
index 526131098..0ec98ac2e 100644
--- a/app/models/message.rb
+++ b/app/models/message.rb
@@ -8,28 +8,23 @@
 #  content_type       :integer          default("text")
 #  message_type       :integer          not null
 #  private            :boolean          default(FALSE)
+#  sender_type        :string
 #  status             :integer          default("sent")
 #  created_at         :datetime         not null
 #  updated_at         :datetime         not null
 #  account_id         :integer          not null
-#  contact_id         :bigint
 #  conversation_id    :integer          not null
 #  inbox_id           :integer          not null
+#  sender_id          :bigint
 #  source_id          :string
-#  user_id            :integer
 #
 # Indexes
 #
-#  index_messages_on_account_id       (account_id)
-#  index_messages_on_contact_id       (contact_id)
-#  index_messages_on_conversation_id  (conversation_id)
-#  index_messages_on_inbox_id         (inbox_id)
-#  index_messages_on_source_id        (source_id)
-#  index_messages_on_user_id          (user_id)
-#
-# Foreign Keys
-#
-#  fk_rails_...  (contact_id => contacts.id)
+#  index_messages_on_account_id                 (account_id)
+#  index_messages_on_conversation_id            (conversation_id)
+#  index_messages_on_inbox_id                   (inbox_id)
+#  index_messages_on_sender_type_and_sender_id  (sender_type,sender_id)
+#  index_messages_on_source_id                  (source_id)
 #
 
 class Message < ApplicationRecord
@@ -65,8 +60,11 @@ class Message < ApplicationRecord
   belongs_to :account
   belongs_to :inbox
   belongs_to :conversation, touch: true
+
+  # FIXME: phase out user and contact after 1.4 since the info is there in sender
   belongs_to :user, required: false
   belongs_to :contact, required: false
+  belongs_to :sender, polymorphic: true, required: false
 
   has_many :attachments, dependent: :destroy, autosave: true, before_add: :validate_attachments_limit
 
@@ -88,7 +86,8 @@ class Message < ApplicationRecord
       conversation_id: conversation.display_id
     )
     data.merge!(attachments: attachments.map(&:push_event_data)) if attachments.present?
-    data.merge!(sender: user.push_event_data) if user
+    data.merge!(sender: sender.push_event_data) if sender && !sender.is_a?(AgentBot)
+    data.merge!(sender: sender.push_event_data(inbox)) if sender&.is_a?(AgentBot)
     data
   end
 
@@ -105,8 +104,7 @@ class Message < ApplicationRecord
       content_type: content_type,
       content_attributes: content_attributes,
       source_id: source_id,
-      sender: user.try(:webhook_data),
-      contact: contact.try(:webhook_data),
+      sender: sender.try(:webhook_data),
       inbox: inbox.webhook_data,
       conversation: conversation.webhook_data,
       account: account.webhook_data
diff --git a/app/models/user.rb b/app/models/user.rb
index 6157767a2..aa014852a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -65,7 +65,7 @@ class User < ApplicationRecord
   has_many :assigned_conversations, foreign_key: 'assignee_id', class_name: 'Conversation', dependent: :nullify
   has_many :inbox_members, dependent: :destroy
   has_many :inboxes, through: :inbox_members, source: :inbox
-  has_many :messages
+  has_many :messages, as: :sender
   has_many :invitees, through: :account_users, class_name: 'User', foreign_key: 'inviter_id', dependent: :nullify
 
   has_many :notifications, dependent: :destroy
diff --git a/app/services/twilio/incoming_message_service.rb b/app/services/twilio/incoming_message_service.rb
index f7a7d0f0c..c50016971 100644
--- a/app/services/twilio/incoming_message_service.rb
+++ b/app/services/twilio/incoming_message_service.rb
@@ -11,7 +11,7 @@ class Twilio::IncomingMessageService
       account_id: @inbox.account_id,
       inbox_id: @inbox.id,
       message_type: :incoming,
-      contact_id: @contact.id,
+      sender: @contact,
       source_id: params[:SmsSid]
     )
     attach_files
diff --git a/app/services/twitter/direct_message_parser_service.rb b/app/services/twitter/direct_message_parser_service.rb
index bf83958ba..04e68e590 100644
--- a/app/services/twitter/direct_message_parser_service.rb
+++ b/app/services/twitter/direct_message_parser_service.rb
@@ -12,7 +12,7 @@ class Twitter::DirectMessageParserService < Twitter::WebhooksBaseService
       account_id: @inbox.account_id,
       inbox_id: @inbox.id,
       message_type: outgoing_message? ? :outgoing : :incoming,
-      contact_id: @contact.id,
+      sender: @contact,
       source_id: direct_message_data['id']
     )
   end
diff --git a/app/services/twitter/tweet_parser_service.rb b/app/services/twitter/tweet_parser_service.rb
index 84ea87ff4..def65d35e 100644
--- a/app/services/twitter/tweet_parser_service.rb
+++ b/app/services/twitter/tweet_parser_service.rb
@@ -73,7 +73,7 @@ class Twitter::TweetParserService < Twitter::WebhooksBaseService
     set_conversation
     @conversation.messages.create(
       account_id: @inbox.account_id,
-      contact_id: @contact.id,
+      sender: @contact,
       content: tweet_text,
       inbox_id: @inbox.id,
       message_type: message_type,
diff --git a/app/views/api/v1/accounts/conversations/messages/create.json.jbuilder b/app/views/api/v1/accounts/conversations/messages/create.json.jbuilder
index 0dc5b827e..4c3eff2a9 100644
--- a/app/views/api/v1/accounts/conversations/messages/create.json.jbuilder
+++ b/app/views/api/v1/accounts/conversations/messages/create.json.jbuilder
@@ -7,4 +7,5 @@ json.content_type @message.content_type
 json.content_attributes @message.content_attributes
 json.created_at @message.created_at.to_i
 json.private @message.private
+json.sender @message.sender.push_event_data
 json.attachments @message.attachments.map(&:push_event_data) if @message.attachments.present?
diff --git a/app/views/api/v1/accounts/conversations/messages/index.json.jbuilder b/app/views/api/v1/accounts/conversations/messages/index.json.jbuilder
index e04a7dd9c..a89ea7f0c 100644
--- a/app/views/api/v1/accounts/conversations/messages/index.json.jbuilder
+++ b/app/views/api/v1/accounts/conversations/messages/index.json.jbuilder
@@ -17,6 +17,6 @@ json.payload do
     json.private message.private
     json.source_id message.source_id
     json.attachments message.attachments.map(&:push_event_data) if message.attachments.present?
-    json.sender message.user.push_event_data if message.user
+    json.sender message.sender.push_event_data if message.sender
   end
 end
diff --git a/app/views/api/v1/widget/messages/create.json.jbuilder b/app/views/api/v1/widget/messages/create.json.jbuilder
index ff442b304..57276bdf1 100644
--- a/app/views/api/v1/widget/messages/create.json.jbuilder
+++ b/app/views/api/v1/widget/messages/create.json.jbuilder
@@ -7,4 +7,4 @@ json.created_at @message.created_at.to_i
 json.private @message.private
 json.source_id @message.source_id
 json.attachments @message.attachments.map(&:push_event_data) if @message.attachments.present?
-json.sender @message.user.push_event_data if @message.user
+json.sender @message.sender.push_event_data if @message.sender
diff --git a/app/views/api/v1/widget/messages/index.json.jbuilder b/app/views/api/v1/widget/messages/index.json.jbuilder
index f6491991c..a0462b545 100644
--- a/app/views/api/v1/widget/messages/index.json.jbuilder
+++ b/app/views/api/v1/widget/messages/index.json.jbuilder
@@ -7,5 +7,5 @@ json.array! @messages do |message|
   json.created_at message.created_at.to_i
   json.conversation_id message.conversation.display_id
   json.attachments message.attachments.map(&:push_event_data) if message.attachments.present?
-  json.sender message.user.push_event_data if message.user
+  json.sender message.sender.push_event_data if message.sender
 end
diff --git a/db/migrate/20200418124534_add_sender_to_messages.rb b/db/migrate/20200418124534_add_sender_to_messages.rb
new file mode 100644
index 000000000..02efdc4a5
--- /dev/null
+++ b/db/migrate/20200418124534_add_sender_to_messages.rb
@@ -0,0 +1,25 @@
+class AddSenderToMessages < ActiveRecord::Migration[6.0]
+  def change
+    add_reference :messages, :sender, polymorphic: true, index: true
+    add_sender_from_message
+    remove_index :messages, name: 'index_messages_on_contact_id', column: 'contact_id'
+    remove_index :messages, name: 'index_messages_on_user_id', column: 'user_id'
+    remove_column :messages, :user_id, :integer # rubocop:disable Rails/BulkChangeTable
+    remove_column :messages, :contact_id, :integer
+  end
+
+  def add_sender_from_message
+    ::Message.find_in_batches do |messages_batch|
+      Rails.logger.info "migrated till #{messages_batch.first.id}\n"
+      messages_batch.each do |message|
+        # rubocop:disable Rails/SkipsModelValidations
+        message.update_columns(sender_id: message.user.id, sender_type: 'User') if message.user.present?
+        message.update_columns(sender_id: message.contact.id, sender_type: 'Contact') if message.contact.present?
+        if message.sender.nil?
+          message.update_columns(sender_id: message.conversation.contact.id, sender_type: 'Contact') if message.incoming?
+        end
+        # rubocop:enable Rails/SkipsModelValidations
+      end
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index b3820f7c2..87d6ea781 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -292,18 +292,17 @@ ActiveRecord::Schema.define(version: 2020_06_25_154254) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.boolean "private", default: false
-    t.integer "user_id"
     t.integer "status", default: 0
     t.string "source_id"
     t.integer "content_type", default: 0
     t.json "content_attributes", default: {}
-    t.bigint "contact_id"
+    t.string "sender_type"
+    t.bigint "sender_id"
     t.index ["account_id"], name: "index_messages_on_account_id"
-    t.index ["contact_id"], name: "index_messages_on_contact_id"
     t.index ["conversation_id"], name: "index_messages_on_conversation_id"
     t.index ["inbox_id"], name: "index_messages_on_inbox_id"
+    t.index ["sender_type", "sender_id"], name: "index_messages_on_sender_type_and_sender_id"
     t.index ["source_id"], name: "index_messages_on_source_id"
-    t.index ["user_id"], name: "index_messages_on_user_id"
   end
 
   create_table "notification_settings", force: :cascade do |t|
@@ -436,5 +435,4 @@ ActiveRecord::Schema.define(version: 2020_06_25_154254) do
   add_foreign_key "contact_inboxes", "contacts"
   add_foreign_key "contact_inboxes", "inboxes"
   add_foreign_key "conversations", "contact_inboxes"
-  add_foreign_key "messages", "contacts"
 end
diff --git a/lib/integrations/slack/incoming_message_builder.rb b/lib/integrations/slack/incoming_message_builder.rb
index ab414d21f..e7ebd735f 100644
--- a/lib/integrations/slack/incoming_message_builder.rb
+++ b/lib/integrations/slack/incoming_message_builder.rb
@@ -82,7 +82,7 @@ class Integrations::Slack::IncomingMessageBuilder
       content: params[:event][:text],
       source_id: "slack_#{params[:event][:ts]}",
       private: private_note?,
-      user: sender
+      sender: sender
     )
 
     { status: 'success' }
diff --git a/lib/integrations/slack/send_on_slack_service.rb b/lib/integrations/slack/send_on_slack_service.rb
index 2a61e13f6..4c5bb46c6 100644
--- a/lib/integrations/slack/send_on_slack_service.rb
+++ b/lib/integrations/slack/send_on_slack_service.rb
@@ -20,10 +20,6 @@ class Integrations::Slack::SendOnSlackService < Base::SendOnChannelService
     update_reference_id
   end
 
-  def agent
-    @agent ||= message.user
-  end
-
   def message_content
     private_indicator = message.private? ? 'private: ' : ''
     if conversation.identifier.present?
@@ -38,7 +34,7 @@ class Integrations::Slack::SendOnSlackService < Base::SendOnChannelService
   end
 
   def send_message
-    sender = message.outgoing? ? agent : contact
+    sender = message.sender
     sender_type = sender.class == Contact ? 'Contact' : 'Agent'
 
     @slack_message = slack_client.chat_postMessage(
diff --git a/lib/integrations/widget/incoming_message_builder.rb b/lib/integrations/widget/incoming_message_builder.rb
index abc14aafa..82bc303f3 100644
--- a/lib/integrations/widget/incoming_message_builder.rb
+++ b/lib/integrations/widget/incoming_message_builder.rb
@@ -51,7 +51,8 @@ class Integrations::Widget::IncomingMessageBuilder
       account_id: conversation.account_id,
       inbox_id: conversation.inbox_id,
       message_type: 0,
-      content: options[:content]
+      content: options[:content],
+      sender: contact
     }
   end
 end
diff --git a/lib/integrations/widget/outgoing_message_builder.rb b/lib/integrations/widget/outgoing_message_builder.rb
index fb76f67d6..5791ea64e 100644
--- a/lib/integrations/widget/outgoing_message_builder.rb
+++ b/lib/integrations/widget/outgoing_message_builder.rb
@@ -46,7 +46,7 @@ class Integrations::Widget::OutgoingMessageBuilder
       inbox_id: @conversation.inbox_id,
       message_type: 1,
       content: options[:content],
-      user_id: user.id
+      sender: user
     }
   end
 end
diff --git a/spec/actions/contact_merge_action_spec.rb b/spec/actions/contact_merge_action_spec.rb
index 8afbdf6b4..815518ae3 100644
--- a/spec/actions/contact_merge_action_spec.rb
+++ b/spec/actions/contact_merge_action_spec.rb
@@ -10,7 +10,7 @@ describe ::ContactMergeAction do
   before do
     2.times.each { create(:conversation, contact: base_contact) }
     2.times.each { create(:conversation, contact: mergee_contact) }
-    2.times.each { create(:message, contact: mergee_contact) }
+    2.times.each { create(:message, sender: mergee_contact) }
   end
 
   describe '#perform' do
diff --git a/spec/factories/messages.rb b/spec/factories/messages.rb
index 6f9e37a5d..236095a0a 100644
--- a/spec/factories/messages.rb
+++ b/spec/factories/messages.rb
@@ -9,7 +9,7 @@ FactoryBot.define do
     account { create(:account) }
 
     after(:build) do |message|
-      message.user ||= create(:user, account: message.account)
+      message.sender ||= create(:user, account: message.account)
       message.conversation ||= create(:conversation, account: message.account)
       message.inbox ||= create(:inbox, account: message.account)
     end
diff --git a/spec/lib/integrations/slack/send_on_slack_service_spec.rb b/spec/lib/integrations/slack/send_on_slack_service_spec.rb
index bf083a72a..a709c2ce0 100644
--- a/spec/lib/integrations/slack/send_on_slack_service_spec.rb
+++ b/spec/lib/integrations/slack/send_on_slack_service_spec.rb
@@ -20,7 +20,7 @@ describe Integrations::Slack::SendOnSlackService do
       expect(slack_client).to receive(:chat_postMessage).with(
         channel: hook.reference_id,
         text: message.content,
-        username: "Contact: #{contact.name}",
+        username: "Agent: #{message.sender.name}",
         thread_ts: conversation.identifier,
         icon_url: anything
       )
diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb
index c92ed10d1..629b5fbf5 100644
--- a/spec/models/conversation_spec.rb
+++ b/spec/models/conversation_spec.rb
@@ -254,7 +254,7 @@ RSpec.describe Conversation, type: :model do
         conversation: conversation,
         account: conversation.account,
         inbox: conversation.inbox,
-        user: conversation.assignee
+        sender: conversation.assignee
       }
     end
     let!(:message) do
@@ -279,7 +279,7 @@ RSpec.describe Conversation, type: :model do
         conversation: conversation,
         account: conversation.account,
         inbox: conversation.inbox,
-        user: conversation.assignee,
+        sender: conversation.assignee,
         created_at: 1.minute.ago
       }
     end