Code review comment for lp:~sinzui/launchpad/milestone-table-bug-490648

Hi Curtis,

This is an elegant fix with a nicely big payoff.

Just a couple of comments.

Gavin.

> === modified file 'lib/lp/registry/browser/configure.zcml'
> --- lib/lp/registry/browser/configure.zcml 2009-11-26 06:41:27 +0000
> +++ lib/lp/registry/browser/configure.zcml 2009-12-02 12:04:29 +0000
> @@ -1310,6 +1310,7 @@
> <browser:menus
> classes="
> MilestoneContextMenu
> + MilestoneInlineNavigationMenu
> MilestoneOverviewMenu
> MilestoneOverviewNavigationMenu"
> module="lp.registry.browser.milestone"/>
>
> === modified file 'lib/lp/registry/browser/milestone.py'
> --- lib/lp/registry/browser/milestone.py 2009-11-24 21:27:17 +0000
> +++ lib/lp/registry/browser/milestone.py 2009-12-02 12:04:29 +0000
> @@ -11,6 +11,7 @@
> 'MilestoneContextMenu',
> 'MilestoneDeleteView',
> 'MilestoneEditView',
> + 'MilestoneInlineNavigationMenu',
> 'MilestoneNavigation',
> 'MilestoneOverviewNavigationMenu',
> 'MilestoneSetNavigation',
> @@ -22,6 +23,7 @@
>
> from zope.component import getUtility
> from zope.formlib import form
> +from zope.interface import implements, Interface
> from zope.schema import Choice
>
> from canonical.cachedproperty import cachedproperty
> @@ -131,11 +133,22 @@
> links = ('create_release', )
>
>
> +class IMilestoneInline(Interface):
> + """A marker interface for views that show a milestone inline."""
> +
> +
> +class MilestoneInlineNavigationMenu(NavigationMenu, MilestoneLinkMixin):
> + """An inline navigation menus for milestone views."""

s/menus/menu

> + usedfor = IMilestoneInline
> + facet = 'overview'
> + links = ('edit', )
> +
> +
> class MilestoneView(LaunchpadView, ProductDownloadFileMixin):
> """A View for listing milestones and releases."""
> # XXX sinzui 2009-05-29 bug=381672: Extract the BugTaskListingItem rules
> # to a mixin so that MilestoneView and others can use it.
> -
> + implements(IMilestoneInline)
> show_series_context = False
>
> def __init__(self, context, request):
>
> === modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
> --- lib/lp/registry/browser/tests/milestone-views.txt 2009-09-30 22:49:58 +0000
> +++ lib/lp/registry/browser/tests/milestone-views.txt 2009-12-02 12:04:29 +0000
> @@ -21,8 +21,8 @@
> Milestone defines several menus.
>
> >>> from lp.registry.browser.milestone import (
> - ... MilestoneContextMenu, MilestoneOverviewMenu,
> - ... MilestoneOverviewNavigationMenu)
> + ... MilestoneContextMenu, MilestoneInlineNavigationMenu,
> + ... MilestoneOverviewMenu, MilestoneOverviewNavigationMenu)
> >>> from lp.testing.menu import check_menu_links
>
> >>> check_menu_links(MilestoneContextMenu(milestone))
> @@ -31,6 +31,9 @@
> True
> >>> check_menu_links(MilestoneOverviewNavigationMenu(milestone))
> True
> + >>> check_menu_links(MilestoneInlineNavigationMenu(milestone))
> + True
> +

Should there also be a test to show that the
MilestoneInlineNavigationMenu is chosen for MilestoneView, or is that
going too far?

>
> The MilestoneView provides access to the milestone and to its release if
> it has one.
>
> === modified file 'lib/lp/registry/templates/productseries-milestone-table-row.pt'
> --- lib/lp/registry/templates/productseries-milestone-table-row.pt 2009-11-03 19:04:53 +0000
> +++ lib/lp/registry/templates/productseries-milestone-table-row.pt 2009-12-02 12:04:29 +0000
> @@ -2,8 +2,9 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> xmlns:metal="http://xml.zope.org/namespaces/metal"
> xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> - define="milestone_menu view/milestone/menu:navigation;
> - milestone view/milestone">
> + define="milestone_menu view/menu:navigation;
> + milestone view/milestone;
> + release view/release">
> <tr>
> <td>
> <img src="/@@/milestone" alt="" />
> @@ -43,13 +44,12 @@
> </td>
> <td tal:condition="not: view/is_distroseries_milestone">
> <span
> - tal:define="release milestone/product_release"
> tal:condition="release"
> tal:attributes="title release/datereleased/fmt:datetime"
> tal:content="release/datereleased/fmt:approximatedate">
> 2005-12-13
> </span>
> - <tal:no-release condition="not: milestone/product_release">
> + <tal:no-release condition="not: release">
> <em tal:condition="not: milestone/active">
> <img src="/@@/info"/> This is an inactive milestone
> </em>
>