FEATURE: email attachments in a details (#27804)

This change how we present attachments from incoming emails to now be "hidden" in a "[details]" so they don't "hang" at the end of the post.

This is especially useful when using Discourse as a support tool where email is the main communication channel. For various reasons, images are often duplicated by email user agents, and hiding them behind the details block help keep the conversation focused on the isssue at hand.

Internal ref t/122333

config/locales/server.en.yml

@@ -245,6 +245,7 @@ en:
       maximum_staged_user_per_email_reached: "Reached maximum number of staged users created per email."
       no_subject: "(no subject)"
       no_body: "(no body)"
+      attachments: "(attachments)"
       missing_attachment: "(Attachment %{filename} is missing)"
       continuing_old_discussion:
         one: "Continuing the discussion from [%{title}](%{url}), because it was created more than %{count} day ago."

lib/email/receiver.rb

@@ -462,6 +462,10 @@ module Email
         end
       end
 
+      # keep track of inlined images in html version
+      # so we can later check if they were elided
+      @cids = (html.presence || "").scan(/src\s*=\s*['"](cid:.+?)["']/).flatten
+
       markdown, elided_markdown =
         if html.present?
           # use the first html extracter that matches
@@ -772,7 +776,7 @@ module Email
           # return the email address and name
           [mail, name]
         end
-      rescue Mail::Field::ParseError, Mail::Field::IncompleteParseError => e
+      rescue Mail::Field::ParseError, Mail::Field::IncompleteParseError
         # something went wrong parsing the email header value, return nil
       end
     end
@@ -1366,7 +1370,21 @@ module Email
 
       upload_shas = Upload.where(id: upload_ids).pluck("DISTINCT COALESCE(original_sha1, sha1)")
 
+      is_duplicate = ->(upload_id, upload_sha, attachment) do
+        return true if upload_id && upload_ids.include?(upload_id)
+        return true if upload_sha && upload_shas.include?(upload_sha)
+
+        if attachment.respond_to?(:url) && attachment.url&.start_with?("cid:") &&
+             attachment.content_type&.start_with?("image/")
+          return true if @cids&.include?(attachment.url)
+        end
+
+        false
+      end
+
+      added_attachments = []
       rejected_attachments = []
+
       attachments.each do |attachment|
         tmp = Tempfile.new(["discourse-email-attachment", File.extname(attachment.filename)])
         begin
@@ -1392,11 +1410,11 @@ module Email
                 end
               elsif raw[/\[image:[^\]]*\]/i]
                 raw.sub!(/\[image:[^\]]*\]/i, UploadMarkdown.new(upload).to_markdown)
-              elsif !upload_ids.include?(upload.id) && !upload_shas.include?(upload_sha)
+              elsif !is_duplicate[upload.id, upload_sha, attachment]
-                raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n"
+                added_attachments << upload
               end
-            elsif !upload_ids.include?(upload.id) && !upload_shas.include?(upload_sha)
+            elsif !is_duplicate[upload.id, upload_sha, attachment]
-              raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n"
+              added_attachments << upload
             end
           else
             rejected_attachments << upload
@@ -1406,10 +1424,24 @@ module Email
           tmp&.close!
         end
       end
+
       if rejected_attachments.present? && !user.staged?
         notify_about_rejected_attachment(rejected_attachments)
       end
 
+      if added_attachments.present?
+        markdown =
+          added_attachments.map { |upload| UploadMarkdown.new(upload).to_markdown }.join("\n")
+        if markdown.present?
+          raw << "\n\n"
+          raw << "[details=\"#{I18n.t("emails.incoming.attachments")}\"]"
+          raw << "\n\n"
+          raw << markdown
+          raw << "\n\n"
+          raw << "[/details]"
+        end
+      end
+
       raw
     end
 

spec/lib/email/receiver_spec.rb

@@ -71,13 +71,12 @@ RSpec.describe Email::Receiver do
     user = Fabricate(:user, email: "staged@bar.com", active: false, staged: true)
     post = Fabricate(:post)
 
-    post_reply_key =
+    Fabricate(
-      Fabricate(
+      :post_reply_key,
-        :post_reply_key,
+      user: user,
-        user: user,
+      post: post,
-        post: post,
+      reply_key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
-        reply_key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+    )
-      )
 
     expect { process(:staged_sender) }.not_to raise_error
   end
@@ -91,7 +90,7 @@ RSpec.describe Email::Receiver do
 
     topic = Fabricate(:topic)
     post = Fabricate(:post, topic: topic)
-    user = Fabricate(:user, email: "discourse@bar.com")
+    Fabricate(:user, email: "discourse@bar.com")
 
     mail = email(:old_destination).gsub(":post_id", post.id.to_s)
     expect { Email::Receiver.new(mail).process! }.to raise_error(
@@ -666,18 +665,14 @@ RSpec.describe Email::Receiver do
       post = topic.posts.last
       upload = post.uploads.first
 
-      expect(post.raw).to include(
+      expect(post.raw).to include UploadMarkdown.new(upload).to_markdown
-        "![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})",
-      )
 
       expect { process(:inline_image) }.to change { topic.posts.count }
 
       post = topic.posts.last
       upload = post.uploads.first
 
-      expect(post.raw).to include(
+      expect(post.raw).to include UploadMarkdown.new(upload).to_markdown
-        "![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})",
-      )
     end
 
     it "supports attached images in HTML part" do
@@ -706,7 +701,11 @@ RSpec.describe Email::Receiver do
       expect(post.raw).to eq(<<~MD.chomp)
       [image:#{"0" * 5000}
 
-      ![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})
+      [details="#{I18n.t("emails.incoming.attachments")}"]
+
+      #{UploadMarkdown.new(upload).to_markdown}
+
+      [/details]
       MD
     end
 
@@ -725,7 +724,7 @@ RSpec.describe Email::Receiver do
       <details class='elided'>
       <summary title='Show trimmed content'>&#183;&#183;&#183;</summary>
 
-      <img src="upload://qUm0DGR49PAZshIi7HxMd3cAlzn.png" width="300" height="200">
+      <img src="#{upload.short_url}" width="300" height="200">
 
       </details>
       MD
@@ -740,7 +739,11 @@ RSpec.describe Email::Receiver do
       expect(post.raw).to eq(<<~MD.chomp)
       Please find some text file attached.
 
-      [#{upload.original_filename}|attachment](#{upload.short_url}) (20 Bytes)
+      [details="#{I18n.t("emails.incoming.attachments")}"]
+      
+      #{UploadMarkdown.new(upload).to_markdown}
+
+      [/details]
       MD
 
       expect { process(:apple_mail_attachment) }.to change { topic.posts.count }
@@ -758,33 +761,21 @@ RSpec.describe Email::Receiver do
 
     it "tries not to repeat duplicate attachments" do
       SiteSetting.authorized_extensions = "jpg"
+      SiteSetting.always_show_trimmed_content = true
 
-      expect { process(:logo_1) }.to change { UploadReference.count }.by(1)
+      expect { process(:logo_1) }.to change { Upload.count }.by(1)
-      expect(topic.posts.last.raw).to match %r{upload://}
+      logo = Upload.last
+      expect(topic.posts.last.raw).to include logo.short_url
 
-      expect { process(:logo_2) }.not_to change { UploadReference.count }
+      expect { process(:logo_2) }.not_to change { Upload.count }
-      expect(topic.posts.last.raw).not_to match %r{upload://}
+      expect(topic.posts.last.raw).to include logo.short_url
-    end
-
-    it "tries not to repeat duplicate secure attachments" do
-      setup_s3
-      stub_s3_store
-      SiteSetting.secure_uploads = true
-      SiteSetting.authorized_extensions = "jpg"
-
-      expect { process(:logo_1) }.to change { UploadReference.count }.by(1)
-      expect(topic.posts.last.raw).to match %r{upload://}
-
-      expect { process(:logo_2) }.not_to change { UploadReference.count }
-      expect(topic.posts.last.raw).not_to match %r{upload://}
     end
 
     it "works with removed attachments" do
       SiteSetting.authorized_extensions = "jpg"
 
       expect { process(:removed_attachments) }.to change { topic.posts.count }
-      post = topic.posts.last
+      expect(topic.posts.last.uploads).to be_empty
-      expect(post.uploads).to be_empty
     end
 
     it "supports eml attachments" do
@@ -796,7 +787,11 @@ RSpec.describe Email::Receiver do
       expect(post.raw).to eq(<<~MD.chomp)
       Please find the eml file attached.
 
-      [#{upload.original_filename}|attachment](#{upload.short_url}) (193 Bytes)
+      [details="#{I18n.t("emails.incoming.attachments")}"]
+
+      #{UploadMarkdown.new(upload).to_markdown}
+
+      [/details]
       MD
     end
 
@@ -837,9 +832,7 @@ RSpec.describe Email::Receiver do
       post = topic.posts.last
       upload = post.uploads.last
 
-      expect(post.raw).to include(
+      expect(post.raw).to include UploadMarkdown.new(upload).to_markdown
-        "[#{upload.original_filename}|attachment](#{upload.short_url}) (64 KB)",
-      )
     end
 
     it "supports liking via email" do
@@ -1020,20 +1013,16 @@ RSpec.describe Email::Receiver do
 
       it "extracts address and uses it for comparison" do
         expect { process(:reply_to_whitespaces) }.to change(Topic, :count).by(1)
-        user = User.last
         incoming = IncomingEmail.find_by(message_id: "TXULO4v6YU0TzeL2buFAJNU2MK21c7t4@example.com")
-        topic = incoming.topic
         expect(incoming.from_address).to eq("johndoe@example.com")
-        expect(user.email).to eq("johndoe@example.com")
+        expect(User.last.email).to eq("johndoe@example.com")
       end
 
       it "handles emails where there is a Reply-To address, using that instead of the from address, if X-Original-From is present" do
         expect { process(:reply_to_different_to_from) }.to change(Topic, :count).by(1)
-        user = User.last
         incoming = IncomingEmail.find_by(message_id: "3848c3m98r439c348mc349@test.mailinglist.com")
-        topic = incoming.topic
         expect(incoming.from_address).to eq("arthurmorgan@reddeadtest.com")
-        expect(user.email).to eq("arthurmorgan@reddeadtest.com")
+        expect(User.last.email).to eq("arthurmorgan@reddeadtest.com")
       end
 
       it "allows for quotes around the display name of the Reply-To address" do
@@ -1041,20 +1030,16 @@ RSpec.describe Email::Receiver do
           Topic,
           :count,
         ).by(1)
-        user = User.last
         incoming = IncomingEmail.find_by(message_id: "3848c3m98r439c348mc349@test.mailinglist.com")
-        topic = incoming.topic
         expect(incoming.from_address).to eq("johnmarston@reddeadtest.com")
-        expect(user.email).to eq("johnmarston@reddeadtest.com")
+        expect(User.last.email).to eq("johnmarston@reddeadtest.com")
       end
 
       it "does not use the reply-to address if an X-Original-From header is not present" do
         expect { process(:reply_to_different_to_from_no_x_original) }.to change(Topic, :count).by(1)
-        user = User.last
         incoming = IncomingEmail.find_by(message_id: "3848c3m98r439c348mc349@test.mailinglist.com")
-        topic = incoming.topic
         expect(incoming.from_address).to eq("westernsupport@test.mailinglist.com")
-        expect(user.email).to eq("westernsupport@test.mailinglist.com")
+        expect(User.last.email).to eq("westernsupport@test.mailinglist.com")
       end
 
       it "does not use the reply-to address if the X-Original-From header is different from the reply-to address" do
@@ -1062,11 +1047,9 @@ RSpec.describe Email::Receiver do
           Topic,
           :count,
         ).by(1)
-        user = User.last
         incoming = IncomingEmail.find_by(message_id: "3848c3m98r439c348mc349@test.mailinglist.com")
-        topic = incoming.topic
         expect(incoming.from_address).to eq("westernsupport@test.mailinglist.com")
-        expect(user.email).to eq("westernsupport@test.mailinglist.com")
+        expect(User.last.email).to eq("westernsupport@test.mailinglist.com")
       end
     end
 
@@ -1138,9 +1121,7 @@ RSpec.describe Email::Receiver do
       post = Topic.last.first_post
       upload = post.uploads.first
 
-      expect(post.raw).to include(
+      expect(post.raw).to include UploadMarkdown.new(upload).to_markdown
-        "[#{upload.original_filename}|attachment](#{upload.short_url}) (#{upload.filesize} Bytes)",
-      )
     end
 
     it "reenables user's PM email notifications when user emails new topic to group" do
@@ -1401,7 +1382,7 @@ RSpec.describe Email::Receiver do
       end
 
       it "processes the reply from the user as a brand new topic if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is disabled" do
-        email_log, group_post = reply_as_group_user
+        email_log, _group_post = reply_as_group_user
 
         reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
@@ -1415,7 +1396,7 @@ RSpec.describe Email::Receiver do
 
       it "processes the reply from the user as a reply if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is enabled" do
         group.update!(allow_unknown_sender_topic_replies: true)
-        email_log, group_post = reply_as_group_user
+        email_log, _group_post = reply_as_group_user
 
         reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
@@ -2216,7 +2197,7 @@ RSpec.describe Email::Receiver do
       SiteSetting.strip_incoming_email_lines = true
 
       receiver = Email::Receiver.new(email)
-      text, elided, format = receiver.select_body
+      text, _elided, _format = receiver.select_body
       expect(text).to eq(stripped_text)
     end