Merge lp:~adeuring/launchpad/bug-688130 into lp:launchpad

[Abel Deuring]

[r=allenap][bug=688130,720199] ensure that factory.makeCurrentTranslation fails if a POTemplate and a POTMsgSet are specified which are not linked by TranslationTemplateItem; do not count current translations which are diverged for the other side as suggestions for 'this' side in another context than the diverged target; tests to ensure that diverged translations in general are not counted as translations or suggestions outside the diverged context; removed a strange and not-working attempt to round floating point numbers to two decimal digits in rosettastats. Consider POTMsgSets which have a plural form but which miss at least one of the plural/singluar variants as untranslated in POTMsgSet._countTranslations()

This branch fixes bug 688130: "Statistics clean-ups and extra tests".

Many translation statistics are calculated in POFile.updateStatistics(), which uses the two methods POFile._countTranslations() and POFile._countNewSuggestions().

These methods did not treat empty translations properly and translations which are only partially translated, i.e. those which required singlar and plural variants.

Jeroen recently fixed the case for empty translations, leaving only the partial translations: If a message is not completely translated, we want to count it as untranslated.

The SQL query in _countTranslations() now uses different expressions for the result column "has_other_msgstrs" and for the WHERE clause "has_msgstrs" to ensure that only TranslationMessage records are counted which have all required plural forms.

_countSuggestions() included translations which are diverged for another target than the POFile/POTemplate for which _countSuggestions() was called; this is fixed too.

As a drive-by fix, I added an "assert" to factory.makeCurrentTranslationMessage(): It allowed to create a translation being diverged for a POTemplate that does not contain the POTMsgSet, i.e., where no TranslationTemplateItem record exists that links the POTMsgSet with the POTemplate.

As another drive-by fix I removed a silly attempt to round a floating point number to two decimal digits from RosettaStats.asPercentage(). There are no tests for this change: The method could not round the result for example of 33/10 anyway (and of most other numbers which, represented as fraction of two integers, have a denominator with any prime factor other than two), so we either already have formatting issues or the callsites already format the values using something like '%3.2f' % (33.0/10) (bug 720199).

test: ./bin/test -vvt test_pofile

no lint

[Gavin Panella (allenap)]

Looks good to me.

[1]

+ tti_for_message_in_template = TranslationTemplateItem.selectOneBy(
+ potmsgset=potmsgset, potemplate=pofile.potemplate)

I think we should try to avoid the SQLObject shim. How about:

    IStore(TranslationTemplateItem).find(
        TranslationTemplateItem.potmsgset == potmsgset,
        TranslationTemplateItem.potemplate == pofile.potemplate).one()

[2]

+ def test_POFile_updateStatistics_diverged_message_this_side(self):
+ # Translations that are diverged for a given target do not
+ # appear in the statistical data for another target.
...
+ def test_POFile_updateStatistics_diverged_message_other_side(self):
+ # Translations that are diverged for a given target do not
+ # appear in the statistical data for another target.

These have the same explanation. Is there a difference?

[3]

I can't pretend to know enough about Translations to be able to do a
really thorough review of this, so if you feel less than confident
about this branch perhaps you should ask a domain expert to go over it
too.

review: Approve

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-02-22 16:56:05 +0000
+++ lib/lp/testing/factory.py	2011-02-25 15:04:52 +0000
@@ -294,6 +294,9 @@
 from lp.translations.model.translationimportqueue import (
     TranslationImportQueueEntry,
     )
+from lp.translations.model.translationtemplateitem import (
+    TranslationTemplateItem,
+    )
 from lp.translations.utilities.sanitize import (
     sanitize_translations_from_webui,
     )
@@ -2841,6 +2844,8 @@
             'Cannot specify both language and pofile.')
         assert None in (side, pofile), (
             'Cannot specify both side and pofile.')
+        link_potmsgset_with_potemplate = (
+            (pofile is None and potemplate is None) or potmsgset is None)
         if pofile is None:
             pofile = self.makePOFile(
                 language=language, side=side, potemplate=potemplate)
@@ -2850,6 +2855,19 @@
         potemplate = pofile.potemplate
         if potmsgset is None:
             potmsgset = self.makePOTMsgSet(potemplate)
+        if link_potmsgset_with_potemplate:
+            # If we have a new pofile or a new potmsgset, we must link
+            # the potmsgset to the pofile's potemplate.
+            potmsgset.setSequence(
+                pofile.potemplate, self.getUniqueInteger())
+        else:
+            # Otherwise it is the duty of the callsite to ensure
+            # consistency.
+            store = IStore(TranslationTemplateItem)
+            tti_for_message_in_template = store.find(
+                TranslationTemplateItem.potmsgset == potmsgset,
+                TranslationTemplateItem.potemplate == pofile.potemplate).any()
+            assert tti_for_message_in_template is not None
         if translator is None:
             translator = self.makePerson()
         if reviewer is None:

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2011-02-22 01:57:30 +0000
+++ lib/lp/translations/model/pofile.py	2011-02-25 15:04:52 +0000
@@ -844,15 +844,19 @@
 
         side_traits = getUtility(ITranslationSideTraitsSet).getForTemplate(
             self.potemplate)
+        complete_plural_clause_this_side = ' AND '.join(
+            self._appendCompletePluralFormsConditions(
+                [], table_name='Current'))
+        complete_plural_clause_other_side = ' AND '.join(
+            self._appendCompletePluralFormsConditions(
+                [], table_name='Other'))
         params = {
             'potemplate': quote(self.potemplate),
             'language': quote(self.language),
             'flag': side_traits.flag_name,
             'other_flag': side_traits.other_side_traits.flag_name,
-            'has_msgstrs':
-                compose_sql_translationmessage_has_translations("Current"),
-            'has_other_msgstrs':
-                compose_sql_translationmessage_has_translations("Other"),
+            'has_msgstrs': complete_plural_clause_this_side,
+            'has_other_msgstrs': complete_plural_clause_other_side,
         }
         # The "distinct on" combined with the "order by potemplate nulls
         # last" makes diverged messages mask their shared equivalents.
@@ -864,6 +868,7 @@
                     %(has_other_msgstrs)s AS has_other_msgstrs,
                     (Other.id = Current.id) AS same_on_both_sides
                 FROM TranslationTemplateItem AS TTI
+                JOIN POTMsgSet ON POTMsgSet.id = TTI.potmsgset
                 JOIN TranslationMessage AS Current ON
                     Current.potmsgset = TTI.potmsgset AND
                     Current.language = %(language)s AND
@@ -950,7 +955,9 @@
                             Suggestion.date_created > COALESCE(
                                 Current.date_reviewed,
                                 Current.date_created,
-                                TIMESTAMP 'epoch')
+                                TIMESTAMP 'epoch') AND
+                            COALESCE(Suggestion.potemplate, %(potemplate)s) =
+                                %(potemplate)s
                     )
                 ORDER BY TTI.potmsgset, Current.potemplate NULLS LAST
             ) AS messages_with_suggestions

=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py	2011-02-22 01:57:30 +0000
+++ lib/lp/translations/tests/test_pofile.py	2011-02-25 15:04:52 +0000
@@ -1848,6 +1848,35 @@
         self.assertEqual(0, self.pofile.newCount())
         self.assertEqual(0, self.pofile.updatesCount())
 
+    def test_partial_translations_count_as_untranslated(self):
+        # A translation requiring plural forms is considered to be
+        # untranslated if at least one plural translation required
+        # for the given language is missing.
+        plural_potmsgset = self.factory.makePOTMsgSet(
+            self.potemplate, singular='singular-en', plural='plural-en')
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-singular', 'sr-plural-1'])
+        self.pofile.updateStatistics()
+        self.assertEqual(0, self.pofile.translatedCount())
+        self.assertEqual(2, self.pofile.untranslatedCount())
+        self.assertEqual(0, self.pofile.newCount())
+        self.assertEqual(0, self.pofile.updatesCount())
+
+    def test_complete_translations_count_as_translated(self):
+        # A translation requiring plural forms is considered to be
+        # translated if all variants are translated.
+        plural_potmsgset = self.factory.makePOTMsgSet(
+            self.potemplate, singular='singular-en', plural='plural-en')
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-singular', 'sr-plural-1', 'sr-plural-2'])
+        self.pofile.updateStatistics()
+        self.assertEqual(1, self.pofile.translatedCount())
+        self.assertEqual(1, self.pofile.untranslatedCount())
+        self.assertEqual(1, self.pofile.newCount())
+        self.assertEqual(0, self.pofile.updatesCount())
+
     def test_empty_messages_on_this_side_count_as_untranslated(self):
         # A POTMsgSet whose current TranslationMessage on this side is
         # empty is counted only as untranslated, regardless of any
@@ -1863,6 +1892,25 @@
         self.assertEqual(0, self.pofile.newCount())
         self.assertEqual(0, self.pofile.updatesCount())
 
+    def test_partial_messages_on_this_side_count_as_untranslated(self):
+        # A POTMsgSet whose current TranslationMessage on this side is
+        # partially translated is considered to be untranslated, regardless
+        # of any translations it may have on the other side.
+        plural_potmsgset = self.factory.makePOTMsgSet(
+            self.potemplate, singular='singular-en', plural='plural-en')
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-singular', 'sr-plural-1'])
+        other_message = self.factory.makeSuggestion(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-ubuntu', 'sr-ubuntu-2', 'sr-ubuntu-3'])
+        other_message.is_current_ubuntu = True
+        self.pofile.updateStatistics()
+        self.assertEqual(0, self.pofile.translatedCount())
+        self.assertEqual(2, self.pofile.untranslatedCount())
+        self.assertEqual(0, self.pofile.newCount())
+        self.assertEqual(0, self.pofile.updatesCount())
+
     def test_empty_messages_on_other_side_count_as_untranslated(self):
         # A POTMsgSet that's translated on this side but has an empty
         # translation on the other side counts as translated on this
@@ -1882,6 +1930,29 @@
         self.assertEqual(0, self.pofile.updatesCount())
         self.assertEqual(0, self.pofile.currentCount())
 
+    def test_partial_messages_on_other_side_count_as_untranslated(self):
+        # A POTMsgSet that's translated on this side and has a different
+        # partial translation on the other side counts as translated on
+        # this side, but not equal between both sides (currentCount) or
+        # translated differently between the two sides (updatesCount).
+        # Instead, it's counted as translated on this side but not on
+        # the other (newCount).
+        plural_potmsgset = self.factory.makePOTMsgSet(
+            self.potemplate, singular='singular-en', plural='plural-en')
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-singular', 'sr-plural1', 'sr-plural2'])
+        other_message = self.factory.makeSuggestion(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-ubuntu'])
+        other_message.is_current_ubuntu = True
+        self.pofile.updateStatistics()
+        self.assertEqual(1, self.pofile.translatedCount())
+        self.assertEqual(1, self.pofile.untranslatedCount())
+        self.assertEqual(1, self.pofile.newCount())
+        self.assertEqual(0, self.pofile.updatesCount())
+        self.assertEqual(0, self.pofile.currentCount())
+
     def test_tracking_empty_messages_count_as_untranslated(self):
         # An empty TranslationMessage that's current on both sides
         # counts as untranslated.
@@ -1895,6 +1966,59 @@
         self.assertEqual(0, self.pofile.updatesCount())
         self.assertEqual(0, self.pofile.currentCount())
 
+    def test_tracking_partial_translations_count_as_untranslated(self):
+        # A partial Translations that's current on both sides
+        # counts as untranslated.
+        plural_potmsgset = self.factory.makePOTMsgSet(
+            self.potemplate, singular='singular-en', plural='plural-en')
+        self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=plural_potmsgset,
+            translations=['sr-singular', 'sr-plural1'], current_other=True)
+        self.pofile.updateStatistics()
+        self.assertEqual(0, self.pofile.translatedCount())
+        self.assertEqual(2, self.pofile.untranslatedCount())
+        self.assertEqual(0, self.pofile.newCount())
+        self.assertEqual(0, self.pofile.updatesCount())
+        self.assertEqual(0, self.pofile.currentCount())
+
+    def makeDivergedTranslationForOtherTarget(self, for_sourcepackage):
+        """Create a translation message that is diverged for another target.
+        """
+        if for_sourcepackage:
+            ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+            distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
+            sourcepackage = self.factory.makeSourcePackage(
+                distroseries=distroseries)
+            sourcepackagename = sourcepackage.sourcepackagename
+        else:
+            distroseries = None
+            sourcepackagename = None
+        other_potemplate = self.factory.makePOTemplate(
+            distroseries=distroseries, sourcepackagename=sourcepackagename)
+        other_pofile = self.factory.makePOFile(
+            language_code=self.pofile.language.code,
+            potemplate=other_potemplate)
+        self.potmsgset.setSequence(
+            other_potemplate, self.factory.getUniqueInteger())
+        self.factory.makeCurrentTranslationMessage(
+            pofile=other_pofile, potmsgset=self.potmsgset, diverged=True)
+
+    def test_POFile_updateStatistics_diverged_message_this_side(self):
+        # Translations that are diverged for a given target on this side
+        # do not appear in the statistical data for another target.
+        self.makeDivergedTranslationForOtherTarget(for_sourcepackage=False)
+        self.pofile.updateStatistics()
+        self.assertEqual(self.pofile.rosettaCount(), 0)
+        self.assertEqual(self.pofile.unreviewedCount(), 0)
+
+    def test_POFile_updateStatistics_diverged_message_other_side(self):
+        # Translations that are diverged for a given target on the other side
+        # do not appear in the statistical data for another target.
+        self.makeDivergedTranslationForOtherTarget(for_sourcepackage=True)
+        self.pofile.updateStatistics()
+        self.assertEqual(self.pofile.rosettaCount(), 0)
+        self.assertEqual(self.pofile.unreviewedCount(), 0)
+
 
 class TestPOFile(TestCaseWithFactory):
     """Test PO file methods."""
@@ -2222,6 +2346,8 @@
     def test_getTranslationMessages_other_pofile(self):
         # A message from another POFiles is not included.
         other_pofile = self.factory.makePOFile('de')
+        self.potmsgset.setSequence(
+            other_pofile.potemplate, self.factory.getUniqueInteger())
         self.factory.makeCurrentTranslationMessage(
             potmsgset=self.potmsgset, pofile=other_pofile)
 
@@ -2253,6 +2379,8 @@
         # A message matching given condition but located in another POFile
         # is not included.
         other_pofile = self.factory.makePOFile('de')
+        self.potmsgset.setSequence(
+            other_pofile.potemplate, self.factory.getUniqueInteger())
         self.factory.makeCurrentTranslationMessage(
             potmsgset=self.potmsgset, pofile=other_pofile,
             diverged=True)

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2011-02-22 21:44:23 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2011-02-25 15:04:52 +0000
@@ -920,6 +920,8 @@
 
         message_this = self.factory.makeCurrentTranslationMessage(
             pofile=self.pofile, potmsgset=self.potmsgset)
+        self.potmsgset.setSequence(
+            other_potemplate, self.factory.getUniqueInteger())
         message_other = self.factory.makeCurrentTranslationMessage(
             pofile=other_pofile, potmsgset=self.potmsgset)
         traits = getUtility(ITranslationSideTraitsSet).getTraits(
@@ -1323,6 +1325,7 @@
             productseries=series2, name=pofile.potemplate.name)
         pofile2 = template2.getPOFileByLang(pofile.language.code)
         translation = {0: self.factory.getUniqueString()}
+        potmsgset.setSequence(template2, self.factory.getUniqueInteger())
         diverged_message = self.factory.makeCurrentTranslationMessage(
             pofile=pofile2, potmsgset=potmsgset, translator=owner,
             translations=translation, diverged=True)
@@ -1771,6 +1774,8 @@
         diverged = self.factory.makeDivergedTranslationMessage(
             pofile=pofile, potmsgset=potmsgset,
             translations=translations)
+        potmsgset.setSequence(
+            other_pofile.potemplate, self.factory.getUniqueInteger())
         shared = self.factory.makeCurrentTranslationMessage(
             pofile=other_pofile, potmsgset=potmsgset,
             translations=translations)
@@ -1839,6 +1844,8 @@
         other_pofile = self.factory.makePOFile(pofile.language.code)
         potmsgset.setSequence(pofile.potemplate, 1)
 
+        potmsgset.setSequence(
+            other_pofile.potemplate, self.factory.getUniqueInteger())
         self.factory.makeCurrentTranslationMessage(
             pofile=other_pofile, potmsgset=potmsgset,
             translations=translations, diverged=True)

=== modified file 'lib/lp/translations/utilities/rosettastats.py'
--- lib/lp/translations/utilities/rosettastats.py	2010-02-08 23:41:43 +0000
+++ lib/lp/translations/utilities/rosettastats.py	2011-02-25 15:04:52 +0000
@@ -80,16 +80,9 @@
         if self.messageCount() > 0:
             percent = float(value) / self.messageCount()
             percent *= 100
-            percent = round(percent, 2)
         else:
             percent = 0
-        # We use float(str()) to prevent problems with some floating point
-        # representations that could give us:
-        # >>> x = 3.141592
-        # >>> round(x, 2)
-        # 3.1400000000000001
-        # >>>
-        return float(str(percent))
+        return percent
 
     def translatedPercentage(self, language=None):
         """See IRosettaStats."""
@@ -114,4 +107,3 @@
     def rosettaPercentage(self, language=None):
         """See IRosettaStats."""
         return self.asPercentage(self.rosettaCount(language))
-