launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-simplify-hasgitrepositories into lp:launchpad.

Commit message:
Add IGitRepositorySet.getRepositories(user, target), and reduce IHasGitRepositories to a marker interface.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1032731 in Launchpad itself: "Support for Launchpad-hosted Git repositories"
  https://bugs.launchpad.net/launchpad/+bug/1032731

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-simplify-hasgitrepositories/+merge/251809

Add IGitRepositorySet.getRepositories(user, target), and reduce IHasGitRepositories to a marker interface.

This fits better with the pattern we're using elsewhere of not putting domain-specific methods on registry classes.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-simplify-hasgitrepositories into lp:launchpad.

=== 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():
         """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',