Code review comment for lp:~jtv/launchpad/translationimportqueueentry-info

> > === added file
> 'lib/lp/translations/browser/tests/test_translationimportqueueentry.py'
> > --- lib/lp/translations/browser/tests/test_translationimportqueueentry.py
> 1970-01-01 00:00:00 +0000
> > +++ lib/lp/translations/browser/tests/test_translationimportqueueentry.py
> 2010-02-10 13:06:21 +0000
> > @@ -0,0 +1,165 @@
> > +# Copyright 2010 Canonical Ltd. This software is licensed under the
> > +# GNU Affero General Public License version 3 (see the file LICENSE).
> > +
> > +"""Unit tests for translation import queue views."""
> > +
> > +from datetime import datetime
> > +from pytz import timezone
> > +from unittest import TestLoader
> > +
> > +from zope.component import getMultiAdapter
> > +from zope.security.proxy import removeSecurityProxy
> > +
> > +from canonical.testing import LaunchpadFunctionalLayer
> > +
> > +from canonical.launchpad.layers import TranslationsLayer, setFirstLayer
> > +from canonical.launchpad.webapp import canonical_url
> > +from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> > +from lp.registry.model.sourcepackage import SourcePackage
> > +from lp.testing import TestCaseWithFactory
> > +from lp.translations.model.translationimportqueue import (
> > + TranslationImportQueue)
>
> Don't you get an warnings about this? Should test code be importing from
> model code? I thought I remember that rule (and it makes sense if we
> profess to test against Interfaces).

No warnings, but now that you mention it, I'm not sure this is allowed.
Changed.

> > +class TestTranslationImportQueueEntryView(TestCaseWithFactory):
> > + """Tests for the queue entry review form."""
> > +
> > + layer = LaunchpadFunctionalLayer
> > +
> > + def setUp(self):
> > + super(TestTranslationImportQueueEntryView, self).setUp(
> > + '<email address hidden>')
> > + self.queue = TranslationImportQueue()
>
> I think this should really be:
> self.queue = getUtility(ITranslationImportQueue)

Done.

> > + def test_import_target_sourcepackage(self):
> > + # If the entry has a DistroSeries and a SourcePackageName, the
> > + # import_target is the corresponding SourcePackage.
> > + series = self.factory.makeDistroSeries()
> > + packagename = self.factory.makeSourcePackageName()
> > + package = SourcePackage(packagename, series)
>
> package = self.factory.makeSourcePackage(packagename, series)
>
> Works just as well ... ;-)

Oh, nice! Saves me an import.

> > + # No translatable series.
> > + series_text = view.product_translatable_series
> > + self.assertEqual("Project has no translatable series.",
> series_text)
> > +
> > + # One translatable series.
> > + extra_series = self.factory.makeProductSeries(product=product)
> > + self.factory.makePOTemplate(productseries=extra_series)
> > + series_text = view.product_translatable_series
> > + self.assertIn("Project has translatable series:", series_text)
> > + # A link follows, and the sentence ends in a period.
> > + self.assertEqual('</a>.', series_text[-5:])
>
> I am not all too happy about this testing style and wonder if doing this
> in a unit test rather than a doctest is the Right Thing. But I guess
> it's not a big deal, so you can leave it for now.

The unit test should go through the corner cases. It's ugly, but I
don't see any other way without bloating the code, writing helpers to
get helpful failure messages etc. The pagetest does the "happy path."

This particular test does have doctest feel to it. In this case I
thought a "story" setup made sense because of the close juxtaposition of
the 0, 1, n, and n > 3 cases as extensions that share both setup and
assertions.

> > === modified file 'lib/lp/translations/browser/translationimportqueue.py'
> > --- lib/lp/translations/browser/translationimportqueue.py 2010-01-13
> 04:46:11 +0000
> > +++ lib/lp/translations/browser/translationimportqueue.py 2010-02-10

> > @@ -129,6 +133,84 @@
> > return None
> >
> > @property
> > + def import_target(self):
> > + """The entry's `ProductSeries` or `SourcePackage`."""
> > + productseries = self.context.productseries
> > + distroseries = self.context.distroseries
> > + sourcepackagename = self.context.sourcepackagename
> > + if distroseries is None:
> > + return productseries
> > + else:
> > + factory = getUtility(ISourcePackageFactory)
> > + return factory.new(sourcepackagename, distroseries)
>
> Is this a "getOrMake" factory? Then this makes sense.

There is no distinction in this case. SourcePackage is immutable.
Stateless, not ORM-backed. Comparisons are on value, not identity:

assert SourcePackage(x, y) is not SourcePackage(x, y)
assert SourcePackage(x, y) == SourcePackage(x, y)

> > + @property
> > + def productseries_templates_link(self):
> > + """Return link to `ProductSeries`' templates.
> > +
> > + Use this only if the entry is attached to a `ProductSeries`.
> > + """
> > + assert self.context.productseries is not None, (
> > + "Entry is not attached to a ProductSeries.")
> > +
> > + template_count = self.context.productseries.potemplate_count
> > + if template_count == 0:
> > + return "no templates"
> > + else:
> > + link = "%s/+templates" % canonical_url(
> > + self.context.productseries, rootsite='translations')
>
> I think this works, too:
> link = canonical_url(
> self.context.productseries,
> rootsite='translations',
> view="+templates")

Alas, it does not!

Jeroen