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

[Brad Crittenden]

Catch CyclicalTeamMembershipError and display a pleasing warning message rather than filing an OOPS when +editproposedmembers would create a cycle.

= Summary =

If a cycle of team membership could be created by accepting proposed
teams then the model will throw a CyclicalTeamMembershipError. This
error needs to be caught in the UI and transformed into a pleasing
message to the user.

== Proposed fix ==

Catch it and create a notification message.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t test_team_view

== Demo and Q/A ==

Follow the QA outlined in the test:

1) Create Team A.
2) Create Team B.
3) Go to https://launchpad.dev/~team-a
4) Select 'Add one of my teams' and choose Team B.
5) Go to https://launchpad.dev/~team-b
6) Select 'Add one of my teams' and choose Team A.
7) Go to https://launchpad.dev/~team-a and select "Edit proposed members".
8) Accept Team B
9) Go to https://launchpad.dev/~team-b and select "Edit proposed members".
10) Accept Team A
11) Note message. Verify you stayed on the +editproposedmembers page.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_team_view.py
  lib/lp/registry/browser/team.py
  lib/lp/registry/browser/person.py

[Leonard Richardson (leonardr)]

"These teams should be declined." is ambiguous, as often happens with "should". Is it referring to something the end-user ought to do, or something that should have just happened automatically? I'm pretty sure it's the former, but you should reword it.

Can you refactor lines 152-183? (It might not be worth it.)

This is just a suggetion, but I'd kind of like to see a more general final test. Create team C and team D as
well as A and B. Team C is just like team A: team C is a member of B, and B has an outstanding invitation to C. Team D also has an outstanding invitation to B, but no current memberships.

Attempt to approve all three team memberships. Then demonstrate that A and C could not be added to B (with error message), but that D was added to B. I think this will make your coverage a little better, but not so much that I'll insist on you making this change.

(If you do this, call team B "Everyone wants to join this team" or something more memorable--it took me a while to get those last two paragraphs right, and I'm still not sure it's right.)

review: Approve

[lp:~bac/launchpad/bug-237722]

11489. By Brad Crittenden on 2010-09-08

Added another test and refactored the common parts.

[Brad Crittenden (bac)]

Hi Leonard, thanks for the review. I have done the refactoring and added another test.

Your suggested word change was good. I've reworded and am asking Matthew Revell for a text review.

[Matthew Revell (matthew.revell)]

review: Approve (text)

[lp:~bac/launchpad/bug-237722]

11490. By Brad Crittenden on 2010-09-09

Merge from devel

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-09-03 03:12:39 +0000
+++ lib/lp/registry/browser/person.py	2010-09-09 14:21:48 +0000
@@ -208,10 +208,7 @@
     DateTimeFormatterAPI,
     PersonFormatterAPI,
     )
-from canonical.lazr.utils import (
-    safe_hasattr,
-    smartquote,
-    )
+from canonical.lazr.utils import smartquote
 from canonical.widgets import (
     LaunchpadDropdownWidget,
     LaunchpadRadioWidget,
@@ -4435,7 +4432,7 @@
                 verb = 'have been'
                 team_string= (
                     ', '.join(team_names[:-1]) + ' and ' + team_names[-1])
-            full_message += '%s %s %s ' % (team_string, verb, message)
+            full_message += '%s %s %s' % (team_string, verb, message)
         self.request.response.addInfoNotification(full_message)
 
 

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2010-09-03 03:12:39 +0000
+++ lib/lp/registry/browser/team.py	2010-09-09 14:21:48 +0000
@@ -89,7 +89,10 @@
     TeamContactMethod,
     TeamSubscriptionPolicy,
     )
-from lp.registry.interfaces.teammembership import TeamMembershipStatus
+from lp.registry.interfaces.teammembership import (
+    CyclicalTeamMembershipError,
+    TeamMembershipStatus,
+    )
 from lp.services.fields import PublicPersonChoice
 from lp.services.propertycache import cachedproperty
 
@@ -934,31 +937,60 @@
     @action('Save changes', name='save')
     def action_save(self, action, data):
         expires = self.context.defaultexpirationdate
-        for person in self.context.proposedmembers:
+        statuses = dict(
+            approve=TeamMembershipStatus.APPROVED,
+            decline=TeamMembershipStatus.DECLINED,
+            )
+        target_team = self.context
+        failed_joins = []
+        for person in target_team.proposedmembers:
             action = self.request.form.get('action_%d' % person.id)
-            if action == "approve":
-                status = TeamMembershipStatus.APPROVED
-            elif action == "decline":
-                status = TeamMembershipStatus.DECLINED
-            else:
+            status = statuses.get(action)
+            if status is None:
                 # The action is "hold" or no action was specified for this
                 # person, which could happen if the set of proposed members
                 # changed while the form was being processed.
                 continue
-
-            self.context.setMembershipData(
-                person, status, reviewer=self.user, expires=expires,
-                comment=self.request.form.get('comment'))
+            try:
+                target_team.setMembershipData(
+                    person, status, reviewer=self.user, expires=expires,
+                    comment=self.request.form.get('comment'))
+            except CyclicalTeamMembershipError:
+                failed_joins.append(person)
+
+        if len(failed_joins) > 0:
+            failed_names = [person.displayname for person in failed_joins]
+            failed_list = ", ".join(failed_names)
+
+            mapping=dict(
+                this_team=target_team.displayname,
+                failed_list=failed_list)
+
+            if len(failed_joins) == 1:
+                self.request.response.addInfoNotification(
+                    _('${this_team} is a member of the following team, so it '
+                      'could not be accepted:  '
+                      '${failed_list}.  You need to "Decline" that team.',
+                      mapping=mapping))
+            else:
+                self.request.response.addInfoNotification(
+                    _('${this_team} is a member of the following teams, so '
+                      'they could not be accepted:  '
+                      '${failed_list}.  You need to "Decline" those teams.',
+                      mapping=mapping))
+            self.next_url = ''
+        else:
+            self.next_url = self._next_url
 
     @property
     def page_title(self):
         return 'Proposed members of %s' % self.context.displayname
 
     @property
-    def next_url(self):
+    def _next_url(self):
         return '%s/+members' % canonical_url(self.context)
 
-    cancel_url = next_url
+    cancel_url = _next_url
 
 
 class TeamBrandingView(BrandingChangeView):

=== added file 'lib/lp/registry/browser/tests/test_team_view.py'
--- lib/lp/registry/browser/tests/test_team_view.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_team_view.py	2010-09-09 14:21:48 +0000
@@ -0,0 +1,143 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""
+Test team views.
+"""
+
+__metaclass__ = type
+
+import transaction
+
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.registry.interfaces.person import TeamSubscriptionPolicy
+
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class TestProposedTeamMembersEditView(TestCaseWithFactory):
+    """Tests for ProposedTeamMembersEditView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProposedTeamMembersEditView, self).setUp()
+        self.owner = self.factory.makePerson(name="team-owner")
+        self.a_team = self.makeTeam("team-a", "A-Team")
+        self.b_team = self.makeTeam("team-b", "B-Team")
+        transaction.commit()
+        login_person(self.owner)
+
+    def makeTeam(self, name, displayname):
+        """Make a moderated team."""
+        return self.factory.makeTeam(
+            name=name,
+            owner=self.owner,
+            displayname=displayname,
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+
+    def inviteToJoin(self, joinee, joiner):
+        """Invite the joiner team into the joinee team."""
+        # Joiner is proposed to join joinee.
+        form = {
+            'field.teams': joiner.name,
+            'field.actions.continue': 'Continue',
+            }
+        view = create_initialized_view(
+            joinee, "+add-my-teams", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = u"%s has been proposed to this team." % (
+            joiner.displayname)
+        self.assertEqual(
+            expected,
+            notifications[0].message)
+
+    def acceptTeam(self, joinee, successful, failed):
+        """Accept the teams into the joinee team.
+
+        The teams in 'successful' are expected to be allowed.
+        The teams in 'failed' are expected to fail.
+        """
+        failed_names = ', '.join([team.displayname for team in failed])
+        if len(failed) == 1:
+            failed_message = (
+                u'%s is a member of the following team, '
+                'so it could not be accepted:  %s.  '
+                'You need to "Decline" that team.' %
+                (joinee.displayname, failed_names))
+        else:
+            failed_message = (
+                u'%s is a member of the following teams, '
+                'so they could not be accepted:  %s.  '
+                'You need to "Decline" those teams.' %
+                (joinee.displayname, failed_names))
+
+        form = {
+            'field.actions.save': 'Save changes',
+            }
+        for team in successful + failed:
+            # Construct the team selection field, based on the id of the
+            # team.
+            selector = 'action_%d' % team.id
+            form[selector] = 'approve'
+
+        view = create_initialized_view(
+            joinee, "+editproposedmembers", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        if len(failed) == 0:
+            self.assertEqual(0, len(notifications))
+        else:
+            self.assertEqual(1, len(notifications))
+            self.assertEqual(
+                failed_message,
+                notifications[0].message)
+
+    def test_circular_proposal_acceptance(self):
+        """Two teams can invite each other without horrifying results."""
+
+        # Make the criss-cross invitations.
+
+        # Owner proposes Team B join Team A.
+        self.inviteToJoin(self.a_team, self.b_team)
+
+        # Owner proposes Team A join Team B.
+        self.inviteToJoin(self.b_team, self.a_team)
+
+        # Accept Team B into Team A.
+        self.acceptTeam(self.a_team, successful=(self.b_team,), failed=())
+
+        # Accept Team A into Team B, and fail trying.
+        self.acceptTeam(self.b_team, successful=(), failed=(self.a_team,))
+
+    def test_circular_proposal_acceptance_with_some_noncircular(self):
+        """Accepting a mix of successful and failed teams works."""
+        # Create some extra teams.
+        self.c_team = self.makeTeam("team-c", "C-Team")
+        self.d_team = self.makeTeam("team-d", "D-Team")
+        self.super_team = self.makeTeam("super-team", "Super Team")
+
+        # Everyone wants to join Super Team.
+        for team in [self.a_team, self.b_team, self.c_team, self.d_team]:
+            self.inviteToJoin(self.super_team, team)
+
+        # Super Team joins two teams.
+        for team in [self.a_team, self.b_team]:
+            self.inviteToJoin(team, self.super_team)
+
+        # Super Team is accepted into both.
+        for team in [self.a_team, self.b_team]:
+            self.acceptTeam(team, successful=(self.super_team, ), failed=())
+
+        # Now Super Team attempts to accept all teams.  Two succeed but the
+        # two with that would cause a cycle fail.
+        failed = (self.a_team, self.b_team)
+        successful = (self.c_team, self.d_team)
+        self.acceptTeam(self.super_team, successful, failed)