Merge lp:~thumper/launchpad/catch-email-send-failures into lp:launchpad
- catch-email-send-failures
- Merge into devel
Proposed by
Tim Penhey
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | not available | ||||
Proposed branch: | lp:~thumper/launchpad/catch-email-send-failures | ||||
Merge into: | lp:launchpad | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~thumper/launchpad/catch-email-send-failures | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Abel Deuring (community) | Approve | ||
Review via email:
|
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Penhey (thumper) wrote : | # |
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Abel Deuring (adeuring) wrote : | # |
Hi Tim,
a nice branch. Just two nitpicks.
Abel
+ def test_generateEm
+ # If BaseMailer.
s/getnerateEmai
+ def test_sendall_
+ # A failure to send to a particular email address doesn't stop ending
+ # to others.
s/ending/sending/
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/services/mail/basemailer.py' |
2 | --- lib/lp/services/mail/basemailer.py 2009-07-17 00:26:05 +0000 |
3 | +++ lib/lp/services/mail/basemailer.py 2009-09-04 05:02:48 +0000 |
4 | @@ -7,12 +7,15 @@ |
5 | |
6 | __all__ = ['BaseMailer'] |
7 | |
8 | +import logging |
9 | +from smtplib import SMTPException |
10 | |
11 | from canonical.launchpad.helpers import get_email_template |
12 | -from canonical.launchpad.mail import format_address, MailController |
13 | from canonical.launchpad.mailout import text_delta |
14 | + |
15 | from lp.services.mail.notificationrecipientset import ( |
16 | NotificationRecipientSet) |
17 | +from lp.services.mail.sendmail import format_address, MailController |
18 | |
19 | |
20 | class BaseMailer: |
21 | @@ -26,7 +29,8 @@ |
22 | """ |
23 | |
24 | def __init__(self, subject, template_name, recipients, from_address, |
25 | - delta=None, message_id=None, notification_type=None): |
26 | + delta=None, message_id=None, notification_type=None, |
27 | + mail_controller_class=None): |
28 | """Constructor. |
29 | |
30 | :param subject: A Python dict-replacement template for the subject |
31 | @@ -38,6 +42,8 @@ |
32 | and "new_values", such as BranchMergeProposalDelta. |
33 | :param message_id: The Message-Id to use for generated emails. If |
34 | not supplied, random message-ids will be used. |
35 | + :param mail_controller_class: The class of the mail controller to |
36 | + use to send the mails. Defaults to `MailController`. |
37 | """ |
38 | self._subject_template = subject |
39 | self._template_name = template_name |
40 | @@ -48,11 +54,15 @@ |
41 | self.delta = delta |
42 | self.message_id = message_id |
43 | self.notification_type = notification_type |
44 | + self.logger = logging.getLogger('lp.services.mail.basemailer') |
45 | + if mail_controller_class is None: |
46 | + mail_controller_class = MailController |
47 | + self._mail_controller_class = mail_controller_class |
48 | |
49 | def _getToAddresses(self, recipient, email): |
50 | return [format_address(recipient.displayname, email)] |
51 | |
52 | - def generateEmail(self, email, recipient): |
53 | + def generateEmail(self, email, recipient, force_no_attachments=False): |
54 | """Generate the email for this recipient. |
55 | |
56 | :param email: Email address of the recipient to send to. |
57 | @@ -63,10 +73,15 @@ |
58 | headers = self._getHeaders(email) |
59 | subject = self._getSubject(email) |
60 | body = self._getBody(email) |
61 | - ctrl = MailController( |
62 | + ctrl = self._mail_controller_class( |
63 | self.from_address, to_addresses, subject, body, headers, |
64 | envelope_to=[email]) |
65 | - self._addAttachments(ctrl, email) |
66 | + if force_no_attachments: |
67 | + ctrl.addAttachment( |
68 | + 'Excessively large attachments removed.', |
69 | + content_type='text/plain', inline=True) |
70 | + else: |
71 | + self._addAttachments(ctrl, email) |
72 | return ctrl |
73 | |
74 | def _getSubject(self, email): |
75 | @@ -119,6 +134,19 @@ |
76 | |
77 | def sendAll(self): |
78 | """Send notifications to all recipients.""" |
79 | + # We never want SMTP errors to propagate from this function. |
80 | for email, recipient in self._recipients.getRecipientPersons(): |
81 | - ctrl = self.generateEmail(email, recipient) |
82 | - ctrl.send() |
83 | + try: |
84 | + ctrl = self.generateEmail(email, recipient) |
85 | + ctrl.send() |
86 | + except SMTPException, e: |
87 | + # If the initial sending failed, try again without |
88 | + # attachments. |
89 | + try: |
90 | + ctrl = self.generateEmail( |
91 | + email, recipient, force_no_attachments=True) |
92 | + ctrl.send() |
93 | + except SMTPException, e: |
94 | + # Don't want an entire stack trace, just some details. |
95 | + self.logger.warning( |
96 | + 'send failed for %s, %s' % (email, e)) |
97 | |
98 | === modified file 'lib/lp/services/mail/tests/test_basemailer.py' |
99 | --- lib/lp/services/mail/tests/test_basemailer.py 2009-07-17 02:25:09 +0000 |
100 | +++ lib/lp/services/mail/tests/test_basemailer.py 2009-09-04 05:02:48 +0000 |
101 | @@ -1,15 +1,19 @@ |
102 | # Copyright 2009 Canonical Ltd. This software is licensed under the |
103 | # GNU Affero General Public License version 3 (see the file LICENSE). |
104 | +"""Tests for the BaseMailer class.""" |
105 | + |
106 | |
107 | __metaclass__ = type |
108 | |
109 | - |
110 | +from smtplib import SMTPException |
111 | import unittest |
112 | |
113 | from canonical.testing import LaunchpadZopelessLayer |
114 | |
115 | from lp.testing import TestCaseWithFactory |
116 | +from lp.testing.mail_helpers import pop_notifications |
117 | from lp.services.mail.basemailer import BaseMailer |
118 | +from lp.services.mail.sendmail import MailController |
119 | |
120 | |
121 | class FakeSubscription: |
122 | @@ -35,6 +39,44 @@ |
123 | return email.upper() |
124 | |
125 | |
126 | +class AttachmentMailer(BaseMailerSubclass): |
127 | + """Subclass the test mailer to add an attachment.""" |
128 | + |
129 | + def _addAttachments(self, ctrl, email): |
130 | + ctrl.addAttachment('attachment1') |
131 | + ctrl.addAttachment('attachment2') |
132 | + |
133 | + |
134 | +class RaisingMailController(MailController): |
135 | + """A mail controller that can raise errors.""" |
136 | + |
137 | + def raiseOnSend(self): |
138 | + """Make send fail for the specified email address.""" |
139 | + self.raise_on_send = True |
140 | + |
141 | + def send(self, bulk=True): |
142 | + if getattr(self, 'raise_on_send', False): |
143 | + raise SMTPException('boom') |
144 | + else: |
145 | + super(RaisingMailController, self).send(bulk) |
146 | + |
147 | + |
148 | +class RaisingMailControllerFactory: |
149 | + """Pretends to be a class to make raising mail controllers.""" |
150 | + |
151 | + def __init__(self, bad_email_addr, raise_count): |
152 | + self.bad_email_addr = bad_email_addr |
153 | + self.raise_count = raise_count |
154 | + |
155 | + def __call__(self, *args, **kwargs): |
156 | + ctrl = RaisingMailController(*args, **kwargs) |
157 | + if ((self.bad_email_addr in kwargs['envelope_to']) |
158 | + and self.raise_count): |
159 | + self.raise_count -= 1 |
160 | + ctrl.raiseOnSend() |
161 | + return ctrl |
162 | + |
163 | + |
164 | class TestBaseMailer(TestCaseWithFactory): |
165 | |
166 | layer = LaunchpadZopelessLayer |
167 | @@ -47,8 +89,8 @@ |
168 | fake_to = self.factory.makePerson(email='to@example.com', |
169 | displayname='Example To') |
170 | recipients = {fake_to: FakeSubscription()} |
171 | - mailer = BaseMailerSubclass('subject', None, recipients, |
172 | - 'from@example.com') |
173 | + mailer = BaseMailerSubclass( |
174 | + 'subject', None, recipients, 'from@example.com') |
175 | ctrl = mailer.generateEmail('to@example.com', fake_to) |
176 | self.assertEqual(['to@example.com'], ctrl.envelope_to) |
177 | self.assertEqual(['Example To <to@example.com>'], ctrl.to_addrs) |
178 | @@ -61,11 +103,90 @@ |
179 | """ |
180 | fake_to = self.factory.makePerson(email='to@example.com') |
181 | recipients = {fake_to: FakeSubscription()} |
182 | - mailer = ToAddressesUpper('subject', None, recipients, |
183 | - 'from@example.com') |
184 | + mailer = ToAddressesUpper( |
185 | + 'subject', None, recipients, 'from@example.com') |
186 | ctrl = mailer.generateEmail('to@example.com', fake_to) |
187 | self.assertEqual(['TO@EXAMPLE.COM'], ctrl.to_addrs) |
188 | |
189 | + def test_generateEmail_adds_attachments(self): |
190 | + # BaseMailer.generateEmail calls _addAttachments. |
191 | + fake_to = self.factory.makePerson(email='to@example.com') |
192 | + recipients = {fake_to: FakeSubscription()} |
193 | + mailer = AttachmentMailer( |
194 | + 'subject', None, recipients, 'from@example.com') |
195 | + ctrl = mailer.generateEmail('to@example.com', fake_to) |
196 | + self.assertEqual(2, len(ctrl.attachments)) |
197 | + |
198 | + def test_generateEmail_force_no_attachments(self): |
199 | + # If BaseMailer.getnerateEmail is called with |
200 | + # force_no_attachments=True then attachments are not added. |
201 | + fake_to = self.factory.makePerson(email='to@example.com') |
202 | + recipients = {fake_to: FakeSubscription()} |
203 | + mailer = AttachmentMailer( |
204 | + 'subject', None, recipients, 'from@example.com') |
205 | + ctrl = mailer.generateEmail( |
206 | + 'to@example.com', fake_to, force_no_attachments=True) |
207 | + self.assertEqual(1, len(ctrl.attachments)) |
208 | + attachment = ctrl.attachments[0] |
209 | + self.assertEqual( |
210 | + 'Excessively large attachments removed.', |
211 | + attachment.get_payload()) |
212 | + self.assertEqual('text/plain', attachment['Content-Type']) |
213 | + self.assertEqual('inline', attachment['Content-Disposition']) |
214 | + |
215 | + def test_sendall_single_failure_doesnt_kill_all(self): |
216 | + # A failure to send to a particular email address doesn't stop ending |
217 | + # to others. |
218 | + recipients = { |
219 | + self.factory.makePerson(name='good', email='good@example.com'): |
220 | + FakeSubscription(), |
221 | + self.factory.makePerson(name='bad', email='bad@example.com'): |
222 | + FakeSubscription()} |
223 | + controller_factory = RaisingMailControllerFactory( |
224 | + 'bad@example.com', 2) |
225 | + mailer = BaseMailerSubclass( |
226 | + 'subject', None, recipients, 'from@example.com', |
227 | + mail_controller_class=controller_factory) |
228 | + mailer.sendAll() |
229 | + # One email is still sent. |
230 | + notifications = pop_notifications() |
231 | + self.assertEqual(1, len(notifications)) |
232 | + self.assertEqual('Good <good@example.com>', notifications[0]['To']) |
233 | + |
234 | + def test_sendall_first_failure_strips_attachments(self): |
235 | + # If sending an email fails, we try again without the (almost |
236 | + # certainly) large attachment. |
237 | + recipients = { |
238 | + self.factory.makePerson(name='good', email='good@example.com'): |
239 | + FakeSubscription(), |
240 | + self.factory.makePerson(name='bad', email='bad@example.com'): |
241 | + FakeSubscription()} |
242 | + # Only raise the first time for bob. |
243 | + controller_factory = RaisingMailControllerFactory( |
244 | + 'bad@example.com', 1) |
245 | + mailer = AttachmentMailer( |
246 | + 'subject', None, recipients, 'from@example.com', |
247 | + mail_controller_class=controller_factory) |
248 | + mailer.sendAll() |
249 | + # Both emails are sent. |
250 | + notifications = pop_notifications() |
251 | + self.assertEqual(2, len(notifications)) |
252 | + bad, good = notifications |
253 | + # The good email as the expected attachments. |
254 | + good_parts = good.get_payload() |
255 | + self.assertEqual(3, len(good_parts)) |
256 | + self.assertEqual( |
257 | + 'attachment1', good_parts[1].get_payload(decode=True)) |
258 | + self.assertEqual( |
259 | + 'attachment2', good_parts[2].get_payload(decode=True)) |
260 | + # The bad email has the normal attachments stripped off and replaced |
261 | + # with the text. |
262 | + bad_parts = bad.get_payload() |
263 | + self.assertEqual(2, len(bad_parts)) |
264 | + self.assertEqual( |
265 | + 'Excessively large attachments removed.', |
266 | + bad_parts[1].get_payload(decode=True)) |
267 | + |
268 | |
269 | def test_suite(): |
270 | suite = unittest.TestSuite() |
= Summary =
Found partly due to tracking down why a MySQL branch subscriber failed to get
an email about a new merge proposal. One of the subscribers was going to be
getting a very large diff, which caused the SMTP server to barf. This caused
no emails to be sent out, and the proposal wasn't created.
Also fixes: https:/ /bugs.edge. launchpad. net/launchpad- code/+bug/ 139071
== Proposed fix ==
Check in the BaseMailer.sendAll method to make sure we catch and handle
STMPExceptions.
One of the main reasons we get this message is due to a size problem with the
attachments, so we add a plain text attachment that informs the user that we
didn't attach a large one and try again.
== Tests ==
TestBaseMailer
In order to simulate SMTP send failures, I had to create a new test mail
controller, and instead of messing with metaclasses, used a factory that is
passed in to override the default MailController class used by the BaseMailer.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: services/ mail/tests/ test_basemailer .py services/ mail/basemailer .py
lib/lp/
lib/lp/