Merge lp:~adiroiban/launchpad/bug-509252-take-2 into lp:launchpad

= Bug 509252 =
After landing the fix for bug 340662 (https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662-take-2/+merge/17598) the POTemplateSubsetNavigation will render the AdminPOTemplateSubset useless.

We should clean the security.py.

AdminPOTemplateSubset was added to fix bug 497438

== Proposed fix ==

Remove unused classed from security.py and refactor

== Pre-implementation notes ==

Talking with Henning we decide not to include security checking code in the model. This is why the logic of showing translations of a series was spitted between the model and the view and the security checks were done in multiple places.

This restriction lead to a strange implementation where the check was done both in the view and the model and it was a source of confusion.

I merged this logic in a browser helper function and now the security check is only done once.

== Implementation details ==

These changes renders the AdminDistroSeriesLanguage class from security.py useless, so I have also removed it.

== Tests ==
lp-test -t distroseries-translations -t distroseries-views

== Demo and Q/A ==
Make sure you are a member of Ubuntu Translation Coordinators team (ubuntu-l10n-coordinator).
https://launchpad.dev/~ubuntu-l10n-coordinator

Go to the translation page for a distribution and hide translations for this series
ie: https://translations.launchpad.dev/ubuntu/hoary/+admin

Next go to the distribution series +template page
https://translations.launchpad.dev/ubuntu/hoary/+templates

You should see the Administer page for Evolution templates, including disabled-template .

As a member of Ubuntu Translation Coordinators team you should be able to administer them, just like for a normal series.

Login as a normal user you should see a page informing that the translations are currently closed

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/registry/browser/distroseries.py
  lib/lp/registry/browser/sourcepackage.py
  lib/lp/registry/interfaces/distroseries.py
  lib/lp/registry/model/distroseries.py
  lib/lp/translations/browser/browser_helpers.py
  lib/lp/translations/browser/distroseries.py
  lib/lp/translations/stories/distroseries/xx-distroseries-translations.txt

== Pylint notices ==

lib/lp/registry/browser/sourcepackage.py
    29: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)

lib/lp/registry/interfaces/distroseries.py
    23: [F0401] Unable to import 'lazr.enum' (No module named enum)
    50: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    51: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    117: [E1002, DistroSeriesVersionField._validate] Use super on an old style class
    447: [C0322, IDistroSeriesPublic.getPackageUploads] Operator not preceded by a space
    description=_("Return items that are more recent than this "
    ^
    "timestamp."),
    required=False),
    status=Choice(

    vocabulary=DBEnumeratedType,
    title=_("Package Upload Status"),
    des...

Hi Adi,

Thanks for all the cleanups that you've done in this branch.

After reading the recent email discussion at:

https://lists.launchpad.net/launchpad-dev/msg02442.html

and particularly BjornT's message:

https://lists.launchpad.net/launchpad-dev/msg02504.html

I don't think putting the security check into a browser helper is the way to go.

As outlined in the above email:
"The current solution is to have this code in model code, and have the
security adapter ask the model. No code duplication really."

this ensures that any security can be applied not only in the view, but also via other modes of access (API), even if they are not yet enabled.

As it turns out in your code, it seems most of IDistroSeries.checkTranslationsViewable() is not actually security code, but simply appropriate error generation. The first few lines however are security related.

So I wonder if we can do something like this:

http://pastebin.ubuntu.com/379120/

That is, simply add a launchpad.View check for IDistroSeriesLanguage, which itself ensures that launchpad.View is granted if the user is an admin. That way, as in the example in the diff, the current double security checks:

         if not check_permission(
            'launchpad.TranslationsAdmin', distroserieslang):
            self.context.checkTranslationsViewable()

could be replaced simply by:

         if not check_permission(
            'launchpad.View', distroserieslang):
            translation_unavailable_error()

that way you'd have all the security in the right place, separate from the error generation (ie. translation_unavailable_error() would be your proposed view helper, check_distroseries_translations_viewable, just without the security checks?

I'm not 100%, but as far as I can see, the above should do what you're after while at the same time keeping all the security code in the one place. But I'll check with Henninge to be sure (hence marking this as needs info).

IRC log of conversation starts at:

http://irclogs.ubuntu.com/2010/02/18/%23launchpad-reviews.html#t11:18

OK, I spoke to Adi, and he said that the security for IDistroSeriesLanguage needs to go anyway, which means that the above paste is useless, as we can't update the permission checks for DistroSeries or SourcePackage (or is there a way to have checks for these objects only when they are traversed in the translations context? - an extra 'facet' option to check_permission?)

Without this, I can't see a way to keep these security checks encapsulated in security.py or on the model. I'm going to have to defer to Danilo or Henninge.

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. ;-)

DistroSeriesView.checkTranslationsViewable() is used in a „metal” tag... so it really doesn't matter its return value as its sole purpose is to raise those exceptions.

<metal:check-available tal:omit-tag="view/checkTranslationsViewable" />

If we want to remove this behavior, we would need to raise those exception in Distribution URL traversal for series... and then things will get a bit complicated as those exception should be raised only for „translations” facet.

-------

There are a couple of view tests in translations/browser/tests/distroseries-views.txt and registry/doc/distroseries.txt using these exceptions. I guess we need to put them in a single file (ie: translations/browser/tests/distroseries-views.txt)

-----

Thanks for the getDistroSeriesLanguageOrDummy tip :)

Hi Adi,
sorry for not replying but I had expected that you push an updated branch with whatever changes you see possible.

I did not know that metal tags are being used to raise exceptions and I am pretty sure that should not be that way. If you think it's too much work to improve that in this branch, I can understand that. Please file a bug so that it gets done at some later time.

So, a new push to the branch would be the next thing to get the review forward.

Cheers,
Henning

Hi,
Don't worry about the communication problem ... be happy :)

I have opened a new bug for TAL expression used for raising an exception.
I have merged the tests for translation permissions.

Here is the diff. Thanks!
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2010-02-17 13:06:35 +0000
+++ lib/lp/registry/browser/distroseries.py 2010-02-24 14:57:59 +0000
@@ -37,10 +37,8 @@
 from lp.services.worlddata.interfaces.country import ICountry
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.distroseries import IDistroSeries
-from lp.translations.browser.browser_helpers import (
+from lp.translations.browser.distroseries import (
     check_distroseries_translations_viewable)
-from lp.translations.interfaces.distroserieslanguage import (
- IDistroSeriesLanguageSet)
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.registry.browser.structuralsubscription import (
     StructuralSubscriptionMenuMixin,
@@ -81,19 +79,12 @@
         except IndexError:
             # Unknown language code.
             raise NotFoundError
- distroserieslang = self.context.getDistroSeriesLanguage(lang)

- if distroserieslang is None:
- # There is no IDistroSeriesLanguage yet for this IDistroSeries,
- # but we still need to list it as an available language, so we
- # generate a dummy one so users have a chance to get to it in the
- # navigation and start adding translations for it.
- distroserieslangset = getUtility(IDistroSeriesLanguageSet)
- distroserieslang = distroserieslangset.getDummy(
- self.context, lang)
+ distroserieslang = self.context.getDistroSeriesLanguageOrDummy(lang)

         # Check if user is able to view the translations for
- # this distribution series
+ # this distribution series language.
+ # If not, raise TranslationUnavailable.
         check_distroseries_translations_viewable(self.context)

         return distroserieslang

=== modified file 'lib/lp/registry/browser/sourcepackage.py'
--- lib/lp/registry/browser/sourcepackage.py 2010-02-17 13:06:35 +0000
+++ lib/lp/registry/browser/sourcepackage.py 2010-02-24 14:55:26 +0000
@@ -40,7 +40,7 @@
 from lp.registry.interfaces.packaging import IPackaging
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.sourcepackage import ISourcePackage
-from lp.translations.browser.browser_helpers import (
+from lp.translations.browser.distroseries import (
     check_distroseries_translations_viewable)
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from canonical.launchpad import _
@@ -68,7 +68,8 @@

         # If we are able to view the translations for distribution series
         # we should also be allowed to see them for a distribution
- # source package
+ # source package.
+ # If not, raise TranslationUnavailable.
         check_distroseries_translations_viewable(self.context.distroseries)

         return sourcepackage_pots

=== modified file 'lib/lp/registry/doc/distroseries.t...

Thanks, this looks much better. Unfortunately the diff shows conflicts, so I guess you need to merge devel and push again for the diff to be correct.

Also, is there no unit test that could go through all the different SeriesStatus values? Doing this in a doc test looks like it's in the wrong place. But please only do this if it is fairly easy. I think this branch is already old enough ... ;-)

În data de Mi, 24-02-2010 la 17:42 +0000, Henning Eggers a scris:
> Thanks, this looks much better. Unfortunately the diff shows
> conflicts, so I guess you need to merge devel and push again for the
> diff to be correct.
Conflict solved.

> Also, is there no unit test that could go through all the different
> SeriesStatus values? Doing this in a doc test looks like it's in the
> wrong place. But please only do this if it is fairly easy. I think
> this branch is already old enough ... ;-)
I was thinking that view unit tests should be put in:
lib/lp/<application>/browser/tests

translations/browser/tests/distroseries-views.txt was already there and
I have just moved those tests from registry/doc/distroseries.txt to this
file.

Are you saying that we should not write unit tests using doctest format?

Cheers

--
Adi Roiban

> Conflict solved.

Cool, looks good. ;)

> Are you saying that we should not write unit tests using doctest format?

Yes, at least that's what I do. They execute slower, don't they? But as you pointed out, the test was already there, so you just extended it. That's what I do, too ... ;) So you can keep it as it is if you want to.

Thanks for the extra comments in the code, btw. And all the clean-up work!
You have a double "allowed allowed" in there somewhere, btw.

Cheers,
Henning

În data de Mi, 24-02-2010 la 18:27 +0000, Henning Eggers a scris:
> Review: Approve code
> > Conflict solved.
>
> Cool, looks good. ;)
>
>
> > Are you saying that we should not write unit tests using doctest format?
>
> Yes, at least that's what I do. They execute slower, don't they? But
> as you pointed out, the test was already there, so you just extended
> it. That's what I do, too ... ;) So you can keep it as it is if you
> want to.
OK. This is not the only unit test written in doctest format.

$ ls lib/lp/translations/browser/tests/*.txt
lib/lp/translations/browser/tests/distroseries-views.txt
lib/lp/translations/browser/tests/language-views.txt
lib/lp/translations/browser/tests/poexport-request-views.txt
lib/lp/translations/browser/tests/pofile-base-views.txt
lib/lp/translations/browser/tests/pofile-views.txt
lib/lp/translations/browser/tests/potemplate-views.txt
lib/lp/translations/browser/tests/productseries-views.txt
lib/lp/translations/browser/tests/translationimportqueue-views.txt
lib/lp/translations/browser/tests/translationmessage-views.txt
lib/lp/translations/browser/tests/translator-views.txt

Maybe we should open a bug to convert all those test from doctest to
"pure" python code.

> Thanks for the extra comments in the code, btw. And all the clean-up
> work!
> You have a double "allowed allowed" in there somewhere, btw.

Typo fixed and pushed.

When you have time, can you please send this branch to ec2 test?

Kindest regards,

--
Adi Roiban

I am having trouble landing this. First, there is the current problem with ec2 test not sending emails, so the failure was kind of obscure. Secondly, after merging the current devel, it failed because "python-zope.interface" needed to be installed. You will have to do that, too!

Now, this error remains when running "make build":

    ZopeXMLConfigurationError: File "/home/henning/canonical/lp-branches/adiroiban-bug-509252-take-2/lib/lp/registry/configure.zcml", line 172.4-176.34
    ImportError: cannot import name check_distroseries_translations_viewable

Hm...

I did not get the „python-zope.interface” error but got the circular import error.

I have fixed the problem and pushed the branch.