launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02130
[Merge] lp:~sinzui/launchpad/closed-teams-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/closed-teams-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#654476 Open teams can have PPAs
https://bugs.launchpad.net/bugs/654476
#662844 Do not permit open teams to join restricted or moderated teams
https://bugs.launchpad.net/bugs/662844
Do not permit teams to be compromised by changes to the subscription policy.
Launchpad bug:
https://bugs.launchpad.net/bugs/662844
https://bugs.launchpad.net/bugs/654476
Pre-implementation: jcsacket, gary, benji, flacoste, bac, bigjools
Test command: ./bin/test -vv -t TestTeamSubscriptionPolicy
A restricted or moderated team is effectively an open team if it has an open
team as a member.
The branch that updated the picker fixed the first issue in this bug -- closed
teams cannot add an open team as a member or make an open team team the owner.
The remaining issue is to guard changes to the subscription policy:
* A closed team cannot become open if it is a member of another closed team
* An open team cannot become closed if it has an open team as a member
The guard only needs to check the immediate super teams or sub teams, since
the deeper teams in the hierarchy must also conform to these rules.
ADDENDUM
Open teams cannot have PPAs, only vetted members may upload to the archive.
--------------------------------------------------------------------
RULES
* Add TeamSubscriptionPolicyError when a subscription policy change is
not permitted
* Add a validator to check that changes to a team's subscription policies
are enforced.
1. Closed teams cannot become open if they are members of closed team.
Rule 1 means a closed team cannot become open if it is a member of
any team
2. Open teams cannot become closed if it has any direct or indirect
open teams as members.
* The validator can raise TeamSubscriptionPolicyError when the state is
invalid. it is also: webservice_error(httplib.FORBIDDEN)
* Use the validator when subscriptionpolicy is changed.
* A field validator is needed to ensure api changes are sane
* Add a guard to prevent closed teams with PPAs from becoming open.
QA
* Try to change ~launchpad to open (rule 1)
* Try to change an open team with open teams to closed (rule 2)
* Try to change a team with a PPA to an open team.
LINT
lib/lp/registry/errors.py
lib/lp/registry/interfaces/person.py
lib/lp/registry/tests/test_team.py
IMPLEMENTATION
Added TeamSubscriptionPolicyError that will show the specific error message
in the UI. Created TeamSubsciptionPolicyChoice that enforces both the
vocabulary and the constraints imposed by team memberships and ppas. The
contraint work is actually done by a helper function called
team_subscription_policy_can_transition.
lib/lp/registry/errors.py
lib/lp/registry/interfaces/person.py
lib/lp/registry/tests/test_team.py
--
https://code.launchpad.net/~sinzui/launchpad/closed-teams-1/+merge/43375
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/closed-teams-1 into lp:launchpad.
=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py 2010-11-11 11:55:53 +0000
+++ lib/lp/registry/errors.py 2010-12-10 17:42:17 +0000
@@ -18,6 +18,7 @@
'PPACreationError',
'PrivatePersonLinkageError',
'TeamMembershipTransitionError',
+ 'TeamSubscriptionPolicyError',
'UserCannotChangeMembershipSilently',
'UserCannotSubscribePerson',
]
@@ -25,6 +26,7 @@
import httplib
from lazr.restful.declarations import webservice_error
+from zope.schema.interfaces import ConstraintNotSatisfied
from zope.security.interfaces import Unauthorized
from lp.app.errors import NameLookupFailed
@@ -132,6 +134,30 @@
webservice_error(httplib.BAD_REQUEST)
+class TeamSubscriptionPolicyError(ConstraintNotSatisfied):
+ """The team cannot have the specified TeamSubscriptionPolicy.
+
+ The error can be raised because a super team or member team prevents
+ the this team from setting a specific policy. The error can also be
+ raised if the team has an active PPA.
+ """
+ webservice_error(httplib.BAD_REQUEST)
+
+ _default_message = "Team Subscription Policy Error"
+
+ def __init__(self, message=None):
+ if message is None:
+ message = self._default_message
+ self.message = message
+
+ def doc(self):
+ """See `Invalid`."""
+ return self.message
+
+ def __str__(self):
+ return self.message
+
+
class JoinNotAllowed(Exception):
"""User is not allowed to join a given team."""
webservice_error(httplib.BAD_REQUEST)
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-12-10 07:52:21 +0000
+++ lib/lp/registry/interfaces/person.py 2010-12-10 17:42:17 +0000
@@ -114,7 +114,10 @@
IHasRequestedReviews,
)
from lp.code.interfaces.hasrecipes import IHasRecipes
-from lp.registry.errors import PrivatePersonLinkageError
+from lp.registry.errors import (
+ PrivatePersonLinkageError,
+ TeamSubscriptionPolicyError,
+ )
from lp.registry.interfaces.gpg import IGPGKey
from lp.registry.interfaces.irc import IIrcID
from lp.registry.interfaces.jabber import IJabberID
@@ -146,6 +149,7 @@
StrippedTextLine,
)
from lp.services.worlddata.interfaces.language import ILanguage
+from lp.soyuz.enums import ArchiveStatus
from lp.translations.interfaces.hastranslationimports import (
IHasTranslationImports,
)
@@ -483,6 +487,80 @@
super(PersonNameField, self)._validate(input)
+def team_subscription_policy_can_transition(team, policy):
+ """Can the team can change its subscription policy
+
+ Returns True when the policy can change. or raises an error. OPEN teams
+ cannot be members of MODERATED or RESTRICTED teams. OPEN teams
+ cannot have PPAs. Changes from between OPEN and the two closed states
+ can be blocked by team membership and team artifacts.
+
+ :param team: The team to change.
+ :param policy: The TeamSubsciptionPolicy to change to.
+ :raises TeamSubsciptionPolicyError: Raised when a membership constrain
+ or a team artifact prevents the policy from being set.
+ """
+ if team is None or policy == team.subscriptionpolicy:
+ # The team is being initialized or the policy is not changing.
+ return True
+ elif policy == TeamSubscriptionPolicy.OPEN:
+ # The team can be open if its super teams are open.
+ for team in team.super_teams:
+ if team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN:
+ raise TeamSubscriptionPolicyError(
+ "The team subscription policy cannot be %s because one "
+ "or more if its super teams are not open." % policy)
+ # The team can be open if it has PPAs.
+ for ppa in team.ppas:
+ if ppa.status != ArchiveStatus.DELETED:
+ raise TeamSubscriptionPolicyError(
+ "The team subscription policy cannot be %s because it "
+ "has one or more active PPAs." % policy)
+ elif policy != TeamSubscriptionPolicy.OPEN:
+ # The team can become MODERATED or RESTRICTED if its member teams
+ # are not OPEN.
+ for member in team.activemembers:
+ if member.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
+ raise TeamSubscriptionPolicyError(
+ "The team subscription policy cannot be %s because one "
+ "or more if its member teams are Open." % policy)
+ else:
+ # The policy change is between MODERATED and RESTRICTED.
+ pass
+ return True
+
+
+class TeamSubsciptionPolicyChoice(Choice):
+ """A valid team subscription policy."""
+
+ def _getTeam(self):
+ """Return the context if it is a team or None."""
+ if IPerson.providedBy(self.context):
+ return self.context
+ else:
+ return None
+
+ def constraint(self, value):
+ """See `IField`."""
+ team = self._getTeam()
+ policy = value
+ try:
+ return team_subscription_policy_can_transition(team, policy)
+ except TeamSubscriptionPolicyError:
+ return False
+
+ def _validate(self, value):
+ """Ensure the TeamSubsciptionPolicy is valid for state of the team.
+
+ Returns True if the team can change its subscription policy to the
+ `TeamSubscriptionPolicy`, otherwise raise TeamSubscriptionPolicyError.
+ """
+ team = self._getTeam()
+ policy = value
+ team_subscription_policy_can_transition(team, policy)
+ super(TeamSubsciptionPolicyChoice, self)._validate(value)
+
+
class IPersonClaim(Interface):
"""The schema used by IPerson's +claim form."""
@@ -1688,7 +1766,7 @@
exported_as='team_description')
subscriptionpolicy = exported(
- Choice(title=_('Subscription policy'),
+ TeamSubsciptionPolicyChoice(title=_('Subscription policy'),
vocabulary=TeamSubscriptionPolicy,
default=TeamSubscriptionPolicy.MODERATED, required=True,
description=_(
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py 2010-12-01 17:07:49 +0000
+++ lib/lp/registry/tests/test_team.py 2010-12-10 17:42:17 +0000
@@ -8,19 +8,27 @@
import transaction
from zope.component import getUtility
from zope.interface.exceptions import Invalid
+from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.database.emailaddress import EmailAddress
from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
from canonical.launchpad.interfaces.lpstorm import IMasterStore
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+ DatabaseFunctionalLayer,
+ FunctionalLayer,
+ )
from lp.registry.enum import PersonTransferJobType
+from lp.registry.errors import TeamSubscriptionPolicyError
from lp.registry.interfaces.mailinglist import MailingListStatus
from lp.registry.interfaces.person import (
+ IPersonSet,
ITeamPublic,
PersonVisibility,
TeamMembershipRenewalPolicy,
+ TeamSubscriptionPolicy,
)
from lp.registry.model.persontransferjob import PersonTransferJob
+from lp.soyuz.enums import ArchiveStatus
from lp.testing import (
login_celebrity,
login_person,
@@ -191,6 +199,119 @@
ITeamPublic['defaultmembershipperiod'].validate(3650)
+class TestTeamSubscriptionPolicyError(TestCaseWithFactory):
+ """Test `TeamSubscriptionPolicyError` messages."""
+
+ layer = FunctionalLayer
+
+ def test_default_message(self):
+ error = TeamSubscriptionPolicyError()
+ self.assertEqual('Team Subscription Policy Error', error.message)
+
+ def test_str(self):
+ # The string is the error message.
+ error = TeamSubscriptionPolicyError('a message')
+ self.assertEqual('a message', str(error))
+
+ def test_doc(self):
+ # The doc() method returns the message. It is called when rendering
+ # an error in the UI. eg structure error.
+ error = TeamSubscriptionPolicyError('a message')
+ self.assertEqual('a message', error.doc())
+
+
+class TestTeamSubscriptionPolicyChoice(TestCaseWithFactory):
+ """Test `TeamSubsciptionPolicyChoice` constraints."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUpTeams(self, policy, other_policy=None):
+ if other_policy is None:
+ other_policy = policy
+ self.team = self.factory.makeTeam(subscription_policy=policy)
+ self.other_team = self.factory.makeTeam(
+ subscription_policy=other_policy, owner=self.team.teamowner)
+ self.field = ITeamPublic['subscriptionpolicy'].bind(self.team)
+ login_person(self.team.teamowner)
+
+ def test___getTeam_with_team(self):
+ # _getTeam returns the context team for team updates.
+ self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+ self.assertEqual(self.team, self.field._getTeam())
+
+ def test___getTeam_with_person_set(self):
+ # _getTeam returns the context person set for team creation.
+ person_set = getUtility(IPersonSet)
+ field = ITeamPublic['subscriptionpolicy'].bind(person_set)
+ self.assertEqual(None, field._getTeam())
+
+ def test_closed_team_with_closed_super_team_cannot_become_open(self):
+ # The team cannot compromise the membership of the super team
+ # by becoming open. The user must remove his team from the super team
+ # first.
+ self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+ self.other_team.addMember(self.team, self.team.teamowner)
+ self.assertFalse(
+ self.field.constraint(TeamSubscriptionPolicy.OPEN))
+ self.assertRaises(
+ TeamSubscriptionPolicyError, self.field.validate,
+ TeamSubscriptionPolicy.OPEN)
+
+ def test_closed_team_with_open_super_team_can_become_open(self):
+ # The team can become open if its super teams are open.
+ self.setUpTeams(
+ TeamSubscriptionPolicy.MODERATED, TeamSubscriptionPolicy.OPEN)
+ self.other_team.addMember(self.team, self.team.teamowner)
+ self.assertTrue(
+ self.field.constraint(TeamSubscriptionPolicy.OPEN))
+ self.assertEqual(
+ None, self.field.validate(TeamSubscriptionPolicy.OPEN))
+
+ def test_open_team_with_open_sub_team_cannot_become_closed(self):
+ # The team cannot become closed if its membership will be
+ # compromised by an open subteam. The user must remove the subteam
+ # first
+ self.setUpTeams(TeamSubscriptionPolicy.OPEN)
+ self.team.addMember(self.other_team, self.team.teamowner)
+ self.assertFalse(
+ self.field.constraint(TeamSubscriptionPolicy.MODERATED))
+ self.assertRaises(
+ TeamSubscriptionPolicyError, self.field.validate,
+ TeamSubscriptionPolicy.MODERATED)
+
+ def test_open_team_with_closed_sub_team_can_become_closed(self):
+ # The team can become closed.
+ self.setUpTeams(
+ TeamSubscriptionPolicy.OPEN, TeamSubscriptionPolicy.MODERATED)
+ self.team.addMember(self.other_team, self.team.teamowner)
+ self.assertTrue(
+ self.field.constraint(TeamSubscriptionPolicy.MODERATED))
+ self.assertEqual(
+ None, self.field.validate(TeamSubscriptionPolicy.MODERATED))
+
+ def test_closed_team_with_active_ppas_cannot_become_open(self):
+ # The team cannot become open if it has PPA because it compromises the
+ # the control of who can upload.
+ self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+ self.team.createPPA()
+ self.assertFalse(
+ self.field.constraint(TeamSubscriptionPolicy.OPEN))
+ self.assertRaises(
+ TeamSubscriptionPolicyError, self.field.validate,
+ TeamSubscriptionPolicy.OPEN)
+
+ def test_closed_team_without_active_ppas_can_become_open(self):
+ # The team can become if it has deleted PPAs.
+ self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+ ppa = self.team.createPPA()
+ ppa.delete(self.team.teamowner)
+ removeSecurityProxy(ppa).status = ArchiveStatus.DELETED
+ self.assertTrue(
+ self.field.constraint(TeamSubscriptionPolicy.OPEN))
+ self.assertEqual(
+ None, self.field.validate(TeamSubscriptionPolicy.OPEN))
+
+
class TestVisibilityConsistencyWarning(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
