launchpad-reviewers team mailing list archive

[Martin Pool <martinpool@xxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #777983 in Launchpad itself: "incoming mail code is messy"
  https://bugs.launchpad.net/launchpad/+bug/777983

For more details, see:
https://code.launchpad.net/~mbp/launchpad/mail-cleanup/+merge/60283

Some more cleanups to mail/incoming.py:

 * avoid passing around quite so many variables between functions
 * lift up the code that parses incoming messages; if it fails log an error and delete the mail, because we're unlikely to succeed on another try

lint clean

tested with './bin/test -mv mail' 
-- 
https://code.launchpad.net/~mbp/launchpad/mail-cleanup/+merge/60283
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/mail-cleanup into lp:launchpad.

=== modified file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	2011-05-05 20:18:37 +0000
+++ lib/lp/services/features/scopes.py	2011-05-07 15:42:36 +0000
@@ -36,7 +36,7 @@
     documentation, so write them accordingly.
     """
 
-    # The regex pattern used to decide if a handler can evalute a particular
+    # The regex pattern used to decide if a handler can evaluate a particular
     # scope.  Also used on +feature-info.
     pattern = None
 

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2011-04-28 22:41:18 +0000
+++ lib/lp/services/mail/incoming.py	2011-05-07 15:42:36 +0000
@@ -269,7 +269,7 @@
 ORIGINAL_TO_HEADER = 'X-Launchpad-Original-To'
 
 
-def extract_addresses(mail, raw_mail, file_alias_url, log):
+def extract_addresses(mail, file_alias_url, log):
     """Extract the domain the mail was sent to.
 
     Mails sent to Launchpad should have an X-Launchpad-Original-To header.
@@ -278,7 +278,7 @@
     if ORIGINAL_TO_HEADER in mail:
         return [mail[ORIGINAL_TO_HEADER]]
 
-    if ORIGINAL_TO_HEADER in raw_mail:
+    if ORIGINAL_TO_HEADER in mail.as_string():
         # Doesn't have an X-Launchpad-Original-To in the headers, but does
         # have one in the body, because of a forwarding loop or attempted
         # spam.  See <https://bugs.launchpad.net/launchpad/+bug/701976>
@@ -336,8 +336,19 @@
                 log.exception('Upload to Librarian failed')
                 continue
             try:
-                handle_one_mail(trans, log, raw_mail,
-                    file_alias, file_alias_url, signature_timestamp_checker)
+                mail = signed_message_from_string(raw_mail)
+            except email.Errors.MessageError:
+                # If we can't parse the message, we can't send a reply back to
+                # the user, but logging an exception will let us investigate.
+                log.exception(
+                    "Couldn't convert email to email.Message: %s" % (
+                    file_alias_url, ))
+                mailbox.delete(mail_id)
+                continue
+            try:
+                trans.begin()
+                handle_one_mail(log, mail, file_alias, file_alias_url,
+                    signature_timestamp_checker)
                 trans.commit()
                 mailbox.delete(mail_id)
             except (KeyboardInterrupt, SystemExit):
@@ -351,8 +362,7 @@
                 log.exception(
                     "An exception was raised inside the handler:\n%s"
                     % (file_alias_url,))
-                _send_email_oops(trans, log,
-                    signed_message_from_string(raw_mail), raw_mail,
+                _send_email_oops(trans, log, mail,
                     "Unhandled exception", file_alias_url)
                 mailbox.delete(mail_id)
     finally:
@@ -360,7 +370,7 @@
         mailbox.close()
 
 
-def _send_email_oops(trans, log, mail, raw_mail, error_msg, file_alias_url):
+def _send_email_oops(trans, log, mail, error_msg, file_alias_url):
     """Handle an error that generates an oops.
 
     It does the following:
@@ -397,7 +407,7 @@
     return file_alias
 
 
-def handle_one_mail(trans, log, raw_mail, file_alias, file_alias_url,
+def handle_one_mail(log, mail, file_alias, file_alias_url,
     signature_timestamp_checker):
     """Process one message.
 
@@ -406,15 +416,6 @@
     sent if appropriate.
     """
 
-    trans.begin()
-
-    try:
-        mail = signed_message_from_string(raw_mail)
-    except email.Errors.MessageError, error:
-        log.warn("Couldn't convert email to email.Message: %s" % (
-                file_alias_url, ),
-            exc_info=True)
-        return
     log.debug('processing mail from %r message-id %r' %
         (mail['from'], mail['message-id']))
 
@@ -443,7 +444,7 @@
         log.info("Inactive account found for %s" % mail['From'])
         return
 
-    addresses = extract_addresses(mail, raw_mail, file_alias_url, log)
+    addresses = extract_addresses(mail, file_alias_url, log)
     log.debug('mail was originally to: %r' % (addresses,))
 
     try:

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2011-03-04 03:15:09 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2011-05-07 15:42:36 +0000
@@ -101,6 +101,7 @@
         # let's just treat everything as invalid, and trust that the regular
         # implementation of extraction and checking of timestamps is correct,
         # or at least tested.
+
         def fail_all_timestamps(timestamp, context):
             raise helpers.IncomingEmailError("fail!")
         self.assertRaises(
@@ -131,7 +132,7 @@
         original_to = 'eric@xxxxxxxxxxxxxxxxxxx'
         mail[ORIGINAL_TO_HEADER] = original_to
         self.assertThat(
-            extract_addresses(mail, None, None, None),
+            extract_addresses(mail, None, None),
             Equals([original_to]))
 
     def test_original_to_in_body(self):
@@ -143,7 +144,7 @@
         log = BufferLogger()
         mail = self.factory.makeSignedMessage(
             body=body, to_address=header_to)
-        addresses = extract_addresses(mail, mail.as_string(), alias, log)
+        addresses = extract_addresses(mail, alias, log)
         self.assertThat(addresses, Equals([header_to]))
         self.assertThat(
             log.getLogBuffer(),
@@ -154,7 +155,7 @@
         alias = 'librarian-somewhere'
         log = BufferLogger()
         mail = self.factory.makeSignedMessage(to_address=header_to)
-        addresses = extract_addresses(mail, mail.as_string(), alias, log)
+        addresses = extract_addresses(mail, alias, log)
         self.assertThat(addresses, Equals([header_to]))
         self.assertThat(
             log.getLogBuffer(),