Explicit multipart messages respect :parts_order #8044

Merged
merged 1 commit into from Nov 19, 2012

Conversation

Projects
None yet
4 participants
Contributor

nateberkopec commented Oct 27, 2012

My original issue was #7978. Our app's emails were showing up in Gmail as plain text only, and I couldn't figure out why. I checked the raw text being sent, and confirmed both HTML and plaintext parts were in there.

However, what I wasn't aware of was this little line in RFC 2046:

In general, user agents that compose "multipart/alternative" entities
must place the body parts in increasing order of preference, that is,
with the preferred format last. [..] Receiving user agents should pick and display the last format
they are capable of displaying.

Aha! So I checked the order of my mime parts, and, lo and behold, the plaintext part was being sent last! So Gmail was following the spec correctly - it was displaying the last format it was capable of displaying. (Side note: other mail clients seem to display HTML regardless).

Now, what the heck was actually setting this order? I read through the ActionMailer documentation and discovered the [:parts_order] option. Great! All I have to do is set this and it'll work fine. Nope. After about a day worth of digging and frustration, I discovered that passing a block to mail sets the parts order to the same order as the block!

This pull request changes mail to respect :parts_order regardless of the order of any block passed into mail. I decided this change, rather than just clarifying the documentation, was appropriate for several reasons:

  • It wasn't clear what should happen when :parts_order and the order of the block conflicted. Which should take precedence?
  • Setting the mime part order in two places seemed unnecessarily confusing.
  • When an email is multipart, I can think of few reasons why html would not be the preferred format. It seems too easy to accidentally send an email you think will show up as HTML but actually will be plaintext (this is what happened to me). :parts_order already sets a sensible default (html last), so let's make it difficult to unintentionally mess up this setting.
  • In ActionController, the convention when writing an explicit block is put format.html first in the order of possible formats. This is how most examples/documentation is written. In ActionMailer, in order for HTML to actually display in an end user's mail client, you have to do the opposite. Flipping this convention in ActionMailer seems unneccessary.

Thanks for considering this change. I lost about a day or two figuring this one out, just want to prevent anyone from having the same issue.

actionmailer/CHANGELOG.md
@@ -31,4 +31,7 @@
settings, headers, attachments, delivery settings or change delivery using
`before_filter`, `after_filter` etc. *Justin S. Leitgeb*
+* Explicit multipart messages no longer set the order of the MIME parts.
+ *Nate Berkopec*
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Owner

Please move this to the top of the changelog file.

actionmailer/lib/action_mailer/base.rb
if block_given?
collector = ActionMailer::Collector.new(lookup_context) { render(action_name) }
yield(collector)
- parts_order = collector.responses.map { |r| r[:content_type] }
responses = collector.responses
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2012

Owner

Can you take the opportunity and realign this line?

It kinda makes sense that the parts_order option take precedence, if it's being set, it's because you know the order you want to generate it.

/cc @josevalim @rafaelfranca

@@ -377,23 +377,7 @@ If you use this setting, you should pass the `only_path: false` option when usin
Action Mailer will automatically send multipart emails if you have different templates for the same action. So, for our UserMailer example, if you have `welcome_email.text.erb` and `welcome_email.html.erb` in `app/views/user_mailer`, Action Mailer will automatically send a multipart email with the HTML and text versions setup as different parts.
-The order of the parts getting inserted is determined by the `:parts_order` inside of the `ActionMailer::Base.default` method. If you want to explicitly alter the order, you can either change the `:parts_order` or explicitly render the parts in a different order:
@rafaelfranca

rafaelfranca Oct 28, 2012

Owner

I didn't get why you removed the "explicitly render the parts in a different order" it still works if you don't set the :parts_order option

@nateberkopec

nateberkopec Oct 29, 2012

Contributor

Hm, I guess I took it out because of the context of "explicitly render" in the test suite. The tests which deal with passing a block to mail are called "explicit" and those where mail is not passed a block are called "implicit". So the documentation as written, when it says "explicitly render", is referring to the block syntax. And, of course, I've removed the ability to set the mimepart ordering when "explicitly rendering".

But... I just realized I'm not sure what you mean by "it still works" - what exactly still works?

@rafaelfranca

rafaelfranca Oct 29, 2012

Owner

The ability of set the mimepart ordering when "explicitly rendering". But I just realized that what is guaranteeing the order is the default_params. Sorry

actionmailer/lib/action_mailer/base.rb
@@ -697,7 +697,7 @@ def mail(headers={}, &block)
m.charset = charset
if m.multipart?
- parts_order ||= explicit_order || headers[:parts_order]
+ parts_order ||= headers[:parts_order]
@rafaelfranca

rafaelfranca Oct 29, 2012

Owner

Do we need the line 699? If we don't you can remove this ||=

Member

steveklabnik commented Nov 18, 2012

This will need a rebase.

Contributor

nateberkopec commented Nov 19, 2012

Cool - followed the guide you added: https://github.com/rails/rails/pull/8152/files

Think I got it, let me know if I didn't. First time contributing to open source.

actionmailer/CHANGELOG.md
* Do not render views when mail() isn't called.
Fix #7761
*Yves Senn*
+
+* Support `Mailer.deliver_foo(*args)` as a synonym for
@rafaelfranca

rafaelfranca Nov 19, 2012

Owner

This CHANGELOG entry was removed in master. Please remove it from your commit too.

A fast way to do this is:

  1. removing it from this file.
  2. git add actionmailer/CHANGELOG.md
  3. git commit --amend -C HEAD
  4. git push -f
Member

steveklabnik commented Nov 19, 2012

First time contributing to open source.

Awesome. :D

Explicit multipart messages respect :parts_order
As issue #7978, the order in which ActionMailer
sends multipart messages could be unintentionally
overwritten if a block is passed to the mail
method. This changes the mail method such that
:parts_order is always respected, regardless of
whether a block is passed to mail.
Contributor

nateberkopec commented Nov 19, 2012

@rafaelfranca fixed, thanks.

rafaelfranca added a commit that referenced this pull request Nov 19, 2012

Merge pull request #8044 from nateberkopec/block_does_not_set_parts_o…
…rder

Explicit multipart messages respect :parts_order

@rafaelfranca rafaelfranca merged commit 60790e8 into rails:master Nov 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment