-
Notifications
You must be signed in to change notification settings - Fork 58
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
Changes from 5 commits
868435f
d584bf0
eff3d7b
594b670
3284a76
eda9ad5
53a614c
1db5e2d
b06685e
06a548b
f68ef1d
e50ec11
0b4ec30
121b465
d8599f4
119dd29
d6525a1
dcf862d
d538d5a
6f1a63f
f21df6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
from django.template import Context as DjangoContext | ||
from esp.middleware.threadlocalrequest import AutoRequestContext as Context | ||
from esp.middleware import ESPError | ||
from django.template import loader | ||
|
||
class CommModule(ProgramModuleObj): | ||
""" Want to email all ESP students within a 60 mile radius of NYC? | ||
|
@@ -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, | ||
'user': ActionHandler(esp_firstuser, esp_firstuser), | ||
'program': ActionHandler(self.program, esp_firstuser)}))) | ||
|
||
renderedtext = Template(htmlbody).render(DjangoContext(contextdict)) | ||
|
||
return render_to_response(self.baseDir()+'preview.html', request, | ||
|
@@ -175,7 +181,10 @@ def commfinal(self, request, tl, one, two, module, extra, prog): | |
sendto_fn_name = sendto_fn_name, | ||
sender = fromemail, | ||
creator = request.user, | ||
msgtext = body, | ||
msgtext = unicode(loader.get_template('email/default_email_html.txt').render(DjangoContext( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise here I think |
||
{'msgbdy': body, | ||
'user': request.user, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This template should get rendered twice. First, here, we render I wrote too quickly above and was a bit incorrect: any template variables that you put in |
||
'program': self.program }))), | ||
special_headers_dict | ||
= { 'Reply-To': replytoemail, }, ) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<html> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is redundant -- we tell users to include their own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this file should either be called |
||
{% autoescape off %} | ||
{{ msgbdy }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
{% endautoescape %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if the default template should include an unsubscribe footer that links to the disable account page. (If so, the text should definitely say so.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 👍 👍 and maybe some phone-compatibility code? If we include unsubscribe footers (I have a good one of those that I can add) and other stuff, then does that tie us into making them use HTML or Markdown? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're allowing both HTML and plain text, we probably actually want to have two templates; the plain-text one can just include the URL. What do you mean by phone-compatibility code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if we want, we could consider including a per-user unsubscribe link that doesn't require logging in, since making people log in to unsubscribe is really annoying. It would require a bit more code so perhaps it's better to do that separately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean code that makes emails display more nicely on phones (there are sometimes issues with rotating screens and font sizing that you can throw enough HTML at to make them vanish). The per-user link would be nice if that's possible. We can also include an "email us if you want to unsubscribe" if the additional code wouldn't be worth the hassle. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, ok, cool. If by "email us if you want to unsubscribe" you intend for a human to handle it, that's fine; we can't really do an automated thing since the from/reply-to could be an arbitrary address controlled by the chapter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry. I mean for humans to handle the request if logging in is annoying or they forgot their password. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a nice template, making a 1-click unsubscribe wouldn't be too hard -- there might even be a library that will do most of the work for us. But in any case, we can do that after this is merged. |
||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,8 @@ <h2>Step 2:</h2> | |
Please write your email/message now. Note that you can use <tt>{{ parameter }}</tt> to enter a parameter | ||
into the text of the message—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 <html> and </html> 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can actually probably link straight to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
Don't forget to write the body in HTML, but no need for <html> and </html> tags.</p> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should still allow writing plain-text emails, because it's easier and if you don't need any formatting it's better. (Ideally we may want to switch to Markdown for emails, because that's both easier and can be used to make pretty things.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, my thought was to include a default HTML email that did something basic (such as replacing newlines with paragraph tags, adding some kind of standard unsubscribe message, and maybe some phone-compatibility things) but benign so that it will automatically format it a bit for them. Most of the chapter board was very enthusiastic about this idea, and it seems nice to include some baseline level of professionalism (it's actually illegal to send mass emails like ours without some unsubscribe instructions, for instance, and unformatted plain text doesn't look very nice). Of course, programs could modify this file as they wish (that's the whole point), but having a default with some easy prettify-ing HTML in there seems like a nice way to add a little style to the emails. But of course I'm open to discussion on this point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to do that, we should actually do that, probably by converting markdown (perhaps optionally). You can look at how we do it for QSD as a guide. If we do that I'm fine not allowing plaintext, but as I understand it, as-is this will just eat the newlines and such, and I do think for short emails unformatted text looks fine. Incidentally, I believe CAN-SPAM doesn't actually apply to our use case since we are non-commercial and all users have signed up for our mailing list by registering an account, although we certainly should include unsubscribe links. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, while we're considering this, we may want to think about whether we should always be attaching a plain-text version of HTML emails, since there are still some email clients that will be sad if you don't. If we do markdown and people don't include raw HTML tags, this is fairly easy; in the more general case it could be a lot trickier. So we may not want to bother, or if we do, may want to do it in a separate pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be nice to do the full-blown version. I'll take a look at the markdown conversion. Good call on including the plain-text version though. That seems worth doing. And maybe a line to the effect of "click here to see this email in your browser" if we can manage that without too much trouble? I'm kind of tempted to force admins to write in something that looks nice. We could include an optional if-statement to catch emails under 50 words with no formatting tags (or whatever) that would just wrap them in <p> tags and add the default headers/footers and for emails over 50 words, require admins to use HTML. Thoughts on that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Click here to see this email in your browser" is fairly reasonable; we'd just have to render the same template into a page. The only problem is that we'd either have to always make people log in for it, or somehow make it user-specific without doing that, which is a little tricky to do correctly (i.e. without leaking user data). But it's probably doable. Markdown seems better (and probably easier) than trying to autowrap Incidentally, we'll want to make sure the release notes of whatever release this goes in include a mention of the fact that the defaults have changed, since some of these are breaking changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, cool. I don't know how to safely render emails without making them log in, but I can search around a bit. Sounds good. I'm not super familiar with how Markdown works, but I'll check out the QSD-conversion code and see if I can figure out how to adapt it. Good call on the release notes. At this point, I think I should open a new pull request after making some changes to this one. To summarize, next steps are:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically we'd want to do it similar to an unsubscribe link: give each user a unique unguessable link, which the website can decode to get their username and therefore render the correct email for them. I'm not sure what the best way to do that in practice is, but there may be some package out there that will do the crypto for you. For release notes, if you want you can just add a blank release notes file now, and put your stuff in it, or you can do it when we actually get around to cutting the release. It would probably be nice if we did more of the former, but since we haven't in the past it's fine if you want to do the latter. Those steps sound good! |
||
|
||
<form action="/manage/{{program.getUrlBase}}/commprev" method="post" name="comm2"> | ||
<table border="0" cellspacing="0" width="100%"> | ||
|
There was a problem hiding this comment.
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 thanget_template()
andunicode()
, and in that case you don't need to call theContext
instance yourself, you just pass a dict.There was a problem hiding this comment.
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 ofbody
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.