Merge lp:~sinzui/launchpad/hide-question-comment into lp:launchpad

[Curtis Hovey]

Allow users to hide their question comments.

Users can hide their bug comments, but not their question comment.
The QuestionMessageDisplayView.canSeeSpamControls checks if the
user has launchpad.Moderate on the question, which users do not have.
The test must check the comment.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * security.SetQuestionCommentVisibility (launchpad.Moderate) only
      allows ~admins and ~registry
      * The question owner check is missing.
      * IQuestion is checked, but IQuestionComment is the context.
    * Change the checker to be for IQuestionMessage and include
      the comment owner in th check.
    * Revise or remove SetQuestionCommentVisibility because the permission
      does not have enough information to know if the user is the comment
      owner. Maybe move the permission check into setCommentVisibility()
      as done by the rules to delete bug tasks.

QA

    As a non-priv person (non-project maintainer and not ~registry)
    * Visit https://answers.qastaging.launchpad.net/launchpad/+question/188577
    * Verify you can add a comment.
    * Verify you can hide the comment after you reload the page.
    * Verify you can unhide the comment.

LINT

    lib/lp/security.py
    lib/lp/answers/configure.zcml
    lib/lp/answers/browser/question.py
    lib/lp/answers/browser/tests/test_questionmessages.py
    lib/lp/answers/doc/faq.txt
    lib/lp/answers/model/question.py
    lib/lp/answers/tests/test_question_webservice.py

TEST

    ./bin/test -vvc lp.answers.browser.tests.test_questionmessage
    ./bin/test -vvc -t Comment lp.answers.tests.test_question_webservice

LoC

    This is feature work...maybe the last disclosure bug that needs to
    be fixed.

IMPLEMENTATION

Fixed the doctest to reflect the permission used by the code.
    lib/lp/answers/doc/faq.txt

Ensure comment owners can see their hidden comments and the hide/unhide
controls. Tests should *never* use admins as the creator of data -- no
admin asks a question in Lp.
    lib/lp/security.py
    lib/lp/answers/browser/question.py
    lib/lp/answers/browser/tests/test_questionmessages.py

Removed SetQuestionCommentVisibility because the argument is what
determines permission to IQuestionMessage.message.visibility. Moved
setCommentVisibility() into the methods anyone can call, but added a
launchpad.Moderate check on the QuestionMessage to ensure access is
still guarded. Admins do not ask questions...set the test up as
questions are really used.
    lib/lp/security.py
    lib/lp/answers/configure.zcml
    lib/lp/answers/model/question.py
    lib/lp/answers/tests/test_question_webservice.py

[j.c.sackett (jcsackett)]

Looks good, Curtis. Thanks.

review: Approve

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2012-02-21 22:46:28 +0000
+++ lib/lp/answers/browser/question.py	2012-10-30 17:38:23 +0000
@@ -923,7 +923,9 @@
             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]
+            messages = [
+                message for message in messages
+                if message.visible or message.owner == self.user]
         return messages
 
     def canAddComment(self, action):
@@ -1143,7 +1145,7 @@
 
     @cachedproperty
     def canSeeSpamControls(self):
-        return check_permission('launchpad.Moderate', self.context.question)
+        return check_permission('launchpad.Moderate', self.context)
 
     def getBoardCommentCSSClass(self):
         css_classes = ["boardComment"]

=== modified file 'lib/lp/answers/browser/tests/test_questionmessages.py'
--- lib/lp/answers/browser/tests/test_questionmessages.py	2012-01-01 02:58:52 +0000
+++ lib/lp/answers/browser/tests/test_questionmessages.py	2012-10-30 17:38:23 +0000
@@ -18,6 +18,7 @@
     person_logged_in,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.pages import find_tag_by_id
 
 
 class TestQuestionMessageVisibility(
@@ -28,9 +29,10 @@
     def makeHiddenMessage(self):
         """Required by the mixin."""
         administrator = getUtility(ILaunchpadCelebrities).admin.teamowner
+        self.commenter = self.factory.makePerson()
         with person_logged_in(administrator):
             question = self.factory.makeQuestion()
-            comment = question.addComment(administrator, self.comment_text)
+            comment = question.addComment(self.commenter, self.comment_text)
             removeSecurityProxy(comment).message.visible = False
         return question
 
@@ -42,6 +44,12 @@
             no_login=no_login)
         return view
 
+    def test_commenter_can_see_comments(self):
+        # The author of the comment can see the hidden comment.
+        context = self.makeHiddenMessage()
+        view = self.getView(context=context, user=self.commenter)
+        self.assertIn(self.comment_text, view.contents)
+
 
 class TestHideQuestionMessageControls(
         BrowserTestCase, TestHideMessageControlMixin):
@@ -53,10 +61,11 @@
     def getContext(self, comment_owner=None):
         """Required by the mixin."""
         administrator = getUtility(ILaunchpadCelebrities).admin.teamowner
+        user = comment_owner or administrator
         question = self.factory.makeQuestion()
         body = self.factory.getUniqueString()
-        with person_logged_in(administrator):
-            question.addComment(administrator, body)
+        with person_logged_in(user):
+            question.addComment(user, body)
         return question
 
     def getView(self, context, user=None, no_login=False):
@@ -66,3 +75,11 @@
             user=user,
             no_login=no_login)
         return view
+
+    def test_comment_owner_sees_hide_control(self):
+        # The comment owner sees the hide control.
+        user = self.factory.makePerson()
+        context = self.getContext(comment_owner=user)
+        view = self.getView(context=context, user=user)
+        hide_link = find_tag_by_id(view.contents, self.control_text)
+        self.assertIsNot(None, hide_link)

=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/answers/configure.zcml	2012-10-30 17:38:23 +0000
@@ -110,7 +110,8 @@
     <require
       permission="launchpad.AnyPerson"
       attributes="requestInfo giveAnswer expireQuestion addComment
-                  addCommentWithoutNotify linkFAQ subscribe unsubscribe"
+                  addCommentWithoutNotify linkFAQ subscribe unsubscribe
+                  setCommentVisibility"
       set_attributes="title description datedue target language"
       />
     <require
@@ -119,10 +120,6 @@
       set_attributes="assignee"
       />
     <require
-      permission="launchpad.Moderate"
-      attributes="setCommentVisibility"
-      />
-    <require
       permission="launchpad.Admin"
       attributes="setStatus"
       set_attributes="priority whiteboard"

=== modified file 'lib/lp/answers/doc/faq.txt'
--- lib/lp/answers/doc/faq.txt	2012-07-27 13:12:41 +0000
+++ lib/lp/answers/doc/faq.txt	2012-10-30 17:38:23 +0000
@@ -29,7 +29,7 @@
     >>> verifyObject(IFAQTarget, removeSecurityProxy(ubuntu))
     True
 
-Any user who has 'launchpad.Moderate' on the project can create a new
+Any user who has 'launchpad.Append' on the project can create a new
 FAQ. (That permission is granted to project's registrant and answer
 contacts.)
 

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2012-07-27 13:12:41 +0000
+++ lib/lp/answers/model/question.py	2012-10-30 17:38:23 +0000
@@ -43,6 +43,7 @@
     implements,
     providedBy,
     )
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import isinstance as zope_isinstance
 
 from lp.answers.enums import (
@@ -113,6 +114,7 @@
     MessageChunk,
     )
 from lp.services.propertycache import cachedproperty
+from lp.services.webapp.authorization import check_permission
 from lp.services.worlddata.helpers import is_english_variant
 from lp.services.worlddata.interfaces.language import ILanguage
 from lp.services.worlddata.model.language import Language
@@ -680,7 +682,12 @@
 
     def setCommentVisibility(self, user, comment_number, visible):
         """See `IQuestion`."""
-        message = self.messages[comment_number].message
+        question_message = self.messages[comment_number]
+        if not check_permission('launchpad.Moderate', question_message):
+            raise Unauthorized(
+                "Only admins, project maintainers, and comment authors "
+                "can change a comment's visibility.")
+        message = question_message.message
         message.visible = visible
 
 

=== modified file 'lib/lp/answers/tests/test_question_webservice.py'
--- lib/lp/answers/tests/test_question_webservice.py	2012-10-09 10:28:02 +0000
+++ lib/lp/answers/tests/test_question_webservice.py	2012-10-30 17:38:23 +0000
@@ -9,7 +9,6 @@
 from lazr.restfulclient.errors import HTTPError
 from simplejson import dumps
 import transaction
-from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.answers.errors import (
@@ -21,7 +20,6 @@
     NotQuestionOwnerError,
     QuestionTargetError,
     )
-from lp.registry.interfaces.person import IPersonSet
 from lp.testing import (
     celebrity_logged_in,
     launchpadlib_for,
@@ -144,13 +142,11 @@
 
     def setUp(self):
         super(TestSetCommentVisibility, self).setUp()
-        self.person_set = getUtility(IPersonSet)
-        admins = self.person_set.getByName('admins')
-        self.admin = admins.teamowner
-        with person_logged_in(self.admin):
+        self.commenter = self.factory.makePerson()
+        with person_logged_in(self.commenter):
             self.question = self.factory.makeQuestion()
             self.message = self.question.addComment(
-                self.admin, 'Some comment')
+                self.commenter, 'Some comment')
         transaction.commit()
 
     def _get_question_for_user(self, user=None):
@@ -169,8 +165,8 @@
     def test_random_user_cannot_set_visible(self):
         # Logged in users without privs can't set question comment
         # visibility.
-        nopriv = self.person_set.getByName('no-priv')
-        question = self._get_question_for_user(nopriv)
+        random_user = self.factory.makePerson()
+        question = self._get_question_for_user(random_user)
         self.assertRaises(
             HTTPError,
             self._set_visibility,
@@ -185,13 +181,18 @@
             self._set_visibility,
             question)
 
+    def test_comment_owner_can_set_visible(self):
+        # Members of registry experts can set question comment
+        # visibility.
+        question = self._get_question_for_user(self.commenter)
+        self._set_visibility(question)
+        self.assertFalse(self.message.visible)
+
     def test_registry_admin_can_set_visible(self):
         # Members of registry experts can set question comment
         # visibility.
-        registry = self.person_set.getByName('registry')
-        person = self.factory.makePerson()
-        with person_logged_in(registry.teamowner):
-            registry.addMember(person, registry.teamowner)
+        with celebrity_logged_in('registry_experts') as registry:
+            person = registry
         question = self._get_question_for_user(person)
         self._set_visibility(question)
         self.assertFalse(self.message.visible)
@@ -199,10 +200,8 @@
     def test_admin_can_set_visible(self):
         # Admins can set question comment
         # visibility.
-        admins = self.person_set.getByName('admins')
-        person = self.factory.makePerson()
-        with person_logged_in(admins.teamowner):
-            admins.addMember(person, admins.teamowner)
+        with celebrity_logged_in('admin') as admin:
+            person = admin
         question = self._get_question_for_user(person)
         self._set_visibility(question)
         self.assertFalse(self.message.visible)

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-10-22 13:53:11 +0000
+++ lib/lp/security.py	2012-10-30 17:38:23 +0000
@@ -1983,15 +1983,6 @@
         return objects
 
 
-class SetQuestionCommentVisibility(AuthorizationBase):
-    permission = 'launchpad.Moderate'
-    usedfor = IQuestion
-
-    def checkAuthenticated(self, user):
-        """Admins and registry admins can set bug comment visibility."""
-        return (user.in_admin or user.in_registry_experts)
-
-
 class AdminQuestion(AdminByAdminsTeam):
     permission = 'launchpad.Admin'
     usedfor = IQuestion
@@ -2043,6 +2034,17 @@
     usedfor = IQuestionMessage
 
 
+class ModerateQuestionMessage(AuthorizationBase):
+    permission = 'launchpad.Moderate'
+    usedfor = IQuestionMessage
+
+    def checkAuthenticated(self, user):
+        """Admins, Registry, Maintainers, and comment owners can moderate."""
+        return (user.in_admin or user.in_registry_experts
+                or user.inTeam(self.obj.owner)
+                or user.inTeam(self.obj.question.target.owner))
+
+
 class AppendFAQTarget(EditByOwnersOrAdmins):
     permission = 'launchpad.Append'
     usedfor = IFAQTarget