Merge lp:~bac/launchpad/bug-662994 into lp:launchpad

[Brad Crittenden]

[r=gmb][ui=none][bug=662994] Allow registry experts to change project group name.

= Summary =

A task for CHR and Curtis is to change project group names. The
registry team has never had the ability to do so.

== Proposed fix ==

Move the permission of name from launchpad.Edit to launchpad.Moderate
and from the +edit to the +review page.

== Pre-implementation notes ==

Talk with Curtis

== Implementation details ==

The change described gives ~registry the ability to change a PG name but
removes it from the PG owner. This effect is necessary due to the way
our permissions are partitioned between owners, admins, and registry.
On a positive note, it brings behavior of PG editing in-line with projects.

I also changed the matchers to expose 'Contains' in __all__ which had
been inadvertently left out.

== Tests ==

bin/test -vvm lp.registry -t xx-project-edit.txt -t test_projectgroup

== Demo and Q/A ==

= Launchpad lint =

I will fix these lint issues.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/stories/project/xx-project-edit.txt
  lib/lp/registry/tests/test_projectgroup.py
  lib/lp/registry/stories/object/xx-nameblacklist.txt
  lib/lp/registry/interfaces/projectgroup.py
  lib/lp/registry/browser/tests/test_projectgroup.py
  lib/lp/testing/matchers.py
  lib/lp/registry/browser/project.py

./lib/lp/registry/stories/object/xx-nameblacklist.txt
       7: source has bad indentation.
      14: source exceeds 78 characters.
      20: source has bad indentation.
      24: source exceeds 78 characters.
      30: source has bad indentation.
      39: source has bad indentation.
      56: source has bad indentation.
      61: source has bad indentation.
./lib/lp/testing/matchers.py
     144: E231 missing whitespace after ':'
     236: Line exceeds 78 characters.

[Graham Binns (gmb)]

Hi Brad,

Just a few stylistic nitpicks; nothing major.

74 + view = create_initialized_view(self.project_group, '+index',
75 + principal=user)

Arguments should start on the line after the opening parenthesis,
indented by 4 spaces.

85 + view = create_initialized_view(self.project_group, '+index',
86 + principal=user)

Here too.

95 + view = create_initialized_view(self.project_group, '+index',
96 + principal=user)

And here.

290 + self.assertRaises(Unauthorized,
291 + setattr, self.pg, 'owner', new_owner)

And here.

295 + self.assertRaises(Unauthorized,
296 + setattr, self.pg, 'name', 'new-name')

And here.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/project.py'
--- lib/lp/registry/browser/project.py	2010-12-01 04:27:44 +0000
+++ lib/lp/registry/browser/project.py	2010-12-14 19:21:42 +0000
@@ -350,11 +350,11 @@
 class ProjectEditView(LaunchpadEditFormView):
     """View class that lets you edit a Project object."""
     implements(IProjectGroupEditMenu)
-
     label = "Change project group details"
+    page_title = label
     schema = IProjectGroup
     field_names = [
-        'name', 'displayname', 'title', 'summary', 'description',
+        'displayname', 'title', 'summary', 'description',
         'bug_reporting_guidelines', 'bug_reported_acknowledgement',
         'homepageurl', 'bugtracker', 'sourceforgeproject',
         'freshmeatproject', 'wikiurl']
@@ -388,8 +388,10 @@
         self.field_names = self.default_field_names[:]
         admin = check_permission('launchpad.Admin', self.context)
         if not admin:
+            self.field_names.remove('owner')
+        moderator = check_permission('launchpad.Moderate', self.context)
+        if not moderator:
             self.field_names.remove('name')
-            self.field_names.remove('owner')
         super(ProjectReviewView, self).setUpFields()
         self.form_fields = self._createAliasesField() + self.form_fields
         if admin:

=== added file 'lib/lp/registry/browser/tests/test_projectgroup.py'
--- lib/lp/registry/browser/tests/test_projectgroup.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_projectgroup.py	2010-12-14 19:21:42 +0000
@@ -0,0 +1,84 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for project group views."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+from testtools.matchers import Not
+from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.interfaces import ILaunchBag
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import IPersonSet
+from lp.testing import (
+    celebrity_logged_in,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import (
+    Contains,
+    )
+from lp.testing.sampledata import ADMIN_EMAIL
+from lp.testing.views import create_initialized_view
+
+
+class TestProjectGroupEditView(TestCaseWithFactory):
+    """Tests the edit view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProjectGroupEditView, self).setUp()
+        self.project_group = self.factory.makeProject(name='grupo')
+
+    def test_links_admin(self):
+        # An admin can change details and administer a project group.
+        with celebrity_logged_in('admin'):
+            user = getUtility(ILaunchBag).user
+            view = create_initialized_view(
+                self.project_group, '+index', principal=user)
+            contents = view.render()
+            self.assertThat(contents, Contains("Change details"))
+            self.assertThat(contents, Contains("Administer"))
+
+    def test_links_registry_expert(self):
+        # A registry expert cannot change details but can administer a project
+        # group.
+        with celebrity_logged_in('registry_experts'):
+            user = getUtility(ILaunchBag).user
+            view = create_initialized_view(
+                self.project_group, '+index', principal=user)
+            contents = view.render()
+            self.assertThat(contents, Not(Contains("Change details")))
+            self.assertThat(contents, Contains("Administer"))
+
+    def test_links_owner(self):
+        # An owner can change details but not administer a project group.
+        with person_logged_in(self.project_group.owner):
+            user = getUtility(ILaunchBag).user
+            view = create_initialized_view(
+                self.project_group, '+index', principal=user)
+            contents = view.render()
+            self.assertThat(contents, Contains("Change details"))
+            self.assertThat(contents, Not(Contains("Administer")))
+
+    def test_edit_view_admin(self):
+        admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+        browser = self.getUserBrowser(user=admin)
+        browser.open(canonical_url(self.project_group, view_name='+edit'))
+        browser.open(canonical_url(self.project_group, view_name='+review'))
+
+    def test_edit_view_registry_expert(self):
+        registry_expert = self.factory.makeRegistryExpert()
+        browser = self.getUserBrowser(user=registry_expert)
+        url = canonical_url(self.project_group, view_name='+edit')
+        self.assertRaises(Unauthorized, browser.open, url)
+        browser.open(canonical_url(self.project_group, view_name='+review'))
+
+    def test_edit_view_owner(self):
+        browser = self.getUserBrowser(user=self.project_group.owner)
+        browser.open(canonical_url(self.project_group, view_name='+edit'))
+        url = canonical_url(self.project_group, view_name='+review')
+        self.assertRaises(Unauthorized, browser.open, url)

=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
--- lib/lp/registry/interfaces/projectgroup.py	2010-12-01 20:53:19 +0000
+++ lib/lp/registry/interfaces/projectgroup.py	2010-12-14 19:21:42 +0000
@@ -104,6 +104,15 @@
             title=_('Reviewed'), required=False,
             description=_("Whether or not this project group has been "
                           "reviewed.")))
+    name = exported(
+        ProjectNameField(
+            title=_('Name'),
+            required=True,
+            description=_(
+                "A unique name, used in URLs, identifying the project "
+                "group.  All lowercase, no special characters. "
+                "Examples: apache, mozilla, gimp."),
+            constraint=name_validator))
 
 
 class IProjectGroupPublic(
@@ -133,16 +142,6 @@
             description=_("Project group registrant. Must be a valid "
                           "Launchpad Person.")))
 
-    name = exported(
-        ProjectNameField(
-            title=_('Name'),
-            required=True,
-            description=_(
-                "A unique name, used in URLs, identifying the project "
-                "group.  All lowercase, no special characters. "
-                "Examples: apache, mozilla, gimp."),
-            constraint=name_validator))
-
     displayname = exported(
         TextLine(
             title=_('Display Name'),

=== modified file 'lib/lp/registry/stories/object/xx-nameblacklist.txt'
--- lib/lp/registry/stories/object/xx-nameblacklist.txt	2010-11-03 20:38:32 +0000
+++ lib/lp/registry/stories/object/xx-nameblacklist.txt	2010-12-14 19:21:42 +0000
@@ -4,44 +4,42 @@
 
 Try creating a project with a blacklisted name:
 
->>> admin_browser.open('http://launchpad.dev/projectgroups/+new')
->>> admin_browser.getControl('Name', index=0).value = 'blacklisted'
->>> admin_browser.getControl('Display Name').value = 'Whatever'
->>> admin_browser.getControl('Title').value = 'Whatever'
->>> admin_browser.getControl('Project Group Summary').value = 'Whatever'
->>> admin_browser.getControl('Description').value = 'Whatever'
->>> admin_browser.getControl('Add').click()
->>> "The name 'blacklisted' has been blocked by the Launchpad administrators" \
-...     in admin_browser.contents
-True
+    >>> admin_browser.open('http://launchpad.dev/projectgroups/+new')
+    >>> admin_browser.getControl('Name', index=0).value = 'blacklisted'
+    >>> admin_browser.getControl('Display Name').value = 'Whatever'
+    >>> admin_browser.getControl('Title').value = 'Whatever'
+    >>> admin_browser.getControl('Project Group Summary').value = 'Whatever'
+    >>> admin_browser.getControl('Description').value = 'Whatever'
+    >>> admin_browser.getControl('Add').click()
+    >>> "The name 'blacklisted' has been blocked" in admin_browser.contents
+    True
 
 Try renaming a project to a blacklisted name:
 
->>> admin_browser.open('http://launchpad.dev/mozilla')
->>> admin_browser.getLink('Change details').click()
->>> admin_browser.getControl('Name', index=0).value = 'blacklisted'
->>> admin_browser.getControl('Change Details').click()
->>> "The name 'blacklisted' has been blocked by the Launchpad administrators" \
-...     in admin_browser.contents
-True
+    >>> admin_browser.open('http://launchpad.dev/mozilla')
+    >>> admin_browser.getLink('Administer').click()
+    >>> admin_browser.getControl('Name', index=0).value = 'blacklisted'
+    >>> admin_browser.getControl('Change Details').click()
+    >>> "The name 'blacklisted' has been blocked" in admin_browser.contents
+    True
 
 Same behavior for products:
 
->>> admin_browser.open('http://launchpad.dev/firefox')
->>> admin_browser.getLink('Administer').click()
->>> admin_browser.getControl('Name').value = 'blacklisted'
->>> admin_browser.getControl('Change').click()
->>> "The name 'blacklisted' has been blocked" in admin_browser.contents
-True
+    >>> admin_browser.open('http://launchpad.dev/firefox')
+    >>> admin_browser.getLink('Administer').click()
+    >>> admin_browser.getControl('Name').value = 'blacklisted'
+    >>> admin_browser.getControl('Change').click()
+    >>> "The name 'blacklisted' has been blocked" in admin_browser.contents
+    True
 
 Same behavior for people:
 
->>> admin_browser.open('http://launchpad.dev/~stub')
->>> admin_browser.getLink('Change details').click()
->>> admin_browser.getControl('Name', index=1).value = 'admin42'
->>> admin_browser.getControl('Save').click()
->>> "The name 'admin42' has been blocked" in admin_browser.contents
-True
+    >>> admin_browser.open('http://launchpad.dev/~stub')
+    >>> admin_browser.getLink('Change details').click()
+    >>> admin_browser.getControl('Name', index=1).value = 'admin42'
+    >>> admin_browser.getControl('Save').click()
+    >>> "The name 'admin42' has been blocked" in admin_browser.contents
+    True
 
 Note that it is possible to have an object with a blacklisted name. These
 objects were either created before the blacklist was implemented, or have
@@ -51,15 +49,14 @@
 be a Launchpad Celebrity.
 
 We can edit the details of an object with a blacklisted name quite
-happily without generating 
-
->>> admin_browser.open('http://launchpad.dev/~admins')
->>> admin_browser.getLink('Change details').click()
->>> admin_browser.getControl('Display Name').value = 'Different'
->>> admin_browser.getControl('Save').click()
-
->>> print get_feedback_messages(admin_browser.contents)
-[]
->>> "has been blocked" in admin_browser.contents
-False
-
+happily without generating
+
+    >>> admin_browser.open('http://launchpad.dev/~admins')
+    >>> admin_browser.getLink('Change details').click()
+    >>> admin_browser.getControl('Display Name').value = 'Different'
+    >>> admin_browser.getControl('Save').click()
+
+    >>> print get_feedback_messages(admin_browser.contents)
+    []
+    >>> "has been blocked" in admin_browser.contents
+    False

=== modified file 'lib/lp/registry/stories/project/xx-project-edit.txt'
--- lib/lp/registry/stories/project/xx-project-edit.txt	2010-08-20 18:13:20 +0000
+++ lib/lp/registry/stories/project/xx-project-edit.txt	2010-12-14 19:21:42 +0000
@@ -10,7 +10,6 @@
     Change project group details...
 
     >>> soup = find_main_content(browser.contents)
-    >>> browser.getControl(name='field.name').value = 'new-name'
     >>> browser.getControl('Display Name').value = 'New Name'
     >>> browser.getControl('Title').value = 'New Title'
     >>> browser.getControl('Project Group Summary').value = 'New Summary.'
@@ -20,7 +19,7 @@
     >>> browser.getControl('Change Details').click()
 
     >>> print browser.url
-    http://launchpad.dev/new-name
+    http://launchpad.dev/gnome
 
 Regular users can't access the +review page.
 
@@ -31,18 +30,19 @@
 
 But administrators can access the page:
 
-    >>> admin_browser.open('http://launchpad.dev/new-name')
+    >>> admin_browser.open('http://launchpad.dev/gnome')
     >>> admin_browser.getLink('Administer').click()
 
     >>> print admin_browser.url
-    http://launchpad.dev/new-name/+review
+    http://launchpad.dev/gnome/+review
 
     >>> print admin_browser.title
-    Review upstream project group...
+    Change project group details : New Name
 
-Mark the project as reviewed.
+Mark the project as reviewed and change the name.
 
     >>> admin_browser.getControl('Reviewed').selected = True
+    >>> admin_browser.getControl(name='field.name').value = 'new-name'
     >>> admin_browser.getControl('Change').click()
 
     >>> print admin_browser.url
@@ -148,17 +148,14 @@
     Traceback (most recent call last):
     ...
     LookupError: label 'Registrant'
-    >>> expert_browser.getControl('Name')
-    Traceback (most recent call last):
-    ...
-    LookupError: label 'Name'
 
+    >>> expert_browser.getControl('Name').value = 'newer-name'
     >>> expert_browser.getControl('Aliases').value = 'sleepy'
     >>> expert_browser.getControl('Active').selected = False
     >>> expert_browser.getControl('Reviewed').selected = False
     >>> expert_browser.getControl('Change').click()
 
-    >>> expert_browser.open('http://launchpad.dev/new-name')
+    >>> expert_browser.open('http://launchpad.dev/newer-name')
     >>> expert_browser.getLink('Administer').click()
     >>> print expert_browser.getControl('Aliases').value
     sleepy

=== renamed file 'lib/lp/registry/tests/test_project.py' => 'lib/lp/registry/tests/test_projectgroup.py'
--- lib/lp/registry/tests/test_project.py	2010-12-08 17:22:23 +0000
+++ lib/lp/registry/tests/test_projectgroup.py	2010-12-14 19:21:42 +0000
@@ -7,8 +7,8 @@
 
 from lazr.restfulclient.errors import ClientError
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 
-from canonical.launchpad.ftests import login
 from canonical.launchpad.webapp.errorlog import globalErrorUtility
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
@@ -17,6 +17,7 @@
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
 from lp.testing import (
     launchpadlib_for,
+    login_celebrity,
     login_person,
     TestCaseWithFactory,
     )
@@ -108,6 +109,33 @@
         self.assertEqual(self.project3, results[0])
 
 
+class TestProjectGroupPermissions(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProjectGroupPermissions, self).setUp()
+        self.pg = self.factory.makeProject(name='my-project-group')
+
+    def test_attribute_changes_by_admin(self):
+        login_celebrity('admin')
+        self.pg.name = 'new-name'
+        self.pg.owner = self.factory.makePerson(name='project-group-owner')
+
+    def test_attribute_changes_by_registry_admin(self):
+        login_celebrity('registry_experts')
+        new_owner = self.factory.makePerson(name='project-group-owner')
+        self.pg.name = 'new-name'
+        self.assertRaises(
+            Unauthorized, setattr, self.pg, 'owner', new_owner)
+
+    def test_attribute_changes_by_owner(self):
+        login_person(self.pg.owner)
+        self.assertRaises(
+            Unauthorized, setattr, self.pg, 'name', 'new-name')
+        self.pg.owner = self.factory.makePerson(name='project-group-owner')
+
+
 class TestLaunchpadlibAPI(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -117,8 +145,8 @@
         # is raised when trying to deactivate a project that has source
         # releases.
         last_oops = globalErrorUtility.getLastOopsReport()
+
         launchpad = launchpadlib_for("test", "salgado", "WRITE_PUBLIC")
-
         project = launchpad.projects['evolution']
         project.active = False
         e = self.assertRaises(ClientError, project.lp_save)

=== modified file 'lib/lp/testing/matchers.py'
--- lib/lp/testing/matchers.py	2010-10-19 22:28:51 +0000
+++ lib/lp/testing/matchers.py	2010-12-14 19:21:42 +0000
@@ -3,6 +3,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'Contains',
     'DoesNotCorrectlyProvide',
     'DoesNotProvide',
     'HasQueryCount',
@@ -141,7 +142,7 @@
         for query in self.query_collector.queries:
             result.append(unicode(query).encode('utf8'))
         return {'queries': Content(UTF8_TEXT, lambda:['\n'.join(result)])}
- 
+
 
 class IsNotProxied(Mismatch):
     """An object is not proxied."""