Partials support? #53

ohing504 · 2020-01-14T05:08:13Z

When I try render partials with maildown, the rendered partial show as text not html.
In specific, I want add footer with markdown format in mailer layout.

app/views/layouts/mailer.html.erb

<!DOCTYPE html>
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8;"/>
</head>
<body>
<%= yield %>

<%= render partial: "mailer/shared/footer" %>
</body>
</html>

and, this is footer

app/views/mailer/shared/_footer.en.md.erb

-----------------------------------------------

This is Footer in English

The result is:

body content

<hr /> <p>This is Footer in English</p>

Any Idea?

The text was updated successfully, but these errors were encountered:

schneems · 2020-01-17T20:19:43Z

I don't know. For sure.

What I think is happening is that maildown is actually not limited to just emails. So what it's doing is rendering the _footer.en.md.erb as if it's an HTML template in the call to render. I think you need to remove the .md.erb file extension and then it will render as raw text and then after I believe be converted to html where it's needed/wanted. Maybe something like _footer.en.raw_md or something?

I'm not sure though, can you give me an example app that reproduces the problem https://www.codetriage.com/example_apps. No guarantees I can fix it but reproducing the issue is the first step.

sakamies · 2020-02-04T12:24:51Z

I'm not an expert, but solved this same issue in app views by forcing the rendered partials to be html safe like so.

<%= render(partial: "mailer/shared/footer").html_safe %>

The fact that maildown is not limited to just emails is awesome by the way. I use it all over my app! Makes longer translatable texts so much nicer than using a yaml language file.

mkamensky · 2020-03-03T06:59:20Z

I believe the problem is the following: In https://github.com/codetriage/maildown/blob/a71449c3f1b18a74c63c64ddb529cc732791f444/lib/maildown/handlers/markdown.rb#L39 Maildown uses template.format to determine the format. However, this field is not set for the partial template. I overcome this as follows. First, my partial rendering looks like this:

<%= render(partial: 'foo', locals: {"_no_cache_#{@current_template.format || ''}" => true}).html_safe %>

The locals bit is required, since the templates are cached, and we need to distinguish different formats. Then, after https://github.com/codetriage/maildown/blob/a71449c3f1b18a74c63c64ddb529cc732791f444/lib/maildown/ext/action_view.rb#L21 I added the following:

  class PartialRenderer
    alias :original_find_partial :find_partial

    def find_partial(*args)
      template = original_find_partial(*args)
      template.instance_variable_set('@format', formats.first)
      template
    end
  end

This is of course completely horrible, and I hope there is a better solution, but it works for me.

schneems · 2020-09-18T02:58:27Z

Is this still an issue? If so can you give me an example app that shows the problem? https://www.codetriage.com/example_app

mkamensky · 2020-12-21T17:36:09Z

I believe the problem is the following: In

https://github.com/codetriage/maildown/blob/a71449c3f1b18a74c63c64ddb529cc732791f444/lib/maildown/handlers/markdown.rb#L39
Maildown uses template.format to determine the format. However, this field is not set for the partial template. I overcome this as follows. First, my partial rendering looks like this:
<%= render(partial: 'foo', locals: {"_no_cache_#{@current_template.format || ''}" => true}).html_safe %>
The locals bit is required, since the templates are cached, and we need to distinguish different formats. Then, after

https://github.com/codetriage/maildown/blob/a71449c3f1b18a74c63c64ddb529cc732791f444/lib/maildown/ext/action_view.rb#L21
I added the following:
  class PartialRenderer
    alias :original_find_partial :find_partial

    def find_partial(*args)
      template = original_find_partial(*args)
      template.instance_variable_set('@format', formats.first)
      template
    end
  end
This is of course completely horrible, and I hope there is a better solution, but it works for me.

This no longer works in Rails 6.1, the method name was changed to find_template.

rathboma · 2021-12-02T16:18:16Z

Hey! This is still a problem for sure. Simply include any partial with a .md.erb extension in your email to reproduce.

schneems · 2021-12-07T21:24:53Z

I'm glad you could reproduce it. Can you give me an example app that shows the problem? https://www.codetriage.com/example_app

I generally won't work on something if I can't have a verified reproduction. Which is why I've asked for one. Alternatively, I'll take a look at a patch with a test if you've got a fix lined up.

schneems · 2022-07-31T22:37:33Z

It's been awhile, closing for now. Please provide an example app and I'm happy to re-open.

joshuapinter · 2022-08-30T14:22:06Z

FYI, seeing the same thing. Using .html_safe used to work in Rails < 6.1 but with Rails 6.1, it renders the partial as HTML instead of plain text when the format is plain text. Would be great to get a proper solution. I don't have time to make an example app to reproduce it but I'll try and take a look at a solution/workaround.

joshuapinter · 2022-08-30T17:11:45Z

NOTE: THIS IS INCORRECT.

For the record, @mkamensky's fork of this fixes it perfectly so anybody that needs this should use this in their Gemfile until this gets fixed in the main repo:

gem "maildown", github: "mkamensky/maildown"

@schneems It's not an example app but @mkamensky's fork shows exactly what is not working and how to fix it. Should be able to include this in this main repo. I tried looking at it myself for a bit but not sure of the intricacies.

joshuapinter · 2022-08-30T17:54:56Z

Nevermind, I take it back. @mkamensky's fork doesn't fix it like I thought. But it does permit a strange workaround that might shed some light on what's going wrong and how to fix it.

So, with the @mkamensky's fork, I found that doing the following worked:

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: nil ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% end %>

What you choose to put in the locals doesn't matter, just that the keys are different for different formats. Here is a collection of what works and what doesn't work and you can start to see the pattern:

Doesn't work

<% if formats.first == :text %>
  <%= render( partial: "my_partial", formats: formats ).html_safe %>
<% else %>
  <%= render( partial: "my_partial" ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad" } ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom", my: "dad" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad", my: "mom" } ).html_safe %>
<% end %>

Works

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: nil ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { mom: "your" } ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad", my: "mom" } ).html_safe %>
<% end %>

Based on this, it seems like some kind of caching issue where having different keys in the locals variable causes it to attempt to re-render.

I also did some debugging and added a binding.pry inside of Maildown::Markdown.call and when the keys in locals were identical, it would only hit this .call method once for the partial (with :html as the format) but when the keys in locals were different, it would hit the .call method twice for the partial (once with :html as the format and once with :text as the format) and the plain text version of the partial was rendered correctly.

@schneems Hopefully this is enough information to maybe look into or at the very least re-open this Issue. If you do that, that'll let me know you'll take a look at it if I get an Example App / test reproducing the issue.

For now, the workaround is to use @mkamensky's fork and then ensure your locals are unique for each format using something like this:

<%= render( partial: "my_partial", locals: { formats.first => true } ).html_safe %>

mkamensky · 2022-08-30T18:47:25Z

Nevermind, I take it back. @mkamensky's fork doesn't fix it like I thought. But it does permit a strange workaround that might shed some light on what's going wrong and how to fix it.

So, with the @mkamensky's fork, I found that doing the following worked:
<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: nil ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% end %>
What you choose to put in the locals doesn't matter, just that the keys are different for different formats. Here is a collection of what works and what doesn't work and you can start to see the pattern:

Doesn't work
<% if formats.first == :text %>
  <%= render( partial: "my_partial", formats: formats ).html_safe %>
<% else %>
  <%= render( partial: "my_partial" ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad" } ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom", my: "dad" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad", my: "mom" } ).html_safe %>
<% end %>
Works
<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: nil ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { mom: "your" } ).html_safe %>
<% end %>

<% if formats.first == :text %>
  <%= render( partial: "my_partial", locals: { your: "mom" } ).html_safe %>
<% else %>
  <%= render( partial: "my_partial", locals: { your: "dad", my: "mom" } ).html_safe %>
<% end %>
Based on this, it seems like some kind of caching issue where having different keys in the locals variable causes it to attempt to re-render.

I also did some debugging and added a binding.pry inside of Maildown::Markdown.call and when the keys in locals were identical, it would only hit this .call method once for the partial (with :html as the format) but when the keys in locals were different, it would hit the .call method twice for the partial (once with :html as the format and once with :text as the format) and the plain text version of the partial was rendered correctly.

@schneems Hopefully this is enough information to maybe look into or at the very least re-open this Issue. If you do that, that'll let me know you'll take a look at it if I get an Example App / test reproducing the issue.

For now, the workaround is to use @mkamensky's fork and then ensure your locals are unique for each format using something like this:
<%= render( partial: "my_partial", locals: { formats.first => true } ).html_safe %>

Yes, that's what I was trying to say in my original comment. I believe my suggestion for the locals works better than various family members since it ends up being unique per partial and format

joshuapinter · 2022-08-30T18:52:49Z

Right on. I didn't get that the keys of the locals had to be unique. I found that to be a really strange requirement.

And for the record, the family member examples was just to illustrate that the keys had to be unique for the template to be re-rendered with a new format, not just the value of the keys.

We ended up using:

<%= render( "my_partial", { formats.first => true } ).html_safe %>

But we're not passing any locals for our markdown mailer partials so the chance of text or html collision is not an issue. Your way looks to be much more unique.

Still, I'd love to actually solve this properly and get it working without passing any unique locals.

joshuapinter · 2022-09-21T16:16:59Z

Hi @schneems,

I finally got around to creating a simple example app that re-creates the issue of markdown (.md.erb) partials not rendering correctly in plain text. You can view the repo here:

https://github.com/joshuapinter/maildown_partials_issue

I've included the repro instructions in the README but here they are as well:

Clone the repo and install gems via bundle.
Run the server via bundle exec rails s
Navigate to http://localhost:3000/rails/mailers/notifier_mailer/markdown_partial_test in your browser:

By default the HTML format will appear, and it appears correctly.
Click on the format drop down and select "View as plain-text email".

You will see that the partial gets rendered incorrectly as HTML instead of plain text.
To see what it should look like, navigate to http://localhost:3000/rails/mailers/notifier_mailer/non_markdown_partial_test.txt in your browser.

You will see that using two files for formats (html.erb and text.erb) and not using markdown (md.erb) via maildown renders the partials correctly for both HTML and plain text.

I did this in Rails 6 and Ruby 3 because that's what I had handy.

I hope this is enough to re-open this Issue and take another look. I've spent a few hours trying to figure it out myself and digging into how Rails renders partials but it's a bit over my head.

Let me know if you need anything from my side or if you want me to do some testing.

Rendering emails in markdown is incredible and I feel like this is the missing piece to make it truly clean, DRY and beautiful.

Thanks in advance!

schneems · 2022-09-21T20:00:20Z

Awesome sauce, thanks a ton! I opened a new issue to put my thoughts into this. Also I'm going to try fishing for some hacktober commits but this project is a bit unwieldy. I honestly have to re-learn how it works every time I touch it.

schneems · 2022-09-21T20:03:17Z

New issue is #69

joshuapinter · 2022-09-21T21:13:59Z

Awesome. Like I said, I got pretty deep into it but after a few hours, I just couldn't get too much progress so I figured my time was better spent creating the example app instead.

I'll follow along on the new Issue and if there's something I can dig into for you, let me know. We use this for work so I can justify some time spent here and there on it, if I'm given some direction.

schneems closed this as completed Jul 31, 2022

schneems mentioned this issue Sep 21, 2022

Bug/Feature: Partials not rendered correctly #69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partials support? #53

Partials support? #53

ohing504 commented Jan 14, 2020

schneems commented Jan 17, 2020

sakamies commented Feb 4, 2020

mkamensky commented Mar 3, 2020

schneems commented Sep 18, 2020

mkamensky commented Dec 21, 2020

rathboma commented Dec 2, 2021

schneems commented Dec 7, 2021

schneems commented Jul 31, 2022

joshuapinter commented Aug 30, 2022

joshuapinter commented Aug 30, 2022 •

edited

Loading

joshuapinter commented Aug 30, 2022 •

edited

Loading

mkamensky commented Aug 30, 2022

Doesn't work

Works

joshuapinter commented Aug 30, 2022 •

edited

Loading

joshuapinter commented Sep 21, 2022

schneems commented Sep 21, 2022

schneems commented Sep 21, 2022

joshuapinter commented Sep 21, 2022

Partials support? #53

Partials support? #53

Comments

ohing504 commented Jan 14, 2020

schneems commented Jan 17, 2020

sakamies commented Feb 4, 2020

mkamensky commented Mar 3, 2020

schneems commented Sep 18, 2020

mkamensky commented Dec 21, 2020

rathboma commented Dec 2, 2021

schneems commented Dec 7, 2021

schneems commented Jul 31, 2022

joshuapinter commented Aug 30, 2022

joshuapinter commented Aug 30, 2022 • edited Loading

joshuapinter commented Aug 30, 2022 • edited Loading

Doesn't work

Works

mkamensky commented Aug 30, 2022

Doesn't work

Works

joshuapinter commented Aug 30, 2022 • edited Loading

joshuapinter commented Sep 21, 2022

schneems commented Sep 21, 2022

schneems commented Sep 21, 2022

joshuapinter commented Sep 21, 2022

joshuapinter commented Aug 30, 2022 •

edited

Loading

joshuapinter commented Aug 30, 2022 •

edited

Loading

joshuapinter commented Aug 30, 2022 •

edited

Loading