Merge lp:~wallyworld/launchpad/improved-broken-link-handling into lp:launchpad

[Ian Booth]

Add invalid-link style to invalid lp branch short links and prevent click through

Summary
------------
Render lp: shortcuts such that any invalid ones are shown as grey and their onclick() is suppressed.
This branch supports processing +branch links.

Implementation detail
----------------------------
When the links are harvested via the linkify_substitution() api, they are assigned a class "short-branch-link". AN onload method harvests such links and makes an ajax call to have them validated. Any invalid links have their class changed to "invalid-link" and the css styling takes care of the final rendering.

Tests
------

New tests:
lp/app/browser/tests/test_linkchecker.py
lp/code/windmill/tests/test_branch_broken_links.py

The windmill test is broken because windmill has an issue making the ajax call. Manual testing shows the functionality works as expected.

Lint
----

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css.in
  lib/lp/app/configure.zcml
  lib/lp/app/browser/configure.zcml
  lib/lp/app/browser/linkchecker.py
  lib/lp/app/browser/stringformatter.py
  lib/lp/app/browser/tests/test_linkchecker.py
  lib/lp/app/doc/displaying-paragraphs-of-text.txt
  lib/lp/app/javascript/lp-links.js
  lib/lp/app/templates/base-layout-macros.pt
  lib/lp/code/windmill/tests/test_branch_broken_links.py

./lib/canonical/launchpad/icing/style-3-0.css.in
     144: Line exceeds 78 characters.
    1324: Line exceeds 78 characters.
    1941: Line exceeds 78 characters.
    1957: Line exceeds 78 characters.
    1961: Line exceeds 78 characters.
    1969: Line exceeds 78 characters.
    1973: Line exceeds 78 characters.
    2001: Line exceeds 78 characters.
    2017: Line exceeds 78 characters.
    2025: Line exceeds 78 characters.
    2029: Line exceeds 78 characters.
    2033: Line exceeds 78 characters.
    2037: Line exceeds 78 characters.
    2041: Line exceeds 78 characters.
    2045: Line exceeds 78 characters.
    2058: Line exceeds 78 characters.
    2062: Line exceeds 78 characters.
    2094: Line exceeds 78 characters.
    2106: Line exceeds 78 characters.
    2110: Line exceeds 78 characters.
    2114: Line exceeds 78 characters.
    2122: Line exceeds 78 characters.
    2126: Line exceeds 78 characters.
    2134: Line exceeds 78 characters.
    2162: Line exceeds 78 characters.
    2166: Line exceeds 78 characters.
    2194: Line exceeds 78 characters.
    2202: Line exceeds 78 characters.
    2207: Line exceeds 78 characters.
    2211: Line exceeds 78 characters.
    2216: Line exceeds 78 characters.
    2220: Line exceeds 78 characters.
    2224: Line exceeds 78 characters.
    2228: Line exceeds 78 characters.
    2232: Line exceeds 78 characters.
    2284: Line exceeds 78 characters.
    2292: Line exceeds 78 characters.
    2296: Line exceeds 78 characters.
    2304: Line exceeds 78 characters.
    2316: Line exceeds 78 characters.
    2324: Line exceeds 78 characters.
    2328: Line exceeds 78 characters.
    2332: Line exceeds 78 characters.
    2340: Line exceeds 78 characters.
    2344: Line exceeds 78 characters.
    2348: Line exceeds 78 characters.
    2352: Line exceeds 78 characters.
    2356: Line exceeds 78 characters.
    2365: Line exceeds 78 characters.
    2373: Line exceeds 78 characters.
    2505: Line exceeds 78 characters.
    2506: Line exceeds 78 characters.
    2575: Line exceeds 78 characters.
    2576: Line exceeds 78 characters.
./lib/lp/app/browser/stringformatter.py
     409: E501 line too long (90 characters)
     413: E501 line too long (88 characters)
     409: Line exceeds 78 characters.
     413: Line exceeds 78 characters.
./lib/lp/app/doc/displaying-paragraphs-of-text.txt
       1: narrative uses a moin header.
       7: narrative uses a moin header.
      32: want exceeds 78 characters.
      33: want exceeds 78 characters.
      43: source exceeds 78 characters.
      49: want exceeds 78 characters.
      83: want exceeds 78 characters.
      84: want exceeds 78 characters.
      86: want exceeds 78 characters.
      87: want exceeds 78 characters.
     102: want exceeds 78 characters.
     103: want exceeds 78 characters.
     104: want exceeds 78 characters.
     105: want exceeds 78 characters.
     106: want exceeds 78 characters.
     107: want exceeds 78 characters.
     110: narrative uses a moin header.
     138: source exceeds 78 characters.
     155: want exceeds 78 characters.
     156: want exceeds 78 characters.
     157: want exceeds 78 characters.
     158: want exceeds 78 characters.
     159: want exceeds 78 characters.
     160: want exceeds 78 characters.
     161: want exceeds 78 characters.
     162: want exceeds 78 characters.
     163: want exceeds 78 characters.
     164: want exceeds 78 characters.
     165: want exceeds 78 characters.
     166: want exceeds 78 characters.
     167: want exceeds 78 characters.
     168: want exceeds 78 characters.
     169: want exceeds 78 characters.
     170: want exceeds 78 characters.
     171: want exceeds 78 characters.
     172: want exceeds 78 characters.
     173: want exceeds 78 characters.
     174: want exceeds 78 characters.
     175: want exceeds 78 characters.
     176: want exceeds 78 characters.
     177: want exceeds 78 characters.
     178: want exceeds 78 characters.
     179: want exceeds 78 characters.
     180: want exceeds 78 characters.
     181: want exceeds 78 characters.
     182: want exceeds 78 characters.
     183: want exceeds 78 characters.
     184: want exceeds 78 characters.
     185: want exceeds 78 characters.
     186: want exceeds 78 characters.
     187: want exceeds 78 characters.
     188: want exceeds 78 characters.
     189: want exceeds 78 characters.
     190: want exceeds 78 characters.
     191: want exceeds 78 characters.
     204: narrative uses a moin header.
     314: narrative uses a moin header.
     332: want exceeds 78 characters.
     343: narrative uses a moin header.
     361: want exceeds 78 characters.
     363: want exceeds 78 characters.
     387: narrative uses a moin header.
     424: want exceeds 78 characters.
     430: want exceeds 78 characters.
     434: want exceeds 78 characters.
     438: want exceeds 78 characters.
     442: want exceeds 78 characters.
     472: narrative uses a moin header.
     487: narrative exceeds 78 characters.
./lib/lp/code/windmill/tests/test_branch_broken_links.py
      90: E501 line too long (102 characters)
      96: E501 line too long (96 characters)
     123: E302 expected 2 blank lines, found 3
      90: Line exceeds 78 characters.
      96: Line exceeds 78 characters.

[Jeroen T. Vermeulen (jtv)]

Just a note, not a review: why so much 2009 and so little 2010 in the copyright notices? :-)

[Ian Booth (wallyworld)]

There's only one real instance, due to a cut'n'paste :-) The other one
was a file that was deleted and then reverted and bzr has treated it as
a delete and then an add, so it is a false positive.

On 21/10/10 19:47, Jeroen T. Vermeulen wrote:
> Just a note, not a review: why so much 2009 and so little 2010 in the copyright notices? :-)

[Gavin Panella (allenap)]

This is a nice piece of functionality. I have a lot of comments but
most if not all of them are trivial or minor. I'm going to ask someone
else who has more up-to-date experience with Javascript in Launchpad
to give this a look too.

[1]

+ <browser:page
+ for="*"
+ name="+check-links"
+ class="lp.app.browser.linkchecker.LinkCheckerAPI"
+ permission="zope.Public"
+ />

Although the indentation is inconsistent in that file, and although it
doesn't matter very much, can you make this consistent with what's
already there?

[2]

+from zope.component._api import getUtility

getUtility is normally imported directly from zope.component.

[3]

+from zope.component._api import getUtility
+
+from lp.code.interfaces.branchlookup import IBranchLookup
+
+__metaclass__ = type
+
+__all__ = [
+ 'LinkCheckerAPI',
+ ]
+
+import simplejson

I think __metaclass__ and __all__ normally appear first in Launchpad
source, followed by imports. I think utilities/format-imports expects
that too (and you should also run this on your changed files, or use
utilities/format-new-and-modified-imports -r submit: as a convenience).

[4]

+ """ Validates Launchpad shortcut links.

s/" V/"V/

[5]

+ result['invalid_'+link_type]=invalid_links

s/[+=]/ \1 /

[6]

+ path = link.strip('/')[len('+branch/'):]

Is it possible that the branch link does not begin with +branch? I'm
not used to seeing branches with +branch in their URLs.

[7]

+ try:
+ branch_lookup.getByLPPath(path)
+ except:
+ invalid_links.append(link)

Don't use default except clauses like this. Catch Exception if
absolutely necessary, or, preferably, catch just those exceptions that
could be raised.

[8]

+ return '<a href="%s" class="%s">%s</a>%s' % (
                 cgi.escape(url, quote=True),
+ "branch-short-link",

Just chuck "branch-short-link" into the format string rather than
interpolate.

[9]

+ self.assertEqual(len(invalid_links), len(links_to_check))
+ self.assertEqual(set(invalid_links), set(links_to_check))

Only the second test is needed. In fact, if the first test fails the
error message will be less informative than if the second fails (which
it will if the first fails).

[10]

+ def invoke_branch_link_checker(
+ self, valid_branch_urls=dict(), invalid_branch_urls=dict()):

Default arguments must not be mutable. Use None as a default then
check for that in the method.

[11]

Is it likely that we hit any query length problems when requesting the
status of a page with many links?

[12]

+ // Get any links of the specified link_class and put store them as the
+ // specified link_type in the specified links_holder

s/put store/store/

[13]

+function harvest_links(Y, links_holder, link_class, link_type) {

I think this function would be cleaner if it returned the link_info
array instead of putting it into links_holder. That way it need only
accept two arguments, Y and link_class. Callers should check if the
returned array is empty.

[14]

Also, given [13] and the presence of the YUI collection module,
harvest_links() could be much shorter ...

review: Needs Fixing

[Ian Booth (wallyworld)]

Hi

Thanks for the most excellent review. It's the most thorough one I've
had. I've addressed most of the issues except for the one item - see my
comments. The import issues were due to the "curse" of having an IDE
that auto imports missing references and it guessed wrong. A small price
to pay given all the other great stuff it does.

>
> I think __metaclass__ and __all__ normally appear first in Launchpad
> source, followed by imports. I think utilities/format-imports expects
> that too (and you should also run this on your changed files, or use
> utilities/format-new-and-modified-imports -r submit: as a convenience).
>

Thanks for the tip - I had no idea those scripts existed.

>
> [6]
>
> + path = link.strip('/')[len('+branch/'):]
>
> Is it possible that the branch link does not begin with +branch? I'm
> not used to seeing branches with +branch in their URLs.
>

No, all branch links processed by this new functionality begin with
+branch. See line 276 of stringformatter.py:

    url = '/+branch/%s' % path

>
> [11]
>
> Is it likely that we hit any query length problems when requesting the
> status of a page with many links?
>

I don't think so? A post operation is used so I thought that would have
it covered?

>
> [13]
>
> +function harvest_links(Y, links_holder, link_class, link_type) {
>
> I think this function would be cleaner if it returned the link_info
> array instead of putting it into links_holder. That way it need only
> accept two arguments, Y and link_class. Callers should check if the
> returned array is empty.
>

The code has been written so support processing for than just one type
of link. At the moment, there's only branch links. But there will also
be bugs links etc. So the code will look like:

var links_to_check = {}

harvest_links(Y, links_to_check, 'branch-short-link', 'branch_links');
harvest_links(Y, links_to_check, 'bug-link', 'bugs_links');
etc

So we are building a dict mapping link_type_name->[hrefs]

The review comment would be valid if we only ever were going to process
the one type of link. I think the implementation as written is ok given
the above explanation?

>
> [14]
>
> Also, given [13] and the presence of the YUI collection module,
> harvest_links() could be much shorter and probably quicker:
>
> return Y.Array.unique(
> Y.all('.' + link_class).get('href'));
>
> To get the collection module you'll need to add the following in the
> right place (I added it at line 108) in base-layout-macros.pt:
>
> <script type="text/javascript"
> tal:attributes="src string:${yui}/collection/collection.js"></script>
>

I think Array.unique() is only available in YUI 3.2 and at the time of
writing we were not using that version yet. I originally tried to use
Array.unique() only to be disappointed :-(

>
> [15]
>
> + alert(msg);
>
> Really? Just checking :)

I wanted to give the user feedback why the "link" they just clicked on
didn't seem to work instead of just swallowing the onclick().

> [16]
>
> In TestBranchBugLinks (which should probably be renamed), instead of
> creating a new question via the browser, consider creating a new
> question...

[Gavin Panella (allenap)]

> Thanks for the most excellent review. It's the most thorough one
> I've had.

I hope it was useful...

I often do quite thorough reviews and I worry that I'm being too
thorough. Let me know if you ever feel that my reviews are
demotivating or too picky. That would mean I'm doing it wrong :)

[14]

> I think Array.unique() is only available in YUI 3.2 and at the time
> of writing we were not using that version yet. I originally tried to
> use Array.unique() only to be disappointed :-(

Okay. Fwiw, this should work now in Launchpad, though it's confusing
because Array.unique() is a static function (or whatever YUI calls
it), and is not available on instances of Array.

[19]

> *news flash* - problem "solved" wtf. By changing the test to
> creating a bug instead of a question, it works in windmill. Go
> figure. But it works now \o/

Hurrah! And weird. But mostly hurrah :)

review: Approve

[Ian Booth (wallyworld)]

On 22/10/10 19:11, Gavin Panella wrote:
> Review: Approve
>> Thanks for the most excellent review. It's the most thorough one
>> I've had.
>
> I hope it was useful...
>

Extremely. Thanks. Especially since I'm a new to lp and have figured
stuff out mostly by trial and error or by looking at what others have
done or reverse engineering etc.

> I often do quite thorough reviews and I worry that I'm being too
> thorough. Let me know if you ever feel that my reviews are
> demotivating or too picky. That would mean I'm doing it wrong :)
>
>

On the contrary, I see code reviews as a great learning opportunity,
regardless of how experienced the code author may or may not be. A 2nd
set of eyes always picks up issues that the author is often too close to
the code to see. Plus they are a great way to help share knowledge
across team boundaries.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2010-09-23 11:17:45 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2010-10-27 04:31:25 +0000
@@ -284,6 +284,12 @@
 a.help.icon, a.sprite.maybe.help {
     border: none;
 }
+a.invalid-link {
+    disabled: True;
+    color: #909090;
+    text-decoration: none;
+    cursor: default;
+    }
 img, a img {
     /* No border on images that are links. */
     border: none;

=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2010-10-15 01:27:04 +0000
+++ lib/lp/app/browser/configure.zcml	2010-10-27 04:31:25 +0000
@@ -98,6 +98,12 @@
       template="../templates/launchpad-search-form.pt"
       permission="zope.Public" />
 
+  <browser:page
+      for="*"
+      name="+check-links"
+      class="lp.app.browser.linkchecker.LinkCheckerAPI"
+      permission="zope.Public"/>
+
   <!-- TALES namespaces. -->
 
   <!-- TALES lp: namespace (should be deprecated) -->

=== added file 'lib/lp/app/browser/linkchecker.py'
--- lib/lp/app/browser/linkchecker.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/linkchecker.py	2010-10-27 04:31:25 +0000
@@ -0,0 +1,77 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# pylint: disable-msg=E0211,E0213
+
+__metaclass__ = type
+__all__ = [
+    'LinkCheckerAPI',
+    ]
+
+import simplejson
+from zope.component import getUtility
+
+from lp.app.errors import NotFoundError
+from lp.code.errors import (
+    CannotHaveLinkedBranch,
+    InvalidNamespace,
+    NoLinkedBranch,
+    NoSuchBranch,
+    )
+from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.registry.interfaces.product import InvalidProductName
+
+
+class LinkCheckerAPI:
+    """Validates Launchpad shortcut links.
+
+    This class provides the endpoint of an Ajax call to .../+check-links.
+    When invoked with a collection of links harvested from a page, it will
+    check the validity of each one and send a response containing those that
+    are invalid. Javascript on the page will set the style of invalid links to
+    something appropriate.
+
+    This initial implementation supports processing links like the following:
+        /+branch/foo/bar
+
+    The implementation can easily be extended to handle other forms by
+    providing a method to handle the link type extracted from the json
+    request.
+    """
+
+    def __init__(self, context, request):
+        # We currently only use the request.
+        # self.context = context
+        self.request = request
+
+        # Each link type has it's own validation method.
+        self.link_checkers = dict(
+            branch_links=self.check_branch_links,
+        )
+
+    def __call__(self):
+        result = {}
+        links_to_check_data = self.request.get('link_hrefs')
+        links_to_check = simplejson.loads(links_to_check_data)
+
+        for link_type in links_to_check:
+            links = links_to_check[link_type]
+            invalid_links = self.link_checkers[link_type](links)
+            result['invalid_'+link_type] = invalid_links
+
+        self.request.response.setHeader('Content-type', 'application/json')
+        return simplejson.dumps(result)
+
+    def check_branch_links(self, links):
+        """Check links of the form /+branch/foo/bar"""
+        invalid_links = []
+        branch_lookup = getUtility(IBranchLookup)
+        for link in links:
+            path = link.strip('/')[len('+branch/'):]
+            try:
+                branch_lookup.getByLPPath(path)
+            except (CannotHaveLinkedBranch, InvalidNamespace,
+                    InvalidProductName, NoLinkedBranch, NoSuchBranch,
+                    NotFoundError):
+                invalid_links.append(link)
+        return invalid_links

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-09-25 14:29:32 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-10-27 04:31:25 +0000
@@ -274,7 +274,9 @@
                 return FormattersAPI._linkify_bug_number(
                     lp_url, path, trailers)
             url = '/+branch/%s' % path
-            return '<a href="%s">%s</a>%s' % (
+            # Mark the links with a 'branch-short-link' class so they can be
+            # harvested and validated when the page is rendered.
+            return '<a href="%s" class="branch-short-link">%s</a>%s' % (
                 cgi.escape(url, quote=True),
                 cgi.escape(lp_url),
                 cgi.escape(trailers))

=== added file 'lib/lp/app/browser/tests/test_linkchecker.py'
--- lib/lp/app/browser/tests/test_linkchecker.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_linkchecker.py	2010-10-27 04:31:25 +0000
@@ -0,0 +1,83 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for the LinkCheckerAPI."""
+
+__metaclass__ = type
+
+from random import shuffle
+
+import simplejson
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.linkchecker import LinkCheckerAPI
+from lp.testing import TestCaseWithFactory
+
+
+class TestLinkCheckerAPI(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    BRANCH_URL_TEMPLATE = '/+branch/%s'
+
+    def check_invalid_links(self, result_json, link_type, invalid_links):
+        link_dict = simplejson.loads(result_json)
+        links_to_check = link_dict[link_type]
+        self.assertEqual(len(invalid_links), len(links_to_check))
+        self.assertEqual(set(invalid_links), set(links_to_check))
+
+    def make_valid_links(self):
+        branch = self.factory.makeProductBranch()
+        valid_branch_url = self.BRANCH_URL_TEMPLATE % branch.unique_name
+        product = self.factory.makeProduct()
+        product_branch = self.factory.makeProductBranch(product=product)
+        removeSecurityProxy(product).development_focus.branch = product_branch
+        valid_product_url = self.BRANCH_URL_TEMPLATE % product.name
+
+        return [
+            valid_branch_url,
+            valid_product_url,
+        ]
+
+    def make_invalid_links(self):
+        return [
+            self.BRANCH_URL_TEMPLATE % 'foo',
+            self.BRANCH_URL_TEMPLATE % 'bar',
+            ]
+
+    def invoke_branch_link_checker(
+        self, valid_branch_urls=None, invalid_branch_urls=None):
+        if valid_branch_urls is None:
+            valid_branch_urls = {}
+        if invalid_branch_urls is None:
+            invalid_branch_urls = {}
+
+        branch_urls = list(valid_branch_urls)
+        branch_urls.extend(invalid_branch_urls)
+        shuffle(branch_urls)
+
+        links_to_check = dict(branch_links=branch_urls)
+        link_json = simplejson.dumps(links_to_check)
+
+        request = LaunchpadTestRequest(link_hrefs=link_json)
+        link_checker = LinkCheckerAPI(object(), request)
+        result_json = link_checker()
+        self.check_invalid_links(
+            result_json, 'invalid_branch_links', invalid_branch_urls)
+
+    def test_only_valid_branchlinks(self):
+        branch_urls = self.make_valid_links()
+        self.invoke_branch_link_checker(valid_branch_urls=branch_urls)
+
+    def test_only_invalid_branchlinks(self):
+        branch_urls = self.make_invalid_links()
+        self.invoke_branch_link_checker(invalid_branch_urls=branch_urls)
+
+    def test_valid_and_invald_branchlinks(self):
+        valid_branch_urls = self.make_valid_links()
+        invalid_branch_urls = self.make_invalid_links()
+        self.invoke_branch_link_checker(
+            valid_branch_urls=valid_branch_urls,
+            invalid_branch_urls=invalid_branch_urls)

=== added file 'lib/lp/app/configure.zcml'
--- lib/lp/app/configure.zcml	1970-01-01 00:00:00 +0000
+++ lib/lp/app/configure.zcml	2010-10-27 04:31:25 +0000
@@ -0,0 +1,14 @@
+<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
+     GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure
+    xmlns="http://namespaces.zope.org/zope"
+    xmlns:browser="http://namespaces.zope.org/browser"
+    xmlns:i18n="http://namespaces.zope.org/i18n"
+    xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc"
+    xmlns:lp="http://namespaces.canonical.com/lp"
+    i18n_domain="launchpad">
+    <include
+        package=".browser"/>
+</configure>

=== removed file 'lib/lp/app/configure.zcml'
--- lib/lp/app/configure.zcml	2009-07-17 02:25:09 +0000
+++ lib/lp/app/configure.zcml	1970-01-01 00:00:00 +0000
@@ -1,14 +0,0 @@
-<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
-     GNU Affero General Public License version 3 (see the file LICENSE).
--->
-
-<configure
-    xmlns="http://namespaces.zope.org/zope"
-    xmlns:browser="http://namespaces.zope.org/browser"
-    xmlns:i18n="http://namespaces.zope.org/i18n"
-    xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc"
-    xmlns:lp="http://namespaces.canonical.com/lp"
-    i18n_domain="launchpad">
-    <include
-        package=".browser"/>
-</configure>

=== modified file 'lib/lp/app/doc/displaying-paragraphs-of-text.txt'
--- lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-10-27 04:31:25 +0000
@@ -357,17 +357,17 @@
     ...     'lp:///foo\n'
     ...     'lp:/foo\n')
     >>> print test_tales('foo/fmt:text-to-html', foo=text)
-    <p><a href="/+branch/~foo/bar/baz">lp:~foo/bar/baz</a><br />
-    <a href="/+branch/~foo/bar/bug-123">lp:~foo/bar/bug-123</a><br />
-    <a href="/+branch/~foo/+junk/baz">lp:~foo/+junk/baz</a><br />
-    <a href="/+branch/~foo/ubuntu/jaunty/evolution/baz">lp:~foo/ubuntu/jaunty/evolution/baz</a><br />
-    <a href="/+branch/foo/bar">lp:foo/bar</a><br />
-    <a href="/+branch/foo">lp:foo</a><br />
-    <a href="/+branch/foo">lp:foo</a>,<br />
-    <a href="/+branch/foo/bar">lp:foo/bar</a>.<br />
-    <a href="/+branch/foo/bar/baz">lp:foo/bar/baz</a><br />
-    <a href="/+branch/foo">lp:///foo</a><br />
-    <a href="/+branch/foo">lp:/foo</a></p>
+    <p><a href="/+branch/~foo/bar/baz" class="...">lp:~foo/bar/baz</a><br />
+    <a href="/+branch/~foo/bar/bug-123" class="...">lp:~foo/bar/bug-123</a><br />
+    <a href="/+branch/~foo/+junk/baz" class="...">lp:~foo/+junk/baz</a><br />
+    <a href="/+branch/~foo/ubuntu/jaunty/evolution/baz" class="...">lp:~foo/ubuntu/jaunty/evolution/baz</a><br />
+    <a href="/+branch/foo/bar" class="...">lp:foo/bar</a><br />
+    <a href="/+branch/foo" class="...">lp:foo</a><br />
+    <a href="/+branch/foo" class="...">lp:foo</a>,<br />
+    <a href="/+branch/foo/bar" class="...">lp:foo/bar</a>.<br />
+    <a href="/+branch/foo/bar/baz" class="...">lp:foo/bar/baz</a><br />
+    <a href="/+branch/foo" class="...">lp:///foo</a><br />
+    <a href="/+branch/foo" class="...">lp:/foo</a></p>
 
 Text that looks like a branch reference, but is followed only by digits is
 treated as a link to a bug.

=== added file 'lib/lp/app/javascript/lp-links.js'
--- lib/lp/app/javascript/lp-links.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/lp-links.js	2010-10-27 04:31:25 +0000
@@ -0,0 +1,105 @@
+/**
+ * Launchpad utilities for manipulating links.
+ *
+ * @module app
+ * @submodule links
+ */
+
+YUI.add('lp.app.links', function(Y) {
+
+    function harvest_links(Y, links_holder, link_class, link_type) {
+        // Get any links of the specified link_class and store them as the
+        // specified link_type in the specified links_holder
+        var link_info = new Array();
+        Y.all('.'+link_class).each(function(link) {
+            var href = link.getAttribute('href');
+            if( link_info.indexOf(href)<0 ) {
+                link_info.push(href);
+            }
+        });
+        if( link_info.length > 0 ) {
+            links_holder[link_type] = link_info;
+        }
+    }
+
+    function process_invalid_links(
+            Y, link_info, link_class, link_type, title) {
+        // We have a collection of invalid links possibly containing links of
+        // type link_type, so we need to remove the existing link_class,
+        // replace it with an invalid-link class, and set the link title.
+        var invalid_links = Y.Array(link_info['invalid_'+link_type]);
+
+        if( invalid_links.length > 0) {
+            Y.all('.'+link_class).each(function(link) {
+                var href = link.getAttribute('href');
+                if( invalid_links.indexOf(href)>=0 ) {
+                    var msg = title + href;
+                    link.removeClass(link_class);
+                    link.addClass('invalid-link');
+                    link.title = msg
+                    link.on('click', function(e) {
+                        e.halt();
+                        alert(msg);
+                    });
+                }
+            });
+        }
+    }
+
+    var links = Y.namespace('lp.app.links');
+
+    links.check_valid_lp_links = function() {
+        // Grabs any lp: style links on the page and checks that they are
+        // valid. Invalid ones have their class changed to "invalid-link".
+        // ATM, we only handle +branch links.
+
+        var links_to_check = {}
+
+        // We get all the links with defined css classes.
+        // At the moment, we just handle branch links, but in future...
+        harvest_links(Y, links_to_check, 'branch-short-link', 'branch_links');
+
+        // Do we have anything to do?
+        if( Y.Object.size(links_to_check) == 0 ) {
+            return;
+        }
+
+        // Get the final json to send
+        var json_link_info = Y.JSON.stringify(links_to_check);
+        var qs = '';
+        qs = LP.client.append_qs(qs, 'link_hrefs', json_link_info);
+
+        var config = {
+            on: {
+                failure: function(id, response, args) {
+                    // If we have firebug installed, log the error.
+                    if( console != undefined ) {
+                        console.log("Link Check Error: " + args + ': '
+                                + response.status + ' - ' +
+                                response.statusText + ' - '
+                                + response.responseXML);
+                    }
+                },
+                success: function(id, response) {
+                    var link_info = Y.JSON.parse(response.responseText)
+                    // ATM, we just handle branch links, but in future...
+                    process_invalid_links(Y, link_info, 'branch-short-link',
+                            'branch_links', "Invalid branch: ");
+                }
+            }
+        }
+        var uri = '+check-links';
+        var on = Y.merge(config.on);
+        var client = this;
+        var y_config = { method: "POST",
+                         headers: {'Accept': 'application/json'},
+                         on: on,
+                         'arguments': [client, uri],
+                         data: qs};
+        Y.io(uri, y_config);
+    };
+
+}, "0.1", {"requires": [
+    "base", "node", "io", "dom", "json"
+    ]});
+

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-10-25 13:16:10 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-10-27 04:31:25 +0000
@@ -175,6 +175,8 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/lp-mochi.js"></script>
     <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/app/lp-links.js"></script>
+    <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/dragscroll.js"></script>
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/picker.js"></script>
@@ -304,6 +306,13 @@
         // anywhere outside of it.
         Y.on('click', handleClickOnPage, window);
     });
+
+    LPS.use('lp.app.links',
+      function(Y) {
+        Y.on('load', function(e) {
+            Y.lp.app.links.check_valid_lp_links();
+        }, window);
+    });
   </script>
 </metal:page-javascript>
 

=== modified file 'lib/lp/bugs/windmill/tests/test_bug_commenting.py'
--- lib/lp/bugs/windmill/tests/test_bug_commenting.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/windmill/tests/test_bug_commenting.py	2010-10-27 04:31:25 +0000
@@ -18,7 +18,7 @@
 WAIT_ELEMENT_COMPLETE = u'30000'
 WAIT_CHECK_CHANGE = u'1000'
 ADD_COMMENT_BUTTON = (
-    u'//input[@id="field.actions.save" and @class="button js-action"]')
+    u'//input[@id="field.actions.save" and contains(@class, "button")]')
 
 
 class TestBugCommenting(WindmillTestCase):

=== added file 'lib/lp/code/windmill/tests/test_branch_broken_links.py'
--- lib/lp/code/windmill/tests/test_branch_broken_links.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/windmill/tests/test_branch_broken_links.py	2010-10-27 04:31:25 +0000
@@ -0,0 +1,113 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test for links between branches and bugs or specs."""
+
+__metaclass__ = type
+__all__ = []
+
+import unittest
+
+import transaction
+import windmill
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.windmill.testing import lpuser
+from canonical.launchpad.windmill.testing.constants import SLEEP
+from lp.code.windmill.testing import CodeWindmillLayer
+from lp.testing import WindmillTestCase
+
+
+ADD_COMMENT_BUTTON = (
+    u'//input[@id="field.actions.save" and contains(@class, "button")]')
+
+
+class TestBranchLinks(WindmillTestCase):
+    """Test the rendering of broken branch links."""
+
+    layer = CodeWindmillLayer
+    suite_name = "Broken branch links"
+
+    BUG_TEXT_TEMPLATE = u"""
+    Here is the bug. Which branches are valid?
+    Valid: %s
+    Invalid %s
+    """
+
+    BRANCH_URL_TEMPLATE = "lp:%s"
+
+    def make_product_and_valid_links(self):
+        branch = self.factory.makeProductBranch()
+        valid_branch_url = self.BRANCH_URL_TEMPLATE % branch.unique_name
+        product = self.factory.makeProduct()
+        product_branch = self.factory.makeProductBranch(product=product)
+        naked_product = removeSecurityProxy(product)
+        naked_product.development_focus.branch = product_branch
+        valid_product_url = self.BRANCH_URL_TEMPLATE % product.name
+
+        return (naked_product, [
+            valid_branch_url,
+            valid_product_url,
+        ])
+
+    def make_invalid_links(self):
+        return [
+            self.BRANCH_URL_TEMPLATE % 'foo',
+            self.BRANCH_URL_TEMPLATE % 'bar',
+            ]
+
+    def test_invalid_url_rendering(self):
+        """Link a bug from the branch page."""
+        client = self.client
+
+        lpuser.FOO_BAR.ensure_login(client)
+
+        naked_product, valid_links = self.make_product_and_valid_links()
+        invalid_links = self.make_invalid_links()
+        bug_description = self.BUG_TEXT_TEMPLATE % (
+            ', '.join(valid_links), ', '.join(invalid_links))
+        bug = self.factory.makeBug(product=naked_product,
+                                        title="The meaning of life is broken",
+                                        description=bug_description)
+        transaction.commit()
+
+        bug_url = (
+            windmill.settings['TEST_URL'] + '%s/+bug/%s'
+            % (naked_product.name, bug.id))
+        client.open(url=bug_url)
+        client.waits.forElement(xpath=ADD_COMMENT_BUTTON)
+
+        # Let the Ajax call run
+        client.waits.sleep(milliseconds=SLEEP)
+
+        code = """
+            var good_a = windmill.testWin().document.getElementsByClassName(
+                            'branch-short-link', 'a');
+            var good_links = [];
+            for( i=0; i<good_a.length; i++ ) {
+                good_links.push(good_a[i].innerHTML);
+            }
+
+            var bad_a = windmill.testWin().document.getElementsByClassName(
+                            'invalid-link', 'a');
+            var bad_links = [];
+            for( i=0; i<bad_a.length; i++ ) {
+                bad_links.push(bad_a[i].innerHTML);
+            }
+
+
+            var result = {};
+            result.good = good_links;
+            result.bad = bad_links;
+            result
+        """
+        raw_result = self.client.commands.execJS(js=code)
+        result = raw_result['result']
+        result_valid_links = result['good']
+        result_invalid_links = result['bad']
+        self.assertEqual(set(invalid_links), set(result_invalid_links))
+        self.assertEqual(set(valid_links), set(result_valid_links))
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/windmill/tests/test_branchmergeproposal_review.py'
--- lib/lp/code/windmill/tests/test_branchmergeproposal_review.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/windmill/tests/test_branchmergeproposal_review.py	2010-10-27 04:31:25 +0000
@@ -27,7 +27,7 @@
 WAIT_ELEMENT_COMPLETE = u'30000'
 WAIT_CHECK_CHANGE = u'1000'
 ADD_COMMENT_BUTTON = (
-    u'//input[@id="field.actions.add" and @class="button js-action"]')
+    u'//input[@id="field.actions.add" and contains(@class, "button")]')
 
 
 class TestRequestReview(WindmillTestCase):