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

Revision history for this message
Graham Binns (gmb) wrote :

Oops. Incremental diff for the first two parts of the code / js reviews:

=== modified file 'lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js'
--- lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 13:05:28 +0000
+++ lib/canonical/launchpad/javascript/bugs/filebug-dupefinder.js 2009-12-09 13:20:27 +0000
@@ -16,7 +16,8 @@
     INNER_HTML = 'innerHTML',
     LAZR_CLOSED = 'lazr-closed',
     NONE = 'none',
- SRC = 'src';
+ SRC = 'src',
+ UNSEEN = 'unseen';

 var bugs = Y.namespace('bugs');

@@ -118,34 +119,36 @@
             // 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. " +
+ "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>";
+ "below.";
         } else {
             // Any other error code gets a generic message.
             error_message =
- "<p>An error occured whilst trying to find bugs matching " +
+ "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>";
+ "details of your bug below.";
         }

- Y.one('#possible-duplicates').set(INNER_HTML, error_message);
+ var error_node = Y.Node.create('<p></p>');
+ error_node.set('text', error_message);
+ Y.one('#possible-duplicates').appendChild(error_node);

- Y.one('#spinner').setStyle(DISPLAY, NONE);
+ Y.one('#spinner').addClass(UNSEEN);
         show_bug_reporting_form();

         Y.one(Y.DOM.byId('field.title')).set(
             'value', search_field.get('value'));
         search_button.set('value', 'Check again');
- search_button.setStyle(DISPLAY, INLINE);
+ search_button.removeClass(UNSEEN);
     }

     function on_success(transaction_id, response, args) {
         // Hide the spinner and show the duplicates.
- Y.one('#spinner').setStyle(DISPLAY, NONE);
+ Y.one('#spinner').addClass(UNSEEN);

         var duplicate_div = Y.one('#possible-duplicates');
         duplicate_div.set(INNER_HTML, response.responseText);
@@ -163,13 +166,13 @@

         // Copy the value from the search field into the title field
         // on the filebug form.
- Y.one(Y.DOM.byId('field.title')).set(
+ Y.one('#bug_reporting_form input[name=field.title]').set(
             'value', search_field.get('value'));

         // Finally, change the label on the search button and show it
         // again.
         search_button.set('value', 'Check again');
- search_button.setStyle(DISPLAY, INLINE);
+ search_button.removeClass(UNSEEN);
     }

     var search_term = encodeURI(search_field.get('value'));
@@ -179,8 +182,8 @@

     // Hide the button, show the spinner and clear the contents of the
     // possible duplicates div.
- search_button.setStyle(DISPLAY, NONE);
- Y.one('#spinner').setStyle(DISPLAY, INLINE);
+ search_button.addClass(UNSEEN);
+ Y.one('#spinner').removeClass(UNSEEN);
     Y.one('#possible-duplicates').set(INNER_HTML, '');

     config = {on: {success: on_success,
@@ -353,7 +356,7 @@

                     // If the bug reporting form is shown, hide it.
                     if (bug_reporting_form.getStyle(DISPLAY) == BLOCK) {
- bug_reporting_form.setStyle(DISPLAY, NONE);
+ bug_reporting_form.addClass(UNSEEN);
                     }
                 }
             });
@@ -371,7 +374,7 @@
         });

         // Hide the bug reporting form.
- bug_reporting_form.setStyle(DISPLAY, NONE);
+ bug_reporting_form.addClass(UNSEEN);
     }

     bug_not_reported_button = Y.one('#bug-not-already-reported');

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py 2009-12-04 12:12:35 +0000
+++ lib/lp/bugs/browser/bugtarget.py 2009-12-09 13:45:24 +0000
@@ -707,7 +707,7 @@
         If a token was passed to this view, it will be be passed through
         to the inline bug filing form via the returned URL.
         """
- url = urlappend(canonical_url(self.context), '+filebug-inline-form')
+ url = canonical_url(self.context, view_name='+filebug-inline-form')
         if self.extra_data_token is not None:
             url = urlappend(url, self.extra_data_token)
         return url
@@ -715,7 +715,7 @@
     @property
     def duplicate_search_url(self):
         """The URL to the inline duplicate search view."""
- url = urlappend(canonical_url(self.context), '+filebug-show-similar')
+ url = canonical_url(self.context, view_name='+filebug-show-similar')
         if self.extra_data_token is not None:
             url = urlappend(url, self.extra_data_token)
         return url

=== modified file 'lib/lp/bugs/templates/bugtarget-filebug-search.pt'
--- lib/lp/bugs/templates/bugtarget-filebug-search.pt 2009-12-09 10:59:47 +0000
+++ lib/lp/bugs/templates/bugtarget-filebug-search.pt 2009-12-09 13:45:24 +0000
@@ -92,7 +92,7 @@
               </td>
               <td style="text-align: left;">
                 <input tal:replace="structure view/actions/field.actions.search/render" />
- <span id="spinner" style="text-align: center; display: none;">
+ <span id="spinner" style="text-align: center" class="unseen">
                   <img src="/@@/spinner" />
                 </span>
               </td>

« Back to merge proposal