launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/jsify-bottom-link-bug-867514 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #867514 in Launchpad itself: "The bottom "view all comments" page on bugs with lots of comments doesn't get JS-ified"
  https://bugs.launchpad.net/launchpad/+bug/867514
  Bug #867516 in Launchpad itself: "Expando-triangle shouldn't appear in the "x comments hidden" box"
  https://bugs.launchpad.net/launchpad/+bug/867516

For more details, see:
https://code.launchpad.net/~gmb/launchpad/jsify-bottom-link-bug-867514/+merge/78288

This branch fixes two bugs in the new async bug comment loading code. The "show all comments" link in the notice at the footer of the page is now JS-ified and the expando-triangle in the middle box is no longer shown.
-- 
https://code.launchpad.net/~gmb/launchpad/jsify-bottom-link-bug-867514/+merge/78288
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/jsify-bottom-link-bug-867514 into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-09-26 06:30:07 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-10-05 17:12:29 +0000
@@ -1156,6 +1156,21 @@
     var request = io_provider.io(batched_comments_url, {on: handlers});
 };
 
+/**
+ * Set up the click handling for a single show-more-comments link.
+ *
+ * @method setup_show_more_comments_link
+ * @param link {Node} The link to set up.
+ * @param url {Object} The current batched_comments_url
+ * @param container {Node} The node into which to load the comments.
+ */
+namespace.setup_show_more_comments_link = function(link, url, container) {
+    link.on('click', function(e) {
+        e.preventDefault();
+        namespace.load_more_comments(url, container);
+    });
+    link.addClass('js-action');
+};
 
 /**
  *  Set up click handling to load the rest of the comments for the bug
@@ -1164,20 +1179,18 @@
  * @method setup_load_comments
  */
 namespace.setup_load_comments = function() {
-    var show_comments_link = Y.one('#show-comments-link');
-    if (Y.Lang.isValue(show_comments_link)) {
+    var show_comments_links = Y.all('.show-comments-link');
+    if (show_comments_links) {
         var current_offset = LP.cache.initial_comment_batch_offset;
         var batched_comments_url =
             LP.cache.context.self_link.replace('/api/devel', '') +
             "/+batched-comments?offset=" +
             current_offset;
         var comments_container = Y.one('#comments-container');
-        show_comments_link.on('click', function(e) {
-            e.preventDefault();
-            namespace.load_more_comments(
-                batched_comments_url, comments_container);
+        Y.each(show_comments_links, function(link) {
+            namespace.setup_show_more_comments_link(
+                link, batched_comments_url, comments_container);
         });
-        show_comments_link.addClass('js-action');
     }
 };
 

=== modified file 'lib/lp/bugs/javascript/tests/test_async_comment_loading.js'
--- lib/lp/bugs/javascript/tests/test_async_comment_loading.js	2011-09-26 06:30:07 +0000
+++ lib/lp/bugs/javascript/tests/test_async_comment_loading.js	2011-10-05 17:12:29 +0000
@@ -7,6 +7,7 @@
     fetchCSS: false
       }).use('event', 'lp.bugs.bugtask_index', 'lp.client', 'node',
              'lp.testing.mockio', 'test', 'widget-stack', 'console',
+             'node-event-simulate',
              function(Y) {
 
 
@@ -38,6 +39,10 @@
             'bug': {
               self_link: "http://bugs.example.com/bugs/1234";
           }}};
+        // Some tests monkey-patch load_more_comments, so we save it
+        // here to restore it after each test.
+        this.old_load_more_comments = module.load_more_comments;
+
         // Add some HTML to the page for us to use.
         this.comments_container = Y.Node.create(
             '<div id="comments-container"></div>');
@@ -50,6 +55,8 @@
     tearDown: function() {
         this.comments_container.remove();
         this.add_comment_form_container.remove();
+        // Restore load_more_comments in case it's been monkey-patched.
+        module.load_more_comments = this.old_load_more_comments;
     },
 
     /**
@@ -113,6 +120,25 @@
             '<div>' + comments_markup + '</div>';
         Assert.areEqual(
             expected_markup, this.comments_container.get('innerHTML'));
+    },
+
+    /**
+     * setup_show_more_comments_link() will set the onClick handler for
+     * the link passed to it, and will also add a js-action class to the
+     * link.
+     */
+    test_setup_show_more_comments_link_jsifies_link: function() {
+        // We monkey-patch load_mode_comments so that we can use it to
+        // test whether the link has been JS-ified properly.
+        var load_more_comments_called = false;
+        module.load_more_comments = function() {
+            load_more_comments_called = true;
+        };
+        var link = Y.Node.create('<a href="#">A link</a>');
+        module.setup_show_more_comments_link(link, '#', Y.Node.create());
+        Assert.isTrue(link.hasClass('js-action'));
+        link.simulate('click');
+        Assert.isTrue(load_more_comments_called);
     }
 
 }));

=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
--- lib/lp/bugs/templates/bugcomment-macros.pt	2011-09-07 11:26:19 +0000
+++ lib/lp/bugs/templates/bugcomment-macros.pt	2011-10-05 17:12:29 +0000
@@ -68,7 +68,7 @@
             <td class="bug-comment-index">
               <a href="?comments=all"
                  id="show-comments-link"
-                 class="sprite retry"
+                 class="show-comments-link sprite retry"
                  style="white-space: nowrap">
                  view all <span
                  tal:replace="view/total_comments"

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2011-09-10 01:11:45 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2011-10-05 17:12:29 +0000
@@ -319,7 +319,8 @@
             <tal:what-next
                 define="view_all_href
                         string:${context/fmt:url}?comments=all">
-              <a href="#" tal:attributes="href view_all_href">
+              <a class="show-comments-link"
+                  href="#" tal:attributes="href view_all_href">
                 View all <span
                 tal:replace="view/total_comments" />
                 comments</a> or <a href="#" tal:attributes="href