Code review comment for lp:~gmb/launchpad/ajax-dupefinder-has-risen-from-the-grave

Hi Graham,

Not a lot to say. I have a few suggestions, but that's all they
are. Looking good :)

Gavin.

> === modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
> --- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-08 10:57:44 +0000
> +++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 12:13:43 +0000
> @@ -81,13 +81,6 @@
> * @param e The Event triggering this function.
> */
> function show_bug_reporting_form(e) {
> - if (Y.Lang.isValue(bug_already_reported_expanders)) {
> - // Collapse any duplicate-details divs.
> - Y.each(bug_already_reported_expanders, function(expander) {
> - collapse_bug_details(expander);
> - });
> - }
> -
> // If the bug reporting form is in a hidden container, as it is on
> // the AJAX dupe search, show it.
> var filebug_form_container = Y.get('#filebug-form-container');

Salgado, in his YUI() -> LPS branch, changed a lot of Y.get() calls to
Y.one(). I guess that means things break non-silently when the targets
don't exist. Might be a good idea here too.

> @@ -117,8 +110,37 @@
> * display them in-line.
> */
> function search_for_and_display_dupes() {
> - function show_failure_message() {
> - Y.get('#possible-duplicates').set(INNER_HTML, 'FAIL');
> + function show_failure_message(transaction_id, response, args) {
> + // If the request failed due to a timeout, display a message
> + // explaining how the user may be able to work around it.
> + var error_message = '';
> + if (response.status == 503) {
> + // We treat 503 (service unavailable) as a timeout because
> + // that's what timeouts in LP return.
> + error_message =
> + "<p>Searching for your bug in Launchpad took too long. " +
> + "Try reducing the number of words in the summary " +
> + "field and click \"Check again\" to retry your search. " +
> + "Alternatively, you can enter the details of your bug " +
> + "below.</p>";
> + } else {
> + // Any other error code gets a generic message.
> + error_message =
> + "<p>An error occured whilst trying to find bugs matching " +
> + "the summary you entered. Click \"Check again\" to retry " +
> + "your search. Alternatively, you can enter the " +
> + "details of your bug below.</p>";
> + }
> +
> + Y.get('#possible-duplicates').set(INNER_HTML, error_message);

It works, but I think using innerHTML is bad except for a few
cases. Consider having error_message as just a message, then create a
<p> node and .set('text', error_message). Don't stress too much
though.

> +
> + Y.get('#spinner').setStyle(DISPLAY, NONE);

Consider using the "unseen" CSS class instead, and remove it to make
the spinner visible. Doesn't matter too much though. I'm sure I've
heard arguments that classes are better than manipulating style
directly, but I can't summon them right now.

> + show_bug_reporting_form();
> +
> + Y.get(Y.DOM.byId('field.title')).set(
> + 'value', search_field.get('value'));

I think this would be clearer and cleaner as:

   Y.one('#bug_reporting_form input[name=field.title]').set('value', ...)

I wish Zope forms didn't define ids for fields. It makes it more
difficult to combine different forms into a page.

> + search_button.set('value', 'Check again');
> + search_button.setStyle(DISPLAY, INLINE);

Perhaps disable the search button (and re-enable here) rather than
making it disappear altogether? Taking it out of the page might mean
the browser has to do layout again.

> }
>
> function on_success(transaction_id, response, args) {
> @@ -131,18 +153,11 @@
> bug_already_reported_expanders = Y.all(
> 'img.bug-already-reported-expander');
> if (Y.Lang.isValue(bug_already_reported_expanders)) {
> - // If there are duplicates shown, change the title of the page
> - // and set up the JavaScript of the duplicates that have been
> - // returned.
> + // If there are duplicates shown, set up the JavaScript of
> + // the duplicates that have been returned.
> Y.bugs.setup_dupes();
> - Y.get('#page-title').set(
> - INNER_HTML,
> - 'Is the bug you&rsquo;re reporting one of these?');
> } else {
> - // Otherwise, set the title to one that doesn't suggest
> - // there were dupes returned and show the bug reporting
> - // form.
> - Y.get('#page-title').set(INNER_HTML, 'Report a bug');
> + // Otherwise, show the bug reporting form.
> show_bug_reporting_form();
> }
>
> @@ -242,7 +257,7 @@
> var weight = radio_button.get('checked') ? 'bold' : 'normal';
> radio_button.get('parentNode').setStyle('fontWeight', weight);
> });
> - });
> + }
>
> return subscribe_form_overlay;
> }
> @@ -271,18 +286,18 @@
> search_field.set('name', 'field.search');
> search_field.set('id', 'field.search');
>
> - // 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')
> -
> // 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);
> + // Set up the handler for the search form.
> + search_form = Y.get('#filebug-search-form');
> + search_form.on('submit', function(e) {
> + // Prevent the event from propagating; we don't want to reload
> + // the page.
> + e.halt();
> + search_for_and_display_dupes();
> + });
> }
>
> Y.bugs.setup_dupes = function() {
> @@ -352,11 +367,6 @@
>
> 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);
> - }
> });
> });
>
> @@ -386,6 +396,7 @@
> };
>
> Y.bugs.setup_dupe_finder = function() {
> + Y.log("In setup_dupe_finder");
> Y.on('domready', function() {
> config = {on: {success: set_up_dupe_finder,
> failure: function() {}}};
> @@ -404,4 +415,4 @@
> };
>
> }, "0.1", {"requires": [
> - "base", "oop", "node", "event", "lazr.formoverlay", "lazr.effects"]});
> + "base", "io", "oop", "node", "event", "lazr.formoverlay", "lazr.effects"]});
>