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
1=== modified file 'lib/canonical/launchpad/doc/incomingmail.txt'
2--- lib/canonical/launchpad/doc/incomingmail.txt 2010-10-04 19:50:45 +0000
3+++ lib/canonical/launchpad/doc/incomingmail.txt 2010-10-12 13:37:43 +0000
4@@ -1,4 +1,5 @@
5-= Incoming Mail =
6+Incoming Mail
7+=============
8
9 When an email is sent to Launchpad we need to handle it somehow. This
10 is done by handleEmails:
11@@ -14,7 +15,9 @@
12 * Deletes the message from the mail box
13
14
15-== Mail Handlers ==
16+-------------
17+Mail Handlers
18+-------------
19
20 A mail handler is a utility which knows how to handle mail sent to a
21 specific domain. It is registered as a named utility providing
22@@ -79,9 +82,16 @@
23 >>> config.push('bugmail_error_from_address', bugmail_error_from_address)
24
25 The test mails are now in Launchpad's mail box, so now we can call
26-handleMail, so that every mail gets handled by the correct handler.
27+handleMail, so that every mail gets handled by the correct handler. (We
28+see warnings about missing `X-Launchpad-Original-To`_ headers, which are
29+discussed below; this output merely shows that we emit warnings when the
30+header is missing.)
31
32 >>> handleMail(zopeless_transaction)
33+ WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
34+ WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
35+ WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
36+ WARNING:canonical.launchpad.mail:No X-Launchpad-Original-To header was present ...
37
38 Now we can see that each handler handled the emails sent to its domain:
39
40@@ -90,8 +100,9 @@
41 >>> set(bar_handler.handledMails) ^ set(msgids['bar.com'])
42 set([])
43
44-
45-== Unhandled Mail ==
46+--------------
47+Unhandled Mail
48+--------------
49
50 So, what happened to the message that got sent to baz.com? Since there
51 wasn't a handler registered for that domain, an OOPS was recorded with
52@@ -118,14 +129,15 @@
53
54 >>> stub.test_emails = []
55
56-
57-== Mail from Persons not registered in Launchpad ==
58+---------------------------------------------
59+Mail from Persons not registered in Launchpad
60+---------------------------------------------
61
62 If a Person who isn't registered in Launchpad sends an email, we'll
63 most of the time reject the email:
64
65 >>> moin_change = read_test_message('moin-change.txt')
66- >>> moin_change['X-Original-To'] = '123@foo.com'
67+ >>> moin_change['X-Launchpad-Original-To'] = '123@foo.com'
68 >>> msgid = "<%s>" % sendmail(moin_change)
69 >>> handleMail(zopeless_transaction)
70 >>> msgid not in foo_handler.handledMails
71@@ -140,7 +152,7 @@
72
73 So if we send the mail to bar.com, bar_handler will handle the mail:
74
75- >>> moin_change.replace_header('To', '123@bar.com')
76+ >>> moin_change.replace_header('X-Launchpad-Original-To', '123@bar.com')
77 >>> msgid = "<%s>" % sendmail(moin_change)
78 >>> handleMail(zopeless_transaction)
79 >>> msgid in bar_handler.handledMails
80@@ -148,8 +160,9 @@
81
82 >>> stub.test_emails = []
83
84-
85-== Mail from Persons with with an inactive Launchpad account ==
86+---------------------------------------------------------
87+Mail from Persons with with an inactive Launchpad account
88+---------------------------------------------------------
89
90 If a Person who's account is inactive sends an email, it will be
91 silently rejected.
92@@ -173,15 +186,31 @@
93 >>> msgid not in foo_handler.handledMails
94 True
95
96-== X-Original-To ==
97-
98-Ideally, we would use X-Original-To, as this reflects the actual address
99-that the mail was sent to. Unfortunately, due to mail configuration issues,
100-X-Original-To is very wrong, so we must rely on To and Cc, even though these
101-may be inaccurate.
102-
103-
104-== OOPSes processing incoming mail ==
105+-----------------------
106+X-Launchpad-Original-To
107+-----------------------
108+
109+If available, the X-Launchpad-Original-To header is used to determine to
110+which address the email was sent to:
111+
112+ >>> msg = read_test_message('signed_detached.txt')
113+ >>> msg.replace_header('To', '123@foo.com')
114+ >>> msg['CC'] = '123@foo.com'
115+ >>> msg['X-Launchpad-Original-To'] = '123@bar.com'
116+ >>> msgid = '<%s>' % sendmail (msg, ['123@bar.com'])
117+ >>> handleMail(zopeless_transaction)
118+ >>> msgid in bar_handler.handledMails
119+ True
120+
121+Only the address in X-Launchpad-Original-To header will be used. The
122+addresses in the To and CC headers will be ignored:
123+
124+ >>> msgid in foo_handler.handledMails
125+ False
126+
127+-------------------------------
128+OOPSes processing incoming mail
129+-------------------------------
130
131 If an unhandled exception occurs when we try to process an email from
132 a user, we record an OOPS with the exception and send it to the user.
133@@ -203,7 +232,7 @@
134 >>> msg = email.message_from_string(
135 ... """From: Foo Bar <foo.bar@canonical.com>
136 ... To: launchpad@oops.com
137- ... X-Original-To: launchpad@oops.com
138+ ... X-Launchpad-Original-To: launchpad@oops.com
139 ... Subject: doesn't matter
140 ...
141 ... doesn't matter
142@@ -232,7 +261,7 @@
143 ...
144 From: Foo Bar <foo.bar@canonical.com>
145 To: launchpad@oops.com
146- X-Original-To: launchpad@oops.com
147+ X-Launchpad-Original-To: launchpad@oops.com
148 Subject: doesn't matter
149 ...
150
151@@ -251,7 +280,7 @@
152 >>> msg = email.message_from_string(
153 ... """From: Foo Bar <foo.bar@canonical.com>
154 ... To: launchpad@unauthorized.com
155- ... X-Original-To: launchpad@unauthorized.com
156+ ... X-Launchpad-Original-To: launchpad@unauthorized.com
157 ... Subject: doesn't matter
158 ...
159 ... doesn't matter
160@@ -277,14 +306,15 @@
161 ...
162 From: Foo Bar <foo.bar@canonical.com>
163 To: launchpad@unauthorized.com
164- X-Original-To: launchpad@unauthorized.com
165+ X-Launchpad-Original-To: launchpad@unauthorized.com
166 Subject: doesn't matter
167 ...
168
169 >>> stub.test_emails = []
170
171-
172-== DB exceptions ==
173+-------------
174+DB exceptions
175+-------------
176
177 If something goes wrongs in the handler, a DB exception can be raised,
178 leaving the database in a bad state. If that happens a traceback should
179@@ -305,7 +335,7 @@
180 >>> exception_raiser = email.message_from_string(
181 ... """From: Foo Bar <foo.bar@canonical.com>
182 ... To: something@except.com
183- ... X-Original-To: something@except.com
184+ ... X-Launchpad-Original-To: something@except.com
185 ... Subject: Raise an exception
186 ...
187 ... This part is not important.
188@@ -328,6 +358,7 @@
189 ...
190 DataError: division by zero
191 <BLANKLINE>
192+ WARNING...
193
194 The second mail we sent got handled despite the exception:
195
196@@ -340,8 +371,9 @@
197 >>> len(stub.test_emails)
198 1
199
200-
201-== Librarian not running ==
202+---------------------
203+Librarian not running
204+---------------------
205
206 If for some reason the Librarian isn't up and running, we shouldn't
207 lose any emails. All that should happen is that an error should get
208@@ -368,8 +400,9 @@
209 >>> LibrarianLayer.reveal()
210 >>> stub.test_emails = []
211
212-
213-== Handling bounces ==
214+----------------
215+Handling bounces
216+----------------
217
218 Some broken mailers might not respect the Errors-To and Return-Path
219 headers, send error messages back to the address, from which the email
220@@ -427,7 +460,7 @@
221 0
222
223
224-== Doctest cleanup ==
225+.. Doctest cleanup
226
227 >>> config_data = config.pop('bugmail_error_from_address')
228 >>> mail_handlers.add('foo.com', None)
229
230=== modified file 'lib/canonical/launchpad/mail/incoming.py'
231--- lib/canonical/launchpad/mail/incoming.py 2010-10-03 15:30:06 +0000
232+++ lib/canonical/launchpad/mail/incoming.py 2010-10-12 13:37:43 +0000
233@@ -149,7 +149,7 @@
234 % (signing_domain, from_domain))
235 return False
236 if not _isDkimDomainTrusted(signing_domain):
237- log.warning("valid DKIM signature from untrusted domain %s"
238+ log.warning("valid DKIM signature from untrusted domain %s"
239 % (signing_domain,))
240 return False
241 return True
242@@ -338,8 +338,7 @@
243 if mail['Return-Path'] == '<>':
244 _handle_error(
245 "Message had an empty Return-Path.",
246- file_alias_url, notify=False
247- )
248+ file_alias_url, notify=False)
249 continue
250 if mail.get_content_type() == 'multipart/report':
251 # Mails with a content type of multipart/report are
252@@ -351,8 +350,7 @@
253 if 'precedence' in mail:
254 _handle_error(
255 "Got a message with a precedence header.",
256- file_alias_url, notify=False
257- )
258+ file_alias_url, notify=False)
259 continue
260
261 try:
262@@ -366,14 +364,21 @@
263 file_alias_url, notify=False)
264 continue
265
266- # Extract the domain the mail was sent to. Mails sent to
267- # Launchpad should have an X-Original-To header, but
268- # it has an incorrect address.
269- # Process all addresses found as a fall back.
270- cc = mail.get_all('cc') or []
271- to = mail.get_all('to') or []
272- names_addresses = getaddresses(to + cc)
273- addresses = [addr for name, addr in names_addresses]
274+ # Extract the domain the mail was sent to. Mails sent to
275+ # Launchpad should have an X-Launchpad-Original-To header.
276+ if 'X-Launchpad-Original-To' in mail:
277+ addresses = [mail['X-Launchpad-Original-To']]
278+ else:
279+ log = logging.getLogger('canonical.launchpad.mail')
280+ log.warn(
281+ "No X-Launchpad-Original-To header was present "
282+ "in email: %s" %
283+ file_alias_url)
284+ # Process all addresses found as a fall back.
285+ cc = mail.get_all('cc') or []
286+ to = mail.get_all('to') or []
287+ names_addresses = getaddresses(to + cc)
288+ addresses = [addr for name, addr in names_addresses]
289
290 try:
291 do_paranoid_envelope_to_validation(addresses)
292@@ -400,8 +405,7 @@
293 if principal is None and not handler.allow_unknown_users:
294 _handle_error(
295 'Unknown user: %s ' % mail['From'],
296- file_alias_url, notify=False
297- )
298+ file_alias_url, notify=False)
299 continue
300
301 handled = handler.process(mail, email_addr, file_alias)
302
303=== modified file 'lib/lp/blueprints/doc/spec-mail-exploder.txt'
304--- lib/lp/blueprints/doc/spec-mail-exploder.txt 2010-07-30 12:56:27 +0000
305+++ lib/lp/blueprints/doc/spec-mail-exploder.txt 2010-10-12 13:37:43 +0000
306@@ -264,7 +264,7 @@
307 True
308
309 >>> moin_change = read_test_message('moin-change.txt')
310- >>> moin_change['To'] += ", notifications@%s" % (
311+ >>> moin_change['X-Launchpad-Original-To'] = "notifications@%s" % (
312 ... config.launchpad.specs_domain)
313 >>> moin_change['Sender'] = 'webmaster@ubuntu.com'
314