Launchpad itself

Merge lp:~benji/launchpad/better-HTML-generation into lp:launchpad

  1. better-HTML-generation
  2. Merge into devel
Proposed by Benji York
Status: Merged
Approved by: Benji York
Approved revision: no longer in the source branch.
Merged at revision: 12731
Proposed branch: lp:~benji/launchpad/better-HTML-generation
Merge into: lp:launchpad
Diff against target: 930 lines (+311/-290)
2 files modified
lib/lp/registry/javascript/structural-subscription.js (+301/-281)
lib/lp/registry/javascript/tests/test_structural_subscription.js (+10/-9)
To merge this branch: bzr merge lp:~benji/launchpad/better-HTML-generation
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+55960@code.launchpad.net

Commit message

[r=bac][no-qa] Clean up HTML generation, primarily to avoid XSS.

Description of the change

This branch fixes the way structural subscription JavaScript constructs HTML, moving away from string concatenation to Y.Node.create() and friends.

There's also a fair bit of lint fixing required to quite JSLint.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

This is a vast improvement over our original way of creating the html content for the overlay. Thanks for cleaning it all up.

As we discussed on IRC I am a little concerned that there are still large sections of string concatenation to create the recipient picker and overlay container. You pointed out it is all constant string values so it shouldn't present a problem. In the future we can always break those sections down if it proves to be needed.

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/registry/javascript/structural-subscription.js'
2--- lib/lp/registry/javascript/structural-subscription.js 2011-03-30 15:23:42 +0000
3+++ lib/lp/registry/javascript/structural-subscription.js 2011-04-01 17:57:54 +0000
4@@ -11,9 +11,6 @@
5
6 var namespace = Y.namespace('lp.registry.structural_subscription');
7
8-var INNER_HTML = 'innerHTML',
9- VALUE = 'value';
10-
11 var FILTER_COMMENTS = 'filter-comments',
12 FILTER_WRAPPER = 'filter-wrapper',
13 ACCORDION_WRAPPER = 'accordion-wrapper',
14@@ -21,8 +18,7 @@
15 ADDED_OR_CHANGED = 'added-or-changed',
16 ADVANCED_FILTER = 'advanced-filter',
17 MATCH_ALL = 'match-all',
18- MATCH_ANY = 'match-any',
19- SS_COLLAPSIBLE = 'ss-collapsible'
20+ MATCH_ANY = 'match-any'
21 ;
22
23 var add_subscription_overlay;
24@@ -61,7 +57,7 @@
25 function list_contains(list, target) {
26 // The list may be undefined in some cases.
27 return Y.Lang.isArray(list) && list.indexOf(target) !== -1;
28-};
29+}
30
31 // Expose to tests.
32 namespace._list_contains = list_contains;
33@@ -209,8 +205,9 @@
34 function save_subscription(form_data) {
35 var who;
36 var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
37- if (has_errors)
38+ if (has_errors) {
39 return false;
40+ }
41 if (form_data.recipient[0] === 'user') {
42 who = LP.links.me;
43 } else {
44@@ -223,15 +220,18 @@
45
46 function check_for_errors_in_overlay(overlay) {
47 var has_errors = false;
48- var errors = new Array();
49- for (var field in overlay.field_errors) {
50- if (overlay.field_errors[field]) {
51- has_errors = true;
52- errors.push(field);
53+ var errors = [];
54+ var field;
55+ for (field in overlay.field_errors) {
56+ if (overlay.field_errors.hasOwnProperty(field)) {
57+ if (overlay.field_errors[field]) {
58+ has_errors = true;
59+ errors.push(field);
60+ }
61 }
62- };
63+ }
64 if (has_errors) {
65- var error_text = errors.pop()
66+ var error_text = errors.pop();
67 if (errors.length > 0) {
68 error_text = errors.join(', ') + ' and ' + error_text;
69 }
70@@ -242,40 +242,38 @@
71 } else {
72 return false;
73 }
74-};
75+}
76
77 /**
78 * Handle the activation of the edit subscription link.
79 */
80 function edit_subscription_handler(context, form_data) {
81 var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
82- if (has_errors)
83+ var filter_id = '#filter-description-'+context.filter_id.toString();
84+ if (has_errors) {
85 return false;
86+ }
87 var on = {success: function (new_data) {
88 var filter = new_data.getAttrs();
89- var description_node = Y.one(
90- '#filter-description-'+context.filter_id.toString());
91- description_node.set(
92- INNER_HTML, render_filter_description(filter));
93- var name_node = Y.one(
94- '#filter-name-'+context.filter_id.toString());
95- name_node.set(
96- INNER_HTML, render_filter_name(context.filter_info, filter));
97+ var description_node = Y.one(filter_id);
98+ description_node
99+ .empty()
100+ .appendChild(create_filter_description(filter));
101+ description_node.ancestor('.subscription-filter').one('.filter-name')
102+ .empty()
103+ .appendChild(render_filter_title(context.filter_info, filter));
104 add_subscription_overlay.hide();
105 }};
106 patch_bug_filter(context.filter_info.filter, form_data, on);
107 }
108
109+/**
110+ * Initialize the overlay errors and set up field validators.
111+ */
112 function setup_overlay_validators(overlay, overlay_id) {
113- overlay.field_validators = {
114- tags: get_error_for_tags_list
115- };
116 overlay.field_errors = {};
117- for (var field_name in overlay.field_validators) {
118- add_input_validator(overlay, overlay_id, field_name,
119- overlay.field_validators[field_name]);
120- };
121-};
122+ add_input_validator(overlay, overlay_id, 'tags', get_error_for_tags_list);
123+}
124
125 /**
126 * Populate the overlay element with the contents of the add/edit form.
127@@ -295,8 +293,9 @@
128 form_submit_callback: function(formdata) {
129 // Do not clean up if saving was not successful.
130 var save_succeeded = submit_callback(formdata);
131- if (save_succeeded !== false)
132+ if (save_succeeded !== false) {
133 clean_up();
134+ }
135 }
136 });
137 add_subscription_overlay.render(content_box_id);
138@@ -373,10 +372,14 @@
139 * @param {String} name Name of the control.
140 */
141 function make_cell(item, name) {
142- return '<td style="padding-left:3px"><label><input type="checkbox" ' +
143- 'name="' + name +'" ' +
144- 'value="' + item + '" checked="checked">' +
145- item + '</label><td>';
146+ var cell = Y.Node.create('<td style="padding-left:3px"><td>');
147+ cell.appendChild('<label></label>')
148+ .append(Y.Node.create('<input type="checkbox" checked></input>')
149+ .set('name', name)
150+ .set('value', item))
151+ .append(Y.Node.create('<span></span>')
152+ .set('text', item));
153+ return cell;
154 }
155 /**
156 * Make a table.
157@@ -388,19 +391,15 @@
158 * @param {Int} num_cols The number of columns for the table to use.
159 */
160 function make_table(list, name, num_cols) {
161- var html = '<table>';
162- var i;
163+ var table = Y.Node.create('<table></table>');
164+ var i, row;
165 for (i=0; i<list.length; i++) {
166 if (i % num_cols === 0) {
167- if (i !== 0) {
168- html += '</tr>';
169- }
170- html += '<tr>';
171+ row = table.appendChild('<tr></tr>');
172 }
173- html += make_cell(list[i], name);
174+ row.appendChild(make_cell(list[i], name));
175 }
176- html += '</tr></table>';
177- return html;
178+ return table;
179 }
180
181 /**
182@@ -413,16 +412,18 @@
183 * @return {Object} Hash with 'all_name', 'none_name', and 'html' keys.
184 */
185 function make_selector_controls(parent) {
186+ var selectors_id = parent + '-selectors';
187 var rv = {};
188- rv.all_name = parent + '-select-all';
189- rv.none_name = parent + '-select-none';
190- rv.html = '<div id="'+ parent + '-selectors" '+
191- 'style="margin-left: 10px;margin-bottom: 10px">' +
192- ' <a href="#" id="' + rv.all_name +
193- '">Select all</a> &nbsp;' +
194- ' <a href="#" id="' + rv.none_name +
195- '">Select none</a>' +
196- '</div>';
197+ rv.all_link = Y.Node.create(
198+ '<a href="#" class="select-all">Select all</a>');
199+ rv.none_link = Y.Node.create(
200+ '<a href="#" class="select-none">Select none</a>');
201+ rv.node = Y.Node.create(
202+ '<div style="margin-left: 10px;margin-bottom: 10px"></div>')
203+ .set('id', selectors_id)
204+ .append(rv.all_link)
205+ .append(' &nbsp; ')
206+ .append(rv.none_link);
207
208 return rv;
209 }
210@@ -474,27 +475,23 @@
211 id: "tags_ai",
212 contentHeight: {method: "auto"}
213 } );
214-
215 tags_container = tags_ai;
216-
217- tags_ai.set("bodyContent",
218- '<div>\n' +
219- '<div>\n' +
220- ' <input type="radio" name="tag_match" value="' +
221- MATCH_ALL + '" checked> Match all tags\n' +
222- ' <input type="radio" name="tag_match" value="' +
223- MATCH_ANY + '"> Match any tags\n' +
224- '</div>\n' +
225- '<div style="padding-bottom:10px;">\n' +
226- ' <input type="text" name="tags" size="60"/>\n' +
227+ tags_ai.set("bodyContent", Y.Node.create('<div><div></div></div>')
228+ .append(Y.Node.create('<input type="radio" name="tag_match" checked>')
229+ .set('value', MATCH_ALL))
230+ .append('Match all tags')
231+ .append(Y.Node.create('<input type="radio" name="tag_match" checked>')
232+ .set('value', MATCH_ANY))
233+ .append('Match any tags')
234+ .append(
235+ '<div style="padding-bottom:10px;">' +
236+ ' <input type="text" name="tags" size="60"/>' +
237 ' <a target="help"'+
238 ' href="/+help/structural-subscription-tags.html" ' +
239 ' class="sprite maybe">&nbsp;'+
240 '<span class="invisible-link">Structural subscription tags '+
241- ' help</span></a>\n ' +
242- '</div>\n' +
243- '</div>\n');
244-
245+ ' help</span></a> ' +
246+ '</div>'));
247 accordion.addItem(tags_ai);
248
249 // Build importances pane.
250@@ -507,20 +504,17 @@
251 } );
252 var importances = LP.cache.importances;
253 var selectors = make_selector_controls('importances');
254- var importances_html = '<div id="importances-wrapper">' +
255- selectors.html +
256- make_table(importances, 'importances', 4) +
257- '</div>';
258- importances_ai.set("bodyContent", importances_html);
259+ importances_ai.set("bodyContent",
260+ Y.Node.create('<div id="importances-wrapper"></div>')
261+ .append(selectors.node)
262+ .append(make_table(importances, 'importances', 4)));
263 accordion.addItem(importances_ai);
264 // Wire up the 'all' and 'none' selectors.
265- var all_link = content_node.one('#' + selectors.all_name);
266- var none_link = Y.one('#' + selectors.none_name);
267 var node = content_node.one('#importances-wrapper');
268- var select_all_handler = make_select_handler(node, importances, true);
269- var select_none_handler = make_select_handler(node, importances, false);
270- all_link.on('click', select_all_handler);
271- none_link.on('click', select_none_handler);
272+ selectors.all_link.on('click',
273+ make_select_handler(node, importances, true));
274+ selectors.none_link.on('click',
275+ make_select_handler(node, importances, false));
276
277 // Build statuses pane.
278 statuses_ai = new Y.AccordionItem( {
279@@ -532,18 +526,17 @@
280 } );
281 var statuses = LP.cache.statuses;
282 selectors = make_selector_controls('statuses');
283- var status_html = '<div id="statuses-wrapper">' +
284- selectors.html + make_table(statuses, 'statuses', 3)+
285- '</div>';
286- statuses_ai.set("bodyContent", status_html);
287+ statuses_ai.set("bodyContent",
288+ Y.Node.create('<div id="statuses-wrapper"></div>')
289+ .append(selectors.node)
290+ .append(make_table(statuses, 'statuses', 3)));
291 accordion.addItem(statuses_ai);
292- all_link = content_node.one('#' + selectors.all_name);
293- none_link = Y.one('#' + selectors.none_name);
294+ // Wire up the 'all' and 'none' selectors.
295 node = content_node.one('#statuses-wrapper');
296- select_all_handler = make_select_handler(node, statuses, true);
297- select_none_handler = make_select_handler(node, statuses, false);
298- all_link.on('click', select_all_handler);
299- none_link.on('click', select_none_handler);
300+ selectors.all_link.on('click',
301+ make_select_handler(node, statuses, true));
302+ selectors.none_link.on('click',
303+ make_select_handler(node, statuses, false));
304
305 return accordion;
306 }
307@@ -624,105 +617,125 @@
308 }
309
310 /**
311+ * Add a recipient picker to the overlay.
312+ */
313+function add_recipient_picker(content_box, hide) {
314+ var no_recipient_picker = Y.Node.create(
315+ '<input type="hidden" name="recipient" value="user">' +
316+ '<span>Yourself</span>');
317+ var recipient_picker = Y.Node.create(
318+ '<label><input type="radio" name="recipient" value="user" checked>' +
319+ ' Yourself</label><br>' +
320+ '<label><input type="radio" name="recipient" value="team">' +
321+ ' One of the teams you administer</label><br>' +
322+ '<dl style="margin-left:25px;">' +
323+ ' <dt></dt>' +
324+ ' <dd>' +
325+ ' <select name="team" id="structural-subscription-teams">' +
326+ ' </select>' +
327+ ' </dd>' +
328+ '</dl>');
329+ var teams = LP.cache.administratedTeams;
330+ var node = content_box.one('#bug-mail-recipient');
331+ node.empty();
332+ // Populate the team drop down from LP.cache data, if appropriate.
333+ if (!hide && teams.length > 0) {
334+ var select = recipient_picker.one('#structural-subscription-teams');
335+ var i;
336+ for (i=0; i<teams.length; i++) {
337+ select.append(Y.Node.create('<option></option>')
338+ .set('text', teams[i].title)
339+ .set('value', teams[i].link));
340+ }
341+ select.on(
342+ 'focus',
343+ function () {
344+ Y.one('input[value="team"][name="recipient"]').set(
345+ 'checked', true);
346+ }
347+ );
348+ node.append(recipient_picker);
349+ } else {
350+ node.append(no_recipient_picker);
351+ }
352+}
353+
354+/**
355 * Construct the overlay and populate it with the add/edit form.
356 */
357 function setup_overlay(content_box_id, hide_recipient_picker) {
358 var content_node = Y.one(content_box_id);
359- var container = Y.Node.create('<div id="overlay-container"></div>');
360- var accordion_overlay_id = 'accordion-overlay';
361- var teams = LP.cache.administratedTeams;
362- var no_recipient_picker =
363- ' <input type="hidden" name="recipient" value="user">\n' +
364- ' <span>Yourself</span>\n',
365- recipient_picker =
366- ' <input type="radio" name="recipient" value="user"\n'+
367- ' id="structural-subscription-recipient-user" checked>\n'+
368- ' <label for="structural-subscription-recipient-user">\n'+
369- ' Yourself</label><br>\n' +
370- ' <input type="radio" name="recipient"\n'+
371- ' id="structural-subscription-recipient-team"\n'+
372- ' value="team">\n'+
373- ' <label for="structural-subscription-recipient-team">One of\n'+
374- ' the teams you administer</label><br>\n' +
375- ' <dl style="margin-left:25px;">\n' +
376- ' <dt></dt>\n' +
377- ' <dd>\n' +
378- ' <select name="team" id="structural-subscription-teams">\n'+
379- ' </select>\n' +
380- ' </dd>\n' +
381- ' </dl>\n',
382- control_code =
383- '<dl>\n' +
384- ' <dt>Bug mail recipient</dt>\n' +
385- ' <dd>\n' +
386- ((!hide_recipient_picker && teams.length > 0) ?
387- recipient_picker : no_recipient_picker) +
388- ' </dd>\n' +
389- ' <dt>Subscription name</dt>\n' +
390- ' <dd>\n' +
391- ' <input type="text" name="name">\n' +
392- ' <a target="help" class="sprite maybe"\n' +
393- ' href="/+help/structural-subscription-name.html">&nbsp;\n' +
394- ' <span class="invisible-link">Structural subscription\n'+
395- ' description help</span></a>\n ' +
396- ' </dd>\n' +
397- ' <dt>Receive mail for bugs affecting\n'+
398- ' <span id="structural-subscription-context-title">\n'+
399- ' '+LP.cache.context.title+'</span> that</dt>\n' +
400- ' <dd>\n' +
401- ' <div id="events">\n' +
402- ' <input type="radio" name="events"\n' +
403- ' value="' + ADDED_OR_CLOSED + '"\n'+
404- ' id="' + ADDED_OR_CLOSED + '" checked>\n'+
405- ' <label for="'+ADDED_OR_CLOSED+'">are added or '+
406- ' closed</label>\n'+
407- ' <br>\n' +
408- ' <input type="radio" name="events"\n'+
409- ' value="' + ADDED_OR_CHANGED + '"\n' +
410- ' id="' + ADDED_OR_CHANGED + '">\n'+
411- ' <label for="'+ADDED_OR_CHANGED+'">are added or changed in\n'+
412- ' any way\n'+
413- ' <em id="'+ADDED_OR_CHANGED+'-more">(more options...)</em>\n'+
414- ' </label>\n' +
415- ' </div>\n' +
416- ' <div id="' + FILTER_WRAPPER + '" class="ss-collapsible">\n' +
417- ' <dl style="margin-left:25px;">\n' +
418- ' <dt></dt>\n' +
419- ' <dd>\n' +
420- ' <input type="checkbox" name="filters"\n' +
421- ' value="' + FILTER_COMMENTS + '"\n'+
422- ' id="'+FILTER_COMMENTS+'">\n' +
423- ' <label for="'+FILTER_COMMENTS+'">Don\'t send mail about\n'+
424- ' comments</label><br>\n' +
425- ' <input type="checkbox" name="filters"\n' +
426- ' value="' + ADVANCED_FILTER + '"\n' +
427- ' id="' + ADVANCED_FILTER + '">\n' +
428- ' <label for="'+ADVANCED_FILTER+'">Bugs must match this\n'+
429- ' filter <em id="'+ADVANCED_FILTER+'-more">(...)</em>\n'+
430- ' </label><br>\n' +
431- ' <div id="' + ACCORDION_WRAPPER + '" \n' +
432- ' class="' + SS_COLLAPSIBLE + '">\n' +
433- ' <dl>\n' +
434- ' <dt></dt>\n' +
435- ' <dd style="margin-left:25px;">\n' +
436- ' <div id="' + accordion_overlay_id + '"\n' +
437- ' style="position:relative; '+
438- 'overflow:hidden;"></div>\n' +
439- ' </dd>\n' +
440- ' </dl>\n' +
441- ' </div> \n' +
442- ' </dd>\n' +
443- ' </dl>\n' +
444- ' </div> \n' +
445- ' </dd>\n' +
446- ' <dt></dt>\n' +
447- '</dl>';
448-
449- content_node.appendChild(container);
450- container.appendChild(Y.Node.create(control_code));
451-
452- var accordion = create_accordion(
453- '#' + accordion_overlay_id, content_node);
454+ var container = Y.Node.create(
455+ '<div id="overlay-container"><dl>' +
456+ ' <dt>Bug mail recipient</dt>' +
457+ ' <dd id="bug-mail-recipient">' +
458+ ' </dd>' +
459+ ' <dt>Subscription name</dt>' +
460+ ' <dd>' +
461+ ' <input type="text" name="name">' +
462+ ' <a target="help" class="sprite maybe"' +
463+ ' href="/+help/structural-subscription-name.html">&nbsp;' +
464+ ' <span class="invisible-link">Structural subscription' +
465+ ' description help</span></a> ' +
466+ ' </dd>' +
467+ ' <dt>Receive mail for bugs affecting' +
468+ ' <span id="structural-subscription-context-title"></span> that</dt>' +
469+ ' <dd>' +
470+ ' <div id="events">' +
471+ ' <input type="radio" name="events"' +
472+ ' value="added-or-closed"' +
473+ ' id="added-or-closed" checked>' +
474+ ' <label for="added-or-closed">are added or ' +
475+ ' closed</label>' +
476+ ' <br>' +
477+ ' <input type="radio" name="events"' +
478+ ' value="added-or-changed"' +
479+ ' id="added-or-changed">' +
480+ ' <label for="added-or-changed">are added or changed in' +
481+ ' any way' +
482+ ' <em id="added-or-changed-more">(more options...)</em>' +
483+ ' </label>' +
484+ ' </div>' +
485+ ' <div id="filter-wrapper" class="ss-collapsible">' +
486+ ' <dl style="margin-left:25px;">' +
487+ ' <dt></dt>' +
488+ ' <dd>' +
489+ ' <input type="checkbox" name="filters"' +
490+ ' value="filter-comments"' +
491+ ' id="filter-comments">' +
492+ ' <label for="filter-comments">Don\'t send mail about' +
493+ ' comments</label><br>' +
494+ ' <input type="checkbox" name="filters"' +
495+ ' value="advanced-filter"' +
496+ ' id="advanced-filter">' +
497+ ' <label for="advanced-filter">Bugs must match this' +
498+ ' filter <em id="advanced-filter-more">(...)</em>' +
499+ ' </label><br>' +
500+ ' <div id="accordion-wrapper" ' +
501+ ' class="ss-collapsible">' +
502+ ' <dl>' +
503+ ' <dt></dt>' +
504+ ' <dd style="margin-left:25px;">' +
505+ ' <div id="accordion-overlay"' +
506+ ' style="position:relative; overflow:hidden;"></div>' +
507+ ' </dd>' +
508+ ' </dl>' +
509+ ' </div> ' +
510+ ' </dd>' +
511+ ' </dl>' +
512+ ' </div> ' +
513+ ' </dd>' +
514+ ' <dt></dt>' +
515+ '</dl></div>');
516+
517+ // Assemble some nodes and set the title.
518+ content_node
519+ .appendChild(container)
520+ .one('#structural-subscription-context-title')
521+ .set('text', LP.cache.context.title);
522+
523+ var accordion = create_accordion('#accordion-overlay', content_node);
524+ add_recipient_picker(container, hide_recipient_picker);
525
526 // Set up click handlers for the events radio buttons.
527 var radio_group = Y.all('#events input');
528@@ -735,26 +748,6 @@
529 advanced_filter.on(
530 'change',
531 function() {handle_change(ADVANCED_FILTER, ACCORDION_WRAPPER);});
532- // Populate the team drop down from LP.cache data, if appropriate.
533- if (!hide_recipient_picker && teams.length > 0) {
534- var select = Y.one('#structural-subscription-teams');
535- var i;
536- var team;
537- for (i=0; i<teams.length; i++) {
538- team = teams[i];
539- var option = Y.Node.create('<option></option>');
540- option.set(INNER_HTML, team.title);
541- option.set(VALUE, team.link);
542- select.appendChild(option);
543- }
544- select.on(
545- 'focus',
546- function () {
547- Y.one('input[value="team"][name="recipient"]').set(
548- 'checked', true);
549- }
550- );
551- }
552 return '#' + container._node.id;
553 } // setup_overlay
554 // Expose in the namespace for testing purposes.
555@@ -807,7 +800,7 @@
556 var overlay_id = setup_overlay(config.content_box, true);
557 var submit_button = Y.Node.create(
558 '<button type="submit" name="field.actions.create" ' +
559- 'value="Save Changes" class="lazr-pos lazr-btn" '+
560+ 'value="Save Changes" class="lazr-pos lazr-btn" ' +
561 '>OK</button>');
562 // This is a bit of an odd approach, but it lets us retrofit code
563 // without a large refactoring. When edit_subscription_handler is
564@@ -848,8 +841,9 @@
565 overlay.field_errors[field_name] = true;
566 // Accordion sets fixed height for the accordion item,
567 // so we have to resize the tags container.
568- if (field_name == 'tags')
569+ if (field_name === 'tags') {
570 tags_container.resize();
571+ }
572 // Firefox prohibits focus from inside the 'focus lost' event
573 // handler (probably to stop loops), so we need to run
574 // it from a different context (which we do with setTimeout).
575@@ -859,7 +853,7 @@
576 overlay.field_errors[field_name] = false;
577 }
578 });
579-};
580+}
581
582 function get_error_for_tags_list(value) {
583 // See database/schema/trusted.sql valid_name() function
584@@ -874,7 +868,8 @@
585 'digits 0-9 and symbols "+", "-" or ".", and they ' +
586 'must start with a lowercase letter or a digit.');
587 }
588-};
589+}
590+
591 // Export for testing
592 namespace._get_error_for_tags_list = get_error_for_tags_list;
593
594@@ -956,12 +951,12 @@
595 var i;
596 for (i=0; i<teams.length; i++) {
597 if (teams[i].link === filter_info.subscriber_link){
598- recipient_label.set(INNER_HTML, teams[i].title);
599+ recipient_label.set('text', teams[i].title);
600 break;
601 }
602 }
603 } else {
604- recipient_label.set(INNER_HTML, 'Yourself');
605+ recipient_label.set('text', 'Yourself');
606 }
607 content_node.one('[name="name"]').set('value',filter.description);
608 if (is_lifecycle) {
609@@ -1011,10 +1006,10 @@
610 context.filter_info = filter_info;
611 context.filter_id = filter_id;
612 var title = subscription.target_title;
613- Y.one('#structural-subscription-context-title').set(
614- INNER_HTML, title);
615- Y.one('#subscription-overlay-title').set(
616- INNER_HTML, 'Edit subscription for '+title+' bugs');
617+ Y.one('#structural-subscription-context-title')
618+ .set('text', title);
619+ Y.one('#subscription-overlay-title')
620+ .set('text', 'Edit subscription for '+title+' bugs');
621 add_subscription_overlay.show();
622 }
623 };
624@@ -1035,14 +1030,14 @@
625 headers: {'X-HTTP-Method-Override': 'DELETE'},
626 on: {success: function(transactionid, response, args){
627 var subscriber = Y.one(
628- '#subscription-'+subscriber_id.toString()),
629- node = subscriber,
630- filters = subscriber.all('.subscription-filter');
631+ '#subscription-'+subscriber_id.toString());
632+ var to_collapse = subscriber;
633+ var filters = subscriber.all('.subscription-filter');
634 if (!filters.isEmpty()) {
635- node = Y.one(
636+ to_collapse = Y.one(
637 '#subscription-filter-'+filter_id.toString());
638 }
639- collapse_node(node);
640+ collapse_node(to_collapse);
641 },
642 failure: error_handler.getFailureHandler()
643 }
644@@ -1066,11 +1061,12 @@
645 var filter_info = sub.filters[j];
646 if (!filter_info.subscriber_is_team ||
647 filter_info.user_is_team_admin) {
648- var edit_link = Y.one('#edit-'+filter_id.toString());
649+ var node = Y.one('#subscription-filter-'+filter_id.toString());
650+ var edit_link = node.one('a.edit-subscription');
651 var edit_handler = make_edit_handler(
652 sub, filter_info, filter_id, config, context);
653 edit_link.on('click', edit_handler);
654- var delete_link = Y.one('#unsubscribe-'+filter_id.toString());
655+ var delete_link = node.one('a.delete-subscription');
656 var delete_handler = make_delete_handler(
657 filter_info.filter, filter_id, i);
658 delete_link.on('click', delete_handler);
659@@ -1085,22 +1081,27 @@
660 */
661 function fill_in_bug_subscriptions(config, context) {
662 validate_config(config);
663+
664 var listing = Y.one('#subscription-listing');
665 var subscription_info = LP.cache.subscription_info;
666- var html = '<div class="yui-g"><div id="structural-subscriptions">';
667+ var top_node = Y.Node.create(
668+ '<div class="yui-g"><div id="structural-subscriptions"></div></div>');
669 var filter_id = 0;
670 var i;
671 var j;
672 for (i=0; i<subscription_info.length; i++) {
673 var sub = subscription_info[i];
674- html +=
675+ var sub_node = top_node.appendChild(Y.Node.create(
676 '<div style="margin-top: 2em; padding: 0 1em 1em 1em; '+
677- ' border: 1px solid #ddd;"'+
678- ' id="subscription-'+i.toString()+'">'+
679- ' <span style="float: left; margin-top: -0.6em; padding: 0 1ex;'+
680- ' background-color: #fff;">Subscriptions to'+
681- ' <a href="'+sub.target_url+'">'+sub.target_title+'</a>'+
682- ' </span>';
683+ ' border: 1px solid #ddd;"></div>')
684+ .set('id', 'subscription-'+i.toString()));
685+ sub_node.appendChild(Y.Node.create(
686+ ' <span style="float: left; margin-top: -0.6em; '+
687+ ' padding: 0 1ex; background-color: #fff;"></a>'))
688+ .appendChild('<span>Subscriptions to </span>')
689+ .appendChild(Y.Node.create('<a></a>')
690+ .set('href', sub.target_url)
691+ .set('text', sub.target_title));
692
693 for (j=0; j<sub.filters.length; j++) {
694 var filter = sub.filters[j].filter;
695@@ -1110,52 +1111,53 @@
696 // and see the information you expect to see.
697 LP.cache['structural-subscription-filter-'+filter_id.toString()] =
698 filter;
699- html +=
700+ var filter_node = sub_node.appendChild(Y.Node.create(
701 '<div style="margin: 1em 0em 0em 1em"'+
702- ' id="subscription-filter-'+filter_id.toString()+'"'+
703- ' class="subscription-filter">'+
704- ' <div style="margin-top: 1em">'+
705- ' <strong id="filter-name-'+
706- filter_id.toString()+'">'+
707- render_filter_name(sub.filters[j], filter)+'</strong>';
708+ ' class="subscription-filter"></div>')
709+ .set('id', 'subscription-filter-'+filter_id.toString()))
710+ .appendChild(Y.Node.create(
711+ '<div style="margin-top: 1em"></div>'));
712+ filter_node.appendChild(Y.Node.create(
713+ '<strong class="filter-name"></strong>'))
714+ .appendChild(render_filter_title(sub.filters[j], filter));
715+
716 if (!sub.filters[j].subscriber_is_team ||
717 sub.filters[j].user_is_team_admin) {
718 // User can edit the subscription.
719- html +=
720+ filter_node.appendChild(Y.Node.create(
721 '<span style="float: right">'+
722- '<a href="#" class="sprite modify edit js-action"'+
723- ' id="edit-'+filter_id.toString()+'">'+
724+ '<a href="#" class="sprite modify edit js-action '+
725+ ' edit-subscription">'+
726 ' Edit this subscription</a> or '+
727- '<a href="#" class="sprite modify remove js-action"'+
728- ' id="unsubscribe-'+filter_id.toString()+'">'+
729- ' Unsubscribe</a></span>';
730+ '<a href="#" class="sprite modify remove js-action '+
731+ ' delete-subscription">'+
732+ ' Unsubscribe</a></span>'));
733 } else {
734 // User cannot edit the subscription, because this is a
735 // team and the user does not have admin privileges.
736- html +=
737+ filter_node.appendChild(Y.Node.create(
738 '<span style="float: right"><em>'+
739 'You do not have privileges to change this subscription'+
740- '</em></span>';
741+ '</em></span>'));
742 }
743- html += '</div>';
744- html +=
745- '<div style="padding-left: 1em"'+
746- ' id="filter-description-'+filter_id.toString()+'">'+
747- render_filter_description(filter)+'</div>';
748-
749- html += '</div>';
750+
751+ filter_node.appendChild(Y.Node.create(
752+ '<div style="padding-left: 1em"></div>')
753+ .set('id', 'filter-description-'+filter_id.toString()))
754+ .appendChild(create_filter_description(filter));
755+
756 filter_id += 1;
757 }
758
759 // We can remove this once we enforce at least one filter per
760 // subscription.
761 if (subscription_info[i].filters.length === 0) {
762- html += '<strong>All messages</strong>';
763+ sub_node.appendChild(
764+ '<div style="clear: both; padding: 1em 0 0 1em"></div>')
765+ .appendChild('<strong>All messages</strong>');
766 }
767- html += '</div>';
768 }
769- html += '</div></div>';
770- listing.appendChild(Y.Node.create(html));
771+ listing.appendChild(top_node);
772
773 wire_up_edit_links(config, context);
774 }
775@@ -1163,7 +1165,8 @@
776 /**
777 * Construct a one-line textual description of a filter's name.
778 */
779-function render_filter_name(filter_info, filter) {
780+function render_filter_title(filter_info, filter) {
781+ var title = Y.Node.create('<span></span>');
782 var description;
783 if (filter.description) {
784 description = '"'+filter.description+'"';
785@@ -1171,62 +1174,79 @@
786 description = '(unnamed)';
787 }
788 if (filter_info.subscriber_is_team) {
789- return '<a href="'+filter_info.subscriber_url+'">'+
790- filter_info.subscriber_title+"</a> subscription: "+description;
791+ title.appendChild(Y.Node.create('<a></a>'))
792+ .set('href', filter_info.subscriber_url)
793+ .set('text', filter_info.subscriber_title);
794+ title.appendChild(Y.Node.create('<span></span>'))
795+ .set('text', ' subscription: '+description);
796 } else {
797- return 'Your subscription: '+description;
798+ title.set('text', 'Your subscription: '+description);
799 }
800+ return title;
801 }
802
803 /**
804 * Construct a textual description of all of filter's properties.
805 */
806-function render_filter_description(filter) {
807- var html = '';
808- var filter_items = '';
809+function create_filter_description(filter) {
810+ var description = Y.Node.create('<div></div>');
811+
812+ var filter_items = [];
813 // Format status conditions.
814 if (filter.statuses.length !== 0) {
815- filter_items += '<li> have status ' +
816- filter.statuses.join(', ');
817+ filter_items.push(Y.Node.create('<li></li>')
818+ .set('text', 'have status '+filter.statuses.join(', ')));
819 }
820
821 // Format importance conditions.
822 if (filter.importances.length !== 0) {
823- filter_items += '<li> are of importance ' +
824- filter.importances.join(', ');
825+ filter_items.push(Y.Node.create('<li></li>')
826+ .set('text', 'are of importance '+filter.importances.join(', ')));
827 }
828
829 // Format tag conditions.
830 if (filter.tags.length !== 0) {
831- filter_items += '<li> are tagged with ';
832+ var tag_desc = Y.Node.create('<li>are tagged with </li>')
833+ .append(Y.Node.create('<strong></strong>'))
834+ .append(Y.Node.create('<span> of these tags: </span>'))
835+ .append(Y.Node.create('<span></span>')
836+ .set('text', filter.tags.join(', ')));
837+
838 if (filter.find_all_tags) {
839- filter_items += '<strong>all</strong>';
840+ tag_desc.one('strong').set('text', 'all');
841 } else {
842- filter_items += '<strong>any</strong>';
843+ tag_desc.one('strong').set('text', 'any');
844 }
845- filter_items += ' of these tags: ' +
846- filter.tags.join(', ');
847+ filter_items.push(tag_desc);
848 }
849
850 // If there were any conditions to list, stich them in with an
851 // intro.
852- if (filter_items !== '') {
853- html += 'You are subscribed to bugs that'+
854- '<ul class="bulleted">'+filter_items+'</ul>';
855+ if (filter_items.length > 0) {
856+ var ul = Y.Node.create('<ul class="bulleted"></ul>');
857+ Y.each(filter_items, function (li) {ul.appendChild(li);});
858+ description.appendChild(
859+ Y.Node.create('<span>You are subscribed to bugs that</span>'));
860+ description.appendChild(ul);
861 }
862
863 // Format event details.
864+ var events; // When will email be sent?
865 if (filter.bug_notification_level === 'Discussion') {
866- html += 'You will recieve an email when any change '+
867+ events = 'You will recieve an email when any change '+
868 'is made or a comment is added.';
869 } else if (filter.bug_notification_level === 'Details') {
870- html += 'You will recieve an email when any changes '+
871+ events = 'You will recieve an email when any changes '+
872 'are made to the bug. Bug comments will not be sent.';
873 } else if (filter.bug_notification_level === 'Lifecycle') {
874- html += 'You will recieve an email when bugs are '+
875+ events = 'You will recieve an email when bugs are '+
876 'opened or closed.';
877+ } else {
878+ throw new Error('Unrecognized events.');
879 }
880- return html;
881+ description.appendChild(Y.Node.create(events));
882+
883+ return description;
884 }
885
886 /**
887
888=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
889--- lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-03-30 15:23:42 +0000
890+++ lib/lp/registry/javascript/tests/test_structural_subscription.js 2011-04-01 17:57:54 +0000
891@@ -147,11 +147,12 @@
892 test_make_selector_controls: function() {
893 // Verify the creation of select all/none controls.
894 var selectors = module.make_selector_controls('sharona');
895- Assert.areEqual('sharona-select-all', selectors['all_name']);
896- Assert.areEqual('sharona-select-none', selectors['none_name']);
897- Assert.areEqual(
898- '<div id="sharona-selectors"',
899- selectors['html'].slice(0, 27));
900+ Assert.areEqual(
901+ 'Select all', selectors.all_link.get('text'));
902+ Assert.areEqual(
903+ 'Select none', selectors.none_link.get('text'));
904+ Assert.areEqual(
905+ 'sharona-selectors', selectors.node.get('id'));
906 }
907 });
908 suite.add(test_case);
909@@ -419,8 +420,8 @@
910 // accordion pane.
911 module.setup(this.configuration);
912 var checkboxes = Y.all('input[name="importances"]');
913- var select_all = Y.one('#importances-select-all');
914- var select_none = Y.one('#importances-select-none');
915+ var select_all = Y.one('#importances-selectors > a.select-all');
916+ var select_none = Y.one('#importances-selectors > a.select-none');
917 Assert.isTrue(test_checked(checkboxes, true));
918 // Simulate a click on the select_none control.
919 Y.Event.simulate(Y.Node.getDOMNode(select_none), 'click');
920@@ -435,8 +436,8 @@
921 // accordion pane.
922 module.setup(this.configuration);
923 var checkboxes = Y.all('input[name="statuses"]');
924- var select_all = Y.one('#statuses-select-all');
925- var select_none = Y.one('#statuses-select-none');
926+ var select_all = Y.one('#statuses-selectors > a.select-all');
927+ var select_none = Y.one('#statuses-selectors > a.select-none');
928 Assert.isTrue(test_checked(checkboxes, true));
929 // Simulate a click on the select_none control.
930 Y.Event.simulate(Y.Node.getDOMNode(select_none), 'click');