message body parts should be a sub body

[willpower232]

Resolves #375

[adamcrown]

It seems like this outer condition that checks for plain text only messages should still exist. There's no reason to use multipart/alternative for those, right?

[willpower232]

I was initially erring on the side of caution but also the comment I linked to from the issue suggests that if you include a text/plain attachment without this arrangement, the attachment can get lost on some platforms.

Either way, its better to clearly define the parts within a message as theres no knowing what email software and servers will be involved in the passage of the message to its recipient.

[sgregori]

Works like a charm !

[alexander-rieder]

when will this get merged?

[alexander-rieder]

Any news?

[jaydrogers]

I also discovered this issue and am interested in seeing this get merged. Also, I manually applied this fix to my install and it resolved my issue.

[alexander-rieder]

@naft-a What is stopping this pull request from getting merged?

[adamcooke]

I'd say the main thing holding this up is that we likely don't want plain text only e-mails to be within the multipart/alternative block.

• If there's a plain text only, it should just have that.
• If there's HTML only it should just have that.
• If there's both it should use multipart/alternative.

This should be the behaviour of Mail (and seems to be based on a quick test we just did) but maybe there's been a change to that recently.

[catphish]

I've just tested this, and it doesn't appear that this patch is necessary. The current behaviour using the current mail gem appears to already be what this pull request aims to create:

irb(main):002:0> mail = Mail.new

irb(main):003:0> mail.text_part = Mail::Part.new
irb(main):004:0> mail.text_part.body = "arf"

irb(main):005:0> mail.html_part = Mail::Part.new
irb(main):006:0> mail.html_part.content_type = "text/html; charset=UTF-8"
irb(main):007:0> mail.html_part.body = 'html here'

irb(main):008:0> puts mail.to_s

Date: Thu, 05 Dec 2019 17:04:35 +0000
Message-ID: <[email protected]>
Mime-Version: 1.0
Content-Type: multipart/alternative;
 boundary="--==_mimepart_5de938625b425_1d646b90a4714ab";
 charset=UTF-8
Content-Transfer-Encoding: 7bit


----==_mimepart_5de938625b425_1d646b90a4714ab
Content-Type: text/plain;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

arf
----==_mimepart_5de938625b425_1d646b90a4714ab
Content-Type: text/html;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

html here
----==_mimepart_5de938625b425_1d646b90a4714ab--

[catphish]

@alexander-rieder Are you seeing some behaviour that implies that this is not working correctly?
@willpower232 I won't merge this as-is because it breaks the ability to send a normal plain-text-only email. If necessary, we can split it out to create a multipart message when both parts are specified, but as far as I can see, the mail gem (at least now) does this automatically.

[Josh-G]

The current behaviour using the current mail gem appears to already be what this pull request aims to create

@catphish not quite - your example doesn't involve attachments which is the crux of the issue.
I've created a little example to help demonstrate the issue at hand. Please bear with me as I'm not familiar with Ruby at all.

I've created a gist with a minimal repro using the mail gem directly, in an identical way to Postal before and after this PR. It outputs two eml files, one of which has an invalid structure.

The root part of test-broken.eml is multipart/alternative but includes a part that represents a test PDF attachment. However, it should be a multipart/mixed containing a multipart/alternative part followed by the PDF part. The test-broken email does not render correctly on a few clients, including but not limited to:

• Outlook for Windows (no attachment visible)
• Apple Mail (attachment visible but no body rendered) (includes iOS mail app?)
• Thunderbird (I haven't personally tested this one)

It looks like this was reported to the gem back in 2013 but closed by the gem author as not a bug. The gem author wants the user to create the mixed wrapper themselves. The change by @willpower232 implements this.

We work on a similar closed-source project that parses, transforms and then outputs mime messages through a custom SMTP server. We've had similar problems with attachments and inline images in some clients (looking at you Apple Mail). Through testing, we have found the most compatible way to format the message is to always have:

mixed
    alternative
        text/plain
        multipart/related
            text/html
            any/images
    any/attachments

We always transform the input message into the above, creating parts as necessary. This ensures that all other manipulation of the message we perform doesn't result in rendering issues.

I'd say the main thing holding this up is that we likely don't want plain text only e-mails to be within the multipart/alternative block.

If I understand correctly, @adamcooke, you would like the following behaviour after this PR is implemented:

• text part only generates a text/plain email
• html part only generates a text/html email
• both text/html generates a multipart/alternative email
• any combination of text + html that includes attachments generates a multipart/mixed with an alternative part and the attachments

I'm discounting multipart/related here as I'm guessing it isn't possible to embed images?

I hope this clears up what we're trying to achieve.

[mattbaylor]

This may be a naive question, however: Is this PR going to be merged? It appears to fix a bug in Postal, at least it fixes the attachment/body issue I brought up in #2236. Thanks for helping me understand the process.

[catphish]

Merged, I now understand the problem and this appears to solve it! I do apologize for taking SO long to test and merge this.

[Paz08]

Was this really merged? I tried running postal upgrade to get the latest version and I got the same version I previously had which was 2.1.2 (which has this pressing issue).

[catphish]

Was this really merged? I tried running postal upgrade to get the latest version and I got the same version I previously had which was 2.1.2 (which has this pressing issue).

It was merged a couple of days ago. There hasn't yet been a release that contains it.

[Paz08]

Do you know when this fix will be released? It's pretty crucial to the work I'm doing atm.

[Paz08]

Any news on the release of this fix?

[catphish]

No. The next release is awaiting the completion of #2392 which I need to test and merge.
No ETA but we're getting there.