Merge lp:~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2 into lp:launchpad/db-devel

[Michael Nelson]

Adds in-line commenting on DistroSeriesDifferences.

Overview
========

This branch allows users to add comments to distro series differences. I'm initially just after a UI review, after which I'll add a windmill test and tidy-up the code.

See the LEP here:
https://dev.launchpad.net/LEP/DerivativeDistributions

and the specific mockup here:

https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png

You can view a 1min demo of the UI here:

http://people.canonical.com/~michaeln/tmp/649559-blacklisting-with-comments.ogv

Details
=======

There were a few things I wasn't happy with code-wise in this branch:
1) Importing model code in a view simply to use order_by - is there a better way? (I tried a string instead ('IDistroSeriesDifferenceComment.id'), but got storm errors I couldn't resolve. I can, but shouldn't have to, add an optional arg for ordering to getComments(), just let me know what you'd prefer.
2) Specifying the template manually for CommentXHTMLRepresentation. As it's an adapter, I'm not sure that the template can be defined in the zcml. I'll try now to be sure.

To test:
========
bin/test -vvm test_distroseriesdifference_expander

To demo locally:
================
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs

[Henning Eggers (henninge)]

This looks really great! Thank you!

As discussed on IRC, though, I am really worried about the reverse ordering of the comments. Not that I don't like it, it is just inconsistent with other commenting we have in LP (bugs, mps, questions). You mentioned that you considered it more like a "wall" where people post notices. I see three possible solutions:

 1. You simply adopt forward ordering for these comments and new comments are written at the bottom. I think it would be ok to only see latest 5 comments or so.

 2. We consider adopting the reverse-ordered "wall" concept for all our commenting on Launchpad. That would have to be discussed on the mailing list but you already declined it yourself for MPs.

 3. We make the "wall" concept visually different from the "comment" concept, so people are aware that they work differently. This should be more than just a different headline (although that would work as a temporary solution) and might take us back to your original design of the comments (I am sorry, I did not notice the reverse ordering then).

That said, I can only repeat that the page looks great and I just love to hear your voice! ;-)

Henning

review: Needs Fixing (ui*)

[Michael Nelson (michael.nelson)]

IRC conversation with Henning and James:

12:15 < noodles775> Hi henninge, can I interest you in another UI review? http://people.canonical.com/~michaeln/tmp/649559-blacklisting-with-comments.ogv
12:15 < noodles775> Actual MP here: https://code.edge.launchpad.net/~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2/+merge/37114
12:16 < henninge> noodles775: sure, but I have to finish something first.
12:17 * henninge watches screencast right away, though ... ;)
12:17 < noodles775> henninge: no problem (or if you're busy, salgado will be around a bit later too). Thanks.
12:17 < noodles775> Ah :)
12:18 < henninge> noodles775: hm, aren't comments always sorted from oldest to newest in LP? We should stick to that.
12:20 < noodles775> henninge: I wasn't sure in this case - it is more like a wall... as a user, the information I want is the most recent comments. But it's easy to switch.
12:20 < henninge> noodles775: also, in the same vein, "Add comment" is usually placed at the end of the list of comments.
12:20 < henninge> noodles775: Did I mention that this looks great! ;-D
12:21 < henninge> noodles775: sorry for starting off with the negative things ... ;)
12:21 < noodles775> henninge: yes, if the comments are going oldest to newest, then of course it would be at the end. It's only there because I did it more like a wall.
12:21 < noodles775> No problem! Thanks for the feedback.
12:21 < henninge> noodles775: well, I don't see how this case is different from other commenting we have in LP
12:22 < noodles775> henninge: The biggest difference, IMO, is that we're displaying lists of comments for multiple objects on the one page. Ideally, if I had more time, we'd only be showing the 5 latest comments for each (in the details section).
12:22 < henninge> noodles775: mp's have a "Add a review or comment" button on top of the comments that scrolls you down to the end.
12:22 < henninge> dunno why bugs don't.
12:24 < henninge> noodles775: I am not opposed to introducing a "wall" concept for short comments - but they should look different to mark the difference in behaviour.
12:24 < noodles775> james_w: if you're around and have a minute, would you also prefer standard oldest to newest comments for the differences (see above screencast). I've currently got them with the newest first, assuming that's the info that will be needed, but it's easy to switch back.
12:26 < noodles775> henninge: If the heading was "Most recent comments" instead of "Comments" (and later we added a link to the full details for a particular difference with all comments) would that help differentiate?
12:26 < noodles775> (er, and we only showed the 5 most recent on this page?)
12:28 < henninge> noodles775: That would probably help but I still think people will get confused about the ordering.
12:29 < henninge> noodles775: I actually think the reverse ordering is preferrable
12:29 < noodles775> henninge: yeah, perhaps. Let's see what james_w or other people who will be using this say. I'm happy to change it - it's an easy switch.
12:30 < henninge> noodles775: I don't know if it has been discussed before but maybe you could post your screencast to the list and start a discussion to a...

[Curtis Hovey (sinzui)]

I share Henning's view. I am most concerned about point 3. We need this too look differently if it behaves differently. I expect a wall to support threading so that replies are top-to-bottom, but this does not. I think there is a better analogy, changelog. This is not necessarily a change log because my change/action is not directly tied to my comment. If we presented this like a changelog, I think most users will understand what is happening.

Michael, I think the next step is to follow up on Henning's suggestion to send an email to the list. I think changelog/activity logs are great, that we do not use them as often as we should, and that they should be newest to oldest. I hope we can get quick agreement about how they look and behave. We can "fix" bugs, questions, and karma. I hope this will be a foundation for me dream to show activity logs instead of recent portlets on pillars and people.

review: Needs Fixing (ui)

[Michael Nelson (michael.nelson)]

17:07 < noodles775> sinzui: I'm happy to email the dev list regarding activity-streams etc., but I need to keep working on the next branch. Would you be ok if I re-order the comments and put the add-comment link at the bottom as henninge suggested, so that I'm not blocked on this?
17:08 < sinzui> noodles775, yes
17:08 < noodles775> Great, thanks.
17:08 < henninge> noodles775, sinzui: I agree that this is an acceptable temporary solution. ;)
17:09 < noodles775> Great, thanks henninge
17:09 < sinzui> yes, I really think reverse listing for activity are what users want
17:09 < noodles775> Yes, me too.

[Henning Eggers (henninge)]

review: Approve (ui*)

[Robert Collins (lifeless)]

On order_by - we probably should just remove the import fascist.

-Rob

[Abel Deuring (adeuring)]

Hi Michael,

a nice branch. I agree that importing a model class to browser just in order to get a parameter for order_by() is a bit odd, but I think it won't hurt much.

Just one stylistic nitpick: The phrase "This method is responsible for adding..." appears at least in two doc strings; I would prefer a simple "This method adds..."

review: Approve (code)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-09-30 06:16:23 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-10-01 10:26:00 +0000
@@ -188,6 +188,9 @@
         path_expression="string:comments/${id}"
         attribute_to_parent="distro_series_difference"
         rootsite="mainsite"/>
+    <adapter
+      factory="lp.registry.browser.distroseriesdifference.CommentXHTMLRepresentation"
+      name="lazr.restful.EntryResource" />
     <browser:menus
         classes="
             DistroSeriesFacets

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2010-09-29 08:19:42 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2010-10-01 10:26:00 +0000
@@ -5,11 +5,17 @@
 
 __metaclass__ = type
 __all__ = [
+    'CommentXHTMLRepresentation',
     'DistroSeriesDifferenceView',
     ]
 
+from lazr.restful.interfaces import IWebServiceClientRequest
+from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.form.browser.itemswidgets import RadioWidget
-from zope.component import getUtility
+from zope.component import (
+    adapts,
+    getUtility,
+    )
 from zope.interface import (
     implements,
     Interface,
@@ -22,6 +28,7 @@
 
 from canonical.launchpad.webapp import (
     LaunchpadFormView,
+    LaunchpadView,
     Navigation,
     stepthrough,
     )
@@ -32,8 +39,12 @@
     IDistroSeriesDifference,
     )
 from lp.registry.interfaces.distroseriesdifferencecomment import (
+    IDistroSeriesDifferenceComment,
     IDistroSeriesDifferenceCommentSource,
     )
+from lp.registry.model.distroseriesdifferencecomment import (
+    DistroSeriesDifferenceComment,
+    )
 from lp.services.comments.interfaces.conversation import (
     IComment,
     IConversation,
@@ -107,9 +118,11 @@
     @property
     def comments(self):
         """See `IConversation`."""
+        comments = self.context.getComments().order_by(
+            DistroSeriesDifferenceComment.id)
         return [
             DistroSeriesDifferenceDisplayComment(comment) for
-                comment in self.context.getComments()]
+                comment in comments]
 
     @property
     def show_edit_options(self):
@@ -132,3 +145,16 @@
         self.comment_author = comment.comment_author
         self.comment_date = comment.comment_date
         self.body_text = comment.body_text
+
+
+class CommentXHTMLRepresentation(LaunchpadView):
+    """Render individual comments when requested via the API."""
+    adapts(IDistroSeriesDifferenceComment, IWebServiceClientRequest)
+    implements(Interface)
+
+    template = ViewPageTemplateFile(
+        '../templates/distroseriesdifferencecomment-fragment.pt')
+
+    @property
+    def comment(self):
+        return DistroSeriesDifferenceDisplayComment(self.context)

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2010-09-28 07:00:56 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2010-10-01 10:26:00 +0000
@@ -82,18 +82,131 @@
      *                          options.
      * @param source_name {string} The name of the source to update.
      */
-    var setup_blacklist_options = function(blacklist_options,
-                                                 source_name) {
-        var api_uri = [
-            LP.client.cache.context.self_link,
-            '+difference',
-            source_name
-            ].join('/')
+    var setup_blacklist_options = function(
+        blacklist_options, source_name, api_uri) {
         Y.on('click', blacklist_handler, blacklist_options.all('input'),
              blacklist_options, api_uri, source_name);
     };
 
     /**
+     * Toggle the spinner and enable/disable comment fields.
+     *
+     * @param comment_form {Node} The node that contains the relevant
+     *                     comment fields.
+     */
+    var toggle_comment_in_progress = function(comment_form) {
+        var spinner = comment_form.one('img');
+        if (Y.Lang.isNull(spinner)) {
+            comment_form.one('div.widget-bd').append(
+                '<img src="/@@/spinner" />');
+            comment_form.all('textarea,button').set(
+                'disabled', 'disabled');
+        } else {
+            comment_form.one('img').remove();
+            comment_form.all('textarea,button').set(
+                'disabled', '');
+        }
+    };
+
+    /**
+     * Handle the add comment event.
+     *
+     * This method adds a comment via the API and update the UI.
+     *
+     * @param comment_form {Node} The node that contains the relevant comment
+     *                            fields.
+     * @param api_uri {string} The uri for the distroseriesdifference to which
+     *                the comment is to be added.
+     */
+    var add_comment_handler = function(comment_form, api_uri) {
+
+        var comment_text = comment_form.one('textarea').get('value');
+
+        toggle_comment_in_progress(comment_form);
+
+        var success_handler = function(comment_entry) {
+            // Grab the XHTML representation of the comment
+            // and prepend it to the list of comments.
+            config = {
+                on: {
+                    success: function(comment_html) {
+                        comment_node = Y.Node.create(comment_html);
+                        comment_form.insert(comment_node, 'before');
+                        var anim = Y.lazr.anim.green_flash({
+                            node: comment_node
+                            });
+                        anim.run();
+                    }
+                    },
+                accept: LP.client.XHTML
+                };
+            lp_client.get(comment_entry.get('self_link'), config);
+            comment_form.one('textarea').set('value', '');
+            toggle_comment_in_progress(comment_form);
+        };
+        var failure_handler = function(id, response) {
+            // Re-enable field with red flash.
+            toggle_comment_in_progress(comment_form);
+            var anim = Y.lazr.anim.red_flash({
+                node: comment_form
+                });
+            anim.run();
+        };
+
+        var config = {
+            on: {
+                success: success_handler,
+                failure: failure_handler
+                },
+            parameters: {
+                comment: comment_text
+                }
+            };
+        lp_client.named_post(api_uri, 'addComment', config);
+    };
+
+    /**
+     * Add the comment fields ready for sliding out.
+     *
+     * This method adds the markup for a slide-out comment and sets
+     * the event handlers.
+     *
+     * @param placeholder {Node} The node that is to contain the comment
+     *                            fields.
+     * @param api_uri {string} The uri for the distroseriesdifference to which
+     *                the comment is to be added.
+     */
+    var setup_add_comment = function(placeholder, api_uri) {
+        placeholder.insert([
+            '<a class="widget-hd js-action sprite add" href="#">',
+            '  Add comment</a>',
+            '<div class="widget-bd lazr-closed" ',
+            '     style="height:0;overflow:hidden">',
+            '  <textarea></textarea><button>Save comment</button>',
+            '</div>'
+            ].join(''), 'replace');
+
+        // The comment area should slide in when the 'Add comment'
+        // action is clicked.
+        var slide;
+        placeholder.one('a.widget-hd').on('click', function(e) {
+            e.preventDefault();
+            if (!slide) {
+                slide = Y.lazr.effects.slide_out(
+                    placeholder.one('div.widget-bd'));
+            } else {
+                slide.set('reverse', !slide.get('reverse'));
+            }
+            slide.stop();
+            slide.run();
+        });
+
+        placeholder.one('button').on('click', function(e) {
+            add_comment_handler(placeholder, api_uri);
+        });
+    };
+
+    /**
      * Get the extra information for this diff to display.
      *
      * @param uri {string} The uri for the extra diff info.
@@ -111,19 +224,28 @@
         var success_cb = function(transaction_id, response, args) {
             args.container.one('div.diff-extra-container').insert(
                 response.responseText, 'replace');
+            var api_uri = [
+                LP.client.cache.context.self_link,
+                '+difference',
+                source_name
+                ].join('/')
             setup_blacklist_options(args.container.one(
-                'div.blacklist-options'), source_name);
+                'div.blacklist-options'), source_name, api_uri);
+            setup_add_comment(args.container.one(
+                'div.add-comment-placeholder'), api_uri);
             };
 
         var failure_cb = function(transaction_id, response, args){
            var retry_handler = function(e) {
                e.preventDefault();
-               get_extra_diff_info(args.uri, args.container);
+               get_extra_diff_info(
+                    args.uri, args.container, args.source_name);
                };
            var failure_message = Y.lp.soyuz.base.makeFailureNode(
                'Failed to fetch difference details.',
                retry_handler);
-           args.container.insert(failure_message, 'replace');
+           args.container.one('div.diff-extra-container').insert(
+                failure_message, 'replace');
 
            var anim = Y.lazr.anim.red_flash({
                 node: args.container
@@ -138,7 +260,8 @@
             },
             arguments: {
                 'container': container,
-                'uri': uri
+                'uri': uri,
+                'source_name': source_name
             }
         };
         Y.io(uri, config);
@@ -177,4 +300,5 @@
 
 };
 
-}, "0.1", {"requires": ["io-base", "lp.soyuz.base", "lazr.anim"]});
+}, "0.1", {"requires": [
+    "event-simulate", "io-base", "lp.soyuz.base", "lazr.anim", "lazr.effects"]});

=== modified file 'lib/lp/registry/model/distroseriesdifferencecomment.py'
--- lib/lp/registry/model/distroseriesdifferencecomment.py	2010-09-29 08:19:42 +0000
+++ lib/lp/registry/model/distroseriesdifferencecomment.py	2010-10-01 10:26:00 +0000
@@ -77,7 +77,9 @@
         dsd_comment.distro_series_difference = distro_series_difference
         dsd_comment.message = message
 
-        return store.add(dsd_comment)
+        comment = store.add(dsd_comment)
+        store.flush()
+        return comment
 
     @staticmethod
     def getForDifference(distro_series_difference, id):

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-09-28 08:29:14 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-10-01 10:26:00 +0000
@@ -54,5 +54,9 @@
 
   <h2>Comments:</h2>
   <tal:conversation replace="structure view/@@+render"/>
+  <tal:show_options condition="view/show_edit_options"
+                    define="src_name context/source_package_name/name">
+  <div tal:attributes="class string:add-comment-placeholder ${src_name}"></div>
+  </tal:show_options>
 
 </tal:root>

=== added file 'lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt'
--- lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt	2010-10-01 10:26:00 +0000
@@ -0,0 +1,2 @@
+<tal:fragment replace="structure view/comment/@@+render"
+    xmlns:tal="http://xml.zope.org/namespaces/tal" />

=== modified file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	2010-09-27 15:24:18 +0000
+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	2010-10-01 10:26:00 +0000
@@ -70,3 +70,17 @@
         self.assertEqual(
             DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
             diff_reloaded.status)
+
+        # Finally, add a comment to this difference.
+        self.client.click(link=u'Add comment')
+        self.client.click(
+            xpath=u"//div[@class='add-comment-placeholder foo']//textarea")
+        self.client.type(
+            xpath=u"//div[@class='add-comment-placeholder foo']//textarea",
+            text=u"Here's a comment.")
+        self.client.click(
+            xpath=u"//div[@class='add-comment-placeholder foo']//button")
+        self.client.waits.forElement(
+            classname=u'boardComment', timeout=constants.FOR_ELEMENT)
+        self.client.asserts.assertText(
+            classname=u'boardCommentBody', validator=u"Here's a comment.")