Hi Henning,

overall a nice branc, but I have a couple of questions, see
below.

Abel

> = Bug 523810 =
>
> The import queue gardener removes entries from the import
> queue that have reached a certain age in certain states.
> The age is configurable through a look-up table and the new
> "Needs Information" status needs an entry in that table.
> The maximum age should be identical to "Needs Review" for
> starters.
>
> == Implementation details ==
>
> Driven by the want to extend test coverage to all entry
> states that are being cleaned out I moved the look-up
> table to the interface module where it can be reached
> from the test.
>
> I also found a nice little use of the with statement again. ;-)
>
> == Tests ==
>
> Run all autoapproval test to verify that the introduction of
> the GardenerDbUserMixin did not mess up existing tests.
>
> bin/test -vvt autoapproval
>
> == Demo/QA ==
>
> In order to not have to wait half a year for an entry to
> expire on staging, some SQL magic will be needed to make an
> entry in the "Needs Information" stage that old. Then wait
> for a day or so to see it disappear. Staging updates might
> be a nuissance here.
>
> = Launchpad lint =
>
> Checking for conflicts. and issues in doctests and templates.
> Running jslint, xmllint, pyflakes, and pylint.
> Using normal rules.
>
> Linting changed files:
>   lib/lp/translations/interfaces/translationimportqueue.py
>   lib/lp/translations/model/translationimportqueue.py
>   lib/lp/translations/tests/test_autoapproval.py
>
>
> == Pylint notices ==
>
> lib/lp/translations/interfaces/translationimportqueue.py
>     12: [F0401] Unable to import 'lazr.enum' (No module named enum)
>     22: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
>     23: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
>     24: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
>
>
> === modified file 'lib/lp/translations/interfaces/translationimportqueue.py'
> --- lib/lp/translations/interfaces/translationimportqueue.py	2010-02-05 10:39:45 +0000
> +++ lib/lp/translations/interfaces/translationimportqueue.py	2010-02-19 09:03:16 +0000
> @@ -3,6 +3,8 @@
>
>  # pylint: disable-msg=E0211,E0213
>
> +from datetime import timedelta
> +
>  from zope.interface import Interface, Attribute
>  from zope.schema import (
>      Bool, Choice, Datetime, Field, Int, Object, Text, TextLine)
> @@ -39,6 +41,7 @@
>      'RosettaImportStatus',
>      'SpecialTranslationImportTargetFilter',
>      'TranslationFileType',
> +    'TranslationImportQueueEntryAge',
>      'UserCannotSetTranslationImportStatus',
>      ]
>
> @@ -111,6 +114,22 @@
>          """)
>
>
> +# Some time spans in days.
> +_month = 30
> +_half_year = 366 / 2

This being constants, they should be defined as MONTH and HALF_YEAR,
I think, but...

> +
> +
> +# Period after which entries with certain statuses are culled from the
> +# queue.
> +TranslationImportQueueEntryAge = {
> +    RosettaImportStatus.DELETED: timedelta(days=3),
> +    RosettaImportStatus.FAILED: timedelta(days=_month),

...seeing this, I think the constants are probably better called
DAYS_OF_MONTH/DAYS_OF_HALF_YEAR or DAYS_IN_MONTH/DAYS_IN_HALF_YEAR.

> +    RosettaImportStatus.IMPORTED: timedelta(days=3),
> +    RosettaImportStatus.NEEDS_INFORMATION: timedelta(days=_half_year),
> +    RosettaImportStatus.NEEDS_REVIEW: timedelta(days=_half_year),
> +}

Our coding style convetions say that this should called
translation_import_queue_entry_age. Also, I don't think that you
need to define it in the interfaces module, you could also
add it to the __all__ list of the model module.

> +
> +
>  class SpecialTranslationImportTargetFilter(DBEnumeratedType):
>      """Special "meta-targets" to filter the queue view by."""
>
>
> === modified file 'lib/lp/translations/model/translationimportqueue.py'
> --- lib/lp/translations/model/translationimportqueue.py	2010-02-05 09:23:07 +0000
> +++ lib/lp/translations/model/translationimportqueue.py	2010-02-19 09:03:16 +0000
> @@ -58,6 +58,7 @@
>      RosettaImportStatus,
>      SpecialTranslationImportTargetFilter,
>      TranslationImportQueueConflictError,
> +    TranslationImportQueueEntryAge,
>      UserCannotSetTranslationImportStatus)
>  from lp.translations.interfaces.potemplate import IPOTemplate
>  from lp.translations.interfaces.translations import TranslationConstants
> @@ -66,19 +67,6 @@
>  from canonical.librarian.interfaces import ILibrarianClient
>
>
> -# Approximate number of days in a 6-month period.
> -half_year = 366 / 2
> -
> -# Period after which entries with certain statuses are culled from the
> -# queue.
> -entry_gc_age = {
> -    RosettaImportStatus.DELETED: datetime.timedelta(days=3),
> -    RosettaImportStatus.IMPORTED: datetime.timedelta(days=3),
> -    RosettaImportStatus.FAILED: datetime.timedelta(days=30),
> -    RosettaImportStatus.NEEDS_REVIEW: datetime.timedelta(days=half_year),
> -}
> -
> -
>  def is_gettext_name(path):
>      """Does given file name indicate it's in gettext (PO or POT) format?"""
>      base_name, extension = os.path.splitext(path)
> @@ -1256,8 +1244,8 @@
>          """
>          now = datetime.datetime.now(pytz.UTC)
>          deletion_clauses = []
> -        for status, gc_age in entry_gc_age.iteritems():
> -            cutoff = now - gc_age
> +        for status, max_age in TranslationImportQueueEntryAge.iteritems():
> +            cutoff = now - max_age
>              deletion_clauses.append(And(
>                  TranslationImportQueueEntry.status == status,
>                  TranslationImportQueueEntry.date_status_changed < cutoff))
>
> === modified file 'lib/lp/translations/tests/test_autoapproval.py'
> --- lib/lp/translations/tests/test_autoapproval.py	2010-01-26 15:25:53 +0000
> +++ lib/lp/translations/tests/test_autoapproval.py	2010-02-19 09:03:16 +0000
> @@ -8,6 +8,9 @@
>  through the possibilities should go here.
>  """
>
> +from __future__ import with_statement
> +
> +from contextlib import contextmanager
>  from datetime import datetime, timedelta
>  from pytz import UTC
>  import transaction
> @@ -31,17 +34,28 @@
>      TranslationImportQueue, TranslationImportQueueEntry)
>  from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
>  from lp.translations.interfaces.translationimportqueue import (
> -    RosettaImportStatus)
> +    RosettaImportStatus, TranslationImportQueueEntryAge)
>  from lp.testing import TestCaseWithFactory
>  from lp.testing.factory import LaunchpadObjectFactory
>  from canonical.launchpad.webapp.testing import verifyObject
>  from canonical.testing import LaunchpadZopelessLayer
>
>
> -def become_the_gardener(layer):
> +class GardenerDbUserMixin(object):
>      """Switch to the translations import queue gardener database role."""
> -    transaction.commit()
> -    layer.switchDbUser('translations_import_queue_gardener')
> +
> +    def become_the_gardener(self):

The method name should be camelCased.

> +        """Switch the user once."""
> +        transaction.commit()
> +        self.layer.switchDbUser('translations_import_queue_gardener')
> +
> +    @contextmanager
> +    def being_the_gardener(self):

I think this name should also be camelCased, but I am not sure --
do we consider context meanagers to be similar to methods or similar
to properties?

> +        """Context manager to restore the launchpad user."""
> +        self.become_the_gardener()
> +        yield
> +        transaction.commit()
> +        self.layer.switchDbUser('launchpad')

Did you consider to drop the method become_the_gardener() and
to include the code directly in the context manager? I see
that you call become_the_gardener() quite often in the tests,
but you could also use "with self.being_the-gardener()" there.
OK, that would add the second commit() and switchDbUser()
calls to all these tests, but would this be a noticable slow down?

Perhaps it is a sign that I need even more coffee this morning,
but my first thought seeing this code was "the code before and
after the 'yield' looks somewhat asymmetric, and why do we need
to commit() and switchDbUser() as a cleanup?" Sure, it dawned to
me that these calls are needed due to the corresponding calls in
become_teh_gardener(), but at least for my coffee-starved brain
that was slightly difficult ;)

[...]

> -    def test_cleanUpObsoleteEntries_needs_review(self):
> -        # _cleanUpObsoleteEntries cleans up entries in Needs Review
> -        # state after a very long wait.
> -        entry = self._makeProductEntry()
> -        entry.potemplate = self.factory.makePOTemplate()
> -        self._setStatus(entry, RosettaImportStatus.NEEDS_REVIEW, None)
> -        entry_id = entry.id
> -
> -        self.queue._cleanUpObsoleteEntries(self.store)
> -        self.assertTrue(self._exists(entry_id))
> -
> -        entry.date_status_changed -= timedelta(days=200)
> -        entry.syncUpdate()
> -
> -        become_the_gardener(self.layer)
> -        self.queue._cleanUpObsoleteEntries(self.store)
> -        self.assertFalse(self._exists(entry_id))
> +        affected_statuses = [
> +            RosettaImportStatus.DELETED,
> +            RosettaImportStatus.FAILED,
> +            RosettaImportStatus.IMPORTED,
> +            RosettaImportStatus.NEEDS_INFORMATION,
> +            RosettaImportStatus.NEEDS_REVIEW,
> +            ]
> +        for status in affected_statuses:
> +            entry = self._makeProductEntry()
> +            entry.potemplate = self.factory.makePOTemplate()
> +            maximum_age = TranslationImportQueueEntryAge[status]
> +            # Avoid the exact date here by a day because that could introduce
> +            # spurious test failures.
> +            almost_oldest_possible_date = (
> +                datetime.now(UTC) - maximum_age + timedelta(days=1))
> +            self._setStatus(entry, status, almost_oldest_possible_date)
> +            entry_id = entry.id
> +
> +            self.queue._cleanUpObsoleteEntries(self.store)
> +            self.assertTrue(self._exists(entry_id))

so, here _cleanUpObsoleteEntries(self.store) is called by the
regular user...

> +
> +            # Now cross the border, again cushoning it by a day.
> +            entry.date_status_changed -= timedelta(days=2)
> +            entry.syncUpdate()
> +
> +            with self.being_the_gardener():
> +                self.queue._cleanUpObsoleteEntries(self.store)
> +                self.assertFalse(
> +                    self._exists(entry_id),
> +                    "Queue entry in state '%s' was not removed." % status)

while it is called here by the gardener? While I'm writing this
comments, it dawns to me that the former call does not involve
any DB write or delete operations, while the latter does, but
this might be worth a short comment.