Merge lp:~lifeless/launchpad/bug-227494 into lp:launchpad

[Robert Collins]

Make perosn.inTeam(foo) and foo.members agree even for team owners.

Per the attached bug we currently have an awkward situation where if you ask 'is teamX.owner in the list of members of team X' and 'is teamX.owner a member of team X' you get different answers.

This exhibits itself as team owners being able to do anything the team can do even if they are not in the team and not shown as the team in the web UI.

This small patch corrects that. It leaves team owners as *administrators* of the team they own, because thats much less confusing.

[William Grant (wgrant)]

login_person in the middle of the test seems a bit arbitrary. Could you do it earlier? I also worry that it may invalidate more tests, as you only changed it because one case depended on the logged in user.

review: Approve (code*)

[Robert Collins (lifeless)]

On Tue, May 31, 2011 at 5:35 PM, William Grant <email address hidden> wrote:
> Review: Approve code*
> login_person in the middle of the test seems a bit arbitrary. Could you do it earlier? I also worry that it may invalidate more tests, as you only changed it because one case depended on the logged in user.

Earlier scenarios use a different user. Or is that different to what you mean?

The test script in question passes, and I found the two failing
scripts via an ec2 test run.

[Robert Collins (lifeless)]

18:47 < lifeless> wgrant: follow up on https://code.launchpad.net/~lifeless/launchpad/bug-227494/+merge/62943 ?
18:49 < wgrant> lifeless: You are arbitrarily changing the user to an admin half-way through because the owner has no privs.
18:49 < wgrant> That may be invalidating subsequent tests.
18:49 < wgrant> Even if it isn't, it's rather confusing to change it partially globally when only that one block needs it.
18:50 < lifeless> wgrant: have you looked through that doctest ?
18:50 < lifeless> hint: it does this a lot already
18:50 < wgrant> It's a doctest. No.
18:50 < lifeless> and the previous user was one it thought was an admin.
18:50 < wgrant> Ah, I see.
18:50 < wgrant> OK.
18:50 < wgrant> An admin, but not an Admin.
18:50 < wgrant> Not an ~admin, sorry.
18:50 < lifeless> yes, a ~admin
18:51 < wgrant> How did it stop being an admin?
18:51 < lifeless> because it was mark
18:51 < wgrant> Because it was the owner of ~admins?
18:51 < lifeless> yes
18:51 < wgrant> But the test removed that membership?
18:51 < lifeless> yes
18:51 < wgrant> ahhhhhhhh
18:51 < lifeless> it was BROKEN
18:51 < wgrant> Carry on, then.
18:51 < wgrant> Preferably beating that test to death on the way.

[William Grant (wgrant)]

Yay

review: Approve (code)

=== modified file 'lib/lp/registry/doc/teammembership-email-notification.txt'
--- lib/lp/registry/doc/teammembership-email-notification.txt	2011-05-27 19:53:20 +0000
+++ lib/lp/registry/doc/teammembership-email-notification.txt	2011-05-31 05:29:26 +0000
@@ -41,6 +41,8 @@
     >>> kamion = personset.getByName('kamion')
     >>> sampleperson = personset.getByName('name12')
     >>> ubuntu_team = personset.getByName('ubuntu-team')
+    >>> from lp.testing.sampledata import ADMIN_EMAIL
+    >>> admin_person = personset.getByEmail(ADMIN_EMAIL)
 
 
 Now Robert Collins proposes himself as a member of the Ubuntu Team. This
@@ -826,8 +828,9 @@
     ...     hwdb_admins)
     >>> print dumper_hwdb_membership.status.title
     Approved
-    >>> setStatus(dumper_hwdb_membership,
-    ...     TeamMembershipStatus.DEACTIVATED, reviewer=mark, silent=True)
+    >>> login_person(admin_person)
+    >>> setStatus(dumper_hwdb_membership, TeamMembershipStatus.DEACTIVATED,
+    ...     reviewer=admin_person, silent=True)
     >>> run_mail_jobs()
     >>> len(stub.test_emails)
     0

=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt	2011-04-21 05:18:26 +0000
+++ lib/lp/registry/doc/teammembership.txt	2011-05-31 05:29:26 +0000
@@ -410,7 +410,7 @@
 It's important to note that even if the owner leaves the team, which
 removes his membership, he will still be the team's owner and retain his
 rights over it. This ensures we'll never have teams which can't be
-managed.
+managed. This does not imply that the owner will be a member of the team.
 
     >>> login_person(t5.teamowner)
     >>> t5.teamowner.leave(t5)
@@ -418,8 +418,23 @@
     >>> [m.displayname for m in t5.allmembers]
     []
     >>> t5.teamowner.inTeam(t5)
-    True
-
+    False
+
+The team owner can make themselves a member again even if the team is
+restricted:
+
+    >>> t5.teamowner.join(t5, requester=t5.teamowner)
+    >>> flush_database_updates()
+    >>> t5.teamowner in t5.allmembers
+    True
+    >>> t5.teamowner.inTeam(t5)
+    True
+
+And escalate their privileges back to administrator:
+
+    >>> membership = membershipset.getByPersonAndTeam(t5.teamowner, t5)
+    >>> membership.setStatus(TeamMembershipStatus.ADMIN, t5.teamowner)
+    True
 
 Changing membership data
 ------------------------

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-05-18 03:36:29 +0000
+++ lib/lp/registry/interfaces/person.py	2011-05-31 05:29:26 +0000
@@ -1214,7 +1214,7 @@
     # @operation_parameters(team=copy_field(ITeamMembership['team']))
     # @export_read_operation()
     def inTeam(team):
-        """Is this person is a member or the owner of `team`?
+        """Is this person is a member of `team`?
 
         Returns `True` when you ask if an `IPerson` (or an `ITeam`,
         since it inherits from `IPerson`) is a member of himself

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-05-27 21:53:34 +0000
+++ lib/lp/registry/model/person.py	2011-05-31 05:29:26 +0000
@@ -1336,18 +1336,7 @@
                 pass
 
         tp = TeamParticipation.selectOneBy(team=team, person=self)
-        if tp is not None or self.id == team.teamownerID:
-            in_team = True
-        elif not team.teamowner.inTeam(team):
-            # The owner is not a member but must retain his rights over
-            # this team. This person may be a member of the owner, and in this
-            # case it'll also have rights over this team.
-            # Note that this query and the tp query above can be consolidated
-            # when we get to a finer grained level of optimisations.
-            in_team = self.inTeam(team.teamowner)
-        else:
-            in_team = False
-
+        in_team = tp is not None
         self._inTeam_cache[team.id] = in_team
         return in_team