diff --git a/docs/subsystems/email.md b/docs/subsystems/email.md index 0b30457f0d..3ea819254a 100644 --- a/docs/subsystems/email.md +++ b/docs/subsystems/email.md @@ -13,12 +13,12 @@ with only a few things you need to know to get started. - All email templates are in `templates/zerver/emails/`. Each email has three template files: `.subject.txt`, `.txt`, and - `.source.html`. Email templates, along with all other templates + `.html`. Email templates, along with all other templates in the `templates/` directory, are Jinja2 templates. -- Most of the CSS and HTML layout for emails is in `email_base.html`. Note +- Most of the CSS and HTML layout for emails is in `email_base_default.html`. Note that email has to ship with all of its CSS and HTML, so nothing in `static/` is useful for an email. If you're adding new CSS or HTML for an - email, there's a decent chance it should go in `email_base.html`. + email, there's a decent chance it should go in `email_base_default.html`. - All email is eventually sent by `zerver.lib.send_email.send_email`. There are several other functions in `zerver.lib.send_email`, but all of them eventually call the `send_email` function. The most interesting one is @@ -104,11 +104,6 @@ email_password = gmail_password ### Notes -- After changing any HTML email or `email_base.html`, you need to run - `scripts/setup/inline_email_css.py` for the changes to be reflected - in the development environment. The script generates files like - `templates/zerver/emails/compiled/.html`. - - Images won't be displayed in a real email client unless you change the `base_image_uri` used for emails to a public URL such as `https://chat.zulip.org/static/images/emails` (image links to @@ -134,29 +129,18 @@ using a combination of the [css-inline](https://github.com/Stranger6667/css-inline) library and having two copies of each email (plain-text and HTML). -So for each email, there are two source templates: the `.txt` version -(for plain-text format) as well as a `.source.html` template. The -`.txt` version is used directly; while the `.source.html` template is -processed by `scripts/setup/inline_email_css.py` (generating a `.html` template -under `templates/zerver/emails/compiled`); that tool (powered by -`css-inline`) injects the CSS we use for styling our emails -(`templates/zerver/emails/email.css`) into the templates inline. - -What this means is that when you're editing emails, **you need to run -`scripts/setup/inline_email_css.py`** after making changes to see the changes -take effect. Our tooling automatically runs this as part of -`tools/provision` and production deployments; but you should bump -`PROVISION_VERSION` when making changes to emails that change test -behavior, or other developers will get test failures until they -provision. +So, for each email, there are two source templates: the `.txt` version +(for plain-text format) as well as a `.html` template. The `.txt` version +is used directly, while `.html` is processed by `css-inline`, which injects +the CSS we use for styling our emails (`templates/zerver/emails/email.css`) +into the templates just before sending an email. While this model is great for the markup side, it isn't ideal for [translations](../translating/translating.md). The Django translation system works with exact strings, and having different new markup can require translators to re-translate strings, which can result in problems like needing 2 copies of each string (one for -plain-text, one for HTML) and/or needing to re-translate a bunch of -strings after making a CSS tweak. Re-translating these strings is +plain-text, one for HTML). Re-translating these strings is relatively easy in Transifex, but annoying. So when writing email templates, we try to translate individual @@ -166,7 +150,7 @@ translators to not have to deal with multiple versions of each string in our emails. One can test whether you did the translating part right by running -`scripts/setup/inline_email_css.py && manage.py makemessages` and then searching +`manage.py makemessages` and then searching for the strings in `locale/en/LC_MESSAGES/django.po`; if there are multiple copies or they contain CSS colors, you did it wrong. @@ -179,5 +163,5 @@ code path for the "you don't have an account email" might not be, since we might not know what language to use in the second case. Future work in this space could be to actually generate the plain-text -versions of emails from the `.source.html` markup, so that we don't +versions of emails from the `.html` markup, so that we don't need to maintain two copies of each email's text. diff --git a/scripts/setup/inline_email_css.py b/scripts/setup/inline_email_css.py deleted file mode 100755 index aca7020bf6..0000000000 --- a/scripts/setup/inline_email_css.py +++ /dev/null @@ -1,91 +0,0 @@ -#!/usr/bin/env python3 -import os -from typing import Set - -import css_inline - -ZULIP_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../") -EMAIL_TEMPLATES_PATH = os.path.join(ZULIP_PATH, "templates", "zerver", "emails") -CSS_SOURCE_PATH = os.path.join(EMAIL_TEMPLATES_PATH, "email.css") - - -def get_inliner_instance() -> css_inline.CSSInliner: - with open(CSS_SOURCE_PATH) as file: - content = file.read() - return css_inline.CSSInliner(extra_css=content) - - -inliner = get_inliner_instance() - - -def inline_template(template_source_name: str) -> None: - template_name = template_source_name.split(".source.html")[0] - template_path = os.path.join(EMAIL_TEMPLATES_PATH, template_source_name) - compiled_template_path = os.path.join( - os.path.dirname(template_path), "compiled", os.path.basename(template_name) + ".html" - ) - - os.makedirs(os.path.dirname(compiled_template_path), exist_ok=True) - - with open(template_path) as template_source_file: - template_str = template_source_file.read() - output = inliner.inline(template_str) - - output = escape_jinja2_characters(output) - - # Inline method of css-inline will try to complete the DOM tree, - # adding html, head, and body tags if they aren't there. - # While this is correct for the email_base_default template, - # it is wrong for the other templates that extend this - # template, since we'll end up with 2 copies of those tags. - # Thus, we strip this stuff out if the template extends - # another template. - if template_name not in ["email_base_default", "email_base_marketing", "macros"]: - output = strip_unnecessary_tags(output) - - if ( - "zerver/emails/compiled/email_base_default.html" in output - or "zerver/emails/compiled/email_base_marketing.html" in output - or "zerver/emails/email_base_messages.html" in output - ): - assert output.count("") == 0 - assert output.count("") == 0 - assert output.count("") == 0 - assert output.count("") == 0 - - with open(compiled_template_path, "w") as final_template_file: - final_template_file.write(output) - - -def escape_jinja2_characters(text: str) -> str: - escaped_jinja2_characters = [("%7B%7B%20", "{{ "), ("%20%7D%7D", " }}"), (">", ">")] - for escaped, original in escaped_jinja2_characters: - text = text.replace(escaped, original) - return text - - -def strip_unnecessary_tags(text: str) -> str: - end_block = "" - start_block = "{% extends" - start = text.find(start_block) - end = text.rfind(end_block) - if start != -1 and end != -1: - text = text[start:end] - return text - else: - raise ValueError(f"Template does not have {start_block} or {end_block}") - - -def get_all_templates_from_directory(directory: str) -> Set[str]: - result = set() - for f in os.listdir(directory): - if f.endswith(".source.html"): - result.add(f) - return result - - -if __name__ == "__main__": - templates_to_inline = get_all_templates_from_directory(EMAIL_TEMPLATES_PATH) - - for template_source_name in templates_to_inline: - inline_template(template_source_name) diff --git a/templates/zerver/development/dev_tools.html b/templates/zerver/development/dev_tools.html index 7b53485d96..453390be1c 100644 --- a/templates/zerver/development/dev_tools.html +++ b/templates/zerver/development/dev_tools.html @@ -45,9 +45,6 @@ /emails - ./scripts/setup/inline_email_css.py
- Run the command if you made changes to source.html email templates. - View outgoing and example emails. diff --git a/templates/zerver/digest_base.html b/templates/zerver/digest_base.html index 7aa1b90842..8c8e1205f5 100644 --- a/templates/zerver/digest_base.html +++ b/templates/zerver/digest_base.html @@ -16,7 +16,7 @@

Zulip digest

- {% include 'zerver/emails/compiled/digest.html' %} + {% include 'zerver/emails/digest.html' %}

diff --git a/templates/zerver/emails/confirm_new_email.source.html b/templates/zerver/emails/confirm_new_email.html similarity index 92% rename from templates/zerver/emails/confirm_new_email.source.html rename to templates/zerver/emails/confirm_new_email.html index 6b6bcd188d..e755a74e9d 100644 --- a/templates/zerver/emails/confirm_new_email.source.html +++ b/templates/zerver/emails/confirm_new_email.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/confirm_registration.source.html b/templates/zerver/emails/confirm_registration.html similarity index 93% rename from templates/zerver/emails/confirm_registration.source.html rename to templates/zerver/emails/confirm_registration.html index 5279217ee4..bb20f7c5db 100644 --- a/templates/zerver/emails/confirm_registration.source.html +++ b/templates/zerver/emails/confirm_registration.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/custom_email_base.pre.html b/templates/zerver/emails/custom_email_base.pre.html index 05c9470b83..1a2fdd01cd 100644 --- a/templates/zerver/emails/custom_email_base.pre.html +++ b/templates/zerver/emails/custom_email_base.pre.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_marketing.html" %} +{% extends "zerver/emails/email_base_marketing.html" %} {% block illustration %} diff --git a/templates/zerver/emails/deactivate.source.html b/templates/zerver/emails/deactivate.html similarity index 89% rename from templates/zerver/emails/deactivate.source.html rename to templates/zerver/emails/deactivate.html index 87f29813ee..e3261dd2ab 100644 --- a/templates/zerver/emails/deactivate.source.html +++ b/templates/zerver/emails/deactivate.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/digest.source.html b/templates/zerver/emails/digest.html similarity index 100% rename from templates/zerver/emails/digest.source.html rename to templates/zerver/emails/digest.html diff --git a/templates/zerver/emails/email_base_default.source.html b/templates/zerver/emails/email_base_default.html similarity index 97% rename from templates/zerver/emails/email_base_default.source.html rename to templates/zerver/emails/email_base_default.html index 59ced18287..c29c2a0bf0 100644 --- a/templates/zerver/emails/email_base_default.source.html +++ b/templates/zerver/emails/email_base_default.html @@ -1,4 +1,4 @@ -{% import 'zerver/emails/compiled/macros.html' as macros %} +{% import 'zerver/emails/macros.html' as macros %} diff --git a/templates/zerver/emails/email_base_marketing.source.html b/templates/zerver/emails/email_base_marketing.html similarity index 97% rename from templates/zerver/emails/email_base_marketing.source.html rename to templates/zerver/emails/email_base_marketing.html index 0cd178ca64..f2bc0a53f8 100644 --- a/templates/zerver/emails/email_base_marketing.source.html +++ b/templates/zerver/emails/email_base_marketing.html @@ -1,4 +1,4 @@ -{% import 'zerver/emails/compiled/macros.html' as macros %} +{% import 'zerver/emails/macros.html' as macros %} diff --git a/templates/zerver/emails/find_team.source.html b/templates/zerver/emails/find_team.html similarity index 91% rename from templates/zerver/emails/find_team.source.html rename to templates/zerver/emails/find_team.html index aa2e7640a0..7e3ffa5624 100644 --- a/templates/zerver/emails/find_team.source.html +++ b/templates/zerver/emails/find_team.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/followup_day1.source.html b/templates/zerver/emails/followup_day1.html similarity index 97% rename from templates/zerver/emails/followup_day1.source.html rename to templates/zerver/emails/followup_day1.html index 3611ff5a15..169e7fbeda 100644 --- a/templates/zerver/emails/followup_day1.source.html +++ b/templates/zerver/emails/followup_day1.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/followup_day2.source.html b/templates/zerver/emails/followup_day2.html similarity index 96% rename from templates/zerver/emails/followup_day2.source.html rename to templates/zerver/emails/followup_day2.html index 24bd30815b..bd65fd98dc 100644 --- a/templates/zerver/emails/followup_day2.source.html +++ b/templates/zerver/emails/followup_day2.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/invitation.source.html b/templates/zerver/emails/invitation.html similarity index 92% rename from templates/zerver/emails/invitation.source.html rename to templates/zerver/emails/invitation.html index cb3a54284d..09e40b7e8c 100644 --- a/templates/zerver/emails/invitation.source.html +++ b/templates/zerver/emails/invitation.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/invitation_reminder.source.html b/templates/zerver/emails/invitation_reminder.html similarity index 93% rename from templates/zerver/emails/invitation_reminder.source.html rename to templates/zerver/emails/invitation_reminder.html index c39a560ff9..b5db6553e3 100644 --- a/templates/zerver/emails/invitation_reminder.source.html +++ b/templates/zerver/emails/invitation_reminder.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/macros.source.html b/templates/zerver/emails/macros.html similarity index 100% rename from templates/zerver/emails/macros.source.html rename to templates/zerver/emails/macros.html diff --git a/templates/zerver/emails/missed_message.source.html b/templates/zerver/emails/missed_message.html similarity index 100% rename from templates/zerver/emails/missed_message.source.html rename to templates/zerver/emails/missed_message.html diff --git a/templates/zerver/emails/notify_change_in_email.source.html b/templates/zerver/emails/notify_change_in_email.html similarity index 89% rename from templates/zerver/emails/notify_change_in_email.source.html rename to templates/zerver/emails/notify_change_in_email.html index 6696b4643a..e60ade2614 100644 --- a/templates/zerver/emails/notify_change_in_email.source.html +++ b/templates/zerver/emails/notify_change_in_email.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/notify_new_login.source.html b/templates/zerver/emails/notify_new_login.html similarity index 96% rename from templates/zerver/emails/notify_new_login.source.html rename to templates/zerver/emails/notify_new_login.html index 5ce7525557..ea4efd35df 100644 --- a/templates/zerver/emails/notify_new_login.source.html +++ b/templates/zerver/emails/notify_new_login.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block preheader %} {% trans organization_url=realm_uri %}Organization: {{ organization_url }} Time: {{ login_time }} Email: {{ user_email }}{% endtrans %} diff --git a/templates/zerver/emails/password_reset.source.html b/templates/zerver/emails/password_reset.html similarity index 96% rename from templates/zerver/emails/password_reset.source.html rename to templates/zerver/emails/password_reset.html index a2aae3c8fb..cca91a5c83 100644 --- a/templates/zerver/emails/password_reset.source.html +++ b/templates/zerver/emails/password_reset.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/realm_auto_downgraded.source.html b/templates/zerver/emails/realm_auto_downgraded.html similarity index 92% rename from templates/zerver/emails/realm_auto_downgraded.source.html rename to templates/zerver/emails/realm_auto_downgraded.html index 34ca26b7e9..8238594d74 100644 --- a/templates/zerver/emails/realm_auto_downgraded.source.html +++ b/templates/zerver/emails/realm_auto_downgraded.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/templates/zerver/emails/realm_reactivation.source.html b/templates/zerver/emails/realm_reactivation.html similarity index 93% rename from templates/zerver/emails/realm_reactivation.source.html rename to templates/zerver/emails/realm_reactivation.html index eb42bac120..c02d7fee9b 100644 --- a/templates/zerver/emails/realm_reactivation.source.html +++ b/templates/zerver/emails/realm_reactivation.html @@ -1,4 +1,4 @@ -{% extends "zerver/emails/compiled/email_base_default.html" %} +{% extends "zerver/emails/email_base_default.html" %} {% block illustration %} diff --git a/tools/build-release-tarball b/tools/build-release-tarball index 7ce7c1d89d..19b9da2cc9 100755 --- a/tools/build-release-tarball +++ b/tools/build-release-tarball @@ -109,7 +109,7 @@ echo "$version" >version cd "$OUTPUT_DIR" -tar --append -f "$TARBALL" "$prefix/prod-static" "$prefix/build_id" "$prefix/version" "$prefix/zulip-git-version" "$prefix/locale" "$prefix/staticfiles.json" "$prefix/templates/zerver/emails/compiled" "$prefix/webpack-stats-production.json" +tar --append -f "$TARBALL" "$prefix/prod-static" "$prefix/build_id" "$prefix/version" "$prefix/zulip-git-version" "$prefix/locale" "$prefix/staticfiles.json" "$prefix/webpack-stats-production.json" rm -rf "$prefix" diff --git a/tools/check-templates b/tools/check-templates index 342e5adb7a..1d71b2ce08 100755 --- a/tools/check-templates +++ b/tools/check-templates @@ -23,7 +23,7 @@ EXCLUDED_FILES = [ ## Test data Files for testing modules in tests "tools/tests/test_template_data", # Our parser doesn't handle the way its conditionals are layered - "templates/zerver/emails/missed_message.source.html", + "templates/zerver/emails/missed_message.html", # Previously unchecked and our parser doesn't like its indentation "web/images/icons/template.hbs", # Template checker recommends very hard to read indentation. diff --git a/tools/lib/provision_inner.py b/tools/lib/provision_inner.py index e708e8e190..843342d24d 100755 --- a/tools/lib/provision_inner.py +++ b/tools/lib/provision_inner.py @@ -72,15 +72,6 @@ def compilemessages_paths() -> List[str]: return paths -def inline_email_css_paths() -> List[str]: - paths = [ - "scripts/setup/inline_email_css.py", - "templates/zerver/emails/email.css", - ] - paths += glob.glob("templates/zerver/emails/*.source.html") - return paths - - def configure_rabbitmq_paths() -> List[str]: paths = [ "scripts/setup/configure-rabbitmq", @@ -180,16 +171,6 @@ def need_to_run_compilemessages() -> bool: ) -def need_to_run_inline_email_css() -> bool: - if not os.path.exists("templates/zerver/emails/compiled/"): - return True - - return is_digest_obsolete( - "last_email_source_files_hash", - inline_email_css_paths(), - ) - - def need_to_run_configure_rabbitmq(settings_list: List[str]) -> bool: obsolete = is_digest_obsolete( "last_configure_rabbitmq_hash", @@ -246,15 +227,6 @@ def main(options: argparse.Namespace) -> int: else: print("No need to run `tools/setup/build_timezone_values`.") - if options.is_force or need_to_run_inline_email_css(): - run(["scripts/setup/inline_email_css.py"]) - write_new_digest( - "last_email_source_files_hash", - inline_email_css_paths(), - ) - else: - print("No need to run `scripts/setup/inline_email_css.py`.") - if not options.is_build_release_tarball_only: # The following block is skipped when we just need the development # environment to build a release tarball. diff --git a/tools/update-prod-static b/tools/update-prod-static index 7e960ddcf8..894ff900e9 100755 --- a/tools/update-prod-static +++ b/tools/update-prod-static @@ -33,9 +33,6 @@ setup_node_modules(production=True) # Build emoji run(["./tools/setup/emoji/build_emoji"]) -# Inline CSS in emails -run(["./scripts/setup/inline_email_css.py"]) - # Copy over static files from the zulip_bots package run(["./tools/setup/generate_zulip_bots_static_files.py"]) diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index 17637bb246..eea1889473 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -11,6 +11,7 @@ from email.utils import formataddr, parseaddr from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, Tuple, Union import backoff +import css_inline import orjson from django.conf import settings from django.core.mail import EmailMultiAlternatives, get_connection @@ -21,18 +22,19 @@ from django.core.management import CommandError from django.db import transaction from django.http import HttpRequest from django.template import loader -from django.template.exceptions import TemplateDoesNotExist from django.utils.timezone import now as timezone_now from django.utils.translation import gettext as _ from django.utils.translation import override as override_language from confirmation.models import generate_key, one_click_unsubscribe_link -from scripts.setup.inline_email_css import inline_template from zerver.lib.logging_util import log_to_file from zerver.models import EMAIL_TYPES, Realm, ScheduledEmail, UserProfile, get_user_profile_by_id from zproject.email_backends import EmailLogBackEnd, get_forward_address MAX_CONNECTION_TRIES = 3 +ZULIP_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../") +EMAIL_TEMPLATES_PATH = os.path.join(ZULIP_PATH, "templates", "zerver", "emails") +CSS_SOURCE_PATH = os.path.join(EMAIL_TEMPLATES_PATH, "email.css") ## Logging setup ## @@ -40,6 +42,12 @@ logger = logging.getLogger("zulip.send_email") log_to_file(logger, settings.EMAIL_LOG_PATH) +def get_inliner_instance() -> css_inline.CSSInliner: + with open(CSS_SOURCE_PATH) as file: + content = file.read() + return css_inline.CSSInliner(extra_css=content) + + class FromAddress: SUPPORT = parseaddr(settings.ZULIP_ADMINISTRATOR)[1] NOREPLY = parseaddr(settings.NOREPLY_EMAIL_ADDRESS)[1] @@ -124,6 +132,10 @@ def build_email( "physical_address": settings.PHYSICAL_ADDRESS, } + def get_inlined_template(template: str) -> str: + inliner = get_inliner_instance() + return inliner.inline(template) + def render_templates() -> Tuple[str, str, str]: email_subject = ( loader.render_to_string( @@ -136,14 +148,8 @@ def build_email( template_prefix + ".txt", context=context, using="Jinja2_plaintext" ) - try: - html_message = loader.render_to_string(template_prefix + ".html", context) - except TemplateDoesNotExist: - emails_dir = os.path.dirname(template_prefix) - template = os.path.basename(template_prefix) - compiled_template_prefix = os.path.join(emails_dir, "compiled", template) - html_message = loader.render_to_string(compiled_template_prefix + ".html", context) - return (html_message, message, email_subject) + html_message = loader.render_to_string(template_prefix + ".html", context) + return (get_inlined_template(html_message), message, email_subject) # The i18n story for emails is a bit complicated. For emails # going to a single user, we want to use the language that user @@ -518,13 +524,12 @@ def send_custom_email( parsed_email_template = Parser(policy=default).parsestr(text) email_template_hash = hashlib.sha256(text.encode()).hexdigest()[0:32] - email_filename = f"custom/custom_email_{email_template_hash}.source.html" email_id = f"zerver/emails/custom/custom_email_{email_template_hash}" markdown_email_base_template_path = "templates/zerver/emails/custom_email_base.pre.html" - html_source_template_path = f"templates/{email_id}.source.html" + html_template_path = f"templates/{email_id}.html" plain_text_template_path = f"templates/{email_id}.txt" subject_path = f"templates/{email_id}.subject.txt" - os.makedirs(os.path.dirname(html_source_template_path), exist_ok=True) + os.makedirs(os.path.dirname(html_template_path), exist_ok=True) # First, we render the Markdown input file just like our # user-facing docs with render_markdown_path. @@ -536,18 +541,13 @@ def send_custom_email( rendered_input = render_markdown_path(plain_text_template_path.replace("templates/", "")) # And then extend it with our standard email headers. - with open(html_source_template_path, "w") as f: + with open(html_template_path, "w") as f: with open(markdown_email_base_template_path) as base_template: - # Note that we're doing a hacky non-Jinja2 substitution here; - # we do this because the normal render_markdown_path ordering - # doesn't commute properly with inline_email_css. - f.write(base_template.read().replace("{{ rendered_input }}", rendered_input)) + f.write(base_template.read()) with open(subject_path, "w") as f: f.write(get_header(options.get("subject"), parsed_email_template.get("subject"), "subject")) - inline_template(email_filename) - # Finally, we send the actual emails. for user_profile in users: if options.get("admins_only") and not user_profile.is_realm_admin: @@ -556,6 +556,7 @@ def send_custom_email( "realm_uri": user_profile.realm.uri, "realm_name": user_profile.realm.name, "unsubscribe_link": one_click_unsubscribe_link(user_profile, "marketing"), + "rendered_input": rendered_input, } with suppress(EmailNotDeliveredError): send_email( diff --git a/zerver/management/commands/makemessages.py b/zerver/management/commands/makemessages.py index 58872010e9..88c30dbb20 100644 --- a/zerver/management/commands/makemessages.py +++ b/zerver/management/commands/makemessages.py @@ -150,7 +150,6 @@ class Command(makemessages.Command): try: ignore_patterns = options.get("ignore_patterns", []) ignore_patterns.append("docs/*") - ignore_patterns.append("templates/zerver/emails/compiled/*") ignore_patterns.append("templates/zerver/emails/custom/*") ignore_patterns.append("var/*") options["ignore_patterns"] = ignore_patterns diff --git a/zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.source.html b/zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.html similarity index 100% rename from zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.source.html rename to zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.html diff --git a/zerver/tests/fixtures/email/custom_emails/email_base_headers_test.source.html b/zerver/tests/fixtures/email/custom_emails/email_base_headers_test.html similarity index 100% rename from zerver/tests/fixtures/email/custom_emails/email_base_headers_test.source.html rename to zerver/tests/fixtures/email/custom_emails/email_base_headers_test.html diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index 1d66168aad..a429a38a9d 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -101,7 +101,7 @@ class TestCustomEmails(ZulipTestCase): def test_send_custom_email_headers(self) -> None: hamlet = self.example_user("hamlet") markdown_template_path = ( - "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.source.html" + "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.html" ) send_custom_email( [hamlet], @@ -120,7 +120,9 @@ class TestCustomEmails(ZulipTestCase): hamlet = self.example_user("hamlet") from_name = "from_name_test" email_subject = "subject_test" - markdown_template_path = "zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.source.html" + markdown_template_path = ( + "zerver/tests/fixtures/email/custom_emails/email_base_headers_no_headers_test.html" + ) from zerver.lib.send_email import NoEmailArgumentError @@ -151,7 +153,7 @@ class TestCustomEmails(ZulipTestCase): from_name = "from_name_test" email_subject = "subject_test" markdown_template_path = ( - "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.source.html" + "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.html" ) from zerver.lib.send_email import DoubledEmailArgumentError @@ -185,7 +187,7 @@ class TestCustomEmails(ZulipTestCase): non_admin_user = self.example_user("cordelia") markdown_template_path = ( - "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.source.html" + "zerver/tests/fixtures/email/custom_emails/email_base_headers_test.html" ) send_custom_email( [admin_user, non_admin_user], @@ -449,6 +451,10 @@ class TestMissedMessages(ZulipTestCase): s = s.strip() return re.sub(r"\s+", " ", s) + def remove_style_attribute(self, s: str) -> str: + pattern = r'\sstyle="[^"]+"' + return re.sub(pattern, "", s) + def _get_tokens(self) -> List[str]: return ["mm" + str(random.getrandbits(32)) for _ in range(30)] @@ -463,6 +469,7 @@ class TestMissedMessages(ZulipTestCase): verify_body_does_not_include: Sequence[str] = [], trigger: str = "", mentioned_user_group_id: Optional[int] = None, + remove_style: bool = False, ) -> None: othello = self.example_user("othello") hamlet = self.example_user("hamlet") @@ -500,7 +507,10 @@ class TestMissedMessages(ZulipTestCase): if verify_html_body: for text in verify_body_include: assert isinstance(msg.alternatives[0][0], str) - self.assertIn(text, self.normalize_string(msg.alternatives[0][0])) + html = self.normalize_string(msg.alternatives[0][0]) + if remove_style: + html = self.remove_style_attribute(html) + self.assertIn(text, html) else: for text in verify_body_include: self.assertIn(text, self.normalize_string(msg.body)) @@ -1261,11 +1271,16 @@ class TestMissedMessages(ZulipTestCase): f"http://zulip.testserver/user_avatars/{realm.id}/emoji/images/{realm_emoji_id}.png" ) verify_body_include = [ - f':green_tick:' + f':green_tick:' ] email_subject = "DMs with Othello, the Moor of Venice" self._test_cases( - msg_id, verify_body_include, email_subject, send_as_user=False, verify_html_body=True + msg_id, + verify_body_include, + email_subject, + send_as_user=False, + verify_html_body=True, + remove_style=True, ) def test_emojiset_in_missed_message(self) -> None: @@ -1278,11 +1293,16 @@ class TestMissedMessages(ZulipTestCase): "Extremely personal message with a hamburger :hamburger:!", ) verify_body_include = [ - ':hamburger:' + ':hamburger:' ] email_subject = "DMs with Othello, the Moor of Venice" self._test_cases( - msg_id, verify_body_include, email_subject, send_as_user=False, verify_html_body=True + msg_id, + verify_body_include, + email_subject, + send_as_user=False, + verify_html_body=True, + remove_style=True, ) def test_stream_link_in_missed_message(self) -> None: @@ -1298,7 +1318,12 @@ class TestMissedMessages(ZulipTestCase): ] email_subject = "DMs with Othello, the Moor of Venice" self._test_cases( - msg_id, verify_body_include, email_subject, send_as_user=False, verify_html_body=True + msg_id, + verify_body_include, + email_subject, + send_as_user=False, + verify_html_body=True, + remove_style=True, ) def test_pm_link_in_missed_message_header(self) -> None: @@ -1338,7 +1363,8 @@ class TestMissedMessages(ZulipTestCase): self.assertIn("Iago:\n> @**King Hamlet**\n\n--\nYou are", mail.outbox[0].body) # If message content starts with

tag the sender name is appended inside the

tag. self.assertIn( - '

Iago: Iago: * 1\n> *2\n\n--\nYou are receiving", mail.outbox[1].body) # If message content does not starts with

tag sender name is appended before the

tag self.assertIn( - " Iago:

    \n
  • 1
    \n *2
    • \n
\n", - mail.outbox[1].alternatives[0][0], + " Iago:
    \n
  • 1
    \n *2
    • \n
\n", + self.remove_style_attribute(mail.outbox[1].alternatives[0][0]), ) assert isinstance(mail.outbox[2], EmailMultiAlternatives) @@ -1356,7 +1382,7 @@ class TestMissedMessages(ZulipTestCase): # Sender name is not appended to message for PM missed messages self.assertIn( ">\n \n

Hello

\n", - mail.outbox[2].alternatives[0][0], + self.remove_style_attribute(mail.outbox[2].alternatives[0][0]), ) def test_multiple_missed_personal_messages(self) -> None: diff --git a/zerver/views/development/email_log.py b/zerver/views/development/email_log.py index 04d1da3447..4f6ddd8845 100644 --- a/zerver/views/development/email_log.py +++ b/zerver/views/development/email_log.py @@ -1,5 +1,4 @@ import os -import subprocess import urllib from contextlib import suppress from typing import Optional @@ -52,13 +51,6 @@ def clear_emails(request: HttpRequest) -> HttpResponse: @require_safe def generate_all_emails(request: HttpRequest) -> HttpResponse: - if not settings.TEST_SUITE: # nocoverage - # It's really convenient to automatically inline the email CSS - # here, since that saves a step when testing out changes to - # the email CSS. But we don't run this inside the test suite, - # because by role, the tests shouldn't be doing a provision-like thing. - subprocess.check_call(["./scripts/setup/inline_email_css.py"]) - # We import the Django test client inside the view function, # because it isn't needed in production elsewhere, and not # importing it saves ~50ms of unnecessary manage.py startup time.