Merge lp:~jtv/launchpad/extract-approval-js into lp:launchpad

[Jeroen T. Vermeulen]

Extract translations import queue entry JS, fix chromium layout problem.

= Translations Import Queue Entry: Bug 589567 and Extraction =

The TAL for the translations import queue entry form contains javascript to make the form adjust dynamically to the choice of file type. Fields that don't apply to the selected file type disappear, and ones that do flash into view as soon as a new type is selected.

One small problem with this is that on Chromium, the fields that disappear continue to take up the same space, making for a very awkward form layout.

In this branch I move that javascript out into a .js file of its own. This opens the door to some improvements I'm planning to facilitate import queue review, as well as (I hope) future unit-testing of its individual functions, and either replacing getElemById with something generic or putting it in a more reusable place. I also took the opportunity to clean things up a bit.

Functionally, nothing changes apart from the fix to bug 589567 (the layout problem in Chromium), and the removal of some CSS classes (unused and undefined) that the JS assigned to all form fields as an implementation detail of its initialization algorithm. I fixed the Chromium issue by replacing the "visibility: collapsed" for hidden form rows with "display: none." For that I used a CSS class we already had: unseen.

On top of this I did make some fairly radical, but semantically neutral changes to how the code finds the DOM nodes it needs to show or hide, and how it shows or hides them.

First of these, the way it finds the nodes. The old code found and cached lists of DOM nodes for each file type as the form first rendered. This was quite complicated, and there were then two separate functions to adjust the form: one assumed that all fields were hidden and the other showed a green flash on fields that it made visible. The former was used for initial display (the node-lists initialization hid all fields as a side effect) and the other handles changes of the selected file type by the user.

The new code throws away all this optimization in favour of simplicity. A single method goes through the lists of relevant fields per file type, hides the irrelevant ones, and shows the relevant ones. The green flash is optional, controlled by an argument. There's no more caching of node lists. Only one function needs to know which fields are visible for which file types. For the predominant usage scenario, where the file type never changes, this is actually a lot less work than the optimized version—but I doubt it matters.

Second, there's how the nodes are manipulated. Instead of trying to do everything in a big smart loop I have several very simple loops iterating over the nodes to do one simple thing. This transformation is known as "loop fission." The loop is a bit complex so I extracted it into a function; the manipulation to be applied to each node (show, hide, animate with green flash) is passed in as another function.

The javascript is covered by a windmill test, which only needed a tiny update to the CSS it looks for to recognize hidden form rows plus some drive-by cleanups. To run the test:
{{{
./bin/test -vv -m lp.translations.windmill -t ImportQueueEntry
}}}

I had some timeouts running this locally but it came through EC2 fine (and amazingly, it did actually run the tests). To run the above on EC2, do:
{{{
./utilities/ec2 test -o "-vv -m lp.translations.windmill -t ImportQueueEntry"
}}}

Q/A takes Rosetta or Launchpad admin rights, or membership of the Ubuntu Translations Coordinator team. Go to https://translations.${YOUR_LP_SERVER}/+imports/ and pick an entry. At the bottom of the entry it should say either "Will be imported into" or "No import target selected yet." If you have the necessary privileges, there will be an "edit" link next to that. Follow it to get to the entry's form.

On the form, verify that only fields relevant to the file's type are shown. For instance, a Translations (PO) file will have a Language field but a Template (POT) will not. On the other hand a Template will have a Translation Domain field which a Translations file doesn't. The fields adjust when you select different file types, with new fields appearing in a green flash.

Jeroen

[Jelmer Vernooij (jelmer)]

As I mentioned on IRC my JavaScript knowledge is a bit rusty.

I understand what you're doing and as far as I can tell it matches the JavaScript style guide. The tests fail here locally and ec2, which is a bit of a worry.

I would prefer somebody with a bit more experience reviewing Launchpad JavaScript code has a look as well. The fact that the test (spuriously?) fails is a blocker, it would be interesting to know why it is failing on some machines.

[Jelmer Vernooij (jelmer)]

FWIW, the test failure:

Failure in test lp.translations.windmill.tests.test_import_queue.ImportQueueEntryTest.test_import_queue_entry
Traceback (most recent call last):
_StringException: Text attachment: traceback
------------
Traceback (most recent call last):
  File "/var/launchpad/tmp/eggs/testtools-0.9.2-py2.5.egg/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/var/launchpad/tmp/eggs/testtools-0.9.2-py2.5.egg/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/var/launchpad/test/lib/lp/translations/windmill/tests/test_import_queue.py", line 91, in test_import_queue_entry
    client.waits.forElement(id=u'field.file_type', timeout=u'8000')
  File "/var/launchpad/tmp/eggs/windmill-1.3beta3_lp_r1440-py2.5.egg/windmill/authoring/__init__.py", line 161, in __call__
    return self.exec_method(self.action_name, **kwargs)
  File "/var/launchpad/tmp/eggs/windmill-1.3beta3_lp_r1440-py2.5.egg/windmill/authoring/__init__.py", line 214, in _exec_test
    raise WindmillTestClientException(result['result'])
WindmillTestClientException: {'suite_name': 'Translations import queue entry', 'result': False, 'starttime': '2010-5-30T23:3:7.392Z', 'params': {'id': 'field.file_type', 'timeout': '8000'}, 'output': None, 'endtime': '2010-5-30T23:3:15.491Z', 'method': 'waits.forElement'}
------------

[Jelmer Vernooij (jelmer)]

review: Needs Fixing (code)

[Jeroen T. Vermeulen (jtv)]

Yeah, that's the timeout. I get that for the other windmill tests as well. I thought it was just my local setup (which is why I went to EC2) but perhaps it's more. I'm investigating.

[Michael Nelson (michael.nelson)]

{{{
16:01 < jelmer> noodles: Could you perhaps also have a quick look at jtv's branch? I haven't reviewed JS code before
16:02 < jelmer> not necessarily extensive but just making sure I haven't missed something obvious
16:08 < jelmer> noodles, MP is here: https://code.edge.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852
16:13 < jtv> noodles: I must admit I'm a bit rusty at the JS coding standards myself, so high time I did that cleanup. :)
16:17 < noodles> jtv: me too.. it's been a while.
16:17 < jtv> noodles: or we can just decide that it looks good enough and whatever I did must be the right way. :-)
16:17 < noodles> Yeah, I won't be doing a thorough review... just double checking jelmer's excellent work :)
16:19 < noodles> jtv: how is it that you've got new code (ie. I can't see where it's moved from) with XXX's from henning? ;)
16:20 < noodles> jtv: Ah, you just added henning's name... OK :)
16:20 < jtv> noodles: he wrote it, and I went back to figure out the date so I could meet XXX standards.
16:20 < jtv> "Launchpad, the only major open-source software project with formal XXX standards. Your seal of quality."
16:20 < henninge> jtv: thanks ;)
16:20 < noodles> lol
16:21 * jtv still wants raunchpad.net
16:24 < noodles> jtv, jelmer : Looks great... and yay for future unit tests.
16:24 < jtv> yes, that would be nice wouldn't it?
}}}

[Jelmer Vernooij (jelmer)]

review: Approve (code)

[Michael Nelson (michael.nelson)]

I've not done a thorough review, but have just made sure nothing obvious is missing. As in the MP, the branch seems to clean up previous JS - the only behavioral change being the trivial chromium fix. I've not tested the changes locally but have left that to Jelmer.

Lots of nice cleanups and additional doc strings... thanks for the attention to detail (and sorry I can't give it the same attention right now).

review: Approve (js)

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-06-28 13:59:04 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-07-01 10:46:30 +0000
@@ -226,6 +226,8 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/translations/importqueue.js"></script>
     <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/translations/importqueueentry.js"></script>
+    <script type="text/javascript"
             tal:attributes="src string:${lp_js}/translations/languages.js"></script>
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/translations/pofile.js"></script>

=== added file 'lib/lp/translations/javascript/importqueueentry.js'
--- lib/lp/translations/javascript/importqueueentry.js	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/javascript/importqueueentry.js	2010-07-01 10:46:30 +0000
@@ -0,0 +1,159 @@
+/* Copyright 2010 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * @module lp.translations.importqueueentry
+ * @requires node, lazr.anim
+ */
+
+YUI.add('lp.translations.importqueueentry', function(Y) {
+
+var namespace = Y.namespace('lp.translations.importqueueentry');
+
+
+/**
+ * Groups of form fields to be visible per file type.
+ * @private
+ */
+var field_groups = {
+    'POT': ['name', 'translation_domain', 'languagepack'],
+    'PO': ['potemplate', 'potemplate_name', 'language', 'variant'],
+    'UNSPEC': []
+};
+
+
+var last_file_type = 'UNSPEC';
+
+var hidden_field_class = 'unseen';
+
+
+/**
+ * Find a page element by HTML id.
+ *
+ * Works around a YUI bug.
+ *
+ * @private
+ * @method getElemById
+ * @param elem_id The HTML id of an element on the current page.
+ * @return The DOM Node with that id, or None.
+ */
+function getElemById(elem_id) {
+    /* XXX HenningEggers 2009-03-24: 'elem_id' is a Zope form field, and
+     * triggers YUI bug #2423101.  We'll work around it.
+     */
+    return Y.one(Y.DOM.byId(elem_id));
+}
+
+
+/**
+ * Find the DOM input element for a given zope field.
+ * @private
+ * @method getFormField
+ * @param field_name Name of a zope form field.
+ * @return DOM input field corresponding to the zope form field.
+ */
+function getFormField(field_name) {
+    return getElemById('field.' + field_name);
+}
+
+
+/**
+ * Find the table row that contains the given form field.
+ * @private
+ * @method getFormEntry
+ * @param field_name Name of a zope form field.
+ * @return DOM entry containing the entire zope form field including
+ *  markup.
+ */
+function getFormEntry(field_name) {
+    /* We're interested in the tr tag surrounding the input element that
+     * Zope generated for the field.  The input element's id consists of
+     * the string "field" and the field's name, joined by a dot.
+     */
+    var field = getFormField(field_name);
+    return (field === null) ? null : field.ancestor('tr');
+}
+
+
+/**
+ * Apply function `alteration` to the form fields for a given file type.
+ * @private
+ * @method alterFields
+ * @param file_type Apply to fields associated with this file type.
+ * @param alteration Function to run for each field; takes the field's
+ *  DOM element as an argument.
+ */
+function alterFields(file_type, alteration) {
+    var field_names = field_groups[file_type];
+    if (field_names !== null) {
+        Y.Array.each(field_names, function (field_name) {
+            var tr = getFormEntry(field_name);
+            if (tr !== null) {
+                alteration(tr);
+            }
+        });
+    }
+}
+
+
+/**
+ * Change selected file type.
+ * @private
+ * @param file_type The newly selected file type.
+ * @param interactively Animate the appareance of previously hidden fields.
+ */
+function updateCurrentFileType(file_type, interactively) {
+    // Hide irrelevant fields.
+    var hideElement = function (element) {
+        element.addClass(hidden_field_class);
+    };
+    for (var group in field_groups) {
+        if (group !== file_type) {
+            alterFields(group, hideElement);
+        }
+    }
+
+    // Reveal relevant fields.
+    var showElement = function (element) {
+        element.removeClass(hidden_field_class);
+    };
+    alterFields(file_type, function (element) {
+        element.removeClass(hidden_field_class);
+    });
+
+    if (interactively) {
+        // Animate revealed fields.
+        var animateElement = function (element) {
+            Y.lazr.anim.green_flash({node: element}).run();
+        };
+        alterFields(file_type, animateElement);
+    }
+
+    last_file_type = file_type;
+}
+
+
+/**
+ * Handle change event for current file type.
+ * @private
+ * @method handleFileTypeChange
+ */
+function handleFileTypeChange() {
+    var file_type = this.get('value');
+    if (file_type != last_file_type) {
+        updateCurrentFileType(file_type, true);
+    }
+}
+
+
+/**
+ * Set up the import-queue-entry page.
+ * @method setup_page
+ */
+namespace.setup_page = function () {
+    var file_type_field = getFormField('file_type');
+    var preselected_file_type = file_type_field.get('value');
+    updateCurrentFileType(preselected_file_type, false);
+    file_type_field.on('change', handleFileTypeChange);
+};
+
+}, "0.1", {"requires": ['node', 'lazr.anim']});

=== modified file 'lib/lp/translations/templates/translationimportqueueentry-index.pt'
--- lib/lp/translations/templates/translationimportqueueentry-index.pt	2010-02-10 13:04:10 +0000
+++ lib/lp/translations/templates/translationimportqueueentry-index.pt	2010-07-01 10:46:30 +0000
@@ -10,92 +10,14 @@
   <metal:script fill-slot="head_epilogue">
     <style type="text/css">
     .dont_show_fields {
-        visibility: collapse;
+        display: none;
     }
     </style>
     <script type="text/javascript">
-    LPS.use('node', 'lazr.anim', function(Y) {
-        var fields = {'POT':
-                            ['field.name', 'field.translation_domain',
-                             'field.languagepack'],
-                      'PO':
-                            ['field.potemplate', 'field.potemplate_name',
-                             'field.language', 'field.variant'],
-                      'UNSPEC': []
-                     };
-        var nodes = {};
-        var last_file_type = 'UNSPEC';
-
-        function getElemById(elem_id) {
-            // XXX 'elem_id' is a Zope form field, and triggers
-            // YUI bug #2423101.  We'll work around it.
-            return Y.one(Y.DOM.byId(elem_id));
-        }
-
-        function getEnclosingTR(fieldname) {
-            var field = getElemById(fieldname);
-            if (field == null) {
-                return null;
-            }
-            return field.ancestor('tr');
-        }
-
-        function buildNodeLists() {
-            for (var ftype in fields) {
-                var the_class = ftype + '_row';
-                Y.Array.each(fields[ftype], function(field) {
-                    var tr = getEnclosingTR(field);
-                    if (tr != null) {
-                        tr.addClass(the_class);
-                        tr.addClass('dont_show_fields');
-                    }
-                });
-                nodes[ftype] = Y.all('.' + the_class);
-            }
-        }
-
-        function updateCurrentFileType(file_type) {
-            for (ftype in nodes) {
-                // Logic has been inverted in the next line to avoid
-                // breaking XHTML compliance of the template due to
-                // ampersand usage.
-                if (!(ftype == file_type || nodes[ftype] == null)) {
-                    nodes[ftype].addClass('dont_show_fields');
-                }
-            }
-            if (nodes[file_type] != null) {
-                nodes[file_type].removeClass('dont_show_fields');
-                nodes[file_type].each( function(node) {
-                    var anim = Y.lazr.anim.green_flash({ node: node });
-                    anim.run();
-                });
-            }
-            last_file_type = file_type;
-        }
-
-        function initCurrentFileType(file_type) {
-            // Same as updateCurrentFileType but without collapsing
-            // everything and without the green flash.
-            if (nodes[file_type] != null) {
-                nodes[file_type].removeClass('dont_show_fields');
-            }
-            last_file_type = file_type;
-        }
-
-        function handleFileTypeChange() {
-            var file_type = this.get('value');
-            if (file_type != last_file_type) {
-                updateCurrentFileType(file_type);
-            }
-        }
-
-        Y.on('domready', function(){
-            buildNodeLists();
-            var file_type_field = getElemById('field.file_type');
-            initCurrentFileType(file_type_field.get('value'));
-            file_type_field.on('change', handleFileTypeChange);
-        });
-    });
+      LPS.use('node', 'lp.translations.importqueueentry',
+      function (Y) {
+        Y.on('domready', Y.lp.translations.importqueueentry.setup_page);
+      });
     </script>
   </metal:script>
 

=== modified file 'lib/lp/translations/windmill/tests/test_import_queue.py'
--- lib/lp/translations/windmill/tests/test_import_queue.py	2010-02-18 10:51:34 +0000
+++ lib/lp/translations/windmill/tests/test_import_queue.py	2010-07-01 10:46:30 +0000
@@ -20,6 +20,7 @@
 from lp.translations.windmill.testing import TranslationsWindmillLayer
 from lp.testing import WindmillTestCase
 
+
 class ImportQueueEntryTest(WindmillTestCase):
     """Test that the entries in the import queue can switch types."""
 
@@ -42,13 +43,14 @@
         'field.potemplate',
         'field.language',
     ]
+
     def _getHiddenTRXpath(self, field_id):
         if field_id in self.SELECT_FIELDS:
             input_tag = 'select'
         else:
             input_tag = 'input'
         return (
-            u"//tr[contains(@class,'dont_show_fields')]"
+            u"//tr[contains(@class,'unseen')]"
             u"//%s[@id='%s']" % (input_tag, field_id)
                 )
 
@@ -87,8 +89,7 @@
         # the first ones are hidden. Finally, all fields are hidden if the
         # file type is unspecified.
         client.waits.forElement(id=u'field.file_type', timeout=u'8000')
-        client.asserts.assertSelected(id=u'field.file_type',
-                                           validator=u'POT')
+        client.asserts.assertSelected(id=u'field.file_type', validator=u'POT')
         self._assertAllFieldsVisible(client, 'POT')
         self._assertAllFieldsHidden(client, 'PO')
 
@@ -105,6 +106,7 @@
 IMPORT_STATUS_1 = IMPORT_STATUS % 1
 OPEN_CHOICELIST = u"//div[contains(@class, 'yui-ichoicelist-content')]"
 
+
 class ImportQueueStatusTest(WindmillTestCase):
     """Test that the entries in the import queue can switch types."""