Correctly wrap inline attachments. #26445

dracos · 2016-09-10T09:56:46Z

Summary

This changes the behaviour of actionmailer to correct wrap inline attachments when there are also other attachments. This fixes #2686.

The structure of the MIME email in such a case changes from:

multipart/related
  multipart/alternative
    text/plain
    text/html
  attachment (disposition: inline, image1)
  attachment (disposition: attachment, file1)
  attachment (disposition: attachment, file2)

(where the normal attachments are treated as inline and are not accessible), to:

multipart/mixed
  multipart/related
    multipart/alternative
      text/plain
      text/html
    attachment (disposition: inline, image1)
  attachment (disposition: attachment, file1)
  attachment (disposition: attachment, file2)

rails-bot · 2016-09-10T09:56:54Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

dracos · 2016-09-10T10:51:51Z

(The test failure appears to come from bin/setup outputting extra lines like "The git source git://github.com/sass/sass.git uses the git protocol, which transmits data without encryption. Disable this warning with bundle config git.allow_insecure true, or switch to the https protocol to keep your data secure." rather than any change I have made.)

prathamesh-sonpatki · 2016-09-10T14:48:34Z

@dracos I have opened #26449 to fix the failure you mentioned.

dracos · 2016-09-16T14:00:37Z

Thanks! I've rebased on master now that's been merged and Travis passes :)

dracos · 2016-09-23T13:29:13Z

Anything I can do to help move this along?

clemens · 2016-09-29T15:38:12Z

actionmailer/lib/action_mailer/base.rb

+      def wrap_inline_attachments(message)
+        # If we have both types of attachment, wrap all the inline attachments
+        # in multipart/related, but not the actual attachments
+        if message.attachments.detect(&:inline?) && message.attachments.detect { |a| !a.inline? }


This could be a guard clause instead (return unless ...).

clemens · 2016-09-29T15:38:42Z

actionmailer/lib/action_mailer/base.rb

+          m.header = message.header.to_s
+          if message.header[:bcc]
+            # copy bcc manually because it is omitted in header.to_s
+            m.bcc = message.header[:bcc].value


Inline the conditional?

clemens · 2016-09-29T15:55:00Z

I don't know about other people but to me this looks like it should be fixed in the mail gem – not ActionMailer. And brief research shows this might already be fixed – see mikel/mail#853 and mikel/mail#675. There's also a recent issue that deals with a similar issue (mikel/mail#1022).

dracos · 2016-09-29T16:01:47Z

Thanks for the feedback, I'll implement your two suggestions now.

Those mail issues you link are to about attachment ordering, not structure, which is what this is about. It is actionmailer, not mail, that is pulling the parts together and deciding the structure, and it is actionmailer that is not doing it correctly. As someone wrote on #2686 in 2014, "This isn't Mail's problem (as the division of labor currently stands). Mail is the low-level email library and Rails is responsible for building the MIME envelopes for multi-part emails."

clemens · 2016-09-29T16:52:20Z

I still think the approach of essentially rewriting the mail from scratch is questionable and @jonleighton probably just wrote it like this to prevent monkeypatching. I might have a better idea – I'll try it as soon as I'm back at a computer.

Sent from my iPhone.

On 29.09.2016, at 18:02, M Somerville notifications@github.com wrote:

Thanks for the feedback, I'll implement your two suggestions now.

Those mail issues you link are to about attachment ordering, not structure, which is what this is about. It is actionmailer, not mail, that is pulling the parts together and deciding the structure, and it is actionmailer that is not doing it correctly. As someone wrote on #2686 in 2014, "This isn't Mail's problem (as the division of labor currently stands). Mail is the low-level email library and Rails is responsible for building the MIME envelopes for multi-part emails."

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

eGust · 2016-11-23T01:06:52Z

I don't like this way...
There is my way to fix this issue. Since mail#1022 has been fixed, it should work fine.

 # ActionMailer::Base

  def set_content_type(m, user_content_type, class_default)
    params = m.content_type_parameters || {}
    case
    when user_content_type.present?
      user_content_type
    when m.has_attachments?
      if m.attachments.all?(&:inline?)
        ["multipart", "related", params]
      else
        if m.attachments.any?(&:inline?)
          related = Mail::Part.new
          related.content_type = "multipart/related"
          mixed = [ related ]

          m.parts.each do |p|
            if p.attachment? && !p.inline?
              mixed << p
            else
              related.add_part(p)
            end
          end

          m.parts.clear
          mixed.each { |c| m.add_part(c) }
        end
        ["multipart", "mixed", params]
      end
    when m.multipart?
      ["multipart", "alternative", params]
    else
      m.content_type || class_default
    end
  end

dracos · 2016-11-23T13:32:24Z

Thanks, that does look more straightforward! I have updated the PR with this, the new test still passes here. Travis PR failure is in unrelated area, actionmailer passed fine e.g. https://travis-ci.org/rails/rails/jobs/178288055

eGust · 2016-11-23T21:55:11Z

I doubt it's the best place to put my code. The current cost of attachment detection is minimum, but it sounds like a huge side effect while you just want to update content_type. I was wandering where to put it: just inside ActionMailer::Base#create_parts_from_responses, or another function call between create_parts_from_responses and set_content_type in ActionMailer::Base#mail. That's why I did not commit my code immediately while I found the solution.
BTW, this line looks very strange: m.content_type = set_content_type(...), I would like to move m.content_type = into set_content_type.

My way only fixes inline structure. Your initial approach also fixed the problem caused by mikel/mail#1022. But it's not a part of inline problem.
While several weeks ago I tested my code, a mail contains a html-only content body and a normal .txt attachment would be generated like this:

multipart/mixed
    type: text/plain, disposition: attachment
    type: text/html (no disposition)

Some mail clients recognize 1st part (.txt attachment) as content and 2nd part (html content) becomes a weird attachment.

It's a corner case caused by ActionMailer::Base.default_params (parts_order: [ "text/plain", "text/enriched", "text/html" ]) and Mail::PartsList#get_order_value did not care about if it's an attachment. Although it already got fixed in mikel/mail's master branch, I'm afraid test cases don't cover it yet.
Correct orders:

multipart/mixed
    type: text/html (no disposition)
    type: text/plain, disposition: attachment

dracos · 2016-11-24T14:12:14Z

Yes, after a bit more thinking, I've put it as another function call (sort of structurally what I had before, but with your more straightforward code) between create_parts_from_responses and set_content_type, as that's clearest to see the effect. Let's try and keep this focussed only on the fix to #2686, not other bugs that may exist; this bug can be fixed independently of ordering.

utilum · 2018-05-07T19:55:24Z

This resolves, perhaps imperfectly, the longest surviving issue. Is it on hold until a prefect solution is found?

nick-f · 2018-06-13T23:50:56Z

Is there anything that is still blocking this PR from being merged?

mariozaizar · 2018-06-14T17:56:17Z

👍

XenorPLxx · 2018-06-22T21:47:40Z

Yes, is there something that blocks this PR from being merged? I've just had to monkey patch my mailer using code provided by @dracos, because as much as mail is displayed correctly in Thunderbird if you'll try to forward it it won't attach any attachments. After applying the fix provided 'normal' attachment is attached in FW as it should've been.

SweeD · 2018-08-24T10:14:39Z

@guilleiguaran @matthewd @rafaelfranca this bug is still existing.
Is there anything holding back from merging this in?

dillonwelch · 2018-10-01T13:28:01Z

👍

dracos · 2018-10-01T15:29:54Z

I've just rebased on current master, I don't think the test failure is related to this PR. I'd certainly still like this to be included if possible :)

kylesnowschwartz · 2019-01-21T21:07:36Z

Hello @dracos! Thanks for being aware of this issue and trying to resolve it. I want to check on the process of this PR. I see that you rebased on master in October, but CI failed (the error appears to be something unrelated). Is there a schedule or timeline for trying again? I'm asking because I work on a piece of commercial software that maintains its own rails fork, and one of the reason why we do so is to include a monkey patch for this very issue.

Thanks! I'm a bit unschooled as to what the release process is like to rails, so any basic information would be helpful as well.

This switches the behaviour from: multipart/related multipart/alternative text/plain text/html attachment (disposition: inline, image1) attachment (disposition: attachment, file1) attachment (disposition: attachment, file2) to: multipart/mixed multipart/related multipart/alternative text/plain text/html attachment (disposition: inline, image1) attachment (disposition: attachment, file1) attachment (disposition: attachment, file2) Fixes rails#2686. Thanks to clemens and eGust for reviewing.

dracos · 2019-01-22T09:54:01Z

I have rebased on top of current master; as you say the previous test failure wasn't related to this change, so hopefully it might pass now, let's see. (Update: it has, yay)

I too have no input on the rails release process, I just know I believe I fixed rails' oldest open issue (from August 2011) well over two years ago now :)

RudyOnRails · 2019-01-25T13:27:00Z

grabs 🍿

mkarbowiak · 2019-07-12T12:47:24Z

Is there still hope?

rails-bot · 2019-12-17T22:52:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

dracos · 2019-12-18T09:56:56Z

Dear @rails-bot, as I said back in January at #26445 (comment) - this PR fixes rails' oldest open issue (over 8 years old now), the tests pass, and so I'd be more than happy for any recent activity to occur in order to get this into rails. If you can help me, @rails-bot, do let me know!

nick-f · 2019-12-21T00:30:21Z

@dracos I just posted about this to https://groups.google.com/forum/#!forum/rubyonrails-core to try and get some movement on this. Will update once the post is approved and visible.

Edit: https://groups.google.com/forum/#!topic/rubyonrails-core/D49bG5CypjM

nick-f · 2020-02-24T01:20:14Z

@clemens @guilleiguaran @matthewd Any chance there could be a review on this? Posting in rubyonrails-core got no traction.

rails-bot · 2020-05-24T01:31:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

mariozaizar · 2020-05-24T06:56:15Z

@tenderlove it seems that this PR went under the radar for an owner review. Is there still hope?

rails-bot assigned matthewd Sep 10, 2016

dracos mentioned this pull request Sep 10, 2016

Attachments not visible in mail clients when additional inline attachments present #2686

Closed

maclover7 added the actionmailer label Sep 10, 2016

guilleiguaran self-assigned this Sep 10, 2016

dracos force-pushed the multiparty branch from a69c835 to fc067ca Compare September 16, 2016 12:59

dracos force-pushed the multiparty branch 3 times, most recently from c9a606b to e82a2d3 Compare September 17, 2016 12:58

clemens reviewed Sep 29, 2016

View reviewed changes

HookyQR pushed a commit to HookyQR/rails that referenced this pull request Oct 17, 2016

Include changes from rails#26445

679cf3e

dracos force-pushed the multiparty branch from b9bfb1c to 2b1419a Compare November 23, 2016 13:32

dracos force-pushed the multiparty branch from 2b1419a to 540faba Compare November 24, 2016 14:03

dracos force-pushed the multiparty branch from 540faba to 978160e Compare October 1, 2018 14:31

dracos force-pushed the multiparty branch from 978160e to c95e980 Compare January 22, 2019 09:51

garethrees mentioned this pull request Oct 22, 2019

Attachment Display Failure mysociety/alaveteli#339

Open

rails-bot bot added the stale label Dec 17, 2019

rails-bot bot removed the stale label Dec 18, 2019

rails-bot bot added the stale label May 24, 2020

rails-bot bot removed the stale label May 24, 2020

tenderlove merged commit 397693a into rails:master Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly wrap inline attachments. #26445

Correctly wrap inline attachments. #26445

dracos commented Sep 10, 2016

rails-bot commented Sep 10, 2016

dracos commented Sep 10, 2016

prathamesh-sonpatki commented Sep 10, 2016

dracos commented Sep 16, 2016

dracos commented Sep 23, 2016

clemens Sep 29, 2016

clemens Sep 29, 2016

clemens commented Sep 29, 2016

dracos commented Sep 29, 2016

clemens commented Sep 29, 2016

eGust commented Nov 23, 2016 •

edited

Loading

dracos commented Nov 23, 2016 •

edited

Loading

eGust commented Nov 23, 2016 •

edited

Loading

dracos commented Nov 24, 2016

utilum commented May 7, 2018

nick-f commented Jun 13, 2018

mariozaizar commented Jun 14, 2018

XenorPLxx commented Jun 22, 2018

SweeD commented Aug 24, 2018

dillonwelch commented Oct 1, 2018

dracos commented Oct 1, 2018

kylesnowschwartz commented Jan 21, 2019

dracos commented Jan 22, 2019 •

edited

Loading

RudyOnRails commented Jan 25, 2019

mkarbowiak commented Jul 12, 2019

rails-bot bot commented Dec 17, 2019

dracos commented Dec 18, 2019

nick-f commented Dec 21, 2019 •

edited

Loading

nick-f commented Feb 24, 2020

rails-bot bot commented May 24, 2020

mariozaizar commented May 24, 2020

Correctly wrap inline attachments. #26445

Correctly wrap inline attachments. #26445

Conversation

dracos commented Sep 10, 2016

Summary

rails-bot commented Sep 10, 2016

dracos commented Sep 10, 2016

prathamesh-sonpatki commented Sep 10, 2016

dracos commented Sep 16, 2016

dracos commented Sep 23, 2016

clemens Sep 29, 2016

Choose a reason for hiding this comment

clemens Sep 29, 2016

Choose a reason for hiding this comment

clemens commented Sep 29, 2016

dracos commented Sep 29, 2016

clemens commented Sep 29, 2016

eGust commented Nov 23, 2016 • edited Loading

dracos commented Nov 23, 2016 • edited Loading

eGust commented Nov 23, 2016 • edited Loading

dracos commented Nov 24, 2016

utilum commented May 7, 2018

nick-f commented Jun 13, 2018

mariozaizar commented Jun 14, 2018

XenorPLxx commented Jun 22, 2018

SweeD commented Aug 24, 2018

dillonwelch commented Oct 1, 2018

dracos commented Oct 1, 2018

kylesnowschwartz commented Jan 21, 2019

dracos commented Jan 22, 2019 • edited Loading

RudyOnRails commented Jan 25, 2019

mkarbowiak commented Jul 12, 2019

rails-bot bot commented Dec 17, 2019

dracos commented Dec 18, 2019

nick-f commented Dec 21, 2019 • edited Loading

nick-f commented Feb 24, 2020

rails-bot bot commented May 24, 2020

mariozaizar commented May 24, 2020

eGust commented Nov 23, 2016 •

edited

Loading

dracos commented Nov 23, 2016 •

edited

Loading

eGust commented Nov 23, 2016 •

edited

Loading

dracos commented Jan 22, 2019 •

edited

Loading

nick-f commented Dec 21, 2019 •

edited

Loading