launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/comment-display-rules-questions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #769173 in Launchpad itself: ""Hidden" comments should be visible to registry experts"
  https://bugs.launchpad.net/launchpad/+bug/769173

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/comment-display-rules-questions/+merge/59104

Summary
=======
Messages on questions can be hidden via the API, but once hidden *no one* can see them. Admins and registry experts, who have the ability to show or hide messages, should still be able to see them so they can review if a hidden message should actually be shown. Per the styling and UI in bug messages, those hidden messages should be styled so as to be clearly marked as hidden messages when an admin or registry expert sees them.

Preimplementation
=================
Spoke with Curtis Hovey

Implementation
==============
lib/lp/answers/browser/question.py
----------------------------------
Added a visible_messages property to the question view to weed out questionmessages marked as not visible if the user isn't an admin or registry_expert.

Added a method to provide the hidden admin comment css class for messages that are marked as not visible but are rendering because the user is an admin or registy expert.

ib/lp/answers/interfaces/questionmessage.py
lib/lp/answers/model/questionmessage.py
---------------------------------------
Added an index property to QuestionMessage, to display the index of the message when viewing questions. Otherwise currently you have to count the messages by hand to find the right one to hide via the API. Which is terrible.

lib/lp/answers/templates/question-index.pt
lib/lp/answers/templates/questionmessage-display.pt
---------------------------------------------------
Updated the question view template to use visible_messages to determine which messages to show, rather than checking message.visible.

Updated the questionmessage template to get the css class from the new method so it appropriately styles hidden messages.

lib/lp/answers/browser/tests/test_questioncomment_visibility.py
---------------------------------------------------------------
Tests.

QA
==
Hide a comment on a question using setCommentVisibility

{{{
    lp = Launchpad.login_with(...)
    q = lp.load('/$product/+question/$question_number')
    q.setCommentVisibility(comment_number=$number_to_hide, visible=False)
}}}

The comment should be invisible if you are not logged in. It should be visible, but colored yellow (per the style for hidden bug comments) if you are logged in as an admin or registry_expert.

You can also double check that messages on questions now show their index to the right of the message box (e.g. the first message is #1).

Tests
=====
bin/test -vvct test_questioncomment_visibility

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/browser/question.py
  lib/lp/answers/browser/tests/test_questioncomment_visibility.py
  lib/lp/answers/interfaces/questionmessage.py
  lib/lp/answers/model/questionmessage.py
  lib/lp/answers/templates/question-index.pt
  lib/lp/answers/templates/questionmessage-display.pt

-- 
https://code.launchpad.net/~jcsackett/launchpad/comment-display-rules-questions/+merge/59104
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/comment-display-rules-questions into lp:launchpad.

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2011-04-18 01:07:08 +0000
+++ lib/lp/answers/browser/question.py	2011-04-26 17:35:58 +0000
@@ -64,6 +64,7 @@
 from canonical.launchpad.interfaces.launchpadstatistic import (
     ILaunchpadStatisticSet,
     )
+from canonical.launchpad.utilities.personroles import PersonRoles
 from canonical.launchpad.webapp import (
     ApplicationMenu,
     canonical_url,
@@ -879,6 +880,17 @@
                 return True
         return False
 
+    @property
+    def visible_messages(self):
+        messages = self.context.messages
+        strip_invisible = True
+        if self.user is not None:
+            role = PersonRoles(self.user)
+            strip_invisible = not (role.in_admin or role.in_registry_experts)
+        if strip_invisible:
+            messages = [message for message in messages if message.visible]
+        return messages
+
     def canAddComment(self, action):
         """Return whether the comment action should be displayed.
 
@@ -1094,6 +1106,14 @@
         else:
             return "boardCommentBody"
 
+    def getBoardCommentCSSClass(self):
+        css_classes = ["boardComment"]
+        if not self.context.visible:
+            # If a comment that isn't visible is being rendered, it's being
+            # rendered for an admin or registry_expert.
+            css_classes.append("adminHiddenComment")
+        return " ".join(css_classes)
+
     def canConfirmAnswer(self):
         """Return True if the user can confirm this answer."""
         return (self.display_confirm_button and

=== added file 'lib/lp/answers/browser/tests/test_questioncomment_visibility.py'
--- lib/lp/answers/browser/tests/test_questioncomment_visibility.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/browser/tests/test_questioncomment_visibility.py	2011-04-26 17:35:58 +0000
@@ -0,0 +1,74 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the various rules around question comment visibility."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import (
+    BrowserTestCase,
+    person_logged_in,
+    )
+
+
+class TestQuestionCommentVisibility(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def _makeQuestionWithHiddenComment(self, questionbody=None):
+        administrator = getUtility(ILaunchpadCelebrities).admin.teamowner
+        with person_logged_in(administrator):
+            question = self.factory.makeQuestion()
+            comment = question.addComment(administrator, questionbody)
+            removeSecurityProxy(comment).message.visible = False
+        return question
+
+    def _getUserForTest(self, team=None):
+        person = self.factory.makePerson()
+        if team is not None:
+            with person_logged_in(team.teamowner):
+                team.addMember(person, team.teamowner)
+        return person
+
+    def test_admin_can_see_comments(self):
+        comment_text = "You can't see me."
+        question = self._makeQuestionWithHiddenComment(comment_text)
+        admin_team = getUtility(ILaunchpadCelebrities).admin
+        administrator = self._getUserForTest(admin_team)
+        view = self.getViewBrowser(
+            context=question, user=administrator)
+        self.assertTrue(
+           comment_text in view.contents,
+           "Administrator cannot see the hidden comment.")
+
+    def test_registry_can_see_comments(self):
+        comment_text = "You can't see me."
+        question = self._makeQuestionWithHiddenComment(comment_text)
+        registry_team = getUtility(ILaunchpadCelebrities).registry_experts
+        registry_expert = self._getUserForTest(registry_team)
+        view = self.getViewBrowser(
+            context=question, user=registry_expert)
+        self.assertTrue(
+           comment_text in view.contents,
+           "Registy member cannot see the hidden comment.")
+
+    def test_anon_cannot_see_comments(self):
+        comment_text = "You can't see me."
+        question = self._makeQuestionWithHiddenComment(comment_text)
+        view = self.getViewBrowser(context=question, no_login=True)
+        self.assertFalse(
+           comment_text in view.contents,
+           "Anonymous person can see the hidden comment.")
+
+    def test_random_cannot_see_comments(self):
+        comment_text = "You can't see me."
+        question = self._makeQuestionWithHiddenComment(comment_text)
+        view = self.getViewBrowser(context=question)
+        self.assertFalse(
+           comment_text in view.contents,
+           "Random user can see the hidden comment.")

=== modified file 'lib/lp/answers/interfaces/questionmessage.py'
--- lib/lp/answers/interfaces/questionmessage.py	2011-03-27 21:40:32 +0000
+++ lib/lp/answers/interfaces/questionmessage.py	2011-04-26 17:35:58 +0000
@@ -15,6 +15,7 @@
     Bool,
     Choice,
     Field,
+    Int,
     )
 
 from canonical.launchpad import _
@@ -48,8 +49,12 @@
         "related the action operated by this message."), required=True,
         readonly=True, default=QuestionStatus.OPEN,
         vocabulary=QuestionStatus)
+    index = Int(
+        title=_("Message index."),
+        description=_("The messages index in the question's list of "
+        "messages."),
+        readonly=True)
     visible = Bool(
         title=_("Message visibility."),
         description=_("Whether or not the message is visible."),
         readonly=True)
-

=== modified file 'lib/lp/answers/model/questionmessage.py'
--- lib/lp/answers/model/questionmessage.py	2011-03-27 21:40:32 +0000
+++ lib/lp/answers/model/questionmessage.py	2011-04-26 17:35:58 +0000
@@ -23,6 +23,7 @@
     QuestionStatus,
     )
 from lp.answers.interfaces.questionmessage import IQuestionMessage
+from lp.services.propertycache import cachedproperty
 
 
 class QuestionMessage(SQLBase):
@@ -49,6 +50,11 @@
         # Delegates do not proxy __ methods, because of the name mangling.
         return iter(self.chunks)
 
+    @cachedproperty
+    def index(self):
+        # Return the index + 1 so that messages appear 1-indexed in the UI.
+        return list(self.question.messages).index(self) + 1
+
     @property
     def visible(self):
         """See `IQuestionMessage.`"""

=== modified file 'lib/lp/answers/templates/question-index.pt'
--- lib/lp/answers/templates/question-index.pt	2011-03-27 21:40:32 +0000
+++ lib/lp/answers/templates/question-index.pt	2011-04-26 17:35:58 +0000
@@ -91,9 +91,8 @@
       </div>
 
 
-      <tal:message repeat="message context/messages">
-      <div tal:condition="message/visible"
-           tal:replace="structure message/@@+display" />
+      <tal:message repeat="message view/visible_messages">
+      <div tal:replace="structure message/@@+display" />
       </tal:message>
 
       <div id="question"

=== modified file 'lib/lp/answers/templates/questionmessage-display.pt'
--- lib/lp/answers/templates/questionmessage-display.pt	2009-09-25 08:39:54 +0000
+++ lib/lp/answers/templates/questionmessage-display.pt	2011-04-26 17:35:58 +0000
@@ -2,9 +2,9 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal";
   xmlns:metal="http://xml.zope.org/namespaces/metal";
   omit-tag="">
-
-<div class="boardComment">
-
+<div
+  tal:define="css_classes view/getBoardCommentCSSClass"
+  tal:attributes="class string:boardComment ${css_classes}">
   <div class="boardCommentDetails">
     <tal:bestanswer condition="view/isBestAnswer">
       <img src="/@@/favourite-yes" style="float:right;" alt="Best"
@@ -18,6 +18,8 @@
       tal:attributes="title context/datecreated/fmt:datetime"
       tal:content="context/datecreated/fmt:displaydate">Thursday
     13:21</span>:
+    <span style="float:right;"
+       tal:content="string: #${context/index}" />
   </div>
 
   <div class="boardCommentBody"