launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/send-bug-notifications-oops into lp:launchpad.

Commit message:
Turn SMTPExceptions when sending bug notifications into OOPSes rather than letting the entire script crash.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #314420 in Launchpad itself: "send-bug-notifications.py will explode if the SMTP server refuses to send to a recipient"
  https://bugs.launchpad.net/launchpad/+bug/314420
  Bug #916939 in Launchpad itself: "send-bug-notifications.py can't handle rejections due to oversized messages"
  https://bugs.launchpad.net/launchpad/+bug/916939

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/send-bug-notifications-oops/+merge/264814

Turn SMTPExceptions when sending bug notifications into OOPSes rather than letting the entire script crash.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/send-bug-notifications-oops into lp:launchpad.

=== modified file 'cronscripts/send-bug-notifications.py'
--- cronscripts/send-bug-notifications.py	2013-01-07 02:40:55 +0000
+++ cronscripts/send-bug-notifications.py	2015-07-15 10:00:18 +0000
@@ -13,51 +13,8 @@
 
 import _pythonpath
 
-from zope.component import getUtility
-
-from lp.bugs.enums import BugNotificationStatus
-from lp.bugs.interfaces.bugnotification import IBugNotificationSet
-from lp.bugs.scripts.bugnotification import (
-    get_email_notifications,
-    process_deferred_notifications,
-    )
+from lp.bugs.scripts.bugnotification import SendBugNotifications
 from lp.services.config import config
-from lp.services.database.constants import UTC_NOW
-from lp.services.mail.sendmail import sendmail
-from lp.services.scripts.base import LaunchpadCronScript
-
-
-class SendBugNotifications(LaunchpadCronScript):
-    def main(self):
-        notifications_sent = False
-        bug_notification_set = getUtility(IBugNotificationSet)
-        deferred_notifications = \
-            bug_notification_set.getDeferredNotifications()
-        process_deferred_notifications(deferred_notifications)
-        pending_notifications = get_email_notifications(
-            bug_notification_set.getNotificationsToSend())
-        for (bug_notifications,
-             omitted_notifications,
-             messages) in pending_notifications:
-            for message in messages:
-                self.logger.info("Notifying %s about bug %d." % (
-                    message['To'], bug_notifications[0].bug.id))
-                sendmail(message)
-                self.logger.debug(message.as_string())
-            for notification in bug_notifications:
-                notification.date_emailed = UTC_NOW
-                notification.status = BugNotificationStatus.SENT
-            for notification in omitted_notifications:
-                notification.date_emailed = UTC_NOW
-                notification.status = BugNotificationStatus.OMITTED
-            notifications_sent = True
-            # Commit after each batch of email sent, so that we won't
-            # re-mail the notifications in case of something going wrong
-            # in the middle.
-            self.txn.commit()
-
-        if not notifications_sent:
-            self.logger.debug("No notifications are pending to be sent.")
 
 
 if __name__ == '__main__':

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2015-07-06 17:27:09 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2015-07-15 10:00:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions related to sending bug notifications."""
@@ -13,12 +13,18 @@
 
 from itertools import groupby
 from operator import itemgetter
+from smtplib import SMTPException
+import sys
 
 from storm.store import Store
 import transaction
 from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
 
-from lp.bugs.enums import BugNotificationLevel
+from lp.bugs.enums import (
+    BugNotificationLevel,
+    BugNotificationStatus,
+    )
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
 from lp.bugs.mail.bugnotificationbuilder import (
     BugNotificationBuilder,
@@ -26,10 +32,14 @@
     )
 from lp.bugs.mail.newbug import generate_bug_add_email
 from lp.registry.model.person import get_recipients
+from lp.services.database.constants import UTC_NOW
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.mailwrapper import MailWrapper
+from lp.services.mail.sendmail import sendmail
+from lp.services.scripts.base import LaunchpadCronScript
 from lp.services.scripts.logger import log
 from lp.services.webapp import canonical_url
+from lp.services.webapp.errorlog import ScriptRequest
 
 
 def get_activity_key(notification):
@@ -327,3 +337,47 @@
             message=message,
             recipients=recipients,
             activity=activity)
+
+
+class SendBugNotifications(LaunchpadCronScript):
+    def main(self):
+        notifications_sent = False
+        bug_notification_set = getUtility(IBugNotificationSet)
+        deferred_notifications = \
+            bug_notification_set.getDeferredNotifications()
+        process_deferred_notifications(deferred_notifications)
+        pending_notifications = get_email_notifications(
+            bug_notification_set.getNotificationsToSend())
+        for (bug_notifications,
+             omitted_notifications,
+             messages) in pending_notifications:
+            try:
+                for message in messages:
+                    self.logger.info("Notifying %s about bug %d." % (
+                        message['To'], bug_notifications[0].bug.id))
+                    sendmail(message)
+                    self.logger.debug(message.as_string())
+            except SMTPException:
+                request = ScriptRequest([
+                    ("script_name", self.name),
+                    ("path", sys.argv[0]),
+                    ])
+                getUtility(IErrorReportingUtility).raising(
+                    sys.exc_info(), request)
+                self.logger.info(request.oopsid)
+                self.txn.abort()
+                continue
+            for notification in bug_notifications:
+                notification.date_emailed = UTC_NOW
+                notification.status = BugNotificationStatus.SENT
+            for notification in omitted_notifications:
+                notification.date_emailed = UTC_NOW
+                notification.status = BugNotificationStatus.OMITTED
+            notifications_sent = True
+            # Commit after each batch of email sent, so that we won't
+            # re-mail the notifications in case of something going wrong
+            # in the middle.
+            self.txn.commit()
+
+        if not notifications_sent:
+            self.logger.debug("No notifications are pending to be sent.")

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2015-07-08 16:05:11 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2015-07-15 10:00:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 """Tests for construction bug notification emails for sending."""
 
@@ -10,6 +10,7 @@
     )
 import logging
 import re
+from smtplib import SMTPException
 import StringIO
 import unittest
 
@@ -22,6 +23,7 @@
     getUtility,
     )
 from zope.interface import implementer
+from zope.sendmail.interfaces import IMailDelivery
 
 from lp.app.enums import InformationType
 from lp.bugs.adapters.bugchange import (
@@ -38,6 +40,7 @@
     CveLinkedToBug,
     CveUnlinkedFromBug,
     )
+from lp.bugs.enums import BugNotificationStatus
 from lp.bugs.interfaces.bug import (
     CreateBugParams,
     IBug,
@@ -63,6 +66,7 @@
     notification_batches,
     notification_comment_batches,
     process_deferred_notifications,
+    SendBugNotifications,
     )
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
@@ -76,6 +80,8 @@
     get_contact_email_addresses,
     get_email_template,
     )
+from lp.services.mail.sendmail import set_immediate_mail_delivery
+from lp.services.mail.stub import TestMailer
 from lp.services.messages.interfaces.message import IMessageSet
 from lp.services.propertycache import cachedproperty
 from lp.testing import (
@@ -87,6 +93,7 @@
     lp_dbuser,
     switch_dbuser,
     )
+from lp.testing.fixture import ZopeUtilityFixture
 from lp.testing.layers import LaunchpadZopelessLayer
 from lp.testing.matchers import Contains
 
@@ -1281,3 +1288,69 @@
         # And there are no longer any deferred.
         deferred = self.notification_set.getDeferredNotifications()
         self.assertEqual(0, deferred.count())
+
+
+class BrokenMailer(TestMailer):
+    """A mailer that raises an exception for certain recipient addresses."""
+
+    def __init__(self, broken):
+        self.broken = broken
+
+    def send(self, from_addr, to_addrs, message):
+        if any(to_addr in self.broken for to_addr in to_addrs):
+            raise SMTPException("test requested delivery failure")
+        return super(BrokenMailer, self).send(from_addr, to_addrs, message)
+
+
+class TestSendBugNotifications(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestSendBugNotifications, self).setUp()
+        self.notification_set = getUtility(IBugNotificationSet)
+        # Ensure there are no outstanding notifications.
+        for notification in self.notification_set.getNotificationsToSend():
+            notification.destroySelf()
+        self.ten_minutes_ago = datetime.now(pytz.UTC) - timedelta(minutes=10)
+
+    def test_oops_on_failed_delivery(self):
+        # If one notification fails to send, it logs an OOPS and doesn't get
+        # in the way of sending other notifications.
+        set_immediate_mail_delivery(False)
+        subscribers = []
+        for i in range(3):
+            bug = self.factory.makeBug()
+            subscriber = self.factory.makePerson()
+            subscribers.append(subscriber.preferredemail.email)
+            bug.default_bugtask.target.addSubscription(subscriber, subscriber)
+            message = getUtility(IMessageSet).fromText(
+                "subject", "a comment.", bug.owner,
+                datecreated=self.ten_minutes_ago)
+            bug.addCommentNotification(message)
+
+        notifications = list(get_email_notifications(
+            self.notification_set.getNotificationsToSend()))
+        self.assertEqual(3, len(notifications))
+
+        mailer = BrokenMailer([subscribers[1]])
+        with ZopeUtilityFixture(mailer, IMailDelivery, "Mail"):
+            switch_dbuser(config.malone.bugnotification_dbuser)
+            script = SendBugNotifications(
+                "send-bug-notifications", config.malone.bugnotification_dbuser,
+                ["-q"])
+            script.txn = self.layer.txn
+            script.main()
+
+        self.assertEqual(1, len(self.oopses))
+        self.assertIn(
+            "SMTPException: test requested delivery failure",
+            self.oopses[0]["tb_text"])
+        for i, (bug_notifications, _, messages) in enumerate(notifications):
+            for bug_notification in bug_notifications:
+                if i == 1:
+                    self.assertEqual(
+                        BugNotificationStatus.PENDING, bug_notification.status)
+                else:
+                    self.assertEqual(
+                        BugNotificationStatus.SENT, bug_notification.status)