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/test_file_importer.py (+26/-2)
lib/lp/translations/utilities/translation_import.py (+8/-1)
Reviewer Review Type Date Requested Status
Colin Watson (community) Needs Information
Review via email: mp+383669@code.qastaging.launchpad.net

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

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.