launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-delete-api into lp:launchpad.

Commit message:
Export archive deletion on the API.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #814633 in Launchpad itself: "Cannot delete a PPA over the API"
  https://bugs.launchpad.net/launchpad/+bug/814633

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-delete-api/+merge/271239

Export archive deletion on the API.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ppa-delete-api into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	2015-05-19 02:24:48 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2015-09-16 07:56:13 +0000
@@ -12,7 +12,10 @@
     Unauthorized as LRUnauthorized,
     )
 from testtools import ExpectedException
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import getUtility
 
@@ -107,6 +110,40 @@
             get_permissions, create_permission, 1)
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
+    def test_delete(self):
+        with admin_logged_in():
+            ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+            ppa_url = api_url(ppa)
+            ws = webservice_for_person(
+                ppa.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        # DELETE on an archive resource doesn't actually remove it
+        # immediately, but it asks the publisher to delete it later.
+        self.assertEqual(
+            'Active',
+            ws.get(ppa_url, api_version='devel').jsonBody()['status'])
+        self.assertEqual(200, ws.delete(ppa_url, api_version='devel').status)
+        self.assertEqual(
+            'Deleting',
+            ws.get(ppa_url, api_version='devel').jsonBody()['status'])
+
+        # Deleting the PPA again fails.
+        self.assertThat(
+            ws.delete(ppa_url, api_version='devel'),
+            MatchesStructure.byEquality(
+                status=400, body="Archive already deleted."))
+
+    def test_delete_is_restricted(self):
+        with admin_logged_in():
+            ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+            ppa_url = api_url(ppa)
+            ws = webservice_for_person(
+                self.factory.makePerson(),
+                permission=OAuthPermission.WRITE_PRIVATE)
+
+        # A random user can't delete someone else's PPA.
+        self.assertEqual(401, ws.delete(ppa_url, api_version='devel').status)
+
 
 class TestExternalDependencies(WebServiceTestCase):
 

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2015-08-25 16:10:09 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2015-09-16 07:56:13 +0000
@@ -59,6 +59,7 @@
     error_status,
     export_as_webservice_collection,
     export_as_webservice_entry,
+    export_destructor_operation,
     export_factory_operation,
     export_operation_as,
     export_read_operation,
@@ -183,6 +184,11 @@
     """Raised on some queries when version is specified but name is not."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class ArchiveAlreadyDeleted(Exception):
+    """Archive already deleted."""
+
+
 @error_status(httplib.FORBIDDEN)
 class CannotUploadToArchive(Exception):
     """A reason for not being able to upload to an archive."""
@@ -1968,6 +1974,9 @@
     def disable():
         """Disable the archive."""
 
+    @export_destructor_operation()
+    @call_with(deleted_by=REQUEST_USER)
+    @operation_for_version('devel')
     def delete(deleted_by):
         """Delete this archive.
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2015-08-28 16:47:33 +0000
+++ lib/lp/soyuz/model/archive.py	2015-09-16 07:56:13 +0000
@@ -134,6 +134,7 @@
     )
 from lp.soyuz.interfaces.archive import (
     AlreadySubscribed,
+    ArchiveAlreadyDeleted,
     ArchiveDependencyError,
     ArchiveDisabled,
     ArchiveNotPrivate,
@@ -2088,9 +2089,8 @@
 
     def delete(self, deleted_by):
         """See `IArchive`."""
-        assert self.status not in (
-            ArchiveStatus.DELETING, ArchiveStatus.DELETED,
-            "This archive is already deleted.")
+        if self.status != ArchiveStatus.ACTIVE:
+            raise ArchiveAlreadyDeleted("Archive already deleted.")
 
         # Mark the archive's status as DELETING so the repository can be
         # removed by the publisher.