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

= 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

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

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

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

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

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

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

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

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>