launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06342
[Merge] lp:~stevenk/launchpad/make-person-visibility-mutate into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/make-person-visibility-mutate into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #928440 in Launchpad itself: "When attempting to create a new team, I'm told I am "Not allowed here""
https://bugs.launchpad.net/launchpad/+bug/928440
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/make-person-visibility-mutate/+merge/92715
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.
--
https://code.launchpad.net/~stevenk/launchpad/make-person-visibility-mutate/+merge/92715
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/make-person-visibility-mutate into lp:launchpad.
=== 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 05:42:21 +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,
@@ -311,6 +315,10 @@
self.field_names.remove('teamowner')
super(TeamEditView, self).setUpFields()
self.conditionallyOmitVisibility()
+ if 'visibility' in [field.__name__ for field in self.form_fields]:
+ self.form_fields = self.form_fields.omit('visibility')
+ visibility = copy_field(ITeam['visibility'], readonly=False)
+ self.form_fields += Fields(visibility)
def setUpWidgets(self):
super(TeamEditView, self).setUpWidgets()
@@ -348,6 +356,9 @@
@action('Save', name='save')
def action_save(self, action, data):
try:
+ visibility = data.get('visibility')
+ if visibility:
+ self.context.transitionVisibility(visibility, self.user)
self.updateContextFromData(data)
except ImmutableVisibilityError, error:
self.request.response.addErrorNotification(str(error))
@@ -449,7 +460,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 +493,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)))
@@ -990,7 +1001,6 @@
page_title = 'Register a new team in Launchpad'
label = page_title
- schema = ITeamCreation
custom_widget('teamowner', HiddenUserWidget)
custom_widget(
@@ -1000,6 +1010,9 @@
orientation='vertical')
custom_widget('teamdescription', TextAreaWidget, height=10, width=30)
+ class schema(ITeamCreation):
+ visibility = copy_field(ITeamCreation['visibility'], readonly=False)
+
def setUpFields(self):
"""See `LaunchpadViewForm`.
@@ -1022,7 +1035,7 @@
subscriptionpolicy, defaultmembershipperiod, defaultrenewalperiod)
visibility = data.get('visibility')
if visibility:
- team.visibility = visibility
+ team.transitionVisibility(visibility, self.user)
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 05:42:21 +0000
@@ -519,6 +519,32 @@
'visibility',
[field.__name__ for field in view.form_fields])
+<<<<<<< TREE
+=======
+ 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)
+
+>>>>>>> MERGE-SOURCE
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 05:42:21 +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 05:42:21 +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,33 @@
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."""
+ @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: Raised when the visibility can not
+ be changed.
+ :return: None.
+ """
+
class IPersonLimitedView(IHasIcon, IHasLogo):
"""IPerson attributes that require launchpad.LimitedView permission."""
@@ -1801,19 +1828,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 +1887,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 +2549,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 05:42:21 +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,18 @@
"""See `IPerson.`"""
return self.subscriptionpolicy in CLOSED_TEAM_POLICY
+ def transitionVisibility(self, visibility, user):
+ validate_person_visibility(self, 'visibility', visibility)
+ feature_flag = getFeatureFlag(
+ 'disclosure.show_visibility_for_team_add.enabled')
+ if feature_flag:
+ if not user.hasCurrentCommercialSubscription():
+ raise ImmutableVisibilityError()
+ else:
+ if not check_permission('launchpad.Commercial', self):
+ raise ImmutableVisibilityError()
+ self.visibility = visibility
+
class PersonSet:
"""The set of persons."""
=== 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 05:42:21 +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)
+
class TestPersonStates(TestCaseWithFactory):
