launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02042
[Merge] lp:~jcsackett/launchpad/merge-person-participation-676494 into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/merge-person-participation-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
=======
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
--
https://code.launchpad.net/~jcsackett/launchpad/merge-person-participation-676494/+merge/41932
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/merge-person-participation-676494 into lp:launchpad.
=== 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-11-26 07:08:33 +0000
@@ -245,6 +245,10 @@
if self.target_person is 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-11-26 07:08:33 +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)
@@ -131,6 +132,23 @@
login_celebrity('admin')
self.dupe_team.join(super_team, self.dupe_team.teamowner)
login_celebrity('registry_experts')
+ 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_celebrity('registry_experts')
view = self.getView()
self.assertEqual([], view.errors)
self.assertEqual(self.target_team, self.dupe_team.merged)
=== 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-11-26 07:08:33 +0000
@@ -1474,6 +1474,37 @@
# 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.
participants = store.find(
@@ -3816,6 +3847,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)
@@ -3991,6 +4025,7 @@
cur.execute('UPDATE %s SET %s=%d WHERE %s=%d' % (
src_tab, src_col, to_person.id, src_col, from_person.id))
+ # TODO this needs to NEVER happen for teams
self._mergeTeamMembership(cur, from_id, to_id)
# Flag the person as merged
=== 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-11-26 07:08:33 +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 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)
+ test_team.deactivateAllMembers(
+ comment='',
+ reviewer=test_team.teamowner)
+ self.person_set.merge(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)
+
+ def doMerge():
+ test_team.deactivateAllMembers(
+ comment='',
+ reviewer=test_team.teamowner)
+ self.person_set.merge(test_team, target_team)
+
+ self.assertRaises(AssertionError, doMerge)
+
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-11-26 07:08:33 +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):
+ self.superteam = self.factory.makeTeam(name='super')
+ self.sharedteam = self.factory.makeTeam(name='shared')
+ self.anotherteam = self.factory.makeTeam(name='another')
+ self.targetteam = self.factory.makeTeam(name='target')
+ self.person = self.factory.makePerson()
+ login_celebrity('admin')
+ self.person.join(self.targetteam)
+ self.person.join(self.sharedteam)
+ self.person.join(self.anotherteam)
+ self.targetteam.join(self.superteam, self.targetteam.teamowner)
+ self.targetteam.join(self.sharedteam, self.targetteam.teamowner)
+ self.assertTrue(self.superteam in self.person.teams_participated_in)
+ self.targetteam.deactivateAllMembers(
+ comment='test',
+ reviewer=self.targetteam.teamowner)
+ self.assertEqual(
+ sorted([self.sharedteam, self.anotherteam]),
+ sorted([team for team in self.person.teams_participated_in]))
