launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/kill-comment-feature-flag into lp:launchpad with lp:~jcsackett/launchpad/hidden-comments-are-userdata as a prerequisite.

Requested reviews:
  Curtis Hovey (sinzui)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/kill-comment-feature-flag/+merge/114532

Summary
=======
We want to get rid of the users_can_hide_own_comments. It was protective b/c
it leaked access, but the access rules have shifted over to being managed
entirely through USERDATA (in the prereq branch).

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
Remove uses of the feature flag.

Tests
=====
bin/test -vvc -t bugcomment -t bug_messages

QA
==
Ensure features work without the flag.

LoC
===
This is negative LoC. It is also part of disclosure.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugcomment.py
  lib/lp/bugs/browser/bugcomment.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bug_messages.py
  lib/lp/services/features/flags.py
  lib/lp/bugs/tests/test_bug_messages_webservice.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/kill-comment-feature-flag/+merge/114532
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py	2012-06-11 00:47:38 +0000
+++ lib/lp/bugs/browser/bugcomment.py	2012-07-11 22:24:25 +0000
@@ -15,10 +15,7 @@
     'group_comments_with_activity',
     ]
 
-from datetime import (
-    datetime,
-    timedelta,
-    )
+from datetime import timedelta
 from itertools import (
     chain,
     groupby,
@@ -27,7 +24,6 @@
 
 from lazr.delegates import delegates
 from lazr.restful.interfaces import IWebServiceClientRequest
-from pytz import utc
 from zope.component import (
     adapts,
     getMultiAdapter,
@@ -44,7 +40,6 @@
 from lp.services.comments.browser.comment import download_body
 from lp.services.comments.browser.messagecomment import MessageComment
 from lp.services.config import config
-from lp.services.features import getFeatureFlag
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.messages.interfaces.message import IMessage
 from lp.services.propertycache import (
@@ -218,10 +213,7 @@
 
         self.synchronized = False
         # We use a feature flag to control users deleting their own comments.
-        user_owns_comment = False
-        flag = 'disclosure.users_hide_own_bug_comments.enabled'
-        if bool(getFeatureFlag(flag)):
-            user_owns_comment = user is not None and user == self.owner
+        user_owns_comment = user is not None and user == self.owner
         self.show_spam_controls = show_spam_controls or user_owns_comment
         self.hide_text = (display == 'hide')
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-11 22:24:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2012-07-11 22:24:25 +0000
@@ -32,7 +32,6 @@
 from lp.registry.interfaces.accesspolicy import (
     IAccessPolicySource,
     )
-from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.testing import verifyObject
 from lp.testing import (
@@ -237,8 +236,6 @@
 
     layer = DatabaseFunctionalLayer
 
-    feature_flag = {'disclosure.users_hide_own_bug_comments.enabled': 'on'}
-
     def getContext(self, comment_owner=None):
         """Required by the mixin."""
         bug = self.factory.makeBug()
@@ -256,10 +253,9 @@
         view = self.getView(context=context, user=user)
         hide_link = find_tag_by_id(view.contents, self.control_text)
         self.assertIs(None, hide_link)
-        with FeatureFixture(self.feature_flag):
-            view = self.getView(context=context, user=user)
-            hide_link = find_tag_by_id(view.contents, self.control_text)
-            self.assertIsNot(None, hide_link)
+        view = self.getView(context=context, user=user)
+        hide_link = find_tag_by_id(view.contents, self.control_text)
+        self.assertIsNot(None, hide_link)
 
     def test_comment_owner_sees_hide_control(self):
         # The comment owner sees the hide control.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-11 22:24:24 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-11 22:24:25 +0000
@@ -2004,10 +2004,7 @@
         bug_message = bug_message_set.getByBugAndMessage(
             self, self.messages[comment_number])
 
-        user_owns_comment = False
-        flag = 'disclosure.users_hide_own_bug_comments.enabled'
-        if bool(getFeatureFlag(flag)):
-            user_owns_comment = bug_message.owner == user
+        user_owns_comment = (bug_message.owner == user)
         if (not self.userCanSetCommentVisibility(user)
             and not user_owns_comment):
             raise Unauthorized(
@@ -2069,9 +2066,7 @@
         roles = IPersonRoles(user)
         if roles.in_admin or roles.in_registry_experts:
             return True
-        # Those who can access user data for the bug have permission too.
-        flag = 'disclosure.users_hide_own_bug_comments.enabled'
-        return bool(getFeatureFlag(flag)) and self.userCanAccessUserData(user)
+        return self.userCanAccessUserData(user)
 
     def userCanAccessUserData(self, user):
         """ Return True if the user has access to USER_DATA data."""

=== modified file 'lib/lp/bugs/tests/test_bug_messages.py'
--- lib/lp/bugs/tests/test_bug_messages.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_bug_messages.py	2012-07-11 22:24:25 +0000
@@ -5,7 +5,6 @@
 
 __metaclass__ = type
 
-from lp.services.features.testing import FeatureFixture
 from lp.testing import (
     login,
     login_celebrity,
@@ -47,15 +46,12 @@
 
     layer = DatabaseFunctionalLayer
 
-    feature_flag = {'disclosure.users_hide_own_bug_comments.enabled': 'on'}
-
     def test_random_user_cannot_toggle_comment_visibility(self):
         # A random user cannot set bug comment visibility.
         person = self.factory.makePerson()
         bug = self.factory.makeBug()
         self.assertFalse(bug.userCanSetCommentVisibility(person))
-        with FeatureFixture(self.feature_flag):
-            self.assertFalse(bug.userCanSetCommentVisibility(person))
+        self.assertFalse(bug.userCanSetCommentVisibility(person))
 
     def test_registry_admin_can_toggle_comment_visibility(self):
         # Members of registry experts can set bug comment visibility.
@@ -75,8 +71,7 @@
         product = self.factory.makeProduct(owner=person)
         bug = self.factory.makeBug(product=product)
         self.assertFalse(bug.userCanSetCommentVisibility(person))
-        with FeatureFixture(self.feature_flag):
-            self.assertTrue(bug.userCanSetCommentVisibility(person))
+        self.assertTrue(bug.userCanSetCommentVisibility(person))
 
     def test_pillar_driver_can_toggle_comment_visibility(self):
         # Pillar driver can set bug comment visibility.
@@ -84,8 +79,7 @@
         product = self.factory.makeProduct(driver=person)
         bug = self.factory.makeBug(product=product)
         self.assertFalse(bug.userCanSetCommentVisibility(person))
-        with FeatureFixture(self.feature_flag):
-            self.assertTrue(bug.userCanSetCommentVisibility(person))
+        self.assertTrue(bug.userCanSetCommentVisibility(person))
 
     def test_pillar_bug_supervisor_can_toggle_comment_visibility(self):
         # Pillar bug supervisor can set bug comment visibility.
@@ -93,8 +87,7 @@
         product = self.factory.makeProduct(bug_supervisor=person)
         bug = self.factory.makeBug(product=product)
         self.assertFalse(bug.userCanSetCommentVisibility(person))
-        with FeatureFixture(self.feature_flag):
-            self.assertTrue(bug.userCanSetCommentVisibility(person))
+        self.assertTrue(bug.userCanSetCommentVisibility(person))
 
     def test_pillar_security_contact_can_toggle_comment_visibility(self):
         # Pillar security contact can set bug comment visibility.
@@ -102,5 +95,4 @@
         product = self.factory.makeProduct(security_contact=person)
         bug = self.factory.makeBug(product=product)
         self.assertFalse(bug.userCanSetCommentVisibility(person))
-        with FeatureFixture(self.feature_flag):
-            self.assertTrue(bug.userCanSetCommentVisibility(person))
+        self.assertTrue(bug.userCanSetCommentVisibility(person))

=== modified file 'lib/lp/bugs/tests/test_bug_messages_webservice.py'
--- lib/lp/bugs/tests/test_bug_messages_webservice.py	2011-12-28 17:03:06 +0000
+++ lib/lp/bugs/tests/test_bug_messages_webservice.py	2012-07-11 22:24:25 +0000
@@ -2,8 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Webservice unit tests related to Launchpad Bug messages."""
-from lp.services.features.testing import FeatureFixture
-
 
 __metaclass__ = type
 
@@ -57,8 +55,6 @@
 
     layer = DatabaseFunctionalLayer
 
-    feature_flag = {'disclosure.users_hide_own_bug_comments.enabled': 'on'}
-
     def setUp(self):
         super(TestSetCommentVisibility, self).setUp()
         self.person_set = getUtility(IPersonSet)
@@ -137,9 +133,8 @@
             HTTPError,
             self._set_visibility,
             bug)
-        with FeatureFixture(self.feature_flag):
-            self._set_visibility(bug)
-            self.assertCommentHidden()
+        self._set_visibility(bug)
+        self.assertCommentHidden()
 
     def test_pillar_owner_can_set_visible(self):
         # Pillar owner can set bug comment visibility.

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-07-11 01:24:42 +0000
+++ lib/lp/services/features/flags.py	2012-07-11 22:24:25 +0000
@@ -209,12 +209,6 @@
      '',
      '',
      ''),
-    ('disclosure.users_hide_own_bug_comments.enabled',
-     'boolean',
-     'Allows users in project roles and comment owners to hide bug comments.',
-     '',
-     '',
-     ''),
     ('disclosure.add-team-person-picker.enabled',
      'boolean',
      'Allows users to add a new team directly from the person picker.',