Merge lp:~wgrant/launchpad/recipe-cancel into lp:launchpad

[William Grant]

Allow archive owners to cancel their recipe builds, and make recipes use the normal cancellation infrastructure.

Allow archive owners to cancel their recipe builds, and make recipes use the normal cancellation infrastructure rather than just setting themselves to SUPERSEDED.

[Colin Watson (cjwatson)]

Very nice, thanks.

review: Approve

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2015-09-10 11:20:58 +0000
+++ lib/lp/code/browser/configure.zcml	2015-09-11 14:23:48 +0000
@@ -1176,7 +1176,7 @@
             class="lp.code.browser.sourcepackagerecipebuild.SourcePackageRecipeBuildCancelView"
             name="+cancel"
             template="../../app/templates/generic-edit.pt"
-            permission="launchpad.Admin"/>
+            permission="launchpad.Edit"/>
         <browser:page
             for="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"
             class="lp.code.browser.sourcepackagerecipebuild.SourcePackageRecipeBuildRescoreView"

=== modified file 'lib/lp/code/browser/sourcepackagerecipebuild.py'
--- lib/lp/code/browser/sourcepackagerecipebuild.py	2013-11-18 07:51:18 +0000
+++ lib/lp/code/browser/sourcepackagerecipebuild.py	2015-09-11 14:23:48 +0000
@@ -39,13 +39,6 @@
     )
 
 
-UNEDITABLE_BUILD_STATES = (
-    BuildStatus.FULLYBUILT,
-    BuildStatus.FAILEDTOBUILD,
-    BuildStatus.SUPERSEDED,
-    BuildStatus.FAILEDTOUPLOAD,)
-
-
 class SourcePackageRecipeBuildNavigation(Navigation, FileNavigationMixin):
 
     usedfor = ISourcePackageRecipeBuild
@@ -60,21 +53,17 @@
 
     links = ('cancel', 'rescore')
 
-    @enabled_with_permission('launchpad.Admin')
+    @enabled_with_permission('launchpad.Edit')
     def cancel(self):
-        if self.context.status in UNEDITABLE_BUILD_STATES:
-            enabled = False
-        else:
-            enabled = True
-        return Link('+cancel', 'Cancel build', icon='remove', enabled=enabled)
+        return Link(
+            '+cancel', 'Cancel build', icon='remove',
+            enabled=self.context.can_be_cancelled)
 
     @enabled_with_permission('launchpad.Admin')
     def rescore(self):
-        if self.context.status in UNEDITABLE_BUILD_STATES:
-            enabled = False
-        else:
-            enabled = True
-        return Link('+rescore', 'Rescore build', icon='edit', enabled=enabled)
+        return Link(
+            '+rescore', 'Rescore build', icon='edit',
+            enabled=self.context.can_be_rescored)
 
 
 class SourcePackageRecipeBuildView(LaunchpadView):
@@ -152,7 +141,7 @@
     @action('Cancel build', name='cancel')
     def request_action(self, action, data):
         """Cancel the build."""
-        self.context.cancelBuild()
+        self.context.cancel()
 
 
 class SourcePackageRecipeBuildRescoreView(LaunchpadFormView):

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2015-08-03 12:59:18 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2015-09-11 14:23:48 +0000
@@ -22,7 +22,6 @@
     BrowserTestCase,
     login,
     logout,
-    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
@@ -83,17 +82,19 @@
             daily_build_archive=self.ppa)
         build = self.factory.makeSourcePackageRecipeBuild(
             recipe=recipe)
+        build.queueBuild()
         return build
 
     def test_cancel_build(self):
-        """An admin can cancel a build."""
+        """The archive owner can cancel a build."""
         queue = self.factory.makeSourcePackageRecipeBuild().queueBuild()
         build = queue.specific_build
         transaction.commit()
         build_url = canonical_url(build)
+        owner = build.archive.owner
         logout()
 
-        browser = self.getUserBrowser(build_url, user=self.admin)
+        browser = self.getUserBrowser(build_url, user=owner)
         browser.getLink('Cancel build').click()
 
         self.assertEqual(
@@ -107,12 +108,10 @@
             build_url)
 
         login(ANONYMOUS)
-        self.assertEqual(
-            BuildStatus.SUPERSEDED,
-            build.status)
+        self.assertEqual(BuildStatus.CANCELLED, build.status)
 
-    def test_cancel_build_not_admin(self):
-        """No one but an admin can cancel a build."""
+    def test_cancel_build_not_owner(self):
+        """A normal user can't cancel a build."""
         queue = self.factory.makeSourcePackageRecipeBuild().queueBuild()
         build = queue.specific_build
         transaction.commit()
@@ -131,12 +130,14 @@
     def test_cancel_build_wrong_state(self):
         """If the build isn't queued, you can't cancel it."""
         build = self.makeRecipeBuild()
-        build.cancelBuild()
+        with admin_logged_in():
+            build.cancel()
         transaction.commit()
         build_url = canonical_url(build)
+        owner = build.archive.owner
         logout()
 
-        browser = self.getUserBrowser(build_url, user=self.admin)
+        browser = self.getUserBrowser(build_url, user=owner)
         self.assertRaises(
             LinkNotFoundError,
             browser.getLink, 'Cancel build')
@@ -212,7 +213,8 @@
     def test_rescore_build_wrong_state(self):
         """If the build isn't queued, you can't rescore it."""
         build = self.makeRecipeBuild()
-        build.cancelBuild()
+        with admin_logged_in():
+            build.cancel()
         transaction.commit()
         build_url = canonical_url(build)
         logout()
@@ -228,25 +230,11 @@
         This is the case where the user has a stale link that they click on.
         """
         build = self.factory.makeSourcePackageRecipeBuild()
-        build.cancelBuild()
-        index_url = canonical_url(build)
-        browser = self.getViewBrowser(build, '+rescore', user=self.admin)
-        self.assertEqual(index_url, browser.url)
-        self.assertIn(
-            'Cannot rescore this build because it is not queued.',
-            browser.contents)
-
-    def test_rescore_build_wrong_state_stale_page(self):
-        """Show sane error if you attempt to rescore a non-queued build.
-
-        This is the case where the user is on the rescore page and submits.
-        """
-        build = self.factory.makeSourcePackageRecipeBuild()
-        index_url = canonical_url(build)
-        browser = self.getViewBrowser(build, '+rescore', user=self.admin)
-        with person_logged_in(self.admin):
-            build.cancelBuild()
-        browser.getLink('Rescore build').click()
+        build.queueBuild()
+        with admin_logged_in():
+            build.cancel()
+        index_url = canonical_url(build)
+        browser = self.getViewBrowser(build, '+rescore', user=self.admin)
         self.assertEqual(index_url, browser.url)
         self.assertIn(
             'Cannot rescore this build because it is not queued.',

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2015-09-01 17:10:46 +0000
+++ lib/lp/code/configure.zcml	2015-09-11 14:23:48 +0000
@@ -1015,7 +1015,12 @@
 
   <class
      class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuild">
-    <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"/>
+    <require
+        permission="launchpad.View"
+        interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildView"/>
+    <require
+        permission="launchpad.Edit"
+        interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildEdit"/>
   </class>
 
   <securedutility

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipebuild.py'
--- lib/lp/code/interfaces/sourcepackagerecipebuild.py	2015-05-14 08:50:41 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipebuild.py	2015-09-11 14:23:48 +0000
@@ -9,11 +9,17 @@
     'ISourcePackageRecipeBuildSource',
     ]
 
-from lazr.restful.declarations import export_as_webservice_entry
+from lazr.restful.declarations import (
+    export_as_webservice_entry,
+    export_write_operation,
+    exported,
+    operation_for_version,
+    )
 from lazr.restful.fields import (
     CollectionField,
     Reference,
     )
+from zope.interface import Interface
 from zope.schema import (
     Bool,
     Int,
@@ -21,9 +27,7 @@
     )
 
 from lp import _
-from lp.buildmaster.interfaces.buildfarmjob import (
-    ISpecificBuildFarmJobSource,
-    )
+from lp.buildmaster.interfaces.buildfarmjob import ISpecificBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.code.interfaces.sourcepackagerecipe import (
     ISourcePackageRecipe,
@@ -35,9 +39,7 @@
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 
 
-class ISourcePackageRecipeBuild(IPackageBuild):
-    """A build of a source package."""
-    export_as_webservice_entry()
+class ISourcePackageRecipeBuildView(IPackageBuild):
 
     id = Int(title=_("Identifier for this build."))
 
@@ -56,6 +58,16 @@
     recipe = Object(
         schema=ISourcePackageRecipe, title=_("The recipe being built."))
 
+    can_be_rescored = exported(Bool(
+        title=_("Can be rescored"),
+        required=True, readonly=True,
+        description=_("Whether this build record can be rescored manually.")))
+
+    can_be_cancelled = exported(Bool(
+        title=_("Can be cancelled"),
+        required=True, readonly=True,
+        description=_("Whether this build record can be cancelled.")))
+
     manifest = Object(
         schema=ISourcePackageRecipeData, title=_(
             'A snapshot of the recipe for this build.'))
@@ -70,13 +82,36 @@
     def getFileByName(filename):
         """Return the file under +files with specified name."""
 
-    def cancelBuild():
-        """Cancel the build."""
+
+class ISourcePackageRecipeBuildEdit(Interface):
+
+    @export_write_operation()
+    @operation_for_version("devel")
+    def cancel():
+        """Cancel the build if it is either pending or in progress.
+
+        Check the can_be_cancelled property prior to calling this method to
+        find out if cancelling the build is possible.
+
+        If the build is in progress, it is marked as CANCELLING until the
+        buildd manager terminates the build and marks it CANCELLED.  If the
+        build is not in progress, it is marked CANCELLED immediately and is
+        removed from the build queue.
+
+        If the build is not in a cancellable state, this method is a no-op.
+        """
 
     def destroySelf():
         """Delete the build itself."""
 
 
+class ISourcePackageRecipeBuild(ISourcePackageRecipeBuildView,
+                                ISourcePackageRecipeBuildEdit):
+    """A build of a source package."""
+
+    export_as_webservice_entry()
+
+
 class ISourcePackageRecipeBuildSource(ISpecificBuildFarmJobSource):
     """A utility of this interface be used to create source package builds."""
 

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2015-09-11 13:28:31 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2015-09-11 14:23:48 +0000
@@ -259,11 +259,31 @@
                     builds.append(build)
         return builds
 
-    def cancelBuild(self):
-        """See `ISourcePackageRecipeBuild.`"""
-        self.updateStatus(BuildStatus.SUPERSEDED)
-        if self.buildqueue_record is not None:
-            self.buildqueue_record.destroySelf()
+    @property
+    def can_be_rescored(self):
+        """See `IBuild`."""
+        return self.status is BuildStatus.NEEDSBUILD
+
+    @property
+    def can_be_cancelled(self):
+        """See `ISourcePackageRecipeBuild`."""
+        if not self.buildqueue_record:
+            return False
+
+        cancellable_statuses = [
+            BuildStatus.BUILDING,
+            BuildStatus.NEEDSBUILD,
+            ]
+        return self.status in cancellable_statuses
+
+    def cancel(self):
+        """See `ISourcePackageRecipeBuild`."""
+        if not self.can_be_cancelled:
+            return
+        # BuildQueue.cancel() will decide whether to go straight to
+        # CANCELLED, or go through CANCELLING to let buildd-manager
+        # clean up the slave.
+        self.buildqueue_record.cancel()
 
     def destroySelf(self):
         if self.buildqueue_record is not None:

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2015-04-30 01:45:30 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2015-09-11 14:23:48 +0000
@@ -666,7 +666,8 @@
         # Cancelled builds are not considered pending.
         recipe = self.factory.makeSourcePackageRecipe()
         build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
-        build.cancelBuild()
+        build.queueBuild()
+        build.cancel()
         self.assertEqual([build], list(recipe.builds))
         self.assertEqual([build], list(recipe.completed_builds))
         self.assertEqual([], list(recipe.pending_builds))

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2015-09-11 06:04:36 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2015-09-11 14:23:48 +0000
@@ -38,6 +38,7 @@
 from lp.services.mail.sendmail import format_address
 from lp.services.webapp.authorization import check_permission
 from lp.testing import (
+    admin_logged_in,
     ANONYMOUS,
     login,
     person_logged_in,
@@ -78,7 +79,8 @@
         # SourcePackageRecipeBuild provides IPackageBuild and
         # ISourcePackageRecipeBuild.
         spb = self.makeSourcePackageRecipeBuild()
-        self.assertProvides(spb, ISourcePackageRecipeBuild)
+        with admin_logged_in():
+            self.assertProvides(spb, ISourcePackageRecipeBuild)
 
     def test_implements_interface(self):
         build = self.makeSourcePackageRecipeBuild()
@@ -88,7 +90,8 @@
         # A source package recipe build can be stored in the database
         spb = self.makeSourcePackageRecipeBuild()
         transaction.commit()
-        self.assertProvides(spb, ISourcePackageRecipeBuild)
+        with admin_logged_in():
+            self.assertProvides(spb, ISourcePackageRecipeBuild)
 
     def test_queueBuild(self):
         spb = self.makeSourcePackageRecipeBuild()
@@ -411,7 +414,8 @@
         # ISourcePackageRecipeBuild should make sure to remove jobs and build
         # queue entries and then invalidate itself.
         build = self.factory.makeSourcePackageRecipeBuild()
-        build.destroySelf()
+        with admin_logged_in():
+            build.destroySelf()
 
     def test_destroySelf_clears_release(self):
         # Destroying a sourcepackagerecipebuild removes references to it from
@@ -420,7 +424,8 @@
         release = self.factory.makeSourcePackageRelease(
             source_package_recipe_build=build)
         self.assertEqual(build, release.source_package_recipe_build)
-        build.destroySelf()
+        with admin_logged_in():
+            build.destroySelf()
         self.assertIs(None, release.source_package_recipe_build)
         transaction.commit()
 
@@ -433,18 +438,19 @@
         # Ensure database ids are set.
         store.flush()
         build_farm_job_id = naked_build.build_farm_job_id
-        build.destroySelf()
+        with admin_logged_in():
+            build.destroySelf()
         self.assertIs(None, store.get(BuildFarmJob, build_farm_job_id))
 
-    def test_cancelBuild(self):
+    def test_cancel(self):
         # ISourcePackageRecipeBuild should make sure to remove jobs and build
         # queue entries and then invalidate itself.
         build = self.factory.makeSourcePackageRecipeBuild()
-        build.cancelBuild()
+        build.queueBuild()
+        with admin_logged_in():
+            build.cancel()
 
-        self.assertEqual(
-            BuildStatus.SUPERSEDED,
-            build.status)
+        self.assertEqual(BuildStatus.CANCELLED, build.status)
 
     def test_getUploader(self):
         # For ACL purposes the uploader is the build requester.
@@ -524,7 +530,8 @@
         build = self.factory.makeSourcePackageRecipeBuild(
             recipe=cake, distroseries=secret, archive=pantry)
         build.updateStatus(BuildStatus.FULLYBUILT)
-        cake.destroySelf()
+        with admin_logged_in():
+            cake.destroySelf()
         IStore(build).flush()
         build.notify()
         notifications = pop_notifications()