Code review comment for lp:~danilo/launchpad/bug-516317

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

Hi Danilo,
thank you for this branch, great improvement in this. I am happy with you
landing this but please consider my few little comments.

 review approve code,ui

Cheers,
Henning

> === modified file 'lib/lp/registry/browser/configure.zcml'
> === modified file 'lib/lp/registry/browser/distribution.py'
> --- lib/lp/registry/browser/distribution.py 2010-12-16 19:59:20 +0000
> +++ lib/lp/registry/browser/distribution.py 2010-12-29 12:49:35 +0000
> @@ -430,7 +430,7 @@
> def configure_translations(self):
> text = 'Configure translations'
> summary = 'Allow users to provide translations for this project.'
> - return Link('+edit', text, summary, icon='edit')
> + return Link('+configure-translations', text, summary, icon='edit')

This may be missing a site="translations" parameter but I just checked the
other links on the project page and they all go to the root site. In fact they
seem to be available on all sites. All other links to +configure-translations
go to the translations site. I don't know if there is some kind of concept
behind this setup or if it is just an oversight.

For a distribution, +configure-translations is only available on the
translations site. This adds to my confusion here.

But I don't think you need to do anything about it now. ;)

>
>
> class DerivativeDistributionOverviewMenu(DistributionOverviewMenu):
>
> === modified file 'lib/lp/registry/browser/product.py'
> === modified file 'lib/lp/registry/configure.zcml'
> === modified file 'lib/lp/registry/interfaces/distribution.py'
> --- lib/lp/registry/interfaces/distribution.py 2010-12-01 11:26:57 +0000
> +++ lib/lp/registry/interfaces/distribution.py 2010-12-29 12:49:35 +0000
> @@ -284,9 +284,9 @@
> "get the full functionality of LP")
>
> translation_focus = Choice(
> - title=_("Translation Focus"),
> + title=_("Translation focus"),
> description=_(
> - "The DistroSeries that should get the translation effort focus."),
> + "The release series translators should focus on."),

"The release series *that* translators should focus on." is much better to read.

> required=False,
> vocabulary='FilteredDistroSeries')
>
> @@ -636,7 +636,8 @@
> archive = ubuntu.main_archive
> series = ubuntu.current_series
> print archive.getPublishedSources(exact_match=True,
> - source_name="apport", distro_series=series)[0].source_package_version
> + source_name="apport",
> + distro_series=series)[0].source_package_version
> """
> export_as_webservice_entry()
>
>
> === modified file 'lib/lp/registry/interfaces/product.py'
> --- lib/lp/registry/interfaces/product.py 2010-12-01 11:26:57 +0000
> +++ lib/lp/registry/interfaces/product.py 2010-12-29 12:49:35 +0000
> @@ -679,11 +679,11 @@
>
> translation_focus = exported(
> ReferenceChoice(
> - title=_("Translation Focus"), required=False,
> + title=_("Translation focus"), required=False,
> vocabulary='FilteredProductSeries',
> schema=IProductSeries,
> description=_(
> - 'The ProductSeries where translations are focused.')))
> + 'Project series translators should focus on.')))

Also, please add a "that".

>
> translatable_packages = Attribute(
> "A list of the source packages for this product that can be "
>
> === modified file 'lib/lp/translations/browser/configure.zcml'
> --- lib/lp/translations/browser/configure.zcml 2010-12-22 12:05:12 +0000
> +++ lib/lp/translations/browser/configure.zcml 2010-12-29 12:49:35 +0000
> @@ -809,12 +809,12 @@
> template="../templates/hastranslationimports-index.pt"
> layer="lp.translations.publisher.TranslationsLayer"/>
> <browser:page
> - name="+settings"
> + name="+configure-translations"
> for="lp.registry.interfaces.product.IProduct"
> + facet="overview"

AFAIK the facet attribute is old and useless. The layer attribute is calling
the shots nowadays. I guess you can just leave it out.

> class="lp.translations.browser.product.ProductSettingsView"
> permission="launchpad.TranslationsAdmin"
> - template="../templates/set-translators.pt"
> - layer="lp.translations.publisher.TranslationsLayer"/>
> + template="../templates/configure-translations.pt"/>
> <browser:pages
> for="lp.registry.interfaces.product.IProduct"
> permission="zope.Public"
> @@ -872,7 +872,7 @@
> for="lp.registry.interfaces.projectgroup.IProjectGroup"
> class="lp.translations.browser.project.ProjectSettingsView"
> permission="launchpad.TranslationsAdmin"
> - template="../templates/set-translators.pt"
> + template="../templates/configure-translations.pt"
> layer="lp.translations.publisher.TranslationsLayer"/>

See, only available on the TranslationsLayer.

>
> <!-- Distribution -->
> @@ -903,11 +903,11 @@
> class="lp.translations.browser.translations.HelpTranslateButtonView"
> permission="zope.Public"/>
> <browser:page
> - name="+settings"
> + name="+configure-translations"
> for="lp.registry.interfaces.distribution.IDistribution"
> class="lp.translations.browser.distribution.DistributionSettingsView"
> permission="launchpad.TranslationsAdmin"
> - template="../templates/set-translators.pt"
> + template="../templates/configure-translations.pt"
> layer="lp.translations.publisher.TranslationsLayer"/>

This one, too. As I said, I wonder if this is different from the
configuration of IProduct for a reason or just by accident.

> <browser:page
> for="lp.registry.interfaces.distribution.IDistribution"
>
> === modified file 'lib/lp/translations/browser/distribution.py'
> --- lib/lp/translations/browser/distribution.py 2010-09-23 14:33:51 +0000
> +++ lib/lp/translations/browser/distribution.py 2010-12-29 12:49:35 +0000
> @@ -44,7 +44,7 @@
>
> @enabled_with_permission('launchpad.TranslationsAdmin')
> def settings(self):
> - text = 'Change permissions'
> + text = 'Configure translations'
> return Link('+settings', text, icon='edit', site='translations')
>
> @enabled_with_permission('launchpad.TranslationsAdmin')
> @@ -129,13 +129,14 @@
>
>
> class DistributionSettingsView(TranslationsMixin, DistributionEditView):
> - label = "Set permissions and policies"
> - field_names = ["translationgroup", "translationpermission"]
> -
> - @property
> - def page_title(self):
> - return "Set translation permissions for %s" % (
> - self.context.displayname)
> + label = "Translations settings"

Yeah! Finally this page is converted to 3.0! ;) I think, though, that it is
custom to include the displayname of the context in the label.

> + page_title = "Settings"
> + field_names = [
> + "official_rosetta",
> + "translation_focus",
> + "translationgroup",
> + "translationpermission",
> + ]
>
> @property
> def cancel_url(self):
>
> === modified file 'lib/lp/translations/browser/product.py'
> === modified file 'lib/lp/translations/stories/distribution/xx-distribution-translations.txt'
> === modified file 'lib/lp/translations/stories/standalone/xx-potemplate-index.txt'
> === modified file 'lib/lp/translations/stories/translationfocus/xx-product-translationfocus.txt'
> === modified file 'lib/lp/translations/stories/translationgroups/10-distro-translation-group.txt'
> === modified file 'lib/lp/translations/stories/translationgroups/15-product-translation-group.txt'
> === modified file 'lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt'
> === modified file 'lib/lp/translations/stories/translationgroups/xx-change-translation-policy.txt'
> === renamed file 'lib/lp/translations/templates/set-translators.pt' => 'lib/lp/translations/templates/configure-translations.pt'
> === modified file 'lib/lp/translations/templates/distribution-translations.pt'
> === modified file 'lib/lp/translations/templates/hastranslationgroup-portlet-translation-groups-and-permission.pt'

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

iEYEARECAAYFAk0bW8YACgkQBT3oW1L17ih8sQCfc0gL4k9/DmvIABTKSHfmap3o
OdkAmwZP3Fm7/td15YKsknmrVErcDnOk
=Fjng
-----END PGP SIGNATURE-----