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

Proposed by Thiago F. Pappacena
Status: Needs review
Proposed branch: ~pappacena/launchpad:update-stats-on-translation-copy
Merge into: launchpad:master
Diff against target: 87 lines (+34/-3)
2 files modified
lib/lp/translations/utilities/tests/ (+26/-2)
lib/lp/translations/utilities/ (+8/-1)
Reviewer Review Type Date Requested Status
Colin Watson Needs Information
Review via email:

Commit message

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

To post a comment you must log in.
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

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.

Revision history for this message
Ioana Lasc (ilasc) wrote (last edit ):

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

Revision history for this message
Colin Watson (cjwatson) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/translations/utilities/tests/ b/lib/lp/translations/utilities/tests/
2index 013d13f..939b742 100644
3--- a/lib/lp/translations/utilities/tests/
4+++ b/lib/lp/translations/utilities/tests/
5@@ -1,4 +1,4 @@
6-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
8 # GNU Affero General Public License version 3 (see the file LICENSE).
10 """Translation File Importer tests."""
11@@ -12,13 +12,20 @@ from zope.component import getUtility
12 from import removeSecurityProxy
14 from lp.registry.interfaces.person import IPersonSet
15+from import config
16+from import JobRunner
17 from import FakeLibrarian
18-from lp.testing import TestCaseWithFactory
19+from lp.testing import (
20+ admin_logged_in,
21+ TestCaseWithFactory,
22+ )
23+from lp.testing.dbuser import dbuser
24 from lp.testing.layers import (
25 LaunchpadZopelessLayer,
26 ZopelessDatabaseLayer,
27 )
28 from lp.translations.enums import TranslationPermission
29+from lp.translations.interfaces.pofilestatsjob import IPOFileStatsJobSource
30 from lp.translations.interfaces.potemplate import IPOTemplateSet
31 from lp.translations.interfaces.side import TranslationSide
32 from lp.translations.interfaces.translationfileformat import (
33@@ -395,6 +402,23 @@ class FileImporterTestCase(TestCaseWithFactory):
34 "POFileImporter.importFile did not create an "
35 "ITranslationMessage object in the database.")
37+ # Checks if statistics update was scheduled correctly.
38+ with admin_logged_in():
39+ self.assertEqual(0, po_importer.pofile.translatedCount())
40+ self.assertEqual(0, po_importer.pofile.untranslatedCount())
41+ self.assertEqual(0, pot_importer.potemplate.translatedCount())
42+ self.assertEqual(0, pot_importer.potemplate.untranslatedCount())
44+ jobs = getUtility(IPOFileStatsJobSource).iterReady()
45+ self.assertGreaterEqual(jobs.count(), 2)
46+ with dbuser(config.IPOFileStatsJobSource.dbuser):
47+ JobRunner(jobs).runAll()
49+ self.assertEqual(1, po_importer.pofile.translatedCount())
50+ self.assertEqual(0, po_importer.pofile.untranslatedCount())
51+ self.assertEqual(0, pot_importer.potemplate.translatedCount())
52+ self.assertEqual(1, pot_importer.potemplate.untranslatedCount())
54 def test_FileImporter_importFile_conflict(self):
55 (pot_importer, po_importer) = (
56 self._createImporterForExportedEntries())
57diff --git a/lib/lp/translations/utilities/ b/lib/lp/translations/utilities/
58index f9af932..5a3962a 100644
59--- a/lib/lp/translations/utilities/
60+++ b/lib/lp/translations/utilities/
61@@ -1,4 +1,4 @@
62-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
63+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
64 # GNU Affero General Public License version 3 (see the file LICENSE).
66 __metaclass__ = type
67@@ -570,6 +570,7 @@ class FileImporter(object):
69 :return: The errors encountered during the import.
70 """
71+ from lp.translations.model import pofilestatsjob
72 # Collect errors here.
73 self.errors = []
75@@ -577,6 +578,12 @@ class FileImporter(object):
76 if message.msgid_singular:
77 self.importMessage(message)
79+ # Update statistics in background.
80+ if self.pofile is not None:
81+ pofilestatsjob.schedule(
82+ elif self.potemplate is not None:
83+ pofilestatsjob.schedule(
85 return self.errors, self.translation_file.syntax_warnings
87 @property