Merge lp:~jcsackett/launchpad/merge-person-participation-676494 into lp:launchpad

[j.c.sackett]

[r=bac][ui=none][bug=676494] Fixes bad execution path that leads to extra teams being assigned to ~registry on merge and bad entries in TeamParticipation.

Summary

=======

A long execution path on merging teams as part of a delete leads to corruption of the TeamParticipation table and the addition of teams to ~registry that should never be part of it.

To remedy this, this branch adds an extra guard to prevent super teams from making it through the team merge, as well as cleaning up team participation for indirect entries.
Additional, use of flush_db_updates lower in the call stack helps sync team removal.

Proposed fix

============

Add additional guards to the process of merging teams, add additional db flushes, and make sure to delete all the team participation entries for members when deactivateAllMembers is called.

Preimplementation talk

======================

Talked with and explored the execution path with Curtis Hovey.

Implementation details

======================

As in proposed. The cleanup for team participation is a particularly complicated sql query--I'm very open to discussing better implementations if you have an idea. This branch presents the best I could come up with.

Tests

=====

bin/test -t test_peoplemerge.py

bin/test -t test_person.py

bin/test -t test_team.py

Lint

====

make lint output:

= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:

  lib/lp/registry/browser/peoplemerge.py

  lib/lp/registry/browser/tests/test_peoplemerge.py

  lib/lp/registry/model/person.py

  lib/lp/registry/tests/test_person.py

  lib/lp/registry/tests/test_team.py

[Brad Crittenden (bac)]

Hi Jon,

Thanks for this fix. I hope it completely solves the problem.

In test_team_with_super_teams_raises_error the doMerge method could be shared with test_team_without_super_teams_is_fine.

The comment in test_team_without_super_teams_is_fine should end with punctuation.

In test_deactivateAllMembers_cleans_up_team_participation you create a bunch of new objects that are created as objects on the instance but they are not used outside the method. Please change them to just be local variables.

I've gone over the changes carefully and they all make sense. I'm very happy you and Curtis worked this out. I think this fix will solve the other TeamParticipation problem (bug 651210).

review: Approve (code)

[j.c.sackett (jcsackett)]

> In test_team_with_super_teams_raises_error the doMerge method could be shared with test_team_without_super_teams_is_fine.

I've restructured so they share a method, _doMerge.

> The comment in test_team_without_super_teams_is_fine should end with punctuation.

Done.

> In test_deactivateAllMembers_cleans_up_team_participation you create a bunch of new objects that are created as objects on the instance but they are not used outside the method. Please change them to just be local variables.

Good catch. That's taken care of.

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2010-11-23 23:22:27 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2010-12-02 15:08:17 +0000
@@ -242,9 +242,13 @@
         self.dupe_person.setContactAddress(None)
         # The registry experts does not want to acquire super teams from a
         # merge.
-        if self.target_person is self.registry_experts:
+        if self.target_person == self.registry_experts:
             for team in self.dupe_person.teams_participated_in:
                 self.dupe_person.retractTeamMembership(team, self.user)
+        # We have sent another series of calls to the db, potentially a long
+        # sequence depending on the merge. We want everything synced up
+        # before proceeding.
+        flush_database_updates()
         super(AdminTeamMergeView, self).doMerge(data)
 
     def validate(self, data):

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	2010-11-12 05:36:22 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-02 15:08:17 +0000
@@ -95,12 +95,13 @@
         self.target_team = self.factory.makeTeam()
         login_celebrity('registry_experts')
 
-    def getView(self):
-        form = {
-            'field.dupe_person': self.dupe_team.name,
-            'field.target_person': self.target_team.name,
-            'field.actions.deactivate_members_and_merge': 'Merge',
-            }
+    def getView(self, form=None):
+        if form is None:
+            form = {
+                'field.dupe_person': self.dupe_team.name,
+                'field.target_person': self.target_team.name,
+                'field.actions.deactivate_members_and_merge': 'Merge',
+                }
         return create_initialized_view(
             self.person_set, '+adminteammerge', form=form)
 
@@ -130,7 +131,24 @@
         super_team = self.factory.makeTeam()
         login_celebrity('admin')
         self.dupe_team.join(super_team, self.dupe_team.teamowner)
-        login_celebrity('registry_experts')
+        login_person(self.dupe_team.teamowner)
+        form = {
+            'field.dupe_person': self.dupe_team.name,
+            'field.target_person': self.target_team.name,
+            'field.actions.merge': 'Merge',
+            }
+        view = self.getView()
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.target_team, self.dupe_team.merged)
+
+    def test_merge_team_as_delete_with_super_teams_into_registry_experts(
+            self):
+        # Super team memberships are removed.
+        self.target_team = getUtility(ILaunchpadCelebrities).registry_experts
+        super_team = self.factory.makeTeam()
+        login_celebrity('admin')
+        self.dupe_team.join(super_team, self.dupe_team.teamowner)
+        login_person(self.dupe_team.teamowner)
         view = self.getView()
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)

=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt	2010-10-10 15:39:28 +0000
+++ lib/lp/registry/doc/person-merge.txt	2010-12-02 15:08:17 +0000
@@ -423,9 +423,11 @@
 
     >>> comment = "We're merging this team into another one."
     >>> test_team.deactivateAllMembers(comment, personset.getByName('name16'))
+    >>> for team in test_team.super_teams:
+    ...     test_team.retractTeamMembership(team, test_team.teamowner)
     >>> personset.merge(test_team, landscape)
 
-    # The resulting Landscape-developers has now a super team, but has
+    # The resulting Landscape-developers no new super teams, has
     # no polls and its members are still the same two from before the
     # merge.
     >>> landscape.teamowner.name
@@ -433,6 +435,6 @@
     >>> [member.name for member in landscape.allmembers]
     [u'salgado', u'name12']
     >>> [team.name for team in landscape.super_teams]
-    [u'launchpad']
+    []
     >>> list(IPollSubset(landscape).getAll())
     []

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-11-24 10:10:07 +0000
+++ lib/lp/registry/model/person.py	2010-12-02 15:08:17 +0000
@@ -1473,6 +1473,38 @@
         # Since we've updated the database behind Storm's back,
         # flush its caches.
         store.invalidate()
+        
+
+        # Remove all indirect TeamParticipation entries resulting from this
+        # team. If this were just a select, it would be a complicated but
+        # feasible set of joins. Since it's a delete, we have to use
+        # some sub selects.
+        cur.execute('''
+            DELETE FROM TeamParticipation
+                WHERE
+                    -- The person needs to be a member of the team in question
+                    person IN
+                        (SELECT person from TeamParticipation WHERE
+                            team = %(team)s) AND
+
+                    -- The teams being deleted should be teams that this team
+                    -- is a member of.
+                    team IN
+                        (SELECT team from TeamMembership WHERE
+                            person = %(team)s) AND
+
+                    -- The person needs to not have direct membership in the
+                    -- team.
+                    NOT EXISTS
+                        (SELECT tm1.person from TeamMembership tm1
+                            WHERE
+                                tm1.person = TeamParticipation.person and
+                                tm1.team = TeamParticipation.team);
+            ''', dict(team=self.id))
+
+        # Since we've updated the database behind Storm's back yet again,
+        # we need to flush its caches, again.
+        store.invalidate()
 
         # Remove all members from the TeamParticipation table
         # except for the team, itself.
@@ -3816,6 +3848,9 @@
             raise AssertionError(
                 "Only teams without active members can be merged")
 
+        if from_person.is_team and from_person.super_teams.count() > 0:
+            raise AssertionError(
+                "Only teams without super teams can be merged.")
         # since we are doing direct SQL manipulation, make sure all
         # changes have been flushed to the database
         store = Store.of(from_person)

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-11-08 01:08:15 +0000
+++ lib/lp/registry/tests/test_person.py	2010-12-02 15:08:17 +0000
@@ -226,7 +226,7 @@
     def test_inTeam_person_string_missing_team(self):
         # If a check against a string is done, the team lookup is implicit:
         # treat a missing team as an empty team so that any pages that choose
-        # to do this don't blow up unnecessarily. Similarly feature flags 
+        # to do this don't blow up unnecessarily. Similarly feature flags
         # team: scopes depend on this.
         self.assertFalse(self.user.inTeam('does-not-exist'))
 
@@ -599,6 +599,36 @@
         self.person_set.merge(duplicate, person)
         self.assertEqual(oldest_date, person.datecreated)
 
+    def _doMerge(self, test_team, target_team):
+        test_team.deactivateAllMembers(
+            comment='',
+            reviewer=test_team.teamowner)
+        self.person_set.merge(test_team, target_team)
+
+    def test_team_without_super_teams_is_fine(self):
+        # A team with no members and no super teams
+        # merges without errors.
+        test_team = self.factory.makeTeam()
+        target_team = self.factory.makeTeam()
+
+        login_person(test_team.teamowner)
+        self._doMerge(test_team, target_team)
+
+    def test_team_with_super_teams_raises_error(self):
+        # A team with no members but with superteams
+        # raises an assertion error.
+        test_team = self.factory.makeTeam()
+        super_team = self.factory.makeTeam()
+        target_team = self.factory.makeTeam()
+
+        login_person(test_team.teamowner)
+        test_team.join(super_team, test_team.teamowner)
+        self.assertRaises(
+            AssertionError,
+            self._doMerge,
+            test_team,
+            target_team)
+
 
 class TestPersonSetCreateByOpenId(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-11-23 16:16:20 +0000
+++ lib/lp/registry/tests/test_team.py	2010-12-02 15:08:17 +0000
@@ -210,3 +210,28 @@
         self.assertEqual(
             None,
             self.team.visibilityConsistencyWarning(PersonVisibility.PRIVATE))
+
+
+class TestMembershipManagement(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_deactivateAllMembers_cleans_up_team_participation(self):
+        superteam = self.factory.makeTeam(name='super')
+        sharedteam = self.factory.makeTeam(name='shared')
+        anotherteam = self.factory.makeTeam(name='another')
+        targetteam = self.factory.makeTeam(name='target')
+        person = self.factory.makePerson()
+        login_celebrity('admin')
+        person.join(targetteam)
+        person.join(sharedteam)
+        person.join(anotherteam)
+        targetteam.join(superteam, targetteam.teamowner)
+        targetteam.join(sharedteam, targetteam.teamowner)
+        self.assertTrue(superteam in person.teams_participated_in)
+        targetteam.deactivateAllMembers(
+            comment='test',
+            reviewer=targetteam.teamowner)
+        self.assertEqual(
+            sorted([sharedteam, anotherteam]),
+            sorted([team for team in person.teams_participated_in]))