Merge lp:~wallyworld/launchpad/rename-private-team-795771 into lp:launchpad

[Ian Booth]

[r=sinzui][bug=795771] Allow private teams to be renamed

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

[Robert Collins (lifeless)]

+ if not has_mailing_list:
+ reason = 'has a PPA'
+ elif not has_ppa:
+ reason = 'has a mailing list'

This made me blink.

Perhaps:
reasons = []
if has_mailing_list:
    reasons.append('has a mailing list')
if has_ppa:
    reasons.append('has a PPA')
assert reasons, 'rejecting but no reasons found!'
reason = ' and '.join(reasons)

[Ian Booth (wallyworld)]

What's there is how the code was originally written, just with one check
for is_private removed. It's not possible to reject for a reason other
than a mailing list or ppa so the assert is not strictly required. The
code below will work but does change the text output if both a ppa and
mailing list exist (a bit more verbose).

I'll change it as suggested and modify any tests accordingly.

On 23/08/11 18:04, Robert Collins wrote:
> + if not has_mailing_list:
> + reason = 'has a PPA'
> + elif not has_ppa:
> + reason = 'has a mailing list'
>
> This made me blink.
>
> Perhaps:
> reasons = []
> if has_mailing_list:
> reasons.append('has a mailing list')
> if has_ppa:
> reasons.append('has a PPA')
> assert reasons, 'rejecting but no reasons found!'
> reason = ' and '.join(reasons)
>

[Steve Kowalik (stevenk)]

At the moment you only check that the widget is editable and then check the error conditions. Please add an end-to-end test that confirms a private team can be renamed.

review: Needs Fixing (code)

[Ian Booth (wallyworld)]

I removed the entire xx-team-edit doc test (replaced with unit tests).
End-end rename test is added, not just for private teams but also teams
with purged mailing lists.

On 24/08/11 11:11, Steve Kowalik wrote:
> Review: Needs Fixing code
> At the moment you only check that the widget is editable and then check the error conditions. Please add an end-to-end test that confirms a private team can be renamed.

[Curtis Hovey (sinzui)]

I am very wary of converting stories into unittests. Most are predicated on issues
that are relevant. A story only needs to show the happy path that demonstrates
that the user can perform a goal. In this case I believe only one browser test
is needs to show that a user can navigate from the team page to the edit page
and submit a change to be returned to the team page.

Do not use TestBrowser (an integration test) to test what a view
does (unittest). Browser tests do not document form contracts or directly
exercise the lines in the view it claims to be testing.

test_team_name_already_used is not testing the view's error message.
It is testing that base-layout-macros, which I am pretty sure is not telling
us anything more than the 100 other tests like this. I image a test that
realley exercises that the view does soemthing like this:

    def test_team_name_already_used(self):
        # If we try to use a name which is already in use, we'll get an error
        # message explaining it.

        self.factory.makeTeam(name="existing")
        owner = self.factory.makePerson()
        team = self.factory.makeTeam(name="team", owner=owner)

        form = {
            'field.name': 'existing'
            # May need more field to document that actual contract the
            # for requires.
            'field.actions.save': 'Save changes',
            }
        login_person(owner)
        view = create_initialized_view(team, '+edit' form=form)
        self.assertEqual(1, len(view.errors))
        notifications = view.request.response.notifications
        self.assertEqual(1, len(notifications))
        self.assertEqual(
            'existing is already in use by another person or team.',
            notifications[0].message)

The page rendering has nothing to do with permission. This is a zope
secuirty rule on a view. Never test with "mark" you do not know why
permission is given. Test that the team owner has permission and other
users do not. Also this ancient test is unaware that registry-experts
is the celebrity that actually does most of the administration today.

from canonical.launchpad.webapp.authorization import check_permission

    def test_edit_team_view_permission(self):
        # Only an administrator (as well as the team owner) of a team can
        # change the details of that team.
        person = self.factory.makePerson()
        team = self.factory.makeTeam()
        view = create_view(team, '+edit')
        login_person(person)
        self.assertFalse(check_permission('launchpad.Edit', view))
        login.person(team.owner)
        self.assertTrue(check_permission('launchpad.Edit', view))

review: Needs Fixing (test)

[Ian Booth (wallyworld)]

On 25/08/11 00:53, Curtis Hovey wrote:
> Review: Needs Fixing test
> I am very wary of converting stories into unittests. Most are predicated on issues
> that are relevant. A story only needs to show the happy path that demonstrates
> that the user can perform a goal. In this case I believe only one browser test
> is needs to show that a user can navigate from the team page to the edit page
> and submit a change to be returned to the team page.
>
> Do not use TestBrowser (an integration test) to test what a view
> does (unittest). Browser tests do not document form contracts or directly
> exercise the lines in the view it claims to be testing.
>

I used the browser because I needed the end-end behaviour to test the
saving of a private team as requested in a review comment. The other
uses were from porting the story code and I didn't know how to do it
using the view. Many thanks for providing the examples to show how.

[Ian Booth (wallyworld)]

> > Do not use TestBrowser (an integration test) to test what a view
> > does (unittest). Browser tests do not document form contracts or directly
> > exercise the lines in the view it claims to be testing.
> >
>
> I used the browser because I needed the end-end behaviour to test the
> saving of a private team as requested in a review comment. The other
> uses were from porting the story code and I didn't know how to do it
> using the view. Many thanks for providing the examples to show how.

Al tests except the end-end test now only use the view, not the browser.

[Curtis Hovey (sinzui)]

Thank you

review: Approve (code)

=== 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-25 01:46:27 +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'
-            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'
+            reasons = []
+            if has_mailing_list:
+                reasons.append('has a mailing list')
+            if has_ppa:
+                reasons.append('has a PPA')
+            reason = ' and '.join(reasons)
             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-25 01:46:27 +0000
@@ -8,16 +8,28 @@
 __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.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.registry.interfaces.mailinglist import MailingListStatus
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    TeamMembershipRenewalPolicy)
 from lp.testing import (
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.views import create_initialized_view
+from lp.testing.views import (
+    create_view,
+    create_initialized_view,
+    )
 
 
 class TestProposedTeamMembersEditView(TestCaseWithFactory):
@@ -141,3 +153,153 @@
         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_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):
+        # Because renaming mailing lists is non-trivial in Mailman 2.1,
+        # renaming teams with mailing lists is prohibited.
+        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)
+
+    def test_cannot_rename_team_with_multiple_reasons(self):
+        # Since public teams can have mailing lists and PPAs simultaneously,
+        # there will be scenarios where more than one of these conditions are
+        # actually blocking the team to be renamed.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        self.factory.makeMailingList(team, 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 mailing list '
+                 'and has a PPA.'),
+                view.widgets['name'].hint)
+
+    def test_edit_team_view_permission(self):
+        # Only an administrator or the team owner of a team can
+        # change the details of that team.
+        person = self.factory.makePerson()
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        view = create_view(team, '+edit')
+        login_person(person)
+        self.assertFalse(check_permission('launchpad.Edit', view))
+        login_person(owner)
+        self.assertTrue(check_permission('launchpad.Edit', view))
+
+    def test_edit_team_view_data(self):
+        # The edit view renders the team's details correctly.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            name="team", displayname='A Team',
+            description="A great team", owner=owner,
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+        with person_logged_in(owner):
+            view = create_initialized_view(team, name="+edit")
+            self.assertEqual('team', view.widgets['name']._data)
+            self.assertEqual(
+                'A Team', view.widgets['displayname']._data)
+            self.assertEqual(
+                'A great team', view.widgets['teamdescription']._data)
+            self.assertEqual(
+                TeamSubscriptionPolicy.MODERATED,
+                view.widgets['subscriptionpolicy']._data)
+            self.assertEqual(
+                TeamMembershipRenewalPolicy.NONE,
+                view.widgets['renewal_policy']._data)
+            self.assertIsNone(view.widgets['defaultrenewalperiod']._data)
+
+    def test_edit_team_view_save(self):
+        # A team can be edited and saved, including a name change, even if it
+        # is a private team and has a purged mailing list.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(
+            name="team", displayname='A Team',
+            description="A great team", owner=owner,
+            visibility=PersonVisibility.PRIVATE,
+            subscription_policy=TeamSubscriptionPolicy.MODERATED)
+
+        with person_logged_in(owner):
+            team_list = self.factory.makeMailingList(team, owner)
+            team_list.deactivate()
+            team_list.transitionToStatus(MailingListStatus.INACTIVE)
+            team_list.purge()
+            url = canonical_url(team)
+        browser = self.getUserBrowser(url, user=owner)
+        browser.getLink('Change details').click()
+        browser.getControl('Name', index=0).value = 'ubuntuteam'
+        browser.getControl('Display Name').value = 'Ubuntu Team'
+        browser.getControl('Team Description').value = ''
+        browser.getControl('Restricted Team').selected = True
+        browser.getControl('Save').click()
+
+        # We're now redirected to the team's home page, which is now on a
+        # different URL since we changed its name.
+        self.assertEqual('http://launchpad.dev/~ubuntuteam', browser.url)
+
+        # Check the values again.
+        browser.getLink('Change details').click()
+        self.assertEqual(
+            'ubuntuteam', browser.getControl('Name', index=0).value)
+        self.assertEqual(
+            'Ubuntu Team', browser.getControl('Display Name', index=0).value)
+        self.assertEqual(
+            '', browser.getControl('Team Description', index=0).value)
+        self.assertTrue(
+            browser.getControl('Restricted Team', index=0).selected)
+
+    def test_team_name_already_used(self):
+        # If we try to use a name which is already in use, we'll get an error
+        # message explaining it.
+
+        self.factory.makeTeam(name="existing")
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(name="team", owner=owner)
+
+        form = {
+            'field.name': 'existing',
+            'field.actions.save': 'Save',
+            }
+        login_person(owner)
+        view = create_initialized_view(team, '+edit', form=form)
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            'existing is already in use by another person or team.',
+            view.errors[0].doc())

=== removed file 'lib/lp/registry/stories/team/xx-team-edit.txt'
--- lib/lp/registry/stories/team/xx-team-edit.txt	2011-08-07 08:04:38 +0000
+++ lib/lp/registry/stories/team/xx-team-edit.txt	1970-01-01 00:00:00 +0000
@@ -1,180 +0,0 @@
-Editing team details
-====================
-
-Only an administrator (as well as the team owner) of a team can change the
-details of that team.
-
-    >>> user_browser.open('http://launchpad.dev/~ubuntu-team/+edit')
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
-
-    >>> browser.addHeader('Authorization', 'Basic [e-mail]:test')
-    >>> browser.open('http://launchpad.dev/~ubuntu-team')
-    >>> browser.getLink('Change details').click()
-    >>> browser.url
-    'http://launchpad.dev/~ubuntu-team/+edit'
-    >>> browser.getControl('Name', index=0).value
-    'ubuntu-team'
-    >>> browser.getControl('Display Name').value
-    'Ubuntu Team'
-    >>> browser.getControl('Team Description').value
-    'This Team is responsible for the Ubuntu Distribution'
-    >>> browser.getControl('Moderated Team').selected
-    True
-    >>> browser.getControl('invite them to apply for renewal').selected
-    True
-    >>> print browser.getControl('Renewal period').value
-
-Now we'll change some values.
-
-    >>> browser.getControl('Name', index=0).value = 'ubuntuteam'
-    >>> browser.getControl('Display Name').value = 'The Ubuntu Team'
-    >>> browser.getControl('Team Description').value = ''
-    >>> browser.getControl('Restricted Team').selected = True
-
-    >>> browser.getControl('Save').click()
-
-We're now redirected to the team's home page, which is now on a different
-URL since we changed its name.
-
-    >>> browser.url
-    'http://launchpad.dev/~ubuntuteam'
-    >>> browser.getLink('Change details').click()
-
-    >>> browser.url
-    'http://launchpad.dev/~ubuntuteam/+edit'
-    >>> browser.getControl('Name', index=0).value
-    'ubuntuteam'
-    >>> browser.getControl('Display Name').value
-    'The Ubuntu Team'
-    >>> browser.getControl('Team Description').value
-    ''
-    >>> browser.getControl('Restricted Team').selected
-    True
-
-If we try to use a name which is already in use, we'll get an error
-message explaining it.
-
-    >>> browser.getControl('Name', index=0).value = 'mark'
-    >>> browser.getControl('Save').click()
-
-    >>> browser.url
-    'http://launchpad.dev/%7Eubuntuteam/+edit'
-    >>> for tag in find_tags_by_class(browser.contents, 'message'):
-    ...     print extract_text(tag)
-    There is 1 error.
-    mark is already in use by another person or team.
-
-
-Teams with mailing lists may not be renamed
--------------------------------------------
-
-Because renaming mailing lists is non-trivial in Mailman 2.1, renaming teams
-with mailing lists is prohibited.  This is done by making the 'name' field of
-such team pages read-only.
-
-    >>> browser.open('http://launchpad.dev/~landscape-developers')
-    >>> browser.getLink(url='+mailinglist').click()
-    >>> browser.getControl('Create new Mailing List').click()
-
-    # Approval of mailing lists is not yet exposed through the web.
-    >>> from lp.registry.model.mailinglist import MailingListSet
-    >>> from canonical.launchpad.ftests import login, logout
-    >>> login('[e-mail]')
-    >>> mailing_list = MailingListSet().get('landscape-developers')
-    >>> mailing_list.syncUpdate()
-    >>> logout()
-
-Because the 'name' field is read-only, it's not a control that we can get
-a hold of to change.
-
-    >>> browser.getLink('Change details').click()
-    >>> browser.getControl(name='field.name').value
-    Traceback (most recent call last):
-    ...
-    LookupError: name 'field.name'
-
-The reason why the field is immutable is displayed in the field
-description.
-
-    >>> print extract_text(
-    ...     first_tag_by_class(browser.contents, 'form'))
-    Name: landscape-developers
-    This team cannot be renamed because it has a mailing list.
-    ...
-
-Of course, teams without mailing lists still have an editable name field.
-
-    >>> print MailingListSet().get('guadamen')
-    None
-    >>> browser.open('http://launchpad.dev/~guadamen')
-    >>> browser.getLink('Change details').click()
-    >>> browser.getControl(name='field.name').value
-    'guadamen'
-
-
-Private teams may not be renamed
---------------------------------
-
-    >>> # Create the necessary teams.
-    >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.person import PersonVisibility
-    >>> owner = factory.makePerson(name='team-owner')
-    >>> priv_team = factory.makeTeam(name='private-team',
-    ...     owner=owner,
-    ...     visibility=PersonVisibility.PRIVATE)
-    >>> logout()
-
-    >>> browser.open('http://launchpad.dev/~private-team')
-    >>> browser.getLink('Change details').click()
-    >>> browser.getControl(name='field.name').value
-    Traceback (most recent call last):
-    ...
-    LookupError: name 'field.name'
-
-The reason why the field is immutable is displayed in the field
-description.
-
-    >>> print extract_text(
-    ...     first_tag_by_class(browser.contents, 'form'))
-    Name: private-team
-    This team cannot be renamed because it is private.
-    ...
-
-
-Multiple conditions blocking team renaming
-------------------------------------------
-
-Since public teams can have mailing lists and PPAs simultaneously,
-there will be scenarios where more than one of these conditions are
-actually blocking the team to be renamed.
-
-The Landscape Developers team already has a mailing list, we will
-also activate its PPA.
-
-    >>> browser.open('http://launchpad.dev/~landscape-developers')
-    >>> browser.getLink('Create a new PPA').click()
-    >>> browser.getControl(
-    ...     name="field.displayname").value = 'Devel PPA'
-    >>> browser.getControl(name="field.accepted").value = True
-    >>> browser.getControl(
-    ...    name="field.description").value = 'Hoohay for Team PPA.'
-    >>> browser.getControl("Activate").click()
-
-Back in the team edit form, the 'name' field remains read-only and has
-a complete explanation about why it cannot be modified.
-
-    >>> browser.getLink('Overview').click()
-    >>> browser.getLink('Change details').click()
-
-    >>> browser.getControl(name='field.name').value
-    Traceback (most recent call last):
-    ...
-    LookupError: name 'field.name'
-
-    >>> print extract_text(
-    ...     first_tag_by_class(browser.contents, 'form'))
-    Name: landscape-developers
-    This team cannot be renamed because it has a mailing list and a PPA.
-    ...

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-16 09:14:07 +0000
+++ lib/lp/testing/factory.py	2011-08-25 01:46:27 +0000
@@ -755,6 +755,7 @@
             address, person, email_status, account)
 
     def makeTeam(self, owner=None, displayname=None, email=None, name=None,
+                 description=None,
                  subscription_policy=TeamSubscriptionPolicy.OPEN,
                  visibility=None, members=None):
         """Create and return a new, arbitrary Team.
@@ -764,6 +765,7 @@
         :type owner: `IPerson` or string
         :param displayname: The team's display name.  If not given we'll use
             the auto-generated name.
+        :param description: Team team's description.
         :type string:
         :param email: The email address to use as the team's contact address.
         :type email: string
@@ -789,7 +791,8 @@
             displayname = SPACE.join(
                 word.capitalize() for word in name.split('-'))
         team = getUtility(IPersonSet).newTeam(
-            owner, name, displayname, subscriptionpolicy=subscription_policy)
+            owner, name, displayname, teamdescription=description,
+            subscriptionpolicy=subscription_policy)
         if visibility is not None:
             # Visibility is normally restricted to launchpad.Commercial, so
             # removing the security proxy as we don't care here.