Merge lp:~sinzui/launchpad/base-layout-bug-407052 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to address two base-layout issues.

    lp:~sinzui/launchpad/base-layout-bug-407052
    Diff size: 226
    Launchpad bug: https://bugs.launchpad.net/bugs/407052
                   https://bugs.launchpad.net/bugs/407055
    Test command: ./bin/test -vvt "lp/app/.*/(base-layout|menu)"
    Pre-implementation: no one.
    Target release: 2.2.8

= Two base-layout issues =

Bug #407052 base-layout heading slot breaks the title edit widget

    The title edit widget creates it's own <h1> with an id and class. When
    used with the heading slot a <h1> is generated inside the <h1>.

    The solution is that the <h1> only be created when base-layout is
    automatically creating a title for the context object.

Bug #407055 NavigationMenu is still needed but not supported by base-layout

    While working on the project edit pages in an experiment to move
    them to the 3.0 layout. I realised I had dismantled the edit
    NavigationMenu, then added a section to the page that had the same
    links. We should be reusing the navigation menus

    Since many pages need to have a releated pages section with a list
    of links, there should be a view that can be called to make the
    standard markup. This will enforce consistency, reduce the amount of
    typing, and allow us to *not* remove the NavigationMenu from views.

== Rules ==

Bug #407052 base-layout heading slot breaks the title edit widget

    Make the template pass the <h1> to the slot. If the slot is not
    called by the template, base-layout makes a <h1> using the
    context.title

Bug #407055 NavigationMenu is still needed but not supported by base-layout

    There is already a view for the NavigationMenu. We need a new
    template that renders the portlet, heading, and list. This structure
    is the same as the one used on series and milestones now.

== QA ==

Bug #407052 base-layout heading slot breaks the title edit widget

    No page has encountered this yet on edge staging. This fix can only
    verified by a page in development (namely the product-index.pt) that
    I am working on.

Bug #407055 NavigationMenu is still needed but not supported by base-layout

    No page uses this yet, but I have branch that does.

== Lint ==

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

Linting changed files:
  lib/lp/app/browser/configure.zcml
  lib/lp/app/browser/tests/base-layout.txt
  lib/lp/app/browser/tests/menu.txt
  lib/lp/app/browser/tests/testfiles/main-only.pt
  lib/lp/app/browser/tests/testfiles/main-side.pt
  lib/lp/app/browser/tests/testfiles/searchless.pt
  lib/lp/app/templates/base-layout.pt
  lib/lp/app/templates/navigationmenu-related-pages.pt

== Test ==

    * lib/lp/app/browser/tests/menu.txt
      * Created a test to verify the behaviour of the related pages template.
    * lib/lp/app/browser/tests/base-layout.txt
      * I Updated the narative to explain that the <h1> must be passed by
        the callsite.
    * lib/lp/app/browser/tests/testfiles/main-only.pt
    * lib/lp/app/browser/tests/testfiles/main-side.pt
    * lib/lp/app/browser/tests/testfiles/search...

[Abel Deuring (adeuring)]

Hi Curtis,

A nice branch; thanks for the detailed cover letter!

I have only a few cosmetic remarks, see below.

> === added file 'lib/lp/app/browser/tests/menu.txt'
> --- lib/lp/app/browser/tests/menu.txt 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/browser/tests/menu.txt 2009-07-31 02:42:44 +0000
> @@ -0,0 +1,114 @@
> +====================
> +NavigationMenu views
> +====================
> +
> +Navigation menus are used to create links to related pages. The menus
> +are usually bound to a view.
> +
> + >>> from zope.component import provideAdapter
> + >>> from zope.interface import implements, Interface
> + >>> from canonical.launchpad.webapp.interfaces import INavigationMenu
> + >>> from canonical.launchpad.webapp.menu import (
> + ... enabled_with_permission, Link, NavigationMenu)
> + >>> from canonical.launchpad.webapp.publisher import LaunchpadView
> + >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> +
> + >>> class IEditMenuMarker(Interface):
> + ... """A marker for a menu and a view."""
> +
> + >>> class EditMenu(NavigationMenu):
> + ... """A simple menu."""
> + ... usedfor = IEditMenuMarker
> + ... facet = 'overview'
> + ... title = 'Related pages'
> + ... links = ('edit_thing', 'edit_people', 'admin')
> + ...
> + ... def edit_thing(self):
> + ... return Link('+edit', 'Edit thing', icon='edit')
> + ...
> + ... def edit_people(self):
> + ... return Link('+edit-people', 'Edit people related to thing')
> + ...
> + ... @enabled_with_permission('launchpad.Admin')
> + ... def admin(self):
> + ... return Link('+admin', 'Administer this user')
> +
> + >>> class EditView(LaunchpadView):
> + ... """A simple view."""
> + ... implements(IEditMenuMarker)
> + ... # A hack that reveals secret of how facet work.
> + ... __launchpad_facetname__ = 'overview'
> +
> + # Menu's are normally registered using the menu ZCML directive.

s/Menu's/Menus/

> + >>> provideAdapter(
> + ... EditMenu, [IEditMenuMarker], INavigationMenu, name="overview")
> +
> +The related pages portlet is rendered using a TALES call passing the view
> +to the named adapter: <tal:menu replace="structure view/@@+related-pages" />
> +
> + >>> user = factory.makePerson(name='beaker')
> + >>> view = EditView(user, LaunchpadTestRequest())
> + >>> menu_view = create_initialized_view(
> + ... view, '+related-pages', principal=user)
> + >>> print menu_view.template.filename
> + /.../navigationmenu-related-pages.pt
> +
> +The view provides access the menu's title and links. The both enabled
> +and disabled links are included.

s/The both/Both the/

> +
> + >>> print menu_view.title
> + Related pages
> + >>> for link in menu_view.links:
> + ... print link.enabled, link.url
> + True http://launchpad.dev/~beaker/+edit
> + True http://launchpad.dev/~beaker/+edit-people
> + False http://launchpad.dev/~beaker/+admin
> +
> +The view renders the a heading using the menu title and a list of the
> +enabled and linked links. The template uses t...

review: Approve

[Curtis Hovey (sinzui)]

Thanks for the review Abel.

On Fri, 2009-07-31 at 11:33 +0000, Abel Deuring wrote:
> Review: Approve
> Hi Curtis,
>
> A nice branch; thanks for the detailed cover letter!
>
> I have only a few cosmetic remarks, see below.
>
>
> > === added file 'lib/lp/app/browser/tests/menu.txt'
> > --- lib/lp/app/browser/tests/menu.txt 1970-01-01 00:00:00 +0000
> > +++ lib/lp/app/browser/tests/menu.txt 2009-07-31 02:42:44 +0000

...

> > + >>> class EditView(LaunchpadView):
> > + ... """A simple view."""
> > + ... implements(IEditMenuMarker)
> > + ... # A hack that reveals secret of how facet work.

Fixed facet, should be factets

> > + ... __launchpad_facetname__ = 'overview'
> > +
> > + # Menu's are normally registered using the menu ZCML directive.
>
> s/Menu's/Menus/

fixed

> > + >>> provideAdapter(
> > + ... EditMenu, [IEditMenuMarker], INavigationMenu, name="overview")
> > +
> > +The related pages portlet is rendered using a TALES call passing the view
> > +to the named adapter: <tal:menu replace="structure view/@@+related-pages" />
> > +
> > + >>> user = factory.makePerson(name='beaker')
> > + >>> view = EditView(user, LaunchpadTestRequest())
> > + >>> menu_view = create_initialized_view(
> > + ... view, '+related-pages', principal=user)
> > + >>> print menu_view.template.filename
> > + /.../navigationmenu-related-pages.pt
> > +
> > +The view provides access the menu's title and links. The both enabled
> > +and disabled links are included.
>
> s/The both/Both the/

Fixed

> > +
> > + >>> print menu_view.title
> > + Related pages
> > + >>> for link in menu_view.links:
> > + ... print link.enabled, link.url
> > + True http://launchpad.dev/~beaker/+edit
> > + True http://launchpad.dev/~beaker/+edit-people
> > + False http://launchpad.dev/~beaker/+admin
> > +
> > +The view renders the a heading using the menu title and a list of the
> > +enabled and linked links. The template uses the inline-link rules, if the
> > +link has a icon, the classes are set, otherwise a style attribute is used.
>
> s/renders the a/renders the/
> s/a icon/an icon/
>
> "linked links" sounds a bit odd to me ;) What about "list of enabled links"?

The links `enabled` and `linked` properties should not be confused. The
former is about permission, the later is about relevance.

This is the diff of my changes:

=== modified file 'lib/lp/app/browser/tests/menu.txt'
--- lib/lp/app/browser/tests/menu.txt 2009-07-31 02:42:44 +0000
+++ lib/lp/app/browser/tests/menu.txt 2009-07-31 13:23:20 +0000
@@ -36,10 +36,10 @@
     >>> class EditView(LaunchpadView):
     ... """A simple view."""
     ... implements(IEditMenuMarker)
- ... # A hack that reveals secret of how facet work.
+ ... # A hack that reveals secret of how facets work.
     ... __launchpad_facetname__ = 'overview'

- # Menu's are normally registered using the menu ZCML directive.
+ # Menus are normally registered using the menu ZCML directive.
     >>> provideAdapter(
     ... EditMenu, [IEditMenuMarker], INavigationMenu, name="overview")

@@ -53,7 +53,7 @@
     >>> print menu_view.template...

=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2009-07-17 02:25:09 +0000
+++ lib/lp/app/browser/configure.zcml	2009-07-31 02:42:44 +0000
@@ -14,4 +14,11 @@
         template="../templates/base-layout-macros.pt"
         permission="zope.Public"
         />
+    <browser:page
+        for="*"
+        name="+related-pages"
+        class="canonical.launchpad.browser.launchpad.NavigationMenuTabs"
+        template="../templates/navigationmenu-related-pages.pt"
+        permission="zope.Public"
+        />
 </configure>

=== modified file 'lib/lp/app/browser/tests/base-layout.txt'
--- lib/lp/app/browser/tests/base-layout.txt	2009-07-27 20:57:01 +0000
+++ lib/lp/app/browser/tests/base-layout.txt	2009-07-30 20:38:50 +0000
@@ -237,7 +237,8 @@
     >>> print content.h1
     <h1>Heading</h1>
 
-The page heading slot's default behaviour is to use the context.title.
+The page heading slot's default behaviour is to use the context.title and
+place it in a <h1>.
 
     >>> class DefaultHeadingView(LaunchpadView):
     ...     """A simple view to test base-layout."""

=== added file 'lib/lp/app/browser/tests/menu.txt'
--- lib/lp/app/browser/tests/menu.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/menu.txt	2009-07-31 02:42:44 +0000
@@ -0,0 +1,114 @@
+====================
+NavigationMenu views
+====================
+
+Navigation menus are used to create links to related pages. The menus
+are usually bound to a view.
+
+    >>> from zope.component import provideAdapter
+    >>> from zope.interface import implements, Interface
+    >>> from canonical.launchpad.webapp.interfaces import INavigationMenu
+    >>> from canonical.launchpad.webapp.menu import (
+    ...     enabled_with_permission, Link, NavigationMenu)
+    >>> from canonical.launchpad.webapp.publisher import LaunchpadView
+    >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+
+    >>> class IEditMenuMarker(Interface):
+    ...     """A marker for a menu and a view."""
+
+    >>> class EditMenu(NavigationMenu):
+    ...     """A simple menu."""
+    ...     usedfor = IEditMenuMarker
+    ...     facet = 'overview'
+    ...     title = 'Related pages'
+    ...     links = ('edit_thing', 'edit_people', 'admin')
+    ...
+    ...     def edit_thing(self):
+    ...         return Link('+edit', 'Edit thing', icon='edit')
+    ...
+    ...     def edit_people(self):
+    ...         return Link('+edit-people', 'Edit people related to thing')
+    ...
+    ...     @enabled_with_permission('launchpad.Admin')
+    ...     def admin(self):
+    ...         return Link('+admin', 'Administer this user')
+
+    >>> class EditView(LaunchpadView):
+    ...     """A simple view."""
+    ...     implements(IEditMenuMarker)
+    ...     # A hack that reveals secret of how facet work.
+    ...     __launchpad_facetname__ = 'overview'
+
+    # Menu's are normally registered using the menu ZCML directive.
+    >>> provideAdapter(
+    ...     EditMenu, [IEditMenuMarker], INavigationMenu, name="overview")
+
+The related pages portlet is rendered using a TALES call passing the view
+to the named adapter: <tal:menu replace="structure view/@@+related-pages" />
+
+    >>> user = factory.makePerson(name='beaker')
+    >>> view = EditView(user, LaunchpadTestRequest())
+    >>> menu_view = create_initialized_view(
+    ...     view, '+related-pages', principal=user)
+    >>> print menu_view.template.filename
+    /.../navigationmenu-related-pages.pt
+
+The view provides access the menu's title and links. The both enabled
+and disabled links are included.
+
+    >>> print menu_view.title
+    Related pages
+    >>> for link in menu_view.links:
+    ...     print link.enabled, link.url
+    True   http://launchpad.dev/~beaker/+edit
+    True   http://launchpad.dev/~beaker/+edit-people
+    False  http://launchpad.dev/~beaker/+admin
+
+The view renders the a heading using the menu title and a list of the
+enabled and linked links. The template uses the inline-link rules, if the
+link has a icon, the classes are set, otherwise a style attribute is used.
+
+    >>> print menu_view.render()
+    <div id="related-pages" class="portlet">
+      <h2>Related pages</h2>
+      <ul>
+        <li>
+          <a href="..."
+            class="menu-link-edit_thing sprite modify edit">Edit thing</a>
+        </li>
+        <li>
+          <a style="..." href="..."
+            class="menu-link-edit_people">Edit people related to thing</a>
+        </li>
+      </ul>
+    </div>
+
+The link is not displayed if the link matches the requested URL (not linked).
+The page will not render a link to itself.
+
+    >>> request = LaunchpadTestRequest(
+    ...     SERVER_URL='http://launchpad.dev/~beaker/+edit')
+    >>> print request.getURL()
+    http://launchpad.dev/~beaker/+edit
+
+    >>> view = EditView(user, request)
+    >>> menu_view = create_initialized_view(
+    ...     view, '+related-pages', principal=user)
+    >>> for link in menu_view.links:
+    ...     print link.enabled, link.linked, link.url
+    True  False  http://launchpad.dev/~beaker/+edit
+    True  True   http://launchpad.dev/~beaker/+edit-people
+    False True   http://launchpad.dev/~beaker/+admin
+
+    >>> print menu_view.render()
+    <div id="related-pages" class="portlet">
+      <h2>Related pages</h2>
+      <ul>
+        <li>
+          <a style="..." href="..."
+            class="menu-link-edit_people">Edit people related to thing</a>
+        </li>
+      </ul>
+    </div>
+
+

=== modified file 'lib/lp/app/browser/tests/testfiles/main-only.pt'
--- lib/lp/app/browser/tests/testfiles/main-only.pt	2009-07-24 04:26:14 +0000
+++ lib/lp/app/browser/tests/testfiles/main-only.pt	2009-07-30 20:38:50 +0000
@@ -12,7 +12,7 @@
   </head>
 
   <body>
-    <tal:heading metal:fill-slot="heading">Heading</tal:heading>
+    <tal:heading metal:fill-slot="heading"><h1>Heading</h1></tal:heading>
     <tal:main metal:fill-slot="main">
       <div class="top-portlet">
         <p class="registered">

=== modified file 'lib/lp/app/browser/tests/testfiles/main-side.pt'
--- lib/lp/app/browser/tests/testfiles/main-side.pt	2009-07-24 04:26:14 +0000
+++ lib/lp/app/browser/tests/testfiles/main-side.pt	2009-07-30 20:38:50 +0000
@@ -12,7 +12,7 @@
   </head>
 
   <body>
-    <tal:heading metal:fill-slot="heading">Heading</tal:heading>
+    <tal:heading metal:fill-slot="heading"><h1>Heading</h1></tal:heading>
     <tal:main metal:fill-slot="main">
       <div class="top-portlet">
         <p class="registered">

=== modified file 'lib/lp/app/browser/tests/testfiles/searchless.pt'
--- lib/lp/app/browser/tests/testfiles/searchless.pt	2009-07-24 04:26:14 +0000
+++ lib/lp/app/browser/tests/testfiles/searchless.pt	2009-07-30 20:38:50 +0000
@@ -12,7 +12,7 @@
   </head>
 
   <body>
-    <tal:heading metal:fill-slot="heading">Heading</tal:heading>
+    <tal:heading metal:fill-slot="heading"><h1>Heading</h1></tal:heading>
     <tal:main metal:fill-slot="main">
       <div class="top-portlet">
         <p class="registered">

=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt	2009-07-27 20:57:01 +0000
+++ lib/lp/app/templates/base-layout.pt	2009-07-30 20:38:50 +0000
@@ -137,10 +137,10 @@
               xml:lang view/lang|default_language|default;
               dir view/dir|string:ltr">
             <tal:location condition="view/macro:pagehas/applicationtabs">
-              <h1><tal:heading
+              <h1
                 tal:condition="context/title|nothing"
                 tal:content="context/title"
-                metal:define-slot="heading" /></h1>
+                metal:define-slot="heading" />
               <tal:breadcrumbs>
                 <!-- future breadcrumb rule -->
               </tal:breadcrumbs>

=== added file 'lib/lp/app/templates/navigationmenu-related-pages.pt'
--- lib/lp/app/templates/navigationmenu-related-pages.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/app/templates/navigationmenu-related-pages.pt	2009-07-31 02:42:44 +0000
@@ -0,0 +1,16 @@
+<div
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  id="related-pages" class="portlet">
+  <h2 tal:content="view/title">Menu title</h2>
+
+  <ul>
+    <tal:link tal:repeat="link view/links">
+      <tal:enabled tal:condition="link/enabled">
+        <li tal:condition="link/linked">
+          <a tal:replace="structure link/fmt:icon-link" />
+        </li>
+      </tal:enabled>
+    </tal:link>
+  </ul>
+</div>