Merge lp:~barry/launchpad/413158-people into lp:launchpad

[Barry Warsaw]

[Barry Warsaw (barry)]

 reviewer sinzui

= Summary =

This branch fixes bug 413158, updating the /people top level page to UI 3.0.

== Proposed fix ==

Update the people-index.pt template and clean up the UI as necessary.

== Pre-implementation notes ==

Curtis and Martin have seen mockups of the page and made comments on the
original bug report. Many of these have been addressed but there are still
imperfection in the page. In the interest of time boxing, we'll deal with
those at a later date.

== Implementation details ==

There are still two known imperfections that I won't fix now. First, the
style-3.0.css isn't quite right for the <th> column headers. In style.css,
these headings are red and bold, but in style-3.0.css they are only red,
though they are also slightly larger. The problem is that the thead styles
are reset in style-3.0.css and although I copied the thead styles over, the
font-weight does not seem to work. I don't know why.

The other imperfection is that there is only one navigation menu, even though
it contains a few actions. This is a legitimate case for having two menus,
but I have been unable to get two menus working. The single menu approach
doesn't look horrible, so I consider this acceptable in the interest of time
boxing the work.

I have refactored the calculation of whether a batch navigation has multiple
pages into the base class, and added a test. Previously, this test was in the
branch listing page, but it's of general usefulness and belongs in the base
class.

The /people/+portlet-about text has been moved inline so this portlet was
removed, as was a test that depended on it. I also moved
/people/+portlet-stats inline and removed that portlet.

I refactored the top-level object menu into a mixin. This menu will be shared
with the top-level projects and project-group pages when I update their UI
next.

I renamed FOAFSearchView to PeopleSearchView since we're not really doing FOAF.

lint complained about some duplicate methods in class
BranchListingItemsMixin. In fact, there's no way the shadowed (i.e. earlier)
methods could possibly be called, so I removed them. I'm about to send the
branch through ec2 so we'll see if they broke anything, but I'd be quite
surprised if they did.

I did some other refactoring and cleanup I found along the way.

== Tests ==

bin/test -vv -t batch_navigation.txt -t people-views.txt

== Demo and Q/A ==

 * Visit http://launchpad.dev/people as an admin, a normal user, and not
   logged in. As an admin you'll get a little extra text in the narrative,
   and as an anonymous user you'll get a 'Create an account' action link.
   This was approved by Martin.

 * As a normal user, visit http://launchpad.dev/people and search for 's' to
   see the multi-page batch navigation.

 * As a normal user, visit http://launchpad.dev/people and search for 'test'
   to see the single-page batch navigation.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt
  lib/lp/registry/browser/m...

[Curtis Hovey (sinzui)]

Hi Barry.

This is very good. I know you had concerns about the menus, but I think they
are close to solving the issues in this branch and in the other top-level
collections. I think a subtle renaming of the marker interface will help you
see what I see. I have a code fragment at the bottom of how I would name and
restructure the menus so that you have something ready for your next branches.

...
> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 11:22:39 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 17:50:02 +0000
> @@ -590,3 +590,10 @@
> word-wrap: break-word;
> }
>
> +/* ==== Listing tables ==== */
> +
> +table.listing thead {
> + color: #9f2b33;

This colour is the bug colour. We need something neutral #333; maybe.

> + font-size: 1.125em;

This font-size is not supported by YUI. We must specify all sizes
using the percentages from the table in this file. try 116% or 123.1%.

...

> === modified file 'lib/canonical/launchpad/webapp/batching.py'
> --- lib/canonical/launchpad/webapp/batching.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/webapp/batching.py 2009-08-14 18:41:41 +0000
> @@ -62,6 +62,10 @@
> def max_batch_size(self):
> return config.launchpad.max_batch_size
>
> + @property
> + def multiple_pages(self):

can you add a docstring? Is has_multiple_pages a better name for what it
returns?

...

> === modified file 'lib/lp/code/browser/branchlisting.py'
> --- lib/lp/code/browser/branchlisting.py 2009-08-13 04:08:38 +0000
> +++ lib/lp/code/browser/branchlisting.py 2009-08-18 14:11:56 +0000
> @@ -331,56 +331,13 @@
> branches = list(self.currentBatch())
> # XXX: TimPenhey 2009-04-08 bug=324546
> # Until there is an API to do this nicely, shove the launchpad.view
> - # permission into the request cache directly. This is done because the
> - # security proxy sometimes causes extra requests to be made.
> + # permission into the request cache directly. This is done because
> + # the security proxy sometimes causes extra requests to be made.
> request = self.view.request
> precache_permission_for_objects(request, 'launchpad.View', branches)
> return branches
>
> @cachedproperty
> - def branch_ids_with_bug_links(self):

I am not sure why this big chunk of code was removed. I this diff lying?

...

> === added file 'lib/lp/registry/browser/menu.py'
> --- lib/lp/registry/browser/menu.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/browser/menu.py 2009-08-18 13:54:51 +0000
> @@ -0,0 +1,40 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Shared menus."""
> +
> +__metaclass__ = type
> +__all__ = [
> + 'TopLevelMenuMixin',
> + ]
> +
> +
> +from canonical.launchpad.webapp.menu import Link
> +
> +
> +class TopLevelMenuMixin:
> + """Menu shared by top level collection objects."""

I think I would prefer this in __init__.py where other shared code
is? Since these collections are in the root of the mainsit...

review: Needs Fixing (code)

[Barry Warsaw (barry)]

On Aug 18, 2009, at 07:11 PM, Curtis Hovey wrote:

>This is very good. I know you had concerns about the menus, but I think they
>are close to solving the issues in this branch and in the other top-level
>collections. I think a subtle renaming of the marker interface will help you
>see what I see. I have a code fragment at the bottom of how I would name and
>restructure the menus so that you have something ready for your next branches.

Yep, it's been a frustrating branch; thanks for talking me off the ledge. Thanks for the review and it's definitely encouraging to see how close this branch puts me to completing the other top level collections.

>...
>> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
>> --- lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 11:22:39 +0000
>> +++ lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 17:50:02 +0000
>> @@ -590,3 +590,10 @@
>> word-wrap: break-word;
>> }
>>
>> +/* ==== Listing tables ==== */
>> +
>> +table.listing thead {
>> + color: #9f2b33;
>
>This colour is the bug colour. We need something neutral #333; maybe.

I never made the correlation between the heading color and the app color. I thought it was a general heading cue. If not, then I think the default color setting is just fine. You provided the hint for bolding the th in a comment on the bug and that works better, so I'll keep that and remove the color setting.

>> + font-size: 1.125em;
>
>This font-size is not supported by YUI. We must specify all sizes
>using the percentages from the table in this file. try 116% or 123.1%.

116% looks good to me.

>> === modified file 'lib/canonical/launchpad/webapp/batching.py'
>> --- lib/canonical/launchpad/webapp/batching.py 2009-06-25 05:30:52 +0000
>> +++ lib/canonical/launchpad/webapp/batching.py 2009-08-14 18:41:41 +0000
>> @@ -62,6 +62,10 @@
>> def max_batch_size(self):
>> return config.launchpad.max_batch_size
>>
>> + @property
>> + def multiple_pages(self):
>
>can you add a docstring? Is has_multiple_pages a better name for what it
>returns?

Docstring added. 'multiple_pages' was the original name for the property before refactoring, but I agree that 'has_multiple_pages' is a better name. So changed.

>> === modified file 'lib/lp/code/browser/branchlisting.py'
>> --- lib/lp/code/browser/branchlisting.py 2009-08-13 04:08:38 +0000
>> +++ lib/lp/code/browser/branchlisting.py 2009-08-18 14:11:56 +0000
>> @@ -331,56 +331,13 @@
>> branches = list(self.currentBatch())
>> # XXX: TimPenhey 2009-04-08 bug=324546
>> # Until there is an API to do this nicely, shove the launchpad.view
>> - # permission into the request cache directly. This is done because the
>> - # security proxy sometimes causes extra requests to be made.
>> + # permission into the request cache directly. This is done because
>> + # the security proxy sometimes causes extra requests to be made.
>> request = self.view.request
>> precache_permission_for_objects(request, 'launchpad.View', branches)
>> return branches
>>
>> @cachedproperty
>> - def branch_ids_with_bug_links(self):
>
>I am not...

[Curtis Hovey (sinzui)]

Hi Barry.

This looks good. I see have a small concern about one menu and I think
some of the links need to be handled differently.

On Wed, 2009-08-19 at 00:03 +0000, Barry Warsaw wrote:
...

>> + def meetings(self):
> >> + return Link('/sprints/', 'View meetings', icon='info')
> >
> >Seeing this really make me think we need to separate sprints from
> >blueprints. I am certain this link belongs to the mainsite (which
> >is registry)
>
> mainsite seems reasonable. Is there something you want me to change here?
>
No. This a remark. I recently realised how these were implement and I was offended. I think this is a great feature that is hard to find and use. Tim did not know this existed until I showed him yesterday.

...

>> === modified file 'lib/lp/registry/browser/person.py'
> >> --- lib/lp/registry/browser/person.py 2009-08-03 15:21:35 +0000
> >> +++ lib/lp/registry/browser/person.py 2009-08-18 15:28:28 +0000
>
...

> implements(IRegistryCollectionNavigationMenu)
> >
> >So you have one marker interface, and one implementation below that
> >is used in 5 collections. Your other branches will be much smaller.
> >I think this is what you intend to do placed the like definitions in
> >a shared place.
>
> Well, now I have my justification for menu.py! :)
>
I think you are correct.

...

>These look like the menus we want to build.
> >
> >{{{
> >
> >class PersonSetNavigationMenu(NavigationMenu):
> > """Navigation menu for PersonSet actions"""
> > usedfor = IPersonSet
> > links = [
> > register_team','adminpeoplemerge', 'adminteammerge', 'mergeaccounts']
> >
> >
> >class IRegistryCollectionNavigationMenu(Interface):
> > """Marker interface for the registry collection navigation menu."""
> >
> >
> >class RegistryCollectionNavigationMenu(NavigationMenu, TopLevelMenuMixin):
> > """Navigation menu for registry person search collection."""
> > usedfor = IRegistryCollectionNavigationMenu
> > facet = 'overview'
> > links = ['products', 'distributions', 'people', 'meetings']
> >
> >
> >class PeopleSearchView(LaunchpadView):
> > """Search for people and teams on the /people page."""
> > implements(IRegistryCollectionNavigationMenu)
> >
> >}}}
>
> Nice. This works but with a small ugliness. Because `usedfor` in the action
> menu must correspond to the context object's interface, it can't be completely
> generalized. It will force each top level collection view to create a
> subclass containing only the `usedfor` attribute, set to the right interface.
> Icky, but close enough for jazz.
>
No. I do not see that. We created the one shared menu (RegistryCollectionNavigationMenu). All the views (that already exist) just need to add the implements.

I am not sure of the value of RegistryCollectionActionMenuBase. I do not think those links are usable between collections. Those links belong to the IPersonSet only. You might as well move those links to the one true implementing class PersonSetNavigationMenu

Doing this does feel like a hack because it would be better to register both
> menus on the view instead of one on the view and one on the context object.
> But it's a hack that will live another day be...

review: Needs Fixing (code)

[Barry Warsaw (barry)]

On Aug 19, 2009, at 07:30 PM, Curtis Hovey wrote:

>This looks good. I see have a small concern about one menu and I think
>some of the links need to be handled differently.

Okay. Hopefully we're close!

>>These look like the menus we want to build.
>> >
>> >{{{
>> >
>> >class PersonSetNavigationMenu(NavigationMenu):
>> > """Navigation menu for PersonSet actions"""
>> > usedfor = IPersonSet
>> > links = [
>> > register_team','adminpeoplemerge', 'adminteammerge', 'mergeaccounts']
>> >
>> >
>> >class IRegistryCollectionNavigationMenu(Interface):
>> > """Marker interface for the registry collection navigation menu."""
>> >
>> >
>> >class RegistryCollectionNavigationMenu(NavigationMenu, TopLevelMenuMixin):
>> > """Navigation menu for registry person search collection."""
>> > usedfor = IRegistryCollectionNavigationMenu
>> > facet = 'overview'
>> > links = ['products', 'distributions', 'people', 'meetings']
>> >
>> >
>> >class PeopleSearchView(LaunchpadView):
>> > """Search for people and teams on the /people page."""
>> > implements(IRegistryCollectionNavigationMenu)
>> >
>> >}}}
>>
>> Nice. This works but with a small ugliness. Because `usedfor` in the action
>> menu must correspond to the context object's interface, it can't be completely
>> generalized. It will force each top level collection view to create a
>> subclass containing only the `usedfor` attribute, set to the right interface.
>> Icky, but close enough for jazz.
>>
>No. I do not see that. We created the one shared menu (RegistryCollectionNavigationMenu). All the views (that already exist) just need to add the implements.
>
>I am not sure of the value of RegistryCollectionActionMenuBase. I do not think those links are usable between collections. Those links belong to the IPersonSet only. You might as well move those links to the one true implementing class PersonSetNavigationMenu

I'm not so sure about that. Certainly, the admin_merge_* links pertain only to IPersonSet, but that's easily controlled by setting the 'links' attribute. In fact, that's exactly the refactoring I've done in my /projects branch, so I don't want to do it here. But I do think the register_projects and register_teams link make sense on all the top level collections, and I don't think we need to filter that further for the context. Also, create_account when anonymous makes sense on all the collections, so I think that justifies RegistryCollectionActionMenuBase.

>
>Doing this does feel like a hack because it would be better to register both
>> menus on the view instead of one on the view and one on the context object.
>> But it's a hack that will live another day because I'm not going to worry
>> about it any more. :)
>>
>Creating a base that has only one user is a waste, remove it.

It won't be a waste when you see the /projects branch.

>> === modified file 'lib/lp/registry/browser/menu.py'
>> --- lib/lp/registry/browser/menu.py 2009-08-18 13:54:51 +0000
>> +++ lib/lp/registry/browser/menu.py 2009-08-18 23:27:23 +0000
>
>...
>
>> @@ -27,14 +34,72 @@
>> def meetings(self):
>> return Link('/sprints/', 'View meetings', icon='info')
>>
>> +...

[Curtis Hovey (sinzui)]

Hi barry.

Your changes look great. I accept you arguments to keep the base menu.

  review approve

review: Approve

=== modified file 'lib/canonical/launchpad/doc/batch_navigation.txt'
--- lib/canonical/launchpad/doc/batch_navigation.txt	2009-03-24 12:43:49 +0000
+++ lib/canonical/launchpad/doc/batch_navigation.txt	2009-08-14 18:41:41 +0000
@@ -1,4 +1,6 @@
-= Batch Navigation =
+================
+Batch Navigation
+================
 
 Most of the test for this behavior is in the lazr.batchnavigation package.
 
@@ -44,10 +46,12 @@
   >>> reindeer = ['Dasher', 'Dancer', 'Prancer', 'Vixen', 'Comet',
   ...     'Cupid', 'Donner', 'Blitzen', 'Rudolph']
 
-== Performance with SQLObject ==
-
-This section demonstrates that batching generates sensible SQL queries when used
-with SQLObject, i.e. that it puts appropriate LIMIT clauses on queries.
+
+Performance with SQLObject
+==========================
+
+This section demonstrates that batching generates sensible SQL queries when
+used with SQLObject, i.e. that it puts appropriate LIMIT clauses on queries.
 
 Imports:
 
@@ -58,8 +62,7 @@
 Prepare a query, and create a batch of the results:
 
   >>> select_results = EmailAddress.select(orderBy='id')
-  >>> batch_nav = BatchNavigator(select_results, build_request(),
-  ...                            size=10)
+  >>> batch_nav = BatchNavigator(select_results, build_request(), size=10)
   >>> email_batch = batch_nav.currentBatch()
   >>> batch_items = list(email_batch)
 
@@ -85,7 +88,17 @@
   True
 
 
-== Maximum batch size ==
+Multiple pages
+==============
+
+The batch navigator tells us whether multiple pages will be used.
+
+    >>> batch_nav.multiple_pages
+    True
+
+
+Maximum batch size
+==================
 
 Since the batch size is exposed in the URL, it's possible for users to
 tweak the batch parameter to retrieve more results. Since that may
@@ -108,7 +121,8 @@
     >>> ignored = config.pop('max-batch-size')
 
 
-== Batch views ==
+Batch views
+===========
 
 A view is often used with a BatchNavigator to determine when to
 display the current batch.
 
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2009-08-17 11:22:39 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2009-08-17 17:50:02 +0000
@@ -590,3 +590,10 @@
   word-wrap: break-word;
 }
 
+/* ==== Listing tables ==== */
+
+table.listing thead {
+  color: #9f2b33;
+  font-size: 1.125em;
+  font-weight: bold;
+}
 
=== modified file 'lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt'
--- lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2009-08-13 19:20:04 +0000
+++ lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2009-08-17 17:50:02 +0000
@@ -368,7 +368,6 @@
 >>> check("/people/")
 >>> check("/people/+requestmerge", auth=True)
 >>> check_redirect("/people/+mergerequest-sent", auth=True)
->>> check("/people/+portlet-about")
 
 This is for a team:
 
 
=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2009-08-14 18:41:41 +0000
@@ -62,6 +62,10 @@
     def max_batch_size(self):
         return config.launchpad.max_batch_size
 
+    @property
+    def multiple_pages(self):
+        return self.batch.total() > self.batch.size
+
 
 class TableBatchNavigator(BatchNavigator):
     """See canonical.launchpad.interfaces.ITableBatchNavigator."""
 
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2009-08-13 04:08:38 +0000
+++ lib/lp/code/browser/branchlisting.py	2009-08-18 14:11:56 +0000
@@ -331,56 +331,13 @@
         branches = list(self.currentBatch())
         # XXX: TimPenhey 2009-04-08 bug=324546
         # Until there is an API to do this nicely, shove the launchpad.view
-        # permission into the request cache directly.  This is done because the
-        # security proxy sometimes causes extra requests to be made.
+        # permission into the request cache directly.  This is done because
+        # the security proxy sometimes causes extra requests to be made.
         request = self.view.request
         precache_permission_for_objects(request, 'launchpad.View', branches)
         return branches
 
     @cachedproperty
-    def branch_ids_with_bug_links(self):
-        """Return a set of branch ids that should show bug badges."""
-        bug_branches = getUtility(IBugBranchSet).getBugBranchesForBranches(
-            self._branches_for_current_batch, self.view.user)
-        return set(bug_branch.branch.id for bug_branch in bug_branches)
-
-    @cachedproperty
-    def branch_ids_with_spec_links(self):
-        """Return a set of branch ids that should show blueprint badges."""
-        spec_branches = getUtility(
-            ISpecificationBranchSet).getSpecificationBranchesForBranches(
-            self._branches_for_current_batch, self.view.user)
-        return set(spec_branch.branch.id for spec_branch in spec_branches)
-
-    @cachedproperty
-    def branch_ids_with_merge_proposals(self):
-        """Return a set of branches that should show merge proposal badges.
-
-        Branches have merge proposals badges if they've been proposed for
-        merging into another branch (source branches).
-        """
-        branches = self.view._getCollection().visibleByUser(self.view.user)
-        proposals = branches.getMergeProposals(
-            for_branch=self._branch_for_current_batch)
-        return set(proposal.source_branch.id for proposal in proposals)
-
-    @cachedproperty
-    def tip_revisions(self):
-        """Return a set of branch ids that should show blueprint badges."""
-        revisions = getUtility(IRevisionSet).getTipRevisionsForBranches(
-            self._branches_for_current_batch)
-        if revisions is None:
-            revision_map = {}
-        else:
-            # Key the revisions by revision id.
-            revision_map = dict((revision.revision_id, revision)
-                                for revision in revisions)
-
-        # Return a dict keyed on branch id.
-        return dict((branch.id, revision_map.get(branch.last_scanned_id))
-                     for branch in self._branches_for_current_batch)
-
-    @cachedproperty
     def product_series_map(self):
         """Return a map of branch id to a list of product series.
 
@@ -526,10 +483,6 @@
         """Return a list of BranchListingItems."""
         return self.decoratedBranches(self.visible_branches_for_view)
 
-    @cachedproperty
-    def multiple_pages(self):
-        return self.batch.total() > self.batch.size
-
     @property
     def table_class(self):
         # XXX: MichaelHudson 2007-10-18 bug=153894: This means there are two
 
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-08-14 00:52:28 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-08-18 13:54:51 +0000
@@ -794,18 +794,20 @@
         <browser:menus
             module="lp.registry.browser.person"
             classes="
+                PeopleSearchNavigationMenu
+                PersonBugsMenu
+                PersonEditNavigationMenu
                 PersonFacets
                 PersonOverviewMenu
-                TeamOverviewMenu
-                PersonBugsMenu
+                PersonOverviewNavigationMenu
+                PersonRelatedSoftwareNavigationMenu
+                PersonSetContextMenu
                 PersonSpecsMenu
                 TeamBugsMenu
+                TeamOverviewMenu
+                TeamOverviewNavigationMenu
                 TeamSpecsMenu
-                PersonSetContextMenu
-                PersonOverviewNavigationMenu
-                PersonEditNavigationMenu
-                PersonRelatedSoftwareNavigationMenu
-                TeamOverviewNavigationMenu"/>
+                "/>
         <browser:url
             for="lp.registry.interfaces.person.IPerson"
             path_expression="string:~${name}"
@@ -1242,13 +1244,10 @@
         <browser:pages
             for="lp.registry.interfaces.person.IPersonSet"
             permission="zope.Public"
-            class="lp.registry.browser.person.FOAFSearchView">
+            class="lp.registry.browser.person.PeopleSearchView">
             <browser:page
                 name="+index"
                 template="../templates/people-index.pt"/>
-            <browser:page
-                name="+portlet-stats"
-                template="../templates/people-portlet-stats.pt"/>
         </browser:pages>
         <browser:page
             name="+requestmerge"
@@ -1284,13 +1283,6 @@
                 name="+mergerequest-sent"
                 template="../templates/people-mergerequest-sent.pt"/>
         </browser:pages>
-        <browser:pages
-            for="lp.registry.interfaces.person.IPersonSet"
-            permission="zope.Public">
-            <browser:page
-                name="+portlet-about"
-                template="../templates/people-portlet-about.pt"/>
-        </browser:pages>
     </facet>
     <browser:navigation
         module="lp.registry.browser.milestone"
 
=== added file 'lib/lp/registry/browser/menu.py'
--- lib/lp/registry/browser/menu.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/menu.py	2009-08-18 13:54:51 +0000
@@ -0,0 +1,40 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Shared menus."""
+
+__metaclass__ = type
+__all__ = [
+    'TopLevelMenuMixin',
+    ]
+
+
+from canonical.launchpad.webapp.menu import Link
+
+
+class TopLevelMenuMixin:
+    """Menu shared by top level collection objects."""
+
+    def products(self):
+        return Link('/projects/', 'View projects', icon='info')
+
+    def distributions(self):
+        return Link('/distros/', 'View distributions', icon='info')
+
+    def people(self):
+        return Link('/people/', 'View people', icon='info')
+
+    def meetings(self):
+        return Link('/sprints/', 'View meetings', icon='info')
+
+    def register_project(self):
+        text = 'Register a project'
+        return Link('/projects/+new', text, icon='add')
+
+    def register_team(self):
+        text = 'Register a team'
+        return Link('/people/+newteam', text, icon='add')
+
+    def create_account(self):
+        text = 'Create an account'
+        return Link('/people/+login', text, icon='add')
 
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2009-08-03 15:21:35 +0000
+++ lib/lp/registry/browser/person.py	2009-08-18 15:28:28 +0000
@@ -10,11 +10,12 @@
 __all__ = [
     'BeginTeamClaimView',
     'BugSubscriberPackageBugsSearchListingView',
-    'FOAFSearchView',
     'EmailToPersonView',
+    'PeopleSearchMenu'
+    'PeopleSearchView',
     'PersonAccountAdministerView',
+    'PersonAddView',
     'PersonAdministerView',
-    'PersonAddView',
     'PersonAnswerContactForView',
     'PersonAnswersMenu',
     'PersonAssignedBugTaskSearchListingView',
@@ -30,10 +31,10 @@
     'PersonEditHomePageView',
     'PersonEditIRCNicknamesView',
     'PersonEditJabberIDsView',
+    'PersonEditLocationView',
     'PersonEditSSHKeysView',
     'PersonEditView',
     'PersonEditWikiNamesView',
-    'PersonEditLocationView',
     'PersonFacets',
     'PersonGPGView',
     'PersonIndexView',
@@ -42,8 +43,8 @@
     'PersonNavigation',
     'PersonOAuthTokensView',
     'PersonOverviewMenu',
+    'PersonRdfContentsView',
     'PersonRdfView',
-    'PersonRdfContentsView',
     'PersonRelatedBugTaskSearchListingView',
     'PersonRelatedSoftwareView',
     'PersonReportedBugTaskSearchListingView',
@@ -51,9 +52,9 @@
     'PersonSetContextMenu',
     'PersonSetNavigation',
     'PersonSpecFeedbackView',
+    'PersonSpecWorkloadTableView',
+    'PersonSpecWorkloadView',
     'PersonSpecsMenu',
-    'PersonSpecWorkloadView',
-    'PersonSpecWorkloadTableView',
     'PersonSubscribedBugTaskSearchListingView',
     'PersonView',
     'PersonVouchersView',
@@ -66,14 +67,14 @@
     'SearchNeedAttentionQuestionsView',
     'SearchSubscribedQuestionsView',
     'TeamAddMyTeamsView',
+    'TeamBreadcrumbBuilder',
     'TeamEditLocationView',
     'TeamJoinView',
-    'TeamBreadcrumbBuilder',
     'TeamLeaveView',
+    'TeamMembershipView',
+    'TeamMugshotView',
     'TeamNavigation',
     'TeamOverviewMenu',
-    'TeamMembershipView',
-    'TeamMugshotView',
     'TeamReassignmentView',
     'TeamSpecsMenu',
     'archive_to_person',
@@ -198,6 +199,7 @@
 from canonical.launchpad.browser.branding import BrandingChangeView
 from lp.registry.browser.mailinglists import (
     enabled_with_active_mailing_list)
+from lp.registry.browser.menu import TopLevelMenuMixin
 from lp.answers.browser.questiontarget import SearchQuestionsView
 
 from canonical.launchpad.fields import LocationField
@@ -627,29 +629,14 @@
             canonical_url(me, request=self.request), status=303)
 
 
-class PersonSetContextMenu(ContextMenu):
+class PersonSetContextMenu(ContextMenu, TopLevelMenuMixin):
 
     usedfor = IPersonSet
 
-    links = ['products', 'distributions', 'people', 'meetings', 'newteam',
+    links = ['products', 'distributions', 'people', 'meetings',
+             'register_team',
              'adminpeoplemerge', 'adminteammerge', 'mergeaccounts']
 
-    def products(self):
-        return Link('/projects/', 'View projects')
-
-    def distributions(self):
-        return Link('/distros/', 'View distributions')
-
-    def people(self):
-        return Link('/people/', 'View people')
-
-    def meetings(self):
-        return Link('/sprints/', 'View meetings')
-
-    def newteam(self):
-        text = 'Register a team'
-        return Link('+newteam', text, icon='add')
-
     def mergeaccounts(self):
         text = 'Merge accounts'
         return Link('+requestmerge', text, icon='edit')
@@ -1348,35 +1335,64 @@
         return self.proposed_memberships or self.invited_memberships
 
 
-class FOAFSearchView:
+class IPeopleSearchNavigationMenu(Interface):
+    """Marker interface for people search navigation menu."""
+
+
+class PeopleSearchNavigationMenu(NavigationMenu, TopLevelMenuMixin):
+    """Navigation menu for people search."""
+
+    usedfor = IPeopleSearchNavigationMenu
+    facet = 'overview'
+
+    links = ['products', 'distributions', 'people', 'meetings',
+             'register_team', 'register_project']
+
+    def initialize(self):
+        """See `MenuBase`."""
+        if self.context.user is None:
+            # Make a copy of the links so as not to permanently mutate the
+            # class attribute.
+            self.links = PeopleSearchNavigationMenu.links[:]
+            self.links.append('create_account')
+
+
+class PeopleSearchView(LaunchpadView):
+    """Search for people and teams on the /people page."""
+
+    implements(IPeopleSearchNavigationMenu)
 
     def __init__(self, context, request):
-        self.context = context
-        self.request = request
+        super(PeopleSearchView, self).__init__(context, request)
         self.results = []
 
-    def teamsCount(self):
-        return getUtility(IPersonSet).teamsCount()
+    @property
+    def number_of_people(self):
+        return self.context.peopleCount()
 
-    def peopleCount(self):
-        return getUtility(IPersonSet).peopleCount()
+    @property
+    def number_of_teams(self):
+        return self.context.teamsCount()
 
     def searchPeopleBatchNavigator(self):
         name = self.request.get("name")
-
         if not name:
             return None
-
         searchfor = self.request.get("searchfor")
         if searchfor == "peopleonly":
-            results = getUtility(IPersonSet).findPerson(name)
+            results = self.context.findPerson(name)
         elif searchfor == "teamsonly":
-            results = getUtility(IPersonSet).findTeam(name)
+            results = self.context.findTeam(name)
         else:
-            results = getUtility(IPersonSet).find(name)
-
+            results = self.context.find(name)
         return BatchNavigator(results, self.request)
 
+    @property
+    def is_admin(self):
+        """Is the logged in user a Launchpad administrator?"""
+        return (self.user is not None and
+                self.user.inTeam(getUtility(ILaunchpadCelebrities).admin))
+
 
 class PersonAddView(LaunchpadFormView):
     """The page where users can create new Launchpad profiles."""
 
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2009-08-12 13:20:10 +0000
+++ lib/lp/registry/browser/product.py	2009-08-18 13:54:51 +0000
@@ -82,6 +82,7 @@
 from lp.bugs.browser.bugtask import (
     BugTargetTraversalMixin, get_buglisting_search_filter_url)
 from lp.registry.browser.distribution import UsesLaunchpadMixin
+from lp.registry.browser.menu import TopLevelMenuMixin
 from lp.answers.browser.faqtarget import FAQTargetNavigationMixin
 from canonical.launchpad.browser.feeds import FeedsMixin
 from lp.registry.browser.productseries import get_series_branch_error
@@ -546,40 +547,17 @@
     enable_only = ['overview']
 
 
-class ProductSetContextMenu(ContextMenu):
+class ProductSetContextMenu(ContextMenu, TopLevelMenuMixin):
 
     usedfor = IProductSet
 
     links = ['products', 'distributions', 'people', 'meetings',
-             'all', 'register', 'register_team', 'review_licenses']
-
-    def register(self):
-        text = 'Register a project'
-        # We link to the guided form, though people who know the URL can
-        # just jump to +new directly. That might be considered a
-        # feature!
-        return Link('+new', text, icon='add')
-
-    def register_team(self):
-        text = 'Register a team'
-        return Link('/people/+newteam', text, icon='add')
+             'all', 'register_project', 'register_team', 'review_licenses']
 
     def all(self):
         text = 'List all projects'
         return Link('+all', text, icon='list')
 
-    def products(self):
-        return Link('/projects/', 'View projects')
-
-    def distributions(self):
-        return Link('/distros/', 'View distributions')
-
-    def people(self):
-        return Link('/people/', 'View people')
-
-    def meetings(self):
-        return Link('/sprints/', 'View meetings')
-
     @enabled_with_permission('launchpad.ProjectReview')
     def review_licenses(self):
         return Link('+review-licenses', 'Review projects')
 
=== added file 'lib/lp/registry/browser/tests/people-views.txt'
--- lib/lp/registry/browser/tests/people-views.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/people-views.txt	2009-08-14 17:31:10 +0000
@@ -0,0 +1,70 @@
+==============================
+Searching for people and teams
+==============================
+
+The view behind the /people page provides searching for people and teams.  The
+view knows how many people and teams are currently registered in Launchpad.
+
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> login('[e-mail]')
+    >>> person_set = getUtility(IPersonSet)
+
+    >>> view = create_initialized_view(person_set, '+index')
+    >>> view.number_of_people
+    48
+    >>> view.number_of_teams
+    17
+
+The view also knows whether the logged in user, such as Foo Bar is a Launchpad
+administrator or not.
+
+    >>> view.is_admin
+    True
+
+But the anonymous user is not an administrator.
+
+    >>> logout()
+    >>> view = create_initialized_view(person_set, '+index')
+    >>> view.is_admin
+    False
+
+Sample Person is also not an administrator.
+
+    >>> login('[e-mail]')
+    >>> view = create_initialized_view(person_set, '+index')
+    >>> view.is_admin
+    False
+
+
+Batch navigator
+===============
+
+The view returns a batch navigator for searching through sets of teams,
+people, or both.  By default, both are searched for the given name.  There is
+one person and one team matching the 'test' string.
+
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> def print_batch(batch):
+    ...     for thing in batch.currentBatch():
+    ...         naked_thing = removeSecurityProxy(thing)
+    ...         print naked_thing
+
+    >>> form = dict(name='test')
+    >>> view = create_initialized_view(person_set, '+index', form=form)
+    >>> print_batch(view.searchPeopleBatchNavigator())
+    <Person at ... name12 (Sample Person)>
+    <Person at ... testing-spanish-team (testing Spanish team)>
+
+Searching for just people returns Sample Person.
+
+    >>> form['searchfor'] = 'peopleonly'
+    >>> view = create_initialized_view(person_set, '+index', form=form)
+    >>> print_batch(view.searchPeopleBatchNavigator())
+    <Person at ... name12 (Sample Person)>
+
+Searching for just teams returns the testing Spanish team.
+
+    >>> form['searchfor'] = 'teamsonly'
+    >>> view = create_initialized_view(person_set, '+index', form=form)
+    >>> print_batch(view.searchPeopleBatchNavigator())
+    <Person at ... testing-spanish-team (testing Spanish team)>
 
=== modified file 'lib/lp/registry/templates/people-index.pt'
--- lib/lp/registry/templates/people-index.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/people-index.pt	2009-08-18 14:16:16 +0000
@@ -6,39 +6,36 @@
   xml:lang="en"
   lang="en"
   dir="ltr"
-  metal:use-macro="view/macro:page/pillarindex"
+  metal:use-macro="view/macro:page/main_side"
   i18n:domain="launchpad"
 >
   <body>
     <h1 metal:fill-slot="heading">People and teams</h1>
 
-  <metal:leftportlets fill-slot="portlets_one">
-    <div tal:replace="structure context/@@+portlet-about" />
-  </metal:leftportlets>
-
-  <metal:rightportlets fill-slot="portlets_two">
-    <div tal:replace="structure context/@@+portlet-stats" />
-  </metal:rightportlets>
-
-    <h1 metal:fill-slot="application-heading">People and teams</h1>
+    <tal:side metal:fill-slot="side">
+      <tal:menu replace="structure view/@@+global-actions" />
+    </tal:side>
 
     <div metal:fill-slot="main">
-      <tal:block define="bn view/searchPeopleBatchNavigator">
-        <div align="center">
+      There are currently <span tal:replace="view/number_of_people" /> people
+      and <span tal:replace="view/number_of_teams" /> teams registered in
+      Launchpad.
+      <tal:block define="batch view/searchPeopleBatchNavigator">
+        <div>
           <form class="central" name="search" action="." method="GET">
             <table style="margin-bottom: 1em;">
               <tr>
                 <td>
-                  <input type="text" name="name" size="35"
+                  <input type="text" name="name" size="50"
                          tal:attributes="value request/name|nothing"
                          />
                   <input
-                    tal:condition="not: bn"
+                    tal:condition="not: batch"
                     type="submit"
                     value="Search"
                   />
                   <input
-                    tal:condition="bn"
+                    tal:condition="batch"
                     type="submit"
                     value="Search Again"
                   />
@@ -110,57 +107,57 @@
           // --></script>
         </div>
 
-        <tal:block condition="not: bn">
-          <p id="application-summary">
-            Launchpad may create <b>duplicate profiles</b> for you
-            as it automatically reviews translation files, mailing lists and
-            other forums. If you find you have more than one profile here,
-            perhaps for multiple email addresses, you can
-            <a href="+requestmerge">merge the duplicate accounts</a>.
-          </p>
+        <tal:block condition="not: batch">
+          <div id="application-summary">
+            <p>Launchpad creates
+            <a href="https://help.launchpad.net/YourAccount">profiles</a> for
+            people based on Launchpad usage, as well as information collected
+            from public sources such as bug trackers, mailing lists, public
+            key servers, and published application translations.  You can
+            <a href="+me">manage your account</a> in Launchpad, and you can
+            <a href="+newteam">create teams of people</a> for organizing
+            around common interests, discussions or permissions.</p>
+
+            <p>Launchpad may create
+            <a href="https://help.launchpad.net/YourAccount/Merging">duplicate
+            profiles</a> for you as it automatically collects information from
+            public sources.  If you find you have more than one profile on
+            Launchpad, you can <a href="+requestmerge">request a merge</a> of
+            the duplicate profiles.</p>
+
+            <p tal:condition="view/is_admin">As a Launchpad administrator, you
+            can also service
+            <a href="+adminpeoplemerge">person merge requests</a> and
+            <a href="+adminteammerge">team merge requests</a>.
+            </p>
+          </div>
         </tal:block>
-        <tal:block condition="bn">
-          <tal:block
-            define="prev_url bn/prevBatchURL;
-            next_url bn/nextBatchURL;"
-          >
-            <table class="listing">
-              <thead class="results">
-                <tr tal:condition="python:prev_url or next_url">
-                  <td colspan="3">
-                    <div style="float: right; text-align: right;">
-                      <tal:block condition="prev_url">
-                        <a tal:attributes="href prev_url">Previous</a>
-                      </tal:block>
-                      <tal:block repeat="page_link bn/batchPageURLs">
-                        <a tal:attributes="href python:page_link.values()[0]"
-                                tal:content="python:page_link.keys()[0]"></a>
-                      </tal:block>
-                      <tal:block condition="next_url" >
-                        <a tal:attributes="href next_url">Next</a>
-                      </tal:block>
-                    </div>
-                  </td>
-                </tr>
-                <tr>
-                  <th colspan="2">
-                    Name
-                  </th>
-                  <th>
-                    Karma
-                  </th>
-                </tr>
-              </thead>
-              <tbody>
-                <tr tal:repeat="person bn/currentBatch">
-                  <td><a tal:content="person/displayname"
-                         tal:attributes="href person/fmt:url"/>
-                  </td>
-                  <td tal:content="person/name">foobar</td>
-                  <td class="amount" tal:content="person/karma">34</td>
-                </tr>
-              </tbody>
-            </table>
+        <tal:block condition="batch">
+          <tal:block condition="batch/multiple_pages">
+            <tal:navigation
+             replace="structure batch/@@+navigation-links-upper"/>
+          </tal:block>
+          <table class="listing">
+            <thead>
+              <tr>
+                <th>Name</th>
+                <th>Launchpad ID</th>
+                <th>Karma</th>
+              </tr>
+            </thead>
+            <tbody>
+              <tr tal:repeat="person batch/currentBatch">
+                <td><a tal:content="person/displayname"
+                       tal:attributes="href person/fmt:url"/>
+                </td>
+                <td tal:content="person/name">foobar</td>
+                <td class="amount" tal:content="person/karma">34</td>
+              </tr>
+            </tbody>
+          </table>
+          <tal:block condition="batch/multiple_pages">
+            <tal:navigation
+             replace="structure batch/@@+navigation-links-upper"/>
           </tal:block>
         </tal:block>
       </tal:block>
 
=== removed file 'lib/lp/registry/templates/people-portlet-about.pt'
--- lib/lp/registry/templates/people-portlet-about.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/people-portlet-about.pt	1970-01-01 00:00:00 +0000
@@ -1,22 +0,0 @@
-<tal:root
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  omit-tag="">
-<div class="portlet" id="portlet-about">
-
-  <h2>About these profiles</h2>
-
-  <div class="portletBody portletContent">
-
-    <img alt="" src="/@@/info" /> Launchpad profiles are based on actual
-    usage of Launchpad, as well as information available from public sources
-    such as bug trackers, mailing lists, public key servers and published
-    application translations.  You can manage your account in the Launchpad,
-    and you can create teams of people who can be maintainers, translators,
-    members, clubs... almost any kind of grouping you need.
-	
-  </div>
-
-</div>
-</tal:root>
 
=== removed file 'lib/lp/registry/templates/people-portlet-stats.pt'
--- lib/lp/registry/templates/people-portlet-stats.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/people-portlet-stats.pt	1970-01-01 00:00:00 +0000
@@ -1,19 +0,0 @@
-<tal:root
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  omit-tag="">
-<div class="portlet" id="portlet-stats">
-
-  <h2>Statistics</h2>
-
-  <div class="portletBody portletContent">
-
-    There are currently <span tal:replace="view/peopleCount" /> people
-    and <span tal:replace="view/teamsCount" /> teams registered in
-    Launchpad.
-	
-  </div>
-
-</div>
-</tal:root>