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

[Brad Crittenden]

Catch CyclicalTeamMembershipError when accepting invitations and report error.

= 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

[Michael Nelson (michael.nelson)]

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)

[Brad Crittenden (bac)]

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

=== 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 @@
     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/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 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
+
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.browser.person import (
     PersonEditView,
     PersonView,
+    TeamInvitationView,
     )
+
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.person import PersonVisibility
-from lp.registry.interfaces.teammembership import TeamMembershipStatus
+from lp.registry.interfaces.teammembership import (
+    ITeamMembershipSet,
+    TeamMembershipStatus,
+    )
+
 from lp.registry.model.karma import KarmaCategory
 from lp.soyuz.enums import (
     ArchiveStatus,
@@ -594,3 +601,85 @@
         self.assertEqual(1, len(view.errors))
         self.assertEqual(
             'This account is already deactivated.', view.errors[0])
+
+class TestTeamInvitationView(TestCaseWithFactory):
+    """Tests for TeamInvitationView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTeamInvitationView, self).setUp()
+        self.a_team = self.factory.makeTeam(name="team-a",
+                                            displayname="A-Team")
+        self.b_team = self.factory.makeTeam(name="team-b",
+                                            displayname="B-Team")
+        transaction.commit()
+
+    def test_circular_invite(self):
+        """Two teams can invite each other without horrifying results."""
+
+        # Make the criss-cross invitations.
+        # A invites B.
+        login_person(self.a_team.teamowner)
+        form = {
+            'field.newmember': 'team-b',
+            'field.actions.add': 'Add Member',
+            }
+        view = create_initialized_view(
+            self.a_team, "+addmember", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            u'B-Team (team-b) has been invited to join this team.',
+            notifications[0].message)
+
+        # B invites A.
+        login_person(self.b_team.teamowner)
+        form['field.newmember'] = 'team-a'
+        view = create_initialized_view(
+            self.b_team, "+addmember", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            u'A-Team (team-a) has been invited to join this team.',
+            notifications[0].message)
+
+        # Team A accepts the invitation.
+        login_person(self.a_team.teamowner)
+        form = {
+            'field.actions.accept': 'Accept',
+            'field.acknowledger_comment': 'Thanks for inviting us.',
+            }
+        request = LaunchpadTestRequest(form=form, method='POST')
+        request.setPrincipal(self.a_team.teamowner)
+        membership_set = getUtility(ITeamMembershipSet)
+        membership = membership_set.getByPersonAndTeam(self.a_team,
+                                                       self.b_team)
+        view = TeamInvitationView(membership, request)
+        view.initialize()
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            u'This team is now a member of B-Team.',
+            notifications[0].message)
+
+        # Team B attempts to accept the invitation.
+        login_person(self.b_team.teamowner)
+        request = LaunchpadTestRequest(form=form, method='POST')
+        request.setPrincipal(self.b_team.teamowner)
+        membership = membership_set.getByPersonAndTeam(self.b_team,
+                                                       self.a_team)
+        view = TeamInvitationView(membership, request)
+        view.initialize()
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = (
+            u'This team may not be added to A-Team because it is a member '
+            'of B-Team.')
+        self.assertEqual(
+            expected,
+            notifications[0].message)

=== 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 @@
 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 19:32:48 +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.

=== 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 @@
+# 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)