-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Jeroen,
thanks for this improvement. I have a few little issues but please land
this when you are done with them.

 review approve code

Cheers,
Henning

> === 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).

> +
> +
> +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)

> +        self.uploader = self.factory.makePerson()
> +
> +    def _makeProductSeries(self):
> +        """Set up a product series for a translatable product."""
> +        product = self.factory.makeProduct()
> +        product.official_rosetta = True
> +        return product.getSeries('trunk')
> +
> +    def _makeView(self, entry):
> +        """Create view for a queue entry."""
> +        request = LaunchpadTestRequest()
> +        setFirstLayer(request, TranslationsLayer)
> +        view = getMultiAdapter((entry, request), name='+index')
> +        view.initialize()
> +        return view
> +
> +    def _makeEntry(self, productseries=None, distroseries=None,
> +                   sourcepackagename=None):
> +        filename = self.factory.getUniqueString() + '.pot'
> +        contents = self.factory.getUniqueString()
> +        entry = self.queue.addOrUpdateEntry(
> +            filename, contents, False, self.uploader,
> +            productseries=productseries, distroseries=distroseries,
> +            sourcepackagename=sourcepackagename)
> +        return removeSecurityProxy(entry)
> +
> +    def test_import_target_productseries(self):
> +        # If the entry's attached to a ProductSeries, that's what
> +        # import_target returns.
> +        series = self._makeProductSeries()
> +        entry = self._makeEntry(productseries=series)
> +        view = self._makeView(entry)
> +
> +        self.assertEqual(series, view.import_target)
> +
> +    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 ... ;-)

> +        entry = self._makeEntry(
> +            distroseries=series, sourcepackagename=packagename)
> +        view = self._makeView(entry)
> +
> +        self.assertEqual(package, view.import_target)
> +
> +    def test_productseries_templates_link(self):
> +        # productseries_templates_link counts and, if appropriate links
> +        # to, the series' templates.
> +        series = self._makeProductSeries()
> +        entry = self._makeEntry(productseries=series)
> +        view = self._makeView(entry)
> +
> +        # If there are no templates, there is no link.
> +        self.assertEqual("no templates", view.productseries_templates_link)
> +
> +        # For one template, there is a link.  Its text uses the
> +        # singular.
> +        self.factory.makePOTemplate(productseries=series)
> +        self.assertIn('1 template', view.productseries_templates_link)
> +        self.assertNotIn('1 templates', view.productseries_templates_link)
> +        url = canonical_url(series, rootsite='translations') + '/+templates'
> +        self.assertIn(url, view.productseries_templates_link)
> +
> +    def test_product_translatable_series(self):
> +        # If the entry belongs to a productseries, product_translatable_series
> +        # lists the product's translatable series.
> +        series = self._makeProductSeries()
> +        product = series.product
> +        entry = self._makeEntry(productseries=series)
> +        view = self._makeView(entry)
> +
> +        # 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.

[...]

> 
> === 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 13:06:21 +0000
> @@ -1,7 +1,7 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
> -"""Browser views for ITranslationImportQueue."""
> +"""Browser views for `ITranslationImportQueue`."""

Thanks for fixing these little things.

>  
>  __metaclass__ = type
>  
> @@ -21,9 +21,13 @@
>  from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
>  
>  from canonical.database.constants import UTC_NOW
> +from canonical.launchpad.webapp.tales import DateTimeFormatterAPI
> +from canonical.launchpad.webapp.interfaces import (
> +    NotFoundError, UnexpectedFormData)
>  from lp.translations.browser.hastranslationimports import (
>      HasTranslationImportsView)
>  from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
>  from lp.translations.interfaces.translationimportqueue import (
>      ITranslationImportQueueEntry, IEditTranslationImportQueueEntry,
>      ITranslationImportQueue, RosettaImportStatus,
> @@ -31,8 +35,6 @@
>  from lp.services.worlddata.interfaces.language import ILanguageSet
>  from lp.translations.interfaces.pofile import IPOFileSet
>  from lp.translations.interfaces.potemplate import IPOTemplateSet
> -from canonical.launchpad.webapp.interfaces import (
> -    NotFoundError, UnexpectedFormData)
>  
>  from canonical.launchpad.webapp import (
>      action, canonical_url, GetitemNavigation, LaunchpadFormView)
> @@ -48,6 +50,8 @@
>      label = "Review import queue entry"
>      schema = IEditTranslationImportQueueEntry
>  
> +    max_series_to_display = 3
> +
>      @property
>      def initial_values(self):
>          """Initialize some values on the form, when it's possible."""
> @@ -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.

> +
> +    @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")

Other than that, very nice code around here ... ;-)


> +            if template_count == 1:
> +                word = "template"
> +            else:
> +                word = "templates"
> +            return '<a href="%s">%d %s</a>' % (link, template_count, word)
> +
> +    def _composeProductSeriesLink(self, productseries):
> +        """Produce HTML to link to `productseries`."""
> +        return '<a href="%s">%s</a>' % (
> +            canonical_url(productseries, rootsite='translations'),
> +            productseries.name)
> +
> +    @property
> +    def product_translatable_series(self):
> +        """Summarize whether `Product` has translatable series.
> +
> +        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.")
> +
> +        product = self.context.productseries.product
> +        translatable_series = list(product.translatable_series)
> +        if len(translatable_series) == 0:
> +            return "Project has no translatable series."
> +        else:
> +            links = [
> +                self._composeProductSeriesLink(series)
> +                for series in translatable_series[:self.max_series_to_display]
> +                ]
> +            links_text = ', '.join(links)
> +            if len(translatable_series) > self.max_series_to_display:
> +                tail = ", ..."
> +            else:
> +                tail = "."
> +            return "Project has translatable series: " + links_text + tail
> +
> +    @property
> +    def status_change_date(self):
> +        """Show date of last status change.
> +
> +        Says nothing at all if the entry's status has not changed since
> +        upload.
> +        """
> +        change_date = self.context.date_status_changed
> +        if change_date == self.context.dateimported:
> +            return ""
> +        else:
> +            formatter = DateTimeFormatterAPI(change_date)
> +            return "Last changed %s." % formatter.displaydate()

Ah, a TAL formatter. I like that. ;)

> +
> +    @property
>      def next_url(self):
>          """See `LaunchpadFormView`."""
>          # The referer header we want is only available before the view's
> 
[...]
Rest is OK.

Thank you very much for doing this!

All that's left are the UI issues that you need to figure out with your
UI reviewer.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkty5DYACgkQBT3oW1L17ijwGwCcCK34dVQiMRJgKpDbNV+fljYD
dR8AoOL4/436HzQFM5v5VoTDLz5Tlbf3
=DVTN
-----END PGP SIGNATURE-----