Code review comment for lp:~adiroiban/launchpad/bug-525371

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Revision 10369 looks good to me (except the part where you update the getTranslationTemplates docstring to say it returns a list) but brings another problem to my attention: the purpose of the privileges check in POTemplateSubsetNavigation.traverse is a bit unclear. The comment also contains a typo ("is a is a").

You can also make the check easier to read by eliminating individual, simple cases:

if official_rosetta and potemplate.is_current:
    # This template is available for translation.
    return potemplate
elif check_permission('launchpad.Edit', potemplate):
    # User has special privileges for this template.
    return potemplate
else:
    raise NotFoundError(name)

Also, a bit higher up, I don't think the assertion for "unknown context" carries its weight. We have database constraints enforcing it. So might as well do something simpler such as:

if potemplate.distribution is None:
    product_or_distro = potemplate.productseries.product
else:
    product_or_distro = potemplate.distroseries.distribution
official_rosetta = product_or_distro.official_rosetta

« Back to merge proposal