Merge lp:~stevenk/launchpad/better-name-field into lp:launchpad

[Steve Kowalik]

[r=sinzui,wallyworld][bug=494801,932192] Fix blacklist message, make visibility the 3rd field for IPersonSet:+newteam/ITeam:+edit, remove the JS that edits the name when private is selected, render the context when editing a team, and fix IPerson.checkAllowVisibility().

Firstly, slightly change the blacklisted message, and remove some duplication.

Also re-order the visibility field after we re-add it to the form if the user is permitted to set it.

Remove the JS that was adding private- to the name field when the visibility was changed.

[Curtis Hovey (sinzui)]

"Please contact Launchpad Support." might be lacking and we strive to avoid "please". Maybe
    Contact Launchpad Support if you want to use this name.

Your hack is ugly, but two lines. You can consider using the zope way o selecting field name to change the order:
        field_names = [
            field.__name__ for field in self.form_fields
            if field.__name__ =! 'visibility']
        field_names.insert(2, 'visibility')
        self.form_fields = self.form_fields.select(field_names)

review: Approve (code)

[Ian Booth (wallyworld)]

I reviewed the changes introduced by r14801 and they look ok

review: Approve

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2012-02-13 23:02:42 +0000
+++ lib/lp/registry/browser/team.py	2012-02-16 03:21:23 +0000
@@ -272,12 +272,18 @@
                     'Private teams must have a Restricted subscription '
                     'policy.')
 
-    def setUpVisibilityField(self):
+    def setUpVisibilityField(self, render_context=False):
         """Set the visibility field to read-write, or remove it."""
         self.form_fields = self.form_fields.omit('visibility')
         if self.user and self.user.checkAllowVisibility():
             visibility = copy_field(ITeam['visibility'], readonly=False)
-            self.form_fields += Fields(visibility)
+            self.form_fields += Fields(
+                visibility, render_context=render_context)
+            # Shift visibility to be the third field.
+            field_names = [field.__name__ for field in self.form_fields]
+            field = field_names.pop()
+            field_names.insert(2, field)
+            self.form_fields = self.form_fields.select(*field_names)
 
 
 class TeamEditView(TeamFormMixin, PersonRenameFormMixin,
@@ -310,7 +316,7 @@
         self.field_names.remove('contactemail')
         self.field_names.remove('teamowner')
         super(TeamEditView, self).setUpFields()
-        self.setUpVisibilityField()
+        self.setUpVisibilityField(render_context=True)
 
     def setUpWidgets(self):
         super(TeamEditView, self).setUpWidgets()

=== modified file 'lib/lp/registry/browser/tests/private-team-creation-views.txt'
--- lib/lp/registry/browser/tests/private-team-creation-views.txt	2010-10-19 18:44:31 +0000
+++ lib/lp/registry/browser/tests/private-team-creation-views.txt	2012-02-16 03:21:23 +0000
@@ -172,6 +172,7 @@
     >>> for error in view.errors:
     ...     print view.getFieldError(error.field_name)
     The name 'secret-team' has been blocked by the Launchpad administrators.
+    Contact Launchpad Support if you want to use this name.
 
 
 = Private Team Editing =
@@ -422,4 +423,4 @@
     >>> for error in view.errors:
     ...     print view.getFieldError(error.field_name)
     The name 'private-top-secret' has been blocked by the Launchpad
-    administrators.
+    administrators. Contact Launchpad Support if you want to use this name.

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-02-13 05:50:16 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-02-16 03:21:23 +0000
@@ -556,6 +556,22 @@
                     'visibility',
                     [field.__name__ for field in view.form_fields])
 
+    def test_visibility_is_correct_during_edit(self):
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
+            visibility=PersonVisibility.PRIVATE, owner=owner)
+        product = self.factory.makeProduct(owner=owner)
+        self.factory.makeCommercialSubscription(product)
+        with person_logged_in(owner):
+            url = canonical_url(team)
+        with FeatureFixture(self.feature_flag):
+            browser = self.getUserBrowser(url, user=owner)
+            browser.getLink('Change details').click()
+            self.assertEqual(
+                ['PRIVATE'],
+                browser.getControl(name="field.visibility").value)
+        
 
 class TestTeamMenu(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-02-15 02:24:38 +0000
+++ lib/lp/registry/interfaces/person.py	2012-02-16 03:21:23 +0000
@@ -524,9 +524,6 @@
     """
     errormessage = _("%s is already in use by another person or team.")
 
-    blacklistmessage = _("The name '%s' has been blocked by the Launchpad "
-                         "administrators.")
-
     @property
     def _content_iface(self):
         """Return the interface this field belongs to."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-02-15 00:57:40 +0000
+++ lib/lp/registry/model/person.py	2012-02-16 03:21:23 +0000
@@ -191,6 +191,7 @@
 from lp.registry.interfaces.pillar import IPillarNameSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.ssh import (
     ISSHKey,
     ISSHKeySet,
@@ -290,7 +291,6 @@
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import ILoginTokenSet
 from lp.services.verification.model.logintoken import LoginToken
-from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.dbpolicy import MasterDatabasePolicy
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.worlddata.model.language import Language
@@ -3064,14 +3064,13 @@
         return self.subscriptionpolicy in CLOSED_TEAM_POLICY
 
     def checkAllowVisibility(self):
+        role = IPersonRoles(self)
+        if role.in_commercial_admin or role.in_admin:
+            return True
         feature_flag = getFeatureFlag(
             'disclosure.show_visibility_for_team_add.enabled')
-        if feature_flag:
-            if self.hasCurrentCommercialSubscription():
-                return True
-        else:
-            if check_permission('launchpad.Commercial', self):
-                return True
+        if feature_flag and self.hasCurrentCommercialSubscription():
+            return True
         return False
 
     def transitionVisibility(self, visibility, user):

=== modified file 'lib/lp/registry/templates/people-newteam.pt'
--- lib/lp/registry/templates/people-newteam.pt	2009-08-31 17:46:33 +0000
+++ lib/lp/registry/templates/people-newteam.pt	2012-02-16 03:21:23 +0000
@@ -16,7 +16,6 @@
           language.
         </p>
       </div>
-    <metal:js use-macro="context/@@+person-macros/private-team-js" />
     </div>
   </body>
 </html>

=== modified file 'lib/lp/registry/templates/person-macros.pt'
--- lib/lp/registry/templates/person-macros.pt	2012-02-01 15:31:32 +0000
+++ lib/lp/registry/templates/person-macros.pt	2012-02-16 03:21:23 +0000
@@ -242,40 +242,4 @@
   </tr>
 </metal:macro>
 
-
-<metal:macro define-macro="private-team-js">
-  <tal:comment replace="nothing">
-    This macro inserts the javascript necessary to automatically insert the
-    private team prefix into a name field.  It is here since it is shared in
-    multiple templates.
-  </tal:comment>
-  <tal:script define="private_prefix view/private_prefix|nothing"
-              condition="private_prefix">
-  <script type="text/javascript"
-          tal:content="string:
-    LPJS.use('node', 'event', function(Y) {
-      // Prepend/remove 'private-' from team name based on visibility
-      // setting.  User can choose to edit it back out, if they wish.
-      function visibility_on_change(e) {
-        var visibility = e.target;
-        var prefix = (visibility.get('value') == 'PRIVATE')
-          ? '$private_prefix' : '';
-        // XXX: Brad Crittenden 2009-01-30
-        // bug=http://yuilibrary.com/projects/yui3/ticket/2423101
-        // Dotted CSS selectors not parsed correctly.  Therefore not
-        // using Y.one().
-        var name = document.getElementById('field.name');
-        name.value = prefix + name.value.replace(/^$private_prefix/, '');
-
-      };
-      // Attach the function to the onchange event.
-      // XXX: Brad Crittenden 2009-01-30
-      // bug=http://yuilibrary.com/projects/yui3/ticket/2423101
-      // Dotted CSS selectors not parsed correctly.
-      Y.on('change', visibility_on_change, document.getElementById('field.visibility'));
-    });">
-  </script>
-  </tal:script>
-</metal:macro>
-
 </tal:root>

=== modified file 'lib/lp/registry/templates/team-edit.pt'
--- lib/lp/registry/templates/team-edit.pt	2009-08-17 02:34:16 +0000
+++ lib/lp/registry/templates/team-edit.pt	2012-02-16 03:21:23 +0000
@@ -12,13 +12,8 @@
 
 <body>
   <div metal:fill-slot="main">
-
     <div metal:use-macro="context/@@launchpad_form/form" />
-
-    <metal:js use-macro="context/@@+person-macros/private-team-js" />
-
     <tal:menu replace="structure view/@@+related-pages" />
-
   </div>
 </body>
 </html>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-02-15 00:57:40 +0000
+++ lib/lp/registry/tests/test_person.py	2012-02-16 03:21:23 +0000
@@ -5,31 +5,25 @@
 
 from datetime import datetime
 
+from lazr.lifecycle.snapshot import Snapshot
 import pytz
-
 from storm.store import Store
-
 from testtools.matchers import (
     Equals,
     LessThan,
     )
-
 from zope.component import getUtility
 from zope.interface import providedBy
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
-from lazr.lifecycle.snapshot import Snapshot
-
 from lp.answers.model.answercontact import AnswerContact
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.blueprints.model.specification import Specification
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugtask import get_related_bugtasks_search_params
-from lp.registry.errors import (
-    PrivatePersonLinkageError,
-    )
+from lp.registry.errors import PrivatePersonLinkageError
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from lp.registry.interfaces.person import (
     ImmutableVisibilityError,
@@ -47,16 +41,11 @@
     get_recipients,
     Person,
     )
-from lp.services.identity.interfaces.account import (
-    AccountStatus,
-    )
-from lp.services.identity.interfaces.emailaddress import (
-    EmailAddressStatus,
-    )
+from lp.services.features.testing import FeatureFixture
+from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.propertycache import clear_property_cache
-from lp.soyuz.enums import (
-    ArchivePurpose,
-    )
+from lp.soyuz.enums import ArchivePurpose
 from lp.testing import (
     celebrity_logged_in,
     login,
@@ -71,6 +60,7 @@
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import LaunchpadWebServiceCaller
+from lp.testing.sampledata import ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
 
@@ -560,6 +550,13 @@
         person = self.factory.makePerson()
         self.assertFalse(person.hasCurrentCommercialSubscription())
 
+    def test_commercial_admin_with_ff_checkAllowVisibility(self):
+        admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+        feature_flag = {
+            'disclosure.show_visibility_for_team_add.enabled': 'on'}
+        with FeatureFixture(feature_flag):
+            self.assertTrue(admin.checkAllowVisibility())
+
     def test_can_not_set_visibility(self):
         person = self.factory.makePerson()
         self.assertRaises(

=== modified file 'lib/lp/services/fields/__init__.py'
--- lib/lp/services/fields/__init__.py	2012-01-18 22:32:10 +0000
+++ lib/lp/services/fields/__init__.py	2012-02-16 03:21:23 +0000
@@ -490,6 +490,10 @@
 class BlacklistableContentNameField(ContentNameField):
     """ContentNameField that also checks that a name is not blacklisted"""
 
+    blacklistmessage = _("The name '%s' has been blocked by the Launchpad "
+                         "administrators. Contact Launchpad Support if you "
+                         "want to use this name.")
+
     def _validate(self, input):
         """Check that the given name is valid, unique and not blacklisted."""
         super(BlacklistableContentNameField, self)._validate(input)
@@ -505,9 +509,7 @@
         from lp.registry.interfaces.person import IPersonSet
         user = getUtility(ILaunchBag).user
         if getUtility(IPersonSet).isNameBlacklisted(input, user):
-            raise LaunchpadValidationError(
-                "The name '%s' has been blocked by the Launchpad "
-                "administrators." % input)
+            raise LaunchpadValidationError(self.blacklistmessage % input)
 
 
 class PillarAliases(TextLine):