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

Proposed by Nigel Babu on 2011-09-13
Status: Merged
Approved by: Gavin Panella on 2011-09-15
Approved revision: no longer in the source branch.
Merged at revision: 13967
Proposed branch: lp:~nigelbabu/launchpad/bug-title-849121
Merge into: lp:launchpad
Diff against target: 308 lines (+150/-46)
5 files modified
lib/lp/app/browser/linkchecker.py (+17/-11)
lib/lp/app/browser/tests/test_linkchecker.py (+12/-2)
lib/lp/app/javascript/lp-links.js (+35/-33)
lib/lp/app/javascript/tests/test_lp_links.html (+32/-0)
lib/lp/app/javascript/tests/test_lp_links.js (+54/-0)
To merge this branch: bzr merge lp:~nigelbabu/launchpad/bug-title-849121
Reviewer Review Type Date Requested Status
Gavin Panella (community) code 2011-09-15 Approve on 2011-09-15
Raphaël Badin (community) code* 2011-09-13 Approve on 2011-09-15
Review via email: mp+75267@code.launchpad.net

Commit message

[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 of the change

= 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

To post a comment you must log in.
Raphaël Badin (rvb) wrote :

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) wrote :

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) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/browser/linkchecker.py'
2--- lib/lp/app/browser/linkchecker.py 2011-08-28 07:29:11 +0000
3+++ lib/lp/app/browser/linkchecker.py 2011-09-15 19:16:29 +0000
4@@ -65,8 +65,8 @@
5
6 for link_type in links_to_check:
7 links = links_to_check[link_type]
8- invalid_links = self.link_checkers[link_type](links)
9- result['invalid_' + link_type] = invalid_links
10+ link_info = self.link_checkers[link_type](links)
11+ result[link_type] = link_info
12
13 self.request.response.setHeader('Content-type', 'application/json')
14 return simplejson.dumps(result)
15@@ -83,23 +83,29 @@
16 InvalidProductName, NoLinkedBranch, NoSuchBranch,
17 NotFoundError) as e:
18 invalid_links[link] = self._error_message(e)
19- return invalid_links
20+ return {'invalid': invalid_links}
21
22 def check_bug_links(self, links):
23 """Checks links of the form /bugs/100"""
24 invalid_links = {}
25+ valid_links = {}
26 user = self.user
27- bugs = [int(link[len('/bugs/'):]) for link in links]
28- if bugs:
29+ # List of all the bugs we are checking.
30+ bugs_ids = set([int(link[len('/bugs/'):]) for link in links])
31+ if bugs_ids:
32 params = BugTaskSearchParams(
33 user=user, status=None,
34- bug=any(*bugs))
35- bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
36- invalid = set(bugs) - set(bug_ids)
37- for bug in invalid:
38- invalid_links['/bugs/' + str(bug)] = (
39+ bug=any(*bugs_ids))
40+ bugtasks = getUtility(IBugTaskSet).search(params)
41+ for task in bugtasks:
42+ valid_links['/bugs/' + str(task.bug.id)] = task.bug.title
43+ # Remove valid bugs from the list of all the bugs.
44+ bugs_ids.remove(task.bug.id)
45+ # We should now have only invalid bugs in bugs list
46+ for bug in bugs_ids:
47+ invalid_links['/bugs/%d' % bug] = (
48 "Bug %s cannot be found" % bug)
49- return invalid_links
50+ return {'valid': valid_links, 'invalid': invalid_links}
51
52 def _error_message(self, ex):
53 if hasattr(ex, 'display_message'):
54
55=== modified file 'lib/lp/app/browser/tests/test_linkchecker.py'
56--- lib/lp/app/browser/tests/test_linkchecker.py 2011-08-28 07:29:11 +0000
57+++ lib/lp/app/browser/tests/test_linkchecker.py 2011-09-15 19:16:29 +0000
58@@ -24,10 +24,16 @@
59
60 def check_invalid_links(self, result_json, link_type, invalid_links):
61 link_dict = simplejson.loads(result_json)
62- links_to_check = link_dict[link_type]
63+ links_to_check = link_dict[link_type]['invalid']
64 self.assertEqual(len(invalid_links), len(links_to_check))
65 self.assertEqual(set(invalid_links), set(links_to_check))
66
67+ def check_valid_links(self, result_json, link_type, valid_links):
68+ link_dict = simplejson.loads(result_json)
69+ links_to_check = link_dict[link_type]['valid']
70+ self.assertEqual(len(valid_links), len(links_to_check))
71+ self.assertEqual(set(valid_links), set(links_to_check))
72+
73 def make_valid_branch_links(self):
74 branch = self.factory.makeProductBranch()
75 valid_branch_url = self.BRANCH_URL_TEMPLATE % branch.unique_name
76@@ -86,7 +92,11 @@
77 link_checker = LinkCheckerAPI(object(), request)
78 result_json = link_checker()
79 self.check_invalid_links(
80- result_json, 'invalid_branch_links', invalid_branch_urls)
81+ result_json, 'branch_links', invalid_branch_urls)
82+ self.check_invalid_links(
83+ result_json, 'bug_links', invalid_bug_urls)
84+ self.check_valid_links(
85+ result_json, 'bug_links', valid_bug_urls)
86
87 def test_with_no_data(self):
88 request = LaunchpadTestRequest()
89
90=== modified file 'lib/lp/app/javascript/lp-links.js'
91--- lib/lp/app/javascript/lp-links.js 2011-08-15 11:10:29 +0000
92+++ lib/lp/app/javascript/lp-links.js 2011-09-15 19:16:29 +0000
93@@ -12,7 +12,7 @@
94 function harvest_links(links_holder, link_class, link_type) {
95 // Get any links of the specified link_class and store them as the
96 // specified link_type in the specified links_holder
97- var link_info = new Array();
98+ var link_info = [];
99 Y.all('.'+link_class).each(function(link) {
100 var href = link.getAttribute('href');
101 if( link_info.indexOf(href)<0 ) {
102@@ -24,36 +24,38 @@
103 }
104 }
105
106- function process_invalid_links(link_info, link_class, link_type) {
107- // We have a collection of invalid links possibly containing links of
108+ function process_links(link_info, link_class, link_type) {
109+ // We have a collection of valid and invalid links containing links of
110 // type link_type, so we need to remove the existing link_class,
111 // replace it with an invalid-link class, and set the link title.
112- var invalid_links = link_info['invalid_'+link_type];
113-
114- if( Y.Object.size(invalid_links) == 0 )
115- return;
116+ var invalid_links = link_info[link_type].invalid || {};
117+ var valid_links = link_info[link_type].valid || {};
118
119 Y.all('.'+link_class).each(function(link) {
120 var href = link.getAttribute('href');
121- if( !(href in invalid_links) )
122- return;
123- var invalid_link_msg = invalid_links[href];
124- link.removeClass(link_class);
125- link.addClass('invalid-link');
126- link.setAttribute('title', invalid_link_msg);
127- link.on('click', function(e) {
128- e.halt();
129- alert(invalid_link_msg);
130- });
131+ if(invalid_links.hasOwnProperty(href)) {
132+ var invalid_link_msg = invalid_links[href];
133+ link.removeClass(link_class);
134+ link.addClass('invalid-link');
135+ link.setAttribute('title', invalid_link_msg);
136+ link.on('click', function(e) {
137+ e.halt();
138+ alert(invalid_link_msg);
139+ });
140+ } else if(valid_links.hasOwnProperty(href)) {
141+ var valid_link_msg = valid_links[href];
142+ link.setAttribute('title', valid_link_msg);
143+ }
144+
145 });
146 }
147
148- Y.lp.app.links.check_valid_lp_links = function() {
149+ Y.lp.app.links.check_valid_lp_links = function(io_provider) {
150 // Grabs any lp: style links on the page and checks that they are
151 // valid. Invalid ones have their class changed to "invalid-link".
152- // ATM, we only handle +branch links.
153+ // ATM, we only handle +branch and bug links.
154
155- var links_to_check = {}
156+ var links_to_check = {};
157
158 // We get all the links with defined css classes.
159 // At the moment, we just handle branch links, but in future...
160@@ -61,7 +63,7 @@
161 harvest_links(links_to_check, 'bug-link', 'bug_links');
162
163 // Do we have anything to do?
164- if( Y.Object.size(links_to_check) == 0 ) {
165+ if( Y.Object.size(links_to_check) === 0 ) {
166 return;
167 }
168
169@@ -74,23 +76,23 @@
170 on: {
171 failure: function(id, response, args) {
172 // If we have firebug installed, log the error.
173- if( console != undefined ) {
174+ if( Y.Lang.isValue(console) ) {
175 console.log("Link Check Error: " + args + ': '
176- + response.status + ' - ' +
177- response.statusText + ' - '
178- + response.responseXML);
179+ + response.status + ' - ' +
180+ response.statusText + ' - '
181+ + response.responseXML);
182 }
183 },
184 success: function(id, response) {
185- var link_info = Y.JSON.parse(response.responseText)
186- // ATM, we just handle branch links, but in future...
187- process_invalid_links(link_info, 'branch-short-link',
188- 'branch_links');
189- process_invalid_links(link_info, 'bug-link',
190- 'bug_links');
191+ var link_info = Y.JSON.parse(response.responseText);
192+ // We handle bug and branch links. The future is here.
193+ process_links(link_info, 'branch-short-link',
194+ 'branch_links');
195+ process_links(link_info, 'bug-link',
196+ 'bug_links');
197 }
198 }
199- }
200+ };
201 var uri = '+check-links';
202 var on = Y.merge(config.on);
203 var client = this;
204@@ -99,7 +101,7 @@
205 on: on,
206 'arguments': [client, uri],
207 data: qs};
208- Y.io(uri, y_config);
209+ Y.lp.client.get_io_provider(io_provider).io(uri, y_config);
210 };
211
212 }, "0.1", {"requires": ["base", "node", "io", "dom", "json", "lp.client"]});
213
214=== added file 'lib/lp/app/javascript/tests/test_lp_links.html'
215--- lib/lp/app/javascript/tests/test_lp_links.html 1970-01-01 00:00:00 +0000
216+++ lib/lp/app/javascript/tests/test_lp_links.html 2011-09-15 19:16:29 +0000
217@@ -0,0 +1,32 @@
218+<html>
219+ <head>
220+ <title>Launchpad lp-links</title>
221+ <!-- YUI and test setup -->
222+ <script type="text/javascript"
223+ src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
224+ </script>
225+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
226+ <script type="text/javascript"
227+ src="../../../app/javascript/testing/testrunner.js"></script>
228+
229+ <script type="text/javascript" src="../client.js"></script>
230+ <script type="text/javascript"
231+ src="../../../app/javascript/testing/mockio.js"></script>
232+
233+ <!-- The module under test -->
234+ <script type="text/javascript" src="../lp-links.js"></script>
235+
236+ <!-- The test suite -->
237+ <script type="text/javascript" src="test_lp_links.js"></script>
238+ </head>
239+ <body class="yui3-skin-sam">
240+
241+ <!-- The example markup required by the script to run -->
242+ <div id="expected-id">
243+ <a href="/bugs/14" class="bug-link" id="valid-bug">bug 14</a>
244+ <a href="/bugs/200" class="bug-link" id="invalid-bug">bug 200</a>
245+ <a href="/+branch/valid" class="branch-short-link" id="valid-branch">lp:valid</a>
246+ <a href="/+branch/invalid" class="branch-short-link" id="invalid-branch">lp:invalid</a>
247+ </div>
248+ </body>
249+</html>
250
251=== added file 'lib/lp/app/javascript/tests/test_lp_links.js'
252--- lib/lp/app/javascript/tests/test_lp_links.js 1970-01-01 00:00:00 +0000
253+++ lib/lp/app/javascript/tests/test_lp_links.js 2011-09-15 19:16:29 +0000
254@@ -0,0 +1,54 @@
255+YUI().use('lp.testing.runner', 'test', 'console',
256+ 'lp.app.links', 'lp.testing.mockio', 'lp.client',
257+ 'node',
258+ function(Y) {
259+
260+ var links = Y.lp.app.links;
261+ var suite = new Y.Test.Suite("lp.app.links Tests");
262+ var mock_io = new Y.lp.testing.mockio.MockIo();
263+
264+ suite.add(new Y.Test.Case({
265+ name: 'test_bugs',
266+
267+ setUp: function() {
268+ links.check_valid_lp_links(mock_io);
269+ var response = {
270+ "bug_links": {
271+ "valid": {
272+ "/bugs/14": [
273+ "jokosher exposes personal details ",
274+ "in its actions portlet"].join('')},
275+ "invalid": {
276+ "/bugs/200": "Bug 200 cannot be found"}},
277+ "branch_links": {
278+ "invalid": {
279+ "/+branch/invalid":
280+ "No such product: 'invalid'."}}};
281+ mock_io.success({
282+ responseText: Y.JSON.stringify(response),
283+ responseHeaders: {'Content-type': 'application/json'}
284+ });
285+ },
286+
287+ test_bugs: function () {
288+ var validbug = Y.one('#valid-bug');
289+ var invalidbug = Y.one('#invalid-bug');
290+ Y.Assert.isTrue(validbug.hasClass('bug-link'));
291+ Y.Assert.isTrue(invalidbug.hasClass('invalid-link'));
292+ Y.Assert.areSame(
293+ 'jokosher exposes personal details in its actions portlet',
294+ validbug.get('title')
295+ );
296+ },
297+
298+ test_branch: function () {
299+ var validbranch = Y.one('#valid-branch');
300+ var invalidbranch = Y.one('#invalid-branch');
301+ Y.Assert.isTrue(validbranch.hasClass('branch-short-link'));
302+ Y.Assert.isTrue(invalidbranch.hasClass('invalid-link'));
303+ }
304+ }));
305+
306+ Y.lp.testing.Runner.run(suite);
307+});
308+