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

Hi Adi,

Thanks a lot for the work on this. Overall, it's pretty good.

У пон, 17. 05 2010. у 11:07 +0000, Adi Roiban пише:

> == Tests ==
>
> lib/lp/registry/stories/webservice/xx-distroseries.txt
> lib/lp/registry/stories/webservice/xx-project-registry.txt
> lib/lp/registry/stories/webservice/xx-source-package.txt
> lib/lp/translations/stories/webservice/xx-potemplate.txt

It usually helps if you provide a bin/test command which runs them for
lazy reviewers to copy-paste them :)

> == Demo and Q/A ==
>
> To get the potemplates for a sourcepackage
> curl -ks
https://launchpad.dev/api/devel/ubuntu/hoary/+source/evolution/all_translation_templates
>
> To get the potemplates for a productseries
> curl -ks
https://launchpad.dev/api/devel/evolution/trunk/all_translation_templates
>
> To get the potemplates for a distroseries
> curl -ks
https://launchpad.dev/api/devel/ubuntu/hoary/all_translation_templates
>
> "all_translation_templates" are linked from the sourcepackage,
distroseries and productseries
> curl -ks
https://launchpad.dev/api/devel/ubuntu/hoary/+source/evolution
> curl -ks https://launchpad.dev/api/devel/ubuntu/hoary
> curl -ks https://launchpad.dev/api/devel/evolution/trunk
>
> = 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/interfaces/webservice.py

There are many more files with many more lint issues: you should try to
run it after you commit your final change, not before (if you've got
uncommitted changes, it lints just the uncommitted files). Please fix
all the issues.

> == Pyflakes notices ==
>
> lib/lp/translations/interfaces/webservice.py
> 8: 'IHasTranslationImports' imported but unused
> 8: 'ITranslationImportQueue' imported but unused
> 8: 'ITranslationImportQueueEntry' imported but unused
> 13: 'IPOTemplate' imported but unused
> 15: 'IPOFile' imported but unused

You should add the following to webservice.py if that's the intention:

__all__ = [
    'IHasTranslationImports',
    'IPOFile',
    'IPOTemplate',
    'ITranslationImportQueue',
    'ITranslationImportQueueEntry',
    ]

But, why is this necessary?

> warning: Not importing directory '/usr/share/pyshared/lazr': missing
__init__.py
> warning: Not importing directory '/usr/share/pyshared/lazr': missing
__init__.py
> warning: Not importing directory '/usr/share/pyshared/lazr': missing
__init__.py

This might be a bug in lazr that needs fixing. Can you please check and
file appropriate bug if it is?

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2010-05-17 12:43:53
+0000
> +++ lib/canonical/launchpad/security.py 2010-05-19 12:09:43
+0000
> @@ -61,8 +61,7 @@ from lp.hardwaredb.interfaces.hwdb impor
> from lp.services.worlddata.interfaces.language import ILanguage,
ILanguageSet
> from lp.translations.interfaces.languagepack import ILanguagePack
> from canonical.launchpad.interfaces.launchpad import (
> - IBazaarApplication, IHasBug, IHasDrivers, ILaunchpadCelebrities,
> - IPersonRoles)
> + IHasBug, IHasDrivers, ILaunchpadCelebrities, IPersonRoles)
> from lp.registry.interfaces.role import IHasOwner
> from lp.registry.interfaces.location import IPersonLocation
> from lp.registry.interfaces.mailinglist import IMailingListSet
> @@ -1159,6 +1158,11 @@ class AdminDistributionTranslations(Auth
> self.obj).checkAuthenticated(user))
>
>
> +class ViewPOTemplates(AnonymousAuthorization):
> + """Anyone can view an IPOTemplate."""
> + usedfor = IPOTemplate
> +
> +
> class AdminPOTemplateDetails(OnlyRosettaExpertsAndAdmins):
> """Controls administration of an `IPOTemplate`.
>
> @@ -1211,6 +1215,11 @@ class AddPOTemplate(OnlyRosettaExpertsAn
> usedfor = IProductSeries
>
>
> +class ViewPOFile(AnonymousAuthorization):
> + """Anyone can view an IPOFile."""
> + usedfor = IPOFile
> +
> +
> class EditPOFileDetails(EditByOwnersOrAdmins):
> usedfor = IPOFile

Why were these needed? Isn't this the default? Especially since they
don't seem to be registered anywhere.

=== modified file
'lib/lp/registry/stories/webservice/xx-distroseries.txt'
=== modified file
'lib/lp/registry/stories/webservice/xx-project-registry.txt'
=== modified file
'lib/lp/registry/stories/webservice/xx-source-package.txt'

It would be nice if we could unify these. And, I actually believe these
additions should be put into lib/lp/translations/stories/webservice
test instead: they are testing the IHasTranslationTemplates, not the
above objects.

> === modified file 'lib/lp/translations/interfaces/potemplate.py'
...
> source_file = Object(
> title=_('Source file for this translation template'),
> readonly=True, schema=ILibraryFileAlias)

I don't remember why we decided not to export this one? It could be
useful for things like xpipo conversion (with XPI-based translations,
it should contain a reference to imported en-US.xpi).

Though, we should probably just export it as the public URL, so not a
big deal (let's not block this branch on this).

> === modified file 'lib/lp/translations/interfaces/webservice.py'
> --- lib/lp/translations/interfaces/webservice.py 2009-07-17
02:25:09
+0000
> +++ lib/lp/translations/interfaces/webservice.py 2010-05-19
12:09:43
+0000
> @@ -1,9 +1,16 @@
> # Copyright 2009 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> +# pylint: disable-msg=W0611
> +
> """All the interfaces that are exposed through the webservice."""
>
> from lp.translations.interfaces.translationimportqueue import (
> IHasTranslationImports,
> ITranslationImportQueue,
> ITranslationImportQueueEntry)
> +
> +from lp.translations.interfaces.potemplate import (
> + IPOTemplate)
> +from lp.translations.interfaces.pofile import (
> + IPOFile)

See comment above on the lint output.

All else is good.

Cheers,
Danilo