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

[Jeroen T. Vermeulen]

[r=mars][ui=none][bug=611217] Ignore POFiles for obsolete POTemplates during upload approval.

= Bug 611217 =

Not for the first time, the translations import queue gardener broke down recently because of name clashes.

The problem is this: a queue entry contains a translation file for a given productseries or source package (in this case it happened to be git in Ubuntu 10.10). The gardener's auto-approval code tries to match this to an existing POFile, based on a path match in that productseries or source package. POFileSet.getPOFilesByPathAndOrigin implements the match.

That combination of path and productseries/package should normally be unique, and so TranslationImportQueueEntry._guessed_pofile_from_path returns getPOFilesByPathAndOrigin(…).one(). But it is occasionally possible for a POFile belonging to an obsolete POTemplate to have the same path as another POFile belonging a current POTemplate in the same context. KABOOM. The one() unexpectedly finds two records, and blows up.

Actually we have no particular interest in auto-approving translations for obsolete templates anyway. So in this branch I make _guessed_pofile_from_path ignore POFiles for obsolete templates. That avoids the clashes as a side effect.

In addition to this, I cleaned up some browser code that unnecessarily duplicated the logic of figuring out filename extensions (which is also done by the importer). I also found that it clashed with our coding standards and could generally be simpler, so that turned into a bit of a local rewrite. The only practical effect is that the notions of "template file" and "translation file" are no longer directly coupled to gettext ".po" and ".pot" files, respectively.

I ran various tests, but this should just about cover it:
{{{
./bin/test -vvc lp.translations -t queue -t approv -t test_pofile
}}}

The sign that this works will be that we no longer get these crises where Ubuntu and manual translation uploads stop being approved. For active Q/A, we'll want to make sure that manual uploads are still approved automatically in each of 3 scenarios:
 * There is a POFile with a matching path.
 * There is a POFile for the language, but with a different path.
 * There is no POFile for the language.

No lint,

Jeroen

[Māris Fogels (mars)]

Hi Jeroen,

This change looks great, r=mars.

I think you have one stray if-statement around line 72 of the diff: you can probably collapse that into a straight if/elif instead of nested statements.

Looks good!

review: Approve

=== modified file 'lib/lp/translations/browser/tests/translationimportqueue-views.txt'
--- lib/lp/translations/browser/tests/translationimportqueue-views.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/translations/browser/tests/translationimportqueue-views.txt	2011-01-12 09:00:40 +0000
@@ -89,7 +89,7 @@
     ...     'name': 'demo-name',
     ...     'translation_domain': 'demo-domain'}
     >>> validate(view, data)
-    The file name must end with ".po".
+    This filename is not appropriate for a translation.
     Please choose a template.
 
 Also, the filename must be given and the template name must be a valid name.
@@ -118,7 +118,7 @@
     ...     'potemplate': potemplate,
     ...     'language': language }
     >>> validate(view, data)
-    The file name must end with ".pot".
+    This filename is not appropriate for a template.
     Please specify a name for the template.
     Please specify a translation domain for the template.
 

=== modified file 'lib/lp/translations/browser/translationimportqueue.py'
--- lib/lp/translations/browser/translationimportqueue.py	2010-12-02 16:13:51 +0000
+++ lib/lp/translations/browser/translationimportqueue.py	2011-01-12 09:00:40 +0000
@@ -14,10 +14,6 @@
     ]
 
 import os
-from os.path import (
-    basename,
-    splitext,
-    )
 
 from zope.app.form.interfaces import ConversionError
 from zope.component import getUtility
@@ -52,6 +48,9 @@
 from lp.translations.enums import RosettaImportStatus
 from lp.translations.interfaces.pofile import IPOFileSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.translationimporter import (
+    ITranslationImporter,
+    )
 from lp.translations.interfaces.translationimportqueue import (
     IEditTranslationImportQueueEntry,
     ITranslationImportQueue,
@@ -102,11 +101,12 @@
             return field_values
         # Fill the know values.
         field_values['path'] = self.context.path
-        (fname, fext) = splitext(self.context.path)
-        if fext.lower() == '.po':
+
+        importer = getUtility(ITranslationImporter)
+        if importer.isTemplateName(self.context.path):
+            file_type = TranslationFileType.POT
+        elif importer.isTranslationName(self.context.path):
             file_type = TranslationFileType.PO
-        elif fext.lower() == '.pot':
-            file_type = TranslationFileType.POT
         else:
             file_type = TranslationFileType.UNSPEC
         field_values['file_type'] = file_type
@@ -323,56 +323,59 @@
                 productseries=self.context.productseries)
         return potemplate_subset
 
+    def _findObjectionToFilePath(self, file_type, path):
+        """Return textual objection, if any, to setting this file path."""
+        importer = getUtility(ITranslationImporter)
+        if file_type == TranslationFileType.POT:
+            if not importer.isTemplateName(path):
+                return "This filename is not appropriate for a template."
+        else:
+            if not importer.isTranslationName(path):
+                return "This filename is not appropriate for a translation."
+
+        if path == self.context.path:
+            # No change, so no objections.
+            return None
+
+        # The Rosetta Expert decided to change the path of the file.
+        # Before accepting such change, we should check first whether
+        # there is already another entry with that path in the same
+        # context (sourcepackagename/distroseries or productseries).
+        # A duplicate name will confuse the auto-approval
+        # process.
+        if file_type == TranslationFileType.POT:
+            potemplate_set = getUtility(IPOTemplateSet)
+            existing_file = potemplate_set.getPOTemplateByPathAndOrigin(
+                path, self.context.productseries, self.context.distroseries,
+                self.context.sourcepackagename)
+            already_exists = existing_file is not None
+        else:
+            pofile_set = getUtility(IPOFileSet)
+            existing_files = pofile_set.getPOFilesByPathAndOrigin(
+                path, self.context.productseries,
+                self.context.distroseries,
+                self.context.sourcepackagename)
+            already_exists = not existing_files.is_empty()
+
+        if already_exists:
+            # We already have an IPOFile in this path, let's notify
+            # the user about that so they choose another path.
+            return "There is already a file in the given path."
+
+        return None
+
     def _validatePath(self, file_type, path):
-        path_changed = False # Flag for change_action
-        if path == None or path.strip() == "":
-            self.setFieldError('path', 'The file name is missing.')
+        """Should the entry's path be updated?"""
+        if path is None or path.strip() == "":
+            self.setFieldError('path', "The file name is missing.")
+            return False
+
+        objection = self._findObjectionToFilePath(file_type, path)
+        if objection is None:
+            return True
         else:
-            (fname, fext) = splitext(basename(path))
-            if len(fname) == 0:
-                self.setFieldError('path',
-                                   'The file name is incomplete.')
-            if (file_type == TranslationFileType.POT and
-                    fext.lower() != '.pot' and fext.lower() != '.xpi'):
-                self.setFieldError('path',
-                                   'The file name must end with ".pot".')
-            if (file_type == TranslationFileType.PO and
-                    fext.lower() != '.po' and fext.lower() != '.xpi'):
-                self.setFieldError('path',
-                                   'The file name must end with ".po".')
-
-            if self.context.path != path:
-                # The Rosetta Expert decided to change the path of the file.
-                # Before accepting such change, we should check first whether
-                # there is already another entry with that path in the same
-                # context (sourcepackagename/distroseries or productseries).
-                if file_type == TranslationFileType.POT:
-                    potemplate_set = getUtility(IPOTemplateSet)
-                    existing_file = (
-                        potemplate_set.getPOTemplateByPathAndOrigin(
-                            path, self.context.productseries,
-                            self.context.distroseries,
-                            self.context.sourcepackagename))
-                    already_exists = existing_file is not None
-                else:
-                    pofile_set = getUtility(IPOFileSet)
-                    existing_files = pofile_set.getPOFilesByPathAndOrigin(
-                        path, self.context.productseries,
-                        self.context.distroseries,
-                        self.context.sourcepackagename)
-                    already_exists = not existing_files.is_empty()
-
-                if already_exists:
-                    # We already have an IPOFile in this path, let's notify
-                    # the user about that so they choose another path.
-                    self.setFieldError('path',
-                        'There is already a file in the given path.')
-                else:
-                    # There is no other pofile in the given path for this
-                    # context, let's change it as requested by admins.
-                    path_changed = True
-
-        return path_changed
+            self.setFieldError('path', objection)
+            return False
 
     def _validatePOT(self, data):
         name = data.get('name')

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2010-12-30 17:22:40 +0000
+++ lib/lp/translations/model/pofile.py	2011-01-12 09:00:40 +0000
@@ -1473,7 +1473,8 @@
         return DummyPOFile(potemplate, language)
 
     def getPOFilesByPathAndOrigin(self, path, productseries=None,
-                                  distroseries=None, sourcepackagename=None):
+                                  distroseries=None, sourcepackagename=None,
+                                  ignore_obsolete=False):
         """See `IPOFileSet`."""
         # Avoid circular imports.
         from lp.translations.model.potemplate import POTemplate
@@ -1492,35 +1493,35 @@
 
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
 
-        general_conditions = And(
+        conditions = [
             POFile.path == path,
-            POFile.potemplate == POTemplate.id)
+            POFile.potemplate == POTemplate.id,
+            ]
+        if ignore_obsolete:
+            conditions.append(POTemplate.iscurrent == True)
 
         if productseries is not None:
-            return store.find(POFile, And(
-                general_conditions,
-                POTemplate.productseries == productseries))
+            conditions.append(POTemplate.productseries == productseries)
         else:
-            distro_conditions = And(
-                general_conditions,
-                POTemplate.distroseries == distroseries)
+            conditions.append(POTemplate.distroseries == distroseries)
 
             # The POFile belongs to a distribution and it could come from
             # another package that its POTemplate is linked to, so we first
             # check to find it at IPOFile.from_sourcepackagename
-            matches = store.find(POFile, And(
-                distro_conditions,
-                POFile.from_sourcepackagename == sourcepackagename))
+            linked_conditions = conditions + [
+                POFile.from_sourcepackagename == sourcepackagename]
 
+            matches = store.find(POFile, *linked_conditions)
             if not matches.is_empty():
                 return matches
 
             # There is no pofile in that 'path' and
             # 'IPOFile.from_sourcepackagename' so we do a search using the
             # usual sourcepackagename.
-            return store.find(POFile, And(
-                distro_conditions,
-                POTemplate.sourcepackagename == sourcepackagename))
+            conditions.append(
+                POTemplate.sourcepackagename == sourcepackagename)
+
+        return store.find(POFile, *conditions)
 
     def getBatch(self, starting_id, batch_size):
         """See `IPOFileSet`."""

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2010-12-18 19:40:37 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-01-12 09:00:40 +0000
@@ -293,7 +293,8 @@
         return pofile_set.getPOFilesByPathAndOrigin(
             self.path, productseries=self.productseries,
             distroseries=self.distroseries,
-            sourcepackagename=self.sourcepackagename).one()
+            sourcepackagename=self.sourcepackagename,
+            ignore_obsolete=True).one()
 
     def canAdmin(self, roles):
         """See `ITranslationImportQueueEntry`."""

=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2010-10-29 05:58:51 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2011-01-12 09:00:40 +0000
@@ -37,6 +37,7 @@
 from lp.translations.enums import RosettaImportStatus
 from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
 from lp.translations.interfaces.translationimportqueue import (
+    ITranslationImportQueue,
     translation_import_queue_entry_age,
     )
 from lp.translations.model.customlanguagecode import CustomLanguageCode
@@ -536,6 +537,36 @@
 
         self.assertEqual(entry1.potemplate, None)
 
+    def test_getGuessedPOFile_ignores_obsolete_POFiles(self):
+        pofile = self.factory.makePOFile()
+        template = pofile.potemplate
+        template.iscurrent = False
+        queue = getUtility(ITranslationImportQueue)
+        entry = queue.addOrUpdateEntry(
+            pofile.path, 'contents', False, self.factory.makePerson(),
+            productseries=template.productseries)
+
+        self.assertEqual(None, entry.getGuessedPOFile())
+
+    def test_getGuessedPOFile_survives_clashing_obsolete_POFile_path(self):
+        series = self.factory.makeProductSeries()
+        current_template = self.factory.makePOTemplate(productseries=series)
+        current_template.iscurrent = True
+        current_pofile = self.factory.makePOFile(
+            'nl', potemplate=current_template)
+        obsolete_template = self.factory.makePOTemplate(productseries=series)
+        obsolete_template.iscurrent = False
+        obsolete_pofile = self.factory.makePOFile(
+            'nl', potemplate=obsolete_template)
+        obsolete_pofile.path = current_pofile.path
+
+        queue = getUtility(ITranslationImportQueue)
+        entry = queue.addOrUpdateEntry(
+            current_pofile.path, 'contents', False, self.factory.makePerson(),
+            productseries=series)
+
+        self.assertEqual(current_pofile, entry.getGuessedPOFile())
+
     def test_pathless_template_match(self):
         # If an uploaded template has no directory component in its
         # path, and no matching template is found in the database, the

=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py	2011-01-05 14:23:48 +0000
+++ lib/lp/translations/tests/test_pofile.py	2011-01-12 09:00:40 +0000
@@ -1714,6 +1714,25 @@
 
         self.assertContentEqual([pofile], found)
 
+    def test_getPOFilesByPathAndOrigin_includes_obsolete_templates(self):
+        pofile = self.factory.makePOFile()
+        template = pofile.potemplate
+        template.iscurrent = False
+        self.assertContentEqual(
+            [pofile],
+            self.pofileset.getPOFilesByPathAndOrigin(
+                pofile.path, productseries=template.productseries))
+
+    def test_getPOFilesByPathAndOrigin_can_ignore_obsolete_templates(self):
+        pofile = self.factory.makePOFile()
+        template = pofile.potemplate
+        template.iscurrent = False
+        self.assertContentEqual(
+            [],
+            self.pofileset.getPOFilesByPathAndOrigin(
+                pofile.path, productseries=template.productseries,
+                ignore_obsolete=True))
+
 
 class TestPOFileStatistics(TestCaseWithFactory):
     """Test PO files statistics calculation."""