Merge lp:~henninge/launchpad/bug-545354-enable-sharing into lp:~launchpad/launchpad/recife

[Henning Eggers]

Enable message sharing between products and source packages.

= Bug 545354 =

Enlarges the collection of sharing templates in a POTemplateSharingSubset to include templates from linked products and source packages. Actually, in some cases it does not only "enlarge" but also "shift". The old implementation had all source packages of the same name (sourcepackagename) within a distribution share templates, this new implementation makes that connection through linkage to an upstream product. Consequently, all source packages that are linked to any series of the same product will share templates - independently of the name of the source package. Usually though, the sourcepackagename will be identical. By this we will loose packages in older Ubuntu series because the Packaging table (that holds the upstream link information) used to be insufficiently populated so that not each series was properly linked. Since Maverick this has been fixed so that future series will work just fine with this.

== Pre-implementation discussion ==

We discussed some of this in Recife in March but also just recently. The latest findings can be found in my comments on the bug. One design goal was to make sure the number of SQL queries remains constant and independent of the number of templates in the collection.

== Implementation details ==

The query in _queryByProduct is best understood by looking at this little picture: http://paste.ubuntu.com/447067/ Since a POTemplate can be linked to *either* a ProductSeries *or* a SourcePackage (distroseries, sourcepackagename), these need to be left-joined.
Also in this method I used ID throughout although not all relations need to be that explicit but I found mixing those more confusing to read.

== Tests ==

A new test case was created to test getSharingPOTemplates and getOrCreateSharedPOTMsgSet. The latter is only tested briefly because it uses the first and there is no need to re-create all test conditions. The tests also make sure the number of sql statements remains constant (at 1).

./test -vvt MessageSharingProductPackage

== Lint ==

No lint left.

[Henning Eggers (henninge)]

Seeing how many tests were affected, I'd suggest the following test command which tests all translation tests:

bin/test -vvt lp.translations

[Edwin Grubbs (edwin-grubbs)]

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
>+ ),
>+ ...

review: Approve

[Henning Eggers (henninge)]

Am 16.06.2010 23:24, schrieb Edwin Grubbs:
> This looks good. I just have a few comments below.

Thanks for the review. I explained one away and fixed two. ;)

>> === 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?

No. Actually, the len == 0 case is basically a "no sharing possible case" and
with a single id in the list no sharing happens. If we get any ids back, those
are the sharing ones and we can trust them to include all we need.

[...]

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

Yes, thank you. I fixed that by using sqlvalues. Also I replaced "1=1" with
True since we are still in storm land here. I also pulled this out into
another variant of getSharingPOTemplates and added tests for it.

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

True, thanks.

...

[Jeroen T. Vermeulen (jtv)]

I looked over the incremental diff at Henning's request. A few notes, mostly already discussed on IRC. I think having the separate unit test for the regex matching (which was a bit of a wildcard, ha ha) worked out really well.

These came up on IRC:
 * Backquote identifiers in docstrings: "See `IPOTemplateSharingSubset`."""
 * getSharingPOTemplatesRegex says you're getting a regex, not templates; add "By."
 * No need to create a Product and ProductSeries; create a series and use its product.

The unit tests mostly follow the pattern "create templates with names [...]; create a subset; select from subset by regex; verify output." Most of these would be one-liners if you had a method for "create templates with names [...] (parameter 1); create a subset; select from subset by regex (parameter 2); return list of names of the selected templates." The caller could then assertContentsEqual the result against a list of expected strings. That would also help with:
 * Making the output more obvious (the relevance of "templates[1:]" is less clear).
 * Testing lots of other things that could go wrong; see below.
 * Eliminating implicit state. You wouldn't need any setUp at all.

Other notes:
 * You may want to test that you're really getting a match, not a search: "xyz" should not match "y.*" As long as we're not taking the regex from user input but from existing template names, this is a bigger risk than unescaped quotes. (Not that I disagree with your approach of testing for those right from the start!)
 * "# Quotes in the pattern can be dangerous." To me that looks like you're specifying the appropriate behaviour! How about "quotes do not confuse the regex match"?
 * I'd test single and double quotes separately, not in the same string.
 * When you test for quoting robustness, you might as well set up a template to establish that you don't get any unexpected matches.
 * You don't exercise the case where no templates match.

Apart from those notes, the incremental diff has my unreserved blessing.

Jeroen

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-05-20 13:59:11 +0000
+++ database/schema/security.cfg	2010-06-22 07:46:30 +0000
@@ -1341,6 +1341,7 @@
 public.distribution                     = SELECT
 public.distroseries                     = SELECT
 public.language                         = SELECT
+public.packaging                        = SELECT
 public.person                           = SELECT
 public.pofile                           = SELECT, INSERT, UPDATE
 public.pomsgid                          = SELECT

=== modified file 'lib/lp/translations/doc/potemplate.txt'
--- lib/lp/translations/doc/potemplate.txt	2010-04-26 16:00:31 +0000
+++ lib/lp/translations/doc/potemplate.txt	2010-06-22 07:46:30 +0000
@@ -228,7 +228,7 @@
 
 == getPOFileByPath ==
 
-We can get an IPOFile inside a templage based on its path.
+We can get an IPOFile inside a template based on its path.
 
     >>> pofile = potemplate.getPOFileByPath('es.po')
     >>> print pofile.title
@@ -614,7 +614,10 @@
     >>> print exported_translation_file.content_type
     application/x-gtar
 
-Inspecting the tarball content, we have the list of entries exported.
+Inspecting the tarball content, we have the list of entries exported. This
+includes the 'pt' POFile that was created earlier on the 'evolution'
+product as this is sharing translations with the source package that this
+potemplate is from.
 
     >>> from canonical.launchpad.helpers import string_to_tarfile
     >>> tarfile_string = exported_translation_file.read()
@@ -625,7 +628,7 @@
     >>> sorted(name.rstrip('/') for name in tarfile.getnames())
     ['evolution-2.2', 'evolution-2.2/evolution-2.2-es.po',
      'evolution-2.2/evolution-2.2-ja.po', 'evolution-2.2/evolution-2.2-xh.po',
-     'po', 'po/evolution-2.2.pot']
+     'po', 'po/evolution-2.2-pt.po', 'po/evolution-2.2.pot']
 
 The *-es.po file is indeed the Spanish translation...
 
@@ -644,6 +647,7 @@
     evolution-2.2/evolution-2.2-ja.po
     evolution-2.2/evolution-2.2-xh.po
     po/
+    po/evolution-2.2-pt.po
     po/evolution-2.2.pot
     >>> pofile = simple_popen2(
     ...     ['tar', 'zxfO', '-', 'evolution-2.2/evolution-2.2-es.po'],

=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py	2009-11-27 12:50:16 +0000
+++ lib/lp/translations/interfaces/potemplate.py	2010-06-22 07:46:30 +0000
@@ -695,6 +695,29 @@
         :return: A list of all potemplates of the same name from all series.
         """
 
+    def getSharingPOTemplatesByRegex(name_pattern=None):
+        """Find all sharing templates with names matching the given pattern.
+
+        If name_pattern is None, match is performed on the template name.
+        Use with care as it may return all templates in a distribution!
+
+        :param name_pattern: A POSIX regular expression that the template
+           is matched against.
+        :return: A list of all potemplates matching the pattern.
+        """
+
+    def getSharingPOTemplateIDs(potemplate_name):
+        """Find database ids of all sharing templates of the given name.
+
+        For distributions this method requires that sourcepackagename is set.
+        This avoids serialization of full POTemplate objects.
+
+        :param potemplate_name: The name of the template for which to find
+            sharing equivalents.
+        :return: A list of database ids of all potemplates of the same name
+            from all series.
+        """
+
     def groupEquivalentPOTemplates(name_pattern=None):
         """Within given IProduct or IDistribution, find equivalent templates.
 

=== 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-22 07:46:30 +0000
@@ -18,7 +18,6 @@
 import datetime
 import logging
 import os
-import re
 
 from psycopg2.extensions import TransactionRollbackError
 from sqlobject import (
@@ -26,7 +25,7 @@
     StringCol)
 from storm.expr import Alias, And, Desc, In, Join, LeftJoin, Or, SQL
 from storm.info import ClassAlias
-from storm.store import Store
+from storm.store import EmptyResultSet, Store
 from zope.component import getAdapter, getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
@@ -43,6 +42,7 @@
 from lp.services.database.prejoin import prejoin
 from lp.services.worlddata.model.language import Language
 from lp.registry.interfaces.person import validate_public_person
+from lp.registry.model.packaging import Packaging
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.translations.model.pofile import POFile, DummyPOFile
 from lp.translations.model.pomsgid import POMsgID
@@ -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
+
     def __storm_invalidated__(self):
         self.clearPOFileCache()
         self._uses_english_msgids = None
@@ -339,8 +354,7 @@
         if self.productseries is not None:
             return self.productseries
         elif self.distroseries is not None:
-            from lp.registry.model.sourcepackage import \
-                SourcePackage
+            from lp.registry.model.sourcepackage import SourcePackage
             return SourcePackage(distroseries=self.distroseries,
                 sourcepackagename=self.sourcepackagename)
         raise AssertionError('Unknown POTemplate translation target')
@@ -560,31 +574,10 @@
             ]
         clause_tables = ['TranslationTemplateItem']
         if sharing_templates:
-            clauses.extend([
-                'TranslationTemplateItem.potemplate = POTemplate.id',
-                'TranslationTemplateItem.potmsgset = POTMsgSet.id',
-                'POTemplate.name = %s' % sqlvalues(self.name),
-                ])
-            product = None
-            distribution = None
-            clause_tables.append('POTemplate')
-            if self.productseries is not None:
-                product = self.productseries.product
-                clauses.extend([
-                    'POTemplate.productseries=ProductSeries.id',
-                    'ProductSeries.product %s' % self._null_quote(product)
-                    ])
-                clause_tables.append('ProductSeries')
-            elif self.distroseries is not None:
-                distribution = self.distroseries.distribution
-                clauses.extend([
-                    'POTemplate.distroseries=DistroSeries.id',
-                    'DistroSeries.distribution %s' % (
-                        self._null_quote(distribution)),
-                    'POTemplate.sourcepackagename %s' % self._null_quote(
-                        self.sourcepackagename),
-                    ])
-                clause_tables.append('DistroSeries')
+            clauses.append(
+                'TranslationTemplateItem.potemplate in %s' % sqlvalues(
+                    self._sharing_ids)
+                )
         else:
             clauses.append(
                 'TranslationTemplateItem.potemplate = %s' % sqlvalues(self))
@@ -1374,9 +1367,54 @@
         assert distribution or not sourcepackagename, (
             "Picking a source package only makes sense with a distribution.")
         self.potemplateset = potemplateset
-        self.distribution = distribution
-        self.sourcepackagename = sourcepackagename
-        self.product = product
+        self.share_by_name = False
+        self.subset = None
+
+        if distribution is not None:
+            self.distribution = distribution
+            self.sourcepackagename = sourcepackagename
+            if sourcepackagename is not None:
+                product = self._findUpstreamProduct(
+                    distribution, sourcepackagename)
+                if product is None:
+                    self.share_by_name = self._canShareByName(
+                        distribution, sourcepackagename)
+        if product is not None:
+            self.product = product
+
+    def _findUpstreamProduct(self, distribution, sourcepackagename):
+        """Find the upstream product by looking at the translation focus.
+
+        The translation focus is used to pick a distroseries, so a source
+        package instance can be created. If no translation focus is set,
+        the distribution's current series is used."""
+        
+        from lp.registry.model.sourcepackage import SourcePackage
+        distroseries = distribution.translation_focus
+        if distroseries is None:
+            distroseries = distribution.currentseries
+        sourcepackage = SourcePackage(sourcepackagename, distroseries)
+        if sourcepackage.productseries is None:
+            return None
+        return sourcepackage.productseries.product
+
+    def _canShareByName(self, distribution, sourcepackagename):
+        """Determine if sharing by sourcepackagename is a wise thing.
+
+        Without a product, the linkage between sharing packages can only be
+        determined by their name. This is only (fairly) safe if none of these
+        is packaged elsewhere.
+        """
+        from lp.registry.model.distroseries import DistroSeries
+        origin = Join(
+            Packaging, DistroSeries,
+            Packaging.distroseries == DistroSeries.id)
+        result = Store.of(distribution).using(origin).find(
+            Packaging,
+            And(DistroSeries.distribution == distribution.id,
+                Packaging.sourcepackagename == sourcepackagename.id)
+            ).count()
+        return result == 0
 
 
     def _get_potemplate_equivalence_class(self, template):
@@ -1387,45 +1425,107 @@
             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."""
+        """See `IPOTemplateSharingSubset`."""
         if self.distribution is not None:
             assert self.sourcepackagename is not None, (
                    "Need sourcepackagename to select from distribution.")
-
-        escaped_potemplate_name = re.escape(potemplate_name)
-        return self._iterate_potemplates("^%s$" % escaped_potemplate_name)
+        return self._queryPOTemplates(
+            POTemplate.name == potemplate_name)
+
+    def getSharingPOTemplatesByRegex(self, name_pattern=None):
+        """See `IPOTemplateSharingSubset`."""
+        if name_pattern is None:
+            templatename_clause = True
+        else:
+            templatename_clause = (
+                "potemplate.name ~ %s" % sqlvalues(name_pattern))
+
+        return self._queryPOTemplates(templatename_clause)
+
+    def getSharingPOTemplateIDs(self, potemplate_name):
+        """See `IPOTemplateSharingSubset`."""
+        return self.getSharingPOTemplates(potemplate_name).values(
+            POTemplate.id)
 
     def groupEquivalentPOTemplates(self, name_pattern=None):
-        """See IPOTemplateSharingSubset."""
+        """See `IPOTemplateSharingSubset`."""
         equivalents = {}
 
-        for template in self._iterate_potemplates(name_pattern):
+        for template in self.getSharingPOTemplatesByRegex(name_pattern):
             key = self._get_potemplate_equivalence_class(template)
             if key not in equivalents:
                 equivalents[key] = []
@@ -1510,10 +1610,6 @@
                     if flag
                     ])
 
-            # Store sequences so we can detect later whether we changed the
-            # message.
-            sequence = row.sequence
-
             # Store the message.
             messages.append(msgset)
 

=== 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-22 07:46:30 +0000
@@ -7,13 +7,16 @@
 
 import unittest
 
+from storm.exceptions import DataError
+from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from lp.testing.factory import LaunchpadObjectFactory
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing import ZopelessDatabaseLayer
-
-
-class TestTranslationSharingPOTemplate(unittest.TestCase):
+from lp.testing import TestCaseWithFactory
+from lp.translations.interfaces.potemplate import IPOTemplateSet
+
+class TestTranslationSharingPOTemplate(TestCaseWithFactory):
     """Test behaviour of "sharing" PO templates."""
 
     layer = ZopelessDatabaseLayer
@@ -22,20 +25,19 @@
         """Set up context to test in."""
         # Create a product with two series and sharing POTemplates
         # in different series ('devel' and 'stable').
-        factory = LaunchpadObjectFactory()
-        self.factory = factory
-        self.foo = factory.makeProduct()
-        self.foo_devel = factory.makeProductSeries(
+        super(TestTranslationSharingPOTemplate, self).setUp()
+        self.foo = self.factory.makeProduct()
+        self.foo_devel = self.factory.makeProductSeries(
             name='devel', product=self.foo)
-        self.foo_stable = factory.makeProductSeries(
+        self.foo_stable = self.factory.makeProductSeries(
             name='stable', product=self.foo)
         self.foo.official_rosetta = True
 
         # POTemplate is a 'sharing' one if it has the same name ('messages').
-        self.devel_potemplate = factory.makePOTemplate(
+        self.devel_potemplate = self.factory.makePOTemplate(
             productseries=self.foo_devel, name="messages")
-        self.stable_potemplate = factory.makePOTemplate(self.foo_stable,
-                                                        name="messages")
+        self.stable_potemplate = self.factory.makePOTemplate(
+            self.foo_stable, name="messages")
 
         # Create a single POTMsgSet that is used across all tests,
         # and add it to only one of the POTemplates.
@@ -106,8 +108,6 @@
         self.assertEquals(has_message_id, True)
 
     def test_hasPluralMessage(self):
-        naked_potemplate = removeSecurityProxy(self.devel_potemplate)
-
         # At the moment, a POTemplate has no plural form messages.
         self.assertEquals(self.devel_potemplate.hasPluralMessage(), False)
 
@@ -177,5 +177,465 @@
         self.assertEquals(potmsgset, shared_potmsgset)
 
 
+class TestSharingPOTemplatesByRegex(TestCaseWithFactory):
+    """Isolate tests for regular expression use in SharingSubset."""
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        super(TestSharingPOTemplatesByRegex, self).setUp()
+
+    def _makeAndFind(self, names, name_pattern=None):
+        product = self.factory.makeProduct()
+        product.official_rosetta = True
+        trunk = product.getSeries('trunk')
+        for name in names:
+            self.factory.makePOTemplate(productseries=trunk, name=name)
+        subset = getUtility(IPOTemplateSet).getSharingSubset(product=product)
+        return [
+            template.name
+            for template in subset.getSharingPOTemplatesByRegex(name_pattern)]
+
+    def test_getSharingPOTemplatesByRegex_baseline(self):
+        # Baseline test.
+        self.assertContentEqual(
+            ['foo', 'foo-bar', 'foo-two'],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two'], 'foo.*'))
+
+    def test_getSharingPOTemplatesByRegex_not_all(self):
+        # A template may not match.
+        self.assertContentEqual(
+            ['foo-bar', 'foo-two'],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two'], 'foo-.*'))
+
+    def test_getSharingPOTemplatesByRegex_all(self):
+        # Not passing a pattern returns all templates.
+        self.assertContentEqual(
+            ['foo', 'foo-bar', 'foo-two'],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two']))
+
+    def test_getSharingPOTemplatesByRegex_no_match(self):
+        # A not matching pattern returns no templates.
+        self.assertContentEqual(
+            [],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two'], "doo.+dle"))
+
+    def test_getSharingPOTemplatesByRegex_robustness_single_quotes(self):
+        # Single quotes do not confuse the regex match.
+        self.assertContentEqual(
+            [],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two'], "'"))
+
+    def test_getSharingPOTemplatesByRegex_robustness_double_quotes(self):
+        # Double quotes do not confuse the regex match.
+        self.assertContentEqual(
+            [],
+            self._makeAndFind(['foo', 'foo-bar', 'foo-two'], '"'))
+
+    def test_getSharingPOTemplatesByRegex_robustness_backslash(self):
+        # A backslash at the end could escape enclosing quotes without
+        # proper escaping, leading to a SyntaxError or even a successful
+        # exploit. Instead, storm should complain about an invalid expression
+        # by raising DataError.
+        product = self.factory.makeProduct()
+        subset = getUtility(IPOTemplateSet).getSharingSubset(product=product)
+        self.assertRaises(
+            DataError, list, subset.getSharingPOTemplatesByRegex("foo.*\\"))
+
+
+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.
+    """
+
+    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__)