launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13899
[Merge] lp:~sinzui/launchpad/hide-question-comment into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/hide-question-comment into lp:launchpad.
Commit message:
Allow users to hide their question comments.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1072376 in Launchpad itself: "Users cannot hide their question comments"
https://bugs.launchpad.net/launchpad/+bug/1072376
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/hide-question-comment/+merge/132177
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
--
https://code.launchpad.net/~sinzui/launchpad/hide-question-comment/+merge/132177
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/hide-question-comment into lp:launchpad.
=== 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:36:19 +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:36:19 +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:36:19 +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:36:19 +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:36:19 +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:36:19 +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:36:19 +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
Follow ups
