launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:comment-editing-ui-codereview into launchpad:master with ~pappacena/launchpad:comment-editing-ui-bugcomment as a prerequisite.

Commit message:
Adding editing option to code review comments

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/402850
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:comment-editing-ui-codereview into launchpad:master.

diff --git a/lib/lp/code/browser/codereviewcomment.py b/lib/lp/code/browser/codereviewcomment.py
index 1892eb8..9dd89c2 100644
--- a/lib/lp/code/browser/codereviewcomment.py
+++ b/lib/lp/code/browser/codereviewcomment.py
@@ -59,6 +59,7 @@ from lp.services.webapp import (
     Navigation,
     stepthrough,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
 
 
@@ -211,6 +212,10 @@ class CodeReviewCommentView(LaunchpadView):
         return download_body(
             CodeReviewDisplayComment(self.context), self.request)
 
+    @property
+    def can_edit(self):
+        return check_permission('launchpad.Edit', self.context.message)
+
     # Should the comment be shown in full?
     full_comment = True
     # 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):
         comment = self.factory.makeCodeReviewComment(body='x y' * 2000)
         has_read_more = self.has_read_more(comment)
         browser = self.getViewBrowser(comment.branch_merge_proposal)
-        self.assertNotIn('x y' * 2000, browser.contents)
+        txt = extract_text(
+            find_tags_by_class(browser.contents, "comment-text")[0])
+        self.assertNotIn('x y' * 2000, txt)
         self.assertThat(browser.contents, has_read_more)
 
     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.
 
     >>> print_comments('boardCommentDetails')
     Eric the Viking (eric) wrote ...
-    >>> print_comments('boardCommentBody')
+    >>> print_comments('comment-text')
     This is a very long comment about what things should be done to the
     source branch to land it. When this comment is replied to, it should
     wrap the line properly.
@@ -82,7 +82,7 @@ since it failed spuriously in buildbot.
 
 After this, we are taken to the main page for the merge proposal
 
-    >>> print_comments('boardCommentBody', eric_browser, index=1)
+    >>> print_comments('comment-text', eric_browser, index=1)
     I like this.
     I wish I had time to review it properly
     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
 Email addresses in code review comments are hidden for users not logged in.
 
     >>> anon_browser.open(merge_proposal_url)
-    >>> print_comments('boardCommentBody', anon_browser, index=1)
+    >>> print_comments('comment-text', anon_browser, index=1)
     I like this.
     I wish I had time to review it properly
     This is a longer message with several lines
@@ -106,7 +106,7 @@ are also displayed in the new proposal.
     >>> new_merge_proposal_url = canonical_url(new_merge_proposal)
     >>> logout()
     >>> anon_browser.open(new_merge_proposal_url)
-    >>> print_comments('boardCommentBody', anon_browser, index=0)
+    >>> print_comments('comment-text', anon_browser, index=0)
     This is a very long comment about what things should be done to the
     source branch to land it. When this comment is replied to, it should
     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 @@
           'lp.code.branchmergeproposal.status', 'lp.app.comment',
           'lp.app.widgets.expander',
           'lp.code.branch.revisionexpander',
-          'lp.code.branchmergeproposal.inlinecomments', function(Y) {
+          'lp.code.branchmergeproposal.inlinecomments',
+          'lp.services.messages.edit', function(Y) {
 
     Y.on('load', function() {
         var logged_in = LP.links['me'] !== undefined;
@@ -285,6 +286,7 @@
             '.expander-content',
             false,
             Y.lp.code.branch.revisionexpander.bmp_diff_loader);
+        Y.lp.services.messages.edit.setup();
     });
   });
 <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..897c743 100644
--- a/lib/lp/code/templates/codereviewcomment-header.pt
+++ b/lib/lp/code/templates/codereviewcomment-header.pt
@@ -18,7 +18,14 @@
             datetime context/comment_date/fmt:isodate"
           tal:content="context/comment_date/fmt:displaydate">
           7 minutes ago
-        </time>:
+        </time><span tal:condition="not:context/date_last_edited">:</span>
+            <span tal:condition="context/date_last_edited">
+              (last edit <time
+                    itemprop="editTime"
+                    tal:attributes="title context/date_last_edited/fmt:datetime;
+                      datetime context/date_last_edited/fmt:isodate"
+                    tal:content="context/date_last_edited/fmt:displaydate" />):
+            </span>
         <span
           tal:condition="context/from_superseded"
           class="sprite warning-icon"
@@ -29,6 +36,11 @@
             of this proposal</span>
       </td>
 
+      <td>
+        <img class="sprite edit action-icon editable-message-edit-btn"
+             tal:condition="view/can_edit"/>
+      </td>
+
       <td class="bug-comment-index">
         <a itemprop="url"
           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 @@
    <div
     itemscope=""
     itemtype="http://schema.org/UserComments";
-    tal:attributes="class string:boardComment ${context/extra_css_class|nothing}">
+    tal:attributes="class string:boardComment editable-message ${context/extra_css_class|nothing};
+                    data-baseurl context/fmt:url|nothing">
     <div class="boardCommentDetails"
          tal:content="structure context/@@+comment-header">
       Details - everyone has details.
diff --git a/lib/lp/services/messages/javascript/messages.edit.js b/lib/lp/services/messages/javascript/messages.edit.js
index f61eb87..5908b0f 100644
--- a/lib/lp/services/messages/javascript/messages.edit.js
+++ b/lib/lp/services/messages/javascript/messages.edit.js
@@ -121,7 +121,7 @@ YUI.add('lp.services.messages.edit', function(Y) {
 
             // If the edit button is not present, do not try to bind the
             // handlers.
-            if (!edit_btn) {
+            if (!edit_btn || !baseurl) {
                 return;
             }