launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> 
> === modified file 'lib/lp/bugs/scripts/bugnotification.py'
> --- lib/lp/bugs/scripts/bugnotification.py	2015-07-06 17:27:09 +0000
> +++ lib/lp/bugs/scripts/bugnotification.py	2015-07-15 10:00:18 +0000
> @@ -327,3 +337,47 @@
>              message=message,
>              recipients=recipients,
>              activity=activity)
> +
> +
> +class SendBugNotifications(LaunchpadCronScript):
> +    def main(self):
> +        notifications_sent = False
> +        bug_notification_set = getUtility(IBugNotificationSet)
> +        deferred_notifications = \
> +            bug_notification_set.getDeferredNotifications()
> +        process_deferred_notifications(deferred_notifications)
> +        pending_notifications = get_email_notifications(
> +            bug_notification_set.getNotificationsToSend())
> +        for (bug_notifications,
> +             omitted_notifications,
> +             messages) in pending_notifications:
> +            try:
> +                for message in messages:
> +                    self.logger.info("Notifying %s about bug %d." % (
> +                        message['To'], bug_notifications[0].bug.id))
> +                    sendmail(message)
> +                    self.logger.debug(message.as_string())
> +            except SMTPException:
> +                request = ScriptRequest([
> +                    ("script_name", self.name),
> +                    ("path", sys.argv[0]),
> +                    ])
> +                getUtility(IErrorReportingUtility).raising(
> +                    sys.exc_info(), request)
> +                self.logger.info(request.oopsid)
> +                self.txn.abort()
> +                continue

I'd add details of the notification and message that failed to the request. It may also be useful to include the traceback in the log, rather than just an OOPS ID.

> +            for notification in bug_notifications:
> +                notification.date_emailed = UTC_NOW
> +                notification.status = BugNotificationStatus.SENT

This will pretend that any failed notifications have been sent -- an SMTP server outage will cause all notifications within its duration to be quietly and permanently eaten.

> +            for notification in omitted_notifications:
> +                notification.date_emailed = UTC_NOW
> +                notification.status = BugNotificationStatus.OMITTED
> +            notifications_sent = True
> +            # Commit after each batch of email sent, so that we won't
> +            # re-mail the notifications in case of something going wrong
> +            # in the middle.
> +            self.txn.commit()
> +
> +        if not notifications_sent:
> +            self.logger.debug("No notifications are pending to be sent.")


-- 
https://code.launchpad.net/~cjwatson/launchpad/send-bug-notifications-oops/+merge/264814
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.