Merge lp:~abentley/launchpad/team-transition-error into lp:launchpad

[Aaron Bentley]

Fix TeamMembershipTransitionError moving from DECLINED to INVITED

= Summary =
Fix bug #820077: TeamMembershipTransitionError: Bad state transition from DECLINED to INVITED

== Proposed fix ==
Bless transition from DECLINED to INVITED

== Pre-implementation notes ==
Discussed with sinzui

== Implementation details ==
As well as fixing the particular error, I've tested that TeamMemberAddView.add_action cannot raise TeamMembershipTransitionError at all, so there's no need to handle that exception in browser code.

This meant turning PROPOSED to APPROVED if the reviewer was the an admin of the team, instead of changing it to INVITED.

It also meant preventing addMember from downgrading APPROVED/ADMIN to PROPOSED. And while I was there, I prevented downgrading ADMIN to APPROVED.

== Tests ==
bin/test -t test_no_TeamMembershipTransitionError -t test_declined_member_can_be_invited -t test_add_approved -t test_add_admin -t test_implicit_approval

== Demo and Q/A ==
Create team A and team B with different owners. As the owner of team A, attempt to join team B. As the owner of team B, decline the membership. As the owner of team B, attempt to add team A as a member. This should not OOPS.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_team.py
  lib/lp/registry/tests/test_teammembership.py
  lib/lp/registry/model/person.py
  lib/lp/registry/model/teammembership.py

[Curtis Hovey (sinzui)]

As we discussed on IRC, this branch also fixes bug 480157.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2011-04-09 02:23:46 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2011-08-29 13:54:49 +0000
@@ -4,11 +4,16 @@
 __metaclass__ = type
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.registry.browser.person import TeamOverviewMenu
 from lp.registry.interfaces.persontransferjob import IPersonMergeJobSource
+from lp.registry.interfaces.teammembership import (
+    ITeamMembershipSet,
+    TeamMembershipStatus,
+    )
 from lp.testing import (
     login_person,
     TestCaseWithFactory,
@@ -42,7 +47,7 @@
         self.assertEqual('Create a mailing list', link.text)
 
     def test_TeamOverviewMenu_check_menu_links_with_mailing(self):
-        mailing_list = self.factory.makeMailingList(
+        self.factory.makeMailingList(
             self.team, self.team.teamowner)
         menu = TeamOverviewMenu(self.team)
         self.assertEqual(True, check_menu_links(menu))
@@ -139,6 +144,18 @@
             "You can't add a team that doesn't have any active members.",
             view.errors[0])
 
+    def test_no_TeamMembershipTransitionError(self):
+        # Attempting to add a team never triggers a
+        # TeamMembershipTransitionError
+        member_team = self.factory.makeTeam()
+        self.team.addMember(member_team, self.team.teamowner)
+        tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+            member_team, self.team)
+        for status in TeamMembershipStatus.items:
+            removeSecurityProxy(tm).status = status
+            view = create_initialized_view(self.team, "+addmember")
+            view.add_action.success(data={'newmember': member_team})
+
 
 class TestTeamIndexView(TestCaseWithFactory):
 
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-08-25 03:30:40 +0000
+++ lib/lp/registry/model/person.py	2011-08-29 13:54:49 +0000
@@ -1455,6 +1455,12 @@
             "You can't add a member with this status: %s." % status.name)
 
         event = JoinTeamEvent
+        tm = TeamMembership.selectOneBy(person=person, team=self)
+        if tm is not None:
+            if tm.status == TeamMembershipStatus.ADMIN or (
+                tm.status == TeamMembershipStatus.APPROVED and status ==
+                TeamMembershipStatus.PROPOSED):
+                status = tm.status
         if person.is_team:
             assert not self.hasParticipationEntryFor(person), (
                 "Team '%s' is a member of '%s'. As a consequence, '%s' can't "
@@ -1468,12 +1474,21 @@
             is_reviewer_admin_of_new_member = (
                 person in reviewer.getAdministratedTeams())
             if not force_team_add and not is_reviewer_admin_of_new_member:
-                status = TeamMembershipStatus.INVITED
-                event = TeamInvitationEvent
+                if tm is None or tm.status not in (
+                    TeamMembershipStatus.PROPOSED,
+                    TeamMembershipStatus.APPROVED,
+                    TeamMembershipStatus.ADMIN,
+                    ):
+                    status = TeamMembershipStatus.INVITED
+                    event = TeamInvitationEvent
+                else:
+                    if tm.status == TeamMembershipStatus.PROPOSED:
+                        status = TeamMembershipStatus.APPROVED
+                    else:
+                        status = tm.status
 
         status_changed = True
         expires = self.defaultexpirationdate
-        tm = TeamMembership.selectOneBy(person=person, team=self)
         if tm is None:
             tm = TeamMembershipSet().new(
                 person, self, status, reviewer, dateexpires=expires,
 
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2011-08-12 14:39:51 +0000
+++ lib/lp/registry/model/teammembership.py	2011-08-29 13:54:49 +0000
@@ -338,7 +338,7 @@
             deactivated: [proposed, approved, admin, invited],
             expired: [proposed, approved, admin, invited],
             proposed: [approved, admin, declined],
-            declined: [proposed, approved, admin],
+            declined: [proposed, approved, admin, invited],
             invited: [approved, admin, invitation_declined],
             invitation_declined: [invited, approved, admin]}
 
 
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/tests/test_teammembership.py	2011-08-29 13:54:49 +0000
@@ -315,7 +315,7 @@
             ['name16', 'no-priv', 'team5'], self.team4)
         self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
         self.assertEqual(
-            previous_count-8,
+            previous_count - 8,
             self.getTeamParticipationCount())
 
     def testRemovingLeafTeam(self):
@@ -333,7 +333,7 @@
         self.assertParticipantsEquals(['name16'], self.team4)
         self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
         self.assertEqual(
-            previous_count-8,
+            previous_count - 8,
             self.getTeamParticipationCount())
 
 
@@ -393,7 +393,7 @@
             ['name16', 'no-priv', 'team5'], self.team4)
         self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
         self.assertEqual(
-            previous_count-4,
+            previous_count - 4,
             self.getTeamParticipationCount())
 
     def testRemoveTeam5FromTeam4(self):
@@ -410,7 +410,7 @@
         self.assertParticipantsEquals(['name16'], self.team4)
         self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
         self.assertEqual(
-            previous_count-4,
+            previous_count - 4,
             self.getTeamParticipationCount())
 
 
@@ -512,7 +512,7 @@
         self.assertParticipantsEquals(
             ['name16', 'no-priv', 'team2', 'team4', 'team5'], self.team6)
         self.assertEqual(
-            previous_count-3,
+            previous_count - 3,
             self.getTeamParticipationCount())
 
     def testRemoveTeam5FromTeam4(self):
@@ -529,7 +529,7 @@
         self.assertParticipantsEquals(
             ['name16', 'team2', 'team3', 'team4'], self.team6)
         self.assertEqual(
-            previous_count-10,
+            previous_count - 10,
             self.getTeamParticipationCount())
 
     def testTeam3_deactivateActiveMemberships(self):
@@ -576,7 +576,7 @@
         """
         login('[e-mail]')
         person = self.factory.makePerson()
-        login_person(person) # Now login with the future owner of the teams.
+        login_person(person)  # Now login with the future owner of the teams.
         teamA = self.factory.makeTeam(
             person, subscription_policy=TeamSubscriptionPolicy.MODERATED)
         teamB = self.factory.makeTeam(
@@ -837,6 +837,62 @@
             TeamMembershipStatus.INVITATION_DECLINED, self.team2.teamowner)
         self.assertEqual(TeamMembershipStatus.INVITATION_DECLINED, tm.status)
 
+    def test_declined_member_can_be_invited(self):
+        # A team can re-invite a declined member.
+        self.team2.addMember(
+            self.team1, self.no_priv, status=TeamMembershipStatus.PROPOSED,
+            force_team_add=True)
+        tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+            self.team1, self.team2)
+        tm.setStatus(
+            TeamMembershipStatus.DECLINED, self.team1.teamowner)
+        tm.setStatus(
+            TeamMembershipStatus.INVITED, self.team1.teamowner)
+        self.assertEqual(TeamMembershipStatus.INVITED, tm.status)
+
+    def test_add_approved(self):
+        # Adding an approved team is a no-op.
+        member_team = self.factory.makeTeam()
+        self.team1.addMember(
+            member_team, self.team1.teamowner)
+        with person_logged_in(member_team.teamowner):
+            member_team.acceptInvitationToBeMemberOf(self.team1, 'alright')
+        self.team1.addMember(
+            member_team, self.team1.teamowner)
+        tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+            member_team, self.team1)
+        self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
+        self.team1.addMember(
+            member_team, member_team.teamowner)
+        self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
+
+    def test_add_admin(self):
+        # Adding an admin team is a no-op.
+        member_team = self.factory.makeTeam()
+        self.team1.addMember(
+            member_team, self.team1.teamowner,
+            status=TeamMembershipStatus.ADMIN, force_team_add=True)
+        self.team1.addMember(
+            member_team, self.team1.teamowner)
+        tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+            member_team, self.team1)
+        self.assertEqual(TeamMembershipStatus.ADMIN, tm.status)
+        self.team1.addMember(
+            member_team, member_team.teamowner)
+        self.assertEqual(TeamMembershipStatus.ADMIN, tm.status)
+
+    def test_implicit_approval(self):
+        # Inviting a proposed person is an implicit approval.
+        member_team = self.factory.makeTeam()
+        self.team1.addMember(
+            member_team, self.team1.teamowner,
+            status=TeamMembershipStatus.PROPOSED, force_team_add=True)
+        self.team1.addMember(
+            member_team, self.team1.teamowner)
+        tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+            member_team, self.team1)
+        self.assertEqual(TeamMembershipStatus.APPROVED, tm.status)
+
     def test_retractTeamMembership_invited(self):
         # A team can retract a membership invitation.
         self.team2.addMember(self.team1, self.no_priv)