Code review comment for lp:~yellow/launchpad/accordion-client-1

Hi Gary,

Whew, done. There are 20-odd items that need addressing but most of them
are quite small. Bear in mind that some of these may be addressed in the
next branch, but I thought it better to make note of them now just in
case. I'm marking the branch as r=me as I think all of these can be
addressed fairly simply and I don't want to block the landing. If any
major revisions are made someone else can add their vote beside mine.

One thing that I did notice was that certain things (like the
trailing-comma-before-closing-brace problem, see [6]) should have been
picked up by our JS linter (which I think runs as part of `make lint.`
Could you re-run that for me and, if they're not being picked up, file a
bug?

[1]

236 + Y.on('domready', function() {
237 + module.setup_bug_subscriptions(
238 + {content_box: "#structural-subscription-content-box"})

This line needs a semicolon.

239 + });

And this need to be outdented by four spaces to avoid it looking like
it's closing the above JS Object.

[2]

236 + Y.on('domready', function() {
237 + module.setup_bug_subscriptions(
238 + {content_box: "#structural-subscription-content-box"})
239 + });

Same comments as [1].

[3]

395 + Bugs can be tagged to help the project categorise the bug report. You
396 + can filter your subscription based on one or more tags. If you can choose
397 + to <em>Match all tags</em> then your filter will only match bugs that
398 + have each of those tags you enter here.

Should the end of line 396 read "If you choose" rather than "If you can
choose..."?

[4]

476 + // The list may be undefined in some cases.
477 + return list && list.indexOf(target) != -1;

We should use isArray() explicitly here:

    return Y.Lang.isArray(list) && list.indexOf(target) != -1;

[5]

479 +namespace.list_contains = list_contains;

This seems to be a common pattern. Why not just make the function part
of the namespace in the first place:

namespace.list_contains = function(list, target) {
    ///...
};

(This is a common pattern elsewhere in the LP JS, though I don't know if
it's considered a standard). I'll not bother highlighting the other
cases; they're easy enough to find.

[6]

495 + statuses: [],
496 + };

The comma before the closing brace on an object literal causes problems
with some browsers (Opera and some versions of WebKit, IIRC); it should
be removed.

[7]

569 + on: {failure: overlay_error_handler.getFailureHandler()},

The trailing comma needs to die for the same reason as in [6].

[8]

575 +namespace._delete_filter = delete_filter

Needs a semicolon.

[9]

611 +namespace._add_bug_filter = add_bug_filter

So does this.

[10]

633 +function edit_subscription_handler(context, form_data) {

This function needs some documentation commentary.

[11]

649 +function create_overlay(content_box_id, overlay_id, submit_button,
650 + submit_callback) {

As does this one.

[12]

691 + alert('"Subscribe to bug mail" link not found.');

We shouldn't be using the alert() here any more. Y.fail would seem more
appropriate.

[13]

698 + e.preventDefault();

There's a been a bug about the use of e.preventDefault() and
e.stopPropagation() not working properly with some browsers (probably IE
but ISTR some WebKit browsers had problems too). For safety we should
use e.halt(), since in all the cases where one of {preventDefault,
stopPropagation} fails, the other works fine, and e.halt() calls both.
It's probably worth replacing all instances of preventDefault() in this
JS with halt().

[14]

708 +function clear_overlay(content_node) {

This needs documentation.

[15]

902 + * @param node The node to collapse.
903 + */
904 +function collapse_node(node, user_cfg) {

user_cfg needs to be documented here, too.

[16]

939 +/**
940 + * Expand the node and set its arrow to 'collapsed'
941 + * @param node The node to collapse.
942 + */
943 +function expand_node(node, user_cfg) {

Same as [15].

[17]

977 +/**
978 + * Construct the overlay and populate it with the add/edit form.
979 + *
980 + * @method setup_overlay
981 + * @param {String} content_box_id Id of the element on the page where
982 + * the overlay is anchored.
983 + * @return {String} overlay_id Id of the constructed overlay element.
984 + */
985 +function setup_overlay(content_box_id, hide_recipient_picker) {

There's no overlay_id parameter, so you can drop the documentation for
it.

[18]

1242 + // if we want to allow editing of the recipient, do this instead
1243 + // of the if/else block below.
1244 + // set_recipient(
1245 + // content_node, subscriber_is_team, team_link);

Can this be dropped from the code or are we going to be using it later?

[19]

These all need documentation:

1182 +function get_input_by_value(node, value) {
1189 +function set_checkboxes(node, all, checked) {
1200 +function set_options(node, name, value) {
1207 +function set_radio_buttons(node, all, value) {
1211 +function set_recipient(node, is_team, team_link) {
1220 +function make_edit_handler(subscription, filter_info, filter_id,
1221 + config, context) {
1309 +function make_delete_handler(filter, filter_id, subscriber_id) {
1337 +function wire_up_edit_links(config, context) {
1337 +function wire_up_edit_links(config, context) {
1436 +function render_filter_name(filter_info, filter) {
1450 +function render_filter_description(filter) {
1498 +function validate_config(config) {

As do:

1754 + test_logged_in_user: function() {
1761 + test_list_contains: function() {
1773 + test_make_selector_controls: function() {
1838 + test_user_recipient: function() {
1847 + test_team_recipient: function() {
1989 + test_importances_select_all_none: function() {
2003 + test_statuses_select_all_none: function() {
2065 + test_overlay_error_handling_adding: function() {
2083 + test_overlay_error_handling_patching: function() {

The test_extract_* tests in the extract_form_data test case could do
with some documentation too, though I realise there'd be a lot of
boilerplate there. Maybe one comment for the test case would suffice.

[20]

1918 + // When initialized the <div> elements for the filter
1919 + // wrapper and the accordion wrapper should be collapsed.
1920 + // Since the collapsing is done via animation, we must
1921 + // wait a bit for it to happen.

In the comments here you talk about waiting for something to happen, but
I don't see any calls to this.wait() in this test. Can this statement be
removed?