launchpad-reviewers team mailing list archive

[Paul Hummer <paul@xxxxxxxxxxxxxxxxxxxx>]

Paul Hummer has proposed merging lp:~rockstar/launchpad/rescore-builds into lp:launchpad/devel with lp:~rockstar/launchpad/spr-admin as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch adds the ability for an admin to rescore a build.  Here's my thought process for it:

Create the flow to rescore a build.  It's one of those annoying one-field form views, but it's for an admin, so I don't care.  Set the build for the score (with accompanying test).

Make sure the link is only visible by admins.

Make sure that if the admin had to make the change after EOD, so is now really drunk, the admin cannot set the score to anything but an integer.

Oh crap.  The state of the build is really important for this case.  You shouldn't be able to re-score the build if the build isn't queued.

Oh crap, the same problem exists for deleting a build.  Fix that now too.
-- 
https://code.launchpad.net/~rockstar/launchpad/rescore-builds/+merge/29853
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rockstar/launchpad/rescore-builds into lp:launchpad/devel.

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2010-07-14 10:35:56 +0000
+++ lib/lp/code/browser/configure.zcml	2010-07-14 10:35:58 +0000
@@ -1130,6 +1130,12 @@
             name="+cancel"
             template="../../app/templates/generic-edit.pt"
             permission="launchpad.View"/>
+        <browser:page
+            for="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuild"
+            class="lp.code.browser.sourcepackagerecipebuild.SourcePackageRecipeBuildRescoreView"
+            name="+rescore"
+            template="../../app/templates/generic-edit.pt"
+            permission="launchpad.View"/>
         <browser:menus
             classes="
                 SourcePackageRecipeNavigationMenu

=== modified file 'lib/lp/code/browser/sourcepackagerecipebuild.py'
--- lib/lp/code/browser/sourcepackagerecipebuild.py	2010-07-14 10:35:56 +0000
+++ lib/lp/code/browser/sourcepackagerecipebuild.py	2010-07-14 10:35:58 +0000
@@ -10,9 +10,11 @@
     'SourcePackageRecipeBuildNavigation',
     'SourcePackageRecipeBuildView',
     'SourcePackageRecipeBuildCancelView',
+    'SourcePackageRecipeBuildRescoreView',
     ]
 
 from zope.interface import Interface
+from zope.schema import Text
 
 from canonical.launchpad.browser.librarian import FileNavigationMixin
 from canonical.launchpad.webapp import (
@@ -37,11 +39,23 @@
 
     facet = 'branches'
 
-    links = ('cancel',)
+    links = ('cancel', 'rescore')
 
     @enabled_with_permission('launchpad.Edit')
     def cancel(self):
-        return Link('+cancel', 'Cancel build', icon='remove')
+        if self.context.buildqueue_record is None:
+            enabled = False
+        else:
+            enabled = True
+        return Link('+cancel', 'Cancel build', icon='remove', enabled=enabled)
+
+    @enabled_with_permission('launchpad.Edit')
+    def rescore(self):
+        if self.context.buildqueue_record is None:
+            enabled = False
+        else:
+            enabled = True
+        return Link('+rescore', 'Rescore build', icon='edit', enabled=enabled)
 
 
 class SourcePackageRecipeBuildView(LaunchpadView):
@@ -103,7 +117,7 @@
 
 
 class SourcePackageRecipeBuildCancelView(LaunchpadFormView):
-    """View for cancelling a view."""
+    """View for cancelling a build."""
 
     class schema(Interface):
         """Schema for cancelling a build."""
@@ -119,3 +133,38 @@
     def request_action(self, action, data):
         """Cancel the build."""
         self.context.cancelBuild()
+
+
+class SourcePackageRecipeBuildRescoreView(LaunchpadFormView):
+    """View for rescoring a build."""
+
+    class schema(Interface):
+        """Schema for deleting a build."""
+        score = Text(
+            title=u'Score', required=True,
+            description=u'The score of the recipe.')
+
+    page_title = label = "Rescore build"
+
+    @property
+    def cancel_url(self):
+        return canonical_url(self.context)
+    next_url = cancel_url
+
+    def validate(self, data):
+        try:
+            score = int(data['score'])
+        except ValueError:
+            self.setFieldError(
+                'score',
+                'You have specified an invalid value for score. '
+                'Please specify an integer')
+
+    @action('Rescore build', name='rescore')
+    def request_action(self, action, data):
+        """Rescore the build."""
+        self.context.buildqueue_record.lastscore = int(data['score'])
+
+    @property
+    def initial_values(self):
+        return {'score': str(self.context.buildqueue_record.lastscore)}

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2010-07-14 10:35:56 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2010-07-14 10:35:58 +0000
@@ -11,6 +11,8 @@
 from zope.component import getUtility
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.testing.pages import (
+    extract_text, find_tags_by_class)
 from canonical.launchpad.webapp import canonical_url
 from canonical.testing import DatabaseFunctionalLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
@@ -56,6 +58,7 @@
         """An admin can cancel a build."""
         experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
         build = self.makeRecipeBuild()
+        build.queueBuild(build)
         transaction.commit()
         build_url = canonical_url(build)
         logout()
@@ -89,3 +92,96 @@
         self.assertRaises(
             LinkNotFoundError,
             browser.getLink, 'Cancel build')
+
+    def test_cancel_build_wrong_state(self):
+        """If the build isn't queued, you can't cancel it."""
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        build = self.makeRecipeBuild()
+        transaction.commit()
+        build_url = canonical_url(build)
+        logout()
+
+        browser = self.getUserBrowser(build_url, user=experts)
+        self.assertRaises(
+            LinkNotFoundError,
+            browser.getLink, 'Cancel build')
+
+    def test_rescore_build(self):
+        """An admin can rescore a build."""
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        build = self.makeRecipeBuild()
+        build.queueBuild(build)
+        transaction.commit()
+        build_url = canonical_url(build)
+        logout()
+
+        browser = self.getUserBrowser(build_url, user=experts)
+        browser.getLink('Rescore build').click()
+
+        self.assertEqual(
+            browser.getLink('Cancel').url,
+            build_url)
+
+        browser.getControl('Score').value = '1024'
+
+        browser.getControl('Rescore build').click()
+
+        self.assertEqual(
+            browser.url,
+            build_url)
+
+        login(ANONYMOUS)
+        self.assertEqual(
+            build.buildqueue_record.lastscore,
+            1024)
+
+    def test_rescore_build_invalid_score(self):
+        """Build scores can only take numbers."""
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        build = self.makeRecipeBuild()
+        build.queueBuild(build)
+        transaction.commit()
+        build_url = canonical_url(build)
+        logout()
+
+        browser = self.getUserBrowser(build_url, user=experts)
+        browser.getLink('Rescore build').click()
+
+        self.assertEqual(
+            browser.getLink('Cancel').url,
+            build_url)
+
+        browser.getControl('Score').value = 'tentwentyfour'
+
+        browser.getControl('Rescore build').click()
+
+        self.assertEqual(
+            extract_text(find_tags_by_class(browser.contents, 'message')[1]),
+            'You have specified an invalid value for score. '
+            'Please specify an integer')
+
+    def test_rescore_build_not_admin(self):
+        """No one but admins can rescore a build."""
+        build = self.makeRecipeBuild()
+        build.queueBuild(build)
+        transaction.commit()
+        build_url = canonical_url(build)
+        logout()
+
+        browser = self.getUserBrowser(build_url, user=self.chef)
+        self.assertRaises(
+            LinkNotFoundError,
+            browser.getLink, 'Rescore build')
+
+    def test_rescore_build_wrong_state(self):
+        """If the build isn't queued, you can't rescore it."""
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        build = self.makeRecipeBuild()
+        transaction.commit()
+        build_url = canonical_url(build)
+        logout()
+
+        browser = self.getUserBrowser(build_url, user=experts)
+        self.assertRaises(
+            LinkNotFoundError,
+            browser.getLink, 'Rescore build')

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-07-14 10:35:56 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-07-14 10:35:58 +0000
@@ -241,7 +241,7 @@
         return builds
 
     def _unqueueBuild(self):
-        """Remove the packages queue and job."""
+        """Remove the build's queue and job."""
         store = Store.of(self)
         if self.buildqueue_record is not None:
             job = self.buildqueue_record.job

=== modified file 'lib/lp/code/templates/sourcepackagerecipebuild-index.pt'
--- lib/lp/code/templates/sourcepackagerecipebuild-index.pt	2010-07-14 10:35:56 +0000
+++ lib/lp/code/templates/sourcepackagerecipebuild-index.pt	2010-07-14 10:35:58 +0000
@@ -167,6 +167,15 @@
       >
       <a tal:replace="structure link/fmt:link" />
     </div>
+    <div
+      style="margin-top: 1.5em"
+      tal:define="context_menu view/context/menu:context;
+                  link context_menu/rescore"
+      tal:condition="link/enabled"
+      >
+      <a tal:replace="structure link/fmt:link" />
+    </div>
+
 
   </metal:macro>