Merge lp:~bac/launchpad/bug-487965 into lp:launchpad

= Summary =

Bug 487965 reports an OOPS that occurs when attempting to look at
packages for a product when there are more than one distribution
represented.

== Proposed fix ==

The dictionaries representing the distros and packaging used to have an
entry with the distribution name in it. Since the name was used as the
key for the containing dictionary it, having the name in the distro
dictionary looked redundant and it was inadvertently removed.

While I could've just stuffed it back in I though restructuring the code
to be less fragile was a better solution.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt packaging-views.txt

== Demo and Q/A ==

Visit the product in the bug report and ensure it doesn't OOPS.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/packaging-views.txt

== Pylint notices ==

lib/lp/registry/browser/product.py
    54: [F0401] Unable to import 'lazr.delegates' (No module named
delegates)

Hi Brad,

This is a nice improvement.

merge-conditional

-Edwin

>=== modified file 'lib/lp/registry/browser/product.py'
>--- lib/lp/registry/browser/product.py 2009-11-23 14:52:25 +0000
>+++ lib/lp/registry/browser/product.py 2009-11-25 18:30:38 +0000
>@@ -474,9 +474,9 @@
>
> def _sort_distros(a, b):

Since this function doesn't actually sort a & b, it should be
called _compare_distros.

> """Put Ubuntu first, otherwise in alpha order."""
>- if a['name'] == 'ubuntu':
>+ if a == 'ubuntu':
> return -1

This only works if 'ubuntu' is always passed in as the first
parameter. There needs to be a corresponding if-statement for
b=='ubuntu'.

>- return cmp(a['name'], b['name'])
>+ return cmp(a, b)
>
>
> class ProductSetBreadcrumb(Breadcrumb):
>@@ -947,29 +947,33 @@
> title, and an attribute "packagings" which is a list of the relevant
> packagings for this distro and product.
> """
>- distros = {}
>- # first get a list of all relevant packagings
>+ # First get a list of all relevant packagings.
> all_packagings = []
> for series in self.context.series:
> for packaging in series.packagings:
> all_packagings.append(packaging)
>- # we sort it so that the packagings will always be displayed in the
>- # distroseries version, then productseries name order
>+ # We sort it so that the packagings will always be displayed in the
>+ # distroseries version, then productseries name order.
> all_packagings.sort(key=lambda a: (a.distroseries.version,
> a.productseries.name, a.id))
>+
>+ distros = {}
> for packaging in all_packagings:
>- if distros.has_key(packaging.distroseries.distribution.name):
>- distro = distros[packaging.distroseries.distribution.name]
>+ distribution = packaging.distroseries.distribution
>+ if distribution.name in distros:
>+ distro = distros[distribution.name]
> else:
>- distro = {}
>- distro['distribution'] = packaging.distroseries.distribution
>- distro['packagings'] = []
>+ # Create a dictionary for the distribution.
>+ distro = dict(
>+ distribution=packaging.distroseries.distribution,

Since you created the new distribution variable, this could be shortened to:
  distribution=distribution

>+ packagings=[])
> distros[packaging.distroseries.distribution.name] = distro

The index could be shortened to just distribution.name.
Alternatively, the entire if-else-statement could be replaced with:
    distro = distros.setdefault(
        distribution.name, dict(distribution=distribution, packaging=[]))
but that is up to you, since you might find it less readable.

> distro['packagings'].append(packaging)
>- # now we sort the resulting set of "distro" objects, and return that
>- result = distros.values()
>- result.sort(cmp=_sort_distros)
>- return result
>+ # Now we sort the resulting list of "distro" objects, and re...