From 89351cdd19498cc1e8ce6fda671aff5cb2cfc7ea Mon Sep 17 00:00:00 2001 From: Raymond Akornor Date: Fri, 4 Jan 2019 00:50:21 +0000 Subject: [PATCH] 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 | 4 +- zerver/lib/export.py | 1 + zerver/lib/notifications.py | 9 ++-- zerver/lib/send_email.py | 43 ++++++++----------- ...0211_add_users_field_to_scheduled_email.py | 36 ++++++++++++++++ zerver/models.py | 12 ++++-- zerver/tests/test_notifications.py | 18 ++++---- zerver/tests/test_signup.py | 14 +++--- zerver/tests/test_users.py | 31 +++++++++++++ zerver/views/unsubscribe.py | 2 +- 10 files changed, 119 insertions(+), 51 deletions(-) create mode 100644 zerver/migrations/0211_add_users_field_to_scheduled_email.py diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index ea7a94404e..907a96cc85 100644 --- a/zerver/lib/actions.py +++ b/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', diff --git a/zerver/lib/export.py b/zerver/lib/export.py index a4b9bb9b85..9d1bbd1b61 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -107,6 +107,7 @@ ALL_ZULIP_TABLES = { 'zerver_realmfilter', 'zerver_recipient', 'zerver_scheduledemail', + 'zerver_scheduledemail_users', 'zerver_scheduledmessage', 'zerver_service', 'zerver_stream', diff --git a/zerver/lib/notifications.py b/zerver/lib/notifications.py index 917f55f42f..2d2e7cadd1 100644 --- a/zerver/lib/notifications.py +++ b/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 diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index af492c9255..841e7d77f2 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -4,7 +4,7 @@ from django.template import loader 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: diff --git a/zerver/migrations/0211_add_users_field_to_scheduled_email.py b/zerver/migrations/0211_add_users_field_to_scheduled_email.py new file mode 100644 index 0000000000..6750bee9ca --- /dev/null +++ b/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', + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 38bbf5547f..75d2858a23 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -2250,9 +2250,12 @@ class AbstractScheduledJob(models.Model): 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
" 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 "" % (self.type, self.user or self.address, + return "" % (self.type, + self.address or list(self.users.all()), self.scheduled_timestamp) class ScheduledMessage(models.Model): diff --git a/zerver/tests/test_notifications.py b/zerver/tests/test_notifications.py index a77564fa36..dcc9df4d6e 100644 --- a/zerver/tests/test_notifications.py +++ b/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 @@ class TestFollowupEmails(ZulipTestCase): 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 @@ class TestFollowupEmails(ZulipTestCase): AUTH_LDAP_USER_SEARCH=ldap_search): self.login_with_return("newuser@zulip.com", "testing") user = UserProfile.objects.get(email="newuser@zulip.com") - 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 @@ class TestFollowupEmails(ZulipTestCase): self.login_with_return("newuser@zulip.com", "testing") user = UserProfile.objects.get(email="newuser@zulip.com") - 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 @@ class TestFollowupEmails(ZulipTestCase): 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="newuser_email@zulip.com") - 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 @@ class TestFollowupEmails(ZulipTestCase): 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) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 0b1458945a..6b3a889d1b 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -459,7 +459,7 @@ class LoginTest(ZulipTestCase): 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 @@ class InvitationsTestCase(InviteUserBase): # 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,7 +1596,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) - 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") @@ -1604,7 +1604,7 @@ class EmailUnsubscribeTests(ZulipTestCase): # 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 @@ class EmailUnsubscribeTests(ZulipTestCase): 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 @@ class EmailUnsubscribeTests(ZulipTestCase): 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: """ diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index c6ba0dec5f..e7364a4765 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -22,6 +22,7 @@ from zerver.models import UserProfile, Recipient, \ 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 @@ class ActivateTest(ZulipTestCase): 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') diff --git a/zerver/views/unsubscribe.py b/zerver/views/unsubscribe.py index 87545271b7..f55ef86976 100644 --- a/zerver/views/unsubscribe.py +++ b/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)