Merge lp:~sinzui/launchpad/person-owned-teams into lp:launchpad

[Curtis Hovey]

List teams a person owns.

Team owners are not necessarily team members. All code that lists teams
for people are based on membership. There is no way to discover which
teams a person owns. This is a problem for cases where a person leaves
an organisation -- the team owner does not show up when auditing,
and owners can add themselves back to the team to gain team privileges.

RULES

    Pre-implementation: no one
    * Add a method to IPerson to get the teams a person owns. The
      method must take a user argument to filter out private teams the
      observing user cannot see.
    * Add a related teams page to the related packages/projects group
      of pages. The page must batch the teams.

QA

    * Visit https://qastaging.launchpad.net/~
    * Verify there is a link to owned teams.
    * View the link's page.
    * Verify the page lists the teams.

    * Run this script:
    {{{
    from launchpadlib.launchpad import Launchpad
    lp = Launchpad.login_with(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    person = lp.people['sinzui']
    teams = person.getOwnedTeams()
    for team in teams:
        print team.name

    lp = Launchpad.login_anonymously(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    person = lp.people['sinzui']
    teams = person.getOwnedTeams()
    for team in teams:
        print team.name
    }}}

LINT

    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_person.py
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/model/person.py
    lib/lp/registry/templates/person-owned-teams.pt
    lib/lp/registry/templates/person-related-software-navlinks.pt
    lib/lp/registry/tests/test_person.py

LoC

    I have more than 10,000 lines of credit this week.

TEST

    ./bin/test -vc -t OwnedTeams lp.registry

IMPLEMENTATION

I added getOwnedTeams to IPerson that return the teams for a Person that
a user is permitted to see. I exported it to use the requesting user as
the user passed to the method.
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/model/person.py
    lib/lp/registry/tests/test_person.py

I added PersonOwnedTeamsView to the related-software group of pages. I
then added a link to the IPersonRelatedSoftwareMenu to complete the
integration.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_person.py
    lib/lp/registry/templates/person-owned-teams.pt
    lib/lp/registry/templates/person-related-software-navlinks.pt

[Benji York (benji)]

This branch looks good. I had two small observations:

A tad more commentary in PersonOwnedTeamsViewTestCase would be good.

It looks like line 267 of the diff should be dedented two spaces.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2012-12-07 19:59:37 +0000
+++ lib/lp/registry/browser/configure.zcml	2012-12-11 19:03:23 +0000
@@ -909,6 +909,12 @@
             template="../templates/person-related-projects.pt"/>
         <browser:page
             for="lp.registry.interfaces.person.IPerson"
+            permission="zope.Public"
+            class="lp.registry.browser.person.PersonOwnedTeamsView"
+            name="+owned-teams"
+            template="../templates/person-owned-teams.pt"/>
+        <browser:page
+            for="lp.registry.interfaces.person.IPerson"
             class="lp.registry.browser.person.PersonRelatedSoftwareView"
             permission="zope.Public"
             name="+related-software-navlinks"

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-12-07 19:59:53 +0000
+++ lib/lp/registry/browser/person.py	2012-12-11 19:03:23 +0000
@@ -34,6 +34,7 @@
     'PersonNavigation',
     'PersonOAuthTokensView',
     'PersonOverviewMenu',
+    'PersonOwnedTeamsView',
     'PersonRdfContentsView',
     'PersonRdfView',
     'PersonRelatedSoftwareView',
@@ -632,6 +633,11 @@
         enabled = bool(self.person.getAffiliatedPillars(user))
         return Link(target, text, enabled=enabled, icon='info')
 
+    def owned_teams(self):
+        target = '+owned-teams'
+        text = 'Owned teams'
+        return Link(target, text, icon='info')
+
     def subscriptions(self):
         target = '+subscriptions'
         text = 'Direct subscriptions'
@@ -694,6 +700,7 @@
         'activate_ppa',
         'maintained',
         'manage_vouchers',
+        'owned_teams',
         'synchronised',
         'view_ppa_subscriptions',
         'ppa',
@@ -837,7 +844,7 @@
     usedfor = IPersonRelatedSoftwareMenu
     facet = 'overview'
     links = ('related_software_summary', 'maintained', 'uploaded', 'ppa',
-             'synchronised', 'projects')
+             'synchronised', 'projects', 'owned_teams')
 
     @property
     def person(self):
@@ -3778,6 +3785,18 @@
         return "Related projects"
 
 
+class PersonOwnedTeamsView(PersonRelatedSoftwareView):
+    """View for +owned-teams."""
+    page_title = "Owned teams"
+
+    def initialize(self):
+        """Set up the batch navigation."""
+        self.batchnav = BatchNavigator(
+            self.context.getOwnedTeams(self.user), self.request)
+        self.batchnav.setHeadings('team', 'teams')
+        self.batch = list(self.batchnav.currentBatch())
+
+
 class PersonOAuthTokensView(LaunchpadView):
     """Where users can see/revoke their non-expired access tokens."""
 

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2012-12-10 13:43:47 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2012-12-11 19:03:23 +0000
@@ -79,6 +79,7 @@
 from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import (
     extract_text,
+    find_tag_by_id,
     setupBrowserForUser,
     )
 from lp.testing.views import (
@@ -970,6 +971,48 @@
             self.view.max_results_to_display)
 
 
+class PersonOwnedTeamsViewTestCase(TestCaseWithFactory):
+    """Test +owned-teams view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_properties(self):
+        # The batch is created when the view is initialized.
+        owner = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=owner)
+        view = create_initialized_view(owner, '+owned-teams')
+        self.assertEqual('Owned teams', view.page_title)
+        self.assertEqual('team', view.batchnav._singular_heading)
+        self.assertEqual([team], view.batch)
+
+    def test_page_text_with_teams(self):
+        # When the person owns teams, the page shows a a listing
+        # table. There is always a link to the team participation page.
+        owner = self.factory.makePerson(name='snarf')
+        self.factory.makeTeam(owner=owner, name='pting')
+        with person_logged_in(owner):
+            view = create_initialized_view(
+                owner, '+owned-teams', principal=owner)
+            markup = view()
+        soup = find_tag_by_id(markup, 'maincontent')
+        participation_link = 'http://launchpad.dev/~snarf/+participation'
+        self.assertIsNotNone(soup.find('a', {'href': participation_link}))
+        self.assertIsNotNone(soup.find('table', {'id': 'owned-teams'}))
+        self.assertIsNotNone(soup.find('a', {'href': '/~pting'}))
+        self.assertIsNotNone(soup.find('table', {'class': 'upper-batch-nav'}))
+        self.assertIsNotNone(soup.find('table', {'class': 'lower-batch-nav'}))
+
+    def test_page_text_without_teams(self):
+        # When the person does not own teams, the page states the case.
+        owner = self.factory.makePerson(name='pting')
+        with person_logged_in(owner):
+            view = create_initialized_view(
+                owner, '+owned-teams', principal=owner)
+            markup = view()
+        soup = find_tag_by_id(markup, 'maincontent')
+        self.assertIsNotNone(soup.find('p', {'id': 'no-teams'}))
+
+
 class TestPersonSynchronisedPackagesView(TestCaseWithFactory):
     """Test the synchronised packages view."""
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-11-19 21:59:03 +0000
+++ lib/lp/registry/interfaces/person.py	2012-12-11 19:03:23 +0000
@@ -1301,6 +1301,17 @@
         The person's membership may be direct or indirect.
         """
 
+    @call_with(user=REQUEST_USER)
+    @operation_returns_collection_of(Interface)  # Really ITeam.
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getOwnedTeams(user=None):
+        """Return the teams that this person owns.
+
+        The iterator includes the teams that the user owns, but it not
+        a member of.
+        """
+
     def getAdministratedTeams():
         """Return the teams that this person/team is an administrator of.
 
@@ -1308,7 +1319,6 @@
         member with admin privilege, or member of a team with such
         privileges.  It excludes teams which have been merged.
         """
-
     def getTeamAdminsEmailAddresses():
         """Return a set containing the email addresses of all administrators
         of this team.
@@ -2547,6 +2557,8 @@
 #     'lazr.webservice.exported')['return_type'].value_type.schema = IPerson
 IPersonViewRestricted['getMembersByStatus'].queryTaggedValue(
     LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IPerson
+IPersonViewRestricted['getOwnedTeams'].queryTaggedValue(
+    LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = ITeam
 
 # Fix schema of ITeamMembership fields.  Has to be done here because of
 # circular dependencies.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-12-07 10:04:07 +0000
+++ lib/lp/registry/model/person.py	2012-12-11 19:03:23 +0000
@@ -1765,6 +1765,18 @@
         tm.setExpirationDate(expires, reviewer)
         tm.setStatus(status, reviewer, comment=comment)
 
+    def getOwnedTeams(self, user=None):
+        """See `IPerson`."""
+        query = And(
+            get_person_visibility_terms(user),
+            Person.teamowner == self.id,
+            Person.merged == None)
+        store = IStore(Person)
+        results = store.find(
+            Person, query).order_by(
+                Upper(Person.displayname), Upper(Person.name))
+        return results
+
     @cachedproperty
     def administrated_teams(self):
         return list(self.getAdministratedTeams())

=== added file 'lib/lp/registry/templates/person-owned-teams.pt'
--- lib/lp/registry/templates/person-owned-teams.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/templates/person-owned-teams.pt	2012-12-11 19:03:23 +0000
@@ -0,0 +1,70 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  metal:use-macro="view/macro:page/main_only"
+  i18n:domain="launchpad"
+>
+
+<body>
+
+<div metal:fill-slot="heading">
+  <h1 tal:content="view/page_title"/>
+</div>
+
+<div metal:fill-slot="main">
+  <div class="top-portlet">
+    <tal:navlinks replace="structure context/@@+related-software-navlinks"/>
+  </div>
+
+
+  <div id="teams" class="top-portlet">
+    <p>
+      Team owners are not always team members. The
+      <a tal:attributes="href context/menu:overview/memberships/fmt:url">team
+      participation</a> page shows the teams that
+      <tal:name replace="context/displayname" /> is a member of.
+    </p>
+
+    <tal:navigation_top
+         replace="structure view/batchnav/@@+navigation-links-upper" />
+
+    <tal:maintained-packages define="teams view/batch">
+
+      <table id="owned-teams" class="listing"
+        tal:condition="teams">
+        <cols><col width="30%" /><col width="auto" /></cols>
+
+        <thead>
+          <tr>
+            <th>Name</th>
+            <th>Summary</th>
+          </tr>
+        </thead>
+
+        <tbody>
+          <tr tal:repeat="team teams">
+            <td>
+              <a tal:replace="structure team/fmt:link" />
+            </td>
+            <td>
+              <tal:summary replace="team/description/fmt:shorten/60"/>
+            </td>
+          </tr>
+        </tbody>
+      </table>
+
+      <tal:navigation_bottom
+           replace="structure view/batchnav/@@+navigation-links-lower" />
+
+      <p id="no-teams"
+        tal:condition="not: teams">
+        <span tal:replace="context/displayname">Foo Bar</span>
+        doesn't own any teams.
+      </p>
+    </tal:maintained-packages>
+  </div>
+</div>
+</body>
+</html>

=== modified file 'lib/lp/registry/templates/person-related-software-navlinks.pt'
--- lib/lp/registry/templates/person-related-software-navlinks.pt	2011-09-23 07:49:54 +0000
+++ lib/lp/registry/templates/person-related-software-navlinks.pt	2012-12-11 19:03:23 +0000
@@ -29,6 +29,10 @@
         tal:define="link view/menu:navigation/projects"
         tal:condition="link/enabled"
         tal:content="structure link/fmt:link" />
+       <li
+        tal:define="link view/menu:navigation/owned_teams"
+        tal:condition="link/enabled"
+        tal:content="structure link/fmt:link" />
     </ul>
 
 </tal:root>

=== modified file 'lib/lp/registry/templates/product-portlet-requires-subscription.pt'
--- lib/lp/registry/templates/product-portlet-requires-subscription.pt	2012-12-07 19:45:34 +0000
+++ lib/lp/registry/templates/product-portlet-requires-subscription.pt	2012-12-11 19:03:23 +0000
@@ -21,7 +21,7 @@
     <span
       tal:define="user_menu view/user/menu:overview"
       tal:condition="context/qualifies_for_free_hosting">
-      <br />Purchase a commercial subcription from your
+      <br />Purchase a commercial subscription from your
       <a tal:replace="structure user_menu/manage_vouchers/fmt:link" />
       page.
     </span>
@@ -36,7 +36,7 @@
 
     <ul class="bulleted" style="clear:left;">
       <li tal:define="user_menu view/user/menu:overview">
-        Purchase a commercial subcription from your
+        Purchase a commercial subscription from your
         <a tal:replace="structure user_menu/manage_vouchers/fmt:link" />
         page.
       </li>
@@ -45,7 +45,7 @@
         <a class="sprite maybe"
           tal:define="config modules/lp.services.config/config"
           tal:attributes="href config/commercial/licensing_policy_url"
-          >Launchpad's licensing policies</a>
+          >Launchpad's licensing policies</a>.
       </li>
     </ul>
   </tal:proprietary>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-11-26 08:33:03 +0000
+++ lib/lp/registry/tests/test_person.py	2012-12-11 19:03:23 +0000
@@ -70,6 +70,7 @@
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import (
     celebrity_logged_in,
+    launchpadlib_for,
     login,
     login_person,
     logout,
@@ -266,6 +267,49 @@
         retrieved_members = sorted(list(self.a_team.all_members_prepopulated))
         self.assertEqual(expected_members, retrieved_members)
 
+    def test_getOwnedTeams(self):
+        # The iterator contains the teams that person owns, regardless of
+        # membership.
+        owner = self.a_team.teamowner
+        with person_logged_in(owner):
+            owner.leave(self.a_team)
+        results = list(owner.getOwnedTeams(self.user))
+        self.assertEqual([self.a_team], results)
+
+    def test_getOwnedTeams_visibility(self):
+        # The iterator contains the teams that the user can see.
+        owner = self.a_team.teamowner
+        p_team = self.factory.makeTeam(
+            name='p', owner=owner, visibility=PersonVisibility.PRIVATE)
+        results = list(owner.getOwnedTeams(self.user))
+        self.assertEqual([self.a_team], results)
+        results = list(owner.getOwnedTeams(owner))
+        self.assertEqual([self.a_team, p_team], results)
+
+    def test_getOwnedTeams_webservice(self):
+        # The user in the interaction is used as the user arg.
+        owner = self.a_team.teamowner
+        self.factory.makeTeam(
+            name='p', owner=owner, visibility=PersonVisibility.PRIVATE)
+        owner_name = owner.name
+        lp = launchpadlib_for('test', person=self.user)
+        lp_owner = lp.people[owner_name]
+        results = lp_owner.getOwnedTeams()
+        self.assertEqual(['a'], [t.name for t in results])
+
+    def test_getOwnedTeams_webservice_anonymous(self):
+        # The user in the interaction is used as the user arg.
+        # Anonymous scripts also do not reveal private teams.
+        owner = self.a_team.teamowner
+        self.factory.makeTeam(
+            name='p', owner=owner, visibility=PersonVisibility.PRIVATE)
+        owner_name = owner.name
+        logout()
+        lp = launchpadlib_for('test', person=None)
+        lp_owner = lp.people[owner_name]
+        results = lp_owner.getOwnedTeams()
+        self.assertEqual(['a'], [t.name for t in results])
+
     def test_administrated_teams(self):
         # The property Person.administrated_teams is a cached copy of
         # the result of Person.getAdministratedTeams().

=== modified file 'lib/lp/soyuz/stories/soyuz/xx-person-packages.txt'
--- lib/lp/soyuz/stories/soyuz/xx-person-packages.txt	2012-11-19 07:31:45 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-person-packages.txt	2012-12-11 19:03:23 +0000
@@ -20,6 +20,7 @@
     Uploaded packages
     Related PPA packages
     Related projects
+    Owned teams
 
     >>> print browser.getLink("Maintained packages").url
     http://launchpad.dev/~mark/+maintained-packages