launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/set-question-message-visibility into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
  Curtis Hovey (sinzui): code
Related bugs:
  Bug #45419 in Launchpad itself: "Launchpad needs a way of easily flagging spam and abuse"
  https://bugs.launchpad.net/launchpad/+bug/45419

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/set-question-message-visibility/+merge/56588

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


-- 
https://code.launchpad.net/~jcsackett/launchpad/set-question-message-visibility/+merge/56588
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/set-question-message-visibility into lp:launchpad.

=== 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:19:30 +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:19:30 +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:19:30 +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:19:30 +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:19:30 +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:19:30 +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:19:30 +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:19:30 +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:19:30 +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)