launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19225
[Merge] lp:~cjwatson/launchpad/basemailer-refactor into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/basemailer-refactor into lp:launchpad.
Commit message:
Refactor BaseMailer a little, making argument order more consistent and adding _getFromAddress and sendOne methods.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/basemailer-refactor/+merge/268858
Refactor BaseMailer a little. This makes argument order more consistent (no more "recipient, email" in some methods and "email, recipient" in others) and adds _getFromAddress and sendOne methods. This is in preparation for introducing PackageUploadMailer, for which it will be useful to construct a single mailer that can send some mails with a different From address (-changes announcements) and that can intercept the loop body of sendAll to make the dry-run case work.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/basemailer-refactor into lp:launchpad.
=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py 2015-04-16 14:35:17 +0000
+++ lib/lp/code/mail/branch.py 2015-08-23 23:35:32 +0000
@@ -244,8 +244,8 @@
revision_id=revision_id,
notification_type='branch-revision', full_subject=subject)
- def _getHeaders(self, email):
- headers = BaseMailer._getHeaders(self, email)
+ def _getHeaders(self, email, recipient):
+ headers = BaseMailer._getHeaders(self, email, recipient)
reason, rationale = self._recipients.getReason(email)
headers['X-Launchpad-Branch'] = reason.branch.unique_name
if IGitRef.providedBy(reason.branch):
=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
--- lib/lp/code/mail/branchmergeproposal.py 2015-04-16 04:00:33 +0000
+++ lib/lp/code/mail/branchmergeproposal.py 2015-08-23 23:35:32 +0000
@@ -97,7 +97,7 @@
"""Return the address to use for the reply-to header."""
return self.merge_proposal.address
- def _getToAddresses(self, recipient, email):
+ def _getToAddresses(self, email, recipient):
"""Return the addresses to use for the to header.
If the email is being sent directly to the recipient, their email
@@ -105,7 +105,7 @@
reviewers are returned.
"""
if self.direct_email:
- return BaseMailer._getToAddresses(self, recipient, email)
+ return BaseMailer._getToAddresses(self, email, recipient)
to_addrs = [self.merge_proposal.address]
for vote in self.merge_proposal.votes:
if vote.reviewer == vote.registrant:
@@ -117,9 +117,9 @@
to_addrs.append(self._format_user_address(vote.reviewer))
return to_addrs
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""Return the mail headers to use."""
- headers = BranchMailer._getHeaders(self, email)
+ headers = BranchMailer._getHeaders(self, email, recipient)
if self.merge_proposal.root_message_id is not None:
headers['In-Reply-To'] = self.merge_proposal.root_message_id
return headers
=== modified file 'lib/lp/code/mail/codereviewcomment.py'
--- lib/lp/code/mail/codereviewcomment.py 2015-08-04 09:28:32 +0000
+++ lib/lp/code/mail/codereviewcomment.py 2015-08-23 23:35:32 +0000
@@ -131,15 +131,15 @@
return ''.join((
self.body_prefix, append_footer(self.body_main, footer)))
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""Return the mail headers to use."""
- headers = BMPMailer._getHeaders(self, email)
+ headers = BMPMailer._getHeaders(self, email, recipient)
headers['Message-Id'] = self.message.rfc822msgid
if self.message.parent is not None:
headers['In-Reply-To'] = self.message.parent.rfc822msgid
return headers
- def _getToAddresses(self, recipient, email):
+ def _getToAddresses(self, email, recipient):
"""Provide to addresses as if this were a mailing list.
CodeReviewComments which are not replies shall list the merge proposer
=== modified file 'lib/lp/code/mail/sourcepackagerecipebuild.py'
--- lib/lp/code/mail/sourcepackagerecipebuild.py 2014-07-09 03:40:14 +0000
+++ lib/lp/code/mail/sourcepackagerecipebuild.py 2015-08-23 23:35:32 +0000
@@ -42,10 +42,10 @@
notification_type='recipe-build-status')
self.build = build
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""See `BaseMailer`"""
headers = super(
- SourcePackageRecipeBuildMailer, self)._getHeaders(email)
+ SourcePackageRecipeBuildMailer, self)._getHeaders(email, recipient)
headers.update({
'X-Launchpad-Build-State': self.build.status.name,
})
@@ -81,7 +81,7 @@
params['upload_log_url'] = self.build.upload_log_url
return params
- def _getFooter(self, params):
+ def _getFooter(self, email, recipient, params):
"""See `BaseMailer`"""
return ('%(build_url)s\n'
'%(reason)s\n' % params)
=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py 2015-08-04 09:28:32 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py 2015-08-23 23:35:32 +0000
@@ -351,11 +351,10 @@
"""To address for a comment with no parent should be the proposer."""
comment = self.makeCommentAndParticipants()
mailer = CodeReviewCommentMailer.forCreation(comment)
- to = mailer._getToAddresses(
- comment.message.owner, 'comment@xxxxxxxxx')
+ to = mailer._getToAddresses('comment@xxxxxxxxx', comment.message.owner)
self.assertEqual(['Proposer <proposer@xxxxxxxxx>'], to)
to = mailer._getToAddresses(
- comment.branch_merge_proposal.registrant, 'propose@xxxxxxxxx')
+ 'propose@xxxxxxxxx', comment.branch_merge_proposal.registrant)
self.assertEqual(['Proposer <propose@xxxxxxxxx>'], to)
def test_generateEmail_addresses(self):
@@ -379,10 +378,9 @@
reply = comment.branch_merge_proposal.createComment(
second_commenter, 'hello2', parent=comment)
mailer = CodeReviewCommentMailer.forCreation(reply)
- to = mailer._getToAddresses(second_commenter, 'comment2@xxxxxxxxx')
+ to = mailer._getToAddresses('comment2@xxxxxxxxx', second_commenter)
self.assertEqual(['Commenter <commenter@xxxxxxxxx>'], to)
- to = mailer._getToAddresses(
- comment.message.owner, 'comment@xxxxxxxxx')
+ to = mailer._getToAddresses('comment@xxxxxxxxx', comment.message.owner)
self.assertEqual(['Commenter <comment@xxxxxxxxx>'], to)
def test_getToAddresses_with_hidden_address(self):
@@ -394,7 +392,7 @@
reply = comment.branch_merge_proposal.createComment(
second_commenter, 'hello2', parent=comment)
mailer = CodeReviewCommentMailer.forCreation(reply)
- to = mailer._getToAddresses(second_commenter, 'comment2@xxxxxxxxx')
+ to = mailer._getToAddresses('comment2@xxxxxxxxx', second_commenter)
self.assertEqual([mailer.merge_proposal.address], to)
=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py 2015-08-06 12:09:59 +0000
+++ lib/lp/services/mail/basemailer.py 2015-08-23 23:35:32 +0000
@@ -71,7 +71,10 @@
self._mail_controller_class = mail_controller_class
self.request = request
- def _getToAddresses(self, recipient, email):
+ def _getFromAddress(self, email, recipient):
+ return self.from_address
+
+ def _getToAddresses(self, email, recipient):
return [format_address(recipient.displayname, email)]
def generateEmail(self, email, recipient, force_no_attachments=False):
@@ -81,15 +84,16 @@
:param recipient: The Person to send to.
:return: (headers, subject, body) of the email.
"""
- to_addresses = self._getToAddresses(recipient, email)
- headers = self._getHeaders(email)
+ from_address = self._getFromAddress(email, recipient)
+ to_addresses = self._getToAddresses(email, recipient)
+ headers = self._getHeaders(email, recipient)
subject = self._getSubject(email, recipient)
body = self._getBody(email, recipient)
expanded_footer = self._getExpandedFooter(headers, recipient)
if expanded_footer:
body = append_footer(body, expanded_footer)
ctrl = self._mail_controller_class(
- self.from_address, to_addresses, subject, body, headers,
+ from_address, to_addresses, subject, body, headers,
envelope_to=[email])
if force_no_attachments:
ctrl.addAttachment(
@@ -108,7 +112,7 @@
"""Return the address to use for the reply-to header."""
return None
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""Return the mail headers to use."""
reason, rationale = self._recipients.getReason(email)
headers = OrderedDict()
@@ -149,12 +153,12 @@
template = get_email_template(self._template_name, app=self.app)
params = self._getTemplateParams(email, recipient)
body = template % params
- footer = self._getFooter(params)
+ footer = self._getFooter(email, recipient, params)
if footer is not None:
body = append_footer(body, footer)
return body
- def _getFooter(self, params):
+ def _getFooter(self, email, recipient, params):
"""Provide a footer to attach to the body, or None."""
return None
@@ -168,33 +172,35 @@
lines.append('%s: %s\n' % (key[2:], value))
return ''.join(lines)
- def sendAll(self):
- """Send notifications to all recipients."""
+ def sendOne(self, email, recipient):
+ """Send notification to one recipient."""
# We never want SMTP errors to propagate from this function.
- for email, recipient in self._recipients.getRecipientPersons():
- ctrl = self.generateEmail(email, recipient)
+ ctrl = self.generateEmail(email, recipient)
+ try:
+ ctrl.send()
+ except SMTPException:
+ # If the initial sending failed, try again without
+ # attachments.
+ ctrl = self.generateEmail(
+ email, recipient, force_no_attachments=True)
try:
ctrl.send()
except SMTPException:
- # If the initial sending failed, try again without
- # attachments.
- ctrl = self.generateEmail(
- email, recipient, force_no_attachments=True)
- try:
- ctrl.send()
- except SMTPException:
- error_utility = getUtility(IErrorReportingUtility)
- oops_vars = {
- "message_id": ctrl.headers.get("Message-Id"),
- "notification_type": self.notification_type,
- "recipient": ", ".join(ctrl.to_addrs),
- "subject": ctrl.subject,
- }
- with error_utility.oopsMessage(oops_vars):
- oops = error_utility.raising(
- sys.exc_info(), self.request)
- self.logger.info(
- "Mail resulted in OOPS: %s" % oops.get("id"))
+ error_utility = getUtility(IErrorReportingUtility)
+ oops_vars = {
+ "message_id": ctrl.headers.get("Message-Id"),
+ "notification_type": self.notification_type,
+ "recipient": ", ".join(ctrl.to_addrs),
+ "subject": ctrl.subject,
+ }
+ with error_utility.oopsMessage(oops_vars):
+ oops = error_utility.raising(sys.exc_info(), self.request)
+ self.logger.info("Mail resulted in OOPS: %s" % oops.get("id"))
+
+ def sendAll(self):
+ """Send notifications to all recipients."""
+ for email, recipient in sorted(self._recipients.getRecipientPersons()):
+ self.sendOne(email, recipient)
class RecipientReason:
=== modified file 'lib/lp/services/mail/tests/test_basemailer.py'
--- lib/lp/services/mail/tests/test_basemailer.py 2015-08-06 12:09:59 +0000
+++ lib/lp/services/mail/tests/test_basemailer.py 2015-08-23 23:35:32 +0000
@@ -32,10 +32,17 @@
return 'body'
+class FromAddressUpper(BaseMailerSubclass):
+ """Subclass of BaseMailer providing an example getFromAddress."""
+
+ def _getFromAddress(self, email, recipient):
+ return self.from_address.upper()
+
+
class ToAddressesUpper(BaseMailerSubclass):
"""Subclass of BaseMailer providing an example getToAddresses."""
- def _getToAddresses(self, recipient, email):
+ def _getToAddresses(self, email, recipient):
return email.upper()
@@ -95,6 +102,19 @@
self.assertEqual(['to@xxxxxxxxxxx'], ctrl.envelope_to)
self.assertEqual(['Example To <to@xxxxxxxxxxx>'], ctrl.to_addrs)
+ def test_generateEmail_uses_getFromAddress(self):
+ """BaseMailer.generateEmail uses getFromAddress.
+
+ We verify this by using a subclass that provides getFromAddress
+ returning the uppercased email address.
+ """
+ fake_to = self.factory.makePerson(email='to@xxxxxxxxxxx')
+ recipients = {fake_to: FakeSubscription()}
+ mailer = FromAddressUpper(
+ 'subject', None, recipients, 'from@xxxxxxxxxxx')
+ ctrl = mailer.generateEmail('to@xxxxxxxxxxx', fake_to)
+ self.assertEqual('FROM@xxxxxxxxxxx', ctrl.from_addr)
+
def test_generateEmail_uses_getToAddresses(self):
"""BaseMailer.generateEmail uses getToAddresses.
=== modified file 'lib/lp/snappy/mail/snapbuild.py'
--- lib/lp/snappy/mail/snapbuild.py 2015-07-23 16:55:43 +0000
+++ lib/lp/snappy/mail/snapbuild.py 2015-08-23 23:35:32 +0000
@@ -39,9 +39,9 @@
notification_type="snap-build-status")
self.build = build
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""See `BaseMailer`."""
- headers = super(SnapBuildMailer, self)._getHeaders(email)
+ headers = super(SnapBuildMailer, self)._getHeaders(email, recipient)
headers["X-Launchpad-Build-State"] = self.build.status.name
return headers
@@ -76,7 +76,7 @@
params["builder_url"] = canonical_url(build.builder)
return params
- def _getFooter(self, params):
+ def _getFooter(self, email, recipient, params):
"""See `BaseMailer`."""
return ("%(build_url)s\n"
"%(reason)s\n" % params)
=== modified file 'lib/lp/soyuz/mail/binarypackagebuild.py'
--- lib/lp/soyuz/mail/binarypackagebuild.py 2015-07-14 10:57:46 +0000
+++ lib/lp/soyuz/mail/binarypackagebuild.py 2015-08-23 23:35:32 +0000
@@ -150,9 +150,10 @@
self.build = build
self.extra_info = extra_info
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""See `BaseMailer`."""
- headers = super(BinaryPackageBuildMailer, self)._getHeaders(email)
+ headers = super(BinaryPackageBuildMailer, self)._getHeaders(
+ email, recipient)
build = self.build
headers.update({
"X-Launchpad-Archive": build.archive.reference,
@@ -234,7 +235,7 @@
})
return params
- def _getFooter(self, params):
+ def _getFooter(self, email, recipient, params):
"""See `BaseMailer`."""
return ("%(build_title)s\n"
"%(build_url)s\n\n"
=== modified file 'lib/lp/soyuz/mail/livefsbuild.py'
--- lib/lp/soyuz/mail/livefsbuild.py 2014-07-09 06:25:19 +0000
+++ lib/lp/soyuz/mail/livefsbuild.py 2015-08-23 23:35:32 +0000
@@ -39,9 +39,9 @@
notification_type="livefs-build-status")
self.build = build
- def _getHeaders(self, email):
+ def _getHeaders(self, email, recipient):
"""See `BaseMailer`."""
- headers = super(LiveFSBuildMailer, self)._getHeaders(email)
+ headers = super(LiveFSBuildMailer, self)._getHeaders(email, recipient)
headers["X-Launchpad-Build-State"] = self.build.status.name
return headers
@@ -77,7 +77,7 @@
params["builder_url"] = canonical_url(build.builder)
return params
- def _getFooter(self, params):
+ def _getFooter(self, email, recipient, params):
"""See `BaseMailer`."""
return ("%(build_url)s\n"
"%(reason)s\n" % params)
Follow ups
