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

Hi Gavin.

Thanks for the review.

On Wed, 2009-12-02 at 12:11 +0000, Gavin Panella wrote:

...

> > === 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

Fixed.

...

> > === 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?

Yes, I think there should given that I botched the ZCML registration the
first time.

{{{
=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py 2009-12-01 18:31:10 +0000
+++ lib/lp/registry/browser/milestone.py 2009-12-02 14:53:08 +0000
@@ -120,7 +120,7 @@

 class MilestoneOverviewNavigationMenu(NavigationMenu, MilestoneLinkMixin):
- """Overview navigation menus for `IMilestone` objects."""
+ """Overview navigation menu for `IMilestone` objects."""
     usedfor = IMilestone
     facet = 'overview'
     links = ('edit', 'delete', 'subscribe')

=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
--- lib/lp/registry/browser/tests/milestone-views.txt 2009-12-01 18:31:10 +0000
+++ lib/lp/registry/browser/tests/milestone-views.txt 2009-12-02 15:14:33 +0000
@@ -34,6 +34,15 @@
     >>> check_menu_links(MilestoneInlineNavigationMenu(milestone))
     True

+The MilestoneView used can be adapted to a MilestoneInlineNavigationMenu
+for use with inline presentation of milestones.
+
+ >>> from canonical.lazr.canonicalurl import nearest_adapter
+ >>> from canonical.launchpad.webapp.interfaces import INavigationMenu
+
+ >>> view = create_view(milestone, name='+productseries-table-row')
+ >>> nearest_adapter(view, INavigationMenu, name='overview')
+ <lp.registry.browser.milestone.MilestoneInlineNavigationMenu ...>

 The MilestoneView provides access to the milestone and to its release if
 it has one.
}}}

--
__Curtis C. Hovey_________
http://launchpad.net/