Hi Brad,

This is a nice improvement. Besides what we discussed on IRC, I have a couple of minor comments below.

merge-conditional

-Edwin


IRC Log
-------

[3:00 PM] <EdwinGrubbs> bac: What is the purpose of adding the pub_member to the pub_team? It appears to have no effect on the test.
[3:00 PM] <bac> EdwinGrubbs: likely a mistake
[3:01 PM] <bac> EdwinGrubbs: oh, i was going to show that even though pub_owner could see private parts the mere pub_member could not
[3:01 PM] <bac> EdwinGrubbs: i think that's still a good thing to do, i just forgot to do it
[3:05 PM] <bac> EdwinGrubbs: this is what i intended:  http://pastebin.ubuntu.com/351429/
{{{
=== modified file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt	2010-01-04 19:33:33 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt	2010-01-04 21:04:34 +0000
@@ -75,9 +75,18 @@
     >>> pub_owner in priv_team.activemembers
     False
 
-But the public team's owner can see the priv-team's bits since his team
+The public team's owner can now see the priv-team's bits since his team
 has been invited to join.
 
     >>> login_person(pub_owner)
     >>> print priv_team.name
     priv-team
+
+But a non-admin member of the public team still cannot see anything
+about the team.
+
+    >>> login_person(pub_member)
+    >>> print priv_team.name
+    Traceback (most recent call last):
+    ...
+    Unauthorized: (<Person at ... priv-team (Priv Team)>, 'name', 'launchpad.View')
}}}

[3:06 PM] <EdwinGrubbs> that looks good
[3:30 PM] <EdwinGrubbs> bac: I was thinking about the test to see if the user is in getDirectAdministrators(), and it seems like it would cause a problem for teams that are owned by another team. Therefore, you should probably use can_edit_team(invited_team, user)
[3:32 PM] <bac> EdwinGrubbs: good idea, i think




Comments on Diff
----------------

>=== modified file 'lib/lp/registry/doc/private-team-roles.txt'
>--- lib/lp/registry/doc/private-team-roles.txt 2009-10-26 20:08:10 +0000
>+++ lib/lp/registry/doc/private-team-roles.txt 2010-01-04 20:51:08 +0000
>@@ -243,6 +243,7 @@
>     >>> import transaction
>     >>> transaction.abort()
> 
>+
> Structural Subscriptions
> ========================
> 
>
>=== added file 'lib/lp/registry/doc/private-team-visibility.txt'
>--- lib/lp/registry/doc/private-team-visibility.txt    1970-01-01 00:00:00 +0000
>+++ lib/lp/registry/doc/private-team-visibility.txt    2010-01-04 20:51:08 +0000
>@@ -0,0 +1,83 @@
>+=========================
>+ Private team visibility
>+=========================
>+
>+Private and private membership team restrict the visibility of their


s/team/teams/


>+attributes to select sets of users in order to prevent leaking
>+confidential data.
>+
>+Private teams restrict the viewing of the membership list
>+to administrators and other members of the team.


You might want to say "Launchpad administrators", since
the team admins are also members, so it can sound redundant even
though it isn't.


>+    >>> from lp.registry.interfaces.person import PersonVisibility
>+    >>> priv_owner = factory.makePerson(name="priv-owner")
>+    >>> priv_member = factory.makePerson(name="priv-member")
>+    >>> login('<email address hidden>')
>+    >>> priv_team = factory.makeTeam(owner=priv_owner, name="priv-team",
>+    ...                              visibility=PersonVisibility.PRIVATE)
>+    >>> login_person(priv_owner)
>+    >>> priv_team.addMember(priv_member, reviewer=priv_owner)
>+