Merge lp:~cjwatson/launchpad/git-simplify-hasgitrepositories into lp:launchpad

Proposed by Colin Watson on 2015-03-04
Status: Merged
Approved by: Colin Watson on 2015-03-05
Approved revision: no longer in the source branch.
Merged at revision: 17378
Proposed branch: lp:~cjwatson/launchpad/git-simplify-hasgitrepositories
Merge into: lp:launchpad
Diff against target: 301 lines (+70/-68)
10 files modified
lib/lp/code/interfaces/gitrepository.py (+11/-1)
lib/lp/code/interfaces/hasgitrepositories.py (+0/-20)
lib/lp/code/model/gitrepository.py (+10/-2)
lib/lp/code/model/hasgitrepositories.py (+0/-28)
lib/lp/code/model/tests/test_gitrepository.py (+31/-0)
lib/lp/registry/model/distributionsourcepackage.py (+1/-2)
lib/lp/registry/model/person.py (+1/-2)
lib/lp/registry/model/product.py (+1/-2)
lib/lp/registry/tests/test_personmerge.py (+6/-2)
lib/lp/registry/tests/test_product.py (+9/-9)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-simplify-hasgitrepositories
Reviewer Review Type Date Requested Status
William Grant code 2015-03-04 Approve on 2015-03-05
Review via email: mp+251809@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) wrote :

Death to domain-specific pollution of Person and Product.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/interfaces/gitrepository.py'
2--- lib/lp/code/interfaces/gitrepository.py 2015-03-03 15:05:59 +0000
3+++ lib/lp/code/interfaces/gitrepository.py 2015-03-05 13:17:02 +0000
4@@ -320,6 +320,16 @@
5 Return None if no match was found.
6 """
7
8+ def getRepositories(user, target):
9+ """Get all repositories for a target.
10+
11+ :param user: An `IPerson`. Only repositories visible by this user
12+ will be returned.
13+ :param target: An `IHasGitRepositories`.
14+
15+ :return: A collection of `IGitRepository` objects.
16+ """
17+
18 def getDefaultRepository(target):
19 """Get the default repository for a target.
20
21@@ -360,7 +370,7 @@
22 :raises GitTargetError: if `target` is an `IPerson`.
23 """
24
25- def getRepositories():
26+ def empty_list():
27 """Return an empty collection of repositories.
28
29 This only exists to keep lazr.restful happy.
30
31=== modified file 'lib/lp/code/interfaces/hasgitrepositories.py'
32--- lib/lp/code/interfaces/hasgitrepositories.py 2015-02-09 16:49:01 +0000
33+++ lib/lp/code/interfaces/hasgitrepositories.py 2015-03-05 13:17:02 +0000
34@@ -18,23 +18,3 @@
35 A project contains Git repositories, a source package on a distribution
36 contains branches, and a person contains "personal" branches.
37 """
38-
39- def getGitRepositories(visible_by_user=None, eager_load=False):
40- """Returns all Git repositories related to this object.
41-
42- :param visible_by_user: Normally the user who is asking.
43- :param eager_load: If True, load related objects for the whole
44- collection.
45- :returns: A list of `IGitRepository` objects.
46- """
47-
48- def createGitRepository(registrant, owner, name, information_type=None):
49- """Create a Git repository for this target and return it.
50-
51- :param registrant: The `IPerson` who registered the new repository.
52- :param owner: The `IPerson` who owns the new repository.
53- :param name: The repository name.
54- :param information_type: Set the repository's information type to
55- one different from the target's default. The type must conform
56- to the target's code sharing policy. (optional)
57- """
58
59=== modified file 'lib/lp/code/model/gitrepository.py'
60--- lib/lp/code/model/gitrepository.py 2015-03-03 15:05:59 +0000
61+++ lib/lp/code/model/gitrepository.py 2015-03-05 13:17:02 +0000
62@@ -40,7 +40,10 @@
63 GitDefaultConflict,
64 GitTargetError,
65 )
66-from lp.code.interfaces.gitcollection import IAllGitRepositories
67+from lp.code.interfaces.gitcollection import (
68+ IAllGitRepositories,
69+ IGitCollection,
70+ )
71 from lp.code.interfaces.gitlookup import IGitLookup
72 from lp.code.interfaces.gitnamespace import (
73 get_git_namespace,
74@@ -368,6 +371,11 @@
75 return repository
76 return None
77
78+ def getRepositories(self, user, target):
79+ """See `IGitRepositorySet`."""
80+ collection = IGitCollection(target).visibleByUser(user)
81+ return collection.getRepositories(eager_load=True)
82+
83 def getDefaultRepository(self, target):
84 """See `IGitRepositorySet`."""
85 clauses = [GitRepository.target_default == True]
86@@ -439,7 +447,7 @@
87 if previous is not None:
88 previous.setOwnerDefault(False)
89
90- def getRepositories(self):
91+ def empty_list(self):
92 """See `IGitRepositorySet`."""
93 return []
94
95
96=== removed file 'lib/lp/code/model/hasgitrepositories.py'
97--- lib/lp/code/model/hasgitrepositories.py 2015-02-23 17:59:37 +0000
98+++ lib/lp/code/model/hasgitrepositories.py 1970-01-01 00:00:00 +0000
99@@ -1,28 +0,0 @@
100-# Copyright 2015 Canonical Ltd. This software is licensed under the
101-# GNU Affero General Public License version 3 (see the file LICENSE).
102-
103-__metaclass__ = type
104-__all__ = [
105- 'HasGitRepositoriesMixin',
106- ]
107-
108-from zope.component import getUtility
109-
110-from lp.code.interfaces.gitcollection import IGitCollection
111-from lp.code.interfaces.gitrepository import IGitRepositorySet
112-
113-
114-class HasGitRepositoriesMixin:
115- """A mixin implementation for `IHasGitRepositories`."""
116-
117- def createGitRepository(self, registrant, owner, name,
118- information_type=None):
119- """See `IHasGitRepositories`."""
120- return getUtility(IGitRepositorySet).new(
121- registrant, owner, self, name,
122- information_type=information_type)
123-
124- def getGitRepositories(self, visible_by_user=None, eager_load=False):
125- """See `IHasGitRepositories`."""
126- collection = IGitCollection(self).visibleByUser(visible_by_user)
127- return collection.getRepositories(eager_load=eager_load)
128
129=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
130--- lib/lp/code/model/tests/test_gitrepository.py 2015-02-26 17:32:56 +0000
131+++ lib/lp/code/model/tests/test_gitrepository.py 2015-03-05 13:17:02 +0000
132@@ -660,6 +660,37 @@
133 self.assertIsNone(
134 self.repository_set.getByPath(self.factory.makePerson(), path))
135
136+ def test_getRepositories(self):
137+ # getRepositories returns a collection of repositories for the given
138+ # target.
139+ project = self.factory.makeProduct()
140+ repositories = [
141+ self.factory.makeGitRepository(target=project) for _ in range(5)]
142+ self.assertContentEqual(
143+ repositories, self.repository_set.getRepositories(None, project))
144+
145+ def test_getRepositories_inaccessible(self):
146+ # getRepositories only returns repositories that the given user can
147+ # see.
148+ person = self.factory.makePerson()
149+ project = self.factory.makeProduct()
150+ public_repositories = [
151+ self.factory.makeGitRepository(owner=person, target=project)
152+ for _ in range(3)]
153+ other_person = self.factory.makePerson()
154+ private_repository = self.factory.makeGitRepository(
155+ owner=other_person, target=project,
156+ information_type=InformationType.USERDATA)
157+ self.assertContentEqual(
158+ public_repositories,
159+ self.repository_set.getRepositories(None, project))
160+ self.assertContentEqual(
161+ public_repositories,
162+ self.repository_set.getRepositories(person, project))
163+ self.assertContentEqual(
164+ public_repositories + [private_repository],
165+ self.repository_set.getRepositories(other_person, project))
166+
167 def test_setDefaultRepository_refuses_person(self):
168 # setDefaultRepository refuses if the target is a person.
169 person = self.factory.makePerson()
170
171=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
172--- lib/lp/registry/model/distributionsourcepackage.py 2015-02-26 11:34:47 +0000
173+++ lib/lp/registry/model/distributionsourcepackage.py 2015-03-05 13:17:02 +0000
174@@ -43,7 +43,6 @@
175 HasBranchesMixin,
176 HasMergeProposalsMixin,
177 )
178-from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
179 from lp.registry.interfaces.distributionsourcepackage import (
180 IDistributionSourcePackage,
181 )
182@@ -120,7 +119,7 @@
183 HasBranchesMixin,
184 HasCustomLanguageCodesMixin,
185 HasMergeProposalsMixin,
186- HasDriversMixin, HasGitRepositoriesMixin):
187+ HasDriversMixin):
188 """This is a "Magic Distribution Source Package". It is not an
189 SQLObject, but instead it represents a source package with a particular
190 name in a particular distribution. You can then ask it all sorts of
191
192=== modified file 'lib/lp/registry/model/person.py'
193--- lib/lp/registry/model/person.py 2015-02-26 14:39:37 +0000
194+++ lib/lp/registry/model/person.py 2015-03-05 13:17:02 +0000
195@@ -146,7 +146,6 @@
196 HasMergeProposalsMixin,
197 HasRequestedReviewsMixin,
198 )
199-from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
200 from lp.registry.enums import (
201 EXCLUSIVE_TEAM_POLICY,
202 INCLUSIVE_TEAM_POLICY,
203@@ -477,7 +476,7 @@
204 class Person(
205 SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
206 HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin,
207- QuestionsPersonMixin, HasGitRepositoriesMixin):
208+ QuestionsPersonMixin):
209 """A Person."""
210
211 implements(IPerson, IHasIcon, IHasLogo, IHasMugshot)
212
213=== modified file 'lib/lp/registry/model/product.py'
214--- lib/lp/registry/model/product.py 2015-02-26 14:39:37 +0000
215+++ lib/lp/registry/model/product.py 2015-03-05 13:17:02 +0000
216@@ -124,7 +124,6 @@
217 HasCodeImportsMixin,
218 HasMergeProposalsMixin,
219 )
220-from lp.code.model.hasgitrepositories import HasGitRepositoriesMixin
221 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
222 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
223 from lp.registry.enums import (
224@@ -362,7 +361,7 @@
225 OfficialBugTagTargetMixin, HasBranchesMixin,
226 HasCustomLanguageCodesMixin, HasMergeProposalsMixin,
227 HasCodeImportsMixin, InformationTypeMixin,
228- TranslationPolicyMixin, HasGitRepositoriesMixin):
229+ TranslationPolicyMixin):
230 """A Product."""
231
232 implements(
233
234=== modified file 'lib/lp/registry/tests/test_personmerge.py'
235--- lib/lp/registry/tests/test_personmerge.py 2015-02-23 19:47:01 +0000
236+++ lib/lp/registry/tests/test_personmerge.py 2015-03-05 13:17:02 +0000
237@@ -13,6 +13,7 @@
238
239 from lp.app.enums import InformationType
240 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
241+from lp.code.interfaces.gitrepository import IGitRepositorySet
242 from lp.registry.interfaces.accesspolicy import (
243 IAccessArtifactGrantSource,
244 IAccessPolicyGrantSource,
245@@ -279,7 +280,8 @@
246 self._do_premerge(repository.owner, person)
247 login_person(person)
248 duplicate, person = self._do_merge(duplicate, person)
249- repositories = person.getGitRepositories()
250+ repository_set = getUtility(IGitRepositorySet)
251+ repositories = repository_set.getRepositories(None, person)
252 self.assertEqual(1, repositories.count())
253
254 def test_merge_with_duplicated_git_repositories(self):
255@@ -295,7 +297,9 @@
256 self._do_premerge(duplicate, mergee)
257 login_person(mergee)
258 duplicate, mergee = self._do_merge(duplicate, mergee)
259- repositories = [r.name for r in mergee.getGitRepositories()]
260+ repository_set = getUtility(IGitRepositorySet)
261+ repositories = [
262+ r.name for r in repository_set.getRepositories(None, mergee)]
263 self.assertEqual(2, len(repositories))
264 self.assertContentEqual([u'foo', u'foo-1'], repositories)
265
266
267=== modified file 'lib/lp/registry/tests/test_product.py'
268--- lib/lp/registry/tests/test_product.py 2015-02-26 11:34:47 +0000
269+++ lib/lp/registry/tests/test_product.py 2015-03-05 13:17:02 +0000
270@@ -843,11 +843,11 @@
271 'bugtracker', 'canUserAlterAnswerContact', 'codehosting_usage',
272 'coming_sprints', 'commercial_subscription',
273 'commercial_subscription_is_due', 'createBug',
274- 'createCustomLanguageCode', 'createGitRepository',
275- 'custom_language_codes', 'date_next_suggest_packaging',
276- 'datecreated', 'description', 'development_focus',
277- 'development_focusID', 'direct_answer_contacts',
278- 'distrosourcepackages', 'downloadurl', 'driver',
279+ 'createCustomLanguageCode', 'custom_language_codes',
280+ 'date_next_suggest_packaging', 'datecreated', 'description',
281+ 'development_focus', 'development_focusID',
282+ 'direct_answer_contacts', 'distrosourcepackages',
283+ 'downloadurl', 'driver',
284 'enable_bugfiling_duplicate_search', 'findReferencedOOPS',
285 'findSimilarFAQs', 'findSimilarQuestions', 'freshmeatproject',
286 'getAllowedBugInformationTypes',
287@@ -858,10 +858,10 @@
288 'getCustomLanguageCode', 'getDefaultBugInformationType',
289 'getDefaultSpecificationInformationType',
290 'getEffectiveTranslationPermission', 'getExternalBugTracker',
291- 'getFAQ', 'getFirstEntryToImport', 'getGitRepositories',
292- 'getLinkedBugWatches', 'getMergeProposals', 'getMilestone',
293- 'getMilestonesAndReleases', 'getQuestion', 'getQuestionLanguages',
294- 'getPackage', 'getRelease', 'getSeries', 'getSubscription',
295+ 'getFAQ', 'getFirstEntryToImport', 'getLinkedBugWatches',
296+ 'getMergeProposals', 'getMilestone', 'getMilestonesAndReleases',
297+ 'getQuestion', 'getQuestionLanguages', 'getPackage', 'getRelease',
298+ 'getSeries', 'getSubscription',
299 'getSubscriptions', 'getSupportedLanguages', 'getTimeline',
300 'getTopContributors', 'getTopContributorsGroupedByCategory',
301 'getTranslationGroups', 'getTranslationImportQueueEntries',