sighmon / mjml-rails

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

Version 4.2.0 breaks email rendering #36

Closed pbougie closed 6 years ago

pbougie commented 6 years ago

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: 'test@example.com', from: 'hello@example.com') 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: 'test@example.com', from: 'hello@example.com') 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 commented 6 years ago

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

pbougie commented 6 years ago

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

sighmon commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

pbougie commented 6 years ago

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

sighmon commented 6 years ago

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

pbougie commented 6 years ago

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

pbougie commented 6 years ago

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

sighmon commented 6 years ago

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

The bug was introduced here: https://github.com/sighmon/mjml-rails/commit/dba64199dced5ba72cc3bc7b34973425b3aaf5a8#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 commented 6 years ago

@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 commented 6 years ago

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

pbougie commented 6 years ago

@sighmon Thanks!