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);
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
