launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/rename-private-team-795771 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #795771 in Launchpad itself: "Can't rename a private team, can't make it unprivate it to rename it, the only winning move is SQL"
  https://bugs.launchpad.net/launchpad/+bug/795771

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/rename-private-team-795771/+merge/72516

Private teams can be renamed.

== Implementation ==

Remove the private team check in setUpWidgets of TeamEditView

== Tests ==

Write some, there were none.

TestTeamEditView
    test_can_rename_private_team
    test_cannot_rename_team_with_ppa
    test_cannot_rename_team_with_active_mailinglist
    test_can_rename_team_with_purged_mailinglist

== Lint ==

Checking for conflicts and issues in changed files.

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

-- 
https://code.launchpad.net/~wallyworld/launchpad/rename-private-team-795771/+merge/72516
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/rename-private-team-795771 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2011-08-07 04:05:52 +0000
+++ lib/lp/registry/browser/team.py	2011-08-23 02:37:26 +0000
@@ -259,10 +259,9 @@
         has_mailing_list = (
             mailing_list is not None and
             mailing_list.status != MailingListStatus.PURGED)
-        is_private = self.context.visibility == PersonVisibility.PRIVATE
         has_ppa = self.context.archive is not None
 
-        block_renaming = (has_mailing_list or is_private or has_ppa)
+        block_renaming = (has_mailing_list or has_ppa)
         if block_renaming:
             # This makes the field's widget display (i.e. read) only.
             self.form_fields['name'].for_display = True
@@ -273,17 +272,12 @@
         # read-only mode if necessary.
         if block_renaming:
             # Group the read-only mode reasons in textual form.
-            # Private teams can't be associated with mailing lists
-            # or PPAs yet, so it's a dominant condition.
-            if is_private:
-                reason = 'is private'
+            if not has_mailing_list:
+                reason = 'has a PPA'
+            elif not has_ppa:
+                reason = 'has a mailing list'
             else:
-                if not has_mailing_list:
-                    reason = 'has a PPA'
-                elif not has_ppa:
-                    reason = 'has a mailing list'
-                else:
-                    reason = 'has a mailing list and a PPA'
+                reason = 'has a mailing list and a PPA'
             self.widgets['name'].hint = _(
                 'This team cannot be renamed because it %s.' % reason)
 
@@ -549,7 +543,7 @@
         already been approved or declined. This can only happen
         through bypassing the UI.
         """
-        mailing_list = getUtility(IMailingListSet).get(self.context.name)
+        getUtility(IMailingListSet).get(self.context.name)
         if self.getListInState(MailingListStatus.REGISTERED) is None:
             self.addError("This application can't be cancelled.")
 
@@ -985,7 +979,7 @@
             failed_names = [person.displayname for person in failed_joins]
             failed_list = ", ".join(failed_names)
 
-            mapping = dict( this_team=target_team.displayname,
+            mapping = dict(this_team=target_team.displayname,
                 failed_list=failed_list)
 
             if len(failed_joins) == 1:

=== modified file 'lib/lp/registry/browser/tests/test_team_view.py'
--- lib/lp/registry/browser/tests/test_team_view.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/browser/tests/test_team_view.py	2011-08-23 02:37:26 +0000
@@ -8,14 +8,21 @@
 __metaclass__ = type
 
 import transaction
-
-from canonical.testing.layers import DatabaseFunctionalLayer
-
-from lp.registry.interfaces.person import TeamSubscriptionPolicy
-
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.registry.interfaces.mailinglist import MailingListStatus
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    )
 from lp.testing import (
     login_person,
     TestCaseWithFactory,
+    person_logged_in,
     )
 from lp.testing.views import create_initialized_view
 
@@ -141,3 +148,53 @@
         failed = (self.a_team, self.b_team)
         successful = (self.c_team, self.d_team)
         self.acceptTeam(self.super_team, successful, failed)
+
+
+class TestTeamEditView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_can_rename_private_team(self):
+        # A private team can be renamed.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            owner=owner, visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(owner):
+            view = create_initialized_view(team, name="+edit")
+            self.assertFalse(view.form_fields['name'].for_display)
+
+    def test_cannot_rename_team_with_ppa(self):
+        # A team with a ppa cannot be renamed.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        removeSecurityProxy(team).archive = self.factory.makeArchive()
+        with person_logged_in(owner):
+            view = create_initialized_view(team, name="+edit")
+            self.assertTrue(view.form_fields['name'].for_display)
+            self.assertEqual(
+                'This team cannot be renamed because it has a PPA.',
+                view.widgets['name'].hint)
+
+    def test_cannot_rename_team_with_active_mailinglist(self):
+        # A team with a mailing list which isn't purged cannot be renamed.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        self.factory.makeMailingList(team, owner)
+        with person_logged_in(owner):
+            view = create_initialized_view(team, name="+edit")
+            self.assertTrue(view.form_fields['name'].for_display)
+            self.assertEqual(
+                'This team cannot be renamed because it has a mailing list.',
+                view.widgets['name'].hint)
+
+    def test_can_rename_team_with_purged_mailinglist(self):
+        # A team with a mailing list which is purged can be renamed.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        team_list = self.factory.makeMailingList(team, owner)
+        team_list.deactivate()
+        team_list.transitionToStatus(MailingListStatus.INACTIVE)
+        team_list.purge()
+        with person_logged_in(owner):
+            view = create_initialized_view(team, name="+edit")
+            self.assertFalse(view.form_fields['name'].for_display)