Creating mailer layouts by default, including html and body tags #17646
Conversation
| @@ -69,27 +79,41 @@ def test_check_preview_class_collision | |||
| def test_invokes_default_text_template_engine | |||
| run_generator | |||
| assert_file "app/views/notifier/foo.text.erb" do |view| | |||
| assert_match(%r(\sapp/views/notifier/foo\.text\.erb), view) | |||
There was a problem hiding this comment.
The files generated don't seem to have the filename in the content of the file, so I removed these from the tests. Let me know if this is wrong, but from calling puts view I couldn't see them in the content.
|
Link from the original PR justifying an engine that does penalise for this : http://wiki.apache.org/spamassassin/Rules/HTML_MIME_NO_HTML_TAG |
|
Found another mailer test that seems to require the name of the template within the view itself (which hasn't been there the whole time). |
|
I don't think we should have a per-mailer layout by default. I think we should just have one. We don't have a per-controller layout by default either. So |
|
I agree with you, that does make more sense. But it does make me think, shall we not go further...? Generate an Thoughts? |
|
Yeah, probably about time we did that. We have an ApplicationMailer in all our apps. |
|
I'd prefer for this ApplicationMailer to declare |
|
OK fair enough, I'll work on this hopefully at the weekend (if not early next week) and add the commit to the ticket. I assume I need to squash them before it gets merged, but I need to look in to how to do that with Github's PRs. |
|
@andyjeffries You can squash the commits locally and force push to the this branch again. Github will update the PR properly. |
|
👍
|
andyjeffries
force-pushed
the
html_layout_fix
branch
2 times, most recently
from
4f7b0c1
to
a353d79
Compare
|
Thanks @prathamesh-sonpatki , did that and it seems to have worked. @dhh is it as you'd like it now? Anything else I need to do before it can be merged? Thanks for the help on this so far. |
…d body tags to reduce spam score
andyjeffries
force-pushed
the
html_layout_fix
branch
from
e9f9a0e
to
38cfaa8
Compare
|
Looking good, but travis says the build is failing? |
|
Yeah, I don't know why. The failures are nothing to do with code I've touched. The builds that test the code I changed pass (the first 4), the failures in the other sections are all around ActiveJob. Are those tests generally stable or a bit flakey? |
| @@ -8,6 +8,7 @@ class MailerGenerator < NamedBase | |||
|
|
|||
| def create_mailer_file | |||
| template "mailer.rb", File.join('app/mailers', class_path, "#{file_name}.rb") | |||
| template "application_mailer.rb", File.join('app/mailers/application_mailer.rb') | |||
There was a problem hiding this comment.
Doesn't seem like File.join is required here. 😸
|
Good spot, thanks @tgxworld . |
|
Master is failing right now, you can ignore the AJ tests |
|
To answer your question, they're generally stable the issue came from a transitive dependency updating. |
Creating mailer layouts by default, including html and body tags
This should reduce the spam score in some spam detection engines (and is good practice like having an application.html.erb generated by default).
It uses the same name as the mailer if the mailer ends in ...Mailer (e.g. NotifierMailer will generate an
app/views/layouts/notifier_mailer.html.erblayout and no layout line in theapp/mailers/notifier_mailer.rbclass) but if the mailer doesn't end in ...Mailer, then the layout created is prefixed withmailer_(to avoid conflicts with other layouts) and alayout :...line is added to the mailer class.There are tests in there, but I don't use Test::Unit so let me know if you want them reworked/tidied up in some way.
The original pull request was #17606 but the author wanted me to take over adding the layout files, as @dhh requested that they not be dealt with directly in the mailer views.