fix: Update Slack integration to fix message delivery issues (#6093)

lib/integrations/slack/incoming_message_builder.rb

@@ -22,13 +22,23 @@ class Integrations::Slack::IncomingMessageBuilder
   private
 
   def valid_event?
-    supported_event_type? && supported_event?
+    supported_event_type? && supported_event? && should_process_event?
   end
 
   def supported_event_type?
     SUPPORTED_EVENT_TYPES.include?(params[:type])
   end
 
+  # Discard all the subtype of a message event
+  # We are only considering the actual message sent by a Slack user
+  # Any reactions or messages sent by the bot will be ignored.
+  # https://api.slack.com/events/message#subtypes
+  def should_process_event?
+    return true if params[:type] != 'event_callback'
+
+    params[:event][:user].present? && params[:event][:subtype].blank?
+  end
+
   def supported_event?
     hook_verification? || SUPPORTED_EVENTS.include?(params[:event][:type])
   end

lib/integrations/slack/send_on_slack_service.rb

@@ -36,12 +36,13 @@ class Integrations::Slack::SendOnSlackService < Base::SendOnChannelService
     if conversation.identifier.present?
       "#{private_indicator}#{message.content}"
     else
-      "*Inbox: #{message.inbox.name} [#{message.inbox.inbox_type}]* \n\n #{message.content}"
+      "\n*Inbox:* #{message.inbox.name} (#{message.inbox.inbox_type})\n\n#{message.content}"
     end
   end
 
   def avatar_url(sender)
-    sender.try(:avatar_url) || "#{ENV.fetch('FRONTEND_URL', nil)}/admin/avatar_square.png"
+    sender_type = sender.instance_of?(Contact) ? 'contact' : 'user'
+    "#{ENV.fetch('FRONTEND_URL', nil)}/integrations/slack/#{sender_type}.png"
   end
 
   def send_message
@@ -86,7 +87,7 @@ class Integrations::Slack::SendOnSlackService < Base::SendOnChannelService
   end
 
   def sender_name(sender)
-    sender.try(:name) ? "#{sender_type(sender)}: #{sender.try(:name)}" : sender_type(sender)
+    sender.try(:name) ? "#{sender.try(:name)} (#{sender_type(sender)})" : sender_type(sender)
   end
 
   def sender_type(sender)

spec/lib/integrations/slack/incoming_message_builder_spec.rb

@@ -12,6 +12,24 @@ describe Integrations::Slack::IncomingMessageBuilder do
       event_time: 1_588_623_033
     }
   end
+  let(:sub_type_message) do
+    {
+      team_id: 'TLST3048H',
+      api_app_id: 'A012S5UETV4',
+      event: message_event.merge({ type: 'message', subtype: 'bot_message' }),
+      type: 'event_callback',
+      event_time: 1_588_623_033
+    }
+  end
+  let(:message_without_user) do
+    {
+      team_id: 'TLST3048H',
+      api_app_id: 'A012S5UETV4',
+      event: message_event.merge({ type: 'message', user: nil }),
+      type: 'event_callback',
+      event_time: 1_588_623_033
+    }
+  end
   let(:message_with_attachments) { slack_attachment_stub }
   let(:message_without_thread_ts) { slack_message_stub_without_thread_ts }
   let(:verification_params) { slack_url_verification_stub }
@@ -81,6 +99,20 @@ describe Integrations::Slack::IncomingMessageBuilder do
         expect(conversation.messages.count).to eql(messages_count)
       end
 
+      it 'does not create message for message sub type events' do
+        messages_count = conversation.messages.count
+        builder = described_class.new(sub_type_message)
+        builder.perform
+        expect(conversation.messages.count).to eql(messages_count)
+      end
+
+      it 'does not create message if user is missing' do
+        messages_count = conversation.messages.count
+        builder = described_class.new(message_without_user)
+        builder.perform
+        expect(conversation.messages.count).to eql(messages_count)
+      end
+
       it 'does not create message for invalid event type and event files is not present' do
         messages_count = conversation.messages.count
         message_with_attachments[:event][:files] = nil

spec/lib/integrations/slack/send_on_slack_service_spec.rb

@@ -28,8 +28,8 @@ describe Integrations::Slack::SendOnSlackService do
 
         expect(slack_client).to receive(:chat_postMessage).with(
           channel: hook.reference_id,
-          text: "*Inbox: #{inbox.name} [#{inbox.inbox_type}]* \n\n #{message.content}",
-          username: "Contact: #{message.sender.name}",
+          text: "\n*Inbox:* #{inbox.name} (#{inbox.inbox_type})\n\n#{message.content}",
+          username: "#{message.sender.name} (Contact)",
           thread_ts: nil,
           icon_url: anything
         ).and_return(slack_message)
@@ -49,7 +49,7 @@ describe Integrations::Slack::SendOnSlackService do
         expect(slack_client).to receive(:chat_postMessage).with(
           channel: hook.reference_id,
           text: message.content,
-          username: "Contact: #{message.sender.name}",
+          username: "#{message.sender.name} (Contact)",
           thread_ts: conversation.identifier,
           icon_url: anything
         ).and_return(slack_message)
@@ -63,7 +63,7 @@ describe Integrations::Slack::SendOnSlackService do
         expect(slack_client).to receive(:chat_postMessage).with(
           channel: hook.reference_id,
           text: message.content,
-          username: "Contact: #{message.sender.name}",
+          username: "#{message.sender.name} (Contact)",
           thread_ts: conversation.identifier,
           icon_url: anything
         ).and_return(slack_message)
@@ -93,7 +93,7 @@ describe Integrations::Slack::SendOnSlackService do
         expect(slack_client).to receive(:chat_postMessage).with(
           channel: hook.reference_id,
           text: message.content,
-          username: "Contact: #{message.sender.name}",
+          username: "#{message.sender.name} (Contact)",
           thread_ts: conversation.identifier,
           icon_url: anything
         ).and_raise(Slack::Web::Api::Errors::AccountInactive.new('Account disconnected'))