Hi Henning,

This looks good. I just have a few comments below.

merge-conditional

-Edwin

>=== modified file 'lib/lp/translations/model/potemplate.py'
>--- lib/lp/translations/model/potemplate.py	2010-01-12 21:29:03 +0000
>+++ lib/lp/translations/model/potemplate.py	2010-06-16 19:25:05 +0000
>@@ -184,6 +184,21 @@
> 
>     _uses_english_msgids = None
> 
>+    @cachedproperty
>+    def _sharing_ids(self):
>+        """Return the IDs of all sharing templates including this one."""
>+        subset = getUtility(IPOTemplateSet).getSharingSubset(
>+            product=self.product,
>+            distribution=self.distribution,
>+            sourcepackagename=self.sourcepackagename)
>+        # Convert  to a list for caching.
>+        result = list(subset.getSharingPOTemplateIDs(self.name))
>+        if len(result) == 0:
>+            # Always return at least the template itself.
>+            return [self.id]
>+        else:
>+            return result


Should you also make sure that self.id is in result even if len != 0?


>     def __storm_invalidated__(self):
>         self.clearPOFileCache()
>         self._uses_english_msgids = None
>@@ -1387,45 +1425,102 @@
>             package = template.sourcepackagename.name
>         return (template.name, package)
> 
>-    def _iterate_potemplates(self, name_pattern=None):
>-        """Yield all templates matching the provided argument.
>-
>-        This is much like a `IPOTemplateSubset`, except it operates on
>-        `Product`s and `Distribution`s rather than `ProductSeries` and
>-        `DistroSeries`.
>-        """
>-        if self.product:
>-            subsets = [
>-                self.potemplateset.getSubset(productseries=series)
>-                for series in self.product.series
>-                ]
>+    def _queryByProduct(self, templatename_clause):
>+        """Build the query that finds POTemplates by their linked product.
>+
>+        Queries the Packaging table to find templates in linked source
>+        packages, too.
>+
>+        :param templatename_clause: A string or a storm expression to
>+        add to the where clause of the query that will select the template
>+        name.
>+        :return: A ResultSet for the query.
>+        """
>+        from lp.registry.model.productseries import ProductSeries
>+
>+        ProductSeries1 = ClassAlias(ProductSeries)
>+        origin = LeftJoin(
>+            LeftJoin(
>+                POTemplate, ProductSeries,
>+                POTemplate.productseriesID == ProductSeries.id), 
>+            Join(
>+                Packaging, ProductSeries1,
>+                Packaging.productseriesID == ProductSeries1.id),
>+            And(
>+                Packaging.distroseriesID == POTemplate.distroseriesID,
>+                Packaging.sourcepackagenameID == (
>+                        POTemplate.sourcepackagenameID)
>+                )
>+            )
>+        return Store.of(self.product).using(origin).find(
>+            POTemplate,
>+            And(
>+                Or(
>+                    ProductSeries.productID == self.product.id,
>+                    ProductSeries1.productID == self.product.id
>+                    ),
>+                templatename_clause
>+                ))
>+
>+    def _queryBySourcepackagename(self, templatename_clause):
>+        """Build the query that finds POTemplates by their names.
>+
>+        :param templatename_clause: A string or a storm expression to
>+        add to the where clause of the query that will select the template
>+        name.
>+        :return: A ResultSet for the query.
>+        """
>+        from lp.registry.model.distroseries import DistroSeries
>+        origin = Join(
>+            POTemplate, DistroSeries,
>+            POTemplate.distroseries == DistroSeries.id)
>+        return Store.of(self.distribution).using(origin).find(
>+            POTemplate,
>+            And(
>+                DistroSeries.distributionID == self.distribution.id,
>+                POTemplate.sourcepackagename == self.sourcepackagename,
>+                templatename_clause
>+                ))
>+
>+    def _queryByDistribution(self, templatename_clause):
>+        """Special case when templates are searched across a distribution."""
>+        return Store.of(self.distribution).find(
>+            POTemplate, templatename_clause)
>+
>+    def _queryPOTemplates(self, templatename_clause):
>+        """Select the right query to be used."""
>+        if self.product is not None:
>+            return self._queryByProduct(templatename_clause)
>+        elif self.share_by_name:
>+            return self._queryBySourcepackagename(templatename_clause)
>+        elif self.distribution is not None and self.sourcepackagename is None:
>+            return self._queryByDistribution(templatename_clause)
>         else:
>-            subsets = [
>-                self.potemplateset.getSubset(
>-                    distroseries=series,
>-                    sourcepackagename=self.sourcepackagename)
>-                for series in self.distribution.series
>-                ]
>-        for subset in subsets:
>-            for template in subset:
>-                if name_pattern is None or re.match(name_pattern,
>-                                                    template.name):
>-                    yield template
>+            return EmptyResultSet()
> 
>     def getSharingPOTemplates(self, potemplate_name):
>         """See IPOTemplateSharingSubset."""
>         if self.distribution is not None:
>             assert self.sourcepackagename is not None, (
>                    "Need sourcepackagename to select from distribution.")
>+        return self._queryPOTemplates(
>+            POTemplate.name == potemplate_name)
> 
>-        escaped_potemplate_name = re.escape(potemplate_name)
>-        return self._iterate_potemplates("^%s$" % escaped_potemplate_name)
>+    def getSharingPOTemplateIDs(self, potemplate_name):
>+        """See IPOTemplateSharingSubset."""
>+        return self.getSharingPOTemplates(potemplate_name).values(
>+            POTemplate.id)
> 
>     def groupEquivalentPOTemplates(self, name_pattern=None): """See IPOTemplateSharingSubset.""" equivalents = {}
> 
>-        for template in self._iterate_potemplates(name_pattern):
>+        if name_pattern is None:
>+            templatename_clause = "1=1"
>+        else:
>+            templatename_clause = "potemplate.name ~ '%s'" % name_pattern


I think you should either escape the name_pattern with sqlvalues,
or you should convert it to a storm conditional.


>+
>+        for template in self._queryPOTemplates(templatename_clause):
>             key = self._get_potemplate_equivalence_class(template)
>             if key not in equivalents:
>                 equivalents[key] = []
>=== modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
>--- lib/lp/translations/tests/test_shared_potemplate.py	2010-05-18 14:00:10 +0000
>+++ lib/lp/translations/tests/test_shared_potemplate.py	2010-06-16 19:25:05 +0000
>@@ -177,5 +176,398 @@
>         self.assertEquals(potmsgset, shared_potmsgset)
> 
> 
>+class TestMessageSharingProductPackage(TestCaseWithFactory):
>+    """Test message sharing between a product and a package.
>+
>+    Each test uses assertStatementCount to make sure the number of SQL
>+    queries does not change. This was integrated here to avoid having
>+    a second test case just for statement counts.
>+    The current implementation is good and only needs one statement."""


Ending triple-quotes belongs by itself on the following line, since
this is a multiline docstring.


>+    layer = ZopelessDatabaseLayer
>+
>+    def setUp(self):
>+        super(TestMessageSharingProductPackage, self).setUp()
>+
>+        self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
>+        self.hoary = self.ubuntu['hoary']
>+        self.warty = self.ubuntu['warty']
>+        self.ubuntu.translation_focus = self.hoary
>+        self.packagename = self.factory.makeSourcePackageName()
>+
>+        self.product = self.factory.makeProduct()
>+        self.trunk = self.product.getSeries('trunk')
>+        self.stable = self.factory.makeProductSeries(
>+            product=self.product)
>+
>+        self.templatename = self.factory.getUniqueString()
>+        self.trunk_template = self.factory.makePOTemplate(
>+            productseries=self.trunk, name=self.templatename)
>+        self.hoary_template = self.factory.makePOTemplate(
>+            distroseries=self.hoary, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+
>+        self.owner = self.factory.makePerson()
>+        self.potemplateset = getUtility(IPOTemplateSet)
>+
>+    def _assertStatements(self, no_of_statements, resultset):
>+        """Assert constant number of SQL statements when iterating result set.
>+
>+        This encapsulates using the 'list' function to feed the iterator to
>+        the assert method. This iterates the resultset, triggering SQL
>+        statement execution."""
>+        return self.assertStatementCount(no_of_statements, list, resultset)
>+
>+    def test_getSharingPOTemplates_product(self):
>+        # Sharing templates for a product include the same templates from
>+        # a linked source package.
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template], templates)
>+
>+    def test_getSharingPOTemplates_package(self):
>+        # Sharing templates for a source package include the same templates 
>+        # from a linked product.
>+        sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        sourcepackage.setPackaging(self.trunk, self.owner)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template], templates)
>+
>+    def test_getSharingPOTemplates_product_multiple_series(self):
>+        # Sharing templates for a product include the same templates from
>+        # a linked source package, even from multiple product series.
>+        # But templates for the same sourcepackagename are not returned
>+        # if they are not linked.
>+        stable_template = self.factory.makePOTemplate(
>+            productseries=self.stable, name=self.templatename)
>+        # This will not be returned.
>+        self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template, stable_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_package_multiple_series(self):
>+        # Sharing templates for a source package include the same templates 
>+        # from a linked product, even with multiple product series.
>+        # To include templates from other source packages, the product must
>+        # be linked to that one, too.
>+        stable_template = self.factory.makePOTemplate(
>+            productseries=self.stable, name=self.templatename)
>+        warty_template = self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        hoary_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        hoary_sourcepackage.setPackaging(self.trunk, self.owner)
>+        warty_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.warty)
>+        warty_sourcepackage.setPackaging(self.stable, self.owner)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template,
>+             stable_template, warty_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_package_name_changed(self):
>+        # When the name of a package changes (but not the name of the
>+        # template), it will still share translations if it is linked
>+        # to the same product.
>+        changed_name = self.factory.makeSourcePackageName()
>+        warty_template = self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=changed_name,
>+            name=self.templatename)
>+        hoary_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        hoary_sourcepackage.setPackaging(self.trunk, self.owner)
>+        warty_sourcepackage = self.factory.makeSourcePackage(
>+            changed_name, self.warty)
>+        warty_sourcepackage.setPackaging(self.stable, self.owner)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template, warty_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_many_series(self):
>+        # The number of queries for a call to getSharingPOTemplates must
>+        # remain constant.
>+
>+        all_templates = [self.trunk_template, self.hoary_template]
>+        hoary_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        hoary_sourcepackage.setPackaging(self.trunk, self.owner)
>+        # Add a greater number of series and sharing templates on either side.
>+        seriesnames = (
>+            ('0.1', 'feisty'),
>+            ('0.2', 'gutsy'),
>+            ('0.3', 'hardy'),
>+            ('0.4', 'intrepid'),
>+            ('0.5', 'jaunty'),
>+            ('0.6', 'karmic'),
>+            )
>+        for pseries_name, dseries_name in seriesnames:
>+            productseries = self.factory.makeProductSeries(
>+                self.product, pseries_name)
>+            all_templates.append(self.factory.makePOTemplate(
>+                productseries=productseries, name=self.templatename))
>+            distroseries = self.factory.makeDistroSeries(
>+                self.ubuntu, name=dseries_name)
>+            all_templates.append(self.factory.makePOTemplate(
>+                distroseries=distroseries, sourcepackagename=self.packagename,
>+                name=self.templatename))
>+            sourcepackage = self.factory.makeSourcePackage(
>+                self.packagename, distroseries)
>+            sourcepackage.setPackaging(productseries, self.owner)
>+        # Don't forget warty and stable.
>+        all_templates.append(self.factory.makePOTemplate(
>+            productseries=self.stable, name=self.templatename))
>+        all_templates.append(self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.templatename))
>+        warty_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.warty)
>+        warty_sourcepackage.setPackaging(self.stable, self.owner)
>+
>+        # Looking from the product side.
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+        self.assertContentEqual(all_templates, templates)
>+
>+        # Looking from the sourcepackage side.
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+        self.assertContentEqual(all_templates, templates)
>+
>+    def test_getSharingPOTemplates_product_unrelated_templates(self):
>+        # Sharing templates for a product must not include other templates
>+        # from a linked source package.
>+        self.factory.makePOTemplate(
>+            distroseries=self.hoary, sourcepackagename=self.packagename,
>+            name=self.factory.getUniqueString())
>+        self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.factory.getUniqueString())
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_product_different_names_and_series(self):
>+        # A product may be packaged into differently named packages in
>+        # different distroseries.
>+        warty_packagename = self.factory.makeSourcePackageName()
>+        warty_template = self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=warty_packagename,
>+            name=self.templatename)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=warty_packagename,
>+            distroseries=self.warty)
>+        self.trunk.setPackaging(self.warty, warty_packagename, self.owner)
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template, warty_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_product_different_names_same_series(self):
>+        # A product may be packaged into differently named packages even in
>+        # the same distroseries. Must use different product series, though.
>+        other_packagename = self.factory.makeSourcePackageName()
>+        other_template = self.factory.makePOTemplate(
>+            distroseries=self.hoary, sourcepackagename=other_packagename,
>+            name=self.templatename)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=other_packagename,
>+            distroseries=self.hoary)
>+        self.stable.setPackaging(self.hoary, other_packagename, self.owner)
>+        subset = self.potemplateset.getSharingSubset(product=self.product)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template, other_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_package_unrelated_template(self):
>+        # Sharing templates for a source package must not include other
>+        # templates from a linked product.
>+        self.factory.makePOTemplate(
>+            productseries=self.trunk, name=self.factory.getUniqueString())
>+        self.factory.makePOTemplate(
>+            productseries=self.stable, name=self.factory.getUniqueString())
>+        sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        sourcepackage.setPackaging(self.trunk, self.owner)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template],
>+            templates)
>+
>+    def test_getSharingPOTemplates_package_unrelated_template_linked(self):
>+        # Sharing templates for a source package must not include templates
>+        # from sourcepackages of the same name that are linked to a different
>+        # product.
>+        other_productseries = self.factory.makeProductSeries()
>+        other_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.warty)
>+        other_sourcepackage.setPackaging(other_productseries, self.owner)
>+        other_template = self.factory.makePOTemplate(
>+            productseries=other_productseries, name=self.templatename)
>+
>+        sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, self.hoary)
>+        sourcepackage.setPackaging(self.trunk, self.owner)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.trunk_template, self.hoary_template], templates)
>+
>+        # The behavior is controlled by the translation focus of the 
>+        # distribution. The series in focus will be selected.
>+        self.ubuntu.translation_focus = self.warty
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual([other_template], templates)
>+
>+    def test_getSharingPOTemplates_package_only(self):
>+        # Sharing templates for a source package only, is done by the 
>+        # sourcepackagename.
>+        warty_template = self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        other_series = self.factory.makeDistroSeries(self.ubuntu)
>+        other_template = self.factory.makePOTemplate(
>+            distroseries=other_series, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            1, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertContentEqual(
>+            [self.hoary_template, other_template, warty_template], templates)
>+
>+    def test_getSharingPOTemplates_package_one_linked(self):
>+        # Once one a sourcepackage in a distroseries that is neither the
>+        # translation focus nor the current series is linked to a product,
>+        # no sharing by name is possible anymore.
>+        self.factory.makePOTemplate(
>+            distroseries=self.warty, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        other_series = self.factory.makeDistroSeries(self.ubuntu)
>+        self.factory.makePOTemplate(
>+            distroseries=other_series, sourcepackagename=self.packagename,
>+            name=self.templatename)
>+        other_sourcepackage = self.factory.makeSourcePackage(
>+            self.packagename, other_series)
>+        other_sourcepackage.setPackaging(self.trunk, self.owner)
>+
>+        subset = self.potemplateset.getSharingSubset(
>+            distribution=self.ubuntu,
>+            sourcepackagename=self.packagename)
>+        templates = self._assertStatements(
>+            0, subset.getSharingPOTemplates(self.templatename))
>+
>+        self.assertEqual([], templates)
>+
>+    def test_getOrCreateSharedPOTMsgSet_product(self):
>+        # Trying to create an identical POTMsgSet in a product as exists
>+        # in a linked sourcepackage will return the existing POTMsgset.
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        hoary_potmsgset = self.factory.makePOTMsgSet(
>+            potemplate=self.hoary_template, sequence=1)
>+
>+        trunk_potmsgset = self.trunk_template.getOrCreateSharedPOTMsgSet(
>+                singular_text=hoary_potmsgset.singular_text,
>+                plural_text=hoary_potmsgset.plural_text)
>+        self.assertEqual(hoary_potmsgset, trunk_potmsgset)
>+
>+    def test_getOrCreateSharedPOTMsgSet_package(self):
>+        # Trying to create an identical POTMsgSet in a product as exists
>+        # in a linked sourcepackage will return the existing POTMsgset.
>+        self.factory.makeSourcePackagePublishingHistory(
>+            sourcepackagename=self.packagename,
>+            distroseries=self.hoary)
>+        self.trunk.setPackaging(self.hoary, self.packagename, self.owner)
>+        hoary_potmsgset = self.factory.makePOTMsgSet(
>+            potemplate=self.hoary_template, sequence=1)
>+
>+        trunk_potmsgset = self.trunk_template.getOrCreateSharedPOTMsgSet(
>+                singular_text=hoary_potmsgset.singular_text,
>+                plural_text=hoary_potmsgset.plural_text)
>+        self.assertEqual(hoary_potmsgset, trunk_potmsgset)
>+
>+
> def test_suite():
>     return unittest.TestLoader().loadTestsFromName(__name__)
>