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
1=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
2--- lib/canonical/launchpad/javascript/lp/comment.js 2009-10-13 15:56:13 +0000
3+++ lib/canonical/launchpad/javascript/lp/comment.js 2009-10-26 14:01:12 +0000
4@@ -85,9 +85,7 @@
5 if (!this.validate()) {
6 return;
7 }
8- this.set_disabled(true);
9- this.submit_button.get('parentNode').replaceChild(
10- this.progress_message, this.submit_button);
11+ this.activateProgressUI('Saving...');
12 this.post_comment(bind(function(message_entry) {
13 this.get_comment_HTML(
14 message_entry, bind(this.insert_comment_HTML, this));
15@@ -150,7 +148,18 @@
16 this.syncUI();
17 },
18 /**
19- * Stop indicating that a submission is in progress.
20+ * Activate indications of an operation in progress.
21+ *
22+ * @param message A user message describing the operation in progress.
23+ */
24+ activateProgressUI: function(message){
25+ this.progress_message.set('innerHTML', message);
26+ this.set_disabled(true);
27+ this.submit_button.get('parentNode').replaceChild(
28+ this.progress_message, this.submit_button);
29+ },
30+ /**
31+ * Stop indicating that an operation is in progress.
32 *
33 * @method clearProgressUI
34 */
35@@ -202,6 +211,7 @@
36 initializer: function() {
37 this.vote_input = Y.get('[id="field.vote"]');
38 this.review_type = Y.get('[id="field.review_type"]');
39+ this.in_reply_to = null;
40 },
41 /**
42 * Return the Submit button.
43@@ -265,7 +275,8 @@
44 content: this.comment_input.get('value'),
45 subject: '',
46 review_type: this.review_type.get('value'),
47- vote: this.get_vote()
48+ vote: this.get_vote(),
49+ parent: this.in_reply_to.get('self_link')
50 }
51 };
52 this.lp_client.named_post(
53@@ -290,6 +301,39 @@
54 });
55 },
56 /**
57+ * Event handler when a "Reply" link is clicked.
58+ *
59+ * @param e The Event object representing the click.
60+ */
61+ reply_clicked: function(e){
62+ e.halt();
63+ var reply_link = LP.client.normalize_uri(e.target.get('href'));
64+ var root_url = reply_link.substr(0,
65+ reply_link.length - '+reply'.length);
66+ var object_url = '/api/beta' + root_url;
67+ this.activateProgressUI('Loading...');
68+ window.scrollTo(0, Y.get('label [for=field.vote]').getY());
69+ this.lp_client.get(object_url, {
70+ on: {
71+ success: bind(function(comment){
72+ this.set_in_reply_to(comment);
73+ this.clearProgressUI();
74+ this.syncUI();
75+ }, this),
76+ failure: this.error_handler.getFailureHandler()
77+ }
78+ });
79+ },
80+ /**
81+ * Set the comment that the new comment will be in reply to.
82+ *
83+ * @param comment The comment to be in reply to.
84+ */
85+ set_in_reply_to: function(comment) {
86+ this.in_reply_to = comment;
87+ this.comment_input.set('value', comment.get('as_quoted_email'));
88+ },
89+ /**
90 * Reset the widget to a blank state.
91 *
92 * @method reset_contents
93@@ -297,6 +341,7 @@
94 reset_contents: function() {
95 this.review_type.set('value', '');
96 this.vote_input.set('selectedIndex', 0);
97+ this.in_reply_to = null;
98 CodeReviewComment.superclass.reset_contents.apply(this);
99 },
100 /**
101@@ -329,6 +374,7 @@
102 this.vote_input.on('mouseup', bind(this.syncUI, this));
103 this.review_type.on('keyup', bind(this.syncUI, this));
104 this.review_type.on('mouseup', bind(this.syncUI, this));
105+ Y.all('a.menu-link-reply').on('click', bind(this.reply_clicked, this));
106 },
107 /**
108 * Implementation of Widget.syncUI: Update appearance according to state.
109
110=== modified file 'lib/lp/code/browser/codereviewcomment.py'
111--- lib/lp/code/browser/codereviewcomment.py 2009-10-09 02:43:04 +0000
112+++ lib/lp/code/browser/codereviewcomment.py 2009-10-26 14:01:12 +0000
113@@ -11,7 +11,6 @@
114 'CodeReviewCommentView',
115 'CodeReviewDisplayComment',
116 ]
117-from textwrap import TextWrapper
118
119 from zope.app.form.browser import TextAreaWidget, DropdownWidget
120 from zope.interface import Interface, implements
121@@ -31,38 +30,6 @@
122 from lp.services.comments.interfaces.conversation import IComment
123
124
125-def quote_text_as_email(text, width=80):
126- """Quote the text as if it is an email response.
127-
128- Uses '> ' as a line prefix, and breaks long lines.
129-
130- Trailing whitespace is stripped.
131- """
132- # Empty text begets empty text.
133- if text is None:
134- return ''
135- text = text.rstrip()
136- if not text:
137- return ''
138- prefix = '> '
139- # The TextWrapper's handling of code is somewhat suspect.
140- wrapper = TextWrapper(
141- initial_indent=prefix,
142- subsequent_indent=prefix,
143- width=width,
144- replace_whitespace=False)
145- result = []
146- # Break the string into lines, and use the TextWrapper to wrap the
147- # individual lines.
148- for line in text.rstrip().split('\n'):
149- # TextWrapper won't do an indent of an empty string.
150- if line.strip() == '':
151- result.append(prefix)
152- else:
153- result.extend(wrapper.wrap(line))
154- return '\n'.join(result)
155-
156-
157 class CodeReviewDisplayComment:
158 """A code review comment or activity or both.
159
160@@ -222,7 +189,7 @@
161 quoted comment being replied to.
162 """
163 if self.is_reply:
164- comment = quote_text_as_email(self.reply_to.message_body)
165+ comment = self.reply_to.as_quoted_email
166 else:
167 comment = ''
168 return {'comment': comment}
169
170=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
171--- lib/lp/code/browser/tests/test_codereviewcomment.py 2009-06-25 04:06:00 +0000
172+++ lib/lp/code/browser/tests/test_codereviewcomment.py 2009-10-26 14:01:12 +0000
173@@ -5,11 +5,9 @@
174
175 __metaclass__ = type
176
177-from textwrap import dedent
178 import unittest
179
180-from lp.code.browser.codereviewcomment import quote_text_as_email
181-from lp.testing import login_person, TestCase, TestCaseWithFactory
182+from lp.testing import login_person, TestCaseWithFactory
183 from canonical.launchpad.webapp.interfaces import IPrimaryContext
184 from canonical.testing import DatabaseFunctionalLayer
185
186@@ -32,62 +30,5 @@
187 IPrimaryContext(comment.branch_merge_proposal).context)
188
189
190-class TestQuoteTextAsEmail(TestCase):
191- """Test the quote_text_as_email helper method."""
192-
193- def test_empty_string(self):
194- # Nothing just gives us an empty string.
195- self.assertEqual('', quote_text_as_email(''))
196-
197- def test_none_string(self):
198- # If None is passed the quoted text is an empty string.
199- self.assertEqual('', quote_text_as_email(None))
200-
201- def test_whitespace_string(self):
202- # Just whitespace gives us an empty string.
203- self.assertEqual('', quote_text_as_email(' \t '))
204-
205- def test_long_string(self):
206- # Long lines are wrapped.
207- long_line = ('This is a very long line that needs to be wrapped '
208- 'onto more than one line given a short length.')
209- self.assertEqual(
210- dedent("""\
211- > This is a very long line that needs to
212- > be wrapped onto more than one line
213- > given a short length."""),
214- quote_text_as_email(long_line, 40))
215-
216- def test_code_sample(self):
217- # Initial whitespace is not trimmed.
218- code = """\
219- def test_whitespace_string(self):
220- # Nothing just gives us the prefix.
221- self.assertEqual('', wrap_text(' \t '))"""
222- self.assertEqual(
223- dedent("""\
224- > def test_whitespace_string(self):
225- > # Nothing just gives us the prefix.
226- > self.assertEqual('', wrap_text(' '))"""),
227- quote_text_as_email(code, 60))
228-
229- def test_empty_line_mid_string(self):
230- # Lines in the middle of the string are quoted too.
231- value = dedent("""\
232- This is the first line.
233-
234- This is the second line.
235- """)
236- expected = dedent("""\
237- > This is the first line.
238- >
239- > This is the second line.""")
240- self.assertEqual(expected, quote_text_as_email(value))
241-
242- def test_trailing_whitespace(self):
243- # Trailing whitespace is removed.
244- self.assertEqual('> foo', quote_text_as_email(' foo \n '))
245-
246-
247 def test_suite():
248 return unittest.TestLoader().loadTestsFromName(__name__)
249
250=== modified file 'lib/lp/code/interfaces/codereviewcomment.py'
251--- lib/lp/code/interfaces/codereviewcomment.py 2009-07-17 00:26:05 +0000
252+++ lib/lp/code/interfaces/codereviewcomment.py 2009-10-26 14:01:12 +0000
253@@ -66,6 +66,11 @@
254 attachments.
255 """
256
257+ as_quoted_email = exported(
258+ TextLine(
259+ title=_('The message as quoted in email.'),
260+ readonly=True))
261+
262
263 class ICodeReviewCommentDeletion(Interface):
264 """This interface provides deletion of CodeReviewComments.
265
266=== modified file 'lib/lp/code/model/codereviewcomment.py'
267--- lib/lp/code/model/codereviewcomment.py 2009-07-17 00:26:05 +0000
268+++ lib/lp/code/model/codereviewcomment.py 2009-10-26 14:01:12 +0000
269@@ -8,6 +8,8 @@
270 'CodeReviewComment',
271 ]
272
273+from textwrap import TextWrapper
274+
275 from zope.interface import implements
276
277 from sqlobject import ForeignKey, StringCol
278@@ -21,6 +23,38 @@
279 from lp.code.interfaces.branchtarget import IHasBranchTarget
280
281
282+def quote_text_as_email(text, width=80):
283+ """Quote the text as if it is an email response.
284+
285+ Uses '> ' as a line prefix, and breaks long lines.
286+
287+ Trailing whitespace is stripped.
288+ """
289+ # Empty text begets empty text.
290+ if text is None:
291+ return ''
292+ text = text.rstrip()
293+ if not text:
294+ return ''
295+ prefix = '> '
296+ # The TextWrapper's handling of code is somewhat suspect.
297+ wrapper = TextWrapper(
298+ initial_indent=prefix,
299+ subsequent_indent=prefix,
300+ width=width,
301+ replace_whitespace=False)
302+ result = []
303+ # Break the string into lines, and use the TextWrapper to wrap the
304+ # individual lines.
305+ for line in text.rstrip().split('\n'):
306+ # TextWrapper won't do an indent of an empty string.
307+ if line.strip() == '':
308+ result.append(prefix)
309+ else:
310+ result.extend(wrapper.wrap(line))
311+ return '\n'.join(result)
312+
313+
314 class CodeReviewComment(SQLBase):
315 """A table linking branch merge proposals and messages."""
316
317@@ -72,3 +106,7 @@
318 attachment for attachment in attachments
319 if attachment not in display_attachments]
320 return display_attachments, other_attachments
321+
322+ @property
323+ def as_quoted_email(self):
324+ return quote_text_as_email(self.message_body)
325
326=== modified file 'lib/lp/code/model/tests/test_codereviewcomment.py'
327--- lib/lp/code/model/tests/test_codereviewcomment.py 2009-07-23 17:47:50 +0000
328+++ lib/lp/code/model/tests/test_codereviewcomment.py 2009-10-26 14:01:12 +0000
329@@ -3,12 +3,14 @@
330
331 """Unit tests for CodeReviewComment"""
332
333+from textwrap import dedent
334 import unittest
335
336 from canonical.launchpad.database.message import MessageSet
337 from lp.code.enums import CodeReviewVote
338 from lp.code.event.branchmergeproposal import NewCodeReviewCommentEvent
339-from lp.testing import TestCaseWithFactory
340+from lp.code.model.codereviewcomment import quote_text_as_email
341+from lp.testing import TestCaseWithFactory, TestCase
342 from canonical.testing import (
343 DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
344
345@@ -183,6 +185,62 @@
346 self.assertEqual(([attachment.blob], []), comment.getAttachments())
347
348
349+class TestQuoteTextAsEmail(TestCase):
350+ """Test the quote_text_as_email helper method."""
351+
352+ def test_empty_string(self):
353+ # Nothing just gives us an empty string.
354+ self.assertEqual('', quote_text_as_email(''))
355+
356+ def test_none_string(self):
357+ # If None is passed the quoted text is an empty string.
358+ self.assertEqual('', quote_text_as_email(None))
359+
360+ def test_whitespace_string(self):
361+ # Just whitespace gives us an empty string.
362+ self.assertEqual('', quote_text_as_email(' \t '))
363+
364+ def test_long_string(self):
365+ # Long lines are wrapped.
366+ long_line = ('This is a very long line that needs to be wrapped '
367+ 'onto more than one line given a short length.')
368+ self.assertEqual(
369+ dedent("""\
370+ > This is a very long line that needs to
371+ > be wrapped onto more than one line
372+ > given a short length."""),
373+ quote_text_as_email(long_line, 40))
374+
375+ def test_code_sample(self):
376+ # Initial whitespace is not trimmed.
377+ code = """\
378+ def test_whitespace_string(self):
379+ # Nothing just gives us the prefix.
380+ self.assertEqual('', wrap_text(' \t '))"""
381+ self.assertEqual(
382+ dedent("""\
383+ > def test_whitespace_string(self):
384+ > # Nothing just gives us the prefix.
385+ > self.assertEqual('', wrap_text(' '))"""),
386+ quote_text_as_email(code, 60))
387+
388+ def test_empty_line_mid_string(self):
389+ # Lines in the middle of the string are quoted too.
390+ value = dedent("""\
391+ This is the first line.
392+
393+ This is the second line.
394+ """)
395+ expected = dedent("""\
396+ > This is the first line.
397+ >
398+ > This is the second line.""")
399+ self.assertEqual(expected, quote_text_as_email(value))
400+
401+ def test_trailing_whitespace(self):
402+ # Trailing whitespace is removed.
403+ self.assertEqual('> foo', quote_text_as_email(' foo \n '))
404+
405
406 def test_suite():
407 return unittest.TestLoader().loadTestsFromName(__name__)
408
409=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
410--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-10-15 23:45:40 +0000
411+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-10-26 14:01:12 +0000
412@@ -73,6 +73,7 @@
413 >>> print len(all_comments['entries'])
414 2
415 >>> pprint_entry(all_comments['entries'][0])
416+ as_quoted_email: u'> This is great work'
417 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
418 id: 1
419 message_body: u'This is great work'
420@@ -85,6 +86,7 @@
421 >>> comment_2 = webservice.named_get(
422 ... merge_proposal['self_link'], 'getComment', id=2).jsonBody()
423 >>> pprint_entry(comment_2)
424+ as_quoted_email: u'> This is mediocre work.'
425 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
426 id: 2
427 message_body: u'This is mediocre work.'
428@@ -153,6 +155,7 @@
429 >>> comment_link = comment_result.getHeader('Location')
430 >>> comment = reviewer_webservice.get(comment_link).jsonBody()
431 >>> pprint_entry(comment)
432+ as_quoted_email: u'> This is great work'
433 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/1'
434 id: 3
435 message_body: u'This is great work'