Merge lp:~sinzui/launchpad/mailing-list-name-field into lp:launchpad

[Curtis Hovey]

iDo not imply a team can be renamed when it has a mailing list.

    Launchpad bug: https://bugs.launchpad.net/bugs/889496
    Pre-implementation: myself

The field to rename was shown implying I could do the rename. The form
did not check that there was an mailing list. I was confused actually,
because the team page did not show an list. This list was being created
:(.

This is not an issue with the +edit page. I was using the +review page.
I think the two view need to share some code.

    * +review implies I can change the name of a user with an active PPA...
      PersonRenameFormMixin. This is broken for users and teams.
    * There are two options. A. subclass TeamEditView or
      create a new class using TeamFormMixin, PersonRenameFormMixin.

--------------------------------------------------------------------

RULES

    * Extract the rename tests from TestPersonEditView to a mixin.
    * Add a testcase for PersonAdministerView that used the new mixin.
    * Update PersonAdministerView to extend PersonRenameFormMixin
    * Extract the rename tests from TestTeamEditView to a mixin.
    * Add a test for for TeamAdministerView that uses the mixin.
    * Create TeamAdministerView from PersonAdministerViewthat does
      not have standing.

QA

    * Visit https://qastaging.launchpad.net/~test234/+review
    * Verify that the name field is not editable.
    * Visit https://qastaging.launchpad.net/~sinzui/+review
    * Verify that the name field is not editable.

LINT

    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/team.py
    lib/lp/registry/browser/tests/test_person_view.py
    lib/lp/registry/browser/tests/test_team.py

TEST

    ./bin/test -vv -t TestPersonEditView -t PersonAdministerViewTestCase \
        lp.registry.browser.tests.test_person_view
    ./bin/test -vvc -t TestTeamEditView -t TeamAdminisiterViewTestCase \
        lp.registry.browser.tests.test_team

IMPLEMENTATION

Updated PersonAdministerView to use PersonRenameFormMixin. Extracted the
common rename tests to a mixin and used it in both the edit and
administer test cases. I had to move PersonRenameFormMixin which inflated
the diff. I modernised the setup of the mixing tests. I added tests about
which fields are visible to admins and registry_experts because I did not
see the rules verified in other browser tests.
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_person_view.py

Subclassed PersonAdministerView so that I do not see the user standing
fields when looking at a team. Extracted the common rename tests to a mixin
and used it in both the edit and administer test cases.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/team.py
    lib/lp/registry/browser/tests/test_team.py

[Brad Crittenden (bac)]

Hi Curtis,

Thanks for fixing this bad UI.

This was my first encounter with checkRename(). I found it confusing that a return of None meant things were a-ok. Since your use is the only call site perhaps a brief comment?

typo: s/do no see/do not see/ (twice)

Otherwise it looks great.

--bac

review: Approve (code)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2012-01-18 16:49:09 +0000
+++ lib/lp/registry/browser/configure.zcml	2012-01-27 17:43:25 +0000
@@ -1152,6 +1152,12 @@
             permission="launchpad.Edit"
             template="../templates/team-edit.pt"/>
         <browser:page
+            name="+review"
+            for="lp.registry.interfaces.person.ITeam"
+            class="lp.registry.browser.team.TeamAdministerView"
+            permission="launchpad.Moderate"
+            template="../templates/person-review.pt"/>
+        <browser:page
             name="+branding"
             for="lp.registry.interfaces.person.ITeam"
             class="lp.registry.browser.team.TeamBrandingView"

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-01-25 04:56:50 +0000
+++ lib/lp/registry/browser/person.py	2012-01-27 17:43:25 +0000
@@ -1225,7 +1225,26 @@
         return encodeddata
 
 
-class PersonAdministerView(LaunchpadEditFormView):
+class PersonRenameFormMixin(LaunchpadEditFormView):
+
+    def setUpWidgets(self):
+        """See `LaunchpadViewForm`.
+
+        Renames are prohibited if a person/team has an active PPA or an
+        active mailing list.
+        """
+        reason = self.context.checkRename()
+        # Reason is a message about why a rename cannot happen.
+        # No message means renames are permitted.
+        if reason:
+            # This makes the field's widget display (i.e. read) only.
+            self.form_fields['name'].for_display = True
+        super(PersonRenameFormMixin, self).setUpWidgets()
+        if reason:
+            self.widgets['name'].hint = reason
+
+
+class PersonAdministerView(PersonRenameFormMixin):
     """Administer an `IPerson`."""
     schema = IPerson
     label = "Review person"
@@ -3475,23 +3494,6 @@
     page_title = label
 
 
-class PersonRenameFormMixin(LaunchpadEditFormView):
-
-    def setUpWidgets(self):
-        """See `LaunchpadViewForm`.
-
-        Renames are prohibited if a person/team has an active PPA or an
-        active mailing list.
-        """
-        reason = self.context.checkRename()
-        if reason:
-            # This makes the field's widget display (i.e. read) only.
-            self.form_fields['name'].for_display = True
-        super(PersonRenameFormMixin, self).setUpWidgets()
-        if reason:
-            self.widgets['name'].hint = reason
-
-
 class PersonEditView(PersonRenameFormMixin, BasePersonEditView):
     """The Person 'Edit' page."""
 

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2012-01-25 04:56:50 +0000
+++ lib/lp/registry/browser/team.py	2012-01-27 17:43:25 +0000
@@ -95,6 +95,7 @@
 from lp.registry.browser.objectreassignment import ObjectReassignmentView
 from lp.registry.browser.person import (
     CommonMenuLinks,
+    PersonAdministerView,
     PersonIndexView,
     PersonNavigation,
     PersonRenameFormMixin,
@@ -353,6 +354,12 @@
     cancel_url = next_url
 
 
+class TeamAdministerView(PersonAdministerView):
+    """A view to administer teams on behalf of users."""
+    label = "Review team"
+    default_field_names = ['name', 'displayname']
+
+
 def generateTokenAndValidationEmail(email, team):
     """Send a validation message to the given email."""
     login = getUtility(ILaunchBag).login

=== removed file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_person.py	1970-01-01 00:00:00 +0000
@@ -1,52 +0,0 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Test harness for person views unit tests."""
-
-__metaclass__ = type
-
-from textwrap import dedent
-
-from lp.registry.browser.person import PersonView
-from lp.services.config import config
-from lp.services.webapp.servers import LaunchpadTestRequest
-from lp.testing import (
-    login_person,
-    TestCaseWithFactory,
-    )
-from lp.testing.layers import DatabaseFunctionalLayer
-
-
-class PersonView_openid_identity_url_TestCase(TestCaseWithFactory):
-    """Tests for the public OpenID identifier shown on the profile page."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        TestCaseWithFactory.setUp(self)
-        self.user = self.factory.makePerson(name='eris')
-        self.request = LaunchpadTestRequest(
-            SERVER_URL="http://launchpad.dev/")
-        login_person(self.user, self.request)
-        self.view = PersonView(self.user, self.request)
-        # Marker allowing us to reset the config.
-        config.push(self.id(), '')
-        self.addCleanup(config.pop, self.id())
-
-    def test_should_be_profile_page_when_delegating(self):
-        """The profile page is the OpenID identifier in normal situation."""
-        self.assertEquals(
-            'http://launchpad.dev/~eris', self.view.openid_identity_url)
-
-    def test_should_be_production_profile_page_when_not_delegating(self):
-        """When the profile page is not delegated, the OpenID identity URL
-        should be the one on the main production site."""
-        config.push('non-delegating', dedent('''
-            [vhost.mainsite]
-            openid_delegate_profile: False
-
-            [launchpad]
-            non_restricted_hostname: prod.launchpad.dev
-            '''))
-        self.assertEquals(
-            'http://prod.launchpad.dev/~eris', self.view.openid_identity_url)

=== renamed file 'lib/lp/registry/browser/tests/test_person_view.py' => 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2012-01-20 15:42:44 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-01-27 17:43:25 +0000
@@ -3,6 +3,7 @@
 
 __metaclass__ = type
 
+from textwrap import dedent
 import doctest
 
 import soupmatchers
@@ -21,10 +22,7 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.model.bugtask import BugTask
 from lp.buildmaster.enums import BuildStatus
-from lp.registry.browser.person import (
-    PersonEditView,
-    PersonView,
-    )
+from lp.registry.browser.person import PersonView
 from lp.registry.browser.team import TeamInvitationView
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.person import (
@@ -57,6 +55,7 @@
 from lp.testing import (
     ANONYMOUS,
     login,
+    login_celebrity,
     login_person,
     person_logged_in,
     StormStatementRecorder,
@@ -79,6 +78,41 @@
     )
 
 
+class PersonViewOpenidIdentityUrlTestCase(TestCaseWithFactory):
+    """Tests for the public OpenID identifier shown on the profile page."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.user = self.factory.makePerson(name='eris')
+        self.request = LaunchpadTestRequest(
+            SERVER_URL="http://launchpad.dev/")
+        login_person(self.user, self.request)
+        self.view = PersonView(self.user, self.request)
+        # Marker allowing us to reset the config.
+        config.push(self.id(), '')
+        self.addCleanup(config.pop, self.id())
+
+    def test_should_be_profile_page_when_delegating(self):
+        """The profile page is the OpenID identifier in normal situation."""
+        self.assertEquals(
+            'http://launchpad.dev/~eris', self.view.openid_identity_url)
+
+    def test_should_be_production_profile_page_when_not_delegating(self):
+        """When the profile page is not delegated, the OpenID identity URL
+        should be the one on the main production site."""
+        config.push('non-delegating', dedent('''
+            [vhost.mainsite]
+            openid_delegate_profile: False
+
+            [launchpad]
+            non_restricted_hostname: prod.launchpad.dev
+            '''))
+        self.assertEquals(
+            'http://prod.launchpad.dev/~eris', self.view.openid_identity_url)
+
+
 class TestPersonIndexView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -136,7 +170,7 @@
     def test_private_superteams_anonymous(self):
         # If the viewer is anonymous, the portlet is not shown.
         team = self.createTeams()
-        viewer = self.factory.makePerson()
+        self.factory.makePerson()
         view = create_initialized_view(
             team, '+index', server_url=canonical_url(team), path_info='')
         html = view()
@@ -324,18 +358,7 @@
         self.failUnless(person_view.should_show_ppa_section)
 
 
-class TestPersonEditView(TestCaseWithFactory):
-
-    layer = LaunchpadFunctionalLayer
-
-    def setUp(self):
-        TestCaseWithFactory.setUp(self)
-        self.valid_email_address = self.factory.getUniqueEmailAddress()
-        self.person = self.factory.makePerson(email=self.valid_email_address)
-        login_person(self.person)
-        self.ppa = self.factory.makeArchive(owner=self.person)
-        self.view = PersonEditView(
-            self.person, LaunchpadTestRequest())
+class TestPersonRenameFormMixin:
 
     def test_can_rename_with_empty_PPA(self):
         # If a PPA exists but has no packages, we can rename the
@@ -380,6 +403,19 @@
         self.view.initialize()
         self.assertFalse(self.view.form_fields['name'].for_display)
 
+
+class TestPersonEditView(TestPersonRenameFormMixin, TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonEditView, self).setUp()
+        self.valid_email_address = self.factory.getUniqueEmailAddress()
+        self.person = self.factory.makePerson(email=self.valid_email_address)
+        login_person(self.person)
+        self.ppa = self.factory.makeArchive(owner=self.person)
+        self.view = create_initialized_view(self.person, '+edit')
+
     def test_add_email_good_data(self):
         email_address = self.factory.getUniqueEmailAddress()
         form = {
@@ -423,6 +459,34 @@
         self.assertEqual(expected_msg, error_msg)
 
 
+class PersonAdministerViewTestCase(TestPersonRenameFormMixin,
+                                   TestCaseWithFactory):
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(PersonAdministerViewTestCase, self).setUp()
+        self.person = self.factory.makePerson()
+        login_celebrity('admin')
+        self.ppa = self.factory.makeArchive(owner=self.person)
+        self.view = create_initialized_view(self.person, '+review')
+
+    def test_init_admin(self):
+        # An admin sees all the fields.
+        self.assertEqual('Review person', self.view.label)
+        self.assertEqual(
+            ['name', 'displayname', 'personal_standing',
+             'personal_standing_reason'],
+            self.view.field_names)
+
+    def test_init_registry_expert(self):
+        # Registry experts do not see the the displayname field.
+        login_celebrity('registry_experts')
+        self.view.setUpFields()
+        self.assertEqual(
+            ['name', 'personal_standing', 'personal_standing_reason'],
+            self.view.field_names)
+
+
 class TestTeamCreationView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-01-23 18:15:34 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-01-27 17:43:25 +0000
@@ -30,6 +30,7 @@
 from lp.services.webapp.publisher import canonical_url
 from lp.soyuz.enums import ArchiveStatus
 from lp.testing import (
+    login_celebrity,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -173,9 +174,9 @@
         self.acceptTeam(self.super_team, successful, failed)
 
 
-class TestTeamEditView(TestCaseWithFactory):
+class TestTeamPersonRenameFormMixin:
 
-    layer = LaunchpadFunctionalLayer
+    view_name = None
 
     def test_cannot_rename_team_with_active_ppa(self):
         # A team with an active PPA that contains publications cannot be
@@ -186,7 +187,7 @@
         self.factory.makeSourcePackagePublishingHistory(archive=archive)
         get_property_cache(team).archive = archive
         with person_logged_in(owner):
-            view = create_initialized_view(team, name="+edit")
+            view = create_initialized_view(team, name=self.view_name)
             self.assertTrue(view.form_fields['name'].for_display)
             self.assertEqual(
                 'This team has an active PPA with packages published and '
@@ -201,7 +202,7 @@
         removeSecurityProxy(archive).status = ArchiveStatus.DELETED
         get_property_cache(team).archive = archive
         with person_logged_in(owner):
-            view = create_initialized_view(team, name="+edit")
+            view = create_initialized_view(team, name=self.view_name)
             self.assertFalse(view.form_fields['name'].for_display)
 
     def test_cannot_rename_team_with_active_mailinglist(self):
@@ -211,7 +212,7 @@
         team = self.factory.makeTeam(owner=owner)
         self.factory.makeMailingList(team, owner)
         with person_logged_in(owner):
-            view = create_initialized_view(team, name="+edit")
+            view = create_initialized_view(team, name=self.view_name)
             self.assertTrue(view.form_fields['name'].for_display)
             self.assertEqual(
                 'This team has a mailing list and may not be renamed.',
@@ -226,7 +227,7 @@
         team_list.transitionToStatus(MailingListStatus.INACTIVE)
         team_list.purge()
         with person_logged_in(owner):
-            view = create_initialized_view(team, name="+edit")
+            view = create_initialized_view(team, name=self.view_name)
             self.assertFalse(view.form_fields['name'].for_display)
 
     def test_cannot_rename_team_with_multiple_reasons(self):
@@ -240,13 +241,19 @@
         self.factory.makeSourcePackagePublishingHistory(archive=archive)
         get_property_cache(team).archive = archive
         with person_logged_in(owner):
-            view = create_initialized_view(team, name="+edit")
+            view = create_initialized_view(team, name=self.view_name)
             self.assertTrue(view.form_fields['name'].for_display)
             self.assertEqual(
                 'This team has an active PPA with packages published and '
                 'a mailing list and may not be renamed.',
                 view.widgets['name'].hint)
 
+
+class TestTeamEditView(TestTeamPersonRenameFormMixin, TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+    view_name = '+edit'
+
     def test_edit_team_view_permission(self):
         # Only an administrator or the team owner of a team can
         # change the details of that team.
@@ -437,6 +444,29 @@
             view.errors[0].doc())
 
 
+class TeamAdminisiterViewTestCase(TestTeamPersonRenameFormMixin,
+                                  TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+    view_name = '+review'
+
+    def test_init_admin(self):
+        # An admin sees all the fields.
+        team = self.factory.makeTeam()
+        login_celebrity('admin')
+        view = create_initialized_view(team, name=self.view_name)
+        self.assertEqual('Review team', view.label)
+        self.assertEqual(
+            ['name', 'displayname'], view.field_names)
+
+    def test_init_registry_expert(self):
+        # Registry experts do not see the the displayname field.
+        team = self.factory.makeTeam()
+        login_celebrity('registry_experts')
+        view = create_initialized_view(team, name=self.view_name)
+        self.assertEqual(['name'], view.field_names)
+
+
 class TestTeamMenu(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer