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

Add default email template #1898

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

milescalabresi
Copy link
Contributor

Adds editable default HTML template for emails sent through the comm panel. The Travis build failed, but I believe this is (now, after a few fixes) because of the test. The code works when I tested by hand, and the Travis build appears to test the message body in a way that assumes the old code.
Of course, I could be wrong, so someone who knows Travis better than I do should verify, and perhaps update the build if this change seems like something worth implementing

(chapter request) add a default template that can be overridden via /admin/ for chapters who regularly use HTML templates and don't want to copy-paste into the box
per a chapter request, this change makes the comm module use a default email template to wrap around all messages sent through the comm panel. Now users just type in HTML into the comm panel and it is automatically put into a template set via template overrides (default is just to put <html> tags around the message typed into the comm panel)
@benjaminjkraft
Copy link
Contributor

Yep, it looks like the test is failing because the exact test changed. So you can just update the test to use the correct message body instead of the one it has.

@@ -109,6 +110,11 @@ def commprev(self, request, tl, one, two, module, extra, prog):
esp_firstuser = ESPUser(firstuser)
contextdict = {'user' : ActionHandler(esp_firstuser, esp_firstuser),
'program': ActionHandler(self.program, esp_firstuser) }

htmlbody = unicode(loader.get_template('email/default_email_html.txt').render(DjangoContext({'msgbdy': body,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the more standard thing is to use render_to_string() rather than get_template() and unicode(), and in that case you don't need to call the Context instance yourself, you just pass a dict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think you want htmlbody instead of body here, otherwise lines 105-108 are meaningless. Also also, passing the user and program in here is unnecessary.

Edit: Oh, I see why you might want to. I guess it's fine, although it might be good to have a warning somewhere (not sure where) that if you modify the template you should make sure not to use any user vars (ideally not even any program vars) inside the {% autoescape off %} block.

@benjaminjkraft
Copy link
Contributor

Ooh, cool! I'm assuming that the point of this is to make it easier for chapters to add a standard header/footer to their message?

@@ -38,7 +38,8 @@
Please write your email/message now. Note that you can use <tt>{{ parameter }}</tt> to enter a parameter
into the text of the message&mdash;the parameters are listed below.

<p><b>Tip:</b> If you would like to send your message in HTML format, wrap your entire message body in &lt;html&gt; and &lt;/html&gt; tags.</p>
<p><b>Tip:</b> The default {{ settings.ORGANIZATION_SHORT_NAME }} template (which you can modify <a href="/admin/utils/templateoverride/">here</a>) is automatically wrapped around your message below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually probably link straight to /admin/utils/templateoverride/?name=default_email_html.txt or something to automagically do the search/filter for the right TO (if it exists).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea! Thanks! I forgot you could search by name. I'll add that in once we settle on a good name for file, which will probably depend on what we decide for this guy.

@milescalabresi
Copy link
Contributor Author

Yes exactly! Neat idea, right? Yale Splash came up with the idea after people got tired of typing the "click here to unsubscribe" footer and wanted it automatically inserted. Now people can add, for instance, inline images from the filebrowser or make the emails more phone-friendly without having to copy-paste the same markup background text each time they use the comm panel.

As you can see, I executed this whole thing a little sloppily. I really appreciate your detailed comments, by the way! I will get to work on fixing it up. Regarding the loader.render_to_string command, I saw that, but when I tried to use it, it prematurely rendered the file so user-entered parameters would show up unrendered. Any idea on how to fix that without doing my hack?

@benjaminjkraft
Copy link
Contributor

How were you calling render_to_string, and what was the output?

@milescalabresi
Copy link
Contributor Author

Oh, on second thought, I might have done that before I remembered to turn autoescaping off. I'll check again after fixing the other stuff. I'll probably open a new pull request, depending on what we decide on the HTML/plain text/Markdown question.

msgtext = body,
msgtext = unicode(loader.get_template('email/default_email_html.txt').render(DjangoContext(
{'msgbdy': body,
'user': request.user,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes the template to render user variables in the template itself (not message body) with information about the sender. Not a problem with the default since there's no use of the user, but this is what's causing Yale's bug. Since the template is going to get rendered into one copy for all users, which then gets further transformed later into the individual copies, user information shouldn't be passed here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to add user information into the template? It would be very useful to put, for instance, the username the email was sent to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can still put user information in the template; it will just get interpreted on the second render pass, when we add user variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't quite understand what the difference is / what needs to be done in order to make sure the right thing happens instead of the variables being rendered in the template?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This template should get rendered twice. First, here, we render msgbdy into default_email_html.txt to get msgtext. This happens once, and therefore should not include any user data, because we don't yet know what user the email will be sent to. Then, later, below, we the output of that (msgtext) into the actual email, once for each user, injecting their information into the template. Any variables in msgtext will get interpreted on the second pass. When you include request.user the first time, it means that any user variables in default_email_html.txt get rendered according to the user sending the email, because that's who request.user is; instead you want to render them the second time with the email's recipient as the user.

I wrote too quickly above and was a bit incorrect: any template variables that you put in default_email_html.txt that should get rendered on the second pass, i.e., any user variables, need to have their template tags excaped, using {% templatetag openvariable %} instead of {{ and {% templatetag closevariable %} instead of }}. This way, the first rendering pass will convert those to {{ and }} and then the second rendering pass will see them as normal variables, and interpolate the correct user's information.

@benjaminjkraft
Copy link
Contributor

@milescalabresi any progress on this? Anything I or others can help out with? If it would be helpful we can talk through some of the tricky bits on Skype sometime.

@milescalabresi
Copy link
Contributor Author

No, but thanks for the poke. I'll get on it this week and definitely take you up on the offer. Much appreciated.

@harianbarasu
Copy link

harianbarasu commented Jul 9, 2016

Next steps are probably going to be:

  1. Add radio buttons to step2.html giving users options for either plain text in email or HTML
  2. Implement quick check to confirm plain text vs HTML (if we see <p> tags and they've selected plain text, yell at them; if we see <html> or </html> either way, also yell at them.)
  3. If plain text is checked, run through simple regex parser to convert whitespace to <br> and add <p> tags

@milescalabresi
Copy link
Contributor Author

milescalabresi commented Aug 23, 2016

More specifics on the steps (thanks to Ben for the advice):

If the user selects neither HTML nor plaintext on the radio buttons, display a default Django forms error.

Step 2:
If they select plain text but then enter <p> or something, then use a jquery-ui (https://jqueryui.com/dialog/ for the dialog) that forces them to acknowledge "yes I know, do it anyway" so that the tags would get sanitized to &lt;p&gt; (or the corresponding "something") before the text is converted to HTML (and the whitespace is adjusted).

Details on Step 3:
if you choose plaintext, we want to replace <, >, and & with their codes, double newlines with </p> <p>, single newlines with <br />, in that order, and then wrap the whole thing inside

[template stuff]
<p>
...body...
</p>

(As I learned, <html> and </html> tags are not required in emails.)

Details on the template:
we would need to set the mimetype to text/html (I should double check that, though), but the standard Django libraries should be able to handle that for usand see http://www.inboxinspector.com/ for a preview.
Wikipedia says text/html; charset=UTF8 should do the trick, and Django should fill that in (see https://docs.djangoproject.com/en/1.9/topics/email/ and we can even include both HTML and text parts: https://docs.djangoproject.com/en/1.9/topics/email/#sending-alternative-content-types).

Added steps : consider having a "view this email in a browser" link, and see http://www.inboxinspector.com/ for a preview.

@willgearty
Copy link
Member

willgearty commented Feb 6, 2019

So...hate to be THAT guy, but I'm going to guess that #2666 will break this. But I do think #2666 is an important addition, so hopefully it won't be too hard to integrate these changes with that?

@milescalabresi
Copy link
Contributor Author

thanks for the heads-up. I'll look at it before I finish this branch

@willgearty willgearty added this to the Stable Release 12 milestone Apr 8, 2019
@willgearty
Copy link
Member

Any update on this? Do we think this will be ready for SR12?

@milescalabresi
Copy link
Contributor Author

haven't touched it in a while, sorry. I can try to make this the March priority, but I want to discuss the scope with the team given the impact of the GUI

@willgearty
Copy link
Member

Maybe something like #2885 would be more appropriate then? We could have some array of buttons like they do in other mass-mailing services (e.g. Mailchimp) that would import/inject a template into the commpanel GUI. The most basic version could pull all of templates from a template folder (e.g. /program/commpanel/templates) and then just show the name of the template in the button. A more advanced version could actually render the templates within the buttons. This way, chapters could create any number of custom templates (as template overrides) within this folder, and each one would be selectable from the comm panel.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants