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
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-08-31 14:00:54 +0000
+++ lib/lp/registry/browser/person.py 2010-09-02 19:32:48 +0000
@@ -293,6 +293,7 @@
293 SSHKeyType,293 SSHKeyType,
294 )294 )
295from lp.registry.interfaces.teammembership import (295from lp.registry.interfaces.teammembership import (
296 CyclicalTeamMembershipError,
296 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,297 DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
297 ITeamMembership,298 ITeamMembership,
298 ITeamMembershipSet,299 ITeamMembershipSet,
@@ -721,11 +722,20 @@
721 _("This invitation has already been processed."))722 _("This invitation has already been processed."))
722 return723 return
723 member = self.context.person724 member = self.context.person
724 member.acceptInvitationToBeMemberOf(725 try:
725 self.context.team, data['acknowledger_comment'])726 member.acceptInvitationToBeMemberOf(
726 self.request.response.addInfoNotification(727 self.context.team, data['acknowledger_comment'])
727 _("This team is now a member of ${team}", mapping=dict(728 except CyclicalTeamMembershipError:
728 team=self.context.team.displayname)))729 self.request.response.addInfoNotification(
730 _("This team may not be added to ${that_team} because it is "
731 "a member of ${this_team}.",
732 mapping=dict(
733 that_team=self.context.team.displayname,
734 this_team=member.displayname)))
735 else:
736 self.request.response.addInfoNotification(
737 _("This team is now a member of ${team}.", mapping=dict(
738 team=self.context.team.displayname)))
729739
730 @action(_("Decline"), name="decline")740 @action(_("Decline"), name="decline")
731 def decline_action(self, action, data):741 def decline_action(self, action, data):
732742
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py 2010-08-27 11:19:54 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2010-09-02 19:32:48 +0000
@@ -19,15 +19,22 @@
19 LaunchpadFunctionalLayer,19 LaunchpadFunctionalLayer,
20 LaunchpadZopelessLayer,20 LaunchpadZopelessLayer,
21 )21 )
22
22from lp.app.errors import NotFoundError23from lp.app.errors import NotFoundError
23from lp.buildmaster.enums import BuildStatus24from lp.buildmaster.enums import BuildStatus
24from lp.registry.browser.person import (25from lp.registry.browser.person import (
25 PersonEditView,26 PersonEditView,
26 PersonView,27 PersonView,
28 TeamInvitationView,
27 )29 )
30
28from lp.registry.interfaces.karma import IKarmaCacheManager31from lp.registry.interfaces.karma import IKarmaCacheManager
29from lp.registry.interfaces.person import PersonVisibility32from lp.registry.interfaces.person import PersonVisibility
30from lp.registry.interfaces.teammembership import TeamMembershipStatus33from lp.registry.interfaces.teammembership import (
34 ITeamMembershipSet,
35 TeamMembershipStatus,
36 )
37
31from lp.registry.model.karma import KarmaCategory38from lp.registry.model.karma import KarmaCategory
32from lp.soyuz.enums import (39from lp.soyuz.enums import (
33 ArchiveStatus,40 ArchiveStatus,
@@ -594,3 +601,85 @@
594 self.assertEqual(1, len(view.errors))601 self.assertEqual(1, len(view.errors))
595 self.assertEqual(602 self.assertEqual(
596 'This account is already deactivated.', view.errors[0])603 'This account is already deactivated.', view.errors[0])
604
605class TestTeamInvitationView(TestCaseWithFactory):
606 """Tests for TeamInvitationView."""
607
608 layer = DatabaseFunctionalLayer
609
610 def setUp(self):
611 super(TestTeamInvitationView, self).setUp()
612 self.a_team = self.factory.makeTeam(name="team-a",
613 displayname="A-Team")
614 self.b_team = self.factory.makeTeam(name="team-b",
615 displayname="B-Team")
616 transaction.commit()
617
618 def test_circular_invite(self):
619 """Two teams can invite each other without horrifying results."""
620
621 # Make the criss-cross invitations.
622 # A invites B.
623 login_person(self.a_team.teamowner)
624 form = {
625 'field.newmember': 'team-b',
626 'field.actions.add': 'Add Member',
627 }
628 view = create_initialized_view(
629 self.a_team, "+addmember", form=form)
630 self.assertEqual([], view.errors)
631 notifications = view.request.response.notifications
632 self.assertEqual(1, len(notifications))
633 self.assertEqual(
634 u'B-Team (team-b) has been invited to join this team.',
635 notifications[0].message)
636
637 # B invites A.
638 login_person(self.b_team.teamowner)
639 form['field.newmember'] = 'team-a'
640 view = create_initialized_view(
641 self.b_team, "+addmember", form=form)
642 self.assertEqual([], view.errors)
643 notifications = view.request.response.notifications
644 self.assertEqual(1, len(notifications))
645 self.assertEqual(
646 u'A-Team (team-a) has been invited to join this team.',
647 notifications[0].message)
648
649 # Team A accepts the invitation.
650 login_person(self.a_team.teamowner)
651 form = {
652 'field.actions.accept': 'Accept',
653 'field.acknowledger_comment': 'Thanks for inviting us.',
654 }
655 request = LaunchpadTestRequest(form=form, method='POST')
656 request.setPrincipal(self.a_team.teamowner)
657 membership_set = getUtility(ITeamMembershipSet)
658 membership = membership_set.getByPersonAndTeam(self.a_team,
659 self.b_team)
660 view = TeamInvitationView(membership, request)
661 view.initialize()
662 self.assertEqual([], view.errors)
663 notifications = view.request.response.notifications
664 self.assertEqual(1, len(notifications))
665 self.assertEqual(
666 u'This team is now a member of B-Team.',
667 notifications[0].message)
668
669 # Team B attempts to accept the invitation.
670 login_person(self.b_team.teamowner)
671 request = LaunchpadTestRequest(form=form, method='POST')
672 request.setPrincipal(self.b_team.teamowner)
673 membership = membership_set.getByPersonAndTeam(self.b_team,
674 self.a_team)
675 view = TeamInvitationView(membership, request)
676 view.initialize()
677 self.assertEqual([], view.errors)
678 notifications = view.request.response.notifications
679 self.assertEqual(1, len(notifications))
680 expected = (
681 u'This team may not be added to A-Team because it is a member '
682 'of B-Team.')
683 self.assertEqual(
684 expected,
685 notifications[0].message)
597686
=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/teammembership.py 2010-09-02 19:32:48 +0000
@@ -56,8 +56,8 @@
56class UserCannotChangeMembershipSilently(Unauthorized):56class UserCannotChangeMembershipSilently(Unauthorized):
57 """User not permitted to change membership status silently.57 """User not permitted to change membership status silently.
5858
59 Raised when a user tries to change someone's membership silently, and is not59 Raised when a user tries to change someone's membership silently, and is
60 a Launchpad Administrator.60 not a Launchpad Administrator.
61 """61 """
62 webservice_error(401) # HTTP Error: 'Unauthorized'62 webservice_error(401) # HTTP Error: 'Unauthorized'
6363
@@ -213,7 +213,8 @@
213213
214 A membership can be renewed if the team's renewal policy is ONDEMAND,214 A membership can be renewed if the team's renewal policy is ONDEMAND,
215 the membership itself is active (status = [ADMIN|APPROVED]) and it's215 the membership itself is active (status = [ADMIN|APPROVED]) and it's
216 set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT days.216 set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT
217 days.
217 """218 """
218219
219 def sendSelfRenewalNotification():220 def sendSelfRenewalNotification():
@@ -241,8 +242,8 @@
241 @operation_parameters(242 @operation_parameters(
242 status=copy_field(status),243 status=copy_field(status),
243 comment=copy_field(reviewer_comment),244 comment=copy_field(reviewer_comment),
244 silent=Bool(title=_("Do not send notifications of status change. For "245 silent=Bool(title=_("Do not send notifications of status change. "
245 "use by Launchpad administrators only."),246 "For use by Launchpad administrators only."),
246 required=False, default=False))247 required=False, default=False))
247 @export_write_operation()248 @export_write_operation()
248 def setStatus(status, user, comment=None, silent=False):249 def setStatus(status, user, comment=None, silent=False):
@@ -326,4 +327,4 @@
326 any cyclical relationships. So if A is a member of B and B is327 any cyclical relationships. So if A is a member of B and B is
327 a member of C then attempting to make C a member of A will328 a member of C then attempting to make C a member of A will
328 result in this error being raised.329 result in this error being raised.
329 """ 330 """
330331
=== modified file 'lib/lp/registry/stories/teammembership/xx-add-member.txt'
--- lib/lp/registry/stories/teammembership/xx-add-member.txt 2010-01-04 22:48:23 +0000
+++ lib/lp/registry/stories/teammembership/xx-add-member.txt 2010-09-02 19:32:48 +0000
@@ -1,4 +1,3 @@
1========================
2Adding members to a team1Adding members to a team
3========================2========================
43
@@ -17,7 +16,8 @@
17 ... print tag.renderContents()16 ... print tag.renderContents()
18 Celso Providelo (cprov) has been added as a member of this team.17 Celso Providelo (cprov) has been added as a member of this team.
1918
20Let's make sure that 'cprov' is now an Approved member of 'landscape-developers'19Let's make sure that 'cprov' is now an Approved member of
20'landscape-developers'.
2121
22 >>> from lp.registry.model.person import Person22 >>> from lp.registry.model.person import Person
23 >>> from lp.registry.model.teammembership import TeamMembership23 >>> from lp.registry.model.teammembership import TeamMembership
@@ -66,7 +66,7 @@
6666
6767
68Adding teams68Adding teams
69============69------------
7070
71Teams are not added as members like we do with people. Instead, teams are71Teams are not added as members like we do with people. Instead, teams are
72invited and one of their admins have to accept the invitation for them to72invited and one of their admins have to accept the invitation for them to
@@ -124,7 +124,7 @@
124rights to edit the membership in question) can do it.124rights to edit the membership in question) can do it.
125125
126 >>> landscape_admin_browser.open(126 >>> landscape_admin_browser.open(
127 ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')127 ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')
128 Traceback (most recent call last):128 Traceback (most recent call last):
129 ...129 ...
130 Unauthorized: ...130 Unauthorized: ...
@@ -149,7 +149,7 @@
149 'http://launchpad.dev/~launchpad'149 'http://launchpad.dev/~launchpad'
150 >>> print extract_text(150 >>> print extract_text(
151 ... find_tags_by_class(browser.contents, 'informational')[0])151 ... find_tags_by_class(browser.contents, 'informational')[0])
152 This team is now a member of Landscape Developers152 This team is now a member of Landscape Developers.
153153
154Now we'll decline the invitation sent on behalf of Ubuntu Team to154Now we'll decline the invitation sent on behalf of Ubuntu Team to
155Warty Security Team:155Warty Security Team:
@@ -164,7 +164,7 @@
164164
165165
166Corner cases166Corner cases
167============167------------
168168
169Given that team can have more than one admin, it's possible that at the time169Given that team can have more than one admin, it's possible that at the time
170one admin is browsing the invitation page, another admin might be doing the170one admin is browsing the invitation page, another admin might be doing the
@@ -204,7 +204,7 @@
204 >>> for tag in find_tags_by_class(browser.contents,204 >>> for tag in find_tags_by_class(browser.contents,
205 ... 'informational message'):205 ... 'informational message'):
206 ... print tag.renderContents()206 ... print tag.renderContents()
207 This team is now a member of Ubuntu Team207 This team is now a member of Ubuntu Team.
208208
209Accepting the invitation in the second browser, redirects to the team page209Accepting the invitation in the second browser, redirects to the team page
210and a message is displayed.210and a message is displayed.
@@ -220,7 +220,7 @@
220220
221221
222Evil workarounds222Evil workarounds
223================223----------------
224224
225One thing to keep in mind is that we can't invite a team without active225One thing to keep in mind is that we can't invite a team without active
226members.226members.
227227
=== added file 'lib/lp/registry/tests/test_add_member.py'
--- lib/lp/registry/tests/test_add_member.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_add_member.py 2010-09-02 19:32:48 +0000
@@ -0,0 +1,43 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Test team membership changes."""
5
6from __future__ import with_statement
7
8__metaclass__ = type
9
10from canonical.testing import DatabaseFunctionalLayer
11from lp.registry.interfaces.teammembership import CyclicalTeamMembershipError
12from lp.testing import (
13 person_logged_in,
14 TestCaseWithFactory,
15 )
16
17
18class CircularMemberAdditionTestCase(TestCaseWithFactory):
19 layer = DatabaseFunctionalLayer
20
21 def setUp(self):
22 super(CircularMemberAdditionTestCase, self).setUp()
23 self.a_team = self.factory.makeTeam(name="a")
24 self.b_team = self.factory.makeTeam(name="b")
25
26 def test_circular_invite(self):
27 """Two teams can invite each other without horrifying results."""
28 # Make the criss-cross invitations.
29 with person_logged_in(self.a_team.teamowner):
30 self.a_team.addMember(self.b_team, self.a_team.teamowner)
31 with person_logged_in(self.b_team.teamowner):
32 self.b_team.addMember(self.a_team, self.b_team.teamowner)
33
34 # A-team accepts B's kind invitation.
35 with person_logged_in(self.a_team.teamowner):
36 self.a_team.acceptInvitationToBeMemberOf(
37 self.b_team, None)
38 # B-team accepts A's kind invitation.
39 with person_logged_in(self.b_team.teamowner):
40 self.assertRaises(
41 CyclicalTeamMembershipError,
42 self.b_team.acceptInvitationToBeMemberOf,
43 self.a_team, None)