Code review comment for lp:~adiroiban/launchpad/bug-509252-take-2

Hi Adi,
thanks for cleaning this up. Your code is almost good, I'd just ask two changes of you.

I agree with Michael's point that check_distroseries_translations_viewable should not go into browser_helper. DistroSeries.checkTranslationsViewable also is a bad place to raise exceptions because these should go into the browser code. So you were right in removing this.

The code in check_distroseries_translations_viewable is not a security check but an error handling routine that uses existing security checks. So I think it is perfectly valid in browser code.

Something that was already wrong before this branch but that should be cleaned up is this: DistroSeriesView.checkTranslationsViewable is only used in a tal:omit-tag and is therefore expected to return a bool. Even with your branch it raises exceptions. I don't know why that worked before but I guess this is some historic garbage floating around.

So my first request is that you find a new implementation for DistroSeriesView.checkTranslationsViewable that is a bool view property and does not raise exceptions. But maybe you can remove it all together if that one call site proves to be garbage.

My other request is that you move check_distroseries_translations_viewable into lib/lp/translations/browser/distroseries.py. I would have liked to see it in a Mixin that you can add to DistroSeriesNavigation and SourcePackageNavigation but that may not make too much sense. Think about it, though.

Finally I noticed DistroSeries.getDistroSeriesLanguageOrDummy which could be used to make the code in DistroSeriesNavigation.traverse_lang much shorter ;-)

Thanks. ;-)