Hi Jeroen,
very nice branch and I learned a few new things by reviewing it. Thanks. ;-)

I could not find anything serious so please bear my nit-picks with patience.
And thank you for all the drive-by fixes! ;)

Cheers,
Henning

Am 03.09.2010 08:46, schrieb Jeroen T. Vermeulen:
> > === modified file 'lib/lp/registry/model/pillar.py'
> > @@ -244,18 +230,12 @@
> >                  stacklevel=2)
> >          pillars = []
> >          # Prefill pillar.product.licenses.
> > -        for pillar_name, other, product, project, distro, license_ids in (
> > +        for pillar_name, other, product, project, distro, licenses in (
> >              result[:limit]):
You can fix this confusing indention by pulling the de-tupling into the loop body.

> >              pillar = pillar_name.pillar
> >              if IProduct.providedBy(pillar):
> > -                licenses = [
> > -                    License.items[license_id]
> > -                    for license_id in license_ids]
> > -                product_with_licenses = ProductWithLicenses(
> > -                    pillar, tuple(sorted(licenses)))
> > -                pillars.append(product_with_licenses)
> > -            else:
> > -                pillars.append(pillar)
> > +                pillar = ProductWithLicenses(pillar, licenses)
> > +            pillars.append(pillar)
> >          return pillars
> >
> >      def add_featured_project(self, project):
> >
> > === modified file 'lib/lp/registry/model/product.py'
> > @@ -201,14 +202,25 @@
> >          return LicenseStatus.OPEN_SOURCE
> >
> >
> > +class Array(NamedFunc):
> > +    """Implements the postgres "array" function in Storm."""
> > +    name = 'array'
> > +
> > +
> >  class ProductWithLicenses:
> >      """Caches `Product.licenses`."""
> >
> >      delegates(IProduct, 'product')
> >
> > -    def __init__(self, product, licenses):
> > +    def __init__(self, product, license_ids):
> > +        """Initialize a `ProductWithLicenses`.
> > +
> > +        :param product: the `Product` to wrap.
> > +        :param license_ids: a sequence of numeric `License` ids.
> > +        """
> >          self.product = product
> > -        self._licenses = licenses
> > +        self._licenses = tuple([
> > +            License.items[id] for id in sorted(license_ids)])
You don't need the [] inside the tuple(). Go straight from iterator to tuple.
(I just found that out recently myself.)

> >
> >      @property
> >      def licenses(self):
> > === modified file 'lib/lp/translations/interfaces/translationgroup.py'
> > --- lib/lp/translations/interfaces/translationgroup.py	2010-08-20 20:31:18 +0000
> > +++ lib/lp/translations/interfaces/translationgroup.py	2010-09-03 06:46:46 +0000
> > @@ -186,8 +186,34 @@
> >      def fetchTranslatorData():
> >          """Fetch translators and related data.
> >
> > -        :return: A tuple (`Translator`, `Language`, `Person`), ordered
> > -            by language name in English.
> > +        Prefetches display-related properties.
> > +
> > +        :return: A result set of (`Translator`, `Language`, `Person`),
> > +            ordered by language name in English.
> > +        """
> > +
> > +    def fetchProjectsForDisplay():
I would ask you to use "fetchProductsForDisplay" as long as it has not been
renamed but I fear that others have already done the same thing in other
places of the code ... This goes for all uses of "project" in this branch.

> > +        """Fetch `Product`s using this group, for display purposes.
> > +
> > +        Prefetches display-related properties.
> > +
> > +        :return: A result set of `Product`, ordered by display name.
> > +        """
> > +
> > +    def fetchProjectGroupsForDisplay():
> > +        """Fetch `Project`s using this group, for display purposes.
> > +
> > +        Prefetches display-related properties.
> > +
> > +        :return: A result set of `Project`, ordered by display name.
> > +        """
> > +
> > +    def fetchDistrosForDisplay():
> > +        """Fetch `Distribution`s using this group, for display purposes.
> > +
> > +        Prefetches display-related properties.
> > +
> > +        :return: A result set of `Distribution`, ordered by display name.
> >          """
> >
> >
> >
> > === modified file 'lib/lp/translations/model/translationgroup.py'
> > --- lib/lp/translations/model/translationgroup.py	2010-08-20 20:31:18 +0000
> > +++ lib/lp/translations/model/translationgroup.py	2010-09-03 06:46:46 +0000
> > @@ -1,4 +1,4 @@
> > -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> > +# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
> >  # GNU Affero General Public License version 3 (see the file LICENSE).
> >
> >  # pylint: disable-msg=E0611,W0212
> > @@ -6,7 +6,7 @@
> >  __metaclass__ = type
> >  __all__ = [
> >      'TranslationGroup',
> > -    'TranslationGroupSet'
> > +    'TranslationGroupSet',
> >      ]
> >
> >  from sqlobject import (
> > @@ -16,7 +16,10 @@
> >      SQLRelatedJoin,
> >      StringCol,
> >      )
> > -from storm.expr import Join
> > +from storm.expr import (
> > +    Join,
> > +    LeftJoin,
> > +    )
> >  from storm.store import Store
> >  from zope.component import getUtility
> >  from zope.interface import implements
> > @@ -24,17 +27,27 @@
> >  from canonical.database.constants import DEFAULT
> >  from canonical.database.datetimecol import UtcDateTimeCol
> >  from canonical.database.sqlbase import SQLBase
> > +from canonical.launchpad.database.librarian import (
> > +    LibraryFileAlias,
> > +    LibraryFileContent,
> > +    )
> >  from canonical.launchpad.webapp.interfaces import (
> >      DEFAULT_FLAVOR,
> >      IStoreSelector,
> >      MAIN_STORE,
> > +    SLAVE_FLAVOR,
Can you check if you cannot use ISlaveStore instead, please?

> >      )
> >  from lp.app.errors import NotFoundError
> >  from lp.registry.interfaces.person import validate_public_person
> > +from lp.registry.model.distribution import Distribution
> >  from lp.registry.model.person import Person
> > -from lp.registry.model.product import Product
> > +from lp.registry.model.product import (
> > +    Product,
> > +    ProductWithLicenses,
> > +    )
> >  from lp.registry.model.projectgroup import ProjectGroup
> >  from lp.registry.model.teammembership import TeamParticipation
> > +from lp.services.database.prejoin import prejoin
> >  from lp.services.worlddata.model.language import Language
> >  from lp.translations.interfaces.translationgroup import (
> >      ITranslationGroup,
> > @@ -77,10 +90,10 @@
> >              Translator.translationgroup == self,
> >              Translator.languageID == Language.id,
> >              Language.code == language_code)
> > -
> > +
> >          translator = query.one()
> >          if translator is None:
> > -            raise NotFoundError, language_code
> > +            raise NotFoundError(language_code)
> >
> >          return translator
> >
> > @@ -141,14 +154,106 @@
> >              return 0
> >
> >      def fetchTranslatorData(self):
> > -        """See ITranslationGroup."""
> > +        """See `ITranslationGroup`."""
> >          store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> > -        translator_data = store.find(
> > -            (Translator, Language, Person),
> > +        # Fetch Translator, Language, and Person; but also prefetch the
> > +        # icon information.
> > +        using = [
> > +            Translator,
> > +            Language,
> > +            Person,
> > +            LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Person.iconID),
> > +            LeftJoin(
> > +                LibraryFileContent,
> > +                LibraryFileContent.id == LibraryFileAlias.contentID),
> > +            ]
> > +        tables = (
> > +            Translator,
> > +            Language,
> > +            Person,
> > +            LibraryFileAlias,
> > +            LibraryFileContent,
> > +            )
> > +        translator_data = store.using(*using).find(
> > +            tables,
> >              Translator.translationgroup == self,
> >              Language.id == Translator.languageID,
> >              Person.id == Translator.translatorID)
> > -        return translator_data.order_by(Language.englishname)
> > +
> > +        return prejoin(
> > +            translator_data.order_by(Language.englishname),
> > +            slice(0, 3))
Did you know that you can apply the "order_by" to the result of "prejoin" as
well? I don't know, though, if that would really look better.

> > +
> > +    def fetchProjectsForDisplay(self):
> > +        """See `ITranslationGroup`."""
> > +        store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
> > +        using = [
> > +            Product,
> > +            LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Product.iconID),
> > +            LeftJoin(
> > +                LibraryFileContent,
> > +                LibraryFileContent.id == LibraryFileAlias.contentID),
> > +            ]
> > +        columns = (
> > +            Product,
> > +            ProductWithLicenses.composeLicensesColumn(),
> > +            LibraryFileAlias,
> > +            LibraryFileContent,
> > +            )
> > +        product_data = store.using(*using).find(
> > +            columns,
> > +            Product.translationgroupID == self.id, Product.active == True)
> > +        product_data = product_data.order_by(Product.displayname)
> > +
> > +        return [
> > +            ProductWithLicenses(product, tuple(licenses))
> > +            for product, licenses, icon_alias, icon_content in product_data]
> > +
> > +    def fetchProjectGroupsForDisplay(self):
> > +        """See `ITranslationGroup`."""
> > +        store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
> > +        using = [
> > +            ProjectGroup,
> > +            LeftJoin(
> > +                LibraryFileAlias, LibraryFileAlias.id == ProjectGroup.iconID),
> > +            LeftJoin(
> > +                LibraryFileContent,
> > +                LibraryFileContent.id == LibraryFileAlias.contentID),
> > +            ]
> > +        tables = (
> > +            ProjectGroup,
> > +            LibraryFileAlias,
> > +            LibraryFileContent,
> > +            )
> > +        product_data = store.using(*using).find(
> > +            tables,
> > +            ProjectGroup.translationgroupID == self.id,
> > +            ProjectGroup.active == True)
> > +
> > +        return prejoin(
> > +            product_data.order_by(ProjectGroup.displayname), slice(0, 1))
slice(0, 1) is the default for prejoin, so you could leave it out here.

> > +
> > +    def fetchDistrosForDisplay(self):
> > +        """See `ITranslationGroup`."""
> > +        store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
> > +        using = [
> > +            Distribution,
> > +            LeftJoin(
> > +                LibraryFileAlias, LibraryFileAlias.id == Distribution.iconID),
> > +            LeftJoin(
> > +                LibraryFileContent,
> > +                LibraryFileContent.id == LibraryFileAlias.contentID),
> > +            ]
> > +        tables = (
> > +            Distribution,
> > +            LibraryFileAlias,
> > +            LibraryFileContent,
> > +            )
> > +        product_data = store.using(*using).find(
> > +            tables, Distribution.translationgroupID == self.id)
> > +
> > +        return prejoin(
> > +            product_data.order_by(Distribution.displayname), slice(0, 1))
> >
> >
> >  class TranslationGroupSet:
> > @@ -174,7 +279,7 @@
> >          try:
> >              return TranslationGroup.byName(name)
> >          except SQLObjectNotFound:
> > -            raise NotFoundError, name
> > +            raise NotFoundError(name)
> >
> >      def new(self, name, title, summary, translation_guide_url, owner):
> >          """See ITranslationGroupSet."""
> > @@ -193,7 +298,7 @@
> >              Join(Translator,
> >                  Translator.translationgroupID == TranslationGroup.id),
> >              Join(TeamParticipation,
> > -                TeamParticipation.teamID == Translator.translatorID)
> > +                TeamParticipation.teamID == Translator.translatorID),
> >              ]
> >          result = store.using(*origin).find(
> >              TranslationGroup, TeamParticipation.person == person)
> > @@ -203,4 +308,3 @@
> >      def getGroupsCount(self):
> >          """See ITranslationGroupSet."""
> >          return TranslationGroup.select().count()
> > -
> >