Hi Henning,

I have just a few suggestions to improve this branch a bit.

 review needs-fixing

On Thu, 2010-01-14 at 17:22 +0000, Henning Eggers wrote:
> = Bug 488765 =
> 
> A product has an attribute primary_translatable that returns the
> product series or source package that should be translated. Returning
> source packages here seems to be a newer feature that broke a
> product's +translations page if the primary_translatable was a source
> package because the template assumed it's a product series.
> 
> == Proposed fx ==
> 
> I filed bug 507534 about that behaviour and fixed the page by
> filtering out source packages in the view so that only product series
> are displayed by the template.
> 
> I also discovered that bug 371632 can be fixed with this.
> 
> == Implementation details ==
> 
> lib/lp/translations/browser/product.py
> 
>  * The view already had a property "primary_translatable" that was
> obviously unused and returned a dictionary. I re-used it to return the
> primary_translatable of the context or None if it is not a product
> series.
> 
> lib/lp/translations/stories/standalone/xx-product-translations.txt
> 
>  * The part that only checked for up/download links was expanded to
> check for the whole recommendation section. This way it can be tested
> for complete absence if no primary_translatable is available.
> 
>  * This test did reproducce the error because evolution is linked to a
> translatable source package in the sample data, so
> Product.primary_translatable returns a source package when all
> templates in the product series have been disabled.
> 
>  * Disabling all the templates is a bit of noise but needed to
> reproduce the error.
> 
>  * Appended a test to show the notice when no translations are
> available.
> 
> lib/lp/translations/templates/product-translations.pt
> 
>  * Use view/primary_translatable instead of context's.
> 
>  * Changed some ids and classes needed for testing.
> 
>  * Added notice for when no translations are available.
> 
>  * Changed conditions so that page is never empty.
> 
> 
> == Test ==
> 
> bin/test -vvct product-translations
> 
> == Demo/QA ==
> 
> On launchpad.dev:
>  1. Disable all templates for evolution trunk.
>  2. Got to https://translations.launchpad.dev/evolution
>  3. You should not get an oops but a notice that there are no
> translations for this project.
> 
> To reproduce on staging you'll need a project that is linked to a
> source pacakage with translations.
> 


> === modified file 'lib/lp/translations/browser/product.py'
> --- lib/lp/translations/browser/product.py	2009-10-26 18:40:04 +0000
> +++ lib/lp/translations/browser/product.py	2010-01-15 07:41:14 +0000
> @@ -89,28 +89,14 @@
>  
>      @cachedproperty
>      def primary_translatable(self):
> -        """Return a dictionary with the info for a primary translatable.
> -
> -        If there is no primary translatable object, returns an empty
> -        dictionary.
> -
> -        The dictionary has the keys:
> -         * 'title': The title of the translatable object.
> -         * 'potemplates': a set of PO Templates for this object.
> -         * 'base_url': The base URL to reach the base URL for this object.
> +        """Return the context's primary translatable if it's a product series.
>          """
>          translatable = self.context.primary_translatable
> -        naked_translatable = removeSecurityProxy(translatable)
> -
> -        if (translatable is None or
> -            not isinstance(naked_translatable, ProductSeries)):
> -            return {}
> -
> -        return {
> -            'title': translatable.title,
> -            'potemplates': translatable.getCurrentTranslationTemplates(),
> -            'base_url': canonical_url(translatable)
> -            }
> +
> +        if not isinstance(removeSecurityProxy(translatable), ProductSeries):

You can use zope.security.proxy.isinstance here to avoid having to
remove the security proxy.  

Actually, you should use IProductSeries.providedBy(translatable), which
would allow you to not import ProductSeries here, as that must not be
done.  I wonder why the import fascist doesn't emit a warning about this
import...

> +            return None
> +
> +        return translatable
>  
>      @cachedproperty
>      def untranslatable_series(self):
> 
> === modified file 'lib/lp/translations/stories/standalone/xx-product-translations.txt'
> --- lib/lp/translations/stories/standalone/xx-product-translations.txt	2009-11-01 20:18:32 +0000
> +++ lib/lp/translations/stories/standalone/xx-product-translations.txt	2010-01-15 07:41:14 +0000
> @@ -168,32 +168,62 @@
>      Language                   Untranslated    Unreviewed       Changed
>  
> 
> -== Download and upload links ==
> -
> -A logged-in user is invited to download translations.
> -
> -    >>> def find_download_upload_invitation(browser):
> -    ...     """Find the text inviting the user to upload or download."""
> -    ...     tag = find_tag_by_id(browser.contents, 'downloadupload')
> +== Translation recommendation ==
> +
> +The page mentions which product series should be translated.
> +
> +    >>> def find_translation_recommendation(browser):
> +    ...     """Find the text recommending to translate."""
> +    ...     tag = find_tag_by_id(
> +    ...         browser.contents, 'translation-recommendation')
>      ...     if tag is None:
>      ...         return None
>      ...     return extract_text(tag.renderContents())
>  
>      >>> product_url = 'http://translations.launchpad.dev/evolution'
>  
> +That's all an anonymous user will see.
> +
> +    >>> anon_browser.open(product_url)
> +    >>> print find_translation_recommendation(anon_browser)
> +    Launchpad currently recommends translating Evolution trunk series.
> +
> +A logged-in user is also invited to download translations.
> +
>      >>> user_browser.open(product_url)
> -    >>> print find_download_upload_invitation(user_browser)
> +    >>> print find_translation_recommendation(user_browser)
> +    Launchpad currently recommends translating Evolution trunk series.
>      You can also download translations for trunk.
>  
> -An anonymous user does not see this invitation.
> -
> -    >>> anon_browser.open(product_url)
> -    >>> print find_download_upload_invitation(anon_browser)
> -    None
> -
>  A user with upload rights sees the invitation not just to download but
>  to upload as well.
>  
>      >>> admin_browser.open(product_url)
> -    >>> print find_download_upload_invitation(admin_browser)
> +    >>> print find_translation_recommendation(admin_browser)
> +    Launchpad currently recommends translating Evolution trunk series.
>      You can also download or upload translations for trunk.
> +
> +If there is no translatable series, no such recommendation is displayed.
> +A series is not translatable if all templates are disabled. We need to jump
> +through some hoops to create that situation.
> +
> +    >>> login('<email address hidden>')
> +    >>> from zope.component import getUtility
> +    >>> from lp.registry.interfaces.product import IProductSet
> +    >>> evotrunk = getUtility(IProductSet).getByName(
> +    ...     'evolution').getSeries('trunk')
> +    >>> from lp.translations.interfaces.potemplate import IPOTemplateSet
> +    >>> potemplates = getUtility(IPOTemplateSet).getSubset(
> +    ...     productseries=evotrunk, iscurrent=True)
> +    >>> for potemplate in potemplates:
> +    ...     potemplate.iscurrent = False
> +    >>> logout()
> +    >>> admin_browser.open(product_url)
> +    >>> print find_translation_recommendation(admin_browser)
> +    None

I think it'd be nice to show here that the product has a translatable
source package and explain that is only in that case that we don't show
recommendations.  It's also important to do that because the test
assumes there's a translatable source package associated to evolution
(as you described in the cover letter), but the test itself doesn't make
it clear nor does it assert that.

> +
> +Instead a notice is displayed that the product has no translations.
> +
> +    >>> notice = first_tag_by_class(admin_browser.contents, 'notice')
> +    >>> print extract_text(notice)
> +    There are no translations for this project.
> 

> === modified file 'lib/lp/translations/templates/product-translations.pt'
> --- lib/lp/translations/templates/product-translations.pt	2009-12-16 15:21:36 +0000
> +++ lib/lp/translations/templates/product-translations.pt	2010-01-15 07:41:14 +0000
> @@ -14,7 +14,7 @@
>    </div>
>  
>      <div metal:fill-slot="main"
> -         tal:define="uses_translations view/uses_translations;
> +         tal:define="official_rosetta context/official_rosetta;
>                       admin_user context/required:launchpad.TranslationsAdmin">
>        <div class="translation-help-links">
>          <a href="https://help.launchpad.net/Translations"
> @@ -23,26 +23,28 @@
>          </a>
>          <div></div><!-- to clear-up all floats -->
>        </div>
> -      <div class="top-portlet">
> +      <div class="top-portlet notice">
>          <tal:official_use replace="
>              structure
>              context/@@+portlet-not-using-launchpad"/>
> +        <div tal:condition="python:not view.uses_translations and official_rosetta">

Using python: in tal is strongly discouraged, and in this case we can
easily use a view property.


> +        There are no translations for this project.
> +        </div>
>        </div>
>  
>        <tal:uses-translations
> -         condition="python:uses_translations or admin_user">
> -        <tal:translatable define="target context/primary_translatable">
> +         condition="python:official_rosetta or admin_user">

This could also be moved into a view property, for bonus points. :)

> +        <tal:translatable define="target view/primary_translatable">
>  
>            <div class="yui-g">
>              <div class="yui-u first">
>                <div class="portlet">
>                  <h3>Translation details</h3>
> -                <p tal:condition="target">
> +                <p tal:condition="target" id="translation-recommendation">
>                    Launchpad currently recommends translating
>                    <tal:target replace="structure target/fmt:link"
>                                >trunk</tal:target>.
> -                  <span id="downloadupload"
> -                       tal:condition="context/required:launchpad.AnyPerson">
> +                  <span tal:condition="context/required:launchpad.AnyPerson">
>                      You can also
>                      <a tal:attributes="href target/fmt:url/+export"
>                         >download</a>

-- 
Guilherme Salgado <email address hidden>