From 237efdee6c84db959776140f191e03c37f5cabbd Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@zulip.com>
Date: Tue, 11 May 2021 11:24:52 -1000
Subject: [PATCH] send_email: Show more information about messages which failed
 to send.

---
 zerver/lib/send_email.py        | 18 +++++++++--
 zerver/tests/test_send_email.py | 57 +++++++++++++++++++++++++--------
 2 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py
index 2da7139b17..a62f58eb12 100644
--- a/zerver/lib/send_email.py
+++ b/zerver/lib/send_email.py
@@ -252,10 +252,22 @@ def send_email(
         # This will call .open() for us, which is a no-op if it's already open;
         # it will only call .close() if it was not open to begin with
         if connection.send_messages([mail]) == 0:
-            logger.error("Error sending %s email to %s", template, mail.to)
+            logger.error("Unknown error sending %s email to %s", template, mail.to)
             raise EmailNotDeliveredException
-    except smtplib.SMTPException:
-        logger.error("Error sending %s email to %s", template, mail.to)
+    except smtplib.SMTPResponseException as e:
+        logger.exception(
+            "Error sending %s email to %s with error code %s: %s",
+            template,
+            mail.to,
+            e.smtp_code,
+            e.smtp_error,
+            stack_info=True,
+        )
+        raise EmailNotDeliveredException
+    except smtplib.SMTPException as e:
+        logger.exception(
+            "Error sending %s email to %s: %s", template, mail.to, str(e), stack_info=True
+        )
         raise EmailNotDeliveredException
 
 
diff --git a/zerver/tests/test_send_email.py b/zerver/tests/test_send_email.py
index d86ecd7329..c581db1885 100644
--- a/zerver/tests/test_send_email.py
+++ b/zerver/tests/test_send_email.py
@@ -1,4 +1,4 @@
-from smtplib import SMTP, SMTPDataError, SMTPException
+from smtplib import SMTP, SMTPDataError, SMTPException, SMTPRecipientsRefused
 from unittest import mock
 
 from django.core.mail.backends.locmem import EmailBackend
@@ -60,11 +60,39 @@ class TestBuildEmail(ZulipTestCase):
                     )
         self.assertEqual(len(info_log.records), 2)
         self.assertEqual(
-            info_log.output,
-            [
-                f"INFO:{logger.name}:Sending password_reset email to {mail.to}",
-                f"ERROR:{logger.name}:Error sending password_reset email to {mail.to}",
-            ],
+            info_log.output[0], f"INFO:{logger.name}:Sending password_reset email to {mail.to}"
+        )
+        self.assertTrue(
+            info_log.output[1].startswith(
+                f"ERROR:{logger.name}:Error sending password_reset email to {mail.to} with error code 242: From field too long."
+            )
+        )
+
+    def test_build_and_send_refused(self) -> None:
+        hamlet = self.example_user("hamlet")
+        address = "zulip@example.com"
+        with mock.patch.object(
+            EmailBackend,
+            "send_messages",
+            side_effect=SMTPRecipientsRefused(recipients={address: (550, b"User unknown")}),
+        ):
+            with self.assertLogs(logger=logger) as info_log:
+                with self.assertRaises(EmailNotDeliveredException):
+                    send_email(
+                        "zerver/emails/password_reset",
+                        to_emails=[hamlet],
+                        from_name="Zulip",
+                        from_address=address,
+                        language="en",
+                    )
+        self.assertEqual(len(info_log.records), 2)
+        self.assertEqual(
+            info_log.output[0], f"INFO:{logger.name}:Sending password_reset email to {[hamlet]}"
+        )
+        self.assertFalse(
+            info_log.output[1].startswith(
+                f"ERROR:{logger.name}:Error sending password_reset email to {[hamlet]}: {{'{address}': (550, 'User unknown')}}"
+            )
         )
 
 
@@ -119,10 +147,13 @@ class TestSendEmail(ZulipTestCase):
         )
 
         # We test the two cases that should raise an EmailNotDeliveredException
-        side_effect = [0, SMTPException]
+        errors = {
+            f"Unknown error sending password_reset email to {mail.to}": [0],
+            f"Error sending password_reset email to {mail.to}": [SMTPException()],
+        }
 
-        with mock.patch.object(EmailBackend, "send_messages", side_effect=side_effect):
-            for i in range(len(side_effect)):
+        for message, side_effect in errors.items():
+            with mock.patch.object(EmailBackend, "send_messages", side_effect=side_effect):
                 with self.assertLogs(logger=logger) as info_log:
                     with self.assertRaises(EmailNotDeliveredException):
                         send_email(
@@ -134,9 +165,7 @@ class TestSendEmail(ZulipTestCase):
                         )
                 self.assertEqual(len(info_log.records), 2)
                 self.assertEqual(
-                    info_log.output,
-                    [
-                        f"INFO:{logger.name}:Sending password_reset email to {mail.to}",
-                        f"ERROR:{logger.name}:Error sending password_reset email to {mail.to}",
-                    ],
+                    info_log.output[0],
+                    f"INFO:{logger.name}:Sending password_reset email to {mail.to}",
                 )
+                self.assertTrue(info_log.output[1].startswith(f"ERROR:zulip.send_email:{message}"))