launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/do-it-by-default-bug-872225 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #872225 in Launchpad itself: "Batched comment loading should start on page load"
  https://bugs.launchpad.net/launchpad/+bug/872225

For more details, see:
https://code.launchpad.net/~gmb/launchpad/do-it-by-default-bug-872225/+merge/78948

This branch enable batched comment loading on pageload (rather than waiting for the user to click something). The JSified links are still available as a fallback.

I've added a JS test to cover the calling of load_more_comments() from within setup_load_comments().

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/tests/test_async_comment_loading.js
-- 
https://code.launchpad.net/~gmb/launchpad/do-it-by-default-bug-872225/+merge/78948
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/do-it-by-default-bug-872225 into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-10-06 18:51:36 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-10-11 12:05:27 +0000
@@ -153,7 +153,7 @@
         setup_add_attachment();
         setup_link_branch_picker();
         if (batched_comment_loading_enabled) {
-            namespace.setup_load_comments();
+            namespace.setup_load_comments(true);
         }
     }, window);
 };
@@ -1173,20 +1173,32 @@
  *  via javascript.
  *
  * @method setup_load_comments
+ * @param load_more_comments {Boolean} If True, load_more_comments will
+ *        be called immediately.
  */
-namespace.setup_load_comments = function() {
-    var show_comments_links = Y.all('.show-comments-link');
-    if (show_comments_links) {
+namespace.setup_load_comments = function(load_more_comments) {
+    var comments_container = Y.one('#comments-container');
+    if (Y.Lang.isValue(comments_container)) {
         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');
-        Y.each(show_comments_links, function(link) {
-            namespace.setup_show_more_comments_link(
-                link, batched_comments_url, comments_container);
-        });
+        // Set up the show more comments links to be JSified. This gives
+        // us a fallback should the initial load_more_comments call fail
+        // for some reason, rather than leaving the user without any way
+        // to view all the comments.
+        var show_comments_links = Y.all('.show-comments-link');
+        if (show_comments_links) {
+            Y.each(show_comments_links, function(link) {
+                namespace.setup_show_more_comments_link(
+                    link, batched_comments_url, comments_container);
+            });
+        }
+        if (load_more_comments === true) {
+            namespace.load_more_comments(
+                batched_comments_url, comments_container);
+        }
     }
 };
 

=== modified file 'lib/lp/bugs/javascript/tests/test_async_comment_loading.js'
--- lib/lp/bugs/javascript/tests/test_async_comment_loading.js	2011-10-05 16:42:59 +0000
+++ lib/lp/bugs/javascript/tests/test_async_comment_loading.js	2011-10-11 12:05:27 +0000
@@ -35,10 +35,15 @@
             config.on.success();
           };
         LP = {
-          'cache': {
-            'bug': {
-              self_link: "http://bugs.example.com/bugs/1234";
-          }}};
+            'cache': {
+                'bug': {
+                    self_link: "http://bugs.example.com/bugs/1234";
+                },
+                'context': {
+                    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;
@@ -139,6 +144,27 @@
         Assert.isTrue(link.hasClass('js-action'));
         link.simulate('click');
         Assert.isTrue(load_more_comments_called);
+    },
+
+    /**
+     * setup_load_comments() will call load_more_comments if it's told
+     * to.
+     */
+    test_setup_load_comments_calls_load_more_comments: function() {
+        // Monkey-patch load_more_comments for the purposes of this
+        // test.
+        var load_more_comments_called = false;
+        module.load_more_comments = function() {
+            load_more_comments_called = true;
+        };
+        // A parameterless call to setup_load_comments won't call
+        // load_more_comments.
+        module.setup_load_comments();
+        Assert.isFalse(load_more_comments_called);
+        // Passing true for the load_more_comments parameter will cause
+        // load_more_comments to be called.
+        module.setup_load_comments(true);
+        Assert.isTrue(load_more_comments_called);
     }
 
 }));