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

[Adi Roiban]

Refactor checkTranslationsViewable(). Merge the model and view logic and move it to a view for distroseries. Landed by henninge.

[Adi Roiban (adiroiban)]

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

[Michael Nelson (michael.nelson)]

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

review: Needs Information (code)

[Michael Nelson (michael.nelson)]

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.

[Henning Eggers (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. ;-)

review: Needs Fixing (code)

[Adi Roiban (adiroiban)]

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 :)

[Henning Eggers (henninge)]

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

[Adi Roiban (adiroiban)]

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

[Henning Eggers (henninge)]

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

[Adi Roiban (adiroiban)]

Î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

[Henning Eggers (henninge)]

> 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

review: Approve (code)

[Adi Roiban (adiroiban)]

Î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

[Henning Eggers (henninge)]

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

[Adi Roiban (adiroiban)]

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.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-02-24 23:02:56 +0000
+++ lib/canonical/launchpad/security.py	2010-03-01 23:03:20 +0000
@@ -49,8 +49,6 @@
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage)
 from lp.registry.interfaces.distroseries import IDistroSeries
-from lp.translations.interfaces.distroserieslanguage import (
-    IDistroSeriesLanguage)
 from lp.registry.interfaces.entitlement import IEntitlement
 from lp.hardwaredb.interfaces.hwdb import (
     IHWDBApplication, IHWDevice, IHWDeviceClass, IHWDriver, IHWDriverName,
@@ -1664,19 +1662,6 @@
             self.obj.distribution).checkAuthenticated(user))
 
 
-class AdminDistroSeriesLanguage(AuthorizationBase):
-    permission = 'launchpad.TranslationsAdmin'
-    usedfor = IDistroSeriesLanguage
-
-    def checkAuthenticated(self, user):
-        """Is the user able to manage `IDistroSeriesLanguage` translations.
-
-        Distribution managers can also manage IDistroSeriesLanguage
-        """
-        return (AdminDistroSeriesTranslations(
-            self.obj.distroseries).checkAuthenticated(user))
-
-
 class BranchSubscriptionEdit(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IBranchSubscription

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2010-02-26 19:15:17 +0000
+++ lib/lp/registry/browser/distroseries.py	2010-03-01 23:03:20 +0000
@@ -37,8 +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.interfaces.distroserieslanguage import (
-    IDistroSeriesLanguageSet)
+from lp.translations.browser.distroseries import (
+    check_distroseries_translations_viewable)
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.registry.browser.structuralsubscription import (
     StructuralSubscriptionMenuMixin,
@@ -48,7 +48,6 @@
 from canonical.launchpad.webapp import (
     StandardLaunchpadFacets, GetitemNavigation, action, custom_widget)
 from canonical.launchpad.webapp.batching import BatchNavigator
-from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.launchpadform import (
     LaunchpadEditFormView, LaunchpadFormView)
@@ -80,20 +79,13 @@
         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)
-
-        if not check_permission(
-            'launchpad.TranslationsAdmin', distroserieslang):
-            self.context.checkTranslationsViewable()
+
+        distroserieslang = self.context.getDistroSeriesLanguageOrDummy(lang)
+
+        # Check if user is able to view the translations for
+        # 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-23 19:43:58 +0000
+++ lib/lp/registry/browser/sourcepackage.py	2010-03-01 23:03:20 +0000
@@ -52,7 +52,6 @@
     action, ApplicationMenu, custom_widget, GetitemNavigation,
     LaunchpadFormView, Link, redirection, StandardLaunchpadFacets, stepto)
 from canonical.launchpad.webapp import canonical_url
-from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp.menu import structured
 
@@ -70,9 +69,13 @@
             distroseries=self.context.distroseries,
             sourcepackagename=self.context.sourcepackagename)
 
-        if not check_permission(
-            'launchpad.TranslationsAdmin', sourcepackage_pots):
-            self.context.distroseries.checkTranslationsViewable()
+        # If we are able to view the translations for distribution series
+        # we should also be allowed to see them for a distribution
+        # source package.
+        # If not, raise TranslationUnavailable.
+        from lp.translations.browser.distroseries import (
+            check_distroseries_translations_viewable)
+        check_distroseries_translations_viewable(self.context.distroseries)
 
         return sourcepackage_pots
 
@@ -92,6 +95,7 @@
 
 class SourcePackageBreadcrumb(Breadcrumb):
     """Builds a breadcrumb for an `ISourcePackage`."""
+
     @property
     def text(self):
         return smartquote('"%s" source package') % (self.context.name)

=== modified file 'lib/lp/registry/doc/distroseries.txt'
--- lib/lp/registry/doc/distroseries.txt	2010-02-25 16:27:34 +0000
+++ lib/lp/registry/doc/distroseries.txt	2010-03-01 23:03:20 +0000
@@ -1,4 +1,5 @@
-= DistroSeries =
+DistroSeries
+============
 
 From the DerivationOverview spec
 <https://launchpad.canonical.com/DerivationOverview>:
@@ -200,7 +201,8 @@
    True
 
 
-== Package searching ==
+Package searching
+-----------------
 
 You can search through binary packages publishing in a distribution
 release by using the searchPackages method, which uses magical fti:
@@ -223,7 +225,8 @@
     DistroSeriesBinaryPackage: at
 
 
-== DistroSeriess have components and sections ==
+DistroSeriess have components and sections
+------------------------------------------
 
 A distroseries has some number of components and/or sections which
 are valid for that distroseries. These selections are used by (among
@@ -286,7 +289,8 @@
     partner
 
 
-== DistroSeries can be initialised from their parents ==
+DistroSeries can be initialised from their parents
+--------------------------------------------------
 
 When a distroseries is derived from another distroseries (be it a
 derivative distribution, or simply the next release in a sequence from
@@ -396,74 +400,8 @@
     netapplet
 
 
-Hiding translations
--------------------
-
-The hide_all_translations flag, if set, hides a distroseries'
-translations in the user interface.  The check for visibility happens in
-checkTranslationsViewable.
-
-    >>> untranslatable_series = factory.makeDistroRelease()
-    >>> untranslatable_series.hide_all_translations = False
-
-When translations are visible, checkTranslationsViewable() completes
-normally.
-
-    >>> from canonical.launchpad.webapp.interfaces import (
-    ...     TranslationUnavailable)
-    >>> def get_visibility_notice(series):
-    ...     """Print the notice about series' translations being hidden."""
-    ...     try:
-    ...         series.checkTranslationsViewable()
-    ...     except TranslationUnavailable, e:
-    ...         return str(e)
-    ...     return None
-
-    >>> print get_visibility_notice(untranslatable_series)
-    None
-
-But when translations are hidden, it raises TranslationUnavailable.
-
-    >>> untranslatable_series.hide_all_translations = True
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series...
-
-Exactly what message is displayed depends on the series' status.
-
-    >>> untranslatable_series.status = SeriesStatus.EXPERIMENTAL
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not available yet.
-
-    >>> untranslatable_series.status = SeriesStatus.DEVELOPMENT
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not available yet.
-
-    >>> untranslatable_series.status = SeriesStatus.FROZEN
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not currently available.
-    Please come back soon.
-
-    >>> untranslatable_series.status = SeriesStatus.CURRENT
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not currently available.
-    Please come back soon.
-
-    >>> untranslatable_series.status = SeriesStatus.SUPPORTED
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not currently available.
-    Please come back soon.
-
-    >>> untranslatable_series.status = SeriesStatus.OBSOLETE
-    >>> print get_visibility_notice(untranslatable_series)
-    This release series is obsolete.  Its translations are no longer
-    available.
-
-    >>> untranslatable_series.status = SeriesStatus.FUTURE
-    >>> print get_visibility_notice(untranslatable_series)
-    Translations for this release series are not available yet.
-
-
-== Translatable Packages and Packaging ==
+Translatable Packages and Packaging
+-----------------------------------
 
 You can easily find out what packages are translatable in a
 distribution release:
@@ -668,7 +606,8 @@
    True
 
 
-== SourcePackagePublishingHistory ==
+SourcePackagePublishingHistory
+------------------------------
 
 IDistroSeries.getSourcePackagePublishing returns all the ISPPH
 records for a given status in a given pocket. It makes easy to
@@ -1042,7 +981,8 @@
     thinclient-local-devices
 
 
-= Drivers =
+Drivers
+=======
 
 Distributions have drivers, who are people that have permission to approve
 bugs and features for specific releases. The rules are that:
@@ -1124,7 +1064,8 @@
     mark
 
 
-== Lastest Uploads ==
+Lastest Uploads
+---------------
 
 IDistroSeries provides the 'getLatestUpload' method which returns a
 list of the last 5 (five) IDistroSeriesSourcePackageRelease (IDRSPR)
@@ -1150,7 +1091,8 @@
     0
 
 
-== Getting build records for a distro series ==
+Getting build records for a distro series
+-----------------------------------------
 
 IDistroSeries inherits the IHasBuildRecords interfaces and therefore provides
 a getBuildRecords() method.

=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2010-02-27 10:19:18 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2010-03-01 23:03:20 +0000
@@ -54,6 +54,7 @@
 from lp.translations.interfaces.languagepack import ILanguagePack
 
 
+
 class DistroSeriesNameField(ContentNameField):
     """A class to ensure `IDistroSeries` has unique names."""
     errormessage = _("%s is already in use by another series.")
@@ -132,6 +133,7 @@
 
 class IDistroSeriesEditRestricted(Interface):
     """IDistroSeries properties which require launchpad.Edit."""
+
     @rename_parameters_as(dateexpected='date_targeted')
     @export_factory_operation(
         IMilestone, ['name', 'dateexpected', 'summary', 'code_name'])
@@ -198,7 +200,7 @@
             Interface, # Really IDistribution, see circular import fix below.
             title=_("Distribution"), required=True,
             description=_("The distribution for which this is a series.")))
-    named_version =  Attribute('The combined display name and version.')
+    named_version = Attribute('The combined display name and version.')
     parent = Attribute('The structural parent of this series - the distro')
     components = Attribute("The series components.")
     upload_components = Attribute("The series components that can be "
@@ -240,7 +242,7 @@
         title=_("Defer translation imports"),
         description=_("Suspends any translation imports for this series"),
         default=True,
-        required=True
+        required=True,
         )
     binarycount = Attribute("Binary Packages Counter")
 
@@ -497,17 +499,6 @@
         :return: A result set containing `IPackageUpload`
         """
 
-    def checkTranslationsViewable():
-        """Raise `TranslationUnavailable` if translations are hidden.
-
-        Checks the `hide_all_translations` flag.  If it is set, these
-        translations are not to be shown to the public.  In that case an
-        appropriate message is composed based on the series' `status`,
-        and a `TranslationUnavailable` exception is raised.
-
-        Simply returns if translations are not hidden.
-        """
-
     def getUnlinkedTranslatableSourcePackages():
         """Return a list of source packages that can be translated in
         this distribution series but which lack Packaging links.

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-02-25 16:27:34 +0000
+++ lib/lp/registry/model/distroseries.py	2010-03-01 23:03:20 +0000
@@ -118,8 +118,7 @@
 from canonical.launchpad.mail import signed_message_from_string
 from lp.registry.interfaces.person import validate_public_person
 from canonical.launchpad.webapp.interfaces import (
-    IStoreSelector, MAIN_STORE, NotFoundError, SLAVE_FLAVOR,
-    TranslationUnavailable)
+    IStoreSelector, MAIN_STORE, NotFoundError, SLAVE_FLAVOR)
 from lp.soyuz.interfaces.sourcepackageformat import (
     ISourcePackageFormatSelectionSet)
 
@@ -134,7 +133,7 @@
             SeriesStatus.DEVELOPMENT,
             SeriesStatus.FROZEN,
             SeriesStatus.CURRENT,
-            SeriesStatus.SUPPORTED
+            SeriesStatus.SUPPORTED,
             ]
 
 
@@ -162,7 +161,7 @@
         dbName='releasestatus', notNull=True, schema=SeriesStatus)
     date_created = UtcDateTimeCol(notNull=False, default=UTC_NOW)
     datereleased = UtcDateTimeCol(notNull=False, default=None)
-    parent_series =  ForeignKey(
+    parent_series = ForeignKey(
         dbName='parent_series', foreignKey='DistroSeries', notNull=False)
     owner = ForeignKey(
         dbName='owner', foreignKey='Person',
@@ -173,7 +172,7 @@
     lucilleconfig = StringCol(notNull=False, default=None)
     changeslist = StringCol(notNull=False, default=None)
     nominatedarchindep = ForeignKey(
-        dbName='nominatedarchindep',foreignKey='DistroArchSeries',
+        dbName='nominatedarchindep', foreignKey='DistroArchSeries',
         notNull=False, default=None)
     messagecount = IntCol(notNull=True, default=0)
     binarycount = IntCol(notNull=True, default=DEFAULT)
@@ -751,7 +750,7 @@
 
         # filter based on completion. see the implementation of
         # Specification.is_complete() for more details
-        completeness =  Specification.completeness_clause
+        completeness = Specification.completeness_clause
 
         if SpecificationFilter.COMPLETE in filter:
             query += ' AND ( %s ) ' % completeness
@@ -886,29 +885,6 @@
              DistroSeriesSourcePackageRelease(self, release))
             for release in releases)
 
-    def checkTranslationsViewable(self):
-        """See `IDistroSeries`."""
-        if not self.hide_all_translations:
-            # Yup, viewable.
-            return
-
-        future = [
-            SeriesStatus.EXPERIMENTAL,
-            SeriesStatus.DEVELOPMENT,
-            SeriesStatus.FUTURE,
-            ]
-        if self.status in future:
-            raise TranslationUnavailable(
-                "Translations for this release series are not available yet.")
-        elif self.status == SeriesStatus.OBSOLETE:
-            raise TranslationUnavailable(
-                "This release series is obsolete.  Its translations are no "
-                "longer available.")
-        else:
-            raise TranslationUnavailable(
-                "Translations for this release series are not currently "
-                "available.  Please come back soon.")
-
     def getTranslatableSourcePackages(self):
         """See `IDistroSeries`."""
         query = """
@@ -1069,13 +1045,12 @@
             SourcePackagePublishingHistory.archive IN %s AND
             SourcePackagePublishingHistory.status=%s AND
             SourcePackagePublishingHistory.pocket=%s
-            """ %  sqlvalues(self, archives, status, pocket)
+            """ % sqlvalues(self, archives, status, pocket)
 
         if component:
             clause += (
                 " AND SourcePackagePublishingHistory.component=%s"
-                % sqlvalues(component)
-                )
+                % sqlvalues(component))
 
         orderBy = ['SourcePackageName.name']
         clauseTables = ['SourcePackageRelease', 'SourcePackageName']
@@ -1136,7 +1111,7 @@
 
         clauseTables = ['BinaryPackagePublishingHistory', 'DistroArchSeries',
                         'BinaryPackageRelease', 'BinaryPackageName', 'Build',
-                        'SourcePackageRelease', 'SourcePackageName' ]
+                        'SourcePackageRelease', 'SourcePackageName']
 
         result = BinaryPackagePublishingHistory.select(
             query, distinct=False, clauseTables=clauseTables, orderBy=orderBy)
@@ -1971,8 +1946,9 @@
             return '%s%s' % (self.name, pocketsuffix[pocket])
 
     def isSourcePackageFormatPermitted(self, format):
-        return getUtility(ISourcePackageFormatSelectionSet
-            ).getBySeriesAndFormat(self, format) is not None
+        return getUtility(
+            ISourcePackageFormatSelectionSet).getBySeriesAndFormat(
+                self, format) is not None
 
 
 class DistroSeriesSet:
@@ -1990,8 +1966,7 @@
         result_set = store.using((DistroSeries, POTemplate)).find(
             DistroSeries,
             DistroSeries.hide_all_translations == False,
-            DistroSeries.id == POTemplate.distroseriesID
-            ).config(distinct=True)
+            DistroSeries.id == POTemplate.distroseriesID).config(distinct=True)
         # XXX: henninge 2009-02-11 bug=217644: Convert to sequence right here
         # because ResultSet reports a wrong count() when using DISTINCT. Also
         # ResultSet does not implement __len__(), which would make it more

=== modified file 'lib/lp/translations/browser/browser_helpers.py'
--- lib/lp/translations/browser/browser_helpers.py	2009-09-04 11:54:50 +0000
+++ lib/lp/translations/browser/browser_helpers.py	2010-03-01 23:03:20 +0000
@@ -30,16 +30,16 @@
     """Replace Rosetta escape sequences with the real characters."""
     return helpers.text_replaced(text, {'[tab]': '\t',
                                         r'\[tab]': '[tab]',
-                                        '[nbsp]' : u'\u00a0',
-                                        r'\[nbsp]' : '[nbsp]' })
+                                        '[nbsp]': u'\u00a0',
+                                        r'\[nbsp]': '[nbsp]'})
 
 
 def expand_rosetta_escapes(unicode_text):
     """Replace characters needing a Rosetta escape sequences."""
     escapes = {u'\t': TranslationConstants.TAB_CHAR,
                u'[tab]': TranslationConstants.TAB_CHAR_ESCAPED,
-               u'\u00a0' : TranslationConstants.NO_BREAK_SPACE_CHAR,
-               u'[nbsp]' : TranslationConstants.NO_BREAK_SPACE_CHAR_ESCAPED }
+               u'\u00a0': TranslationConstants.NO_BREAK_SPACE_CHAR,
+               u'[nbsp]': TranslationConstants.NO_BREAK_SPACE_CHAR_ESCAPED}
     return helpers.text_replaced(unicode_text, escapes)
 
 
@@ -187,5 +187,3 @@
         raise UnrecognisedCFormatString(string)
 
     return segments
-
-

=== modified file 'lib/lp/translations/browser/distroseries.py'
--- lib/lp/translations/browser/distroseries.py	2010-02-02 15:32:00 +0000
+++ lib/lp/translations/browser/distroseries.py	2010-03-01 23:03:20 +0000
@@ -11,15 +11,16 @@
     'DistroSeriesTranslationsAdminView',
     'DistroSeriesTranslationsMenu',
     'DistroSeriesView',
+    'check_distroseries_translations_viewable',
     ]
 
 from zope.component import getUtility
 
 from canonical.cachedproperty import cachedproperty
 from canonical.launchpad import helpers
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp import action
 from canonical.launchpad.webapp.authorization import check_permission
+from canonical.launchpad.webapp.interfaces import TranslationUnavailable
 from canonical.launchpad.webapp.launchpadform import LaunchpadEditFormView
 from canonical.launchpad.webapp.menu import (
     Link, NavigationMenu, enabled_with_permission)
@@ -27,6 +28,7 @@
     canonical_url, LaunchpadView)
 
 from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.series import SeriesStatus
 from lp.translations.browser.translations import TranslationsMixin
 from lp.translations.browser.potemplate import BaseSeriesTemplatesView
 from lp.translations.interfaces.distroserieslanguage import (
@@ -175,34 +177,11 @@
             self.context.version)
 
     def checkTranslationsViewable(self):
-        """Check that these translations are visible to the current user.
-
-        Launchpad admins, Translations admins, and users with admin
-        rights on the `DistroSeries` are always allowed.  For others
-        this delegates to `IDistroSeries.checkTranslationsViewable`,
-        which raises `TranslationUnavailable` if the translations are
-        set to be hidden.
-
-        :return: Returns normally if this series' translations are
-            viewable to the current user.
-        :raise TranslationUnavailable: if this series' translations are
-            hidden and the user is not one of the limited caste that is
-            allowed to access them.
-        """
-        if check_permission('launchpad.TranslationsAdmin', self.context):
-            # Anyone with admin rights on this series passes.  This
-            # includes Launchpad admins.
-            return
-
-        user = self.user
-        experts = getUtility(ILaunchpadCelebrities).rosetta_experts
-        if user is not None and user.inTeam(experts):
-            # Translations admins also pass.
-            return
-
-        # Everyone else passes only if translations are viewable to the
-        # public.
-        self.context.checkTranslationsViewable()
+        """ Check if user can view translations for this `IDistroSeries`"""
+
+        # Is user allowed to see translations for this distroseries?
+        # If not, raise TranslationUnavailable.
+        check_distroseries_translations_viewable(self.context)
 
     def distroserieslanguages(self):
         """Produces a list containing a DistroSeriesLanguage object for
@@ -271,3 +250,47 @@
 
     def language_packs(self):
         return Link('+language-packs', 'Language packs')
+
+
+def check_distroseries_translations_viewable(distroseries):
+    """Check that these distribution series translations are visible.
+
+    Launchpad admins, Translations admins, and users with admin
+    rights on the `IDistroSeries` are always allowed.
+
+    Checks the `hide_all_translations` flag.  If it is set, these
+    translations are not to be shown to the public. In that case an
+    appropriate message is composed based on the series' `status`,
+    and a `TranslationUnavailable` exception is raised.
+
+    :return: Returns normally if this series' translations are
+        viewable to the current user.
+    :raise TranslationUnavailable: if this series' translations are
+        hidden and the user is not one of the limited caste that is
+        allowed to access them.
+    """
+
+    if not distroseries.hide_all_translations:
+        # Yup, viewable.
+        return
+
+    if check_permission(
+        'launchpad.TranslationsAdmin', distroseries):
+        return
+
+    future = [
+        SeriesStatus.EXPERIMENTAL,
+        SeriesStatus.DEVELOPMENT,
+        SeriesStatus.FUTURE,
+        ]
+    if distroseries.status in future:
+        raise TranslationUnavailable(
+            "Translations for this release series are not available yet.")
+    elif distroseries.status == SeriesStatus.OBSOLETE:
+        raise TranslationUnavailable(
+            "This release series is obsolete.  Its translations are no "
+            "longer available.")
+    else:
+        raise TranslationUnavailable(
+            "Translations for this release series are not currently "
+            "available.  Please come back soon.")

=== modified file 'lib/lp/translations/browser/tests/distroseries-views.txt'
--- lib/lp/translations/browser/tests/distroseries-views.txt	2009-12-13 11:55:40 +0000
+++ lib/lp/translations/browser/tests/distroseries-views.txt	2010-03-01 23:03:20 +0000
@@ -1,9 +1,9 @@
-= DistroSeries translations view classes =
+DistroSeries translations view classes
+======================================
+
+Let's use ubuntu/hoary for these tests.
 
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-
-Let's use ubuntu/hoary for these tests.
-
     >>> from canonical.launchpad.interfaces import IDistributionSet
     >>> from lp.registry.interfaces.series import SeriesStatus
     >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
@@ -15,7 +15,9 @@
     >>> login('[e-mail]')
     >>> hoary.status = SeriesStatus.CURRENT
 
-== Hiding translations ==
+
+Hiding translations
+-------------------
 
 Each distroseries has a switch that allows administrators to either
 reveal its translations to the public or hide them from the public.
@@ -95,9 +97,65 @@
     release series are not currently available.  Please come back soon.
 
 The same goes for anonymous users.
-
-    >>> login(ANONYMOUS)
-    >>> check_effect_of_hiding(hoary)
-    User can access revealed translations.
-    User can not access hidden translations: Translations for this
-    release series are not currently available.  Please come back soon.
+Exactly what message is displayed depends on the series' status.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.EXPERIMENTAL
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not available yet.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.DEVELOPMENT
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not available yet.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.FROZEN
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not currently available.
+    Please come back soon.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.CURRENT
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not currently available.
+    Please come back soon.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.SUPPORTED
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not currently available.
+    Please come back soon.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.OBSOLETE
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    This release series is obsolete.  Its translations are no longer
+    available.
+
+    >>> login('[e-mail]')
+    >>> hoary.status = SeriesStatus.FUTURE
+    >>> login(ANONYMOUS)
+    >>> check_effect_of_hiding(hoary)
+    User can access revealed translations.
+    User can not access hidden translations:
+    Translations for this release series are not available yet.
+

=== modified file 'lib/lp/translations/stories/distroseries/xx-distroseries-translations.txt'
--- lib/lp/translations/stories/distroseries/xx-distroseries-translations.txt	2009-12-21 17:18:12 +0000
+++ lib/lp/translations/stories/distroseries/xx-distroseries-translations.txt	2010-03-01 23:03:20 +0000
@@ -46,9 +46,7 @@
 ... but the link is available to administrators:
 
     >>> dtc_browser = setupDTCBrowser()
-
     >>> dtc_browser.open('http://translations.launchpad.dev/ubuntu/hoary')
-
     >>> dtc_browser.getLink('Change settings').click()
 
 Once the administrator hides all translations...
@@ -115,8 +113,8 @@
 non-administrative users.
 
     >>> user_browser.open(
-    ...     'http://translations.launchpad.dev/ubuntu/hoary/+sources/evolution/'
-    ...     '+pots/evolution-2.2')
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/'
+    ...     '+sources/evolution/+pots/evolution-2.2')
     Traceback (most recent call last):
     ...
     TranslationUnavailable: ...
@@ -126,7 +124,7 @@
 
     >>> dtc_browser.open(
     ...     'http://translations.launchpad.dev/ubuntu/hoary/'
-    ...     '+sources/evolution')
+    ...     '+sources/evolution/+pots/evolution-2.2')
 
 There is also an option to set/unset whether translation imports for a
 distribution should be deferred. That option is set also from the same

=== modified file 'lib/lp/translations/templates/distroseries-langchart.pt'
--- lib/lp/translations/templates/distroseries-langchart.pt	2009-12-27 20:58:33 +0000
+++ lib/lp/translations/templates/distroseries-langchart.pt	2010-03-01 23:03:20 +0000
@@ -8,6 +8,11 @@
     <strong>Translations for this series are currently hidden.</strong>
 
     <!-- Bounce regular users to "translations unavailable" page. -->
+    <tal:XXX condition="nothing">
+        20100224 adiroiban bug=527069: The following tal:omit-tag is only
+        used with the sole purpose of raising an exception.
+        There should be a better solution for this problem.
+    </tal:XXX>
     <metal:check-available tal:omit-tag="view/checkTranslationsViewable" />
   </p>