FIX: Failed to show details about some bounced emails

Bounces sent to reply_by_email_address could not be found.

app/controllers/admin/email_controller.rb

@@ -175,13 +175,20 @@ class Admin::EmailController < Admin::AdminController
     params.require(:id)
 
     begin
-      bounced = EmailLog.find_by(id: params[:id].to_i)
-      raise Discourse::InvalidParameters if bounced.nil?
+      email_log = EmailLog.find_by(id: params[:id].to_i, bounced: true)
+      raise Discourse::InvalidParameters if email_log&.bounce_key.blank?
 
-      email_local_part, email_domain = SiteSetting.notification_email.split('@')
-      bounced_to_address = "#{email_local_part}+verp-#{bounced.bounce_key}@#{email_domain}"
+      if Email::Sender.bounceable_reply_address?
+        bounced_to_address = Email::Sender.bounce_address(email_log.bounce_key)
+        incoming_email = IncomingEmail.find_by(to_addresses: bounced_to_address)
+      end
+
+      if incoming_email.nil?
+        email_local_part, email_domain = SiteSetting.notification_email.split('@')
+        bounced_to_address = "#{email_local_part}+verp-#{email_log.bounce_key}@#{email_domain}"
+        incoming_email = IncomingEmail.find_by(to_addresses: bounced_to_address)
+      end
 
-      incoming_email = IncomingEmail.find_by(to_addresses: bounced_to_address)
       raise Discourse::NotFound if incoming_email.nil?
 
       serializer = IncomingEmailDetailsSerializer.new(incoming_email, root: false)

app/serializers/incoming_email_details_serializer.rb

@@ -25,7 +25,7 @@ class IncomingEmailDetailsSerializer < ApplicationSerializer
   end
 
   def include_error_description?
-    @error_string[EMAIL_RECEIVER_ERROR_PREFIX]
+    @error_string && @error_string[EMAIL_RECEIVER_ERROR_PREFIX]
   end
 
   def headers

lib/email/sender.rb

@@ -165,13 +165,13 @@ module Email
         @message.header['List-Post'] = "<mailto:#{email}>"
       end
 
-      if SiteSetting.reply_by_email_address.present? && SiteSetting.reply_by_email_address["+"]
+      if Email::Sender.bounceable_reply_address?
         email_log.bounce_key = SecureRandom.hex
 
         # WARNING: RFC claims you can not set the Return Path header, this is 100% correct
         # however Rails has special handling for this header and ends up using this value
         # as the Envelope From address so stuff works as expected
-        @message.header[:return_path] = SiteSetting.reply_by_email_address.sub("%{reply_key}", "verp-#{email_log.bounce_key}")
+        @message.header[:return_path] = Email::Sender.bounce_address(email_log.bounce_key)
       end
 
       email_log.post_id = post_id if post_id.present?
@@ -282,5 +282,12 @@ module Email
         header_value('Reply-To').gsub!("%{reply_key}", reply_key)
     end
 
+    def self.bounceable_reply_address?
+      SiteSetting.reply_by_email_address.present? && SiteSetting.reply_by_email_address["+"]
+    end
+
+    def self.bounce_address(bounce_key)
+      SiteSetting.reply_by_email_address.sub("%{reply_key}", "verp-#{bounce_key}")
+    end
   end
 end

spec/requests/admin/email_controller_spec.rb

@@ -227,6 +227,77 @@ describe Admin::EmailController do
     end
   end
 
+  describe '#incoming_from_bounced' do
+    it 'raises an error when the email log entry does not exist' do
+      get "/admin/email/incoming_from_bounced/12345.json"
+      expect(response.status).to eq(404)
+
+      json = JSON.parse(response.body)
+      expect(json["errors"]).to include("Discourse::InvalidParameters")
+    end
+
+    it 'raises an error when the email log entry is not marked as bounced' do
+      get "/admin/email/incoming_from_bounced/#{email_log.id}.json"
+      expect(response.status).to eq(404)
+
+      json = JSON.parse(response.body)
+      expect(json["errors"]).to include("Discourse::InvalidParameters")
+    end
+
+    context 'bounced email log entry exists' do
+      let(:email_log) { Fabricate(:email_log, bounced: true, bounce_key: SecureRandom.hex) }
+      let(:error_message) { "Email::Receiver::BouncedEmailError" }
+
+      it 'returns an incoming email sent to the reply_by_email_address' do
+        SiteSetting.reply_by_email_address = "replies+%{reply_key}@example.com"
+
+        Fabricate(:incoming_email,
+                  is_bounce: true,
+                  error: error_message,
+                  to_addresses: Email::Sender.bounce_address(email_log.bounce_key)
+        )
+
+        get "/admin/email/incoming_from_bounced/#{email_log.id}.json"
+        expect(response.status).to eq(200)
+
+        json = JSON.parse(response.body)
+        expect(json["error"]).to eq(error_message)
+      end
+
+      it 'returns an incoming email sent to the notification_email address' do
+        Fabricate(:incoming_email,
+                  is_bounce: true,
+                  error: error_message,
+                  to_addresses: SiteSetting.notification_email.sub("@", "+verp-#{email_log.bounce_key}@")
+        )
+
+        get "/admin/email/incoming_from_bounced/#{email_log.id}.json"
+        expect(response.status).to eq(200)
+
+        json = JSON.parse(response.body)
+        expect(json["error"]).to eq(error_message)
+      end
+
+      it 'raises an error if the bounce_key is blank' do
+        email_log.update(bounce_key: nil)
+
+        get "/admin/email/incoming_from_bounced/#{email_log.id}.json"
+        expect(response.status).to eq(404)
+
+        json = JSON.parse(response.body)
+        expect(json["errors"]).to include("Discourse::InvalidParameters")
+      end
+
+      it 'raises an error if there is no incoming email' do
+        get "/admin/email/incoming_from_bounced/#{email_log.id}.json"
+        expect(response.status).to eq(404)
+
+        json = JSON.parse(response.body)
+        expect(json["errors"]).to include("Discourse::NotFound")
+      end
+    end
+  end
+
   describe '#advanced_test' do
     it 'should ...' do
       email = <<~EMAIL