Merge lp:~jtv/launchpad/bug-618393 into lp:launchpad

[Jeroen T. Vermeulen]

TranslationGroup:+index optimization, ProductWithLicenses cleanup

= Bug 618393 =

Working on some timeouts on the TranslationGroup pages.

I looked at one oops in detail. A narrow majority of the time was taken up by database queries. So I mostly attacked those before resorting to python profiling.

Here I exploit several optimization opportunities:
 * Prejoin icon data for persons, persons, etc. (20% of queries).
 * Prejoin project license information (15% of queries).
 * Cache whether user has Edit privileges in the view.
 * Query translators once; avoid redundant is_empty check.
 * Query for projects/project groups/distributions on the slave store.

The most interesting work is on prefetching project license information. What makes this necessary is the link formatter for Product: it queries for licenses in order to flag projects whose licenses have not yet been set.

These are not queries one can avoid through Storm caching. Luckily there is a class that lets me get around that: ProductWithLicenses. It's a wrapper for Product that you can pre-seed with the project's licenses, which you can obtain from the same query that gave you the Product itself. This helper is still a bit rough around the edges; I added a class method to compose the necessary join in Storm and took some of the data massage from the call site into the constructor.

You'll notice that I had to change the license_status property in ProductWithLicenses. The reason is a subtle mismatch in access permissions between Product and ProductWithLicenses: license_status is publicly visible either way, but it is implemented in terms of license_approved and license_reviewed which require special privileges. The Product implementation got around this trivially: license_status is public, and once you're in the property code, you're past the security proxy and can access self.license_approved etc. without problems. But the ProductWithLicenses implementation is an outsider: when it evaluates self.license_approved it delegates to self.product, which is still shielded by a security proxy. Thus ProductWithLicenses de facto required special privileges. I bypassed the proxy for this, exposing license_status but not license_approved—exactly as in Product.

To test,
{{{
./bin/test -vvc lp.translations -t translationgroup
./bin/test -vvc lp.registry -t productwithlicenses -t pillar.txt
}}}

To Q/A, go to

    https://translations.edge.launchpad.net/+groups/ubuntu-translators/+index

It should not time out. Also, the page should issue at most 600 or so queries, as opposed to the current 1,000.

No lint,

Jeroen

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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! ;)

 review approve code

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

review: Approve (code)

[Henning Eggers (henninge)]

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

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks. Wow, you did some careful research!

All your suggestions implemented, except moving the order_by out of the prejoin. (If nothing else, I like to avoid any risk of triggering query-before-slice bugs a priori). I didn't know tuple comprehensions were in python 2.5; I thought they were 2.6-only and so didn't use them.

You're also right that I shouldn't have said "Project" for "Product" in model code. It was reckless of me, and it is now undone.

=== modified file 'lib/lp/registry/model/pillar.py'
--- lib/lp/registry/model/pillar.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/pillar.py	2010-09-03 11:18:46 +0000
@@ -17,11 +17,7 @@
     ForeignKey,
     StringCol,
     )
-from storm.expr import (
-    LeftJoin,
-    NamedFunc,
-    Select,
-    )
+from storm.expr import LeftJoin
 from storm.info import ClassAlias
 from storm.locals import SQL
 from storm.store import Store
@@ -51,11 +47,9 @@
 from lp.registry.interfaces.product import (
     IProduct,
     IProductSet,
-    License,
     )
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
 from lp.registry.model.featuredproject import FeaturedProject
-from lp.registry.model.productlicense import ProductLicense
 
 
 __all__ = [
@@ -197,23 +191,15 @@
 
     def search(self, text, limit):
         """See `IPillarSet`."""
-        # These classes are imported in this method to prevent an import loop.
-        from lp.registry.model.product import (
-            Product, ProductWithLicenses)
+        # Avoid circular import.
+        from lp.registry.model.product import ProductWithLicenses
         if limit is None:
             limit = config.launchpad.default_batch_size
 
-        class Array(NamedFunc):
-            """Storm representation of the array() PostgreSQL function."""
-            name = 'array'
-
         # Pull out the licenses as a subselect which is converted
         # into a PostgreSQL array so that multiple licenses per product
         # can be retrieved in a single row for each product.
-        extra_column = Array(
-            Select(columns=[ProductLicense.license],
-                   where=(ProductLicense.product == Product.id),
-                   tables=[ProductLicense]))
+        extra_column = ProductWithLicenses.composeLicensesColumn()
         result = self.build_search_query(text, [extra_column])
 
         # If the search text matches the name or title of the
@@ -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]):
             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'
--- lib/lp/registry/model/product.py	2010-09-03 03:12:39 +0000
+++ lib/lp/registry/model/product.py	2010-09-03 11:18: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
 
@@ -25,11 +25,13 @@
     SQLObjectNotFound,
     StringCol,
     )
+from storm.expr import NamedFunc
 from storm.locals import (
     And,
     Desc,
     Int,
     Join,
+    Select,
     SQL,
     Store,
     Unicode,
@@ -85,7 +87,6 @@
     SpecificationDefinitionStatus,
     SpecificationFilter,
     SpecificationImplementationStatus,
-    SpecificationSort,
     )
 from lp.blueprints.model.specification import (
     HasSpecificationsMixin,
@@ -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)])
 
     @property
     def licenses(self):
@@ -223,8 +235,40 @@
         `Product.licenses`, which is not cached, instead of
         `ProductWithLicenses.licenses`, which is cached.
         """
+        naked_product = removeSecurityProxy(self.product)
         return get_license_status(
-            self.license_approved, self.license_reviewed, self.licenses)
+            naked_product.license_approved, naked_product.license_reviewed,
+            self.licenses)
+
+    @classmethod
+    def composeLicensesColumn(cls, for_class=None):
+        """Compose a Storm column specification for licenses.
+
+        Use this to render a list of `Product` linkes without querying
+        licenses for each one individually.
+
+        It lets you prefetch the licensing information in the same
+        query that fetches a `Product`.  Just add the column spec
+        returned by this function to the query, and pass it to the
+        `ProductWithLicenses` constructor:
+
+        license_column = ProductWithLicenses.composeLicensesColumn()
+        products_with_licenses = [
+            ProductWithLicenses(product, licenses)
+            for product, licenses in store.find(Product, license_column)
+            ]
+
+        :param for_class: Class to find licenses for.  Defaults to
+            `Product`, but could also be a Storm `ClassAlias`.
+        """
+        if for_class is None:
+            for_class = Product
+
+        return Array(
+            Select(
+                columns=[ProductLicense.license],
+                where=(ProductLicense.product == for_class.id),
+                tables=[ProductLicense]))
 
 
 class Product(SQLBase, BugTargetBase, MakesAnnouncements,

=== added file 'lib/lp/registry/tests/test_productwithlicenses.py'
--- lib/lp/registry/tests/test_productwithlicenses.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_productwithlicenses.py	2010-09-03 11:18:46 +0000
@@ -0,0 +1,114 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for `ProductWithLicenses`."""
+
+__metaclass__ = type
+
+from operator import attrgetter
+
+from storm.store import Store
+from zope.interface.verify import verifyObject
+
+from canonical.launchpad.ftests import (
+    ANONYMOUS,
+    login,
+    )
+from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.interfaces.product import (
+    IProduct,
+    License,
+    LicenseStatus,
+    )
+from lp.registry.model.product import (
+    Product,
+    ProductWithLicenses,
+    )
+from lp.testing import TestCaseWithFactory
+
+
+class TestProductWithLicenses(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_baseline(self):
+        product = self.factory.makeProduct()
+        product_with_licenses = ProductWithLicenses(product, [])
+        # Log in--a full verification takes Edit privileges.
+        login('[e-mail]')
+        self.assertTrue(verifyObject(IProduct, product_with_licenses))
+
+    def test_uses_cached_licenses(self):
+        # The ProductWithLicenses' licensing information is based purely
+        # on the cached licenses list.  The database is not queried to
+        # determine licensing status.
+        product = self.factory.makeProduct(licenses=[License.BSD])
+        product_with_licenses = ProductWithLicenses(
+            product, [License.OTHER_PROPRIETARY.value])
+        license_status = product_with_licenses.license_status
+        self.assertEqual(LicenseStatus.PROPRIETARY, license_status)
+
+    def test_sorts_licenses(self):
+        # The ProductWithLicenses constructor sorts the Licenses by
+        # numeric value.
+        product = self.factory.makeProduct()
+        licenses = [License.AFFERO, License.BSD, License.MIT]
+
+        # Feed the constructor a list of ids in the wrong order.
+        product_with_licenses = ProductWithLicenses(
+            product,
+            sorted([license.value for license in licenses], reverse=True))
+
+        expected = sorted(licenses, key=attrgetter('value'))
+        self.assertEqual(tuple(expected), product_with_licenses.licenses)
+
+    def test_compose_column_without_licenses_produces_empty(self):
+        # The licenses column that ProductWithLicenses produces for a
+        # product without licenses contains an empty list.
+        product = self.factory.makeProduct(licenses=[])
+        column = ProductWithLicenses.composeLicensesColumn()
+        store = Store.of(product)
+        result = list(store.find((Product, column), Product.id == product.id))
+
+        self.assertEqual([(product, [])], result)
+
+    def test_licenses_column_contains_licensing_info(self):
+        # Feeding the licenses column into the ProductWithLicenses
+        # constructor seeds it with the appropriate licenses.
+        product = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        column = ProductWithLicenses.composeLicensesColumn()
+        store = Store.of(product)
+        row = store.find((Product, column), Product.id == product.id).one()
+
+        product_with_licenses = ProductWithLicenses(*row)
+        licenses = product_with_licenses.licenses
+        license_status = product_with_licenses.license_status
+        self.assertEqual((License.OTHER_PROPRIETARY, ), licenses)
+        self.assertEqual(LicenseStatus.PROPRIETARY, license_status)
+
+    def test_licenses_column_aggregates(self):
+        # Adding a licensing column for a product with multiple licenses
+        # still finds a single product, not one per license.
+        licenses = [License.AFFERO, License.GNU_GPL_V3]
+        product = self.factory.makeProduct(licenses=licenses)
+        column = ProductWithLicenses.composeLicensesColumn()
+        store = Store.of(product)
+        result = list(store.find((Product, column), Product.id == product.id))
+
+        self.assertEqual(1, len(result))
+        found_product, found_licenses = result[0]
+        self.assertEqual(product, found_product)
+        self.assertEqual(len(licenses), len(found_licenses))
+
+    def test_license_status_is_public(self):
+        # The license_status attribute can be read by anyone, on
+        # ProductWithLicenses as on Product.
+        product = self.factory.makeProduct(licenses=[License.BSD])
+        product_with_licenses = ProductWithLicenses(
+            product, [License.BSD.value])
+        login(ANONYMOUS)
+        self.assertEqual(
+            LicenseStatus.OPEN_SOURCE, product.license_status)
+        self.assertEqual(
+            LicenseStatus.OPEN_SOURCE, product_with_licenses.license_status)

=== modified file 'lib/lp/translations/browser/tests/test_translationgroup.py'
--- lib/lp/translations/browser/tests/test_translationgroup.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/browser/tests/test_translationgroup.py	2010-09-03 11:18:46 +0000
@@ -5,8 +5,7 @@
 
 __metaclass__ = type
 
-import unittest
-
+import transaction
 from zope.component import getUtility
 
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -40,6 +39,7 @@
         # translator_list composes dicts using _makeTranslatorDict.
         group = self._makeGroup()
         tr_translator = self.factory.makeTranslator('tr', group)
+        transaction.commit()
         view = self._makeView(group)
         translator_dict = view._makeTranslatorDict(
             tr_translator, tr_translator.language, tr_translator.translator)
@@ -62,7 +62,3 @@
         self.assertEqual(xhosa.datecreated, output['datecreated'])
         self.assertEqual(xhosa.style_guide_url, output['style_guide_url'])
         self.assertEqual(xhosa, output['context'])
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/browser/translationgroup.py'
--- lib/lp/translations/browser/translationgroup.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/browser/translationgroup.py	2010-09-03 11:18: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).
 
 """Browser code for translation groups."""
@@ -26,6 +26,7 @@
     LaunchpadFormView,
     LaunchpadView,
     )
+from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from lp.app.errors import NotFoundError
 from lp.registry.browser.objectreassignment import ObjectReassignmentView
@@ -67,6 +68,7 @@
         self.context = context
         self.request = request
         self.translation_groups = getUtility(ITranslationGroupSet)
+        self.user_can_edit = check_permission('launchpad.Edit', self.context)
 
     @property
     def label(self):
@@ -96,8 +98,7 @@
         """List of dicts describing the translation teams."""
         return [
             self._makeTranslatorDict(*data)
-            for data in self.context.fetchTranslatorData()
-            ]
+            for data in self.context.fetchTranslatorData()]
 
 
 class TranslationGroupAddTranslatorView(LaunchpadFormView):

=== modified file 'lib/lp/translations/doc/translationgroup.txt'
--- lib/lp/translations/doc/translationgroup.txt	2010-02-22 09:06:38 +0000
+++ lib/lp/translations/doc/translationgroup.txt	2010-09-03 11:18:46 +0000
@@ -1,4 +1,5 @@
-= Translation Groups =
+Translation Groups
+==================
 
     >>> from zope.component import getUtility
 
@@ -63,12 +64,12 @@
 
 No Privileges Person isn't allowed to translate into Welsh.
 
-   >>> series = evolution.getSeries('trunk')
-   >>> subset = potemplate_set.getSubset(productseries=series)
-   >>> potemplate = subset['evolution-2.2']
-   >>> pofile = potemplate.newPOFile('cy')
-   >>> pofile.canEditTranslations(no_priv)
-   False
+    >>> series = evolution.getSeries('trunk')
+    >>> subset = potemplate_set.getSubset(productseries=series)
+    >>> potemplate = subset['evolution-2.2']
+    >>> pofile = potemplate.newPOFile('cy')
+    >>> pofile.canEditTranslations(no_priv)
+    False
 
 Let's add him to the group.
 
@@ -176,6 +177,7 @@
     >>> de_translator = factory.makeTranslator('de', group, person=de_team)
     >>> nl_translator = factory.makeTranslator('nl', group, person=nl_team)
     >>> la_translator = factory.makeTranslator('la', group, person=la_team)
+    >>> transaction.commit()
 
 The method returns tuples of respectively a Translator ("translation
 group membership entry"), its language, and the actual team or person

=== 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 11:18: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():
+        """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 11:18: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,25 +16,32 @@
     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
 
 from canonical.database.constants import DEFAULT
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.sqlbase import SQLBase
-from canonical.launchpad.webapp.interfaces import (
-    DEFAULT_FLAVOR,
-    IStoreSelector,
-    MAIN_STORE,
+from canonical.launchpad.database.librarian import (
+    LibraryFileAlias,
+    LibraryFileContent,
     )
+from canonical.launchpad.interfaces.lpstorm import ISlaveStore
 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,
@@ -71,16 +78,15 @@
 
     def __getitem__(self, language_code):
         """See `ITranslationGroup`."""
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        query = store.find(
+        query = Store.of(self).find(
             Translator,
             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 +147,102 @@
             return 0
 
     def fetchTranslatorData(self):
-        """See ITranslationGroup."""
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        translator_data = store.find(
-            (Translator, Language, Person),
+        """See `ITranslationGroup`."""
+        # 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.of(self).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))
+
+    def fetchProjectsForDisplay(self):
+        """See `ITranslationGroup`."""
+        using = [
+            Product,
+            LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Product.iconID),
+            LeftJoin(
+                LibraryFileContent,
+                LibraryFileContent.id == LibraryFileAlias.contentID),
+            ]
+        columns = (
+            Product,
+            ProductWithLicenses.composeLicensesColumn(),
+            LibraryFileAlias,
+            LibraryFileContent,
+            )
+        product_data = ISlaveStore(Product).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`."""
+        using = [
+            ProjectGroup,
+            LeftJoin(
+                LibraryFileAlias, LibraryFileAlias.id == ProjectGroup.iconID),
+            LeftJoin(
+                LibraryFileContent,
+                LibraryFileContent.id == LibraryFileAlias.contentID),
+            ]
+        tables = (
+            ProjectGroup,
+            LibraryFileAlias,
+            LibraryFileContent,
+            )
+        project_data = ISlaveStore(ProjectGroup).using(*using).find(
+            tables,
+            ProjectGroup.translationgroupID == self.id,
+            ProjectGroup.active == True)
+
+        return prejoin(
+            project_data.order_by(ProjectGroup.displayname), slice(0, 1))
+
+    def fetchDistrosForDisplay(self):
+        """See `ITranslationGroup`."""
+        using = [
+            Distribution,
+            LeftJoin(
+                LibraryFileAlias, LibraryFileAlias.id == Distribution.iconID),
+            LeftJoin(
+                LibraryFileContent,
+                LibraryFileContent.id == LibraryFileAlias.contentID),
+            ]
+        tables = (
+            Distribution,
+            LibraryFileAlias,
+            LibraryFileContent,
+            )
+        distro_data = ISlaveStore(Distribution).using(*using).find(
+            tables, Distribution.translationgroupID == self.id)
+
+        return prejoin(
+            distro_data.order_by(Distribution.displayname), slice(0, 1))
 
 
 class TranslationGroupSet:
@@ -174,7 +268,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 +287,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 +297,3 @@
     def getGroupsCount(self):
         """See ITranslationGroupSet."""
         return TranslationGroup.select().count()
-

=== modified file 'lib/lp/translations/templates/translationgroup-index.pt'
--- lib/lp/translations/templates/translationgroup-index.pt	2010-02-22 09:06:38 +0000
+++ lib/lp/translations/templates/translationgroup-index.pt	2010-09-03 11:18:46 +0000
@@ -13,7 +13,7 @@
         <a tal:content="context/owner/fmt:displayname"
            tal:attributes="href context/owner/fmt:url"
            class="link" />
-        <a tal:condition="context/required:launchpad.Edit"
+        <a tal:condition="view/user_can_edit"
            href="+reassign" title="Change administrator"
            class="edit sprite" id="link-reassign"></a>
       </p>
@@ -32,7 +32,7 @@
           translations in this translation group.</em>
       </p>
 
-      <div tal:condition="context/required:launchpad.Edit">
+      <div tal:condition="view/user_can_edit">
         <a href="+edit" class="edit sprite">Change details</a>
       </div>
 
@@ -40,7 +40,8 @@
 
       <div id="translation-teams-listing">
       <h2><a name="teams"></a>Translation teams</h2>
-        <tal:translators condition="context/translators">
+      <tal:translator-list define="translator_list view/translator_list">
+        <tal:translators condition="translator_list">
           <table class="sortable listing" id="group-members">
             <thead>
               <tr>
@@ -48,11 +49,11 @@
                 <th>Team/Supervisor</th>
                 <th>Guidelines</th>
                 <th>Appointed</th>
-                <th tal:condition="context/required:launchpad.Edit"></th>
+                <th tal:condition="view/user_can_edit"></th>
               </tr>
             </thead>
             <tbody>
-              <tr tal:repeat="translator view/translator_list">
+              <tr tal:repeat="translator translator_list">
                 <td class="translator-language">
                   <a tal:attributes="href translator/language/fmt:url"
                      tal:content="translator/language/displayname">
@@ -82,7 +83,7 @@
                     none
                   </span>
                   <tal:notadmin
-                     condition="not:context/required:launchpad.Edit">
+                     condition="not:view/user_can_edit">
                     &nbsp;<a tal:condition="
                          translator/context/required:launchpad.Edit"
                          tal:attributes="href string:${translator/code}/+edit"
@@ -96,7 +97,7 @@
                     2007-09-17
                   </span>
                 </td>
-                <td tal:condition="context/required:launchpad.Edit">
+                <td tal:condition="view/user_can_edit">
                   <a tal:attributes="
                       href translator/code;
                       id string:edit-${translator/code}-translator"
@@ -110,22 +111,22 @@
             </tbody>
           </table>
         </tal:translators>
-        <tal:no-translators condition="not: context/translators">
+        <tal:no-translators condition="not: translator_list">
           No translation teams or supervisors have been appointed in
           this group yet.
         </tal:no-translators>
         <div style="margin-top:1em; margin-bottom: 2em;">
-          <a tal:condition="context/required:launchpad.Edit"
+          <a tal:condition="view/user_can_edit"
              href="+appoint" class="add sprite">Appoint a new translation
             team</a>
         </div>
-        </div><!-- id="translations-team-listing" -->
-
-        <div class="section">
-          <a name="projects"></a>
-          <div tal:replace="structure context/@@+portlet-projects" />
-        </div>
-
+      </tal:translator-list>
+      </div><!-- id="translations-team-listing" -->
+
+      <div class="section">
+        <a name="projects"></a>
+        <div tal:replace="structure context/@@+portlet-projects" />
+      </div>
     </div><!-- main -->
 
   </body>

=== modified file 'lib/lp/translations/templates/translationgroup-portlet-projects.pt'
--- lib/lp/translations/templates/translationgroup-portlet-projects.pt	2010-04-19 08:11:52 +0000
+++ lib/lp/translations/templates/translationgroup-portlet-projects.pt	2010-09-03 11:18:46 +0000
@@ -13,33 +13,34 @@
     <a href="#teams">translation teams</a>.
   </p>
 
-  <div id="related-projects">
-    <div tal:condition="context/distributions" style="margin-top:1em;">
+  <div id="related-projects"
+       tal:define="
+        distributions context/fetchDistrosForDisplay;
+        projectgroups context/fetchProjectGroupsForDisplay;
+        projects context/fetchProjectsForDisplay">
+    <div tal:condition="distributions" style="margin-top:1em;">
       <h3 style="display: inline;">Distributions:</h3>
-      <tal:distribution
-         repeat="distribution context/distributions">
+      <tal:distribution repeat="distribution distributions">
         <a href="#" tal:replace="structure distribution/fmt:link">Ubuntu
         </a><tal:comma condition="not:repeat/distribution/end">, </tal:comma>
       </tal:distribution>
     </div>
 
-    <div tal:condition="context/projects" style="margin-top:1em;">
+    <div tal:condition="projectgroups" style="margin-top:1em;">
       <h3 style="display: inline;">Project groups:</h3>
-      <tal:project
-         repeat="projectgroup context/projects">
+      <tal:projectgroup repeat="projectgroup projectgroups">
         <a href="#" tal:replace="structure projectgroup/fmt:link">GNOME
         </a><tal:comma condition="not:repeat/projectgroup/end">, </tal:comma>
+      </tal:projectgroup>
+    </div>
+
+    <div tal:condition="projects" style="margin-top:1em;">
+      <h3 style="display: inline;">Projects:</h3>
+      <tal:project repeat="project projects">
+        <a href="#" tal:replace="structure project/fmt:link">Firefox
+        </a><tal:comma condition="not:repeat/project/end">, </tal:comma>
       </tal:project>
     </div>
 
-    <div tal:condition="context/products" style="margin-top:1em;">
-      <h3 style="display: inline;">Projects:</h3>
-      <tal:product
-       repeat="product context/products">
-        <a href="#" tal:replace="structure product/fmt:link">Firefox
-        </a><tal:comma condition="not:repeat/product/end">, </tal:comma>
-      </tal:product>
-    </div>
-
   </div>
 </tal:root>