launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/db-devel-690196-singular-text into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #690196 POTMsgSet.singular_text may need to be a method
  https://bugs.launchpad.net/bugs/690196

For more details, see:
https://code.launchpad.net/~henninge/launchpad/db-devel-690196-singular-text/+merge/44911

= Summary =

POTMsgSet.singular_text looks up English msgids in an "en" POFile for
symbolic msgids in XPI files. With the new model, it has to pick a
side to read the current translation from. Currently it first tries
Ubuntu, then upstream.

== Proposed fix ==

Make singular text look-up the msgid from the upstream side because
that is where it will be coming from. Imports on the Ubuntu side are
marked as "by_maintainer" anyway, so these will becom upstream
translations, too.

As a fallback, the Ubuntu translation (if any) can be used but that
should really only happen with old or broken data.

== Pre-implementation notes ==

Danilo and I decided on using upstream side. I decided on the
fall-back myself, so maybe that is not a good idea. ;)

== Implementation details ==

The test looked like it was a converted doc test. I split it up and
put it into its own TestCase because its setup is quite different
from the rest. The last two tests, which I added for this bug, got a
fairly complex setup because the right sharing situation had to be
created.

== Tests ==

bin/test -vvcm lp.translations.tests.test_potmsgset \
  -t TestPOTMsgSetText

== Demo and Q/A ==

Checking mozilla templates still look sane.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_potmsgset.py
  lib/lp/translations/model/potmsgset.py
-- 
https://code.launchpad.net/~henninge/launchpad/db-devel-690196-singular-text/+merge/44911
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/db-devel-690196-singular-text into lp:launchpad/db-devel.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-12-17 10:30:55 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-12-30 17:03:00 +0000
@@ -246,16 +246,14 @@
             return self._cached_singular_text
 
         # Singular text is stored as an "English translation." Search on
-        # both sides.
-        # XXX henninge 2010-12-14, bug=690196 This may still need some
-        # re-thinking. Maybe even add "getAnyCurrentTranslation".
+        # both sides but prefer upstream translations.
         translation_message = self.getCurrentTranslation(
             None, getUtility(ILaunchpadCelebrities).english,
-            TranslationSide.UBUNTU)
+            TranslationSide.UPSTREAM)
         if translation_message is None:
             translation_message = self.getCurrentTranslation(
                 None, getUtility(ILaunchpadCelebrities).english,
-                TranslationSide.UPSTREAM)
+                TranslationSide.UBUNTU)
         if translation_message is not None:
             msgstr0 = translation_message.msgstr0
             if msgstr0 is not None:

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-12-17 10:30:55 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-12-30 17:03:00 +0000
@@ -133,43 +133,6 @@
         transaction.commit()
         self.assertFalse(self.potmsgset.uses_english_msgids)
 
-    def test_POTMsgSet_singular_text(self):
-        """Test that `singular_text` property works correctly."""
-
-        BASE_STRING = u"Base string"
-        ENGLISH_STRING = u"English string"
-        DIVERGED_ENGLISH_STRING = u"Diverged English string"
-
-        # We create a POTMsgSet with a base English string.
-        potmsgset = self.factory.makePOTMsgSet(self.devel_potemplate,
-                                               BASE_STRING)
-        potmsgset.setSequence(self.devel_potemplate, 2)
-
-        # Gettext PO format uses English strings as msgids.
-        self.devel_potemplate.source_file_format = TranslationFileFormat.PO
-        transaction.commit()
-        self.assertEquals(potmsgset.singular_text, BASE_STRING)
-
-        # Mozilla XPI format doesn't use English strings as msgids,
-        # unless there is no English POFile object.
-        self.devel_potemplate.source_file_format = TranslationFileFormat.XPI
-        transaction.commit()
-        self.assertEquals(potmsgset.singular_text, BASE_STRING)
-
-        # POTMsgSet singular_text is read from a shared English translation.
-        en_pofile = self.factory.makePOFile('en', self.devel_potemplate)
-        translation = self.factory.makeSharedTranslationMessage(
-            pofile=en_pofile, potmsgset=potmsgset,
-            translations=[ENGLISH_STRING])
-        self.assertEquals(potmsgset.singular_text, ENGLISH_STRING)
-
-        # A diverged (translation.potemplate != None) English translation
-        # is not used as a singular_text.
-        translation = self.factory.makeCurrentTranslationMessage(
-            pofile=en_pofile, potmsgset=potmsgset,
-            translations=[DIVERGED_ENGLISH_STRING], diverged=True)
-        self.assertEquals(potmsgset.singular_text, ENGLISH_STRING)
-
     def test_getCurrentTranslationMessageOrDummy_returns_upstream_tm(self):
         pofile = self.factory.makePOFile('nl')
         message = self.factory.makeCurrentTranslationMessage(pofile=pofile)
@@ -1139,6 +1102,141 @@
             self.pofile, now - timedelta(1))
 
 
+class TestPOTMsgSetText(TestCaseWithFactory):
+    """Tests for singular_text."""
+
+    layer = ZopelessDatabaseLayer
+
+    def _makePOTMsgSetAndTemplate(self, msgid_text, format,
+                                  productseries=None):
+        """Create a POTMsgSet in a template of the given format.
+
+        :returns: A tuple (POTMsgSet, POTemplate).
+        """
+        potemplate = self.factory.makePOTemplate(productseries=productseries)
+        potemplate.source_file_format = format
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate, msgid_text, sequence=1)
+        return (potmsgset, potemplate)
+
+    def _makePOTMsgSet(self, msgid_text, format, productseries=None):
+        """Create a POTMsgSet in a template of the given format.
+
+        :returns: A POTMsgSet.
+        """
+        return self._makePOTMsgSetAndTemplate(
+            msgid_text, format, productseries)[0]
+
+    def test_singular_text_po(self):
+        # Gettext PO format uses English strings as msgids.
+        english_msgid = self.factory.getUniqueString()
+        potmsgset = self._makePOTMsgSet(
+            english_msgid, TranslationFileFormat.PO)
+        self.assertEquals(english_msgid, potmsgset.singular_text)
+
+    def test_singular_text_xpi(self):
+        # Mozilla XPI format uses English strings as msgids if no English
+        # pofile exists.
+        symbolic_msgid = self.factory.getUniqueString()
+        potmsgset = self._makePOTMsgSet(
+            symbolic_msgid, TranslationFileFormat.XPI)
+        self.assertEquals(symbolic_msgid, potmsgset.singular_text)
+
+    def test_singular_text_xpi_english(self):
+        # Mozilla XPI format uses English strings as msgids if no English
+        # pofile exists.
+        # POTMsgSet singular_text is read from a shared English translation.
+        symbolic_msgid = self.factory.getUniqueString()
+        english_msgid = self.factory.getUniqueString()
+        potmsgset, potemplate = self._makePOTMsgSetAndTemplate(
+            symbolic_msgid, TranslationFileFormat.XPI)
+        en_pofile = self.factory.makePOFile('en', potemplate)
+        self.factory.makeCurrentTranslationMessage(
+            pofile=en_pofile, potmsgset=potmsgset,
+            translations=[english_msgid])
+
+        self.assertEquals(english_msgid, potmsgset.singular_text)
+
+    def test_singular_text_xpi_english_diverged(self):
+        # A diverged (translation.potemplate != None) English translation
+        # is not used as a singular_text.
+        symbolic_msgid = self.factory.getUniqueString()
+        english_msgid = self.factory.getUniqueString()
+        diverged_msgid = self.factory.getUniqueString()
+        potmsgset, potemplate = self._makePOTMsgSetAndTemplate(
+            symbolic_msgid, TranslationFileFormat.XPI)
+        en_pofile = self.factory.makePOFile('en', potemplate)
+        self.factory.makeCurrentTranslationMessage(
+            pofile=en_pofile, potmsgset=potmsgset,
+            translations=[english_msgid])
+        self.factory.makeCurrentTranslationMessage(
+            pofile=en_pofile, potmsgset=potmsgset,
+            translations=[diverged_msgid], diverged=True)
+
+        self.assertEquals(english_msgid, potmsgset.singular_text)
+
+    def _setUpSharingWithUbuntu(self):
+        """Create a potmsgset shared in upstream and Ubuntu."""
+        productseries = self.factory.makeProductSeries()
+
+        # Create the source package that this product is linked to.
+        distroseries = self.factory.makeUbuntuDistroSeries()
+        distroseries.distribution.translation_focus = distroseries
+        sourcepackagename = self.factory.makeSourcePackageName()
+        sourcepackage = self.factory.makeSourcePackage(
+            distroseries=distroseries, sourcepackagename=sourcepackagename)
+        sourcepackage.setPackaging(productseries, self.factory.makePerson())
+
+        # Create two sharing templates.
+        self.potmsgset, upstream_potemplate = self._makePOTMsgSetAndTemplate(
+            None, TranslationFileFormat.XPI, productseries)
+        ubuntu_potemplate = self.factory.makePOTemplate(
+            distroseries=distroseries, sourcepackagename=sourcepackagename,
+            name=upstream_potemplate.name)
+        ubuntu_potemplate.source_file_format = TranslationFileFormat.XPI
+        self.potmsgset.setSequence(ubuntu_potemplate, 1)
+
+        # The pofile is automatically created for all sharing templates.
+        self.upstream_pofile = self.factory.makePOFile(
+            'en', upstream_potemplate, create_sharing=True)
+        self.ubuntu_pofile = ubuntu_potemplate.getPOFileByLang('en')
+        self.assertIsNot(None, self.ubuntu_pofile)
+
+    def test_singular_text_xpi_english_uses_upstream(self):
+        # POTMsgSet singular_text is read from the upstream English
+        # translation.
+        self._setUpSharingWithUbuntu()
+        # Create different "English translations" for this potmsgset.
+        ubuntu_msgid = self.factory.getUniqueString()
+        upstream_msgid = self.factory.getUniqueString()
+
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.upstream_pofile, potmsgset=self.potmsgset,
+            translations=[upstream_msgid])
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.ubuntu_pofile, potmsgset=self.potmsgset,
+            translations=[ubuntu_msgid])
+
+        removeSecurityProxy(self.potmsgset)._cached_singular_text = None
+        self.assertEquals(upstream_msgid, self.potmsgset.singular_text)
+
+    def test_singular_text_xpi_english_falls_back_to_ubuntu(self):
+        # POTMsgSet singular_text is read from the Ubuntu English
+        # translation if no upstream one exists. This is a safeguard against
+        # old or broken data.
+        self._setUpSharingWithUbuntu()
+
+        # Create different "English translations" for this potmsgset.
+        ubuntu_msgid = self.factory.getUniqueString()
+
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.ubuntu_pofile, potmsgset=self.potmsgset,
+            translations=[ubuntu_msgid])
+
+        removeSecurityProxy(self.potmsgset)._cached_singular_text = None
+        self.assertEquals(ubuntu_msgid, self.potmsgset.singular_text)
+
+
 class TestPOTMsgSetCornerCases(TestCaseWithFactory):
     """Test corner cases and constraints."""