Merge lp:~sinzui/launchpad/closed-teams-0 into lp:launchpad

[Curtis Hovey]

Need a closed team and user vocabulary and picker

    Launchpad bug: https://bugs.launchpad.net/bugs/681035
    Pre-implementation: jcsackett, flacoste, gary_poster
    Test command: ./bin/test -vv \
        -t test_popup -t test_team -t test_person_vocabularies

There are several cases where Launchpad must prevent users from selecting a
team with an open subscription policy because it compromises security or
discloses information. The person picker and forms need a vocabulary that is
composed on closed teams (restricted and moderated), and users. The
picker/vocabulary is needed is needed in:

1. closed team owner role
2. closed team +addmember
3. moderated team +proposemember

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

RULES

    * Update the ValidTeamMemberVocabulary and ValidTeamOwnerVocabulary
      to exclude open teams when the context team is closed.
    * Update IHugeVocabulary, <intermediate classes>, VocabularyPickerWidget
      to use the vocabs step_title so that users understand what can and
      cannot be searched for. This issue was discovered in user testing.
    * Update all IHugeVocabularies to have a step_title
    * Update the add member ajax feature to also use the vocabulary's
      step_title

QA

    Using a moderated or restricted team to test.
    * Verify that open teams to not appear in the person picker to add
      a new member from the team's front page.
    * Verify that open teams to not appear in the person picker to add
      a new member from the teams +addmember page.
    * Verify that open teams to not appear in the person picker to change
      the team owner

LINT

    lib/canonical/launchpad/vocabularies/dbobjects.py
    lib/canonical/launchpad/webapp/vocabulary.py
    lib/canonical/widgets/popup.py
    lib/lp/answers/vocabulary.py
    lib/lp/app/widgets/
    lib/lp/app/widgets/__init__.py
    lib/lp/app/widgets/tests/
    lib/lp/app/widgets/tests/__init__.py
    lib/lp/app/widgets/tests/test_popup.py
    lib/lp/blueprints/vocabularies/specificationdependency.py
    lib/lp/code/vocabularies/branch.py
    lib/lp/registry/vocabularies.py
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_team.py
    lib/lp/registry/javascript/team.js
    lib/lp/registry/templates/team-portlet-membership.pt
    lib/lp/registry/tests/test_person_vocabularies.py

IMPLEMENTATION

Updated ValidTeamMemberVocabulary and ValidTeamOwnerVocabulary to exclude
open teams when the context team is closed.
    lib/lp/registry/vocabularies.py
    lib/lp/registry/tests/test_person_vocabularies.py

Added step_title to the vocabulary. The value is the same that appears in
the UI now.
    lib/canonical/launchpad/webapp/vocabulary.py
    lib/canonical/launchpad/vocabularies/dbobjects.py
    lib/lp/answers/vocabulary.py
    lib/lp/blueprints/vocabularies/specificationdependency.py
    lib/lp/code/vocabularies/branch.py
    lib/lp/registry/vocabularies.py

Updated VocabularyPickerWidget to use the vocab's step_title. There are
no direct tests of this widget. Moving the whole module would make the diff
hard to read so I create a test where the module should be moved.
    lib/canonical/widgets/popup.py
    lib/lp/app/widgets/
    lib/lp/app/widgets/__init__.py
    lib/lp/app/widgets/tests/
    lib/lp/app/widgets/tests/__init__.py
    lib/lp/app/widgets/tests/test_popup.py

Updated the TeamIndexView and team membership portlet to use the step_title
from the vocabulary.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_team.py
    lib/lp/registry/javascript/team.js
    lib/lp/registry/templates/team-portlet-membership.pt

[Gavin Panella (allenap)]

Hi Curtis. Some incredibly minor comments, +1.

[1]

         js = js_template % simplejson.dumps(args)
         # If the YUI widget or javascript is not supported in the browser,
         # it will degrade to being this "Find..." link instead of the
@@ -154,8 +163,7 @@
             css = ''
         return ('<span class="%s">(<a id="%s" href="/people/">'
                 'Find&hellip;</a>)</span>'
- '\n<script>\n%s\n</script>'
- ) % (css, self.show_widget_id, js)
+ '\n<script>\n%s\n</script>') % (css, self.show_widget_id, js)

It's not part of your change, but using simplejson.dumps() can be
hazardous for embedding javascript into <script> tags. See:

  http://code.google.com/p/simplejson/issues/detail?id=66

Instead the following should be used:

  simplejson.dumps(args, cls=simplejson.encoder.JSONEncoderForHTML)

Or:

  simplejson.encoder.JSONEncoderForHTML().encode(obj)

[2]

+ Y.lp.registry.team.setup_add_member_handler(
+ '${step_title}');

The lack of escaping and all that jazz makes me a little uneasy, but
it's not user-supplied so I'm not too worried.

[3]

+ def test_open_team_cannot_be_a_member_or_a_closed_team(self):

s/or/of/

[4]

+ def test_open_team_can_be_a_member_or_an_open_team(self):

s/or/of/

[5]

+ return 'Search for a restricted or moderated team or person'

This could be parsed as:

  Search for a (restricted or moderated) (team or person)

Perhaps the following would be less ambiguous:

  Search for a restricted team, a moderated team, or a person

?

[6]

+ Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)

Maybe it's worth taking a defensive approach here (to someone adding
another TeamSubscriptionPolicy member) instead:

  In(Person.subscriptionpolicy, (
      TeamSubscriptionPolicy.RESTRICTED,
      TeamSubscriptionPolicy.MODERATED))

The same clause appears twice.

review: Approve

[Curtis Hovey (sinzui)]

On Thu, 2010-12-02 at 17:12 +0000, Gavin Panella wrote:
> Review: Approve
> Hi Curtis. Some incredibly minor comments, +1.
>
>
> [1]
>
> js = js_template % simplejson.dumps(args)
> # If the YUI widget or javascript is not supported in the browser,
> # it will degrade to being this "Find..." link instead of the
> @@ -154,8 +163,7 @@
> css = ''
> return ('<span class="%s">(<a id="%s" href="/people/">'
> 'Find&hellip;</a>)</span>'
> - '\n<script>\n%s\n</script>'
> - ) % (css, self.show_widget_id, js)
> + '\n<script>\n%s\n</script>') % (css, self.show_widget_id, js)
>
> It's not part of your change, but using simplejson.dumps() can be
> hazardous for embedding javascript into <script> tags. See:
>
> http://code.google.com/p/simplejson/issues/detail?id=66
>
> Instead the following should be used:
>
> simplejson.dumps(args, cls=simplejson.encoder.JSONEncoderForHTML)
>
> Or:
>
> simplejson.encoder.JSONEncoderForHTML().encode(obj)

Interesting. We cannot do this right now because Lp is using 2.0.9. I
can see it my version of 2.1.1

> [2]
>
> + Y.lp.registry.team.setup_add_member_handler(
> + '${step_title}');
>
> The lack of escaping and all that jazz makes me a little uneasy, but
> it's not user-supplied so I'm not too worried.

This content is in a tal:content rule and is escaped. A single-quote
will break the script. I updated the method to escape quotes and added a
test.

> [3]
>
> + def test_open_team_cannot_be_a_member_or_a_closed_team(self):
>
> s/or/of/

fixed

> [4]
>
> + def test_open_team_can_be_a_member_or_an_open_team(self):
>
> s/or/of/

fixed

> [5]
>
> + return 'Search for a restricted or moderated team or person'
>
> This could be parsed as:
>
> Search for a (restricted or moderated) (team or person)
>
> Perhaps the following would be less ambiguous:
>
> Search for a restricted team, a moderated team, or a person

I accept your proposal.

> [6]
>
> + Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
>
> Maybe it's worth taking a defensive approach here (to someone adding
> another TeamSubscriptionPolicy member) instead:
>
> In(Person.subscriptionpolicy, (
> TeamSubscriptionPolicy.RESTRICTED,
> TeamSubscriptionPolicy.MODERATED))

We have not changed the policies in 5 years. I do not think we will be
adding other policies. If I am wrong, we may need to update the proposed
revision. Since this issue is about OPEN, I think the clause should
remain as is.

--
__Curtis C. Hovey_________
http://launchpad.net/

=== modified file 'lib/canonical/launchpad/vocabularies/dbobjects.py'
--- lib/canonical/launchpad/vocabularies/dbobjects.py	2010-11-09 08:38:23 +0000
+++ lib/canonical/launchpad/vocabularies/dbobjects.py	2010-12-03 00:04:45 +0000
@@ -141,6 +141,7 @@
 class BugTrackerVocabulary(SQLObjectVocabularyBase):
     """All web and email based external bug trackers."""
     displayname = 'Select a bug tracker'
+    step_title = 'Search'
     implements(IHugeVocabulary)
     _table = BugTracker
     _filter = True
@@ -401,6 +402,7 @@
         Person.q.id == Archive.q.ownerID,
         Archive.q.purpose == ArchivePurpose.PPA)
     displayname = 'Select a PPA'
+    step_title = 'Search'
 
     def toTerm(self, archive):
         """See `IVocabulary`."""

=== modified file 'lib/canonical/launchpad/webapp/vocabulary.py'
--- lib/canonical/launchpad/webapp/vocabulary.py	2010-11-09 08:38:23 +0000
+++ lib/canonical/launchpad/webapp/vocabulary.py	2010-12-03 00:04:45 +0000
@@ -70,7 +70,10 @@
     """
 
     displayname = Attribute(
-        'A name for this vocabulary, to be displayed in the popup window.')
+        'A name for this vocabulary, to be displayed in the picker window.')
+
+    step_title = Attribute(
+        'The search step title in the picker window.')
 
     def searchForTerms(query=None):
         """Return a `CountableIterator` of `SimpleTerm`s that match the query.
@@ -365,6 +368,7 @@
     implements(IHugeVocabulary)
     _orderBy = 'name'
     displayname = None
+    step_title = 'Search'
     # The iterator class will be used to wrap the results; its iteration
     # methods should return SimpleTerms, as the reference implementation
     # CountableIterator does.

=== modified file 'lib/canonical/widgets/popup.py'
--- lib/canonical/widgets/popup.py	2010-08-24 10:45:57 +0000
+++ lib/canonical/widgets/popup.py	2010-12-03 00:04:45 +0000
@@ -37,7 +37,7 @@
     style = ''
     cssClass = ''
 
-    step_title = 'Search'
+    step_title = None
     # Defaults to self.vocabulary.displayname.
     header = None
 
@@ -79,19 +79,20 @@
 
     def inputField(self):
         d = {
-            'formToken' : cgi.escape(self.formToken, quote=True),
+            'formToken': cgi.escape(self.formToken, quote=True),
             'name': self.name,
             'displayWidth': self.displayWidth,
             'displayMaxWidth': self.displayMaxWidth,
             'onKeyPress': self.onKeyPress,
             'style': self.style,
-            'cssClass': self.cssClass
-        }
+            'cssClass': self.cssClass,
+            }
         return """<input type="text" value="%(formToken)s" id="%(name)s"
                          name="%(name)s" size="%(displayWidth)s"
                          maxlength="%(displayMaxWidth)s"
                          onKeyPress="%(onKeyPress)s" style="%(style)s"
                          class="%(cssClass)s" />""" % d
+
     @property
     def suffix(self):
         return self.name.replace('.', '-')
@@ -126,23 +127,31 @@
                 % (choice.context, choice.__name__))
         return choice.vocabularyName
 
-    def chooseLink(self):
-        js_file = os.path.join(os.path.dirname(__file__),
-                               'templates/vocabulary-picker.js.template')
-        js_template = open(js_file).read()
-
+    def js_template_args(self):
+        """return a dict of args to configure the picker javascript."""
         if self.header is None:
             header = self.vocabulary.displayname
         else:
             header = self.header
 
-        args = dict(
+        if self.step_title is None:
+            step_title = self.vocabulary.step_title
+        else:
+            step_title = self.step_title
+
+        return dict(
             vocabulary=self.vocabulary_name,
             header=header,
-            step_title=self.step_title,
+            step_title=step_title,
             show_widget_id=self.show_widget_id,
             input_id=self.name,
             extra_no_results_message=self.extra_no_results_message)
+
+    def chooseLink(self):
+        js_file = os.path.join(os.path.dirname(__file__),
+                               'templates/vocabulary-picker.js.template')
+        js_template = open(js_file).read()
+        args = self.js_template_args()
         js = js_template % simplejson.dumps(args)
         # If the YUI widget or javascript is not supported in the browser,
         # it will degrade to being this "Find..." link instead of the
@@ -154,8 +163,7 @@
             css = ''
         return ('<span class="%s">(<a id="%s" href="/people/">'
                 'Find&hellip;</a>)</span>'
-                '\n<script>\n%s\n</script>'
-               ) % (css, self.show_widget_id, js)
+                '\n<script>\n%s\n</script>') % (css, self.show_widget_id, js)
 
     @property
     def nonajax_uri(self):

=== modified file 'lib/lp/answers/vocabulary.py'
--- lib/lp/answers/vocabulary.py	2010-08-20 20:31:18 +0000
+++ lib/lp/answers/vocabulary.py	2010-12-03 00:04:45 +0000
@@ -24,6 +24,7 @@
     implements(IHugeVocabulary)
 
     displayname = 'Select a FAQ'
+    step_title = 'Search'
 
     def __init__(self, context):
         """Create a new vocabulary for the context.
@@ -72,5 +73,3 @@
         """See `IHugeVocabulary`."""
         results = self.context.findSimilarFAQs(query)
         return CountableIterator(results.count(), results, self.toTerm)
-
-

=== added directory 'lib/lp/app/widgets'
=== added file 'lib/lp/app/widgets/__init__.py'
=== added directory 'lib/lp/app/widgets/tests'
=== added file 'lib/lp/app/widgets/tests/__init__.py'
=== added file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2010-12-03 00:04:45 +0000
@@ -0,0 +1,43 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.schema.vocabulary import getVocabularyRegistry
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.widgets.popup import VocabularyPickerWidget
+from lp.registry.interfaces.person import ITeam
+from lp.testing import TestCaseWithFactory
+
+
+class TestVocabularyPickerWidget(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestVocabularyPickerWidget, self).setUp()
+        context = self.factory.makeTeam()
+        field = ITeam['teamowner']
+        self.bound_field = field.bind(context)
+        vocabulary_registry = getVocabularyRegistry()
+        self.vocabulary = vocabulary_registry.get(context, 'ValidTeamOwner')
+        self.request = LaunchpadTestRequest()
+
+    def test_js_template_args(self):
+        picker_widget = VocabularyPickerWidget(
+            self.bound_field, self.vocabulary, self.request)
+        js_template_args = picker_widget.js_template_args()
+        self.assertEqual(
+            'ValidTeamOwner', js_template_args['vocabulary'])
+        self.assertEqual(
+            self.vocabulary.displayname, js_template_args['header'])
+        self.assertEqual(
+            self.vocabulary.step_title, js_template_args['step_title'])
+        self.assertEqual(
+            'show-widget-field-teamowner', js_template_args['show_widget_id'])
+        self.assertEqual(
+            'field.teamowner', js_template_args['input_id'])
+        self.assertEqual(
+            None, js_template_args['extra_no_results_message'])

=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
--- lib/lp/blueprints/vocabularies/specificationdependency.py	2010-11-01 03:36:04 +0000
+++ lib/lp/blueprints/vocabularies/specificationdependency.py	2010-12-03 00:04:45 +0000
@@ -58,6 +58,7 @@
     _table = Specification
     _orderBy = 'name'
     displayname = 'Select a blueprint'
+    step_title = 'Search'
 
     def _is_valid_candidate(self, spec, check_target=False):
         """Is `spec` a valid candidate spec for self.context?

=== modified file 'lib/lp/code/vocabularies/branch.py'
--- lib/lp/code/vocabularies/branch.py	2010-09-20 01:06:36 +0000
+++ lib/lp/code/vocabularies/branch.py	2010-12-03 00:04:45 +0000
@@ -44,6 +44,7 @@
     _table = Branch
     _orderBy = ['name', 'id']
     displayname = 'Select a branch'
+    step_title = 'Search'
 
     def toTerm(self, branch):
         """The display should include the URL if there is one."""

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-11-30 20:34:31 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-12-03 00:04:45 +0000
@@ -1127,7 +1127,7 @@
             template="../templates/team-portlet-map.pt"/>
         <browser:page
             for="lp.registry.interfaces.person.ITeam"
-            class="lp.registry.browser.person.PersonIndexView"
+            class="lp.registry.browser.person.TeamIndexView"
             permission="zope.Public"
             name="+portlet-membership"
             template="../templates/team-portlet-membership.pt"/>

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-11-30 12:53:20 +0000
+++ lib/lp/registry/browser/person.py	2010-12-03 00:04:45 +0000
@@ -3567,6 +3567,13 @@
             return 'portlet'
         return 'portlet private'
 
+    @property
+    def add_member_step_title(self):
+        """A string for setup_add_member_handler with escaped quotes."""
+        vocabulary_registry = getVocabularyRegistry()
+        vocabulary = vocabulary_registry.get(self.context, 'ValidTeamMember')
+        return vocabulary.step_title.replace("'", "\\'").replace('"', '\\"')
+
 
 class PersonCodeOfConductEditView(LaunchpadView):
     """View for the ~person/+codesofconduct pages."""

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2010-11-23 04:42:24 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2010-12-03 00:04:45 +0000
@@ -135,3 +135,17 @@
         self.assertEqual(
             "You can't add a team that doesn't have any active members.",
             view.errors[0])
+
+
+class TestTeamIndexView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTeamIndexView, self).setUp()
+        self.team = self.factory.makeTeam(name='test-team')
+        login_person(self.team.teamowner)
+
+    def test_add_member_step_title(self):
+        view = create_initialized_view(self.team, '+index')
+        self.assertEqual('Search', view.add_member_step_title)

=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt	2010-11-05 14:35:36 +0000
+++ lib/lp/registry/doc/vocabularies.txt	2010-12-03 00:04:45 +0000
@@ -1052,103 +1052,11 @@
     [(u'Ubuntu Team', u'Mark Shuttleworth')]
 
 
-=== ValidTeamMember ===
-
-With the exception of all teams that have this team as a member and the
-team itself, all valid persons and teams are valid members.
-
-    >>> login('[e-mail]')
-    >>> team = person_set.getByName('ubuntu-team')
-    >>> team2 = person_set.getByName('guadamen')
-    >>> person = person_set.getByName('name16')
-
-ValidTeamMember needs a context:
-
-    >>> vocab = get_naked_vocab(None, "ValidTeamMember")
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-ValidTeamMember's context must implement ITeam:
-
-    >>> vocab = get_naked_vocab(person, "ValidTeamMember")
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-'name16' is a valid member for 'ubuntu-team':
-
-    >>> vocab = get_naked_vocab(team, "ValidTeamMember")
-    >>> vocab.displayname
-    'Select a Person or Team'
-
-    >>> person in vocab
-    True
-    >>> [person.name for person in vocab.search('foo.bar')]
-    [u'name16']
-
-'ubuntu-team' is not a valid member for itself:
-
-    >>> team in vocab
-    False
-    >>> [person.name for person in vocab.search('ubuntu-team')]
-    [u'name18', u'ubuntu-security']
-
-'ubuntu-team' is a member of 'guadamen', so 'guadamen' can't be a member
-of 'ubuntu-team'.
-
-    >>> team2 in vocab
-    False
-    >>> [person.name for person in vocab.search('guadamen')]
-    []
-
-
-=== ValidTeamOwner ===
-
-With the exception of the team itself and all teams owned by that team,
-all valid persons and teams are valid owners for the team in context.
-
-ValidTeamOwner needs a context.
-
-    >>> vocab = get_naked_vocab(None, "ValidTeamOwner")
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-ValidTeamOwner's context must provide IPerson or IPersonSet.
-
-    >>> get_naked_vocab(team, "ValidTeamOwner")
-    <...ValidTeamOwnerVocabulary...
-    >>> get_naked_vocab(person_set, "ValidTeamOwner")
-    <...ValidTeamOwnerVocabulary...
-    >>> get_naked_vocab(firefox, "ValidTeamOwner")
-    Traceback (most recent call last):
-    ...
-    AssertionError: ...
-
-'ubuntu-team' is not a valid owner for itself.
-
-    >>> vocab = get_naked_vocab(team, "ValidTeamOwner")
-    >>> vocab.displayname
-    'Select a Person or Team'
-
-    >>> team in vocab
-    False
-    >>> [person.name for person in vocab.search('ubuntu-team')]
-    [u'name18', u'ubuntu-security']
-
-'name16' is a valid owner for 'ubuntu-team'.
-
-    >>> person in vocab
-    True
-    >>> [person.name for person in vocab.search('foo.bar')]
-    [u'name16']
-
-
 === ValidPerson ===
 
 All 'valid' persons who are not a team.
 
+    >>> login('[e-mail]')
     >>> vocab = get_naked_vocab(None, "ValidPerson")
     >>> vocab.displayname
     'Select a Person'

=== modified file 'lib/lp/registry/javascript/team.js'
--- lib/lp/registry/javascript/team.js	2010-10-21 22:06:57 +0000
+++ lib/lp/registry/javascript/team.js	2010-12-03 00:04:45 +0000
@@ -14,14 +14,14 @@
  *
  * @method setup_add_member_handler
  */
-module.setup_add_member_handler = function() {
+module.setup_add_member_handler = function(step_title) {
     if (Y.UA.ie) {
         return;
     }
 
     var config = {
         header: 'Add a member',
-        step_title: 'Search',
+        step_title: step_title,
         picker_activator: '.menu-link-add_member'
     };
 

=== modified file 'lib/lp/registry/templates/team-portlet-membership.pt'
--- lib/lp/registry/templates/team-portlet-membership.pt	2010-11-10 15:33:47 +0000
+++ lib/lp/registry/templates/team-portlet-membership.pt	2010-12-03 00:04:45 +0000
@@ -101,14 +101,16 @@
   <table style="margin: 0px 0px .5em 0px;">
     <tr>
       <td style="padding: 0px 1em 1em 0px;"
-          tal:define="link context/menu:overview/add_member"
+          tal:define="link context/menu:overview/add_member;
+                      step_title view/add_member_step_title;"
           tal:condition="link/enabled">
         <script type="text/javascript"
                 tal:content="string:
           LPS.use('lp.registry.team', function(Y) {
               Y.on('load',
                   function(e) {
-                      Y.lp.registry.team.setup_add_member_handler();
+                      Y.lp.registry.team.setup_add_member_handler(
+                          '${step_title}');
                   },
                   window);
           });

=== modified file 'lib/lp/registry/tests/test_person_vocabularies.py'
--- lib/lp/registry/tests/test_person_vocabularies.py	2010-10-20 15:45:40 +0000
+++ lib/lp/registry/tests/test_person_vocabularies.py	2010-12-03 00:04:45 +0000
@@ -11,36 +11,117 @@
 
 from canonical.launchpad.ftests import login_person
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    )
 from lp.testing import TestCaseWithFactory
 
 
-class TestValidTeamMemberVocabulary(TestCaseWithFactory):
+class VocabularyTestBase:
+
+    vocabulary_name = None
+
+    def setUp(self):
+        super(VocabularyTestBase, self).setUp()
+        self.vocabulary_registry = getVocabularyRegistry()
+
+    def getVocabulary(self, context):
+        return self.vocabulary_registry.get(context, self.vocabulary_name)
+
+    def searchVocabulary(self, context, text):
+        Store.of(context).flush()
+        vocabulary = self.getVocabulary(context)
+        return removeSecurityProxy(vocabulary)._doSearch(text)
+
+    def test_open_team_cannot_be_a_member_of_a_closed_team(self):
+        context_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        open_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        moderated_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        restricted_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
+        user = self.factory.makePerson()
+        all_possible_members = self.searchVocabulary(context_team, '')
+        self.assertNotIn(open_team, all_possible_members)
+        self.assertIn(moderated_team, all_possible_members)
+        self.assertIn(restricted_team, all_possible_members)
+        self.assertIn(user, all_possible_members)
+
+    def test_open_team_can_be_a_member_of_an_open_team(self):
+        context_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        open_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        moderated_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        restricted_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED)
+        user = self.factory.makePerson()
+        all_possible_members = self.searchVocabulary(context_team, '')
+        self.assertIn(open_team, all_possible_members)
+        self.assertIn(moderated_team, all_possible_members)
+        self.assertIn(restricted_team, all_possible_members)
+        self.assertIn(user, all_possible_members)
+
+    def test_vocabulary_displayname(self):
+        context_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        vocabulary = self.getVocabulary(context_team)
+        self.assertEqual(
+            'Select a Team or Person', vocabulary.displayname)
+
+    def test_open_team_vocabulary_step_title(self):
+        context_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.OPEN)
+        vocabulary = self.getVocabulary(context_team)
+        self.assertEqual('Search', vocabulary.step_title)
+
+    def test_closed_team_vocabulary_step_title(self):
+        context_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        vocabulary = self.getVocabulary(context_team)
+        self.assertEqual(
+            'Search for a restricted team, a moderated team, or a person',
+            vocabulary.step_title)
+
+
+class TestValidTeamMemberVocabulary(VocabularyTestBase, TestCaseWithFactory):
     """Test that the ValidTeamMemberVocabulary behaves as expected."""
 
     layer = DatabaseFunctionalLayer
-
-    def searchVocabulary(self, team, text):
-        vocabulary_registry = getVocabularyRegistry()
-        naked_vocabulary = removeSecurityProxy(
-            vocabulary_registry.get(team, 'ValidTeamMember'))
-        return naked_vocabulary._doSearch(text)
+    vocabulary_name = 'ValidTeamMember'
 
     def test_public_team_cannot_be_a_member_of_itself(self):
         # A public team should be filtered by the vocab.extra_clause
         # when provided a search term.
-        team_owner = self.factory.makePerson()
-        login_person(team_owner)
-        team = self.factory.makeTeam(owner=team_owner)
-        Store.of(team).flush()
-        self.assertFalse(team in self.searchVocabulary(team, team.name))
+        team = self.factory.makeTeam()
+        self.assertNotIn(team, self.searchVocabulary(team, team.name))
 
     def test_private_team_cannot_be_a_member_of_itself(self):
         # A private team should be filtered by the vocab.extra_clause
         # when provided a search term.
-        team_owner = self.factory.makePerson()
-        login_person(team_owner)
         team = self.factory.makeTeam(
-            owner=team_owner, visibility=PersonVisibility.PRIVATE)
-        Store.of(team).flush()
-        self.assertFalse(team in self.searchVocabulary(team, team.name))
+            visibility=PersonVisibility.PRIVATE)
+        login_person(team.teamowner)
+        self.assertNotIn(team, self.searchVocabulary(team, team.name))
+
+
+class TestValidTeamOwnerVocabulary(VocabularyTestBase, TestCaseWithFactory):
+    """Test that the ValidTeamOwnerVocabulary behaves as expected."""
+
+    layer = DatabaseFunctionalLayer
+    vocabulary_name = 'ValidTeamOwner'
+
+    def test_team_cannot_own_itself(self):
+        context_team = self.factory.makeTeam()
+        results = self.searchVocabulary(context_team, context_team.name)
+        self.assertNotIn(context_team, results)
+
+    def test_team_cannot_own_its_owner(self):
+        context_team = self.factory.makeTeam()
+        owned_team = self.factory.makeTeam(owner=context_team)
+        results = self.searchVocabulary(context_team, owned_team.name)
+        self.assertNotIn(owned_team, results)

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-11-09 08:38:23 +0000
+++ lib/lp/registry/vocabularies.py	2010-12-03 00:04:45 +0000
@@ -47,6 +47,7 @@
     'SourcePackageNameVocabulary',
     'UserTeamsParticipationVocabulary',
     'UserTeamsParticipationPlusSelfVocabulary',
+    'ValidPersonOrClosedTeamVocabulary',
     'ValidPersonOrTeamVocabulary',
     'ValidPersonVocabulary',
     'ValidTeamMemberVocabulary',
@@ -143,6 +144,7 @@
     IPersonSet,
     ITeam,
     PersonVisibility,
+    TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.pillar import IPillarName
 from lp.registry.interfaces.product import (
@@ -219,6 +221,7 @@
 class ProductVocabulary(SQLObjectVocabularyBase):
     """All `IProduct` objects vocabulary."""
     implements(IHugeVocabulary)
+    step_title = 'Search'
 
     _table = Product
     _orderBy = 'displayname'
@@ -271,6 +274,7 @@
     _table = ProjectGroup
     _orderBy = 'displayname'
     displayname = 'Select a project group'
+    step_title = 'Search'
 
     def __contains__(self, obj):
         where = "active='t' and id=%d"
@@ -361,6 +365,7 @@
 
     _orderBy = ['displayname']
     displayname = 'Select a Person or Team'
+    step_title = 'Search'
 
     def __contains__(self, obj):
         return obj in self._select()
@@ -391,6 +396,7 @@
 
     _orderBy = ['displayname']
     displayname = 'Select a Person to Merge'
+    step_title = 'Search'
     must_have_email = True
 
     def __contains__(self, obj):
@@ -439,7 +445,7 @@
     implements(IHugeVocabulary)
 
     displayname = 'Select a Person or Team'
-
+    step_title = 'Search'
     # This is what subclasses must change if they want any extra filtering of
     # results.
     extra_clause = True
@@ -722,11 +728,32 @@
     cache_table_name = 'ValidPersonCache'
 
 
-class ValidTeamMemberVocabulary(ValidPersonOrTeamVocabulary):
+class TeamVocabularyMixin:
+    """Common methods for team vocabularies."""
+
+    displayname = 'Select a Team or Person'
+
+    @property
+    def is_closed_team(self):
+        return self.team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN
+
+    @property
+    def step_title(self):
+        """See `IHugeVocabulary`."""
+        if self.is_closed_team:
+            return (
+                'Search for a restricted team, a moderated team, or a person')
+        else:
+            return 'Search'
+
+
+class ValidTeamMemberVocabulary(TeamVocabularyMixin,
+                                ValidPersonOrTeamVocabulary):
     """The set of valid members of a given team.
 
     With the exception of all teams that have this team as a member and the
-    team itself, all valid persons and teams are valid members.
+    team itself, all valid persons and teams are valid members. Restricted
+    and moderated teams cannot have open teams as members.
     """
 
     def __init__(self, context):
@@ -740,19 +767,29 @@
                 "Got %s" % str(context))
 
         ValidPersonOrTeamVocabulary.__init__(self, context)
-        self.extra_clause = """
+
+    @property
+    def extra_clause(self):
+        clause = SQL("""
             Person.id NOT IN (
                 SELECT team FROM TeamParticipation
                 WHERE person = %d
                 )
-            """ % self.team.id
-
-
-class ValidTeamOwnerVocabulary(ValidPersonOrTeamVocabulary):
+            """ % self.team.id)
+        if self.is_closed_team:
+            clause = And(
+                clause,
+                Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
+        return clause
+
+
+class ValidTeamOwnerVocabulary(TeamVocabularyMixin,
+                               ValidPersonOrTeamVocabulary):
     """The set of Persons/Teams that can be owner of a team.
 
     With the exception of the team itself and all teams owned by that team,
-    all valid persons and teams are valid owners for the team.
+    all valid persons and teams are valid owners for the team. Restricted
+    and moderated teams cannot have open teams as members.
     """
 
     def __init__(self, context):
@@ -760,9 +797,7 @@
             raise AssertionError('ValidTeamOwnerVocabulary needs a context.')
 
         if IPerson.providedBy(context):
-            self.extra_clause = """
-                (person.teamowner != %d OR person.teamowner IS NULL) AND
-                person.id != %d""" % (context.id, context.id)
+            self.team = context
         elif IPersonSet.providedBy(context):
             # The context is an IPersonSet, which means we're creating a new
             # team and thus we don't need any extra_clause --any valid person
@@ -774,6 +809,17 @@
                 "or IPersonSet.")
         ValidPersonOrTeamVocabulary.__init__(self, context)
 
+    @property
+    def extra_clause(self):
+        clause = SQL("""
+            (person.teamowner != %d OR person.teamowner IS NULL) AND
+            person.id != %d""" % (self.team.id, self.team.id))
+        if self.is_closed_team:
+            clause = And(
+                clause,
+                Person.subscriptionpolicy != TeamSubscriptionPolicy.OPEN)
+        return clause
+
 
 class AllUserTeamsParticipationVocabulary(ValidTeamVocabulary):
     """The set of teams where the current user is a member.
@@ -844,6 +890,7 @@
     implements(IHugeVocabulary)
 
     displayname = 'Select an active mailing list.'
+    step_title = 'Search'
 
     def __init__(self, context):
         assert context is None, (
@@ -965,6 +1012,7 @@
     implements(IHugeVocabulary)
 
     displayname = 'Select a Product Release'
+    step_title = 'Search'
     _table = ProductRelease
     # XXX carlos Perello Marin 2005-05-16 bugs=687:
     # Sorting by version won't give the expected results, because it's just a
@@ -1032,6 +1080,7 @@
     implements(IHugeVocabulary)
 
     displayname = 'Select a Release Series'
+    step_title = 'Search'
     _table = ProductSeries
     _order_by = [Product.name, ProductSeries.name]
     _clauseTables = ['Product']
@@ -1265,6 +1314,7 @@
 
     _table = Product
     _orderBy = 'displayname'
+    step_title = 'Search'
 
     @property
     def displayname(self):