Merge lp:~jcsackett/launchpad/set-question-message-visibility into lp:launchpad

[j.c.sackett]

[r=sinzui][bug=45419] Adds a method via the API to hide question comments.

NB: This is a resubmit to re-land this branch on devel; previously it was landed against db-devel, but that branch was not qa'ed in time for deployment. The branch that this previously depended on was deployed successfully, so this can now be easily landed on devel. See previous MP for discussion and approval for landing on db-devel.

Summary
=======

As part of ongoing Maintenance and spam work, this branch adds a means of hiding comments on questions to silence question spam via the API.

There may be some noise because of the dependent branch including some bits not included in this branch. To get around the noise of those bits, you should look at http://paste.ubuntu.com/583961/ instead of the generated diff.

Implementation
==============

I've added a new method to Question, setCommentVisibility, that's a pretty close copy of the same method on Bug. This method is also exported over the webservice.

It's security is set to launchpad.Moderate, as set in the configure.zcml, with an adapter in the main security.py file.

Tests
=====

bin/test -t answers.*webservice.*Visibililty

QA
==

Hide a comment using the API; it shouldn't be visible on the answers page. Setting visible back to True should show it on the answers page.

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/canonical/launchpad/browser/message.py
  lib/lp/answers/configure.zcml
  lib/lp/answers/interfaces/question.py
  lib/lp/answers/interfaces/webservice.py
  lib/lp/answers/model/question.py
  lib/lp/answers/tests/test_question_webservice.py

[Curtis Hovey (sinzui)]

Hi JC.

I am very excited to see Questions over the API. I have a few trivial requests and a question.

The setCommentVisibility method's docstring does not conform to PEP 257. I expect to see a single line with an optional paragraph of further information.

IQuestion.priority is unused. We do not want this exported.

I do not see a security checker set for IQuestionMessage in security.py. In the past, this prevented anonymous users from getting the object. That is to say question.messages is always an empty list. Is this true when you access the list as anon? The fix to security.py is pretty trivial:

{{{
class ViewQuestionMessage(AnonymousAuthorization):
    """Anyone can view an IQustionMessage."""
    usedfor = IQuestionMessage
}}}

However, I think this will show visible and hidden the comments over the API. I am unsure if this is an issue. You may want to override the checkUnauthenticated() method.

review: Needs Information (code)

[Aaron Bentley (abentley)]

So apparently the provided diff doesn't accurately represent the changes. Please provide an accurate diff, preferably by fully merging your prerequisite branch.

In addition to Curtis' comments, I recommend using WebServiceTestCase as the basis of TestSetCommentVisibility.

review: Needs Fixing

[j.c.sackett (jcsackett)]

> Hi JC.
> The setCommentVisibility method's docstring does not conform to PEP 257. I
> expect to see a single line with an optional paragraph of further information.

Done.

> IQuestion.priority is unused. We do not want this exported.

Also done.

> I do not see a security checker set for IQuestionMessage in security.py. In
> the past, this prevented anonymous users from getting the object. That is to
> say question.messages is always an empty list. Is this true when you access
> the list as anon? The fix to security.py is pretty trivial:
>
> {{{
> class ViewQuestionMessage(AnonymousAuthorization):
> """Anyone can view an IQustionMessage."""
> usedfor = IQuestionMessage
> }}}
>
> However, I think this will show visible and hidden the comments over the API.
> I am unsure if this is an issue. You may want to override the
> checkUnauthenticated() method.

Per your comment in IRC and the bug you showed, I think the correct option is to just not expose messages for now; the comment hiding code still works as messages don't need to be exposes for a method on question to operate on them.

[j.c.sackett (jcsackett)]

> So apparently the provided diff doesn't accurately represent the changes.
> Please provide an accurate diff, preferably by fully merging your prerequisite
> branch.

Diff is now accurate. The pre-req is completely merged in.

> In addition to Curtis' comments, I recommend using WebServiceTestCase as the
> basis of TestSetCommentVisibility.

I looked into that, but WebServiceTestCase can't provide an anonymous connection to the API, which is needed to check that anon connections can't alter comment visibility. It may be worth it to update the TestCase class, but I feel that's out of scope for this branch. As it stands, the current test case does provide an instance of launchpadlib to work with, with a logged in user or anonymous connection as needed.

Thanks for pointing me to the WebServiceTestCase though--that'll be very useful in subsequent work.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-03-23 03:03 PM, j.c.sackett wrote:
> I looked into that, but WebServiceTestCase can't provide an anonymous connection to the API, which is needed to check that anon connections can't alter comment visibility. It may be worth it to update the TestCase class, but I feel that's out of scope for this branch. As it stands, the current test case does provide an instance of launchpadlib to work with, with a logged in user or anonymous connection as needed.

In that case, I recommend using testing.ws_object in place of
hand-crafting a url etc.

Also, launchpadlib_for(.., user=None) appears to be equivalent to
launchpadlib_for(...), so I think your conditional is redundant.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2KRqcACgkQ0F+nu1YWqI3j0wCfWRnNe5FyixB9oY50Elefx+qK
pRcAn28hzJFLbGNtMwh/sKiuWsrEq3dV
=whBr
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

Looking at the diff, I see you are un-exporting messages. Is that deliberate?

Also, I've never seen endInteraction used before. What does it do?

[j.c.sackett (jcsackett)]

> Looking at the diff, I see you are un-exporting messages. Is that deliberate?

Yes, per my comment to Curtis in this MP. There are some security concerns when exposing the various message objects and it's unnecessary to export them in order for the setCommentVisibility method to work.

> Also, I've never seen endInteraction used before. What does it do?

It kills the existing _thread.interaction data so you can start a new one. Without it, calling up new lplib instances in the test can run into an error about establishing _thread.interaction while the previous one is present. This is particularly common with the anonymous login case. I believe Salgado was the one who pointed it out to me for some other, similar test cases a few months ago.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-03-23 03:46 PM, j.c.sackett wrote:
>> Looking at the diff, I see you are un-exporting messages. Is that deliberate?
>
> Yes, per my comment to Curtis in this MP. There are some security concerns when exposing the various message objects and it's unnecessary to export them in order for the setCommentVisibility method to work.
>
>> Also, I've never seen endInteraction used before. What does it do?
>
> It kills the existing _thread.interaction data so you can start a new one. Without it, calling up new lplib instances in the test can run into an error about establishing _thread.interaction while the previous one is present. This is particularly common with the anonymous login case. I believe Salgado was the one who pointed it out to me for some other, similar test cases a few months ago.

It appears to be the underlying implementation of logout. logout is
much more familiar to everyone. Is there a reason you're not using it?

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2KUYcACgkQ0F+nu1YWqI3q9QCfZOnXzryg25OlfKSqLCDaijdK
ilIAnRNr12aZtxTKxglRKdykIqydJbQd
=WNC7
-----END PGP SIGNATURE-----

[j.c.sackett (jcsackett)]

> It appears to be the underlying implementation of logout. logout is
> much more familiar to everyone. Is there a reason you're not using it?

Only because in a similar situation when I was starting on LP I was shown endInteraction. logout works, and I've pushed that up as a change.

[Aaron Bentley (abentley)]

review: Approve

[Curtis Hovey (sinzui)]

This is good to land in devel now that the schema supports it.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/browser/message.py'
--- lib/canonical/launchpad/browser/message.py	2011-03-27 15:30:05 +0000
+++ lib/canonical/launchpad/browser/message.py	2011-04-06 15:38:21 +0000
@@ -10,6 +10,7 @@
 from canonical.launchpad.interfaces.message import IIndexedMessage
 from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
 
+
 class QuestionMessageCanonicalUrlData:
     """Question messages have a canonical_url within the question."""
     implements(ICanonicalUrlData)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-03-30 10:46:24 +0000
+++ lib/canonical/launchpad/security.py	2011-04-06 15:38:21 +0000
@@ -1656,6 +1656,15 @@
     checkUnauthenticated = _checkAccess
 
 
+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

=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml	2011-03-21 19:58:39 +0000
+++ lib/lp/answers/configure.zcml	2011-04-06 15:38:21 +0000
@@ -108,6 +108,10 @@
       set_attributes="assignee"
       />
     <require
+      permission="launchpad.Moderate"
+      attributes="setCommentVisibility"
+      />
+    <require
       permission="launchpad.Admin"
       attributes="setStatus"
       set_attributes="priority whiteboard"

=== modified file 'lib/lp/answers/interfaces/question.py'
--- lib/lp/answers/interfaces/question.py	2011-03-27 15:30:33 +0000
+++ lib/lp/answers/interfaces/question.py	2011-04-06 15:38:21 +0000
@@ -20,6 +20,10 @@
     call_with,
     export_as_webservice_entry,
     exported,
+    export_write_operation,
+    operation_for_version,
+    operation_parameters,
+    REQUEST_USER,
     )
 from lazr.restful.fields import (
     CollectionField,
@@ -475,6 +479,21 @@
             notify along the rationale for doing so.
         """
 
+    @operation_parameters(
+        comment_number=Int(
+            title=_('The number of the comment in the list of messages.'),
+            required=True),
+        visible=Bool(title=_('Show this comment?'), required=True))
+    @call_with(user=REQUEST_USER)
+    @export_write_operation()
+    @operation_for_version('devel')
+    def setCommentVisibility(user, comment_number, visible):
+        """Set the visible attribute on a question message.
+        
+        This is restricted to Launchpad admins and registry members, and will
+        return a HTTP Error 401: Unauthorized error for non-admin callers.
+        """
+
 
 # These schemas are only used by browser/question.py and should really live
 # there. See Bug #66950.

=== modified file 'lib/lp/answers/interfaces/questionmessage.py'
--- lib/lp/answers/interfaces/questionmessage.py	2010-08-20 20:31:18 +0000
+++ lib/lp/answers/interfaces/questionmessage.py	2011-04-06 15:38:21 +0000
@@ -12,6 +12,7 @@
     ]
 
 from zope.schema import (
+    Bool,
     Choice,
     Field,
     )
@@ -47,4 +48,8 @@
         "related the action operated by this message."), required=True,
         readonly=True, default=QuestionStatus.OPEN,
         vocabulary=QuestionStatus)
+    visible = Bool(
+        title=_("Message visibility."),
+        description=_("Whether or not the message is visible."),
+        readonly=True)
 

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2011-01-29 16:52:18 +0000
+++ lib/lp/answers/model/question.py	2011-04-06 15:38:21 +0000
@@ -646,6 +646,11 @@
         """See BugLinkTargetMixin."""
         return QuestionBug(question=self, bug=bug)
 
+    def setCommentVisibility(self, user, comment_number, visible):
+        """See `IQuestion`."""
+        message = self.messages[comment_number].message
+        message.visible = visible
+
 
 class QuestionSet:
     """The set of questions in the Answer Tracker."""

=== modified file 'lib/lp/answers/model/questionmessage.py'
--- lib/lp/answers/model/questionmessage.py	2010-10-03 15:30:06 +0000
+++ lib/lp/answers/model/questionmessage.py	2011-04-06 15:38:21 +0000
@@ -48,3 +48,8 @@
         """See IMessage."""
         # Delegates do not proxy __ methods, because of the name mangling.
         return iter(self.chunks)
+
+    @property
+    def visible(self):
+        """See `IQuestionMessage.`"""
+        return self.message.visible

=== modified file 'lib/lp/answers/templates/question-index.pt'
--- lib/lp/answers/templates/question-index.pt	2011-03-04 15:16:53 +0000
+++ lib/lp/answers/templates/question-index.pt	2011-04-06 15:38:21 +0000
@@ -92,7 +92,8 @@
 
 
       <tal:message repeat="message context/messages">
-        <div tal:replace="structure message/@@+display" />
+      <div tal:condition="message/visible"
+           tal:replace="structure message/@@+display" />
       </tal:message>
 
       <div id="question"

=== modified file 'lib/lp/answers/tests/test_question_webservice.py'
--- lib/lp/answers/tests/test_question_webservice.py	2011-03-24 22:49:57 +0000
+++ lib/lp/answers/tests/test_question_webservice.py	2011-04-06 15:38:21 +0000
@@ -6,13 +6,21 @@
 __metaclass__ = type
 
 from BeautifulSoup import BeautifulSoup
+from lazr.restfulclient.errors import HTTPError
 from simplejson import dumps
+import transaction
+from zope.component import getUtility
 
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import IPersonSet
 from lp.testing import (
     TestCaseWithFactory,
     celebrity_logged_in,
+    launchpadlib_for,
+    logout,
+    person_logged_in,
+    ws_object
     )
 
 
@@ -67,3 +75,82 @@
         self.assertEqual(
             self.findQuestionTitle(response),
             "<p>No, this is a question</p>")
+
+
+class TestSetCommentVisibility(TestCaseWithFactory):
+    """Tests who can successfully set comment visibility."""
+
+    layer = DatabaseFunctionalLayer
+
+    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.question = self.factory.makeQuestion()
+            self.message = self.question.addComment(
+                self.admin, 'Some comment')
+        transaction.commit()
+
+    def _get_question_for_user(self, user=None):
+        """Convenience function to get the api question reference."""
+        # End any open lplib instance.
+        logout()
+        if user is not None:
+            lp = launchpadlib_for("test", user)
+        else:
+            lp = launchpadlib_for("test")
+
+        question_entry = lp.load(
+            '/%s/+question/%d/' % (
+                self.question.target.name, self.question.id))
+        lp = launchpadlib_for("test", user)
+        return ws_object(lp, self.question)
+
+    def _set_visibility(self, question):
+        """Method to set visibility; needed for assertRaises."""
+        question.setCommentVisibility(
+            comment_number=0,
+            visible=False)
+
+    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)
+        self.assertRaises(
+            HTTPError,
+            self._set_visibility,
+            question)
+
+    def test_anon_cannot_set_visible(self):
+        # Anonymous users can't set question comment
+        # visibility.
+        question = self._get_question_for_user()
+        self.assertRaises(
+            HTTPError,
+            self._set_visibility,
+            question)
+
+    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)
+        question = self._get_question_for_user(person)
+        self._set_visibility(question)
+        self.assertFalse(self.message.visible)
+
+    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)
+        question = self._get_question_for_user(person)
+        self._set_visibility(question)
+        self.assertFalse(self.message.visible)