launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === added file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2015-02-16 15:43:03 +0000
> @@ -0,0 +1,389 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for Git repositories."""
> +
> +__metaclass__ = type
> +
> +from datetime import datetime
> +
> +from lazr.lifecycle.event import ObjectModifiedEvent
> +import pytz
> +from zope.component import getUtility
> +from zope.event import notify
> +from zope.security.proxy import removeSecurityProxy
> +
> +from lp.app.enums import (
> +    InformationType,
> +    PRIVATE_INFORMATION_TYPES,
> +    PUBLIC_INFORMATION_TYPES,
> +    )
> +from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> +from lp.code.errors import (
> +    GitRepositoryCreatorNotMemberOfOwnerTeam,
> +    GitRepositoryCreatorNotOwner,
> +    GitTargetError,
> +    )
> +from lp.code.interfaces.gitnamespace import (
> +    IGitNamespacePolicy,
> +    IGitNamespaceSet,
> +    )
> +from lp.code.interfaces.gitrepository import IGitRepository
> +from lp.registry.enums import BranchSharingPolicy
> +from lp.services.database.constants import UTC_NOW
> +from lp.services.webapp.authorization import check_permission
> +from lp.testing import (
> +    admin_logged_in,
> +    celebrity_logged_in,
> +    person_logged_in,
> +    TestCaseWithFactory,
> +    verifyObject,
> +    )
> +from lp.testing.layers import DatabaseFunctionalLayer
> +
> +
> +class TestGitRepository(TestCaseWithFactory):
> +    """Test basic properties about Launchpad database Git repositories."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_implements_IGitRepository(self):
> +        repository = self.factory.makeGitRepository()
> +        verifyObject(IGitRepository, repository)
> +
> +    def test_unique_name_project(self):
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeProjectGitRepository(project=project)
> +        self.assertEqual(
> +            "~%s/%s/+git/%s" % (
> +                repository.owner.name, project.name, repository.name),
> +            repository.unique_name)
> +
> +    def test_unique_name_package(self):
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makePackageGitRepository(
> +            distro_source_package=dsp)
> +        self.assertEqual(
> +            "~%s/%s/+source/%s/+git/%s" % (
> +                repository.owner.name, dsp.distribution.name,
> +                dsp.sourcepackagename.name, repository.name),
> +            repository.unique_name)
> +
> +    def test_unique_name_personal(self):
> +        repository = self.factory.makePersonalGitRepository()
> +        self.assertEqual(
> +            "~%s/+git/%s" % (repository.owner.name, repository.name),
> +            repository.unique_name)
> +
> +    def test_target_project(self):
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeProjectGitRepository(project=project)
> +        self.assertEqual(project, repository.target)
> +
> +    def test_target_package(self):
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makePackageGitRepository(
> +            distro_source_package=dsp)
> +        self.assertEqual(dsp, repository.target)
> +
> +    def test_target_personal(self):
> +        repository = self.factory.makePersonalGitRepository()
> +        self.assertEqual(repository.owner, repository.target)
> +
> +
> +class TestGitIdentityMixin(TestCaseWithFactory):
> +    """Test the defaults and identities provided by GitIdentityMixin."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def assertGitIdentity(self, repository, identity_path):
> +        """Assert that the Git identity of 'repository' is 'identity_path'.
> +
> +        Actually, it'll be lp:<identity_path>.
> +        """
> +        self.assertEqual(
> +            identity_path, repository.shortened_path, "shortened path")
> +        self.assertEqual(
> +            "lp:%s" % identity_path, repository.git_identity, "git identity")
> +
> +    def test_git_identity_default(self):
> +        # By default, the Git identity is the repository's unique name.
> +        repository = self.factory.makeAnyGitRepository()
> +        self.assertGitIdentity(repository, repository.unique_name)
> +
> +    def test_identities_no_defaults(self):
> +        # If there are no defaults, the only repository identity is the
> +        # unique name.
> +        repository = self.factory.makeAnyGitRepository()
> +        self.assertEqual(
> +            [(repository.unique_name, repository)],
> +            repository.getRepositoryIdentities())
> +
> +    # XXX cjwatson 2015-02-12: This will need to be expanded once support
> +    # for default repositories is in place.
> +
> +
> +class TestGitRepositoryDateLastModified(TestCaseWithFactory):
> +    """Exercise the situations where date_last_modified is updated."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_initial_value(self):
> +        # The initial value of date_last_modified is date_created.
> +        repository = self.factory.makeAnyGitRepository()
> +        self.assertEqual(
> +            repository.date_created, repository.date_last_modified)
> +
> +    def test_modifiedevent_sets_date_last_modified(self):
> +        # When a GitRepository receives an object modified event, the last
> +        # modified date is set to UTC_NOW.
> +        repository = self.factory.makeAnyGitRepository(
> +            date_created=datetime(2015, 02, 04, 17, 42, 0, tzinfo=pytz.UTC))
> +        notify(ObjectModifiedEvent(
> +            removeSecurityProxy(repository), repository,
> +            [IGitRepository["name"]]))
> +        self.assertSqlAttributeEqualsDate(
> +            repository, "date_last_modified", UTC_NOW)
> +
> +    # XXX cjwatson 2015-02-04: This will need to be expanded once Launchpad
> +    # actually notices any interesting kind of repository modifications.
> +
> +
> +class TestCodebrowse(TestCaseWithFactory):
> +    """Tests for Git repository codebrowse support."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_simple(self):
> +        # The basic codebrowse URL for a repository is an 'https' URL.
> +        repository = self.factory.makeAnyGitRepository()
> +        self.assertEqual(
> +            "https://git.launchpad.dev/"; + repository.unique_name,
> +            repository.getCodebrowseUrl())
> +
> +
> +class TestGitRepositoryNamespace(TestCaseWithFactory):
> +    """Test `IGitRepository.namespace`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_namespace_personal(self):
> +        # The namespace attribute of a personal repository points to the
> +        # namespace that corresponds to ~owner.
> +        repository = self.factory.makePersonalGitRepository()
> +        namespace = getUtility(IGitNamespaceSet).get(person=repository.owner)
> +        self.assertEqual(namespace, repository.namespace)
> +
> +    def test_namespace_project(self):
> +        # The namespace attribute of a project repository points to the
> +        # namespace that corresponds to ~owner/project.
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeProjectGitRepository(project=project)
> +        namespace = getUtility(IGitNamespaceSet).get(
> +            person=repository.owner, project=project)
> +        self.assertEqual(namespace, repository.namespace)
> +
> +    def test_namespace_package(self):
> +        # The namespace attribute of a package repository points to the
> +        # namespace that corresponds to
> +        # ~owner/distribution/+source/sourcepackagename.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makePackageGitRepository(
> +            distro_source_package=dsp)
> +        namespace = getUtility(IGitNamespaceSet).get(
> +            person=repository.owner, distribution=dsp.distribution,
> +            sourcepackagename=dsp.sourcepackagename)
> +        self.assertEqual(namespace, repository.namespace)
> +
> +
> +class TestGitRepositoryGetAllowedInformationTypes(TestCaseWithFactory):
> +    """Test `IGitRepository.getAllowedInformationTypes`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_normal_user_sees_namespace_types(self):
> +        # An unprivileged user sees the types allowed by the namespace.
> +        repository = self.factory.makeGitRepository()
> +        policy = IGitNamespacePolicy(repository.namespace)
> +        self.assertContentEqual(
> +            policy.getAllowedInformationTypes(),
> +            repository.getAllowedInformationTypes(repository.owner))
> +        self.assertNotIn(
> +            InformationType.PROPRIETARY,
> +            repository.getAllowedInformationTypes(repository.owner))
> +        self.assertNotIn(
> +            InformationType.EMBARGOED,
> +            repository.getAllowedInformationTypes(repository.owner))
> +
> +    def test_admin_sees_namespace_types(self):
> +        # An admin sees all the types, since they occasionally need to
> +        # override the namespace rules.  This is hopefully temporary, and
> +        # can go away once the new sharing rules (granting non-commercial
> +        # projects limited use of private repositories) are deployed.
> +        repository = self.factory.makeGitRepository()
> +        admin = self.factory.makeAdministrator()
> +        self.assertContentEqual(
> +            PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
> +            repository.getAllowedInformationTypes(admin))
> +        self.assertIn(
> +            InformationType.PROPRIETARY,
> +            repository.getAllowedInformationTypes(admin))
> +
> +
> +class TestGitRepositoryModerate(TestCaseWithFactory):
> +    """Test that project owners and commercial admins can moderate Git
> +    repositories."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_moderate_permission(self):
> +        # Test the ModerateGitRepository security checker.
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeProjectGitRepository(project=project)
> +        with person_logged_in(project.owner):
> +            self.assertTrue(check_permission("launchpad.Moderate", repository))
> +        with celebrity_logged_in("commercial_admin"):
> +            self.assertTrue(check_permission("launchpad.Moderate", repository))
> +        with person_logged_in(self.factory.makePerson()):
> +            self.assertFalse(
> +                check_permission("launchpad.Moderate", repository))
> +
> +    def test_attribute_smoketest(self):
> +        # Users with launchpad.Moderate can set attributes.
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeProjectGitRepository(project=project)
> +        with person_logged_in(project.owner):
> +            repository.name = u"not-secret"
> +        self.assertEqual(u"not-secret", repository.name)
> +
> +
> +class TestGitRepositorySetOwner(TestCaseWithFactory):
> +    """Test `IGitRepository.setOwner`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_owner_sets_team(self):
> +        # The owner of the repository can set the owner of the repository to
> +        # be a team they are a member of.
> +        repository = self.factory.makeAnyGitRepository()
> +        team = self.factory.makeTeam(owner=repository.owner)
> +        with person_logged_in(repository.owner):
> +            repository.setOwner(team, repository.owner)
> +        self.assertEqual(team, repository.owner)
> +
> +    def test_owner_cannot_set_nonmember_team(self):
> +        # The owner of the repository cannot set the owner to be a team they
> +        # are not a member of.
> +        repository = self.factory.makeAnyGitRepository()
> +        team = self.factory.makeTeam()
> +        with person_logged_in(repository.owner):
> +            self.assertRaises(
> +                GitRepositoryCreatorNotMemberOfOwnerTeam,
> +                repository.setOwner, team, repository.owner)
> +
> +    def test_owner_cannot_set_other_user(self):
> +        # The owner of the repository cannot set the new owner to be another
> +        # person.
> +        repository = self.factory.makeAnyGitRepository()
> +        person = self.factory.makePerson()
> +        with person_logged_in(repository.owner):
> +            self.assertRaises(
> +                GitRepositoryCreatorNotOwner,
> +                repository.setOwner, person, repository.owner)
> +
> +    def test_admin_can_set_any_team_or_person(self):
> +        # A Launchpad admin can set the repository to be owned by any team
> +        # or person.
> +        repository = self.factory.makeAnyGitRepository()
> +        team = self.factory.makeTeam()
> +        # To get a random administrator, choose the admin team owner.
> +        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
> +        with person_logged_in(admin):
> +            repository.setOwner(team, admin)
> +            self.assertEqual(team, repository.owner)
> +            person = self.factory.makePerson()
> +            repository.setOwner(person, admin)
> +            self.assertEqual(person, repository.owner)
> +
> +
> +class TestGitRepositorySetTarget(TestCaseWithFactory):
> +    """Test `IGitRepository.setTarget`."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_personal_to_project(self):
> +        # A personal repository can be moved to a project.
> +        repository = self.factory.makePersonalGitRepository()
> +        project = self.factory.makeProduct()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=project, user=repository.owner)
> +        self.assertEqual(project, repository.target)
> +
> +    def test_personal_to_package(self):
> +        # A personal repository can be moved to a package.
> +        repository = self.factory.makePersonalGitRepository()
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=dsp, user=repository.owner)
> +        self.assertEqual(dsp, repository.target)
> +
> +    def test_project_to_other_project(self):
> +        # Move a repository from one project to another.
> +        repository = self.factory.makeProjectGitRepository()
> +        project = self.factory.makeProduct()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=project, user=repository.owner)
> +        self.assertEqual(project, repository.target)
> +
> +    def test_project_to_package(self):
> +        # Move a repository from a project to a package.
> +        repository = self.factory.makeProjectGitRepository()
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=dsp, user=repository.owner)
> +        self.assertEqual(dsp, repository.target)
> +
> +    def test_project_to_personal(self):
> +        # Move a repository from a project to a personal namespace.
> +        owner = self.factory.makePerson()
> +        repository = self.factory.makeProjectGitRepository(owner=owner)
> +        with person_logged_in(owner):
> +            repository.setTarget(target=owner, user=owner)
> +        self.assertEqual(owner, repository.target)
> +
> +    def test_package_to_other_package(self):
> +        # Move a repository from one package to another.
> +        repository = self.factory.makePackageGitRepository()
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=dsp, user=repository.owner)
> +        self.assertEqual(dsp, repository.target)
> +
> +    def test_package_to_project(self):
> +        # Move a repository from a package to a project.
> +        repository = self.factory.makePackageGitRepository()
> +        project = self.factory.makeProduct()
> +        with person_logged_in(repository.owner):
> +            repository.setTarget(target=project, user=repository.owner)
> +        self.assertEqual(project, repository.target)
> +
> +    def test_package_to_personal(self):
> +        # Move a repository from a package to a personal namespace.
> +        owner = self.factory.makePerson()
> +        repository = self.factory.makePackageGitRepository(owner=owner)
> +        with person_logged_in(owner):
> +            repository.setTarget(target=owner, user=owner)
> +        self.assertEqual(owner, repository.target)
> +
> +    def test_public_to_proprietary_only_project(self):
> +        # A repository cannot be moved to a target where the sharing policy
> +        # does not allow it.
> +        owner = self.factory.makePerson()
> +        commercial_project = self.factory.makeProduct(
> +            owner=owner, branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
> +        repository = self.factory.makeGitRepository(
> +            owner=owner, information_type=InformationType.PUBLIC)
> +        with admin_logged_in():
> +            self.assertRaises(
> +                GitTargetError, repository.setTarget,
> +                target=commercial_project, user=owner)
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2015-01-29 16:28:30 +0000
> +++ lib/lp/testing/factory.py	2015-02-16 15:43:03 +0000
> @@ -2,7 +2,7 @@
>  # NOTE: The first line above must stay first; do not move the copyright
>  # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
>  #
> -# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Testing infrastructure for the Launchpad application.
> @@ -118,6 +118,7 @@
>  from lp.code.interfaces.codeimportevent import ICodeImportEventSet
>  from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
>  from lp.code.interfaces.codeimportresult import ICodeImportResultSet
> +from lp.code.interfaces.gitnamespace import get_git_namespace
>  from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
>  from lp.code.interfaces.revision import IRevisionSet
>  from lp.code.interfaces.sourcepackagerecipe import (
> @@ -1668,6 +1669,93 @@
>              revision_date=revision_date)
>          return branch.createBranchRevision(sequence, revision)
>  
> +    def makeGitRepository(self, owner=None, project=_DEFAULT,
> +                          distro_source_package=None, registrant=None,
> +                          name=None, information_type=None,
> +                          **optional_repository_args):
> +        """Create and return a new, arbitrary GitRepository.
> +
> +        Any parameters for `IGitNamespace.createRepository` can be specified
> +        to override the default ones.
> +        """
> +        if owner is None:
> +            owner = self.makePerson()
> +        if name is None:
> +            name = self.getUniqueString('gitrepository').decode('utf-8')
> +
> +        if distro_source_package is None:
> +            if project is _DEFAULT:
> +                project = self.makeProduct()
> +            target = project
> +        else:
> +            assert project is _DEFAULT, (
> +                "Passed both distribution source package and project details")
> +            target = distro_source_package

Any reason to take the split arguments rather than just target?

> +
> +        if registrant is None:
> +            if owner.is_team:
> +                registrant = removeSecurityProxy(owner).teamowner
> +            else:
> +                registrant = owner
> +
> +        namespace = get_git_namespace(target, owner)
> +        repository = namespace.createRepository(
> +            registrant=registrant, name=name, **optional_repository_args)
> +        naked_repository = removeSecurityProxy(repository)
> +        if information_type is not None:
> +            naked_repository.transitionToInformationType(
> +                information_type, registrant, verify_policy=False)
> +        return repository
> +
> +    def makePackageGitRepository(self, distro_source_package=None,
> +                                 distribution=None, sourcepackagename=None,
> +                                 **kwargs):
> +        """Make a package Git repository on an arbitrary package.
> +
> +        See `makeGitRepository` for more information on arguments.
> +
> +        You can pass in either `distro_source_package` or one or both of
> +        `distribution` and `sourcepackagename`, but not combinations or all
> +        of them.
> +        """
> +        assert not(distro_source_package is not None and
> +                   distribution is not None), (
> +            "Don't pass in both distro_source_package and distribution")
> +        assert not(distro_source_package is not None
> +                   and sourcepackagename is not None), (
> +            "Don't pass in both distro_source_package and sourcepackagename")
> +        if distro_source_package is None:
> +            distro_source_package = self.makeDistributionSourcePackage(
> +                distribution=distribution, sourcepackagename=sourcepackagename)
> +        return self.makeGitRepository(
> +            distro_source_package=distro_source_package, **kwargs)
> +
> +    def makePersonalGitRepository(self, owner=None, **kwargs):
> +        """Make a personal Git repository on an arbitrary person.
> +
> +        See `makeGitRepository` for more information on arguments.
> +        """
> +        if owner is None:
> +            owner = self.makePerson()
> +        return self.makeGitRepository(
> +            owner=owner, project=None, distro_source_package=None, **kwargs)
> +
> +    def makeProjectGitRepository(self, project=None, **kwargs):
> +        """Make a project Git repository on an arbitrary project.
> +
> +        See `makeGitRepository` for more information on arguments.
> +        """
> +        if project is None:
> +            project = self.makeProduct()
> +        return self.makeGitRepository(project=project, **kwargs)
> +
> +    def makeAnyGitRepository(self, **kwargs):
> +        """Make a Git repository without caring about its container.
> +
> +        See `makeGitRepository` for more information on arguments.
> +        """
> +        return self.makeProjectGitRepository(**kwargs)

Are all these target-specific helpers worth it? I haven't checked how extensively the Branch equivalents are used.

> +
>      def makeBug(self, target=None, owner=None, bug_watch_url=None,
>                  information_type=None, date_closed=None, title=None,
>                  date_created=None, description=None, comment=None,
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-testing/+merge/249840
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.