launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === 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:43:22 +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')

Unlike Branch, this should be 'devel' for GitRepository, since this interface is new.

> +    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:43:22 +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:43:22 +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:43:22 +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:43:22 +0000
> @@ -1657,8 +1657,8 @@
>              revision_date=revision_date)
>          return branch.createBranchRevision(sequence, revision)
>  
> -    def makeGitRepository(self, owner=None, target=_DEFAULT, registrant=None,
> -                          name=None, information_type=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,11 +1681,14 @@
>  
>          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(
>                  information_type, registrant, verify_policy=False)
> +        if reviewer is not None:
> +            naked_repository.reviewer = reviewer

You do need to set the reviewer, but this does it twice, once by way of namespace.createRepository and once directly.  You could drop the second one.

This is inconsistent with *BranchNamespace.createRepository, which doesn't take a reviewer keyword argument and handles it via rSP in the test factory.  I think the approach you've taken here is cleaner though, with the exception of the duplication.

>          return repository
>  
>      def makeGitRefs(self, repository=None, paths=None):
> 


-- 
https://code.launchpad.net/~blr/launchpad/git-reviewer/+merge/256351
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.