Launchpad itself

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

  1. 649559-ajax-comment-on-dsdiff-2
  2. Merge into db-devel
Proposed by Michael Nelson
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: 9856
Proposed branch: lp:~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2
Merge into: lp:launchpad/db-devel
Diff against target: 337 lines (+190/-15)
7 files modified
lib/lp/registry/browser/configure.zcml (+3/-0)
lib/lp/registry/browser/distroseriesdifference.py (+28/-2)
lib/lp/registry/javascript/distroseriesdifferences_details.js (+136/-12)
lib/lp/registry/model/distroseriesdifferencecomment.py (+3/-1)
lib/lp/registry/templates/distroseriesdifference-listing-extra.pt (+4/-0)
lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt (+2/-0)
lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py (+14/-0)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Henning Eggers (community) ui* Approve
Curtis Hovey (community) ui Needs Fixing
Review via email: mp+37114@code.launchpad.net

Commit message

Adds in-line commenting on DistroSeriesDifferences.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

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*)
Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (4.0 KiB)

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...

Read more...

Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.

Revision history for this message
Henning Eggers (henninge) :
review: Approve (ui*)
Revision history for this message
Robert Collins (lifeless) wrote :

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

-Rob

Revision history for this message
Abel Deuring (adeuring) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
188 path_expression="string:comments/${id}"188 path_expression="string:comments/${id}"
189 attribute_to_parent="distro_series_difference"189 attribute_to_parent="distro_series_difference"
190 rootsite="mainsite"/>190 rootsite="mainsite"/>
191 <adapter
192 factory="lp.registry.browser.distroseriesdifference.CommentXHTMLRepresentation"
193 name="lazr.restful.EntryResource" />
191 <browser:menus194 <browser:menus
192 classes="195 classes="
193 DistroSeriesFacets196 DistroSeriesFacets
194197
=== 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 @@
55
6__metaclass__ = type6__metaclass__ = type
7__all__ = [7__all__ = [
8 'CommentXHTMLRepresentation',
8 'DistroSeriesDifferenceView',9 'DistroSeriesDifferenceView',
9 ]10 ]
1011
12from lazr.restful.interfaces import IWebServiceClientRequest
13from z3c.ptcompat import ViewPageTemplateFile
11from zope.app.form.browser.itemswidgets import RadioWidget14from zope.app.form.browser.itemswidgets import RadioWidget
12from zope.component import getUtility15from zope.component import (
16 adapts,
17 getUtility,
18 )
13from zope.interface import (19from zope.interface import (
14 implements,20 implements,
15 Interface,21 Interface,
@@ -22,6 +28,7 @@
2228
23from canonical.launchpad.webapp import (29from canonical.launchpad.webapp import (
24 LaunchpadFormView,30 LaunchpadFormView,
31 LaunchpadView,
25 Navigation,32 Navigation,
26 stepthrough,33 stepthrough,
27 )34 )
@@ -32,8 +39,12 @@
32 IDistroSeriesDifference,39 IDistroSeriesDifference,
33 )40 )
34from lp.registry.interfaces.distroseriesdifferencecomment import (41from lp.registry.interfaces.distroseriesdifferencecomment import (
42 IDistroSeriesDifferenceComment,
35 IDistroSeriesDifferenceCommentSource,43 IDistroSeriesDifferenceCommentSource,
36 )44 )
45from lp.registry.model.distroseriesdifferencecomment import (
46 DistroSeriesDifferenceComment,
47 )
37from lp.services.comments.interfaces.conversation import (48from lp.services.comments.interfaces.conversation import (
38 IComment,49 IComment,
39 IConversation,50 IConversation,
@@ -107,9 +118,11 @@
107 @property118 @property
108 def comments(self):119 def comments(self):
109 """See `IConversation`."""120 """See `IConversation`."""
121 comments = self.context.getComments().order_by(
122 DistroSeriesDifferenceComment.id)
110 return [123 return [
111 DistroSeriesDifferenceDisplayComment(comment) for124 DistroSeriesDifferenceDisplayComment(comment) for
112 comment in self.context.getComments()]125 comment in comments]
113126
114 @property127 @property
115 def show_edit_options(self):128 def show_edit_options(self):
@@ -132,3 +145,16 @@
132 self.comment_author = comment.comment_author145 self.comment_author = comment.comment_author
133 self.comment_date = comment.comment_date146 self.comment_date = comment.comment_date
134 self.body_text = comment.body_text147 self.body_text = comment.body_text
148
149
150class CommentXHTMLRepresentation(LaunchpadView):
151 """Render individual comments when requested via the API."""
152 adapts(IDistroSeriesDifferenceComment, IWebServiceClientRequest)
153 implements(Interface)
154
155 template = ViewPageTemplateFile(
156 '../templates/distroseriesdifferencecomment-fragment.pt')
157
158 @property
159 def comment(self):
160 return DistroSeriesDifferenceDisplayComment(self.context)
135161
=== 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 @@
82 * options.82 * options.
83 * @param source_name {string} The name of the source to update.83 * @param source_name {string} The name of the source to update.
84 */84 */
85 var setup_blacklist_options = function(blacklist_options,85 var setup_blacklist_options = function(
86 source_name) {86 blacklist_options, source_name, api_uri) {
87 var api_uri = [
88 LP.client.cache.context.self_link,
89 '+difference',
90 source_name
91 ].join('/')
92 Y.on('click', blacklist_handler, blacklist_options.all('input'),87 Y.on('click', blacklist_handler, blacklist_options.all('input'),
93 blacklist_options, api_uri, source_name);88 blacklist_options, api_uri, source_name);
94 };89 };
9590
96 /**91 /**
92 * Toggle the spinner and enable/disable comment fields.
93 *
94 * @param comment_form {Node} The node that contains the relevant
95 * comment fields.
96 */
97 var toggle_comment_in_progress = function(comment_form) {
98 var spinner = comment_form.one('img');
99 if (Y.Lang.isNull(spinner)) {
100 comment_form.one('div.widget-bd').append(
101 '<img src="/@@/spinner" />');
102 comment_form.all('textarea,button').set(
103 'disabled', 'disabled');
104 } else {
105 comment_form.one('img').remove();
106 comment_form.all('textarea,button').set(
107 'disabled', '');
108 }
109 };
110
111 /**
112 * Handle the add comment event.
113 *
114 * This method adds a comment via the API and update the UI.
115 *
116 * @param comment_form {Node} The node that contains the relevant comment
117 * fields.
118 * @param api_uri {string} The uri for the distroseriesdifference to which
119 * the comment is to be added.
120 */
121 var add_comment_handler = function(comment_form, api_uri) {
122
123 var comment_text = comment_form.one('textarea').get('value');
124
125 toggle_comment_in_progress(comment_form);
126
127 var success_handler = function(comment_entry) {
128 // Grab the XHTML representation of the comment
129 // and prepend it to the list of comments.
130 config = {
131 on: {
132 success: function(comment_html) {
133 comment_node = Y.Node.create(comment_html);
134 comment_form.insert(comment_node, 'before');
135 var anim = Y.lazr.anim.green_flash({
136 node: comment_node
137 });
138 anim.run();
139 }
140 },
141 accept: LP.client.XHTML
142 };
143 lp_client.get(comment_entry.get('self_link'), config);
144 comment_form.one('textarea').set('value', '');
145 toggle_comment_in_progress(comment_form);
146 };
147 var failure_handler = function(id, response) {
148 // Re-enable field with red flash.
149 toggle_comment_in_progress(comment_form);
150 var anim = Y.lazr.anim.red_flash({
151 node: comment_form
152 });
153 anim.run();
154 };
155
156 var config = {
157 on: {
158 success: success_handler,
159 failure: failure_handler
160 },
161 parameters: {
162 comment: comment_text
163 }
164 };
165 lp_client.named_post(api_uri, 'addComment', config);
166 };
167
168 /**
169 * Add the comment fields ready for sliding out.
170 *
171 * This method adds the markup for a slide-out comment and sets
172 * the event handlers.
173 *
174 * @param placeholder {Node} The node that is to contain the comment
175 * fields.
176 * @param api_uri {string} The uri for the distroseriesdifference to which
177 * the comment is to be added.
178 */
179 var setup_add_comment = function(placeholder, api_uri) {
180 placeholder.insert([
181 '<a class="widget-hd js-action sprite add" href="#">',
182 ' Add comment</a>',
183 '<div class="widget-bd lazr-closed" ',
184 ' style="height:0;overflow:hidden">',
185 ' <textarea></textarea><button>Save comment</button>',
186 '</div>'
187 ].join(''), 'replace');
188
189 // The comment area should slide in when the 'Add comment'
190 // action is clicked.
191 var slide;
192 placeholder.one('a.widget-hd').on('click', function(e) {
193 e.preventDefault();
194 if (!slide) {
195 slide = Y.lazr.effects.slide_out(
196 placeholder.one('div.widget-bd'));
197 } else {
198 slide.set('reverse', !slide.get('reverse'));
199 }
200 slide.stop();
201 slide.run();
202 });
203
204 placeholder.one('button').on('click', function(e) {
205 add_comment_handler(placeholder, api_uri);
206 });
207 };
208
209 /**
97 * Get the extra information for this diff to display.210 * Get the extra information for this diff to display.
98 *211 *
99 * @param uri {string} The uri for the extra diff info.212 * @param uri {string} The uri for the extra diff info.
@@ -111,19 +224,28 @@
111 var success_cb = function(transaction_id, response, args) {224 var success_cb = function(transaction_id, response, args) {
112 args.container.one('div.diff-extra-container').insert(225 args.container.one('div.diff-extra-container').insert(
113 response.responseText, 'replace');226 response.responseText, 'replace');
227 var api_uri = [
228 LP.client.cache.context.self_link,
229 '+difference',
230 source_name
231 ].join('/')
114 setup_blacklist_options(args.container.one(232 setup_blacklist_options(args.container.one(
115 'div.blacklist-options'), source_name);233 'div.blacklist-options'), source_name, api_uri);
234 setup_add_comment(args.container.one(
235 'div.add-comment-placeholder'), api_uri);
116 };236 };
117237
118 var failure_cb = function(transaction_id, response, args){238 var failure_cb = function(transaction_id, response, args){
119 var retry_handler = function(e) {239 var retry_handler = function(e) {
120 e.preventDefault();240 e.preventDefault();
121 get_extra_diff_info(args.uri, args.container);241 get_extra_diff_info(
242 args.uri, args.container, args.source_name);
122 };243 };
123 var failure_message = Y.lp.soyuz.base.makeFailureNode(244 var failure_message = Y.lp.soyuz.base.makeFailureNode(
124 'Failed to fetch difference details.',245 'Failed to fetch difference details.',
125 retry_handler);246 retry_handler);
126 args.container.insert(failure_message, 'replace');247 args.container.one('div.diff-extra-container').insert(
248 failure_message, 'replace');
127249
128 var anim = Y.lazr.anim.red_flash({250 var anim = Y.lazr.anim.red_flash({
129 node: args.container251 node: args.container
@@ -138,7 +260,8 @@
138 },260 },
139 arguments: {261 arguments: {
140 'container': container,262 'container': container,
141 'uri': uri263 'uri': uri,
264 'source_name': source_name
142 }265 }
143 };266 };
144 Y.io(uri, config);267 Y.io(uri, config);
@@ -177,4 +300,5 @@
177300
178};301};
179302
180}, "0.1", {"requires": ["io-base", "lp.soyuz.base", "lazr.anim"]});303}, "0.1", {"requires": [
304 "event-simulate", "io-base", "lp.soyuz.base", "lazr.anim", "lazr.effects"]});
181305
=== 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 @@
77 dsd_comment.distro_series_difference = distro_series_difference77 dsd_comment.distro_series_difference = distro_series_difference
78 dsd_comment.message = message78 dsd_comment.message = message
7979
80 return store.add(dsd_comment)80 comment = store.add(dsd_comment)
81 store.flush()
82 return comment
8183
82 @staticmethod84 @staticmethod
83 def getForDifference(distro_series_difference, id):85 def getForDifference(distro_series_difference, id):
8486
=== 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 @@
5454
55 <h2>Comments:</h2>55 <h2>Comments:</h2>
56 <tal:conversation replace="structure view/@@+render"/>56 <tal:conversation replace="structure view/@@+render"/>
57 <tal:show_options condition="view/show_edit_options"
58 define="src_name context/source_package_name/name">
59 <div tal:attributes="class string:add-comment-placeholder ${src_name}"></div>
60 </tal:show_options>
5761
58</tal:root>62</tal:root>
5963
=== 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 @@
1<tal:fragment replace="structure view/comment/@@+render"
2 xmlns:tal="http://xml.zope.org/namespaces/tal" />
03
=== 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 @@
70 self.assertEqual(70 self.assertEqual(
71 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,71 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
72 diff_reloaded.status)72 diff_reloaded.status)
73
74 # Finally, add a comment to this difference.
75 self.client.click(link=u'Add comment')
76 self.client.click(
77 xpath=u"//div[@class='add-comment-placeholder foo']//textarea")
78 self.client.type(
79 xpath=u"//div[@class='add-comment-placeholder foo']//textarea",
80 text=u"Here's a comment.")
81 self.client.click(
82 xpath=u"//div[@class='add-comment-placeholder foo']//button")
83 self.client.waits.forElement(
84 classname=u'boardComment', timeout=constants.FOR_ELEMENT)
85 self.client.asserts.assertText(
86 classname=u'boardCommentBody', validator=u"Here's a comment.")

Subscribers

People subscribed via source and target branches

to status/vote changes: