From 38587b3aa1acacd97efc1d45de12d93eaf2efb78 Mon Sep 17 00:00:00 2001
From: Pranav Raj S <pranav@chatwoot.com>
Date: Sat, 17 Dec 2022 16:41:11 -0800
Subject: [PATCH] fix: Update Slack integration to fix message delivery issues
 (#6093)

---
 .../slack/incoming_message_builder.rb         |  12 ++++++-
 .../slack/send_on_slack_service.rb            |   7 ++--
 public/integrations/slack/contact.png         | Bin 0 -> 2252 bytes
 public/integrations/slack/user.png            | Bin 0 -> 2189 bytes
 .../slack/incoming_message_builder_spec.rb    |  32 ++++++++++++++++++
 .../slack/send_on_slack_service_spec.rb       |  10 +++---
 6 files changed, 52 insertions(+), 9 deletions(-)
 create mode 100644 public/integrations/slack/contact.png
 create mode 100644 public/integrations/slack/user.png

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 0000000000000000000000000000000000000000..d20892b7c34445b21fb70e9cb51bf8c342aad6a7
GIT binary patch
literal 2252
zcmb_e`9G8k7q^U>2kB|%3K27yG3C;b%4jCdgBaU9wkS)c#9UEiX}aMu^Nc0?avAPL
z6U9|AvUQa(O^IxkPbOunt|YsbQL5K{KkuLL{&2qM`#tCT+xdLXIccz~{U+sY%5rjY
zn-~tZ?s9VS$iIP7T(3y-9Vgb4A?)mFw>!RVPu$%-@ppgx__0pjFRw;bJ`wd!dF6Hw
zOf1?a-z&N^rui${eBG}Y?e6NK*c4>!&)+X6hxTOH(mgpNbEAD{u@q1)kpD{F!Amz$
z@eJ;P^%5Tmnw54jmQG~p`bcRbVKT}H%JM~$;RQ+3<xlK|tA$m*M)H{a;>Ql5*{#3N
zZ;a@B>*i$AmtiPj`1uAr$7!5;wD10a&j@bYz65k_cuuC?XBOX-HRl`=&x?3TA%x|q
zJ^4GX>~RYl`>eaABEPWFwlsnl`&;xgqtVN*K2_A(^EN&eOet=>$~*~h4phJjI#5LN
z|Ai)IkG?MDwUIa+(aM({cH;RZI3RWA)lVw=vj>WbUVZm~kO3gkL$ctBZtotF<)eg4
ze!-G&@(JSc+}?}{?O3iOIsMEn;8;)=hhMySc<Q1CXI=Dj_`idP&I9y}{6Wp0mu}kw
z4K0k^4MI;KNSC|u{G>r+Ww3MomJ%C(C?+Fg(I>Qv=7$tor0yVc6Lu?vIg*jOCnU8c
za9gexUh@+EPHys&oC-d}8NfARP(p*u3WNtbrl-1rV^UBz5NDyUt#Gg^h-sO3_;D<g
ztS!K&uuq8#8#8jF#F9cd$_0HTMv?{Ce?pp1hzHBy7$-I(QD8-g#-RyCNHB_yexL^6
z<{mZ^o^Sa=MlRZj!DKkb9!-EH{b+j_Y2FQKT!dro*bX91OZ}lqG|e7iMj&4Hh^dZn
zHhHih(JU5X(=k_F(e^n=GggesgQK8S9FU{}*>E3p$P3k+rYX2EF{XJtXeC1O6d{d5
z*tBf0pu^1cPY}Ys5(6=8s4hZGv|!zifb>d4M=gblE+lo=X$afH$Lg~o*g}Z!JOPvO
zh>=7Idvp=`r4zBD3cblJl`@e(dD4_R8QX~*pbFi|LPaJ?-C59q4OPOnE+XdGS{w__
ztTvxi#^`gP4%BFl3Yr*U3L7ro>Oh&U2oh5!Iwene@7gz}lXZXQn*`F&N*;;5BO=Fm
z_J+FD>g9<@;pWZZG*0wbm$~J!cf$@;PH`8{yN1fC@8b=t#B;iz^1Rpklh1j>{WQ+%
zcnXtDm4Od<1G!Wgsh7uGiIW|C!W;0U$$X?dW^=qOVvIMZLY3NzQ*KbHQusbEJe(@^
zdzfNUMwPPDzsH75ZcKG~TXugQJxSK|9k_I=uX-jdS}i1vd~-Z8@{`Kp4J;$Ux0FJ_
zpVpSW_g~Qz^rFvU;AXct&{-Ba0(HkaNf#<AR?}3kB2xqJ`a4da7%TT^U%nM6wtlO@
zYN0K!>Md6Lh#S)FO6-K5tv;j#Qr_ap1Nh7aykJ2!K;2%@J{1^tm-RKS>2&kz-=gV^
zeh+i=xvYdv?HDH_(tX9)jCN-t@0*($@)=-IEVKo?&Y`~Afq7pBrL(wfFo<?Yko`;4
z06QbggHdz#qrGJ~wA`dyq7%BZ$F4aLGQPnZwmwP<w=6a-%=r>B{9!Yvq;jl`k9ktJ
z0)H0VsMBgy!W=!)#2sUW9P#RojzofoWi^d=er?MWX%9_3rN5qNpFP~tPU6lSyIb@_
zcE79Ub|a2^Hb7MV8SMR>^XR&QVbQ>kfRApKoh{UTr&>X%EQR;=yc!Z;{yL>69rwn#
z+PXUXpZLN$FvaMN3HST95WTFIYrD<>O0EyvUU_9TWL2w0SzkMCgSD6{>`ec_6OV_k
z^~wn@?YU=70jck9eHs{OdbS}DXvMB&8&#Do=q~8Y+I8OJl|FqvW!*mHuOmm8-qjGd
zc5YsI+i!P<zNc->K{a?4rqq>F<M*Au`y&4Aosi~3iVmWqt4%(;lMSRd+Fx9M$=i}u
zS;4m1Qj%*7)p~2Ad}11NEK&NZqg!goNNcnv4HQcV!VPrvm8Yrl==c`oBCXRuyQJlq
zdg_It2F)Yb)>`1CFGMP}4`WQ<Hu9WB*XC20kTKd9L(jO5#;PJEn}m2bNe5Sy#v5$L
zMMr`?QwXe|CKW^={K36M)q1ezD#~JxbejMu{}<Q3<<>AUh9hplPXEJ)?<;g-@;U<a
z{(5hibi2RbBT{8$Sp8yVo;+S`hyDPVr2Tw#1ThFhn)eFRGvQ|h3&U1THfBbKrnx|R
zdcw<$=~o5?AJJ$J{2nb-v8RK}20BsQJ)fS#;130X(e8K?EmWzegVP3j#lfDP`Ka%d
zs?bk6NWJPe->NEnTTVZ_;}9s=YfK#E_I4Cn-2%)NeP<?!IS2l7>3$pOsxT7f+8;vS
z@kjp35x8gSP2ARkn-og9QfWOUJZ@{{O^R8$QnBOyrm?N;@uLn|bAE_gFtkHMm|hI`
z+oh7RTq<g4ySsalAv?x_;h%TO6HoAFaNYR@egt6WHH~-IG`WY%ONOom5FOqKiHH=a
z&l?oG3U&g%Qow*PX&NgICkV4kVK)p>3hW`UQcGbkjQ%WgaD9b-FoQv84e_B@zc%{t
z6JoN4<gQPmTyz5$CuaJ1fmSkrpv2;q!yaI(3^=L8N-c-IK>alY_s3uX>iC}wHjGm>
z2k=VJI(5T2?*?L&k+KEEL>X}-vho#J`|~4;6eeSdlUOBjC{f5vf|p7Mg$GHit^yc0
zBuLB=LE$rN@86uy1W4$XYV5Q7F*C^i<7rlt$>7joae}`J_V@v<=~4biEmqoI*0ZcB
zUrgKMRa^3~QLe%Iz4p@?tyX_=DnubNbN>3=)HyLfgILe4r<Fe%U%0od+;&CO^iRWu
aMeEJp0UsSZh2PfS4LOFLt8Fb5$o&uQ|NE8z

literal 0
HcmV?d00001

diff --git a/public/integrations/slack/user.png b/public/integrations/slack/user.png
new file mode 100644
index 0000000000000000000000000000000000000000..7752b7ea0cc1972eef34f6259d22da1f59e4b995
GIT binary patch
literal 2189
zcmb_e`8$*g8=lcP-d8kZ27_#~m@%X#Tgf)_8X8%eY{yB-99xA-DMOZqmqWIR2q%Nk
zV#)N$8e&K)N~%L7CQ6Bn<)qT#_`bj4`{B8t=en=wx}N)o`+BZBmE*csQC>qHfj}ra
zvRQ5j1QPylp*C$W-8uUvHX4EB>|ww6lyh_9&St*J2ClEKZ(!GDm+^<CD+?c=-3p%_
z>%r>{X>Tl}BHUcv5zqCrbP~X11OmW2vX~yck-4$`?l0NO2&w9md-bZ3HAeHtbH_^x
z0#0KCDLEGVm3LD+PCrc-O{W?PO+<^dD|}A|#c@3<C$2Vj{<w<NWWFsTGW!G}@6JN>
z8OHX8Vr#tZ<TK=R-jbb^$1wLD1I*1oVH-}K{`BC&sh`Y$h(F`iwH4V9Ul@vgMqu=>
zQKq`u<(4y9?LXi>k|LgCkyIFMw&QbD&K~ZlocKZvoew(u$>8!kP?Z1QC=w1^TD4oz
z#5qsWx^^go=N~4u;v^67zr$^$@47be1eUjKlfz{XU_2l!hRRgaew6&}>N{$8QfNB;
zq3buY&m>|OlZiNi8QWZ<SOR++IArDulq+FTx1*Yuo5eL{JwlF#k{>xH_h9(NER0is
z?;WKGd1R9&{HJMxvz|mh1K|#)a0)O)XE^qlvWXSU<vIZFrto2!mhUeB<d%<R^z6gH
z&8C?)TOgv*H5PNYp3q0fBqu2%OK4yt9{J9A7-wJ!O$q6Fd3R2^_J)G|5Qtb)7U4Wx
zPwmsh8Jvfv@-R*pdW*0hFmSs&gk%DTv5duD^PE`#rv^PElqL{ZUw~B9#X)s0%07P&
zDS!iasBrZ=iDy|U5}=tRMr971Vf9V`{H-#6G;fte#sPV+2Y$?9KDqBI#+VIFz#Ik^
z!lMR`H{1&5mlcN1yuM0L&~w(pd=SCyn;==I6ip2IRK$LE2^csHs}%|&UDcSl>qR)q
z0PH&rTbP!PTa+eHSVcJbqtLL~#%<DEzMup5I1}7yMVQa0CsViWp`=fMFET+(DO{5Y
z;&@Q4zgSZ^5S;MBo_w!@><-E9PuXzCMOa3NF_!Ei_nM(_O{)15a7Sb?MH<5gd4P6b
zk+nCx;799AlyyYsC0To4U71iyl|xrr@eTp;=eO*k@}wW%B;wVqcp2S^SE8(V6^|3~
zqcOa;Q<>#A*51~RL`%hWa<xV0rHl8`#eNrVBSI|*JgG&toK8}d^q3H>!;Y2~RLbdW
zJ}-S_-knTu@NY$)JTlQq!P<nrmvkl@XorT_#h)pi(hN$w5gV*UqY?8n8Vstmtc`$2
zuZoN9LL<+erIiiJa%qCt6qo5d*+4Fx@+P!e>-t}l<r6_VE=ysJVa1EvW_S5%T1G!!
z`g>ZfPGs~!XK3NBk9h9Ghl;tN^2S#YBYKjj?IW?HM%?)~TId$io6$jw`WA(@d+xS%
zqlxqr4u-~^<z;VI(e&d^6gVo@;3zsP&avy+80M>va(TEJj4teq!oyQs`@)mMYa;yR
z&(!MOUw#j24_mWqjk5rjEX76F19-g7?*^-%TEK-F3#Y(y{z5xvyQH%khpD31Ozo=C
zJ0}dlRnr!2n!47F>{tJEX@KFeULiN!JdQBGYk;1yi9@yrZ?2ug)?|rB?}}Dc^fPXc
zF1G*~k&-iG{taKZ&X0r;`~G|=AN@y>S=PQEp9$UP;&X+&j%+4`^k?jUTv09U`ir4v
z=y7v#A#JYy{srnyh4)WA(VZS@?zeQ@hzO0!>k`HsyGq@Z8EzGO`?PNLNR!uhW1u%p
z_sU+mM(ro7E@y2}PMqSGF()lFqrSN`FI_J|W!wt%*pmH&REf)2J%Y|IF{@KR&!FYk
z%gyXvkYM4`>U*7gGfiLHNd2#nyA#%y*M4{`wRKHC$st-bx9&b9d@0oYw5D8t923$6
z423TYwT;{UoE{Fi^h>&5Q_tdx<n`-^5#JVOXQfA8cxvd1HEg=v<KkpkH{U1`)u3%!
z4qKq!Ls%IAA{IG-jQl3O<yr7t*5GVeTWtfjXE1zQHCPY=`8l!6m-LN5+zKqz>O8x%
zO8G9sw@+TyNPw?{n7eSvSo?BSSZRy+Wr9GWi~0bwh^d45qO>Hs3B|)3xI{y`XqyFR
zmk9RuTRTq7?+vqd6!ez$IAcG~htw31*;5DnDd6m6+1K#w4(q30>fxK~hE!Uz+L5Il
z_Q2oZ9hPddITyc`d)=j1q$G1#%(<54k%O5eO%=kR4YWUpBO$S(QE&yvM<Jp6<F)W~
zLhofLY$ss+KgN~zQYEqE^shJXn^aR~C&;14FveG5Mj~f|49{#x&2THAi}&Rl;3(dp
zbs&_Si_yoCM>u+99#}!-re6_p$j(x1^hH=<2Z2LY=7FUe$Z)gT>R@J2`C-eqs&F#i
z%uazH=eZWQoC~N2K_x7GIu25inMuA|sRtfZJm%?yy{j=;nMc;o5O`%}qiTdLl~wW@
zA(tAJXc+S1Q7U;D3^XM4xc2&h{<E;XAy>|lTa~-<^*=UZjc4JKTn?AYs)1!J385qf
zy&r5J0E~lPLn#ZmMOCqyE%!O3CZRg}0Jxv9?`Cdh@xVz!z&DV;4$IgQmWy>kmI;^5
z7nozQ^6-ByVcd16+HpO~z~*s~Bo7}WasQCvK8X~=GNsd&Tvz_UN|E4zF3Op~9xits
z{>qo;?U5w%96uu;8Go@QN9_=DsI9oHTR7iT#Tj_|U7Ym`QVa&Nx;A!6=&NLIPQx|S
zF0xbbm%;dLvMbv+)pvgFx5q@T8r1(bYDkDP6XdUrZx^>J+4@iZO=+tT%?0oJZCPho
XrQhwj4L@rmG9Vo7U0Jswzl47QxH!4e

literal 0
HcmV?d00001

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