Code review comment for lp:~henninge/launchpad/bug-488765-oops-translations

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

Yes, I don't like that either. I was tempted by the easiness of the
way... ;-)

>
>
>> + 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. :)

I want bonus points!!!

All done, thanks for helping me improve this branch! The incremental
diff is attached.

Henning