Launchpad itself

Merge lp:~jcsackett/launchpad/membership-status-matters-676494 into lp:launchpad

  1. membership-status-matters-676494
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 12047
Proposed branch: lp:~jcsackett/launchpad/membership-status-matters-676494
Merge into: lp:launchpad
Diff against target: 130 lines (+52/-7)
4 files modified
lib/lp/registry/interfaces/teammembership.py (+4/-0)
lib/lp/registry/model/person.py (+7/-4)
lib/lp/registry/model/teammembership.py (+1/-3)
lib/lp/registry/tests/test_team.py (+40/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/membership-status-matters-676494
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+43303@code.launchpad.net

Commit message

[r=bac][ui=none][bug=676494] Fixes a bad filter in deactivateAllMemberships that led to not all TP entries being deleted

Description of the change

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

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Aside from moving ACTIVE_STATES to in the teammembership interface, as discussed on IRC, this looks good. Thanks.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py 2010-09-29 14:38:30 +0000
+++ lib/lp/registry/interfaces/teammembership.py 2010-12-13 13:35:15 +0000
@@ -8,6 +8,7 @@
8__metaclass__ = type8__metaclass__ = type
99
10__all__ = [10__all__ = [
11 'ACTIVE_STATES',
11 'CyclicalTeamMembershipError',12 'CyclicalTeamMembershipError',
12 'DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT',13 'DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT',
13 'ITeamMembership',14 'ITeamMembership',
@@ -111,6 +112,9 @@
111 """)112 """)
112113
113114
115ACTIVE_STATES = [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
116
117
114class ITeamMembership(Interface):118class ITeamMembership(Interface):
115 """TeamMembership for Users"""119 """TeamMembership for Users"""
116 export_as_webservice_entry()120 export_as_webservice_entry()
117121
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-12-10 07:52:21 +0000
+++ lib/lp/registry/model/person.py 2010-12-13 13:35:15 +0000
@@ -232,7 +232,10 @@
232 SSHKeyCompromisedError,232 SSHKeyCompromisedError,
233 SSHKeyType,233 SSHKeyType,
234 )234 )
235from lp.registry.interfaces.teammembership import TeamMembershipStatus235from lp.registry.interfaces.teammembership import (
236 ACTIVE_STATES,
237 TeamMembershipStatus,
238 )
236from lp.registry.interfaces.wikiname import (239from lp.registry.interfaces.wikiname import (
237 IWikiName,240 IWikiName,
238 IWikiNameSet,241 IWikiNameSet,
@@ -1473,7 +1476,6 @@
1473 # Since we've updated the database behind Storm's back,1476 # Since we've updated the database behind Storm's back,
1474 # flush its caches.1477 # flush its caches.
1475 store.invalidate()1478 store.invalidate()
1476
14771479
1478 # Remove all indirect TeamParticipation entries resulting from this1480 # Remove all indirect TeamParticipation entries resulting from this
1479 # team. If this were just a select, it would be a complicated but1481 # team. If this were just a select, it would be a complicated but
@@ -1499,8 +1501,9 @@
1499 (SELECT tm1.person from TeamMembership tm11501 (SELECT tm1.person from TeamMembership tm1
1500 WHERE1502 WHERE
1501 tm1.person = TeamParticipation.person and1503 tm1.person = TeamParticipation.person and
1502 tm1.team = TeamParticipation.team);1504 tm1.team = TeamParticipation.team and
1503 ''', dict(team=self.id))1505 tm1.status IN %(active_states)s);
1506 ''', dict(team=self.id, active_states=ACTIVE_STATES))
15041507
1505 # Since we've updated the database behind Storm's back yet again,1508 # Since we've updated the database behind Storm's back yet again,
1506 # we need to flush its caches, again.1509 # we need to flush its caches, again.
15071510
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2010-10-05 01:55:03 +0000
+++ lib/lp/registry/model/teammembership.py 2010-12-13 13:35:15 +0000
@@ -60,6 +60,7 @@
60 IMembershipNotificationJobSource,60 IMembershipNotificationJobSource,
61 )61 )
62from lp.registry.interfaces.teammembership import (62from lp.registry.interfaces.teammembership import (
63 ACTIVE_STATES,
63 CyclicalTeamMembershipError,64 CyclicalTeamMembershipError,
64 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,65 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
65 ITeamMembership,66 ITeamMembership,
@@ -69,9 +70,6 @@
69 )70 )
7071
7172
72ACTIVE_STATES = [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
73
74
75class TeamMembership(SQLBase):73class TeamMembership(SQLBase):
76 """See `ITeamMembership`."""74 """See `ITeamMembership`."""
7775
7876
=== 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-13 13:35:15 +0000
@@ -216,6 +216,46 @@
216216
217 layer = DatabaseFunctionalLayer217 layer = DatabaseFunctionalLayer
218218
219 def test_deactivateAllMembers_cleans_up_teamparticipation_deactivated(
220 self):
221 superteam = self.factory.makeTeam(name='super')
222 targetteam = self.factory.makeTeam(name='target')
223 login_celebrity('admin')
224 targetteam.join(superteam, targetteam.teamowner)
225
226 # Now we create a deactivated link for the target team's teamowner.
227 targetteam.teamowner.join(superteam, targetteam.teamowner)
228 targetteam.teamowner.leave(superteam)
229
230 self.assertEqual(
231 sorted([superteam, targetteam]),
232 sorted([team for team in
233 targetteam.teamowner.teams_participated_in]))
234 targetteam.deactivateAllMembers(
235 comment='test',
236 reviewer=targetteam.teamowner)
237 self.assertEqual(
238 [],
239 sorted([team for team in
240 targetteam.teamowner.teams_participated_in]))
241
242 def test_deactivateAllMembers_cleans_up_teamparticipation_teamowner(self):
243 superteam = self.factory.makeTeam(name='super')
244 targetteam = self.factory.makeTeam(name='target')
245 login_celebrity('admin')
246 targetteam.join(superteam, targetteam.teamowner)
247 self.assertEqual(
248 sorted([superteam, targetteam]),
249 sorted([team for team
250 in targetteam.teamowner.teams_participated_in]))
251 targetteam.deactivateAllMembers(
252 comment='test',
253 reviewer=targetteam.teamowner)
254 self.assertEqual(
255 [],
256 sorted([team for team
257 in targetteam.teamowner.teams_participated_in]))
258
219 def test_deactivateAllMembers_cleans_up_team_participation(self):259 def test_deactivateAllMembers_cleans_up_team_participation(self):
220 superteam = self.factory.makeTeam(name='super')260 superteam = self.factory.makeTeam(name='super')
221 sharedteam = self.factory.makeTeam(name='shared')261 sharedteam = self.factory.makeTeam(name='shared')