Code review comment for lp:~jtv/launchpad/bug-146855

= Bug 146855 =

Actually this branch resolves more long-standing gripes with the
translations import auto-approver than just that one bug.

But let's start with the original bug. An entry on the translations
import queue can be in one of several states: Needs Review, Approved,
Imported, Failed, Blocked, or Deleted. Failed is the state it ends up
in when its import failed, typically because of a syntax error.

Successfully imported entries get gc'ed off the queue after a few days.
Currently, Failed ones do not. The idea is that its owner uploads an
updated copy of the same file; it reuses the same queue entry; it gets
processed; it ends up successfully Imported; and so finally the entry
drops out of the queue. But in practice, especially for Ubuntu, Failed
entries lie around effectively forever.

So I fixed that. We want to clean up these entries less aggressively
than we do Imported ones, so the owner gets a fair chance to notice and
fix the problem (and sometimes, so that we get to restart a bunch of
entries that failed because of operational problems). This branch makes
the garbage-collection of entries in various states data-driven: 3 days
for Imported or Deleted entries, one month for Failed ones. There is no
need to test each state's "grace period" separately since we'd only be
unit-testing the contents of a dict.

== Other gripes ==

The approver's LaunchpadCronScript still lived in the same module as the
script it was originally spliced out of. Bit of a Zeus-and-Athena
scenario there. I gave it its own module in lp.translations.scripts.

Also, the class in there now inherits from LaunchpadCronScript instead
of having it instantiated from a separate LaunchpadCronScript-derived
class in the main script file. Unfortunately this did move some debug
output into the doctest; I switched from the FakeLogger to the
MockLogger so I could set a higher debug level and avoid this.

You may noticed that where transaction managers are passed around, I
changed their names from ztm (Zopeless Transaction Manager) to txn. We
don't use the ztm any more. Actually txn is just an alias for the
transaction module, so the argument isn't needed at all now. But this
is a relatively elegant way of telling a method whether you want it to
commit transactions. Passing a boolean for "please commit" is just too
ugly.

Oh, and another biggie: entries for obsolete distroseries. Right now
perhaps a fifth of the queue (certainly over 25K entries, out of 150K!)
are for obsolete Ubuntu releases that will never need any further
translations updates--they won't even get security updates. So I made
the script do some cleanup on those entries.

Deleting all of these would cause major database mayhem every time an
Ubuntu release slipped into obsolescence, so this cleanup is limited to
100 entries per run. Experience shows that that's not a noticeable load
for database replication, yet it'll get rid of all of these entries in a
few days. I didn't bother testing this since it's an operational detail
but I did consistently test that none of these cleanups affect rows that
shouldn't be affected.

To test:
{{{
./bin/test -vv -t poimport -t approval
}}}

No lint.

Jeroen