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

[WIP] jinja2 whitespace stripping and email testing #5385

Merged
merged 2 commits into from Jun 22, 2017
Merged

Conversation

vaidap
Copy link
Member

@vaidap vaidap commented Jun 14, 2017

fixes #5274.

The only places I found control blocks not being on their own line was in digest.txt & missed_message.txt (both of them will be redone so i left them as-is), and invitation_reminder.subject -- i didn't find it to cause problems as it's all in one line.

the second commit adds a test value so invitation_reminder.subject triggers the {% if referrer_realm_name %} block.

@smarx
Copy link

smarx commented Jun 14, 2017

Automated message from Dropbox CLA bot

@vaidap, it looks like you've already signed the Dropbox CLA. Thanks!

@@ -335,6 +335,8 @@ def get_secret(key):
})
non_html_template_engine_settings['OPTIONS'].update({
'autoescape': False,
# 'trim_blocks': True,
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to leave these commented out?

@@ -48,6 +48,7 @@ def email_page(request):
},
'referrer_name': 'Road Runner',
'referrer_email': 'runner@acme.com',
'referrer_realm_name': 'The Road Runners',
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be "Acme Corporation"

@rishig
Copy link
Member

rishig commented Jun 16, 2017

Thanks @vaidap! Left two small comments.

@vaidap
Copy link
Member Author

vaidap commented Jun 19, 2017

sorry for those silly mistakes! updated now.

@codecov-io
Copy link

codecov-io commented Jun 20, 2017

Codecov Report

Merging #5385 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5385   +/-   ##
======================================
  Coverage    86.5%   86.5%           
======================================
  Files         430     430           
  Lines       53111   53111           
  Branches     2069    2069           
======================================
  Hits        45942   45942           
  Misses       7169    7169
Impacted Files Coverage Δ
zproject/settings.py 84.81% <ø> (ø) ⬆️
zerver/views/test_emails.py 93.33% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c72284...6f3b5ab. Read the comment docs.

@rishig
Copy link
Member

rishig commented Jun 21, 2017

Great, thanks @vaidap!

For the first commit, it would be good to go through all the text emails at /emails, and fix all of their spacing. You'll also have to check and make sure the spacing is good for the missed_message, digest, password_reset, and any other emails not shown in /emails.

@showell
Copy link
Contributor

showell commented Jun 22, 2017

Pinging @vaidap and @rishig on this.

@vaidap
Copy link
Member Author

vaidap commented Jun 22, 2017

i'm having troubles understanding the task @rishig, would be amazing if you could expand on this: when it comes to fixing the spacing, is it for things such as

at the following link:
    http://localhost:9991

?
also, i am not entirely sure how to generate the output from the text files. is there a jinja2 loader somewhere that would generate that?

@jrowan
Copy link
Collaborator

jrowan commented Jun 22, 2017

@vaidap A kind of hacky way to generate output from jinja2 templates is to open a python shell and load the templates there

>>> from jinja2 import Template
>>> with open("invitation_reminder.txt", "r") as f:
...     email_template = f.read()
...
>>> template = Template(email_template)
>>> email = template.render(referrer_name="Wile E. Coyote", referrer_email="wile@example.com", activate_url="zulip.example.com/activate", support_email="support@example.com")

Then you can write to a text file and cat, for example, to view the email.

@rishig
Copy link
Member

rishig commented Jun 22, 2017

For many of them, you can also go to localhost:9991/emails in the dev environment.

@rishig
Copy link
Member

rishig commented Jun 22, 2017

Though at this point I think it might actually make sense for @jrowan to take this over, since he's editing so much of the text anyway. Would both of you be ok with that?

@vaidap
Copy link
Member Author

vaidap commented Jun 22, 2017

I'd be fine with that, especially since I feel more comfortable with helping with the design than with the templating work!

@jrowan
Copy link
Collaborator

jrowan commented Jun 22, 2017

Sure, I'll work on this.

@showell showell merged commit 1fb8eb8 into zulip:master Jun 22, 2017
@showell
Copy link
Contributor

showell commented Jun 22, 2017

Merged! Thanks @vaidap!

@rishig
Copy link
Member

rishig commented Jun 22, 2017

ah oops, probably should not have been merged! Fine for now though, we'll fix anything off in the other email PR.

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.

emails: Remove whitespace caused by jinja2 controls in non-HTML templates.
7 participants