Merge lp:~abentley/launchpad/team-transition-error into lp:launchpad

Proposed by Aaron Bentley
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 13816
Proposed branch: lp:~abentley/launchpad/team-transition-error
Merge into: lp:launchpad
Diff against target: 233 lines (+100/-12)
4 files modified
lib/lp/registry/browser/tests/test_team.py (+18/-1)
lib/lp/registry/model/person.py (+18/-3)
lib/lp/registry/model/teammembership.py (+1/-1)
lib/lp/registry/tests/test_teammembership.py (+63/-7)
To merge this branch: bzr merge lp:~abentley/launchpad/team-transition-error
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+73103@code.launchpad.net

Commit message

Fix TeamMembershipTransitionError moving from DECLINED to INVITED

Description of the change

= Summary =
Fix bug #820077: TeamMembershipTransitionError: Bad state transition from DECLINED to INVITED

== Proposed fix ==
Bless transition from DECLINED to INVITED

== Pre-implementation notes ==
Discussed with sinzui

== Implementation details ==
As well as fixing the particular error, I've tested that TeamMemberAddView.add_action cannot raise TeamMembershipTransitionError at all, so there's no need to handle that exception in browser code.

This meant turning PROPOSED to APPROVED if the reviewer was the an admin of the team, instead of changing it to INVITED.

It also meant preventing addMember from downgrading APPROVED/ADMIN to PROPOSED. And while I was there, I prevented downgrading ADMIN to APPROVED.

== Tests ==
bin/test -t test_no_TeamMembershipTransitionError -t test_declined_member_can_be_invited -t test_add_approved -t test_add_admin -t test_implicit_approval

== Demo and Q/A ==
Create team A and team B with different owners. As the owner of team A, attempt to join team B. As the owner of team B, decline the membership. As the owner of team B, attempt to add team A as a member. This should not OOPS.

= Launchpad lint =

Checking for conflicts and issues in changed files.

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

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

As we discussed on IRC, this branch also fixes bug 480157.

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/browser/tests/test_team.py'
2--- lib/lp/registry/browser/tests/test_team.py 2011-04-09 02:23:46 +0000
3+++ lib/lp/registry/browser/tests/test_team.py 2011-08-29 13:54:49 +0000
4@@ -4,11 +4,16 @@
5 __metaclass__ = type
6
7 from zope.component import getUtility
8+from zope.security.proxy import removeSecurityProxy
9
10 from canonical.launchpad.webapp.publisher import canonical_url
11 from canonical.testing.layers import DatabaseFunctionalLayer
12 from lp.registry.browser.person import TeamOverviewMenu
13 from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
14+from lp.registry.interfaces.teammembership import (
15+ ITeamMembershipSet,
16+ TeamMembershipStatus,
17+ )
18 from lp.testing import (
19 login_person,
20 TestCaseWithFactory,
21@@ -42,7 +47,7 @@
22 self.assertEqual('Create a mailing list', link.text)
23
24 def test_TeamOverviewMenu_check_menu_links_with_mailing(self):
25- mailing_list = self.factory.makeMailingList(
26+ self.factory.makeMailingList(
27 self.team, self.team.teamowner)
28 menu = TeamOverviewMenu(self.team)
29 self.assertEqual(True, check_menu_links(menu))
30@@ -139,6 +144,18 @@
31 "You can't add a team that doesn't have any active members.",
32 view.errors[0])
33
34+ def test_no_TeamMembershipTransitionError(self):
35+ # Attempting to add a team never triggers a
36+ # TeamMembershipTransitionError
37+ member_team = self.factory.makeTeam()
38+ self.team.addMember(member_team, self.team.teamowner)
39+ tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
40+ member_team, self.team)
41+ for status in TeamMembershipStatus.items:
42+ removeSecurityProxy(tm).status = status
43+ view = create_initialized_view(self.team, "+addmember")
44+ view.add_action.success(data={'newmember': member_team})
45+
46
47 class TestTeamIndexView(TestCaseWithFactory):
48
49
50=== modified file 'lib/lp/registry/model/person.py'
51--- lib/lp/registry/model/person.py 2011-08-25 03:30:40 +0000
52+++ lib/lp/registry/model/person.py 2011-08-29 13:54:49 +0000
53@@ -1455,6 +1455,12 @@
54 "You can't add a member with this status: %s." % status.name)
55
56 event = JoinTeamEvent
57+ tm = TeamMembership.selectOneBy(person=person, team=self)
58+ if tm is not None:
59+ if tm.status == TeamMembershipStatus.ADMIN or (
60+ tm.status == TeamMembershipStatus.APPROVED and status ==
61+ TeamMembershipStatus.PROPOSED):
62+ status = tm.status
63 if person.is_team:
64 assert not self.hasParticipationEntryFor(person), (
65 "Team '%s' is a member of '%s'. As a consequence, '%s' can't "
66@@ -1468,12 +1474,21 @@
67 is_reviewer_admin_of_new_member = (
68 person in reviewer.getAdministratedTeams())
69 if not force_team_add and not is_reviewer_admin_of_new_member:
70- status = TeamMembershipStatus.INVITED
71- event = TeamInvitationEvent
72+ if tm is None or tm.status not in (
73+ TeamMembershipStatus.PROPOSED,
74+ TeamMembershipStatus.APPROVED,
75+ TeamMembershipStatus.ADMIN,
76+ ):
77+ status = TeamMembershipStatus.INVITED
78+ event = TeamInvitationEvent
79+ else:
80+ if tm.status == TeamMembershipStatus.PROPOSED:
81+ status = TeamMembershipStatus.APPROVED
82+ else:
83+ status = tm.status
84
85 status_changed = True
86 expires = self.defaultexpirationdate
87- tm = TeamMembership.selectOneBy(person=person, team=self)
88 if tm is None:
89 tm = TeamMembershipSet().new(
90 person, self, status, reviewer, dateexpires=expires,
91
92=== modified file 'lib/lp/registry/model/teammembership.py'
93--- lib/lp/registry/model/teammembership.py 2011-08-12 14:39:51 +0000
94+++ lib/lp/registry/model/teammembership.py 2011-08-29 13:54:49 +0000
95@@ -338,7 +338,7 @@
96 deactivated: [proposed, approved, admin, invited],
97 expired: [proposed, approved, admin, invited],
98 proposed: [approved, admin, declined],
99- declined: [proposed, approved, admin],
100+ declined: [proposed, approved, admin, invited],
101 invited: [approved, admin, invitation_declined],
102 invitation_declined: [invited, approved, admin]}
103
104
105=== modified file 'lib/lp/registry/tests/test_teammembership.py'
106--- lib/lp/registry/tests/test_teammembership.py 2011-05-27 21:12:25 +0000
107+++ lib/lp/registry/tests/test_teammembership.py 2011-08-29 13:54:49 +0000
108@@ -315,7 +315,7 @@
109 ['name16', 'no-priv', 'team5'], self.team4)
110 self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
111 self.assertEqual(
112- previous_count-8,
113+ previous_count - 8,
114 self.getTeamParticipationCount())
115
116 def testRemovingLeafTeam(self):
117@@ -333,7 +333,7 @@
118 self.assertParticipantsEquals(['name16'], self.team4)
119 self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
120 self.assertEqual(
121- previous_count-8,
122+ previous_count - 8,
123 self.getTeamParticipationCount())
124
125
126@@ -393,7 +393,7 @@
127 ['name16', 'no-priv', 'team5'], self.team4)
128 self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
129 self.assertEqual(
130- previous_count-4,
131+ previous_count - 4,
132 self.getTeamParticipationCount())
133
134 def testRemoveTeam5FromTeam4(self):
135@@ -410,7 +410,7 @@
136 self.assertParticipantsEquals(['name16'], self.team4)
137 self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
138 self.assertEqual(
139- previous_count-4,
140+ previous_count - 4,
141 self.getTeamParticipationCount())
142
143
144@@ -512,7 +512,7 @@
145 self.assertParticipantsEquals(
146 ['name16', 'no-priv', 'team2', 'team4', 'team5'], self.team6)
147 self.assertEqual(
148- previous_count-3,
149+ previous_count - 3,
150 self.getTeamParticipationCount())
151
152 def testRemoveTeam5FromTeam4(self):
153@@ -529,7 +529,7 @@
154 self.assertParticipantsEquals(
155 ['name16', 'team2', 'team3', 'team4'], self.team6)
156 self.assertEqual(
157- previous_count-10,
158+ previous_count - 10,
159 self.getTeamParticipationCount())
160
161 def testTeam3_deactivateActiveMemberships(self):
162@@ -576,7 +576,7 @@
163 """
164 login('test@canonical.com')
165 person = self.factory.makePerson()
166- login_person(person) # Now login with the future owner of the teams.
167+ login_person(person) # Now login with the future owner of the teams.
168 teamA = self.factory.makeTeam(
169 person, subscription_policy=TeamSubscriptionPolicy.MODERATED)
170 teamB = self.factory.makeTeam(
171@@ -837,6 +837,62 @@
172 TeamMembershipStatus.INVITATION_DECLINED, self.team2.teamowner)
173 self.assertEqual(TeamMembershipStatus.INVITATION_DECLINED, tm.status)
174
175+ def test_declined_member_can_be_invited(self):
176+ # A team can re-invite a declined member.
177+ self.team2.addMember(
178+ self.team1, self.no_priv, status=TeamMembershipStatus.PROPOSED,
179+ force_team_add=True)
180+ tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
181+ self.team1, self.team2)
182+ tm.setStatus(
183+ TeamMembershipStatus.DECLINED, self.team1.teamowner)
184+ tm.setStatus(
185+ TeamMembershipStatus.INVITED, self.team1.teamowner)
186+ self.assertEqual(TeamMembershipStatus.INVITED, tm.status)
187+
188+ def test_add_approved(self):
189+ # Adding an approved team is a no-op.
190+ member_team = self.factory.makeTeam()
191+ self.team1.addMember(
192+ member_team, self.team1.teamowner)
193+ with person_logged_in(member_team.teamowner):
194+ member_team.acceptInvitationToBeMemberOf(self.team1, 'alright')
195+ self.team1.addMember(
196+ member_team, self.team1.teamowner)
197+ tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
198+ member_team, self.team1)
199+ self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
200+ self.team1.addMember(
201+ member_team, member_team.teamowner)
202+ self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
203+
204+ def test_add_admin(self):
205+ # Adding an admin team is a no-op.
206+ member_team = self.factory.makeTeam()
207+ self.team1.addMember(
208+ member_team, self.team1.teamowner,
209+ status=TeamMembershipStatus.ADMIN, force_team_add=True)
210+ self.team1.addMember(
211+ member_team, self.team1.teamowner)
212+ tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
213+ member_team, self.team1)
214+ self.assertEqual(TeamMembershipStatus.ADMIN, tm.status)
215+ self.team1.addMember(
216+ member_team, member_team.teamowner)
217+ self.assertEqual(TeamMembershipStatus.ADMIN, tm.status)
218+
219+ def test_implicit_approval(self):
220+ # Inviting a proposed person is an implicit approval.
221+ member_team = self.factory.makeTeam()
222+ self.team1.addMember(
223+ member_team, self.team1.teamowner,
224+ status=TeamMembershipStatus.PROPOSED, force_team_add=True)
225+ self.team1.addMember(
226+ member_team, self.team1.teamowner)
227+ tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
228+ member_team, self.team1)
229+ self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
230+
231 def test_retractTeamMembership_invited(self):
232 # A team can retract a membership invitation.
233 self.team2.addMember(self.team1, self.no_priv)