launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-237722 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #237722 Cyclical team membership allowed in +editproposedmembers
  https://bugs.launchpad.net/bugs/237722


= 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
-- 
https://code.launchpad.net/~bac/launchpad/bug-237722/+merge/34790
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-237722 into lp:launchpad/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-07 19:20:58 +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-07 19:20:58 +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,50 @@
     @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
+            try:
+                target_team.setMembershipData(
+                    person, status, reviewer=self.user, expires=expires,
+                    comment=self.request.form.get('comment'))
+            except CyclicalTeamMembershipError:
+                failed_joins.append(person)
 
-            self.context.setMembershipData(
-                person, status, reviewer=self.user, expires=expires,
-                comment=self.request.form.get('comment'))
+        if len(failed_joins) > 0:
+            failed_names = [person.displayname for person in failed_joins]
+            failed_list = ",".join(failed_names)
+            self.request.response.addInfoNotification(
+                _("${this_team} is a member of the following teams, so those "
+                  "teams could not be accepted:  "
+                  "${failed_list}.  These teams should be declined.",
+                  mapping=dict(
+                      this_team=target_team.displayname,
+                      failed_list=failed_list)))
+            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-07 19:20:58 +0000
@@ -0,0 +1,108 @@
+# 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.factory.makeTeam(
+            name="team-a",
+            owner=self.owner,
+            displayname="A-Team",
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        self.b_team = self.factory.makeTeam(
+            name="team-b",
+            owner=self.owner,
+            displayname="B-Team",
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        transaction.commit()
+
+    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.
+        login_person(self.owner)
+        form = {
+            'field.teams': 'team-b',
+            'field.actions.continue': 'Continue',
+            }
+        view = create_initialized_view(
+            self.a_team, "+add-my-teams", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            u'B-Team has been proposed to this team.',
+            notifications[0].message)
+
+
+        # Owner proposes Team A join Team B.
+        login_person(self.owner)
+        form = {
+            'field.teams': 'team-a',
+            'field.actions.continue': 'Continue',
+            }
+        view = create_initialized_view(
+            self.b_team, "+add-my-teams", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            u'A-Team has been proposed to this team.',
+            notifications[0].message)
+
+        # Accept Team B into Team A.
+        # Construct the team selection field, based on the id of the team.
+        selector = 'action_%d' % self.b_team.id
+        form = {
+            selector: 'approve',
+            'field.actions.save': 'Save changes',
+            }
+        view = create_initialized_view(
+            self.a_team, "+editproposedmembers", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(0, len(notifications))
+
+        # Accept Team A into Team B, or at least try.
+        selector = 'action_%d' % self.a_team.id
+        form = {
+            selector: 'approve',
+            'field.actions.save': 'Save changes',
+            }
+        view = create_initialized_view(
+            self.b_team, "+editproposedmembers", form=form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = (
+            u'B-Team is a member of the following teams, so those teams '
+            'could not be accepted:  A-Team.  These teams should be '
+            'declined.')
+        self.assertEqual(
+            expected,
+            notifications[0].message)