Launchpad itself

Merge lp:~jml/launchpad/validate-ppa-function into lp:launchpad

  1. validate-ppa-function
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 15387
Proposed branch: lp:~jml/launchpad/validate-ppa-function
Merge into: lp:launchpad
Diff against target: 276 lines (+62/-61)
7 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+2/-2)
lib/lp/registry/model/person.py (+6/-3)
lib/lp/soyuz/browser/archive.py (+5/-2)
lib/lp/soyuz/configure.zcml (+1/-1)
lib/lp/soyuz/interfaces/archive.py (+0/-7)
lib/lp/soyuz/model/archive.py (+32/-31)
lib/lp/soyuz/tests/test_archive.py (+16/-15)
To merge this branch: bzr merge lp:~jml/launchpad/validate-ppa-function
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+109490@code.launchpad.net

Commit message

Move Archive.validatePPA to be a function, validate_ppa.

Description of the change

Moves Archive.validatePPA to be a function, validate_ppa. Costs 1 line of code. I'm confident I'll earn that one back.

The reason is that it doesn't need to be on the class. It's got nothing to do with it, so why not make it a function?

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you.

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/browser/sourcepackagerecipe.py'
2--- lib/lp/code/browser/sourcepackagerecipe.py 2012-06-04 11:41:47 +0000
3+++ lib/lp/code/browser/sourcepackagerecipe.py 2012-06-09 16:46:20 +0000
4@@ -114,7 +114,7 @@
5 from lp.services.webapp.authorization import check_permission
6 from lp.services.webapp.breadcrumb import Breadcrumb
7 from lp.soyuz.interfaces.archive import ArchiveDisabled
8-from lp.soyuz.model.archive import Archive
9+from lp.soyuz.model.archive import validate_ppa
10
11
12 class IRecipesForPerson(Interface):
13@@ -830,7 +830,7 @@
14 self.setFieldError(
15 'ppa_name', 'You need to specify a name for the PPA.')
16 else:
17- error = Archive.validatePPA(owner, ppa_name)
18+ error = validate_ppa(owner, ppa_name)
19 if error is not None:
20 self.setFieldError('ppa_name', error)
21
22
23=== modified file 'lib/lp/registry/model/person.py'
24--- lib/lp/registry/model/person.py 2012-06-06 21:24:57 +0000
25+++ lib/lp/registry/model/person.py 2012-06-09 16:46:20 +0000
26@@ -313,7 +313,10 @@
27 from lp.soyuz.interfaces.archive import IArchiveSet
28 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
29 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
30-from lp.soyuz.model.archive import Archive
31+from lp.soyuz.model.archive import (
32+ Archive,
33+ validate_ppa,
34+ )
35 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
36 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
37 from lp.translations.model.hastranslationimports import (
38@@ -2976,11 +2979,11 @@
39 private=False, suppress_subscription_notifications=False):
40 """See `IPerson`."""
41 # XXX: We pass through the Person on whom the PPA is being created,
42- # but validatePPA assumes that that Person is also the one creating
43+ # but validate_ppa assumes that that Person is also the one creating
44 # the PPA. This is not true in general, and particularly not for
45 # teams. Instead, both the acting user and the target of the PPA
46 # creation ought to be passed through.
47- errors = Archive.validatePPA(self, name, private)
48+ errors = validate_ppa(self, name, private)
49 if errors:
50 raise PPACreationError(errors)
51 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
52
53=== modified file 'lib/lp/soyuz/browser/archive.py'
54--- lib/lp/soyuz/browser/archive.py 2012-05-25 22:14:21 +0000
55+++ lib/lp/soyuz/browser/archive.py 2012-06-09 16:46:20 +0000
56@@ -165,7 +165,10 @@
57 inactive_publishing_status,
58 IPublishingSet,
59 )
60-from lp.soyuz.model.archive import Archive
61+from lp.soyuz.model.archive import (
62+ Archive,
63+ validate_ppa,
64+ )
65 from lp.soyuz.model.binarypackagename import BinaryPackageName
66 from lp.soyuz.model.publishing import (
67 BinaryPackagePublishingHistory,
68@@ -1973,7 +1976,7 @@
69 'The default PPA is already activated. Please specify a '
70 'name for the new PPA and resubmit the form.')
71
72- errors = Archive.validatePPA(self.context, proposed_name)
73+ errors = validate_ppa(self.context, proposed_name)
74 if errors is not None:
75 self.addError(errors)
76
77
78=== modified file 'lib/lp/soyuz/configure.zcml'
79--- lib/lp/soyuz/configure.zcml 2012-06-07 20:35:53 +0000
80+++ lib/lp/soyuz/configure.zcml 2012-06-09 16:46:20 +0000
81@@ -408,7 +408,7 @@
82 NOTE: The 'private' permission controls who can turn a public
83 archive into a private one, and vice versa. The logic that says this
84 requires launchpad.Commercial permissions is duplicated in
85- IArchive.validatePPA.
86+ validate_ppa.
87 -->
88 <require
89 permission="launchpad.Commercial"
90
91=== modified file 'lib/lp/soyuz/interfaces/archive.py'
92--- lib/lp/soyuz/interfaces/archive.py 2012-06-06 21:24:57 +0000
93+++ lib/lp/soyuz/interfaces/archive.py 2012-06-09 16:46:20 +0000
94@@ -882,13 +882,6 @@
95 def getPackageDownloadTotal(bpr):
96 """Get the total download count for a given package."""
97
98- def validatePPA(person, proposed_name):
99- """Check if a proposed name for a PPA is valid.
100-
101- :param person: A Person identifying the requestor.
102- :param proposed_name: A String identifying the proposed PPA name.
103- """
104-
105 def getPockets():
106 """Return iterable containing valid pocket names for this archive."""
107
108
109=== modified file 'lib/lp/soyuz/model/archive.py'
110--- lib/lp/soyuz/model/archive.py 2012-06-06 21:24:57 +0000
111+++ lib/lp/soyuz/model/archive.py 2012-06-09 16:46:20 +0000
112@@ -10,6 +10,7 @@
113 __all__ = [
114 'Archive',
115 'ArchiveSet',
116+ 'validate_ppa',
117 ]
118
119 from operator import attrgetter
120@@ -2022,37 +2023,6 @@
121 restricted.add(family)
122 self.enabled_restricted_families = restricted
123
124- @classmethod
125- def validatePPA(self, person, proposed_name, private=False):
126- ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
127- if private:
128- # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
129- # which says that one needs 'launchpad.Commercial' permission to
130- # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
131- # which determines who is granted launchpad.Commercial
132- # permissions.
133- role = IPersonRoles(person)
134- if not (role.in_admin or role.in_commercial_admin):
135- return '%s is not allowed to make private PPAs' % person.name
136- if person.is_team and (
137- person.subscriptionpolicy in OPEN_TEAM_POLICY):
138- return "Open teams cannot have PPAs."
139- if proposed_name is not None and proposed_name == ubuntu.name:
140- return (
141- "A PPA cannot have the same name as its distribution.")
142- if proposed_name is None:
143- proposed_name = 'ppa'
144- try:
145- person.getPPAByName(proposed_name)
146- except NoSuchPPA:
147- return None
148- else:
149- text = "You already have a PPA named '%s'." % proposed_name
150- if person.is_team:
151- text = "%s already has a PPA named '%s'." % (
152- person.displayname, proposed_name)
153- return text
154-
155 def getPockets(self):
156 """See `IArchive`."""
157 if self.is_ppa:
158@@ -2084,6 +2054,37 @@
159 job.destroySelf()
160
161
162+def validate_ppa(person, proposed_name, private=False):
163+ ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
164+ if private:
165+ # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
166+ # which says that one needs 'launchpad.Commercial' permission to
167+ # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
168+ # which determines who is granted launchpad.Commercial
169+ # permissions.
170+ role = IPersonRoles(person)
171+ if not (role.in_admin or role.in_commercial_admin):
172+ return '%s is not allowed to make private PPAs' % person.name
173+ if person.is_team and (
174+ person.subscriptionpolicy in OPEN_TEAM_POLICY):
175+ return "Open teams cannot have PPAs."
176+ if proposed_name is not None and proposed_name == ubuntu.name:
177+ return (
178+ "A PPA cannot have the same name as its distribution.")
179+ if proposed_name is None:
180+ proposed_name = 'ppa'
181+ try:
182+ person.getPPAByName(proposed_name)
183+ except NoSuchPPA:
184+ return None
185+ else:
186+ text = "You already have a PPA named '%s'." % proposed_name
187+ if person.is_team:
188+ text = "%s already has a PPA named '%s'." % (
189+ person.displayname, proposed_name)
190+ return text
191+
192+
193 class ArchiveSet:
194 implements(IArchiveSet)
195 title = "Archives registered in Launchpad"
196
197=== modified file 'lib/lp/soyuz/tests/test_archive.py'
198--- lib/lp/soyuz/tests/test_archive.py 2012-06-06 21:24:57 +0000
199+++ lib/lp/soyuz/tests/test_archive.py 2012-06-09 16:46:20 +0000
200@@ -72,7 +72,7 @@
201 from lp.soyuz.interfaces.component import IComponentSet
202 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
203 from lp.soyuz.interfaces.processor import IProcessorFamilySet
204-from lp.soyuz.model.archive import Archive
205+from lp.soyuz.model.archive import validate_ppa
206 from lp.soyuz.model.archivepermission import (
207 ArchivePermission,
208 ArchivePermissionSet,
209@@ -1655,21 +1655,21 @@
210
211 def test_open_teams(self):
212 team = self.factory.makeTeam()
213- self.assertEqual('Open teams cannot have PPAs.',
214- Archive.validatePPA(team, None))
215+ self.assertEqual(
216+ 'Open teams cannot have PPAs.', validate_ppa(team, None))
217
218 def test_distribution_name(self):
219 ppa_owner = self.factory.makePerson()
220 self.assertEqual(
221 'A PPA cannot have the same name as its distribution.',
222- Archive.validatePPA(ppa_owner, 'ubuntu'))
223+ validate_ppa(ppa_owner, 'ubuntu'))
224
225 def test_private_ppa_non_commercial_admin(self):
226 ppa_owner = self.factory.makePerson()
227 self.assertEqual(
228 '%s is not allowed to make private PPAs' % (ppa_owner.name,),
229- Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
230- private=True))
231+ validate_ppa(
232+ ppa_owner, self.factory.getUniqueString(), private=True))
233
234 def test_private_ppa_commercial_admin(self):
235 ppa_owner = self.factory.makePerson()
236@@ -1677,30 +1677,31 @@
237 comm = getUtility(ILaunchpadCelebrities).commercial_admin
238 comm.addMember(ppa_owner, comm.teamowner)
239 self.assertIsNone(
240- Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
241- private=True))
242+ validate_ppa(
243+ ppa_owner, self.factory.getUniqueString(), private=True))
244
245 def test_private_ppa_admin(self):
246 ppa_owner = self.factory.makeAdministrator()
247 self.assertIsNone(
248- Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
249- private=True))
250+ validate_ppa(
251+ ppa_owner, self.factory.getUniqueString(), private=True))
252
253 def test_two_ppas(self):
254 ppa = self.factory.makeArchive(name='ppa')
255- self.assertEqual("You already have a PPA named 'ppa'.",
256- Archive.validatePPA(ppa.owner, 'ppa'))
257+ self.assertEqual(
258+ "You already have a PPA named 'ppa'.", validate_ppa(ppa.owner, 'ppa'))
259
260 def test_two_ppas_with_team(self):
261 team = self.factory.makeTeam(
262 subscription_policy=TeamSubscriptionPolicy.MODERATED)
263 self.factory.makeArchive(owner=team, name='ppa')
264- self.assertEqual("%s already has a PPA named 'ppa'." % (
265- team.displayname), Archive.validatePPA(team, 'ppa'))
266+ self.assertEqual(
267+ "%s already has a PPA named 'ppa'." % team.displayname,
268+ validate_ppa(team, 'ppa'))
269
270 def test_valid_ppa(self):
271 ppa_owner = self.factory.makePerson()
272- self.assertIsNone(Archive.validatePPA(ppa_owner, None))
273+ self.assertIsNone(validate_ppa(ppa_owner, None))
274
275
276 class TestGetComponentsForSeries(TestCaseWithFactory):