Code review comment for lp:~henninge/launchpad/bug-523810-needs-information-age

> 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! ;-)