Launchpad itself

Merge lp:~mars/launchpad/fix-picker-result-parsing-487975 into lp:launchpad/db-devel

  1. fix-picker-result-parsing-487975
  2. Merge into db-devel
Proposed by Māris Fogels
Status: Merged
Approved by: Aaron Bentley
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~mars/launchpad/fix-picker-result-parsing-487975
Merge into: lp:launchpad/db-devel
Diff against target: 43 lines (+16/-12)
1 file modified
lib/canonical/launchpad/javascript/lp/picker.js (+16/-12)
To merge this branch: bzr merge lp:~mars/launchpad/fix-picker-result-parsing-487975
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Approve
Tim Penhey (community) Approve
Aaron Bentley (community) code Approve
Review via email: mp+15436@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

Hi,

This branch fixes the Picker widget's API result parsing code. This fixes bug
#487975, which prevented the "Assign Me" link in the picker widget from working
correctly.

I updated the picker parsing code so that is processes the result as Node
objects rather than raw DOM elements. This saves us some tree traversal code.
There was no error checking code before, and I did not add any myself. It would
require changes beyond an RC fix.

I did not include a windmill test for this feature due to release-critical time
constraints.

Tested on launchpad.dev
Bugs Windmill tests appear to pass.
I haven't run the Bugs JS unit test suite.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
   lib/canonical/launchpad/javascript/lp/picker.js

== JSLint notices ==

jslint: No problem found in
'/home/mars/canonical/lp-branches/fix-picker-result-parsing-487975/lib/canonical/launchpad/javascript/lp/picker.js'.

Maris

Revision history for this message
Aaron Bentley (abentley) wrote :

There's an extra space before the = on line 9 of the patch. Otherwise, looks fine.

review: Approve (code)
Revision history for this message
Tim Penhey (thumper) wrote :

You have two spaces after "var success_message_node".

But apart from that, this looks good.

review: Approve
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

Please file a bug about rewriting this to not parse HTML output and mark it as XXX. Request the field representation directly.

review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/lp/picker.js'
2--- lib/canonical/launchpad/javascript/lp/picker.js 2009-11-24 09:30:01 +0000
3+++ lib/canonical/launchpad/javascript/lp/picker.js 2009-12-01 15:15:22 +0000
4@@ -78,24 +78,28 @@
5 var save = function (picker_result) {
6 activator.renderProcessing();
7 var success_handler = function (entry) {
8- var xhtml = Y.DOM.create(entry)[1];
9+ // XXX mars 2009-12-1
10+ // The approach we use here is deprecated. Instead of requesting
11+ // the entire entity we should only request the fields we need.
12+ // Then most of this code can go away. See bug #490826.
13+ var success_message_node = null;
14+ var xhtml = Y.Node.create(entry);
15 var current_field = null;
16- for (var i = 0; i < xhtml.childNodes.length; i++) {
17- var element = xhtml.childNodes[i];
18- if (element.tagName == 'DT') {
19- current_field = element.innerHTML;
20- continue;
21- }
22- if (element.tagName == 'DD') {
23+ // The entry is an XHTML document with a <dl> node at the root. We
24+ // want to process each <dt><dd> tag pair under that root.
25+ xhtml.all('dl *').each(function(element) {
26+ if (element.get('tagName') == 'DT') {
27+ current_field = element.get('innerHTML');
28+ } else if (element.get('tagName') == 'DD') {
29 if (current_field == attribute_name) {
30 // The field value is found
31- node = Y.one(element).one('span');
32+ success_message_node = element.one('span');
33 } else if (current_field == 'self_link') {
34- picker._resource_uri = element.innerHTML;
35+ picker._resource_uri = element.get('innerHTML');
36 }
37 }
38- }
39- activator.renderSuccess(node);
40+ });
41+ activator.renderSuccess(success_message_node);
42 show_hide_buttons();
43 };
44

Subscribers

People subscribed via source and target branches

to status/vote changes: