Merge lp:~wgrant/launchpad/html-wtf into lp:launchpad

[William Grant]

[r=stevenk][no-qa] Fix various bits of invalid HTML in the base template.

This branch fixes some invalid HTML in the base template, with a convenient improvement as fallout.

 - Fixed the size attribute of the front page search box from "25%" to "25", since it's not a percentage. Browsers just ignored the %.
 - Removed the empty <noscript> from base-layout.pt. This was invalid markup which caused mechanise to fail to detect <meta http-equiv="Refresh"> elements, so two apport bug filing tests needed adjusting to prevent the redirect.
 - Fixed the HTML for the debug timeline, and moved the conditional up a level to stop the anchor from being rendered even when the timeline was not meant to be shown.
 - Made the debug timeline valid by moving it into the body. This brought it up to before the summarize_requests comment, causing the UserRequestOops ID to never show up in the comment. I adjusted the UserRequestOops code to function correctly when called multiple times, which has the handy side-effect of showing the OOPS ID in the visible_render_time.

[Steve Kowalik (stevenk)]

Aside from the disgusting use of structure() in tal:replace thanks to IE (but it isn't your fault), this looks great.

review: Approve (code)

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2012-02-15 22:09:43 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2012-03-07 10:24:27 +0000
@@ -474,25 +474,24 @@
 
 
 <metal:debug-timeline define-macro="debug-timeline">
-  <a name="debug_timeline"
-    id="debug_timeline"
-    class="hidden"
-    >
-    <table
-      tal:condition="request/features/visible_render_time"
-      tal:define="timeline_actions modules/lp.services.webapp.adapter/get_timeline_actions"
-      class="debug-timeline listing"
-      >
+  <a tal:condition="request/features/visible_render_time"
+     tal:define="timeline_actions modules/lp.services.webapp.adapter/get_timeline_actions"
+     id="debug_timeline" class="hidden">
+    <table class="debug-timeline listing">
       <thead>
-	<th>Duration</th>
-	<th>Action</th>
+        <tr>
+          <th>Duration</th>
+          <th>Action</th>
+        </tr>
       </thead>
-      <tr tal:repeat="action timeline_actions">
-	<td class="amount" tal:content="action/duration/fmt:millisecondduration"/>
-	<td style="font-family: monospace; text-align: left;">
-          <pre class="wrap"><span class="action-category" tal:content="action/category"/>: <span class="action-details" tal:content="action/detail"/></pre>
-	</td>
-      </tr>
+      <tbody>
+        <tr tal:repeat="action timeline_actions">
+          <td class="amount" tal:content="action/duration/fmt:millisecondduration"/>
+          <td style="font-family: monospace; text-align: left;">
+            <pre class="wrap"><span class="action-category" tal:content="action/category"/>: <span class="action-details" tal:content="action/detail"/></pre>
+          </td>
+        </tr>
+      </tbody>
     </table>
   </a>
 </metal:debug-timeline>

=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt	2012-02-24 04:54:53 +0000
+++ lib/lp/app/templates/base-layout.pt	2012-03-07 10:24:27 +0000
@@ -43,15 +43,6 @@
       html, body {background-image: url(/@@/demo) !important;}
     </style>
 
-    <tal:comment condition="nothing">
-      Removing the below <noscript></noscript> snippet makes two tests fail
-      with redirection problems:
-        xx-ubuntu-filebug.txt
-        xx-bug-reporting-tools.txt
-      No time to look into it now, so keeping it in. XXX Danilo 20110715
-    </tal:comment>
-    <noscript></noscript>
-
     <tal:view condition="not: view/macro:is-page-contentless">
       <meta name="description"
         tal:condition="view/page_description | nothing"
@@ -176,6 +167,21 @@
 
     <metal:lp-client-cache
       use-macro="context/@@+base-layout-macros/lp-client-cache" />
+    <metal:debug-timeline
+      use-macro="context/@@+base-layout-macros/debug-timeline" />
+    <tal:comment
+        tal:condition="request/features/visible_render_time"
+        define="render_time modules/lp.services.webapp.adapter/summarize_requests;"
+        replace='structure string:&lt;script type="text/javascript"&gt;
+      var render_time = "${render_time}";
+      LPJS.use("node", "lp.ajax_log" , function(Y) {
+        Y.on("domready", function() {
+          var node = Y.one("#rendertime");
+          node.set("innerHTML", render_time);
+          var ajax_log = new Y.lp.ajax_log();
+        });
+      });
+    &lt;/script&gt;' />
   </body>
 
 <tal:template>
@@ -196,24 +202,5 @@
 
     --&gt;" />
 </tal:template>
-
-
-<metal:debug-timeline
-  use-macro="context/@@+base-layout-macros/debug-timeline" />
-
-<tal:comment
-    tal:condition="request/features/visible_render_time"
-    define="render_time modules/lp.services.webapp.adapter/summarize_requests;"
-    replace='structure string:&lt;script type="text/javascript"&gt;
-  var render_time = "${render_time}";
-  LPJS.use("node", "lp.ajax_log" , function(Y) {
-    Y.on("domready", function() {
-      var node = Y.one("#rendertime");
-      node.set("innerHTML", render_time);
-      var ajax_log = new Y.lp.ajax_log();
-    });
-  });
-&lt;/script&gt;' />
-
 </html>
 </metal:page>

=== modified file 'lib/lp/app/templates/root-index.pt'
--- lib/lp/app/templates/root-index.pt	2012-02-24 04:44:55 +0000
+++ lib/lp/app/templates/root-index.pt	2012-03-07 10:24:27 +0000
@@ -136,7 +136,7 @@
               xml:lang="en" lang="en" dir="ltr"
               tal:attributes="action string:${rooturl}+search"
               method="get" accept-charset="UTF-8">
-              <input id="text" type="text" name="field.text" size="25%" />
+              <input id="text" type="text" name="field.text" size="25" />
               <input id="search" type="submit" value="Search Launchpad" />
             </form>
             <script type="text/javascript">

=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-tools.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-tools.txt	2011-12-29 05:29:36 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-tools.txt	2012-03-07 10:24:27 +0000
@@ -61,22 +61,26 @@
 The most common case will be that the user is sent to the guided
 +filebug page and the user goes through the workflow there.
 
-    >>> filebug_url = (
-    ...    'http://launchpad.dev/ubuntu/+source/mozilla-firefox/+filebug/'
-    ...     '%s' % blob_token)
-    >>> user_browser.open(filebug_url)
+    >>> filebug_host = 'launchpad.dev'
+    >>> filebug_path = (
+    ...    '/ubuntu/+source/mozilla-firefox/+filebug/%s' % blob_token)
+    >>> filebug_url = 'http://%s%s' % (filebug_host, filebug_path)
+    >>> contents = str(http(
+    ...     "GET %s HTTP/1.1\nHostname: %s\n"
+    ...     "Authorization: Basic [e-mail]:test\n\n"
+    ...     % (filebug_path, filebug_host)))
 
 At first, the user will be shown a message telling them that the extra
 data is being processed.
 
-    >>> for message in find_tags_by_class(user_browser.contents, 'message'):
+    >>> for message in find_tags_by_class(contents, 'message'):
     ...     print message.renderContents()
     Please wait while bug data is processed. This page will refresh
     every 10 seconds until processing is complete.
 
 The page header contains a 10-second meta refresh tag.
 
-    >>> '<meta http-equiv="refresh" content="10"' in user_browser.contents
+    >>> '<meta http-equiv="refresh" content="10"' in contents
     True
 
 Once the data has been processed, the +filebug process can continue as

=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-ubuntu-filebug.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-ubuntu-filebug.txt	2011-12-29 05:29:36 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-ubuntu-filebug.txt	2012-03-07 10:24:27 +0000
@@ -71,6 +71,14 @@
     ...     extra_filebug_data, 'not/important', 'not.important')
     >>> anon_browser.getControl(name='FORM_SUBMIT').click()
     >>> blob_token = anon_browser.headers['X-Launchpad-Blob-Token']
+    >>> from lp.bugs.interfaces.apportjob import IProcessApportBlobJobSource
+    >>> login('[e-mail]')
+    >>> job = getUtility(IProcessApportBlobJobSource).getByBlobUUID(
+    ...     blob_token)
+    >>> job.job.start()
+    >>> job.run()
+    >>> job.job.complete()
+    >>> logout()
     >>> filebug_url = (
     ...    'http://launchpad.dev/ubuntu/+source/mozilla-firefox/+filebug/'
     ...     '%s' % blob_token)

=== modified file 'lib/lp/services/webapp/adapter.py'
--- lib/lp/services/webapp/adapter.py	2011-12-30 08:13:14 +0000
+++ lib/lp/services/webapp/adapter.py	2012-03-07 10:24:27 +0000
@@ -226,11 +226,11 @@
     timeline = get_request_timeline(request)
     from lp.services.webapp.errorlog import (
         maybe_record_user_requested_oops)
-    oopsid = maybe_record_user_requested_oops()
-    if oopsid is None:
+    maybe_record_user_requested_oops()
+    if request.oopsid is None:
         oops_str = ""
     else:
-        oops_str = " %s" % oopsid
+        oops_str = " %s" % request.oopsid
     log = "%s queries/external actions issued in %.2f seconds%s" % (
         len(timeline.actions), secs, oops_str)
     return log

=== modified file 'lib/lp/services/webapp/errorlog.py'
--- lib/lp/services/webapp/errorlog.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/errorlog.py	2012-03-07 10:24:27 +0000
@@ -542,18 +542,16 @@
 def maybe_record_user_requested_oops():
     """If an OOPS has been requested, report one.
 
-    :return: The oopsid of the requested oops.  Returns None if an oops was
-        not requested, or if there is already an OOPS.
+    It will be stored in request.oopsid.
     """
     request = get_current_browser_request()
-    # If there is no request, or there is an oops already, then return.
-    if (request is None or
-        request.oopsid is not None or
-        not request.annotations.get(LAZR_OOPS_USER_REQUESTED_KEY, False)):
-        return None
-    globalErrorUtility.raising(
-        (UserRequestOops, UserRequestOops(), None), request)
-    return request.oopsid
+    # If there's a request and no existing OOPS, but an OOPS has been
+    # requested, record one.
+    if (request is not None
+        and request.oopsid is None
+        and request.annotations.get(LAZR_OOPS_USER_REQUESTED_KEY, False)):
+        globalErrorUtility.raising(
+            (UserRequestOops, UserRequestOops(), None), request)
 
 
 class OopsNamespace(view):

=== modified file 'lib/lp/services/webapp/tests/test_user_requested_oops.py'
--- lib/lp/services/webapp/tests/test_user_requested_oops.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/tests/test_user_requested_oops.py	2012-03-07 10:24:27 +0000
@@ -37,20 +37,28 @@
     def test_none_requested(self):
         # If an oops was not requested, then maybe_record_user_requested_oops
         # does not record an oops.
-        oops_id = maybe_record_user_requested_oops()
-        self.assertIs(None, oops_id)
         request = get_current_browser_request()
+        maybe_record_user_requested_oops()
         self.assertIs(None, request.oopsid)
 
     def test_annotation_key(self):
-        # The request for an oops is stored in the request annotations.  If a
-        # user request oops is recorded, the oops id is returned, and also
-        # stored in the request.
-        request = get_current_browser_request()
-        request.annotations[LAZR_OOPS_USER_REQUESTED_KEY] = True
-        oops_id = maybe_record_user_requested_oops()
-        self.assertIsNot(None, oops_id)
-        self.assertEqual(oops_id, request.oopsid)
+        # The request for an oops is stored in the request annotations.
+        # If a user request oops is recorded, the oops id is stored in
+        # the request.
+        request = get_current_browser_request()
+        request.annotations[LAZR_OOPS_USER_REQUESTED_KEY] = True
+        self.assertIs(None, request.oopsid)
+        maybe_record_user_requested_oops()
+        self.assertIsNot(None, request.oopsid)
+
+    def test_multiple_calls(self):
+        # Asking to record the OOPS twice just returns the same ID.
+        request = get_current_browser_request()
+        request.annotations[LAZR_OOPS_USER_REQUESTED_KEY] = True
+        maybe_record_user_requested_oops()
+        orig_oops_id = request.oopsid
+        maybe_record_user_requested_oops()
+        self.assertEqual(orig_oops_id, request.oopsid)
 
     def test_existing_oops_stops_user_requested(self):
         # If there is already an existing oops id in the request, then the
@@ -58,8 +66,7 @@
         request = get_current_browser_request()
         request.oopsid = "EXISTING"
         request.annotations[LAZR_OOPS_USER_REQUESTED_KEY] = True
-        oops_id = maybe_record_user_requested_oops()
-        self.assertIs(None, oops_id)
+        maybe_record_user_requested_oops()
         self.assertEqual("EXISTING", request.oopsid)
 
     def test_OopsNamespace_traverse(self):

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-builder-page.txt'
--- lib/lp/soyuz/stories/soyuz/xx-builder-page.txt	2012-01-15 11:06:57 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-builder-page.txt	2012-03-07 10:24:27 +0000
@@ -4,6 +4,7 @@
 builder state. In the sampledata, the builder 'bob' is building
 'mozilla-firefox'.
 
+    >>> anon_browser.mech_browser.set_handle_equiv(False)
     >>> anon_browser.open("http://launchpad.dev/builders")
     >>> anon_browser.getLink("bob").click()
 
@@ -27,6 +28,7 @@
 timezone. This way they can easily find out if they are reading
 outdated information.
 
+    >>> user_browser.mech_browser.set_handle_equiv(False)
     >>> user_browser.open(anon_browser.url)
     >>> print extract_text(find_portlet(
     ...     user_browser.contents, 'View full history Current status'))
@@ -68,6 +70,7 @@
 
     >>> cprov_browser = setupBrowser(
     ...     auth='Basic [e-mail]:test')
+    >>> cprov_browser.mech_browser.set_handle_equiv(False)
 
     >>> cprov_browser.open('http://launchpad.dev/builders')
     >>> cprov_browser.getLink('bob').click()
@@ -193,7 +196,7 @@
 when they judge it convenient, for instance, when the builder present
 transient failures or is used for another purpose.
 
-    >>> cprov_browser.open('http://launchpad.dev/+builds')
+    >>> cprov_browser.open('http://launchpad.dev/builders')
     >>> cprov_browser.getLink('bob').click()
     >>> print backslashreplace(cprov_browser.title)
     Bob The Builder : Build Farm

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt'
--- lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt	2011-12-23 23:44:59 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-buildfarm-index.txt	2012-03-07 10:24:27 +0000
@@ -4,6 +4,7 @@
 link to it yet because we are not yet sure of the benefits of doing
 this, since the audience of this page is still restricted.
 
+    >>> anon_browser.mech_browser.set_handle_equiv(False)
     >>> anon_browser.open('http://launchpad.dev/+builds')
 
 The BuildFarm contains a list of all builders registered in Launchpad
@@ -120,6 +121,7 @@
 
 Administrators can create new builders.
 
+    >>> admin_browser.mech_browser.set_handle_equiv(False)
     >>> admin_browser.open("http://launchpad.dev/+builds/+index")
 
     >>> admin_browser.getLink("Register a new build machine").click()

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-builds-pages.txt'
--- lib/lp/soyuz/stories/soyuz/xx-builds-pages.txt	2012-01-06 11:08:30 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-builds-pages.txt	2012-03-07 10:24:27 +0000
@@ -69,7 +69,8 @@
 
 For Builder, same as Distribution:
 
-    >>> anon_browser.open("http://launchpad.dev/+builds/bob")
+    >>> anon_browser.mech_browser.set_handle_equiv(False)
+    >>> anon_browser.open("http://launchpad.dev/builders/bob")
     >>> anon_browser.getLink("View full history").click()
     >>> print anon_browser.title
     Build history : Bob The Builder : Build Farm

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-private-builds.txt'
--- lib/lp/soyuz/stories/soyuz/xx-private-builds.txt	2012-01-15 11:06:57 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-private-builds.txt	2012-03-07 10:24:27 +0000
@@ -65,6 +65,7 @@
 The status shown to an anonymous user hides the private source it is
 building.
 
+    >>> anon_browser.mech_browser.set_handle_equiv(False)
     >>> anon_browser.open("http://launchpad.dev/+builds/frog")
     >>> print extract_text(find_main_content(anon_browser.contents))
     The frog builder...
@@ -74,6 +75,7 @@
 
 Launchpad Administrators are allowed to see the build:
 
+    >>> admin_browser.mech_browser.set_handle_equiv(False)
     >>> admin_browser.open("http://launchpad.dev/+builds/frog")
     >>> print extract_text(find_main_content(admin_browser.contents))
     The frog builder...
@@ -85,6 +87,7 @@
 Buildd Administrators are not allowed to see the build in the portlet:
 
     >>> name12_browser = setupBrowser(auth="Basic [e-mail]:test")
+    >>> name12_browser.mech_browser.set_handle_equiv(False)
     >>> name12_browser.open("http://launchpad.dev/+builds/frog")
     >>> print extract_text(find_main_content(name12_browser.contents))
     The frog builder...
@@ -96,6 +99,7 @@
 
     >>> cprov_browser = setupBrowser(
     ...     auth="Basic [e-mail]:test")
+    >>> cprov_browser.mech_browser.set_handle_equiv(False)
     >>> cprov_browser.open("http://launchpad.dev/+builds/frog")
     >>> print extract_text(find_main_content(cprov_browser.contents))
     The frog builder...
@@ -295,7 +299,6 @@
 
 Now the anonymous user can see the build:
 
-    >>> anon_browser = setupBrowser()
     >>> anon_browser.open("http://launchpad.dev/+builds")
     >>> print extract_text(find_main_content(anon_browser.contents))
     The Launchpad build farm
@@ -307,6 +310,7 @@
 
 Any other logged-in user will also see the build:
 
+    >>> browser.mech_browser.set_handle_equiv(False)
     >>> browser.open("http://launchpad.dev/+builds")
     >>> print extract_text(find_main_content(browser.contents))
     The Launchpad build farm

=== modified file 'lib/lp/translations/stories/buildfarm/xx-build-summary.txt'
--- lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2012-02-09 23:09:36 +0000
+++ lib/lp/translations/stories/buildfarm/xx-build-summary.txt	2012-03-07 10:24:27 +0000
@@ -71,6 +71,7 @@
 The job's summary shows that what type of job this is.  It also links
 to the branch.
 
+    >>> user_browser.mech_browser.set_handle_equiv(False)
     >>> user_browser.open(builder_page)
     >>> print extract_text(find_build_summary(user_browser))
     Working on TranslationTemplatesBuildJob for branch ...