launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02174
[Merge] lp:~abentley/launchpad/email-url-body into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/email-url-body into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#504080 Please put the URL to the merge proposal in the body of the email
https://bugs.launchpad.net/bugs/504080
= 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.
--
https://code.launchpad.net/~abentley/launchpad/email-url-body/+merge/43838
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/email-url-body into lp:launchpad.
=== 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('baz.quxx@xxxxxxxxxxx', 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 <baz.qux@xxxxxxxxxxx>', ctrl.from_addr)
+ self.assertEqual('Baz Qux <baz.qux@xxxxxxxxxxx>', 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('baz.quxx@xxxxxxxxxxx', 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('baz.quxx@xxxxxxxxxxx', 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):
