launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-548375 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #548375 Rosetta import empty strings (e.g. "") from .po files
  https://bugs.launchpad.net/bugs/548375


= Bug #548375 =

Ignore empty translations on import (always).  We've also considered
keeping the ability to revert translations with non-published uploads,
but it would be more work for dubious benefit.  For now, what users
want is for imports to simply not introduce empty messages. We get that
here.

Most of the changes are cleanups and lint fixes.  Basically, only the change 

@@ -449,7 +449,9 @@ class FileImporter(object):
             # store English strings in an IPOFile.
             return None
 
-        if not message.translations:
+        if (not message.translations or
+            set(message.translations) == set([u'']) or
+            set(message.translations) == set([None])):
             # We don't have anything to import.
             return None
 

is important here.  Since message.translations is never going to be
more than 6 items, I didn't bother optimizing this (i.e. do a
set(message.translations) once and use that in further comparisons).

The tests are split into two methods so it's clearer what we do for
both cases (I've first written tests which assert previous behavior,
then unified the code using a helper private method).

== Tests ==

bin/test -cvvt storeTranslationsInDatabase_empty

== Demo and Q/A ==

Export a PO file and re-import it as "updated translation" and watch for empty messages not being created.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/utilities/tests/test_translation_importer.py
  lib/lp/translations/utilities/tests/test_file_importer.py
  lib/lp/translations/utilities/translation_import.py
-- 
https://code.launchpad.net/~danilo/launchpad/bug-548375/+merge/34544
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-548375 into lp:launchpad/devel.

=== modified file 'lib/lp/translations/utilities/tests/test_file_importer.py'
--- lib/lp/translations/utilities/tests/test_file_importer.py	2010-08-29 19:55:16 +0000
+++ lib/lp/translations/utilities/tests/test_file_importer.py	2010-09-03 14:04:45 +0000
@@ -20,6 +20,8 @@
     ITranslationImportQueue,
     )
 from lp.translations.utilities.gettext_po_importer import GettextPOImporter
+from lp.translations.utilities.translation_common_format import (
+    TranslationMessageData)
 from lp.translations.utilities.translation_import import (
     FileImporter,
     POFileImporter,
@@ -217,6 +219,34 @@
             "FileImporter.getOrCreatePOTMessageSet did not get an existing "
             "IPOTMsgSet object from the database.")
 
+    def _test_storeTranslationsInDatabase_empty(self, is_published=True):
+        """Check whether we store empty messages appropriately."""
+        # Construct a POFile importer.
+        pot_importer = self._createPOTFileImporter(
+            TEST_TEMPLATE_EXPORTED, is_published=True)
+        importer = self._createPOFileImporter(
+            pot_importer, TEST_TRANSLATION_EXPORTED, is_published=True,
+            person=self.importer_person)
+
+        # Empty message to import.
+        message = TranslationMessageData()
+        message.addTranslation(0, u'')
+
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate = importer.potemplate, sequence=50)
+        translation = importer.storeTranslationsInDatabase(
+            message, potmsgset)
+        # No TranslationMessage is created.
+        self.assertIs(None, translation)
+
+    def test_storeTranslationsInDatabase_empty_imported(self):
+        """Storing empty messages for published imports appropriately."""
+        self._test_storeTranslationsInDatabase_empty(is_published=True)
+
+    def test_storeTranslationsInDatabase_empty_user(self):
+        """Store empty messages for user uploads appropriately."""
+        self._test_storeTranslationsInDatabase_empty(is_published=False)
+
     def test_FileImporter_storeTranslationsInDatabase_privileges(self):
         """Test `storeTranslationsInDatabase` privileges."""
 

=== modified file 'lib/lp/translations/utilities/tests/test_translation_importer.py'
--- lib/lp/translations/utilities/tests/test_translation_importer.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/utilities/tests/test_translation_importer.py	2010-09-03 14:04:45 +0000
@@ -5,13 +5,12 @@
 
 __metaclass__ = type
 
-import unittest
-
 from zope.component import getUtility
 from zope.interface.verify import verifyObject
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.registry.interfaces.product import IProductSet
+from lp.testing import TestCaseWithFactory
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat,
@@ -29,11 +28,12 @@
     )
 
 
-class TranslationImporterTestCase(unittest.TestCase):
+class TranslationImporterTestCase(TestCaseWithFactory):
     """Class test for translation importer component"""
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
+        super(TranslationImporterTestCase, self).setUp()
         # Add a new entry for testing purposes. It's a template one.
         productset = getUtility(IProductSet)
         evolution = productset.getByName('evolution')
@@ -246,10 +246,3 @@
         msg2._translations = ["le foo", "les foos", "beaucoup des foos", None]
         self.assertTrue(is_identical_translation(msg1, msg2),
             "Identical multi-form messages not accepted as identical.")
-
-
-def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest(unittest.makeSuite(TranslationImporterTestCase))
-    return suite
-

=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py	2010-08-24 10:45:57 +0000
+++ lib/lp/translations/utilities/translation_import.py	2010-09-03 14:04:45 +0000
@@ -11,7 +11,6 @@
 
 import datetime
 from operator import attrgetter
-import os
 
 import gettextpo
 import posixpath
@@ -282,6 +281,7 @@
         else:
             return False
 
+
 class TranslationImporter:
     """Handle translation resources imports."""
 
@@ -345,13 +345,13 @@
     def importFile(self, translation_import_queue_entry, logger=None):
         """See ITranslationImporter."""
         assert translation_import_queue_entry is not None, (
-            "The translation import queue entry cannot be None.")
+            "Import queue entry cannot be None.")
         assert (translation_import_queue_entry.status ==
                 RosettaImportStatus.APPROVED), (
-                "The entry is not approved!.")
+            "Import queue entry is not approved.")
         assert (translation_import_queue_entry.potemplate is not None or
                 translation_import_queue_entry.pofile is not None), (
-                "The entry has not any import target.")
+            "Import queue entry has no import target.")
 
         importer = self.getTranslationFormatImporter(
             translation_import_queue_entry.format)
@@ -438,7 +438,7 @@
         is added to the list in self.errors but the translations are stored
         anyway, marked as having an error.
 
-        :param message: The message who's translations will be stored.
+        :param message: The message for which translations will be stored.
         :param potmsgset: The POTMsgSet that this message belongs to.
 
         :return: The updated translation_message entry or None, if no storing
@@ -449,7 +449,9 @@
             # store English strings in an IPOFile.
             return None
 
-        if not message.translations:
+        if (not message.translations or
+            set(message.translations) == set([u'']) or
+            set(message.translations) == set([None])):
             # We don't have anything to import.
             return None
 
@@ -488,7 +490,6 @@
                             potmsgset.id)
                 return None
 
-
         just_replaced_msgid = (
             self.importer.uses_source_string_msgids and
             self.pofile.language.code == 'en')
@@ -548,7 +549,6 @@
                         self.translation_import_queue_entry.format)
         return self._cached_format_exporter
 
-
     def _addUpdateError(self, message, potmsgset, errormsg):
         """Add an error returned by updateTranslation.
 
@@ -567,7 +567,7 @@
             'pofile': self.pofile,
             'pomessage': self.format_exporter.exportTranslationMessageData(
                 message),
-            'error-message': unicode(errormsg)
+            'error-message': unicode(errormsg),
         })
 
     def _addConflictError(self, message, potmsgset):