Merge lp:~gary/launchpad/bug650343 into lp:launchpad

[Gary Poster]

See bug 650343 for the motivation. This is primarily just reverting the change done described in that bug, because the constraint/problem in the data center email configuration has been resolved.

I kept a spelling correction from the change. I also changed things per what lint described (e.g., converted to ReST), except for the singleton tuple complaints (e.g., in "log.warning('DKIM error: %r' % (e,))" the linter wants to see "(e, )").

./lib/canonical/launchpad/mail/incoming.py
     126: E231 missing whitespace after ','
     130: E231 missing whitespace after ','
     133: E231 missing whitespace after ','
     134: E231 missing whitespace after ','
     142: E231 missing whitespace after ','
     153: E231 missing whitespace after ','

[Gary Poster (gary)]

Sorry, bug 650343.

[Māris Fogels (mars)]

Hi Gary,

The code for this change looks good to land. r=mars. I have a few questions about the tests though:

  * Do we want to silence the log warnings in the doctest output on line 26 of the diff? They look really odd. If we can not silence them, we could at least explain why they were left in there, and why there are four of them.

  * We need some way to move tests like this into lib/lp. The problem is that this test lives in canonical/launchpad/doc, which runs in a different layer than the pagetests (LaunchpadFunctionalLayer I believe), and I do not think we have a place yet in the lib/lp that runs tests at this level.

This looks good to me. r=mars.

Maris

review: Approve

[Jonathan Lange (jml)]

On Tue, Oct 5, 2010 at 5:37 PM, Māris Fogels <email address hidden> wrote:
>  * We need some way to move tests like this into lib/lp.  The problem is that this test lives in canonical/launchpad/doc, which runs in a different layer than the pagetests (LaunchpadFunctionalLayer I believe), and I do not think we have a place yet in the lib/lp that runs tests at this level.

It's easy enough to move them. Just copy what other lib/lp packages do
for their doctests.

jml

[Francis J. Lacoste (flacoste)]

The header name should be changed to X-Launchpad-Original-To.

review: Needs Fixing

[Gary Poster (gary)]

This branch now has the change Francis pointed out was necessary, and so is ready for his review.

The requests for moving files around from jml and mars resulted in a 600+ line diff (`make lint` always gets its chunk of flesh) that can be found in its own branch: https://code.edge.launchpad.net/~gary/launchpad/bug650343-2 . That is running through ec2 and I'll ask for a review when I get a successful test run.

[Francis J. Lacoste (flacoste)]

Looks good now, thanks.

review: Approve

=== modified file 'lib/canonical/launchpad/doc/incomingmail.txt'
--- lib/canonical/launchpad/doc/incomingmail.txt	2010-10-04 19:50:45 +0000
+++ lib/canonical/launchpad/doc/incomingmail.txt	2010-10-12 13:37:43 +0000
@@ -1,4 +1,5 @@
-= Incoming Mail =
+Incoming Mail
+=============
 
 When an email is sent to Launchpad we need to handle it somehow. This
 is done by handleEmails:
@@ -14,7 +15,9 @@
     * Deletes the message from the mail box
 
 
-== Mail Handlers ==
+-------------
+Mail Handlers
+-------------
 
 A mail handler is a utility which knows how to handle mail sent to a
 specific domain. It is registered as a named utility providing
@@ -79,9 +82,16 @@
     >>> config.push('bugmail_error_from_address', bugmail_error_from_address)
 
 The test mails are now in Launchpad's mail box, so now we can call
-handleMail, so that every mail gets handled by the correct handler.
+handleMail, so that every mail gets handled by the correct handler.  (We
+see warnings about missing `X-Launchpad-Original-To`_ headers, which are
+discussed below; this output merely shows that we emit warnings when the
+header is missing.)
 
     >>> handleMail(zopeless_transaction)
+    WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
+    WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
+    WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
+    WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
 
 Now we can see that each handler handled the emails sent to its domain:
 
@@ -90,8 +100,9 @@
     >>> set(bar_handler.handledMails) ^ set(msgids['bar.com'])
     set([])
 
-
-== Unhandled Mail ==
+--------------
+Unhandled Mail
+--------------
 
 So, what happened to the message that got sent to baz.com? Since there
 wasn't a handler registered for that domain, an OOPS was recorded with
@@ -118,14 +129,15 @@
 
     >>> stub.test_emails = []
 
-
-== Mail from Persons not registered in Launchpad ==
+---------------------------------------------
+Mail from Persons not registered in Launchpad
+---------------------------------------------
 
 If a Person who isn't registered in Launchpad sends an email, we'll
 most of the time reject the email:
 
     >>> moin_change = read_test_message('moin-change.txt')
-    >>> moin_change['X-Original-To'] = '[e-mail]'
+    >>> moin_change['X-Launchpad-Original-To'] = '[e-mail]'
     >>> msgid = "<%s>" % sendmail(moin_change)
     >>> handleMail(zopeless_transaction)
     >>> msgid not in foo_handler.handledMails
@@ -140,7 +152,7 @@
 
 So if we send the mail to bar.com, bar_handler will handle the mail:
 
-    >>> moin_change.replace_header('To', '[e-mail]')
+    >>> moin_change.replace_header('X-Launchpad-Original-To', '[e-mail]')
     >>> msgid = "<%s>" % sendmail(moin_change)
     >>> handleMail(zopeless_transaction)
     >>> msgid in bar_handler.handledMails
@@ -148,8 +160,9 @@
 
     >>> stub.test_emails = []
 
-
-== Mail from Persons with with an inactive Launchpad account ==
+---------------------------------------------------------
+Mail from Persons with with an inactive Launchpad account
+---------------------------------------------------------
 
 If a Person who's account is inactive sends an email, it will be
 silently rejected.
@@ -173,15 +186,31 @@
     >>> msgid not in foo_handler.handledMails
     True
 
-== X-Original-To ==
-
-Ideally, we would use X-Original-To, as this reflects the actual address
-that the mail was sent to.  Unfortunately, due to mail configuration issues,
-X-Original-To is very wrong, so we must rely on To and Cc, even though these
-may be inaccurate.
-
-
-== OOPSes processing incoming mail ==
+-----------------------
+X-Launchpad-Original-To
+-----------------------
+
+If available, the X-Launchpad-Original-To header is used to determine to
+which address the email was sent to:
+
+    >>> msg = read_test_message('signed_detached.txt')
+    >>> msg.replace_header('To', '[e-mail]')
+    >>> msg['CC'] = '[e-mail]'
+    >>> msg['X-Launchpad-Original-To'] = '[e-mail]'
+    >>> msgid = '<%s>' % sendmail (msg, ['[e-mail]'])
+    >>> handleMail(zopeless_transaction)
+    >>> msgid in bar_handler.handledMails
+    True
+
+Only the address in X-Launchpad-Original-To header will be used. The
+addresses in the To and CC headers will be ignored:
+
+    >>> msgid in foo_handler.handledMails
+    False
+
+-------------------------------
+OOPSes processing incoming mail
+-------------------------------
 
 If an unhandled exception occurs when we try to process an email from
 a user, we record an OOPS with the exception and send it to the user.
@@ -203,7 +232,7 @@
     >>> msg = email.message_from_string(
     ... """From: Foo Bar <[e-mail]>
     ... To: [e-mail]
-    ... X-Original-To: [e-mail]
+    ... X-Launchpad-Original-To: [e-mail]
     ... Subject: doesn't matter
     ...
     ... doesn't matter
@@ -232,7 +261,7 @@
     ...
     From: Foo Bar <[e-mail]>
     To: [e-mail]
-    X-Original-To: [e-mail]
+    X-Launchpad-Original-To: [e-mail]
     Subject: doesn't matter
     ...
 
@@ -251,7 +280,7 @@
     >>> msg = email.message_from_string(
     ... """From: Foo Bar <[e-mail]>
     ... To: [e-mail]
-    ... X-Original-To: [e-mail]
+    ... X-Launchpad-Original-To: [e-mail]
     ... Subject: doesn't matter
     ...
     ... doesn't matter
@@ -277,14 +306,15 @@
     ...
     From: Foo Bar <[e-mail]>
     To: [e-mail]
-    X-Original-To: [e-mail]
+    X-Launchpad-Original-To: [e-mail]
     Subject: doesn't matter
     ...
 
     >>> stub.test_emails = []
 
-
-== DB exceptions ==
+-------------
+DB exceptions
+-------------
 
 If something goes wrongs in the handler, a DB exception can be raised,
 leaving the database in a bad state. If that happens a traceback should
@@ -305,7 +335,7 @@
     >>> exception_raiser = email.message_from_string(
     ... """From: Foo Bar <[e-mail]>
     ... To: [e-mail]
-    ... X-Original-To: [e-mail]
+    ... X-Launchpad-Original-To: [e-mail]
     ... Subject: Raise an exception
     ...
     ... This part is not important.
@@ -328,6 +358,7 @@
     ...
     DataError: division by zero
     <BLANKLINE>
+    WARNING...
 
 The second mail we sent got handled despite the exception:
 
@@ -340,8 +371,9 @@
     >>> len(stub.test_emails)
     1
 
-
-== Librarian not running ==
+---------------------
+Librarian not running
+---------------------
 
 If for some reason the Librarian isn't up and running, we shouldn't
 lose any emails. All that should happen is that an error should get
@@ -368,8 +400,9 @@
     >>> LibrarianLayer.reveal()
     >>> stub.test_emails = []
 
-
-== Handling bounces ==
+----------------
+Handling bounces
+----------------
 
 Some broken mailers might not respect the Errors-To and Return-Path
 headers, send error messages back to the address, from which the email
@@ -427,7 +460,7 @@
     0
 
 
-== Doctest cleanup ==
+.. Doctest cleanup
 
     >>> config_data = config.pop('bugmail_error_from_address')
     >>> mail_handlers.add('foo.com', None)

=== modified file 'lib/canonical/launchpad/mail/incoming.py'
--- lib/canonical/launchpad/mail/incoming.py	2010-10-03 15:30:06 +0000
+++ lib/canonical/launchpad/mail/incoming.py	2010-10-12 13:37:43 +0000
@@ -149,7 +149,7 @@
             % (signing_domain, from_domain))
         return False
     if not _isDkimDomainTrusted(signing_domain):
-        log.warning("valid DKIM signature from untrusted domain %s" 
+        log.warning("valid DKIM signature from untrusted domain %s"
             % (signing_domain,))
         return False
     return True
@@ -338,8 +338,7 @@
                 if mail['Return-Path'] == '<>':
                     _handle_error(
                         "Message had an empty Return-Path.",
-                        file_alias_url, notify=False
-                        )
+                        file_alias_url, notify=False)
                     continue
                 if mail.get_content_type() == 'multipart/report':
                     # Mails with a content type of multipart/report are
@@ -351,8 +350,7 @@
                 if 'precedence' in mail:
                     _handle_error(
                         "Got a message with a precedence header.",
-                        file_alias_url, notify=False
-                        )
+                        file_alias_url, notify=False)
                     continue
 
                 try:
@@ -366,14 +364,21 @@
                         file_alias_url, notify=False)
                     continue
 
-                # Extract the domain the mail was sent to. Mails sent to
-                # Launchpad should have an X-Original-To header, but
-                # it has an incorrect address.
-                # Process all addresses found as a fall back.
-                cc = mail.get_all('cc') or []
-                to = mail.get_all('to') or []
-                names_addresses = getaddresses(to + cc)
-                addresses = [addr for name, addr in names_addresses]
+                # Extract the domain the mail was sent to.  Mails sent to
+                # Launchpad should have an X-Launchpad-Original-To header.
+                if 'X-Launchpad-Original-To' in mail:
+                    addresses = [mail['X-Launchpad-Original-To']]
+                else:
+                    log = logging.getLogger('canonical.launchpad.mail')
+                    log.warn(
+                        "No X-Launchpad-Original-To header was present "
+                        "in email: %s" %
+                         file_alias_url)
+                    # Process all addresses found as a fall back.
+                    cc = mail.get_all('cc') or []
+                    to = mail.get_all('to') or []
+                    names_addresses = getaddresses(to + cc)
+                    addresses = [addr for name, addr in names_addresses]
 
                 try:
                     do_paranoid_envelope_to_validation(addresses)
@@ -400,8 +405,7 @@
                 if principal is None and not handler.allow_unknown_users:
                     _handle_error(
                         'Unknown user: %s ' % mail['From'],
-                        file_alias_url, notify=False
-                        )
+                        file_alias_url, notify=False)
                     continue
 
                 handled = handler.process(mail, email_addr, file_alias)

=== modified file 'lib/lp/blueprints/doc/spec-mail-exploder.txt'
--- lib/lp/blueprints/doc/spec-mail-exploder.txt	2010-07-30 12:56:27 +0000
+++ lib/lp/blueprints/doc/spec-mail-exploder.txt	2010-10-12 13:37:43 +0000
@@ -264,7 +264,7 @@
     True
 
     >>> moin_change = read_test_message('moin-change.txt')
-    >>> moin_change['To'] += ", notifications@%s" % (
+    >>> moin_change['X-Launchpad-Original-To'] = "notifications@%s" % (
     ...     config.launchpad.specs_domain)
     >>> moin_change['Sender'] = '[e-mail]'