Launchpad itself

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

  1. bug-488765-oops-translations
  2. Merge into devel
Proposed by Henning Eggers
Status: Merged
Approved by: Henning Eggers
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~henninge/launchpad/bug-488765-oops-translations
Merge into: lp:launchpad
Diff against target: 236 lines (+88/-51)
4 files modified
lib/lp/translations/browser/product.py (+22/-25)
lib/lp/translations/browser/tests/test_product_view.py (+2/-1)
lib/lp/translations/stories/standalone/xx-product-translations.txt (+54/-15)
lib/lp/translations/templates/product-translations.pt (+10/-10)
To merge this branch: bzr merge lp:~henninge/launchpad/bug-488765-oops-translations
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) code Approve
Canonical Launchpad Engineering code Pending
Review via email: mp+17396@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) 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.

= 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

Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (10.5 KiB)

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
Revision history for this message
Henning Eggers (henninge) wrote :
Download full text (4.5 KiB)

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

Read more...

inc.diff
1=== modified file 'lib/lp/translations/browser/product.py'
2--- lib/lp/translations/browser/product.py 2010-01-15 07:39:43 +0000
3+++ lib/lp/translations/browser/product.py 2010-01-15 14:37:06 +0000
4@@ -11,14 +11,12 @@
5 'ProductView',
6 ]
7
8-from zope.security.proxy import removeSecurityProxy
9-
10 from canonical.cachedproperty import cachedproperty
11 from canonical.launchpad.webapp import (
12 LaunchpadView, Link, canonical_url, enabled_with_permission)
13+from canonical.launchpad.webapp.authorization import check_permission
14 from canonical.launchpad.webapp.menu import NavigationMenu
15-from lp.registry.interfaces.product import IProduct
16-from lp.registry.model.productseries import ProductSeries
17+from lp.registry.interfaces.product import IProduct, IProductSeries
18 from lp.registry.browser.product import ProductEditView
19 from lp.translations.browser.translations import TranslationsMixin
20
21@@ -85,7 +83,20 @@
22 @cachedproperty
23 def uses_translations(self):
24 """Whether this product has translatable templates."""
25- return (self.context.official_rosetta and self.primary_translatable)
26+ return (self.context.official_rosetta and
27+ self.primary_translatable is not None)
28+
29+ @cachedproperty
30+ def no_translations_available(self):
31+ """Has no translation templates but does support translations."""
32+ return (self.context.official_rosetta and
33+ self.primary_translatable is None)
34+
35+ @cachedproperty
36+ def show_page_content(self):
37+ """Whether the main content of the page should be shown."""
38+ return (self.context.official_rosetta or
39+ check_permission("launchpad.TranslationsAdmin", self.context))
40
41 @cachedproperty
42 def primary_translatable(self):
43@@ -93,7 +104,7 @@
44 """
45 translatable = self.context.primary_translatable
46
47- if not isinstance(removeSecurityProxy(translatable), ProductSeries):
48+ if not IProductSeries.providedBy(translatable):
49 return None
50
51 return translatable
52
53=== modified file 'lib/lp/translations/stories/standalone/xx-product-translations.txt'
54--- lib/lp/translations/stories/standalone/xx-product-translations.txt 2010-01-14 17:03:56 +0000
55+++ lib/lp/translations/stories/standalone/xx-product-translations.txt 2010-01-15 13:56:37 +0000
56@@ -222,6 +222,15 @@
57 >>> print find_translation_recommendation(admin_browser)
58 None
59
60+At the moment, translatable source packages are not recommended, although
61+the product is linked to one.
62+
63+ >>> source_package = find_tag_by_id(
64+ ... admin_browser.contents, 'portlet-translatable-packages')
65+ >>> print extract_text(source_package)
66+ All translatable distribution packages
67+ “evolution” source package in Hoary
68+
69 Instead a notice is displayed that the product has no translations.
70
71 >>> notice = first_tag_by_class(admin_browser.contents, 'notice')
72
73=== modified file 'lib/lp/translations/templates/product-translations.pt'
74--- lib/lp/translations/templates/product-translations.pt 2010-01-15 07:39:43 +0000
75+++ lib/lp/translations/templates/product-translations.pt 2010-01-15 14:39:59 +0000
76@@ -14,8 +14,7 @@
77 </div>
78
79 <div metal:fill-slot="main"
80- tal:define="official_rosetta context/official_rosetta;
81- admin_user context/required:launchpad.TranslationsAdmin">
82+ tal:define="admin_user context/required:launchpad.TranslationsAdmin">
83 <div class="translation-help-links">
84 <a href="https://help.launchpad.net/Translations"
85 id="link-to-translations-help"
86@@ -27,13 +26,12 @@
87 <tal:official_use replace="
88 structure
89 context/@@+portlet-not-using-launchpad"/>
90- <div tal:condition="python:not view.uses_translations and official_rosetta">
91+ <div tal:condition="view/no_translations_available">
92 There are no translations for this project.
93 </div>
94 </div>
95
96- <tal:uses-translations
97- condition="python:official_rosetta or admin_user">
98+ <tal:page_content condition="view/show_page_content">
99 <tal:translatable define="target view/primary_translatable">
100
101 <div class="yui-g">
102@@ -100,7 +98,7 @@
103 <div tal:replace="structure context/@@+rosetta-status-legend" />
104 </div>
105 </tal:translatable>
106- </tal:uses-translations>
107+ </tal:page_content>
108 </div>
109 </body>
110 </html>
Revision history for this message
Guilherme Salgado (salgado) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/browser/product.py'
2--- lib/lp/translations/browser/product.py 2009-10-26 18:40:04 +0000
3+++ lib/lp/translations/browser/product.py 2010-01-16 08:51:16 +0000
4@@ -11,14 +11,12 @@
5 'ProductView',
6 ]
7
8-from zope.security.proxy import removeSecurityProxy
9-
10 from canonical.cachedproperty import cachedproperty
11 from canonical.launchpad.webapp import (
12 LaunchpadView, Link, canonical_url, enabled_with_permission)
13+from canonical.launchpad.webapp.authorization import check_permission
14 from canonical.launchpad.webapp.menu import NavigationMenu
15-from lp.registry.interfaces.product import IProduct
16-from lp.registry.model.productseries import ProductSeries
17+from lp.registry.interfaces.product import IProduct, IProductSeries
18 from lp.registry.browser.product import ProductEditView
19 from lp.translations.browser.translations import TranslationsMixin
20
21@@ -85,32 +83,31 @@
22 @cachedproperty
23 def uses_translations(self):
24 """Whether this product has translatable templates."""
25- return (self.context.official_rosetta and self.primary_translatable)
26+ return (self.context.official_rosetta and
27+ self.primary_translatable is not None)
28+
29+ @cachedproperty
30+ def no_translations_available(self):
31+ """Has no translation templates but does support translations."""
32+ return (self.context.official_rosetta and
33+ self.primary_translatable is None)
34+
35+ @cachedproperty
36+ def show_page_content(self):
37+ """Whether the main content of the page should be shown."""
38+ return (self.context.official_rosetta or
39+ check_permission("launchpad.TranslationsAdmin", self.context))
40
41 @cachedproperty
42 def primary_translatable(self):
43- """Return a dictionary with the info for a primary translatable.
44-
45- If there is no primary translatable object, returns an empty
46- dictionary.
47-
48- The dictionary has the keys:
49- * 'title': The title of the translatable object.
50- * 'potemplates': a set of PO Templates for this object.
51- * 'base_url': The base URL to reach the base URL for this object.
52+ """Return the context's primary translatable if it's a product series.
53 """
54 translatable = self.context.primary_translatable
55- naked_translatable = removeSecurityProxy(translatable)
56-
57- if (translatable is None or
58- not isinstance(naked_translatable, ProductSeries)):
59- return {}
60-
61- return {
62- 'title': translatable.title,
63- 'potemplates': translatable.getCurrentTranslationTemplates(),
64- 'base_url': canonical_url(translatable)
65- }
66+
67+ if not IProductSeries.providedBy(translatable):
68+ return None
69+
70+ return translatable
71
72 @cachedproperty
73 def untranslatable_series(self):
74
75=== modified file 'lib/lp/translations/browser/tests/test_product_view.py'
76--- lib/lp/translations/browser/tests/test_product_view.py 2009-07-17 02:25:09 +0000
77+++ lib/lp/translations/browser/tests/test_product_view.py 2010-01-16 08:51:16 +0000
78@@ -36,7 +36,8 @@
79 pot = self.factory.makePOTemplate(
80 distroseries=sourcepackage.distroseries,
81 sourcepackagename=sourcepackage.sourcepackagename)
82- self.assertEquals(self.view.primary_translatable, {})
83+ self.assertEquals(None, self.view.primary_translatable)
84
85 def test_suite():
86 return unittest.TestLoader().loadTestsFromName(__name__)
87+
88
89=== modified file 'lib/lp/translations/stories/standalone/xx-product-translations.txt'
90--- lib/lp/translations/stories/standalone/xx-product-translations.txt 2009-11-01 20:18:32 +0000
91+++ lib/lp/translations/stories/standalone/xx-product-translations.txt 2010-01-16 08:51:16 +0000
92@@ -168,32 +168,71 @@
93 Language Untranslated Unreviewed Changed
94
95
96-== Download and upload links ==
97-
98-A logged-in user is invited to download translations.
99-
100- >>> def find_download_upload_invitation(browser):
101- ... """Find the text inviting the user to upload or download."""
102- ... tag = find_tag_by_id(browser.contents, 'downloadupload')
103+== Translation recommendation ==
104+
105+The page mentions which product series should be translated.
106+
107+ >>> def find_translation_recommendation(browser):
108+ ... """Find the text recommending to translate."""
109+ ... tag = find_tag_by_id(
110+ ... browser.contents, 'translation-recommendation')
111 ... if tag is None:
112 ... return None
113 ... return extract_text(tag.renderContents())
114
115 >>> product_url = 'http://translations.launchpad.dev/evolution'
116
117+That's all an anonymous user will see.
118+
119+ >>> anon_browser.open(product_url)
120+ >>> print find_translation_recommendation(anon_browser)
121+ Launchpad currently recommends translating Evolution trunk series.
122+
123+A logged-in user is also invited to download translations.
124+
125 >>> user_browser.open(product_url)
126- >>> print find_download_upload_invitation(user_browser)
127+ >>> print find_translation_recommendation(user_browser)
128+ Launchpad currently recommends translating Evolution trunk series.
129 You can also download translations for trunk.
130
131-An anonymous user does not see this invitation.
132-
133- >>> anon_browser.open(product_url)
134- >>> print find_download_upload_invitation(anon_browser)
135- None
136-
137 A user with upload rights sees the invitation not just to download but
138 to upload as well.
139
140 >>> admin_browser.open(product_url)
141- >>> print find_download_upload_invitation(admin_browser)
142+ >>> print find_translation_recommendation(admin_browser)
143+ Launchpad currently recommends translating Evolution trunk series.
144 You can also download or upload translations for trunk.
145+
146+If there is no translatable series, no such recommendation is displayed.
147+A series is not translatable if all templates are disabled. We need to jump
148+through some hoops to create that situation.
149+
150+ >>> login('foo.bar@canonical.com')
151+ >>> from zope.component import getUtility
152+ >>> from lp.registry.interfaces.product import IProductSet
153+ >>> evotrunk = getUtility(IProductSet).getByName(
154+ ... 'evolution').getSeries('trunk')
155+ >>> from lp.translations.interfaces.potemplate import IPOTemplateSet
156+ >>> potemplates = getUtility(IPOTemplateSet).getSubset(
157+ ... productseries=evotrunk, iscurrent=True)
158+ >>> for potemplate in potemplates:
159+ ... potemplate.iscurrent = False
160+ >>> logout()
161+ >>> admin_browser.open(product_url)
162+ >>> print find_translation_recommendation(admin_browser)
163+ None
164+
165+At the moment, translatable source packages are not recommended, although
166+the product is linked to one.
167+
168+ >>> source_package = find_tag_by_id(
169+ ... admin_browser.contents, 'portlet-translatable-packages')
170+ >>> print extract_text(source_package)
171+ All translatable distribution packages
172+ “evolution” source package in Hoary
173+
174+Instead a notice is displayed that the product has no translations.
175+
176+ >>> notice = first_tag_by_class(admin_browser.contents, 'notice')
177+ >>> print extract_text(notice)
178+ There are no translations for this project.
179
180=== modified file 'lib/lp/translations/templates/product-translations.pt'
181--- lib/lp/translations/templates/product-translations.pt 2009-12-16 15:21:36 +0000
182+++ lib/lp/translations/templates/product-translations.pt 2010-01-16 08:51:16 +0000
183@@ -14,8 +14,7 @@
184 </div>
185
186 <div metal:fill-slot="main"
187- tal:define="uses_translations view/uses_translations;
188- admin_user context/required:launchpad.TranslationsAdmin">
189+ tal:define="admin_user context/required:launchpad.TranslationsAdmin">
190 <div class="translation-help-links">
191 <a href="https://help.launchpad.net/Translations"
192 id="link-to-translations-help"
193@@ -23,26 +22,27 @@
194 </a>
195 <div></div><!-- to clear-up all floats -->
196 </div>
197- <div class="top-portlet">
198+ <div class="top-portlet notice">
199 <tal:official_use replace="
200 structure
201 context/@@+portlet-not-using-launchpad"/>
202+ <div tal:condition="view/no_translations_available">
203+ There are no translations for this project.
204+ </div>
205 </div>
206
207- <tal:uses-translations
208- condition="python:uses_translations or admin_user">
209- <tal:translatable define="target context/primary_translatable">
210+ <tal:page_content condition="view/show_page_content">
211+ <tal:translatable define="target view/primary_translatable">
212
213 <div class="yui-g">
214 <div class="yui-u first">
215 <div class="portlet">
216 <h3>Translation details</h3>
217- <p tal:condition="target">
218+ <p tal:condition="target" id="translation-recommendation">
219 Launchpad currently recommends translating
220 <tal:target replace="structure target/fmt:link"
221 >trunk</tal:target>.
222- <span id="downloadupload"
223- tal:condition="context/required:launchpad.AnyPerson">
224+ <span tal:condition="context/required:launchpad.AnyPerson">
225 You can also
226 <a tal:attributes="href target/fmt:url/+export"
227 >download</a>
228@@ -98,7 +98,7 @@
229 <div tal:replace="structure context/@@+rosetta-status-legend" />
230 </div>
231 </tal:translatable>
232- </tal:uses-translations>
233+ </tal:page_content>
234 </div>
235 </body>
236 </html>