Merge lp:~jtv/launchpad/bug-994650-cache-potmsgsets into lp:launchpad

[Jeroen T. Vermeulen]

Cache POTMsgSets per template in POFileTranslator scrubber.

This is one of two further optimizations to the POFileTranslator scrubber as mentioned in this MP: https://code.launchpad.net/~jtv/launchpad/bug-994650-scrub-faster

It's a very simple change. For each POFile that the scrubbing loop processes, it needs to know the ids of all POTMsgSets that participate in the template. But that set may well be the same for two or more POFiles in the batch, so this caches them.

There is no caching across batches, because one batch is handled in one transaction, and a concurrent template import could update the set of POTMsgSets participating in a template between transactions.

To test, use my combined branch which also includes schema changes: lp:~jtv/launchpad/combined-async-pofiletranslator

Run this test:
{{{
./bin/test -vvc lp.translations.scripts.tests.test_scrub_pofiletranslator
}}}

No lint.

Jeroen

[j.c.sackett (jcsackett)]

Jeroen--

Looks good. One suggestion:

Instead of

13 + if template_id not in cached_potmsgsets:
14 + cached_potmsgsets[template_id] = get_potmsgset_ids(template_id)
15 + potmsgset_ids = cached_potmsgsets[template_id]

I think you can just do

13 + potmsgset_ids = cached_potmsgsets.setdefault(template_id, get_potmsgset_ids(template_id))

dict.setdefault returns the value if the key exists, otherwise it function like dict.get(), returning the second parameter, but *also* setting it for the supplied key.

It's really just an LoC reduction, so not a major improvement, but it's a bit cleaner than the if clause set up. I'll leave it to you whether to make that change or not. I'll leave it to you whether or not you want to implement the change.

It doesn't look like this total arc of work reduces LoC, but I'm not familiar enough with everything you've been doing in the LP code base lately to know if you've got credit or not. I don't want to block a fix for a critical, so I'll just leave this as a reminder to please try and reduce the overall LoC going forward if you haven't already.

Thanks!

review: Approve

[Francis J. Lacoste (flacoste)]

On 12-05-10 11:13 AM, j.c.sackett wrote:
> It doesn't look like this total arc of work reduces LoC, but I'm not familiar enough with everything you've been doing in the LP code base lately to know if you've got credit or not. I don't want to block a fix for a critical, so I'll just leave this as a reminder to please try and reduce the overall LoC going forward if you haven't already.

Jeroen gets a waiver because even though this doesn't reduce LoC, it
does reduce maintenance burden a lot, as it will allow us to open new
translation series without the multi-day fuss that is happening right
now :-)

--
Francis J. Lacoste
<email address hidden>

[Jeroen T. Vermeulen (jtv)]

Thanks for the review! I was wondering when the LoC thing would come up. :) In defense of Francis' note, not entirely without self-interest, I'm also obviating considerable command-lining and human-scripting (“disable X, click Y”) for every Ubuntu cycle.

> Looks good. One suggestion:
>
> Instead of
>
> 13 + if template_id not in cached_potmsgsets:
> 14 + cached_potmsgsets[template_id] = get_potmsgset_ids(template_id)
> 15 + potmsgset_ids = cached_potmsgsets[template_id]
>
> I think you can just do
>
> 13 + potmsgset_ids = cached_potmsgsets.setdefault(template_id,
> get_potmsgset_ids(template_id))

That would evaluate get_potmsgset_ids(template_id) before the setdefault() call. There's not a very great distance between the call constructing a query that will be executed only when needed, and it also executing the query in order to return some other data structure. So it would make the caching a bit brittle!

Does make me wonder if dict.setdefault shouldn't accept a callable plus arguments so that it can evaluate lazily.

Jeroen

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-05-09 13:50:03 +0000
+++ database/schema/security.cfg	2012-05-24 05:53:33 +0000
@@ -2181,6 +2181,7 @@
 public.openidconsumerassociation        = SELECT, DELETE
 public.openidconsumernonce              = SELECT, DELETE
 public.person                           = SELECT, DELETE
+public.pofiletranslator                 = SELECT, INSERT, UPDATE, DELETE
 public.potranslation                    = SELECT, DELETE
 public.potmsgset                        = SELECT, DELETE
 public.product                          = SELECT

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-05-09 01:35:41 +0000
+++ lib/lp/scripts/garbo.py	2012-05-24 05:53:33 +0000
@@ -105,6 +105,9 @@
 from lp.translations.model.translationtemplateitem import (
     TranslationTemplateItem,
     )
+from lp.translations.scripts.scrub_pofiletranslator import (
+    ScrubPOFileTranslator,
+    )
 
 
 ONE_DAY_IN_SECONDS = 24 * 60 * 60
@@ -1418,6 +1421,7 @@
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
         RevisionAuthorEmailLinker,
+        ScrubPOFileTranslator,
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
         UnusedPOTMsgSetPruner,

=== added file 'lib/lp/translations/scripts/scrub_pofiletranslator.py'
--- lib/lp/translations/scripts/scrub_pofiletranslator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/scrub_pofiletranslator.py	2012-05-24 05:53:33 +0000
@@ -0,0 +1,281 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Keep `POFileTranslator` more or less consistent with the real data."""
+
+__metaclass__ = type
+__all__ = [
+    'ScrubPOFileTranslator',
+    ]
+
+from collections import namedtuple
+
+from storm.expr import (
+    Coalesce,
+    Desc,
+    )
+import transaction
+
+from lp.registry.model.distribution import Distribution
+from lp.registry.model.distroseries import DistroSeries
+from lp.registry.model.product import Product
+from lp.registry.model.productseries import ProductSeries
+from lp.services.database.bulk import (
+    load,
+    load_related,
+    )
+from lp.services.database.lpstorm import IStore
+from lp.services.looptuner import TunableLoop
+from lp.services.worlddata.model.language import Language
+from lp.translations.model.pofile import POFile
+from lp.translations.model.pofiletranslator import POFileTranslator
+from lp.translations.model.potemplate import POTemplate
+from lp.translations.model.translationmessage import TranslationMessage
+from lp.translations.model.translationtemplateitem import (
+    TranslationTemplateItem,
+    )
+
+
+def get_pofile_ids():
+    """Retrieve ids of POFiles to scrub.
+
+    The result's ordering is aimed at maximizing cache effectiveness:
+    by POTemplate name for locality of shared POTMsgSets, and by language
+    for locality of shared TranslationMessages.
+    """
+    store = IStore(POFile)
+    query = store.find(
+        POFile.id,
+        POFile.potemplateID == POTemplate.id,
+        POTemplate.iscurrent == True)
+    return query.order_by(POTemplate.name, POFile.languageID)
+
+
+def summarize_pofiles(pofile_ids):
+    """Retrieve relevant parts of `POFile`s with given ids.
+
+    This gets just enough information to determine whether any of the
+    `POFile`s need their `POFileTranslator` records fixed.
+
+    :param pofile_ids: Iterable of `POFile` ids.
+    :return: Dict mapping each id in `pofile_ids` to a duple of
+        `POTemplate` id and `Language` id for the associated `POFile`.
+    """
+    store = IStore(POFile)
+    rows = store.find(
+        (POFile.id, POFile.potemplateID, POFile.languageID),
+        POFile.id.is_in(pofile_ids))
+    return dict((row[0], row[1:]) for row in rows)
+
+
+def get_potmsgset_ids(potemplate_id):
+    """Get the ids for each current `POTMsgSet` in a `POTemplate`."""
+    store = IStore(POTemplate)
+    return store.find(
+        TranslationTemplateItem.potmsgsetID,
+        TranslationTemplateItem.potemplateID == potemplate_id,
+        TranslationTemplateItem.sequence > 0)
+
+
+def summarize_contributors(potemplate_id, language_id, potmsgset_ids):
+    """Return the set of ids of persons who contributed to a `POFile`.
+
+    This is a limited version of `get_contributions` that is easier to
+    compute.
+    """
+    store = IStore(POFile)
+    contribs = store.find(
+        TranslationMessage.submitterID,
+        TranslationMessage.potmsgsetID.is_in(potmsgset_ids),
+        TranslationMessage.languageID == language_id,
+        TranslationMessage.msgstr0 != None,
+        Coalesce(TranslationMessage.potemplateID, potemplate_id) ==
+            potemplate_id)
+    contribs.config(distinct=True)
+    return set(contribs)
+
+
+def get_contributions(pofile, potmsgset_ids):
+    """Map all users' most recent contributions to a `POFile`.
+
+    Returns a dict mapping `Person` id to the creation time of their most
+    recent `TranslationMessage` in `POFile`.
+
+    This leaves some small room for error: a contribution that is masked by
+    a diverged entry in this POFile will nevertheless produce a
+    POFileTranslator record.  Fixing that would complicate the work more than
+    it is probably worth.
+
+    :param pofile: The `POFile` to find contributions for.
+    :param potmsgset_ids: The ids of the `POTMsgSet`s to look for, as returned
+        by `get_potmsgset_ids`.
+    """
+    store = IStore(pofile)
+    language_id = pofile.language.id
+    template_id = pofile.potemplate.id
+    contribs = store.find(
+        (TranslationMessage.submitterID, TranslationMessage.date_created),
+        TranslationMessage.potmsgsetID.is_in(potmsgset_ids),
+        TranslationMessage.languageID == language_id,
+        TranslationMessage.msgstr0 != None,
+        Coalesce(TranslationMessage.potemplateID, template_id) ==
+            template_id)
+    contribs = contribs.config(distinct=(TranslationMessage.submitterID,))
+    contribs = contribs.order_by(
+        TranslationMessage.submitterID, Desc(TranslationMessage.date_created))
+    return dict(contribs)
+
+
+def get_pofiletranslators(pofile_id):
+    """Get `Person` ids from `POFileTranslator` entries for a `POFile`.
+
+    Returns a `set` of `Person` ids.
+    """
+    store = IStore(POFileTranslator)
+    return set(store.find(
+        POFileTranslator.personID,
+        POFileTranslator.pofileID == pofile_id))
+
+
+def remove_pofiletranslators(logger, pofile, person_ids):
+    """Delete `POFileTranslator` records."""
+    logger.debug(
+        "Removing %d POFileTranslator(s) for %s.",
+        len(person_ids), pofile.title)
+    store = IStore(pofile)
+    pofts = store.find(
+        POFileTranslator,
+        POFileTranslator.pofileID == pofile.id,
+        POFileTranslator.personID.is_in(person_ids))
+    pofts.remove()
+
+
+def remove_unwarranted_pofiletranslators(logger, pofile, pofts, contribs):
+    """Delete `POFileTranslator` records that shouldn't be there."""
+    excess = pofts - set(contribs)
+    if len(excess) > 0:
+        remove_pofiletranslators(logger, pofile, excess)
+
+
+def create_missing_pofiletranslators(logger, pofile, pofts, contribs):
+    """Create `POFileTranslator` records that were missing."""
+    shortage = set(contribs) - pofts
+    if len(shortage) == 0:
+        return
+    logger.debug(
+        "Adding %d POFileTranslator(s) for %s.",
+        len(shortage), pofile.title)
+    store = IStore(pofile)
+    for missing_contributor in shortage:
+        store.add(POFileTranslator(
+            pofile=pofile, personID=missing_contributor,
+            date_last_touched=contribs[missing_contributor]))
+
+
+def fix_pofile(logger, pofile, potmsgset_ids, pofiletranslators):
+    """This `POFile` needs fixing.  Load its data & fix it."""
+    contribs = get_contributions(pofile, potmsgset_ids)
+    remove_unwarranted_pofiletranslators(
+        logger, pofile, pofiletranslators, contribs)
+    create_missing_pofiletranslators(
+        logger, pofile, pofiletranslators, contribs)
+
+
+def needs_fixing(template_id, language_id, potmsgset_ids, pofiletranslators):
+    """Does the `POFile` with given details need `POFileTranslator` changes?
+
+    :param template_id: id of the `POTemplate` for the `POFile`.
+    :param language_id: id of the `Language` the `POFile` translates to.
+    :param potmsgset_ids: ids of the `POTMsgSet` items participating in the
+        template.
+    :param pofiletranslators: `POFileTranslator` objects for the `POFile`.
+    :return: Bool: does the existing set of `POFileTranslator` need fixing?
+    """
+    contributors = summarize_contributors(
+        template_id, language_id, potmsgset_ids)
+    return pofiletranslators != set(contributors)
+
+
+# A tuple describing a POFile that needs its POFileTranslators fixed.
+WorkItem = namedtuple("WorkItem", [
+    'pofile_id',
+    'potmsgset_ids',
+    'pofiletranslators',
+    ])
+
+
+def gather_work_items(pofile_ids):
+    """Produce `WorkItem`s for those `POFile`s that need fixing.
+
+    :param pofile_ids: An iterable of `POFile` ids to check.
+    :param pofile_summaries: Dict as returned by `summarize_pofiles`.
+    :return: A sequence of `WorkItem`s for those `POFile`s that need fixing.
+    """
+    pofile_summaries = summarize_pofiles(pofile_ids)
+    cached_potmsgsets = {}
+    work_items = []
+    for pofile_id in pofile_ids:
+        template_id, language_id = pofile_summaries[pofile_id]
+        if template_id not in cached_potmsgsets:
+            cached_potmsgsets[template_id] = get_potmsgset_ids(template_id)
+        potmsgset_ids = cached_potmsgsets[template_id]
+        pofts = get_pofiletranslators(pofile_id)
+        if needs_fixing(template_id, language_id, potmsgset_ids, pofts):
+            work_items.append(WorkItem(pofile_id, potmsgset_ids, pofts))
+
+    return work_items
+
+
+def preload_work_items(work_items):
+    """Bulk load data that will be needed to process `work_items`.
+
+    :param work_items: A sequence of `WorkItem` records.
+    :return: A dict mapping `POFile` ids from `work_items` to their
+        respective `POFile` objects.
+    """
+    pofiles = load(POFile, [work_item.pofile_id for work_item in work_items])
+    load_related(Language, pofiles, ['languageID'])
+    templates = load_related(POTemplate, pofiles, ['potemplateID'])
+    distroseries = load_related(DistroSeries, templates, ['distroseriesID'])
+    load_related(Distribution, distroseries, ['distributionID'])
+    productseries = load_related(
+        ProductSeries, templates, ['productseriesID'])
+    load_related(Product, productseries, ['productID'])
+    return dict((pofile.id, pofile) for pofile in pofiles)
+
+
+def process_work_items(logger, work_items, pofiles):
+    """Fix the `POFileTranslator` records covered by `work_items`."""
+    for work_item in work_items:
+        pofile = pofiles[work_item.pofile_id]
+        fix_pofile(
+            logger, pofile, work_item.potmsgset_ids,
+            work_item.pofiletranslators)
+
+
+class ScrubPOFileTranslator(TunableLoop):
+    """Tunable loop, meant for running from inside Garbo."""
+
+    maximum_chunk_size = 500
+
+    def __init__(self, *args, **kwargs):
+        super(ScrubPOFileTranslator, self).__init__(*args, **kwargs)
+        self.pofile_ids = tuple(get_pofile_ids())
+        self.next_offset = 0
+
+    def __call__(self, chunk_size):
+        """See `ITunableLoop`."""
+        start_offset = self.next_offset
+        self.next_offset = start_offset + int(chunk_size)
+        batch = self.pofile_ids[start_offset:self.next_offset]
+        if len(batch) == 0:
+            self.next_offset = None
+        else:
+            work_items = gather_work_items(batch)
+            pofiles = preload_work_items(work_items)
+            process_work_items(self.log, work_items, pofiles)
+            transaction.commit()
+
+    def isDone(self):
+        """See `ITunableLoop`."""
+        return self.next_offset is None

=== added file 'lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py'
--- lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py	2012-05-24 05:53:33 +0000
@@ -0,0 +1,313 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test scrubbing of `POFileTranslator`."""
+
+__metaclass__ = type
+
+from datetime import (
+    datetime,
+    timedelta,
+    )
+
+import pytz
+import transaction
+
+from lp.services.database.constants import UTC_NOW
+from lp.services.database.lpstorm import IStore
+from lp.services.log.logger import DevNullLogger
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import ZopelessDatabaseLayer
+from lp.translations.model.pofiletranslator import POFileTranslator
+from lp.translations.scripts.scrub_pofiletranslator import (
+    fix_pofile,
+    gather_work_items,
+    get_contributions,
+    get_pofile_ids,
+    get_pofiletranslators,
+    get_potmsgset_ids,
+    ScrubPOFileTranslator,
+    summarize_contributors,
+    summarize_pofiles,
+    )
+
+
+fake_logger = DevNullLogger()
+
+
+def size_distance(sequence, item1, item2):
+    """Return the absolute distance between items in a sequence."""
+    container = list(sequence)
+    return abs(container.index(item2) - container.index(item1))
+
+
+class TestScrubPOFileTranslator(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def query_pofiletranslator(self, pofile, person):
+        """Query `POFileTranslator` for a specific record.
+
+        :return: Storm result set.
+        """
+        store = IStore(pofile)
+        return store.find(POFileTranslator, pofile=pofile, person=person)
+
+    def make_message_with_pofiletranslator(self, pofile=None):
+        """Create a normal `TranslationMessage` with `POFileTranslator`."""
+        if pofile is None:
+            pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate,
+            sequence=self.factory.getUniqueInteger())
+        # A database trigger on TranslationMessage automatically creates
+        # a POFileTranslator record for each new TranslationMessage.
+        return self.factory.makeSuggestion(pofile=pofile, potmsgset=potmsgset)
+
+    def make_message_without_pofiletranslator(self, pofile=None):
+        """Create a `TranslationMessage` without `POFileTranslator`."""
+        tm = self.make_message_with_pofiletranslator(pofile)
+        IStore(pofile).flush()
+        self.becomeDbUser('postgres')
+        self.query_pofiletranslator(pofile, tm.submitter).remove()
+        return tm
+
+    def make_pofiletranslator_without_message(self, pofile=None):
+        """Create a `POFileTranslator` without `TranslationMessage`."""
+        if pofile is None:
+            pofile = self.factory.makePOFile()
+        poft = POFileTranslator(
+            pofile=pofile, person=self.factory.makePerson(),
+            date_last_touched=UTC_NOW)
+        IStore(poft.pofile).add(poft)
+        return poft
+
+    def test_get_pofile_ids_gets_pofiles_for_active_templates(self):
+        pofile = self.factory.makePOFile()
+        self.assertIn(pofile.id, get_pofile_ids())
+
+    def test_get_pofile_ids_skips_inactive_templates(self):
+        pofile = self.factory.makePOFile()
+        pofile.potemplate.iscurrent = False
+        self.assertNotIn(pofile.id, get_pofile_ids())
+
+    def test_get_pofile_ids_clusters_by_template_name(self):
+        # POFiles for templates with the same name are bunched together
+        # in the get_pofile_ids() output.
+        templates = [
+            self.factory.makePOTemplate(name='shared'),
+            self.factory.makePOTemplate(name='other'),
+            self.factory.makePOTemplate(name='andanother'),
+            self.factory.makePOTemplate(
+                name='shared', distroseries=self.factory.makeDistroSeries()),
+            ]
+        pofiles = [
+            self.factory.makePOFile(potemplate=template)
+            for template in templates]
+        ordering = get_pofile_ids()
+        self.assertEqual(
+            1, size_distance(ordering, pofiles[0].id, pofiles[-1].id))
+
+    def test_get_pofile_ids_clusters_by_language(self):
+        # POFiles for sharing templates and the same language are
+        # bunched together in the get_pofile_ids() output.
+        templates = [
+            self.factory.makePOTemplate(
+                name='shared', distroseries=self.factory.makeDistroSeries())
+            for counter in range(2)]
+        # POFiles per language & template.  We create these in a strange
+        # way to avoid the risk of mistaking accidental orderings such
+        # as per-id from being mistaken for the proper order.
+        languages = ['nl', 'fr']
+        pofiles_per_language = dict((language, []) for language in languages)
+        for language, pofiles in pofiles_per_language.items():
+            for template in templates:
+                pofiles.append(
+                    self.factory.makePOFile(language, potemplate=template))
+
+        ordering = get_pofile_ids()
+        for pofiles in pofiles_per_language.values():
+            self.assertEqual(
+                1, size_distance(ordering, pofiles[0].id, pofiles[1].id))
+
+    def test_summarize_pofiles_maps_id_to_template_and_language_ids(self):
+        pofile = self.factory.makePOFile()
+        self.assertEqual(
+            {pofile.id: (pofile.potemplate.id, pofile.language.id)},
+            summarize_pofiles([pofile.id]))
+
+    def test_get_potmsgset_ids_returns_potmsgset_ids(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=1)
+        self.assertContentEqual(
+            [potmsgset.id], get_potmsgset_ids(pofile.potemplate.id))
+
+    def test_get_potmsgset_ids_ignores_inactive_messages(self):
+        pofile = self.factory.makePOFile()
+        self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=0)
+        self.assertContentEqual([], get_potmsgset_ids(pofile.potemplate.id))
+
+    def test_summarize_contributors_gets_contributors(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertContentEqual(
+            [tm.submitter.id],
+            summarize_contributors(
+                pofile.potemplate.id, pofile.language.id, potmsgset_ids))
+
+    def test_summarize_contributors_ignores_inactive_potmsgsets(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=0)
+        self.factory.makeSuggestion(pofile=pofile, potmsgset=potmsgset)
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertContentEqual(
+            [],
+            summarize_contributors(
+                pofile.potemplate.id, pofile.language.id, potmsgset_ids))
+
+    def test_summarize_contributors_includes_diverged_msgs_for_template(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = pofile.potemplate
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertContentEqual(
+            [tm.submitter.id],
+            summarize_contributors(
+                pofile.potemplate.id, pofile.language.id, potmsgset_ids))
+
+    def test_summarize_contributors_excludes_other_diverged_messages(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = self.factory.makePOTemplate()
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertContentEqual(
+            [],
+            summarize_contributors(
+                pofile.potemplate.id, pofile.language.id, potmsgset_ids))
+
+    def test_get_contributions_gets_contributions(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertEqual(
+            {tm.submitter.id: tm.date_created},
+            get_contributions(pofile, potmsgset_ids))
+
+    def test_get_contributions_uses_latest_contribution(self):
+        pofile = self.factory.makePOFile()
+        today = datetime.now(pytz.UTC)
+        yesterday = today - timedelta(1, 1, 1)
+        old_tm = self.factory.makeSuggestion(
+            pofile=pofile, date_created=yesterday)
+        new_tm = self.factory.makeSuggestion(
+            translator=old_tm.submitter, pofile=pofile, date_created=today)
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertNotEqual(old_tm.date_created, new_tm.date_created)
+        self.assertContentEqual(
+            [new_tm.date_created],
+            get_contributions(pofile, potmsgset_ids).values())
+
+    def test_get_contributions_ignores_inactive_potmsgsets(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=0)
+        self.factory.makeSuggestion(pofile=pofile, potmsgset=potmsgset)
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertEqual({}, get_contributions(pofile, potmsgset_ids))
+
+    def test_get_contributions_includes_diverged_messages_for_template(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = pofile.potemplate
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertContentEqual(
+            [tm.submitter.id], get_contributions(pofile, potmsgset_ids))
+
+    def test_get_contributions_excludes_other_diverged_messages(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = self.factory.makePOTemplate()
+        potmsgset_ids = get_potmsgset_ids(pofile.potemplate.id)
+        self.assertEqual({}, get_contributions(pofile, potmsgset_ids))
+
+    def test_get_pofiletranslators_gets_translators_for_pofile(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_with_pofiletranslator(pofile)
+        self.assertContentEqual(
+            [tm.submitter.id], get_pofiletranslators(pofile.id))
+
+    def test_fix_pofile_leaves_good_pofiletranslator_in_place(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_with_pofiletranslator(pofile)
+        old_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+
+        fix_pofile(
+            fake_logger, pofile, [tm.potmsgset.id], set([tm.submitter.id]))
+
+        new_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+        self.assertEqual(old_poft, new_poft)
+
+    def test_fix_pofile_deletes_unwarranted_entries(self):
+        # Deleting POFileTranslator records is not something the app
+        # server ever does, so it requires special privileges.
+        self.becomeDbUser('postgres')
+        poft = self.make_pofiletranslator_without_message()
+        (pofile, person) = (poft.pofile, poft.person)
+        fix_pofile(fake_logger, pofile, [], set([person.id]))
+        self.assertIsNone(self.query_pofiletranslator(pofile, person).one())
+
+    def test_fix_pofile_adds_missing_entries(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_without_pofiletranslator(pofile)
+
+        fix_pofile(fake_logger, pofile, [tm.potmsgset.id], set())
+
+        new_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+        self.assertEqual(tm.submitter, new_poft.person)
+        self.assertEqual(pofile, new_poft.pofile)
+
+    def test_gather_work_items_caches_potmsgset_ids_for_same_template(self):
+        template = self.factory.makePOTemplate()
+        pofiles = [
+            self.factory.makePOFile(potemplate=template)
+            for counter in range(2)]
+        for pofile in pofiles:
+            self.make_message_without_pofiletranslator(pofile)
+        work_items = gather_work_items([pofile.id for pofile in pofiles])
+        # The potmsgset_ids entries are references to one and the same
+        # object.
+        self.assertIs(
+            work_items[0].potmsgset_ids, work_items[1].potmsgset_ids)
+
+    def test_gather_work_items_does_not_cache_across_templates(self):
+        pofiles = [self.factory.makePOFile() for counter in range(2)]
+        for pofile in pofiles:
+            self.make_message_without_pofiletranslator(pofile)
+        work_items = gather_work_items([pofile.id for pofile in pofiles])
+        # The POFiles are for different templates, so they do not share
+        # the same potmsgset_ids.
+        self.assertNotEqual(
+            work_items[0].potmsgset_ids, work_items[1].potmsgset_ids)
+
+    def test_tunable_loop(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_without_pofiletranslator(pofile)
+        bad_poft = self.make_pofiletranslator_without_message(pofile)
+        noncontributor = bad_poft.person
+        transaction.commit()
+
+        self.becomeDbUser('garbo')
+        ScrubPOFileTranslator(fake_logger).run()
+        # Try to break the loop if it failed to commit its changes.
+        transaction.abort()
+
+        # An unwarranted POFileTranslator record has been deleted.
+        self.assertIsNotNone(
+            self.query_pofiletranslator(pofile, tm.submitter).one())
+        # A missing POFileTranslator has been created.
+        self.assertIsNone(
+            self.query_pofiletranslator(pofile, noncontributor).one())