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
1=== modified file 'lib/lp/registry/interfaces/teammembership.py'
2--- lib/lp/registry/interfaces/teammembership.py 2010-09-29 14:38:30 +0000
3+++ lib/lp/registry/interfaces/teammembership.py 2010-12-13 13:35:15 +0000
4@@ -8,6 +8,7 @@
5 __metaclass__ = type
6
7 __all__ = [
8+ 'ACTIVE_STATES',
9 'CyclicalTeamMembershipError',
10 'DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT',
11 'ITeamMembership',
12@@ -111,6 +112,9 @@
13 """)
14
15
16+ACTIVE_STATES = [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
17+
18+
19 class ITeamMembership(Interface):
20 """TeamMembership for Users"""
21 export_as_webservice_entry()
22
23=== modified file 'lib/lp/registry/model/person.py'
24--- lib/lp/registry/model/person.py 2010-12-10 07:52:21 +0000
25+++ lib/lp/registry/model/person.py 2010-12-13 13:35:15 +0000
26@@ -232,7 +232,10 @@
27 SSHKeyCompromisedError,
28 SSHKeyType,
29 )
30-from lp.registry.interfaces.teammembership import TeamMembershipStatus
31+from lp.registry.interfaces.teammembership import (
32+ ACTIVE_STATES,
33+ TeamMembershipStatus,
34+ )
35 from lp.registry.interfaces.wikiname import (
36 IWikiName,
37 IWikiNameSet,
38@@ -1473,7 +1476,6 @@
39 # Since we've updated the database behind Storm's back,
40 # flush its caches.
41 store.invalidate()
42-
43
44 # Remove all indirect TeamParticipation entries resulting from this
45 # team. If this were just a select, it would be a complicated but
46@@ -1499,8 +1501,9 @@
47 (SELECT tm1.person from TeamMembership tm1
48 WHERE
49 tm1.person = TeamParticipation.person and
50- tm1.team = TeamParticipation.team);
51- ''', dict(team=self.id))
52+ tm1.team = TeamParticipation.team and
53+ tm1.status IN %(active_states)s);
54+ ''', dict(team=self.id, active_states=ACTIVE_STATES))
55
56 # Since we've updated the database behind Storm's back yet again,
57 # we need to flush its caches, again.
58
59=== modified file 'lib/lp/registry/model/teammembership.py'
60--- lib/lp/registry/model/teammembership.py 2010-10-05 01:55:03 +0000
61+++ lib/lp/registry/model/teammembership.py 2010-12-13 13:35:15 +0000
62@@ -60,6 +60,7 @@
63 IMembershipNotificationJobSource,
64 )
65 from lp.registry.interfaces.teammembership import (
66+ ACTIVE_STATES,
67 CyclicalTeamMembershipError,
68 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
69 ITeamMembership,
70@@ -69,9 +70,6 @@
71 )
72
73
74-ACTIVE_STATES = [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
75-
76-
77 class TeamMembership(SQLBase):
78 """See `ITeamMembership`."""
79
80
81=== modified file 'lib/lp/registry/tests/test_team.py'
82--- lib/lp/registry/tests/test_team.py 2010-12-01 17:07:49 +0000
83+++ lib/lp/registry/tests/test_team.py 2010-12-13 13:35:15 +0000
84@@ -216,6 +216,46 @@
85
86 layer = DatabaseFunctionalLayer
87
88+ def test_deactivateAllMembers_cleans_up_teamparticipation_deactivated(
89+ self):
90+ superteam = self.factory.makeTeam(name='super')
91+ targetteam = self.factory.makeTeam(name='target')
92+ login_celebrity('admin')
93+ targetteam.join(superteam, targetteam.teamowner)
94+
95+ # Now we create a deactivated link for the target team's teamowner.
96+ targetteam.teamowner.join(superteam, targetteam.teamowner)
97+ targetteam.teamowner.leave(superteam)
98+
99+ self.assertEqual(
100+ sorted([superteam, targetteam]),
101+ sorted([team for team in
102+ targetteam.teamowner.teams_participated_in]))
103+ targetteam.deactivateAllMembers(
104+ comment='test',
105+ reviewer=targetteam.teamowner)
106+ self.assertEqual(
107+ [],
108+ sorted([team for team in
109+ targetteam.teamowner.teams_participated_in]))
110+
111+ def test_deactivateAllMembers_cleans_up_teamparticipation_teamowner(self):
112+ superteam = self.factory.makeTeam(name='super')
113+ targetteam = self.factory.makeTeam(name='target')
114+ login_celebrity('admin')
115+ targetteam.join(superteam, targetteam.teamowner)
116+ self.assertEqual(
117+ sorted([superteam, targetteam]),
118+ sorted([team for team
119+ in targetteam.teamowner.teams_participated_in]))
120+ targetteam.deactivateAllMembers(
121+ comment='test',
122+ reviewer=targetteam.teamowner)
123+ self.assertEqual(
124+ [],
125+ sorted([team for team
126+ in targetteam.teamowner.teams_participated_in]))
127+
128 def test_deactivateAllMembers_cleans_up_team_participation(self):
129 superteam = self.factory.makeTeam(name='super')
130 sharedteam = self.factory.makeTeam(name='shared')