launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

Death to domain-specific pollution of Person and Product.

Diff comments:

> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2015-03-03 15:05:59 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2015-03-04 19:09:38 +0000
> @@ -320,6 +320,16 @@
>          Return None if no match was found.
>          """
>  
> +    def getRepositories(user, target):
> +        """Get all repositories for a target.
> +
> +        :param user: An `IPerson`.  Only repositories visible by this user
> +            will be returned.
> +        :param target: An `IHasGitRepositories`.
> +
> +        :return: A collection of `IGitRepository` objects.
> +        """
> +
>      def getDefaultRepository(target):
>          """Get the default repository for a target.
>  
> @@ -360,7 +370,7 @@
>          :raises GitTargetError: if `target` is an `IPerson`.
>          """
>  
> -    def getRepositories():
> +    def getAllRepositories():

This name isn't exposed anywhere, so it really only serves to be confusing internally.

>          """Return an empty collection of repositories.
>  
>          This only exists to keep lazr.restful happy.
> 
> === modified file 'lib/lp/code/interfaces/hasgitrepositories.py'
> --- lib/lp/code/interfaces/hasgitrepositories.py	2015-02-09 16:49:01 +0000
> +++ lib/lp/code/interfaces/hasgitrepositories.py	2015-03-04 19:09:38 +0000
> @@ -18,23 +18,3 @@
>      A project contains Git repositories, a source package on a distribution
>      contains branches, and a person contains "personal" branches.
>      """
> -
> -    def getGitRepositories(visible_by_user=None, eager_load=False):
> -        """Returns all Git repositories related to this object.
> -
> -        :param visible_by_user: Normally the user who is asking.
> -        :param eager_load: If True, load related objects for the whole
> -            collection.
> -        :returns: A list of `IGitRepository` objects.
> -        """
> -
> -    def createGitRepository(registrant, owner, name, information_type=None):
> -        """Create a Git repository for this target and return it.
> -
> -        :param registrant: The `IPerson` who registered the new repository.
> -        :param owner: The `IPerson` who owns the new repository.
> -        :param name: The repository name.
> -        :param information_type: Set the repository's information type to
> -            one different from the target's default.  The type must conform
> -            to the target's code sharing policy.  (optional)
> -        """
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2015-03-03 15:05:59 +0000
> +++ lib/lp/code/model/gitrepository.py	2015-03-04 19:09:38 +0000
> @@ -40,7 +40,10 @@
>      GitDefaultConflict,
>      GitTargetError,
>      )
> -from lp.code.interfaces.gitcollection import IAllGitRepositories
> +from lp.code.interfaces.gitcollection import (
> +    IAllGitRepositories,
> +    IGitCollection,
> +    )
>  from lp.code.interfaces.gitlookup import IGitLookup
>  from lp.code.interfaces.gitnamespace import (
>      get_git_namespace,
> @@ -368,6 +371,11 @@
>              return repository
>          return None
>  
> +    def getRepositories(self, user, target):
> +        """See `IGitRepositorySet`."""
> +        collection = IGitCollection(target).visibleByUser(user)
> +        return collection.getRepositories(eager_load=True)
> +
>      def getDefaultRepository(self, target):
>          """See `IGitRepositorySet`."""
>          clauses = [GitRepository.target_default == True]
> @@ -439,7 +447,7 @@
>              if previous is not None:
>                  previous.setOwnerDefault(False)
>  
> -    def getRepositories(self):
> +    def getAllRepositories(self):
>          """See `IGitRepositorySet`."""
>          return []
>  
> 
> === removed file 'lib/lp/code/model/hasgitrepositories.py'
> --- lib/lp/code/model/hasgitrepositories.py	2015-02-23 17:59:37 +0000
> +++ lib/lp/code/model/hasgitrepositories.py	1970-01-01 00:00:00 +0000
> @@ -1,28 +0,0 @@
> -# Copyright 2015 Canonical Ltd.  This software is licensed under the
> -# GNU Affero General Public License version 3 (see the file LICENSE).
> -
> -__metaclass__ = type
> -__all__ = [
> -    'HasGitRepositoriesMixin',
> -    ]
> -
> -from zope.component import getUtility
> -
> -from lp.code.interfaces.gitcollection import IGitCollection
> -from lp.code.interfaces.gitrepository import IGitRepositorySet
> -
> -
> -class HasGitRepositoriesMixin:
> -    """A mixin implementation for `IHasGitRepositories`."""
> -
> -    def createGitRepository(self, registrant, owner, name,
> -                            information_type=None):
> -        """See `IHasGitRepositories`."""
> -        return getUtility(IGitRepositorySet).new(
> -            registrant, owner, self, name,
> -            information_type=information_type)
> -
> -    def getGitRepositories(self, visible_by_user=None, eager_load=False):
> -        """See `IHasGitRepositories`."""
> -        collection = IGitCollection(self).visibleByUser(visible_by_user)
> -        return collection.getRepositories(eager_load=eager_load)
> 
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	2015-02-26 17:32:56 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2015-03-04 19:09:38 +0000
> @@ -660,6 +660,37 @@
>          self.assertIsNone(
>              self.repository_set.getByPath(self.factory.makePerson(), path))
>  
> +    def test_getRepositories(self):
> +        # getRepositories returns a collection of repositories for the given
> +        # target.
> +        project = self.factory.makeProduct()
> +        repositories = [
> +            self.factory.makeGitRepository(target=project) for _ in range(5)]
> +        self.assertContentEqual(
> +            repositories, self.repository_set.getRepositories(None, project))
> +
> +    def test_getRepositories_inaccessible(self):
> +        # getRepositories only returns repositories that the given user can
> +        # see.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct()
> +        public_repositories = [
> +            self.factory.makeGitRepository(owner=person, target=project)
> +            for _ in range(3)]
> +        other_person = self.factory.makePerson()
> +        private_repository = self.factory.makeGitRepository(
> +            owner=other_person, target=project,
> +            information_type=InformationType.USERDATA)
> +        self.assertContentEqual(
> +            public_repositories,
> +            self.repository_set.getRepositories(None, project))
> +        self.assertContentEqual(
> +            public_repositories,
> +            self.repository_set.getRepositories(person, project))
> +        self.assertContentEqual(
> +            public_repositories + [private_repository],
> +            self.repository_set.getRepositories(other_person, project))
> +
>      def test_setDefaultRepository_refuses_person(self):
>          # setDefaultRepository refuses if the target is a person.
>          person = self.factory.makePerson()
> 
> === modified file 'lib/lp/registry/model/distributionsourcepackage.py'
> --- lib/lp/registry/model/distributionsourcepackage.py	2015-02-26 11:34:47 +0000
> +++ lib/lp/registry/model/distributionsourcepackage.py	2015-03-04 19:09:38 +0000
> @@ -43,7 +43,6 @@
>      HasBranchesMixin,
>      HasMergeProposalsMixin,
>      )
> -from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
>  from lp.registry.interfaces.distributionsourcepackage import (
>      IDistributionSourcePackage,
>      )
> @@ -120,7 +119,7 @@
>                                  HasBranchesMixin,
>                                  HasCustomLanguageCodesMixin,
>                                  HasMergeProposalsMixin,
> -                                HasDriversMixin, HasGitRepositoriesMixin):
> +                                HasDriversMixin):
>      """This is a "Magic Distribution Source Package". It is not an
>      SQLObject, but instead it represents a source package with a particular
>      name in a particular distribution. You can then ask it all sorts of
> 
> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2015-02-26 14:39:37 +0000
> +++ lib/lp/registry/model/person.py	2015-03-04 19:09:38 +0000
> @@ -146,7 +146,6 @@
>      HasMergeProposalsMixin,
>      HasRequestedReviewsMixin,
>      )
> -from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
>  from lp.registry.enums import (
>      EXCLUSIVE_TEAM_POLICY,
>      INCLUSIVE_TEAM_POLICY,
> @@ -477,7 +476,7 @@
>  class Person(
>      SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
>      HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin,
> -    QuestionsPersonMixin, HasGitRepositoriesMixin):
> +    QuestionsPersonMixin):
>      """A Person."""
>  
>      implements(IPerson, IHasIcon, IHasLogo, IHasMugshot)
> 
> === modified file 'lib/lp/registry/model/product.py'
> --- lib/lp/registry/model/product.py	2015-02-26 14:39:37 +0000
> +++ lib/lp/registry/model/product.py	2015-03-04 19:09:38 +0000
> @@ -124,7 +124,6 @@
>      HasCodeImportsMixin,
>      HasMergeProposalsMixin,
>      )
> -from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
>  from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
>  from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
>  from lp.registry.enums import (
> @@ -362,7 +361,7 @@
>                OfficialBugTagTargetMixin, HasBranchesMixin,
>                HasCustomLanguageCodesMixin, HasMergeProposalsMixin,
>                HasCodeImportsMixin, InformationTypeMixin,
> -              TranslationPolicyMixin, HasGitRepositoriesMixin):
> +              TranslationPolicyMixin):
>      """A Product."""
>  
>      implements(
> 
> === modified file 'lib/lp/registry/tests/test_personmerge.py'
> --- lib/lp/registry/tests/test_personmerge.py	2015-02-23 19:47:01 +0000
> +++ lib/lp/registry/tests/test_personmerge.py	2015-03-04 19:09:38 +0000
> @@ -13,6 +13,7 @@
>  
>  from lp.app.enums import InformationType
>  from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> +from lp.code.interfaces.gitrepository import IGitRepositorySet
>  from lp.registry.interfaces.accesspolicy import (
>      IAccessArtifactGrantSource,
>      IAccessPolicyGrantSource,
> @@ -279,7 +280,8 @@
>          self._do_premerge(repository.owner, person)
>          login_person(person)
>          duplicate, person = self._do_merge(duplicate, person)
> -        repositories = person.getGitRepositories()
> +        repository_set = getUtility(IGitRepositorySet)
> +        repositories = repository_set.getRepositories(None, person)
>          self.assertEqual(1, repositories.count())
>  
>      def test_merge_with_duplicated_git_repositories(self):
> @@ -295,7 +297,9 @@
>          self._do_premerge(duplicate, mergee)
>          login_person(mergee)
>          duplicate, mergee = self._do_merge(duplicate, mergee)
> -        repositories = [r.name for r in mergee.getGitRepositories()]
> +        repository_set = getUtility(IGitRepositorySet)
> +        repositories = [
> +            r.name for r in repository_set.getRepositories(None, mergee)]
>          self.assertEqual(2, len(repositories))
>          self.assertContentEqual([u'foo', u'foo-1'], repositories)
>  
> 
> === modified file 'lib/lp/registry/tests/test_product.py'
> --- lib/lp/registry/tests/test_product.py	2015-02-26 11:34:47 +0000
> +++ lib/lp/registry/tests/test_product.py	2015-03-04 19:09:38 +0000
> @@ -843,11 +843,11 @@
>              'bugtracker', 'canUserAlterAnswerContact', 'codehosting_usage',
>              'coming_sprints', 'commercial_subscription',
>              'commercial_subscription_is_due', 'createBug',
> -            'createCustomLanguageCode', 'createGitRepository',
> -            'custom_language_codes', 'date_next_suggest_packaging',
> -            'datecreated', 'description', 'development_focus',
> -            'development_focusID', 'direct_answer_contacts',
> -            'distrosourcepackages', 'downloadurl', 'driver',
> +            'createCustomLanguageCode', 'custom_language_codes',
> +            'date_next_suggest_packaging', 'datecreated', 'description',
> +            'development_focus', 'development_focusID',
> +            'direct_answer_contacts', 'distrosourcepackages',
> +            'downloadurl', 'driver',
>              'enable_bugfiling_duplicate_search', 'findReferencedOOPS',
>              'findSimilarFAQs', 'findSimilarQuestions', 'freshmeatproject',
>              'getAllowedBugInformationTypes',
> @@ -858,10 +858,10 @@
>              'getCustomLanguageCode', 'getDefaultBugInformationType',
>              'getDefaultSpecificationInformationType',
>              'getEffectiveTranslationPermission', 'getExternalBugTracker',
> -            'getFAQ', 'getFirstEntryToImport', 'getGitRepositories',
> -            'getLinkedBugWatches', 'getMergeProposals', 'getMilestone',
> -            'getMilestonesAndReleases', 'getQuestion', 'getQuestionLanguages',
> -            'getPackage', 'getRelease', 'getSeries', 'getSubscription',
> +            'getFAQ', 'getFirstEntryToImport', 'getLinkedBugWatches',
> +            'getMergeProposals', 'getMilestone', 'getMilestonesAndReleases',
> +            'getQuestion', 'getQuestionLanguages', 'getPackage', 'getRelease',
> +            'getSeries', 'getSubscription',
>              'getSubscriptions', 'getSupportedLanguages', 'getTimeline',
>              'getTopContributors', 'getTopContributorsGroupedByCategory',
>              'getTranslationGroups', 'getTranslationImportQueueEntries',
> 


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