Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Tidy up ActionMailer rendering logic to take advantage of view path c…

…ache instead of using file system lookups
  • Loading branch information...
commit b2504f8ba0f9baadb9298647fd58ef2c136f9aae 1 parent ed8a882
@josh josh authored
View
49 actionmailer/lib/action_mailer/base.rb
@@ -216,7 +216,7 @@ module ActionMailer #:nodoc:
# * <tt>:domain</tt> - If you need to specify a HELO domain, you can do it here.
# * <tt>:user_name</tt> - If your mail server requires authentication, set the username in this setting.
# * <tt>:password</tt> - If your mail server requires authentication, set the password in this setting.
- # * <tt>:authentication</tt> - If your mail server requires authentication, you need to specify the authentication type here.
+ # * <tt>:authentication</tt> - If your mail server requires authentication, you need to specify the authentication type here.
# This is a symbol and one of <tt>:plain</tt>, <tt>:login</tt>, <tt>:cram_md5</tt>.
#
# * <tt>sendmail_settings</tt> - Allows you to override options for the <tt>:sendmail</tt> delivery method.
@@ -233,10 +233,10 @@ module ActionMailer #:nodoc:
# * <tt>deliveries</tt> - Keeps an array of all the emails sent out through the Action Mailer with <tt>delivery_method :test</tt>. Most useful
# for unit and functional testing.
#
- # * <tt>default_charset</tt> - The default charset used for the body and to encode the subject. Defaults to UTF-8. You can also
+ # * <tt>default_charset</tt> - The default charset used for the body and to encode the subject. Defaults to UTF-8. You can also
# pick a different charset from inside a method with +charset+.
# * <tt>default_content_type</tt> - The default content type used for the main part of the message. Defaults to "text/plain". You
- # can also pick a different content type from inside a method with +content_type+.
+ # can also pick a different content type from inside a method with +content_type+.
# * <tt>default_mime_version</tt> - The default mime version used for the message. Defaults to <tt>1.0</tt>. You
# can also pick a different value from inside a method with +mime_version+.
# * <tt>default_implicit_parts_order</tt> - When a message is built implicitly (i.e. multiple parts are assembled from templates
@@ -253,9 +253,6 @@ class Base
class_inheritable_accessor :view_paths
cattr_accessor :logger
- cattr_accessor :template_extensions
- @@template_extensions = ['erb', 'builder', 'rhtml', 'rxml']
-
@@smtp_settings = {
:address => "localhost",
:port => 25,
@@ -414,15 +411,10 @@ def deliver(mail)
new.deliver!(mail)
end
- # Register a template extension so mailer templates written in a
- # templating language other than rhtml or rxml are supported.
- # To use this, include in your template-language plugin's init
- # code or on a per-application basis, this can be invoked from
- # <tt>config/environment.rb</tt>:
- #
- # ActionMailer::Base.register_template_extension('haml')
def register_template_extension(extension)
- template_extensions << extension
+ ActiveSupport::Deprecation.warn(
+ "ActionMailer::Base.register_template_extension has been deprecated." +
+ "Use ActionView::Base.register_template_extension instead", caller)
end
def template_root
@@ -455,16 +447,18 @@ def create!(method_name, *parameters) #:nodoc:
# "the_template_file.text.html.erb", etc.). Only do this if parts
# have not already been specified manually.
if @parts.empty?
- templates = Dir.glob("#{template_path}/#{@template}.*")
- templates.each do |path|
- basename = File.basename(path)
- template_regex = Regexp.new("^([^\\\.]+)\\\.([^\\\.]+\\\.[^\\\.]+)\\\.(" + template_extensions.join('|') + ")$")
- next unless md = template_regex.match(basename)
- template_name = basename
- content_type = md.captures[1].gsub('.', '/')
- @parts << Part.new(:content_type => content_type,
- :disposition => "inline", :charset => charset,
- :body => render_message(template_name, @body))
+ Dir.glob("#{template_path}/#{@template}.*").each do |path|

I haven’t written a test against it yet but this piece of code breaks most emails. If it finds a template but the mail is not multipart it never calls render_message. So all my mail is showing up with the hash passed to TMail#body as the body of the message.

I recommend checking this against an app – I’ll try to find time to come up with some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ template = template_root["#{mailer_name}/#{File.basename(path)}"]
+
+ # Skip unless template has a multipart format
+ next unless template.multipart?
+
+ @parts << Part.new(
+ :content_type => template.content_type,
+ :disposition => "inline",
+ :charset => charset,
+ :body => render_message(template, @body)
+ )
end
unless @parts.empty?
@content_type = "multipart/alternative"
@@ -476,9 +470,8 @@ def create!(method_name, *parameters) #:nodoc:
# also render a "normal" template (without the content type). If a
# normal template exists (or if there were no implicit parts) we render
# it.
- template_exists = @parts.empty?
- template_exists ||= Dir.glob("#{template_path}/#{@template}.*").any? { |i| File.basename(i).split(".").length == 2 }
- @body = render_message(@template, @body) if template_exists
+ template = template_root["#{mailer_name}/#{@template}"]
+ @body = render_message(@template, @body) if template
# Finally, if there are other message parts and a textual body exists,
# we shift it onto the front of the parts and set the body to nil (so
@@ -538,7 +531,7 @@ def render_message(method_name, body)
def render(opts)
body = opts.delete(:body)
- if opts[:file] && opts[:file] !~ /\//
+ if opts[:file] && (opts[:file] !~ /\// && !opts[:file].respond_to?(:render))
opts[:file] = "#{mailer_name}/#{opts[:file]}"
end
initialize_template_class(body).render(opts)
View
31 actionmailer/test/mail_service_test.rb
@@ -219,7 +219,7 @@ def nested_multipart(recipient)
end
attachment :content_type => "application/octet-stream",:filename => "test.txt", :body => "test abcdefghijklmnopqstuvwxyz"
end
-
+
def nested_multipart_with_body(recipient)
recipients recipient
subject "nested multipart with body"
@@ -321,7 +321,7 @@ def test_nested_parts
assert_nothing_raised { created = TestMailer.create_nested_multipart(@recipient)}
assert_equal 2,created.parts.size
assert_equal 2,created.parts.first.parts.size
-
+
assert_equal "multipart/mixed", created.content_type
assert_equal "multipart/alternative", created.parts.first.content_type
assert_equal "bar", created.parts.first.header['foo'].to_s
@@ -366,7 +366,7 @@ def test_signed_up
assert_not_nil ActionMailer::Base.deliveries.first
assert_equal expected.encoded, ActionMailer::Base.deliveries.first.encoded
end
-
+
def test_custom_template
expected = new_mail
expected.to = @recipient
@@ -382,7 +382,6 @@ def test_custom_template
end
def test_custom_templating_extension
- #
# N.b., custom_templating_extension.text.plain.haml is expected to be in fixtures/test_mailer directory
expected = new_mail
expected.to = @recipient
@@ -390,18 +389,10 @@ def test_custom_templating_extension
expected.body = "Hello there, \n\nMr. #{@recipient}"
expected.from = "system@loudthinking.com"
expected.date = Time.local(2004, 12, 12)
-
+
# Stub the render method so no alternative renderers need be present.
ActionView::Base.any_instance.stubs(:render).returns("Hello there, \n\nMr. #{@recipient}")
-
- # If the template is not registered, there should be no parts.
- created = nil
- assert_nothing_raised { created = TestMailer.create_custom_templating_extension(@recipient) }
- assert_not_nil created
- assert_equal 0, created.parts.length
-
- ActionMailer::Base.register_template_extension('haml')
-
+
# Now that the template is registered, there should be one part. The text/plain part.
created = nil
assert_nothing_raised { created = TestMailer.create_custom_templating_extension(@recipient) }
@@ -428,7 +419,7 @@ def test_cancelled_account
assert_not_nil ActionMailer::Base.deliveries.first
assert_equal expected.encoded, ActionMailer::Base.deliveries.first.encoded
end
-
+
def test_cc_bcc
expected = new_mail
expected.to = @recipient
@@ -550,7 +541,7 @@ def test_perform_deliveries_flag
TestMailer.deliver_signed_up(@recipient)
assert_equal 1, ActionMailer::Base.deliveries.size
end
-
+
def test_doesnt_raise_errors_when_raise_delivery_errors_is_false
ActionMailer::Base.raise_delivery_errors = false
TestMailer.any_instance.expects(:perform_delivery_test).raises(Exception)
@@ -670,7 +661,7 @@ def test_extended_headers
assert_not_nil ActionMailer::Base.deliveries.first
assert_equal expected.encoded, ActionMailer::Base.deliveries.first.encoded
end
-
+
def test_utf8_body_is_not_quoted
@recipient = "Foo áëô îü <extended@example.net>"
expected = new_mail "utf-8"
@@ -760,7 +751,7 @@ def test_multipart_with_mime_version
mail = TestMailer.create_multipart_with_mime_version(@recipient)
assert_equal "1.1", mail.mime_version
end
-
+
def test_multipart_with_utf8_subject
mail = TestMailer.create_multipart_with_utf8_subject(@recipient)
assert_match(/\nSubject: =\?utf-8\?Q\?Foo_.*?\?=/, mail.encoded)
@@ -825,7 +816,7 @@ def test_implicitly_multipart_messages_with_charset
mail = TestMailer.create_implicitly_multipart_example(@recipient, 'iso-8859-1')
assert_equal "multipart/alternative", mail.header['content-type'].body
-
+
assert_equal 'iso-8859-1', mail.parts[0].sub_header("content-type", "charset")
assert_equal 'iso-8859-1', mail.parts[1].sub_header("content-type", "charset")
assert_equal 'iso-8859-1', mail.parts[2].sub_header("content-type", "charset")
@@ -852,7 +843,7 @@ def test_various_newlines_multipart
assert_equal "line #1\nline #2\nline #3\nline #4\n\n", mail.parts[0].body
assert_equal "<p>line #1</p>\n<p>line #2</p>\n<p>line #3</p>\n<p>line #4</p>\n\n", mail.parts[1].body
end
-
+
def test_headers_removed_on_smtp_delivery
ActionMailer::Base.delivery_method = :smtp
TestMailer.deliver_cc_bcc(@recipient)
View
5 actionpack/lib/action_view/base.rb
@@ -300,6 +300,8 @@ def file_exists?(template_path)
# # => 'users/legacy.rhtml'
#
def pick_template(template_path)
+ return template_path if template_path.respond_to?(:render)
+
path = template_path.sub(/^\//, '')
if m = path.match(/(.*)\.(\w+)$/)
template_file_name, template_file_extension = m[1], m[2]
@@ -343,7 +345,8 @@ def render_file(template_path, use_full_path = nil, local_assigns = {}) #:nodoc:
ActiveSupport::Deprecation.warn("use_full_path option has been deprecated and has no affect.", caller)
end
- if defined?(ActionMailer) && defined?(ActionMailer::Base) && controller.is_a?(ActionMailer::Base) && !template_path.include?("/")
+ if defined?(ActionMailer) && defined?(ActionMailer::Base) && controller.is_a?(ActionMailer::Base) &&
+ template_path.is_a?(String) && !template_path.include?("/")
raise ActionViewError, <<-END_ERROR
Due to changes in ActionMailer, you need to provide the mailer_name along with the template name.
View
10 actionpack/lib/action_view/template.rb
@@ -22,6 +22,14 @@ def format_and_extension
end
memoize :format_and_extension
+ def multipart?
+ format && format.include?('.')
+ end
+
+ def content_type
+ format.gsub('.', '/')
+ end
+
def mime_type
Mime::Type.lookup_by_extension(format) if format
end
@@ -84,7 +92,7 @@ def find_full_path(path, load_paths)
# [base_path, name, format, extension]
def split(file)
if m = file.match(/^(.*\/)?([^\.]+)\.?(\w+)?\.?(\w+)?\.?(\w+)?$/)
- if m[5] # Mulipart formats
+ if m[5] # Multipart formats
[m[1], m[2], "#{m[3]}.#{m[4]}", m[5]]
elsif m[4] # Single format
[m[1], m[2], m[3], m[4]]
View
33 actionpack/test/controller/assert_select_test.rb
@@ -17,6 +17,8 @@
end
end
+ActionMailer::Base.template_root = FIXTURE_LOAD_PATH
+
class AssertSelectTest < Test::Unit::TestCase
class AssertSelectController < ActionController::Base
def response_with=(content)
@@ -69,11 +71,10 @@ def setup
ActionMailer::Base.deliveries = []
end
-
def teardown
ActionMailer::Base.deliveries.clear
end
-
+
def assert_failure(message, &block)
e = assert_raises(AssertionFailedError, &block)
assert_match(message, e.message) if Regexp === message
@@ -91,7 +92,6 @@ def test_assert_select
assert_failure(/Expected at least 1 element matching \"p\", found 0/) { assert_select "p" }
end
-
def test_equality_true_false
render_html %Q{<div id="1"></div><div id="2"></div>}
assert_nothing_raised { assert_select "div" }
@@ -102,7 +102,6 @@ def test_equality_true_false
assert_nothing_raised { assert_select "p", false }
end
-
def test_equality_string_and_regexp
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
assert_nothing_raised { assert_select "div", "foo" }
@@ -116,7 +115,6 @@ def test_equality_string_and_regexp
assert_raises(AssertionFailedError) { assert_select "p", :text=>/foobar/ }
end
-
def test_equality_of_html
render_html %Q{<p>\n<em>"This is <strong>not</strong> a big problem,"</em> he said.\n</p>}
text = "\"This is not a big problem,\" he said."
@@ -135,7 +133,6 @@ def test_equality_of_html
assert_raises(AssertionFailedError) { assert_select "pre", :html=>text }
end
-
def test_counts
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
assert_nothing_raised { assert_select "div", 2 }
@@ -166,7 +163,6 @@ def test_counts
end
end
-
def test_substitution_values
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
assert_select "div#?", /\d+/ do |elements|
@@ -181,7 +177,6 @@ def test_substitution_values
end
end
-
def test_nested_assert_select
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
assert_select "div" do |elements|
@@ -200,7 +195,7 @@ def test_nested_assert_select
assert_select "#3", false
end
end
-
+
assert_failure(/Expected at least 1 element matching \"#4\", found 0\./) do
assert_select "div" do
assert_select "#4"
@@ -208,7 +203,6 @@ def test_nested_assert_select
end
end
-
def test_assert_select_text_match
render_html %Q{<div id="1"><span>foo</span></div><div id="2"><span>bar</span></div>}
assert_select "div" do
@@ -225,7 +219,6 @@ def test_assert_select_text_match
end
end
-
# With single result.
def test_assert_select_from_rjs_with_single_result
render_rjs do |page|
@@ -255,19 +248,16 @@ def test_assert_select_from_rjs_with_multiple_results
end
end
-
#
# Test css_select.
#
-
def test_css_select
render_html %Q{<div id="1"></div><div id="2"></div>}
assert 2, css_select("div").size
assert 0, css_select("p").size
end
-
def test_nested_css_select
render_html %Q{<div id="1">foo</div><div id="2">foo</div>}
assert_select "div#?", /\d+/ do |elements|
@@ -286,7 +276,6 @@ def test_nested_css_select
end
end
-
# With one result.
def test_css_select_from_rjs_with_single_result
render_rjs do |page|
@@ -309,12 +298,10 @@ def test_css_select_from_rjs_with_multiple_results
assert_equal 1, css_select("#2").size
end
-
#
# Test assert_select_rjs.
#
-
# Test that we can pick up all statements in the result.
def test_assert_select_rjs_picks_up_all_statements
render_rjs do |page|
@@ -381,7 +368,6 @@ def test_assert_select_rjs_with_id
assert_raises(AssertionFailedError) { assert_select_rjs "test4" }
end
-
def test_assert_select_rjs_for_replace
render_rjs do |page|
page.replace "test1", "<div id=\"1\">foo</div>"
@@ -479,7 +465,7 @@ def test_assert_select_rjs_for_show_ignores_block
end
end
end
-
+
# Simple hide
def test_assert_select_rjs_for_hide
render_rjs do |page|
@@ -500,7 +486,7 @@ def test_assert_select_rjs_for_hide_ignores_block
end
end
end
-
+
# Simple toggle
def test_assert_select_rjs_for_toggle
render_rjs do |page|
@@ -521,7 +507,7 @@ def test_assert_select_rjs_for_toggle_ignores_block
end
end
end
-
+
# Non-positioned insert.
def test_assert_select_rjs_for_nonpositioned_insert
render_rjs do |page|
@@ -568,7 +554,7 @@ def test_assert_select_rjs_for_positioned_insert
assert_select "div", 4
end
end
-
+
# Simple selection from a single result.
def test_nested_assert_select_rjs_with_single_result
render_rjs do |page|
@@ -600,7 +586,6 @@ def test_nested_assert_select_rjs_with_two_results
end
end
-
def test_feed_item_encoded
render_xml <<-EOF
<rss version="2.0">
@@ -654,7 +639,6 @@ def test_feed_item_encoded
end
end
-
#
# Test assert_select_email
#
@@ -670,7 +654,6 @@ def test_assert_select_email
end
end
-
protected
def render_html(html)
@controller.response_with = html

13 comments on commit b2504f8

@josh
Collaborator

If you ticket me on LH with a failing unit test, I’ll get it fixed for you. Sorry.

@harrylove

Josh, any idea when this will be fixed?

@harrylove

Josh, any idea when this will be fixed?

@leword

I found a workaround, or maybe the way things are supposed to work: for the template, instead of …/mail.html.erb do …/mail.erb that seems to get things rendering again for me.

@harrylove

Yep, that worked for me as well. Thanks, leword.

@josh
Collaborator

This is a serious bug. I still haven’t got any unit tests, but I’m finally going get around to looking into it tonight ;)

@josh
Collaborator

Thanks guys. Fixed in http://github.com/rails/rails/commit/04248c62086b188ae354ed90ae40c832b79fd19c

@josh
Collaborator

Thanks guys. Fixed in http://github.com/rails/rails/commit/04248c62086b188ae354ed90ae40c832b79fd19c

@defunkt

Just using a sha should auto-link it. Test: 04248c62086b188ae354ed90ae40c832b79fd19c

@JackDanger

You’re great Josh. Thank you :-)

@lifo
Collaborator

@defunkt fai!

@lifo
Collaborator

@defunkt fai!

@lifo
Collaborator

@defunkt fai!

Please sign in to comment.
Something went wrong with that request. Please try again.