Merge lp:~adiroiban/launchpad/bug-525371 into lp:launchpad

Hi Adi,

It's really great that you're doing this. I'm reading it through and it looks good to me so far. One note though about the testing in xx-distroseries.txt.

You've got the problem here that you have to rely on existing test data, which can be a bit arbitrary. For example, the fact that there are 6 templates in the distroseries is pretty meaningless in the context of the test. But it's a lot of trouble to set up a fresh distroseries with fresh translation templates, especially if the existing tests don't do it either.

So here's an idea: instead of printing the exact number of templates, why not show that int(all_translation_templates['total_size']) is identical to len(list(getUtility(ILaunchpadCelebrities).ubuntu.getSeries('hoary'))).

Jeroen

Hi,

I have improved the tests to check for templates count from the db.

Here is the latest diff:

=== modified file 'lib/lp/registry/stories/webservice/xx-distroseries.txt'
--- lib/lp/registry/stories/webservice/xx-distroseries.txt 2010-05-17 09:35:18 +0000
+++ lib/lp/registry/stories/webservice/xx-distroseries.txt 2010-05-18 14:51:40 +0000
@@ -107,9 +107,19 @@
 All templates associated to a distribution series are available at the
 'all_translation_templates' collection link.

+ >>> from zope.component import getUtility
+ >>> from canonical.launchpad.interfaces.launchpad import (
+ ... ILaunchpadCelebrities)
+ >>> from lp.translations.interfaces.potemplate import IPOTemplateSet
+ >>> login('<email address hidden>')
+ >>> hoary = getUtility(ILaunchpadCelebrities).ubuntu.getSeries('hoary')
+ >>> templates = getUtility(IPOTemplateSet).getSubset(distroseries=hoary)
+ >>> db_count = len(list(templates))
+ >>> logout()
     >>> all_translation_templates = anon_webservice.get(
     ... '/ubuntu/hoary/all_translation_templates').jsonBody()
- >>> print all_translation_templates['total_size']
- 6
+ >>> api_count = all_translation_templates['total_size']
+ >>> api_count == db_count
+ True
     >>> print(all_translation_templates['entries'][0]['resource_type_link'])
     http://.../#potemplate

=== modified file 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
--- lib/lp/registry/stories/webservice/xx-project-registry.txt 2010-05-17 09:35:18 +0000
+++ lib/lp/registry/stories/webservice/xx-project-registry.txt 2010-05-18 15:10:45 +0000
@@ -897,11 +897,20 @@
 "all_translation_templates" will list all POTemplates associated with this
 product series.

+
+ >>> from zope.component import getUtility
+ >>> from lp.translations.interfaces.potemplate import IPOTemplateSet
+ >>> login('<email address hidden>')
+ >>> templates = getUtility(
+ ... IPOTemplateSet).getSubset(productseries=foobadoo)
+ >>> db_count = len(list(templates))
+ >>> logout()
     >>> all_translation_templates = anon_webservice.get(
     ... babadoo_foobadoo[
     ... 'all_translation_templates_collection_link']).jsonBody()
- >>> print all_translation_templates['total_size']
- 1
+ >>> api_count = all_translation_templates['total_size']
+ >>> api_count == db_count
+ True
     >>> print(all_translation_templates['entries'][0]['resource_type_link'])
     http://.../#potemplate

=== modified file 'lib/lp/registry/stories/webservice/xx-source-package.txt'
--- lib/lp/registry/stories/webservice/xx-source-package.txt 2010-05-17 09:35:18 +0000
+++ lib/lp/registry/stories/webservice/xx-source-package.txt 2010-05-18 15:23:51 +0000
@@ -130,10 +130,27 @@
 All translation templates for a source package are available at the
 'all_translation_templates' collection link.

+
+ >>> from zope.component import getUtility
+ >>> from canonical.launchpad.interfaces.launchpad import (
+ ... ILaunchpadCelebrities)
+ >>> from lp.registry.interfaces.sourcepackagename import (
+ ... ISourcePackageNameSet)
+ >>> from lp.translations.interfaces.potemplate import IPOTemplateSet
+ >...

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

On Wed, 2010-05-19 at 14:43 +0000, Данило Шеган wrote:
> 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 :)

Sorry. I will add it with the next MP.

./bin/test -t xx-potemplate.txt -t xx-distroseries.txt \
 -t xx-project-registry.txt -t xx-source-package.txt

:)

[snip]
> > = 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.

I have fixed some of the other warning.

There are still some warnings for code like:

    @operation_parameters(
        pocket=Choice(
            title=_("Pocket"), required=True,
            vocabulary=DBEnumeratedType))

and it looks fine to me, but pylint complains about:

lib/lp/registry/interfaces/sourcepackage.py
    191: [C0322, ISourcePackage.getBranch] Operator not preceded by a
space
    required=True,
    ^
    vocabulary=DBEnumeratedType))

> > == 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?

Only interfaces listed in webservices.py will be exported by
lazr.restful.

Adding __all__ will still generate Pyflakes notices as they are still
unused in that file.

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

Is is not a problem in lazr. I have the same problem with:
'/usr/share/pyshared/google': missing __init__.py

Those file were installed as dependencies for ubuntuone-client-gnome by
python-protobuf and python-lazr-uri, python-lazr-restfulclient and are
maintained by main Ubuntu developers... not in the LP PPA.

> > === modified file 'lib/...

Hi Adi,

Thanks for the updates. There's still just a little bit more to do :)

У сре, 19. 05 2010. у 20:13 +0000, Adi Roiban пише:

> There are still some warnings for code like:
>
> @operation_parameters(
> pocket=Choice(
> title=_("Pocket"), required=True,
> vocabulary=DBEnumeratedType))
>
> and it looks fine to me, but pylint complains about:
>
> lib/lp/registry/interfaces/sourcepackage.py
> 191: [C0322, ISourcePackage.getBranch] Operator not preceded by a
> space
> required=True,
> ^
> vocabulary=DBEnumeratedType))
>
>
> > > == 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?
>
> Only interfaces listed in webservices.py will be exported by
> lazr.restful.
>
> Adding __all__ will still generate Pyflakes notices as they are still
> unused in that file.

It actually fixed it for me. I'm attaching a patch which fixes all the
lint issues I've seen. It would be nicer if you did that instead :)

Also, "operator not preceded by space" lint issues in this particular
context seem to be opposed to our style guide. It would be best to
raise this on launchpad-dev mailing list to come to an agreement and to
see if we can fix either pylint or style guide.

> > > 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?
>
> Is is not a problem in lazr. I have the same problem with:
> '/usr/share/pyshared/google': missing __init__.py
>
> Those file were installed as dependencies for ubuntuone-client-gnome by
> python-protobuf and python-lazr-uri, python-lazr-restfulclient and are
> maintained by main Ubuntu developers... not in the LP PPA.

Well, LAZR is a library shared between Launchpad and UbuntuOne. It's
still a bug that would need to be filed. I've worked-around it locally
by doing "sudo touch /usr/share/pyshared/lazr/__init__.py", and there is
probably no reason why the package shouldn't do the same for everybody.

> Should POTemplate and POFile API be available only to authenticated
> users?

Not, it's fine if they are anonymously available.

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

On Thu, 2010-05-20 at 16:33 +0000, Данило Шеган wrote:
> Hi Adi,
>
> Thanks for the updates. There's still just a little bit more to do :)
>
> У сре, 19. 05 2010. у 20:13 +0000, Adi Roiban пише:
>
> > There are still some warnings for code like:
> >
> > @operation_parameters(
> > pocket=Choice(
> > title=_("Pocket"), required=True,
> > vocabulary=DBEnumeratedType))
> >
> > and it looks fine to me, but pylint complains about:
> >
> > lib/lp/registry/interfaces/sourcepackage.py
> > 191: [C0322, ISourcePackage.getBranch] Operator not preceded by a
> > space
> > required=True,
> > ^
> > vocabulary=DBEnumeratedType))
> >
> >
> > > > == 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?
> >
> > Only interfaces listed in webservices.py will be exported by
> > lazr.restful.
> >
> > Adding __all__ will still generate Pyflakes notices as they are still
> > unused in that file.
>
> It actually fixed it for me. I'm attaching a patch which fixes all the
> lint issues I've seen. It would be nicer if you did that instead :)

I have no idea why this is not fixing for me.

Here is the output for `make lint` after applying your patch:

= 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/registry/interfaces/distroseries.py
  lib/lp/registry/interfaces/sourcepackage.py
  lib/lp/translations/interfaces/webservice.py
  lib/lp/translations/model/potemplate.py

== Pyflakes notices ==

lib/lp/translations/interfaces/webservice.py
    16: 'IHasTranslationImports' imported but unused
    16: 'ITranslationImportQueue' imported but unused
    16: 'ITranslationImportQueueEntry' imported but unused
    21: 'IPOTemplate' imported but unused
    23: 'IPOFile' imported but unused

> Also, "operator not preceded by space" lint issues in this particular
> context seem to be opposed to our style guide. It would be best to
> raise this on launchpad-dev mailing list to come to an agreement and to
> see if we can fix either pylint or style guide.

I have sent and email to lp-dev ML.

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

Hey Adi, we are getting very close. Just a few more small issues to
fix.

У чет, 20. 05 2010. у 18:00 +0000, Adi Roiban пише:

> > It actually fixed it for me. I'm attaching a patch which fixes all the
> > lint issues I've seen. It would be nicer if you did that instead :)
>
> I have no idea why this is not fixing for me.
>
> Here is the output for `make lint` after applying your patch:
>
> = 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/registry/interfaces/distroseries.py
> lib/lp/registry/interfaces/sourcepackage.py
> lib/lp/translations/interfaces/webservice.py
> lib/lp/translations/model/potemplate.py
>
> == Pyflakes notices ==
>
> lib/lp/translations/interfaces/webservice.py
> 16: 'IHasTranslationImports' imported but unused
> 16: 'ITranslationImportQueue' imported but unused
> 16: 'ITranslationImportQueueEntry' imported but unused
> 21: 'IPOTemplate' imported but unused
> 23: 'IPOFile' imported but unused

Hum, very interesting. Perhaps it's a problem with pyflakes
incompatibilities? (FWIW, it seems your branch now specifies __all__
twice)

FWIW, defining __all__ fixes pyflakes warnings for me, but not the
pylint warnings.

> > Also, "operator not preceded by space" lint issues in this particular
> > context seem to be opposed to our style guide. It would be best to
> > raise this on launchpad-dev mailing list to come to an agreement and to
> > see if we can fix either pylint or style guide.
>
> I have sent and email to lp-dev ML.

Cool. It seems everybody agrees that we should not change the
styleguide, but instead not use pylint as is. You can ignore those
warnings and revert that bit of my patch. It'd still be very useful to
resolve the pyflakes warnings that you are getting, though.

> I have filled a bug report for that:
> https://bugs.edge.launchpad.net/ubuntu/+source/lazr.restfulclient/+bug/583426

Excellent, thanks a lot.

> [snip]
> >
> > > > > === 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).
> > >
> > > I will leave it for another branch, but for me, it would make sense if
> > > this attribute would be used for all translation templates, not only
> > > XPI.
> >
> > Well, we can reconstruct the gettext POT file completely from what we
> > have in the DB. Thus, it's not needed.
>
> My note was about creating a more consistent API so that in the API
> source_file (or whatever name we choose for it) would return the
> template file for all formats, not only XPI.

Sure...

> Instead of...

On Thu, 2010-05-20 at 18:48 +0000, Данило Шеган wrote:
> Hey Adi, we are getting very close. Just a few more small issues to
> fix.
>
> У чет, 20. 05 2010. у 18:00 +0000, Adi Roiban пише:
>
> > > It actually fixed it for me. I'm attaching a patch which fixes all the
> > > lint issues I've seen. It would be nicer if you did that instead :)
> >
> > I have no idea why this is not fixing for me.
> >
> > Here is the output for `make lint` after applying your patch:
> >
> > = 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/registry/interfaces/distroseries.py
> > lib/lp/registry/interfaces/sourcepackage.py
> > lib/lp/translations/interfaces/webservice.py
> > lib/lp/translations/model/potemplate.py
> >
> > == Pyflakes notices ==
> >
> > lib/lp/translations/interfaces/webservice.py
> > 16: 'IHasTranslationImports' imported but unused
> > 16: 'ITranslationImportQueue' imported but unused
> > 16: 'ITranslationImportQueueEntry' imported but unused
> > 21: 'IPOTemplate' imported but unused
> > 23: 'IPOFile' imported but unused
>
> Hum, very interesting. Perhaps it's a problem with pyflakes
> incompatibilities? (FWIW, it seems your branch now specifies __all__
> twice)
>
> FWIW, defining __all__ fixes pyflakes warnings for me, but not the
> pylint warnings.

I have removed the duplicate __all__ and only leave it before the
imports.

[snip]
> > > Add __all__: have you tried it at all? It totally works for me, and I'd
> > > be very surprised if it doesn't work for you. Also, __all__ is required
> > > in all our modules as well (importfascist used to check that before,
> > > maybe it doesn't anymore)
>
> I'd really like to get this resolved. It seems the line length is also
> slightly differently defined for you:
>
> === modified file 'lib/lp/translations/interfaces/pofile.py'
> --- lib/lp/translations/interfaces/pofile.py 2010-05-19 17:04:11 +0000
> +++ lib/lp/translations/interfaces/pofile.py 2010-05-20 17:58:16 +0000
> @@ -127,9 +127,9 @@
> '''),
> required=False)
>
> - translation_messages = Attribute(_('''
> - All `ITranslationMessage` objects related to this translation file.
> - '''))
> + translation_messages = Attribute(_(
> + "All `ITranslationMessage` objects related to this "
> + "translation file."))
>
> plural_forms = Int(
> title=_('Number of plural forms for the language of this PO file.'),
>
>
> This is not needed for me, because just putting it as
>
> translation_messages = Attribute(_(
> 'All `ITranslationMessage` objects related to this translation file.'
> ))
>
> works fine. Not that there's anything wrong with your approach, I am just noting this.

This will break the styleguide as there is a space before the ')'

> > > > === modified file 'lib/lp/translations/interfaces/pofile.py'
> > > > --- lib/lp/translations/interfaces/pofile.py 2010-03-08 21:06:34 +0000
> > > > +++ lib/lp/translations/interfaces/pofile.py 2010-05-19 17:04:11 +0...

Hi Adi,

У чет, 20. 05 2010. у 19:06 +0000, Adi Roiban пише:
> > This is not needed for me, because just putting it as
> >
> > translation_messages = Attribute(_(
> > 'All `ITranslationMessage` objects related to this translation file.'
> > ))
> >
> > works fine. Not that there's anything wrong with your approach, I am just noting this.
>
> This will break the styleguide as there is a space before the ')'

Heh, this is not really a space: it's a multiline wrapping of braces,
and we do exactly the same thing with tuples (we'd do it with imports as
well if we didn't have too many of those), as mentioned on [1]:

  something = (
      FirstVeryLongName,
      SecondVeryLongName,
  )

[1] https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces

Anyway, as I said, your style is just as fine as well.

 review approve
 merge approve

I'll get this into ec2 land soon.

Cheers,
Danilo

Hi Adi,

Ok, I realized there are more things to fix first. We've discussed them
over on IRC, but I will repeat myself a bit for reference.

 review needs-fixing

Attribute names should include as little gettextisms as possible:
"all_pofiles" and "all_potemplates" should be "translation_files" and
"translation_templates" (this one is already almost alright in the
branch: I am not sure about the "all").

And further on that topic, we should only show most relevant translation
templates: these are usually only the current, non-obsolete ones.
Obsolete templates are useful only for management purposes, and if we
want, we can export them either separately, or export all of

  getCurrentTranslationTemplates
  getObsoleteTranslationTemplates
  getTranslationTemplates

There is no need to wrap either of the methods in a property in order to
export them. What's else, these methods expose some implementation
details (like "just_ids" parameter, needed for performance reasons
elsewhere) and we should not get them exported as such.

Since I want to see this branch move in soon, I don't want to dwell on
cleaning up these right now.

Now, performance reasons are very important to consider here. We may
have to investigate whether `doNotSnapshot` is needed, and how batching
works over Collection results ([1] suggests it splits results into
batches of 75, but we'd have to check that). If API framework splits
the results into batches and only serializes a single batch (vs
serializing a whole result set), it'd be ok even if we don't use
doNotSnapshot (unless it turns out to be too slow once it hits
edge/staging, when we'll need to fix it).

Also, at the moment, we should export only the most useful list of
templates (because most of this code needs slight refactoring, we should
not export all methods and then stop exporting them soon after). At the
moment, for the "reporting" use case, I believe only "current" templates
make sense to be exported. Now, exporting it with just_ids parameter is
fine as well, and we can fix it with the refactoring in the future.

Next, we did mention how it should already allow read-write access if
you've got sufficient privileges. In my tests that has turned out to be
false. This is definitely not a blocker (it's ok if we start with
read-only API), but we at least need to understand why this is so and
how is write access created (so we don't do it by accident).

Example script I tried out (after doing 'make run' in this branch, run
this in a python console and log in with sufficient privileges, eg.
<email address hidden>:test):

  cachedir = "/home/danilo/.launchpadlib/cache/"
  from launchpadlib.launchpad import Launchpad
  launchpad = Launchpad.login_with('dev testing', 'dev', cachedir)
  ubuntu = launchpad.distributions['ubuntu']
  hoary = ubuntu.series[2]
  pots = hoary.all_translation_templates
  pot = pots[0]
  pot.translation_domain = 'something_else'
  pot.lp_save()

So, suggested course of action to get this landed:

 0. fix "all_pofile" attribute name
 1. Check if we get batching of results on API method calls/attributes
for free (without needing to do anything else)
 2. If 1 is true, check if batching snapshots the en...

On Wed, 2010-05-26 at 16:12 +0000, Данило Шеган wrote:
> Review: Needs Fixing
> Hi Adi,
>
> Ok, I realized there are more things to fix first. We've discussed them
> over on IRC, but I will repeat myself a bit for reference.
>
> review needs-fixing
>
> Attribute names should include as little gettextisms as possible:
> "all_pofiles" and "all_potemplates" should be "translation_files" and
> "translation_templates" (this one is already almost alright in the
> branch: I am not sure about the "all").

renamed all_pofiles to translation_files
removed all_translation_templates since its content will be provided by
a method.

I have renamed the name used for exporting pofile and potemplate entries
and collections.

> And further on that topic, we should only show most relevant translation
> templates: these are usually only the current, non-obsolete ones.
> Obsolete templates are useful only for management purposes, and if we
> want, we can export them either separately, or export all of
>
> getCurrentTranslationTemplates
> getObsoleteTranslationTemplates
> getTranslationTemplates
>
> There is no need to wrap either of the methods in a property in order to
> export them. What's else, these methods expose some implementation
> details (like "just_ids" parameter, needed for performance reasons
> elsewhere) and we should not get them exported as such.
>
> Since I want to see this branch move in soon, I don't want to dwell on
> cleaning up these right now.

We can hide the "just_ids" in the exported API call.
We can export getCurrentTranslationTemplates() method, with just_ids
hidden and always set to False.

I still think that exporting getTranslationTemplates() will make the API
simpler (rather than exporting all 3 methods) and the client side check
for active/inactive templates is not that complicated.

> Now, performance reasons are very important to consider here. We may
> have to investigate whether `doNotSnapshot` is needed, and how batching
> works over Collection results ([1] suggests it splits results into
> batches of 75, but we'd have to check that). If API framework splits
> the results into batches and only serializes a single batch (vs
> serializing a whole result set), it'd be ok even if we don't use
> doNotSnapshot (unless it turns out to be too slow once it hits
> edge/staging, when we'll need to fix it).

I will raise this discussion over the lp-dev ML. I still have to read
the code and see exactly why, where and how those snosphots are
needed/used in order to ask a valid question.

I think that we should land this branch and the start of next cycle.
The first attempt should be without doNotSnapshot. If something goes
wrong we can reconsider our options.

> Also, at the moment, we should export only the most useful list of
> templates (because most of this code needs slight refactoring, we should
> not export all methods and then stop exporting them soon after). At the
> moment, for the "reporting" use case, I believe only "current" templates
> make sense to be exported. Now, exporting it with just_ids parameter is
> fine as well, and we can fix it with the refactoring in the future.

See above comments.

> Next, we...

We've been discussing this in IRC. It turns out that IDistroSeries.getTranslationTemplates wasn't batching properly because it used shortlist() to limit its results. As a side effect, that listifies the query result—causing all its results to be fetched and marshaled. The batching logic then slices the resulting list instead of the query result, which is somewhat pointless.

The docstring deliberately makes no promise that getTranslationTemplates returns a list, so it's perfectly valid to return the query result directly instead of a shortlisted version of it. Unfortunately the distroseries translations copying script does assert that len(child.getTranslationTemplates()) == 0. This assumes a list-like return value; with a query it should assert that child.getTranslationTemplates.is_empty().

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

For the record I should add that we first discussed the bigger issues (performance, security, various batching hazards) as per Danilo's proposed course of action above. All those points have been satisfied, so the nitpicking about small bits of code means that the branch is in good shape. There is no problematic snapshotting, performance is good, and with the removal of that shortlist we no longer fetch all templates in a distroseries at once.

Hi,

Here is the diff with the changes discussed in the latest comment.

If everything is ok, can you please sent this branch to ec2-test?

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2010-05-19 17:24:21 +0000
+++ lib/lp/registry/model/distroseries.py 2010-06-03 15:06:35 +0000
@@ -1886,7 +1886,7 @@
         """See `IHasTranslationTemplates`."""
         result = POTemplate.selectBy(distroseries=self,
                                      orderBy=['-priority', 'name'])
- return shortlist(result, 2000)
+ return result

     def getCurrentTranslationTemplates(self, just_ids=False):
         """See `IHasTranslationTemplates`."""

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py 2010-05-17 09:35:18 +0000
+++ lib/lp/registry/model/productseries.py 2010-06-03 15:06:14 +0000
@@ -427,7 +427,7 @@
         """See `IHasTranslationTemplates`."""
         result = POTemplate.selectBy(
             productseries=self, orderBy=['-priority', 'name'])
- return shortlist(result, 300)
+ return result

     def getCurrentTranslationTemplates(self, just_ids=False):
         """See `IHasTranslationTemplates`."""

=== modified file 'lib/lp/registry/model/sourcepackage.py'
--- lib/lp/registry/model/sourcepackage.py 2010-05-17 09:35:18 +0000
+++ lib/lp/registry/model/sourcepackage.py 2010-06-03 15:19:06 +0000
@@ -602,7 +602,7 @@
         result = POTemplate.selectBy(
             distroseries=self.distroseries,
             sourcepackagename=self.sourcepackagename)
- return shortlist(result.orderBy(['-priority', 'name']), 300)
+ return result.orderBy(['-priority', 'name'])

     def getCurrentTranslationTemplates(self, just_ids=False):
         """See `IHasTranslationTemplates`."""

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2010-05-27 14:38:56 +0000
+++ lib/lp/translations/browser/potemplate.py 2010-06-03 16:02:24 +0000
@@ -774,18 +774,17 @@

         # Get whether the target for the requested template is officially
         # using Launchpad Translations.
- if potemplate.distribution is not None:
- official_rosetta = potemplate.distribution.official_rosetta
- elif potemplate.product is not None:
- official_rosetta = potemplate.product.official_rosetta
+ if potemplate.distribution is None:
+ product_or_distro = potemplate.productseries.product
         else:
- raise AssertionError('Unknown context for %s' % potemplate.title)
+ product_or_distro = potemplate.distroseries.distribution
+ official_rosetta = product_or_distro.official_rosetta

- if ((official_rosetta and potemplate.iscurrent) or
- check_permission('launchpad.Edit', potemplate)):
- # The target is using officially Launchpad Translations and the
- # template is available to be translated, or the user is a is a
- # Launchpad administrator in which case we show everything.
+ if official_rosetta and potemplate.iscurrent:
+ # This template is av...