launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02312
[Merge] lp:~jtv/launchpad/bug-611217 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-611217 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#611217 Gardener breaks on non-unique POFile path
https://bugs.launchpad.net/bugs/611217
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-611217/+merge/45883
= 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 entry, based on a path match in that productseries or source package. POFileSet.getPOFilesByPathAndOrigin implements such a match.
That combination 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
--
https://code.launchpad.net/~jtv/launchpad/bug-611217/+merge/45883
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-611217 into lp:launchpad.
=== 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-11 16:55:47 +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-11 16:55:47 +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,60 @@
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-11 16:55:47 +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-11 16:55:47 +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-11 16:55:47 +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-11 16:55:47 +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."""
Follow ups
