Fixing digest_custom_html to be treated as HTML (was: Overriding digest.html.erb)

Dev
1

TL;DR:

I’m trying to add some stuff to the digest. I used to override the template, but can’t do that now, for reasons unclear.

Because overriding templates is bad, there are some places in the digest that get replaced with digest_custom_html, but it inserts the text, so those need .html_safe appended. At least one is in the wrong place (has “above” in the name, but is really below).

I think I have the skills for a PR on this one, unless, because it’s so simple, it might be easier for someone else to do it.

The Longer, more painful story.

I did it here: GitHub - pfaffman/discourse-add-to-summary: Add text to summary before and after title but that seems to have stopped working.

I think what I want to do, without changes to core, is override the digest.html.erb template. I used to be able to do that by putting it in app/views/user_notifications/digest.html.erb and that file would get processed instead of the one in core. That no longer seems to work.

Now, we do have some cool digest_custom_html like this:

The clever reader will notice that popular topics starts up around line 78, long before line 277. I’m not sure how much time I lost thinking that nothing was happening because I was looking in the wrong place. But I digress.

I did manage to do this in plugin.rb after_initialize.

  require_dependency "user_notifications"
  module ::UserNotificationsHelperOverride
    def digest_custom_html(position_key)
      puts "doing improved the digest: #{position_key}"
      if position_key == "below_popular_topics"
        puts "doing the custom html for above_popular_topics"

        # Custom HTML for the popular topics position
        "<div class='custom-popular-topics'>MY COOOOOOOL TEXT</div>"
      else
        puts "doing the super for #{position_key}"
        super
      end
    end
  end

And it is indeed adding text to the place I would expect it to be changed in the template!

Alas, it’s not treated as HTML, but as text. So. . . .

I looked in all_the_plugins and find no examples of anyone using this code.

Would it be a PR-welcome if I were to change the lines like

        <%= digest_custom_html("below_popular_topics") %>

and replace them with

        <%= digest_custom_html("below_popular_topics").html_safe %>

And, while I’m at it, maybe make sure that the names of the position_keys are more similar to their position?

1 Like
2

I made a PR:

2 Likes
3

I decided that digest_custom_html not producing HTML that can be parsed is a bug, so I’m recategorizing.

1 Like
4

@martin, sorry to call you out, but you were the last person to touch digest.html.erb (two years ago). Would you mind taking a look at this PR?

1 Like
5

This plugin has been deployed and I’ll probably forget about this very soon, so if anyone wants to use these digest_custom_html fields as intended, you can add code like this to your app.yml to patch the source. I was too lazy to make a regex that replaces them all, and instead just did the one that I was using. Modify as appropriate for your use case.

I created a template in the plugin that can then be included in the app.yml. That makes it a tiny bit easier than fussing with a whole block of yaml.

hooks:
  after_code:
    - replace:
        filename: "/var/www/discourse/app/views/user_notifications/digest.html.erb"
        from: 'digest_custom_html("above_footer") '
        to: 'digest_custom_html("above_footer").html_safe '
1 Like
6

Would it make sense to have a tag to help alert team to open PRs?

1 Like
7

Hey @david. I’m going to try one more time to see if anyone’s interested in this PR. As described above; the code clearly isn’t working as intended, but not one has ever cared. I’ve made a work-around and deployed the code with a fix to patch the template on deployment, but it’s not a pretty solution.

1 Like
8

Could you add the .html_safe to the result of your method override? I don’t think there’s any reason it needs to be in the erb template?

The general aim, both in Rails and in Ember, is to put the “this string is html safe” as close to the authoring/generating point, so that it’s super clear to developers that they need to make sure the HTML is indeed safe (i.e. any user input is escaped)

What you’re doing is ok (as long as it’s tested), but it’s not the “intended” use for these methods. If it was an intentional plugin api, it would be in plugin/instance.rb with the others.

The intended use for this method is to put markdown into the matching translation key:

(Also note the html_safe there - that’s the same technique you’d need to use in any override)

2 Likes
9

O.M.G. Apparently . . . I can!!! It never would have occurred to me that I could put it there and not all the way up in the template where (in my head, at least) it’s rendered.

I now see and understand that digest_custom and that it’s calling .html_safe, pretty as you please, in code that I looked at and tried to understadnd, but it’s confusing (to me) that it’s using that funky no-parens syntax. (Or maybe I never would have seen it).

Well, I do see the values in the locales, but compared to overwriting the template like I was doing before, this is a much less risky solution.

Thanks so very much for putting me on the right path!

Here’s one other thing I’m doing that feels silly. My digest_custom_html needs the user that it’s rendered for, so what I’m doing is overriding digest to set @user so that my digets_custom_html can have access to the user that the digest is for. Is there some super-easy way that I can avoid doing that?

after_initialize do
  # Code which should run after Rails has finished booting

  # require_relative "lib/discourse_add_jobs_to_digest/user_notifications_helper_override"
  require_relative "lib/discourse_add_jobs_to_digest/engine"
  require_relative "lib/discourse_add_jobs_to_digest/job_api"

  require_dependency "user_notifications"
  module ::UserNotificationsHelperOverride
    def digest_custom_html(position_key)
      if position_key == "above_footer"
        DiscourseAddJobsToDigest::JobApi.get_jobs_html(@user).html_safe
      else
        super
      end
    end
  end

  UserNotificationsHelper.prepend(::UserNotificationsHelperOverride)

  module ::UserNotificationsOverride
    def digest(user, opts = {})
      @user = user
      super
    end
  end
  UserNotifications.prepend(::UserNotificationsOverride)
end
2 Likes
10

I don’t see anything else in the template/methods making using a user variable, so yeah what you’ve done is probably the best way to do it.

But still, these kind of overrides can’t be ‘supported’, and may break at any time. Make sure you have some tests to catch that when it eventually happens.

1 Like
11

Thanks for the confirmation! I appreciate your time.

Understood. This is still much better than overriding the whole template, which was my previous solution for a similar plugin.

And, yeah, there should be tests. :wink:

1 Like