Dear {% firstof user.full_name user.email %},
- {% block content %}{% endblock content %} -
- --
- Kind regards,
- Blender Extensions Team
-
+ {{ quoted_message|truncatewords:15|truncatechars:140 }}
+
+ Read all notifications at {{ notifications_url }} +
+ {# TODO: store follow flags on Notifications, otherwise it's impossible to tell why this email is sent #} + {% comment %} + You are receiving this email because you are a moderator subscribed to notification emails. + You are receiving this email because you are subscribed to notifications on this extension. + {% endcomment %} +{% endspaceless %}{% endblock content %} + +{% block footer_links %}{% spaceless %} + Unsubscribe by adjusting your preferences at {{ profile_url }} +{% endspaceless %}{% endblock footer_links %} diff --git a/emails/templates/emails/new_activity_subject.txt b/emails/templates/emails/new_activity_subject.txt new file mode 100644 index 00000000..667a20a0 --- /dev/null +++ b/emails/templates/emails/new_activity_subject.txt @@ -0,0 +1,21 @@ +{% spaceless %}{% load i18n %} +{% with target_type=target.get_type_display what=target|safe name=target.name someone=action.actor verb=action.verb %} +{% if verb == Verb.APPROVED %} + {% blocktrans %}{{ target_type }} approved: "{{ name }}"{% endblocktrans %} +{% elif verb == Verb.COMMENTED %} + {% blocktrans %}New comment on {{ what }}{% endblocktrans %} +{% elif verb == Verb.RATED_EXTENSION %} + {% blocktrans %}{{ target_type }} rated: "{{ name }}"{% endblocktrans %} +{% elif verb == Verb.REPORTED_EXTENSION %} + {% blocktrans %}{{ target_type }} reported: "{{ name }}"{% endblocktrans %} +{% elif verb == Verb.REPORTED_RATING %} + {% blocktrans %}{{ target_type }} rating reported: "{{ name }}"{% endblocktrans %} +{% elif verb == Verb.REQUESTED_CHANGES %} + {% blocktrans %}{{ target_type }} changes requested: "{{ name }}"{% endblocktrans %} +{% elif verb == Verb.REQUESTED_REVIEW %} + {% blocktrans %}{{ target_type }} review requested: "{{ name }}"{% endblocktrans %} +{% else %} + {% blocktrans %}{{ someone }} {{ verb }} on {{ what }}{% endblocktrans %} +{% endif %} +{% endwith %} +{% endspaceless %} diff --git a/emails/util.py b/emails/util.py index 16d991c2..b1e4ed7b 100644 --- a/emails/util.py +++ b/emails/util.py @@ -1,35 +1,17 @@ """Utilities for rendering email templates.""" -from typing import List, Optional, Tuple, Dict, Any +from typing import List, Tuple, Dict, Any import logging from django.conf import settings -from django.contrib.sites.shortcuts import get_current_site -from django.template import loader from django.core.mail import get_connection, EmailMultiAlternatives -from django.urls import reverse +from django.template import loader, TemplateDoesNotExist + +from utils import absolute_url, html_to_text logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) -def _get_site_url(): - domain = get_current_site(None).domain - return f'https://{domain}' - - -def absolute_url( - view_name: str, args: Optional[tuple] = None, kwargs: Optional[dict] = None -) -> str: - """Same as django.urls.reverse() but then as absolute URL. - - For simplicity this assumes HTTPS is used. - """ - from urllib.parse import urljoin - - relative_url = reverse(view_name, args=args, kwargs=kwargs) - return urljoin(_get_site_url(), relative_url) - - def is_noreply(email: str) -> bool: """Return True if the email address is a no-reply address.""" return email.startswith('noreply@') or email.startswith('no-reply@') @@ -38,8 +20,9 @@ def is_noreply(email: str) -> bool: def get_template_context() -> Dict[str, str]: """Return additional context for use in an email template.""" return { - 'site_url': _get_site_url(), - # 'profile_url': absolute_url('profile_update'), + 'site_url': absolute_url('extensions:home'), + 'profile_url': absolute_url('users:my-profile'), + 'notifications_url': absolute_url('notifications:notifications'), 'DEFAULT_REPLY_TO_EMAIL': settings.DEFAULT_REPLY_TO_EMAIL, } @@ -47,8 +30,11 @@ def get_template_context() -> Dict[str, str]: def construct_email(email_name: str, context: Dict[str, Any]) -> Tuple[str, str, str]: """Construct an email message. + If plain text template is not found, text version will be generated from the HTML of the email. + :return: tuple (html, text, subject) """ + context.update(**get_template_context()) base_path = 'emails' subj_tmpl, html_tmpl, txt_tmpl = ( f'{base_path}/{email_name}_subject.txt', @@ -60,7 +46,11 @@ def construct_email(email_name: str, context: Dict[str, Any]) -> Tuple[str, str, context['subject'] = subject.strip() email_body_html = loader.render_to_string(html_tmpl, context) - email_body_txt = loader.render_to_string(txt_tmpl, context) + try: + email_body_txt = loader.render_to_string(txt_tmpl, context) + except TemplateDoesNotExist: + # Generate plain text content from the HTML one + email_body_txt = html_to_text(email_body_html) return email_body_html, email_body_txt, context['subject'] diff --git a/notifications/management/commands/send_notification_emails.py b/notifications/management/commands/send_notification_emails.py index 971a9898..e7f5e7a9 100644 --- a/notifications/management/commands/send_notification_emails.py +++ b/notifications/management/commands/send_notification_emails.py @@ -1,11 +1,10 @@ """Send user notifications as emails, at most once delivery.""" import logging -from django.conf import settings -from django.core.mail import send_mail from django.core.management.base import BaseCommand from django.utils import timezone +from emails.util import construct_and_send_email from notifications.models import Notification logger = logging.getLogger(__name__) @@ -36,12 +35,6 @@ class Command(BaseCommand): def send_notification_email(notification): - # TODO construct a proper phrase, depending on the verb, - # possibly share a template with NotificationsView - subject, message = notification.format_email() - send_mail( - subject, - message, - settings.DEFAULT_FROM_EMAIL, - [notification.recipient.email], - ) + template_name = notification.template_name + context = notification.get_template_context() + construct_and_send_email(template_name, context, recipient_list=[notification.recipient.email]) diff --git a/notifications/models.py b/notifications/models.py index 124d45b4..a0ca69b6 100644 --- a/notifications/models.py +++ b/notifications/models.py @@ -1,11 +1,15 @@ +import logging + from actstream.models import Action from django.contrib.auth import get_user_model from django.db import models +from django.template import loader, TemplateDoesNotExist from constants.activity import Verb from utils import absolutify User = get_user_model() +logger = logging.getLogger(__name__) class Notification(models.Model): @@ -30,12 +34,49 @@ class Notification(models.Model): ] unique_together = ['recipient', 'action'] - def format_email(self): + @property + def original_template_name(self) -> str: + """Template name constructed from action type, target and so on. + + If we want to override email template for this specific notification, + this is the name of the template that should be created. + """ action = self.action - subject = f'New Activity: {action.actor} {action.verb} {action.target}' - url = self.get_absolute_url() - mesage = f'{action.actor} {action.verb} {action.target}: {url}' - return (subject, mesage) + action_object = action.action_object + target_name = action.target.__class__.__name__.lower() + action_object_name = action_object.__class__.__name__.lower() if action_object else '' + return f"new_activity_{action_object_name}_{target_name}_{action.verb.replace(' ', '_')}" + + @property + def template_name(self) -> str: + """Template name to be used for constructing notification email.""" + default_name = 'new_activity' + name = self.original_template_name + args = {'name': name, 'default_name': default_name} + + try: + loader.get_template(f'emails/{name}.html') + except TemplateDoesNotExist: + logger.warning('Template %(name)s does not exist, using %(default_name)s', args) + return default_name + return name + + def get_template_context(self): + """Return template context to be used in the notification email.""" + action = self.action + action_object = action.action_object + quoted_message = getattr(action_object, 'message', getattr(action_object, 'text', '')) + context = { + 'Verb': Verb, + 'action': action, + 'action_object': action_object, + 'notification': self, + 'quoted_message': quoted_message, + 'target': action.target, + 'url': self.get_absolute_url(), + 'user': self.recipient, + } + return context def get_absolute_url(self): if self.action.verb == Verb.RATED_EXTENSION: diff --git a/notifications/tests/test_follow_logic.py b/notifications/tests/test_follow_logic.py index ca381dcb..0bab4142 100644 --- a/notifications/tests/test_follow_logic.py +++ b/notifications/tests/test_follow_logic.py @@ -1,7 +1,10 @@ from pathlib import Path +from django.core import mail +from django.core.management import call_command from django.test import TestCase from django.urls import reverse +from django.utils import timezone from common.tests.factories.extensions import create_approved_version, create_version from common.tests.factories.files import FileFactory @@ -17,7 +20,9 @@ class TestTasks(TestCase): fixtures = ['dev', 'licenses'] def test_ratings(self): - extension = create_approved_version(ratings=[]).extension + extension = create_approved_version( + ratings=[], file__user__confirmed_email_at=timezone.now() + ).extension author = extension.authors.first() notification_nr = Notification.objects.filter(recipient=author).count() some_user = UserFactory() @@ -29,14 +34,35 @@ class TestTasks(TestCase): new_notification_nr = Notification.objects.filter(recipient=author).count() self.assertEqual(new_notification_nr, notification_nr + 1) + # Call the command that sends notification emails + call_command('send_notification_emails') + + # Test that one message has been sent. + self.assertEqual(len(mail.outbox), 1) + + email = mail.outbox[0] + self.assertEqual(email.subject, f'Add-on rated: "{extension.name}"') + self.assertEqual(email.to, [author.email]) + email_text = email.body + self.maxDiff = None + expected_text = expected_rated_text.format(**locals()) + self.assertEqual(email_text, expected_text) + + # Check that most important action and relevant URL are in the HTML version + email_html = email.alternatives[0][0] + self.assertIn(' rated ', email_html) + self.assertIn( + f'https://extensions.local:8111/add-ons/{extension.slug}/reviews/', email_html + ) + def test_abuse(self): extension = create_approved_version(ratings=[]).extension - moderator = create_moderator() + moderator = create_moderator(confirmed_email_at=timezone.now()) notification_nr = Notification.objects.filter(recipient=moderator).count() some_user = UserFactory() self.client.force_login(some_user) url = extension.get_report_url() - self.client.post( + response = self.client.post( url, { 'message': 'test message', @@ -44,11 +70,31 @@ class TestTasks(TestCase): 'version': '', }, ) + self.assertEqual(response.status_code, 302) + report_url = response['Location'] new_notification_nr = Notification.objects.filter(recipient=moderator).count() self.assertEqual(new_notification_nr, notification_nr + 1) + call_command('send_notification_emails') + + # Test that one message has been sent. + self.assertEqual(len(mail.outbox), 1) + + email = mail.outbox[0] + self.assertEqual(email.subject, f'Add-on reported: "{extension.name}"') + self.assertEqual(email.to, [moderator.email]) + email_text = email.body + self.maxDiff = None + expected_text = expected_abuse_report_text.format(**locals()) + self.assertEqual(email_text, expected_text) + + # Check that most important action and relevant URL are in the HTML version + email_html = email.alternatives[0][0] + self.assertIn(report_url, email_html) + self.assertIn(' reported ', email_html) + def test_new_extension_submitted(self): - moderator = create_moderator() + moderator = create_moderator(confirmed_email_at=timezone.now()) notification_nr = Notification.objects.filter(recipient=moderator).count() some_user = UserFactory() file_data = { @@ -117,10 +163,29 @@ class TestTasks(TestCase): new_notification_nr = Notification.objects.filter(recipient=moderator).count() self.assertEqual(new_notification_nr, notification_nr + 1) + call_command('send_notification_emails') + + # Test that one message has been sent. + self.assertEqual(len(mail.outbox), 1) + + email = mail.outbox[0] + extension = file.version.extension + self.assertEqual(email.subject, 'Add-on review requested: "Edit Breakdown"') + self.assertEqual(email.to, [moderator.email]) + email_text = email.body + self.maxDiff = None + expected_text = expected_review_requested_text.format(**locals()) + self.assertEqual(email_text, expected_text) + + # Check that most important action and relevant URL are in the HTML version + email_html = email.alternatives[0][0] + self.assertIn(' requested review of ', email_html) + self.assertIn(f'https://extensions.local:8111/approval-queue/{extension.slug}/', email_html) + def test_approval_queue_activity(self): extension = create_approved_version(ratings=[]).extension author = extension.authors.first() - moderator = create_moderator() + moderator = create_moderator(confirmed_email_at=timezone.now()) some_user = UserFactory() notification_nrs = {} for user in [author, moderator, some_user]: @@ -136,7 +201,88 @@ class TestTasks(TestCase): self.assertEqual(new_notification_nrs[moderator.pk], notification_nrs[moderator.pk] + 1) self.assertEqual(new_notification_nrs[some_user.pk], notification_nrs[some_user.pk] + 1) + call_command('send_notification_emails') + + # Test that one message has been sent (only moderator has email confirmed here). + self.assertEqual(len(mail.outbox), 1) + + email = mail.outbox[0] + self.assertEqual(email.subject, f'New comment on Add-on "{extension.name}"') + email_text = email.body + expected_text = expected_new_comment_text.format(**locals()) + self.maxDiff = None + self.assertEqual(email_text, expected_text) + + # Check that most important action and relevant URL are in the HTML version + email_html = email.alternatives[0][0] + self.assertIn(' commented on ', email_html) + self.assertIn(f'https://extensions.local:8111/approval-queue/{extension.slug}/', email_html) + def _leave_a_comment(self, user, extension, text): self.client.force_login(user) url = reverse('reviewers:approval-comment', args=[extension.slug]) self.client.post(url, {'type': ApprovalActivity.ActivityType.COMMENT, 'message': text}) + + # TODO: test for notifications about a reported rating + # TODO: test for notifications about extension approved by moderators + + +expected_abuse_report_text = """Add-on reported: "{extension.name}" +{some_user.full_name} reported Add-on "{extension.name}" +: + +“test message” + +https://extensions.local:8111{report_url} +Read all notifications at https://extensions.local:8111/notifications/ + + +Unsubscribe by adjusting your preferences at https://extensions.local:8111/settings/profile/ + +https://extensions.local:8111/ +""" + +expected_new_comment_text = """New comment on Add-on "{extension.name}" +{some_user.full_name} commented on Add-on "{extension.name}" +: + +“this is bad” + +https://extensions.local:8111/approval-queue/{extension.slug}/ +Read all notifications at https://extensions.local:8111/notifications/ + + +Unsubscribe by adjusting your preferences at https://extensions.local:8111/settings/profile/ + +https://extensions.local:8111/ +""" + +expected_rated_text = """Add-on rated: "{extension.name}" +{some_user.full_name} rated extension Add-on "{extension.name}" +: + +“rating text” + +https://extensions.local:8111/add-ons/{extension.slug}/reviews/ +Read all notifications at https://extensions.local:8111/notifications/ + + +Unsubscribe by adjusting your preferences at https://extensions.local:8111/settings/profile/ + +https://extensions.local:8111/ +""" + +expected_review_requested_text = """Add-on review requested: "Edit Breakdown" +{some_user.full_name} requested review of Add-on "Edit Breakdown" +: + +“Extension is ready for initial review” + +https://extensions.local:8111/approval-queue/edit-breakdown/ +Read all notifications at https://extensions.local:8111/notifications/ + + +Unsubscribe by adjusting your preferences at https://extensions.local:8111/settings/profile/ + +https://extensions.local:8111/ +""" diff --git a/utils.py b/utils.py index 64008585..e628f63c 100644 --- a/utils.py +++ b/utils.py @@ -1,3 +1,4 @@ +from html.parser import HTMLParser from typing import Optional from urllib.parse import urljoin import datetime @@ -20,6 +21,7 @@ from django.core.exceptions import ValidationError from django.core.validators import validate_ipv46_address from django.http import HttpRequest from django.http.response import HttpResponseRedirectBase +from django.urls import reverse from django.utils.encoding import force_bytes, force_str from django.utils.http import _urlparse import django.utils.text @@ -189,3 +191,50 @@ def absolutify(url: str, request=None) -> str: proto = 'http' if settings.DEBUG else 'https' domain = get_current_site(request).domain return urljoin(f'{proto}://{domain}', url) + + +def absolute_url( + view_name: str, args: Optional[tuple] = None, kwargs: Optional[dict] = None +) -> str: + """Same as django.urls.reverse() but returned as an absolute URL.""" + relative_url = reverse(view_name, args=args, kwargs=kwargs) + return absolutify(relative_url) + + +class HTMLFilter(HTMLParser): + skip_text_of = ('a', 'style') + text = '' + skip_tag_text = False + + def handle_starttag(self, tag, attrs): + if tag in self.skip_text_of: + self.skip_tag_text = True + for name, value in attrs: + if name == 'href': + self.skip_tag_text = True + self.text += value + if tag in ('quote', 'q'): + self.text += '“' + + def handle_endtag(self, tag): + if tag in self.skip_text_of: + self.skip_tag_text = False + if tag in ('quote', 'q'): + self.text += '”\n\n' + + def handle_data(self, data): + if self.skip_tag_text: + return + self.text += data + + +def html_to_text(data: str) -> str: + f = HTMLFilter() + f.feed(data) + lines = [_.lstrip(' \t') for _ in f.text.split('\n')] + skip_empty = 0 + for line in lines: + if not re.match(r'^\s*$', line): + break + skip_empty += 1 + return '\n'.join(lines[skip_empty:])