> Hi Henning, Hallo Abel! > overall a nice branc, but I have a couple of questions, see > below. Thanks for the review, though. ;) > > === 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. Yes, you are right about the capitalization. And the names are good suggestions, too. Fixed that. > > > + 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. I renamed it but left it in place here, as per our discussion on IRC. We agreed that the general rule is that tests should not import from model code. I also noted that I consider these expiration times as implementation-independent and I don't see a pressing reason *not* to have this here in the interface module. You agreed to the latter. > > > + > > + > > class SpecialTranslationImportTargetFilter(DBEnumeratedType): > > """Special "meta-targets" to filter the queue view by.""" > > > > > > === modified file 'lib/lp/translations/model/translationimportqueue.py' [...] > > === 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. Right, fixed. > > > + """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? It needs to be called(), so I'd consider it a method. Changed to camelCase. > > > + """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? I try to avoid any slowdown in tests. I documented this as a "one-way" function now, so this should be clearer. > > 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 ;) Sorry about that. I noticed the asymmetry, too, but did nothing about it. Shame on me. It looks nice and symmetric now. ;-) > > [...] > > > - 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. Comment added. Thank you for the review! ;-)