Merge lp:~jtv/launchpad/bug-994650-scrub-in-batches into lp:launchpad

[Jeroen T. Vermeulen]

Further speed up scrubbing of POFileTranslator.

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

Here you see two parts of the scrubbing process separated further: finding which POFiles in a batch need their POFileTranslator entries fixed, and actually doing so. The benefit is in a new, intervening step: bulk-load all objects needed for doing this work. Disappointingly, about half of the relevant object graph is needed for log output. But we probably should have it anyway, because tweaking such processes without helpful logs can be highly demotivating.

As an added bonus, it turns out we don't really need to load full POFileTranslator records, let alone cache them across these steps. That's one big memory load off my mind. I deliberately didn't make the scrubber check and correct dates on existing POFileTranslator records; a bit of imprecision is fine.

As a next step, which should further reduce memory load as well as DB querying, we can cache sets of POTMsgSet ids per template across items in a batch. (But not outside batches, since they may change between transactions).

The tests don't go into the new components. They already cover the aggregate behaviour of fix_pofile() and the main loop; there'd be a lot of duplication and I'd hate to lose integration test coverage as a side effect of moving things into more fine-grained unit tests. Also I'm trying to correct for a past of focusing too much on fine-grained tests. But, dear reviewer, if you see anything that you do want tested then please say so.

Jeroen

[j.c.sackett (jcsackett)]

Jeroen--

This looks good. Thanks.

review: Approve

=== 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:14:30 +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:14:30 +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:14:30 +0000
@@ -0,0 +1,278 @@
+# 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)
+    work_items = []
+    for pofile_id in pofile_ids:
+        template_id, language_id = pofile_summaries[pofile_id]
+        potmsgset_ids = get_potmsgset_ids(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:14:30 +0000
@@ -0,0 +1,288 @@
+# 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,
+    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=1)
+        # 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_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())