launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:dmarc-for-merges-and-code into launchpad:master.

Commit message:
Use DMARC-compliant From addresses in code review emails

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1879740 in Launchpad itself: "Make Launchpad Code Review / Merge Review / Code Notices DMARC Compliant"
  https://bugs.launchpad.net/launchpad/+bug/1879740

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/406528

Most of this work is by Thomas Ward; I just tidied a few things up and fixed the tests.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:dmarc-for-merges-and-code into launchpad:master.

diff --git a/lib/lp/code/doc/branch-merge-proposal-notifications.txt b/lib/lp/code/doc/branch-merge-proposal-notifications.txt
index 70d15f0..e7134c2 100644
--- a/lib/lp/code/doc/branch-merge-proposal-notifications.txt
+++ b/lib/lp/code/doc/branch-merge-proposal-notifications.txt
@@ -129,7 +129,7 @@ An email is sent to subscribers of either branch and the default reviewer.
 
     >>> notification = notifications[0]
     >>> print(notification['From'])
-    Eric <eric@xxxxxxxxxxx>
+    Eric <mp+...@xxxxxxxxxxxxxxxxxxx>
     >>> print(notification['Subject'])
     [Merge] lp://dev/~person-name... into lp://dev/~person-name...
     >>> print(notification['X-Launchpad-Project'])
diff --git a/lib/lp/code/doc/codereviewcomment.txt b/lib/lp/code/doc/codereviewcomment.txt
index 8023afd..263f73e 100644
--- a/lib/lp/code/doc/codereviewcomment.txt
+++ b/lib/lp/code/doc/codereviewcomment.txt
@@ -86,7 +86,7 @@ Now run the pending job to send the email.
     >>> notifications = [email for email in notifications if
     ...                  email['X-Launchpad-Message-Rationale'] == 'Owner']
     >>> print_emails(include_reply_to=True, notifications=notifications)
-    From: Sender Person <sender@xxxxxxxxxxx>
+    From: Sender Person <mp+...@xxxxxxxxxxxxxxxxxxx>
     To: ...
     Reply-To: mp+...@xxxxxxxxxxxxxxxxxxx
     Subject: Please merge
diff --git a/lib/lp/code/mail/branchmergeproposal.py b/lib/lp/code/mail/branchmergeproposal.py
index 9566ae8..d876ad5 100644
--- a/lib/lp/code/mail/branchmergeproposal.py
+++ b/lib/lp/code/mail/branchmergeproposal.py
@@ -8,9 +8,9 @@ __metaclass__ = type
 
 from lp.code.enums import CodeReviewNotificationLevel
 from lp.code.mail.branch import BranchMailer
-from lp.services.config import config
 from lp.services.mail.basemailer import BaseMailer
 from lp.services.mail.sendmail import (
+    format_address,
     format_address_for_person,
     get_msgid,
     )
@@ -52,9 +52,8 @@ class BMPMailer(BranchMailer):
         recipients = merge_proposal.getNotificationRecipients(
             CodeReviewNotificationLevel.STATUS)
 
-        assert from_user.preferredemail is not None, (
-            'The sender must have an email address.')
-        from_address = format_address_for_person(from_user)
+        from_address = format_address(
+            from_user.display_name, merge_proposal.address)
 
         return cls(
             '%(proposal_title)s',
@@ -74,11 +73,10 @@ class BMPMailer(BranchMailer):
         recipients = merge_proposal.getNotificationRecipients(
             CodeReviewNotificationLevel.STATUS)
         if from_user is not None:
-            assert from_user.preferredemail is not None, (
-                'The sender must have an email address.')
-            from_address = format_address_for_person(from_user)
+            from_address = format_address(
+                from_user.display_name, merge_proposal.address)
         else:
-            from_address = config.canonical.noreply_from_address
+            from_address = merge_proposal.address
         return cls(
             '%(proposal_title)s',
             'branch-merge-proposal-updated.txt', recipients,
@@ -88,7 +86,8 @@ class BMPMailer(BranchMailer):
     @classmethod
     def forReviewRequest(cls, reason, merge_proposal, from_user):
         """Return a mailer for a request to review a BranchMergeProposal."""
-        from_address = format_address_for_person(from_user)
+        from_address = format_address(
+            from_user.display_name, merge_proposal.address)
         recipients = {reason.subscriber: reason}
         return cls(
             '%(proposal_title)s',
diff --git a/lib/lp/code/mail/codereviewcomment.py b/lib/lp/code/mail/codereviewcomment.py
index 0a885d2..f9e882f 100644
--- a/lib/lp/code/mail/codereviewcomment.py
+++ b/lib/lp/code/mail/codereviewcomment.py
@@ -44,7 +44,8 @@ class CodeReviewCommentMailer(BMPMailer):
         self.message = code_review_comment.message
         from_person = self.message.owner
         from_address = format_address(
-            from_person.displayname, from_person.preferredemail.email)
+            from_person.display_name,
+            code_review_comment.branch_merge_proposal.address)
         merge_proposal = code_review_comment.branch_merge_proposal
         BMPMailer.__init__(
             self, self.message.subject, None, recipients, merge_proposal,
diff --git a/lib/lp/code/mail/tests/test_branchmergeproposal.py b/lib/lp/code/mail/tests/test_branchmergeproposal.py
index f121fa7..06bc638 100644
--- a/lib/lp/code/mail/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/mail/tests/test_branchmergeproposal.py
@@ -143,7 +143,9 @@ class TestMergeProposalMailing(TestCaseWithFactory):
              'Reply-To': bmp.address,
              'Message-Id': '<foobar-example-com>'},
             ctrl.headers)
-        self.assertEqual('Baz Qux <baz.qux@xxxxxxxxxxx>', ctrl.from_addr)
+        self.assertEqual(
+            'Baz Qux <mp+%d@%s>' % (bmp.id, config.launchpad.code_domain),
+            ctrl.from_addr)
         reviewer_id = format_address_for_person(reviewer)
         self.assertEqual(set([reviewer_id, bmp.address]), set(ctrl.to_addrs))
         mailer.sendAll()
@@ -337,7 +339,9 @@ class TestMergeProposalMailing(TestCaseWithFactory):
         mailer.message_id = '<foobar-example-com>'
         ctrl = mailer.generateEmail('baz.quxx@xxxxxxxxxxx', subscriber)
         self.assertEqual('<foobar-example-com>', ctrl.headers['Message-Id'])
-        self.assertEqual('Baz Qux <baz.qux@xxxxxxxxxxx>', ctrl.from_addr)
+        self.assertEqual(
+            'Baz Qux <mp+%d@%s>' % (bmp.id, config.launchpad.code_domain),
+            ctrl.from_addr)
         bmp.root_message_id = None
         pop_notifications()
         mailer.sendAll()
@@ -576,7 +580,9 @@ class TestMergeProposalMailing(TestCaseWithFactory):
         mailer = BMPMailer.forReviewRequest(
             request, request.merge_proposal, requester)
         self.assertEqual(
-            'Requester <requester@xxxxxxxxxxx>', mailer.from_address)
+            'Requester <mp+%d@%s>' % (
+                request.merge_proposal.id, config.launchpad.code_domain),
+            mailer.from_address)
         self.assertEqual(
             request.merge_proposal.preview_diff,
             mailer.preview_diff)
diff --git a/lib/lp/code/mail/tests/test_codereviewcomment.py b/lib/lp/code/mail/tests/test_codereviewcomment.py
index 7d4ec13..8c65a49 100644
--- a/lib/lp/code/mail/tests/test_codereviewcomment.py
+++ b/lib/lp/code/mail/tests/test_codereviewcomment.py
@@ -99,8 +99,9 @@ class TestCodeReviewComment(TestCaseWithFactory):
         self.assertEqual(
             comment.branch_merge_proposal, mailer.merge_proposal)
         sender = comment.message.owner
-        sender_address = format_address(sender.displayname,
-                                        sender.preferredemail.email)
+        sender_address = format_address(
+            sender.displayname,
+            "mp+%d@%s" % (bmp.id, config.launchpad.code_domain))
         self.assertEqual(sender_address, mailer.from_address)
         self.assertEqual(comment, mailer.code_review_comment)
 
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index e8e7205..69d0595 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -2924,8 +2924,7 @@ class TestGitRepositoryDetectMerges(TestCaseWithFactory):
         self.assertIn(
             "Work in progress => Merged",
             notifications[0].get_payload(decode=True).decode("UTF-8"))
-        self.assertEqual(
-            config.canonical.noreply_from_address, notifications[0]["From"])
+        self.assertEqual(proposal.address, notifications[0]["From"])
         recipients = set(msg["x-envelope-to"] for msg in notifications)
         expected = set(
             [proposal.source_git_repository.registrant.preferredemail.email,