diff --git a/lib/integrations/slack/incoming_message_builder.rb b/lib/integrations/slack/incoming_message_builder.rb index a2f75ce3a..44d373259 100644 --- a/lib/integrations/slack/incoming_message_builder.rb +++ b/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 diff --git a/lib/integrations/slack/send_on_slack_service.rb b/lib/integrations/slack/send_on_slack_service.rb index 1ded0547f..87440cb6e 100644 --- a/lib/integrations/slack/send_on_slack_service.rb +++ b/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) diff --git a/public/integrations/slack/contact.png b/public/integrations/slack/contact.png new file mode 100644 index 000000000..d20892b7c Binary files /dev/null and b/public/integrations/slack/contact.png differ diff --git a/public/integrations/slack/user.png b/public/integrations/slack/user.png new file mode 100644 index 000000000..7752b7ea0 Binary files /dev/null and b/public/integrations/slack/user.png differ diff --git a/spec/lib/integrations/slack/incoming_message_builder_spec.rb b/spec/lib/integrations/slack/incoming_message_builder_spec.rb index 2ec88c026..00a67df95 100644 --- a/spec/lib/integrations/slack/incoming_message_builder_spec.rb +++ b/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 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 b0018d0ec..e9806d174 100644 --- a/spec/lib/integrations/slack/send_on_slack_service_spec.rb +++ b/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'))