← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~sinzui/launchpad/team-membership-policy-1 into lp:launchpad/db-devel

 

Curtis Hovey has proposed merging lp:~sinzui/launchpad/team-membership-policy-1 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #700724 Subscription policy inherited from parent team member
  https://bugs.launchpad.net/bugs/700724

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/team-membership-policy-1/+merge/48203

Allow open teams to control how members join.

    Launchpad bug: https://bugs.launchpad.net/bugs/700724
    Pre-implementation: floacoste, lifeless
    Test command: ./bin/test -vv \
      -t test_person_vocabularies -t doc/vocabularies \
      -t test_team -t teammembership.txt -t team-join-views \
      -t doc/archive.txt

Ubuntu loco teams where upset by the change to ensure team membership is
not compromised. Many OPEN teams are now Moderated and the work of approving
membership is disruptive. Providing the teams with an API script to
automatically approve members undermines the need manage membership of teams
that control secured assets.

The underling issue is that ~locoteams does not own any assets that need to
be secure. It is MODERATED because it needs to manage *how* users become
members. ~locoteams delegates user membership to its member teams, and it
only manages the direct members. The team *IS* open to anyone, but guards
who is a direct member using the propose-member feature of moderated teams.

There are two kinds of closed teams: RESTRICTED and MODERATED. Their first
concern is control. They limit membership because they control secured assets.

There could be two kinds of open teams: OPEN and DELEGATED. (Maybe change
OPEN to PERMISSIVE). They encourage membership to build communities. The
DELEGATED team reviews who is a direct member to manage the community
hierarchy. OPEN has no structure.

I do not believe this issue is about adding an exception for ~locoteams or
changing security to an ad hoc team declaration. We can validation the
community need by asking if other large communities need a DELEGATED policy
to organise their hierarchy.

--------------------------------------------------------------------

RULES

    This bug:
    * Add the DELEGATED TeamSubscriptionPolicy.
      This is an enum in the DB so the change will happen in staging.
    * Revise the descriptions of all TeamSubscriptionPolicy to clarify
      why they are used.
    * Update the help text on the new team and team edit forms
    * Update the hover help text that is shown next to the subscription
      policy field on team pages
    * Update the propose-member rules to be enforced for MODERATED and
      DELEGATED teams.
    * PS. line 1969 in archive.txt sets up a test that looks insecure.
    * Update code that checks for OPEN to check for both OPEN and DELEGATED.
    * Extra credit: change the comma used in the via column on +participation
      to an arrow.
    * Update help.launchpad.net

    In a follow up bug:
    * Change ~locoteams to DELEGATED.
    * Restore the affected subteams (listed in the this bug) to OPEN.


QA

    * Create a DELEGATED team
    * Verify that users see the join the team link.
    * Have another user create an OPEN team and propose membership
    * Accept the membership
    * Verify the OPEN team can add members.
    * Verify the DELEGATED team is listed on the new member's +participation
      page.

LINT

    lib/canonical/launchpad/icing/style-3-0.css.in
    lib/lp/registry/vocabularies.py
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/team.py
    lib/lp/registry/help/team-subscription-policy.html
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/model/person.py
    lib/lp/registry/templates/person-macros.pt
    lib/lp/registry/tests/test_team.py
    lib/lp/soyuz/doc/archive.txt
    lib/lp/soyuz/model/archive.py


IMPLEMENTATION

Rewrote the description of the enums and added DELEGATED. I changed the order
so that they start with the most open enum and finish with the most closed.
I added OPEN_TEAM_POLICY and CLOSED_TEAM_POLICY so that code that tests
the rules of control have a common definition of enums. I updated
subscriptionpolicy() to reuse the enum docstring.
    lib/lp/registry/interfaces/person.py

Update the code to use the common definition of open and closed teams.
The test_team diff is hard to read, so let me explain. There are no new
test methods. I refactored the test to be three tests of common, open, and
closed behaviours. I then subclasses the open and closed behaviour tests
to verify that each enum behaves as expected.
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/vocabularies.py
    lib/lp/registry/tests/test_team.py
    lib/lp/soyuz/model/archive.py

Updated the proposed rule in IPerson.join() to place users in the PROPOSED
status when a DELEGATED team is passed. I added unittest coverage for
join().
    lib/lp/registry/model/person.py
    lib/lp/registry/tests/test_team.py

Removed an obsolete property to generate the enum description. The template
macro was using the enum itself. However, the description was only shown
as a tooltip for the help icon. Clicking the help icon did nothing. I moved
the tooltip to the actual text, and added a real help file and link for the
policy. I am not happy with the duplication of the enum text in the help file.
I reported bug 711358 about how we might generate help for enums.
    lib/lp/registry/browser/person.py
    lib/lp/registry/templates/person-macros.pt
    lib/lp/registry/help/team-subscription-policy.html

Fixed a common bug in form where the help text exceeds the width of form
instructions. The forms now show the enum descriptions.
    lib/canonical/launchpad/icing/style-3-0.css.in
    lib/lp/registry/browser/team.py

Fixed ambiguous documentation. The docs used an OPEN team when working with
the archive because the join() method would require a second step to approve
the member. I updated the test to use MODERATED and used the addMember()
method instead.
    lib/lp/soyuz/doc/archive.txt
-- 
https://code.launchpad.net/~sinzui/launchpad/team-membership-policy-1/+merge/48203
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/team-membership-policy-1 into lp:launchpad/db-devel.
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2011-01-12 00:00:56 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2011-02-01 17:42:58 +0000
@@ -813,6 +813,7 @@
     margin-left: 2.6em;
     }
 .formHelp {
+    max-width: 45em;
     margin: 0.2em 0 0.5em 0.2em;
     color: #777;
     }

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2011-01-04 23:32:39 +0000
+++ lib/lp/registry/browser/person.py	2011-02-01 17:42:58 +0000
@@ -3009,27 +3009,6 @@
             profile_url.host = config.launchpad.non_restricted_hostname
         return str(profile_url)
 
-    @property
-    def subscription_policy_description(self):
-        """Return the description of this team's subscription policy."""
-        assert self.team.isTeam(), (
-            'This method can only be called when the context is a team.')
-        if self.team.subscriptionpolicy == TeamSubscriptionPolicy.RESTRICTED:
-            description = _(
-                "This is a restricted team; new members can only be added "
-                "by one of the team's administrators.")
-        elif self.team.subscriptionpolicy == TeamSubscriptionPolicy.MODERATED:
-            description = _(
-                "This is a moderated team; all subscriptions are subjected "
-                "to approval by one of the team's administrators.")
-        elif self.team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
-            description = _(
-                "This is an open team; any user can join and no approval "
-                "is required.")
-        else:
-            raise AssertionError('Unknown subscription policy.')
-        return description
-
     def getURLToAssignedBugsInProgress(self):
         """Return an URL to a page which lists all bugs assigned to this
         person that are In Progress.

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2010-12-17 04:26:11 +0000
+++ lib/lp/registry/browser/team.py	2011-02-01 17:42:58 +0000
@@ -60,6 +60,7 @@
 from canonical.widgets import (
     HiddenUserWidget,
     LaunchpadRadioWidget,
+    LaunchpadRadioWidgetWithDescription,
     )
 from lp.app.browser.tales import PersonFormatterAPI
 from lp.app.browser.launchpadform import (
@@ -211,7 +212,8 @@
     custom_widget(
         'renewal_policy', LaunchpadRadioWidget, orientation='vertical')
     custom_widget(
-        'subscriptionpolicy', LaunchpadRadioWidget, orientation='vertical')
+        'subscriptionpolicy', LaunchpadRadioWidgetWithDescription,
+        orientation='vertical')
     custom_widget('teamdescription', TextAreaWidget, height=10, width=30)
 
     def setUpFields(self):
@@ -891,7 +893,8 @@
     custom_widget(
         'renewal_policy', LaunchpadRadioWidget, orientation='vertical')
     custom_widget(
-        'subscriptionpolicy', LaunchpadRadioWidget, orientation='vertical')
+        'subscriptionpolicy', LaunchpadRadioWidgetWithDescription,
+        orientation='vertical')
     custom_widget('teamdescription', TextAreaWidget, height=10, width=30)
 
     def setUpFields(self):

=== added file 'lib/lp/registry/help/team-subscription-policy.html'
--- lib/lp/registry/help/team-subscription-policy.html	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/help/team-subscription-policy.html	2011-02-01 17:42:58 +0000
@@ -0,0 +1,56 @@
+<html>
+  <head>
+    <title>Team subscription policy</title>
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssreset/reset.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssfonts/fonts.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssbase/base.css" />
+  </head>
+  <body>
+    <h1>Team subscription policy</h1>
+
+    <p>
+      There are four kinds of polcy that contol who and how
+      a user or team can become a member.
+    </p>
+
+    <dl>
+      <dt>Open</dt>
+      <dd>
+        Membership is open, no approval required, and subteams can be open or
+        closed. Any user can be a member of the team and no approval is
+        required. Subteams can be Open, Delegated, Moderated, or Restricted.
+        Open is a good choice for encouraging a community of contributors.
+      </dd>
+
+      <dt>Delegated</dt>
+      <dd>
+        Membership is open, requires approval, and subteams can be open or
+        closed. Any user can be a member of the team via a subteam, but team
+        administrators approve direct memberships. Subteams can be Open,
+        Delegated, Moderated, or Restricted. Delegated is a good choice for
+        managing a large community of contributors.
+      </dd>
+
+      <dt>Moderated</dt>
+      <dd>
+        Membership is closed, requires approval, and subteams must be closed.
+        Any user can propose a new member, but team administrators approve
+        membership. Subteams must be Moderated or Restricted. Moderated is a
+        good choice for teams that manage things that need to be secure, like
+        projects, branches, or PPAs, but want to encourage users to help.
+      </dd>
+
+      <dt>Restricted</dt>
+      <dd>
+        Membership is closed, requires approval, and subteams must be closed.
+        Only the team's administrators can invite a user to be a member.
+        Subteams must be Moderated or Restricted. Restricted is a good choice
+        for teams that manage things that need to be secure, like projects,
+        branches, or PPAs.
+      </dd>
+    </dl>
+  </body>
+</html>

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-01-10 14:55:31 +0000
+++ lib/lp/registry/interfaces/person.py	2011-02-01 17:42:58 +0000
@@ -8,6 +8,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'CLOSED_TEAM_POLICY',
     'IAdminPeopleMergeSchema',
     'IAdminTeamMergeSchema',
     'IHasStanding',
@@ -27,6 +28,7 @@
     'ImmutableVisibilityError',
     'InvalidName',
     'NoSuchPerson',
+    'OPEN_TEAM_POLICY',
     'PersonCreationRationale',
     'PersonVisibility',
     'PersonalStanding',
@@ -392,31 +394,59 @@
 class TeamSubscriptionPolicy(DBEnumeratedType):
     """Team Subscription Policies
 
-    The policies that apply to a team and specify how new subscriptions must
-    be handled. More information can be found in the TeamMembershipPolicies
-    spec.
+    The policies that describe who can be a member and how new memberships
+    are handled. The choice of policy reflects the need to build a community
+    verses the need to control Launchpad assets.
     """
 
+    OPEN = DBItem(2, """
+        Open Team
+
+        Membership is open, no approval required, and subteams can be open or
+        closed. Any user can be a member of the team and no approval is
+        required. Subteams can be Open, Delegated, Moderated, or Restricted.
+        Open is a good choice for encouraging a community of contributors.
+        """)
+
+    DELEGATED = DBItem(4, """
+        Delegated Team
+
+        Membership is open, requires approval, and subteams can be open or
+        closed. Any user can be a member of the team via a subteam, but team
+        administrators approve direct memberships. Subteams can be Open,
+        Delegated, Moderated, or Restricted. Delegated is a good choice for
+        managing a large community of contributors.
+        """)
+
     MODERATED = DBItem(1, """
         Moderated Team
 
-        All subscriptions for this team are subject to approval by one of
-        the team's administrators.
-        """)
-
-    OPEN = DBItem(2, """
-        Open Team
-
-        Any user can join and no approval is required.
+        Membership is closed, requires approval, and subteams must be closed.
+        Any user can propose a new member, but team administrators approve
+        membership. Subteams must be Moderated or Restricted. Moderated is a
+        good choice for teams that manage things that need to be secure, like
+        projects, branches, or PPAs, but want to encourage users to help.
         """)
 
     RESTRICTED = DBItem(3, """
         Restricted Team
 
-        New members can only be added by one of the team's administrators.
+        Membership is closed, requires approval, and subteams must be closed.
+        Only the team's administrators can invite a user to be a member.
+        Subteams must be Moderated or Restricted. Restricted is a good choice
+        for teams that manage things that need to be secure, like projects,
+        branches, or PPAs.
         """)
 
 
+OPEN_TEAM_POLICY = (
+    TeamSubscriptionPolicy.OPEN, TeamSubscriptionPolicy.DELEGATED)
+
+
+CLOSED_TEAM_POLICY = (
+    TeamSubscriptionPolicy.RESTRICTED, TeamSubscriptionPolicy.MODERATED)
+
+
 class PersonVisibility(DBEnumeratedType):
     """The visibility level of person or team objects.
 
@@ -503,10 +533,10 @@
     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:
+    elif policy in OPEN_TEAM_POLICY:
         # The team can be open if its super teams are open.
         for team in team.super_teams:
-            if team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN:
+            if team.subscriptionpolicy in CLOSED_TEAM_POLICY:
                 raise TeamSubscriptionPolicyError(
                     "The team subscription policy cannot be %s because one "
                     "or more if its super teams are not open." % policy)
@@ -516,11 +546,11 @@
                 raise TeamSubscriptionPolicyError(
                     "The team subscription policy cannot be %s because it "
                     "has one or more active PPAs." % policy)
-    elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
+    elif team.subscriptionpolicy in OPEN_TEAM_POLICY:
         # The team can become MODERATED or RESTRICTED if its member teams
         # are not OPEN.
         for member in team.activemembers:
-            if member.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
+            if member.subscriptionpolicy in OPEN_TEAM_POLICY:
                 raise TeamSubscriptionPolicyError(
                     "The team subscription policy cannot be %s because one "
                     "or more if its member teams are Open." % policy)
@@ -1770,10 +1800,7 @@
                vocabulary=TeamSubscriptionPolicy,
                default=TeamSubscriptionPolicy.MODERATED, required=True,
                description=_(
-                   "'Moderated' means all subscriptions must be approved. "
-                   "'Open' means any user can join without approval. "
-                   "'Restricted' means new members can be added only by a "
-                   "team administrator.")),
+                TeamSubscriptionPolicy.__doc__.split('\n\n')[1])),
         exported_as='subscription_policy')
 
     renewal_policy = exported(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-01-29 12:20:53 +0000
+++ lib/lp/registry/model/person.py	2011-02-01 17:42:58 +0000
@@ -1272,7 +1272,8 @@
 
         if team.subscriptionpolicy == TeamSubscriptionPolicy.RESTRICTED:
             raise JoinNotAllowed("This is a restricted team")
-        elif team.subscriptionpolicy == TeamSubscriptionPolicy.MODERATED:
+        elif (team.subscriptionpolicy == TeamSubscriptionPolicy.MODERATED
+            or team.subscriptionpolicy == TeamSubscriptionPolicy.DELEGATED):
             status = proposed
         elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
             status = approved

=== modified file 'lib/lp/registry/templates/person-macros.pt'
--- lib/lp/registry/templates/person-macros.pt	2010-05-18 07:44:47 +0000
+++ lib/lp/registry/templates/person-macros.pt	2011-02-01 17:42:58 +0000
@@ -61,10 +61,10 @@
       </dl>
       <dl id="subscription-policy">
         <dt>Subscription policy:</dt>
-        <dd>
+        <dd
+          tal:attributes="title context/subscriptionpolicy/description">
           <span tal:replace="context/subscriptionpolicy/title" />
-          <img src="/@@/maybe"
-            tal:attributes="title context/subscriptionpolicy/description" />
+          <a class="sprite maybe" href="/+help/team-subscription-policy.html" target="help">&nbsp;</a>
         </dd>
       </dl>
 

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-12-13 18:08:19 +0000
+++ lib/lp/registry/tests/test_team.py	2011-02-01 17:42:58 +0000
@@ -18,7 +18,10 @@
     FunctionalLayer,
     )
 from lp.registry.enum import PersonTransferJobType
-from lp.registry.errors import TeamSubscriptionPolicyError
+from lp.registry.errors import (
+    JoinNotAllowed,
+    TeamSubscriptionPolicyError,
+    )
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import (
     IPersonSet,
@@ -220,23 +223,31 @@
         self.assertEqual('a message', error.doc())
 
 
-class TestTeamSubscriptionPolicyChoice(TestCaseWithFactory):
-    """Test `TeamSubsciptionPolicyChoice` constraints."""
+class TeamSubscriptionPolicyBase(TestCaseWithFactory):
+    """`TeamSubsciptionPolicyChoice` base test class."""
 
     layer = DatabaseFunctionalLayer
+    POLICY = None
 
-    def setUpTeams(self, policy, other_policy=None):
+    def setUpTeams(self, other_policy=None):
         if other_policy is None:
-            other_policy = policy
-        self.team = self.factory.makeTeam(subscription_policy=policy)
+            other_policy = self.POLICY
+        self.team = self.factory.makeTeam(subscription_policy=self.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)
 
+
+class TestTeamSubscriptionPolicyChoiceCommon(TeamSubscriptionPolicyBase):
+    """Test `TeamSubsciptionPolicyChoice` constraints."""
+
+    # Any policy can be used in set of tests.
+    POLICY = TeamSubscriptionPolicy.MODERATED
+
     def test___getTeam_with_team(self):
         # _getTeam returns the context team for team updates.
-        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.setUpTeams()
         self.assertEqual(self.team, self.field._getTeam())
 
     def test___getTeam_with_person_set(self):
@@ -245,11 +256,17 @@
         field = ITeamPublic['subscriptionpolicy'].bind(person_set)
         self.assertEqual(None, field._getTeam())
 
+
+class TestTeamSubscriptionPolicyChoiceModerated(TeamSubscriptionPolicyBase):
+    """Test `TeamSubsciptionPolicyChoice` Moderated constraints."""
+
+    POLICY = TeamSubscriptionPolicy.MODERATED
+
     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.setUpTeams()
         self.other_team.addMember(self.team, self.team.teamowner)
         self.assertFalse(
             self.field.constraint(TeamSubscriptionPolicy.OPEN))
@@ -259,29 +276,16 @@
 
     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.setUpTeams(other_policy=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_closed_team_can_change_to_another_closed_policy(self):
         # A closed team can change between the two closed polcies.
-        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.setUpTeams()
         self.team.addMember(self.other_team, self.team.teamowner)
         super_team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED,
@@ -292,20 +296,10 @@
         self.assertEqual(
             None, self.field.validate(TeamSubscriptionPolicy.RESTRICTED))
 
-    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.setUpTeams()
         self.team.createPPA()
         self.assertFalse(
             self.field.constraint(TeamSubscriptionPolicy.OPEN))
@@ -315,7 +309,7 @@
 
     def test_closed_team_without_active_ppas_can_become_open(self):
         # The team can become if it has deleted PPAs.
-        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.setUpTeams(other_policy=TeamSubscriptionPolicy.MODERATED)
         ppa = self.team.createPPA()
         ppa.delete(self.team.teamowner)
         removeSecurityProxy(ppa).status = ArchiveStatus.DELETED
@@ -325,6 +319,47 @@
             None, self.field.validate(TeamSubscriptionPolicy.OPEN))
 
 
+class TestTeamSubscriptionPolicyChoiceRestrcted(
+                                   TestTeamSubscriptionPolicyChoiceModerated):
+    """Test `TeamSubsciptionPolicyChoice` Restricted constraints."""
+
+    POLICY = TeamSubscriptionPolicy.RESTRICTED
+
+
+class TestTeamSubscriptionPolicyChoiceOpen(TeamSubscriptionPolicyBase):
+    """Test `TeamSubsciptionPolicyChoice` Open constraints."""
+
+    POLICY = 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()
+        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(other_policy=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))
+
+
+class TestTeamSubscriptionPolicyChoiceDelegated(
+                                        TestTeamSubscriptionPolicyChoiceOpen):
+    """Test `TeamSubsciptionPolicyChoice` Delegated constraints."""
+
+    POLICY = TeamSubscriptionPolicy.DELEGATED
+
+
 class TestVisibilityConsistencyWarning(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -346,6 +381,52 @@
             self.team.visibilityConsistencyWarning(PersonVisibility.PRIVATE))
 
 
+class TestPersonJoinTeam(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_join_restricted_team_error(self):
+        # Calling join with a Restricted team raises an error.
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
+        user = self.factory.makePerson()
+        login_person(user)
+        self.assertRaises(JoinNotAllowed, user.join, team, user)
+
+    def test_join_moderated_team_proposed(self):
+        # Joining a Moderated team creates a Proposed TeamMembership.
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        user = self.factory.makePerson()
+        login_person(user)
+        user.join(team, user)
+        users = list(team.proposedmembers)
+        self.assertEqual(1, len(users))
+        self.assertEqual(user, users[0])
+
+    def test_join_delegated_team_proposed(self):
+        # Joining a Delegated team creates a Proposed TeamMembership.
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.DELEGATED)
+        user = self.factory.makePerson()
+        login_person(user)
+        user.join(team, user)
+        users = list(team.proposedmembers)
+        self.assertEqual(1, len(users))
+        self.assertEqual(user, users[0])
+
+    def test_join_open_team_appoved(self):
+        # Joining an Open team creates an Approved TeamMembership.
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        user = self.factory.makePerson()
+        login_person(user)
+        user.join(team, user)
+        members = list(team.approvedmembers)
+        self.assertEqual(1, len(members))
+        self.assertEqual(user, members[0])
+
+
 class TestMembershipManagement(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-12-22 02:48:42 +0000
+++ lib/lp/registry/vocabularies.py	2011-02-01 17:42:58 +0000
@@ -140,11 +140,11 @@
     IProjectGroupMilestone,
     )
 from lp.registry.interfaces.person import (
+    CLOSED_TEAM_POLICY,
     IPerson,
     IPersonSet,
     ITeam,
     PersonVisibility,
-    TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.pillar import IPillarName
 from lp.registry.interfaces.product import (
@@ -737,7 +737,7 @@
 
     @property
     def is_closed_team(self):
-        return self.team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN
+        return self.team.subscriptionpolicy in CLOSED_TEAM_POLICY
 
     @property
     def step_title(self):
@@ -781,7 +781,7 @@
         if self.is_closed_team:
             clause = And(
                 clause,
-                Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
+                Person.subscriptionpolicy.is_in(CLOSED_TEAM_POLICY))
         return clause
 
 
@@ -819,7 +819,7 @@
         if self.is_closed_team:
             clause = And(
                 clause,
-                Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
+                Person.subscriptionpolicy.is_in(CLOSED_TEAM_POLICY))
         return clause
 
 

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2011-01-03 22:15:46 +0000
+++ lib/lp/soyuz/doc/archive.txt	2011-02-01 17:42:58 +0000
@@ -1971,7 +1971,7 @@
     >>> from lp.registry.interfaces.person import (
     ...     TeamSubscriptionPolicy)
     >>> team = getUtility(IPersonSet).newTeam(mark, 't1', 't1',
-    ...     subscriptionpolicy=TeamSubscriptionPolicy.OPEN)
+    ...     subscriptionpolicy=TeamSubscriptionPolicy.MODERATED)
     >>> copy = factory.makeCopyArchiveLocation(distribution=ubuntu,
     ...                                        name="team-archive",
     ...                                        owner=team)
@@ -2070,7 +2070,7 @@
 And if the archive is owned by a team, then anyone in the team will also
 be able to view the private team archive:
 
-    >>> cprov.join(team)
+    >>> ignore = team.addMember(cprov, team.teamowner)
     >>> ubuntu_copy_archives = archive_set.getArchivesForDistribution(
     ...     ubuntu, purposes=[ArchivePurpose.COPY], user=cprov)
     >>> print_archive_names(ubuntu_copy_archives)

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-01-20 18:27:00 +0000
+++ lib/lp/soyuz/model/archive.py	2011-02-01 17:42:58 +0000
@@ -80,8 +80,8 @@
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.person import (
+    OPEN_TEAM_POLICY,
     PersonVisibility,
-    TeamSubscriptionPolicy,
     validate_person,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -1704,7 +1704,7 @@
     def validatePPA(self, person, proposed_name):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         if person.isTeam() and (
-            person.subscriptionpolicy == TeamSubscriptionPolicy.OPEN):
+            person.subscriptionpolicy in OPEN_TEAM_POLICY):
             return "Open teams cannot have PPAs."
         if proposed_name is not None and proposed_name == ubuntu.name:
             return (


Follow ups