Launchpad itself

Merge lp:~thumper/launchpad/really-send-reviewer-email-to-teams into lp:launchpad/db-devel

  1. really-send-reviewer-email-to-teams
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/really-send-reviewer-email-to-teams
Merge into: lp:launchpad/db-devel
Diff against target: 129 lines (+53/-14)
4 files modified
lib/lp/code/mail/branch.py (+6/-5)
lib/lp/code/mail/tests/test_branch.py (+28/-4)
lib/lp/code/model/branchmergeproposal.py (+4/-5)
lib/lp/code/model/tests/test_branchmergeproposals.py (+15/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/really-send-reviewer-email-to-teams
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+24094@code.launchpad.net

Commit message

Really send code review email to teams.

Description of the change

Send emails to all members of a review team.

Part of this work was completed already for the actual reviewer email notification, but this was missed and caught while performing QA on staging.

tests:
  test_getNotificationRecipients
  test_forReview

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Looks fine to me, nice to make things a bit more regular.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/mail/branch.py'
2--- lib/lp/code/mail/branch.py 2010-03-31 22:50:17 +0000
3+++ lib/lp/code/mail/branch.py 2010-04-26 02:05:33 +0000
4@@ -82,7 +82,8 @@
5 else:
6 reason_template = (
7 '%(entity_is)s reviewing %(merge_proposal)s.')
8- return cls(reviewer, reviewer, branch, 'Reviewer',
9+ return cls(reviewer, reviewer, branch,
10+ cls.makeRationale('Reviewer', reviewer),
11 reason_template, branch_merge_proposal,
12 branch_identity_cache=branch_identity_cache)
13
14@@ -122,16 +123,16 @@
15 The owner will be the sole recipient.
16 """
17 return cls(branch.owner, recipient, branch,
18- cls.makeRationale('Owner', branch.owner, recipient),
19+ cls.makeRationale('Owner', branch.owner),
20 'You are getting this email as %(lc_entity_is)s the'
21 ' owner of the branch and someone has edited the'
22 ' details.',
23 branch_identity_cache=branch_identity_cache)
24
25 @staticmethod
26- def makeRationale(rationale_base, subscriber, recipient):
27- if subscriber.isTeam():
28- return '%s @%s' % (rationale_base, subscriber.name)
29+ def makeRationale(rationale_base, person):
30+ if person.is_team:
31+ return '%s @%s' % (rationale_base, person.name)
32 else:
33 return rationale_base
34
35
36=== modified file 'lib/lp/code/mail/tests/test_branch.py'
37--- lib/lp/code/mail/tests/test_branch.py 2010-03-17 03:44:12 +0000
38+++ lib/lp/code/mail/tests/test_branch.py 2010-04-26 02:05:33 +0000
39@@ -69,16 +69,40 @@
40 self.assertEqual(
41 vote_reference.branch_merge_proposal.source_branch, reason.branch)
42
43- def test_getReasonReviewer(self):
44- bmp, vote_reference, subscriber = self.makeReviewerAndSubscriber()
45- pending_review = vote_reference.comment is None
46- reason = RecipientReason.forReviewer(bmp, pending_review, subscriber)
47+ def test_forReview_individual_pending(self):
48+ bmp = self.factory.makeBranchMergeProposal()
49+ reviewer = self.factory.makePerson(name='eric')
50+ reason = RecipientReason.forReviewer(bmp, True, reviewer)
51+ self.assertEqual('Reviewer', reason.mail_header)
52 self.assertEqual(
53 'You are requested to review the proposed merge of %s into %s.'
54 % (bmp.source_branch.bzr_identity,
55 bmp.target_branch.bzr_identity),
56 reason.getReason())
57
58+ def test_forReview_individual_in_progress(self):
59+ bmp = self.factory.makeBranchMergeProposal()
60+ reviewer = self.factory.makePerson(name='eric')
61+ reason = RecipientReason.forReviewer(bmp, False, reviewer)
62+ self.assertEqual('Reviewer', reason.mail_header)
63+ self.assertEqual(
64+ 'You are reviewing the proposed merge of %s into %s.'
65+ % (bmp.source_branch.bzr_identity,
66+ bmp.target_branch.bzr_identity),
67+ reason.getReason())
68+
69+ def test_forReview_team_pending(self):
70+ bmp = self.factory.makeBranchMergeProposal()
71+ reviewer = self.factory.makeTeam(name='vikings')
72+ reason = RecipientReason.forReviewer(bmp, True, reviewer)
73+ self.assertEqual('Reviewer @vikings', reason.mail_header)
74+ self.assertEqual(
75+ 'Your team Vikings is requested to review the proposed merge'
76+ ' of %s into %s.'
77+ % (bmp.source_branch.bzr_identity,
78+ bmp.target_branch.bzr_identity),
79+ reason.getReason())
80+
81 def test_getReasonPerson(self):
82 """Ensure the correct reason is generated for individuals."""
83 merge_proposal, subscription = self.makeProposalWithSubscription()
84
85=== modified file 'lib/lp/code/model/branchmergeproposal.py'
86--- lib/lp/code/model/branchmergeproposal.py 2010-04-01 04:48:01 +0000
87+++ lib/lp/code/model/branchmergeproposal.py 2010-04-26 02:05:33 +0000
88@@ -277,11 +277,10 @@
89 # aleady.
90 for review in self.votes:
91 reviewer = review.reviewer
92- if not reviewer.is_team:
93- pending = review.comment is None
94- recipients[reviewer] = RecipientReason.forReviewer(
95- self, pending, reviewer,
96- branch_identity_cache=branch_identity_cache)
97+ pending = review.comment is None
98+ recipients[reviewer] = RecipientReason.forReviewer(
99+ self, pending, reviewer,
100+ branch_identity_cache=branch_identity_cache)
101 # If the registrant of the proposal is getting emails, update the
102 # rationale to say that they registered it. Don't however send them
103 # emails if they aren't asking for any.
104
105=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
106--- lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-06 03:37:16 +0000
107+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-26 02:05:33 +0000
108@@ -752,6 +752,21 @@
109 subscriber_set = set([source_owner, target_owner, reviewer])
110 self.assertEqual(subscriber_set, set(recipients.keys()))
111
112+ def test_getNotificationRecipientsIncludesTeamReviewers(self):
113+ # If the reviewer is a team, the team gets the email.
114+ bmp = self.factory.makeBranchMergeProposal()
115+ # Both of the branch owners are now subscribed to their own
116+ # branches with full code review notification level set.
117+ source_owner = bmp.source_branch.owner
118+ target_owner = bmp.target_branch.owner
119+ login_person(source_owner)
120+ reviewer = self.factory.makeTeam()
121+ bmp.nominateReviewer(reviewer, registrant=source_owner)
122+ recipients = bmp.getNotificationRecipients(
123+ CodeReviewNotificationLevel.STATUS)
124+ subscriber_set = set([source_owner, target_owner, reviewer])
125+ self.assertEqual(subscriber_set, set(recipients.keys()))
126+
127 def test_getNotificationRecipients_Registrant(self):
128 # If the registrant of the proposal is being notified of the
129 # proposals, they get their rationale set to "Registrant".

Subscribers

People subscribed via source and target branches

to status/vote changes: