launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/membership-status-matters-676494 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #676494 merge causes OOPs in Person:+participation by introducing missing key in getPathsToTeams()
  https://bugs.launchpad.net/bugs/676494


Summary
=======

In merges, there was an execution path that didn't fully clean up TeamParticipation entries for indirect memberships. A branch was landed to deal with this, but it filtered out TeamParticipation entries corresponding to any TeamMembership entry. To be correct, it should only avoid deleting TeamParticipation entries that correspond to an ACTIVE TeamMembership entry.

This branch modifies the cleanup query to filter only on ACTIVE states.

Preimplementation Talks
=======================

Long debate and exploration with the Registry Team, esp Curtis Hovey.

Implementation
==============

The query to cleanup indirect membership TeamParticipation entries was modified to only avoid deleting entries corresponding to TeamMemberships that are active (e.g. status of APPROVED or ADMIN)

Tests
=====

bin/test -vvct MembershipManagement

Lint
====

make lint output:


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team.py

-- 
https://code.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/membership-status-matters-676494 into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-12-06 21:15:42 +0000
+++ lib/lp/registry/model/person.py	2010-12-09 22:46:30 +0000
@@ -276,6 +276,9 @@
     )
 
 
+ACTIVE_STATES = [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
+
+
 class JoinTeamEvent:
     """See `IJoinTeamEvent`."""
 
@@ -1498,8 +1501,9 @@
                         (SELECT tm1.person from TeamMembership tm1
                             WHERE
                                 tm1.person = TeamParticipation.person and
-                                tm1.team = TeamParticipation.team);
-            ''', dict(team=self.id))
+                                tm1.team = TeamParticipation.team and
+                                tm1.status IN %(active_states)s);
+            ''', dict(team=self.id, active_states=ACTIVE_STATES))
 
         # Since we've updated the database behind Storm's back yet again,
         # we need to flush its caches, again.

=== 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-09 22:46:30 +0000
@@ -216,6 +216,46 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_deactivateAllMembers_cleans_up_teamparticipation_deactivated(
+            self):
+        superteam = self.factory.makeTeam(name='super')
+        targetteam = self.factory.makeTeam(name='target')
+        login_celebrity('admin')
+        targetteam.join(superteam, targetteam.teamowner)
+
+        # Now we create a deactivated link for the target team's teamowner.
+        targetteam.teamowner.join(superteam, targetteam.teamowner)
+        targetteam.teamowner.leave(superteam)
+
+        self.assertEqual(
+                sorted([superteam, targetteam]),
+                sorted([team for team in
+                    targetteam.teamowner.teams_participated_in]))
+        targetteam.deactivateAllMembers(
+            comment='test',
+            reviewer=targetteam.teamowner)
+        self.assertEqual(
+            [],
+            sorted([team for team in
+                targetteam.teamowner.teams_participated_in]))
+
+    def test_deactivateAllMembers_cleans_up_teamparticipation_teamowner(self):
+        superteam = self.factory.makeTeam(name='super')
+        targetteam = self.factory.makeTeam(name='target')
+        login_celebrity('admin')
+        targetteam.join(superteam, targetteam.teamowner)
+        self.assertEqual(
+                sorted([superteam, targetteam]),
+                sorted([team for team
+                            in targetteam.teamowner.teams_participated_in]))
+        targetteam.deactivateAllMembers(
+            comment='test',
+            reviewer=targetteam.teamowner)
+        self.assertEqual(
+            [],
+            sorted([team for team
+                in targetteam.teamowner.teams_participated_in]))
+
     def test_deactivateAllMembers_cleans_up_team_participation(self):
         superteam = self.factory.makeTeam(name='super')
         sharedteam = self.factory.makeTeam(name='shared')