-----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())
> + >>> 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
Nice trick to avoid depending on the generic strings. ;-)
> +
> +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.renderContents())
> + >>> 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.
> + ...
> +
Would it be too much to list these series here?
> +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)
> + >>> details = find_tag_by_id(admin_browser.contents, 'portlet-details')
> + >>> print extract_text(details.renderContents())
Why is the renderContents necessary here? Just for my own understanding.
> + Upload attached to
> + ...
> + Release series has 2 templates.
> + ...
> +
> +
> +== 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.renderContents())
> + >>> 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-09 23:36:23 +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.
As mentioned above, this might go out again.
> +
> +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."
I am surprised you can do that here. I couldn't do it in view code IIRC
and had to use setErrorOutput. What's different?
> + >>> admin_browser.open(entry_url)
> + >>> output_panel = find_error_output(admin_browser)
> + >>> print extract_text(output_panel.renderContents())
> + Error output for this entry:
> + Things went horribly wrong.
> +
> +The output is properly HTML-escaped, so is safe to display in this way.
Good thinking! ;)
> +
> + >>> entry.error_output = "