> == Proposed fix ==
> Many changes!  3.0 goodness.  View and see.
> e.g. https://dogfood.launchpad.net/ubuntu/+source/amarok

Looks great, thanks for working on this!

> == Implementation details ==
> The old page was tough to render, this one is no different.  The hard part is
> the table of the versions in each series.
> 
> Most of the logic is done in the view class, which pumps out rows of
> dictionaries that the template then throws on the page.  It uses 3 different
> types of row:
>  * a distro series header
>  * package details
>  * blank row
> 
> The type of row is determined by boolean dictionary properties in each row
> returned by view/version_table.  I couldn't think of a better way of doing
> this since all the knowledge of how to render stuff is in the view.
> 
> The package detail rows also have a JS expander that does an XHR call to get
> more data.  This is the same XHR call that the PPA page does to open rows
> there.  I had to tweak the JS a little to make it work outside of an archive
> context.  It does this by coding the archive url in the template as a <script>
> element - not pleasant but having discussed this at length with Michael and
> Celso we could not think of any other way of doing it.

It sounds like you had pretty thorough pre-imps about a number of tricky
issues.  That helps with the review a lot, thanks for having them.

> == Demo and Q/A ==
> I set this up on dogfood since the .dev data is rubbish.
> See:
> https://dogfood.launchpad.net/ubuntu/+source/amarok
> https://dogfood.launchpad.net/ubuntu/+source/firefox-3.5
> etc.

That was very helpful!

> = Launchpad lint =
> Once again I have been foiled by lint being at the bottom of the email
> template instead of at the top.  I'm not re-writing this email, so please
> consider the lint fixing done as I will be doing it right after sending this.

This bites me all the time too :(.  No problem.

The code looks generally pretty good.  I have a number of comments, but I
think they will be easy to address, so I'll give the branch merge-conditional,
r=me with their consideration.

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-08-27 11:34:14 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-08-27 20:18:32 +0000
> @@ -523,6 +523,7 @@
>          classes="
>              DistributionSourcePackageFacets
>              DistributionSourcePackageOverviewMenu
> +            DistributionSourcePackageActionMenu
>              DistributionSourcePackageBugsMenu"

Please sort alphabetically.

>          module="lp.registry.browser.distributionsourcepackage"/>
>      <browser:url

=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py	2009-08-27 10:13:51 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py	2009-08-27 20:18:32 +0000
> @@ -127,7 +149,29 @@
>          return self._package_diffs
>  
>  
> +class IDistributionSourcePackageActionMenu(Interface):
> +    """Marker interface for the action menu."""
> +
> +
> +class DistributionSourcePackageActionMenu(
> +    NavigationMenu, DistributionSourcePackageLinksMixin):
> +    """Action menu for distro source packages."""
> +    usedfor = IDistributionSourcePackageActionMenu
> +    facet = 'overview'
> +    title = 'Actions'
> +    links = ('change_log', 'subscribe', 'edit')

While I prefer using tuples for these, as you've shown here, our defacto
standard is to use lists.  Is there a reason why you did it this way?
Otherwise, might as well stick with the trend and use a list here too.

>  class DistributionSourcePackageView(LaunchpadFormView):
> +    implements(IDistributionSourcePackageActionMenu)

Docstring.

> +
> +    @property
> +    def page_title(self):
> +        return self.context.title
>  
>      @property
>      def page_title(self):
> @@ -268,6 +312,7 @@
>          """Render a submit input for the delete_packaging_action."""
>          assert self.can_delete_packaging, 'User cannot delete Packaging.'
>          return ('<input type="submit" class="button" value="Delete Link" '
> +                'style="padding: 0pt; font-size: 80%%" '

Do you think it would be better to move this style setting into a class inside
style-3-0.css?

> @@ -301,41 +346,112 @@
>                distroseries=distroseries.displayname)))
>          self.next_url = canonical_url(self.context)
>  
> -    def version_listing(self):
> -        result = []
> -        for sourcepackage in self.context.get_distroseries_packages():
> -            packaging = sourcepackage.direct_packaging
> +    @cachedproperty
> +    def active_distroseries_packages(self):
> +        """Cached proxy call to context/get_distroseries_packages."""
> +        return self.context.get_distroseries_packages()
> +
> +    @property
> +    def packages_by_active_distroseries(self):
> +        """Dict of packages keyed by distroseries."""
> +        packages_dict = {}
> +        for package in self.active_distroseries_packages:
> +            packages_dict[package.distroseries] = package
> +        return packages_dict
> +
> +    @property
> +    def active_series(self):
> +        """Return active distroseries where this package is published.
> +
> +        Used in the template code that shows the table of versions.
> +        The returned series are sorted in reverse order of creation.
> +        """
> +        series = set()
> +        for package in self.active_distroseries_packages:
> +            series.add(package.distroseries)
> +        result = list(series)
> +        result.sort(key=operator.attrgetter("version"), reverse=True)

You should be able to write these last two lines as:

        result = sorted(series, key=operator.attrgetter('version'),
                        reverse=True)

> +        return result
> +
> +    def published_by_version(self, sourcepackage):
> +        """Return a dict of publications keyed by version.
> +
> +        :param sourcepackage: ISourcePackage
> +        """
> +        publications = sourcepackage.distroseries.getPublishedReleases(
> +            sourcepackage.sourcepackagename)
> +        pocket_dict = {}
> +        for pub in publications:
> +            version = pub.source_package_version
> +            if version not in pocket_dict:
> +                pocket_dict[version] = [pub]
> +            else:
> +                pocket_dict[version].append(pub)

What do you think about this instead...

            pocket_dict.setdefault(version, []).append(pub)

instead of the 4 preceding lines?

> +        return pocket_dict
> +
> +    @property
> +    def version_table(self):
> +        """Rows of data for the template to render in the packaging table."""
> +        rows = []
> +        packages_by_series = self.packages_by_active_distroseries
> +        for distroseries in self.active_series:
> +            # The first row for each series is the "title" row.
> +            packaging = packages_by_series[distroseries].direct_packaging
>              if packaging is None:
>                  delete_packaging_form_id = None
> -                packaging_field = None
> +                hidden_packaging_field = None
>              else:
>                  delete_packaging_form_id = "delete_%s_%s_%s" % (
>                      packaging.distroseries.name,
>                      packaging.productseries.product.name,
>                      packaging.productseries.name)
> -                packaging_field = self._renderHiddenPackagingField(packaging)
> -            series_result = []
> -            for published in \
> -                sourcepackage.published_by_pocket.iteritems():
> -                for drspr in published[1]:
> -                    series_result.append({
> -                        'series': sourcepackage.distroseries,
> -                        'pocket': published[0].name.lower(),
> -                        'package': drspr,
> -                        'packaging': packaging,
> -                        'delete_packaging_form_id': delete_packaging_form_id,
> -                        'packaging_field': packaging_field,
> -                        'sourcepackage': sourcepackage
> -                        })
> -            for row in range(len(series_result)-1, 0, -1):
> -                for column in ['series', 'pocket', 'package', 'packaging',
> -                               'packaging_field', 'sourcepackage']:
> -                    if (series_result[row][column] ==
> -                            series_result[row-1][column]):
> -                        series_result[row][column] = None
> -            for row in series_result:
> -                result.append(row)
> -        return result
> +                hidden_packaging_field = self._renderHiddenPackagingField(
> +                    packaging)
> +            package = packages_by_series[distroseries]
> +            title_row = {
> +                'blank_row': False,
> +                'title_row': True,
> +                'data_row': False,
> +                'distroseries': distroseries,
> +                'series_package': package,
> +                'packaging': packaging,
> +                'hidden_packaging_field': hidden_packaging_field,
> +                'delete_packaging_form_id': delete_packaging_form_id,
> +                }
> +            rows.append(title_row)
> +
> +            # After the title row, we list each package version that's
> +            # currently published, and which pockets it's published in.
> +            pocket_dict = self.published_by_version(package)
> +            for version in pocket_dict.iterkeys():

Is there a known efficiency reason to use iterkeys()?  Is pocket_dict huge?
If not, then just use implicit .keys(), e.g.:

            for version in pocket_dict:

> +                most_recent_publication = pocket_dict[version][0]
> +                if most_recent_publication.datepublished is None:
> +                    published_since = None
> +                else:
> +                    published_since = datetime.now(
> +                        tz=pytz.UTC) - most_recent_publication.datepublished

What about splitting this up into multiple lines:

                    now = datetime.now(tz=pytz.UTC)
                    date_published = most_recent_publication.datepublished
                    published_since = now - date_published

> +                row = {
> +                    'blank_row': False,
> +                    'title_row': False,
> +                    'data_row': True,
> +                    'version': version,
> +                    'publication': most_recent_publication,
> +                    'pockets': ", ".join(
> +                        [pub.pocket.name for pub in pocket_dict[version]]),

It might be more readable to move this join() to above the dict instantiation.

> +                    'component': most_recent_publication.component_name,
> +                    'published_since': published_since,
> +                    }
> +                rows.append(row)
> +            # We need a blank row after each section, so the series
> +            # header row doesn't appear too close to the previous
> +            # section.
> +            rows.append({
> +                'blank_row': True,
> +                'title_row': False,
> +                'data_row': False,
> +                })
> +
> +        return rows
>  
>      def releases(self):
>          dspr_pubs = self.context.getReleasesAndPublishingHistory()

=== modified file 'lib/lp/registry/stories/distribution/xx-distribution-packages.txt'
--- lib/lp/registry/stories/distribution/xx-distribution-packages.txt	2009-08-26 12:05:05 +0000
+++ lib/lp/registry/stories/distribution/xx-distribution-packages.txt	2009-08-28 14:59:57 +0000
> @@ -1,4 +1,9 @@
> -= Distribution Package Search Results =
> +============================
> +Distribution Source Packages
> +============================

Thanks for the conversion to reST style!

> +
> +Distribution Package Search Results
> +-----------------------------------

I think the 2nd level heading under the top-and-bottom-equals would be:

Distribution Package Search Results
===================================

>  
>  Although there is a separate search page, we can search from the
>  distribution overview page which takes us there and displays the
> @@ -24,11 +29,10 @@
>      >>> browser.url
>      'http://localhost/ubuntu/+source/pmount'
>  
> -Get pmount 0.1-1 version
> +Get pmount 0.1-2 version
>  
> -    >>> browser.getLink("0.1-1").click()
> -    >>> browser.url
> -    'http://localhost/ubuntu/+source/pmount/0.1-1'
> +    >>> browser.getLink("0.1-2").url
> +    'http://localhost/ubuntu/+source/pmount/0.1-2'

Better to use print here to reduce noise:

    >>> print browser.getLink('0.1-2').url
    http://localhost/ubuntu/+source/pmount/0.1-2

=== modified file 'lib/lp/registry/templates/distributionsourcepackage-index.pt'
--- lib/lp/registry/templates/distributionsourcepackage-index.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/distributionsourcepackage-index.pt	2009-08-28 15:25:46 +0000
> @@ -3,28 +3,52 @@
>    xmlns:tal="http://xml.zope.org/namespaces/tal"
>    xmlns:metal="http://xml.zope.org/namespaces/metal"
>    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> -  xml:lang="en"
> -  lang="en"
> -  dir="ltr"
> -  metal:use-macro="context/@@main_template/master"
> +  metal:use-macro="view/macro:page/main_side"
>    i18n:domain="launchpad"
>  >
>  
>  <body>
>  
> -  <metal:portlets fill-slot="portlets">
> -    <div tal:replace="structure context/@@+portlet-structural-subscribers" />
> -    <div tal:replace="structure context/@@+portlet-latestbugs" />
> -    <div tal:replace="structure context/@@+portlet-latestquestions" />
> -  </metal:portlets>
> -
> -<div metal:fill-slot="main">
> -
> -  <h1>
> -    &ldquo;<span tal:replace="context/name" />&rdquo; source package in
> -    <span tal:replace="context/distribution/displayname" />
> -  </h1>
> -
> +<metal:block fill-slot="head_epilogue">
> +  <tal:devmode condition="devmode">
> +    <tal:archive_js define="lp_js string:${icingroot}/build">
> +      <script type="text/javascript"
> +              tal:attributes="src string:${lp_js}/soyuz/base.js">
> +      </script>
> +    </tal:archive_js>
> +  </tal:devmode>
> +</metal:block>

Do you think this is better...

<metal:block fill-slot="head_epilogue">
  <tal:archive_js define="lp_js string:${icingroot}/build"
                  tal:condition="devmode">
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/soyuz/base.js" />
  </tal:archive_js>
</metal:block>

?

> @@ -40,61 +64,165 @@
> +  <div class="top-portlet" id="bugs-and-questions-summary"
> +       tal:define="newbugs context/new_bugtasks/count;
> +                   open_questions view/open_questions/count">
> +    This package has
> +    <a tal:attributes="href view/menu:overview/new_bugs/url"><strong class="bugs-stat"><tal:newbugs content="newbugs"/> new bug<tal:plural content="string:s" condition="python: newbugs!=1"/><strong></a>
> +     and <a tal:attributes="href view/menu:overview/open_questions/url"><strong class="answers-stat"><tal:openq content="open_questions"/> open question<tal:plural content="string:s" condition="python: open_questions!=1"/>.</strong></a>

Thanks for using the new bugs-stat class!  But ouch! this run-on TAL is really
hard to read.  Can you reformat it to make the code easier on the eyes?

> +          <sup>*</sup>actual publishing details may vary in this distribution, these are just the package defaults.

Wrap line?

(By now my eyes are melting at all the TAL)

=== modified file 'lib/lp/soyuz/templates/archive-macros.pt'
--- lib/lp/soyuz/templates/archive-macros.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/soyuz/templates/archive-macros.pt	2009-08-25 17:05:15 +0000
> @@ -64,9 +64,18 @@
>          }
>      };
>  
> +    /*
> +     * If a page wants to use this outside of an archive context then it
> +     * car define LP.client.cache['archive_context'], which should be a

s/car/can/

> +     * full or relative URL to the context archive page required.
> +     */
> +    var base_url = (typeof(LP.client.cache['archive_context_url']) != "undefined") ? LP.client.cache['archive_context_url'] : '';

Wrap line?

> +    if (base_url !== ''){

Space between ) and {

> +        base_url += '/'
> +    }
>      var pub_id = container.get('id').replace(
>          '-container', '').replace('pub', '');
> -    var uri = '+sourcepub/' + pub_id + '/+listing-archive-extra';
> +    var uri = base_url + '+sourcepub/' + pub_id + '/+listing-archive-extra';
>      Y.io(uri, config);
>  }
>