launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/no-queue-rescore into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #676495 AttributeError on SourcePacakageRecipeBuild +rescore page
  https://bugs.launchpad.net/bugs/676495

For more details, see:
https://code.launchpad.net/~abentley/launchpad/no-queue-rescore/+merge/45560

= Summary =
Fix bug https://bugs.launchpad.net/launchpad/+bug/676495

== Proposed fix ==
Detect when the view is not applicable and redirect to the build index page
with an explanation.

== Pre-implementation notes ==
Discussed various implementations with Gary.

== Implementation details ==
The whole view is predicated on the fact that buildqueue is not None, so as
Gary suggested, I handle this case at the top of the call stack, in __call__.

To use getViewBrowser with an expert user, I added a 'user' parameter.  I used
DEFAULT as the default value, so that None can mean 'no user'.

I also cleaned up some lint.

== Tests ==
bin/test -t test_rescore_build_wrong_state_stale test_sourcepackagerecipebuild

== Demo and Q/A ==
Create a sourcepackage recipe build.  In a second window, click "rescore".

Cancel the build.  Append '/+rescore' to the URL, and click enter.  You should
be redirected, with the message "Cannot rescore this build because it is not
queued."

In the second window, click the "rescore" button.  You should get the
same thing.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/sourcepackagerecipebuild.py
  lib/lp/testing/__init__.py
  lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py

./lib/lp/testing/__init__.py
     138: 'anonymous_logged_in' imported but unused
     138: 'with_anonymous_login' imported but unused
     157: 'launchpadlib_for' imported but unused
     157: 'launchpadlib_credentials_for' imported but unused
     138: 'with_person_logged_in' imported but unused
     138: 'person_logged_in' imported but unused
     157: 'oauth_access_token_for' imported but unused
     138: 'login_celebrity' imported but unused
     138: 'with_celebrity_logged_in' imported but unused
     156: 'test_tales' imported but unused
     138: 'celebrity_logged_in' imported but unused
     138: 'run_with_login' imported but unused
     138: 'is_logged_in' imported but unused
     138: 'login_team' imported but unused
     138: 'login_person' imported but unused
     138: 'login_as' imported but unused
-- 
https://code.launchpad.net/~abentley/launchpad/no-queue-rescore/+merge/45560
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/no-queue-rescore into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipebuild.py'
--- lib/lp/code/browser/sourcepackagerecipebuild.py	2010-11-26 00:29:09 +0000
+++ lib/lp/code/browser/sourcepackagerecipebuild.py	2011-01-07 20:16:22 +0000
@@ -43,6 +43,7 @@
     BuildStatus.SUPERSEDED,
     BuildStatus.FAILEDTOUPLOAD,)
 
+
 class SourcePackageRecipeBuildNavigation(Navigation, FileNavigationMixin):
 
     usedfor = ISourcePackageRecipeBuild
@@ -163,6 +164,13 @@
 
     page_title = label = "Rescore build"
 
+    def __call__(self):
+        if self.context.buildqueue_record is not None:
+            return super(SourcePackageRecipeBuildRescoreView, self).__call__()
+        self.request.response.addWarningNotification(
+            'Cannot rescore this build because it is not queued.')
+        self.request.response.redirect(canonical_url(self.context))
+
     @property
     def cancel_url(self):
         return canonical_url(self.context)

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2011-01-07 20:16:22 +0000
@@ -28,6 +28,7 @@
     BrowserTestCase,
     login,
     logout,
+    person_logged_in,
     )
 
 
@@ -208,6 +209,38 @@
             LinkNotFoundError,
             browser.getLink, 'Rescore build')
 
+    def test_rescore_build_wrong_state_stale_link(self):
+        """Show sane error if you attempt to rescore a non-queued build.
+
+        This is the case where the user has a stale link that they click on.
+        """
+        build = self.factory.makeSourcePackageRecipeBuild()
+        build.cancelBuild()
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        index_url = canonical_url(build)
+        browser = self.getViewBrowser(build, '+rescore', user=experts)
+        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()
+        experts = getUtility(ILaunchpadCelebrities).bazaar_experts.teamowner
+        index_url = canonical_url(build)
+        browser = self.getViewBrowser(build, '+rescore', user=experts)
+        with person_logged_in(experts):
+            build.cancelBuild()
+        browser.getLink('Rescore build').click()
+        self.assertEqual(index_url, browser.url)
+        self.assertIn(
+            'Cannot rescore this build because it is not queued.',
+            browser.contents)
+
     def test_builder_history(self):
         build = self.makeRecipeBuild()
         Store.of(build).flush()

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-12-30 23:45:34 +0000
+++ lib/lp/testing/__init__.py	2011-01-07 20:16:22 +0000
@@ -105,6 +105,7 @@
 from zope.testing.testrunner.runner import TestResult as ZopeTestResult
 
 from canonical.config import config
+from canonical.database.constants import DEFAULT
 from canonical.launchpad.webapp import (
     canonical_url,
     errorlog,
@@ -513,7 +514,7 @@
         # Evaluate the log when called, not later, to permit the librarian to
         # be shutdown before the detail is rendered.
         chunks = fixture.logChunks()
-        content = Content(UTF8_TEXT, lambda:chunks)
+        content = Content(UTF8_TEXT, lambda: chunks)
         self.addDetail('librarian-log', content)
 
     def setUp(self):
@@ -723,17 +724,21 @@
         self.user = self.factory.makePerson(password='test')
 
     def getViewBrowser(self, context, view_name=None, no_login=False,
-                       rootsite=None):
-        login(ANONYMOUS)
-        url = canonical_url(context, view_name=view_name ,rootsite=rootsite)
-        logout()
+                       rootsite=None, user=DEFAULT):
+        if user is DEFAULT:
+            user = self.user
         if no_login:
+            user = None
+        login(ANONYMOUS)
+        url = canonical_url(context, view_name=view_name, rootsite=rootsite)
+        logout()
+        if user is None:
             from canonical.launchpad.testing.pages import setupBrowser
             browser = setupBrowser()
             browser.open(url)
             return browser
         else:
-            return self.getUserBrowser(url, self.user)
+            return self.getUserBrowser(url, user)
 
     def getMainContent(self, context, view_name=None, rootsite=None,
                        no_login=False):
@@ -936,9 +941,9 @@
         features.per_thread.features = old_features
 
 
-# XXX: This doesn't seem like a generically-useful testing function. Perhaps
-# it should go in a sub-module or something? -- jml
 def get_lsb_information():
+    # XXX: This doesn't seem like a generically-useful testing function.
+    # Perhaps it should go in a sub-module or something? -- jml
     """Returns a dictionary with the LSB host information.
 
     Code stolen form /usr/bin/lsb-release
@@ -1012,9 +1017,9 @@
     return " ".join(string.split())
 
 
-# XXX: This doesn't seem to be a generically useful testing function. Perhaps
-# it should go into a sub-module? -- jml
 def map_branch_contents(branch):
+    # XXX: This doesn't seem to be a generically useful testing function.
+    # Perhaps it should go into a sub-module? -- jml
     """Return all files in branch at `branch_url`.
 
     :param branch_url: the URL for an accessible branch.