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

[Tim Penhey]

Really send code review email to teams.

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

[Michael Hudson-Doyle (mwhudson)]

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

review: Approve

=== 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 @@
         else:
             reason_template = (
                 '%(entity_is)s reviewing %(merge_proposal)s.')
-        return cls(reviewer, reviewer, branch, 'Reviewer',
+        return cls(reviewer, reviewer, branch,
+                   cls.makeRationale('Reviewer', reviewer),
                    reason_template, branch_merge_proposal,
                    branch_identity_cache=branch_identity_cache)
 
@@ -122,16 +123,16 @@
         The owner will be the sole recipient.
         """
         return cls(branch.owner, recipient, branch,
-                     cls.makeRationale('Owner', branch.owner, recipient),
+                     cls.makeRationale('Owner', branch.owner),
                      'You are getting this email as %(lc_entity_is)s the'
                      ' owner of the branch and someone has edited the'
                      ' details.',
                      branch_identity_cache=branch_identity_cache)
 
     @staticmethod
-    def makeRationale(rationale_base, subscriber, recipient):
-        if subscriber.isTeam():
-            return '%s @%s' % (rationale_base, subscriber.name)
+    def makeRationale(rationale_base, person):
+        if person.is_team:
+            return '%s @%s' % (rationale_base, person.name)
         else:
             return rationale_base
 

=== 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 @@
         self.assertEqual(
             vote_reference.branch_merge_proposal.source_branch, reason.branch)
 
-    def test_getReasonReviewer(self):
-        bmp, vote_reference, subscriber = self.makeReviewerAndSubscriber()
-        pending_review = vote_reference.comment is None
-        reason = RecipientReason.forReviewer(bmp, pending_review, subscriber)
+    def test_forReview_individual_pending(self):
+        bmp = self.factory.makeBranchMergeProposal()
+        reviewer = self.factory.makePerson(name='eric')
+        reason = RecipientReason.forReviewer(bmp, True, reviewer)
+        self.assertEqual('Reviewer', reason.mail_header)
         self.assertEqual(
             'You are requested to review the proposed merge of %s into %s.'
             % (bmp.source_branch.bzr_identity,
                bmp.target_branch.bzr_identity),
             reason.getReason())
 
+    def test_forReview_individual_in_progress(self):
+        bmp = self.factory.makeBranchMergeProposal()
+        reviewer = self.factory.makePerson(name='eric')
+        reason = RecipientReason.forReviewer(bmp, False, reviewer)
+        self.assertEqual('Reviewer', reason.mail_header)
+        self.assertEqual(
+            'You are reviewing the proposed merge of %s into %s.'
+            % (bmp.source_branch.bzr_identity,
+               bmp.target_branch.bzr_identity),
+            reason.getReason())
+
+    def test_forReview_team_pending(self):
+        bmp = self.factory.makeBranchMergeProposal()
+        reviewer = self.factory.makeTeam(name='vikings')
+        reason = RecipientReason.forReviewer(bmp, True, reviewer)
+        self.assertEqual('Reviewer @vikings', reason.mail_header)
+        self.assertEqual(
+            'Your team Vikings is requested to review the proposed merge'
+            ' of %s into %s.'
+            % (bmp.source_branch.bzr_identity,
+               bmp.target_branch.bzr_identity),
+            reason.getReason())
+
     def test_getReasonPerson(self):
         """Ensure the correct reason is generated for individuals."""
         merge_proposal, subscription = self.makeProposalWithSubscription()

=== 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 @@
         # aleady.
         for review in self.votes:
             reviewer = review.reviewer
-            if not reviewer.is_team:
-                pending = review.comment is None
-                recipients[reviewer] = RecipientReason.forReviewer(
-                    self, pending, reviewer,
-                    branch_identity_cache=branch_identity_cache)
+            pending = review.comment is None
+            recipients[reviewer] = RecipientReason.forReviewer(
+                self, pending, reviewer,
+                branch_identity_cache=branch_identity_cache)
         # If the registrant of the proposal is getting emails, update the
         # rationale to say that they registered it.  Don't however send them
         # emails if they aren't asking for any.

=== 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 @@
         subscriber_set = set([source_owner, target_owner, reviewer])
         self.assertEqual(subscriber_set, set(recipients.keys()))
 
+    def test_getNotificationRecipientsIncludesTeamReviewers(self):
+        # If the reviewer is a team, the team gets the email.
+        bmp = self.factory.makeBranchMergeProposal()
+        # Both of the branch owners are now subscribed to their own
+        # branches with full code review notification level set.
+        source_owner = bmp.source_branch.owner
+        target_owner = bmp.target_branch.owner
+        login_person(source_owner)
+        reviewer = self.factory.makeTeam()
+        bmp.nominateReviewer(reviewer, registrant=source_owner)
+        recipients = bmp.getNotificationRecipients(
+            CodeReviewNotificationLevel.STATUS)
+        subscriber_set = set([source_owner, target_owner, reviewer])
+        self.assertEqual(subscriber_set, set(recipients.keys()))
+
     def test_getNotificationRecipients_Registrant(self):
         # If the registrant of the proposal is being notified of the
         # proposals, they get their rationale set to "Registrant".