Merge lp:~cjwatson/launchpad/send-bug-notifications-oops into lp:launchpad

Proposed by Colin Watson
Status: Merged
Merged at revision: 17682
Proposed branch: lp:~cjwatson/launchpad/send-bug-notifications-oops
Merge into: lp:launchpad
Diff against target: 290 lines (+143/-47)
3 files modified
cronscripts/send-bug-notifications.py (+1/-44)
lib/lp/bugs/scripts/bugnotification.py (+68/-2)
lib/lp/bugs/scripts/tests/test_bugnotification.py (+74/-1)
To merge this branch: bzr merge lp:~cjwatson/launchpad/send-bug-notifications-oops
Reviewer Review Type Date Requested Status
William Grant code Approve
Review via email: mp+264814@code.launchpad.net

Commit message

Turn SMTPExceptions when sending bug notifications into OOPSes rather than letting the entire script crash.

Description of the change

Turn SMTPExceptions when sending bug notifications into OOPSes rather than letting the entire script crash.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) :
review: Needs Fixing (code)
Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
William Grant (wgrant) wrote :

Thanks. I'd also include the BugNotification ID for easy debugging.

review: Approve (code)
Revision history for this message
Colin Watson (cjwatson) wrote :

As noted on IRC, this is rather too cumbersome at the moment because construct_email_notifications doesn't keep a clear record of which notification generated which message.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cronscripts/send-bug-notifications.py'
2--- cronscripts/send-bug-notifications.py 2013-01-07 02:40:55 +0000
3+++ cronscripts/send-bug-notifications.py 2015-07-21 15:47:29 +0000
4@@ -13,51 +13,8 @@
5
6 import _pythonpath
7
8-from zope.component import getUtility
9-
10-from lp.bugs.enums import BugNotificationStatus
11-from lp.bugs.interfaces.bugnotification import IBugNotificationSet
12-from lp.bugs.scripts.bugnotification import (
13- get_email_notifications,
14- process_deferred_notifications,
15- )
16+from lp.bugs.scripts.bugnotification import SendBugNotifications
17 from lp.services.config import config
18-from lp.services.database.constants import UTC_NOW
19-from lp.services.mail.sendmail import sendmail
20-from lp.services.scripts.base import LaunchpadCronScript
21-
22-
23-class SendBugNotifications(LaunchpadCronScript):
24- def main(self):
25- notifications_sent = False
26- bug_notification_set = getUtility(IBugNotificationSet)
27- deferred_notifications = \
28- bug_notification_set.getDeferredNotifications()
29- process_deferred_notifications(deferred_notifications)
30- pending_notifications = get_email_notifications(
31- bug_notification_set.getNotificationsToSend())
32- for (bug_notifications,
33- omitted_notifications,
34- messages) in pending_notifications:
35- for message in messages:
36- self.logger.info("Notifying %s about bug %d." % (
37- message['To'], bug_notifications[0].bug.id))
38- sendmail(message)
39- self.logger.debug(message.as_string())
40- for notification in bug_notifications:
41- notification.date_emailed = UTC_NOW
42- notification.status = BugNotificationStatus.SENT
43- for notification in omitted_notifications:
44- notification.date_emailed = UTC_NOW
45- notification.status = BugNotificationStatus.OMITTED
46- notifications_sent = True
47- # Commit after each batch of email sent, so that we won't
48- # re-mail the notifications in case of something going wrong
49- # in the middle.
50- self.txn.commit()
51-
52- if not notifications_sent:
53- self.logger.debug("No notifications are pending to be sent.")
54
55
56 if __name__ == '__main__':
57
58=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
59--- lib/lp/bugs/scripts/bugnotification.py 2015-07-06 17:27:09 +0000
60+++ lib/lp/bugs/scripts/bugnotification.py 2015-07-21 15:47:29 +0000
61@@ -1,4 +1,4 @@
62-# Copyright 2009 Canonical Ltd. This software is licensed under the
63+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
64 # GNU Affero General Public License version 3 (see the file LICENSE).
65
66 """Functions related to sending bug notifications."""
67@@ -13,12 +13,18 @@
68
69 from itertools import groupby
70 from operator import itemgetter
71+from smtplib import SMTPException
72+import sys
73
74 from storm.store import Store
75 import transaction
76 from zope.component import getUtility
77+from zope.error.interfaces import IErrorReportingUtility
78
79-from lp.bugs.enums import BugNotificationLevel
80+from lp.bugs.enums import (
81+ BugNotificationLevel,
82+ BugNotificationStatus,
83+ )
84 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
85 from lp.bugs.mail.bugnotificationbuilder import (
86 BugNotificationBuilder,
87@@ -26,10 +32,14 @@
88 )
89 from lp.bugs.mail.newbug import generate_bug_add_email
90 from lp.registry.model.person import get_recipients
91+from lp.services.database.constants import UTC_NOW
92 from lp.services.mail.helpers import get_email_template
93 from lp.services.mail.mailwrapper import MailWrapper
94+from lp.services.mail.sendmail import sendmail
95+from lp.services.scripts.base import LaunchpadCronScript
96 from lp.services.scripts.logger import log
97 from lp.services.webapp import canonical_url
98+from lp.services.webapp.errorlog import ScriptRequest
99
100
101 def get_activity_key(notification):
102@@ -327,3 +337,59 @@
103 message=message,
104 recipients=recipients,
105 activity=activity)
106+
107+
108+class SendBugNotifications(LaunchpadCronScript):
109+ def main(self):
110+ notifications_sent = False
111+ bug_notification_set = getUtility(IBugNotificationSet)
112+ deferred_notifications = \
113+ bug_notification_set.getDeferredNotifications()
114+ process_deferred_notifications(deferred_notifications)
115+ pending_notifications = get_email_notifications(
116+ bug_notification_set.getNotificationsToSend())
117+ for (bug_notifications,
118+ omitted_notifications,
119+ messages) in pending_notifications:
120+ try:
121+ for message in messages:
122+ try:
123+ self.logger.info("Notifying %s about bug %d." % (
124+ message['To'], bug_notifications[0].bug.id))
125+ sendmail(message)
126+ self.logger.debug(message.as_string())
127+ except SMTPException:
128+ request = ScriptRequest([
129+ ("script_name", self.name),
130+ ("path", sys.argv[0]),
131+ ])
132+ error_utility = getUtility(IErrorReportingUtility)
133+ oops_vars = {
134+ "message_id": message.get("Message-Id"),
135+ "notification_type": "bug",
136+ "recipient": message["To"],
137+ "subject": message["Subject"],
138+ }
139+ with error_utility.oopsMessage(oops_vars):
140+ error_utility.raising(sys.exc_info(), request)
141+ self.logger.info(request.oopsid)
142+ self.txn.abort()
143+ # Re-raise to get out of this loop and go to the
144+ # next iteration of the outer loop.
145+ raise
146+ except SMTPException:
147+ continue
148+ for notification in bug_notifications:
149+ notification.date_emailed = UTC_NOW
150+ notification.status = BugNotificationStatus.SENT
151+ for notification in omitted_notifications:
152+ notification.date_emailed = UTC_NOW
153+ notification.status = BugNotificationStatus.OMITTED
154+ notifications_sent = True
155+ # Commit after each batch of email sent, so that we won't
156+ # re-mail the notifications in case of something going wrong
157+ # in the middle.
158+ self.txn.commit()
159+
160+ if not notifications_sent:
161+ self.logger.debug("No notifications are pending to be sent.")
162
163=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
164--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2015-07-08 16:05:11 +0000
165+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2015-07-21 15:47:29 +0000
166@@ -1,4 +1,4 @@
167-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
168+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
169 # GNU Affero General Public License version 3 (see the file LICENSE).
170 """Tests for construction bug notification emails for sending."""
171
172@@ -10,6 +10,7 @@
173 )
174 import logging
175 import re
176+from smtplib import SMTPException
177 import StringIO
178 import unittest
179
180@@ -22,6 +23,7 @@
181 getUtility,
182 )
183 from zope.interface import implementer
184+from zope.sendmail.interfaces import IMailDelivery
185
186 from lp.app.enums import InformationType
187 from lp.bugs.adapters.bugchange import (
188@@ -38,6 +40,7 @@
189 CveLinkedToBug,
190 CveUnlinkedFromBug,
191 )
192+from lp.bugs.enums import BugNotificationStatus
193 from lp.bugs.interfaces.bug import (
194 CreateBugParams,
195 IBug,
196@@ -63,6 +66,7 @@
197 notification_batches,
198 notification_comment_batches,
199 process_deferred_notifications,
200+ SendBugNotifications,
201 )
202 from lp.registry.interfaces.person import IPersonSet
203 from lp.registry.interfaces.product import IProductSet
204@@ -76,6 +80,8 @@
205 get_contact_email_addresses,
206 get_email_template,
207 )
208+from lp.services.mail.sendmail import set_immediate_mail_delivery
209+from lp.services.mail.stub import TestMailer
210 from lp.services.messages.interfaces.message import IMessageSet
211 from lp.services.propertycache import cachedproperty
212 from lp.testing import (
213@@ -87,6 +93,7 @@
214 lp_dbuser,
215 switch_dbuser,
216 )
217+from lp.testing.fixture import ZopeUtilityFixture
218 from lp.testing.layers import LaunchpadZopelessLayer
219 from lp.testing.matchers import Contains
220
221@@ -1281,3 +1288,69 @@
222 # And there are no longer any deferred.
223 deferred = self.notification_set.getDeferredNotifications()
224 self.assertEqual(0, deferred.count())
225+
226+
227+class BrokenMailer(TestMailer):
228+ """A mailer that raises an exception for certain recipient addresses."""
229+
230+ def __init__(self, broken):
231+ self.broken = broken
232+
233+ def send(self, from_addr, to_addrs, message):
234+ if any(to_addr in self.broken for to_addr in to_addrs):
235+ raise SMTPException("test requested delivery failure")
236+ return super(BrokenMailer, self).send(from_addr, to_addrs, message)
237+
238+
239+class TestSendBugNotifications(TestCaseWithFactory):
240+
241+ layer = LaunchpadZopelessLayer
242+
243+ def setUp(self):
244+ super(TestSendBugNotifications, self).setUp()
245+ self.notification_set = getUtility(IBugNotificationSet)
246+ # Ensure there are no outstanding notifications.
247+ for notification in self.notification_set.getNotificationsToSend():
248+ notification.destroySelf()
249+ self.ten_minutes_ago = datetime.now(pytz.UTC) - timedelta(minutes=10)
250+
251+ def test_oops_on_failed_delivery(self):
252+ # If one notification fails to send, it logs an OOPS and doesn't get
253+ # in the way of sending other notifications.
254+ set_immediate_mail_delivery(False)
255+ subscribers = []
256+ for i in range(3):
257+ bug = self.factory.makeBug()
258+ subscriber = self.factory.makePerson()
259+ subscribers.append(subscriber.preferredemail.email)
260+ bug.default_bugtask.target.addSubscription(subscriber, subscriber)
261+ message = getUtility(IMessageSet).fromText(
262+ "subject", "a comment.", bug.owner,
263+ datecreated=self.ten_minutes_ago)
264+ bug.addCommentNotification(message)
265+
266+ notifications = list(get_email_notifications(
267+ self.notification_set.getNotificationsToSend()))
268+ self.assertEqual(3, len(notifications))
269+
270+ mailer = BrokenMailer([subscribers[1]])
271+ with ZopeUtilityFixture(mailer, IMailDelivery, "Mail"):
272+ switch_dbuser(config.malone.bugnotification_dbuser)
273+ script = SendBugNotifications(
274+ "send-bug-notifications", config.malone.bugnotification_dbuser,
275+ ["-q"])
276+ script.txn = self.layer.txn
277+ script.main()
278+
279+ self.assertEqual(1, len(self.oopses))
280+ self.assertIn(
281+ "SMTPException: test requested delivery failure",
282+ self.oopses[0]["tb_text"])
283+ for i, (bug_notifications, _, messages) in enumerate(notifications):
284+ for bug_notification in bug_notifications:
285+ if i == 1:
286+ self.assertEqual(
287+ BugNotificationStatus.PENDING, bug_notification.status)
288+ else:
289+ self.assertEqual(
290+ BugNotificationStatus.SENT, bug_notification.status)