Merge lp:~edwin-grubbs/launchpad/bug-490659-series-timeout into lp:launchpad/db-devel

[Edwin Grubbs]

Summary
-------

Bug 490659 reports a timeout caused by a project with over 700 releases.
The ProductSeries' milestones and releases attributes both used a python
function to expand the version numbers in the milestone name so that
milestone version numbers such as 1.1, 1.10, and 1.100 would sort
correctly. To avoid loading all 700 releases from the database just to
sort them in python to find the most recent ones, I added the
milestone_sort_key(timestamp dateexpected, text name) postgresql
function, so that the sorting can take place in the db. I also added an
index to the Milestone table using this function.

I have never submitted a db function for review before, so I'm sure that
I'm doing something wrong. For some reason, the configuration I added to
security.cfg doesn't seem to have any effect, so I have to run the
following sql on launchpad_ftest_template and laucnhpad_dev.

GRANT EXECUTE ON FUNCTION milestone_sort_key(timestamp, text) TO PUBLIC;

Implementation details
----------------------

Added milestone_sort_key(timestamp, text) db function.
    database/schema/patch-2207-99-0.sql
    database/schema/security.cfg

Added __len__ to DecoratedResultSet, so that code expecting the model to
return a list won't blow up on the result set.
    lib/canonical/launchpad/components/decoratedresultset.py
    lib/canonical/launchpad/zcml/decoratedresultset.zcml

Changed the productseries detailed-display macro into its own page since
it makes it cleaner to limit the number of milestones and releases in
the view attributes.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/productseries.py
    lib/lp/registry/templates/productseries-macros.pt
    lib/lp/registry/templates/productseries-status.pt
    lib/lp/registry/stories/productseries/xx-productseries-series.txt

Converted HasMilestones' milestones and releases attributes to return a
DecoratedResultSet instead of alist.
    lib/lp/registry/model/milestone.py
    lib/lp/registry/browser/tests/productseries-views.txt

Tests
-----

./bin/test -vv -t 'xx-productseries-series.txt|productseries-views.txt'

Demo and Q/A
------------

Create a bunch of sample milestones and releases on launchpad.dev:
    INSERT INTO Milestone (name, product, productseries)
    SELECT 'a' || i, (select id from product where name = 'bzr'),
        (select id from productseries where name='trunk' and product = (
                select id from product where name = 'bzr'))
    FROM generate_series(1, 100) as i;

    INSERT INTO Milestone (name, product, productseries, active)
    SELECT
        'rel-' || i,
        (select id from product where name = 'bzr'),
        (select id from productseries where name='trunk' and product = (
                select id from product where name = 'bzr')),
        FALSE
    FROM generate_series(1, 100) as i;

    INSERT INTO ProductRelease (owner, milestone, datereleased)
    SELECT 1, id, now()
    FROM Milestone
    WHERE name ~ 'rel-.*';

* Open http://launchpad.dev/bzr/+series
  * Only 12 milestones and 12 releases should be listed in the gray box
    for trunk.

[Robert Collins (lifeless)]

I don't like the __len__ introduction as it blurs the line between lists and resultsets more : and we suffer because the line is blurry already. I realise you probably have a stack of stuff to put on top of this that is doing lens : however I'm worried about the knock on effects of the change : we should do it in IResultSet if we're doing it at all, not piecemeal on DecoratedResultSet. You could use a lazr.delegates to add __len__ just to your specific case (or, and perhaps better?) work up the stack replacing listified stuff with resultset aware code.

Other than that I think this is conceptually fine and an appropriate solution.

I'm a tad rusty on my plpython gotchas, so I'll let Stuart review that for you.

review: Needs Fixing

[Stuart Bishop (stub)]

The function and a database comment should be added directly to trusted.sql. You will still need the database patch to add the index. I'll allocate a number shortly.

The function should return """ '%s %s' % (str(date_expected), name) """, rather than the implicit cast of repr(date_expected, name).

You are importing plpy but no longer using it.

I agree with Robert on __len__ - it is deliberately left out to avoid it accidentally being called, as it can be very expensive. In particular, Python invoked __len__ if it exists when casting something to a list, doubling the amount of database time materializing a result set (we reported this as a Python bug - not sure if it is still open).

review: Approve (db)

[Stuart Bishop (stub)]

patch-2208-01-0.sql

[Edwin Grubbs (edwin-grubbs)]

I got rid of __len__ on DecoratedResultSet. Instead, I added IHasMilestones.has_milestones, since this is what len() was being used to test for, and a database count is much more intensive than finding a single unsorted matching row.

I made the changes to to the db function, and I discovered why my configuration in security.cfg wasn't granting execute to public. It expects me to specify "timestamp without time zone" instead of just "timestamp". A GRANT statement with just "timestamp" works, so it must just be the introspection that security.py does to figure out whether it is dealing with a function or a table.

I have included a new full diff below, since it is less confusing than the incremental diff and not much bigger.

-Edwin

=== added file 'database/schema/patch-2208-01-0.sql'
--- database/schema/patch-2208-01-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-01-0.sql 2010-08-19 17:15:22 +0000
@@ -0,0 +1,9 @@
+-- Copyright 2010 Canonical Ltd. This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+CREATE INDEX milestone_dateexpected_name_sort
+ON Milestone
+USING btree (milestone_sort_key(dateexpected, name));
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 01, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2010-08-10 05:49:16 +0000
+++ database/schema/security.cfg 2010-08-19 23:52:51 +0000
@@ -17,6 +17,7 @@
 public.person_sort_key(text, text) = EXECUTE
 public.calculate_bug_heat(integer) = EXECUTE
 public.debversion_sort_key(text) = EXECUTE
+public.milestone_sort_key(timestamp without time zone, text) = EXECUTE
 public.null_count(anyarray) = EXECUTE
 public.valid_name(text) = EXECUTE
 public.valid_bug_name(text) = EXECUTE

=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql 2010-08-06 09:32:02 +0000
+++ database/schema/trusted.sql 2010-08-19 19:50:52 +0000
@@ -1705,3 +1705,30 @@

     return int(total_heat)
 $$;
+
+-- This function is not STRICT, since it needs to handle
+-- dateexpected when it is NULL.
+CREATE OR REPLACE FUNCTION milestone_sort_key(
+ dateexpected timestamp, name text)
+ RETURNS text
+AS $_$
+ # If this method is altered, then any functional indexes using it
+ # need to be rebuilt.
+ import re
+ import datetime
+
+ date_expected, name = args
+
+ def substitude_filled_numbers(match):
+ return match.group(0).zfill(5)
+
+ name = re.sub(u'\d+', substitude_filled_numbers, name)
+ if date_expected is None:
+ # NULL dates are considered to be in the future.
+ date_expected = datetime.datetime(datetime.MAXYEAR, 1, 1)
+ return '%s %s' % (date_expected, name)
+$_$
+LANGUAGE plpythonu IMMUTABLE;
+
+COMMENT ON FUNCTION milestone_sort_key(timestamp, text) IS
+'Sort by the Milestone dateexpected and name. If the dateexpected is NULL, then it is converted to a date far in the future, so it will be sorted as a milestone in the future.';

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__....

[Brad Crittenden (bac)]

Hi Edwin,

Interesting branch!

typo: substitude -> substitute

Should has_milestones be exported?

Any reason you cannot use ISlaveStore for the queries?

review: Approve (code)

[Edwin Grubbs (edwin-grubbs)]

> Hi Edwin,
>
> Interesting branch!
>
> typo: substitude -> substitute

Fixed both the db function and the code that I stole that from.

> Should has_milestones be exported?

I think it would be better not to slow down every query for the pillars and series by adding another attribute that is hardly ever used. The other milestones attributes are references to collections, so you don't actually invoke the extra query unless you follow that link.

> Any reason you cannot use ISlaveStore for the queries?

IStore() chooses the slave or master based on whether the user has modified anything lately. If I forced it to use ISlaveStore, it would not show the user any changes they made until they propagated to the slave servers.

[Robert Collins (lifeless)]

69- def substitude_filled_numbers(match):
70+
71+ def substitute_filled_numbers(match):

That new VWS breaks up a small function - closures like that are more readable when they look like one conceptual thing rather than two IMO; please consider removing the new VWS.

I think the index is new? Needs a new stamp from stuart if so.

Lastly, it seems to me that LBYL isn't needed here: surely doing *neither* a .count() nor a .any() is appropriate: rather just iterate the latest_milestones, and if the iterator outputs no rows don't show the table? Perhaps we don't have a construct for doing that; if thats the case I'm happy with this approach, but suggest that you file a bug saying we should have such a construct - it will be the least work of all and thus fastest.

review: Approve

[Stuart Bishop (stub)]

db still approved.

review: Approve (db)

[Edwin Grubbs (edwin-grubbs)]

> 69- def substitude_filled_numbers(match):
> 70+
> 71+ def substitute_filled_numbers(match):

Brad also pointed that out, and it has been fixed.

> That new VWS breaks up a small function - closures like that are more readable
> when they look like one conceptual thing rather than two IMO; please consider
> removing the new VWS.

I actually disagree that it makes it less readable. Secondly, pocketlint complains when you don't have a blank line above a function definition.

> I think the index is new? Needs a new stamp from stuart if so.

The index isn't new. Since I included a full diff after the first set of changes instead of an incremental diff, it may have appeared new, but it was in the patch file below the db function before the db function was moved to trusted.sql.

> Lastly, it seems to me that LBYL isn't needed here: surely doing *neither* a
> .count() nor a .any() is appropriate: rather just iterate the
> latest_milestones, and if the iterator outputs no rows don't show the table?

The problem with not checking .any() before iterating is that a storm ResultSet object will query the database every time you iterate over it; therefore, you would have to create a new cached property on the view that would hold a list in order to eliminate the query. We can't have the model just cache the property as a list for us, since the whole point of this refactoring was to allow the view code to slice the model's attribute without the model first querying a really large number of rows.

> Perhaps we don't have a construct for doing that; if thats the case I'm happy
> with this approach, but suggest that you file a bug saying we should have such
> a construct - it will be the least work of all and thus fastest.

I definitely think there is a better universal solution to this, but it will need more discussion on the mailing list, since either template/view coding practices or storm's ResultSet will have to change significantly. Also, to put it into perspective, we are talking about eliminating one or two single-row queries, compared to the 700-rows of results that this branch eliminates, so improving this might not be as urgent as other optimizations.

=== added file 'database/schema/patch-2208-01-0.sql'
--- database/schema/patch-2208-01-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-01-0.sql	2010-08-25 04:29:43 +0000
@@ -0,0 +1,9 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+CREATE INDEX milestone_dateexpected_name_sort
+ON Milestone
+USING btree (milestone_sort_key(dateexpected, name));
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 01, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-08-15 11:29:23 +0000
+++ database/schema/security.cfg	2010-08-25 04:29:43 +0000
@@ -17,6 +17,7 @@
 public.person_sort_key(text, text)         = EXECUTE
 public.calculate_bug_heat(integer)         = EXECUTE
 public.debversion_sort_key(text)           = EXECUTE
+public.milestone_sort_key(timestamp without time zone, text) = EXECUTE
 public.null_count(anyarray)                = EXECUTE
 public.valid_name(text)                    = EXECUTE
 public.valid_bug_name(text)                = EXECUTE

=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql	2010-08-06 09:32:02 +0000
+++ database/schema/trusted.sql	2010-08-25 04:29:43 +0000
@@ -1705,3 +1705,30 @@
 
     return int(total_heat)
 $$;
+
+-- This function is not STRICT, since it needs to handle
+-- dateexpected when it is NULL.
+CREATE OR REPLACE FUNCTION milestone_sort_key(
+    dateexpected timestamp, name text)
+    RETURNS text
+AS $_$
+    # If this method is altered, then any functional indexes using it
+    # need to be rebuilt.
+    import re
+    import datetime
+
+    date_expected, name = args
+
+    def substitute_filled_numbers(match):
+        return match.group(0).zfill(5)
+
+    name = re.sub(u'\d+', substitute_filled_numbers, name)
+    if date_expected is None:
+        # NULL dates are considered to be in the future.
+        date_expected = datetime.datetime(datetime.MAXYEAR, 1, 1)
+    return '%s %s' % (date_expected, name)
+$_$
+LANGUAGE plpythonu IMMUTABLE;
+
+COMMENT ON FUNCTION milestone_sort_key(timestamp, text) IS
+'Sort by the Milestone dateexpected and name. If the dateexpected is NULL, then it is converted to a date far in the future, so it will be sorted as a milestone in the future.';

=== modified file 'lib/canonical/launchpad/webapp/sorting.py'
--- lib/canonical/launchpad/webapp/sorting.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/webapp/sorting.py	2010-08-25 04:29:43 +0000
@@ -25,9 +25,10 @@
 
     """
     assert(isinstance(unicode_text, unicode))
-    def substitude_filled_numbers(match):
+
+    def substitute_filled_numbers(match):
         return match.group(0).zfill(fill_digits)
-    return re.sub(u'\d+', substitude_filled_numbers, unicode_text)
+    return re.sub(u'\d+', substitute_filled_numbers, unicode_text)
 
 
 # Create translation table for numeric ordinals to their

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/__init__.py	2010-08-25 04:29:43 +0000
@@ -80,7 +80,7 @@
     @property
     def milestone_table_class(self):
         """The milestone table will be unseen if there are no milestones."""
-        if len(self.context.all_milestones) > 0:
+        if self.context.has_milestones:
             return 'listing'
         else:
             # The page can remove the 'unseen' class to make the table

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-08-13 21:30:24 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-08-25 04:29:43 +0000
@@ -1655,6 +1655,13 @@
     </browser:pages>
     <browser:page
         for="lp.registry.interfaces.productseries.IProductSeries"
+        class="lp.registry.browser.productseries.ProductSeriesDetailedDisplayView"
+        template="../templates/productseries-detailed-display.pt"
+        facet="overview"
+        permission="zope.Public"
+        name="+detailed-display"/>
+    <browser:page
+        for="lp.registry.interfaces.productseries.IProductSeries"
         class="lp.registry.browser.productseries.ProductSeriesRdfView"
         facet="overview"
         permission="zope.Public"

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-08-23 04:48:17 +0000
+++ lib/lp/registry/browser/productseries.py	2010-08-25 04:29:43 +0000
@@ -10,6 +10,7 @@
     'ProductSeriesBreadcrumb',
     'ProductSeriesBugsMenu',
     'ProductSeriesDeleteView',
+    'ProductSeriesDetailedDisplayView',
     'ProductSeriesEditView',
     'ProductSeriesFacets',
     'ProductSeriesFileBugRedirect',
@@ -482,6 +483,19 @@
         return None
 
 
+class ProductSeriesDetailedDisplayView(ProductSeriesView):
+
+    @cachedproperty
+    def latest_milestones(self):
+        # Convert to list to avoid the query being run multiple times.
+        return list(self.context.milestones[:12])
+
+    @cachedproperty
+    def latest_releases(self):
+        # Convert to list to avoid the query being run multiple times.
+        return list(self.context.releases[:12])
+
+
 class ProductSeriesUbuntuPackagingView(LaunchpadFormView):
 
     schema = IPackaging

=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
--- lib/lp/registry/browser/tests/productseries-views.txt	2010-08-23 04:48:17 +0000
+++ lib/lp/registry/browser/tests/productseries-views.txt	2010-08-25 04:29:43 +0000
@@ -307,7 +307,7 @@
 specifications that will be unassigned, and release files that will be
 deleted are available.
 
-    >>> view.milestones
+    >>> list(view.milestones)
     []
     >>> view.bugtasks
     []

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2010-08-22 18:31:30 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2010-08-25 04:29:43 +0000
@@ -160,7 +160,7 @@
             }
         view = create_initialized_view(milestone, '+delete', form=form)
         self.assertEqual([], view.errors)
-        self.assertEqual(0, len(product.all_milestones))
+        self.assertEqual([], list(product.all_milestones))
         self.assertEqual(0, product.development_focus.all_bugtasks.count())
 
 

=== modified file 'lib/lp/registry/interfaces/milestone.py'
--- lib/lp/registry/interfaces/milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/milestone.py	2010-08-25 04:29:43 +0000
@@ -253,6 +253,8 @@
     """An interface for classes providing milestones."""
     export_as_webservice_entry()
 
+    has_milestones = Bool(title=_("Whether the object has any milestones."))
+
     milestones = exported(
         CollectionField(
             title=_("The visible and active milestones associated with this "

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/milestone.py	2010-08-25 04:29:43 +0000
@@ -82,6 +82,9 @@
 class HasMilestonesMixin:
     implements(IHasMilestones)
 
+    _milestone_order = (
+        'milestone_sort_key(Milestone.dateexpected, Milestone.name) DESC')
+
     def _getMilestoneCondition(self):
         """Provides condition for milestones and all_milestones properties.
 
@@ -93,11 +96,15 @@
             "Unexpected class for mixin: %r" % self)
 
     @property
+    def has_milestones(self):
+        return self.all_milestones.any() is not None
+
+    @property
     def all_milestones(self):
         """See `IHasMilestones`."""
         store = Store.of(self)
         result = store.find(Milestone, self._getMilestoneCondition())
-        return sorted(result, key=milestone_sort_key, reverse=True)
+        return result.order_by(self._milestone_order)
 
     @property
     def milestones(self):
@@ -106,7 +113,7 @@
         result = store.find(Milestone,
                             And(self._getMilestoneCondition(),
                                 Milestone.active == True))
-        return sorted(result, key=milestone_sort_key, reverse=True)
+        return result.order_by(self._milestone_order)
 
 
 class Milestone(SQLBase, StructuralSubscriptionTargetMixin, HasBugsBase):

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/projectgroup.py	2010-08-25 04:29:43 +0000
@@ -94,7 +94,6 @@
 from lp.registry.model.mentoringoffer import MentoringOffer
 from lp.registry.model.milestone import (
     Milestone,
-    milestone_sort_key,
     ProjectMilestone,
     )
 from lp.registry.model.pillar import HasAliasMixin
@@ -164,8 +163,6 @@
     bug_reported_acknowledgement = StringCol(default=None)
     max_bug_heat = Int()
 
-    # convenient joins
-
     @property
     def products(self):
         return Product.selectBy(project=self, active=True, orderBy='name')
@@ -419,10 +416,25 @@
         result.group_by(Milestone.name)
         if only_active:
             result.having('BOOL_OR(Milestone.active) = TRUE')
-        milestones = shortlist(
+        # MIN(Milestone.dateexpected) has to be used to match the
+        # aggregate function in the `columns` variable.
+        result.order_by(
+            'milestone_sort_key(MIN(Milestone.dateexpected), Milestone.name) '
+            'DESC')
+        return shortlist(
             [ProjectMilestone(self, name, dateexpected, active)
              for name, dateexpected, active in result])
-        return sorted(milestones, key=milestone_sort_key, reverse=True)
+
+    @property
+    def has_milestones(self):
+        """See `IHasMilestones`."""
+        store = Store.of(self)
+        result = store.find(
+            Milestone.id,
+            And(Milestone.product == Product.id,
+                Product.project == self,
+                Product.active == True))
+        return result.any() is not None
 
     @property
     def milestones(self):

=== modified file 'lib/lp/registry/stories/productseries/xx-productseries-series.txt'
--- lib/lp/registry/stories/productseries/xx-productseries-series.txt	2010-05-24 20:23:19 +0000
+++ lib/lp/registry/stories/productseries/xx-productseries-series.txt	2010-08-25 04:29:43 +0000
@@ -29,7 +29,7 @@
     >>> series_trunk = find_tag_by_id(content, 'series-trunk')
     >>> print extract_text(series_trunk)
     trunk series Focus of Development
-    Milestones: 1.0    Releases: 0.9.2, 0.9.1, 0.9
+    Latest milestones: 1.0    Latest releases: 0.9.2, 0.9.1, 0.9
     Bugs targeted: None
     Blueprints targeted: 1 Unknown
     The "trunk" series represents the primary line of development rather ...
@@ -46,7 +46,7 @@
     >>> series_1_0 = find_tag_by_id(content, 'series-1-0')
     >>> print extract_text(series_1_0)
     1.0 series Active Development
-    Releases: 1.0.0
+    Latest releases: 1.0.0
     Bugs targeted: 1 New
     Blueprints targeted: None
     The 1.0 branch of the Mozilla web browser. Currently, this is the ...

=== modified file 'lib/lp/registry/templates/object-milestones.pt'
--- lib/lp/registry/templates/object-milestones.pt	2009-11-07 00:49:26 +0000
+++ lib/lp/registry/templates/object-milestones.pt	2010-08-25 04:29:43 +0000
@@ -28,7 +28,7 @@
   <tal:milestones define="milestones context/all_milestones">
     <table id="milestones" class="listing"
       tal:define="has_series context/series|nothing"
-      tal:condition="milestones">
+      tal:condition="context/has_milestones">
       <thead>
         <tr>
           <th>Version</th>
@@ -45,7 +45,7 @@
       </tbody>
     </table>
 
-    <tal:no-milestones condition="not: milestones">
+    <tal:no-milestones condition="not: context/has_milestones">
       <p>There are no milestones associated with
         <span tal:replace="context/title" />
       </p>

=== added file 'lib/lp/registry/templates/productseries-detailed-display.pt'
--- lib/lp/registry/templates/productseries-detailed-display.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/templates/productseries-detailed-display.pt	2010-08-25 04:29:43 +0000
@@ -0,0 +1,38 @@
+<div
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  >
+  <strong><a tal:attributes="href context/fmt:url"
+             tal:content="context/name" >
+      1.0
+    </a> series
+  </strong>
+  <em tal:condition="context/is_development_focus">Focus of Development</em>
+  <em tal:condition="not: context/is_development_focus"
+      tal:content="context/status/title">
+    This series' status
+  </em>
+  <div>
+    <tal:milestones condition="view/latest_milestones">
+      Latest milestones:
+      <tal:milestone repeat="milestone view/latest_milestones">
+        <a tal:attributes="href milestone/fmt:url" tal:content="milestone/name"
+          >name</a><tal:comma condition="not:repeat/milestone/end">,</tal:comma>
+      </tal:milestone>
+    </tal:milestones>
+    <tal:release repeat="release view/latest_releases">
+      <tal:release-start condition="repeat/release/start">
+        <tal:comment condition="nothing">
+          Releases may be a list or an resultset. We cannot easily know if there
+          releases until the first one is returned.
+        </tal:comment>
+        <tal:space condition="view/latest_milestones"> &nbsp &nbsp </tal:space>
+        Latest releases:
+      </tal:release-start>
+      <a tal:attributes="href release/fmt:url" tal:content="release/version"
+        >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
+    </tal:release>
+  </div>
+</div>

=== modified file 'lib/lp/registry/templates/productseries-macros.pt'
--- lib/lp/registry/templates/productseries-macros.pt	2010-05-24 20:23:19 +0000
+++ lib/lp/registry/templates/productseries-macros.pt	2010-08-25 04:29:43 +0000
@@ -4,48 +4,6 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   omit-tag="">
 
-<metal:detailed_display define-macro="detailed_display">
-  <tal:comment replace="nothing">
-    This macro expects two variables to be defined:
-      - 'series': The ProductSeries
-      - 'is_focus': A boolean saying whether this is the series in which
-                    development is focused.
-  </tal:comment>
-
-  <strong><a tal:attributes="href series/fmt:url"
-             tal:content="series/name" >
-      1.0
-    </a> series
-  </strong>
-  <em tal:condition="is_focus">Focus of Development</em>
-  <em tal:condition="not: is_focus" tal:content="series/status/title">
-    This series' status
-  </em>
-  <div>
-    <tal:milestones condition="series/milestones">
-      Milestones:
-      <tal:milestone repeat="milestone series/milestones">
-        <a tal:attributes="href milestone/fmt:url" tal:content="milestone/name"
-          >name</a><tal:comma condition="not:repeat/milestone/end">,</tal:comma>
-      </tal:milestone>
-    </tal:milestones>
-    <tal:release repeat="release series/releases">
-      <tal:release-start condition="repeat/release/start">
-        <tal:comment condition="nothing">
-          Releases may be a list or an resultset. We cannot easily know if there
-          releases until the first one is returned.
-        </tal:comment>
-        <tal:space condition="series/milestones"> &nbsp &nbsp </tal:space>
-        Releases:
-      </tal:release-start>
-      <a tal:attributes="href release/fmt:url" tal:content="release/version"
-        >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
-    </tal:release>
-  </div>
-  <metal:extra define-slot="extra" />
-</metal:detailed_display>
-
-
 <metal:branch_display define-macro="branch_display">
   <tal:comment condition="nothing">
     This macro expects two variables to be defined:

=== modified file 'lib/lp/registry/templates/productseries-status.pt'
--- lib/lp/registry/templates/productseries-status.pt	2009-07-21 18:17:49 +0000
+++ lib/lp/registry/templates/productseries-status.pt	2010-08-25 04:29:43 +0000
@@ -8,8 +8,7 @@
     is_focus context/is_development_focus;
     spec_count_status view/specification_status_counts;"
   >
-  <metal:series use-macro="series/@@+macros/detailed_display">
-    <div metal:fill-slot="extra">
+  <div tal:replace="structure context/@@+detailed-display"/>
       <div>
         <tal:not-obsolete
           condition="not: view/is_obsolete"
@@ -42,6 +41,4 @@
       <tal:summary
         condition="series/summary"
         content="structure context/summary/fmt:text-to-html" />
-    </div>
-  </metal:series>
 </div>