Merge lp:~jtv/launchpad/bug-894690 into lp:launchpad

[Jeroen T. Vermeulen]

[r=rvb][bug=894690] Restored vanished status field on translations import queue pages.

= 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

[Raphaël Badin (rvb)]

Looks good :)

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks! I do hope we can improve the testability of this code at some point, but I'm afraid that's just yet another one of those tech-debt exercises.

[Francis J. Lacoste (flacoste)]

On 11-11-25 02:23 PM, Jeroen T. Vermeulen wrote:
> Thanks! I do hope we can improve the testability of this code at some point,
> but I'm afraid that's just yet another one of those tech-debt exercises.

Excuses!

JFDI FTW

--
Francis J. Lacoste
<email address hidden>

=== 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);