launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-820055 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #820055 in Launchpad itself: "AttributeError: 'NoneType' object has no attribute 'remove'"
  https://bugs.launchpad.net/launchpad/+bug/820055

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-820055/+merge/73064

This small branch fixes bug 820055 in the way Julian suggests in the comments, by simply returning successfully if permissions are requested to be removed that have already been removed.

QA: run a luanchpadlib script to remove permissions for a user that doesn't have permissions

lint: the make lint report is clean

testing: a test was added to lp.soyuz.tests which can be run thusly:

    bin/test -c -m lp.soyuz.tests -t TestRemovingPermissions
-- 
https://code.launchpad.net/~benji/launchpad/bug-820055/+merge/73064
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-820055 into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/archivepermission.py'
--- lib/lp/soyuz/model/archivepermission.py	2011-06-16 20:12:00 +0000
+++ lib/lp/soyuz/model/archivepermission.py	2011-08-26 15:20:38 +0000
@@ -327,6 +327,15 @@
                 archive=archive, person=person, component=component,
                 permission=ArchivePermissionType.QUEUE_ADMIN)
 
+    @staticmethod
+    def _remove_permission(permission):
+        if permission is None:
+            # The permission has already been removed, so there's nothing more
+            # to do here.
+            return
+        else:
+            Store.of(permission).remove(permission)
+
     def deletePackageUploader(self, archive, person, sourcepackagename):
         """See `IArchivePermissionSet`."""
         sourcepackagename = self._nameToSourcePackageName(sourcepackagename)
@@ -334,7 +343,7 @@
             archive=archive, person=person,
             sourcepackagename=sourcepackagename,
             permission=ArchivePermissionType.UPLOAD)
-        Store.of(permission).remove(permission)
+        self._remove_permission(permission)
 
     def deleteComponentUploader(self, archive, person, component):
         """See `IArchivePermissionSet`."""
@@ -342,7 +351,7 @@
         permission = ArchivePermission.selectOneBy(
             archive=archive, person=person, component=component,
             permission=ArchivePermissionType.UPLOAD)
-        Store.of(permission).remove(permission)
+        self._remove_permission(permission)
 
     def deleteQueueAdmin(self, archive, person, component):
         """See `IArchivePermissionSet`."""
@@ -350,7 +359,7 @@
         permission = ArchivePermission.selectOneBy(
             archive=archive, person=person, component=component,
             permission=ArchivePermissionType.QUEUE_ADMIN)
-        Store.of(permission).remove(permission)
+        self._remove_permission(permission)
 
     def _nameToPackageset(self, packageset):
         """Helper to convert a possible string name to IPackageset."""
@@ -473,10 +482,7 @@
             ArchivePermission, archive=archive, person=person,
             packageset=packageset, permission=ArchivePermissionType.UPLOAD,
             explicit=explicit).one()
-
-        if permission is not None:
-            # Permission found, remove it!
-            store.remove(permission)
+        self._remove_permission(permission)
 
     def packagesetsForSourceUploader(
         self, archive, sourcepackagename, person):

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-08-25 10:16:22 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-08-26 15:20:38 +0000
@@ -77,7 +77,10 @@
 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.archivepermission import ArchivePermission
+from lp.soyuz.model.archivepermission import (
+    ArchivePermission,
+    ArchivePermissionSet,
+    )
 from lp.soyuz.model.binarypackagerelease import (
     BinaryPackageReleaseDownloadCount,
     )
@@ -2269,3 +2272,15 @@
             target_archive.copyPackages, [source_name], source_archive,
             to_pocket.name, to_series=to_series.name, include_binaries=False,
             person=person)
+
+
+class TestRemovingPermissions(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_remove_permission_is_none(self):
+        # Several API functions remove permissions if they are not already
+        # removed.  This verifies that the underlying utility function does
+        # not generate an error if the permission is None.
+        ap_set = ArchivePermissionSet()
+        ap_set._remove_permission(None)