Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 4.2.0 breaks email rendering #36

Closed
pbougie opened this issue Aug 17, 2018 · 13 comments
Closed

Version 4.2.0 breaks email rendering #36

pbougie opened this issue Aug 17, 2018 · 13 comments

Comments

@pbougie
Copy link

pbougie commented Aug 17, 2018

My setup includes email templates, partials, and layouts. Using version 4.1.0, everything renders beautifully. Update to version 4.2.0 (or 4.2.1) and all the files are loading correctly but the MJML does not render into HTML.

I've tried renaming the files as described in the new README but it doesn't help. Tried different variations but nothing worked. Either the template is not found or the MJML does not render.

Working setup in version 4.1.0:

# Gemfile
gem 'mjml-rails', '4.1.0'

# mailers/test_mailer.rb
class TestMailer < ApplicationMailer
  layout 'test'

  def confirmation
    mail(to: '[email protected]', from: '[email protected]') do |format|
      format.mjml
    end
  end
end

# Template + Layout:
# views/layouts/test.mjml
# views/test_mailer/confirmation.mjml.erb

Non-working setup in version 4.2.1

# Gemfile
gem 'mjml-rails', '4.2.1'

# mailers/test_mailer.rb
class TestMailer < ApplicationMailer
  layout 'test'

  def confirmation
    mail(to: '[email protected]', from: '[email protected]') do |format|
      format.html
    end
  end
end

# Template + Layout:
# views/layouts/test.html.mjml
# views/test_mailer/confirmation.html.erb

No errors are thrown but the MJML is not converted into HTML. The log shows all templates and layouts are found and rendered.

@sighmon
Copy link
Owner

sighmon commented Aug 18, 2018

I'm afk, but you definitely don't have any mj-container tags in your MJML files?

@pbougie
Copy link
Author

pbougie commented Aug 18, 2018

No, I don't have any mj-container tags in my MJML files.

@sighmon
Copy link
Owner

sighmon commented Aug 22, 2018

@pbougie in the new tests, @aleksandrs-ledovskis includes this line:

self.view_paths = File.expand_path("../views", __FILE__)

Does that help your missing templates issue?

Could you write a new test using your layout names and structure that fails?

@pbougie
Copy link
Author

pbougie commented Aug 23, 2018

I don't have a missing templates issue. In the example above, the templates are found but the MJML is not rendered. I end up with little formatting and a bunch of CSS with my text. If I inspect the code, I see a bunch of mj-* tags.

@sighmon
Copy link
Owner

sighmon commented Aug 23, 2018

@pbougie Re: #34 do you have the <mjml> root tag outside of your layout file?

@pbougie
Copy link
Author

pbougie commented Aug 24, 2018

No, the <mjml> tag is in my layout file.

@sighmon
Copy link
Owner

sighmon commented Aug 26, 2018

@pbougie and if you create a simple non-layout based template, does it render okay?

@pbougie
Copy link
Author

pbougie commented Aug 26, 2018

I removed the layout and partials, and unfortunately I'm getting the same result.

@pbougie
Copy link
Author

pbougie commented Aug 26, 2018

I've created a test project that you can fork: https://github.com/pbougie/mjml-rails-issue-36

@sighmon
Copy link
Owner

sighmon commented Aug 29, 2018

@pbougie Awesome thanks. It's the owa="desktop" in your <mjml> tag that's breaking things.

The bug was introduced here:
dba6419#diff-694a2aceb95a532b862cfae14a75287f

Would you like to propose a fix and submit a pull request? I wonder whether this regex is too broad? if compiled_source =~ /<mjml(.*)>/

@pbougie
Copy link
Author

pbougie commented Aug 29, 2018

@sighmon I thought I had tried removing that attribute when I was trying various scenarios. Guess not. I've created a pull request: #38

@sighmon
Copy link
Owner

sighmon commented Sep 1, 2018

@pbougie Brilliant, thank you for persisting! I've merged that in now and pushed a new version.

@pbougie
Copy link
Author

pbougie commented Sep 1, 2018

@sighmon Thanks!

@pbougie pbougie closed this as completed Sep 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants