Merge lp:~michael.nelson/launchpad/649559-ajax-comment-on-dsdiff-2 into lp:launchpad/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
1=== modified file 'lib/lp/registry/browser/configure.zcml'
2--- lib/lp/registry/browser/configure.zcml 2010-09-30 06:16:23 +0000
3+++ lib/lp/registry/browser/configure.zcml 2010-10-01 10:26:00 +0000
4@@ -188,6 +188,9 @@
5 path_expression="string:comments/${id}"
6 attribute_to_parent="distro_series_difference"
7 rootsite="mainsite"/>
8+ <adapter
9+ factory="lp.registry.browser.distroseriesdifference.CommentXHTMLRepresentation"
10+ name="lazr.restful.EntryResource" />
11 <browser:menus
12 classes="
13 DistroSeriesFacets
14
15=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
16--- lib/lp/registry/browser/distroseriesdifference.py 2010-09-29 08:19:42 +0000
17+++ lib/lp/registry/browser/distroseriesdifference.py 2010-10-01 10:26:00 +0000
18@@ -5,11 +5,17 @@
19
20 __metaclass__ = type
21 __all__ = [
22+ 'CommentXHTMLRepresentation',
23 'DistroSeriesDifferenceView',
24 ]
25
26+from lazr.restful.interfaces import IWebServiceClientRequest
27+from z3c.ptcompat import ViewPageTemplateFile
28 from zope.app.form.browser.itemswidgets import RadioWidget
29-from zope.component import getUtility
30+from zope.component import (
31+ adapts,
32+ getUtility,
33+ )
34 from zope.interface import (
35 implements,
36 Interface,
37@@ -22,6 +28,7 @@
38
39 from canonical.launchpad.webapp import (
40 LaunchpadFormView,
41+ LaunchpadView,
42 Navigation,
43 stepthrough,
44 )
45@@ -32,8 +39,12 @@
46 IDistroSeriesDifference,
47 )
48 from lp.registry.interfaces.distroseriesdifferencecomment import (
49+ IDistroSeriesDifferenceComment,
50 IDistroSeriesDifferenceCommentSource,
51 )
52+from lp.registry.model.distroseriesdifferencecomment import (
53+ DistroSeriesDifferenceComment,
54+ )
55 from lp.services.comments.interfaces.conversation import (
56 IComment,
57 IConversation,
58@@ -107,9 +118,11 @@
59 @property
60 def comments(self):
61 """See `IConversation`."""
62+ comments = self.context.getComments().order_by(
63+ DistroSeriesDifferenceComment.id)
64 return [
65 DistroSeriesDifferenceDisplayComment(comment) for
66- comment in self.context.getComments()]
67+ comment in comments]
68
69 @property
70 def show_edit_options(self):
71@@ -132,3 +145,16 @@
72 self.comment_author = comment.comment_author
73 self.comment_date = comment.comment_date
74 self.body_text = comment.body_text
75+
76+
77+class CommentXHTMLRepresentation(LaunchpadView):
78+ """Render individual comments when requested via the API."""
79+ adapts(IDistroSeriesDifferenceComment, IWebServiceClientRequest)
80+ implements(Interface)
81+
82+ template = ViewPageTemplateFile(
83+ '../templates/distroseriesdifferencecomment-fragment.pt')
84+
85+ @property
86+ def comment(self):
87+ return DistroSeriesDifferenceDisplayComment(self.context)
88
89=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
90--- lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-09-28 07:00:56 +0000
91+++ lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-10-01 10:26:00 +0000
92@@ -82,18 +82,131 @@
93 * options.
94 * @param source_name {string} The name of the source to update.
95 */
96- var setup_blacklist_options = function(blacklist_options,
97- source_name) {
98- var api_uri = [
99- LP.client.cache.context.self_link,
100- '+difference',
101- source_name
102- ].join('/')
103+ var setup_blacklist_options = function(
104+ blacklist_options, source_name, api_uri) {
105 Y.on('click', blacklist_handler, blacklist_options.all('input'),
106 blacklist_options, api_uri, source_name);
107 };
108
109 /**
110+ * Toggle the spinner and enable/disable comment fields.
111+ *
112+ * @param comment_form {Node} The node that contains the relevant
113+ * comment fields.
114+ */
115+ var toggle_comment_in_progress = function(comment_form) {
116+ var spinner = comment_form.one('img');
117+ if (Y.Lang.isNull(spinner)) {
118+ comment_form.one('div.widget-bd').append(
119+ '<img src="/@@/spinner" />');
120+ comment_form.all('textarea,button').set(
121+ 'disabled', 'disabled');
122+ } else {
123+ comment_form.one('img').remove();
124+ comment_form.all('textarea,button').set(
125+ 'disabled', '');
126+ }
127+ };
128+
129+ /**
130+ * Handle the add comment event.
131+ *
132+ * This method adds a comment via the API and update the UI.
133+ *
134+ * @param comment_form {Node} The node that contains the relevant comment
135+ * fields.
136+ * @param api_uri {string} The uri for the distroseriesdifference to which
137+ * the comment is to be added.
138+ */
139+ var add_comment_handler = function(comment_form, api_uri) {
140+
141+ var comment_text = comment_form.one('textarea').get('value');
142+
143+ toggle_comment_in_progress(comment_form);
144+
145+ var success_handler = function(comment_entry) {
146+ // Grab the XHTML representation of the comment
147+ // and prepend it to the list of comments.
148+ config = {
149+ on: {
150+ success: function(comment_html) {
151+ comment_node = Y.Node.create(comment_html);
152+ comment_form.insert(comment_node, 'before');
153+ var anim = Y.lazr.anim.green_flash({
154+ node: comment_node
155+ });
156+ anim.run();
157+ }
158+ },
159+ accept: LP.client.XHTML
160+ };
161+ lp_client.get(comment_entry.get('self_link'), config);
162+ comment_form.one('textarea').set('value', '');
163+ toggle_comment_in_progress(comment_form);
164+ };
165+ var failure_handler = function(id, response) {
166+ // Re-enable field with red flash.
167+ toggle_comment_in_progress(comment_form);
168+ var anim = Y.lazr.anim.red_flash({
169+ node: comment_form
170+ });
171+ anim.run();
172+ };
173+
174+ var config = {
175+ on: {
176+ success: success_handler,
177+ failure: failure_handler
178+ },
179+ parameters: {
180+ comment: comment_text
181+ }
182+ };
183+ lp_client.named_post(api_uri, 'addComment', config);
184+ };
185+
186+ /**
187+ * Add the comment fields ready for sliding out.
188+ *
189+ * This method adds the markup for a slide-out comment and sets
190+ * the event handlers.
191+ *
192+ * @param placeholder {Node} The node that is to contain the comment
193+ * fields.
194+ * @param api_uri {string} The uri for the distroseriesdifference to which
195+ * the comment is to be added.
196+ */
197+ var setup_add_comment = function(placeholder, api_uri) {
198+ placeholder.insert([
199+ '<a class="widget-hd js-action sprite add" href="#">',
200+ ' Add comment</a>',
201+ '<div class="widget-bd lazr-closed" ',
202+ ' style="height:0;overflow:hidden">',
203+ ' <textarea></textarea><button>Save comment</button>',
204+ '</div>'
205+ ].join(''), 'replace');
206+
207+ // The comment area should slide in when the 'Add comment'
208+ // action is clicked.
209+ var slide;
210+ placeholder.one('a.widget-hd').on('click', function(e) {
211+ e.preventDefault();
212+ if (!slide) {
213+ slide = Y.lazr.effects.slide_out(
214+ placeholder.one('div.widget-bd'));
215+ } else {
216+ slide.set('reverse', !slide.get('reverse'));
217+ }
218+ slide.stop();
219+ slide.run();
220+ });
221+
222+ placeholder.one('button').on('click', function(e) {
223+ add_comment_handler(placeholder, api_uri);
224+ });
225+ };
226+
227+ /**
228 * Get the extra information for this diff to display.
229 *
230 * @param uri {string} The uri for the extra diff info.
231@@ -111,19 +224,28 @@
232 var success_cb = function(transaction_id, response, args) {
233 args.container.one('div.diff-extra-container').insert(
234 response.responseText, 'replace');
235+ var api_uri = [
236+ LP.client.cache.context.self_link,
237+ '+difference',
238+ source_name
239+ ].join('/')
240 setup_blacklist_options(args.container.one(
241- 'div.blacklist-options'), source_name);
242+ 'div.blacklist-options'), source_name, api_uri);
243+ setup_add_comment(args.container.one(
244+ 'div.add-comment-placeholder'), api_uri);
245 };
246
247 var failure_cb = function(transaction_id, response, args){
248 var retry_handler = function(e) {
249 e.preventDefault();
250- get_extra_diff_info(args.uri, args.container);
251+ get_extra_diff_info(
252+ args.uri, args.container, args.source_name);
253 };
254 var failure_message = Y.lp.soyuz.base.makeFailureNode(
255 'Failed to fetch difference details.',
256 retry_handler);
257- args.container.insert(failure_message, 'replace');
258+ args.container.one('div.diff-extra-container').insert(
259+ failure_message, 'replace');
260
261 var anim = Y.lazr.anim.red_flash({
262 node: args.container
263@@ -138,7 +260,8 @@
264 },
265 arguments: {
266 'container': container,
267- 'uri': uri
268+ 'uri': uri,
269+ 'source_name': source_name
270 }
271 };
272 Y.io(uri, config);
273@@ -177,4 +300,5 @@
274
275 };
276
277-}, "0.1", {"requires": ["io-base", "lp.soyuz.base", "lazr.anim"]});
278+}, "0.1", {"requires": [
279+ "event-simulate", "io-base", "lp.soyuz.base", "lazr.anim", "lazr.effects"]});
280
281=== modified file 'lib/lp/registry/model/distroseriesdifferencecomment.py'
282--- lib/lp/registry/model/distroseriesdifferencecomment.py 2010-09-29 08:19:42 +0000
283+++ lib/lp/registry/model/distroseriesdifferencecomment.py 2010-10-01 10:26:00 +0000
284@@ -77,7 +77,9 @@
285 dsd_comment.distro_series_difference = distro_series_difference
286 dsd_comment.message = message
287
288- return store.add(dsd_comment)
289+ comment = store.add(dsd_comment)
290+ store.flush()
291+ return comment
292
293 @staticmethod
294 def getForDifference(distro_series_difference, id):
295
296=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
297--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-28 08:29:14 +0000
298+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-10-01 10:26:00 +0000
299@@ -54,5 +54,9 @@
300
301 <h2>Comments:</h2>
302 <tal:conversation replace="structure view/@@+render"/>
303+ <tal:show_options condition="view/show_edit_options"
304+ define="src_name context/source_package_name/name">
305+ <div tal:attributes="class string:add-comment-placeholder ${src_name}"></div>
306+ </tal:show_options>
307
308 </tal:root>
309
310=== added file 'lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt'
311--- lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt 1970-01-01 00:00:00 +0000
312+++ lib/lp/registry/templates/distroseriesdifferencecomment-fragment.pt 2010-10-01 10:26:00 +0000
313@@ -0,0 +1,2 @@
314+<tal:fragment replace="structure view/comment/@@+render"
315+ xmlns:tal="http://xml.zope.org/namespaces/tal" />
316
317=== modified file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
318--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 2010-09-27 15:24:18 +0000
319+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 2010-10-01 10:26:00 +0000
320@@ -70,3 +70,17 @@
321 self.assertEqual(
322 DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
323 diff_reloaded.status)
324+
325+ # Finally, add a comment to this difference.
326+ self.client.click(link=u'Add comment')
327+ self.client.click(
328+ xpath=u"//div[@class='add-comment-placeholder foo']//textarea")
329+ self.client.type(
330+ xpath=u"//div[@class='add-comment-placeholder foo']//textarea",
331+ text=u"Here's a comment.")
332+ self.client.click(
333+ xpath=u"//div[@class='add-comment-placeholder foo']//button")
334+ self.client.waits.forElement(
335+ classname=u'boardComment', timeout=constants.FOR_ELEMENT)
336+ self.client.asserts.assertText(
337+ classname=u'boardCommentBody', validator=u"Here's a comment.")

Subscribers

People subscribed via source and target branches

to status/vote changes: