From 12916ceca6b6feaa43d845ab0f7c01c2a753c14a Mon Sep 17 00:00:00 2001
From: Pranav Raj S <pranav@chatwoot.com>
Date: Fri, 26 Jan 2024 02:22:18 -0800
Subject: [PATCH] fix: Capture delivery errors to avoid false positives (#8790)

The system did not detect the delivery errors earlier, resulting in some false positives. The user was not informed when an email failed to be delivered. While we do handle failure status in other channels, we were not able to capture the actual delivery status for the email channel.

This pull request makes the following changes:

- Updated the class EmailReplyWorker to use the deliver_now method instead of deliver_later. This change is made to raise any errors that may occur with the SMTP connection. The errors are then captured and sent to Sentry, and the email is marked as failed. Previously, we did not consider the case of retries in the email channel, so this feature is currently not functioning. So, I have disabled the retry option. We will address this in a follow-up ticket.
- Downgraded the net-smtp gem to version 0.3.4. This change is made to avoid an argument error when using XOAUTH2.

Fixes: https://linear.app/chatwoot/issue/CW-3032/argumenterror-wrong-authentication-type-xoauth2-argumenterror
---
 Gemfile                                       |  2 +
 Gemfile.lock                                  |  3 +-
 .../widgets/conversation/Message.vue          |  9 ++-
 .../widgets/conversation/MessagesView.vue     |  1 +
 app/workers/email_reply_worker.rb             |  5 +-
 spec/workers/email_reply_worker_spec.rb       | 64 +++++++++++--------
 6 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/Gemfile b/Gemfile
index 144240b62..a21af8c01 100644
--- a/Gemfile
+++ b/Gemfile
@@ -39,6 +39,8 @@ gem 'rack-attack', '>= 6.7.0'
 gem 'down'
 # authentication type to fetch and send mail over oauth2.0
 gem 'gmail_xoauth'
+# Lock net-smtp to 0.3.4 to avoid issues with gmail_xoauth2
+gem 'net-smtp',  '~> 0.3.4'
 # Prevent CSV injection
 gem 'csv-safe'
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 62c6829ed..96881eb30 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -479,7 +479,7 @@ GEM
       net-protocol
     net-protocol (0.2.2)
       timeout
-    net-smtp (0.4.0)
+    net-smtp (0.3.4)
       net-protocol
     netrc (0.11.0)
     newrelic-sidekiq-metrics (1.6.2)
@@ -903,6 +903,7 @@ DEPENDENCIES
   meta_request
   mock_redis
   neighbor
+  net-smtp (~> 0.3.4)
   newrelic-sidekiq-metrics (>= 1.6.2)
   newrelic_rpm
   omniauth (>= 2.1.2)
diff --git a/app/javascript/dashboard/components/widgets/conversation/Message.vue b/app/javascript/dashboard/components/widgets/conversation/Message.vue
index 8eb03137b..e6890535b 100644
--- a/app/javascript/dashboard/components/widgets/conversation/Message.vue
+++ b/app/javascript/dashboard/components/widgets/conversation/Message.vue
@@ -1,7 +1,10 @@
 <template>
   <li v-if="shouldRenderMessage" :id="`message${data.id}`" :class="alignBubble">
     <div :class="wrapClass">
-      <div v-if="isFailed && !hasOneDayPassed" class="message-failed--alert">
+      <div
+        v-if="isFailed && !hasOneDayPassed && !isAnEmailInbox"
+        class="message-failed--alert"
+      >
         <woot-button
           v-tooltip.top-end="$t('CONVERSATION.TRY_AGAIN')"
           size="tiny"
@@ -203,6 +206,10 @@ export default {
       type: Boolean,
       default: false,
     },
+    isAnEmailInbox: {
+      type: Boolean,
+      default: false,
+    },
     inboxSupportsReplyTo: {
       type: Object,
       default: () => ({}),
diff --git a/app/javascript/dashboard/components/widgets/conversation/MessagesView.vue b/app/javascript/dashboard/components/widgets/conversation/MessagesView.vue
index 62eb6d739..7ff536942 100644
--- a/app/javascript/dashboard/components/widgets/conversation/MessagesView.vue
+++ b/app/javascript/dashboard/components/widgets/conversation/MessagesView.vue
@@ -33,6 +33,7 @@
         :is-a-whatsapp-channel="isAWhatsAppChannel"
         :is-web-widget-inbox="isAWebWidgetInbox"
         :is-a-facebook-inbox="isAFacebookInbox"
+        :is-an-email-inbox="isAnEmailChannel"
         :is-instagram="isInstagramDM"
         :inbox-supports-reply-to="inboxSupportsReplyTo"
         :in-reply-to="getInReplyToMessage(message)"
diff --git a/app/workers/email_reply_worker.rb b/app/workers/email_reply_worker.rb
index dddbae3dd..20cc70d5e 100644
--- a/app/workers/email_reply_worker.rb
+++ b/app/workers/email_reply_worker.rb
@@ -8,6 +8,9 @@ class EmailReplyWorker
     return unless message.email_notifiable_message?
 
     # send the email
-    ConversationReplyMailer.with(account: message.account).email_reply(message).deliver_later
+    ConversationReplyMailer.with(account: message.account).email_reply(message).deliver_now
+  rescue StandardError => e
+    ChatwootExceptionTracker.new(e, account: message.account).capture_exception
+    message.update!(status: :failed, external_error: e.message)
   end
 end
diff --git a/spec/workers/email_reply_worker_spec.rb b/spec/workers/email_reply_worker_spec.rb
index dc2d83e2b..579378918 100644
--- a/spec/workers/email_reply_worker_spec.rb
+++ b/spec/workers/email_reply_worker_spec.rb
@@ -11,34 +11,48 @@ RSpec.describe EmailReplyWorker, type: :worker do
   let(:mailer_action) { double }
 
   describe '#perform' do
-    before do
-      allow(ConversationReplyMailer).to receive(:with).and_return(mailer)
-      allow(mailer).to receive(:email_reply).and_return(mailer_action)
-      allow(mailer_action).to receive(:deliver_later).and_return(true)
+    context 'when emails are successfully sent' do
+      before do
+        allow(ConversationReplyMailer).to receive(:with).and_return(mailer)
+        allow(mailer).to receive(:email_reply).and_return(mailer_action)
+        allow(mailer_action).to receive(:deliver_now).and_return(true)
+      end
+
+      it 'calls mailer action with message' do
+        described_class.new.perform(message.id)
+        expect(mailer).to have_received(:email_reply).with(message)
+        expect(mailer_action).to have_received(:deliver_now)
+      end
+
+      it 'does not call mailer action with a private message' do
+        described_class.new.perform(private_message.id)
+        expect(mailer).not_to have_received(:email_reply)
+        expect(mailer_action).not_to have_received(:deliver_now)
+      end
+
+      it 'calls mailer action with a CSAT message' do
+        described_class.new.perform(template_message.id)
+        expect(mailer).to have_received(:email_reply).with(template_message)
+        expect(mailer_action).to have_received(:deliver_now)
+      end
+
+      it 'does not call mailer action with an incoming message' do
+        described_class.new.perform(incoming_message.id)
+        expect(mailer).not_to have_received(:email_reply)
+        expect(mailer_action).not_to have_received(:deliver_now)
+      end
     end
 
-    it 'calls mailer action with message' do
-      described_class.new.perform(message.id)
-      expect(mailer).to have_received(:email_reply).with(message)
-      expect(mailer_action).to have_received(:deliver_later)
-    end
+    context 'when emails are not sent' do
+      before do
+        allow(ConversationReplyMailer).to receive(:with).and_return(mailer)
+        allow(mailer).to receive(:email_reply).and_return(mailer_action)
+        allow(mailer_action).to receive(:deliver_now).and_raise(ArgumentError)
+      end
 
-    it 'does not call mailer action with a private message' do
-      described_class.new.perform(private_message.id)
-      expect(mailer).not_to have_received(:email_reply)
-      expect(mailer_action).not_to have_received(:deliver_later)
-    end
-
-    it 'calls mailer action with a CSAT message' do
-      described_class.new.perform(template_message.id)
-      expect(mailer).to have_received(:email_reply).with(template_message)
-      expect(mailer_action).to have_received(:deliver_later)
-    end
-
-    it 'does not call mailer action with an incoming message' do
-      described_class.new.perform(incoming_message.id)
-      expect(mailer).not_to have_received(:email_reply)
-      expect(mailer_action).not_to have_received(:deliver_later)
+      it 'mark message as failed' do
+        expect { described_class.new.perform(message.id) }.to change { message.reload.status }.from('sent').to('failed')
+      end
     end
   end
 end