launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/es3-safety into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #372638 in Launchpad itself: "Import queue error display: "null" above output panel"
  https://bugs.launchpad.net/launchpad/+bug/372638
  Bug #372643 in Launchpad itself: "Import queue error display is one long line"
  https://bugs.launchpad.net/launchpad/+bug/372643
  Bug #992663 in Launchpad itself: "Inconsistent filtering on +branches and +recipes"
  https://bugs.launchpad.net/launchpad/+bug/992663
  Bug #999941 in Launchpad itself: "lp.app.links.harvest_links errors in ES3 browsers"
  https://bugs.launchpad.net/launchpad/+bug/999941
  Bug #1000002 in Launchpad itself: "lp.registry.team.mailinglists uses native js arrays"
  https://bugs.launchpad.net/launchpad/+bug/1000002

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/es3-safety/+merge/106058

Pre-implementation: no one

There are several easy MSIE/ES3 related bugs that are now trivial to fix.

--------------------------------------------------------------------

RULES

    * Import queue error display is one long line:
      The pre element is missing class="wrap"
    * Import queue error display: "null" above output panel:
      Do not pass null to innerHTML, the attribute expects a string.
    * lp.app.links.harvest_links errors in ES3 browsers
      Use Y.Array.indexOf(iterable, needle)
    * lp.registry.team.mailinglists uses native js arrays
      Use Y.Array.indexOf().


QA

    The team mailing list API is not live yet. It cannot be QAed.

    Using MSIE
    * Visit https://translations.qastaging.launchpad.net/+imports/+index
    * Expand "docs/games/po/games-pt_BR.po..."
    * Verify the text wraps
    * Verify that the text "null" does not appear before the error message.

    * Visit https://code.qastaging.launchpad.net/~sinzui/gdp/packaging
    * propose a merge into lp://qastaging/gdp
    * Verify the Mp loads without an error (object does not have method).
    * Visit https://code.launchpad.net/pocket-lint/+branches
    * Verify the page reloads when you change the status or sort.



LINT

    lib/lp/app/javascript/lp-links.js
    lib/lp/registry/javascript/team_mailinglists.js
    lib/lp/translations/javascript/importqueue.js
    lib/lp/translations/javascript/tests/test_importqueue.js


TEST

    ./bin/test -vvc --layer=YUITest


IMPLEMENTATION

The linkifyer code assumed that all browsers with JS supports Array.indexOf.
This is a trivial fix. The code is tested, but html5-browser will always
pass since it does support Array.indexOf.
    lib/lp/app/javascript/lp-links.js

The the future mailing list code assumed Y.Array() returns a ES5 compatible
array. It does not. It returns a native array. There are two pointless
calls to Y.Array in the code and there are two calls to Array.IndexOf() that
needs to use Y.Array.indexOf().
This is a trivial fix. The code is tested, but html5-browser will always
pass since it does support Array.indexOf.
    lib/lp/registry/javascript/team_mailinglists.js

The CSS bug was visible in chromium. It was easy to fix. The passing of
null to a method that requires a string was also trivial to fix. I added
tests for the helper method.
    lib/lp/translations/javascript/importqueue.js
    lib/lp/translations/javascript/tests/test_importqueue.js
-- 
https://code.launchpad.net/~sinzui/launchpad/es3-safety/+merge/106058
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/es3-safety into lp:launchpad.

=== modified file 'lib/lp/app/javascript/lp-links.js'
--- lib/lp/app/javascript/lp-links.js	2011-09-17 16:00:20 +0000
+++ lib/lp/app/javascript/lp-links.js	2012-05-16 21:55:21 +0000
@@ -15,7 +15,7 @@
         var link_info = [];
         Y.all('.'+link_class).each(function(link) {
             var href = link.getAttribute('href');
-            if( link_info.indexOf(href)<0 ) {
+            if (Y.Array.indexOf(link_info, href) < 0) {
                 link_info.push(href);
             }
         });

=== modified file 'lib/lp/registry/javascript/team_mailinglists.js'
--- lib/lp/registry/javascript/team_mailinglists.js	2012-03-15 05:40:30 +0000
+++ lib/lp/registry/javascript/team_mailinglists.js	2012-05-16 21:55:21 +0000
@@ -88,8 +88,8 @@
     },
 
     display_messages: function () {
-        var mustache_model = Y.Array([]);
-        var processed_ids = Y.Array([]);
+        var mustache_model = [];
+        var processed_ids = [];
         var messages = Y.Array(this.get('messages'));
         var container = this.get('container');
         this._create_mustache_model(
@@ -107,7 +107,7 @@
         var i;
         filter_func = function(item) {
             var nested_ids = messages[i].nested_messages;
-            var index = nested_ids.indexOf(item.message_id);
+            var index = Y.Array.indexOf(nested_ids, item.message_id);
             return (index !== -1);
         };
         for (i = 0; i < messages.length; i++) {
@@ -115,7 +115,7 @@
             // Only create mustache data for messages not already processed.
             // Messages will have been processed already if they were part of
             // the nested messages for an earlier message in the list.
-            if (processed_ids.indexOf(message.message_id) === -1) {
+            if (Y.Array.indexOf(processed_ids, message.message_id) === -1) {
                 processed_ids.push(message.message_id);
                 mustache_model.push({
                     message_id: message.message_id,

=== modified file 'lib/lp/translations/javascript/importqueue.js'
--- lib/lp/translations/javascript/importqueue.js	2012-05-16 16:52:56 +0000
+++ lib/lp/translations/javascript/importqueue.js	2012-05-16 21:55:21 +0000
@@ -70,14 +70,14 @@
  * Factory for error-output request (and response handlers) for a given
  * output panel.
  */
-var output_loader = function(node) {
+namespace._output_loader = function(node) {
     return {
         on: {
             success: function(entry) {
                 var output_block = entry.get('error_output');
-                var error_pre = node.create('<pre></pre>');
+                var error_pre = node.create('<pre class="wrap"></pre>');
                 error_pre.appendChild(document.createTextNode(output_block));
-                node.set('innerHTML', null);
+                node.set('innerHTML', '');
                 node.appendChild(error_pre);
             },
             failure: function(errcode) {
@@ -105,7 +105,7 @@
     var entry_uri = '+imports/' + entry_id;
     var div = output.one('div');
     var lp = new Y.lp.client.Launchpad();
-    lp.get(entry_uri, output_loader(div));
+    lp.get(entry_uri, namespace._output_loader(div));
 };
 
 /**

=== modified file 'lib/lp/translations/javascript/tests/test_importqueue.js'
--- lib/lp/translations/javascript/tests/test_importqueue.js	2012-04-28 03:11:33 +0000
+++ lib/lp/translations/javascript/tests/test_importqueue.js	2012-05-16 21:55:21 +0000
@@ -2,7 +2,7 @@
  * GNU Affero General Public License version 3 (see the file LICENSE).
  */
 
-YUI().use('lp.testing.runner', 'test', 'console',
+YUI().use('lp.testing.runner', 'test', 'console', 'lp.client',
     'lp.translations.importqueue', function(Y) {
 
 var suite = new Y.Test.Suite("importqueue Tests");
@@ -63,5 +63,32 @@
     }
 }));
 
+suite.add(new Y.Test.Case({
+    name: 'helpers',
+
+    test_output_loader_failure: function() {
+        var output_node = Y.Node.create("<div></div>");
+        var handlers = namespace._output_loader(output_node);
+        handlers.on.failure('ignored');
+        var output = output_node.get('children');
+        Y.Assert.areEqual(1, output._nodes.length);
+        Y.Assert.areEqual('STRONG', output.item(0).get('tagName'));
+    },
+
+    test_output_loader_success: function() {
+        var output_node = Y.Node.create("<div>temporary</div>");
+        var handlers = namespace._output_loader(output_node);
+        var entry = new Y.lp.client.Entry();
+        entry.set('error_output', 'testing<escaped>');
+        handlers.on.success(entry);
+        var output = output_node.get('children');
+        Y.Assert.areEqual(1, output._nodes.length);
+        var pre_node = output.item(0);
+        Y.Assert.areEqual('PRE', pre_node.get('tagName'));
+        Y.Assert.isTrue(pre_node.hasClass('wrap'));
+        Y.Assert.areEqual('testing<escaped>', pre_node.get('text'));
+    }
+}));
+
 Y.lp.testing.Runner.run(suite);
 });