launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/better-review-request-330290 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/better-review-request-330290/+merge/89862

This branch will be part 1 of N. It adds the necessary model behaviour. Subsequent branches will add the behaviour to allow the user to be prompted that they really want to nominate the requested reviewer.

== Pre Implementation ==

Spoke with Curtis about whether subscribing to get visibility is what we wanted. It is for now.

== Implementation ==

Based on comments in bug 330290.
In the branch merge proposal nominateReviewer() method, subscribe the reviewer to the source/target branches if necessary to ensure visibility.

== Tests ==

Add new tests to test_branchmergeproposal:
 - test_nominate_person_grants_visibility
 - test_nominate_team_grants_visibility

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branchmergeproposal.py
  lib/lp/code/model/tests/test_branchmergeproposal.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/better-review-request-330290/+merge/89862
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/better-review-request-330290 into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2012-01-24 11:30:36 +0000
@@ -40,7 +40,10 @@
 
 from lp.code.enums import (
     BranchMergeProposalStatus,
+    BranchSubscriptionDiffSize,
+    BranchSubscriptionNotificationLevel,
     CodeReviewVote,
+    CodeReviewNotificationLevel,
     )
 from lp.code.errors import (
     BadBranchMergeProposalSearchContext,
@@ -613,6 +616,36 @@
                 review_type = review_type.lower()
         return review_type
 
+    def _subscribeUserToStackedBranch(self, branch, user,
+                                      checked_branches=None):
+        """Subscribe the user to the branch and those it is stacked on."""
+        if checked_branches is None:
+            checked_branches = []
+        branch.subscribe(
+            user,
+            BranchSubscriptionNotificationLevel.NOEMAIL,
+            BranchSubscriptionDiffSize.NODIFF,
+            CodeReviewNotificationLevel.FULL,
+            user)
+        if branch.stacked_on is not None:
+            checked_branches.append(branch)
+            if branch.stacked_on not in checked_branches:
+                self._subscribeUserToStackedBranch(
+                    branch.stacked_on, user, checked_branches)
+
+    def _ensureAssociatedBranchesVisibleToReviewer(self, reviewer):
+        """ A reviewer must be able to see the source and target branches.
+
+        Currently, we ensure the required visibility by subscribing the user
+        to the branch and those on which it is stacked.
+        """
+        source = self.source_branch
+        if not source.visibleByUser(reviewer):
+            self._subscribeUserToStackedBranch(source, reviewer)
+        target = self.target_branch
+        if not target.visibleByUser(reviewer):
+            self._subscribeUserToStackedBranch(target, reviewer)
+
     def nominateReviewer(self, reviewer, registrant, review_type=None,
                          _date_created=DEFAULT, _notify_listeners=True):
         """See `IBranchMergeProposal`."""
@@ -630,6 +663,7 @@
                 registrant=registrant,
                 reviewer=reviewer,
                 date_created=_date_created)
+            self._ensureAssociatedBranchesVisibleToReviewer(reviewer)
         vote_reference.review_type = review_type
         if _notify_listeners:
             notify(ReviewerNominatedEvent(vote_reference))

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2012-01-24 11:30:36 +0000
@@ -26,6 +26,7 @@
 from lp.app.interfaces.launchpad import IPrivacy
 from lp.code.enums import (
     BranchMergeProposalStatus,
+    BranchSubscriptionDiffSize,
     BranchSubscriptionNotificationLevel,
     BranchVisibilityRule,
     CodeReviewNotificationLevel,
@@ -1485,6 +1486,57 @@
         # Note we're using the reference from the first call
         self.assertEqual('specific', reference.review_type)
 
+    def _check_mp_branch_visibility(self, branch, reviewer):
+        # The reviewer is subscribed to the branch and can see it.
+        sub = branch.getSubscription(reviewer)
+        self.assertEqual(
+            BranchSubscriptionNotificationLevel.NOEMAIL,
+            sub.notification_level)
+        self.assertEqual(
+            BranchSubscriptionDiffSize.NODIFF,
+            sub.max_diff_lines)
+        self.assertEqual(
+            CodeReviewNotificationLevel.FULL,
+            sub.review_level)
+        # The reviewer can see the branch.
+        self.assertTrue(branch.visibleByUser(reviewer))
+        if branch.stacked_on is not None:
+            self._check_mp_branch_visibility(branch.stacked_on, reviewer)
+
+    def _test_nominate_grants_visibility(self, reviewer):
+        """Nominated reviewers can see the source and target branches."""
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        # We make a source branch stacked on a private one.
+        base_branch = self.factory.makeBranch(
+            owner=owner, private=True, product=product)
+        source_branch = self.factory.makeBranch(
+            stacked_on=base_branch, product=product)
+        target_branch = self.factory.makeBranch(owner=owner, product=product)
+        target_branch.product.setBranchVisibilityTeamPolicy(
+            owner, BranchVisibilityRule.PRIVATE)
+        login_person(owner)
+        merge_proposal = self.factory.makeBranchMergeProposal(
+            source_branch=source_branch,
+            target_branch=target_branch)
+        target_branch.setPrivate(True, owner)
+        # The reviewer can't see the source or target branches.
+        self.assertFalse(source_branch.visibleByUser(reviewer))
+        self.assertFalse(target_branch.visibleByUser(reviewer))
+        merge_proposal.nominateReviewer(
+            reviewer=reviewer,
+            registrant=merge_proposal.source_branch.owner)
+        for branch in [source_branch, target_branch]:
+            self._check_mp_branch_visibility(branch, reviewer)
+
+    def test_nominate_person_grants_visibility(self):
+        reviewer = self.factory.makePerson()
+        self._test_nominate_grants_visibility(reviewer)
+
+    def test_nominate_team_grants_visibility(self):
+        reviewer = self.factory.makeTeam()
+        self._test_nominate_grants_visibility(reviewer)
+
     def test_comment_with_vote_creates_reference(self):
         """A comment with a vote creates a vote reference."""
         reviewer = self.factory.makePerson()