From 96a0781bc181fdc4bda856bfb90b03ce55e342cf Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Wed, 19 Jun 2024 20:11:47 +0800 Subject: [PATCH] FIX: Avoid duplicating e-mail body in summary e-mail (#27535) We recently fixed a problem where secure upload images weren't re-attached when sending the activity summary e-mail. This fix contained a bug that would lead to n copies of the e-mail body being included, n being the number of duplicates. This is because #fix_parts_after_attachments! was called once per attachment, and adding more parts to the multipart e-mail. This PR fixes that by: Adding a failing test case for the above. Moving the looping over multiple posts into #fix_parts_after_attachments! itself. --- lib/email/sender.rb | 64 +++++++++++++++++++---------------- spec/lib/email/sender_spec.rb | 26 +++++++++++--- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index d19bd65e58a..3ca23cc6ff4 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -255,7 +255,7 @@ module Email add_attachments(post) elsif @email_type.to_s == "digest" @stripped_secure_upload_shas = style.stripped_upload_sha_map.values - digest_posts.each { |p| add_attachments(p) } + add_attachments(*digest_posts) end # Suppress images from short emails @@ -354,43 +354,45 @@ module Email Post.where(id: header_value("X-Discourse-Post-Ids")&.split(",")) end - def add_attachments(post) + def add_attachments(*posts) max_email_size = SiteSetting.email_total_attachment_size_limit_kb.kilobytes return if max_email_size == 0 email_size = 0 - post.uploads.each do |original_upload| - optimized_1X = original_upload.optimized_images.first + posts.each do |post| + post.uploads.each do |original_upload| + optimized_1X = original_upload.optimized_images.first - if FileHelper.is_supported_image?(original_upload.original_filename) && - !should_attach_image?(original_upload, optimized_1X) - next - end + if FileHelper.is_supported_image?(original_upload.original_filename) && + !should_attach_image?(original_upload, optimized_1X) + next + end - attached_upload = optimized_1X || original_upload - next if email_size + attached_upload.filesize > max_email_size + attached_upload = optimized_1X || original_upload + next if email_size + attached_upload.filesize > max_email_size - begin - path = - if attached_upload.local? - Discourse.store.path_for(attached_upload) - else - Discourse.store.download!(attached_upload).path - end + begin + path = + if attached_upload.local? + Discourse.store.path_for(attached_upload) + else + Discourse.store.download!(attached_upload).path + end - @message_attachments_index[original_upload.sha1] = @message.attachments.size - @message.attachments[original_upload.original_filename] = File.read(path) - email_size += File.size(path) - rescue => e - Discourse.warn_exception( - e, - message: "Failed to attach file to email", - env: { - post_id: post.id, - upload_id: original_upload.id, - filename: original_upload.original_filename, - }, - ) + @message_attachments_index[original_upload.sha1] = @message.attachments.size + @message.attachments[original_upload.original_filename] = File.read(path) + email_size += File.size(path) + rescue => e + Discourse.warn_exception( + e, + message: "Failed to attach file to email", + env: { + post_id: post.id, + upload_id: original_upload.id, + filename: original_upload.original_filename, + }, + ) + end end end @@ -442,9 +444,11 @@ module Email html_part = @message.html_part @message.html_part = nil + @message.parts.reject! { |p| p.content_type.start_with?("text/html") } text_part = @message.text_part @message.text_part = nil + @message.parts.reject! { |p| p.content_type.start_with?("text/plain") } content = Mail::Part.new do diff --git a/spec/lib/email/sender_spec.rb b/spec/lib/email/sender_spec.rb index 2e09e7d555a..211da031705 100644 --- a/spec/lib/email/sender_spec.rb +++ b/spec/lib/email/sender_spec.rb @@ -642,8 +642,11 @@ RSpec.describe Email::Sender do it "attaches allowed images from multiple posts in the activity summary" do digest_post = Fabricate(:post) + other_digest_post = Fabricate(:post) - Topic.stubs(:for_digest).returns(Topic.where(id: [digest_post.topic_id])) + Topic.stubs(:for_digest).returns( + Topic.where(id: [digest_post.topic_id, other_digest_post.topic_id]), + ) summary = UserNotifications.digest(post.user, since: 24.hours.ago) @@ -655,6 +658,14 @@ RSpec.describe Email::Sender do @secure_image_2.update_secure_status(override: true) @secure_image_2.update(access_control_post_id: digest_post.id) + @secure_image_3 = + UploadCreator.new( + file_from_fixtures("logo.png", "images"), + "something-cooler.png", + ).create_for(Discourse.system_user.id) + @secure_image_3.update_secure_status(override: true) + @secure_image_3.update(access_control_post_id: other_digest_post.id) + Jobs::PullHotlinkedImages.any_instance.expects(:execute) digest_post.update( raw: @@ -662,16 +673,21 @@ RSpec.describe Email::Sender do ) digest_post.rebake! + other_digest_post.update(raw: "#{UploadMarkdown.new(@secure_image_3).image_markdown}") + other_digest_post.rebake! + summary.header["X-Discourse-Post-Id"] = nil - summary.header["X-Discourse-Post-Ids"] = "#{digest_post.id}" + summary.header["X-Discourse-Post-Ids"] = "#{digest_post.id},#{other_digest_post.id}" Email::Sender.new(summary, "digest").send expect(summary.attachments.map(&:filename)).to include( - *[@secure_image, @secure_image_2].map(&:original_filename), + *[@secure_image, @secure_image_2, @secure_image_3].map(&:original_filename), ) - expect(summary.to_s.scan(/cid:[\w\-@.]+/).length).to eq(2) - expect(summary.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2) + expect(summary.to_s.scan("Content-Type: text/html;").length).to eq(1) + expect(summary.to_s.scan("Content-Type: text/plain;").length).to eq(1) + expect(summary.to_s.scan(/cid:[\w\-@.]+/).length).to eq(3) + expect(summary.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(3) end it "does not attach images that are not marked as secure, in the case of a non-secure upload copied to a PM" do