launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00839
[Merge] lp:~bac/launchpad/bug-154587 into lp:launchpad/devel
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-154587 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#154587 Inifinite loop in team invitation code
https://bugs.launchpad.net/bugs/154587
= 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
--
https://code.launchpad.net/~bac/launchpad/bug-154587/+merge/34401
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-154587 into lp:launchpad/devel.
=== 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 11:06:09 +0000
@@ -293,6 +293,7 @@
SSHKeyType,
)
from lp.registry.interfaces.teammembership import (
+ CyclicalTeamMembershipError,
DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
ITeamMembership,
ITeamMembershipSet,
@@ -721,11 +722,20 @@
_("This invitation has already been processed."))
return
member = self.context.person
- member.acceptInvitationToBeMemberOf(
- self.context.team, data['acknowledger_comment'])
- self.request.response.addInfoNotification(
- _("This team is now a member of ${team}", mapping=dict(
- team=self.context.team.displayname)))
+ try:
+ member.acceptInvitationToBeMemberOf(
+ self.context.team, data['acknowledger_comment'])
+ except CyclicalTeamMembershipError:
+ self.request.response.addInfoNotification(
+ _("This team may not be added to ${that_team} because it is "
+ "a member of ${this_team}.",
+ mapping=dict(
+ that_team=self.context.team.displayname,
+ this_team=member.displayname)))
+ else:
+ self.request.response.addInfoNotification(
+ _("This team is now a member of ${team}.", mapping=dict(
+ team=self.context.team.displayname)))
@action(_("Decline"), name="decline")
def decline_action(self, action, data):
=== 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 11:06:09 +0000
@@ -56,8 +56,8 @@
class UserCannotChangeMembershipSilently(Unauthorized):
"""User not permitted to change membership status silently.
- Raised when a user tries to change someone's membership silently, and is not
- a Launchpad Administrator.
+ Raised when a user tries to change someone's membership silently, and is
+ not a Launchpad Administrator.
"""
webservice_error(401) # HTTP Error: 'Unauthorized'
@@ -213,7 +213,8 @@
A membership can be renewed if the team's renewal policy is ONDEMAND,
the membership itself is active (status = [ADMIN|APPROVED]) and it's
- set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT days.
+ set to expire in less than DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT
+ days.
"""
def sendSelfRenewalNotification():
@@ -241,8 +242,8 @@
@operation_parameters(
status=copy_field(status),
comment=copy_field(reviewer_comment),
- silent=Bool(title=_("Do not send notifications of status change. For "
- "use by Launchpad administrators only."),
+ silent=Bool(title=_("Do not send notifications of status change. "
+ "For use by Launchpad administrators only."),
required=False, default=False))
@export_write_operation()
def setStatus(status, user, comment=None, silent=False):
@@ -326,4 +327,4 @@
any cyclical relationships. So if A is a member of B and B is
a member of C then attempting to make C a member of A will
result in this error being raised.
- """
+ """
=== 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 11:06:09 +0000
@@ -1,4 +1,3 @@
-========================
Adding members to a team
========================
@@ -17,7 +16,8 @@
... print tag.renderContents()
Celso Providelo (cprov) has been added as a member of this team.
-Let's make sure that 'cprov' is now an Approved member of 'landscape-developers'
+Let's make sure that 'cprov' is now an Approved member of
+'landscape-developers'.
>>> from lp.registry.model.person import Person
>>> from lp.registry.model.teammembership import TeamMembership
@@ -66,7 +66,7 @@
Adding teams
-============
+------------
Teams are not added as members like we do with people. Instead, teams are
invited and one of their admins have to accept the invitation for them to
@@ -124,7 +124,7 @@
rights to edit the membership in question) can do it.
>>> landscape_admin_browser.open(
- ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')
+ ... 'http://launchpad.dev/~launchpad/+invitation/landscape-developers')
Traceback (most recent call last):
...
Unauthorized: ...
@@ -149,7 +149,7 @@
'http://launchpad.dev/~launchpad'
>>> print extract_text(
... find_tags_by_class(browser.contents, 'informational')[0])
- This team is now a member of Landscape Developers
+ This team is now a member of Landscape Developers.
Now we'll decline the invitation sent on behalf of Ubuntu Team to
Warty Security Team:
@@ -164,7 +164,7 @@
Corner cases
-============
+------------
Given that team can have more than one admin, it's possible that at the time
one admin is browsing the invitation page, another admin might be doing the
@@ -204,7 +204,7 @@
>>> for tag in find_tags_by_class(browser.contents,
... 'informational message'):
... print tag.renderContents()
- This team is now a member of Ubuntu Team
+ This team is now a member of Ubuntu Team.
Accepting the invitation in the second browser, redirects to the team page
and a message is displayed.
@@ -220,7 +220,7 @@
Evil workarounds
-================
+----------------
One thing to keep in mind is that we can't invite a team without active
members.
@@ -250,3 +250,58 @@
... print tag.renderContents()
There is 1 error.
You can't add a team that doesn't have any active members.
+
+
+Avoiding cycles in teams
+------------------------
+
+If two teams invite each other to join, the first one can accept the
+invitation but then attempting to accept the second invitation fails
+with a cyclical team membership error, which is reported to the team
+admin.
+
+ >>> from zope.component import getUtility
+ >>> from canonical.launchpad.interfaces import IPersonSet
+ >>> person_set = getUtility(IPersonSet)
+ >>> login(ANONYMOUS)
+ >>> stevea = person_set.getByName('stevea')
+ >>> team_a = factory.makeTeam(name="team-a", displayname="A Team",
+ ... owner=stevea)
+
+ >>> jdub = person_set.getByName('jdub')
+ >>> team_b = factory.makeTeam(name="team-b", displayname="B Team",
+ ... owner=jdub)
+ >>> logout()
+
+ >>> stevea_browser = setupBrowser(
+ ... auth='Basic steve.alexander@xxxxxxxxxxxxxxx:test')
+ >>> jdub_browser = setupBrowser(
+ ... auth='Basic jeff.waugh@xxxxxxxxxxxxxxx:jdub')
+
+Each team invites the other to become a member.
+
+ >>> stevea_browser.open('http://launchpad.dev/~team-a/+addmember')
+ >>> stevea_browser.getControl('New member').value = 'team-b'
+ >>> stevea_browser.getControl('Add Member').click()
+ >>> for message in get_feedback_messages(stevea_browser.contents):
+ ... print message
+ B Team (team-b) has been invited to join this team.
+
+ >>> jdub_browser.open('http://launchpad.dev/~team-b/+addmember')
+ >>> jdub_browser.getControl('New member').value = 'team-a'
+ >>> jdub_browser.getControl('Add Member').click()
+ >>> for message in get_feedback_messages(jdub_browser.contents):
+ ... print message
+ A Team (team-a) has been invited to join this team.
+
+ >>> stevea_browser.open('http://launchpad.dev/~team-a/+invitation/team-b')
+ >>> stevea_browser.getControl('Accept').click()
+ >>> for message in get_feedback_messages(stevea_browser.contents):
+ ... print message
+ This team is now a member of B Team.
+
+ >>> jdub_browser.open('http://launchpad.dev/~team-b/+invitation/team-a')
+ >>> jdub_browser.getControl('Accept').click()
+ >>> for message in get_feedback_messages(jdub_browser.contents):
+ ... print message
+ This team may not be added to A Team because it is a member of B Team.
=== 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 11:06:09 +0000
@@ -0,0 +1,43 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test team membership changes."""
+
+from __future__ import with_statement
+
+__metaclass__ = type
+
+from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.interfaces.teammembership import CyclicalTeamMembershipError
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+
+
+class CircularMemberAdditionTestCase(TestCaseWithFactory):
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(CircularMemberAdditionTestCase, self).setUp()
+ self.a_team = self.factory.makeTeam(name="a")
+ self.b_team = self.factory.makeTeam(name="b")
+
+ def test_circular_invite(self):
+ """Two teams can invite each other without horrifying results."""
+ # Make the criss-cross invitations.
+ with person_logged_in(self.a_team.teamowner):
+ self.a_team.addMember(self.b_team, self.a_team.teamowner)
+ with person_logged_in(self.b_team.teamowner):
+ self.b_team.addMember(self.a_team, self.b_team.teamowner)
+
+ # A-team accepts B's kind invitation.
+ with person_logged_in(self.a_team.teamowner):
+ self.a_team.acceptInvitationToBeMemberOf(
+ self.b_team, None)
+ # B-team accepts A's kind invitation.
+ with person_logged_in(self.b_team.teamowner):
+ self.assertRaises(
+ CyclicalTeamMembershipError,
+ self.b_team.acceptInvitationToBeMemberOf,
+ self.a_team, None)
