Improve hc.lib.emails.send()

- add optional `from_email` argument
- add test cases that exercise the retry loop

hc/lib/emails.py

@@ -1,5 +1,6 @@
 import smtplib
 from threading import Thread
+import time
 
 from django.conf import settings
 from django.core.mail import EmailMultiAlternatives
@@ -9,41 +10,41 @@ from django.template.loader import render_to_string as render
 class EmailThread(Thread):
     MAX_TRIES = 3
 
-    def __init__(self, subject, text, html, to, headers):
+    def __init__(self, message):
         Thread.__init__(self)
-        self.subject = subject
+        self.message = message
-        self.text = text
-        self.html = html
-        self.to = to
-        self.headers = headers
 
     def run(self):
         for attempt in range(0, self.MAX_TRIES):
             try:
-                msg = EmailMultiAlternatives(
+                # Make sure each retry creates a new connection:
-                    self.subject, self.text, to=(self.to,), headers=self.headers
+                self.message.connection = None
-                )
+                self.message.send()
-
+                # No exception--great, return from the retry loop
-                msg.attach_alternative(self.html, "text/html")
+                return
-                msg.send()
             except smtplib.SMTPServerDisconnected as e:
                 if attempt + 1 == self.MAX_TRIES:
                     # This was the last attempt and it failed:
                     # re-raise the exception
                     raise e
-            else:
+
-                # There was no exception, break out of the retry loop
+                # Wait 1s before retrying
-                break
+                time.sleep(1)
 
 
-def send(name, to, ctx, headers={}):
+def send(name, to, ctx, headers={}, from_email=None):
     ctx["SITE_ROOT"] = settings.SITE_ROOT
 
     subject = render("emails/%s-subject.html" % name, ctx).strip()
-    text = render("emails/%s-body-text.html" % name, ctx)
+    body = render("emails/%s-body-text.html" % name, ctx)
     html = render("emails/%s-body-html.html" % name, ctx)
 
-    t = EmailThread(subject, text, html, to, headers)
+    msg = EmailMultiAlternatives(subject, body, to=(to,), headers=headers)
+    msg.attach_alternative(html, "text/html")
+    if from_email:
+        msg.from_email = from_email
+
+    t = EmailThread(msg)
     if hasattr(settings, "BLOCKING_EMAILS"):
         # In tests, we send emails synchronously
         # so we can inspect the outgoing messages
@@ -63,7 +64,7 @@ def transfer_request(to, ctx):
 
 
 def alert(to, ctx, headers={}):
-    send("alert", to, ctx, headers)
+    send("alert", to, ctx, headers=headers)
 
 
 def verify_email(to, ctx):
@@ -71,11 +72,11 @@ def verify_email(to, ctx):
 
 
 def report(to, ctx, headers={}):
-    send("report", to, ctx, headers)
+    send("report", to, ctx, headers=headers)
 
 
 def deletion_notice(to, ctx, headers={}):
-    send("deletion-notice", to, ctx, headers)
+    send("deletion-notice", to, ctx, headers=headers)
 
 
 def sms_limit(to, ctx):

hc/lib/tests/test_emails.py

@@ -0,0 +1,27 @@
+from smtplib import SMTPServerDisconnected
+from unittest.mock import Mock, patch
+
+from django.test import TestCase
+from hc.lib.emails import EmailThread
+
+
+@patch("hc.lib.emails.time.sleep")
+class EmailsTestCase(TestCase):
+    def test_it_retries(self, mock_time):
+        mock_msg = Mock()
+        mock_msg.send = Mock(side_effect=[SMTPServerDisconnected, None])
+
+        t = EmailThread(mock_msg)
+        t.run()
+
+        self.assertEqual(mock_msg.send.call_count, 2)
+
+    def test_it_limits_retries(self, mock_time):
+        mock_msg = Mock()
+        mock_msg.send = Mock(side_effect=SMTPServerDisconnected)
+
+        with self.assertRaises(SMTPServerDisconnected):
+            t = EmailThread(mock_msg)
+            t.run()
+
+        self.assertEqual(mock_msg.send.call_count, 3)