Merge lp:~stevenk/launchpad/make-person-visibility-mutate into lp:launchpad

[Steve Kowalik]

[r=wallyworld][bug=928440] No longer require launchpad.Commercial to edit IPerson.visibility; use a mutator to allow any user with a current commercial subscription to do so.

Recently I landed a branch that allowed an owner of a team with a commercial subscription to create private teams and change the visibility of existing teams.

However, there was a great, big glaring problem with that. Namely, to edit the visibility attribute on ITeam required the launchpad.Commercial permission.

This branch changes that, visibility is now a property on IPersonPublic, and I've added a mutator method that checks if the calling user has a current commercial subscription if the feature flag is on, and falls back to launchpad.Commercial if it is not.

I have also cleaned up some lint, and some import crazyness in lp.registry.browser.team.

[Ian Booth (wallyworld)]

Looks good. Thanks for making the suggested changes.

With the new error type FORBIDDEN added to the ImmutableVisibilityError, you should add a test like in registry/test_errors.py

    def test_ImmutableVisibilityError_forbidden(self):
        error_view = create_webservice_error_view(
            ImmutableVisibilityError())
        self.assertEqual(FORBIDDEN, error_view.status)

review: Approve

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2012-02-06 14:39:29 +0000
+++ lib/lp/registry/browser/team.py	2012-02-13 23:06:19 +0000
@@ -43,6 +43,7 @@
 import math
 from urllib import unquote
 
+from lazr.restful.interface import copy_field
 from lazr.restful.interfaces import IJSONRequestCache
 from lazr.restful.utils import smartquote
 import pytz
@@ -50,8 +51,11 @@
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.form.browser import TextAreaWidget
 from zope.component import getUtility
-from zope.formlib import form
-from zope.formlib.form import FormFields
+from zope.formlib.form import (
+    Fields,
+    FormField,
+    FormFields,
+    )
 from zope.interface import (
     classImplements,
     implements,
@@ -142,7 +146,6 @@
     )
 from lp.security import ModerateByRegistryExpertsOrAdmins
 from lp.services.config import config
-from lp.services.features import getFeatureFlag
 from lp.services.fields import PublicPersonChoice
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
 from lp.services.privacy.interfaces import IObjectPrivacy
@@ -269,15 +272,12 @@
                     'Private teams must have a Restricted subscription '
                     'policy.')
 
-    def conditionallyOmitVisibility(self):
-        """Remove the visibility field if not authorized."""
-        if check_permission('launchpad.Commercial', self.context):
-            return
-        feature_flag = getFeatureFlag(
-            'disclosure.show_visibility_for_team_add.enabled')
-        if feature_flag and self.user.hasCurrentCommercialSubscription():
-            return
+    def setUpVisibilityField(self):
+        """Set the visibility field to read-write, or remove it."""
         self.form_fields = self.form_fields.omit('visibility')
+        if self.user and self.user.checkAllowVisibility():
+            visibility = copy_field(ITeam['visibility'], readonly=False)
+            self.form_fields += Fields(visibility)
 
 
 class TeamEditView(TeamFormMixin, PersonRenameFormMixin,
@@ -310,7 +310,7 @@
         self.field_names.remove('contactemail')
         self.field_names.remove('teamowner')
         super(TeamEditView, self).setUpFields()
-        self.conditionallyOmitVisibility()
+        self.setUpVisibilityField()
 
     def setUpWidgets(self):
         super(TeamEditView, self).setUpWidgets()
@@ -348,6 +348,10 @@
     @action('Save', name='save')
     def action_save(self, action, data):
         try:
+            visibility = data.get('visibility')
+            if visibility:
+                self.context.transitionVisibility(visibility, self.user)
+                del data['visibility']
             self.updateContextFromData(data)
         except ImmutableVisibilityError, error:
             self.request.response.addErrorNotification(str(error))
@@ -449,7 +453,7 @@
 
         # Replace the default contact_method field by a custom one.
         self.form_fields = (
-            form.FormFields(self.getContactMethodField())
+            FormFields(self.getContactMethodField())
             + self.form_fields.omit('contact_method'))
 
     def getContactMethodField(self):
@@ -482,7 +486,7 @@
             # field.
             del terms[hosted_list_term_index]
 
-        return form.FormField(
+        return FormField(
             Choice(__name__='contact_method',
                    title=_("How do people contact this team's members?"),
                    required=True, vocabulary=SimpleVocabulary(terms)))
@@ -988,9 +992,9 @@
 class TeamAddView(TeamFormMixin, HasRenewalPolicyMixin, LaunchpadFormView):
     """View for adding a new team."""
 
+    schema = ITeamCreation
     page_title = 'Register a new team in Launchpad'
     label = page_title
-    schema = ITeamCreation
 
     custom_widget('teamowner', HiddenUserWidget)
     custom_widget(
@@ -1006,7 +1010,7 @@
         Only Launchpad Admins get to see the visibility field.
         """
         super(TeamAddView, self).setUpFields()
-        self.conditionallyOmitVisibility()
+        self.setUpVisibilityField()
 
     @action('Create Team', name='create')
     def create_action(self, action, data):
@@ -1022,7 +1026,8 @@
             subscriptionpolicy, defaultmembershipperiod, defaultrenewalperiod)
         visibility = data.get('visibility')
         if visibility:
-            team.visibility = visibility
+            team.transitionVisibility(visibility, self.user)
+            del data['visibility']
         email = data.get('contactemail')
         if email is not None:
             generateTokenAndValidationEmail(email, team)

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-02-10 07:17:58 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-02-13 23:06:19 +0000
@@ -519,6 +519,29 @@
                     'visibility',
                     [field.__name__ for field in view.form_fields])
 
+    def test_person_with_cs_can_create_private_team(self):
+        personset = getUtility(IPersonSet)
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        product = self.factory.makeProduct(owner=team)
+        self.factory.makeCommercialSubscription(product)
+        team_name = self.factory.getUniqueString()
+        form = {
+            'field.name': team_name,
+            'field.displayname': 'New Team',
+            'field.subscriptionpolicy': 'RESTRICTED',
+            'field.visibility': 'PRIVATE',
+            'field.actions.create': 'Create',
+            }
+        with person_logged_in(team.teamowner):
+            with FeatureFixture(self.feature_flag):
+                view = create_initialized_view(
+                    personset, name=self.view_name, principal=team.teamowner,
+                    form=form)
+            team = personset.getByName(team_name)
+            self.assertIsNotNone(team)
+            self.assertEqual(PersonVisibility.PRIVATE, team.visibility)
+
     def test_person_with_expired_cs_does_not_see_visibility(self):
         personset = getUtility(IPersonSet)
         team = self.factory.makeTeam(

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-02-08 06:00:46 +0000
+++ lib/lp/registry/configure.zcml	2012-02-13 23:06:19 +0000
@@ -906,8 +906,6 @@
                 interface="lp.registry.interfaces.person.IPersonPublic"/>
             <allow
                 interface="lp.registry.interfaces.person.IHasStanding"/>
-            <allow
-                interface="lp.registry.interfaces.person.IPersonCommAdminWriteRestricted"/>
             <require
                 permission="launchpad.LimitedView"
                 interface="lp.registry.interfaces.person.IPersonLimitedView"/>
@@ -928,14 +926,11 @@
                 set_schema="lp.registry.interfaces.person.IPersonViewRestricted
                             lp.registry.interfaces.person.IPersonPublic
                             lp.registry.interfaces.person.ITeamPublic"
-                set_attributes="displayname icon logo"/>
+                set_attributes="displayname icon logo visibility"/>
             <require
                 permission="launchpad.Moderate"
                 set_attributes="name"/>
             <require
-                permission="launchpad.Commercial"
-                set_schema="lp.registry.interfaces.person.IPersonCommAdminWriteRestricted"/>
-            <require
                 permission="launchpad.Special"
                 interface="lp.registry.interfaces.person.IPersonSpecialRestricted"/>
             <require

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-02-03 11:23:30 +0000
+++ lib/lp/registry/interfaces/person.py	2012-02-13 23:06:19 +0000
@@ -44,6 +44,8 @@
     'validate_subscription_policy',
     ]
 
+import httplib
+
 from lazr.enum import (
     DBEnumeratedType,
     DBItem,
@@ -54,6 +56,7 @@
 from lazr.restful.declarations import (
     call_with,
     collection_default_content,
+    error_status,
     export_as_webservice_collection,
     export_as_webservice_entry,
     export_factory_operation,
@@ -61,6 +64,7 @@
     export_write_operation,
     exported,
     LAZR_WEBSERVICE_EXPORTED,
+    mutator_for,
     operation_for_version,
     operation_parameters,
     operation_returns_collection_of,
@@ -690,10 +694,40 @@
     account_status_comment = Text(
         title=_("Why are you deactivating your account?"), required=False,
         readonly=True)
+    visibility = exported(
+        Choice(title=_("Visibility"),
+               description=_(
+                   "Public visibility is standard.  "
+                   "Private means the team is completely "
+                   "hidden."),
+               required=True, vocabulary=PersonVisibility,
+               default=PersonVisibility.PUBLIC, readonly=True))
 
     def anyone_can_join():
         """Quick check as to whether a team allows anyone to join."""
 
+    def checkAllowVisibility():
+        """Is the user allowed to see the visibility field.
+
+        :param: The user.
+        :return: True if they can, otherwise False.
+        """
+
+    @mutator_for(visibility)
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(visibility=copy_field(visibility))
+    @export_write_operation()
+    @operation_for_version("beta")
+    def transitionVisibility(visibility, user):
+        """Set visibility of IPerson.
+    
+        :param visibility: The PersonVisibility to change to.
+        :param user: The user requesting the change.
+        :raises: `ImmutableVisibilityError` when the visibility can not
+            be changed.
+        :return: None.
+        """
+
 
 class IPersonLimitedView(IHasIcon, IHasLogo):
     """IPerson attributes that require launchpad.LimitedView permission."""
@@ -1801,19 +1835,6 @@
         """
 
 
-class IPersonCommAdminWriteRestricted(Interface):
-    """IPerson attributes that require launchpad.Admin permission to set."""
-
-    visibility = exported(
-        Choice(title=_("Visibility"),
-               description=_(
-                   "Public visibility is standard.  "
-                   "Private means the team is completely "
-                   "hidden."),
-               required=True, vocabulary=PersonVisibility,
-               default=PersonVisibility.PUBLIC))
-
-
 class IPersonSpecialRestricted(Interface):
     """IPerson methods that require launchpad.Special permission to use."""
 
@@ -1873,9 +1894,8 @@
 
 
 class IPerson(IPersonPublic, IPersonLimitedView, IPersonViewRestricted,
-              IPersonEditRestricted, IPersonCommAdminWriteRestricted,
-              IPersonSpecialRestricted, IHasStanding, ISetLocation,
-              IRootContext):
+              IPersonEditRestricted, IPersonSpecialRestricted, IHasStanding,
+              ISetLocation, IRootContext):
     """A Person."""
     export_as_webservice_entry(plural_name='people')
 
@@ -2536,6 +2556,7 @@
     """XMLRPC application root for ISoftwareCenterAgentAPI."""
 
 
+@error_status(httplib.FORBIDDEN)
 class ImmutableVisibilityError(Exception):
     """A change in team membership visibility is not allowed."""
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-02-03 11:23:30 +0000
+++ lib/lp/registry/model/person.py	2012-02-13 23:06:19 +0000
@@ -243,6 +243,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import (
     ensure_unicode,
     shortlist,
@@ -289,6 +290,7 @@
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import ILoginTokenSet
 from lp.services.verification.model.logintoken import LoginToken
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.dbpolicy import MasterDatabasePolicy
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.worlddata.model.language import Language
@@ -3062,6 +3064,25 @@
         """See `IPerson.`"""
         return self.subscriptionpolicy in CLOSED_TEAM_POLICY
 
+    def checkAllowVisibility(self):
+        feature_flag = getFeatureFlag(
+            'disclosure.show_visibility_for_team_add.enabled')
+        if feature_flag:
+            if self.hasCurrentCommercialSubscription():
+                return True
+        else:
+            if check_permission('launchpad.Commercial', self):
+                return True
+        return False
+
+    def transitionVisibility(self, visibility, user):
+        if self.visibility == visibility:
+            return
+        validate_person_visibility(self, 'visibility', visibility)
+        if not user.checkAllowVisibility():
+            raise ImmutableVisibilityError()
+        self.visibility = visibility
+
 
 class PersonSet:
     """The set of persons."""

=== modified file 'lib/lp/registry/stories/webservice/xx-private-team.txt'
--- lib/lp/registry/stories/webservice/xx-private-team.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/registry/stories/webservice/xx-private-team.txt	2012-02-13 23:06:19 +0000
@@ -177,5 +177,5 @@
 
     >>> print modify_team('/~my-new-team-3', {'visibility' : 'Private'},
     ...     'PATCH', user_webservice)
-    HTTP/1.1 401 Unauthorized
+    HTTP/1.1 403 Forbidden
     ...

=== modified file 'lib/lp/registry/tests/test_errors.py'
--- lib/lp/registry/tests/test_errors.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_errors.py	2012-02-13 23:06:19 +0000
@@ -28,6 +28,7 @@
     UserCannotChangeMembershipSilently,
     UserCannotSubscribePerson,
     )
+from lp.registry.interfaces.person import ImmutableVisibilityError
 from lp.testing import TestCase
 from lp.testing.layers import FunctionalLayer
 from lp.testing.views import create_webservice_error_view
@@ -90,3 +91,8 @@
     def test_NameAlreadyTaken_bad_request(self):
         error_view = create_webservice_error_view(NameAlreadyTaken())
         self.assertEqual(CONFLICT, error_view.status)
+
+    def test_ImmutableVisibilityError_forbidden(self):
+        error_view = create_webservice_error_view(
+            ImmutableVisibilityError())
+        self.assertEqual(FORBIDDEN, error_view.status)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-02-03 11:23:30 +0000
+++ lib/lp/registry/tests/test_person.py	2012-02-13 23:06:19 +0000
@@ -538,7 +538,7 @@
         self.assertFalse(person.isAnySecurityContact())
 
     def test_has_current_commercial_subscription(self):
-        # IPerson.hasCurrentCommercialSubscription() checks for one. 
+        # IPerson.hasCurrentCommercialSubscription() checks for one.
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
         product = self.factory.makeProduct(owner=team)
@@ -560,6 +560,12 @@
         person = self.factory.makePerson()
         self.assertFalse(person.hasCurrentCommercialSubscription())
 
+    def test_can_not_set_visibility(self):
+        person = self.factory.makePerson()
+        self.assertRaises(
+            ImmutableVisibilityError, person.transitionVisibility,
+            PersonVisibility.PRIVATE, person)
+
 
 class TestPersonStates(TestCaseWithFactory):