Merge lp:~gmb/launchpad/show-comment-form into lp:launchpad

[Graham Binns]

[r=abentley][ui=none] [r=abentley][bug=838825] Once all comments have been loaded asynchronously, the comment form will now appear.

This branch fixes bug 838825 by making the add comment form appear once all a bug's comments have finished loading asynchronously.

The Javascript change is simple, and I've refactored the comment form template into a macro so that we're not cluttering up bugtask-index.pt more than necessary. I've also cleaned up some lint as a drive-by fix.

QA Plan
=======

 - Make sure that the bugs.batched_comment_loading.enabled flag is set to 'on' on qastaging
 - Visit a bug with lots of comments
 - Click the "show all comments" link
 - All the comments should load
 - The comment form should appear at the bottom

[Aaron Bentley (abentley)]

I don't see any tests, but AFAIK, this is testable. Please add tests or an explanation of why this cannot be tested.

review: Needs Fixing

[Graham Binns (gmb)]

On 7 September 2011 16:09, Aaron Bentley <email address hidden> wrote:
> Review: Needs Fixing
> I don't see any tests, but AFAIK, this is testable.  Please add tests or an explanation of why this cannot be tested.

This isn't tested for the simple reason that testing it properly
requires some kind of integration test. Deryck and Gary's YUI tests
with fixtures work hasn't landed yet and I'm loath to add Windmill
tests unless I absolutely have to.

This code is feature-flagged, so I'm not concerned about untested code
breaking out into the wild. I consider having tests to be one of the
criteria for considering this feature "done."

I've added a comment to this effect to the JS.

[Graham Binns (gmb)]

At Aaron's request I've added some JS tests for the async comment loading code. I've also added a couple of standard_test_templates (HTML and JS) to save people from having to go and cargo-cult from others' work like I did.

[Aaron Bentley (abentley)]

Thanks for adding tests. I know it's a pain being the first to test previously-untested code.

I'm sorry you went through the effort of cargo-culting from others' work. There are already templates at https://dev.launchpad.net/JavascriptUnitTesting so please ensure the two sets of templates are in sync (or delete one).

I notice you don't un-monkeypatch Y.lp.client.Launchpad.prototype.named_post, but I guess it only applies to these tests, so that is fine.

review: Approve

[Graham Binns (gmb)]

Oh! I had no idea those existed. I'll sync them up.

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-09-05 23:14:24 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-09-10 01:15:25 +0000
@@ -1096,37 +1096,52 @@
  *        comments.
  */
 namespace.load_more_comments = function(batched_comments_url,
-                                        comments_container) {
+                                        comments_container,
+                                        io_provider) {
     var spinner = Y.Node.create(
-        '<img src="/@@/spinner" style="text_align: center; display: none" />');
+        '<img src="/@@/spinner" style="text_align: center; ' +
+        'display: none" />');
     var spinner_span = Y.one('#more-comments-spinner');
     spinner_span.setStyle('display', 'inline');
     var handlers = {
-        success: function(transactionid, response, arguments) {
+        success: function(transactionid, response, args) {
             var new_comments_node =
                 Y.Node.create("<div></div>");
             new_comments_node.set(
                 'innerHTML', response.responseText);
             spinner_span.setStyle('display', 'none');
             comments_container.appendChild(new_comments_node);
-            var success_anim = Y.lp.anim.green_flash(
-                {node: new_comments_node});
-            success_anim.run();
+            if (Y.Lang.isValue(Y.lp.anim)) {
+                var success_anim = Y.lp.anim.green_flash(
+                    {node: new_comments_node});
+                success_anim.run();
+            }
             batch_url_div = Y.one('#next-batch-url');
             if (Y.Lang.isValue(batch_url_div)) {
                 batched_comments_url = batch_url_div.get(
                     'innerHTML');
                 batch_url_div.remove();
                 namespace.load_more_comments(
-                    batched_comments_url, comments_container);
+                    batched_comments_url, comments_container, io_provider);
             } else {
-                // Remove the comments-hidden message to avoid
+                // Remove the comments-hidden messages to avoid
                 // confusion.
-                Y.one('#comments-hidden-message').remove();
+                Y.each(Y.all('.comments-hidden-message'), function(message) {
+                    message.remove();
+                });
+                // Show the comment form, if available.
+                var comment_form_container = Y.one(
+                    '#add-comment-form-container');
+                if (Y.Lang.isValue(comment_form_container)) {
+                    comment_form_container.toggleClass('hidden');
+                }
             }
         }
     };
-    var request = Y.io(batched_comments_url, {on: handlers});
+    if (!Y.Lang.isValue(io_provider)) {
+        io_provider = Y.io;
+    }
+    var request = io_provider.io(batched_comments_url, {on: handlers});
 };
 
 
@@ -1152,7 +1167,7 @@
         });
         show_comments_link.addClass('js-action');
     }
-}
+};
 
 
 }, "0.1", {"requires": ["base", "oop", "node", "event", "io-base",

=== added file 'lib/lp/bugs/javascript/tests/test_async_comment_loading.html'
--- lib/lp/bugs/javascript/tests/test_async_comment_loading.html	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_async_comment_loading.html	2011-09-10 01:15:25 +0000
@@ -0,0 +1,55 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+<html>
+  <head>
+  <title>Status Editor</title>
+
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/testrunner.js"></script>
+
+  <!-- Dependency -->
+  <script type="text/javascript"
+    src="../../../../canonical/launchpad/icing/yui/attribute/attribute.js">
+  </script>
+  <script type="text/javascript"
+    src="../../../../canonical/launchpad/icing/yui/event-custom/event-custom.js">
+  </script>
+  <script type="text/javascript"
+    src="../../../../canonical/launchpad/icing/yui/oop/oop.js">
+  </script>
+  <script type="text/javascript"
+    src="../../../app/javascript/overlay/overlay.js">
+  </script>
+  <script type="text/javascript"
+    src="../../../app/javascript/choiceedit/choiceedit.js">
+  </script>
+  <script type="text/javascript"
+    src="../../../app/javascript/client.js"></script>
+  <script type="text/javascript"
+    src="../../../app/javascript/testing/mockio.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../bugtask_index.js"></script>
+
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_async_comment_loading.js"></script>
+
+  <!-- Test layout -->
+  <link rel="stylesheet"
+    href="../../../../canonical/launchpad/javascript/test.css" />
+  <style type="text/css">
+    /* CSS classes specific to this test */
+    .unseen { display: none; }
+  </style>
+  <div id="more-comments-spinner" style="display: hidden">
+    Look, I'm a spinner. *spins*
+  </div>
+</head>
+<body class="yui3-skin-sam">
+</body>
+</html>

=== added file 'lib/lp/bugs/javascript/tests/test_async_comment_loading.js'
--- lib/lp/bugs/javascript/tests/test_async_comment_loading.js	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/javascript/tests/test_async_comment_loading.js	2011-09-10 01:15:25 +0000
@@ -0,0 +1,135 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw',
+    combine: false,
+    fetchCSS: false
+      }).use('event', 'lp.bugs.bugtask_index', 'lp.client', 'node',
+             'lp.testing.mockio', 'test', 'widget-stack', 'console',
+             function(Y) {
+
+
+// Local aliases
+var Assert = Y.Assert,
+    ArrayAssert = Y.ArrayAssert;
+var module = Y.lp.bugs.bugtask_index;
+var suite = new Y.Test.Suite("Async comment loading tests");
+
+var comments_markup =
+    "<div>This is a comment</div>" +
+    "<div>So is this</div>" +
+    "<div>So is this</div>" +
+    "<div>And this, too.</div>";
+
+suite.add(new Y.Test.Case({
+
+    name: 'Basic async comment loading tests',
+
+    setUp: function() {
+        // Monkeypatch LP to avoid network traffic and to make
+        // some things work as expected.
+        Y.lp.client.Launchpad.prototype.named_post =
+          function(url, func, config) {
+            config.on.success();
+          };
+        LP = {
+          'cache': {
+            'bug': {
+              self_link: "http://bugs.example.com/bugs/1234"
+          }}};
+        // Add some HTML to the page for us to use.
+        this.comments_container = Y.Node.create(
+            '<div id="comments-container"></div>');
+        this.add_comment_form_container = Y.Node.create(
+            '<div id="add-comment-form-container" class="hidden"></div>');
+        Y.one('body').appendChild(this.comments_container);
+        Y.one('body').appendChild(this.add_comment_form_container);
+    },
+
+    tearDown: function() {
+        this.comments_container.remove();
+        this.add_comment_form_container.remove();
+    },
+
+    /**
+     * load_more_comments() calls the passed batch_commments_url of the
+     * current bug task and loads more comments from it.
+     */
+    test_load_more_comments_loads_more_comments: function() {
+        var mockio = new Y.lp.testing.mockio.MockIo();
+        module.load_more_comments(
+            '', this.comments_container, mockio);
+        mockio.success({
+            responseText: comments_markup,
+            responseHeaders: {'Content-Type': 'application/xhtml'}
+        });
+        Assert.areEqual(
+            '<div>' + comments_markup + '</div>',
+            this.comments_container.get('innerHTML'));
+    },
+
+    /**
+     * load_more_comments() will show the "add comment" form once all
+     * the comments have loaded.
+     */
+    test_load_more_comments_shows_add_comment_form: function() {
+        var add_comment_form_container = Y.one(
+            '#add-comment-form-container');
+        Assert.isTrue(add_comment_form_container.hasClass('hidden'));
+        var mockio = new Y.lp.testing.mockio.MockIo();
+        module.load_more_comments(
+            '', this.comments_container, mockio);
+        mockio.success({
+            responseText: comments_markup,
+            responseHeaders: {'Content-Type': 'application/xhtml'}
+        });
+        Assert.isFalse(add_comment_form_container.hasClass('hidden'));
+    },
+
+    /**
+     * load_more_comments() will call itself recursively until there are
+     * no more comments to load.
+     */
+    test_load_more_comments_is_recursive: function() {
+        var next_batch_url_div =
+            '<div id="next-batch-url">https://launchpad.dev/</div>';
+        var more_comments_to_load_markup =
+            '<div>Here, have a comment. There are more where this came' +
+            'from</div>';
+        var mockio = new Y.lp.testing.mockio.MockIo();
+        module.load_more_comments(
+            '', this.comments_container, mockio);
+        mockio.success({
+            responseText: next_batch_url_div + more_comments_to_load_markup,
+            responseHeaders: {'Content-Type': 'application/xhtml'}
+        });
+        mockio.success({
+            responseText: comments_markup,
+            responseHeaders: {'Content-Type': 'application/xhtml'}
+        });
+        var expected_markup =
+            '<div>' + more_comments_to_load_markup + '</div>' +
+            '<div>' + comments_markup + '</div>';
+        Assert.areEqual(
+            expected_markup, this.comments_container.get('innerHTML'));
+    },
+
+}));
+
+var handle_complete = function(data) {
+    window.status = '::::' + JSON.stringify(data);
+    };
+Y.Test.Runner.on('complete', handle_complete);
+Y.Test.Runner.add(suite);
+
+var yconsole = new Y.Console({
+    newestOnTop: false
+});
+yconsole.render('#log');
+
+Y.on('domready', function() {
+    Y.Test.Runner.run();
+});
+
+});

=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
--- lib/lp/bugs/templates/bugcomment-macros.pt	2011-08-30 12:29:14 +0000
+++ lib/lp/bugs/templates/bugcomment-macros.pt	2011-09-10 01:15:25 +0000
@@ -51,8 +51,7 @@
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   metal:define-macro="break">
   <div id="comments-container"></div>
-  <div class="boardComment" style="border-bottom: 0"
-    id="comments-hidden-message">
+  <div class="boardComment comments-hidden-message" style="border-bottom: 0">
     <div class="boardCommentDetails">
       <table>
         <tbody>
@@ -82,4 +81,49 @@
   </div>
 </div>
 
+<div
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  metal:define-macro="comment-form">
+  <div tal:define="comment_form nocall:context/@@+addcomment-form;
+       dummy comment_form/initialize" id="add-comment-form">
+    <div tal:condition="context/bug/duplicateof"
+        class="warning message"
+        id="warning-comment-on-duplicate">
+      Remember, this bug report is a duplicate of
+      <a  tal:define="duplicateof context/bug/duplicateof"
+          tal:condition="duplicateof/required:launchpad.View"
+          tal:attributes="href duplicateof/fmt:url; title
+             duplicateof/title; style string:margin-right: 4px;
+             id string:duplicate-of-warning-link;"
+          tal:content="string:bug #${duplicateof/id}."
+      >bug #42</a>
+      <span
+          tal:define="duplicateof context/bug/duplicateof"
+          tal:condition="not:duplicateof/required:launchpad.View"
+          tal:replace="string: a private bug." />
+      Comment here only if you think the duplicate status is wrong.
+    </div>
+    <h2>Add comment</h2>
+    <form action="+addcomment"
+          method="post"
+          enctype="multipart/form-data"
+          accept-charset="UTF-8">
+      <tal:comment-input
+          replace="structure comment_form/widgets/comment" />
+      <div class="actions"
+          tal:content="structure
+          comment_form/actions/field.actions.save/render" />
+    </form>
+    <script type="text/javascript">
+      LPS.use('lp.app.comment', function(Y) {
+          var comment = new Y.lp.app.comment.Comment();
+          comment.render();
+      });
+    </script>
+  </div>
+  <tal:attachment-link
+      define="add_attachment_link context_menu/addcomment"
+      replace="structure add_attachment_link/render" />
+  </div>
 </tal:root>

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2011-09-02 01:01:49 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2011-09-10 01:15:25 +0000
@@ -297,49 +297,8 @@
         <tal:comment-list-complete
             tal:condition="not:view/visible_comments_truncated_for_display">
           <tal:logged-in condition="view/user">
-            <div tal:define="comment_form nocall:context/@@+addcomment-form;
-                 dummy comment_form/initialize"
-                 id="add-comment-form">
-              <div
-                  tal:condition="context/bug/duplicateof"
-                  class="warning message"
-                  id="warning-comment-on-duplicate">
-                  Remember, this bug report is a duplicate of
-      <a
-        tal:define="duplicateof context/bug/duplicateof"
-        tal:condition="duplicateof/required:launchpad.View"
-        tal:attributes="href duplicateof/fmt:url; title
-           duplicateof/title; style string:margin-right: 4px;
-           id string:duplicate-of-warning-link;"
-        tal:content="string:bug #${duplicateof/id}."
-      >bug #42</a>
-        <span
-          tal:define="duplicateof context/bug/duplicateof"
-          tal:condition="not:duplicateof/required:launchpad.View"
-          tal:replace="string: a private bug." />
-                  Comment here only if you think the duplicate status is wrong.
-              </div>
-              <h2>Add comment</h2>
-              <form action="+addcomment"
-                    method="post"
-                    enctype="multipart/form-data"
-                    accept-charset="UTF-8">
-                <tal:comment-input
-                  replace="structure comment_form/widgets/comment" />
-                <div class="actions"
-                  tal:content="structure
-                  comment_form/actions/field.actions.save/render" />
-              </form>
-              <script type="text/javascript">
-                LPS.use('lp.app.comment', function(Y) {
-                    var comment = new Y.lp.app.comment.Comment();
-                    comment.render();
-                });
-              </script>
-            </div>
-            <tal:attachment-link
-                define="add_attachment_link context_menu/addcomment"
-                replace="structure add_attachment_link/render" />
+            <metal:comment-form
+                metal:use-macro="context/@@bugcomment-macros/comment-form" />
           </tal:logged-in>
 
           <tal:not-logged-in condition="not: view/user">
@@ -351,7 +310,7 @@
         </tal:comment-list-complete>
         <tal:comment-list-truncated
             tal:condition="view/visible_comments_truncated_for_display">
-          <div class="informational message">
+          <div class="informational message comments-hidden-message" >
             Displaying first <span
             tal:replace="view/visible_initial_comments">23</span>
             and last <span
@@ -367,6 +326,15 @@
                 view_all_href">add a comment</a>.
             </tal:what-next>
           </div>
+          <tal:if
+            condition="request/features/bugs.batched_comment_loading.enabled">
+              <tal:logged-in condition="view/user">
+                <div id="add-comment-form-container" class="hidden">
+                  <metal:comment-form
+                      metal:use-macro="context/@@bugcomment-macros/comment-form" />
+                </div>
+              </tal:logged-in>
+          </tal:if>
         </tal:comment-list-truncated>
       </tal:comments>
 

=== added file 'standard_test_template.html'
--- standard_test_template.html	1970-01-01 00:00:00 +0000
+++ standard_test_template.html	2011-09-10 01:15:25 +0000
@@ -0,0 +1,31 @@
+<!--
+Copyright 2011 Canonical Ltd.  This software is licensed under the
+GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+<html>
+  <head>
+    <title>Launchpad TESTLIBRARY</title>
+    <!-- YUI and test setup -->
+    <script type="text/javascript"
+            src="../../../../../canonical/launchpad/icing/yui/yui/yui.js">
+    </script>
+    <link rel="stylesheet" href="../../../../app/javascript/testing/test.css" />
+    <script type="text/javascript"
+            src="../../../../app/javascript/testing/testrunner.js"></script>
+
+    <!-- The module under test -->
+    <script type="text/javascript" src="../TESTLIBRARY.js"></script>
+
+    <!-- The test suite -->
+    <script type="text/javascript" src="test_TESTLIBRARY.js"></script>
+  </head>
+  <body class="yui-skin-sam">
+
+    <!-- The example markup required by the script to run -->
+    <div id="expected-id">
+      ...
+    </div>
+  </body>
+</html>
+

=== added file 'standard_test_template.js'
--- standard_test_template.js	1970-01-01 00:00:00 +0000
+++ standard_test_template.js	2011-09-10 01:15:25 +0000
@@ -0,0 +1,74 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw',
+    combine: false,
+    fetchCSS: false
+
+// Don't forget to add the module under test to the use() clause.
+
+      }).use('event', 'lp.client', 'node', 'test', 'widget-stack',
+             'console', function(Y) {
+
+// Local aliases
+var Assert = Y.Assert,
+    ArrayAssert = Y.ArrayAssert;
+var mynamespace = Y.lp.mynamespace;
+var suite = new Y.Test.Suite("mynamespace Tests");
+
+suite.add(new Y.Test.Case({
+    // Test the setup method.
+    name: 'setup',
+
+    _should: {
+        error: {
+            test_config_undefined: true,
+            }
+        },
+
+    setUp: function() {
+        this.tbody = Y.get('#milestone-rows');
+        },
+
+    tearDown: function() {
+        delete this.tbody;
+        mynamespace._milestone_row_uri_template = null;
+        mynamespace._tbody = null;
+        },
+
+    test_good_config: function() {
+        // Verify the config data is stored.
+        var config = {
+            milestone_row_uri_template: '/uri',
+            milestone_rows_id:  '#milestone-rows'
+            };
+        mynamespace.setup(config);
+        Y.Assert.areSame(
+            config.milestone_row_uri_template,
+            mynamespace._milestone_row_uri_template);
+        Y.Assert.areSame(this.tbody, mynamespace._tbody);
+        },
+
+    test_config_undefined: function() {
+        // Verify an error is thrown if there is no config.
+        mynamespace.setup();
+        },
+}));
+
+
+var handle_complete = function(data) {
+    window.status = '::::' + JSON.stringify(data);
+    };
+Y.Test.Runner.on('complete', handle_complete);
+Y.Test.Runner.add(suite);
+
+var yconsole = new Y.Console({
+    newestOnTop: false
+});
+yconsole.render('#log');
+
+Y.on('domready', function() {
+    Y.Test.Runner.run();
+});
+
+});