Merge lp:~nigelbabu/launchpad/bug-title-849121 into lp:launchpad

[Nigel Babu]

[r=allenap,rvb][bug=849121] For bug links in Launchpad, add the bug's title to "title" attribute of the URL if the bug is valid.

= Description =
Adds the bug title to "title" attribute if the bug is valid. My earlier commits to those tests didn't actually test the bugs bit. I've also added YUI tests for lp-links.js. It didn't have a test until now, and my changes were extensive enough that I probably wouldn't get away without it ;-)

Instead of this
{"invalid_bug_links": {"/bugs/200": "Bug 200 cannot be found"}, "invalid_branch_links": {"/+branch/foobar": "No such product: 'foobar'."}}

We now have
{"bug_links": {"valid": {"/bugs/14": "jokosher exposes personal details in its actions portlet"}, "invalid": {"/bugs/200": "Bug 200 cannot be found"}}, "branch_links": {"invalid": {"/+branch/foobar": "No such product: 'foobar'."}}}

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/browser/linkchecker.py
  lib/lp/app/browser/tests/test_linkchecker.py
  lib/lp/app/javascript/lp-links.js
  lib/lp/app/javascript/tests/test_lp_links.html
  lib/lp/app/javascript/tests/test_lp_links.js

./lib/lp/app/javascript/lp-links.js
      43: Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead.
      52: Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead.
make: *** [lint] Error 2

[Raphaël Badin (rvb)]

Hi Nigel, thanks a lot for this branch. Thanks for fixing the lint errors. It looks great but I spotted a few things I'd like you too fix before we land this.

[1]

30 - bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
31 - invalid = set(bugs) - set(bug_ids)
32 - for bug in invalid:
33 + bugtasks = getUtility(IBugTaskSet).search(params)
34 + for task in bugtasks:
35 + valid_links['/bugs/' + str(task.bug.id)] = task.bug.title
36 + bugs.remove(task.bug.id)
37 + for bug in bugs:

Although the logic seems fine, I think this would be more clear if you iterate over a variable named 'invalid_bugs' (or something) like this was done before instead of creating that list dynamically like you do now.

[2]

137 + } else if( (href in valid_links) ) {

I think you can remove some parenthesis here.

9 - // ATM, we only handle +branch links.
150 + // ATM, we only handle +branch and bug links

Nitpick: please add the '.' at the end of the sentence ;).

[3]

257 + // Test the setup method.

I think this is a left-over ...?

[4]

171 + if( console !== undefined ) {

I know this is not your code in the first place but since you're fixing it (and doing a good job at it), you could use YUI's Y.Lang.isValue (see http://developer.yahoo.com/yui/docs/YAHOO.lang.html) to check if console is undefined.

[5]

184 + process_links(link_info, 'branch-short-link',
185 'branch_links');
186 - process_invalid_links(link_info, 'bug-link',
187 + process_links(link_info, 'bug-link',
188 'bug_links');

Again, not your code but maybe you could fix the indentation here.

[6]

261 + links.check_valid_lp_links(mock_io);
262 + var response_json = ['{"bug_links": {"valid": {"/bugs/14"',
263 + ': "jokosher exposes personal details in its actions portlet"}',
264 + ', "invalid": {"/bugs/200": "Bug 200 cannot be found"}}, ',
265 + '"branch_links": {"invalid": {"/+branch/invalid": "No such',
266 + ' product: \'foobar\'."}}}'].join('');

My eyes! Formatting it this way makes it very difficult to read. Because you're using mockio you must build a string but my advice here would be to create the json object (properly formatted) and apply Y.JSON.stringify to it.

[7]

278 + Y.Assert.areSame(
279 + 'jokosher exposes personal details in its actions portlet',
280 + validbug.get('title'));
281 + },
282 + test_branch: function () {

2 small formatting problem here:
- please add an empty line between the functions;
- please fix the indentation of the line which starts with "'jokosher" and the one that follow.

review: Approve (code*)

[Raphaël Badin (rvb)]

Thanks for the changes Nigel. A few more nitpicks:

[1]

@@ -74,7 +83,7 @@
             on: {
                 failure: function(id, response, args) {
                     // If we have firebug installed, log the error.
- if( console != undefined ) {
+ if( Y.Lang.isValue(console) ) {
                         console.log("Link Check Error: " + args + ': '
                                 + response.status + ' - ' +
                                 response.statusText + ' - '

Again, not your changes but could you fix the indentation here?

[2]

+ var response = {"bug_links": {"valid": {"/bugs/14":
+ "jokosher exposes personal details in its actions portlet"},
+ "invalid": {"/bugs/200": "Bug 200 cannot be found"}},
+ "branch_links": {"invalid": {"/+branch/invalid":
+ "No such product: 'invalid'."}}};

Here is how I like my json ;):

- var response = {"bug_links": {"valid": {"/bugs/14":
- "jokosher exposes personal details in its actions portlet"},
- "invalid": {"/bugs/200": "Bug 200 cannot be found"}},
- "branch_links": {"invalid": {"/+branch/invalid":
- "No such product: 'invalid'."}}};
+ var response = {
+ "bug_links": {
+ "valid": {
+ "/bugs/14": [
+ "jokosher exposes personal details ",
+ "in its actions portlet"].join('')},
+ "invalid": {
+ "/bugs/200": "Bug 200 cannot be found"}},
+ "branch_links": {
+ "invalid": {
+ "/+branch/invalid":
+ "No such product: 'invalid'."}}};

[Gavin Panella (allenap)]

In addition to Raphaël's tip top review, I'd like to add a few points:

[1]

- result['invalid_' + link_type] = invalid_links
+ result[link_type] = invalid_links

invalid_links is not really an appropriate variable name now, because
the value takes the form of {"valid": ..., "invalid": ...}. Something
like link_info would be better I think.

[2]

+ # Remove valid bugs from the list of all the bugs.
+ bugs.remove(task.bug.id)

Can you change bugs to be a set rather than a list? Also, it should
probably be called bug_ids rather than bugs.

[3]

+ for task in bugtasks:
+ valid_links['/bugs/' + str(task.bug.id)] = task.bug.title

It's might just be me, but % formatting would be clearer here:

                valid_links['/bugs/%d' % task.bug.id] = task.bug.title

[4]

+ if( Y.Object.size(invalid_links) === 0 ) {
+ invalid_links = {};
+ }
+ if( Y.Object.size(valid_links) === 0 ) {
+ valid_links = {};
+ }

I didn't know about Object.size(), neat.

It's a nitpick, but if invalid_links is {} already, this will set it
to a new {}. It just doesn't seem very neat ;) How about:

        if (invalid_links === undefined) {
            invalid_links = {};
        }
        if (valid_links === undefined) {
            valid_links = {};
        }

Or use Y.Lang.isValue().

In fact, it's common practice to use implicit truth with JavaScript,
so you can probably combine the above with initialization:

        var invalid_links = link_info[link_type].invalid || {};
        var valid_links = link_info[link_type].valid || {};

[5]

+ if(href in invalid_links) {
...
+ } else if(href in valid_links) {

There were lint complaints about these. For the sake of noise, change
these to use hasOwnProperty().

review: Approve (code)

=== modified file 'lib/lp/app/browser/linkchecker.py'
--- lib/lp/app/browser/linkchecker.py	2011-08-28 07:29:11 +0000
+++ lib/lp/app/browser/linkchecker.py	2011-09-15 19:16:29 +0000
@@ -65,8 +65,8 @@
 
         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
+            link_info = self.link_checkers[link_type](links)
+            result[link_type] = link_info
 
         self.request.response.setHeader('Content-type', 'application/json')
         return simplejson.dumps(result)
@@ -83,23 +83,29 @@
                     InvalidProductName, NoLinkedBranch, NoSuchBranch,
                     NotFoundError) as e:
                 invalid_links[link] = self._error_message(e)
-        return invalid_links
+        return {'invalid': invalid_links}
 
     def check_bug_links(self, links):
         """Checks links of the form /bugs/100"""
         invalid_links = {}
+        valid_links = {}
         user = self.user
-        bugs = [int(link[len('/bugs/'):]) for link in links]
-        if bugs:
+        # List of all the bugs we are checking.
+        bugs_ids = set([int(link[len('/bugs/'):]) for link in links])
+        if bugs_ids:
             params = BugTaskSearchParams(
                 user=user, status=None,
-                bug=any(*bugs))
-            bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
-            invalid = set(bugs) - set(bug_ids)
-            for bug in invalid:
-                invalid_links['/bugs/' + str(bug)] = (
+                bug=any(*bugs_ids))
+            bugtasks = getUtility(IBugTaskSet).search(params)
+            for task in bugtasks:
+                valid_links['/bugs/' + str(task.bug.id)] = task.bug.title
+                # Remove valid bugs from the list of all the bugs.
+                bugs_ids.remove(task.bug.id)
+            # We should now have only invalid bugs in bugs list
+            for bug in bugs_ids:
+                invalid_links['/bugs/%d' % bug] = (
                     "Bug %s cannot be found" % bug)
-        return invalid_links
+        return {'valid': valid_links, 'invalid': invalid_links}
 
     def _error_message(self, ex):
         if hasattr(ex, 'display_message'):

=== modified file 'lib/lp/app/browser/tests/test_linkchecker.py'
--- lib/lp/app/browser/tests/test_linkchecker.py	2011-08-28 07:29:11 +0000
+++ lib/lp/app/browser/tests/test_linkchecker.py	2011-09-15 19:16:29 +0000
@@ -24,10 +24,16 @@
 
     def check_invalid_links(self, result_json, link_type, invalid_links):
         link_dict = simplejson.loads(result_json)
-        links_to_check = link_dict[link_type]
+        links_to_check = link_dict[link_type]['invalid']
         self.assertEqual(len(invalid_links), len(links_to_check))
         self.assertEqual(set(invalid_links), set(links_to_check))
 
+    def check_valid_links(self, result_json, link_type, valid_links):
+        link_dict = simplejson.loads(result_json)
+        links_to_check = link_dict[link_type]['valid']
+        self.assertEqual(len(valid_links), len(links_to_check))
+        self.assertEqual(set(valid_links), set(links_to_check))
+
     def make_valid_branch_links(self):
         branch = self.factory.makeProductBranch()
         valid_branch_url = self.BRANCH_URL_TEMPLATE % branch.unique_name
@@ -86,7 +92,11 @@
         link_checker = LinkCheckerAPI(object(), request)
         result_json = link_checker()
         self.check_invalid_links(
-            result_json, 'invalid_branch_links', invalid_branch_urls)
+            result_json, 'branch_links', invalid_branch_urls)
+        self.check_invalid_links(
+            result_json, 'bug_links', invalid_bug_urls)
+        self.check_valid_links(
+            result_json, 'bug_links', valid_bug_urls)
 
     def test_with_no_data(self):
         request = LaunchpadTestRequest()

=== modified file 'lib/lp/app/javascript/lp-links.js'
--- lib/lp/app/javascript/lp-links.js	2011-08-15 11:10:29 +0000
+++ lib/lp/app/javascript/lp-links.js	2011-09-15 19:16:29 +0000
@@ -12,7 +12,7 @@
     function harvest_links(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();
+        var link_info = [];
         Y.all('.'+link_class).each(function(link) {
             var href = link.getAttribute('href');
             if( link_info.indexOf(href)<0 ) {
@@ -24,36 +24,38 @@
         }
     }
 
-    function process_invalid_links(link_info, link_class, link_type) {
-        // We have a collection of invalid links possibly containing links of
+    function process_links(link_info, link_class, link_type) {
+        // We have a collection of valid and invalid links 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 = link_info['invalid_'+link_type];
-
-        if( Y.Object.size(invalid_links) == 0 )
-            return;
+        var invalid_links = link_info[link_type].invalid || {};
+        var valid_links = link_info[link_type].valid || {};
 
         Y.all('.'+link_class).each(function(link) {
             var href = link.getAttribute('href');
-            if( !(href in invalid_links) )
-                return;
-            var invalid_link_msg = invalid_links[href];
-            link.removeClass(link_class);
-            link.addClass('invalid-link');
-            link.setAttribute('title', invalid_link_msg);
-            link.on('click', function(e) {
-                e.halt();
-                alert(invalid_link_msg);
-            });
+            if(invalid_links.hasOwnProperty(href)) {
+                var invalid_link_msg = invalid_links[href];
+                link.removeClass(link_class);
+                link.addClass('invalid-link');
+                link.setAttribute('title', invalid_link_msg);
+                link.on('click', function(e) {
+                    e.halt();
+                    alert(invalid_link_msg);
+                });
+            } else if(valid_links.hasOwnProperty(href)) {
+                var valid_link_msg = valid_links[href];
+                link.setAttribute('title', valid_link_msg);
+            }
+
         });
     }
 
-    Y.lp.app.links.check_valid_lp_links = function() {
+    Y.lp.app.links.check_valid_lp_links = function(io_provider) {
         // 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.
+        // ATM, we only handle +branch and bug links.
 
-        var links_to_check = {}
+        var links_to_check = {};
 
         // We get all the links with defined css classes.
         // At the moment, we just handle branch links, but in future...
@@ -61,7 +63,7 @@
         harvest_links(links_to_check, 'bug-link', 'bug_links');
 
         // Do we have anything to do?
-        if( Y.Object.size(links_to_check) == 0 ) {
+        if( Y.Object.size(links_to_check) === 0 ) {
             return;
         }
 
@@ -74,23 +76,23 @@
             on: {
                 failure: function(id, response, args) {
                     // If we have firebug installed, log the error.
-                    if( console != undefined ) {
+                    if( Y.Lang.isValue(console) ) {
                         console.log("Link Check Error: " + args + ': '
-                                + response.status + ' - ' +
-                                response.statusText + ' - '
-                                + response.responseXML);
+                            + 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(link_info, 'branch-short-link',
-                            'branch_links');
-                    process_invalid_links(link_info, 'bug-link',
-                            'bug_links');
+                    var link_info = Y.JSON.parse(response.responseText);
+                    // We handle bug and branch links. The future is here.
+                    process_links(link_info, 'branch-short-link',
+                        'branch_links');
+                    process_links(link_info, 'bug-link',
+                        'bug_links');
                 }
             }
-        }
+        };
         var uri = '+check-links';
         var on = Y.merge(config.on);
         var client = this;
@@ -99,7 +101,7 @@
                          on: on,
                          'arguments': [client, uri],
                          data: qs};
-        Y.io(uri, y_config);
+        Y.lp.client.get_io_provider(io_provider).io(uri, y_config);
     };
 
 }, "0.1", {"requires": ["base", "node", "io", "dom", "json", "lp.client"]});

=== added file 'lib/lp/app/javascript/tests/test_lp_links.html'
--- lib/lp/app/javascript/tests/test_lp_links.html	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_lp_links.html	2011-09-15 19:16:29 +0000
@@ -0,0 +1,32 @@
+<html>
+  <head>
+    <title>Launchpad lp-links</title>
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/testrunner.js"></script>
+
+  <script type="text/javascript" src="../client.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/mockio.js"></script>
+
+    <!-- The module under test -->
+    <script type="text/javascript" src="../lp-links.js"></script>
+
+    <!-- The test suite -->
+    <script type="text/javascript" src="test_lp_links.js"></script>
+  </head>
+  <body class="yui3-skin-sam">
+
+    <!-- The example markup required by the script to run -->
+    <div id="expected-id">
+      <a href="/bugs/14" class="bug-link" id="valid-bug">bug 14</a>
+      <a href="/bugs/200" class="bug-link" id="invalid-bug">bug 200</a>
+      <a href="/+branch/valid" class="branch-short-link" id="valid-branch">lp:valid</a>
+      <a href="/+branch/invalid" class="branch-short-link" id="invalid-branch">lp:invalid</a>
+    </div>
+  </body>
+</html>

=== added file 'lib/lp/app/javascript/tests/test_lp_links.js'
--- lib/lp/app/javascript/tests/test_lp_links.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_lp_links.js	2011-09-15 19:16:29 +0000
@@ -0,0 +1,54 @@
+YUI().use('lp.testing.runner', 'test', 'console',
+    'lp.app.links', 'lp.testing.mockio', 'lp.client',
+    'node',
+    function(Y) {
+
+    var links = Y.lp.app.links;
+    var suite = new Y.Test.Suite("lp.app.links Tests");
+    var mock_io = new Y.lp.testing.mockio.MockIo();
+
+    suite.add(new Y.Test.Case({
+        name: 'test_bugs',
+
+        setUp: function() {
+            links.check_valid_lp_links(mock_io);
+            var response = {
+                "bug_links": {
+                    "valid": {
+                        "/bugs/14": [
+                            "jokosher exposes personal details ",
+                            "in its actions portlet"].join('')},
+                    "invalid": {
+                        "/bugs/200": "Bug 200 cannot be found"}},
+                "branch_links": {
+                    "invalid": {
+                        "/+branch/invalid":
+                            "No such product: 'invalid'."}}};
+            mock_io.success({
+                responseText: Y.JSON.stringify(response),
+                responseHeaders: {'Content-type': 'application/json'}
+            });
+        },
+
+        test_bugs: function () {
+            var validbug = Y.one('#valid-bug');
+            var invalidbug = Y.one('#invalid-bug');
+            Y.Assert.isTrue(validbug.hasClass('bug-link'));
+            Y.Assert.isTrue(invalidbug.hasClass('invalid-link'));
+            Y.Assert.areSame(
+                'jokosher exposes personal details in its actions portlet',
+                validbug.get('title')
+            );
+        },
+
+        test_branch: function () {
+            var validbranch = Y.one('#valid-branch');
+            var invalidbranch = Y.one('#invalid-branch');
+            Y.Assert.isTrue(validbranch.hasClass('branch-short-link'));
+            Y.Assert.isTrue(invalidbranch.hasClass('invalid-link'));
+        }
+    }));
+
+    Y.lp.testing.Runner.run(suite);
+});
+