Merge lp:~jtv/launchpad/translationimportqueueentry-info into lp:launchpad

[Jeroen T. Vermeulen]

Show translation upload details on approval form.

[Jeroen T. Vermeulen (jtv)]

= Bug 519556 =

When reviewing translations uploads, we translations admins often want quick access to the following information on the approval page:
 * The uploader.
 * The file's contents.
 * The project: what's its license status, is it already using Translations?
 * The release series: does it have any templates yet, and which ones?
 * When was the entry last updated? The current UI fails to show this anywhere.

In addition, it may occasionally be convenient to have a copy of the entry's error_output shown. That makes this page a great place to go when analyzing failures.

In this branch I add all of this information. It's at the bottom of the page, since I want it to be unobtrusive. It wouldn't do to force admins to scroll down before they get to the input fields. (Only translations admins and the Ubuntu translation coordinates can access this page).

For Q/A, I intend to visit a whole bunch of pages for translation import queue entries and see that they all make sense.

To test:
{{{
./bin/test -vv -t importqueue/xx-entry
}}}

No lint.

Jeroen

[Henning Eggers (henninge)]

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

Hi Jeroen,

thanks for working late and creating this branch. Great work!

Am 10.02.2010 00:36, Jeroen T. Vermeulen schrieb:
> = Bug 519556 =
>
> When reviewing translations uploads, we translations admins often want quick access to the following information on the approval page:
> * The uploader.
> * The file's contents.
> * The project: what's its license status, is it already using Translations?
> * The release series: does it have any templates yet, and which ones?
> * When was the entry last updated? The current UI fails to show this anywhere.

Wonderful! That is very helpful information!

>
> In addition, it may occasionally be convenient to have a copy of the entry's error_output shown. That makes this page a great place to go when analyzing failures.

Actually, as you know, I have a branch ready that displays the
error_output in a text field as part of the form, so this part might go
out again, soon. No offence meant. ;-)

>
> In this branch I add all of this information. It's at the bottom of the page, since I want it to be unobtrusive. It wouldn't do to force admins to scroll down before they get to the input fields. (Only translations admins and the Ubuntu translation coordinates can access this page).

Yes, I agree with that, although a two-column layout wouldn't have been
bad, either. The form would go on the left, the information on the
right. But I am not sure if that is easy to do and would fit with our UI
guidelines.

As I can see, you managed to pull this off without adding any view code.
Impressive. ;-) I am going to ask you, though, to move some things to
the view code to keep the TAL clearer. Hope you can go along with that.

 review needs-fixing code

Cheers,
Henning

> === added file 'lib/lp/translations/stories/importqueue/xx-entry-details.txt'
> --- lib/lp/translations/stories/importqueue/xx-entry-details.txt 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/stories/importqueue/xx-entry-details.txt 2010-02-09 23:36:23 +0000
> @@ -0,0 +1,121 @@
> += Entry details =
> +
> +The translation import queue entry page shows various details about an
> +entry and its target that may be helpful in queue review.
> +
> + >>> from zope.security.proxy import removeSecurityProxy
> + >>> from lp.translations.model.translationimportqueue import (
> + ... TranslationImportQueue)
> +
> + >>> filename = 'po/foo.pot'
> +
> + >>> login(ANONYMOUS)
> + >>> queue = TranslationImportQueue()
> + >>> product = factory.makeProduct()
> + >>> removeSecurityProxy(product).official_rosetta = True

I wonder if removeSecurityProxy would be necessary here if you logged in
as foo.bar?

> + >>> trunk = product.getSeries('trunk')
> + >>> uploader = factory.makePerson()
> + >>> entry = queue.addOrUpdateEntry(
> + ... filename, '# empty', False, uploader, productseries=trunk)
> + >>> entry_url = canonical_url(entry, rootsite='translations')
> + >>> logout()
> +
> + >>> admin_browser.open(entry_url)
> + >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
> + >>> details_text = extract_text(details.renderContents())
> + ...

review: Needs Fixing (code)

[Henning Eggers (henninge)]

For the UI review, here are two screenshots, one of an entry for a template file with error outpout, one for a normal translation file without error output. The review is just about the stuff that comes under the actions.

http://people.canonical.com/~henninge/screenshots/import-queue-entry-info-pot.png
http://people.canonical.com/~henninge/screenshots/import-queue-entry-info-po.png

[Jeroen T. Vermeulen (jtv)]

Hi Henning, thank you for the surprise review! And a thorough job you did of it, too. Glad you like it, because this branch did contribute to a headache. :-)

> Actually, as you know, I have a branch ready that displays the
> error_output in a text field as part of the form, so this part might go
> out again, soon. No offence meant. ;-)

I considered that. But the error_output can be pages and pages of text in some cases. Viewing those in the queue UI isn't very convenient, and neither is scrolling through long and wide text in an input box. So I figured we might sometimes appreciate having it displayed more plainly. It's an extra, so I did put it at the bottom of the page.

> Yes, I agree with that, although a two-column layout wouldn't have been
> bad, either. The form would go on the left, the information on the
> right. But I am not sure if that is easy to do and would fit with our UI
> guidelines.

I'm not sure we can do that with generic forms. I did try it with the yui table layout, but had no luck.

In any case, this is nice and compact, no?

> As I can see, you managed to pull this off without adding any view code.
> Impressive. ;-) I am going to ask you, though, to move some things to
> the view code to keep the TAL clearer. Hope you can go along with that.

Quite right you are. TAL can be nice for prototyping though.

> > === added file 'lib/lp/translations/stories/importqueue/xx-entry-
> details.txt'
> > --- lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 1970-01-01 00:00:00 +0000
> > +++ lib/lp/translations/stories/importqueue/xx-entry-details.txt
> 2010-02-09 23:36:23 +0000
> > @@ -0,0 +1,121 @@
> > += Entry details =
> > +
> > +The translation import queue entry page shows various details about an
> > +entry and its target that may be helpful in queue review.
> > +
> > + >>> from zope.security.proxy import removeSecurityProxy
> > + >>> from lp.translations.model.translationimportqueue import (
> > + ... TranslationImportQueue)
> > +
> > + >>> filename = 'po/foo.pot'
> > +
> > + >>> login(ANONYMOUS)
> > + >>> queue = TranslationImportQueue()
> > + >>> product = factory.makeProduct()
> > + >>> removeSecurityProxy(product).official_rosetta = True
>
> I wonder if removeSecurityProxy would be necessary here if you logged in
> as foo.bar?

Maybe not, but it's yet another dependency on the test data with implicit meaning ("logging in as this user because it's an admin"). Stripping the proxy was easy enough.

> > +In that case, the product is also shown to have translatable series.
> > +
> > + >>> print details_text
> > + Upload attached to
> > + ...
> > + Project has translatable series.
> > + ...
> > +
>
> Would it be too much to list these series here?

Almost, but I did it anyway. :-) It now lists up to three series (with linkified names), and if there's more, it adds an ellipsis.

> > +The string "1 template" neatly adjusts to the actual number of
> > +templates.
> > +
> > + >>> login(ANONYMOUS)
> > + >>> template = factory.makePOTemplate(productseries=trunk)
> > + >>> logout()
> > +
> > + >>> admin_browser.open(entry_url)
> > + >>> detail...

[Henning Eggers (henninge)]

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

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

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

[Jeroen T. Vermeulen (jtv)]

UI reviewer: the "translatable series" text now links to the individual series. Updated screenshots at http://people.canonical.com/~jtv/translationimportqueueentry-info/

[Curtis Hovey (sinzui)]

This is fine to land. This is not the first form to but seondary, and possibly distracting information below the form. I had a minor concern that the use of un restrained left and right columns produces a large gutter of white-space between them. I do not see this since I keep my browser narrow, and U do not think you rosetta admins care.

We did discussed two webkit issues that are other bugs: the odd white space between form and buttons and the fact that this page is unreachable in webkit because the edit icons do not render (the fix is already in progress).

review: Approve (ui)

[Curtis Hovey (sinzui)]

The broken icon is because there is space between the anchor and the span in translation-import-queue-macros.pt. This fixes the issue:
                    <tal:block condition="entry/required:launchpad.Admin">
                      <a class="sprite edit"
                         tal:attributes="href entry/fmt:url"><span class="invisible-link">Change
                         this entry</span></a>
                    </tal:block>

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml	2010-01-20 20:36:13 +0000
+++ lib/lp/translations/browser/configure.zcml	2010-02-11 13:21:26 +0000
@@ -137,6 +137,7 @@
             permission="zope.Public"
             layer="canonical.launchpad.layers.TranslationsLayer"
             name="+portlet-details"
+            class="lp.translations.browser.translationimportqueue.TranslationImportQueueEntryView"
             template="../templates/translationimportqueueentry-portlet-details.pt"/>
         <browser:url
             for="lp.translations.interfaces.translationimportqueue.ITranslationImportQueue"

=== 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-11 13:21:26 +0000
@@ -0,0 +1,164 @@
+# 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, getUtility
+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.testing import TestCaseWithFactory
+from lp.translations.interfaces.translationimportqueue import (
+    ITranslationImportQueue)
+
+
+class TestTranslationImportQueueEntryView(TestCaseWithFactory):
+    """Tests for the queue entry review form."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestTranslationImportQueueEntryView, self).setUp(
+            '[e-mail]')
+        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 = self.factory.makeSourcePackage(packagename, series)
+        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:])
+
+        # Two translatable series.
+        extra_series = self.factory.makeProductSeries(product=product)
+        self.factory.makePOTemplate(productseries=extra_series)
+        series_text = view.product_translatable_series
+        # The links to the series are separated by a comma.
+        self.assertIn("</a>, <a ", series_text)
+        # The sentence ends in a period.
+        self.assertEqual('</a>.', series_text[-5:])
+
+        # Many translatable series.  The list is cut short; there's an
+        # ellipsis to indicate this.
+        series_count = len(product.translatable_series)
+        for counter in xrange(series_count, view.max_series_to_display + 1):
+            extra_series = self.factory.makeProductSeries(product=product)
+            self.factory.makePOTemplate(productseries=extra_series)
+        series_text = view.product_translatable_series
+        # The list is cut short.
+        displayed_series_count = series_text.count('</a>')
+        self.assertNotEqual(
+            len(product.translatable_series), displayed_series_count)
+        self.assertEqual(view.max_series_to_display, displayed_series_count)
+        # The list of links ends with an ellipsis.
+        self.assertEqual('</a>, ...', series_text[-9:])
+
+    def test_status_change_date(self):
+        # status_change_date describes the date of the entry's last
+        # status change.
+        series = self._makeProductSeries()
+        product = series.product
+        entry = self._makeEntry(productseries=series)
+        view = self._makeView(entry)
+
+        # If the date equals the upload date, there's no need to show
+        # anything.
+        self.assertEqual('', view.status_change_date)
+
+        # If there is a difference, there's a human-readable
+        # description.
+        UTC = timezone('UTC')
+        entry.dateimported = datetime(year=2005, month=11, day=29, tzinfo=UTC)
+        entry.date_status_changed = datetime(
+            year=2007, month=8, day=14, tzinfo=UTC)
+        self.assertEqual(
+            "Last changed on 2007-08-14.", view.status_change_date)
+
+
+def test_suite():
+    return TestLoader().loadTestsFromName(__name__)

=== 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-11 13:21:26 +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`."""
 
 __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)
+
+    @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')
+            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()
+
+    @property
     def next_url(self):
         """See `LaunchpadFormView`."""
         # The referer header we want is only available before the view's

=== added file 'lib/lp/translations/stories/importqueue/xx-entry-details.txt'
--- lib/lp/translations/stories/importqueue/xx-entry-details.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/stories/importqueue/xx-entry-details.txt	2010-02-11 13:21:26 +0000
@@ -0,0 +1,106 @@
+= Entry details =
+
+The translation import queue entry page shows various details about an
+entry and its target that may be helpful in queue review.
+
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> from lp.translations.model.translationimportqueue import (
+    ...     TranslationImportQueue)
+
+    >>> filename = 'po/foo.pot'
+
+    >>> login(ANONYMOUS)
+    >>> queue = TranslationImportQueue()
+    >>> product = factory.makeProduct()
+    >>> removeSecurityProxy(product).official_rosetta = True
+    >>> trunk = product.getSeries('trunk')
+    >>> uploader = factory.makePerson()
+    >>> entry = queue.addOrUpdateEntry(
+    ...     filename, '# empty', False, uploader, productseries=trunk)
+    >>> entry_url = canonical_url(entry, rootsite='translations')
+    >>> logout()
+
+    >>> admin_browser.open(entry_url)
+    >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
+    >>> details_text = extract_text(details)
+    >>> print details_text
+    Upload attached to ... trunk series.
+    This project...s license is open source.
+    Release series has no templates.
+    Project has no translatable series.
+    File po/foo.pot uploaded by ...
+
+The details include the project the entry is for, and who uploaded it.
+
+    >>> product.displayname in details_text
+    True
+    >>> uploader.displayname in details_text
+    True
+
+There's also a link to the file's contents.
+
+    >>> print admin_browser.getLink(filename).text
+    po/foo.pot
+    >>> print admin_browser.getLink(filename).url
+    http://...foo.pot
+
+
+== Existing templates ==
+
+If there are translatable templates in the series, this will be stated
+and there will be a link to the templates list.
+
+    >>> login(ANONYMOUS)
+    >>> template = factory.makePOTemplate(productseries=trunk)
+    >>> logout()
+
+    >>> admin_browser.open(entry_url)
+    >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
+    >>> details_text = extract_text(details)
+    >>> print details_text
+    Upload attached to
+    ...
+    Release series has 1 template.
+    ...
+
+    >>> print admin_browser.getLink('1 template').url
+    http...://translations.launchpad.dev/.../trunk/+templates
+
+    >>> admin_browser.getLink('1 template').click()
+    >>> print admin_browser.title
+    All templates : Translations : Series trunk : ...
+
+In that case, the product is also shown to have translatable series.
+
+    >>> print details_text
+    Upload attached to
+    ...
+    Project has translatable series: trunk.
+    ...
+
+
+== Source packages ==
+
+The portlet shows different (well, less) information for uploads
+attached to distribution packages.
+
+    >>> from lp.registry.model.distribution import DistributionSet
+
+    >>> login(ANONYMOUS)
+    >>> distro = DistributionSet().getByName('ubuntu')
+    >>> distroseries = distro.getSeries('hoary')
+    >>> packagename = factory.makeSourcePackageName(name='xpad')
+    >>> entry = queue.addOrUpdateEntry(
+    ...     filename, '# nothing', True, uploader, distroseries=distroseries,
+    ...     sourcepackagename=packagename)
+    >>> entry_url = canonical_url(entry, rootsite='translations')
+    >>> logout()
+
+This only shows the source package and the file.
+
+    >>> admin_browser.open(entry_url)
+    >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
+    >>> details_text = extract_text(details)
+    >>> print details_text
+    Upload attached to xpad in Ubuntu Hoary.
+    File po/foo.pot uploaded by ...

=== added file 'lib/lp/translations/stories/importqueue/xx-entry-error-output.txt'
--- lib/lp/translations/stories/importqueue/xx-entry-error-output.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/stories/importqueue/xx-entry-error-output.txt	2010-02-11 13:21:26 +0000
@@ -0,0 +1,46 @@
+= Translation import queue entry error output =
+
+The approval form translation import queue entries shows any error
+or warning output that the entry may have incurred.
+
+By default, this is nothing.
+
+    >>> from lp.translations.model.translationimportqueue import (
+    ...     TranslationImportQueue)
+
+    >>> def find_error_output(browser):
+    ...     """Find error-output section on page."""
+    ...     return find_tag_by_id(browser.contents, 'error-output')
+
+    >>> login(ANONYMOUS)
+    >>> queue = TranslationImportQueue()
+    >>> product = factory.makeProduct()
+    >>> trunk = product.getSeries('trunk')
+    >>> entry = queue.addOrUpdateEntry(
+    ...     'la.po', '# contents', False, product.owner, productseries=trunk)
+    >>> entry_url = canonical_url(entry, rootsite='translations')
+    >>> logout()
+
+    >>> admin_browser.open(entry_url)
+    >>> output_panel = find_error_output(admin_browser)
+    >>> print output_panel
+    None
+
+The section showing the output only shows up when there is output to
+show.
+
+    >>> entry.error_output = "Things went horribly wrong."
+    >>> admin_browser.open(entry_url)
+    >>> output_panel = find_error_output(admin_browser)
+    >>> print extract_text(output_panel)
+    Error output for this entry:
+    Things went horribly wrong.
+
+The output is properly HTML-escaped, so is safe to display in this way.
+
+    >>> entry.error_output = "<h1>Injection &amp; subterfuge</h1>"
+    >>> admin_browser.open(entry_url)
+    >>> output_panel = find_error_output(admin_browser)
+    >>> print output_panel.renderContents()
+    Error output for this entry:
+    ...&lt;h1&gt;Injection &amp;amp; subterfuge&lt;/h1&gt;...

=== modified file 'lib/lp/translations/templates/translationimportqueueentry-index.pt'
--- lib/lp/translations/templates/translationimportqueueentry-index.pt	2009-12-03 18:33:22 +0000
+++ lib/lp/translations/templates/translationimportqueueentry-index.pt	2010-02-11 13:21:26 +0000
@@ -106,6 +106,19 @@
                tal:attributes="value view/referrer_url" />
       </div>
     </div>
+
+    <tal:details replace="structure context/@@+portlet-details" />
+
+    <div tal:condition="context/error_output"
+         class="portlet"
+         id="error-output">
+      Error output for this entry:
+      <br />
+      <tt tal:content="context/error_output">
+        Horrible syntax error near line 192.
+      </tt>
+    </div>
+
   </div>
 
   </body>

=== modified file 'lib/lp/translations/templates/translationimportqueueentry-portlet-details.pt'
--- lib/lp/translations/templates/translationimportqueueentry-portlet-details.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/translations/templates/translationimportqueueentry-portlet-details.pt	2010-02-11 13:21:26 +0000
@@ -4,41 +4,48 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   omit-tag="">
 
-<div class="portlet" id="portlet-details">
-  <h2>Import entry details</h2>
-
-    <div class="portletBody">
-        <div class="portletContent">
-            <b>Origin:</b><br />
-            <a tal:condition="context/sourcepackage"
-               tal:content="context/sourcepackage/title"
-               tal:attributes="href string:${context/sourcepackage/fmt:url}"
-               >evolution in Ubuntu Hoary</a>
-            <a tal:condition="context/productseries"
-               tal:content="context/productseries/title"
-               tal:attributes="href string:${context/productseries/fmt:url}"
-               >Evolution Series: MAIN</a><br />
-            <b>Path:</b>
-            <a tal:attributes="href context/content/http_url"
-               tal:content="context/path">po/foo.pot</a><br />
-            <b>Uploader:</b><br />
-            <a tal:replace="structure context/importer/fmt:link"
-               >Mark Shuttleworth</a><br />
-            <b>Waiting since:</b>
-            <span
-              tal:attributes="title context/dateimported/fmt:datetime"
-              tal:content="context/dateimported/fmt:approximatedate">
-              2004-05-22
-            </span><br />
-            <b>Status:</b>
-            <span tal:replace="context/status/title"></span><br />
-            <b>Status changed:</b>
-            <span
-              tal:attributes="title context/date_status_changed/fmt:datetime"
-              tal:content="context/date_status_changed/fmt:approximatedate">
-              2004-05-21
-            </span><br />
-        </div>
-    </div>
+<div class="columns portlet" id="portlet-details">
+  <div class="two column left">
+    Upload attached to
+    <a tal:replace="structure view/import_target/fmt:link">
+        Evolution in Ubuntu Hoary</a>.
+
+    <tal:productseries condition="context/productseries">
+      <ul class="bulleted" tal:define="product context/productseries/product">
+        <li>
+          <tal:license
+            replace="structure product/license_status/description">
+            This project's license is open source.
+          </tal:license>
+        </li>
+        <li>
+          Release series has
+          <tal:templates replace="structure view/productseries_templates_link">
+              2 templates</tal:templates>.
+        </li>
+        <li>
+          <tal:series replace="structure view/product_translatable_series">
+            Project has translatable series: trunk, 0.1, 0.2, ...
+          </tal:series>
+        </li>
+      </ul>
+    </tal:productseries>
+  </div>
+
+  <div class="two column right">
+    File
+    <a tal:attributes="href context/content/http_url"
+       tal:content="context/path">po/messages.pot</a>
+    uploaded by
+    <a tal:replace="structure context/importer/fmt:link">
+      Arne Goetje
+    </a>
+    <tal:upload_date replace="context/dateimported/fmt:displaydate">
+      2010-02-10
+    </tal:upload_date>.
+    <tal:status_change replace="view/status_change_date">
+      Entry last changed on 2010-02-12.
+    </tal:status_change>
+  </div>
 </div>
 </tal:root>