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

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

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)

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: () => ({}),

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

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

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