launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-702832 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #702832 Translation approval stumbles over divergence
  https://bugs.launchpad.net/bugs/702832

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-702832/+merge/46261

= Bug 702832 =

Fixes a relatively rare oops in the "Recife" Translations code that was written over the past year and finally rolled out last cycle.  A certain piece of data used to be structured as either a list of a message's translations for each of the language's plural forms, or a dict mapping the plural forms to their corresponding translations.  Duck typing still made things work pretty well when a list was passed. One call site still passed a list, and the docstring still, mistakenly, asked for one.

The incorrect type only caused trouble when the data was then passed on to yet another method that did expect a dict.  For invocations from the call site that made this mistake, that last method was only called in a rare scenario.  That's how this managed to escape both unit tests (which passed the right type) and integration tests (which didn't tickle this particular, rare combination of calls).

Run this test to verify that the specific bug is gone:
{{{
./bin/test -vvc lp.translations.tests.test_potmsgset -t clones
}}}

I also removed an assertion that seems a bit silly given later changes: it checks that a required parameter is not None, but it's pretty much always going to be either a constant or a not-null database value.  That also eliminated a now-pointless test.

For Q/A, we should see that approval of existing suggestions and translations still works.


No lint,

Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-702832/+merge/46261
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-702832 into lp:launchpad.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2011-01-05 20:44:50 +0000
+++ lib/lp/translations/model/potmsgset.py	2011-01-14 14:17:57 +0000
@@ -344,7 +344,6 @@
 
     def getCurrentTranslation(self, potemplate, language, side):
         """See `IPOTMsgSet`."""
-        assert side is not None, "Translation side must be specified."
         traits = getUtility(ITranslationSideTraitsSet).getTraits(side)
         flag = removeSecurityProxy(traits.getFlag(TranslationMessage))
 
@@ -1152,7 +1151,7 @@
             return
 
         translator = suggestion.submitter
-        potranslations = suggestion.all_msgstrs
+        potranslations = dictify_translations(suggestion.all_msgstrs)
         activated_message = self._setTranslation(
             pofile, translator, suggestion.origin, potranslations,
             share_with_other_side=share_with_other_side,
@@ -1250,8 +1249,8 @@
         :param pofile: The `POFile` to set the translation in.
         :param submitter: The `Person` who produced this translation.
         :param origin: The translation's `RosettaTranslationOrigin`.
-        :param potranslations: List of `POTranslation`s, with a None for
-            each untranslated plural-form.
+        :param potranslations: A dict mapping plural-form numbers to the
+            respective `POTranslation`s for those forms.
         :param identical_message: The already existing message, if any,
             that's either shared or diverged for `pofile.potemplate`,
             whose translations are identical to the ones we're setting.

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2011-01-04 17:23:42 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2011-01-14 14:17:57 +0000
@@ -1906,13 +1906,6 @@
             pofile.potemplate, pofile.language, self.this_side)
         self.assertEqual(message, current)
 
-    def test_requires_side(self):
-        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
-        self.assertRaises(
-            AssertionError,
-            potmsgset.getCurrentTranslation,
-            pofile.potemplate, pofile.language, None)
-
     def test_other_languages_ignored(self):
         # getCurrentTranslation never returns a translation for another
         # language.

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2010-12-10 11:44:07 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2011-01-14 14:17:57 +0000
@@ -18,7 +18,10 @@
 from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.testing import TestCaseWithFactory
-from lp.translations.interfaces.side import ITranslationSideTraitsSet
+from lp.translations.interfaces.side import (
+    ITranslationSideTraitsSet,
+    TranslationSide,
+    )
 from lp.translations.interfaces.translationmessage import (
     ITranslationMessage,
     TranslationConflict,
@@ -268,6 +271,28 @@
             suggestion.approve,
             pofile, self.factory.makePerson(), lock_timestamp=old)
 
+    def test_approve_clones_message_from_other_side_to_diverge(self):
+        package = self.factory.makeSourcePackage()
+        template=self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        potmsgset = self.factory.makePOTMsgSet(potemplate=template)
+        upstream_pofile = self.factory.makePOFile('nl')
+        ubuntu_pofile = self.factory.makePOFile('nl', potemplate=template)
+        diverged_tm = self.factory.makeDivergedTranslationMessage(
+            pofile=upstream_pofile, potmsgset=potmsgset)
+        ubuntu_tm = self.factory.makeSuggestion(
+            pofile=ubuntu_pofile, potmsgset=potmsgset)
+        ubuntu_tm.is_current_ubuntu = True
+
+        ubuntu_tm.approve(upstream_pofile, self.factory.makePerson())
+
+        upstream_tm = potmsgset.getCurrentTranslation(
+            upstream_pofile.potemplate, upstream_pofile.language,
+            TranslationSide.UPSTREAM)
+        self.assertEqual(ubuntu_tm.all_msgstrs, upstream_tm.all_msgstrs)
+        self.assertNotEqual(ubuntu_tm, upstream_tm)
+
 
 class TestApproveAsDiverged(TestCaseWithFactory):
     """Tests for `TranslationMessage.approveAsDiverged`."""