send_email: Add ScheduledEmail support for multiple recipients.

Follow up on 92dc363. This modifies the ScheduledEmail model
and send_future_email to properly support multiple recipients.

Tweaked by tabbott to add some useful explanatory comments and fix
issues with the migration.

zerver/lib/actions.py

@@ -731,7 +731,7 @@ def do_deactivate_user(user_profile: UserProfile,
     user_profile.save(update_fields=["is_active"])
 
     delete_user_sessions(user_profile)
-    clear_scheduled_emails(user_profile.id)
+    clear_scheduled_emails([user_profile.id])
 
     event_time = timezone_now()
     RealmAuditLog.objects.create(realm=user_profile.realm, modified_user=user_profile,
@@ -3585,7 +3585,7 @@ def do_change_notification_settings(user_profile: UserProfile, name: str, value:
 
     # Disabling digest emails should clear a user's email queue
     if name == 'enable_digest_emails' and not value:
-        clear_scheduled_emails(user_profile.id, ScheduledEmail.DIGEST)
+        clear_scheduled_emails([user_profile.id], ScheduledEmail.DIGEST)
 
     user_profile.save(update_fields=[name])
     event = {'type': 'update_global_notifications',

zerver/lib/export.py

@@ -107,6 +107,7 @@
     'zerver_realmfilter',
     'zerver_recipient',
     'zerver_scheduledemail',
+    'zerver_scheduledemail_users',
     'zerver_scheduledmessage',
     'zerver_service',
     'zerver_stream',

zerver/lib/notifications.py

@@ -464,11 +464,14 @@ def clear_scheduled_invitation_emails(email: str) -> None:
                                           type=ScheduledEmail.INVITATION_REMINDER)
     items.delete()
 
-def clear_scheduled_emails(user_id: int, email_type: Optional[int]=None) -> None:
-    items = ScheduledEmail.objects.filter(user_id=user_id)
+def clear_scheduled_emails(user_ids: List[int], email_type: Optional[int]=None) -> None:
+    items = ScheduledEmail.objects.filter(users__in=user_ids).distinct()
     if email_type is not None:
         items = items.filter(type=email_type)
-    items.delete()
+    for item in items:
+        item.users.remove(*user_ids)
+        if item.users.all().count() == 0:
+            item.delete()
 
 def log_digest_event(msg: str) -> None:
     import logging

zerver/lib/send_email.py

@@ -4,7 +4,7 @@
 from django.utils.timezone import now as timezone_now
 from django.utils.translation import override as override_language
 from django.template.exceptions import TemplateDoesNotExist
-from zerver.models import UserProfile, ScheduledEmail, get_user_profile_by_id, \
+from zerver.models import ScheduledEmail, get_user_profile_by_id, \
     EMAIL_TYPES, Realm
 
 import datetime
@@ -127,16 +127,6 @@ def send_future_email(template_prefix: str, realm: Realm, to_user_ids: Optional[
                       to_emails: Optional[List[str]]=None, from_name: Optional[str]=None,
                       from_address: Optional[str]=None, language: Optional[str]=None,
                       context: Dict[str, Any]={}, delay: datetime.timedelta=datetime.timedelta(0)) -> None:
-    # WARNING: Be careful when using this with multiple recipients;
-    # because the current ScheduledEmail model (used primarily for
-    # cancelling planned emails) does not support multiple recipients,
-    # this is only valid to use for such emails where we don't expect
-    # the cancellation feature to be relevant.
-    #
-    # For that reason, we currently assert that the list of
-    # to_user_ids/to_emails is 1 below, but in theory that could be
-    # changed as long as the callers are in a situation where the
-    # above problem is not relevant.
     template_name = template_prefix.split('/')[-1]
     email_fields = {'template_prefix': template_prefix, 'to_user_ids': to_user_ids, 'to_emails': to_emails,
                     'from_name': from_name, 'from_address': from_address, 'language': language,
@@ -148,23 +138,26 @@ def send_future_email(template_prefix: str, realm: Realm, to_user_ids: Optional[
         # For logging the email
 
     assert (to_user_ids is None) ^ (to_emails is None)
-    if to_user_ids is not None:
-        # The realm is redundant if we have a to_user_id; this assert just
-        # expresses that fact
-        assert(len(to_user_ids) == 1)
-        assert(UserProfile.objects.filter(id__in=to_user_ids, realm=realm).exists())
-        to_field = {'user_id': to_user_ids[0]}  # type: Dict[str, Any]
-    else:
-        assert to_emails is not None
-        assert(len(to_emails) == 1)
-        to_field = {'address': parseaddr(to_emails[0])[1]}
-
-    ScheduledEmail.objects.create(
+    email = ScheduledEmail.objects.create(
         type=EMAIL_TYPES[template_name],
         scheduled_timestamp=timezone_now() + delay,
         realm=realm,
-        data=ujson.dumps(email_fields),
-        **to_field)
+        data=ujson.dumps(email_fields))
+
+    # In order to ensure the efficiency of clear_scheduled_emails, we
+    # need to duplicate the recipient data in the ScheduledEmail model
+    # itself.
+    try:
+        if to_user_ids is not None:
+            email.users.add(*to_user_ids)
+        else:
+            assert to_emails is not None
+            assert(len(to_emails) == 1)
+            email.address = parseaddr(to_emails[0])[1]
+        email.save()
+    except Exception as e:
+        email.delete()
+        raise e
 
 def send_email_to_admins(template_prefix: str, realm: Realm, from_name: Optional[str]=None,
                          from_address: Optional[str]=None, context: Dict[str, Any]={}) -> None:

zerver/migrations/0211_add_users_field_to_scheduled_email.py

@@ -0,0 +1,36 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.20 on 2019-03-14 01:11
+from __future__ import unicode_literals
+
+from django.conf import settings
+from django.db import migrations, models
+from django.db.backends.postgresql_psycopg2.schema import DatabaseSchemaEditor
+from django.db.migrations.state import StateApps
+
+def set_users_for_existing_scheduledemails(
+        apps: StateApps, schema_editor: DatabaseSchemaEditor) -> None:
+    ScheduledEmail = apps.get_model("zerver", "ScheduledEmail")
+    for email in ScheduledEmail.objects.all():
+        if email.user is not None:
+            email.users.add(email.user)
+        email.save()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zerver', '0210_stream_first_message_id'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='scheduledemail',
+            name='users',
+            field=models.ManyToManyField(to=settings.AUTH_USER_MODEL),
+        ),
+        migrations.RunPython(set_users_for_existing_scheduledemails, reverse_code=migrations.RunPython.noop),
+        migrations.RemoveField(
+            model_name='scheduledemail',
+            name='user',
+        ),
+    ]

zerver/models.py

@@ -2250,9 +2250,12 @@ class Meta:
         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: Optional[UserProfile]
+    # Exactly one of users or address should be set. These are
+    # duplicate values, used to efficiently filter the set of
+    # ScheduledEmails for use in clear_scheduled_emails; the
+    # recipients used for actually sending messages are stored in the
+    # data field of AbstractScheduledJob.
+    users = models.ManyToManyField(UserProfile)  # type: Manager
     # Just the address part of a full "name <address>" email address
     address = models.EmailField(null=True, db_index=True)  # type: Optional[str]
 
@@ -2263,7 +2266,8 @@ class ScheduledEmail(AbstractScheduledJob):
     type = models.PositiveSmallIntegerField()  # type: int
 
     def __str__(self) -> str:
-        return "<ScheduledEmail: %s %s %s>" % (self.type, self.user or self.address,
+        return "<ScheduledEmail: %s %s %s>" % (self.type,
+                                               self.address or list(self.users.all()),
                                                self.scheduled_timestamp)
 
 class ScheduledMessage(models.Model):

zerver/tests/test_notifications.py

@@ -28,7 +28,7 @@ class TestFollowupEmails(ZulipTestCase):
     def test_day1_email_context(self) -> None:
         hamlet = self.example_user("hamlet")
         enqueue_welcome_emails(hamlet)
-        scheduled_emails = ScheduledEmail.objects.filter(user=hamlet)
+        scheduled_emails = ScheduledEmail.objects.filter(users=hamlet)
         email_data = ujson.loads(scheduled_emails[0].data)
         self.assertEqual(email_data["context"]["email"], self.example_email("hamlet"))
         self.assertEqual(email_data["context"]["is_realm_admin"], False)
@@ -39,7 +39,7 @@ def test_day1_email_context(self) -> None:
 
         iago = self.example_user("iago")
         enqueue_welcome_emails(iago)
-        scheduled_emails = ScheduledEmail.objects.filter(user=iago)
+        scheduled_emails = ScheduledEmail.objects.filter(users=iago)
         email_data = ujson.loads(scheduled_emails[0].data)
         self.assertEqual(email_data["context"]["email"], self.example_email("iago"))
         self.assertEqual(email_data["context"]["is_realm_admin"], True)
@@ -74,7 +74,7 @@ def test_day1_email_ldap_case_a_login_credentials(self) -> None:
                 AUTH_LDAP_USER_SEARCH=ldap_search):
             self.login_with_return("[email protected]", "testing")
             user = UserProfile.objects.get(email="[email protected]")
-            scheduled_emails = ScheduledEmail.objects.filter(user=user)
+            scheduled_emails = ScheduledEmail.objects.filter(users=user)
 
             self.assertEqual(len(scheduled_emails), 2)
             email_data = ujson.loads(scheduled_emails[0].data)
@@ -107,7 +107,7 @@ def test_day1_email_ldap_case_b_login_credentials(self) -> None:
             self.login_with_return("[email protected]", "testing")
 
             user = UserProfile.objects.get(email="[email protected]")
-            scheduled_emails = ScheduledEmail.objects.filter(user=user)
+            scheduled_emails = ScheduledEmail.objects.filter(users=user)
 
             self.assertEqual(len(scheduled_emails), 2)
             email_data = ujson.loads(scheduled_emails[0].data)
@@ -139,7 +139,7 @@ def test_day1_email_ldap_case_c_login_credentials(self) -> None:
                 AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
             self.login_with_return("newuser", "testing")
             user = UserProfile.objects.get(email="[email protected]")
-            scheduled_emails = ScheduledEmail.objects.filter(user=user)
+            scheduled_emails = ScheduledEmail.objects.filter(users=user)
 
             self.assertEqual(len(scheduled_emails), 2)
             email_data = ujson.loads(scheduled_emails[0].data)
@@ -152,15 +152,15 @@ def test_followup_emails_count(self) -> None:
 
         enqueue_welcome_emails(self.example_user("hamlet"))
         # Hamlet has account only in Zulip realm so both day1 and day2 emails should be sent
-        scheduled_emails = ScheduledEmail.objects.filter(user=hamlet)
+        scheduled_emails = ScheduledEmail.objects.filter(users=hamlet)
         self.assertEqual(2, len(scheduled_emails))
-        self.assertEqual(ujson.loads(scheduled_emails[0].data)["template_prefix"], 'zerver/emails/followup_day2')
-        self.assertEqual(ujson.loads(scheduled_emails[1].data)["template_prefix"], 'zerver/emails/followup_day1')
+        self.assertEqual(ujson.loads(scheduled_emails[1].data)["template_prefix"], 'zerver/emails/followup_day2')
+        self.assertEqual(ujson.loads(scheduled_emails[0].data)["template_prefix"], 'zerver/emails/followup_day1')
 
         ScheduledEmail.objects.all().delete()
 
         enqueue_welcome_emails(cordelia)
-        scheduled_emails = ScheduledEmail.objects.filter(user=cordelia)
+        scheduled_emails = ScheduledEmail.objects.filter(users=cordelia)
         # Cordelia has account in more than 1 realm so day2 email should not be sent
         self.assertEqual(len(scheduled_emails), 1)
         email_data = ujson.loads(scheduled_emails[0].data)

zerver/tests/test_signup.py

@@ -459,7 +459,7 @@ def test_register(self) -> None:
         with queries_captured() as queries:
             self.register(self.nonreg_email('test'), "test")
         # Ensure the number of queries we make is not O(streams)
-        self.assert_length(queries, 74)
+        self.assert_length(queries, 78)
         user_profile = self.nonreg_user('test')
         self.assertEqual(get_session_dict_user(self.client.session), user_profile.id)
         self.assertFalse(user_profile.enable_stream_desktop_notifications)
@@ -1354,14 +1354,14 @@ def test_successful_resend_invitation(self) -> None:
 
         # Verify that the scheduled email exists.
         scheduledemail_filter = ScheduledEmail.objects.filter(
-            address=invitee, type=ScheduledEmail.INVITATION_REMINDER)
+            address__iexact=invitee, type=ScheduledEmail.INVITATION_REMINDER)
         self.assertEqual(scheduledemail_filter.count(), 1)
         original_timestamp = scheduledemail_filter.values_list('scheduled_timestamp', flat=True)
 
         # Resend invite
         result = self.client_post('/json/invites/' + str(prereg_user.id) + '/resend')
         self.assertEqual(ScheduledEmail.objects.filter(
-            address=invitee, type=ScheduledEmail.INVITATION_REMINDER).count(), 1)
+            address__iexact=invitee, type=ScheduledEmail.INVITATION_REMINDER).count(), 1)
 
         # Check that we have exactly one scheduled email, and that it is different
         self.assertEqual(scheduledemail_filter.count(), 1)
@@ -1596,15 +1596,15 @@ def test_welcome_unsubscribe(self) -> None:
         user_profile = self.example_user('hamlet')
         # Simulate a new user signing up, which enqueues 2 welcome e-mails.
         enqueue_welcome_emails(user_profile)
-        self.assertEqual(2, ScheduledEmail.objects.filter(user=user_profile).count())
+        self.assertEqual(2, ScheduledEmail.objects.filter(users=user_profile).count())
 
         # Simulate unsubscribing from the welcome e-mails.
         unsubscribe_link = one_click_unsubscribe_link(user_profile, "welcome")
         result = self.client_get(urllib.parse.urlparse(unsubscribe_link).path)
 
         # The welcome email jobs are no longer scheduled.
         self.assertEqual(result.status_code, 200)
-        self.assertEqual(0, ScheduledEmail.objects.filter(user=user_profile).count())
+        self.assertEqual(0, ScheduledEmail.objects.filter(users=user_profile).count())
 
     def test_digest_unsubscribe(self) -> None:
         """
@@ -1623,7 +1623,7 @@ def test_digest_unsubscribe(self) -> None:
         send_future_email('zerver/emails/digest', user_profile.realm,
                           to_user_ids=[user_profile.id], context=context)
 
-        self.assertEqual(1, ScheduledEmail.objects.filter(user=user_profile).count())
+        self.assertEqual(1, ScheduledEmail.objects.filter(users=user_profile).count())
 
         # Simulate unsubscribing from digest e-mails.
         unsubscribe_link = one_click_unsubscribe_link(user_profile, "digest")
@@ -1635,7 +1635,7 @@ def test_digest_unsubscribe(self) -> None:
 
         user_profile.refresh_from_db()
         self.assertFalse(user_profile.enable_digest_emails)
-        self.assertEqual(0, ScheduledEmail.objects.filter(user=user_profile).count())
+        self.assertEqual(0, ScheduledEmail.objects.filter(users=user_profile).count())
 
     def test_login_unsubscribe(self) -> None:
         """

zerver/tests/test_users.py

@@ -22,6 +22,7 @@
 from zerver.lib.avatar import avatar_url
 from zerver.lib.exceptions import JsonableError
 from zerver.lib.send_email import send_future_email
+from zerver.lib.notifications import clear_scheduled_emails
 from zerver.lib.actions import (
     get_emails_from_user_ids,
     get_recipient_info,
@@ -868,6 +869,36 @@ def test_clear_scheduled_jobs(self) -> None:
         do_deactivate_user(user)
         self.assertEqual(ScheduledEmail.objects.count(), 0)
 
+    def test_send_future_email_with_multiple_recipients(self) -> None:
+        hamlet = self.example_user('hamlet')
+        iago = self.example_user('iago')
+        send_future_email('zerver/emails/followup_day1', iago.realm,
+                          to_user_ids=[hamlet.id, iago.id], delay=datetime.timedelta(hours=1))
+        self.assertEqual(ScheduledEmail.objects.filter(users__in=[hamlet, iago]).distinct().count(), 1)
+        email = ScheduledEmail.objects.all().first()
+        self.assertEqual(email.users.count(), 2)
+
+    def test_clear_scheduled_emails_with_multiple_user_ids(self) -> None:
+        hamlet = self.example_user('hamlet')
+        iago = self.example_user('iago')
+        send_future_email('zerver/emails/followup_day1', iago.realm,
+                          to_user_ids=[hamlet.id, iago.id], delay=datetime.timedelta(hours=1))
+        self.assertEqual(ScheduledEmail.objects.count(), 1)
+        clear_scheduled_emails([hamlet.id, iago.id])
+        self.assertEqual(ScheduledEmail.objects.count(), 0)
+
+    def test_clear_schedule_emails_with_one_user_id(self) -> None:
+        hamlet = self.example_user('hamlet')
+        iago = self.example_user('iago')
+        send_future_email('zerver/emails/followup_day1', iago.realm,
+                          to_user_ids=[hamlet.id, iago.id], delay=datetime.timedelta(hours=1))
+        self.assertEqual(ScheduledEmail.objects.count(), 1)
+        clear_scheduled_emails([hamlet.id])
+        self.assertEqual(ScheduledEmail.objects.count(), 1)
+        self.assertEqual(ScheduledEmail.objects.filter(users=hamlet).count(), 0)
+        self.assertEqual(ScheduledEmail.objects.filter(users=iago).count(), 1)
+
+
 class RecipientInfoTest(ZulipTestCase):
     def test_stream_recipient_info(self) -> None:
         hamlet = self.example_user('hamlet')

zerver/views/unsubscribe.py

@@ -27,7 +27,7 @@ def do_missedmessage_unsubscribe(user_profile: UserProfile) -> None:
     do_change_notification_settings(user_profile, 'enable_offline_email_notifications', False)
 
 def do_welcome_unsubscribe(user_profile: UserProfile) -> None:
-    clear_scheduled_emails(user_profile.id, ScheduledEmail.WELCOME)
+    clear_scheduled_emails([user_profile.id], ScheduledEmail.WELCOME)
 
 def do_digest_unsubscribe(user_profile: UserProfile) -> None:
     do_change_notification_settings(user_profile, 'enable_digest_emails', False)