Get implicit multipart and attachments working together.

4 files changed, 109 insertions, 40 deletions

diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb
index fb1dab7c39..98e559154a 100644
--- a/actionmailer/lib/action_mailer/base.rb
+++ b/actionmailer/lib/action_mailer/base.rb
@@ -390,6 +390,7 @@ module ActionMailer #:nodoc:
       end
     end
 
+    # TODO Add new delivery method goodness
     def mail(headers = {})
       # Guard flag to prevent both the old and the new API from firing
       # Should be removed when old API is deprecated
@@ -402,9 +403,8 @@ module ActionMailer #:nodoc:
 
       # Give preference to headers and fallbacks to the ones set in mail
       content_type = headers[:content_type] || m.content_type
-      charset      = headers[:charset]      || m.charset
-      mime_version = headers[:mime_version] || m.mime_version
-      body = nil
+      charset      = headers[:charset]      || m.charset      || self.class.default_charset.dup
+      mime_version = headers[:mime_version] || m.mime_version || self.class.default_mime_version.dup
 
       m.subject   ||= quote_if_necessary(headers[:subject], charset)          if headers[:subject]
       m.to        ||= quote_address_if_necessary(headers[:to], charset)       if headers[:to]
@@ -420,35 +420,41 @@ module ActionMailer #:nodoc:
         # TODO Ensure that we don't need to pass I18n.locale as detail
         templates = self.class.template_root.find_all(action_name, {}, mailer_name)
 
-        if templates.size == 1
+        if templates.size == 1 && !m.has_attachments?
           content_type ||= templates[0].mime_type.to_s
           m.body = render_to_body(:_template => templates[0])
+        elsif templates.size > 1 && m.has_attachments? 
+          container = Mail::Part.new
+          container.content_type = "multipart/alternate"
+          templates.each { |t| insert_part(container, t, charset) }
+          m.add_part(container)
         else
-          content_type ||= "multipart/alternate"
-
-          templates.each do |template|
-            part = Mail::Part.new
-            part.content_type = template.mime_type.to_s
-            part.charset = charset
-            part.body = render_to_body(:_template => template)
-            m.add_part(part)
-          end
+          templates.each { |t| insert_part(m, t, charset) }
         end
+
+        content_type ||= (m.has_attachments? ? "multipart/mixed" : "multipart/alternate")
       end
-                       
+
+      # Check if the content_type was not overwriten along the way and if so,
+      # fallback to default.
       m.content_type = content_type || self.class.default_content_type.dup
-      m.charset      = charset      || self.class.default_charset.dup
-      m.mime_version = mime_version || self.class.default_mime_version.dup
-                       
-      # TODO Add me and test me
-      # m.body.set_sort_order(headers[:parts_order] || self.class.default_implicit_parts_order.dup)
-      # m.body.sort_parts!
+      m.charset      = charset
+      m.mime_version = mime_version
+
+      unless m.parts.empty?
+        m.body.set_sort_order(headers[:parts_order] || self.class.default_implicit_parts_order.dup)
+        m.body.sort_parts!
+      end
 
       m
     end
 
-    def fill_in_part(part, template, charset)
-      
+    def insert_part(container, template, charset)
+      part = Mail::Part.new
+      part.content_type = template.mime_type.to_s
+      part.charset = charset
+      part.body = render_to_body(:_template => template)
+      container.add_part(part)
     end
 
     # Instantiate a new mailer object. If +method_name+ is not +nil+, the mailer
diff --git a/actionmailer/lib/action_mailer/delivery_methods.rb b/actionmailer/lib/action_mailer/delivery_methods.rb
index c8c4148353..5883e446f2 100644
--- a/actionmailer/lib/action_mailer/delivery_methods.rb
+++ b/actionmailer/lib/action_mailer/delivery_methods.rb
@@ -27,6 +27,7 @@ module ActionMailer
     end
 
     module ClassMethods
+      # TODO Make me class inheritable
       def delivery_settings
         @@delivery_settings ||= Hash.new { |h,k| h[k] = {} }
       end
@@ -51,6 +52,7 @@ module ActionMailer
 
     protected
 
+      # TODO Get rid of this method missing magic
       def method_missing(method_symbol, *parameters) #:nodoc:
         if match = matches_settings_method?(method_symbol)
           if match[2]
diff --git a/actionmailer/test/base_test.rb b/actionmailer/test/base_test.rb
index 8502b29ba4..df0eed695d 100644
--- a/actionmailer/test/base_test.rb
+++ b/actionmailer/test/base_test.rb
@@ -3,13 +3,13 @@ require 'abstract_unit'
 
 #  class Notifier < ActionMailer::Base
 #    delivers_from 'notifications@example.com'
-#    
+#
 #    def welcome(user)
 #      @user = user # available to the view
 #      mail(:subject => 'Welcome!', :to => user.email_address)
 #      # auto renders both welcome.text.erb and welcome.html.erb
 #    end
-#    
+#
 #    def goodbye(user)
 #      headers["X-SPAM"] = 'Not-SPAM'
 #      mail(:subject => 'Goodbye', :to => user.email_address) do |format|
@@ -17,7 +17,7 @@ require 'abstract_unit'
 #        format.text # goodbye.text.erb
 #      end
 #    end
-#    
+#
 #    def surprise(user, gift)
 #      attachments[gift.name] = File.read(gift.path)
 #      mail(:subject => 'Surprise!', :to => user.email_address) do |format|
@@ -25,17 +25,17 @@ require 'abstract_unit'
 #        format.text(:transfer_encoding => "base64") # surprise.text.erb
 #      end
 #    end
-#    
+#
 #    def special_surprise(user, gift)
 #      attachments[gift.name] = { :content_type => "application/x-gzip", :content => File.read(gift.path) }
 #      mail(:to => 'special@example.com') # subject not required
 #      # auto renders both special_surprise.text.erb and special_surprise.html.erb
 #    end
 #  end
-#   
+#
 #  mail = Notifier.welcome(user)         # => returns a Mail object
 #  mail.deliver
-# 
+#
 #  Notifier.welcome(user).deliver # => creates and sends the Mail in one step
 class BaseTest < ActiveSupport::TestCase
   DEFAULT_HEADERS = {
@@ -44,6 +44,8 @@ class BaseTest < ActiveSupport::TestCase
     :subject => 'The first email on new API!'
   }
 
+  # TODO Think on the simple case where I want to send an e-mail
+  # with attachment and small text (without need to add a template).
   class BaseMailer < ActionMailer::Base
     self.mailer_name = "base_mailer"
 
@@ -63,8 +65,9 @@ class BaseTest < ActiveSupport::TestCase
       mail(DEFAULT_HEADERS)
     end
 
-    def implicit_multipart
-      mail(DEFAULT_HEADERS)
+    def implicit_multipart(hash = {})
+      attachments['invoice.pdf'] = 'This is test File content' if hash.delete(:attachments)
+      mail(DEFAULT_HEADERS.merge(hash))
     end
   end
 
@@ -72,6 +75,7 @@ class BaseTest < ActiveSupport::TestCase
     assert_nothing_raised { BaseMailer.deliver_welcome }
   end
 
+  # Basic mail usage without block
   test "mail() should set the headers of the mail message" do
     email = BaseMailer.deliver_welcome
     assert_equal(email.to,      ['mikel@test.lindsaar.net'])
@@ -102,11 +106,13 @@ class BaseTest < ActiveSupport::TestCase
     assert_equal("Welcome", email.body.encoded)
   end
 
+  # Custom headers
   test "custom headers" do
     email = BaseMailer.deliver_welcome
     assert_equal("Not SPAM", email['X-SPAM'].decoded)
   end
 
+  # Attachments
   test "attachment with content" do
     email = BaseMailer.deliver_attachment_with_content
     assert_equal(1, email.attachments.length)
@@ -126,12 +132,22 @@ class BaseTest < ActiveSupport::TestCase
     assert_equal("\312\213\254\232)b", email.attachments['invoice.jpg'].decoded)
   end
 
-  # test "mail sets proper content type when attachment is included" do
-  #   email = BaseMailer.deliver_attachment_with_content
-  #   assert_equal(1, email.attachments.length)
-  #   assert_equal("multipart/mixed", email.content_type)
-  # end
+  test "sets mime type to multipart/mixed when attachment is included" do
+    email = BaseMailer.deliver_attachment_with_content
+    assert_equal(1, email.attachments.length)
+    assert_equal("multipart/mixed", email.mime_type)
+  end
 
+  test "adds the rendered template as part" do
+    email = BaseMailer.deliver_attachment_with_content
+    assert_equal(2, email.parts.length)
+    assert_equal("text/html", email.parts[0].mime_type)
+    assert_equal("Attachment with content", email.parts[0].body.encoded)
+    assert_equal("application/pdf", email.parts[1].mime_type)
+    assert_equal("VGhpcyBpcyB0ZXN0IEZpbGUgY29udGVudA==\r\n", email.parts[1].body.encoded)
+  end
+
+  # Defaults values
   test "uses default charset from class" do
     swap BaseMailer, :default_charset => "US-ASCII" do
       email = BaseMailer.deliver_welcome
@@ -142,6 +158,16 @@ class BaseTest < ActiveSupport::TestCase
     end
   end
 
+  test "uses default content type from class" do
+    swap BaseMailer, :default_content_type => "text/html" do
+      email = BaseMailer.deliver_welcome
+      assert_equal("text/html", email.mime_type)
+
+      email = BaseMailer.deliver_welcome(:content_type => "text/plain")
+      assert_equal("text/plain", email.mime_type)
+    end
+  end
+
   test "uses default mime version from class" do
     swap BaseMailer, :default_mime_version => "2.0" do
       email = BaseMailer.deliver_welcome
@@ -161,18 +187,52 @@ class BaseTest < ActiveSupport::TestCase
     assert_equal "New Subject!", email.subject
   end
 
+  # Implicit multipart
   test "implicit multipart tests" do
-    require 'ruby-debug'
-    $BREAK = true
     email = BaseMailer.deliver_implicit_multipart
-
     assert_equal(2, email.parts.size)
-
     assert_equal("multipart/alternate", email.mime_type)
     assert_equal("text/plain", email.parts[0].mime_type)
-    assert_equal("text/html",  email.parts[1].mime_type)    
+    assert_equal("TEXT Implicit Multipart", email.parts[0].body.encoded)
+    assert_equal("text/html", email.parts[1].mime_type)
+    assert_equal("HTML Implicit Multipart", email.parts[1].body.encoded)
+  end
+
+  test "implicit multipart tests with sort order" do
+    order = ["text/html", "text/plain"]
+    swap BaseMailer, :default_implicit_parts_order => order do
+      email = BaseMailer.deliver_implicit_multipart
+      assert_equal("text/html",  email.parts[0].mime_type)
+      assert_equal("text/plain", email.parts[1].mime_type)
+
+      email = BaseMailer.deliver_implicit_multipart(:parts_order => order.reverse)
+      assert_equal("text/plain", email.parts[0].mime_type)
+      assert_equal("text/html",  email.parts[1].mime_type)
+    end
+  end
+
+  test "implicit multipart with attachments creates nested parts" do
+    email = BaseMailer.deliver_implicit_multipart(:attachments => true)
+    assert_equal("application/pdf", email.parts[0].mime_type)
+    assert_equal("multipart/alternate", email.parts[1].mime_type)
+    assert_equal("text/plain", email.parts[1].parts[0].mime_type)
+    assert_equal("TEXT Implicit Multipart", email.parts[1].parts[0].body.encoded)
+    assert_equal("text/html", email.parts[1].parts[1].mime_type)
+    assert_equal("HTML Implicit Multipart", email.parts[1].parts[1].body.encoded)
   end
 
+  # TODO This should be fixed in mail. Sort parts should be recursive.
+  # test "implicit multipart with attachments and sort order" do
+  #   order = ["text/html", "text/plain"]
+  #   swap BaseMailer, :default_implicit_parts_order => order do
+  #     email = BaseMailer.deliver_implicit_multipart(:attachments => true)
+  #     assert_equal("application/pdf", email.parts[0].mime_type)
+  #     assert_equal("multipart/alternate", email.parts[1].mime_type)
+  #     assert_equal("text/plain", email.parts[1].parts[1].mime_type)
+  #     assert_equal("text/html", email.parts[1].parts[0].mime_type)
+  #   end
+  # end
+
   protected
 
     # Execute the block setting the given values and restoring old values after
diff --git a/actionmailer/test/fixtures/base_mailer/attachment_with_content.erb b/actionmailer/test/fixtures/base_mailer/attachment_with_content.erb
new file mode 100644
index 0000000000..deb9dbd03b
--- /dev/null
+++ b/actionmailer/test/fixtures/base_mailer/attachment_with_content.erb
@@ -0,0 +1 @@
+Attachment with content
\ No newline at end of file