launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04984
[Merge] lp:~nigelbabu/launchpad/bug-title-849121 into lp:launchpad
Nigel Babu has proposed merging lp:~nigelbabu/launchpad/bug-title-849121 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #849121 in Launchpad itself: "Autolinked bugs should also have title attribute with bug title"
https://bugs.launchpad.net/launchpad/+bug/849121
For more details, see:
https://code.launchpad.net/~nigelbabu/launchpad/bug-title-849121/+merge/75267
= 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
--
https://code.launchpad.net/~nigelbabu/launchpad/bug-title-849121/+merge/75267
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~nigelbabu/launchpad/bug-title-849121 into lp:launchpad.
=== 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 10:09:19 +0000
@@ -66,7 +66,7 @@
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
+ result[link_type] = invalid_links
self.request.response.setHeader('Content-type', 'application/json')
return simplejson.dumps(result)
@@ -83,23 +83,26 @@
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:
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:
+ bugtasks = getUtility(IBugTaskSet).search(params)
+ for task in bugtasks:
+ valid_links['/bugs/' + str(task.bug.id)] = task.bug.title
+ bugs.remove(task.bug.id)
+ for bug in bugs:
invalid_links['/bugs/' + str(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 10:09:19 +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 10:09:19 +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,45 @@
}
}
- 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];
+ var invalid_links = link_info[link_type].invalid;
+ var valid_links = link_info[link_type].valid;
- if( Y.Object.size(invalid_links) == 0 )
- return;
+ if( Y.Object.size(invalid_links) === 0 ) {
+ invalid_links = {};
+ }
+ if( Y.Object.size(valid_links) === 0 ) {
+ valid_links = {};
+ }
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( (href in invalid_links) ) {
+ 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( (href in valid_links) ) {
+ 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 +70,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,7 +83,7 @@
on: {
failure: function(id, response, args) {
// If we have firebug installed, log the error.
- if( console != undefined ) {
+ if( console !== undefined ) {
console.log("Link Check Error: " + args + ': '
+ response.status + ' - ' +
response.statusText + ' - '
@@ -82,15 +91,15 @@
}
},
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',
+ 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_invalid_links(link_info, 'bug-link',
+ process_links(link_info, 'bug-link',
'bug_links');
}
}
- }
+ };
var uri = '+check-links';
var on = Y.merge(config.on);
var client = this;
@@ -99,7 +108,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 10:09:19 +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 10:09:19 +0000
@@ -0,0 +1,46 @@
+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({
+ // Test the setup method.
+ name: 'test_bugs',
+
+ setUp: function() {
+ links.check_valid_lp_links(mock_io);
+ var response_json = ['{"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: \'foobar\'."}}}'].join('');
+ mock_io.success({
+ responseText: response_json,
+ 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);
+});
+
Follow ups
