launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-894690 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #894690 in Launchpad itself: "Translations import queue pages no longer show statuses"
  https://bugs.launchpad.net/launchpad/+bug/894690

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-894690/+merge/83413

= Summary =

The entry-status field (including selector, if appropriate) went missing on the translations import queue pages.  We need these fields!


== Proposed fix ==

The cause was a change that replaced a style="display:none" attribute with a more appropriate class="hidden".  Why did that break things?  Because the page's javascript revealed the field by doing setStyle('display', '').

Now that the field is hidden by a class instead of a style attribute, it should be revealed through a removeClass('hidden').


== Pre-implementation notes ==

Raphaël followed my reasoning.  I didn't consult anyone on the fix as such.

This kind of bug is surprisingly hard to test for.  Even if the authors of the code could have seen the change coming, it's hard to catch: a full-blown python/TAL/javascript test might well miss it.  And such a test could be unfeasibly expensive for this kind of detail.


== Implementation details ==

I also fixed a bit of lint, where a comparison was done as “==” instead of the seemingly more appropriate “===”.


== Tests ==

You've got me there.  How does one test this effectively?  I tried setting up a new unit test, to at least test that the function now removes the "hidden" attribute if it's there.  But I found myself unable to get it to work, as well as unable to pinpoint the failure in the debugger (although I did get to see a nice firefox crash).  And yet the clock is ticking while users are hindered in their work.

So, with reluctance and self-loathing, I offer no test for this change.  I'm sorry; I have failed you.


== Demo and Q/A ==

Go to any of the translations import queue pages, such as:

    https://translations.qastaging.launchpad.net/+imports

(Or similar on dev, which is what I did, or elsewhere.)

The right-hand side of the page should show the status of each entry — Approved, Needs Review, Blocked, Imported, Failed, or Needs Information.  This column is currently missing.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/javascript/importqueue.js
-- 
https://code.launchpad.net/~jtv/launchpad/bug-894690/+merge/83413
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-894690 into lp:launchpad.

=== modified file 'lib/lp/translations/javascript/importqueue.js'
--- lib/lp/translations/javascript/importqueue.js	2011-02-24 00:23:04 +0000
+++ lib/lp/translations/javascript/importqueue.js	2011-11-25 16:06:25 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2009 Canonical Ltd.  This software is licensed under the
+/* Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * @module lp.translations.importqueue
@@ -116,7 +116,8 @@
  * which is defined in a code fragment that is included in the page via TAL.
  */
 var init_status_choice = function(content_box, index, list) {
-    content_box.setStyle('display', '');
+    // Reveal the status widget.
+    content_box.removeClass('hidden');
     var conf = choice_confs[index];
     conf.title = 'Change status to';
     conf.contentBox = content_box;
@@ -135,7 +136,7 @@
             on: {
                 success: function(entry) {
                     Y.Array.each(conf.items, function(item) {
-                        if (item.value == new_status) {
+                        if (item.value === new_status) {
                             value_box.addClass(item.css_class);
                         } else {
                             value_box.removeClass(item.css_class);