Merge lp:~henninge/launchpad/bug-488765-oops-translations into lp:launchpad

[Henning Eggers]

Fixed Product:+translations view to always display something and not to oops on source packages.

[Henning Eggers (henninge)]

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

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/browser/product.py
  lib/lp/translations/stories/standalone/xx-product-translations.txt
  lib/lp/translations/templates/product-translations.pt

[Guilherme Salgado (salgado)]

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

review: Needs Fixing

[Henning Eggers (henninge)]

Am 15.01.2010 13:06, Guilherme Salgado schrieb:
> Review: Needs Fixing
> Hi Henning,
>
> I have just a few suggestions to improve this branch a bit.

Cool, thanks for doing the review.

>> === modified file 'lib/lp/translations/browser/product.py'
>> + if not isinstance(removeSecurityProxy(translatable), ProductSeries):
>
> You can use zope.security.proxy.isinstance here to avoid having to
> remove the security proxy.

Cool, I didn't know about this.

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

Yes, you are right on both accounts. I changed it to use providedBy.

>> === modified file 'lib/lp/translations/stories/standalone/xx-product-translations.txt'
>> +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.

Ah, that is not quite right. We don't show the recommendation even if
there is source package because atm we cannot treat a source package
like a product series in this respect. That will have to be fixed in
another branch (bug 507534).

I added a test that shows that the series has translatable source
package. This situation is what triggered the oops that this branch is
fixing but the display of recommendations is independent of a
translatable source package.

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

[Guilherme Salgado (salgado)]

review: Approve (code)

=== 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-16 08:51:16 +0000
@@ -11,14 +11,12 @@
     'ProductView',
     ]
 
-from zope.security.proxy import removeSecurityProxy
-
 from canonical.cachedproperty import cachedproperty
 from canonical.launchpad.webapp import (
     LaunchpadView, Link, canonical_url, enabled_with_permission)
+from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.menu import NavigationMenu
-from lp.registry.interfaces.product import IProduct
-from lp.registry.model.productseries import ProductSeries
+from lp.registry.interfaces.product import IProduct, IProductSeries
 from lp.registry.browser.product import ProductEditView
 from lp.translations.browser.translations import TranslationsMixin
 
@@ -85,32 +83,31 @@
     @cachedproperty
     def uses_translations(self):
         """Whether this product has translatable templates."""
-        return (self.context.official_rosetta and self.primary_translatable)
+        return (self.context.official_rosetta and 
+                self.primary_translatable is not None)
+
+    @cachedproperty
+    def no_translations_available(self):
+        """Has no translation templates but does support translations."""
+        return (self.context.official_rosetta and
+                self.primary_translatable is None)
+
+    @cachedproperty
+    def show_page_content(self):
+        """Whether the main content of the page should be shown."""
+        return (self.context.official_rosetta or
+                check_permission("launchpad.TranslationsAdmin", self.context))
 
     @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 IProductSeries.providedBy(translatable):
+            return None
+
+        return translatable
 
     @cachedproperty
     def untranslatable_series(self):

=== modified file 'lib/lp/translations/browser/tests/test_product_view.py'
--- lib/lp/translations/browser/tests/test_product_view.py	2009-07-17 02:25:09 +0000
+++ lib/lp/translations/browser/tests/test_product_view.py	2010-01-16 08:51:16 +0000
@@ -36,7 +36,8 @@
         pot = self.factory.makePOTemplate(
             distroseries=sourcepackage.distroseries,
             sourcepackagename=sourcepackage.sourcepackagename)
-        self.assertEquals(self.view.primary_translatable, {})
+        self.assertEquals(None, self.view.primary_translatable)
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
+

=== 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-16 08:51:16 +0000
@@ -168,32 +168,71 @@
     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('[e-mail]')
+    >>> 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
+
+At the moment, translatable source packages are not recommended, although
+the product is linked to one.
+
+    >>> source_package = find_tag_by_id(
+    ...     admin_browser.contents, 'portlet-translatable-packages')
+    >>> print extract_text(source_package)
+    All translatable distribution packages
+    “evolution” source package in Hoary
+
+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-16 08:51:16 +0000
@@ -14,8 +14,7 @@
   </div>
 
     <div metal:fill-slot="main"
-         tal:define="uses_translations view/uses_translations;
-                     admin_user context/required:launchpad.TranslationsAdmin">
+         tal:define="admin_user context/required:launchpad.TranslationsAdmin">
       <div class="translation-help-links">
         <a href="https://help.launchpad.net/Translations"
            id="link-to-translations-help"
@@ -23,26 +22,27 @@
         </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="view/no_translations_available">
+        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">
+      <tal:page_content condition="view/show_page_content">
+        <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>
@@ -98,7 +98,7 @@
             <div tal:replace="structure context/@@+rosetta-status-legend" />
           </div>
         </tal:translatable>
-      </tal:uses-translations>
+      </tal:page_content>
     </div>
   </body>
 </html>