From aa845e7f60893d9521c0f351a40b09e5c7b6df1d Mon Sep 17 00:00:00 2001 From: Rishi Gupta Date: Sun, 2 Jul 2017 12:10:41 -0700 Subject: [PATCH] models: Replace ScheduledJob with ScheduledEmail. ScheduledJob was written for much more generality than it ended up being used for. Currently it is used by send_future_email, and nothing else. Tailoring the model to emails in particular will make it easier to do things like selectively clear emails when people unsubscribe from particular email types, or seamlessly handle using the same email on multiple realms. --- docs/email.md | 2 +- zerver/lib/actions.py | 2 +- zerver/lib/digest.py | 2 +- zerver/lib/export.py | 4 +- zerver/lib/notifications.py | 4 +- zerver/lib/send_email.py | 14 +++++-- zerver/management/commands/deliver_email.py | 5 +-- zerver/management/commands/export.py | 2 +- .../commands/print_email_delivery_backlog.py | 10 ++--- .../migrations/0092_create_scheduledemail.py | 34 +++++++++++++++++ zerver/models.py | 37 +++++++++++++------ zerver/tests/test_realm.py | 8 ++-- zerver/tests/test_signup.py | 19 ++++------ zerver/tests/test_users.py | 8 ++-- 14 files changed, 100 insertions(+), 51 deletions(-) create mode 100644 zerver/migrations/0092_create_scheduledemail.py diff --git a/docs/email.md b/docs/email.md index 18aa17d8b1..eff9fd1a2f 100644 --- a/docs/email.md +++ b/docs/email.md @@ -22,7 +22,7 @@ with only a few things you need to know to get started. * 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 - `send_future_email`. The `ScheduledJob` entries are eventually processed + `send_future_email`. The `ScheduledEmail` entries are eventually processed by a cron job that runs `zerver/management/commands/deliver_email.py`. * A good way to find a bunch of example email pathways is to `git grep` for `zerver/emails` in the `zerver/` directory. diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 1f46f15642..537e4457e1 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -43,7 +43,7 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity, get_user_profile_by_email, get_user, get_stream_cache_key, \ UserActivityInterval, get_active_user_dicts_in_realm, get_active_streams, \ realm_filters_for_realm, RealmFilter, receives_offline_notifications, \ - ScheduledJob, get_owned_bot_dicts, \ + get_owned_bot_dicts, \ get_old_unclaimed_attachments, get_cross_realm_emails, \ Reaction, EmailChangeStatus, CustomProfileField, \ custom_profile_fields_for_realm, \ diff --git a/zerver/lib/digest.py b/zerver/lib/digest.py index 4adbeeac23..55f077d48d 100644 --- a/zerver/lib/digest.py +++ b/zerver/lib/digest.py @@ -209,7 +209,7 @@ def handle_digest_email(user_profile_id, cutoff): if enough_traffic(context["unread_pms"], context["hot_conversations"], new_streams_count, new_users_count): logger.info("Sending digest email for %s" % (user_profile.email,)) - # Send now, as a ScheduledJob + # Send now, as a ScheduledEmail send_future_email('zerver/emails/digest', to_user_id=user_profile.id, from_name="Zulip Digest", from_address=FromAddress.NOREPLY, context=context) diff --git a/zerver/lib/export.py b/zerver/lib/export.py index d86fcd51a4..007f10b094 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -77,7 +77,7 @@ ALL_ZERVER_TABLES = [ 'zerver_realmemoji', 'zerver_realmfilter', 'zerver_recipient', - 'zerver_scheduledjob', + 'zerver_scheduledemail', 'zerver_stream', 'zerver_subscription', 'zerver_useractivity', @@ -96,7 +96,7 @@ NON_EXPORTED_TABLES = [ 'zerver_preregistrationuser', 'zerver_preregistrationuser_streams', 'zerver_pushdevicetoken', - 'zerver_scheduledjob', + 'zerver_scheduledemail', 'zerver_userprofile_groups', 'zerver_userprofile_user_permissions', ] diff --git a/zerver/lib/notifications.py b/zerver/lib/notifications.py index f898460a31..f1dbaf3ad4 100644 --- a/zerver/lib/notifications.py +++ b/zerver/lib/notifications.py @@ -12,7 +12,7 @@ from zerver.lib.send_email import send_future_email, \ from zerver.lib.queue import queue_json_publish from zerver.models import ( Recipient, - ScheduledJob, + ScheduledEmail, UserMessage, Stream, get_display_recipient, @@ -398,7 +398,7 @@ def clear_followup_emails_queue(email): """ Clear out queued emails that would otherwise be sent to a specific email address. """ - items = ScheduledJob.objects.filter(type=ScheduledJob.EMAIL, filter_string__iexact = email) + items = ScheduledEmail.objects.filter(address__iexact=email) items.delete() def log_digest_event(msg): diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index c67ce86c73..5c30d6c676 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -2,7 +2,8 @@ from django.conf import settings from django.core.mail import EmailMultiAlternatives from django.template import loader from django.utils.timezone import now as timezone_now -from zerver.models import UserProfile, ScheduledJob, get_user_profile_by_id +from zerver.models import UserProfile, ScheduledEmail, get_user_profile_by_id, \ + EMAIL_TYPES import datetime from email.utils import parseaddr, formataddr @@ -59,6 +60,8 @@ def build_email(template_prefix, to_user_id=None, to_email=None, from_name=None, class EmailNotDeliveredException(Exception): pass +# When changing the arguments to this function, you may need to write a +# migration to change or remove any emails in ScheduledEmail. def send_email(template_prefix, to_user_id=None, to_email=None, from_name=None, from_address=None, reply_to_email=None, context={}): # type: (str, Optional[int], Optional[Text], Optional[Text], Optional[Text], Optional[Text], Dict[str, Any]) -> None @@ -82,6 +85,9 @@ def send_future_email(template_prefix, to_user_id=None, to_email=None, from_name to_user_id = None email_fields = {'template_prefix': template_prefix, 'to_user_id': to_user_id, 'to_email': to_email, 'from_name': from_name, 'from_address': from_address, 'context': context} - ScheduledJob.objects.create(type=ScheduledJob.EMAIL, filter_string=parseaddr(to_email)[1], - data=ujson.dumps(email_fields), - scheduled_timestamp=timezone_now() + delay) + template_name = template_prefix.split('/')[-1] + ScheduledEmail.objects.create( + address=parseaddr(to_email)[1], + type=EMAIL_TYPES[template_name], + scheduled_timestamp=timezone_now() + delay, + data=ujson.dumps(email_fields)) diff --git a/zerver/management/commands/deliver_email.py b/zerver/management/commands/deliver_email.py index de6ddc554c..a26b40e05b 100755 --- a/zerver/management/commands/deliver_email.py +++ b/zerver/management/commands/deliver_email.py @@ -15,7 +15,7 @@ from django.core.management.base import BaseCommand from django.utils.timezone import now as timezone_now from django.utils.html import format_html -from zerver.models import ScheduledJob +from zerver.models import ScheduledEmail from zerver.lib.context_managers import lockfile from zerver.lib.send_email import send_email, EmailNotDeliveredException @@ -58,8 +58,7 @@ Usage: ./manage.py deliver_email with lockfile("/tmp/zulip_email_deliver.lockfile"): while True: - email_jobs_to_deliver = ScheduledJob.objects.filter(type=ScheduledJob.EMAIL, - scheduled_timestamp__lte=timezone_now()) + email_jobs_to_deliver = ScheduledEmail.objects.filter(scheduled_timestamp__lte=timezone_now()) if email_jobs_to_deliver: for job in email_jobs_to_deliver: try: diff --git a/zerver/management/commands/export.py b/zerver/management/commands/export.py index e4cbc3226c..cf183c3662 100644 --- a/zerver/management/commands/export.py +++ b/zerver/management/commands/export.py @@ -35,7 +35,7 @@ class Command(BaseCommand): * Sessions (everyone will need to login again post-export) * Users' passwords and API keys (users will need to use SSO or reset password) * Mobile tokens for APNS/GCM (users will need to reconnect their mobile devices) - * ScheduledJob (Not relevant on a new server) + * ScheduledEmail (Not relevant on a new server) * Deployment (Unused) * third_party_api_results cache (this means rerending all old messages could be expensive) diff --git a/zerver/management/commands/print_email_delivery_backlog.py b/zerver/management/commands/print_email_delivery_backlog.py index 6321978a8d..3de7c3105a 100755 --- a/zerver/management/commands/print_email_delivery_backlog.py +++ b/zerver/management/commands/print_email_delivery_backlog.py @@ -1,7 +1,7 @@ #!/usr/bin/env python """ -Shows backlog count of ScheduledJobs of type Email +Shows backlog count of ScheduledEmail """ from __future__ import absolute_import @@ -12,12 +12,12 @@ from django.conf import settings from django.core.management.base import BaseCommand from django.utils.timezone import now as timezone_now -from zerver.models import ScheduledJob +from zerver.models import ScheduledEmail from datetime import datetime, timedelta class Command(BaseCommand): - help = """Shows backlog count of ScheduledJobs of type Email + help = """Shows backlog count of ScheduledEmail (The number of currently overdue (by at least a minute) email jobs) This is run as part of the nagios health check for the deliver_email command. @@ -27,5 +27,5 @@ Usage: ./manage.py print_email_delivery_backlog def handle(self, *args, **options): # type: (*Any, **Any) -> None - print(len(ScheduledJob.objects.filter(type=ScheduledJob.EMAIL, - scheduled_timestamp__lte=timezone_now()-timedelta(minutes=1)))) + print(ScheduledEmail.objects.filter( + scheduled_timestamp__lte=timezone_now()-timedelta(minutes=1)).count()) diff --git a/zerver/migrations/0092_create_scheduledemail.py b/zerver/migrations/0092_create_scheduledemail.py new file mode 100644 index 0000000000..d424d51154 --- /dev/null +++ b/zerver/migrations/0092_create_scheduledemail.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.2 on 2017-07-11 23:41 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0091_realm_allow_edit_history'), + ] + + operations = [ + migrations.CreateModel( + name='ScheduledEmail', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('scheduled_timestamp', models.DateTimeField(db_index=True)), + ('data', models.TextField()), + ('address', models.EmailField(db_index=True, max_length=254, null=True)), + ('type', models.PositiveSmallIntegerField()), + ('user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'abstract': False, + }, + ), + migrations.DeleteModel( + name='ScheduledJob', + ), + ] diff --git a/zerver/models.py b/zerver/models.py index c7d7b1f9e1..31110065bc 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1687,18 +1687,33 @@ class DefaultStream(models.Model): class Meta(object): unique_together = ("realm", "stream") -class ScheduledJob(models.Model): - scheduled_timestamp = models.DateTimeField(auto_now_add=False, null=False) # type: datetime.datetime - type = models.PositiveSmallIntegerField() # type: int - # Valid types are {email} - # for EMAIL, filter_string is recipient_email - EMAIL = 1 - - # JSON representation of the job's data. Be careful, as we are not relying on Django to do validation +class AbstractScheduledJob(models.Model): + scheduled_timestamp = models.DateTimeField(db_index=True) # type: datetime.datetime + # JSON representation of arguments to consumer data = models.TextField() # type: Text - # Kind if like a ForeignKey, but table is determined by type. - filter_id = models.IntegerField(null=True) # type: Optional[int] - filter_string = models.CharField(max_length=100) # type: Text + + class Meta(object): + abstract = True + +class ScheduledEmail(AbstractScheduledJob): + # Exactly one of user or address should be set. These are used to + # filter the set of ScheduledEmails. + user = models.ForeignKey(UserProfile, null=True, on_delete=CASCADE) # type: UserProfile + # Just the address part of a full "name
" email address + address = models.EmailField(null=True, db_index=True) # type: Text + + # Valid types are below + WELCOME = 1 + DIGEST = 2 + INVITATION_REMINDER = 3 + type = models.PositiveSmallIntegerField() # type: int + +EMAIL_TYPES = { + 'followup_day1': ScheduledEmail.WELCOME, + 'followup_day2': ScheduledEmail.WELCOME, + 'digest': ScheduledEmail.DIGEST, + 'invitation_reminder': ScheduledEmail.INVITATION_REMINDER, +} class RealmAuditLog(models.Model): realm = models.ForeignKey(Realm, on_delete=CASCADE) # type: Realm diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 9e46f15280..1fd5d2c636 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -17,7 +17,7 @@ from zerver.lib.actions import ( from zerver.lib.send_email import send_future_email from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import tornado_redirected_to_list -from zerver.models import get_realm, Realm, UserProfile, ScheduledJob +from zerver.models import get_realm, Realm, UserProfile, ScheduledEmail class RealmTest(ZulipTestCase): def assert_user_profile_cache_gets_new_name(self, user_profile, new_realm_name): @@ -144,10 +144,10 @@ class RealmTest(ZulipTestCase): def test_do_deactivate_realm_clears_scheduled_jobs(self): # type: () -> None user = self.example_user('hamlet') - send_future_email('template_prefix', to_user_id=user.id, delay=datetime.timedelta(hours=1)) - self.assertEqual(ScheduledJob.objects.count(), 1) + send_future_email('zerver/emails/followup_day1', to_user_id=user.id, delay=datetime.timedelta(hours=1)) + self.assertEqual(ScheduledEmail.objects.count(), 1) do_deactivate_realm(user.realm) - self.assertEqual(ScheduledJob.objects.count(), 0) + self.assertEqual(ScheduledEmail.objects.count(), 0) def test_do_deactivate_realm_on_deactived_realm(self): # type: () -> None diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 57ffad6ac1..44586f585b 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -27,8 +27,8 @@ from zerver.models import ( get_unique_open_realm, get_unique_non_system_realm, completely_open, get_recipient, PreregistrationUser, Realm, RealmDomain, Recipient, Message, - ScheduledJob, UserProfile, UserMessage, - Stream, Subscription, ScheduledJob, flush_per_request_caches + ScheduledEmail, UserProfile, UserMessage, + Stream, Subscription, flush_per_request_caches ) from zerver.lib.actions import ( set_default_streams, @@ -708,8 +708,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" send_future_email( "zerver/emails/invitation_reminder", to_email=data["email"], from_address=FromAddress.NOREPLY, context=context) - email_jobs_to_deliver = ScheduledJob.objects.filter( - type=ScheduledJob.EMAIL, + email_jobs_to_deliver = ScheduledEmail.objects.filter( scheduled_timestamp__lte=timezone_now()) self.assertEqual(len(email_jobs_to_deliver), 1) email_count = len(outbox) @@ -794,8 +793,7 @@ class EmailUnsubscribeTests(ZulipTestCase): user_profile = self.example_user('hamlet') # Simulate a new user signing up, which enqueues 2 welcome e-mails. enqueue_welcome_emails(user_profile.id) - self.assertEqual(2, len(ScheduledJob.objects.filter( - type=ScheduledJob.EMAIL, filter_string__iexact=email))) + self.assertEqual(2, ScheduledEmail.objects.filter(address__iexact=email).count()) # Simulate unsubscribing from the welcome e-mails. unsubscribe_link = one_click_unsubscribe_link(user_profile, "welcome") @@ -803,8 +801,7 @@ class EmailUnsubscribeTests(ZulipTestCase): # The welcome email jobs are no longer scheduled. self.assertEqual(result.status_code, 200) - self.assertEqual(0, len(ScheduledJob.objects.filter( - type=ScheduledJob.EMAIL, filter_string__iexact=email))) + self.assertEqual(0, ScheduledEmail.objects.filter(address__iexact=email).count()) def test_digest_unsubscribe(self): # type: () -> None @@ -824,8 +821,7 @@ class EmailUnsubscribeTests(ZulipTestCase): 'new_users': [], 'new_streams': {'plain': []}, 'unsubscribe_link': ''} send_future_email('zerver/emails/digest', to_user_id=user_profile.id, context=context) - self.assertEqual(1, len(ScheduledJob.objects.filter( - type=ScheduledJob.EMAIL, filter_string__iexact=email))) + self.assertEqual(1, ScheduledEmail.objects.filter(address__iexact=email).count()) # Simulate unsubscribing from digest e-mails. unsubscribe_link = one_click_unsubscribe_link(user_profile, "digest") @@ -836,8 +832,7 @@ class EmailUnsubscribeTests(ZulipTestCase): # Circumvent user_profile caching. user_profile = UserProfile.objects.get(email=self.example_email("hamlet")) self.assertFalse(user_profile.enable_digest_emails) - self.assertEqual(0, len(ScheduledJob.objects.filter( - type=ScheduledJob.EMAIL, filter_string__iexact=email))) + self.assertEqual(0, ScheduledEmail.objects.filter(address__iexact=email).count()) class RealmCreationTest(ZulipTestCase): diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 97f5a46a39..691ecb7076 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -22,7 +22,7 @@ from zerver.lib.test_runner import slow from zerver.models import UserProfile, Recipient, \ Realm, RealmDomain, UserActivity, \ get_user, get_realm, get_client, get_stream, \ - Message, get_context_for_message, ScheduledJob + Message, get_context_for_message, ScheduledEmail from zerver.lib.avatar import avatar_url from zerver.lib.email_mirror import create_missed_message_address @@ -345,10 +345,10 @@ class ActivateTest(ZulipTestCase): def test_clear_scheduled_jobs(self): # type: () -> None user = self.example_user('hamlet') - send_future_email('template_prefix', to_user_id=user.id, delay=datetime.timedelta(hours=1)) - self.assertEqual(ScheduledJob.objects.count(), 1) + send_future_email('zerver/emails/followup_day1', to_user_id=user.id, delay=datetime.timedelta(hours=1)) + self.assertEqual(ScheduledEmail.objects.count(), 1) do_deactivate_user(user) - self.assertEqual(ScheduledJob.objects.count(), 0) + self.assertEqual(ScheduledEmail.objects.count(), 0) class GetProfileTest(ZulipTestCase):