Hi Graham,

Your code looks great - it'll be an excellent change when it lands - but
it's quite difficult to review it like this when I can't see what
it does, or test the functionality (as opposed to testing that non of
it is currently run).

Also, there are a few points below where you've still got work-in-progress
code - so I'm ok with approving this branch as long as you're not landing
it on its own like this, but rather as part of a subsequent branch
(just use the 'Prerequisite branch' option when you create the MP
via the UI, so that the next reviewer won't see all these changes.) As it really needs a review where the functionality can be observed and tested.

It would be great if this new functionality could be tested with some JS unit-tests in lib/canonical/launchpad/javascript/bugs/tests/ in a subsequent branch too.

Thanks for all the cleanups too!

> === modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
> --- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js	2009-11-23 19:29:02 +0000
> +++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js	2009-11-26 13:39:58 +0000
[...]
> @@ -99,6 +113,67 @@
>  }
>  
>  /**
> + * Search for bugs that may match the text that the user has entered and
> + * display them in-line.
> + */
> +function search_for_and_display_dupes() {
> +    function show_failure_message() {
> +        Y.get('#possible-duplicates').set(INNER_HTML, 'FAIL');

erm, that's a temporary message right? I understood from your MP that
this branch doesn't enable the functionality, but are you planning to
hold off landing it until the subsequent branch is done?

There is a new error api in lazrjs which needs a little love, but will
be useful here soon.

> @@ -185,124 +260,160 @@
>  }
>  
>  
> -Y.bugs.setup_dupe_finder = function() {
> -    Y.on('domready', function() {
> -        bug_already_reported_expanders = Y.all(
> -            'img.bug-already-reported-expander');
> -        bug_reporting_form = Y.get('#bug_reporting_form');
> -
> -        if (bug_already_reported_expanders !== null &&
> -            bug_already_reported_expanders !== undefined) {
> -
> -            // Set up the onclick handlers for the expanders.
> -            Y.each(Y.all('.similar-bug'), function(row) {
> -                var bug_details_div = row.query('div.duplicate-details');
> -                var image = row.query('img.bug-already-reported-expander');
> -                var bug_title_link = row.query('.duplicate-bug-link');
> -                var view_bug_link = row.query('.view-bug-link');
> -
> -                // Grab the initial height of the element and create a
> -                // config for the slide_out animation. This allows us to
> -                // deal with browsers like Opera in which the JS engine
> -                // takes a punt at the right height for the slid-out
> -                // drawer and gets it completely and utterly wrong.
> -                var initial_height = bug_details_div.get('scrollHeight');
> -                var slide_out_config = {
> -                    to: {
> -                        height: initial_height
> -                    }
> -                };
> -
> -                // Shut down the default action for the link and mark it
> -                // as a JS'd link. We do this as it's simpler than
> -                // trying to find all the bits of the row that we want
> -                // to make clickable.
> -                bug_title_link.addClass('js-action');
> -                bug_title_link.on('click', function(e) {
> -                    e.preventDefault();
> -                });
> -
> -                // The "view this bug" link shouldn't trigger the
> -                // collapsible, so we stop the event from propagating.
> -                view_bug_link.on('click', function(e) {
> -                    e.stopPropagation();
> -                });
> -
> -                // The same is true for the collapsible section. People
> -                // may want to copy and paste this, which involves
> -                // clicking, so we stop the onclick event from
> -                // propagating here, too.
> -                bug_details_div.on('click', function(e) {
> -                    e.stopPropagation();
> -                });
> -
> -                // Set up the on focus handler for the link so that
> -                // tabbing will expand the different bugs.
> -                bug_title_link.on('focus', function(e) {
> -                    if (!bug_details_div.hasClass('lazr-opened')) {
> -                        var anim = Y.lazr.effects.slide_out(
> -                            bug_details_div, slide_out_config);
> -                        anim.run();
> -
> -                        image.set(SRC, EXPANDER_EXPANDED);
> -
> -                        // If the bug reporting form is shown, hide it.
> -                        if (bug_reporting_form.getStyle(DISPLAY) == BLOCK) {
> -                            bug_reporting_form.setStyle(DISPLAY, NONE);
> -                        }
> -                    }
> -                });
> -
> -                row.on('click', function(e) {
> -                    if (bug_details_div.hasClass('lazr-opened')) {
> -                        collapse_bug_details(image);
> -                    } else {
> -                        var anim = Y.lazr.effects.slide_out(
> -                            bug_details_div, slide_out_config);
> -                        anim.run();
> -
> -                        image.set(SRC, EXPANDER_EXPANDED);
> -                    }
> +/**
> + * Set up the dupe finder, overriding the default behaviour of the
> + * +filebug search form.
> + */
> +function set_up_dupe_finder(transaction_id, response, args) {
> +    var filebug_form_container = Y.get('#filebug-form-container');
> +    filebug_form_container.set(INNER_HTML, response.responseText);
> +
> +    // Activate the extra options collapsible section on the bug
> +    // reporting form.
> +    var bug_reporting_form = Y.get('#bug_reporting_form');
> +    if (Y.Lang.isValue(bug_reporting_form)) {
> +        activateCollapsibles();
> +    }
> +
> +    search_button = Y.get(Y.DOM.byId('field.actions.search'));
> +
> +    // Change the name and id of the search field so that it doesn't
> +    // confuse the view when we submit a bug report.
> +    search_field = Y.get(Y.DOM.byId('field.title'));
> +    search_field.set('name', 'field.search');
> +    search_field.set('id', 'field.search');

Hmm... I've read that a few times now, but am still unsure why it's
necessary? When we submit a bug report, the view will be expecting
the field.title won't it? Or is it that you're re-using the form
that you've grabbed here via ajax (a ++form++ url?) to do the actual
submission later? Either way, either my brain has gone to mush
(possible at the end of an OCR day) or a more descriptive comment
is needed above.

> +
> +    // Disable the form so that hitting "enter" in the Summary
> +    // field no longer sends us through to the next page.
> +    // Y.on('submit', function(e) { e.halt(); }, '#my-form')

Why is this commented out, when you said in the MP that
set_up_dupe_finder is not called yet anyway?

> +
> +    // Update the label on the search button so that it no longer
> +    // says "Continue".
> +    search_button.set('value', 'Next');
> +    search_button.set('type', 'button');
> +
> +    // Set up the handlers for the search button and the input
> +    // field.
> +    search_button.on('click', search_for_and_display_dupes);
> +}
> +
> +Y.bugs.setup_dupes = function() {
> +    bug_already_reported_expanders = Y.all(
> +        'img.bug-already-reported-expander');
> +    bug_reporting_form = Y.get('#bug_reporting_form');
> +
> +    if (Y.Lang.isValue(bug_already_reported_expanders)) {
> +        // Collapse all the details divs, since we don't want them
> +        // expanded first up.
> +        Y.each(Y.all('div.duplicate-details'), function(div) {
> +            collapse_bug_details(div);
> +        });
> +
> +        // Set up the onclick handlers for the expanders.
> +        Y.each(Y.all('.similar-bug'), function(row) {
> +            var bug_details_div = row.query('div.duplicate-details');
> +            var image = row.query('img.bug-already-reported-expander');
> +            var bug_title_link = row.query('.duplicate-bug-link');
> +            var view_bug_link = row.query('.view-bug-link');
> +
> +            // Shut down the default action for the link and mark it
> +            // as a JS'd link. We do this as it's simpler than
> +            // trying to find all the bits of the row that we want
> +            // to make clickable.
> +            bug_title_link.addClass('js-action');
> +            bug_title_link.on('click', function(e) {
> +                e.preventDefault();
> +            });
> +
> +            // The "view this bug" link shouldn't trigger the
> +            // collapsible, so we stop the event from propagating.
> +            view_bug_link.on('click', function(e) {
> +                e.stopPropagation();
> +            });
> +
> +            // The same is true for the collapsible section. People
> +            // may want to copy and paste this, which involves
> +            // clicking, so we stop the onclick event from
> +            // propagating here, too.
> +            bug_details_div.on('click', function(e) {
> +                e.stopPropagation();
> +            });
> +
> +            // Set up the on focus handler for the link so that
> +            // tabbing will expand the different bugs.
> +            bug_title_link.on('focus', function(e) {
> +                if (!bug_details_div.hasClass('lazr-opened')) {
> +                    var anim = Y.lazr.effects.slide_out(bug_details_div);
> +                    anim.run();
> +
> +                    image.set(SRC, EXPANDER_EXPANDED);
>  
>                      // If the bug reporting form is shown, hide it.
>                      if (bug_reporting_form.getStyle(DISPLAY) == BLOCK) {
>                          bug_reporting_form.setStyle(DISPLAY, NONE);
>                      }
> -                });
> -            });
> -
> -            // Hide the bug reporting form.
> -            bug_reporting_form.setStyle(DISPLAY, NONE);
> -
> -            // Collapse all the details divs, since we don't want them
> -            // expanded first up.
> -            Y.each(Y.all('div.duplicate-details'), function(div) {
> -                collapse_bug_details(div);
> -            });
> -
> -        }
> -
> -        bug_not_reported_button = Y.get('#bug-not-already-reported');
> -        if (bug_not_reported_button !== null &&
> -            bug_not_reported_button !== undefined) {
> -            // The bug_not_reported_button won't show up if there aren't any
> -            // possible duplicates.
> -            bug_not_reported_button.on('click', show_bug_reporting_form);
> -        }
> -
> -        // Attach the form overlay to the "Yes, this is my bug" forms.
> -        var this_is_my_bug_forms = Y.all('form.this-is-my-bug-form');
> -        Y.each(this_is_my_bug_forms, function(form) {
> -            var subscribe_form_overlay = create_subscribe_overlay(form);
> -
> -            form.on('submit', function(e) {
> -                // We don't care about the original event, so stop it
> -                // and show the form overlay that we just created.
> -                e.halt();
> -                subscribe_form_overlay.show();
> -            });
> -        });
> -    });
> +                }
> +            });
> +
> +            row.on('click', function(e) {
> +                if (bug_details_div.hasClass('lazr-opened')) {
> +                    collapse_bug_details(image);
> +                } else {
> +                    var anim = Y.lazr.effects.slide_out(bug_details_div);
> +                    anim.run();
> +
> +                    image.set(SRC, EXPANDER_EXPANDED);
> +                }
> +
> +                // If the bug reporting form is shown, hide it.
> +                if (bug_reporting_form.getStyle(DISPLAY) == BLOCK) {
> +                    bug_reporting_form.setStyle(DISPLAY, NONE);
> +                }
> +            });
> +        });
> +
> +        // Hide the bug reporting form.
> +        bug_reporting_form.setStyle(DISPLAY, NONE);
> +    }
> +
> +    bug_not_reported_button = Y.get('#bug-not-already-reported');
> +    if (Y.Lang.isValue(bug_not_reported_button)) {
> +        // The bug_not_reported_button won't show up if there aren't any
> +        // possible duplicates.
> +        bug_not_reported_button.on('click', show_bug_reporting_form);
> +    }
> +
> +    // Attach the form overlay to the "Yes, this is my bug" forms.
> +    var this_is_my_bug_forms = Y.all('form.this-is-my-bug-form');
> +    Y.each(this_is_my_bug_forms, function(form) {
> +        var subscribe_form_overlay = create_subscribe_overlay(form);
> +
> +        form.on('submit', function(e) {
> +            // We don't care about the original event, so stop it
> +            // and show the form overlay that we just created.
> +            e.halt();
> +            subscribe_form_overlay.show();
> +        });
> +    });
> +};
> +
> +Y.bugs.setup_dupe_finder = function() {
> +    Y.on('domready', function() {
> +        config = {on: {success: set_up_dupe_finder,
> +                       failure: function() {}}};

Another spot where I'm assuming you'll not be landing this branch until
that's fixed.

> +
> +        // Load the filebug form asynchronously. If this fails we
> +        // degrade to the standard mode for bug filing, clicking through
> +        // to the second part of the bug filing form.
> +        var filebug_form_url_element = Y.get(
> +            '#filebug-form-url');
> +        if (Y.Lang.isValue(filebug_form_url_element)) {
> +            var filebug_form_url = filebug_form_url_element.getAttribute(
> +                'href');
> +            Y.io(filebug_form_url, config);
> +        }
> +    });
> +
>  };
>  
>  }, "0.1", {"requires": [
> 
> === modified file 'lib/canonical/launchpad/templates/launchpad-form.pt'
> --- lib/canonical/launchpad/templates/launchpad-form.pt	2009-09-03 15:39:22 +0000
> +++ lib/canonical/launchpad/templates/launchpad-form.pt	2009-11-26 13:39:58 +0000
> @@ -8,7 +8,8 @@
>    <div metal:define-macro="form">
>  
>      <form action="."
> -          tal:attributes="action view/action_url"
> +          tal:attributes="action view/action_url;
> +                          id launchpad_form_id|nothing;"

Might be best putting that in the subsequent branch, if it's not
used in this one.

>            name="launchpadform"
>            method="post"
>            enctype="multipart/form-data"
> 
> === modified file 'lib/lp/bugs/templates/bugtarget-filebug-submit-bug.pt'
> --- lib/lp/bugs/templates/bugtarget-filebug-submit-bug.pt	2009-10-01 12:09:37 +0000
> +++ lib/lp/bugs/templates/bugtarget-filebug-submit-bug.pt	2009-11-26 13:39:58 +0000
> @@ -15,7 +15,9 @@
>              tal:attributes="src string:${lp_js}/bugs/filebug-dupefinder.js"></script>
>      <script type="text/javascript">
>        YUI().use('base', 'node', 'oop', 'event', 'bugs.dupe_finder', function(Y) {
> -        Y.bugs.setup_dupe_finder();
> +          Y.on('domready', function() {
> +              Y.bugs.setup_dupes();
> +          });
>        });
>      </script>
>    </metal:block>
>