Merge lp:~sinzui/launchpad/milestone-table-bug-490648 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to improve the performance of milestone table rows.
Remove the duplicate queries and do not query objects that will not be used.

    lp:~sinzui/launchpad/milestone-table-bug-490648
    Diff size: 112
    Launchpad bug: https://bugs.launchpad.net/bugs/490648
    Test command: ./bin/test -vv -t "reg.*milestone"
    Pre-implementation: no one
    Target release: 3.1.12

= Improve the performance of the milestone table rows =

For each milestone that is shown in the milestone table, the release is looked
up three times and the structuralsubscription is looked up once, but never
used. The MilestoneView provides the release as view/release. The other two
uses must use the view's copy. The structuralsubscription is gotten because
the navigation menu is used to get the edit link. A new menu is needed that
only provides the edit link to ensure the extra db call is made.

== Rules ==

    * Create a navigation menu for the MilestoneView (used by portlets and
      tables) to provide only th links needed.
    * Update the template to use the view's copy of the release.

== QA ==

    Comparing https://edge.launchpad.net/launchpad-registry/3.1 with
    https://launchpad.net/launchpad-registry/3.1, there should be 12
    fewer queries.

== Lint ==

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/milestone.py
  lib/lp/registry/browser/tests/milestone-views.txt
  lib/lp/registry/templates/productseries-milestone-table-row.pt

== Test ==

    * lib/lp/registry/browser/tests/milestone-views.txt
      * Added a test to verify the link in the new menu is legitimate.

== Implementation ==

    * lib/lp/registry/browser/configure.zcml
      * Registered the new menu.
    * lib/lp/registry/browser/milestone.py
      * Added a new navigation menu and updated MilestoneView to use it.
    * lib/lp/registry/templates/productseries-milestone-table-row.pt
      * Updated the template to use the new menu.
      * Replaced the 2 calls to product_release to use the view's release.

[Gavin Panella (allenap)]

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 milesto...

review: Approve

[Curtis Hovey (sinzui)]

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(MilestoneInlineNavigationMen...

[Gavin Panella (allenap)]

On Wed, 02 Dec 2009 15:18:13 -0000
Curtis Hovey <email address hidden> wrote:

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

Cool, and it looks good.

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

=== 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 15:21:20 +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 15:21:20 +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
@@ -118,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')
@@ -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."""
+    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 15:21:20 +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,18 @@
     True
     >>> check_menu_links(MilestoneOverviewNavigationMenu(milestone))
     True
+    >>> 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.

=== 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 15:21:20 +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>