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

> > === 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.
>

Right. Fixed.

> > @@ -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.
>

I agree. Fixed.

> > +
> > + 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.
> o

I've heard similar from other people. I've changed the setStyle() calls
to (add|remove)Class() as appropriate.

> > + 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.
>

Agreed and fixed.

> > + search_button.set('value', 'Check again');
> > + search_button.setStyle(DISPLAY, INLINE);
>
> Perhaps disable the sar (and re-enable here) the search button rather than
> making it disappear altogether? Taking it out of the page might mean
> the browser has to do layout again.
>

Hmm, possibly. The disappearance of the button was a UI request from
beuno back in the day, so I think we should leave it as-is for now and
see how testing on edge goes.

> > }
> >
> > 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"]});
> >