← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~jtv/launchpad/bug-613823 into lp:launchpad

 

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-613823 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #613823 in Launchpad itself: "Handle translation upload approver conflicts"
  https://bugs.launchpad.net/launchpad/+bug/613823

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-613823/+merge/75773

= Summary =

The Translations Import Queue Gardener is reporting a lot of approval conflicts: uploads that can't be approved because there is more than one candidate translation template with the same domain, and so it doesn't know which one to import the translations to.

These conflicts have traditionally been logged, and echoed to the launchpad-error-reports list where the translations admins could do something about them.  But since then, project owners and such gained the ability to change templates' domains themselves.  And meanwhile, there is no more Translations Team.  So rather than spewing warnings onto an internal mailing list, we should report the problem to the user.


== Proposed fix ==

Store a description of the problem in TranslationImportQueueEntry.error_output.  This field is normally used for error or warning output from the import itself, but there's no reason why the approver shouldn't do it as well.  


== Pre-implementation notes ==

Henning and I went through a few possibilities: is there no risk of an approver error being overwritten by an importer error?  Only after manual approval, in which case the problem itself usually goes away.  (If it doesn't, the error comes back and unless it is fixed or worked around, there will be no import to overwrite it).  The situation is buried pretty deep in the code and multiple execution paths may lead to it.  What if approval does succeed after all?  We decided that successful auto-approval should clear the error field anyway.  You certainly don't want a successful re-import of a file to retain an error from a previous unsuccessful import anyway.


== Implementation details ==

We may want to make this problem more obvious to the user in the future.  I'm not sure a queue entry's error output is available without Ajax, for instance—though it's going to be an improvement over completely ignored log output.  So I carved out a separate method for the notification, which we can extend later with e.g. email if desired.

I stripped getPOTemplateByTranslationDomain from its knowledge of clashes, renamed it, and made it query all matching templates without judgment.  Since I had to add some unit tests for it anyway, I also converted its doctest section into unit tests.


== Tests ==

I deliberately did not change security.cfg before testing the new path under the proper database user, thinking that I was building a proper failing test.  But obviously, the gardener already has the necessary database privilege (UPDATE on TranslationImportQueueEntry). 

{{{
./bin/test -vvc lp.translations.tests.test_autoapproval
./bin/test -vvc lp.translations.tests.test_potemplate
./bin/test -vvc lp.translations.tests.test_translationimportqueue
}}}


== Demo and Q/A ==

Plenty of conflicts have accumulated, so just run the updated approver on staging and query the database for TranslationImportQueueEntry rows with a nonempty error_output and status = 5 (which means RosettaImportStatus.NEEDS_REVIEW).  You should see a few of these notices appear.


= Launchpad lint =

I left the lint in the doctest; I can reformat it after this review if desired, but didn't want to pollute the diff too much.


Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_potemplate.py
  lib/lp/translations/interfaces/potemplate.py
  lib/lp/translations/model/translationimportqueue.py
  lib/lp/translations/tests/test_autoapproval.py
  lib/lp/testing/factory.py
  lib/lp/translations/doc/potemplate.txt
  lib/lp/translations/tests/test_translationimportqueue.py
  lib/lp/translations/model/potemplate.py
  lib/lp/translations/browser/potemplate.py

./lib/lp/translations/doc/potemplate.txt
       1: narrative uses a moin header.
       9: narrative uses a moin header.
      23: narrative uses a moin header.
      49: narrative uses a moin header.
      74: narrative uses a moin header.
      89: narrative uses a moin header.
      99: narrative uses a moin header.
     111: narrative uses a moin header.
     152: narrative uses a moin header.
     168: narrative uses a moin header.
     177: narrative uses a moin header.
     189: narrative uses a moin header.
     215: narrative uses a moin header.
     258: narrative uses a moin header.
     336: want exceeds 78 characters.
     337: want exceeds 78 characters.
     389: want exceeds 78 characters.
     411: want exceeds 78 characters.
     412: want exceeds 78 characters.
     413: want exceeds 78 characters.
     414: want exceeds 78 characters.
     419: want exceeds 78 characters.
     420: want exceeds 78 characters.
     421: want exceeds 78 characters.
     426: want exceeds 78 characters.
     427: want exceeds 78 characters.
     428: want exceeds 78 characters.
     430: want exceeds 78 characters.
     438: want exceeds 78 characters.
     441: want exceeds 78 characters.
     446: want exceeds 78 characters.
     447: want exceeds 78 characters.
     448: want exceeds 78 characters.
     450: want exceeds 78 characters.
     452: want exceeds 78 characters.
     453: want exceeds 78 characters.
     455: want exceeds 78 characters.
     456: want exceeds 78 characters.
     457: want exceeds 78 characters.
     460: want exceeds 78 characters.
     464: want exceeds 78 characters.
     468: want exceeds 78 characters.
     470: want exceeds 78 characters.
     475: want exceeds 78 characters.
     478: want exceeds 78 characters.
     479: want exceeds 78 characters.
     482: want exceeds 78 characters.
     485: want exceeds 78 characters.
     493: want exceeds 78 characters.
     494: want exceeds 78 characters.
     496: want exceeds 78 characters.
     497: want exceeds 78 characters.
     498: want exceeds 78 characters.
     501: want exceeds 78 characters.
     502: want exceeds 78 characters.
     505: want exceeds 78 characters.
     509: want exceeds 78 characters.
     511: want exceeds 78 characters.
     513: want exceeds 78 characters.
     514: want exceeds 78 characters.
     519: want exceeds 78 characters.
     522: want exceeds 78 characters.
     524: want exceeds 78 characters.
     532: narrative uses a moin header.
     568: source exceeds 78 characters.
-- 
https://code.launchpad.net/~jtv/launchpad/bug-613823/+merge/75773
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-613823 into lp:launchpad.
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-09-13 22:43:21 +0000
+++ lib/lp/testing/factory.py	2011-09-16 16:39:26 +0000
@@ -2953,7 +2953,8 @@
     def makePOTemplate(self, productseries=None, distroseries=None,
                        sourcepackagename=None, owner=None, name=None,
                        translation_domain=None, path=None,
-                       copy_pofiles=True, side=None, sourcepackage=None):
+                       copy_pofiles=True, side=None, sourcepackage=None,
+                       iscurrent=True):
         """Make a new translation template."""
         if sourcepackage is not None:
             assert distroseries is None, (
@@ -2994,7 +2995,10 @@
         if path is None:
             path = 'messages.pot'
 
-        return subset.new(name, translation_domain, path, owner, copy_pofiles)
+        pot = subset.new(name, translation_domain, path, owner, copy_pofiles)
+        if not iscurrent:
+            removeSecurityProxy(pot).iscurrent = iscurrent
+        return pot
 
     def makePOTemplateAndPOFiles(self, language_codes, **kwargs):
         """Create a POTemplate and associated POFiles.

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2011-09-13 05:23:16 +0000
+++ lib/lp/translations/browser/potemplate.py	2011-09-16 16:39:26 +0000
@@ -686,9 +686,8 @@
 
     def validateDomain(self, domain, similar_templates,
                        sourcepackage_changed, productseries_changed):
-        other_template = similar_templates.getPOTemplateByTranslationDomain(
-            domain)
-        if other_template is not None:
+        clashes = similar_templates.getPOTemplatesByTranslationDomain(domain)
+        if not clashes.is_empty():
             if sourcepackage_changed:
                 self.setFieldError(
                     'sourcepackagename',

=== modified file 'lib/lp/translations/doc/potemplate.txt'
--- lib/lp/translations/doc/potemplate.txt	2011-03-01 17:40:06 +0000
+++ lib/lp/translations/doc/potemplate.txt	2011-09-16 16:39:26 +0000
@@ -107,67 +107,6 @@
     >>> templates[0].date_last_updated >= templates[1].date_last_updated
     True
 
-== getPOTemplateByTranslationDomain ==
-
-This method gives us the IPOTemplate that belongs to this subset and its
-translation domain is the given one.
-
-=== Filtering for iscurrent ===
-
-IPOTemplate specifies an "iscurrent" flag that indicates if this template
-should be available for translations or not.
-
-    >>> fooproduct=factory.makeProduct(name="fooproduct")
-    >>> productseries = factory.makeProductSeries(
-    ...     product=fooproduct, name="fooseries")
-    >>> current_template = factory.makePOTemplate(
-    ...     productseries=productseries, name="current-template",
-    ...     translation_domain="foodomain")
-    >>> disabled_template = factory.makePOTemplate(
-    ...     productseries=productseries, name="disabled-template",
-    ...     translation_domain="foodomain")
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> removeSecurityProxy(disabled_template).iscurrent = False
-
-The subset can be configured to filter templates on the iscurrent flag. This
-filter is effective for all get* methods but it will only be demonstrated
-here. The filter is applied during creation by specifying the filter value
-as an extra parameter. The default value is "None" which disables the filter
-and both current and disabled templates are returned.
-
-For getPOTemplateByTranslationDomain, this case is an error.  The
-approver needs a unique domain to identify the right template to upload
-to.
-
-    >>> potemplatesubset = potemplate_set.getSubset(
-    ...     productseries=productseries)
-    >>> potemplate = potemplatesubset.getPOTemplateByTranslationDomain(
-    ...     'foodomain')
-    WARNING:lp.translations.model.potemplate:Found 2 competing templates
-    with translation domain 'foodomain':
-    "current-template in Fooproduct fooseries";
-    "disabled-template in Fooproduct fooseries".
-
-    >>> print potemplate
-    None
-
-Setting the filter to "True" will only return current (enabled) templates.
-
-    >>> potemplatesubset = potemplate_set.getSubset(
-    ...     productseries=productseries, iscurrent=True)
-    >>> potemplate = potemplatesubset.getPOTemplateByTranslationDomain(
-    ...     'foodomain')
-    >>> print potemplate.title
-    Template "current-template" in Fooproduct fooseries
-
-Setting the filter to "False" will only return disabled templates.
-
-    >>> potemplatesubset = potemplate_set.getSubset(
-    ...     productseries=productseries, iscurrent=False)
-    >>> potemplate = potemplatesubset.getPOTemplateByTranslationDomain(
-    ...     'foodomain')
-    >>> print potemplate.title
-    Template "disabled-template" in Fooproduct fooseries
 
 == getClosestPOTemplate ==
 

=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py	2011-08-23 10:39:58 +0000
+++ lib/lp/translations/interfaces/potemplate.py	2011-09-16 16:39:26 +0000
@@ -608,16 +608,13 @@
         The `IPOTemplate` is restricted to this concrete `IPOTemplateSubset`.
         """
 
-    def getPOTemplateByTranslationDomain(translation_domain):
-        """Return the `IPOTemplate` with the given translation_domain.
-
-        The `IPOTemplate` is restricted to this concrete
-        `IPOTemplateSubset`.  If multiple templates in the subset match,
-        a warning is logged.
-
-        :return: The single template in this `IPOTemplateSubset` with
-            the given translation_domain, if there is exactly one match.
-            None otherwise.
+    def getPOTemplatesByTranslationDomain(translation_domain):
+        """Return the `IPOTemplate`s with the given translation_domain.
+
+        The search is restricted to this concrete `IPOTemplateSubset`.
+
+        :return: An ORM result set comtaining the templates in the given
+            `IPOTemplateSubset` with the given translation_domain.
         """
 
     def getPOTemplateByPath(path):

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2011-07-26 11:04:37 +0000
+++ lib/lp/translations/model/potemplate.py	2011-09-16 16:39:26 +0000
@@ -1200,28 +1200,11 @@
         result = self._build_query(POTemplate.name == name, ordered=False)
         return result.one()
 
-    def getPOTemplateByTranslationDomain(self, translation_domain):
+    def getPOTemplatesByTranslationDomain(self, translation_domain):
         """See `IPOTemplateSubset`."""
-        query_result = self._build_query(
+        return self._build_query(
             POTemplate.translation_domain == translation_domain)
 
-        # Fetch up to 2 templates, to check for duplicates.
-        matches = query_result.config(limit=2)
-
-        result = [match for match in matches]
-        if len(result) == 0:
-            return None
-        elif len(result) == 1:
-            return result[0]
-        else:
-            templates = ['"%s"' % template.displayname for template in result]
-            templates.sort()
-            log.warn(
-                "Found %d competing templates with translation domain '%s': "
-                "%s."
-                % (len(templates), translation_domain, '; '.join(templates)))
-            return None
-
     def getPOTemplateByPath(self, path):
         """See `IPOTemplateSubset`."""
         result = self._build_query(

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2011-09-12 11:35:46 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-09-16 16:39:26 +0000
@@ -136,6 +136,37 @@
     return info
 
 
+def compose_approval_conflict_notice(domain, templates_count, sample):
+    """Create a note to warn about an approval conflict.
+
+    The note warns about the situation where one productseries, or source
+    package, or in some cases distroseries has multiple actice templates
+    with the same translation domain.
+
+    :param domain: The domain that's causing trouble.
+    :param templates_count: The number of clashing templates.
+    :param sample: Iterable of matching templates.  Does not need to be
+        complete, just enough to report the problem usefully.
+    :return: A string describing the problematic clash.
+    """
+    sample_names = sorted([
+        '"%s"' % template.displayname for template in sample])
+    if templates_count > len(sample_names):
+        sample_names.append("and more (not shown here)")
+    return dedent("""\
+        Can't auto-approve upload: it is not clear what template it belongs
+        to.
+
+        There are %d competing templates with translation domain '%s':
+        %s.
+
+        This may mean that Launchpad's auto-approver is looking for the wrong
+        domain, or that these templates' domains should be changed, or that
+        some of these templates are obsolete and need to be disabled.
+        """
+        ) % (templates_count, domain, ';\n'.join(sample_names))
+
+
 class TranslationImportQueueEntry(SQLBase):
     implements(ITranslationImportQueueEntry)
 
@@ -407,6 +438,51 @@
             # We don't know where this entry should be imported.
             return None
 
+    def reportApprovalConflict(self, domain, templates_count, sample):
+        """Report an approval conflict."""
+        # Not sending out email for now; just tack a notice onto the
+        # queue entry where the user can find it through the queue UI.
+        notice = compose_approval_conflict_notice(
+            domain, templates_count, sample)
+        if notice != self.error_output:
+            self.setErrorOutput(notice)
+
+    def matchPOTemplateByDomain(self, domain, sourcepackagename=None):
+        """Attempt to find the one matching template, by domain.
+
+        Looks within the context of the queue entry.  If multiple templates
+        match, reports an approval conflict.
+
+        :param domain: Translation domain to look for.
+        :param sourcepackagename: Optional `SourcePackageName` to look for.
+            If not given, source package name is not considered in the
+            search.
+        :return: A single `POTemplate`, or None.
+        """
+        potemplateset = getUtility(IPOTemplateSet)
+        subset = potemplateset.getSubset(
+            productseries=self.productseries, distroseries=self.distroseries,
+            sourcepackagename=sourcepackagename, iscurrent=True)
+        templates_query = subset.getPOTemplatesByTranslationDomain(domain)
+
+        # Get a limited sample of the templates.  All we need from the
+        # sample is (1) to detect the presence or more than one match,
+        # and (2) to report a helpful sampling of the problem.
+        samples = list(templates_query[:5])
+
+        if len(samples) == 0:
+            # No matches found, sorry.
+            return None
+        elif len(samples) == 1:
+            # Found the one template we're looking for.
+            return samples[0]
+        else:
+            # There's a conflict.  Report the real number of competing
+            # templates, plus a sampling of template names.
+            self.reportApprovalConflict(
+                domain, templates_query.count(), samples)
+            return None
+
     def _get_pofile_from_language(self, lang_code, translation_domain,
         sourcepackagename=None):
         """Return an IPOFile for the given language and domain.
@@ -431,17 +507,12 @@
             # of just 'es' or 'fr'.
             return None
 
-        potemplateset = getUtility(IPOTemplateSet)
-
-        # Let's try first the sourcepackagename or productseries where the
-        # translation comes from.
-        potemplate_subset = potemplateset.getSubset(
-            distroseries=self.distroseries,
-            sourcepackagename=self.sourcepackagename,
-            productseries=self.productseries,
-            iscurrent=True)
-        potemplate = potemplate_subset.getPOTemplateByTranslationDomain(
-            translation_domain)
+        # Normally we find the translation's template in the
+        # source package or productseries where the translation was
+        # uploaded.  Exactly one template should have the domain we're
+        # looking for.
+        potemplate = self.matchPOTemplateByDomain(
+            translation_domain, sourcepackagename=self.sourcepackagename)
 
         is_for_distro = self.distroseries is not None
         know_package = (
@@ -450,14 +521,10 @@
             self.sourcepackagename.name == sourcepackagename.name)
 
         if potemplate is None and is_for_distro and not know_package:
-            # The source package from where this translation doesn't have the
-            # template that this translation needs it, and thus, we look for
-            # it in a different source package as a second try. To do it, we
-            # need to get a subset of all packages in current distro series.
-            potemplate_subset = potemplateset.getSubset(
-                distroseries=self.distroseries, iscurrent=True)
-            potemplate = potemplate_subset.getPOTemplateByTranslationDomain(
-                translation_domain)
+            # This translation was uploaded to a source package, but the
+            # package does not have the matching template.  Try finding
+            # it elsewhere in the distribution.
+            potemplate = self.matchPOTemplateByDomain(translation_domain)
 
         if potemplate is None:
             # The potemplate is not yet imported; we cannot attach this
@@ -1236,8 +1303,10 @@
 
         # Yay!  We have a POTemplate or POFile to import this entry
         # into.  Approve.
-        entry.setStatus(RosettaImportStatus.APPROVED,
-                        getUtility(ILaunchpadCelebrities).rosetta_experts)
+        entry.setStatus(
+            RosettaImportStatus.APPROVED,
+            getUtility(ILaunchpadCelebrities).rosetta_experts)
+        entry.setErrorOutput(None)
 
         return True
 

=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2011-09-12 12:53:05 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2011-09-16 16:39:26 +0000
@@ -475,39 +475,117 @@
             sourcepackagename=self.from_packagename)
         self.assertEqual(guessed_template, None)
 
-    def test_ByTranslationDomain(self):
-        # getPOTemplateByTranslationDomain looks up a template by
-        # translation domain.
-        self._setUpDistro()
-        subset = POTemplateSubset(distroseries=self.distroseries)
-        self.becomeTheGardener()
-        potemplate = subset.getPOTemplateByTranslationDomain('test1')
-        self.assertEqual(potemplate, self.distrotemplate1)
-
-    def test_ByTranslationDomain_none(self):
-        # Test getPOTemplateByTranslationDomain for the zero-match case.
-        self._setUpDistro()
-        subset = POTemplateSubset(distroseries=self.distroseries)
-        self.becomeTheGardener()
-        potemplate = subset.getPOTemplateByTranslationDomain('notesthere')
-        self.assertEqual(potemplate, None)
-
-    def test_ByTranslationDomain_duplicate(self):
-        # getPOTemplateByTranslationDomain returns no template if there
-        # is more than one match.
-        self._setUpDistro()
-        self.distrotemplate1.iscurrent = True
-        other_package = SourcePackageNameSet().new('other-package')
-        other_subset = POTemplateSubset(
-            distroseries=self.distroseries, sourcepackagename=other_package)
-        clashing_template = other_subset.new(
-            'test3', 'test1', 'test3.pot', self.distro.owner)
-        distro_subset = POTemplateSubset(distroseries=self.distroseries)
-        self.becomeTheGardener()
-        potemplate = distro_subset.getPOTemplateByTranslationDomain('test1')
-        self.assertEqual(potemplate, None)
-
-        clashing_template.destroySelf()
+    def test_ByDomain_finds_by_domain(self):
+        # matchPOTemplateByDomain looks for a template of a given domain
+        # in the entry's context.  It ignores other domains.
+        series = self.factory.makeProductSeries()
+        templates = [
+            self.factory.makePOTemplate(productseries=series)
+            for counter in xrange(2)]
+        entry = self.factory.makeTranslationImportQueueEntry(
+            productseries=series)
+        self.assertEqual(
+            templates[0],
+            removeSecurityProxy(entry).matchPOTemplateByDomain(
+                templates[0].translation_domain))
+
+    def test_byDomain_finds_in_productseries(self):
+        # matchPOTemplateByDomain for a productseries upload looks only
+        # in that productseries.
+        domain = self.factory.getUniqueString()
+        templates = [
+            self.factory.makePOTemplate(
+                translation_domain=domain,
+                productseries=self.factory.makeProductSeries())
+            for counter in xrange(2)]
+        entry = self.factory.makeTranslationImportQueueEntry(
+            productseries=templates[0].productseries)
+        self.assertEqual(
+            templates[0],
+            removeSecurityProxy(entry).matchPOTemplateByDomain(domain))
+
+    def test_byDomain_finds_in_source_package(self):
+        # matchPOTemplateByDomain for a distro upload, if given a source
+        # package, looks only in that source package.  It doesn't matter
+        # if the entry itself is for the same source package or not.
+        domain = self.factory.getUniqueString()
+        distroseries = self.factory.makeDistroSeries()
+        templates = [
+            self.factory.makePOTemplate(
+                translation_domain=domain, distroseries=distroseries,
+                sourcepackagename=self.factory.makeSourcePackageName())
+            for counter in xrange(2)]
+        entry = self.factory.makeTranslationImportQueueEntry(
+            distroseries=distroseries,
+            sourcepackagename=templates[1].sourcepackagename)
+        self.assertEqual(
+            templates[0],
+            removeSecurityProxy(entry).matchPOTemplateByDomain(
+                domain, templates[0].sourcepackagename))
+
+    def test_byDomain_ignores_sourcepackagename_by_default(self):
+        # If no sourcepackagename is given, matchPOTemplateByDomain
+        # on a distroseries searches all packages in the series.
+        distroseries = self.factory.makeDistroSeries()
+        template = self.factory.makePOTemplate(
+            distroseries=distroseries,
+            sourcepackagename=self.factory.makeSourcePackageName())
+        entry = self.factory.makeTranslationImportQueueEntry(
+            distroseries=distroseries,
+            sourcepackagename=self.factory.makeSourcePackageName())
+        self.assertEqual(
+            template,
+            removeSecurityProxy(entry).matchPOTemplateByDomain(
+                template.translation_domain))
+
+    def test_ByDomain_may_return_None(self):
+        # If no templates match, matchPOTemplateByDomain returns None.
+        entry = self.factory.makeTranslationImportQueueEntry()
+        self.assertEqual(
+            None,
+            removeSecurityProxy(entry).matchPOTemplateByDomain("domain"))
+
+    def test_ByDomain_reports_conflicts(self):
+        # If multiple templates match, matchPOTemplateByDomain registers
+        # an error in the entry's error_output, and returns None.
+        domain = self.factory.getUniqueString()
+        series = self.factory.makeProductSeries()
+        templates = [
+            self.factory.makePOTemplate(
+                translation_domain=domain, productseries=series)
+            for counter in xrange(2)]
+        entry = self.factory.makeTranslationImportQueueEntry(
+            productseries=series)
+
+        with self.beingTheGardener():
+            result = removeSecurityProxy(entry).matchPOTemplateByDomain(
+                domain)
+
+        self.assertIs(None, result)
+        self.assertIn(templates[0].displayname, entry.error_output)
+
+    def test_ByDomain_ignores_inactive_templates(self):
+        series = self.factory.makeProductSeries()
+        template = self.factory.makePOTemplate(
+            productseries=series, iscurrent=False)
+        entry = self.factory.makeTranslationImportQueueEntry(
+            productseries=series)
+        self.assertIs(
+            None,
+            removeSecurityProxy(entry).matchPOTemplateByDomain(
+                template.translation_domain))
+
+    def test_approval_clears_error_output(self):
+        # If a previous approval attempt set an error notice on the
+        # entry, successful approval clears it away.
+        template = self.factory.makePOTemplate(path='messages.pot')
+        pofile = self.factory.makePOFile(potemplate=template)
+        entry = self.factory.makeTranslationImportQueueEntry(
+            productseries=pofile.potemplate.productseries,
+            potemplate=pofile.potemplate, pofile=pofile)
+        entry.setErrorOutput("Entry can't be approved for whatever reason.")
+        TranslationImportQueue()._attemptToApprove(entry)
+        self.assertIs(None, entry.error_output)
 
     def test_ClashingEntries(self):
         # Very rarely two entries may have the same uploader, path, and

=== modified file 'lib/lp/translations/tests/test_potemplate.py'
--- lib/lp/translations/tests/test_potemplate.py	2011-07-26 11:28:41 +0000
+++ lib/lp/translations/tests/test_potemplate.py	2011-09-16 16:39:26 +0000
@@ -705,3 +705,154 @@
     def makeOtherSidePOTemplate(self):
         return self.factory.makePOTemplate(
             sourcepackage=self.sourcepackage, name=self.shared_template_name)
+
+
+class TestPOTemplateSubset(TestCaseWithFactory):
+    """Test POTemplate functions not covered by doctests."""
+
+    layer = ZopelessDatabaseLayer
+
+    def test_getPOTemplatesByTranslationDomain_filters_by_domain(self):
+        domain = self.factory.getUniqueString()
+        series = self.factory.makeProductSeries()
+
+        # The template we'll be looking for:
+        template = self.factory.makePOTemplate(
+            translation_domain=domain, productseries=series)
+
+        # Another template in the same context with a different domain:
+        self.factory.makePOTemplate(productseries=series)
+
+        subset = getUtility(IPOTemplateSet).getSubset(productseries=series)
+        self.assertContentEqual(
+            [template], subset.getPOTemplatesByTranslationDomain(domain))
+
+    def test_getPOTemplatesByTranslationDomain_finds_by_productseries(self):
+        domain = self.factory.getUniqueString()
+        productseries = self.factory.makeProductSeries()
+
+        # The template we'll be looking for:
+        template = self.factory.makePOTemplate(
+            translation_domain=domain, productseries=productseries)
+
+        # Similar templates that should not come up in the same search:
+        # * Different series (even for the same product).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            productseries=self.factory.makeProductSeries(
+                product=template.productseries.product))
+        # * Distro and series (even with the same name as the domain
+        # we're looking for).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            distroseries=self.factory.makeDistroSeries(
+                name=domain, distribution=self.factory.makeDistribution(
+                    name=domain)))
+        # * Source package (even with the same name as the domain we're
+        # looking for).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            distroseries=self.factory.makeDistroSeries(),
+            sourcepackagename=self.factory.makeSourcePackageName(name=domain))
+
+        subset = getUtility(IPOTemplateSet).getSubset(
+            productseries=productseries)
+        self.assertContentEqual(
+            [template], subset.getPOTemplatesByTranslationDomain(domain))
+
+    def test_getPOTemplatesByTranslationDomain_finds_by_sourcepackage(self):
+        domain = self.factory.getUniqueString()
+        package = self.factory.makeSourcePackage()
+
+        # The template we'll be looking for:
+        template = self.factory.makePOTemplate(
+            translation_domain=domain, distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+
+        # Similar templates that should not come up in the same search:
+        # * Productseries (even with the same names the domain we're
+        # looking for).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            productseries=self.factory.makeProductSeries(
+                name=domain, product=self.factory.makeProduct(name=domain)))
+
+        # * Different series (even for the same source package name and
+        # distribution).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            sourcepackagename=package.sourcepackagename,
+            distroseries=self.factory.makeDistroSeries(
+                distribution=package.distroseries.distribution))
+
+        subset = getUtility(IPOTemplateSet).getSubset(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        self.assertContentEqual(
+            [template], subset.getPOTemplatesByTranslationDomain(domain))
+
+    def test_getPOTemplatesByTranslationDomain_finds_by_distroseries(self):
+        domain = self.factory.getUniqueString()
+        distroseries = self.factory.makeDistroSeries()
+
+        # The template we'll be looking for:
+        template = self.factory.makePOTemplate(
+            translation_domain=domain, distroseries=distroseries)
+
+        # Similar templates that should not come up in the same search:
+        # * Productseries (even with the same names the domain we're
+        # looking for).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            productseries=self.factory.makeProductSeries(
+                name=domain, product=self.factory.makeProduct(name=domain)))
+
+        # * Different series (even for the same distribution).
+        self.factory.makePOTemplate(
+            translation_domain=domain,
+            distroseries=self.factory.makeDistroSeries(
+                distribution=distroseries.distribution))
+
+        subset = getUtility(IPOTemplateSet).getSubset(
+            distroseries=distroseries)
+        self.assertContentEqual(
+            [template], subset.getPOTemplatesByTranslationDomain(domain))
+
+    def test_getPOTemplatesByTranslationDomain_can_ignore_iscurrent(self):
+        domain = self.factory.getUniqueString()
+        series = self.factory.makeProductSeries()
+        templates = [
+            self.factory.makePOTemplate(
+                translation_domain=domain, productseries=series,
+                iscurrent=iscurrent)
+            for iscurrent in [False, True]]
+
+        subset = getUtility(IPOTemplateSet).getSubset(productseries=series)
+        self.assertContentEqual(
+            templates, subset.getPOTemplatesByTranslationDomain(domain))
+
+    def test_getPOTemplatesByTranslationDomain_can_filter_by_iscurrent(self):
+        domain = self.factory.getUniqueString()
+        series = self.factory.makeProductSeries()
+
+        templates = dict(
+            (iscurrent, [self.factory.makePOTemplate(
+                translation_domain=domain, productseries=series,
+                iscurrent=iscurrent)])
+            for iscurrent in [False, True])
+
+        potset = getUtility(IPOTemplateSet)
+        found_templates = dict((
+            iscurrent,
+            list(potset.getSubset(productseries=series, iscurrent=iscurrent
+                ).getPOTemplatesByTranslationDomain(domain),)
+            )
+            for iscurrent in [False, True])
+
+        self.assertEqual(templates, found_templates)
+
+    def test_getPOTemplatesByTranslationDomain_returns_result_set(self):
+        subset = getUtility(IPOTemplateSet).getSubset(
+            productseries=self.factory.makeProductSeries())
+        self.assertEqual(
+            0, subset.getPOTemplatesByTranslationDomain("foo").count())

=== modified file 'lib/lp/translations/tests/test_translationimportqueue.py'
--- lib/lp/translations/tests/test_translationimportqueue.py	2011-09-02 12:10:22 +0000
+++ lib/lp/translations/tests/test_translationimportqueue.py	2011-09-16 16:39:26 +0000
@@ -3,14 +3,17 @@
 
 __metaclass__ = type
 
+from operator import attrgetter
 import os.path
-
 import transaction
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.interfaces.lpstorm import ISlaveStore
 from canonical.testing.layers import (
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
     )
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
@@ -20,10 +23,15 @@
     TestCaseWithFactory,
     )
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
 from lp.translations.enums import RosettaImportStatus
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
     )
+from lp.translations.model.translationimportqueue import (
+    compose_approval_conflict_notice,
+    TranslationImportQueueEntry,
+    )
 
 
 class TestCanSetStatusBase:
@@ -447,3 +455,92 @@
             potemplate=pot, pofile=pofile, uploader=uploader)
 
         self.assertEquals(tiqe1, tiqe2)
+
+    def test_reportApprovalConflict_sets_error_output_just_once(self):
+        # Repeated occurrence of the same approval conflict will not
+        # result in repeated setting of error_output.
+        series = self.factory.makeProductSeries()
+        domain = self.factory.getUniqueString()
+        templates = [
+            self.factory.makePOTemplate(
+                productseries=series, translation_domain=domain)
+            for counter in xrange(3)]
+        entry = removeSecurityProxy(
+            self.factory.makeTranslationImportQueueEntry())
+
+        entry.reportApprovalConflict(domain, len(templates), templates)
+        original_error = entry.error_output
+        transaction.commit()
+
+        # Try reporting the conflict again, with the templates
+        # reshuffled to see if reportApprovalConflict can be fooled into
+        # thinking it's a different error.  Make as sure as we can that
+        # entry.error_output is not modified.
+        slave_entry = ISlaveStore(entry).get(
+            TranslationImportQueueEntry, entry.id)
+        slave_entry.setErrorOutput = FakeMethod()
+        slave_entry.reportApprovalConflict(
+            domain, len(templates), reversed(templates))
+        self.assertEqual(original_error, slave_entry.error_output)
+        self.assertIn(domain, original_error)
+        self.assertEqual(0, slave_entry.setErrorOutput.call_count)
+
+
+class TestHelpers(TestCaseWithFactory):
+    """Tests for stand-alone helper functions in the module."""
+
+    layer = ZopelessDatabaseLayer
+
+    def test_compose_approval_conflict_notice_summarizes_conflict(self):
+        # The output from compose_approval_conflict_notice summarizes
+        # the conflict: what translation domain is affected and how many
+        # clashing templates are there?
+        domain = self.factory.getUniqueString()
+        num_templates = self.factory.getUniqueInteger()
+
+        notice = compose_approval_conflict_notice(domain, num_templates, [])
+
+        self.assertIn("translation domain '%s'" % domain, notice)
+        self.assertIn(
+            "There are %d competing templates" % num_templates, notice)
+
+    def test_compose_approval_conflict_notice_shows_sample(self):
+        # The notice includes the list of sample templates' display
+        # names, one per line, separated by semicolons but terminated
+        # with a full stop.
+        class FakePOTemplate:
+            def __init__(self, displayname):
+                self.displayname = displayname
+
+        domain = self.factory.getUniqueString()
+        samples = [
+            FakePOTemplate(self.factory.getUniqueString())
+            for counter in range(3)]
+        sorted_samples = sorted(samples, key=attrgetter('displayname'))
+
+        notice = compose_approval_conflict_notice(domain, 3, samples)
+
+        self.assertIn(
+            ';\n'.join([
+                '"%s"' % sample.displayname for sample in sorted_samples]),
+            notice)
+        self.assertIn('"%s".\n' % sorted_samples[-1].displayname, notice)
+
+    def test_compose_approval_conflict_notice_says_when_there_is_more(self):
+        # If there are more clashing templates than the sample lists,
+        # the list of names ends with a note to that effect.
+        class FakePOTemplate:
+            def __init__(self, displayname):
+                self.displayname = displayname
+
+        domain = self.factory.getUniqueString()
+        samples = [
+            FakePOTemplate(self.factory.getUniqueString())
+            for counter in range(3)]
+        samples.sort(key=attrgetter('displayname'))
+
+        notice = compose_approval_conflict_notice(domain, 4, samples)
+
+        self.assertIn(
+            '"%s";\nand more (not shown here).\n' % samples[-1].displayname,
+            notice)