Launchpad itself

Merge lp:~bac/launchpad/bug-154587 into lp:launchpad

  1. bug-154587
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11492
Proposed branch: lp:~bac/launchpad/bug-154587
Merge into: lp:launchpad
Diff against target: 314 lines (+163/-20)
5 files modified
lib/lp/registry/browser/person.py (+15/-5)
lib/lp/registry/browser/tests/test_person_view.py (+90/-1)
lib/lp/registry/interfaces/teammembership.py (+7/-6)
lib/lp/registry/stories/teammembership/xx-add-member.txt (+8/-8)
lib/lp/registry/tests/test_add_member.py (+43/-0)
To merge this branch: bzr merge lp:~bac/launchpad/bug-154587
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Review via email: mp+34401@code.launchpad.net

Commit message

Catch CyclicalTeamMembershipError when accepting invitations and report error.

Description of the change

= Summary =

The problem reported in bug 154587 no longer exists as written. When
cycles in team membership are about to be introduced they are detected
and an appropriate error is raised, rather than spiraling down to the
database.

Unfortunately that error was not caught in the UI so the user still
experiences an OOPS.

== Proposed fix ==

Catch the error and display an appropriate message.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t xx-add-member.txt -t test_add_member

== Demo and Q/A ==

Follow the scenario listed in the bug, though it is somewhat difficult
given our sampledata.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/stories/teammembership/xx-add-member.txt
  lib/lp/registry/interfaces/teammembership.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/tests/test_add_member.py

To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Thanks for all the tidying up too Brad.

This looks great, and I've approved it, but out of interest, was wondering if there was a reason why you added to the doctest rather than putting this in lp.registry.browser.tests.test_person_view?

Thanks!

review: Approve (code)
Revision history for this message
Brad Crittenden (bac) wrote :

Thanks Michael. I'll look at moving it out of the story to the view test...which probably would be more appropriate.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/person.py'
2--- lib/lp/registry/browser/person.py 2010-08-31 14:00:54 +0000
3+++ lib/lp/registry/browser/person.py 2010-09-02 19:32:48 +0000
4@@ -293,6 +293,7 @@
5 SSHKeyType,
6 )
7 from lp.registry.interfaces.teammembership import (
8+ CyclicalTeamMembershipError,
9 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
10 ITeamMembership,
11 ITeamMembershipSet,
12@@ -721,11 +722,20 @@
13 _("This invitation has already been processed."))
14 return
15 member = self.context.person
16- member.acceptInvitationToBeMemberOf(
17- self.context.team, data['acknowledger_comment'])
18- self.request.response.addInfoNotification(
19- _("This team is now a member of ${team}", mapping=dict(
20- team=self.context.team.displayname)))
21+ try:
22+ member.acceptInvitationToBeMemberOf(
23+ self.context.team, data['acknowledger_comment'])
24+ except CyclicalTeamMembershipError:
25+ self.request.response.addInfoNotification(
26+ _("This team may not be added to ${that_team} because it is "
27+ "a member of ${this_team}.",
28+ mapping=dict(
29+ that_team=self.context.team.displayname,
30+ this_team=member.displayname)))
31+ else:
32+ self.request.response.addInfoNotification(
33+ _("This team is now a member of ${team}.", mapping=dict(
34+ team=self.context.team.displayname)))
35
36 @action(_("Decline"), name="decline")
37 def decline_action(self, action, data):
38
39=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
40--- lib/lp/registry/browser/tests/test_person_view.py 2010-08-27 11:19:54 +0000
41+++ lib/lp/registry/browser/tests/test_person_view.py 2010-09-02 19:32:48 +0000
42@@ -19,15 +19,22 @@
43 LaunchpadFunctionalLayer,
44 LaunchpadZopelessLayer,
45 )
46+
47 from lp.app.errors import NotFoundError
48 from lp.buildmaster.enums import BuildStatus
49 from lp.registry.browser.person import (
50 PersonEditView,
51 PersonView,
52+ TeamInvitationView,
53 )
54+
55 from lp.registry.interfaces.karma import IKarmaCacheManager
56 from lp.registry.interfaces.person import PersonVisibility
57-from lp.registry.interfaces.teammembership import TeamMembershipStatus
58+from lp.registry.interfaces.teammembership import (
59+ ITeamMembershipSet,
60+ TeamMembershipStatus,
61+ )
62+
63 from lp.registry.model.karma import KarmaCategory
64 from lp.soyuz.enums import (
65 ArchiveStatus,
66@@ -594,3 +601,85 @@
67 self.assertEqual(1, len(view.errors))
68 self.assertEqual(
69 'This account is already deactivated.', view.errors[0])
70+
71+class TestTeamInvitationView(TestCaseWithFactory):
72+ """Tests for TeamInvitationView."""
73+
74+ layer = DatabaseFunctionalLayer
75+
76+ def setUp(self):
77+ super(TestTeamInvitationView, self).setUp()
78+ self.a_team = self.factory.makeTeam(name="team-a",
79+ displayname="A-Team")
80+ self.b_team = self.factory.makeTeam(name="team-b",
81+ displayname="B-Team")
82+ transaction.commit()
83+
84+ def test_circular_invite(self):
85+ """Two teams can invite each other without horrifying results."""
86+
87+ # Make the criss-cross invitations.
88+ # A invites B.
89+ login_person(self.a_team.teamowner)
90+ form = {
91+ 'field.newmember': 'team-b',
92+ 'field.actions.add': 'Add Member',
93+ }
94+ view = create_initialized_view(
95+ self.a_team, "+addmember", form=form)
96+ self.assertEqual([], view.errors)
97+ notifications = view.request.response.notifications
98+ self.assertEqual(1, len(notifications))
99+ self.assertEqual(
100+ u'B-Team (team-b) has been invited to join this team.',
101+ notifications[0].message)
102+
103+ # B invites A.
104+ login_person(self.b_team.teamowner)
105+ form['field.newmember'] = 'team-a'
106+ view = create_initialized_view(
107+ self.b_team, "+addmember", form=form)
108+ self.assertEqual([], view.errors)
109+ notifications = view.request.response.notifications
110+ self.assertEqual(1, len(notifications))
111+ self.assertEqual(
112+ u'A-Team (team-a) has been invited to join this team.',
113+ notifications[0].message)
114+
115+ # Team A accepts the invitation.
116+ login_person(self.a_team.teamowner)
117+ form = {
118+ 'field.actions.accept': 'Accept',
119+ 'field.acknowledger_comment': 'Thanks for inviting us.',
120+ }
121+ request = LaunchpadTestRequest(form=form, method='POST')
122+ request.setPrincipal(self.a_team.teamowner)
123+ membership_set = getUtility(ITeamMembershipSet)
124+ membership = membership_set.getByPersonAndTeam(self.a_team,
125+ self.b_team)
126+ view = TeamInvitationView(membership, request)
127+ view.initialize()
128+ self.assertEqual([], view.errors)
129+ notifications = view.request.response.notifications
130+ self.assertEqual(1, len(notifications))
131+ self.assertEqual(
132+ u'This team is now a member of B-Team.',
133+ notifications[0].message)
134+
135+ # Team B attempts to accept the invitation.
136+ login_person(self.b_team.teamowner)
137+ request = LaunchpadTestRequest(form=form, method='POST')
138+ request.setPrincipal(self.b_team.teamowner)
139+ membership = membership_set.getByPersonAndTeam(self.b_team,
140+ self.a_team)
141+ view = TeamInvitationView(membership, request)
142+ view.initialize()
143+ self.assertEqual([], view.errors)
144+ notifications = view.request.response.notifications
145+ self.assertEqual(1, len(notifications))
146+ expected = (
147+ u'This team may not be added to A-Team because it is a member '
148+ 'of B-Team.')
149+ self.assertEqual(
150+ expected,
151+ notifications[0].message)
152
153=== modified file 'lib/lp/registry/interfaces/teammembership.py'
154--- lib/lp/registry/interfaces/teammembership.py 2010-08-20 20:31:18 +0000
155+++ lib/lp/registry/interfaces/teammembership.py 2010-09-02 19:32:48 +0000
156@@ -56,8 +56,8 @@
157 class UserCannotChangeMembershipSilently(Unauthorized):
158 """User not permitted to change membership status silently.
159
160- Raised when a user tries to change someone's membership silently, and is not
161- a Launchpad Administrator.
162+ Raised when a user tries to change someone's membership silently, and is
163+ not a Launchpad Administrator.
164 """
165 webservice_error(401) # HTTP Error: 'Unauthorized'
166
167@@ -213,7 +213,8 @@
168
169 A membership can be renewed if the team's renewal policy is ONDEMAND,
170 the membership itself is active (status = [ADMIN|APPROVED]) and it's
171- set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT days.
172+ set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT
173+ days.
174 """
175
176 def sendSelfRenewalNotification():
177@@ -241,8 +242,8 @@
178 @operation_parameters(
179 status=copy_field(status),
180 comment=copy_field(reviewer_comment),
181- silent=Bool(title=_("Do not send notifications of status change. For "
182- "use by Launchpad administrators only."),
183+ silent=Bool(title=_("Do not send notifications of status change. "
184+ "For use by Launchpad administrators only."),
185 required=False, default=False))
186 @export_write_operation()
187 def setStatus(status, user, comment=None, silent=False):
188@@ -326,4 +327,4 @@
189 any cyclical relationships. So if A is a member of B and B is
190 a member of C then attempting to make C a member of A will
191 result in this error being raised.
192- """
193+ """
194
195=== modified file 'lib/lp/registry/stories/teammembership/xx-add-member.txt'
196--- lib/lp/registry/stories/teammembership/xx-add-member.txt 2010-01-04 22:48:23 +0000
197+++ lib/lp/registry/stories/teammembership/xx-add-member.txt 2010-09-02 19:32:48 +0000
198@@ -1,4 +1,3 @@
199-========================
200 Adding members to a team
201 ========================
202
203@@ -17,7 +16,8 @@
204 ... print tag.renderContents()
205 Celso Providelo (cprov) has been added as a member of this team.
206
207-Let's make sure that 'cprov' is now an Approved member of 'landscape-developers'
208+Let's make sure that 'cprov' is now an Approved member of
209+'landscape-developers'.
210
211 >>> from lp.registry.model.person import Person
212 >>> from lp.registry.model.teammembership import TeamMembership
213@@ -66,7 +66,7 @@
214
215
216 Adding teams
217-============
218+------------
219
220 Teams are not added as members like we do with people. Instead, teams are
221 invited and one of their admins have to accept the invitation for them to
222@@ -124,7 +124,7 @@
223 rights to edit the membership in question) can do it.
224
225 >>> landscape_admin_browser.open(
226- ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')
227+ ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')
228 Traceback (most recent call last):
229 ...
230 Unauthorized: ...
231@@ -149,7 +149,7 @@
232 'http://launchpad.dev/~launchpad'
233 >>> print extract_text(
234 ... find_tags_by_class(browser.contents, 'informational')[0])
235- This team is now a member of Landscape Developers
236+ This team is now a member of Landscape Developers.
237
238 Now we'll decline the invitation sent on behalf of Ubuntu Team to
239 Warty Security Team:
240@@ -164,7 +164,7 @@
241
242
243 Corner cases
244-============
245+------------
246
247 Given that team can have more than one admin, it's possible that at the time
248 one admin is browsing the invitation page, another admin might be doing the
249@@ -204,7 +204,7 @@
250 >>> for tag in find_tags_by_class(browser.contents,
251 ... 'informational message'):
252 ... print tag.renderContents()
253- This team is now a member of Ubuntu Team
254+ This team is now a member of Ubuntu Team.
255
256 Accepting the invitation in the second browser, redirects to the team page
257 and a message is displayed.
258@@ -220,7 +220,7 @@
259
260
261 Evil workarounds
262-================
263+----------------
264
265 One thing to keep in mind is that we can't invite a team without active
266 members.
267
268=== added file 'lib/lp/registry/tests/test_add_member.py'
269--- lib/lp/registry/tests/test_add_member.py 1970-01-01 00:00:00 +0000
270+++ lib/lp/registry/tests/test_add_member.py 2010-09-02 19:32:48 +0000
271@@ -0,0 +1,43 @@
272+# Copyright 2010 Canonical Ltd. This software is licensed under the
273+# GNU Affero General Public License version 3 (see the file LICENSE).
274+
275+"""Test team membership changes."""
276+
277+from __future__ import with_statement
278+
279+__metaclass__ = type
280+
281+from canonical.testing import DatabaseFunctionalLayer
282+from lp.registry.interfaces.teammembership import CyclicalTeamMembershipError
283+from lp.testing import (
284+ person_logged_in,
285+ TestCaseWithFactory,
286+ )
287+
288+
289+class CircularMemberAdditionTestCase(TestCaseWithFactory):
290+ layer = DatabaseFunctionalLayer
291+
292+ def setUp(self):
293+ super(CircularMemberAdditionTestCase, self).setUp()
294+ self.a_team = self.factory.makeTeam(name="a")
295+ self.b_team = self.factory.makeTeam(name="b")
296+
297+ def test_circular_invite(self):
298+ """Two teams can invite each other without horrifying results."""
299+ # Make the criss-cross invitations.
300+ with person_logged_in(self.a_team.teamowner):
301+ self.a_team.addMember(self.b_team, self.a_team.teamowner)
302+ with person_logged_in(self.b_team.teamowner):
303+ self.b_team.addMember(self.a_team, self.b_team.teamowner)
304+
305+ # A-team accepts B's kind invitation.
306+ with person_logged_in(self.a_team.teamowner):
307+ self.a_team.acceptInvitationToBeMemberOf(
308+ self.b_team, None)
309+ # B-team accepts A's kind invitation.
310+ with person_logged_in(self.b_team.teamowner):
311+ self.assertRaises(
312+ CyclicalTeamMembershipError,
313+ self.b_team.acceptInvitationToBeMemberOf,
314+ self.a_team, None)