From 758b9dd0ba6f7beb6673db65c3b471b0bbacf0a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 10 Jul 2024 09:59:27 +0200 Subject: [PATCH] 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 | 1 + lib/email/receiver.rb | 42 +++++++++++-- spec/lib/email/receiver_spec.rb | 103 +++++++++++++------------------- 3 files changed, 80 insertions(+), 66 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 227ba7fafaa..a0655d91095 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index c84b9ffcb50..7de0b311538 100644 --- a/lib/email/receiver.rb +++ b/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) - raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n" + elsif !is_duplicate[upload.id, upload_sha, attachment] + added_attachments << upload end - elsif !upload_ids.include?(upload.id) && !upload_shas.include?(upload_sha) - raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n" + elsif !is_duplicate[upload.id, upload_sha, attachment] + 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 diff --git a/spec/lib/email/receiver_spec.rb b/spec/lib/email/receiver_spec.rb index 297fb1a6a1d..aa8ed472fff 100644 --- a/spec/lib/email/receiver_spec.rb +++ b/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( - :post_reply_key, - user: user, - post: post, - reply_key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - ) + Fabricate( + :post_reply_key, + user: user, + post: post, + 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( - "![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})", - ) + expect(post.raw).to include UploadMarkdown.new(upload).to_markdown expect { process(:inline_image) }.to change { topic.posts.count } post = topic.posts.last upload = post.uploads.first - expect(post.raw).to include( - "![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})", - ) + expect(post.raw).to include UploadMarkdown.new(upload).to_markdown 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
··· - +
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(topic.posts.last.raw).to match %r{upload://} + expect { process(:logo_1) }.to change { Upload.count }.by(1) + logo = Upload.last + expect(topic.posts.last.raw).to include logo.short_url - expect { process(:logo_2) }.not_to change { UploadReference.count } - expect(topic.posts.last.raw).not_to match %r{upload://} - 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://} + expect { process(:logo_2) }.not_to change { Upload.count } + expect(topic.posts.last.raw).to include logo.short_url 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(post.uploads).to be_empty + expect(topic.posts.last.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( - "[#{upload.original_filename}|attachment](#{upload.short_url}) (64 KB)", - ) + expect(post.raw).to include UploadMarkdown.new(upload).to_markdown 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( - "[#{upload.original_filename}|attachment](#{upload.short_url}) (#{upload.filesize} Bytes)", - ) + expect(post.raw).to include UploadMarkdown.new(upload).to_markdown 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