launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02598
[Merge] lp:~jtv/launchpad/bug-715854 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-715854 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#715854 Exported Firefox translations contain wrong references to languages other than the current
https://bugs.launchpad.net/bugs/715854
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-715854/+merge/49676
= Summary =
Bug 715854 reports a case where messages in translation templates ("POTMsgSets") do not have the proper file references set. These are annotations that record where in a project's source tree the messages in question were originally found. For XPI handling, which this bug is about, those file references need to be exactly right, or a crucial file-format conversion that exported translations need to go through will break.
But the bug turns out to be one that can happen with any format, not just XPI. Here's how it happens.
When importing a template, a message is processed by looking up whether it's in the database, and if not creating it. (In the database it takes the form of a POTMsgSet). It is linked into the template by looking up a TranslationTemplateItem link-table entry or creating a new one. The link-table entry has its sequence number set appropriately to indicate where the message occurs in the template. Then, the message's filereferences (among other things) are set to whatever is found in the imported file.
When importing a translation, a message consists of two parts: the translatable message which should match one in the template, and a translation. The translatable message is processed in a very similar way, except the link-table entry gets a sequence number of zero to indicate that the translatable message is obsolete (because it isn't really in this template). Then, the message's filereferences are set to whatever is found in the imported file. That doesn't make all that much sense, but recording what information we have may be helpful for debugging.
All this used to work just fine. But then came Translations Message Sharing. Now, POTMsgSets can be shared between templates. So the obsolete POTMsgSet that has its filereferences updated in the second scenario may be the same one that originally had more sensible information attached to it in the first scenario!
== Proposed fix ==
Don't set filereferences on a POTMsgSet when importing a translation, at least not if it already existed in the database.
== Pre-implementation notes ==
Discussed this with Danilo.
Changed to pass initializers for filereferences (and sourcecomment, since it's treated exactly the same way) into the find-existing-potmsgset-or-create-a-new-one methods. Those will be used to initialize a newly created POTMsgSet, but not to override values on an existing one. Therefore scenario 1 remains unchanged, but scenario 2 replaces assignment of POTMsgSet.filereferences with the new initializer argument.
== Implementation details ==
A unit test exercises all behaviour that's expected of the new parameters. Separately, an importer test verifies that the translation importer now behaves differently from the template importer in the same situation: one should set filereferences on a pre-existing POTMsgSet, the other should not.
You'll also note that I replace a call to createMessageSetFromText ("look up POMsgIDs representing these strings, then create a POTMsgSet referring to them") with one to createPOTMsgSetFromMsgIDs ("create a POTMsgSet referring to these strings"). That's because the calling method already has the POMsgIDs at hand; this will save a database query for every message.
== Tests ==
I ran them all, but these are the new ones:
{{{
./bin/test -vvc lp.translations.tests.test_shared_potemplate -t getOrCreate
./bin/test -vvc lp.translations.utilities.tests.test_translation_importer -t importMessage
}}}
None of the existing tests are affected, which is probably a sign of how little importance we attached to setting filereferences on a POTMsgSet that's not actually in a template yet. If it ever does get imported into a template, it'll have fresh metadata set at that point anyway.
== Demo and Q/A ==
Set up two sharing templates, A and B. Import a message into A that isn't in B, with a file references comment: "#: boo/splat.c". Then import a translation for that message to B (in any language except English), with a file references comment: "#: foo/bar.c"
Look up A and B in the web UI. The message should be visible in A, with "boo/splat.c" still as its file reference. The message should not be in B.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/interfaces/potemplate.py
lib/lp/translations/tests/test_shared_potemplate.py
lib/lp/translations/utilities/tests/test_translation_importer.py
lib/lp/translations/utilities/translation_import.py
lib/lp/translations/model/potemplate.py
--
https://code.launchpad.net/~jtv/launchpad/bug-715854/+merge/49676
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-715854 into lp:launchpad.
=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py 2011-02-07 19:11:12 +0000
+++ lib/lp/translations/interfaces/potemplate.py 2011-02-14 17:55:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -497,13 +497,21 @@
:return: The newly created message set.
"""
- def getOrCreateSharedPOTMsgSet(singular_text, plural_text, context=None):
+ def getOrCreateSharedPOTMsgSet(singular_text, plural_text, context=None,
+ initial_file_references=None,
+ initial_source_comment=None):
"""Finds an existing shared POTMsgSet to use or creates a new one.
:param singular_text: string containing singular form.
:param plural_text: string containing plural form.
:param context: context to differentiate between two messages with
same singular_text and plural_text.
+ :param initial_file_references: Initializer for file_references if
+ a new POTMsgSet needs to be created. Will not be set on an
+ existing POTMsgSet.
+ :param initial_source_comment: Initializer for source_comment if
+ a new POTMsgSet needs to be created. Will not be set on an
+ existing POTMsgSet.
:return: existing or new shared POTMsgSet with a sequence of 0
in this POTemplate.
"""
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2011-02-07 18:36:26 +0000
+++ lib/lp/translations/model/potemplate.py 2011-02-14 17:55:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0611,W0212
@@ -881,7 +881,8 @@
context, sequence)
def getOrCreateSharedPOTMsgSet(self, singular_text, plural_text,
- context=None):
+ context=None, initial_file_references=None,
+ initial_source_comment=None):
"""See `IPOTemplate`."""
msgid_singular = self.getOrCreatePOMsgID(singular_text)
if plural_text is None:
@@ -891,8 +892,10 @@
potmsgset = self._getPOTMsgSetBy(msgid_singular, msgid_plural,
context, sharing_templates=True)
if potmsgset is None:
- potmsgset = self.createMessageSetFromText(
- singular_text, plural_text, context, sequence=0)
+ potmsgset = self.createPOTMsgSetFromMsgIDs(
+ msgid_singular, msgid_plural, context, sequence=0)
+ potmsgset.filereferences = initial_file_references
+ potmsgset.sourcecomment = initial_source_comment
return potmsgset
def importFromQueue(self, entry_to_import, logger=None, txn=None):
=== modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
--- lib/lp/translations/tests/test_shared_potemplate.py 2011-01-06 17:52:42 +0000
+++ lib/lp/translations/tests/test_shared_potemplate.py 2011-02-14 17:55:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=C0102
@@ -178,6 +178,52 @@
singular_text, None)
self.assertEquals(potmsgset, shared_potmsgset)
+ def test_getOrCreateSharedPOTMsgSet_initializes_file_references(self):
+ # When creating a POTMsgSet, getOrCreateSharedPOTMsgSet
+ # initializes its filereferences to initial_file_references.
+ singular = self.factory.getUniqueString()
+ file_references = self.factory.getUniqueString()
+ potmsgset = self.devel_potemplate.getOrCreateSharedPOTMsgSet(
+ singular, None, initial_file_references=file_references)
+ self.assertEqual(file_references, potmsgset.filereferences)
+
+ def test_getOrCreateSharedPOTMsgSet_leaves_file_references_intact(self):
+ # In returning an existing POTMsgSet, getOrCreateSharedPOTMsgSet
+ # leaves its existing filereferences unchanged. The
+ # initial_file_references argument is ignored.
+ potmsgset = self.factory.makePOTMsgSet(
+ potemplate=self.devel_potemplate)
+ potmsgset.filereferences = None
+ new_file_references = self.factory.getUniqueString()
+ updated_potmsgset = self.devel_potemplate.getOrCreateSharedPOTMsgSet(
+ potmsgset.singular_text, potmsgset.plural_text,
+ initial_file_references=new_file_references)
+ self.assertEqual(potmsgset, updated_potmsgset)
+ self.assertIs(None, potmsgset.filereferences)
+
+ def test_getOrCreateSharedPOTMsgSet_initializes_source_comment(self):
+ # When creating a POTMsgSet, getOrCreateSharedPOTMsgSet
+ # initializes its sourcecomment to initial_source_comment.
+ singular = self.factory.getUniqueString()
+ source_comment = self.factory.getUniqueString()
+ potmsgset = self.devel_potemplate.getOrCreateSharedPOTMsgSet(
+ singular, None, initial_source_comment=source_comment)
+ self.assertEqual(source_comment, potmsgset.sourcecomment)
+
+ def test_getOrCreateSharedPOTMsgSet_leaves_source_comment_intact(self):
+ # In returning an existing POTMsgSet, getOrCreateSharedPOTMsgSet
+ # leaves its existing sourcecomment unchanged. The
+ # initial_source_comment argument is ignored.
+ potmsgset = self.factory.makePOTMsgSet(
+ potemplate=self.devel_potemplate)
+ potmsgset.sourcecomment = None
+ new_source_comment = self.factory.getUniqueString()
+ updated_potmsgset = self.devel_potemplate.getOrCreateSharedPOTMsgSet(
+ potmsgset.singular_text, potmsgset.plural_text,
+ initial_source_comment=new_source_comment)
+ self.assertEqual(potmsgset, updated_potmsgset)
+ self.assertIs(None, potmsgset.sourcecomment)
+
class TestSharingPOTemplatesByRegex(TestCaseWithFactory):
"""Isolate tests for regular expression use in SharingSubset."""
=== modified file 'lib/lp/translations/utilities/tests/test_translation_importer.py'
--- lib/lp/translations/utilities/tests/test_translation_importer.py 2010-12-02 16:13:51 +0000
+++ lib/lp/translations/utilities/tests/test_translation_importer.py 2011-02-14 17:55:38 +0000
@@ -8,6 +8,7 @@
import transaction
from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.services.log.logger import DevNullLogger
from lp.testing import TestCaseWithFactory
from lp.testing.matchers import Provides
from lp.translations.enums import RosettaImportStatus
@@ -23,10 +24,34 @@
from lp.translations.utilities.translation_import import (
importers,
is_identical_translation,
+ POFileImporter,
+ POTFileImporter,
TranslationImporter,
)
+class FakeImportQueueEntry:
+ by_maintainer = True
+ format = TranslationFileFormat.PO
+ content = None
+
+ def __init__(self, potemplate, pofile=None):
+ self.importer = potemplate.owner
+ self.potemplate = potemplate
+ self.pofile = pofile
+
+
+class FakeTranslationFile:
+ header = None
+
+
+class FakeParser:
+ uses_source_string_msgids = False
+
+ def parse(self, entry):
+ return FakeTranslationFile()
+
+
class TranslationImporterTestCase(TestCaseWithFactory):
"""Class test for translation importer component"""
layer = LaunchpadZopelessLayer
@@ -243,3 +268,36 @@
self.assertTrue(existing_translation.is_current_upstream)
TranslationImporter().importFile(entry)
self.assertTrue(existing_translation.is_current_upstream)
+
+ def test_template_importMessage_updates_file_references(self):
+ # Importing a template message updates the filereferences on an
+ # existing POTMsgSet.
+ template = self.factory.makePOTemplate()
+ potmsgset = self.factory.makePOTMsgSet(potemplate=template)
+ old_file_references = self.factory.getUniqueString()
+ new_file_references = self.factory.getUniqueString()
+ potmsgset.filereferences = old_file_references
+ message = TranslationMessageData()
+ message.msgid_singular = potmsgset.singular_text
+ message.file_references = new_file_references
+ queue_entry = FakeImportQueueEntry(template)
+ importer = POTFileImporter(queue_entry, FakeParser(), DevNullLogger())
+ importer.importMessage(message)
+ self.assertEqual(new_file_references, potmsgset.filereferences)
+
+ def test_translation_importMessage_does_not_update_file_references(self):
+ # Importing a translation message does not update the
+ # filereferences on an existing POTMsgSet. (It used to, which
+ # is what caused bug 715854).
+ pofile = self.factory.makePOFile()
+ potmsgset = self.factory.makePOTMsgSet(potemplate=pofile.potemplate)
+ old_file_references = self.factory.getUniqueString()
+ new_file_references = self.factory.getUniqueString()
+ potmsgset.filereferences = old_file_references
+ message = TranslationMessageData()
+ message.msgid_singular = potmsgset.singular_text
+ message.file_references = new_file_references
+ queue_entry = FakeImportQueueEntry(pofile.potemplate, pofile)
+ importer = POFileImporter(queue_entry, FakeParser(), DevNullLogger())
+ importer.importMessage(message)
+ self.assertEqual(old_file_references, potmsgset.filereferences)
=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py 2011-01-04 17:23:42 +0000
+++ lib/lp/translations/utilities/translation_import.py 2011-02-14 17:55:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -413,11 +413,11 @@
:param message: The message.
:return: The POTMsgSet instance, existing or new.
"""
- potmsgset = (
- self.potemplate.getOrCreateSharedPOTMsgSet(
- message.msgid_singular, plural_text=message.msgid_plural,
- context=message.context))
- return potmsgset
+ return self.potemplate.getOrCreateSharedPOTMsgSet(
+ message.msgid_singular, plural_text=message.msgid_plural,
+ context=message.context,
+ initial_file_references=message.file_references,
+ initial_source_comment=message.source_comment)
@cachedproperty
def share_with_other_side(self):
@@ -581,9 +581,6 @@
:param potmsgset: The current messageset for this message id.
:param errormsg: The errormessage returned by updateTranslation.
"""
-# XXX: henninge 2008-11-05: The error should contain an ID of some sort
-# to provide an explicit identification in tests. Until then error messages
-# must not be rephrased without changing the test as well.
self.errors.append({
'potmsgset': potmsgset,
'pofile': self.pofile,
@@ -801,11 +798,6 @@
return
potmsgset = self.getOrCreatePOTMsgSet(message)
- if potmsgset.getSequence(self.potemplate) == 0:
- # We are importing a message that does not exist in
- # latest translation template so we can update its values.
- potmsgset.sourcecomment = message.source_comment
- potmsgset.filereferences = message.file_references
if 'fuzzy' in message.flags:
message.flags.remove('fuzzy')
Follow ups
