launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/basemailer-oops into lp:launchpad.

Commit message:
Improve OOPS logging when BaseMailer fails to send a message.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/basemailer-oops/+merge/264856

Improve OOPS logging when BaseMailer fails to send a message.

Future scripts using BaseMailer will need to pass in a request argument.  At the moment, all uses of BaseMailer are in jobs or in the buildmaster, and I think in both those cases it's OK just to not bother passing in a request.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/basemailer-oops into lp:launchpad.

=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py	2012-06-29 08:40:05 +0000
+++ lib/lp/services/mail/basemailer.py	2015-07-15 14:35:40 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 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).
 
 """Base class for sending out emails."""
@@ -9,6 +9,10 @@
 
 import logging
 from smtplib import SMTPException
+import sys
+
+from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
 
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
@@ -34,7 +38,7 @@
 
     def __init__(self, subject, template_name, recipients, from_address,
                  delta=None, message_id=None, notification_type=None,
-                 mail_controller_class=None):
+                 mail_controller_class=None, request=None):
         """Constructor.
 
         :param subject: A Python dict-replacement template for the subject
@@ -48,6 +52,7 @@
             not supplied, random message-ids will be used.
         :param mail_controller_class: The class of the mail controller to
             use to send the mails.  Defaults to `MailController`.
+        :param request: An `IErrorReportRequest` to use when logging OOPSes.
         """
         self._subject_template = subject
         self._template_name = template_name
@@ -62,6 +67,7 @@
         if mail_controller_class is None:
             mail_controller_class = MailController
         self._mail_controller_class = mail_controller_class
+        self.request = request
 
     def _getToAddresses(self, recipient, email):
         return [format_address(recipient.displayname, email)]
@@ -153,17 +159,25 @@
             try:
                 ctrl = self.generateEmail(email, recipient)
                 ctrl.send()
-            except SMTPException as e:
+            except SMTPException:
                 # If the initial sending failed, try again without
                 # attachments.
                 try:
                     ctrl = self.generateEmail(
                         email, recipient, force_no_attachments=True)
                     ctrl.send()
-                except SMTPException as e:
-                    # Don't want an entire stack trace, just some details.
-                    self.logger.warning(
-                        'send failed for %s, %s' % (email, e))
+                except SMTPException:
+                    error_utility = getUtility(IErrorReportingUtility)
+                    oops_vars = {
+                        "message_id": self.message_id,
+                        "notification_type": self.notification_type,
+                        "recipient": recipient.id,
+                        }
+                    with error_utility.oopsMessage(oops_vars):
+                        oops = error_utility.raising(
+                            sys.exc_info(), self.request)
+                    self.logger.info(
+                        "Mail resulted in OOPS: %s" % oops.get("id"))
 
 
 class RecipientReason:

=== modified file 'lib/lp/services/mail/tests/test_basemailer.py'
--- lib/lp/services/mail/tests/test_basemailer.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/mail/tests/test_basemailer.py	2015-07-15 14:35:40 +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).
 """Tests for the BaseMailer class."""
 
@@ -150,6 +150,9 @@
         notifications = pop_notifications()
         self.assertEqual(1, len(notifications))
         self.assertEqual('Good <good@xxxxxxxxxxx>', notifications[0]['To'])
+        # And an OOPS is logged.
+        self.assertEqual(1, len(self.oopses))
+        self.assertIn("SMTPException: boom", self.oopses[0]["tb_text"])
 
     def test_sendall_first_failure_strips_attachments(self):
         # If sending an email fails, we try again without the (almost
@@ -184,3 +187,5 @@
         self.assertEqual(
             'Excessively large attachments removed.',
             bad_parts[1].get_payload(decode=True))
+        # And no OOPS is logged.
+        self.assertEqual(0, len(self.oopses))