From 92dc3637df07829ed1a63d4e007452edbae0b4b5 Mon Sep 17 00:00:00 2001 From: Raymond Akornor Date: Mon, 3 Dec 2018 22:26:51 +0000 Subject: [PATCH] send_email: Add support for multiple recipients. This adds a function that sends provided email to all administrators of a realm, but in a single email. As a result, send_email now takes arguments to_user_ids and to_emails instead of to_user_id and to_email. We adjust other APIs to match, but note that send_future_email does not yet support the multiple recipients model for good reasons. Tweaked by tabbott to modify `manage.py deliver_email` to handle backwards-compatibily for any ScheduledEmail objects already in the database. Fixes #10896. --- zerver/forms.py | 4 +- zerver/lib/actions.py | 16 +++-- zerver/lib/digest.py | 2 +- zerver/lib/notifications.py | 6 +- zerver/lib/send_email.py | 60 ++++++++++++------- zerver/management/commands/deliver_email.py | 8 +++ .../commands/send_password_reset_email.py | 2 +- zerver/signals.py | 2 +- zerver/tests/test_digest.py | 6 +- zerver/tests/test_queue_worker.py | 2 +- zerver/tests/test_realm.py | 2 +- zerver/tests/test_signup.py | 5 +- zerver/tests/test_users.py | 2 +- zerver/views/registration.py | 4 +- zerver/views/user_settings.py | 2 +- zerver/worker/queue_processors.py | 2 +- 16 files changed, 76 insertions(+), 49 deletions(-) diff --git a/zerver/forms.py b/zerver/forms.py index da54f1386f..185271efbc 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -250,7 +250,7 @@ class ZulipPasswordResetForm(PasswordResetForm): if user is not None: context['active_account_in_realm'] = True context['reset_url'] = generate_password_reset_url(user, token_generator) - send_email('zerver/emails/password_reset', to_user_id=user.id, + send_email('zerver/emails/password_reset', to_user_ids=[user.id], from_name="Zulip Account Security", from_address=FromAddress.tokenized_no_reply_address(), context=context) @@ -259,7 +259,7 @@ class ZulipPasswordResetForm(PasswordResetForm): active_accounts_in_other_realms = UserProfile.objects.filter(email__iexact=email, is_active=True) if active_accounts_in_other_realms: context['active_accounts_in_other_realms'] = active_accounts_in_other_realms - send_email('zerver/emails/password_reset', to_email=email, + send_email('zerver/emails/password_reset', to_emails=[email], from_name="Zulip Account Security", from_address=FromAddress.tokenized_no_reply_address(), context=context) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index a37d857b49..ae22560727 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -43,7 +43,7 @@ from zerver.lib.message import ( ) from zerver.lib.realm_icon import realm_icon_url from zerver.lib.retention import move_messages_to_archive -from zerver.lib.send_email import send_email, FromAddress +from zerver.lib.send_email import send_email, FromAddress, send_email_to_admins from zerver.lib.stream_subscription import ( get_active_subscriptions_for_stream_id, get_active_subscriptions_for_stream_ids, @@ -795,7 +795,7 @@ def do_start_email_change_process(user_profile: UserProfile, new_email: str) -> 'new_email': new_email, 'activate_url': activation_url }) - send_email('zerver/emails/confirm_new_email', to_email=new_email, + send_email('zerver/emails/confirm_new_email', to_emails=[new_email], from_name='Zulip Account Security', from_address=FromAddress.tokenized_no_reply_address(), context=context) @@ -4429,7 +4429,7 @@ def do_send_confirmation_email(invitee: PreregistrationUser, context = {'referrer_full_name': referrer.full_name, 'referrer_email': referrer.email, 'activate_url': activation_url, 'referrer_realm_name': referrer.realm.name} from_name = "%s (via Zulip)" % (referrer.full_name,) - send_email('zerver/emails/invitation', to_email=invitee.email, from_name=from_name, + send_email('zerver/emails/invitation', to_emails=[invitee.email], from_name=from_name, from_address=FromAddress.tokenized_no_reply_address(), context=context) def email_not_system_bot(email: str) -> None: @@ -5185,12 +5185,10 @@ def missing_any_realm_internal_bots() -> bool: def do_send_realm_reactivation_email(realm: Realm) -> None: confirmation_url = create_confirmation_link(realm, realm.host, Confirmation.REALM_REACTIVATION) - admins = realm.get_admin_users() context = {'confirmation_url': confirmation_url, 'realm_uri': realm.uri, 'realm_name': realm.name} - for admin in admins: - send_email( - 'zerver/emails/realm_reactivation', to_email=admin.email, - from_address=FromAddress.tokenized_no_reply_address(), - from_name="Zulip Account Security", context=context) + send_email_to_admins( + 'zerver/emails/realm_reactivation', realm, + from_address=FromAddress.tokenized_no_reply_address(), + from_name="Zulip Account Security", context=context) diff --git a/zerver/lib/digest.py b/zerver/lib/digest.py index 6c63c33c88..2c3e263753 100644 --- a/zerver/lib/digest.py +++ b/zerver/lib/digest.py @@ -262,6 +262,6 @@ def handle_digest_email(user_profile_id: int, cutoff: float) -> None: new_streams_count, new_users_count): logger.info("Sending digest email for %s" % (user_profile.email,)) # Send now, as a ScheduledEmail - send_future_email('zerver/emails/digest', user_profile.realm, to_user_id=user_profile.id, + send_future_email('zerver/emails/digest', user_profile.realm, to_user_ids=[user_profile.id], from_name="Zulip Digest", from_address=FromAddress.NOREPLY, context=context) diff --git a/zerver/lib/notifications.py b/zerver/lib/notifications.py index 178635cfbf..c14d0a20f8 100644 --- a/zerver/lib/notifications.py +++ b/zerver/lib/notifications.py @@ -382,7 +382,7 @@ def do_send_missedmessage_events_reply_in_zulip(user_profile: UserProfile, email_dict = { 'template_prefix': 'zerver/emails/missed_message', - 'to_user_id': user_profile.id, + 'to_user_ids': [user_profile.id], 'from_name': from_name, 'from_address': from_address, 'reply_to_email': formataddr((reply_to_name, reply_to_address)), @@ -534,12 +534,12 @@ def enqueue_welcome_emails(user: UserProfile, realm_creation: bool=False) -> Non context["ldap_username"] = user.email send_future_email( - "zerver/emails/followup_day1", user.realm, to_user_id=user.id, from_name=from_name, + "zerver/emails/followup_day1", user.realm, to_user_ids=[user.id], from_name=from_name, from_address=from_address, context=context) if other_account_count == 0: send_future_email( - "zerver/emails/followup_day2", user.realm, to_user_id=user.id, from_name=from_name, + "zerver/emails/followup_day2", user.realm, to_user_ids=[user.id], from_name=from_name, from_address=from_address, context=context, delay=followup_day2_email_delay(user)) def convert_html_to_markdown(html: str) -> str: diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py index 69c02051ce..c105fe208f 100644 --- a/zerver/lib/send_email.py +++ b/zerver/lib/send_email.py @@ -33,17 +33,17 @@ class FromAddress: return parseaddr(settings.TOKENIZED_NOREPLY_EMAIL_ADDRESS)[1].format(token=generate_key()) return FromAddress.NOREPLY -def build_email(template_prefix: str, to_user_id: Optional[int]=None, - to_email: Optional[str]=None, from_name: Optional[str]=None, +def build_email(template_prefix: str, to_user_ids: Optional[List[int]]=None, + to_emails: Optional[List[str]]=None, from_name: Optional[str]=None, from_address: Optional[str]=None, reply_to_email: Optional[str]=None, context: Optional[Dict[str, Any]]=None) -> EmailMultiAlternatives: # Callers should pass exactly one of to_user_id and to_email. - assert (to_user_id is None) ^ (to_email is None) - if to_user_id is not None: - to_user = get_user_profile_by_id(to_user_id) + assert (to_user_ids is None) ^ (to_emails is None) + if to_user_ids is not None: + to_users = [get_user_profile_by_id(to_user_id) for to_user_id in to_user_ids] # Change to formataddr((to_user.full_name, to_user.email)) once # https://github.com/zulip/zulip/issues/4676 is resolved - to_email = to_user.delivery_email + to_emails = [to_user.delivery_email for to_user in to_users] if context is None: context = {} @@ -81,7 +81,7 @@ def build_email(template_prefix: str, to_user_id: Optional[int]=None, elif from_address == FromAddress.NOREPLY: reply_to = [FromAddress.NOREPLY] - mail = EmailMultiAlternatives(subject, message, from_email, [to_email], reply_to=reply_to) + mail = EmailMultiAlternatives(subject, message, from_email, to_emails, reply_to=reply_to) if html_message is not None: mail.attach_alternative(html_message, 'text/html') return mail @@ -91,10 +91,11 @@ class EmailNotDeliveredException(Exception): # 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: str, to_user_id: Optional[int]=None, to_email: Optional[str]=None, - from_name: Optional[str]=None, from_address: Optional[str]=None, - reply_to_email: Optional[str]=None, context: Dict[str, Any]={}) -> None: - mail = build_email(template_prefix, to_user_id=to_user_id, to_email=to_email, from_name=from_name, +def send_email(template_prefix: str, to_user_ids: Optional[List[int]]=None, + to_emails: Optional[List[str]]=None, from_name: Optional[str]=None, + from_address: Optional[str]=None, reply_to_email: Optional[str]=None, + context: Dict[str, Any]={}) -> None: + mail = build_email(template_prefix, to_user_ids=to_user_ids, to_emails=to_emails, from_name=from_name, from_address=from_address, reply_to_email=reply_to_email, context=context) template = template_prefix.split("/")[-1] logger.info("Sending %s email to %s" % (template, mail.to)) @@ -106,27 +107,39 @@ def send_email(template_prefix: str, to_user_id: Optional[int]=None, to_email: O def send_email_from_dict(email_dict: Mapping[str, Any]) -> None: send_email(**dict(email_dict)) -def send_future_email(template_prefix: str, realm: Realm, to_user_id: Optional[int]=None, - to_email: Optional[str]=None, from_name: Optional[str]=None, +def send_future_email(template_prefix: str, realm: Realm, to_user_ids: Optional[List[int]]=None, + to_emails: Optional[List[str]]=None, from_name: Optional[str]=None, from_address: 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_id': to_user_id, 'to_email': to_email, + email_fields = {'template_prefix': template_prefix, 'to_user_ids': to_user_ids, 'to_emails': to_emails, 'from_name': from_name, 'from_address': from_address, 'context': context} if settings.DEVELOPMENT and not settings.TEST_SUITE: - send_email(template_prefix, to_user_id=to_user_id, to_email=to_email, from_name=from_name, + send_email(template_prefix, to_user_ids=to_user_ids, to_emails=to_emails, from_name=from_name, from_address=from_address, context=context) # For logging the email - assert (to_user_id is None) ^ (to_email is None) - if to_user_id is not None: + 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(UserProfile.objects.filter(id=to_user_id, realm=realm).exists()) - to_field = {'user_id': to_user_id} # type: Dict[str, Any] + 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: - to_field = {'address': parseaddr(to_email)[1]} + assert(len(to_emails) == 1) + to_field = {'address': parseaddr(to_emails[0])[1]} ScheduledEmail.objects.create( type=EMAIL_TYPES[template_name], @@ -134,3 +147,10 @@ def send_future_email(template_prefix: str, realm: Realm, to_user_id: Optional[i realm=realm, data=ujson.dumps(email_fields), **to_field) + +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: + admins = realm.get_admin_users() + admin_user_ids = [admin.id for admin in admins] + send_email(template_prefix, to_user_ids=admin_user_ids, from_name=from_name, + from_address=from_address, context=context) diff --git a/zerver/management/commands/deliver_email.py b/zerver/management/commands/deliver_email.py index 28a196e779..f249d3b441 100644 --- a/zerver/management/commands/deliver_email.py +++ b/zerver/management/commands/deliver_email.py @@ -48,6 +48,14 @@ Usage: ./manage.py deliver_email scheduled_timestamp__lte=timezone_now()) if email_jobs_to_deliver: for job in email_jobs_to_deliver: + # Reformat any jobs that used the old to_email + # and to_user_ids argument formats. + if 'to_email' in job: + job['to_emails'] = [job['to_email']] + del job['to_email'] + if 'to_user_ids' in job: + job['to_user_ids'] = [job['to_user_id']] + del job['to_user_ids'] try: send_email(**loads(job.data)) job.delete() diff --git a/zerver/management/commands/send_password_reset_email.py b/zerver/management/commands/send_password_reset_email.py index e88599c0dd..f78f833d9a 100644 --- a/zerver/management/commands/send_password_reset_email.py +++ b/zerver/management/commands/send_password_reset_email.py @@ -47,6 +47,6 @@ class Command(ZulipBaseCommand): 'realm_uri': user_profile.realm.uri, 'active_account_in_realm': True, } - send_email('zerver/emails/password_reset', to_user_id=user_profile.id, + send_email('zerver/emails/password_reset', to_user_ids=[user_profile.id], from_address=FromAddress.tokenized_no_reply_address(), from_name="Zulip Account Security", context=context) diff --git a/zerver/signals.py b/zerver/signals.py index 8a2b9a52cb..010befd404 100644 --- a/zerver/signals.py +++ b/zerver/signals.py @@ -96,7 +96,7 @@ def email_on_new_login(sender: Any, user: UserProfile, request: Any, **kwargs: A email_dict = { 'template_prefix': 'zerver/emails/notify_new_login', - 'to_user_id': user.id, + 'to_user_ids': [user.id], 'from_name': 'Zulip Account Security', 'from_address': FromAddress.NOREPLY, 'context': context} diff --git a/zerver/tests/test_digest.py b/zerver/tests/test_digest.py index a715aae01c..83ad3198fa 100644 --- a/zerver/tests/test_digest.py +++ b/zerver/tests/test_digest.py @@ -40,7 +40,7 @@ class TestDigestEmailMessages(ZulipTestCase): self.assertEqual(mock_send_future_email.call_count, 1) kwargs = mock_send_future_email.call_args[1] - self.assertEqual(kwargs['to_user_id'], user_profile.id) + self.assertEqual(kwargs['to_user_ids'], [user_profile.id]) html = kwargs['context']['unread_pms'][0]['header']['html'] expected_url = "'http://zulip.testserver/#narrow/pm-with/{id}-hamlet'".format(id=hamlet.id) self.assertIn(expected_url, html) @@ -74,7 +74,7 @@ class TestDigestEmailMessages(ZulipTestCase): self.assertEqual(mock_send_future_email.call_count, 1) kwargs = mock_send_future_email.call_args[1] - self.assertEqual(kwargs['to_user_id'], user_profile.id) + self.assertEqual(kwargs['to_user_ids'], [user_profile.id]) html = kwargs['context']['unread_pms'][0]['header']['html'] other_user_ids = sorted([ @@ -125,7 +125,7 @@ class TestDigestEmailMessages(ZulipTestCase): self.assertEqual(mock_send_future_email.call_count, 1) kwargs = mock_send_future_email.call_args[1] - self.assertEqual(kwargs['to_user_id'], othello.id) + self.assertEqual(kwargs['to_user_ids'], [othello.id]) hot_convo = kwargs['context']['hot_conversations'][0] diff --git a/zerver/tests/test_queue_worker.py b/zerver/tests/test_queue_worker.py index bfb5df063b..0a57a9c76d 100644 --- a/zerver/tests/test_queue_worker.py +++ b/zerver/tests/test_queue_worker.py @@ -248,7 +248,7 @@ class WorkerTest(ZulipTestCase): data = { 'template_prefix': 'zerver/emails/confirm_new_email', - 'to_email': self.example_email("hamlet"), + 'to_emails': [self.example_email("hamlet")], 'from_name': 'Zulip Account Security', 'from_address': FromAddress.NOREPLY, 'context': {} diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index dc30235ae6..e2cc086d24 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -182,7 +182,7 @@ class RealmTest(ZulipTestCase): def test_do_deactivate_realm_clears_scheduled_jobs(self) -> None: user = self.example_user('hamlet') send_future_email('zerver/emails/followup_day1', user.realm, - to_user_id=user.id, delay=datetime.timedelta(hours=1)) + to_user_ids=[user.id], delay=datetime.timedelta(hours=1)) self.assertEqual(ScheduledEmail.objects.count(), 1) do_deactivate_realm(user.realm) self.assertEqual(ScheduledEmail.objects.count(), 0) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 4a00096190..78da8be357 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -1159,8 +1159,9 @@ so we didn't send them an invitation. We did send invitations to everyone else!" 'referrer_realm_name': referrer.realm.name, }) with self.settings(EMAIL_BACKEND='django.core.mail.backends.console.EmailBackend'): + email = data["email"] send_future_email( - "zerver/emails/invitation_reminder", referrer.realm, to_email=data["email"], + "zerver/emails/invitation_reminder", referrer.realm, to_emails=[email], from_address=FromAddress.NOREPLY, context=context) email_jobs_to_deliver = ScheduledEmail.objects.filter( scheduled_timestamp__lte=timezone_now()) @@ -1553,7 +1554,7 @@ class EmailUnsubscribeTests(ZulipTestCase): context = {'name': '', 'realm_uri': '', 'unread_pms': [], 'hot_conversations': [], 'new_users': [], 'new_streams': {'plain': []}, 'unsubscribe_link': ''} send_future_email('zerver/emails/digest', user_profile.realm, - to_user_id=user_profile.id, context=context) + to_user_ids=[user_profile.id], context=context) self.assertEqual(1, ScheduledEmail.objects.filter(user=user_profile).count()) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 0574e2ba9c..d4e57c509d 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -810,7 +810,7 @@ class ActivateTest(ZulipTestCase): def test_clear_scheduled_jobs(self) -> None: user = self.example_user('hamlet') send_future_email('zerver/emails/followup_day1', user.realm, - to_user_id=user.id, delay=datetime.timedelta(hours=1)) + to_user_ids=[user.id], delay=datetime.timedelta(hours=1)) self.assertEqual(ScheduledEmail.objects.count(), 1) do_deactivate_user(user) self.assertEqual(ScheduledEmail.objects.count(), 0) diff --git a/zerver/views/registration.py b/zerver/views/registration.py index d3eba09c77..0af01e30ac 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -360,7 +360,7 @@ def prepare_activation_url(email: str, request: HttpRequest, return activation_url def send_confirm_registration_email(email: str, activation_url: str) -> None: - send_email('zerver/emails/confirm_registration', to_email=email, + send_email('zerver/emails/confirm_registration', to_emails=[email], from_address=FromAddress.tokenized_no_reply_address(), context={'activate_url': activation_url}) @@ -490,7 +490,7 @@ def find_account(request: HttpRequest) -> HttpResponse: 'realm_uri': user_profile.realm.uri, 'realm_name': user_profile.realm.name, } - send_email('zerver/emails/find_team', to_user_id=user_profile.id, context=ctx) + send_email('zerver/emails/find_team', to_user_ids=[user_profile.id], context=ctx) # Note: Show all the emails in the result otherwise this # feature can be used to ascertain which email addresses diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py index 8655789ca1..6a15448363 100644 --- a/zerver/views/user_settings.py +++ b/zerver/views/user_settings.py @@ -42,7 +42,7 @@ def confirm_email_change(request: HttpRequest, confirmation_key: str) -> HttpRes do_change_user_email(user_profile, new_email) context = {'realm_name': user_profile.realm.name, 'new_email': new_email} - send_email('zerver/emails/notify_change_in_email', to_email=old_email, + send_email('zerver/emails/notify_change_in_email', to_emails=[old_email], from_name="Zulip Account Security", from_address=FromAddress.SUPPORT, context=context) diff --git a/zerver/worker/queue_processors.py b/zerver/worker/queue_processors.py index 987ae5e50e..67ee2793f3 100644 --- a/zerver/worker/queue_processors.py +++ b/zerver/worker/queue_processors.py @@ -232,7 +232,7 @@ class ConfirmationEmailWorker(QueueProcessingWorker): send_future_email( "zerver/emails/invitation_reminder", referrer.realm, - to_email=invitee.email, + to_emails=[invitee.email], from_address=FromAddress.tokenized_no_reply_address(), context=context, delay=datetime.timedelta(days=2))