Launchpad itself

Merge lp:~abentley/launchpad/reply into lp:launchpad

  1. reply
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Muharem Hrnjadovic
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/reply
Merge into: lp:launchpad
Diff against target: 435 lines
7 files modified
lib/canonical/launchpad/javascript/lp/comment.js (+51/-5)
lib/lp/code/browser/codereviewcomment.py (+1/-34)
lib/lp/code/browser/tests/test_codereviewcomment.py (+1/-60)
lib/lp/code/interfaces/codereviewcomment.py (+5/-0)
lib/lp/code/model/codereviewcomment.py (+38/-0)
lib/lp/code/model/tests/test_codereviewcomment.py (+59/-1)
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+3/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/reply
Reviewer Review Type Date Requested Status
Paul Hummer (community) ui* Approve
Barry Warsaw (community) ui* Approve
Muharem Hrnjadovic (community) Approve
Review via email: mp+13739@code.launchpad.net

Commit message

Support inline replies for code review

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
Support inline replies for code review

== Proposed fix ==
When a user clicks on a reply link, the review/comment box is populated
with the comment as an email-style reply and the browser is scrolled
down to the widget.

== Pre-implementation notes ==
Preimplementation was with thumper

== Implementation details ==
I had to drag the email-style-reply code into the model because I wanted
to expose it for the API.

== Tests ==
None

== Demo and Q/A ==
Create a code review comment. Reply to it. In your mail client,
observe that threading is preserved correctly.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/interfaces/codereviewcomment.py
  lib/lp/code/browser/codereviewcomment.py
  lib/lp/code/model/codereviewcomment.py
  lib/canonical/launchpad/javascript/lp/comment.js

== JSLint notices ==
No handlers could be found for logger "bzr"
jslint: No problem found in
'/home/abentley/launchpad/inline-comment/lib/canonical/launchpad/javascript/lp/comment.js'.

jslint: 1 file to lint.

== Pylint notices ==

lib/lp/code/interfaces/codereviewcomment.py
    22: [F0401] Unable to import 'lazr.restful.fields' (No module named
restful)
    23: [F0401] Unable to import 'lazr.restful.declarations' (No module
named restful)

lib/lp/code/browser/codereviewcomment.py
    20: [F0401] Unable to import 'lazr.delegates' (No module named
delegates)
    21: [F0401] Unable to import 'lazr.restful.interface' (No module
named restful)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrffF0ACgkQ0F+nu1YWqI19OgCeOhDWoVdqbv4cEMwfCgZqMF4E
PEYAnReM+F10hrUAc7S9dTWMLk0qk64E
=7YkU
-----END PGP SIGNATURE-----

Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

Looks good, approved.

review: Approve
Revision history for this message
Aaron Bentley (abentley) wrote :

Screenshots at
http://people.canonical.com/~abentley/reply-1.png
http://people.canonical.com/~abentley/reply-2.png
http://people.canonical.com/~abentley/reply-3.png
http://people.canonical.com/~abentley/reply-4.png

Revision history for this message
Barry Warsaw (barry) :
review: Approve (ui*)
Revision history for this message
Paul Hummer (rockstar) :
review: Approve (ui*)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
--- lib/canonical/launchpad/javascript/lp/comment.js 2009-10-13 15:56:13 +0000
+++ lib/canonical/launchpad/javascript/lp/comment.js 2009-10-26 14:01:12 +0000
@@ -85,9 +85,7 @@
85 if (!this.validate()) {85 if (!this.validate()) {
86 return;86 return;
87 }87 }
88 this.set_disabled(true);88 this.activateProgressUI('Saving...');
89 this.submit_button.get('parentNode').replaceChild(
90 this.progress_message, this.submit_button);
91 this.post_comment(bind(function(message_entry) {89 this.post_comment(bind(function(message_entry) {
92 this.get_comment_HTML(90 this.get_comment_HTML(
93 message_entry, bind(this.insert_comment_HTML, this));91 message_entry, bind(this.insert_comment_HTML, this));
@@ -150,7 +148,18 @@
150 this.syncUI();148 this.syncUI();
151 },149 },
152 /**150 /**
153 * Stop indicating that a submission is in progress.151 * Activate indications of an operation in progress.
152 *
153 * @param message A user message describing the operation in progress.
154 */
155 activateProgressUI: function(message){
156 this.progress_message.set('innerHTML', message);
157 this.set_disabled(true);
158 this.submit_button.get('parentNode').replaceChild(
159 this.progress_message, this.submit_button);
160 },
161 /**
162 * Stop indicating that an operation is in progress.
154 *163 *
155 * @method clearProgressUI164 * @method clearProgressUI
156 */165 */
@@ -202,6 +211,7 @@
202 initializer: function() {211 initializer: function() {
203 this.vote_input = Y.get('[id="field.vote"]');212 this.vote_input = Y.get('[id="field.vote"]');
204 this.review_type = Y.get('[id="field.review_type"]');213 this.review_type = Y.get('[id="field.review_type"]');
214 this.in_reply_to = null;
205 },215 },
206 /**216 /**
207 * Return the Submit button.217 * Return the Submit button.
@@ -265,7 +275,8 @@
265 content: this.comment_input.get('value'),275 content: this.comment_input.get('value'),
266 subject: '',276 subject: '',
267 review_type: this.review_type.get('value'),277 review_type: this.review_type.get('value'),
268 vote: this.get_vote()278 vote: this.get_vote(),
279 parent: this.in_reply_to.get('self_link')
269 }280 }
270 };281 };
271 this.lp_client.named_post(282 this.lp_client.named_post(
@@ -290,6 +301,39 @@
290 });301 });
291 },302 },
292 /**303 /**
304 * Event handler when a "Reply" link is clicked.
305 *
306 * @param e The Event object representing the click.
307 */
308 reply_clicked: function(e){
309 e.halt();
310 var reply_link = LP.client.normalize_uri(e.target.get('href'));
311 var root_url = reply_link.substr(0,
312 reply_link.length - '+reply'.length);
313 var object_url = '/api/beta' + root_url;
314 this.activateProgressUI('Loading...');
315 window.scrollTo(0, Y.get('label [for=field.vote]').getY());
316 this.lp_client.get(object_url, {
317 on: {
318 success: bind(function(comment){
319 this.set_in_reply_to(comment);
320 this.clearProgressUI();
321 this.syncUI();
322 }, this),
323 failure: this.error_handler.getFailureHandler()
324 }
325 });
326 },
327 /**
328 * Set the comment that the new comment will be in reply to.
329 *
330 * @param comment The comment to be in reply to.
331 */
332 set_in_reply_to: function(comment) {
333 this.in_reply_to = comment;
334 this.comment_input.set('value', comment.get('as_quoted_email'));
335 },
336 /**
293 * Reset the widget to a blank state.337 * Reset the widget to a blank state.
294 *338 *
295 * @method reset_contents339 * @method reset_contents
@@ -297,6 +341,7 @@
297 reset_contents: function() {341 reset_contents: function() {
298 this.review_type.set('value', '');342 this.review_type.set('value', '');
299 this.vote_input.set('selectedIndex', 0);343 this.vote_input.set('selectedIndex', 0);
344 this.in_reply_to = null;
300 CodeReviewComment.superclass.reset_contents.apply(this);345 CodeReviewComment.superclass.reset_contents.apply(this);
301 },346 },
302 /**347 /**
@@ -329,6 +374,7 @@
329 this.vote_input.on('mouseup', bind(this.syncUI, this));374 this.vote_input.on('mouseup', bind(this.syncUI, this));
330 this.review_type.on('keyup', bind(this.syncUI, this));375 this.review_type.on('keyup', bind(this.syncUI, this));
331 this.review_type.on('mouseup', bind(this.syncUI, this));376 this.review_type.on('mouseup', bind(this.syncUI, this));
377 Y.all('a.menu-link-reply').on('click', bind(this.reply_clicked, this));
332 },378 },
333 /**379 /**
334 * Implementation of Widget.syncUI: Update appearance according to state.380 * Implementation of Widget.syncUI: Update appearance according to state.
335381
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2009-10-09 02:43:04 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2009-10-26 14:01:12 +0000
@@ -11,7 +11,6 @@
11 'CodeReviewCommentView',11 'CodeReviewCommentView',
12 'CodeReviewDisplayComment',12 'CodeReviewDisplayComment',
13 ]13 ]
14from textwrap import TextWrapper
1514
16from zope.app.form.browser import TextAreaWidget, DropdownWidget15from zope.app.form.browser import TextAreaWidget, DropdownWidget
17from zope.interface import Interface, implements16from zope.interface import Interface, implements
@@ -31,38 +30,6 @@
31from lp.services.comments.interfaces.conversation import IComment30from lp.services.comments.interfaces.conversation import IComment
3231
3332
34def quote_text_as_email(text, width=80):
35 """Quote the text as if it is an email response.
36
37 Uses '> ' as a line prefix, and breaks long lines.
38
39 Trailing whitespace is stripped.
40 """
41 # Empty text begets empty text.
42 if text is None:
43 return ''
44 text = text.rstrip()
45 if not text:
46 return ''
47 prefix = '> '
48 # The TextWrapper's handling of code is somewhat suspect.
49 wrapper = TextWrapper(
50 initial_indent=prefix,
51 subsequent_indent=prefix,
52 width=width,
53 replace_whitespace=False)
54 result = []
55 # Break the string into lines, and use the TextWrapper to wrap the
56 # individual lines.
57 for line in text.rstrip().split('\n'):
58 # TextWrapper won't do an indent of an empty string.
59 if line.strip() == '':
60 result.append(prefix)
61 else:
62 result.extend(wrapper.wrap(line))
63 return '\n'.join(result)
64
65
66class CodeReviewDisplayComment:33class CodeReviewDisplayComment:
67 """A code review comment or activity or both.34 """A code review comment or activity or both.
6835
@@ -222,7 +189,7 @@
222 quoted comment being replied to.189 quoted comment being replied to.
223 """190 """
224 if self.is_reply:191 if self.is_reply:
225 comment = quote_text_as_email(self.reply_to.message_body)192 comment = self.reply_to.as_quoted_email
226 else:193 else:
227 comment = ''194 comment = ''
228 return {'comment': comment}195 return {'comment': comment}
229196
=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
--- lib/lp/code/browser/tests/test_codereviewcomment.py 2009-06-25 04:06:00 +0000
+++ lib/lp/code/browser/tests/test_codereviewcomment.py 2009-10-26 14:01:12 +0000
@@ -5,11 +5,9 @@
55
6__metaclass__ = type6__metaclass__ = type
77
8from textwrap import dedent
9import unittest8import unittest
109
11from lp.code.browser.codereviewcomment import quote_text_as_email10from lp.testing import login_person, TestCaseWithFactory
12from lp.testing import login_person, TestCase, TestCaseWithFactory
13from canonical.launchpad.webapp.interfaces import IPrimaryContext11from canonical.launchpad.webapp.interfaces import IPrimaryContext
14from canonical.testing import DatabaseFunctionalLayer12from canonical.testing import DatabaseFunctionalLayer
1513
@@ -32,62 +30,5 @@
32 IPrimaryContext(comment.branch_merge_proposal).context)30 IPrimaryContext(comment.branch_merge_proposal).context)
3331
3432
35class TestQuoteTextAsEmail(TestCase):
36 """Test the quote_text_as_email helper method."""
37
38 def test_empty_string(self):
39 # Nothing just gives us an empty string.
40 self.assertEqual('', quote_text_as_email(''))
41
42 def test_none_string(self):
43 # If None is passed the quoted text is an empty string.
44 self.assertEqual('', quote_text_as_email(None))
45
46 def test_whitespace_string(self):
47 # Just whitespace gives us an empty string.
48 self.assertEqual('', quote_text_as_email(' \t '))
49
50 def test_long_string(self):
51 # Long lines are wrapped.
52 long_line = ('This is a very long line that needs to be wrapped '
53 'onto more than one line given a short length.')
54 self.assertEqual(
55 dedent("""\
56 > This is a very long line that needs to
57 > be wrapped onto more than one line
58 > given a short length."""),
59 quote_text_as_email(long_line, 40))
60
61 def test_code_sample(self):
62 # Initial whitespace is not trimmed.
63 code = """\
64 def test_whitespace_string(self):
65 # Nothing just gives us the prefix.
66 self.assertEqual('', wrap_text(' \t '))"""
67 self.assertEqual(
68 dedent("""\
69 > def test_whitespace_string(self):
70 > # Nothing just gives us the prefix.
71 > self.assertEqual('', wrap_text(' '))"""),
72 quote_text_as_email(code, 60))
73
74 def test_empty_line_mid_string(self):
75 # Lines in the middle of the string are quoted too.
76 value = dedent("""\
77 This is the first line.
78
79 This is the second line.
80 """)
81 expected = dedent("""\
82 > This is the first line.
83 >
84 > This is the second line.""")
85 self.assertEqual(expected, quote_text_as_email(value))
86
87 def test_trailing_whitespace(self):
88 # Trailing whitespace is removed.
89 self.assertEqual('> foo', quote_text_as_email(' foo \n '))
90
91
92def test_suite():33def test_suite():
93 return unittest.TestLoader().loadTestsFromName(__name__)34 return unittest.TestLoader().loadTestsFromName(__name__)
9435
=== modified file 'lib/lp/code/interfaces/codereviewcomment.py'
--- lib/lp/code/interfaces/codereviewcomment.py 2009-07-17 00:26:05 +0000
+++ lib/lp/code/interfaces/codereviewcomment.py 2009-10-26 14:01:12 +0000
@@ -66,6 +66,11 @@
66 attachments.66 attachments.
67 """67 """
6868
69 as_quoted_email = exported(
70 TextLine(
71 title=_('The message as quoted in email.'),
72 readonly=True))
73
6974
70class ICodeReviewCommentDeletion(Interface):75class ICodeReviewCommentDeletion(Interface):
71 """This interface provides deletion of CodeReviewComments.76 """This interface provides deletion of CodeReviewComments.
7277
=== modified file 'lib/lp/code/model/codereviewcomment.py'
--- lib/lp/code/model/codereviewcomment.py 2009-07-17 00:26:05 +0000
+++ lib/lp/code/model/codereviewcomment.py 2009-10-26 14:01:12 +0000
@@ -8,6 +8,8 @@
8 'CodeReviewComment',8 'CodeReviewComment',
9 ]9 ]
1010
11from textwrap import TextWrapper
12
11from zope.interface import implements13from zope.interface import implements
1214
13from sqlobject import ForeignKey, StringCol15from sqlobject import ForeignKey, StringCol
@@ -21,6 +23,38 @@
21from lp.code.interfaces.branchtarget import IHasBranchTarget23from lp.code.interfaces.branchtarget import IHasBranchTarget
2224
2325
26def quote_text_as_email(text, width=80):
27 """Quote the text as if it is an email response.
28
29 Uses '> ' as a line prefix, and breaks long lines.
30
31 Trailing whitespace is stripped.
32 """
33 # Empty text begets empty text.
34 if text is None:
35 return ''
36 text = text.rstrip()
37 if not text:
38 return ''
39 prefix = '> '
40 # The TextWrapper's handling of code is somewhat suspect.
41 wrapper = TextWrapper(
42 initial_indent=prefix,
43 subsequent_indent=prefix,
44 width=width,
45 replace_whitespace=False)
46 result = []
47 # Break the string into lines, and use the TextWrapper to wrap the
48 # individual lines.
49 for line in text.rstrip().split('\n'):
50 # TextWrapper won't do an indent of an empty string.
51 if line.strip() == '':
52 result.append(prefix)
53 else:
54 result.extend(wrapper.wrap(line))
55 return '\n'.join(result)
56
57
24class CodeReviewComment(SQLBase):58class CodeReviewComment(SQLBase):
25 """A table linking branch merge proposals and messages."""59 """A table linking branch merge proposals and messages."""
2660
@@ -72,3 +106,7 @@
72 attachment for attachment in attachments106 attachment for attachment in attachments
73 if attachment not in display_attachments]107 if attachment not in display_attachments]
74 return display_attachments, other_attachments108 return display_attachments, other_attachments
109
110 @property
111 def as_quoted_email(self):
112 return quote_text_as_email(self.message_body)
75113
=== modified file 'lib/lp/code/model/tests/test_codereviewcomment.py'
--- lib/lp/code/model/tests/test_codereviewcomment.py 2009-07-23 17:47:50 +0000
+++ lib/lp/code/model/tests/test_codereviewcomment.py 2009-10-26 14:01:12 +0000
@@ -3,12 +3,14 @@
33
4"""Unit tests for CodeReviewComment"""4"""Unit tests for CodeReviewComment"""
55
6from textwrap import dedent
6import unittest7import unittest
78
8from canonical.launchpad.database.message import MessageSet9from canonical.launchpad.database.message import MessageSet
9from lp.code.enums import CodeReviewVote10from lp.code.enums import CodeReviewVote
10from lp.code.event.branchmergeproposal import NewCodeReviewCommentEvent11from lp.code.event.branchmergeproposal import NewCodeReviewCommentEvent
11from lp.testing import TestCaseWithFactory12from lp.code.model.codereviewcomment import quote_text_as_email
13from lp.testing import TestCaseWithFactory, TestCase
12from canonical.testing import (14from canonical.testing import (
13 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)15 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
1416
@@ -183,6 +185,62 @@
183 self.assertEqual(([attachment.blob], []), comment.getAttachments())185 self.assertEqual(([attachment.blob], []), comment.getAttachments())
184186
185187
188class TestQuoteTextAsEmail(TestCase):
189 """Test the quote_text_as_email helper method."""
190
191 def test_empty_string(self):
192 # Nothing just gives us an empty string.
193 self.assertEqual('', quote_text_as_email(''))
194
195 def test_none_string(self):
196 # If None is passed the quoted text is an empty string.
197 self.assertEqual('', quote_text_as_email(None))
198
199 def test_whitespace_string(self):
200 # Just whitespace gives us an empty string.
201 self.assertEqual('', quote_text_as_email(' \t '))
202
203 def test_long_string(self):
204 # Long lines are wrapped.
205 long_line = ('This is a very long line that needs to be wrapped '
206 'onto more than one line given a short length.')
207 self.assertEqual(
208 dedent("""\
209 > This is a very long line that needs to
210 > be wrapped onto more than one line
211 > given a short length."""),
212 quote_text_as_email(long_line, 40))
213
214 def test_code_sample(self):
215 # Initial whitespace is not trimmed.
216 code = """\
217 def test_whitespace_string(self):
218 # Nothing just gives us the prefix.
219 self.assertEqual('', wrap_text(' \t '))"""
220 self.assertEqual(
221 dedent("""\
222 > def test_whitespace_string(self):
223 > # Nothing just gives us the prefix.
224 > self.assertEqual('', wrap_text(' '))"""),
225 quote_text_as_email(code, 60))
226
227 def test_empty_line_mid_string(self):
228 # Lines in the middle of the string are quoted too.
229 value = dedent("""\
230 This is the first line.
231
232 This is the second line.
233 """)
234 expected = dedent("""\
235 > This is the first line.
236 >
237 > This is the second line.""")
238 self.assertEqual(expected, quote_text_as_email(value))
239
240 def test_trailing_whitespace(self):
241 # Trailing whitespace is removed.
242 self.assertEqual('> foo', quote_text_as_email(' foo \n '))
243
186244
187def test_suite():245def test_suite():
188 return unittest.TestLoader().loadTestsFromName(__name__)246 return unittest.TestLoader().loadTestsFromName(__name__)
189247
=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-10-15 23:45:40 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-10-26 14:01:12 +0000
@@ -73,6 +73,7 @@
73 >>> print len(all_comments['entries'])73 >>> print len(all_comments['entries'])
74 274 2
75 >>> pprint_entry(all_comments['entries'][0])75 >>> pprint_entry(all_comments['entries'][0])
76 as_quoted_email: u'> This is great work'
76 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'77 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
77 id: 178 id: 1
78 message_body: u'This is great work'79 message_body: u'This is great work'
@@ -85,6 +86,7 @@
85 >>> comment_2 = webservice.named_get(86 >>> comment_2 = webservice.named_get(
86 ... merge_proposal['self_link'], 'getComment', id=2).jsonBody()87 ... merge_proposal['self_link'], 'getComment', id=2).jsonBody()
87 >>> pprint_entry(comment_2)88 >>> pprint_entry(comment_2)
89 as_quoted_email: u'> This is mediocre work.'
88 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'90 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
89 id: 291 id: 2
90 message_body: u'This is mediocre work.'92 message_body: u'This is mediocre work.'
@@ -153,6 +155,7 @@
153 >>> comment_link = comment_result.getHeader('Location')155 >>> comment_link = comment_result.getHeader('Location')
154 >>> comment = reviewer_webservice.get(comment_link).jsonBody()156 >>> comment = reviewer_webservice.get(comment_link).jsonBody()
155 >>> pprint_entry(comment)157 >>> pprint_entry(comment)
158 as_quoted_email: u'> This is great work'
156 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'159 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
157 id: 3160 id: 3
158 message_body: u'This is great work'161 message_body: u'This is great work'