sighmon / mjml-rails

MJML + ERb/Haml/Slim view template
https://mjml.io
Other
301 stars 65 forks source link

support for mail layouts #4

Closed kdiogenes closed 8 years ago

kdiogenes commented 8 years ago

I have the following ActionMailer classes:

class ApplicationMailer < ActionMailer::Base
  layout 'mailer'
end

class UserMailer < ApplicationMailer
  def password(user)
    @user = user
    mail to: @user.email, subject: 'Senha de acesso' do |format|
      format.mjml
    end
  end
end

This is my app/views/layouts/mailer.mjml:

<mjml>
  <mj-body>
    <mj-container>
      <mj-section>
        <mj-text>
          header
        </mj-text>
      </mj-section>
      <mj-section>
        <%= yield %>
      </mj-section>
      <mj-section>
        <mj-text>
          footer
        </mj-text>
      </mj-section>
    </mj-container>
  </mj-body>
</mjml

and this is my app/views/user_mailer/password.mjml

<mj-text>
  password content
</mj-text>

When I preview this email I can see the header and footer text, but no content. I also see this in the logs:

19:18:42 log.1     | Started GET "/rails/mailers/user_mailer/password?part=text%2Fhtml" for 127.0.0.1 at 2016-08-05 19:18:42 -0300
19:18:42 log.1     |   Event Load (0.2ms)  SELECT  "events".* FROM "events" WHERE "events"."acronym" = $1 LIMIT 1  [["acronym", "RAILS"]]
19:18:42 log.1     | Processing by Rails::MailersController#preview as HTML
19:18:42 log.1     |   Parameters: {"part"=>"text/html", "path"=>"user_mailer/password"}
19:18:42 log.1     |   User Load (0.5ms)  SELECT  "users".* FROM "users"  ORDER BY "users"."id" ASC LIMIT 1
19:18:42 log.1     |   Rendered user_mailer/password.mjml within layouts/mailer (0.0ms)
19:18:42 web.1     | Warning: Please upgrade your MJML markup to add a <mjml> root tag, <mj-body> as root will no longer be supported soon, see https://github.com/mjmlio/mjml/blob/master/UPGRADE.md
19:18:42 log.1     |   Rendered user_mailer/password.mjml within layouts/mailer (439.8ms)

Any hint in how to make mail layouts work with it?

kdiogenes commented 8 years ago

I discovered that if I rename my password.mjml template to password.mjml.erb I get the expected output. I don't understand why, probably due my lack of a deeper understanding of how rails render templates/layouts.

sighmon commented 8 years ago

@cerdiogenes Interesting, do you get the same result if you use <%= render :partial => 'password', :formats => [:html] %> instead of <%= yield %>?

ezekg commented 8 years ago

I was attempting to do this last week, and couldn't figure out a way to get a <%= yield %> to work. I ended up just duplicating the mailer layout for each view. I tried with an .erb extension, but that wasn't working for me using Rails 5.

kdiogenes commented 8 years ago

@sighmon render also doesn't work for me, maybe it's my mjml bin version (2.3.1). I can't install an older version to test, probably mjml authors doesn't locked dependencies. With this mjml version yield, nor render, methods will work.

The root of the problem is pointed by the following log line:

Warning: Please upgrade your MJML markup to add a <mjml> root tag, <mj-body> as root will no longer be supported soon, see https://github.com/mjmlio/mjml/blob/master/UPGRADE.md

A workaround is to rename password.mjml to app/views/user_mailer/password.html (this also works for partials); You can also use a template handler like app/views/user_mailer/password.html.haml:

%mj-text password content

The rule of thumb would be: the only file that should have a mjml extension is the one returned for the user. There is already one issue related with this in mjml: https://github.com/mjmlio/mjml/issues/157

I will comment there and see if this use case can be a motivation to change some things in the mjml side.

kdiogenes commented 8 years ago

@sighmon using render works for me, I forgot to undo some modifications that I was trying in the gem, sorry!

sighmon commented 8 years ago

@cerdiogenes oh awesome, glad that worked for you.

bpatram commented 7 years ago

I've run into this issue too in the current version (2.4.0).

It seems as though I need to name my mailer views with the .mjml.erb file extension and my partials and layouts with just .mjml

mmahalwy commented 7 years ago

@bpatram I dont understand. I been trying to do the same with layouts and the views, but no luck.

sighmon commented 7 years ago

@bpatram @mmahalwy could you post some example code? Are you calling the partial like this?

<%= render :partial => 'file_name', :formats => [:html] %>
bpatram commented 7 years ago

@sighmon Here is a very slimmed down version of my app setup

Mailer:

# mailers/foo_mailer.rb
class MyMailer < ActionMailer::Base
  layout "default"

  def mail_template(template_name, recipient, subject, **params)
    mail(
      to: recipient.email,
      from: ENV["MAILER_FROM"],
      subject: subject
    ) do |format|
      format.mjml { render template_name, locals: { recipient: recipient }.merge(params) }
    end
  end

  # this function is called to send the email
  def foo(item, user)
    mail_template(
      "foo_bar",
      user,
      "email subject",
      request: item
    )
  end
end

Email layout:

<!-- views/layouts/default.mjml -->
<mjml>
    <mj-body>
        <mj-container>
            <%= yield %>
        </mj-container>
    </mj-body>
</mjml>

Email view:

<!-- views/my_mailer/foo_bar.mjml.erb -->
<%= render partial: "to", formats: [:html], locals: { name: recipient.name } %>

<mj-section>
    <mj-column>
        <mj-text>
            Hello <%= recipient.name %>!
        </mj-text>
    </mj-column>
</mj-section>

Email partial:

<!-- views/my_mailer/_to.mjml -->
<mj-section>
    <mj-column>
        <mj-text>
            <%= name %>,
        </mj-text>
    </mj-column>
</mj-section>
sighmon commented 7 years ago

@bpatram Interesting - and what's the output from MJML? Do you get a blank view rendered? Or errors?

I haven't tried the layout/view combination with MJML-rails, just view/partials so it'll be good to try and debug this.

bpatram commented 7 years ago

@sighmon As far as I can tell, it does exactly what it's supposed to. The rendered html is correct. It's just odd in how the file extensions must be defined for views, layouts, and partials

mvanduijker commented 7 years ago

Could this be part of the documentation / readme how to create mails with a layout?

sighmon commented 7 years ago

@mvanduijker Good idea.. added to the documentation.

betojsx commented 7 years ago

Why am I getting those errors for <%= yeld %>? screenshot from 2017-07-10 12-42-49

in mailers.mjml screenshot from 2017-07-10 12-44-44

sighmon commented 7 years ago

@betodasilva how odd. Is the button link above it <%=root_path%> rendering okay?

Do you need to wrap it in \<mj-raw> tags for some weird reason?

ryanwjackson commented 6 years ago

I'm experiencing similar issues trying to use HAML for the layout. I can get the layout layout.mjml but the actual template account_summary.mjmldoesn't yield into the layout using = yield.

sighmon commented 6 years ago

@ryanwjackson Any chance you could write a test to confirm that issue so that we can fix it?

ryanwjackson commented 6 years ago

@sighmon I'm actually having a hard time getting the bundle installed because of JSON gem issues. That said, I see tests for partials, but is there a test for layouts? I didn't see one, but could copy for HAML as well.

From the documentation, it also seems odd that the file extensions for layouts, views, & partials vary. I am pretty sure I tried every permutation of .html, .mjml, .haml in the file extension and formats/handlers. Sometimes I get different results, but never the actual view yielded in the layout.

I did some debugging by putting print outs in mjml-rails, and it seems that the MJML being passed in actually doesn't have the view yielded either in some permutations of the above. Does that suggest it's actually the handler or something closer to rails core?

sighmon commented 6 years ago

@ryanwjackson I agree about the oddity in file extensions, from memory that was inherited from the gem I forked to base this on.

I don't have the time to get my head into helping unfortunately (sorry!), but if you'd like to write a test or submit a pull-request that'd be great.

ryanwjackson commented 6 years ago

@sighmon skipping past the extensions for now, I think I found the issue. I haven't been able to write a test for it yet because of some json gem dependencies that aren't working on my machine.

Basically, if you have a layout and a view, when the handler tries to render the view (which it does before the layout), it complains because there's not mjml or mj-body tags at the root of the view. But you don't want those there if you're using a layout.

So I'm a bit confused as to why the example in the README works if mjml requires a mjml and mj-body tag and the handler is called for each layout, view and partial.

Does that spark any thoughts? I'm going to keep digging, but figured I'd update and check.

EDIT:

I should also point out that when the mjml compilation fails, the caller backtrace doesn't involve HAML, so I don't think it's related. Still digging.

srghma commented 6 years ago

layout example with = yield doesn't work for me too

mjml-rails (4.0.2) rails (5.1.5)

nothing is yielded as expected

expected

<body>
  <div>smth </div>
</body>

actual

<body>
  <div> </div>
</body>
sighmon commented 6 years ago

@srghma Try updating MJML to v4.0.5 and let me know how you go.

srghma commented 6 years ago

@sighmon its not working with 4.0.5 as I remember and the problem is in how ActionVeiw is working (I even tried to do something https://github.com/srghma/mjml-rails/tree/render_with_block)

I ended up making my own gem https://github.com/srghma/mjml-premailer

sighmon commented 6 years ago

@srghma Thanks for trying! I'll pop a reference to your gem in the README.

srghma commented 6 years ago

@sighmon tnx buddy!

aleksandrs-ledovskis commented 6 years ago

@sighmon Could you please correct README not to include layout-based example which doesn't work?

See below for solution

milgner commented 6 years ago

Why is this issue closed? Breaking the layout mechanism is a major issue in any Rails application.

Update: after much, much effort, I finally managed to get it to work. The secret - at least in this application - was that the layout file extension needs to be .mjml but the individual templates need .mjml.erb.

aleksandrs-ledovskis commented 6 years ago

Can confirm @milgner findings. For layout/templates to work only layout files must end with .mjml proper.

This quirk is caused by Rails rendering logic that processes files ending with .mjml by mjml JS library and combines it with layout's MJML code - consequently any yielded HTML code, not residing within mj-text is lost.

sighmon commented 6 years ago

@milgner Thanks so much for looking into it. So is the documentation currently correct? At the moment is shows a Layout named: <!-- views/layouts/default.mjml --> with a View named: <!-- views/my_mailer/foo_bar.mjml.erb --> and a Partial named: <!-- views/my_mailer/_to.mjml -->

Should that Partial also be named: <!-- views/my_mailer/_to.mjml.erb -->?

@aleksandrs-ledovskis Any ideas for avoiding the quirk in this gem?

aleksandrs-ledovskis commented 6 years ago

@sighmon As I see it, Rails convention of having views in foobar.FORMAT.HANDLER is not adhered to when following examples/tests in mjml-rails.

In my opinion, structure of email views should be following:

In mailer it would allow to keep using Rails convention principles:

def foo_bar
  @some_object = "Hurr durr"
  mail(mail_params) do |format|
    format.text
    format.html
  end
end

also, within views, partial rendering would not require explicit formats: either:

<%= render partial: "to" %>

All it requires is small patch in Handler#call to always follow template.formats.include?(:mjml) branch. I could prepare a PR shortly

sighmon commented 6 years ago

@aleksandrs-ledovskis That's brilliant - thank you so much for the patch. I've pushed it to Rubygems now, so should be live soon.

@milgner et all - does that work for you too?

aleksandrs-ledovskis commented 6 years ago

N.B. There are some problems with my change. See https://github.com/sighmon/mjml-rails/issues/34