launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-delete-with-builds into lp:launchpad.

Commit message:
Allow deleting snaps even if they have builds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-delete-with-builds/+merge/287408

There isn't really a good reason to forbid deleting snaps that have builds.  As far as I remember I did that because I was basing that bit of my code on recipes, but recipes have extra constraints because recipe builds are linked to source packages.  That constraint doesn't exist for snaps, so it should be safe to allow this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-delete-with-builds into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/configure.zcml'
--- lib/lp/snappy/browser/configure.zcml	2016-01-27 12:43:00 +0000
+++ lib/lp/snappy/browser/configure.zcml	2016-02-28 17:16:06 +0000
@@ -71,7 +71,7 @@
             class="lp.snappy.browser.snap.SnapDeleteView"
             permission="launchpad.Edit"
             name="+delete"
-            template="../templates/snap-delete.pt" />
+            template="../../app/templates/generic-edit.pt" />
         <adapter
             provides="lp.services.webapp.interfaces.IBreadcrumb"
             for="lp.snappy.interfaces.snap.ISnap"

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2016-02-19 14:18:22 +0000
+++ lib/lp/snappy/browser/snap.py	2016-02-28 17:16:06 +0000
@@ -514,10 +514,6 @@
 
     field_names = []
 
-    @property
-    def has_builds(self):
-        return not self.context.builds.is_empty()
-
     @action('Delete snap package', name='delete')
     def delete_action(self, action, data):
         owner = self.context.owner

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2016-02-05 14:46:32 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2016-02-28 17:16:06 +0000
@@ -595,14 +595,18 @@
         self.assertRaises(NotFound, browser.open, snap_url)
 
     def test_delete_snap_with_builds(self):
-        # A snap package with builds cannot be deleted.
+        # A snap package with builds can be deleted.
+        self.useFixture(FakeLogger())
         snap = self.factory.makeSnap(registrant=self.person, owner=self.person)
-        self.factory.makeSnapBuild(snap=snap)
+        build = self.factory.makeSnapBuild(snap=snap)
+        self.factory.makeSnapFile(snapbuild=build)
+        snap_url = canonical_url(snap)
+        owner_url = canonical_url(self.person)
         browser = self.getViewBrowser(snap, user=self.person)
         browser.getLink("Delete snap package").click()
-        self.assertIn("This snap package cannot be deleted", browser.contents)
-        self.assertRaises(
-            LookupError, browser.getControl, "Delete snap package")
+        browser.getControl("Delete snap package").click()
+        self.assertEqual(owner_url, browser.url)
+        self.assertRaises(NotFound, browser.open, snap_url)
 
 
 class TestSnapView(BrowserTestCase):

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2016-02-19 14:18:22 +0000
+++ lib/lp/snappy/interfaces/snap.py	2016-02-28 17:16:06 +0000
@@ -7,7 +7,6 @@
 
 __all__ = [
     'BadSnapSearchContext',
-    'CannotDeleteSnap',
     'CannotModifySnapProcessor',
     'DuplicateSnapName',
     'ISnap',
@@ -197,11 +196,6 @@
             "Snap contains private information and cannot be public.")
 
 
-@error_status(httplib.BAD_REQUEST)
-class CannotDeleteSnap(Exception):
-    """This snap package cannot be deleted."""
-
-
 class BadSnapSearchContext(Exception):
     """The context is not valid for a snap package search."""
 

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2016-02-19 14:18:22 +0000
+++ lib/lp/snappy/model/snap.py	2016-02-28 17:16:06 +0000
@@ -65,6 +65,7 @@
     IMasterStore,
     IStore,
     )
+from lp.services.database.sqlbase import quote
 from lp.services.database.stormexpr import (
     Greatest,
     NullsLast,
@@ -75,7 +76,6 @@
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
-    CannotDeleteSnap,
     CannotModifySnapProcessor,
     DuplicateSnapName,
     ISnap,
@@ -355,11 +355,22 @@
 
     def destroySelf(self):
         """See `ISnap`."""
-        if not self.builds.is_empty():
-            raise CannotDeleteSnap("Cannot delete a snap package with builds.")
         store = IStore(Snap)
         store.find(SnapArch, SnapArch.snap == self).remove()
+<<<<<<< TREE
         getUtility(IWebhookSet).delete(self.webhooks)
+=======
+        # XXX cjwatson 2016-02-27 bug=322972: Requires manual SQL due to
+        # lack of support for DELETE FROM ... USING ... in Storm.
+        store.execute("""
+            DELETE FROM SnapFile
+            USING SnapBuild
+            WHERE
+                SnapFile.snapbuild = SnapBuild.id AND
+                SnapBuild.snap = %s
+            """ % quote(self.id))
+        store.find(SnapBuild, SnapBuild.snap == self).remove()
+>>>>>>> MERGE-SOURCE
         store.remove(self)
 
 

=== removed file 'lib/lp/snappy/templates/snap-delete.pt'
--- lib/lp/snappy/templates/snap-delete.pt	2015-09-04 16:20:26 +0000
+++ lib/lp/snappy/templates/snap-delete.pt	1970-01-01 00:00:00 +0000
@@ -1,21 +0,0 @@
-<html
-  xmlns="http://www.w3.org/1999/xhtml";
-  xmlns:tal="http://xml.zope.org/namespaces/tal";
-  xmlns:metal="http://xml.zope.org/namespaces/metal";
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
-  metal:use-macro="view/macro:page/main_only"
-  i18n:domain="launchpad">
-<body>
-
-  <div metal:fill-slot="main">
-    <tal:has-builds condition="view/has_builds">
-      This snap package cannot be deleted as builds have been created for it.
-    </tal:has-builds>
-
-    <tal:has-no-builds condition="not: view/has_builds">
-      <div metal:use-macro="context/@@launchpad_form/form"/>
-    </tal:has-no-builds>
-  </div>
-
-</body>
-</html>

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2016-02-19 14:18:22 +0000
+++ lib/lp/snappy/tests/test_snap.py	2016-02-28 17:16:06 +0000
@@ -32,11 +32,12 @@
     ONE_DAY_AGO,
     UTC_NOW,
     )
+from lp.services.database.interfaces import IMasterStore
+from lp.services.database.sqlbase import flush_database_caches
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
-    CannotDeleteSnap,
     CannotModifySnapProcessor,
     ISnap,
     ISnapSet,
@@ -50,7 +51,11 @@
     SnapPrivacyMismatch,
     SnapPrivateFeatureDisabled,
     )
-from lp.snappy.interfaces.snapbuild import ISnapBuild
+from lp.snappy.interfaces.snapbuild import (
+    ISnapBuild,
+    ISnapBuildSet,
+    )
+from lp.snappy.model.snapbuild import SnapFile
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -359,18 +364,34 @@
             snap.destroySelf()
         self.assertFalse(getUtility(ISnapSet).exists(owner, u"condemned"))
 
+
+class TestSnapDeleteWithBuilds(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestSnapDeleteWithBuilds, self).setUp()
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+
     def test_delete_with_builds(self):
-        # A snap package with builds cannot be deleted.
+        # A snap package with builds can be deleted.  Doing so deletes all
+        # its builds and their files too.
         owner = self.factory.makePerson()
         distroseries = self.factory.makeDistroSeries()
         snap = self.factory.makeSnap(
             registrant=owner, owner=owner, distroseries=distroseries,
             name=u"condemned")
-        self.factory.makeSnapBuild(snap=snap)
+        build = self.factory.makeSnapBuild(snap=snap)
+        snapfile = self.factory.makeSnapFile(snapbuild=build)
         self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
+        build_id = build.id
+        snapfile_id = removeSecurityProxy(snapfile).id
         with person_logged_in(snap.owner):
-            self.assertRaises(CannotDeleteSnap, snap.destroySelf)
-        self.assertTrue(getUtility(ISnapSet).exists(owner, u"condemned"))
+            snap.destroySelf()
+        flush_database_caches()
+        self.assertFalse(getUtility(ISnapSet).exists(owner, u"condemned"))
+        self.assertIsNone(getUtility(ISnapBuildSet).getByID(build_id))
+        self.assertIsNone(IMasterStore(SnapFile).get(SnapFile, snapfile_id))
 
     def test_related_webhooks_deleted(self):
         owner = self.factory.makePerson()