launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/distrofy-misc into lp:launchpad.

Commit message:
ArchivePermissionSet's packageset methods no longer interpret names as Ubuntu series; it needs DistroSeries objects. And APS.isSourceUploadAllowed also no longer defaults to ubuntu.currentseries.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/distrofy-misc/+merge/225660

ArchivePermissionSet's packageset methods no longer interpret names as Ubuntu series; it needs DistroSeries objects. And APS.isSourceUploadAllowed also no longer defaults to ubuntu.currentseries.
-- 
https://code.launchpad.net/~wgrant/launchpad/distrofy-misc/+merge/225660
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/distrofy-misc into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/tests/nascentupload-packageset.txt'
--- lib/lp/archiveuploader/tests/nascentupload-packageset.txt	2012-01-20 15:42:44 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-packageset.txt	2014-07-04 13:09:27 +0000
@@ -142,7 +142,8 @@
 
 name16 has no package set based upload privileges for 'bar' yet.
 
-    >>> ap_set.isSourceUploadAllowed(ubuntu_archive, 'bar', name16)
+    >>> ap_set.isSourceUploadAllowed(
+    ...     ubuntu_archive, 'bar', name16, ubuntu.currentseries)
     False
 
 Now we define a permission for name16 to upload to the 'foo' package set.
@@ -156,7 +157,8 @@
 
 And, voila, name16 has a package set based upload authorization for 'bar'.
 
-    >>> ap_set.isSourceUploadAllowed(ubuntu_archive, 'bar', name16)
+    >>> ap_set.isSourceUploadAllowed(
+    ...     ubuntu_archive, 'bar', name16, ubuntu.currentseries)
     True
 
 With the authorization above the upload should work again.

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2014-04-24 02:12:52 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2014-07-04 13:09:27 +0000
@@ -1024,7 +1024,7 @@
             package sets he has access to.
         :param distroseries: The `IDistroSeries` for which to check
             permissions. If none is supplied then `currentseries` in
-            Ubuntu is assumed.
+            the archive's distribution is assumed.
 
         :raises NoSuchSourcePackageName: if a source package with the
             given name could not be found.

=== modified file 'lib/lp/soyuz/interfaces/archivepermission.py'
--- lib/lp/soyuz/interfaces/archivepermission.py	2013-01-07 02:40:55 +0000
+++ lib/lp/soyuz/interfaces/archivepermission.py	2014-07-04 13:09:27 +0000
@@ -284,8 +284,8 @@
             archive in question.
         """
 
-    def isSourceUploadAllowed(
-        archive, sourcepackagename, person, distroseries=None):
+    def isSourceUploadAllowed(archive, sourcepackagename, person,
+                              distroseries):
         """True if the person is allowed to upload the given source package.
 
         Return True if there exists a permission that combines
@@ -303,8 +303,7 @@
         :param person: An `IPerson` for whom you want to find out which
             package sets he has access to.
         :param distroseries: The `IDistroSeries` for which to check
-            permissions. If none is supplied then `currentseries` in
-            Ubuntu is assumed.
+            permissions.
 
         :raises SourceNotFound: if a source package with the given
             name could not be found.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2014-06-30 13:51:15 +0000
+++ lib/lp/soyuz/model/archive.py	2014-07-04 13:09:27 +0000
@@ -1535,6 +1535,8 @@
     def isSourceUploadAllowed(
         self, sourcepackagename, person, distroseries=None):
         """See `IArchive`."""
+        if distroseries is None:
+            distroseries = self.distribution.currentseries
         permission_set = getUtility(IArchivePermissionSet)
         return permission_set.isSourceUploadAllowed(
             self, sourcepackagename, person, distroseries)

=== modified file 'lib/lp/soyuz/model/archivepermission.py'
--- lib/lp/soyuz/model/archivepermission.py	2013-06-20 05:50:00 +0000
+++ lib/lp/soyuz/model/archivepermission.py	2014-07-04 13:09:27 +0000
@@ -31,7 +31,6 @@
 from zope.security.proxy import isinstance as zope_isinstance
 
 from lp.app.errors import NotFoundError
-from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.sourcepackagename import (
@@ -65,12 +64,6 @@
     IComponentSet,
     )
 from lp.soyuz.interfaces.packageset import IPackageset
-from lp.soyuz.model.packageset import Packageset
-
-
-def _extract_type_name(value):
-    """Extract the type name of the given value."""
-    return str(type(value)).split("'")[-2]
 
 
 class ArchivePermission(SQLBase):
@@ -470,26 +463,6 @@
             permission=ArchivePermissionType.QUEUE_ADMIN, **kwargs)
         self._remove_permission(permission)
 
-    def _nameToPackageset(self, packageset):
-        """Helper to convert a possible string name to IPackageset."""
-        if isinstance(packageset, basestring):
-            # A package set name was passed, assume the current distro series.
-            ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
-            name = packageset
-            store = IStore(Packageset)
-            packageset = store.find(
-                Packageset, name=name,
-                distroseries=ubuntu.currentseries).one()
-            if packageset is not None:
-                return packageset
-            else:
-                raise NotFoundError("No such package set '%s'" % name)
-        elif IPackageset.providedBy(packageset):
-            return packageset
-        else:
-            raise ValueError(
-                'Not a package set: %s' % _extract_type_name(packageset))
-
     def packagesetsForUploader(self, archive, person):
         """See `IArchivePermissionSet`."""
         store = IStore(ArchivePermission)
@@ -508,7 +481,6 @@
     def uploadersForPackageset(
         self, archive, packageset, direct_permissions=True):
         """See `IArchivePermissionSet`."""
-        packageset = self._nameToPackageset(packageset)
         store = IStore(ArchivePermission)
         if direct_permissions == True:
             query = '''
@@ -528,7 +500,6 @@
     def newPackagesetUploader(
         self, archive, person, packageset, explicit=False):
         """See `IArchivePermissionSet`."""
-        packageset = self._nameToPackageset(packageset)
         store = IMasterStore(ArchivePermission)
 
         # First see whether we have a matching permission in the database
@@ -583,7 +554,6 @@
     def deletePackagesetUploader(
         self, archive, person, packageset, explicit=False):
         """See `IArchivePermissionSet`."""
-        packageset = self._nameToPackageset(packageset)
         store = IMasterStore(ArchivePermission)
 
         # Do we have the permission the user wants removed in the database?
@@ -642,13 +612,9 @@
         return rset
 
     def isSourceUploadAllowed(
-        self, archive, sourcepackagename, person, distroseries=None):
+        self, archive, sourcepackagename, person, distroseries):
         """See `IArchivePermissionSet`."""
         sourcepackagename = self._nameToSourcePackageName(sourcepackagename)
-        store = IStore(ArchivePermission)
-        if distroseries is None:
-            ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
-            distroseries = ubuntu.currentseries
 
         # Put together the parameters for the query that follows.
         archive_params = (ArchivePermissionType.UPLOAD, archive.id)
@@ -696,4 +662,4 @@
         END AS number_of_permitted_package_sets;
 
         ''' % sqlvalues(*query_params)
-        return store.execute(query).get_one()[0] > 0
+        return IStore(ArchivePermission).execute(query).get_one()[0] > 0

=== modified file 'lib/lp/soyuz/model/packageset.py'
--- lib/lp/soyuz/model/packageset.py	2013-06-20 05:50:00 +0000
+++ lib/lp/soyuz/model/packageset.py	2014-07-04 13:09:27 +0000
@@ -44,11 +44,6 @@
     return result_set.order_by('name')
 
 
-def _extract_type_name(value):
-    """Extract the type name of the given value."""
-    return str(type(value)).split("'")[-2]
-
-
 class Packageset(Storm):
     """See `IPackageset`."""
     implements(IPackageset)

=== modified file 'lib/lp/soyuz/tests/test_packageset.py'
--- lib/lp/soyuz/tests/test_packageset.py	2013-06-20 05:50:00 +0000
+++ lib/lp/soyuz/tests/test_packageset.py	2014-07-04 13:09:27 +0000
@@ -727,6 +727,7 @@
         self.ap_set = getUtility(IArchivePermissionSet)
         self.archive = self.factory.makeArchive()
         self.packageset = self.factory.makePackageset()
+        self.distroseries = self.packageset.distroseries
         self.person = self.factory.makePerson()
 
     def test_packagesets_for_uploader_empty(self):
@@ -908,22 +909,6 @@
             self.ap_set.uploadersForPackageset(
                 self.archive, child, direct_permissions=False).is_empty())
 
-    def test_uploaders_for_packageset_by_name(self):
-        # a packageset name that doesn't exist will throw an error
-        self.ap_set.newPackagesetUploader(
-            self.archive, self.person, self.packageset)
-        # A correct name will give us a result:
-        self.assertFalse(self.ap_set.uploadersForPackageset(
-            self.archive, self.packageset.name).is_empty())
-        # An incorrect one will raise an exception
-        self.assertRaises(
-            NotFoundError, self.ap_set.uploadersForPackageset,
-            self.archive, self.factory.getUniqueUnicode())
-        # An incorrect type will raise a ValueError
-        self.assertRaises(
-            ValueError, self.ap_set.uploadersForPackageset,
-            self.archive, 42)
-
     def test_archive_permission_per_archive(self):
         # archive permissions are limited to an archive
         archive2 = self.factory.makeArchive()
@@ -961,7 +946,7 @@
         self.packageset.add((package,))
 
         self.assertTrue(self.ap_set.isSourceUploadAllowed(
-            self.archive, package, self.person))
+            self.archive, package, self.person, self.distroseries))
 
     def test_is_source_upload_allowed_denied(self):
         # isSourceUploadAllowed should return false when a user has no
@@ -971,7 +956,7 @@
         package = self.factory.makeSourcePackageName()
 
         self.assertFalse(self.ap_set.isSourceUploadAllowed(
-            self.archive, package, self.person))
+            self.archive, package, self.person, self.distroseries))
 
     def test_explicit_packageset_upload_rights(self):
         # If a package is covered by a packageset with explicit upload rights,
@@ -984,9 +969,9 @@
         self.packageset.add((package, package2))
 
         self.assertTrue(self.ap_set.isSourceUploadAllowed(
-            self.archive, package, self.person))
+            self.archive, package, self.person, self.distroseries))
         self.assertTrue(self.ap_set.isSourceUploadAllowed(
-            self.archive, package2, self.person))
+            self.archive, package2, self.person, self.distroseries))
 
         # Create a packageset with explicit rights to package
         special_person = self.factory.makePerson()
@@ -996,13 +981,13 @@
             self.archive, special_person, special_packageset, True)
 
         self.assertFalse(self.ap_set.isSourceUploadAllowed(
-            self.archive, package, self.person))
-        self.assertTrue(self.ap_set.isSourceUploadAllowed(
-            self.archive, package2, self.person))
-        self.assertTrue(self.ap_set.isSourceUploadAllowed(
-            self.archive, package, special_person))
+            self.archive, package, self.person, self.distroseries))
+        self.assertTrue(self.ap_set.isSourceUploadAllowed(
+            self.archive, package2, self.person, self.distroseries))
+        self.assertTrue(self.ap_set.isSourceUploadAllowed(
+            self.archive, package, special_person, self.distroseries))
         self.assertFalse(self.ap_set.isSourceUploadAllowed(
-            self.archive, package2, special_person))
+            self.archive, package2, special_person, self.distroseries))
 
     def test_delete_packageset_uploader(self):
         # deletePackagesetUploader removes upload rights