Merge lp:~abentley/launchpad/email-url-body into lp:launchpad

[Aaron Bentley]

[r=bac][ui=none][bug=504080] Include merge proposal URL in email bodies.

= Summary =
Fix bug #50480: Please put the URL to the merge proposal in the body of the
email.

== Proposed fix ==
Insert the phrase "For more details, see:\n$URL" in all merge proposal emails.

== Pre-implementation notes ==
None

== Implementation details ==
Much lint was fixed, and some tests were rewritten so that they would fit in
the 78-character limit. The worst was in test_nominateReview_email_content,
where the only way to make the string respect the width limit was """\n""".

== Tests ==
bin/test -t mergeproposal -t merge-proposal

== Demo and Q/A ==
Create all kinds of emails:
1. create a proposal
2. change the values in a proposal
3. request a reviewer
4. make a comment

In all cases, the resulting email should say "For more details, see\n$URL".

= Launchpad lint =
(I don't think it makes sense to lint email templates...)

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/emailtemplates/branch-merge-proposal-created.txt
  lib/canonical/launchpad/emailtemplates/review-requested.txt
  lib/lp/code/mail/tests/test_branchmergeproposal.py
  lib/lp/code/doc/branch-merge-proposal-notifications.txt
  lib/canonical/launchpad/emailtemplates/branch-merge-proposal-updated.txt

./lib/canonical/launchpad/emailtemplates/branch-merge-proposal-created.txt
       1: Line exceeds 78 characters.
       6: Line has trailing whitespace.
./lib/canonical/launchpad/emailtemplates/review-requested.txt
       1: Line exceeds 78 characters.
       8: Line has trailing whitespace.
./lib/canonical/launchpad/emailtemplates/branch-merge-proposal-updated.txt
       1: Line exceeds 78 characters.
       7: Line has trailing whitespace.

[Brad Crittenden (bac)]

Thanks for the bug fix and for the clean up Aaron. The branch looks good.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/emailtemplates/branch-merge-proposal-created.txt'
--- lib/canonical/launchpad/emailtemplates/branch-merge-proposal-created.txt	2010-08-03 00:48:48 +0000
+++ lib/canonical/launchpad/emailtemplates/branch-merge-proposal-created.txt	2010-12-15 22:11:41 +0000
@@ -1,6 +1,8 @@
 %(proposal_registrant)s has proposed merging %(source_branch)s into %(target_branch)s%(prerequisite)s.
 
-%(reviews)s%(related_bugs)s%(gap)s%(comment)s
+%(reviews)s%(related_bugs)s
+For more details, see:
+%(proposal_url)s%(gap)s%(comment)s
 -- 
 %(diff_cutoff_warning)s%(proposal_url)s
 %(reason)s%(edit_subscription)s

=== modified file 'lib/canonical/launchpad/emailtemplates/branch-merge-proposal-updated.txt'
--- lib/canonical/launchpad/emailtemplates/branch-merge-proposal-updated.txt	2008-06-09 15:55:12 +0000
+++ lib/canonical/launchpad/emailtemplates/branch-merge-proposal-updated.txt	2010-12-15 22:11:41 +0000
@@ -1,6 +1,9 @@
 The proposal to merge %(source_branch)s into %(target_branch)s has been updated.
 
 %(delta)s
+
+For more details, see:
+%(proposal_url)s
 -- 
 %(proposal_url)s
 %(reason)s%(edit_subscription)s

=== modified file 'lib/canonical/launchpad/emailtemplates/review-requested.txt'
--- lib/canonical/launchpad/emailtemplates/review-requested.txt	2010-08-03 00:48:48 +0000
+++ lib/canonical/launchpad/emailtemplates/review-requested.txt	2010-12-15 22:11:41 +0000
@@ -1,5 +1,8 @@
 You have been requested to review the proposed merge of %(source_branch)s into %(target_branch)s.
 
+For more details, see:
+%(proposal_url)s
+
 %(comment)s
 
 -- 

=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt	2010-10-26 03:06:30 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt	2010-12-15 22:11:41 +0000
@@ -1,10 +1,12 @@
-= Email Notifications for Branch Merge Proposals =
+Email Notifications for Branch Merge Proposals
+==============================================
 
 Subscribers to any of the branches involved in the merge proposal get
 notifications.
 
 
-== Subscription ==
+Subscription
+------------
 
 When subscribers subscribe to branches, they can specify what level of
 notification they would like to receive.
@@ -48,7 +50,8 @@
     >>> source_owner = bmp.source_branch.owner
 
 
-== Notification Recipients ==
+Notification Recipients
+-----------------------
 
 Recipients are determined using getNotificationRecipients.
 
@@ -88,7 +91,8 @@
     You are subscribed to branch ...
 
 
-== E-mail ==
+E-mail
+------
 
 Jobs for notifications are automagically generated when the merge proposal
 is created.  When those jobs are run, the email is sent from the registrant.
@@ -188,6 +192,9 @@
         Bob the Builder (bob)
         Mary Jones (mary): ui
     <BLANKLINE>
+    For more details, see:
+    http://code.launchpad.dev/~person-name...
+    <BLANKLINE>
     This is the initial commit message.
     <BLANKLINE>
     It is included in the initial email sent out.

=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py	2010-12-01 11:26:57 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py	2010-12-15 22:11:41 +0000
@@ -109,17 +109,24 @@
         bmp.root_message_id = None
         ctrl = mailer.generateEmail('[e-mail]', subscriber)
         reviewer = bmp.target_branch.owner
-        self.assertEqual("""\
-Baz Qux has proposed merging lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar.
-
-Requested reviews:
-  %s
-
---\x20
-%s
-%s
-""" % (reviewer.unique_displayname, canonical_url(bmp),reason.getReason()),
-       ctrl.body)
+        expected = dedent("""\
+        Baz Qux has proposed merging %(source)s into %(target)s.
+
+        Requested reviews:
+          %(reviewer)s
+
+        For more details, see:
+        %(bmp)s
+        --\x20
+        %(bmp)s
+        %(reason)s
+        """) % {
+            'source': bmp.source_branch.bzr_identity,
+            'target': bmp.target_branch.bzr_identity,
+            'reviewer': reviewer.unique_displayname,
+            'bmp': canonical_url(bmp),
+            'reason': reason.getReason()}
+        self.assertEqual(expected, ctrl.body)
         self.assertEqual('[Merge] '
             'lp://dev/~bob/super-product/fix-foo-for-bar into '
             'lp://dev/~mary/super-product/bar', ctrl.subject)
@@ -131,7 +138,7 @@
              'Reply-To': bmp.address,
              'Message-Id': '<foobar-example-com>'},
             ctrl.headers)
-        self.assertEqual('Baz Qux <[e-mail]>', ctrl.from_addr)        
+        self.assertEqual('Baz Qux <[e-mail]>', ctrl.from_addr)
         reviewer_id = mailer._format_user_address(reviewer)
         self.assertEqual(set([reviewer_id, bmp.address]), set(ctrl.to_addrs))
         mailer.sendAll()
@@ -146,8 +153,7 @@
         expected = (
             'Related bugs:\n'
             '  #%d I am a bug\n'
-            '  %s\n' % (bug.id, canonical_url(bug))
-            )
+            '  %s\n' % (bug.id, canonical_url(bug)))
         self.assertIn(expected, ctrl.body)
 
     def test_forCreation_without_bugs(self):
@@ -160,14 +166,16 @@
     def test_forCreation_with_review_request(self):
         """Correctly format list of reviewers."""
         reviewer = self.factory.makePerson(name='review-person')
-        bmp, subscriber = self.makeProposalWithSubscriber(reviewer=reviewer)        
+        bmp, subscriber = self.makeProposalWithSubscriber(reviewer=reviewer)
         bmp.nominateReviewer(reviewer, bmp.registrant, None)
         mailer = BMPMailer.forCreation(bmp, bmp.registrant)
         ctrl = mailer.generateEmail('[e-mail]', subscriber)
         self.assertIn(
             '\nRequested reviews:'
-            '\n  Review-person (review-person)'
-            '\n\n-- \n',
+            '\n  Review-person (review-person)\n'
+            '\n'
+            'For more details, see:\n'
+            '%s\n-- \n' % canonical_url(bmp),
             ctrl.body)
 
     def test_forCreation_with_review_request_and_bug(self):
@@ -175,7 +183,7 @@
         reviewer = self.factory.makePerson(name='review-person')
         bmp, subscriber = self.makeProposalWithSubscriber(reviewer=reviewer)
         bug = self.factory.makeBug(title='I am a bug')
-        bmp.source_branch.linkBug(bug, bmp.registrant)        
+        bmp.source_branch.linkBug(bug, bmp.registrant)
         bmp.nominateReviewer(reviewer, bmp.registrant, None)
         mailer = BMPMailer.forCreation(bmp, bmp.registrant)
         ctrl = mailer.generateEmail('[e-mail]', subscriber)
@@ -184,7 +192,10 @@
             '\n  Review-person (review-person)'
             '\nRelated bugs:'
             '\n  #%d I am a bug'
-            '\n  %s\n\n--' % (bug.id, canonical_url(bug)))
+            '\n  %s\n'
+            '\nFor more details, see:\n'
+            '%s'
+            '\n--' % (bug.id, canonical_url(bug), canonical_url(bmp)))
         self.assertIn(expected, ctrl.body)
 
     def test_forCreation_with_prerequisite_branch(self):
@@ -303,7 +314,8 @@
         self.assertEqual('inline; filename="review-diff.txt"',
                          attachment['Content-Disposition'])
         self.assertEqual(diff_text[:25], attachment.get_payload(decode=True))
-        warning_text = "The attached diff has been truncated due to its size.\n"
+        warning_text = (
+            "The attached diff has been truncated due to its size.\n")
         self.assertTrue(warning_text in ctrl.body)
 
     def getProposalUpdatedEmailJob(self, merge_proposal):
@@ -378,7 +390,7 @@
         merge_proposal = self.factory.makeBranchMergeProposal()
         mailer = BMPMailer.forModification(
             merge_proposal, 'the diff', merge_proposal.registrant)
-        self.assertIsNot(None,  mailer.message_id, 'message_id not set')
+        self.assertIsNot(None, mailer.message_id, 'message_id not set')
 
     def test_forModificationWithModificationTextDelta(self):
         """Ensure the right delta is filled out if there is a change."""
@@ -404,8 +416,9 @@
         self.assertEqual('[Merge] '
             'lp://dev/~bob/super-product/fix-foo-for-bar into\n\t'
             'lp://dev/~mary/super-product/bar', email['subject'])
+        bmp = job.branch_merge_proposal
         expected = dedent("""\
-            The proposal to merge lp://dev/~bob/super-product/fix-foo-for-bar into lp://dev/~mary/super-product/bar has been updated.
+            The proposal to merge %(source)s into %(target)s has been updated.
 
             Commit Message changed to:
 
@@ -414,10 +427,16 @@
             Description changed to:
 
             change description
+
+            For more details, see:
+            %(bmp)s
             --\x20
-            %s
+            %(bmp)s
             You are the owner of lp://dev/~bob/super-product/fix-foo-for-bar.
-            """) % canonical_url(job.branch_merge_proposal)
+            """) % {
+                'source': bmp.source_branch.bzr_identity,
+                'target': bmp.target_branch.bzr_identity,
+                'bmp': canonical_url(bmp)}
         self.assertEqual(expected, email.get_payload(decode=True))
 
     def assertRecipientsMatches(self, recipients, mailer):
@@ -531,17 +550,22 @@
         review_request_job.run()
         [sent_mail] = pop_notifications()
         expected = dedent("""\
-                You have been requested to review the proposed merge of %(source)s into %(target)s.
-
-                This branch is awesome.
-
-                -- 
-                %(bmp)s
-                You are requested to review the proposed merge of %(source)s into %(target)s.
-                """ % {
-                    'source': bmp.source_branch.bzr_identity,
-                    'target': bmp.target_branch.bzr_identity,
-                    'bmp': canonical_url(bmp)})
+            You have been requested to review the proposed merge of"""
+            """ %(source)s into %(target)s.
+
+            For more details, see:
+            %(bmp)s
+
+            This branch is awesome.
+
+            --\x20
+            %(bmp)s
+            You are requested to review the proposed merge of %(source)s"""
+            """ into %(target)s.
+            """ % {
+                'source': bmp.source_branch.bzr_identity,
+                'target': bmp.target_branch.bzr_identity,
+                'bmp': canonical_url(bmp)})
         self.assertEqual(expected, sent_mail.get_payload(decode=True))
 
     def test_nominateReview_emails_team_address(self):