Merge lp:~thumper/launchpad/trusted-reviewer into lp:launchpad

[Tim Penhey]

[Tim Penhey (thumper)]

= Summary =

The (Community) tag is incorrectly being shown on some reviews.

Normally when the branch is owned by a team and there is no review team
specified.

== Proposed fix ==

The browser view was looking at IBranch.reviewer instead of
IBranch.code_reviewer. Instead of just fixing this, I went to the source of
the problem and added a method 'isPersonTrustedReviewer' to the IBranch
interface. It was always intended that the trust is based on being the owner
of the branch OR in the review team. The current implementation only checked
the owner if the review team wasn't specified. This branch fixes this
deficiency.

The 'isPersonValidReviewer' is removed from IBranchMergeProposal, and call
sites are updated to check 'isPersonTrustedReviewer' on the target branch.

== Implementation details ==

The 'makeBranch' factory method is updated to be able to specify the
Branch.reviewer attribute. This is done by removing the security proxy so the
call site doesn't need to worry about launchpad.Edit for the branch.

== Tests ==

TestBranchIsPersonTrustedReviewer

[Michael Hudson-Doyle (mwhudson)]

Hi, looks basically fine.

Two questions:

1) is it really sane for admins to be able to review anything officially?
2) can you add a test of isPersonTrustedReviewer via the web service?

Cheers,
mwh

review: Approve

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2009-08-05 02:18:13 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2009-08-06 03:33:44 +0000
@@ -433,13 +433,11 @@
 
     def __init__(self, context, user, users_vote):
         self.context = context
-        is_mergable = self.context.branch_merge_proposal.isMergable()
+        proposal = self.context.branch_merge_proposal
+        is_mergable = proposal.isMergable()
         self.can_change_review = (user == context.reviewer) and is_mergable
-        branch = context.branch_merge_proposal.source_branch
-        review_team = context.branch_merge_proposal.target_branch.reviewer
-        reviewer = context.reviewer
-        self.trusted = (reviewer is not None and reviewer.inTeam(
-                        review_team))
+        self.trusted = proposal.target_branch.isPersonTrustedReviewer(
+            context.reviewer)
         if user is None:
             self.user_can_review = False
         else:
@@ -802,7 +800,7 @@
     def initial_values(self):
         # If the user is a valid reviewer, then default the revision
         # number to be the tip.
-        if self.context.isPersonValidReviewer(self.user):
+        if self.context.target_branch.isPersonTrustedReviewer(self.user):
             revision_number = self.context.source_branch.revision_count
         else:
             revision_number = self._getRevisionNumberForRevisionId(
@@ -820,14 +818,14 @@
         # If the user is not a valid reviewer for the target branch,
         # then the revision number should be read only, so an
         # untrusted user cannot land changes that have not bee reviewed.
-        if not self.context.isPersonValidReviewer(self.user):
+        if not self.context.target_branch.isPersonTrustedReviewer(self.user):
             self.form_fields['revision_number'].for_display = True
 
     @action('Enqueue', name='enqueue')
     @update_and_notify
     def enqueue_action(self, action, data):
         """Update the whiteboard and enqueue the merge proposal."""
-        if self.context.isPersonValidReviewer(self.user):
+        if self.context.target_branch.isPersonTrustedReviewer(self.user):
             revision_id = self._getRevisionId(data)
         else:
             revision_id = self.context.reviewed_revision_id

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2009-07-30 00:30:02 +0000
+++ lib/lp/code/configure.zcml	2009-08-06 03:33:44 +0000
@@ -181,7 +181,6 @@
                     votes
                     root_comment
                     all_comments
-                    isPersonValidReviewer
                     isMergable
                     getComment
                     getNotificationRecipients
@@ -348,6 +347,7 @@
                     author
                     reviewer
                     code_reviewer
+                    isPersonTrustedReviewer
                     product
                     unique_name
                     displayname

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2009-08-05 04:02:58 +0000
+++ lib/lp/code/interfaces/branch.py	2009-08-06 03:33:44 +0000
@@ -482,6 +482,19 @@
     code_reviewer = Attribute(
         "The reviewer if set, otherwise the owner of the branch.")
 
+    @operation_parameters(
+        reviewer=Reference(
+            title=_("A person for which the reviewer status is in question."),
+            schema=IPerson))
+    @export_read_operation()
+    def isPersonTrustedReviewer(reviewer):
+        """Return true if the `reviewer` is a trusted reviewer.
+
+        The reviewer is trusted if they are either own the branch, or are in
+        the team that owns the branch, or they are in the review team for the
+        branch.
+        """
+
     namespace = Attribute(
         "The namespace of this branch, as an `IBranchNamespace`.")
 

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2009-08-04 05:02:41 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2009-08-06 00:15:48 +0000
@@ -395,21 +395,6 @@
         user-entered data like the whiteboard.
         """
 
-    @operation_parameters(
-        reviewer=Reference(
-            title=_("A person for which the reviewer status is in question."),
-            schema=IPerson))
-    @export_read_operation()
-    def isPersonValidReviewer(reviewer):
-        """Return true if the `reviewer` is able to review the proposal.
-
-        There is an attribute on branches called `reviewer` which allows
-        a specific person or team to be set for a branch as an authorised
-        person to approve merges for a branch.  If a reviewer is not set
-        on the target branch, then the owner of the target branch is used
-        as the authorised user.
-        """
-
     def isMergable():
         """Is the proposal in a state that allows it to being merged?
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2009-07-31 01:07:04 +0000
+++ lib/lp/code/model/branch.py	2009-08-06 03:33:44 +0000
@@ -36,6 +36,7 @@
 
 from canonical.launchpad import _
 from lp.services.job.model.job import Job
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.mailnotification import NotificationRecipientSet
 from canonical.launchpad.webapp import urlappend
 from canonical.launchpad.webapp.interfaces import (
@@ -440,6 +441,20 @@
         else:
             return self.owner
 
+    def isPersonTrustedReviewer(self, reviewer):
+        """See `IBranch`."""
+        if reviewer is None:
+            return False
+        # We trust Launchpad admins.
+        lp_admins = getUtility(ILaunchpadCelebrities).admin
+        # Both the branch owner and the review team are checked.
+        owner = self.owner
+        review_team = self.code_reviewer
+        return (
+            reviewer.inTeam(owner) or
+            reviewer.inTeam(review_team) or
+            reviewer.inTeam(lp_admins))
+
     def latest_revisions(self, quantity=10):
         """See `IBranch`."""
         return self.revision_history.limit(quantity)

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2009-07-23 18:08:35 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2009-08-06 00:15:48 +0000
@@ -47,7 +47,6 @@
     IBranchMergeProposal, IBranchMergeProposalGetter, UserNotBranchReviewer,
     WrongBranchMergeProposal)
 from lp.code.interfaces.branchtarget import IHasBranchTarget
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import IProduct
 from lp.code.mail.branch import RecipientReason
@@ -78,7 +77,7 @@
     # Transitioning to code approved, rejected or queued from
     # work in progress, needs review or merge failed needs the
     # user to be a valid reviewer, other states are fine.
-    valid_reviewer = proposal.isPersonValidReviewer(user)
+    valid_reviewer = proposal.target_branch.isPersonTrustedReviewer(user)
     if (next_state == rejected and not valid_reviewer):
         return False
     # Non-reviewers can toggle between code_approved and queued, but not
@@ -329,15 +328,6 @@
             self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
             self.date_review_requested = UTC_NOW
 
-    def isPersonValidReviewer(self, reviewer):
-        """See `IBranchMergeProposal`."""
-        if reviewer is None:
-            return False
-        # We trust Launchpad admins.
-        lp_admins = getUtility(ILaunchpadCelebrities).admin
-        return (reviewer.inTeam(self.target_branch.code_reviewer) or
-                reviewer.inTeam(lp_admins))
-
     def isMergable(self):
         """See `IBranchMergeProposal`."""
         # As long as the source branch has not been merged, rejected
@@ -348,7 +338,7 @@
         """Set the proposal to one of the two review statuses."""
         # Check the reviewer can review the code for the target branch.
         old_state = self.queue_status
-        if not self.isPersonValidReviewer(reviewer):
+        if not self.target_branch.isPersonTrustedReviewer(reviewer):
             raise UserNotBranchReviewer
         # Check the current state of the proposal.
         self._transitionToState(next_state, reviewer)

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2009-08-05 02:04:06 +0000
+++ lib/lp/code/model/tests/test_branch.py	2009-08-06 03:33:44 +0000
@@ -1651,6 +1651,73 @@
         self.assertEqual(branch.spec_links.count(), 0)
 
 
+class TestBranchIsPersonTrustedReviewer(TestCaseWithFactory):
+    """Test the `IBranch.isPersonTrustedReviewer` method."""
+
+    layer = DatabaseFunctionalLayer
+
+    def assertTrustedReviewer(self, branch, person):
+        """Assert that `person` is a trusted reviewer for the `branch`."""
+        self.assertTrue(branch.isPersonTrustedReviewer(person))
+
+    def assertNotTrustedReviewer(self, branch, person):
+        """Assert that `person` is not a trusted reviewer for the `branch`."""
+        self.assertFalse(branch.isPersonTrustedReviewer(person))
+
+    def test_none_is_not_trusted(self):
+        # If None is passed in as the person, the method returns false.
+        branch = self.factory.makeAnyBranch()
+        self.assertNotTrustedReviewer(branch, None)
+
+    def test_branch_owner_is_trusted(self):
+        # The branch owner is a trusted reviewer.
+        branch = self.factory.makeAnyBranch()
+        self.assertTrustedReviewer(branch, branch.owner)
+
+    def test_non_branch_owner_is_not_trusted(self):
+        # Someone other than the branch owner is not a trusted reviewer.
+        branch = self.factory.makeAnyBranch()
+        reviewer = self.factory.makePerson()
+        self.assertNotTrustedReviewer(branch, reviewer)
+
+    def test_lp_admins_always_trusted(self):
+        # Launchpad admins are special, and as such, are trusted.
+        branch = self.factory.makeAnyBranch()
+        admins = getUtility(ILaunchpadCelebrities).admin
+        # Grab a random admin, the teamowner is good enough here.
+        self.assertTrustedReviewer(branch, admins.teamowner)
+
+    def test_member_of_team_owned_branch(self):
+        # If the branch is owned by a team, any team member is a trusted
+        # reviewer.
+        team = self.factory.makeTeam()
+        branch = self.factory.makeAnyBranch(owner=team)
+        self.assertTrustedReviewer(branch, team.teamowner)
+
+    def test_review_team_member_is_trusted(self):
+        # If the reviewer is a member of the review team, but not the owner
+        # they are still trusted.
+        team = self.factory.makeTeam()
+        branch = self.factory.makeAnyBranch(reviewer=team)
+        self.assertTrustedReviewer(branch, team.teamowner)
+
+    def test_branch_owner_not_review_team_member_is_trusted(self):
+        # If the owner of the branch is not in the review team, they are still
+        # trusted.
+        team = self.factory.makeTeam()
+        branch = self.factory.makeAnyBranch(reviewer=team)
+        self.assertFalse(branch.owner.inTeam(team))
+        self.assertTrustedReviewer(branch, branch.owner)
+
+    def test_community_reviewer(self):
+        # If the reviewer is not a member of the owner, or the review team,
+        # they are not trusted reviewers.
+        team = self.factory.makeTeam()
+        branch = self.factory.makeAnyBranch(reviewer=team)
+        reviewer = self.factory.makePerson()
+        self.assertNotTrustedReviewer(branch, reviewer)
+
+
 class TestBranchSetOwner(TestCaseWithFactory):
     """Tests for IBranch.setOwner."""
 

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-07-22 16:02:55 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-08-06 00:15:48 +0000
@@ -22,6 +22,7 @@
 from canonical.testing import (
     DatabaseFunctionalLayer, LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
 
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.code.model.branchmergeproposaljob import (
     BranchMergeProposalJob, BranchMergeProposalJobType,
     CreateMergeProposalJob, MergeProposalCreatedJob)
@@ -32,7 +33,7 @@
     NewBranchMergeProposalEvent, NewCodeReviewCommentEvent,
     ReviewerNominatedEvent)
 from canonical.launchpad.ftests import (
-    ANONYMOUS, import_secret_test_key, login, logout, syncUpdate)
+    ANONYMOUS, import_secret_test_key, login, syncUpdate)
 from lp.code.enums import (
     BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,
     BranchType, CodeReviewNotificationLevel, CodeReviewVote)
@@ -395,36 +396,6 @@
             proposal.date_created, proposal.date_review_requested)
 
 
-class TestBranchMergeProposalCanReview(TestCase):
-    """Test the different cases that makes a branch deletable or not."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        login('[e-mail]')
-
-        factory = LaunchpadObjectFactory()
-        self.source_branch = factory.makeProductBranch()
-        self.target_branch = factory.makeProductBranch(
-            product=self.source_branch.product)
-        registrant = factory.makePerson()
-        self.proposal = self.source_branch.addLandingTarget(
-            registrant, self.target_branch)
-
-    def tearDown(self):
-        logout()
-
-    def test_validReviewer(self):
-        """A newly created branch can be deleted without any problems."""
-        self.assertEqual(self.proposal.isPersonValidReviewer(None),
-                         False, "No user cannot review code")
-        # The owner of the target branch is a valid reviewer.
-        self.assertEqual(
-            self.proposal.isPersonValidReviewer(
-                self.target_branch.owner),
-            True, "No user cannot review code")
-
-
 class TestBranchMergeProposalQueueing(TestCase):
     """Test the enqueueing and dequeueing of merge proposals."""
 

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2009-07-16 01:22:05 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2009-08-06 00:15:48 +0000
@@ -117,14 +117,7 @@
 
 A review can be performed through the API.
 
-    >>> person = webservice.get('/~target').jsonBody()
-    >>> is_valid = webservice.named_get(
-    ...     merge_proposal['self_link'], 'isPersonValidReviewer',
-    ...     reviewer=person['self_link']).jsonBody()
-    >>> print is_valid
-    True
-
-A review can be requested of the person 'target', since he's a valid reviewer.
+A review can be requested of the person 'target'.
 
     >>> reviewer_webservice = webservice_for_person(
     ...     target_owner, permission=OAuthPermission.WRITE_PUBLIC)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-08-03 21:46:09 +0000
+++ lib/lp/testing/factory.py	2009-08-06 03:33:44 +0000
@@ -574,7 +574,7 @@
     def makeBranch(self, branch_type=None, owner=None,
                    name=None, product=_DEFAULT, url=_DEFAULT, registrant=None,
                    private=False, stacked_on=None, sourcepackage=None,
-                   **optional_branch_args):
+                   reviewer=None, **optional_branch_args):
         """Create and return a new, arbitrary Branch of the given type.
 
         Any parameters for `IBranchNamespace.createBranch` can be specified to
@@ -624,6 +624,8 @@
             removeSecurityProxy(branch).private = True
         if stacked_on is not None:
             removeSecurityProxy(branch).stacked_on = stacked_on
+        if reviewer is not None:
+            removeSecurityProxy(branch).reviewer = reviewer
         return branch
 
     def makePackageBranch(self, sourcepackage=None, distroseries=None,