search

billhorsman / inline_styles_mailer

Inline Styles Mailer Gem
MIT License
14 stars 11 forks source link

Account for namespaced mailers #8

Closed licatajustin closed 8 years ago

licatajustin commented 8 years ago

We ran into a small issue running this with a mounted engine in our Rails app. The template_path wasn't accounting for the namespace.

template = /path/to/app/views/namespace/user_mailer/mailer_method.html.erb
template_path = template.inspect.split("/").slice(-2, 2).join("/")
# => user_mailer/mailer_method.html.erb

My forked version patches this by splitting the template at the "views" string, then keeps the rest of the path.

template = /path/to/app/views/namespace/user_mailer/mailer_method.html.erb
template_path = template.inspect.split("views")[1][1..-1]
# => namespace/user_mailer/mailer_method.html.erb

I will submit a Pull Request with the patch, please let me know if anything else is needed. I also noticed that all the tests were failing locally for me (prior to the patch). Is there something else needed to get them to pass...I see they passed their latest build on Travis.

Thanks for the gem! This gem really brightens up the sad state of HTML emails!

billhorsman commented 8 years ago

A patch would be wonderful, thanks!

What errors/failures are you getting from your tests?

licatajustin commented 8 years ago

I get the following error for all 6 tests.

InlineStylesMailer Switch parts order text/plain first (default) should inline the CSS
     Failure/Error: case _layout

     ArgumentError:
       wrong number of arguments (0 for 1)
     # ./lib/inline_styles_mailer.rb:96:in `layout_to_use'
     # ./lib/inline_styles_mailer.rb:84:in `block (3 levels) in mail'
     # ./lib/inline_styles_mailer.rb:81:in `block (2 levels) in mail'
     # ./lib/inline_styles_mailer.rb:72:in `each'
     # ./lib/inline_styles_mailer.rb:72:in `block in mail'
     # ./lib/inline_styles_mailer.rb:60:in `mail'
     # ./spec/foo_mailer.rb:7:in `foo'

If I comment out that method, then 5/6 tests pass. The failing test is

1) InlineStylesMailer Inlining CSS Default CSS file should inline the CSS
     Failure/Error: mail.body.parts[1].body.should =~ /<p style="color: red;">Testing foo text\/html\.<\/p>/

       expected: /<p style="color: red;">Testing foo text\/html\.<\/p>/
            got: #<Mail::Body:0x007fcfd6c73460 @boundary=nil, @preamble=nil, @epilogue=nil, @charset="US-ASCII", @part_sort_order=["text/plain", "text/enriched", "text/html"], @parts=[], @raw_source="<!DOCTYPE html PUBLIC \"-//W3C//DTD HTML 4.0 Transitional//EN\" \"http://www.w3.org/TR/REC-html40/loose.dtd\">\n<html><body style=\"background: yellow;\"><p style=\"color: blue;\">Testing foo text/html.</p></body></html>\n", @encoding="7bit"> (using =~)
       Diff:
       @@ -1,2 +1,11 @@
       -/<p style="color: red;">Testing foo text\/html\.<\/p>/
       +#<Mail::Body:0x007fcfd6c73460
       + @boundary=nil,
       + @charset="US-ASCII",
       + @encoding="7bit",
       + @epilogue=nil,
       + @part_sort_order=["text/plain", "text/enriched", "text/html"],
       + @parts=[],
       + @preamble=nil,
       + @raw_source=
       +  "<!DOCTYPE html PUBLIC \"-//W3C//DTD HTML 4.0 Transitional//EN\" \"http://www.w3.org/TR/REC-html40/loose.dtd\">\n<html><body style=\"background: yellow;\"><p style=\"color: blue;\">Testing foo text/html.</p></body></html>\n">

     # ./spec/inline_styles_mailer/inline_styles_mailer_spec.rb:20:in `block (4 levels) in <top (required)>'
billhorsman commented 8 years ago

Damn. Using the _layout method of ActionView isn't very smart of me. It's liable to break without notice, which I think it does in Rails 5. See Rails commit face604. We might need to do some tests on that method before knowing how to call it reliably.

I don't believe that problem is related to your pull request but thanks for finding it :)

I'll keep this issue open until we fix that failing test for you in Rails 5.

billhorsman commented 8 years ago

Justin. Would you like to take a look at the rails5 branch? I think the tests should pass for you there. If they do, I'll fix it on master and close this issue.

licatajustin commented 8 years ago

@billhorsman, sorry for the delayed response.

That fix did the trick for Rails 5. :+1:

billhorsman commented 8 years ago

I've just released version 1.0.0. Thanks for your help, Justin.