Launchpad itself

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

  1. bug650343
  2. Merge into devel
Proposed by Gary Poster
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 11705
Proposed branch: lp:~gary/launchpad/bug650343
Merge into: lp:launchpad
Diff against target: 313 lines (+85/-48)
3 files modified
lib/canonical/launchpad/doc/incomingmail.txt (+65/-32)
lib/canonical/launchpad/mail/incoming.py (+19/-15)
lib/lp/blueprints/doc/spec-mail-exploder.txt (+1/-1)
To merge this branch: bzr merge lp:~gary/launchpad/bug650343
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) Approve
Māris Fogels (community) Approve
Review via email: mp+37633@code.launchpad.net

Description of the change

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 ','

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Sorry, bug 650343.

Revision history for this message
Māris Fogels (mars) wrote :

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
Revision history for this message
Jonathan Lange (jml) wrote :

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

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

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

review: Needs Fixing
Revision history for this message
Gary Poster (gary) wrote :

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.

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

Looks good now, thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1= Incoming Mail =1Incoming Mail
2=============
23
3When an email is sent to Launchpad we need to handle it somehow. This4When an email is sent to Launchpad we need to handle it somehow. This
4is done by handleEmails:5is done by handleEmails:
@@ -14,7 +15,9 @@
14 * Deletes the message from the mail box15 * Deletes the message from the mail box
1516
1617
17== Mail Handlers ==18-------------
19Mail Handlers
20-------------
1821
19A mail handler is a utility which knows how to handle mail sent to a22A mail handler is a utility which knows how to handle mail sent to a
20specific domain. It is registered as a named utility providing23specific domain. It is registered as a named utility providing
@@ -79,9 +82,16 @@
79 >>> config.push('bugmail_error_from_address', bugmail_error_from_address)82 >>> config.push('bugmail_error_from_address', bugmail_error_from_address)
8083
81The test mails are now in Launchpad's mail box, so now we can call84The test mails are now in Launchpad's mail box, so now we can call
82handleMail, so that every mail gets handled by the correct handler.85handleMail, so that every mail gets handled by the correct handler. (We
86see warnings about missing `X-Launchpad-Original-To`_ headers, which are
87discussed below; this output merely shows that we emit warnings when the
88header is missing.)
8389
84 >>> handleMail(zopeless_transaction)90 >>> handleMail(zopeless_transaction)
91 WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
92 WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
93 WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
94 WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
8595
86Now we can see that each handler handled the emails sent to its domain:96Now we can see that each handler handled the emails sent to its domain:
8797
@@ -90,8 +100,9 @@
90 >>> set(bar_handler.handledMails) ^ set(msgids['bar.com'])100 >>> set(bar_handler.handledMails) ^ set(msgids['bar.com'])
91 set([])101 set([])
92102
93103--------------
94== Unhandled Mail ==104Unhandled Mail
105--------------
95106
96So, what happened to the message that got sent to baz.com? Since there107So, what happened to the message that got sent to baz.com? Since there
97wasn't a handler registered for that domain, an OOPS was recorded with108wasn't a handler registered for that domain, an OOPS was recorded with
@@ -118,14 +129,15 @@
118129
119 >>> stub.test_emails = []130 >>> stub.test_emails = []
120131
121132---------------------------------------------
122== Mail from Persons not registered in Launchpad ==133Mail from Persons not registered in Launchpad
134---------------------------------------------
123135
124If a Person who isn't registered in Launchpad sends an email, we'll136If a Person who isn't registered in Launchpad sends an email, we'll
125most of the time reject the email:137most of the time reject the email:
126138
127 >>> moin_change = read_test_message('moin-change.txt')139 >>> moin_change = read_test_message('moin-change.txt')
128 >>> moin_change['X-Original-To'] = '123@foo.com'140 >>> moin_change['X-Launchpad-Original-To'] = '123@foo.com'
129 >>> msgid = "<%s>" % sendmail(moin_change)141 >>> msgid = "<%s>" % sendmail(moin_change)
130 >>> handleMail(zopeless_transaction)142 >>> handleMail(zopeless_transaction)
131 >>> msgid not in foo_handler.handledMails143 >>> msgid not in foo_handler.handledMails
@@ -140,7 +152,7 @@
140152
141So if we send the mail to bar.com, bar_handler will handle the mail:153So if we send the mail to bar.com, bar_handler will handle the mail:
142154
143 >>> moin_change.replace_header('To', '123@bar.com')155 >>> moin_change.replace_header('X-Launchpad-Original-To', '123@bar.com')
144 >>> msgid = "<%s>" % sendmail(moin_change)156 >>> msgid = "<%s>" % sendmail(moin_change)
145 >>> handleMail(zopeless_transaction)157 >>> handleMail(zopeless_transaction)
146 >>> msgid in bar_handler.handledMails158 >>> msgid in bar_handler.handledMails
@@ -148,8 +160,9 @@
148160
149 >>> stub.test_emails = []161 >>> stub.test_emails = []
150162
151163---------------------------------------------------------
152== Mail from Persons with with an inactive Launchpad account ==164Mail from Persons with with an inactive Launchpad account
165---------------------------------------------------------
153166
154If a Person who's account is inactive sends an email, it will be167If a Person who's account is inactive sends an email, it will be
155silently rejected.168silently rejected.
@@ -173,15 +186,31 @@
173 >>> msgid not in foo_handler.handledMails186 >>> msgid not in foo_handler.handledMails
174 True187 True
175188
176== X-Original-To ==189-----------------------
177190X-Launchpad-Original-To
178Ideally, we would use X-Original-To, as this reflects the actual address191-----------------------
179that the mail was sent to. Unfortunately, due to mail configuration issues,192
180X-Original-To is very wrong, so we must rely on To and Cc, even though these193If available, the X-Launchpad-Original-To header is used to determine to
181may be inaccurate.194which address the email was sent to:
182195
183196 >>> msg = read_test_message('signed_detached.txt')
184== OOPSes processing incoming mail ==197 >>> msg.replace_header('To', '123@foo.com')
198 >>> msg['CC'] = '123@foo.com'
199 >>> msg['X-Launchpad-Original-To'] = '123@bar.com'
200 >>> msgid = '<%s>' % sendmail (msg, ['123@bar.com'])
201 >>> handleMail(zopeless_transaction)
202 >>> msgid in bar_handler.handledMails
203 True
204
205Only the address in X-Launchpad-Original-To header will be used. The
206addresses in the To and CC headers will be ignored:
207
208 >>> msgid in foo_handler.handledMails
209 False
210
211-------------------------------
212OOPSes processing incoming mail
213-------------------------------
185214
186If an unhandled exception occurs when we try to process an email from215If an unhandled exception occurs when we try to process an email from
187a user, we record an OOPS with the exception and send it to the user.216a user, we record an OOPS with the exception and send it to the user.
@@ -203,7 +232,7 @@
203 >>> msg = email.message_from_string(232 >>> msg = email.message_from_string(
204 ... """From: Foo Bar <foo.bar@canonical.com>233 ... """From: Foo Bar <foo.bar@canonical.com>
205 ... To: launchpad@oops.com234 ... To: launchpad@oops.com
206 ... X-Original-To: launchpad@oops.com235 ... X-Launchpad-Original-To: launchpad@oops.com
207 ... Subject: doesn't matter236 ... Subject: doesn't matter
208 ...237 ...
209 ... doesn't matter238 ... doesn't matter
@@ -232,7 +261,7 @@
232 ...261 ...
233 From: Foo Bar <foo.bar@canonical.com>262 From: Foo Bar <foo.bar@canonical.com>
234 To: launchpad@oops.com263 To: launchpad@oops.com
235 X-Original-To: launchpad@oops.com264 X-Launchpad-Original-To: launchpad@oops.com
236 Subject: doesn't matter265 Subject: doesn't matter
237 ...266 ...
238267
@@ -251,7 +280,7 @@
251 >>> msg = email.message_from_string(280 >>> msg = email.message_from_string(
252 ... """From: Foo Bar <foo.bar@canonical.com>281 ... """From: Foo Bar <foo.bar@canonical.com>
253 ... To: launchpad@unauthorized.com282 ... To: launchpad@unauthorized.com
254 ... X-Original-To: launchpad@unauthorized.com283 ... X-Launchpad-Original-To: launchpad@unauthorized.com
255 ... Subject: doesn't matter284 ... Subject: doesn't matter
256 ...285 ...
257 ... doesn't matter286 ... doesn't matter
@@ -277,14 +306,15 @@
277 ...306 ...
278 From: Foo Bar <foo.bar@canonical.com>307 From: Foo Bar <foo.bar@canonical.com>
279 To: launchpad@unauthorized.com308 To: launchpad@unauthorized.com
280 X-Original-To: launchpad@unauthorized.com309 X-Launchpad-Original-To: launchpad@unauthorized.com
281 Subject: doesn't matter310 Subject: doesn't matter
282 ...311 ...
283312
284 >>> stub.test_emails = []313 >>> stub.test_emails = []
285314
286315-------------
287== DB exceptions ==316DB exceptions
317-------------
288318
289If something goes wrongs in the handler, a DB exception can be raised,319If something goes wrongs in the handler, a DB exception can be raised,
290leaving the database in a bad state. If that happens a traceback should320leaving the database in a bad state. If that happens a traceback should
@@ -305,7 +335,7 @@
305 >>> exception_raiser = email.message_from_string(335 >>> exception_raiser = email.message_from_string(
306 ... """From: Foo Bar <foo.bar@canonical.com>336 ... """From: Foo Bar <foo.bar@canonical.com>
307 ... To: something@except.com337 ... To: something@except.com
308 ... X-Original-To: something@except.com338 ... X-Launchpad-Original-To: something@except.com
309 ... Subject: Raise an exception339 ... Subject: Raise an exception
310 ...340 ...
311 ... This part is not important.341 ... This part is not important.
@@ -328,6 +358,7 @@
328 ...358 ...
329 DataError: division by zero359 DataError: division by zero
330 <BLANKLINE>360 <BLANKLINE>
361 WARNING...
331362
332The second mail we sent got handled despite the exception:363The second mail we sent got handled despite the exception:
333364
@@ -340,8 +371,9 @@
340 >>> len(stub.test_emails)371 >>> len(stub.test_emails)
341 1372 1
342373
343374---------------------
344== Librarian not running ==375Librarian not running
376---------------------
345377
346If for some reason the Librarian isn't up and running, we shouldn't378If for some reason the Librarian isn't up and running, we shouldn't
347lose any emails. All that should happen is that an error should get379lose any emails. All that should happen is that an error should get
@@ -368,8 +400,9 @@
368 >>> LibrarianLayer.reveal()400 >>> LibrarianLayer.reveal()
369 >>> stub.test_emails = []401 >>> stub.test_emails = []
370402
371403----------------
372== Handling bounces ==404Handling bounces
405----------------
373406
374Some broken mailers might not respect the Errors-To and Return-Path407Some broken mailers might not respect the Errors-To and Return-Path
375headers, send error messages back to the address, from which the email408headers, send error messages back to the address, from which the email
@@ -427,7 +460,7 @@
427 0460 0
428461
429462
430== Doctest cleanup ==463.. Doctest cleanup
431464
432 >>> config_data = config.pop('bugmail_error_from_address')465 >>> config_data = config.pop('bugmail_error_from_address')
433 >>> mail_handlers.add('foo.com', None)466 >>> mail_handlers.add('foo.com', None)
434467
=== 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 @@
149 % (signing_domain, from_domain))149 % (signing_domain, from_domain))
150 return False150 return False
151 if not _isDkimDomainTrusted(signing_domain):151 if not _isDkimDomainTrusted(signing_domain):
152 log.warning("valid DKIM signature from untrusted domain %s" 152 log.warning("valid DKIM signature from untrusted domain %s"
153 % (signing_domain,))153 % (signing_domain,))
154 return False154 return False
155 return True155 return True
@@ -338,8 +338,7 @@
338 if mail['Return-Path'] == '<>':338 if mail['Return-Path'] == '<>':
339 _handle_error(339 _handle_error(
340 "Message had an empty Return-Path.",340 "Message had an empty Return-Path.",
341 file_alias_url, notify=False341 file_alias_url, notify=False)
342 )
343 continue342 continue
344 if mail.get_content_type() == 'multipart/report':343 if mail.get_content_type() == 'multipart/report':
345 # Mails with a content type of multipart/report are344 # Mails with a content type of multipart/report are
@@ -351,8 +350,7 @@
351 if 'precedence' in mail:350 if 'precedence' in mail:
352 _handle_error(351 _handle_error(
353 "Got a message with a precedence header.",352 "Got a message with a precedence header.",
354 file_alias_url, notify=False353 file_alias_url, notify=False)
355 )
356 continue354 continue
357355
358 try:356 try:
@@ -366,14 +364,21 @@
366 file_alias_url, notify=False)364 file_alias_url, notify=False)
367 continue365 continue
368366
369 # Extract the domain the mail was sent to. Mails sent to367 # Extract the domain the mail was sent to. Mails sent to
370 # Launchpad should have an X-Original-To header, but368 # Launchpad should have an X-Launchpad-Original-To header.
371 # it has an incorrect address.369 if 'X-Launchpad-Original-To' in mail:
372 # Process all addresses found as a fall back.370 addresses = [mail['X-Launchpad-Original-To']]
373 cc = mail.get_all('cc') or []371 else:
374 to = mail.get_all('to') or []372 log = logging.getLogger('canonical.launchpad.mail')
375 names_addresses = getaddresses(to + cc)373 log.warn(
376 addresses = [addr for name, addr in names_addresses]374 "No X-Launchpad-Original-To header was present "
375 "in email: %s" %
376 file_alias_url)
377 # Process all addresses found as a fall back.
378 cc = mail.get_all('cc') or []
379 to = mail.get_all('to') or []
380 names_addresses = getaddresses(to + cc)
381 addresses = [addr for name, addr in names_addresses]
377382
378 try:383 try:
379 do_paranoid_envelope_to_validation(addresses)384 do_paranoid_envelope_to_validation(addresses)
@@ -400,8 +405,7 @@
400 if principal is None and not handler.allow_unknown_users:405 if principal is None and not handler.allow_unknown_users:
401 _handle_error(406 _handle_error(
402 'Unknown user: %s ' % mail['From'],407 'Unknown user: %s ' % mail['From'],
403 file_alias_url, notify=False408 file_alias_url, notify=False)
404 )
405 continue409 continue
406410
407 handled = handler.process(mail, email_addr, file_alias)411 handled = handler.process(mail, email_addr, file_alias)
408412
=== 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 @@
264 True264 True
265265
266 >>> moin_change = read_test_message('moin-change.txt')266 >>> moin_change = read_test_message('moin-change.txt')
267 >>> moin_change['To'] += ", notifications@%s" % (267 >>> moin_change['X-Launchpad-Original-To'] = "notifications@%s" % (
268 ... config.launchpad.specs_domain)268 ... config.launchpad.specs_domain)
269 >>> moin_change['Sender'] = 'webmaster@ubuntu.com'269 >>> moin_change['Sender'] = 'webmaster@ubuntu.com'
270270