launchpad-reviewers team mailing list archive

[Bayard 'kit' Randel <kit.randel@xxxxxxxxxxxxx>]

Bayard 'kit' Randel has proposed merging lp:~blr/launchpad/git-reviewer into lp:launchpad with lp:~blr/launchpad/db-patch-2209-61-3 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~blr/launchpad/git-reviewer/+merge/256351

Add GitRepository.reviewer, isPersonTrustedReviewer() and relevant tests.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~blr/launchpad/git-reviewer into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2015-04-13 19:02:15 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2015-04-15 16:21:30 +0000
@@ -179,9 +179,26 @@
         title=_("Display name"), readonly=True,
         description=_("Display name of the repository.")))
 
+    code_reviewer = Attribute(
+        "The reviewer if set, otherwise the owner of the repository.")
+
     shortened_path = Attribute(
         "The shortest reasonable version of the path to this repository.")
 
+    @operation_parameters(
+        reviewer=Reference(
+            title=_("A person for which the reviewer status is in question."),
+            schema=IPerson))
+    @export_read_operation()
+    @operation_for_version('beta')
+    def isPersonTrustedReviewer(reviewer):
+        """Return true if the `reviewer` is a trusted reviewer.
+
+        The reviewer is trusted if they either own the repository, or are in the
+        team that owns the repository, or they are in the review team for the
+        repository.
+        """
+
     git_identity = exported(Text(
         title=_("Git identity"), readonly=True,
         description=_(
@@ -343,6 +360,13 @@
     date_last_modified = exported(Datetime(
         title=_("Date last modified"), required=True, readonly=True))
 
+    reviewer = exported(PublicPersonChoice(
+        title=_("Review Team"), required=False, readonly=False,
+        vocabulary="ValidBranchReviewer",
+        description=_("The reviewer of a repository is the person or "
+                      "exclusive team that is responsible for reviewing "
+                      "proposals and merging into this repository.")))
+
     description = exported(Text(
         title=_("Description"), required=False, readonly=False,
         description=_("A short description of this repository.")))

=== modified file 'lib/lp/code/model/gitnamespace.py'
--- lib/lp/code/model/gitnamespace.py	2015-03-17 16:05:54 +0000
+++ lib/lp/code/model/gitnamespace.py	2015-04-15 16:21:30 +0000
@@ -59,8 +59,9 @@
 class _BaseGitNamespace:
     """Common code for Git repository namespaces."""
 
-    def createRepository(self, registrant, name, information_type=None,
-                         date_created=DEFAULT, description=None):
+    def createRepository(self, registrant, name, reviewer=None,
+                         information_type=None, date_created=DEFAULT,
+                         description=None):
         """See `IGitNamespace`."""
 
         self.validateRegistrant(registrant)
@@ -73,7 +74,7 @@
 
         repository = GitRepository(
             registrant, self.owner, self.target, name, information_type,
-            date_created, description=description)
+            date_created, reviewer=reviewer, description=description)
         repository._reconcileAccess()
 
         notify(ObjectCreatedEvent(repository))

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-03-30 09:46:03 +0000
+++ lib/lp/code/model/gitrepository.py	2015-04-15 16:21:30 +0000
@@ -46,7 +46,10 @@
     PUBLIC_INFORMATION_TYPES,
     )
 from lp.app.interfaces.informationtype import IInformationType
-from lp.app.interfaces.launchpad import IPrivacy
+from lp.app.interfaces.launchpad import (
+    ILaunchpadCelebrities,
+    IPrivacy,
+    )
 from lp.app.interfaces.services import IService
 from lp.code.enums import GitObjectType
 from lp.code.errors import (
@@ -150,6 +153,9 @@
     owner_id = Int(name='owner', allow_none=False)
     owner = Reference(owner_id, 'Person.id')
 
+    reviewer_id = Int(name='reviewer', allow_none=True)
+    reviewer = Reference(reviewer_id, 'Person.id')
+
     project_id = Int(name='project', allow_none=True)
     project = Reference(project_id, 'Product.id')
 
@@ -168,12 +174,13 @@
     target_default = Bool(name='target_default', allow_none=False)
 
     def __init__(self, registrant, owner, target, name, information_type,
-                 date_created, description=None):
+                 date_created, reviewer=None, description=None):
         if not getFeatureFlag(GIT_FEATURE_FLAG):
             raise GitFeatureDisabled
         super(GitRepository, self).__init__()
         self.registrant = registrant
         self.owner = owner
+        self.reviewer = reviewer
         self.name = name
         self.description = description
         self.information_type = information_type
@@ -269,6 +276,28 @@
     def display_name(self):
         return self.git_identity
 
+    @property
+    def code_reviewer(self):
+        """See `IGitRepository`."""
+        if self.reviewer:
+            return self.reviewer
+        else:
+            return self.owner
+
+    def isPersonTrustedReviewer(self, reviewer):
+        """See `IGitRepository`."""
+        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 getInternalPath(self):
         """See `IGitRepository`."""
         # This may need to change later to improve support for sharding.

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-03-20 14:54:23 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-04-15 16:21:30 +0000
@@ -801,7 +801,82 @@
         repository = self.factory.makeGitRepository(target=project)
         with person_logged_in(project.owner):
             repository.description = u"something"
+            repository.reviewer = project.owner
         self.assertEqual(u"something", repository.description)
+        self.assertEqual(project.owner, repository.reviewer)
+
+
+class TestGitRepositoryIsPersonTrustedReviewer(TestCaseWithFactory):
+    """Test the `IGitRepository.isPersonTrustedReviewer` method."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositoryIsPersonTrustedReviewer, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+    def assertTrustedReviewer(self, repository, person):
+        """Assert that `person` is a trusted reviewer for the `repository`."""
+        self.assertTrue(repository.isPersonTrustedReviewer(person))
+
+    def assertNotTrustedReviewer(self, repository, person):
+        """Assert that `person` is not a trusted reviewer for the
+        `repository`.
+        """
+        self.assertFalse(repository.isPersonTrustedReviewer(person))
+
+    def test_none_is_not_trusted(self):
+        # If None is passed in as the person, the method returns false.
+        repository = self.factory.makeGitRepository()
+        self.assertNotTrustedReviewer(repository, None)
+
+    def test_repository_owner_is_trusted(self):
+        # The repository owner is a trusted reviewer.
+        repository = self.factory.makeGitRepository()
+        self.assertTrustedReviewer(repository, repository.owner)
+
+    def test_non_repository_owner_is_not_trusted(self):
+        # Someone other than the repository owner is not a trusted reviewer.
+        repository = self.factory.makeGitRepository()
+        reviewer = self.factory.makePerson()
+        self.assertNotTrustedReviewer(repository, reviewer)
+
+    def test_lp_admins_always_trusted(self):
+        # Launchpad admins are special, and as such, are trusted.
+        repository = self.factory.makeGitRepository()
+        admins = getUtility(ILaunchpadCelebrities).admin
+        # Grab a random admin, the teamowner is good enough here.
+        self.assertTrustedReviewer(repository, admins.teamowner)
+
+    def test_member_of_team_owned_repository(self):
+        # If the repository is owned by a team, any team member is a trusted
+        # reviewer.
+        team = self.factory.makeTeam()
+        repository = self.factory.makeGitRepository(owner=team)
+        self.assertTrustedReviewer(repository, 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()
+        repository = self.factory.makeGitRepository(reviewer=team)
+        self.assertTrustedReviewer(repository, team.teamowner)
+
+    def test_repository_owner_not_review_team_member_is_trusted(self):
+        # If the owner of the repository is not in the review team,
+        #they are still trusted.
+        team = self.factory.makeTeam()
+        repository = self.factory.makeGitRepository(reviewer=team)
+        self.assertFalse(repository.owner.inTeam(team))
+        self.assertTrustedReviewer(repository, repository.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()
+        repository = self.factory.makeGitRepository(reviewer=team)
+        reviewer = self.factory.makePerson()
+        self.assertNotTrustedReviewer(repository, reviewer)
 
 
 class TestGitRepositorySetOwner(TestCaseWithFactory):

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2015-04-02 01:14:22 +0000
+++ lib/lp/testing/factory.py	2015-04-15 16:21:30 +0000
@@ -1657,7 +1657,7 @@
             revision_date=revision_date)
         return branch.createBranchRevision(sequence, revision)
 
-    def makeGitRepository(self, owner=None, target=_DEFAULT, registrant=None,
+    def makeGitRepository(self, owner=None, reviewer=None, target=_DEFAULT, registrant=None,
                           name=None, information_type=None,
                           **optional_repository_args):
         """Create and return a new, arbitrary GitRepository.
@@ -1681,7 +1681,8 @@
 
         namespace = get_git_namespace(target, owner)
         repository = namespace.createRepository(
-            registrant=registrant, name=name, **optional_repository_args)
+            registrant=registrant, name=name, reviewer=reviewer,
+            **optional_repository_args)
         naked_repository = removeSecurityProxy(repository)
         if information_type is not None:
             naked_repository.transitionToInformationType(