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
=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py 2010-03-31 22:50:17 +0000
+++ lib/lp/code/mail/branch.py 2010-04-26 02:05:33 +0000
@@ -82,7 +82,8 @@
82 else:82 else:
83 reason_template = (83 reason_template = (
84 '%(entity_is)s reviewing %(merge_proposal)s.')84 '%(entity_is)s reviewing %(merge_proposal)s.')
85 return cls(reviewer, reviewer, branch, 'Reviewer',85 return cls(reviewer, reviewer, branch,
86 cls.makeRationale('Reviewer', reviewer),
86 reason_template, branch_merge_proposal,87 reason_template, branch_merge_proposal,
87 branch_identity_cache=branch_identity_cache)88 branch_identity_cache=branch_identity_cache)
8889
@@ -122,16 +123,16 @@
122 The owner will be the sole recipient.123 The owner will be the sole recipient.
123 """124 """
124 return cls(branch.owner, recipient, branch,125 return cls(branch.owner, recipient, branch,
125 cls.makeRationale('Owner', branch.owner, recipient),126 cls.makeRationale('Owner', branch.owner),
126 'You are getting this email as %(lc_entity_is)s the'127 'You are getting this email as %(lc_entity_is)s the'
127 ' owner of the branch and someone has edited the'128 ' owner of the branch and someone has edited the'
128 ' details.',129 ' details.',
129 branch_identity_cache=branch_identity_cache)130 branch_identity_cache=branch_identity_cache)
130131
131 @staticmethod132 @staticmethod
132 def makeRationale(rationale_base, subscriber, recipient):133 def makeRationale(rationale_base, person):
133 if subscriber.isTeam():134 if person.is_team:
134 return '%s @%s' % (rationale_base, subscriber.name)135 return '%s @%s' % (rationale_base, person.name)
135 else:136 else:
136 return rationale_base137 return rationale_base
137138
138139
=== modified file 'lib/lp/code/mail/tests/test_branch.py'
--- lib/lp/code/mail/tests/test_branch.py 2010-03-17 03:44:12 +0000
+++ lib/lp/code/mail/tests/test_branch.py 2010-04-26 02:05:33 +0000
@@ -69,16 +69,40 @@
69 self.assertEqual(69 self.assertEqual(
70 vote_reference.branch_merge_proposal.source_branch, reason.branch)70 vote_reference.branch_merge_proposal.source_branch, reason.branch)
7171
72 def test_getReasonReviewer(self):72 def test_forReview_individual_pending(self):
73 bmp, vote_reference, subscriber = self.makeReviewerAndSubscriber()73 bmp = self.factory.makeBranchMergeProposal()
74 pending_review = vote_reference.comment is None74 reviewer = self.factory.makePerson(name='eric')
75 reason = RecipientReason.forReviewer(bmp, pending_review, subscriber)75 reason = RecipientReason.forReviewer(bmp, True, reviewer)
76 self.assertEqual('Reviewer', reason.mail_header)
76 self.assertEqual(77 self.assertEqual(
77 'You are requested to review the proposed merge of %s into %s.'78 'You are requested to review the proposed merge of %s into %s.'
78 % (bmp.source_branch.bzr_identity,79 % (bmp.source_branch.bzr_identity,
79 bmp.target_branch.bzr_identity),80 bmp.target_branch.bzr_identity),
80 reason.getReason())81 reason.getReason())
8182
83 def test_forReview_individual_in_progress(self):
84 bmp = self.factory.makeBranchMergeProposal()
85 reviewer = self.factory.makePerson(name='eric')
86 reason = RecipientReason.forReviewer(bmp, False, reviewer)
87 self.assertEqual('Reviewer', reason.mail_header)
88 self.assertEqual(
89 'You are reviewing the proposed merge of %s into %s.'
90 % (bmp.source_branch.bzr_identity,
91 bmp.target_branch.bzr_identity),
92 reason.getReason())
93
94 def test_forReview_team_pending(self):
95 bmp = self.factory.makeBranchMergeProposal()
96 reviewer = self.factory.makeTeam(name='vikings')
97 reason = RecipientReason.forReviewer(bmp, True, reviewer)
98 self.assertEqual('Reviewer @vikings', reason.mail_header)
99 self.assertEqual(
100 'Your team Vikings is requested to review the proposed merge'
101 ' of %s into %s.'
102 % (bmp.source_branch.bzr_identity,
103 bmp.target_branch.bzr_identity),
104 reason.getReason())
105
82 def test_getReasonPerson(self):106 def test_getReasonPerson(self):
83 """Ensure the correct reason is generated for individuals."""107 """Ensure the correct reason is generated for individuals."""
84 merge_proposal, subscription = self.makeProposalWithSubscription()108 merge_proposal, subscription = self.makeProposalWithSubscription()
85109
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2010-04-01 04:48:01 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2010-04-26 02:05:33 +0000
@@ -277,11 +277,10 @@
277 # aleady.277 # aleady.
278 for review in self.votes:278 for review in self.votes:
279 reviewer = review.reviewer279 reviewer = review.reviewer
280 if not reviewer.is_team:280 pending = review.comment is None
281 pending = review.comment is None281 recipients[reviewer] = RecipientReason.forReviewer(
282 recipients[reviewer] = RecipientReason.forReviewer(282 self, pending, reviewer,
283 self, pending, reviewer,283 branch_identity_cache=branch_identity_cache)
284 branch_identity_cache=branch_identity_cache)
285 # If the registrant of the proposal is getting emails, update the284 # If the registrant of the proposal is getting emails, update the
286 # rationale to say that they registered it. Don't however send them285 # rationale to say that they registered it. Don't however send them
287 # emails if they aren't asking for any.286 # emails if they aren't asking for any.
288287
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-06 03:37:16 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2010-04-26 02:05:33 +0000
@@ -752,6 +752,21 @@
752 subscriber_set = set([source_owner, target_owner, reviewer])752 subscriber_set = set([source_owner, target_owner, reviewer])
753 self.assertEqual(subscriber_set, set(recipients.keys()))753 self.assertEqual(subscriber_set, set(recipients.keys()))
754754
755 def test_getNotificationRecipientsIncludesTeamReviewers(self):
756 # If the reviewer is a team, the team gets the email.
757 bmp = self.factory.makeBranchMergeProposal()
758 # Both of the branch owners are now subscribed to their own
759 # branches with full code review notification level set.
760 source_owner = bmp.source_branch.owner
761 target_owner = bmp.target_branch.owner
762 login_person(source_owner)
763 reviewer = self.factory.makeTeam()
764 bmp.nominateReviewer(reviewer, registrant=source_owner)
765 recipients = bmp.getNotificationRecipients(
766 CodeReviewNotificationLevel.STATUS)
767 subscriber_set = set([source_owner, target_owner, reviewer])
768 self.assertEqual(subscriber_set, set(recipients.keys()))
769
755 def test_getNotificationRecipients_Registrant(self):770 def test_getNotificationRecipients_Registrant(self):
756 # If the registrant of the proposal is being notified of the771 # If the registrant of the proposal is being notified of the
757 # proposals, they get their rationale set to "Registrant".772 # proposals, they get their rationale set to "Registrant".

Subscribers

People subscribed via source and target branches

to status/vote changes: