launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/more-comment-footers into lp:launchpad.

Commit message:
Show spam controls for comments on non-mergeable MPs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1680746 in Launchpad itself: "No way to remove merge proposal comment spam"
  https://bugs.launchpad.net/launchpad/+bug/1680746

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/more-comment-footers/+merge/345303

These don't show a Reply link, but they should still show the Hide/Unhide links for users who have permission to use them.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/more-comment-footers into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
--- lib/lp/code/browser/tests/test_codereviewcomment.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/browser/tests/test_codereviewcomment.py	2018-05-09 16:34:28 +0000
@@ -7,6 +7,8 @@
 
 __metaclass__ = type
 
+import re
+
 from soupmatchers import (
     HTMLContains,
     Tag,
@@ -209,6 +211,61 @@
         browser = self.getViewBrowser(comment, view_name='+reply')
         self.assertIn(contents, browser.contents)
 
+    def test_footer_for_mergeable_and_admin(self):
+        """An admin sees Hide/Reply links for a comment on a mergeable MP."""
+        comment = self.makeCodeReviewComment()
+        display_comment = CodeReviewDisplayComment(comment)
+        browser = self.getViewBrowser(
+            display_comment, user=self.factory.makeAdministrator())
+        footer = Tag('comment footer', 'div', {'class': 'boardCommentFooter'})
+        hide_link = Tag('hide link', 'a', text=re.compile(r'\s*Hide\s*'))
+        reply_link = Tag('reply link', 'a', text='Reply')
+        self.assertThat(
+            browser.contents, HTMLContains(hide_link.within(footer)))
+        self.assertThat(
+            browser.contents, HTMLContains(reply_link.within(footer)))
+
+    def test_footer_for_mergeable_and_non_admin(self):
+        """A mortal sees a Reply link for a comment on a mergeable MP."""
+        comment = self.makeCodeReviewComment()
+        display_comment = CodeReviewDisplayComment(comment)
+        browser = self.getViewBrowser(display_comment)
+        footer = Tag('comment footer', 'div', {'class': 'boardCommentFooter'})
+        hide_link = Tag('hide link', 'a', text=re.compile(r'\s*Hide\s*'))
+        reply_link = Tag('reply link', 'a', text='Reply')
+        self.assertThat(browser.contents, Not(HTMLContains(hide_link)))
+        self.assertThat(
+            browser.contents, HTMLContains(reply_link.within(footer)))
+
+    def test_footer_for_non_mergeable_and_admin(self):
+        """An admin sees a Hide link for a comment on a non-mergeable MP."""
+        comment = self.makeCodeReviewComment()
+        merge_proposal = comment.branch_merge_proposal
+        with person_logged_in(merge_proposal.registrant):
+            merge_proposal.markAsMerged(
+                merge_reporter=merge_proposal.registrant)
+        display_comment = CodeReviewDisplayComment(comment)
+        browser = self.getViewBrowser(
+            display_comment, user=self.factory.makeAdministrator())
+        footer = Tag('comment footer', 'div', {'class': 'boardCommentFooter'})
+        hide_link = Tag('hide link', 'a', text=re.compile(r'\s*Hide\s*'))
+        reply_link = Tag('reply link', 'a', text='Reply')
+        self.assertThat(
+            browser.contents, HTMLContains(hide_link.within(footer)))
+        self.assertThat(browser.contents, Not(HTMLContains(reply_link)))
+
+    def test_no_footer_for_non_mergeable_and_non_admin(self):
+        """A mortal sees no footer for a comment on a non-mergeable MP."""
+        comment = self.makeCodeReviewComment()
+        merge_proposal = comment.branch_merge_proposal
+        with person_logged_in(merge_proposal.registrant):
+            merge_proposal.markAsMerged(
+                merge_reporter=merge_proposal.registrant)
+        display_comment = CodeReviewDisplayComment(comment)
+        browser = self.getViewBrowser(display_comment)
+        footer = Tag('comment footer', 'div', {'class': 'boardCommentFooter'})
+        self.assertThat(browser.contents, Not(HTMLContains(footer)))
+
 
 class TestCodeReviewCommentHtmlBzr(
     TestCodeReviewCommentHtmlMixin, BrowserTestCase):

=== modified file 'lib/lp/services/comments/templates/comment.pt'
--- lib/lp/services/comments/templates/comment.pt	2018-04-25 12:01:33 +0000
+++ lib/lp/services/comments/templates/comment.pt	2018-05-09 16:34:28 +0000
@@ -24,7 +24,8 @@
     </div>
     <div class="boardCommentFooter"
          tal:define="link context/menu:context/reply | nothing"
-         tal:condition="link/enabled | nothing">
+         tal:condition="python: context.show_spam_controls or
+                                getattr(link, 'enabled', False)">
         <a tal:condition="context/show_spam_controls"
            tal:attributes="id string:mark-spam-${context/index};"
            class="js-action mark-spam" href="#">
@@ -32,6 +33,7 @@
           <tal:spam condition="context/visible">Hide</tal:spam>
         </a>
         <a itemprop="replyToUrl"
+             tal:condition="link/enabled | nothing"
              tal:attributes="href link/fmt:url"
              tal:content="link/escapedtext">
              Reply