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

@@ -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|
+      posts.each do |post|
-        optimized_1X = original_upload.optimized_images.first
+        post.uploads.each do |original_upload|
+          optimized_1X = original_upload.optimized_images.first
 
-        if FileHelper.is_supported_image?(original_upload.original_filename) &&
+          if FileHelper.is_supported_image?(original_upload.original_filename) &&
-             !should_attach_image?(original_upload, optimized_1X)
+               !should_attach_image?(original_upload, optimized_1X)
-          next
+            next
-        end
+          end
 
-        attached_upload = optimized_1X || original_upload
+          attached_upload = optimized_1X || original_upload
-        next if email_size + attached_upload.filesize > max_email_size
+          next if email_size + attached_upload.filesize > max_email_size
 
-        begin
+          begin
-          path =
+            path =
-            if attached_upload.local?
+              if attached_upload.local?
-              Discourse.store.path_for(attached_upload)
+                Discourse.store.path_for(attached_upload)
-            else
+              else
-              Discourse.store.download!(attached_upload).path
+                Discourse.store.download!(attached_upload).path
-            end
+              end
 
-          @message_attachments_index[original_upload.sha1] = @message.attachments.size
+            @message_attachments_index[original_upload.sha1] = @message.attachments.size
-          @message.attachments[original_upload.original_filename] = File.read(path)
+            @message.attachments[original_upload.original_filename] = File.read(path)
-          email_size += File.size(path)
+            email_size += File.size(path)
-        rescue => e
+          rescue => e
-          Discourse.warn_exception(
+            Discourse.warn_exception(
-            e,
+              e,
-            message: "Failed to attach file to email",
+              message: "Failed to attach file to email",
-            env: {
+              env: {
-              post_id: post.id,
+                post_id: post.id,
-              upload_id: original_upload.id,
+                upload_id: original_upload.id,
-              filename: original_upload.original_filename,
+                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

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("Content-Type: text/html;").length).to eq(1)
-          expect(summary.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2)
+          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