launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/validate-ppa-function into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jml/launchpad/validate-ppa-function/+merge/109490

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?
-- 
https://code.launchpad.net/~jml/launchpad/validate-ppa-function/+merge/109490
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/validate-ppa-function into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2012-06-04 11:41:47 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2012-06-09 16:44:25 +0000
@@ -114,7 +114,7 @@
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.soyuz.interfaces.archive import ArchiveDisabled
-from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.archive import validate_ppa
 
 
 class IRecipesForPerson(Interface):
@@ -830,7 +830,7 @@
                 self.setFieldError(
                     'ppa_name', 'You need to specify a name for the PPA.')
             else:
-                error = Archive.validatePPA(owner, ppa_name)
+                error = validate_ppa(owner, ppa_name)
                 if error is not None:
                     self.setFieldError('ppa_name', error)
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-06-06 21:24:57 +0000
+++ lib/lp/registry/model/person.py	2012-06-09 16:44:25 +0000
@@ -313,7 +313,10 @@
 from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
-from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.archive import (
+    Archive,
+    validate_ppa,
+    )
 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 from lp.translations.model.hastranslationimports import (
@@ -2976,11 +2979,11 @@
                   private=False, suppress_subscription_notifications=False):
         """See `IPerson`."""
         # XXX: We pass through the Person on whom the PPA is being created,
-        # but validatePPA assumes that that Person is also the one creating
+        # but validate_ppa assumes that that Person is also the one creating
         # the PPA.  This is not true in general, and particularly not for
         # teams.  Instead, both the acting user and the target of the PPA
         # creation ought to be passed through.
-        errors = Archive.validatePPA(self, name, private)
+        errors = validate_ppa(self, name, private)
         if errors:
             raise PPACreationError(errors)
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2012-05-25 22:14:21 +0000
+++ lib/lp/soyuz/browser/archive.py	2012-06-09 16:44:25 +0000
@@ -165,7 +165,10 @@
     inactive_publishing_status,
     IPublishingSet,
     )
-from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.archive import (
+    Archive,
+    validate_ppa,
+    )
 from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.publishing import (
     BinaryPackagePublishingHistory,
@@ -1973,7 +1976,7 @@
                 'The default PPA is already activated. Please specify a '
                 'name for the new PPA and resubmit the form.')
 
-        errors = Archive.validatePPA(self.context, proposed_name)
+        errors = validate_ppa(self.context, proposed_name)
         if errors is not None:
             self.addError(errors)
 

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2012-06-07 20:35:53 +0000
+++ lib/lp/soyuz/configure.zcml	2012-06-09 16:44:25 +0000
@@ -408,7 +408,7 @@
            NOTE: The 'private' permission controls who can turn a public
            archive into a private one, and vice versa. The logic that says this
            requires launchpad.Commercial permissions is duplicated in
-           IArchive.validatePPA.
+           validate_ppa.
           -->
         <require
             permission="launchpad.Commercial"

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-06-06 21:24:57 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-06-09 16:44:25 +0000
@@ -882,13 +882,6 @@
     def getPackageDownloadTotal(bpr):
         """Get the total download count for a given package."""
 
-    def validatePPA(person, proposed_name):
-        """Check if a proposed name for a PPA is valid.
-
-        :param person: A Person identifying the requestor.
-        :param proposed_name: A String identifying the proposed PPA name.
-        """
-
     def getPockets():
         """Return iterable containing valid pocket names for this archive."""
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-06-06 21:24:57 +0000
+++ lib/lp/soyuz/model/archive.py	2012-06-09 16:44:25 +0000
@@ -10,6 +10,7 @@
 __all__ = [
     'Archive',
     'ArchiveSet',
+    'validate_ppa',
     ]
 
 from operator import attrgetter
@@ -2022,37 +2023,6 @@
         restricted.add(family)
         self.enabled_restricted_families = restricted
 
-    @classmethod
-    def validatePPA(self, person, proposed_name, private=False):
-        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        if private:
-            # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
-            # which says that one needs 'launchpad.Commercial' permission to
-            # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
-            # which determines who is granted launchpad.Commercial
-            # permissions.
-            role = IPersonRoles(person)
-            if not (role.in_admin or role.in_commercial_admin):
-                return '%s is not allowed to make private PPAs' % person.name
-        if person.is_team and (
-            person.subscriptionpolicy in OPEN_TEAM_POLICY):
-            return "Open teams cannot have PPAs."
-        if proposed_name is not None and proposed_name == ubuntu.name:
-            return (
-                "A PPA cannot have the same name as its distribution.")
-        if proposed_name is None:
-            proposed_name = 'ppa'
-        try:
-            person.getPPAByName(proposed_name)
-        except NoSuchPPA:
-            return None
-        else:
-            text = "You already have a PPA named '%s'." % proposed_name
-            if person.is_team:
-                text = "%s already has a PPA named '%s'." % (
-                    person.displayname, proposed_name)
-            return text
-
     def getPockets(self):
         """See `IArchive`."""
         if self.is_ppa:
@@ -2084,6 +2054,37 @@
         job.destroySelf()
 
 
+def validate_ppa(person, proposed_name, private=False):
+    ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+    if private:
+        # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
+        # which says that one needs 'launchpad.Commercial' permission to
+        # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
+        # which determines who is granted launchpad.Commercial
+        # permissions.
+        role = IPersonRoles(person)
+        if not (role.in_admin or role.in_commercial_admin):
+            return '%s is not allowed to make private PPAs' % person.name
+    if person.is_team and (
+        person.subscriptionpolicy in OPEN_TEAM_POLICY):
+        return "Open teams cannot have PPAs."
+    if proposed_name is not None and proposed_name == ubuntu.name:
+        return (
+            "A PPA cannot have the same name as its distribution.")
+    if proposed_name is None:
+        proposed_name = 'ppa'
+    try:
+        person.getPPAByName(proposed_name)
+    except NoSuchPPA:
+        return None
+    else:
+        text = "You already have a PPA named '%s'." % proposed_name
+        if person.is_team:
+            text = "%s already has a PPA named '%s'." % (
+                person.displayname, proposed_name)
+        return text
+
+
 class ArchiveSet:
     implements(IArchiveSet)
     title = "Archives registered in Launchpad"

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-06 21:24:57 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-06-09 16:44:25 +0000
@@ -72,7 +72,7 @@
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
-from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.archive import validate_ppa
 from lp.soyuz.model.archivepermission import (
     ArchivePermission,
     ArchivePermissionSet,
@@ -1655,21 +1655,21 @@
 
     def test_open_teams(self):
         team = self.factory.makeTeam()
-        self.assertEqual('Open teams cannot have PPAs.',
-            Archive.validatePPA(team, None))
+        self.assertEqual(
+            'Open teams cannot have PPAs.', validate_ppa(team, None))
 
     def test_distribution_name(self):
         ppa_owner = self.factory.makePerson()
         self.assertEqual(
             'A PPA cannot have the same name as its distribution.',
-            Archive.validatePPA(ppa_owner, 'ubuntu'))
+            validate_ppa(ppa_owner, 'ubuntu'))
 
     def test_private_ppa_non_commercial_admin(self):
         ppa_owner = self.factory.makePerson()
         self.assertEqual(
             '%s is not allowed to make private PPAs' % (ppa_owner.name,),
-            Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
-                                private=True))
+            validate_ppa(
+                ppa_owner, self.factory.getUniqueString(), private=True))
 
     def test_private_ppa_commercial_admin(self):
         ppa_owner = self.factory.makePerson()
@@ -1677,30 +1677,31 @@
             comm = getUtility(ILaunchpadCelebrities).commercial_admin
             comm.addMember(ppa_owner, comm.teamowner)
         self.assertIsNone(
-            Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
-                                private=True))
+            validate_ppa(
+                ppa_owner, self.factory.getUniqueString(), private=True))
 
     def test_private_ppa_admin(self):
         ppa_owner = self.factory.makeAdministrator()
         self.assertIsNone(
-            Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
-                                private=True))
+            validate_ppa(
+                ppa_owner, self.factory.getUniqueString(), private=True))
 
     def test_two_ppas(self):
         ppa = self.factory.makeArchive(name='ppa')
-        self.assertEqual("You already have a PPA named 'ppa'.",
-            Archive.validatePPA(ppa.owner, 'ppa'))
+        self.assertEqual(
+            "You already have a PPA named 'ppa'.", validate_ppa(ppa.owner, 'ppa'))
 
     def test_two_ppas_with_team(self):
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
         self.factory.makeArchive(owner=team, name='ppa')
-        self.assertEqual("%s already has a PPA named 'ppa'." % (
-            team.displayname), Archive.validatePPA(team, 'ppa'))
+        self.assertEqual(
+            "%s already has a PPA named 'ppa'." % team.displayname,
+            validate_ppa(team, 'ppa'))
 
     def test_valid_ppa(self):
         ppa_owner = self.factory.makePerson()
-        self.assertIsNone(Archive.validatePPA(ppa_owner, None))
+        self.assertIsNone(validate_ppa(ppa_owner, None))
 
 
 class TestGetComponentsForSeries(TestCaseWithFactory):