Merge ~pappacena/launchpad:comment-editing-ui-codereview into launchpad:master

Proposed by Thiago F. Pappacena
Status: Merged
Approved by: Thiago F. Pappacena
Approved revision: 878eb77596392fa2add442988d196fd89cba5368
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~pappacena/launchpad:comment-editing-ui-codereview
Merge into: launchpad:master
Prerequisite: ~pappacena/launchpad:comment-editing-ui-bugcomment
Diff against target: 145 lines (+29/-8)
6 files modified
lib/lp/code/browser/codereviewcomment.py (+5/-0)
lib/lp/code/browser/tests/test_branchmergeproposal.py (+3/-1)
lib/lp/code/stories/branches/xx-code-review-comments.txt (+4/-4)
lib/lp/code/templates/branchmergeproposal-index.pt (+3/-1)
lib/lp/code/templates/codereviewcomment-header.pt (+12/-1)
lib/lp/services/comments/templates/comment.pt (+2/-1)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+402850@code.launchpad.net

Commit message

Adding editing option to code review comments

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

This should be good to go now.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/lib/lp/code/browser/codereviewcomment.py b/lib/lp/code/browser/codereviewcomment.py
index 094cd79..5891c6f 100644
--- a/lib/lp/code/browser/codereviewcomment.py
+++ b/lib/lp/code/browser/codereviewcomment.py
@@ -59,6 +59,7 @@ from lp.services.webapp import (
59 Navigation,59 Navigation,
60 stepthrough,60 stepthrough,
61 )61 )
62from lp.services.webapp.authorization import check_permission
62from lp.services.webapp.interfaces import ILaunchBag63from lp.services.webapp.interfaces import ILaunchBag
6364
6465
@@ -215,6 +216,10 @@ class CodeReviewCommentView(LaunchpadView):
215 return download_body(216 return download_body(
216 CodeReviewDisplayComment(self.context), self.request)217 CodeReviewDisplayComment(self.context), self.request)
217218
219 @property
220 def can_edit(self):
221 return check_permission('launchpad.Edit', self.context.message)
222
218 # Should the comment be shown in full?223 # Should the comment be shown in full?
219 full_comment = True224 full_comment = True
220 # Show comment expanders?225 # Show comment expanders?
diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
index f32350e..ba2ee85 100644
--- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
@@ -2315,7 +2315,9 @@ class TestBranchMergeProposal(BrowserTestCase):
2315 comment = self.factory.makeCodeReviewComment(body='x y' * 2000)2315 comment = self.factory.makeCodeReviewComment(body='x y' * 2000)
2316 has_read_more = self.has_read_more(comment)2316 has_read_more = self.has_read_more(comment)
2317 browser = self.getViewBrowser(comment.branch_merge_proposal)2317 browser = self.getViewBrowser(comment.branch_merge_proposal)
2318 self.assertNotIn('x y' * 2000, browser.contents)2318 txt = extract_text(
2319 find_tags_by_class(browser.contents, "comment-text")[0])
2320 self.assertNotIn('x y' * 2000, txt)
2319 self.assertThat(browser.contents, has_read_more)2321 self.assertThat(browser.contents, has_read_more)
23202322
2321 def test_short_conversation_comments_no_download(self):2323 def test_short_conversation_comments_no_download(self):
diff --git a/lib/lp/code/stories/branches/xx-code-review-comments.txt b/lib/lp/code/stories/branches/xx-code-review-comments.txt
index 1c3738d..30d3a26 100644
--- a/lib/lp/code/stories/branches/xx-code-review-comments.txt
+++ b/lib/lp/code/stories/branches/xx-code-review-comments.txt
@@ -39,7 +39,7 @@ to the main merge proposal page.
3939
40 >>> print_comments('boardCommentDetails')40 >>> print_comments('boardCommentDetails')
41 Eric the Viking (eric) wrote ...41 Eric the Viking (eric) wrote ...
42 >>> print_comments('boardCommentBody')42 >>> print_comments('comment-text')
43 This is a very long comment about what things should be done to the43 This is a very long comment about what things should be done to the
44 source branch to land it. When this comment is replied to, it should44 source branch to land it. When this comment is replied to, it should
45 wrap the line properly.45 wrap the line properly.
@@ -82,7 +82,7 @@ since it failed spuriously in buildbot.
8282
83After this, we are taken to the main page for the merge proposal83After this, we are taken to the main page for the merge proposal
8484
85 >>> print_comments('boardCommentBody', eric_browser, index=1)85 >>> print_comments('comment-text', eric_browser, index=1)
86 I like this.86 I like this.
87 I wish I had time to review it properly87 I wish I had time to review it properly
88 This is a longer message with several lines88 This is a longer message with several lines
@@ -91,7 +91,7 @@ After this, we are taken to the main page for the merge proposal
91Email addresses in code review comments are hidden for users not logged in.91Email addresses in code review comments are hidden for users not logged in.
9292
93 >>> anon_browser.open(merge_proposal_url)93 >>> anon_browser.open(merge_proposal_url)
94 >>> print_comments('boardCommentBody', anon_browser, index=1)94 >>> print_comments('comment-text', anon_browser, index=1)
95 I like this.95 I like this.
96 I wish I had time to review it properly96 I wish I had time to review it properly
97 This is a longer message with several lines97 This is a longer message with several lines
@@ -106,7 +106,7 @@ are also displayed in the new proposal.
106 >>> new_merge_proposal_url = canonical_url(new_merge_proposal)106 >>> new_merge_proposal_url = canonical_url(new_merge_proposal)
107 >>> logout()107 >>> logout()
108 >>> anon_browser.open(new_merge_proposal_url)108 >>> anon_browser.open(new_merge_proposal_url)
109 >>> print_comments('boardCommentBody', anon_browser, index=0)109 >>> print_comments('comment-text', anon_browser, index=0)
110 This is a very long comment about what things should be done to the110 This is a very long comment about what things should be done to the
111 source branch to land it. When this comment is replied to, it should111 source branch to land it. When this comment is replied to, it should
112 wrap the line properly.112 wrap the line properly.
diff --git a/lib/lp/code/templates/branchmergeproposal-index.pt b/lib/lp/code/templates/branchmergeproposal-index.pt
index 6ba4621..4a01ec5 100644
--- a/lib/lp/code/templates/branchmergeproposal-index.pt
+++ b/lib/lp/code/templates/branchmergeproposal-index.pt
@@ -244,7 +244,8 @@
244 'lp.code.branchmergeproposal.status', 'lp.app.comment',244 'lp.code.branchmergeproposal.status', 'lp.app.comment',
245 'lp.app.widgets.expander',245 'lp.app.widgets.expander',
246 'lp.code.branch.revisionexpander',246 'lp.code.branch.revisionexpander',
247 'lp.code.branchmergeproposal.inlinecomments', function(Y) {247 'lp.code.branchmergeproposal.inlinecomments',
248 'lp.services.messages.edit', function(Y) {
248249
249 Y.on('load', function() {250 Y.on('load', function() {
250 var logged_in = LP.links['me'] !== undefined;251 var logged_in = LP.links['me'] !== undefined;
@@ -285,6 +286,7 @@
285 '.expander-content',286 '.expander-content',
286 false,287 false,
287 Y.lp.code.branch.revisionexpander.bmp_diff_loader);288 Y.lp.code.branch.revisionexpander.bmp_diff_loader);
289 Y.lp.services.messages.edit.setup();
288 });290 });
289 });291 });
290<tal:script replace="structure string:&lt;/script&gt;" />292<tal:script replace="structure string:&lt;/script&gt;" />
diff --git a/lib/lp/code/templates/codereviewcomment-header.pt b/lib/lp/code/templates/codereviewcomment-header.pt
index efd37eb..77c4650 100644
--- a/lib/lp/code/templates/codereviewcomment-header.pt
+++ b/lib/lp/code/templates/codereviewcomment-header.pt
@@ -18,7 +18,13 @@
18 datetime context/comment_date/fmt:isodate"18 datetime context/comment_date/fmt:isodate"
19 tal:content="context/comment_date/fmt:displaydate">19 tal:content="context/comment_date/fmt:displaydate">
20 7 minutes ago20 7 minutes ago
21 </time>:21 </time><span class="editable-message-last-edit-date"><tal:last-edit condition="context/date_last_edited">
22 (last edit <time
23 itemprop="editTime"
24 tal:attributes="title context/date_last_edited/fmt:datetime;
25 datetime context/date_last_edited/fmt:isodate"
26 tal:content="context/date_last_edited/fmt:displaydate" />)</tal:last-edit>:
27 </span>
22 <span28 <span
23 tal:condition="context/from_superseded"29 tal:condition="context/from_superseded"
24 class="sprite warning-icon"30 class="sprite warning-icon"
@@ -29,6 +35,11 @@
29 of this proposal</span>35 of this proposal</span>
30 </td>36 </td>
3137
38 <td>
39 <img class="sprite edit action-icon editable-message-edit-btn"
40 tal:condition="view/can_edit"/>
41 </td>
42
32 <td class="bug-comment-index">43 <td class="bug-comment-index">
33 <a itemprop="url"44 <a itemprop="url"
34 tal:attributes="href context/fmt:url">#</a>45 tal:attributes="href context/fmt:url">#</a>
diff --git a/lib/lp/services/comments/templates/comment.pt b/lib/lp/services/comments/templates/comment.pt
index 19a4d1f..5061471 100644
--- a/lib/lp/services/comments/templates/comment.pt
+++ b/lib/lp/services/comments/templates/comment.pt
@@ -6,7 +6,8 @@
6 <div6 <div
7 itemscope=""7 itemscope=""
8 itemtype="http://schema.org/UserComments"8 itemtype="http://schema.org/UserComments"
9 tal:attributes="class string:boardComment ${context/extra_css_class|nothing}">9 tal:attributes="class string:boardComment editable-message ${context/extra_css_class|nothing};
10 data-baseurl context/fmt:url|nothing">
10 <div class="boardCommentDetails"11 <div class="boardCommentDetails"
11 tal:content="structure context/@@+comment-header">12 tal:content="structure context/@@+comment-header">
12 Details - everyone has details.13 Details - everyone has details.