Merge ~pappacena/launchpad:update-stats-on-translation-copy into launchpad:master

[Thiago F. Pappacena]

Scheduling to run statistics update on PO and POT files when we copy translations.

[Thiago F. Pappacena (pappacena)]

I'm not entirely sure that this is where the translations imports happen, so I would like a deeper review from someone that knows better about this translation part.

[Ioana Lasc (ilasc)]

In theory this looks good to me too but I agree it needs a more senior review..

[Colin Watson (cjwatson)]

Hmm. Interesting, but I'm a bit confused. (Some of this confusion is because I don't know Translations very well.)

TranslationImporter.importFile is I think only called from POFile.importFromQueue and POTemplate.importFromQueue. I was going to say that I thought this sort of thing probably ought to go there rather than in the utility. But then I looked in more detail and found that the substance of it is supposed to be there already: if you look down towards the bottom of POFile.importFromQueue, it calls self.updateStatistics(), and if you look down towards the bottom of POTemplate.importFromQueue, it does a bunch of statistics updates as well. These are mostly similar to what POFileStatsJob does, so I think scheduling a POFileStatsJob here is mainly redundant and we shouldn't do it.

Of course, something is clearly wrong, but I'd approach it differently. Is it possible that the shared template handling in POFileStatsJob isn't done by the existing statistics update code in the models? I think that's probably what the problem is: we're already updating statistics for the POFile itself just fine (or for the POTemplate and all its POFiles), but we aren't dealing with other POFiles that share translations.

However, as I said, LP Translations really isn't my strong point and in particular I've never really got my head around translations sharing, so I'd suggest trying to get hold of William and discuss this with him.

review: Needs Information

Unmerged commits

b43979c... by Thiago F. Pappacena on 2020-05-08

Schedule statistic update on translation import

diff --git a/lib/lp/translations/utilities/tests/test_file_importer.py b/lib/lp/translations/utilities/tests/test_file_importer.py
index 013d13f..939b742 100644
--- a/lib/lp/translations/utilities/tests/test_file_importer.py
+++ b/lib/lp/translations/utilities/tests/test_file_importer.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Translation File Importer tests."""
@@ -12,13 +12,20 @@ from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.registry.interfaces.person import IPersonSet
+from lp.services.config import config
+from lp.services.job.runner import JobRunner
 from lp.services.librarianserver.testing.fake import FakeLibrarian
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    admin_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.dbuser import dbuser
 from lp.testing.layers import (
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
     )
 from lp.translations.enums import TranslationPermission
+from lp.translations.interfaces.pofilestatsjob import IPOFileStatsJobSource
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.side import TranslationSide
 from lp.translations.interfaces.translationfileformat import (
@@ -395,6 +402,23 @@ class FileImporterTestCase(TestCaseWithFactory):
                 "POFileImporter.importFile did not create an "
                 "ITranslationMessage object in the database.")
 
+        # Checks if statistics update was scheduled correctly.
+        with admin_logged_in():
+            self.assertEqual(0, po_importer.pofile.translatedCount())
+            self.assertEqual(0, po_importer.pofile.untranslatedCount())
+            self.assertEqual(0, pot_importer.potemplate.translatedCount())
+            self.assertEqual(0, pot_importer.potemplate.untranslatedCount())
+
+            jobs = getUtility(IPOFileStatsJobSource).iterReady()
+            self.assertGreaterEqual(jobs.count(), 2)
+            with dbuser(config.IPOFileStatsJobSource.dbuser):
+                JobRunner(jobs).runAll()
+
+            self.assertEqual(1, po_importer.pofile.translatedCount())
+            self.assertEqual(0, po_importer.pofile.untranslatedCount())
+            self.assertEqual(0, pot_importer.potemplate.translatedCount())
+            self.assertEqual(1, pot_importer.potemplate.untranslatedCount())
+
     def test_FileImporter_importFile_conflict(self):
         (pot_importer, po_importer) = (
             self._createImporterForExportedEntries())
diff --git a/lib/lp/translations/utilities/translation_import.py b/lib/lp/translations/utilities/translation_import.py
index f9af932..5a3962a 100644
--- a/lib/lp/translations/utilities/translation_import.py
+++ b/lib/lp/translations/utilities/translation_import.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -570,6 +570,7 @@ class FileImporter(object):
 
         :return: The errors encountered during the import.
         """
+        from lp.translations.model import pofilestatsjob
         # Collect errors here.
         self.errors = []
 
@@ -577,6 +578,12 @@ class FileImporter(object):
             if message.msgid_singular:
                 self.importMessage(message)
 
+        # Update statistics in background.
+        if self.pofile is not None:
+            pofilestatsjob.schedule(self.pofile.id)
+        elif self.potemplate is not None:
+            pofilestatsjob.schedule(self.potemplate.id)
+
         return self.errors, self.translation_file.syntax_warnings
 
     @property