launchpad-reviewers team mailing list archive

[Martin Pool <martinpool@xxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/788874-mail-oops into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #788874 in Launchpad itself: "mail handling stalls when a large message is received and not deliverable"
  https://bugs.launchpad.net/launchpad/+bug/788874

For more details, see:
https://code.launchpad.net/~mbp/launchpad/788874-mail-oops/+merge/63343

By way of a small slice of pie, this tries to cope better with problems in reporting an oops back to a user in response to incoming mail.

 * Don't attach the original
 * Do include a URL where it can be seen
 * Log an OOPS if sending fails


-- 
https://code.launchpad.net/~mbp/launchpad/788874-mail-oops/+merge/63343
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/788874-mail-oops into lp:launchpad.

=== modified file 'lib/canonical/launchpad/mail/errortemplates/oops.txt'
--- lib/canonical/launchpad/mail/errortemplates/oops.txt	2010-10-24 21:00:11 +0000
+++ lib/canonical/launchpad/mail/errortemplates/oops.txt	2011-06-03 08:56:00 +0000
@@ -5,3 +5,5 @@
 If this is blocking your work, please file a question at
 https://answers.launchpad.net/launchpad/+addquestion
 and include the error ID %(oops_id)s in the description.
+
+The original message is available at <%(file_alias_url)s>.

=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py	2011-03-10 19:11:04 +0000
+++ lib/canonical/launchpad/mailnotification.py	2011-06-03 08:56:00 +0000
@@ -56,7 +56,7 @@
         :to_address: The address to send the notification to.
         :subject: The subject of the notification.
         :error_msg: The error message that explains the error.
-        :original_msg: The original message sent by the user.
+        :original_msg: The original message sent by the user (or None).
         :failing_command: The command that caused the error to happen.
     """
     if isinstance(failing_command, list):
@@ -83,7 +83,8 @@
     msg['From'] = get_bugmail_error_address()
     msg['Subject'] = subject
     msg.attach(error_part)
-    msg.attach(MIMEMessage(original_msg))
+    if original_msg is not None:
+        msg.attach(MIMEMessage(original_msg))
     sendmail(msg)
 
 

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2011-05-30 23:57:16 +0000
+++ lib/lp/services/mail/incoming.py	2011-06-03 08:56:00 +0000
@@ -383,17 +383,30 @@
         * records an OOPS with error_msg and file_alias_url
         * commits the current transaction to ensure that the
             message gets sent
+
+    The original mail is not included in OOPS bounces, because the one reason
+    it might fail is that the message is very long, and then we won't be able
+    to send it back.  The OOPS includes the text of the original URL and we
+    include the librarian URL.
     """
     log.info('error processing mail: %s' % (error_msg,))
     oops_id = report_oops(
         file_alias_url=file_alias_url,
         error_msg=error_msg)
     log.info('oops %s' % (oops_id,))
-    send_process_error_notification(
-        mail['From'],
-        'Submit Request Failure',
-        get_error_message('oops.txt', oops_id=oops_id),
-        mail)
+    try:
+        send_process_error_notification(
+            mail['From'],
+            'Submit Request Failure',
+            get_error_message('oops.txt',
+                oops_id=oops_id,
+                file_alias_url=file_alias_url),
+            None)
+    except:
+        log.exception("Error while sending email error notification for %s"
+            % (file_alias_url,))
+        report_oops(file_alias_url=file_alias_url,
+            error_msg="Failed to send error notification mail")
     trans.commit()