Launchpad itself

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

bug-427319
Merge into devel

Proposed by Adi Roiban on 2009-12-11

Status:

Merged

Approved by:

Guilherme Salgado on 2009-12-11

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp:~adiroiban/launchpad/bug-427319

Merge into:

lp:launchpad

Diff against target:

619 lines (+365/-103)

7 files modified

lib/lp/translations/browser/configure.zcml (+5/-6)
lib/lp/translations/browser/productserieslanguage.py (+0/-57)
lib/lp/translations/browser/serieslanguage.py (+124/-10)
lib/lp/translations/browser/tests/test_distroserieslanguage_views.py (+5/-1)
lib/lp/translations/browser/tests/test_productserieslanguage_views.py (+3/-1)
lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt (+189/-1)
lib/lp/translations/templates/serieslanguage-index.pt (+39/-27)

To merge this branch:

bzr merge lp:~adiroiban/launchpad/bug-427319

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Guilherme Salgado (community)	code	2009-12-11	Approve on 2009-12-12
Martin Albisetti (community)	ui		Approve on 2009-12-11
Review via email: mp+15994@code.launchpad.net

Commit message

Combine the ProductSeriesLanguage and DistroSeriesLanguage in a common view. Inform translators about what they can do for that series translations.

Revision history for this message

Adi Roiban (adiroiban) wrote on 2009-12-11:

= Bug 427319 =

The current note are a bit generic and also they refer to group coordinator, while linking to the group page (note to the group coordinator)

== Proposed fix ==

As a start add a direct link to team contact.

If there is no team to manage the translations for a language, inform translators and refer the person that can be contacted for setting up a new group.

Inform translators that they need to log in in order to make translations.

Inform translators that their suggestion needs to be approved by the managing team and link to the team page.

== Implementation details ==

Since productseries-langauge and distroseries-language views were very similar, I created a SeriesLangauge View inherited by both ProductSeries and DistroSeries.

== Tests ==
New tests were added to cover the added functionality.

./bin/test -vvc -m translations --layer PageTestLayer

== Demo and Q/A ==
Log in as an Admin and change Ubuntu settings so that there is no translation group for Ubuntu
https://translations.launchpad.dev/ubuntu/+settings

Then go to these pages with an anonymous, an authententicated and admin users.
Observe how the note on top of the templates table changes to reflect you current access to the translations.

http://translations.launchpad.dev/ubuntu/hoary/+lang/es
http://translations.launchpad.dev/ubuntu/hoary/+lang/pt_BR

There is a detailed testplan in standalone/xx-serieslanguage-index.pt

= 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/browser/serieslanguage.py
lib/lp/translations/templates/serieslanguage-index.pt

Revision history for this message

Guilherme Salgado (salgado) wrote on 2009-12-11:

Download full text (19.2 KiB)

Hi Adi,

This is a nice improvement to Launchpad's UI, but is also a very nice
refactoring -- I love when I see duplicated code being removed.

I think there's some room for us to improve the way the access-level
messages are generated, and I also have a few stylistic recommendations
below. Please let me know what you think.

review needsfixing

On Fri, 2009-12-11 at 01:30 +0000, Adi Roiban wrote:
> = Bug 427319 =
>
> The current note are a bit generic and also they refer to group
> coordinator, while linking to the group page (note to the group
> coordinator)
>
> == Proposed fix ==
>
> As a start add a direct link to team contact.
>
> If there is no team to manage the translations for a language, inform
> translators and refer the person that can be contacted for setting up
> a new group.
>
> Inform translators that they need to log in in order to make
> translations.
>
> Inform translators that their suggestion needs to be approved by the
> managing team and link to the team page.
>
> == Implementation details ==
>
> Since productseries-langauge and distroseries-language views were very
> similar, I created a SeriesLangauge View inherited by both
> ProductSeries and DistroSeries.
>

Hi Adi,

This is a nice improvement to Launchpad's UI, but is also a very nice
refactoring -- I love when I see duplicated code being removed.

I think there's some room for us to improve the way the access-level
messages are generated, and I also have a few stylistic recommendations
below.  Please let me know what you think.

review needsfixing

On Fri, 2009-12-11 at 01:30 +0000, Adi Roiban wrote:
> = Bug 427319  =
> 
> The current note are a bit generic and also they refer to group
> coordinator, while linking to the group page (note to the group
> coordinator)
> 
> == Proposed fix ==
> 
> As a start add a direct link to team contact.
> 
> If there is no team to manage the translations for a language, inform
> translators and refer the person that can be contacted for setting up
> a new group.
> 
> Inform translators that they need to log in in order to make
> translations.
> 
> Inform translators that their suggestion needs to be approved by the
> managing team and link to the team page.
> 
> == Implementation details ==
> 
> Since productseries-langauge and distroseries-language views were very
> similar, I created a SeriesLangauge View inherited by both
> ProductSeries and DistroSeries.
>

> === removed file 'lib/lp/translations/browser/productserieslanguage.py'
> --- lib/lp/translations/browser/productserieslanguage.py        2009-09-17 12:45:52 +0000
> +++ lib/lp/translations/browser/productserieslanguage.py        1970-01-01 00:00:00 +0000
> @@ -1,57 +0,0 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> -# GNU Affero General Public License version 3 (see the file LICENSE).
> -
> -"""Browser code for Product Series Languages."""
> -
> -__metaclass__ = type
> -
> -__all__ = [
> -    'ProductSeriesLanguageNavigation',
> -    'ProductSeriesLanguageView',
> -    ]
> -
> -from canonical.cachedproperty import cachedproperty
> -from canonical.launchpad.webapp import LaunchpadView
> -from canonical.launchpad.webapp.batching import BatchNavigator
> -from canonical.launchpad.webapp.publisher import Navigation
> -from lp.translations.interfaces.productserieslanguage import (
> -    IProductSeriesLanguage)
> -
> -
> -class ProductSeriesLanguageView(LaunchpadView):
> -    """View class to render translation status for an `IProductSeries`."""
> -
> -    pofiles = None
> -    label = "Translatable templates"
> -
> -    def initialize(self):
> -        self.form = self.request.form
> -
> -        self.batchnav = BatchNavigator(
> -            self.context.productseries.getCurrentTranslationTemplates(),
> -            self.request)
> -
> -        self.context.recalculateCounts()
> -
> -        self.pofiles = self.context.getPOFilesFor(
> -            self.batchnav.currentBatch())
> -        self.parent = self.context.productseries.product
> -
> -    @cachedproperty
> -    def translation_group(self):
> -        return self.context.productseries.product.translationgroup
> -
> -    @cachedproperty
> -    def translation_team(self):
> -        """Is there a translation team for this translation."""
> -        if self.translation_group is not None:
> -            team = self.translation_group.query_translator(
> -                self.context.language)
> -        else:
> -            team = None
> -        return team
> -
> -
> -class ProductSeriesLanguageNavigation(Navigation):
> -    """Navigation for `IProductSeriesLanguage`."""
> -    usedfor = IProductSeriesLanguage
> 
> === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
> --- lib/lp/translations/browser/distroserieslanguage.py 2009-09-17 11:10:49 +0000
> +++ lib/lp/translations/browser/serieslanguage.py       2009-12-11 01:30:49 +0000
> @@ -8,38 +8,52 @@
>  __all__ = [
>      'DistroSeriesLanguageNavigation',
>      'DistroSeriesLanguageView',
> +    'ProductSeriesLanguageNavigation',
> +    'ProductSeriesLanguageView',
>      ]
>  
> +from canonical.cachedproperty import cachedproperty
>  from canonical.launchpad.webapp import LaunchpadView
>  from canonical.launchpad.webapp.batching import BatchNavigator
>  from canonical.launchpad.webapp.publisher import Navigation
>  from lp.translations.interfaces.distroserieslanguage import (
>      IDistroSeriesLanguage)
> -
> -class DistroSeriesLanguageView(LaunchpadView):
> -    """View class to render translation status for an `IDistroSeries`."""
> +from lp.translations.interfaces.translationsperson import (
> +    ITranslationsPerson)
> +from lp.translations.interfaces.translationgroup import (
> +    TranslationPermission)
> +from lp.translations.interfaces.productserieslanguage import (
> +    IProductSeriesLanguage)
> +
> +
> +class SeriesLanguageView(LaunchpadView):
> +    """View class to render translation status for an `IDistroSeries`
> +    and `IProductSeries`"""

This is actually a base class, and it can't be used directly (as its
initialize() depend on self.series being not None), so it's important to
make that clear in the docstring.

>  
>      pofiles = None
>      label = "Translatable templates"
> +    series = None
> +    parent = None
> +    translationgroup = None
>  
>      def initialize(self):
>          self.form = self.request.form
>  
>          self.batchnav = BatchNavigator(
> -            self.context.distroseries.getCurrentTranslationTemplates(),
> +            self.series.getCurrentTranslationTemplates(),
>              self.request)
>  
>          self.pofiles = self.context.getPOFilesFor(
>              self.batchnav.currentBatch())
> -        self.parent = self.context.distroseries.distribution
>  
> -    @property
> +    @cachedproperty

I think using a cached property here doesn't buy us anything as you're
returning something that's already "cached" in an instance variable.
And even if it wasn't store in an instance variable, it'd be cached by
the ORM.

>      def translation_group(self):
> -        return self.context.distroseries.distribution.translationgroup
> +        """Is there a translation group for these translations."""

I think it'd be nicer if the docstring stated what the property will
return instead of a question, e.g.

"""Return the translation group for these translations.

Return None if there's no translation group for them.
    """

> +        return self.translationgroup
>  
> -    @property
> +    @cachedproperty

In this property you do a search (.query_translator), so the cached
property is beneficial.

>      def translation_team(self):
> -        """Is there a translation team for this translation."""
> +        """Is there a translation team for these translations."""

Same as above for the docstring.

>          if self.translation_group is not None:
>              team = self.translation_group.query_translator(
>                  self.context.language)
> @@ -47,7 +61,106 @@
>              team = None
>          return team
>  
> +    @property
> +    def show_not_logged_in(self):
> +        """Should we display a notice that user is not logged in?"""
> +        return self.user is None

I think this property is trivial enough that it could be inlined
wherever it's used.  In templates you'd do tal:condition="not:view/user"
and in python code you'd do as above.  I think that'd be more readable,
but it'd also allow us to get rid of the above property.

> +
> +    @property
> +    def show_no_license(self):
> +        """Should we display a notice that licence was not accepted?"""
> +        if self.show_not_logged_in:
> +            return False
> +        translations_person = ITranslationsPerson(self.user)
> +        return not translations_person.translations_relicensing_agreement
> +
> +    @property
> +    def show_full_edit(self):
> +        """Should we display a notice that user is not logged in?"""

not logged in!? ;)

> +        if (self.show_not_logged_in or
> +            self.show_no_license):
> +            return False
> +
> +        sample_pofile = self.pofiles[0]
> +        if sample_pofile is None:
> +            return False
> +
> +        return sample_pofile.canEditTranslations(self.user)
> +
> +    @property
> +    def show_can_suggest(self):
> +        """Should we display a notice that user is not logged in?"""
> +        if (self.show_not_logged_in or
> +            self.show_no_license or
> +            self.show_full_edit):
> +            return False
> +
> +        sample_pofile = self.pofiles[0]
> +        if sample_pofile is None:
> +            return False
> +
> +        return sample_pofile.canAddSuggestions(self.user)
> +
> +    @property
> +    def show_only_managers(self):
> +        """Should we display a notice that user is not logged in?"""
> +        if (self.show_not_logged_in or
> +            self.show_no_license or
> +            self.show_full_edit or
> +            self.show_can_suggest):
> +            return False
> +
> +        sample_pofile = self.pofiles[0]
> +        if sample_pofile is None:
> +            return False
> +
> +        if (sample_pofile.translationpermission ==
> +                                        TranslationPermission.CLOSED):
> +            return True
> +        else:
> +            return False
> +
> +    @property
> +    def show_no_managers(self):
> +        """Should we display a notice that user is not logged in?"""
> +        if (self.show_not_logged_in or
> +            self.show_no_license or
> +            self.show_full_edit or
> +            self.show_can_suggest or
> +            self.show_only_managers):
> +            return False
> +        if self.translation_team is None:
> +            return True
> +        else:
> +            return False

It'd be nicer (and a lot simpler) if we generated the access level
message in the view, with the template just calling a single method
which returns it.  Something like:

@property
    def access_level_description(self):
        if self.user is None:
            return "You are not logged in..."

translations_person = ITranslationsPerson(self.user)
        if not translations_person.translations_relicensing_agreement:
            return "To make translations in Launchpad..."

sample_pofile = self.pofiles[0]
        if sample_pofile is not None:
            if sample_pofile.canEditTranslations(self.user):
                return "You can add and review translations."

if sample_pofile.canAddSuggestions(self.user):
                return "Your suggestions will be held for review..."

permission = sample_pofile.translationpermission
            if permission == TranslationPermission.CLOSED:
                return "This templates can be translated only by its managers"

# no-managers
        if self.translation_team is None:
            return "Since there is nobody to manage translation..."

raise AssertionError(
            "BUG! Couldn't identify the user's access level for these "
            "translations.")

And in the template you'd just do like this

<p tal:content="structure view/access_level_description">
      This user's access level for these translations.
   </p>

> +
> +
> +class DistroSeriesLanguageView(SeriesLanguageView, LaunchpadView):
> +    """View class to render translation status for an `IDistroSeries`."""
> +
> +    def initialize(self):
> +        self.series = self.context.distroseries
> +        SeriesLanguageView.initialize(self)

You should use super(DistroSeriesLanguageView, self).initialize() here.

Also, since SeriesLanguageView.initialize() expects self.series to be
defined before it's called, it'd be nice to make it a required argument
of that method, so here you'd do

super(DistroSeriesLanguageView, self).initialize(
        self.context.distroseries)

And it'd assign the given distroseries to self.series.

> +        self.parent = self.series.distribution
> +        self.translationgroup = self.series.distribution.translationgroup
> +
> +
> +class ProductSeriesLanguageView(SeriesLanguageView, LaunchpadView):
> +    """View class to render translation status for an `IProductSeries`."""
> +
> +    def initialize(self):
> +        self.series = self.context.productseries
> +        SeriesLanguageView.initialize(self)

Same here.

> +        self.context.recalculateCounts()
> +        self.parent = self.series.product
> +        self.translationgroup = self.series.product.translationgroup
> +
>  
>  class DistroSeriesLanguageNavigation(Navigation):
>      """Navigation for `IDistroSeriesLanguage`."""
>      usedfor = IDistroSeriesLanguage
> +
> +
> +class ProductSeriesLanguageNavigation(Navigation):
> +    """Navigation for `IProductSeriesLanguage`."""
> +    usedfor = IProductSeriesLanguage
>

s/of/or?

> +========================================================
> +
> +These pages are used by translators for accessing all templates in a 
> +release series and viewing translation statistics for each template for
> +a specific language
>  
>  The Translations page for a product release series with multiple
>  templates links to per-language overviews.
> @@ -11,4 +16,97 @@
>      >>> print browser.title
>      Portuguese (Brazil) (pt_BR) : Translations : Series trunk : Evolution
>  
> +Since there is no translation team to manage Portuguese (Brazil) language
> +in the Evolution's translation group, all user will be informed about it

s/user/users

> +and pointed to the translation group owner.
> +
> +    >>> print extract_text(find_tag_by_id(browser.contents, 'group-team-info'))

The above line has more than 79 chars, doesn't it?

> +    There is no team to manage Evolution ... translations to ...
> +    To set one up, please get in touch with Carlos Perelló Marín.
> +
> +Anonymous users are informed that in order to make translations they
> +need to login first.
> +
> +    >>> browser.getLink('log in').click()
> +    >>> print browser.url
> +    http://translations.launchpad.dev/evolution/trunk/+lang/pt_BR/+login

Where's the message that tell them they should login?

> +
> +Authenticated users will see information about what they can do in
> +these translations. Things like review, only add suggestion or no
> +changes at all.
> +
> +If a product or distribution had no translation group, visitors are
> +informed about this fact and will be able to add translations without
> +requiring a review.
> +
> +    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))

The two lines above are too long as well.

> +    There is no translation group to manage Ubuntu translations.
> +
> +A translation group is created for Ubuntu, togheter with a translation

typo: togheter

> +person for managing Ubuntu Spanish translations.
> +The translation policy is also defined as RESTRICTED

I assume you're creating these things just so that you can test the
behaviour of that page when they exist, right?  That should be explained
in the text above.

> +
> +    >>> from canonical.launchpad.ftests import login, logout

These two names are already available in page tests; no need to import
them.  To see all the names that are available, see setUpGlobs() in
lib/canonical/launchpad/testing/pages.py

> +    >>> from zope.component import getUtility
> +    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
> +    >>> from lp.translations.interfaces.translationgroup import (
> +    ...     TranslationPermission)
> +    >>> login('foo.bar@canonical.com')
> +    >>> utc_member = factory.makePerson(email='utd@ex.com', name='utc.member',
> +    ...                 password='test', displayname='Some Guy')

Do you really need to specify an email, name and password for this
person?  You don't seem to use that anywhere in the test...

> +    >>> utc_team = factory.makeTeam(owner=utc_member, name='utc-team',
> +    ...                 displayname='Ubuntu Translation Coordinators')
> +    >>> utg = factory.makeTranslationGroup(owner=utc_team, name='utg',
> +    ...                 title='Ubuntu Translation Group')
> +    >>> st_coordinator = factory.makePerson(email='st@ex.com',
> +    ...                 name='st-coordinator', password='test',

Same here for the name, email and password.

> +    ...                 displayname='Ubuntu Spanish Translator')

The indentation here is weird. Please use the recommended style:
<https://dev.launchpad.net/PythonStyleGuide#Multiline function calls>

> +
> +    >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> +    >>> ubuntu.translationgroup = utg
> +    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
> +    >>> someone = factory.makeTranslator('es', group=utg, person=st_coordinator)

Line too long.

And I don't think 'someone' is a good name for a translator. ;)

> +    >>> logout()
> +
> +Spanish has a translation team for managing its translations and all
> +Evolution Spanish templates can be access from the distribution series

s/access/accessed

> +translation page.
> +
> +    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
> +    >>> user_browser.getLink('Spanish').click()
> +    >>> print user_browser.url
> +    http://translations.launchpad.dev/ubuntu/hoary/+lang/es
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
> +    These Ubuntu translations are managed by Ubuntu Spanish Translator.
> +
> +Authenticated users can add suggestion but will be held for review by
> +the members of Spanish translations team.
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
> +    Your suggestions ... Ubuntu Spanish Translator...

Can you include a bit more of the text above?

Also, there are two lines which are too long above.

> +
> +Catalan has no translation team for managing translations and since
> +there is no one to review the work, authenticated users can not add
> +suggestions.
> +
> +    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
> +    >>> user_browser.getLink('Catalan').click()
> +    >>> print user_browser.url
> +    http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
> +    There is no team ... Catalan. ... Ubuntu Translation Coordinators.
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))

Lines too long.

> +    Since ... Ubuntu Translation Coordinators. ...

It'd be nice to show a bit more of the two messages above.

> +
> +Members of translation team and translations admins have full access to
> +translations. They can add and review translations.
> +
> +    >>> admin_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
> +    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'translation-access-level'))
> +    You can add ...

Ditto for the long lines and showing more of the message.

> +
>  The details of the page are tested at the view level.
>

-- 
Guilherme Salgado <salgado@canonical.com>

review: Needs Fixing

Revision history for this message

Adi Roiban (adiroiban) wrote on 2009-12-11:

Download full text (19.0 KiB)

Many thanks for the review.
Here is the diff.

from canonical.cachedproperty import cachedproperty
from canonical.launchpad.webapp import LaunchpadView
+from canonical.launchpad.webapp.tales import PersonFormatterAPI
from canonical.launchpad.webapp.batching import BatchNavigator
from canonical.launchpad.webapp.publisher import Navigation
from lp.translations.interfaces.distroserieslanguage import (
@@ -26,9 +27,12 @@
IProductSeriesLanguage)

-class SeriesLanguageView(LaunchpadView):
- """View class to render translation status for an `IDistroSeries`
- and `IProductSeries`"""
+class BaseSeriesLanguageView(LaunchpadView):
+ """View base class to render translation status for an
+ `IDistroSeries` and `IProductSeries`
+
+ This class should not be directly instantiated.
+ """

     pofiles = None
     label = "Translatable templates"
@@ -36,7 +40,9 @@
     parent = None
     translationgroup = None

- def initialize(self):
+ def initialize(self, series, translationgroup):
+ self.series = series
+ self.translationgroup = translationgroup
self.form = self.request.form

         self.batchnav = BatchNavigator(
@@ -46,14 +52,20 @@
         self.pofiles = self.context.getPOFilesFor(
             self.batchnav.currentBatch())

- @cachedproperty
+ @property
def translation_group(self):
- """Is there a translation group for these translations."""
+ """Return the translation group for these translations.
+
+ Return None if there's no translation group for them.
+ """
return self.translationgroup

     @cachedproperty
     def translation_team(self):
- """Is there a translation team for these translations."""
+ """Return the translation team for these translations.
+
+ Return None if there's no translation team for them.
+ """
         if self.translation_group is not None:
             team = self.translation_group.query_translator(
                 self.context.language)
@@ -62,98 +74,80 @@
         return team

@property
- def show_not_logged_in(self):
- """Should we display a notice that user is not logged in?"""
- return self.user is None
+ def access_level_description(self):
+ if self.user is None:
+ return ("You are not logged in. Please log in to work " +
+ "on translations.")

- @property
- def show_no_license(self):
- """Should we display a notice that licence was not accepted?"""
- if self.show_not_logged_in:
- return False
translations_person = ITranslationsPerson(self.user)
- return not translations_person.translations_relicensing_agreement
-
- @property
- def show_full_edit(self):
- """Should we display a notice that user is not logged in?"""
- if (self.show_not_logged_in or
- self.show_no_license):
- return False
-
- ...

Many thanks for the review.
Here is the diff.

=== modified file 'lib/lp/translations/browser/serieslanguage.py'
--- lib/lp/translations/browser/serieslanguage.py	2009-12-11 01:12:22 +0000
+++ lib/lp/translations/browser/serieslanguage.py	2009-12-11 16:09:04 +0000
@@ -14,6 +14,7 @@
 
 from canonical.cachedproperty import cachedproperty
 from canonical.launchpad.webapp import LaunchpadView
+from canonical.launchpad.webapp.tales import PersonFormatterAPI
 from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.launchpad.webapp.publisher import Navigation
 from lp.translations.interfaces.distroserieslanguage import (
@@ -26,9 +27,12 @@
     IProductSeriesLanguage)
 
 
-class SeriesLanguageView(LaunchpadView):
-    """View class to render translation status for an `IDistroSeries`
-    and `IProductSeries`"""
+class BaseSeriesLanguageView(LaunchpadView):
+    """View base class to render translation status for an 
+    `IDistroSeries` and `IProductSeries`
+
+    This class should not be directly instantiated.
+    """
 
     pofiles = None
     label = "Translatable templates"
@@ -36,7 +40,9 @@
     parent = None
     translationgroup = None
 
-    def initialize(self):
+    def initialize(self, series, translationgroup):
+        self.series = series
+        self.translationgroup = translationgroup
         self.form = self.request.form
 
         self.batchnav = BatchNavigator(
@@ -46,14 +52,20 @@
         self.pofiles = self.context.getPOFilesFor(
             self.batchnav.currentBatch())
 
-    @cachedproperty
+    @property
     def translation_group(self):
-        """Is there a translation group for these translations."""
+        """Return the translation group for these translations.
+
+        Return None if there's no translation group for them.
+        """
         return self.translationgroup
 
     @cachedproperty
     def translation_team(self):
-        """Is there a translation team for these translations."""
+        """Return the translation team for these translations.
+
+        Return None if there's no translation team for them.
+        """
         if self.translation_group is not None:
             team = self.translation_group.query_translator(
                 self.context.language)
@@ -62,98 +74,80 @@
         return team
 
     @property
-    def show_not_logged_in(self):
-        """Should we display a notice that user is not logged in?"""
-        return self.user is None
+    def access_level_description(self):
+        if self.user is None:
+            return ("You are not logged in. Please log in to work " +
+                    "on translations.")
 
-    @property
-    def show_no_license(self):
-        """Should we display a notice that licence was not accepted?"""
-        if self.show_not_logged_in:
-            return False
         translations_person = ITranslationsPerson(self.user)
-        return not translations_person.translations_relicensing_agreement
-
-    @property
-    def show_full_edit(self):
-        """Should we display a notice that user is not logged in?"""
-        if (self.show_not_logged_in or
-            self.show_no_license):
-            return False
-
-        sample_pofile = self.pofiles[0]
-        if sample_pofile is None:
-            return False
-
-        return sample_pofile.canEditTranslations(self.user)
-
-    @property
-    def show_can_suggest(self):
-        """Should we display a notice that user is not logged in?"""
-        if (self.show_not_logged_in or
-            self.show_no_license or
-            self.show_full_edit):
-            return False
-
-        sample_pofile = self.pofiles[0]
-        if sample_pofile is None:
-            return False
-
-        return sample_pofile.canAddSuggestions(self.user)
-
-    @property
-    def show_only_managers(self):
-        """Should we display a notice that user is not logged in?"""
-        if (self.show_not_logged_in or
-            self.show_no_license or
-            self.show_full_edit or
-            self.show_can_suggest):
-            return False
-
-        sample_pofile = self.pofiles[0]
-        if sample_pofile is None:
-            return False
-
-        if (sample_pofile.translationpermission ==
-                                        TranslationPermission.CLOSED):
-            return True
-        else:
-            return False
-
-    @property
-    def show_no_managers(self):
-        """Should we display a notice that user is not logged in?"""
-        if (self.show_not_logged_in or
-            self.show_no_license or
-            self.show_full_edit or
-            self.show_can_suggest or
-            self.show_only_managers):
-            return False
+        translations_contact_link = None
+
+        if self.translation_team:
+            translations_contact_link = PersonFormatterAPI(
+                self.translation_team.translator).link(None)
+        elif self.translation_group:
+            translations_contact_link = PersonFormatterAPI(
+                self.translation_group.owner).link(None)
+
+        if not translations_person.translations_relicensing_agreement:
+            translation_license_url = PersonFormatterAPI(
+                translations_person).url() + '/translations/+licensing'
+            return ("To make translations in Launchpad you need to " +
+                    "agree with the " +
+                    "<a href='%s'>Translations licensing</a>.") % (
+                        translation_license_url)
+
+        sample_pofile = self.pofiles[0]
+        if sample_pofile is not None:
+            if sample_pofile.canEditTranslations(self.user):
+                return "You can add and review translations."
+
+            if sample_pofile.canAddSuggestions(self.user):
+                return ("Your suggestions will be held for review by " +
+                        "the managers of these translations. If you " +
+                        "need help, or your translations are not being " +
+                        "reviewed, please get in touch with " +
+                        "%s") % translations_contact_link
+
+            permission = sample_pofile.translationpermission
+            if permission == TranslationPermission.CLOSED:
+                return ("These templates can be translated only by " +
+                        "its managers")
+
+        # no-managers
         if self.translation_team is None:
-            return True
-        else:
-            return False
-
-
-class DistroSeriesLanguageView(SeriesLanguageView, LaunchpadView):
+            return ("Since there is nobody to manage translation " +
+                    "approvals into this language, your cannot add " +
+                    "new suggestions. If you are interested in making " +
+                    "translations, please contact %s") % (
+                        translations_contact_link)
+
+        raise AssertionError(
+            "BUG! Couldn't identify the user's access level for these "
+            "translations.")
+
+
+class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
     """View class to render translation status for an `IDistroSeries`."""
 
     def initialize(self):
-        self.series = self.context.distroseries
-        SeriesLanguageView.initialize(self)
+        series = self.context.distroseries
+        super(DistroSeriesLanguageView, self).initialize(
+            series=series,
+            translationgroup=series.distribution.translationgroup)
         self.parent = self.series.distribution
-        self.translationgroup = self.series.distribution.translationgroup
-
-
-class ProductSeriesLanguageView(SeriesLanguageView, LaunchpadView):
+
+
+class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
     """View class to render translation status for an `IProductSeries`."""
 
     def initialize(self):
-        self.series = self.context.productseries
-        SeriesLanguageView.initialize(self)
+        series = self.context.productseries
+        super(ProductSeriesLanguageView, self).initialize(
+            series=series,
+            translationgroup=series.product.translationgroup) 
         self.context.recalculateCounts()
         self.parent = self.series.product
-        self.translationgroup = self.series.product.translationgroup
 
 
 class DistroSeriesLanguageNavigation(Navigation):

=== modified file 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
--- lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 01:03:25 +0000
+++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 16:15:33 +0000
@@ -1,4 +1,4 @@
-Product of distribution release series language overview
+Product or distribution release series language overview
 ========================================================
 
 These pages are used by translators for accessing all templates in a 
@@ -17,19 +17,20 @@
     Portuguese (Brazil) (pt_BR) : Translations : Series trunk : Evolution
 
 Since there is no translation team to manage Portuguese (Brazil) language
-in the Evolution's translation group, all user will be informed about it
+in the Evolution's translation group, all users will be informed about it
 and pointed to the translation group owner.
 
-    >>> print extract_text(find_tag_by_id(browser.contents, 'group-team-info'))
+    >>> print extract_text(find_tag_by_id(
+    ...     browser.contents, 'group-team-info'))
     There is no team to manage Evolution ... translations to ...
     To set one up, please get in touch with Carlos Perelló Marín.
 
 Anonymous users are informed that in order to make translations they
 need to login first.
 
-    >>> browser.getLink('log in').click()
-    >>> print browser.url
-    http://translations.launchpad.dev/evolution/trunk/+lang/pt_BR/+login
+    >>> print extract_text(
+    ...     find_tag_by_id(browser.contents, 'translation-access-level'))
+    You are not logged in. Please log in to work on translations...
 
 Authenticated users will see information about what they can do in
 these translations. Things like review, only add suggestion or no
@@ -39,53 +40,61 @@
 informed about this fact and will be able to add translations without
 requiring a review.
 
-    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
+    >>> user_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
+    >>> print extract_text(
+    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
     There is no translation group to manage Ubuntu translations.
 
-A translation group is created for Ubuntu, togheter with a translation
-person for managing Ubuntu Spanish translations.
-The translation policy is also defined as RESTRICTED
+Create a translation group for Ubuntu, togheter with a translation
+person for managing Ubuntu Spanish translations and set translation 
+policy to RESTRICTED.
+This is done to so see what the page will look like when they exist.
 
-    >>> from canonical.launchpad.ftests import login, logout
     >>> from zope.component import getUtility
     >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
     >>> from lp.translations.interfaces.translationgroup import (
     ...     TranslationPermission)
     >>> login('foo.bar@canonical.com')
-    >>> utc_member = factory.makePerson(email='utd@ex.com', name='utc.member',
-    ...                 password='test', displayname='Some Guy')
-    >>> utc_team = factory.makeTeam(owner=utc_member, name='utc-team',
-    ...                 displayname='Ubuntu Translation Coordinators')
-    >>> utg = factory.makeTranslationGroup(owner=utc_team, name='utg',
-    ...                 title='Ubuntu Translation Group')
-    >>> st_coordinator = factory.makePerson(email='st@ex.com',
-    ...                 name='st-coordinator', password='test',
-    ...                 displayname='Ubuntu Spanish Translator')
+    >>> utc_owner = factory.makePerson(displayname='Some Guy')
+    >>> utc_team = factory.makeTeam(
+    ...     owner=utc_owner, name='utc-team',
+    ...     displayname='Ubuntu Translation Coordinators')
+    >>> utg = factory.makeTranslationGroup(
+    ...     owner=utc_team, name='utg', title='Ubuntu Translation Group')
+    >>> st_coordinator = factory.makePerson(
+    ...     displayname='Ubuntu Spanish Translator')
 
     >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
     >>> ubuntu.translationgroup = utg
     >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
-    >>> someone = factory.makeTranslator('es', group=utg, person=st_coordinator)
+    >>> translators = factory.makeTranslator(
+    ...     'es', group=utg, person=st_coordinator)
     >>> logout()
 
 Spanish has a translation team for managing its translations and all
-Evolution Spanish templates can be access from the distribution series 
+Evolution Spanish templates can be accessed from the distribution series 
 translation page.
 
-    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
+    >>> user_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
     >>> user_browser.getLink('Spanish').click()
     >>> print user_browser.url
     http://translations.launchpad.dev/ubuntu/hoary/+lang/es
 
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
+    >>> print extract_text(
+    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
     These Ubuntu translations are managed by Ubuntu Spanish Translator.
 
 Authenticated users can add suggestion but will be held for review by
 the members of Spanish translations team.
 
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
-    Your suggestions ... Ubuntu Spanish Translator...
+    >>> print extract_text(
+    ...     find_tag_by_id(
+    ...         user_browser.contents, 'translation-access-level'))
+    Your suggestions will be held for review by the managers of
+    these translations. If you need help, ...
+    please get in touch with Ubuntu Spanish Translator...
 
 Catalan has no translation team for managing translations and since
 there is no one to review the work, authenticated users can not add
@@ -97,16 +106,19 @@
     http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
 
     >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
-    There is no team ... Catalan. ... Ubuntu Translation Coordinators.
+    There is no team to manage ... To set one up, please get in touch
+    with Ubuntu Translation Coordinators.
 
     >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
-    Since ... Ubuntu Translation Coordinators. ...
+    Since there is nobody to manage translation ...
+    your cannot add new suggestions. If you are interested in making
+    translations, please contact Ubuntu Translation Coordinators...
 
 Members of translation team and translations admins have full access to
 translations. They can add and review translations.
 
     >>> admin_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
     >>> print extract_text(find_tag_by_id(admin_browser.contents, 'translation-access-level'))
-    You can add ...
+    You can add and review translations...
 
 The details of the page are tested at the view level.

=== modified file 'lib/lp/translations/templates/serieslanguage-index.pt'
--- lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 01:12:22 +0000
+++ lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 16:09:04 +0000
@@ -47,63 +47,11 @@
           translations.
         </p>
       </tal:without_group>
-      <p condition="view/translation_group" id="translation-access-level">
-        <tal:not-logged-in condition="view/show_not_logged_in">
-          You are not logged in. Please <a href="+login" >log in</a>
-          to work on translations.
-        </tal:not-logged-in>
-
-        <tal:no-license condition="view/show_no_license">
-          To make translations in Launchpad you need to agree with the
-          <a tal:attributes="href request/lp:person/fmt:url:translations/+licensing"
-          >Translations licensing</a> first.
-        </tal:no-license>
-
-        <tal:full-edit condition="view/show_full_edit">
-          You can add and review translations.
-        </tal:full-edit>
-
-        <tal:can-suggest condition="view/show_can_suggest">
-          <tal:comment replace="nothing">
-          If there is no group, translators have full access, but
-          i add this check in case things will change.
-          </tal:comment>
-          <tal:have-group condition="view/translation_group">
-          Your suggestions will be held for review by the managers of
-          these translations. If you need help, or your translations are
-          not being reviewed, please get in touch with
-          <tal:team
-            condition="view/translation_team"
-            replace="structure
-                             view/translation_team/translator/fmt:link">
-          Serbian translators</tal:team><tal:group
-            condition="not: view/translation_team"
-            replace="structure view/translation_group/owner/fmt:link">
-          Serbian translators</tal:group>.
-          </tal:have-group>
-        </tal:can-suggest>
-
-        <tal:no-managers condition="view/show_no_managers">
-          <tal:comment replace="nothing">
-          If there is no group, translators have full access, but
-          i add this check in case things will change.
-          </tal:comment>
-          <tal:have-group condition="view/translation_group">
-          Since there is nobody to manage translation approvals into
-          <a tal:attributes="href context/language/fmt:url"
-             tal:content="context/language/englishname">Serbian</a>
-          , your cannot add new suggestions. If you are interested
-          in making translations, please contact
-          <a tal:replace="structure view/translation_group/owner/fmt:link"
-          >Ubuntu Translators</a>.
-          </tal:have-group>
-        </tal:no-managers>
-
-        <tal:only-managers condition="view/show_only_managers">
-          This templates can be translated only by its managers.
-        </tal:only-managers>
-
+      <p id="translation-access-level" 
+         condition="view/translation_group">
+        <span tal:replace="structure view/access_level_description">
+            You can add suggestions for these translations.
+        </span>
         Templates which are more important to translate are listed first.
       </p>
     </div>

Revision history for this message

Guilherme Salgado (salgado) wrote on 2009-12-11:

Download full text (15.1 KiB)

Hi Adi,

I have just a few minor suggestions but this will probably be the last
round; this branch should be ready to land once they're addressed.

> === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
> --- lib/lp/translations/browser/distroserieslanguage.py 2009-09-17 11:10:49 +0000
> +++ lib/lp/translations/browser/serieslanguage.py 2009-12-11 16:18:29 +0000
>
> +
> +class BaseSeriesLanguageView(LaunchpadView):
[...]
> @@ -47,7 +73,88 @@
> team = None
> return team
>
> + @property
> + def access_level_description(self):
> + if self.user is None:
> + return ("You are not logged in. Please log in to work " +
> + "on translations.")

When you use parenthesis for multi-line strings in python, they're
concatenated automatically, so you don't need the '+' here.

> +
> + translations_person = ITranslationsPerson(self.user)
> + translations_contact_link = None
> +
> + if self.translation_team:
> + translations_contact_link = PersonFormatterAPI(
> + self.translation_team.translator).link(None)
> + elif self.translation_group:
> + translations_contact_link = PersonFormatterAPI(
> + self.translation_group.owner).link(None)

Is it possible that self.translation_team and self.translation_group are
both None, thus leaving translations_contact_link set to None as well?
I hope that's not possible as it'd cause the code below to crash, and in
that case we can make this assumption (that we expect at least one of
them to be non-None) clear in the code by adding an else block with an
AssertionError.

else:
raise AssertionError("No translation team/group found.")

And then you can also remove the "translations_contact_link = None"
line.

> +
> + if not translations_person.translations_relicensing_agreement:
> + translation_license_url = PersonFormatterAPI(
> + translations_person).url() + '/translations/+licensing'

Have you tried calling .url(view_name='+licensing') here? In fact, I
think you need that because the URL generated by the code above is wrong
(e.g. ~user/translations/+licensing, which is a 404). If you use the
view_name argument it will make sure the generated URL is not a 404.

> + return ("To make translations in Launchpad you need to " +
> + "agree with the " +

No need to add the strings here either. :)

Hi Adi,

I have just a few minor suggestions but this will probably be the last
round; this branch should be ready to land once they're addressed.

> === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
> --- lib/lp/translations/browser/distroserieslanguage.py	2009-09-17 11:10:49 +0000
> +++ lib/lp/translations/browser/serieslanguage.py	2009-12-11 16:18:29 +0000
> 
> +
> +class BaseSeriesLanguageView(LaunchpadView):
[...]
> @@ -47,7 +73,88 @@
>              team = None
>          return team
>  
> +    @property
> +    def access_level_description(self):
> +        if self.user is None:
> +            return ("You are not logged in. Please log in to work " +
> +                    "on translations.")

When you use parenthesis for multi-line strings in python, they're
concatenated automatically, so you don't need the '+' here.

> +
> +        translations_person = ITranslationsPerson(self.user)
> +        translations_contact_link = None
> +
> +        if self.translation_team:
> +            translations_contact_link = PersonFormatterAPI(
> +                self.translation_team.translator).link(None)
> +        elif self.translation_group:
> +            translations_contact_link = PersonFormatterAPI(
> +                self.translation_group.owner).link(None)

else:
        raise AssertionError("No translation team/group found.")

And then you can also remove the "translations_contact_link = None"
line.

> +
> +        if not translations_person.translations_relicensing_agreement:
> +            translation_license_url = PersonFormatterAPI(
> +                translations_person).url() + '/translations/+licensing'

Have you tried calling .url(view_name='+licensing') here?  In fact, I
think you need that because the URL generated by the code above is wrong
(e.g. ~user/translations/+licensing, which is a 404). If you use the
view_name argument it will make sure the generated URL is not a 404.

> +            return ("To make translations in Launchpad you need to " +
> +                    "agree with the " +

No need to add the strings here either. :)

> +                    "<a href='%s'>Translations licensing</a>.") % (
> +                        translation_license_url)
> +
> +        sample_pofile = self.pofiles[0]
> +        if sample_pofile is not None:
> +            if sample_pofile.canEditTranslations(self.user):
> +                return "You can add and review translations."
> +
> +            if sample_pofile.canAddSuggestions(self.user):
> +                return ("Your suggestions will be held for review by " +
> +                        "the managers of these translations. If you " +
> +                        "need help, or your translations are not being " +
> +                        "reviewed, please get in touch with " +
> +                        "%s") % translations_contact_link

Redundant '+'

> +
> +            permission = sample_pofile.translationpermission
> +            if permission == TranslationPermission.CLOSED:
> +                return ("These templates can be translated only by " +
> +                        "its managers")
> +
> +        # no-managers

I think this comment is kinda redundant as well, so I'd just remove it.

> +        if self.translation_team is None:
> +            return ("Since there is nobody to manage translation " +
> +                    "approvals into this language, your cannot add " +
> +                    "new suggestions. If you are interested in making " +
> +                    "translations, please contact %s") % (
> +                        translations_contact_link)

Redundant '+' here too

> +
> +        raise AssertionError(
> +            "BUG! Couldn't identify the user's access level for these "
> +            "translations.")
> +
> +
> +class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> +    """View class to render translation status for an `IDistroSeries`."""
> +
> +    def initialize(self):
> +        series = self.context.distroseries
> +        super(DistroSeriesLanguageView, self).initialize(
> +            series=series,
> +            translationgroup=series.distribution.translationgroup)
> +        self.parent = self.series.distribution
> +
> +
> +class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> +    """View class to render translation status for an `IProductSeries`."""
> +
> +    def initialize(self):
> +        series = self.context.productseries
> +        super(ProductSeriesLanguageView, self).initialize(
> +            series=series,

I'd drop the 'series = self.context....' line in both classes above and
just pass series=self.context... to super's initialize().

> +            translationgroup=series.product.translationgroup) 
> +        self.context.recalculateCounts()
> +        self.parent = self.series.product
> +
>  
>  class DistroSeriesLanguageNavigation(Navigation):
>      """Navigation for `IDistroSeriesLanguage`."""
>      usedfor = IDistroSeriesLanguage
> +
> +
> +class ProductSeriesLanguageNavigation(Navigation):
> +    """Navigation for `IProductSeriesLanguage`."""
> +    usedfor = IProductSeriesLanguage

> === renamed file 'lib/lp/translations/stories/productseries/xx-productserieslanguage.txt' => 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
> --- lib/lp/translations/stories/productseries/xx-productserieslanguage.txt	2009-09-12 07:25:21 +0000
> +++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 16:18:29 +0000
> @@ -1,4 +1,9 @@
> -= Product release series language overview =
> +Product or distribution release series language overview
> +========================================================
> +
> +These pages are used by translators for accessing all templates in a 
> +release series and viewing translation statistics for each template for
> +a specific language
>  
>  The Translations page for a product release series with multiple
>  templates links to per-language overviews.
> @@ -11,4 +16,109 @@
>      >>> print browser.title
>      Portuguese (Brazil) (pt_BR) : Translations : Series trunk : Evolution
>  
> +Since there is no translation team to manage Portuguese (Brazil) language
> +in the Evolution's translation group, all users will be informed about it
> +and pointed to the translation group owner.
> +
> +    >>> print extract_text(find_tag_by_id(
> +    ...     browser.contents, 'group-team-info'))
> +    There is no team to manage Evolution ... translations to ...
> +    To set one up, please get in touch with Carlos Perelló Marín.
> +
> +Anonymous users are informed that in order to make translations they
> +need to login first.
> +
> +    >>> print extract_text(
> +    ...     find_tag_by_id(browser.contents, 'translation-access-level'))
> +    You are not logged in. Please log in to work on translations...
> +
> +Authenticated users will see information about what they can do in
> +these translations. Things like review, only add suggestion or no
> +changes at all.
> +
> +If a product or distribution had no translation group, visitors are
> +informed about this fact and will be able to add translations without
> +requiring a review.
> +
> +    >>> user_browser.open(
> +    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
> +    >>> print extract_text(
> +    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
> +    There is no translation group to manage Ubuntu translations.
> +
> +Create a translation group for Ubuntu, togheter with a translation

Oops, you forgot to fix the typo above (together).

> +person for managing Ubuntu Spanish translations and set translation 
> +policy to RESTRICTED.
> +This is done to so see what the page will look like when they exist.
> +
> +    >>> from zope.component import getUtility
> +    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
> +    >>> from lp.translations.interfaces.translationgroup import (
> +    ...     TranslationPermission)
> +    >>> login('foo.bar@canonical.com')
> +    >>> utc_owner = factory.makePerson(displayname='Some Guy')
> +    >>> utc_team = factory.makeTeam(
> +    ...     owner=utc_owner, name='utc-team',
> +    ...     displayname='Ubuntu Translation Coordinators')
> +    >>> utg = factory.makeTranslationGroup(
> +    ...     owner=utc_team, name='utg', title='Ubuntu Translation Group')
> +    >>> st_coordinator = factory.makePerson(
> +    ...     displayname='Ubuntu Spanish Translator')
> +
> +    >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> +    >>> ubuntu.translationgroup = utg
> +    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
> +    >>> translators = factory.makeTranslator(
> +    ...     'es', group=utg, person=st_coordinator)
> +    >>> logout()
> +
> +Spanish has a translation team for managing its translations and all
> +Evolution Spanish templates can be accessed from the distribution series 
> +translation page.
> +
> +    >>> user_browser.open(
> +    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
> +    >>> user_browser.getLink('Spanish').click()
> +    >>> print user_browser.url
> +    http://translations.launchpad.dev/ubuntu/hoary/+lang/es
> +
> +    >>> print extract_text(
> +    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
> +    These Ubuntu translations are managed by Ubuntu Spanish Translator.
> +
> +Authenticated users can add suggestion but will be held for review by
> +the members of Spanish translations team.
> +
> +    >>> print extract_text(
> +    ...     find_tag_by_id(
> +    ...         user_browser.contents, 'translation-access-level'))
> +    Your suggestions will be held for review by the managers of
> +    these translations. If you need help, ...
> +    please get in touch with Ubuntu Spanish Translator...
> +
> +Catalan has no translation team for managing translations and since
> +there is no one to review the work, authenticated users can not add
> +suggestions.
> +
> +    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
> +    >>> user_browser.getLink('Catalan').click()
> +    >>> print user_browser.url
> +    http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
> +    There is no team to manage ... To set one up, please get in touch
> +    with Ubuntu Translation Coordinators.
> +
> +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
> +    Since there is nobody to manage translation ...
> +    your cannot add new suggestions. If you are interested in making
> +    translations, please contact Ubuntu Translation Coordinators...
> +
> +Members of translation team and translations admins have full access to
> +translations. They can add and review translations.
> +
> +    >>> admin_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
> +    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'translation-access-level'))

Some lines are still too long above.  If you use vim I have a nice
config which highlights anything after the 79th character, if you want.
That makes it easy to spot which lines are too long.

> +    You can add and review translations...
> +
>  The details of the page are tested at the view level.
> 
> === modified file 'lib/lp/translations/templates/serieslanguage-index.pt'
> --- lib/lp/translations/templates/serieslanguage-index.pt	2009-09-17 11:10:49 +0000
> +++ lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 16:18:29 +0000
> @@ -17,32 +17,41 @@
>    <tal:block condition="view/pofiles">
>      <div class="top-portlet">
>        <tal:with_group condition="view/translation_group">
> -        <p tal:condition="view/translation_team">
> +        <p tal:condition="view/translation_team" id="group-team-info">
> +          These
>            <tal:product replace="structure
>              view/parent/fmt:link">Evolution</tal:product>
> -          is translated by
> +          translations are managed by
>            <tal:team replace="structure
>                               view/translation_team/translator/fmt:link">
> -            Serbian translators</tal:team> — if you need help, or your
> -          translations are not being reviewed, please get in touch with them.
> +            Serbian translators</tal:team>.
>          </p>
>  
> -        <p tal:condition="not:view/translation_team">
> -          There is no team to translate
> +        <p tal:condition="not:view/translation_team" id="group-team-info">
> +          There is no team to manage
>            <tal:product replace="structure
>              view/parent/fmt:link">Evolution</tal:product>
> -          to
> +          translations to
>            <a tal:attributes="href context/language/fmt:url"
>               tal:content="context/language/englishname">Serbian</a>.
>            To set one up, please get in touch with
> -          <a tal:replace="structure view/translation_group/fmt:link"
> -             >Launchpad Translators</a>
> -          coordinator.
> +          <a tal:replace="structure view/translation_group/owner/fmt:link"
> +             >Launchpad Translators</a>.
>          </p>
>        </tal:with_group>
> -
> -      <p>
> -        Choose a template name to begin translating.
> +      <tal:without_group condition="not: view/translation_group">
> +        <p id="group-team-info">

You used the same id in 3 different elements above.  I understand these
elements are not expected to be present in the page at the same time,
but that's not very clear and if we have a bug and more than one of them
ends up in the page, we'll have even more problems.  In this case you
can wrap everything with a <div> with the id you want, like this:

<div id="group-team-info">
      <p tal:condition=...>
        First
      </p>
      <p tal:condition=...>
        Second
      </p>
      <p tal:condition=...>
        Third
      </p>
    </div>

> +          There is no translation group to manage
> +          <tal:product replace="structure
> +            view/parent/fmt:link">Evolution</tal:product>
> +          translations.
> +        </p>
> +      </tal:without_group>
> +      <p id="translation-access-level" 
> +         condition="view/translation_group">

Oops, there's a bug here.  This <p> will not be left out when there's no
translation group because tal doesn't understand the bare 'condition='
above -- it has to be a 'tal:condition=', although you can use a bare
'condition=' when you use a tal element (e.g. '<tal:foo
condition="...">').

This also suggests to me that we're missing a test to show that the
access level description is left out when there's no translation group.

> +        <span tal:replace="structure view/access_level_description">
> +            You can add suggestions for these translations.
> +        </span>
>          Templates which are more important to translate are listed first.
>        </p>
>      </div>

Revision history for this message

Martin Albisetti (beuno) wrote on 2009-12-11:

11:50 < beuno> adiroiban, nice improvement
11:51 < beuno> a few questions
11:51 < beuno> why not "Your suggestions will be held for review by $TEAM"?
11:53 < beuno> there's also a missing full stop
11:53 < beuno> after the team's name
11:53 < beuno> before "Templates which are..."
11:53 < beuno> other than that, ui=me
11:54 < adiroiban> beuno: ok. I will change that.

review: Approve (ui)

Revision history for this message

Данило Шеган (danilo) wrote on 2009-12-11:

Btw, nice refactoring Adi. Thanks for doing it! It'd be nice to move it even farther where we register only a single view (SeriesLanguageView) for all ISeriesLanguage-implementing objects (note, this interface doesn't exist yet :), but let's leave that for some other time :)

Also, now that these views are unified, it makes sense to also unify the view tests: i.e. I believe mostly setUp method would have to be changed to accommodate different ways to construct distroserieslanguages and productserieslanguages, but that would make it easy to extend these views to sourcepackagelanguage and projectlanguage views in the future.

Revision history for this message

Adi Roiban (adiroiban) wrote on 2009-12-11:

Download full text (29.2 KiB)

În data de Vi, 11-12-2009 la 17:48 +0000, Guilherme Salgado a scris:
> Hi Adi,
>
> I have just a few minor suggestions but this will probably be the last
> round; this branch should be ready to land once they're addressed.
>
> > === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
> > --- lib/lp/translations/browser/distroserieslanguage.py 2009-09-17 11:10:49 +0000
> > +++ lib/lp/translations/browser/serieslanguage.py 2009-12-11 16:18:29 +0000
> >
> > +
> > +class BaseSeriesLanguageView(LaunchpadView):
> [...]
> > @@ -47,7 +73,88 @@
> > team = None
> > return team
> >
> > + @property
> > + def access_level_description(self):
> > + if self.user is None:
> > + return ("You are not logged in. Please log in to work " +
> > + "on translations.")
>
> When you use parenthesis for multi-line strings in python, they're
> concatenated automatically, so you don't need the '+' here.
Done.

> > +
> > + translations_person = ITranslationsPerson(self.user)
> > + translations_contact_link = None
> > +
> > + if self.translation_team:
> > + translations_contact_link = PersonFormatterAPI(
> > + self.translation_team.translator).link(None)
> > + elif self.translation_group:
> > + translations_contact_link = PersonFormatterAPI(
> > + self.translation_group.owner).link(None)
>
> Is it possible that self.translation_team and self.translation_group are
> both None, thus leaving translations_contact_link set to None as well?
> I hope that's not possible as it'd cause the code below to crash, and in
> that case we can make this assumption (that we expect at least one of
> them to be non-None) clear in the code by adding an else block with an
> AssertionError.
>
> else:
> raise AssertionError("No translation team/group found.")
>
> And then you can also remove the "translations_contact_link = None"
> line.

It is possible and it should not be an error.
In such case no information should be display.

In a normal use case we should not reach this line, as that check is
already done in the template.

I added a check and returned "".

În data de Vi, 11-12-2009 la 17:48 +0000, Guilherme Salgado a scris:
> Hi Adi,
> 
> I have just a few minor suggestions but this will probably be the last
> round; this branch should be ready to land once they're addressed.
> 
> > === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
> > --- lib/lp/translations/browser/distroserieslanguage.py	2009-09-17 11:10:49 +0000
> > +++ lib/lp/translations/browser/serieslanguage.py	2009-12-11 16:18:29 +0000
> > 
> > +
> > +class BaseSeriesLanguageView(LaunchpadView):
> [...]
> > @@ -47,7 +73,88 @@
> >              team = None
> >          return team
> >  
> > +    @property
> > +    def access_level_description(self):
> > +        if self.user is None:
> > +            return ("You are not logged in. Please log in to work " +
> > +                    "on translations.")
> 
> When you use parenthesis for multi-line strings in python, they're
> concatenated automatically, so you don't need the '+' here.
Done.

> > +
> > +        translations_person = ITranslationsPerson(self.user)
> > +        translations_contact_link = None
> > +
> > +        if self.translation_team:
> > +            translations_contact_link = PersonFormatterAPI(
> > +                self.translation_team.translator).link(None)
> > +        elif self.translation_group:
> > +            translations_contact_link = PersonFormatterAPI(
> > +                self.translation_group.owner).link(None)
> 
> Is it possible that self.translation_team and self.translation_group are
> both None, thus leaving translations_contact_link set to None as well?
> I hope that's not possible as it'd cause the code below to crash, and in
> that case we can make this assumption (that we expect at least one of
> them to be non-None) clear in the code by adding an else block with an
> AssertionError.
> 
>     else:
>         raise AssertionError("No translation team/group found.")
> 
> And then you can also remove the "translations_contact_link = None"
> line.

It is possible and it should not be an error.
In such case no information should be display.

In a normal use case we should not reach this line, as that check is
already done in the template.

I added a check and returned "".

> > +
> > +        if not translations_person.translations_relicensing_agreement:
> > +            translation_license_url = PersonFormatterAPI(
> > +                translations_person).url() + '/translations/+licensing'
> 
> Have you tried calling .url(view_name='+licensing') here?  In fact, I
> think you need that because the URL generated by the code above is wrong
> (e.g. ~user/translations/+licensing, which is a 404). If you use the
> view_name argument it will make sure the generated URL is not a 404.
> 
> > +            return ("To make translations in Launchpad you need to " +
> > +                    "agree with the " +
> 
> No need to add the strings here either. :)
Done
> > +                    "<a href='%s'>Translations licensing</a>.") % (
> > +                        translation_license_url)
> > +
> > +        sample_pofile = self.pofiles[0]
> > +        if sample_pofile is not None:
> > +            if sample_pofile.canEditTranslations(self.user):
> > +                return "You can add and review translations."
> > +
> > +            if sample_pofile.canAddSuggestions(self.user):
> > +                return ("Your suggestions will be held for review by " +
> > +                        "the managers of these translations. If you " +
> > +                        "need help, or your translations are not being " +
> > +                        "reviewed, please get in touch with " +
> > +                        "%s") % translations_contact_link
> 
> Redundant '+'
Done.
> > +
> > +            permission = sample_pofile.translationpermission
> > +            if permission == TranslationPermission.CLOSED:
> > +                return ("These templates can be translated only by " +
> > +                        "its managers")
> > +
> > +        # no-managers
> 
> I think this comment is kinda redundant as well, so I'd just remove it.
Done.
> > +        if self.translation_team is None:
> > +            return ("Since there is nobody to manage translation " +
> > +                    "approvals into this language, your cannot add " +
> > +                    "new suggestions. If you are interested in making " +
> > +                    "translations, please contact %s") % (
> > +                        translations_contact_link)
> 
> Redundant '+' here too
Done.
> > +
> > +        raise AssertionError(
> > +            "BUG! Couldn't identify the user's access level for these "
> > +            "translations.")
> > +
> > +
> > +class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> > +    """View class to render translation status for an `IDistroSeries`."""
> > +
> > +    def initialize(self):
> > +        series = self.context.distroseries
> > +        super(DistroSeriesLanguageView, self).initialize(
> > +            series=series,
> > +            translationgroup=series.distribution.translationgroup)
> > +        self.parent = self.series.distribution
> > +
> > +
> > +class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> > +    """View class to render translation status for an `IProductSeries`."""
> > +
> > +    def initialize(self):
> > +        series = self.context.productseries
> > +        super(ProductSeriesLanguageView, self).initialize(
> > +            series=series,
> 
> I'd drop the 'series = self.context....' line in both classes above and
> just pass series=self.context... to super's initialize().

I have leave that attribution to avoid a long line that will follow.

translationgroup=self.context.productseries.product.translationgroup

> > +            translationgroup=series.product.translationgroup) 
> > +        self.context.recalculateCounts()
> > +        self.parent = self.series.product
> > +
> >  
> >  class DistroSeriesLanguageNavigation(Navigation):
> >      """Navigation for `IDistroSeriesLanguage`."""
> >      usedfor = IDistroSeriesLanguage
> > +
> > +
> > +class ProductSeriesLanguageNavigation(Navigation):
> > +    """Navigation for `IProductSeriesLanguage`."""
> > +    usedfor = IProductSeriesLanguage
> 
> > === renamed file 'lib/lp/translations/stories/productseries/xx-productserieslanguage.txt' => 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
> > --- lib/lp/translations/stories/productseries/xx-productserieslanguage.txt	2009-09-12 07:25:21 +0000
> > +++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 16:18:29 +0000
> > @@ -1,4 +1,9 @@
> > -= Product release series language overview =
> > +Product or distribution release series language overview
> > +========================================================
> > +
> > +These pages are used by translators for accessing all templates in a 
> > +release series and viewing translation statistics for each template for
> > +a specific language
> >  
> >  The Translations page for a product release series with multiple
> >  templates links to per-language overviews.
> > @@ -11,4 +16,109 @@
> >      >>> print browser.title
> >      Portuguese (Brazil) (pt_BR) : Translations : Series trunk : Evolution
> >  
> > +Since there is no translation team to manage Portuguese (Brazil) language
> > +in the Evolution's translation group, all users will be informed about it
> > +and pointed to the translation group owner.
> > +
> > +    >>> print extract_text(find_tag_by_id(
> > +    ...     browser.contents, 'group-team-info'))
> > +    There is no team to manage Evolution ... translations to ...
> > +    To set one up, please get in touch with Carlos Perelló Marín.
> > +
> > +Anonymous users are informed that in order to make translations they
> > +need to login first.
> > +
> > +    >>> print extract_text(
> > +    ...     find_tag_by_id(browser.contents, 'translation-access-level'))
> > +    You are not logged in. Please log in to work on translations...
> > +
> > +Authenticated users will see information about what they can do in
> > +these translations. Things like review, only add suggestion or no
> > +changes at all.
> > +
> > +If a product or distribution had no translation group, visitors are
> > +informed about this fact and will be able to add translations without
> > +requiring a review.
> > +
> > +    >>> user_browser.open(
> > +    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
> > +    >>> print extract_text(
> > +    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
> > +    There is no translation group to manage Ubuntu translations.
> > +
> > +Create a translation group for Ubuntu, togheter with a translation
> 
> Oops, you forgot to fix the typo above (together).

Hope it's ok this time. I remember that I had changed it.

> > +person for managing Ubuntu Spanish translations and set translation 
> > +policy to RESTRICTED.
> > +This is done to so see what the page will look like when they exist.
> > +
> > +    >>> from zope.component import getUtility
> > +    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
> > +    >>> from lp.translations.interfaces.translationgroup import (
> > +    ...     TranslationPermission)
> > +    >>> login('foo.bar@canonical.com')
> > +    >>> utc_owner = factory.makePerson(displayname='Some Guy')
> > +    >>> utc_team = factory.makeTeam(
> > +    ...     owner=utc_owner, name='utc-team',
> > +    ...     displayname='Ubuntu Translation Coordinators')
> > +    >>> utg = factory.makeTranslationGroup(
> > +    ...     owner=utc_team, name='utg', title='Ubuntu Translation Group')
> > +    >>> st_coordinator = factory.makePerson(
> > +    ...     displayname='Ubuntu Spanish Translator')
> > +
> > +    >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> > +    >>> ubuntu.translationgroup = utg
> > +    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
> > +    >>> translators = factory.makeTranslator(
> > +    ...     'es', group=utg, person=st_coordinator)
> > +    >>> logout()
> > +
> > +Spanish has a translation team for managing its translations and all
> > +Evolution Spanish templates can be accessed from the distribution series 
> > +translation page.
> > +
> > +    >>> user_browser.open(
> > +    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
> > +    >>> user_browser.getLink('Spanish').click()
> > +    >>> print user_browser.url
> > +    http://translations.launchpad.dev/ubuntu/hoary/+lang/es
> > +
> > +    >>> print extract_text(
> > +    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
> > +    These Ubuntu translations are managed by Ubuntu Spanish Translator.
> > +
> > +Authenticated users can add suggestion but will be held for review by
> > +the members of Spanish translations team.
> > +
> > +    >>> print extract_text(
> > +    ...     find_tag_by_id(
> > +    ...         user_browser.contents, 'translation-access-level'))
> > +    Your suggestions will be held for review by the managers of
> > +    these translations. If you need help, ...
> > +    please get in touch with Ubuntu Spanish Translator...
> > +
> > +Catalan has no translation team for managing translations and since
> > +there is no one to review the work, authenticated users can not add
> > +suggestions.
> > +
> > +    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
> > +    >>> user_browser.getLink('Catalan').click()
> > +    >>> print user_browser.url
> > +    http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
> > +
> > +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
> > +    There is no team to manage ... To set one up, please get in touch
> > +    with Ubuntu Translation Coordinators.
> > +
> > +    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
> > +    Since there is nobody to manage translation ...
> > +    your cannot add new suggestions. If you are interested in making
> > +    translations, please contact Ubuntu Translation Coordinators...
> > +
> > +Members of translation team and translations admins have full access to
> > +translations. They can add and review translations.
> > +
> > +    >>> admin_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
> > +    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'translation-access-level'))
> 
> Some lines are still too long above.  If you use vim I have a nice
> config which highlights anything after the 79th character, if you want.
> That makes it easy to spot which lines are too long.

Hope the lines are ok.
I'm using GEdit and trying to move to vim.

> > +    You can add and review translations...
> > +
> >  The details of the page are tested at the view level.
> > 
> > === modified file 'lib/lp/translations/templates/serieslanguage-index.pt'
> > --- lib/lp/translations/templates/serieslanguage-index.pt	2009-09-17 11:10:49 +0000
> > +++ lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 16:18:29 +0000
> > @@ -17,32 +17,41 @@
> >    <tal:block condition="view/pofiles">
> >      <div class="top-portlet">
> >        <tal:with_group condition="view/translation_group">
> > -        <p tal:condition="view/translation_team">
> > +        <p tal:condition="view/translation_team" id="group-team-info">
> > +          These
> >            <tal:product replace="structure
> >              view/parent/fmt:link">Evolution</tal:product>
> > -          is translated by
> > +          translations are managed by
> >            <tal:team replace="structure
> >                               view/translation_team/translator/fmt:link">
> > -            Serbian translators</tal:team> — if you need help, or your
> > -          translations are not being reviewed, please get in touch with them.
> > +            Serbian translators</tal:team>.
> >          </p>
> >  
> > -        <p tal:condition="not:view/translation_team">
> > -          There is no team to translate
> > +        <p tal:condition="not:view/translation_team" id="group-team-info">
> > +          There is no team to manage
> >            <tal:product replace="structure
> >              view/parent/fmt:link">Evolution</tal:product>
> > -          to
> > +          translations to
> >            <a tal:attributes="href context/language/fmt:url"
> >               tal:content="context/language/englishname">Serbian</a>.
> >            To set one up, please get in touch with
> > -          <a tal:replace="structure view/translation_group/fmt:link"
> > -             >Launchpad Translators</a>
> > -          coordinator.
> > +          <a tal:replace="structure view/translation_group/owner/fmt:link"
> > +             >Launchpad Translators</a>.
> >          </p>
> >        </tal:with_group>
> > -
> > -      <p>
> > -        Choose a template name to begin translating.
> > +      <tal:without_group condition="not: view/translation_group">
> > +        <p id="group-team-info">
> 
> You used the same id in 3 different elements above.  I understand these
> elements are not expected to be present in the page at the same time,
> but that's not very clear and if we have a bug and more than one of them
> ends up in the page, we'll have even more problems.  In this case you
> can wrap everything with a <div> with the id you want, like this:

Hope it is ok now.

>     <div id="group-team-info">
>       <p tal:condition=...>
>         First
>       </p>
>       <p tal:condition=...>
>         Second
>       </p>
>       <p tal:condition=...>
>         Third
>       </p>
>     </div>
> 
> > +          There is no translation group to manage
> > +          <tal:product replace="structure
> > +            view/parent/fmt:link">Evolution</tal:product>
> > +          translations.
> > +        </p>
> > +      </tal:without_group>
> > +      <p id="translation-access-level" 
> > +         condition="view/translation_group">
> 
> Oops, there's a bug here.  This <p> will not be left out when there's no
> translation group because tal doesn't understand the bare 'condition='
> above -- it has to be a 'tal:condition=', although you can use a bare
> 'condition=' when you use a tal element (e.g. '<tal:foo
> condition="...">').

It should be fixed.

> This also suggests to me that we're missing a test to show that the
> access level description is left out when there's no translation group.

New test for no translation group added.
Also I'm testing the links.

Hope  browser.getLink('something',index=1) is working.

> > +        <span tal:replace="structure view/access_level_description">
> > +            You can add suggestions for these translations.
> > +        </span>
> >          Templates which are more important to translate are listed first.
> >        </p>
> >      </div>
>

Here is the new diff.

=== modified file 'lib/lp/translations/browser/serieslanguage.py'
--- lib/lp/translations/browser/serieslanguage.py	2009-12-11 16:09:04 +0000
+++ lib/lp/translations/browser/serieslanguage.py	2009-12-11 19:53:07 +0000
@@ -76,7 +76,7 @@
     @property
     def access_level_description(self):
         if self.user is None:
-            return ("You are not logged in. Please log in to work " +
+            return ("You are not logged in. Please log in to work "
                     "on translations.")
 
         translations_person = ITranslationsPerson(self.user)
@@ -89,11 +89,21 @@
             translations_contact_link = PersonFormatterAPI(
                 self.translation_group.owner).link(None)
 
+        if translations_contact_link is None:
+            #Having no translation group is a valid case, but the
+            #template should not call access_level_description for
+            #this condition.
+            #We return a blank screen since the information about
+            #missing group is displaying in a different section
+            return ""
+
         if not translations_person.translations_relicensing_agreement:
             translation_license_url = PersonFormatterAPI(
-                translations_person).url() + '/translations/+licensing'
-            return ("To make translations in Launchpad you need to " +
-                    "agree with the " +
+                translations_person).url(
+                    view_name='+licensing',
+                    rootsite='translations')
+            return ("To make translations in Launchpad you need to "
+                    "agree with the "
                     "<a href='%s'>Translations licensing</a>.") % (
                         translation_license_url)
 
@@ -103,23 +113,23 @@
                 return "You can add and review translations."
 
             if sample_pofile.canAddSuggestions(self.user):
-                return ("Your suggestions will be held for review by " +
-                        "the managers of these translations. If you " +
-                        "need help, or your translations are not being " +
-                        "reviewed, please get in touch with " +
-                        "%s") % translations_contact_link
+                return ("Your suggestions will be held for review by "
+                        "%s. If you need help, or your translations are "
+                        "not being reviewed, please get in touch with "
+                        "%s.") % (
+                            translations_contact_link,
+                            translations_contact_link)
 
             permission = sample_pofile.translationpermission
             if permission == TranslationPermission.CLOSED:
-                return ("These templates can be translated only by " +
-                        "its managers")
+                return ("These templates can be translated only by "
+                        "its managers.")
 
-        # no-managers
         if self.translation_team is None:
-            return ("Since there is nobody to manage translation " +
-                    "approvals into this language, your cannot add " +
-                    "new suggestions. If you are interested in making " +
-                    "translations, please contact %s") % (
+            return ("Since there is nobody to manage translation "
+                    "approvals into this language, your cannot add "
+                    "new suggestions. If you are interested in making "
+                    "translations, please contact %s.") % (
                         translations_contact_link)
 
         raise AssertionError(
@@ -145,7 +155,7 @@
         series = self.context.productseries
         super(ProductSeriesLanguageView, self).initialize(
             series=series,
-            translationgroup=series.product.translationgroup) 
+            translationgroup=series.product.translationgroup)
         self.context.recalculateCounts()
         self.parent = self.series.product

=== modified file 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
--- lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 16:15:33 +0000
+++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-11 19:45:10 +0000
@@ -46,7 +46,7 @@
     ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
     There is no translation group to manage Ubuntu translations.
 
-Create a translation group for Ubuntu, togheter with a translation
+Create a translation group for Ubuntu, together with a translation
 person for managing Ubuntu Spanish translations and set translation 
 policy to RESTRICTED.
 This is done to so see what the page will look like when they exist.
@@ -63,7 +63,8 @@
     >>> utg = factory.makeTranslationGroup(
     ...     owner=utc_team, name='utg', title='Ubuntu Translation Group')
     >>> st_coordinator = factory.makePerson(
-    ...     displayname='Ubuntu Spanish Translator')
+    ...     name="ubuntu-l10n-es",
+    ...     displayname='Ubuntu Spanish Translators')
 
     >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
     >>> ubuntu.translationgroup = utg
@@ -84,7 +85,7 @@
 
     >>> print extract_text(
     ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
-    These Ubuntu translations are managed by Ubuntu Spanish Translator.
+    These Ubuntu translations are managed by Ubuntu Spanish Translators.
 
 Authenticated users can add suggestion but will be held for review by
 the members of Spanish translations team.
@@ -92,33 +93,77 @@
     >>> print extract_text(
     ...     find_tag_by_id(
     ...         user_browser.contents, 'translation-access-level'))
-    Your suggestions will be held for review by the managers of
-    these translations. If you need help, ...
-    please get in touch with Ubuntu Spanish Translator...
+    Your suggestions will be held for review by
+    Ubuntu Spanish Translator...
+    please get in touch with Ubuntu Spanish Translators...
+
+Users will see three references to the team managing these translations.
+
+    >>> print user_browser.getLink(
+    ...     'Ubuntu Spanish Translator',index=0).url
+    http://launchpad.dev/~ubuntu-l10n-es
+
+    >>> print user_browser.getLink(
+    ...     'Ubuntu Spanish Translator',index=1).url
+    http://launchpad.dev/~ubuntu-l10n-es
+
+    >>> print user_browser.getLink(
+    ...     'Ubuntu Spanish Translator',index=2).url
+    http://launchpad.dev/~ubuntu-l10n-es
 
 Catalan has no translation team for managing translations and since
 there is no one to review the work, authenticated users can not add
 suggestions.
 
-    >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/')
+    >>> user_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
     >>> user_browser.getLink('Catalan').click()
     >>> print user_browser.url
     http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
 
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info'))
+    >>> print extract_text(
+    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
     There is no team to manage ... To set one up, please get in touch
     with Ubuntu Translation Coordinators.
 
-    >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level'))
+    >>> print extract_text(find_tag_by_id(
+    ...     user_browser.contents, 'translation-access-level'))
     Since there is nobody to manage translation ...
     your cannot add new suggestions. If you are interested in making
     translations, please contact Ubuntu Translation Coordinators...
 
+    >>> print user_browser.getLink(
+    ...     'Ubuntu Translation Coordinators',index=0).url
+    http://launchpad.dev/~utc-team
+    >>> print user_browser.getLink(
+    ...     'Ubuntu Translation Coordinators',index=1).url
+    http://launchpad.dev/~utc-team
+
 Members of translation team and translations admins have full access to
 translations. They can add and review translations.
 
-    >>> admin_browser.open('http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'translation-access-level'))
+    >>> admin_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+    >>> print extract_text(find_tag_by_id(
+    ...     admin_browser.contents, 'translation-access-level'))
     You can add and review translations...
 
+
+For project with no translation teams, translators see a note stating
+there is no group. No access level information is displayed in this case.
+
+    >>> login('foo.bar@canonical.com')
+    >>> ubuntu.translationgroup = None
+    >>> logout()
+
+    >>> user_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
+    >>> print extract_text(
+    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
+    There is no translation group to manage Ubuntu translations.
+
+    >>> print extract_text(find_tag_by_id(
+    ...     user_browser.contents, 'translation-access-level'))
+    Templates which are more important to translate are listed first.
+
 The details of the page are tested at the view level.

=== modified file 'lib/lp/translations/templates/serieslanguage-index.pt'
--- lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 16:09:04 +0000
+++ lib/lp/translations/templates/serieslanguage-index.pt	2009-12-11 19:42:33 +0000
@@ -16,40 +16,43 @@
 
   <tal:block condition="view/pofiles">
     <div class="top-portlet">
-      <tal:with_group condition="view/translation_group">
-        <p tal:condition="view/translation_team" id="group-team-info">
-          These
-          <tal:product replace="structure
-            view/parent/fmt:link">Evolution</tal:product>
-          translations are managed by
-          <tal:team replace="structure
-                             view/translation_team/translator/fmt:link">
-            Serbian translators</tal:team>.
-        </p>
+      <div id="group-team-info">
+        <tal:with_group condition="view/translation_group">
+          <p tal:condition="view/translation_team">
+            These
+            <tal:product replace="structure
+              view/parent/fmt:link">Evolution</tal:product>
+            translations are managed by
+            <tal:team replace="structure
+                               view/translation_team/translator/fmt:link">
+              Serbian translators</tal:team>.
+          </p>
 
-        <p tal:condition="not:view/translation_team" id="group-team-info">
-          There is no team to manage
-          <tal:product replace="structure
-            view/parent/fmt:link">Evolution</tal:product>
-          translations to
-          <a tal:attributes="href context/language/fmt:url"
-             tal:content="context/language/englishname">Serbian</a>.
-          To set one up, please get in touch with
-          <a tal:replace="structure view/translation_group/owner/fmt:link"
-             >Launchpad Translators</a>.
-        </p>
-      </tal:with_group>
-      <tal:without_group condition="not: view/translation_group">
-        <p id="group-team-info">
-          There is no translation group to manage
-          <tal:product replace="structure
-            view/parent/fmt:link">Evolution</tal:product>
-          translations.
-        </p>
-      </tal:without_group>
-      <p id="translation-access-level" 
-         condition="view/translation_group">
-        <span tal:replace="structure view/access_level_description">
+          <p tal:condition="not:view/translation_team">
+            There is no team to manage
+            <tal:product replace="structure
+              view/parent/fmt:link">Evolution</tal:product>
+            translations to
+            <a tal:attributes="href context/language/fmt:url"
+               tal:content="context/language/englishname">Serbian</a>.
+            To set one up, please get in touch with
+            <a tal:replace="structure view/translation_group/owner/fmt:link"
+               >Launchpad Translators</a>.
+          </p>
+        </tal:with_group>
+        <tal:without_group condition="not: view/translation_group">
+          <p>
+            There is no translation group to manage
+            <tal:product replace="structure
+              view/parent/fmt:link">Evolution</tal:product>
+            translations.
+          </p>
+        </tal:without_group>
+      </div>
+      <p id="translation-access-level">
+        <span 
+          tal:condition="view/translation_group"
+          tal:replace="structure view/access_level_description">
             You can add suggestions for these translations.
         </span>
         Templates which are more important to translate are listed first.

-- 
Adi Roiban

Revision history for this message

Guilherme Salgado (salgado) wrote on 2009-12-11:

Download full text (9.6 KiB)

Hi Adi,

Just a couple more suggestions, but these are really trivial. Once you
push the changes I'll submit your branch to ec2 to run the full test
suite.

Once again, thanks a lot for this improvement and the very nice cleanup!

review approve
status approved

On Fri, 2009-12-11 at 19:54 +0000, Adi Roiban wrote:
> În data de Vi, 11-12-2009 la 17:48 +0000, Guilherme Salgado a scris:

> > > +
> > > + translations_person = ITranslationsPerson(self.user)
> > > + translations_contact_link = None
> > > +
> > > + if self.translation_team:
> > > + translations_contact_link = PersonFormatterAPI(
> > > + self.translation_team.translator).link(None)
> > > + elif self.translation_group:
> > > + translations_contact_link = PersonFormatterAPI(
> > > + self.translation_group.owner).link(None)
> >
> > Is it possible that self.translation_team and self.translation_group are
> > both None, thus leaving translations_contact_link set to None as well?
> > I hope that's not possible as it'd cause the code below to crash, and in
> > that case we can make this assumption (that we expect at least one of
> > them to be non-None) clear in the code by adding an else block with an
> > AssertionError.
> >
> > else:
> > raise AssertionError("No translation team/group found.")
> >
> > And then you can also remove the "translations_contact_link = None"
> > line.
>
> It is possible and it should not be an error.
> In such case no information should be display.
>
> In a normal use case we should not reach this line, as that check is
> already done in the template.
>
> I added a check and returned "".

In this case I think the best is to document that in the method's
docstring (e.g. "This method must not be called if
self.translation_group is None.") and turn the if/elif into an if/else
with an assert, like this:

    if self.translation_team:
        ...
    else:
        assert self.translation_group is not None, (
            "Must not be called when there's no translation group.")
        ...

> > > +class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> > > + """View class to render translation status for an `IDistroSeries`."""
> > > +
> > > + def initialize(self):
> > > + series = self.context.distroseries
> > > + super(DistroSeriesLanguageView, self).initialize(
> > > + series=series,
> > > + translationgroup=series.distribution.translationgroup)
> > > + self.parent = self.series.distribution
> > > +
> > > +
> > > +class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
> > > + """View class to render translation status for an `IProductSeries`."""
> > > +
> > > + def initialize(self):
> > > + series = self.context.productseries
> > > + super(ProductSeriesLanguageView, self).initialize(
> > > + series=series,
> >
> > I'd drop the 'series = self.context....' line in both classes above and
> > just pass series=self.context... to super's initialize().
>
> I have leave that attribution to avoid a long line that will follow.
>
> translationgroup=self.context.pro...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Adi Roiban

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

James Troup

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

 === modified file 'lib/lp/translations/browser/configure.zcml'
 --- lib/lp/translations/browser/configure.zcml	2009-12-12 02:14:16 +0000
 +++ lib/lp/translations/browser/configure.zcml	2009-12-17 16:29:14 +0000
@@ -161,9 +161,8 @@
          attribute_to_parent="distroseries"
          rootsite="translations"/>
      <browser:navigation
--        module="lp.translations.browser.distroserieslanguage"
--        classes="
--            DistroSeriesLanguageNavigation"/>
++        module="lp.translations.browser.serieslanguage"
++        classes="DistroSeriesLanguageNavigation"/>
      <browser:defaultView
          for="lp.translations.interfaces.distroserieslanguage.IDistroSeriesLanguage"
          name="+index"
@@ -182,7 +181,7 @@
          permission="zope.Public"
          for="lp.translations.interfaces.distroserieslanguage.IDistroSeriesLanguage"
          template="../templates/serieslanguage-index.pt"
--        class="lp.translations.browser.distroserieslanguage.DistroSeriesLanguageView"
++        class="lp.translations.browser.serieslanguage.DistroSeriesLanguageView"
          facet="translations"
          layer="canonical.launchpad.layers.TranslationsLayer"/>
      <facet
@@ -272,7 +271,7 @@
          attribute_to_parent="productseries"
          rootsite="translations"/>
      <browser:navigation
--        module="lp.translations.browser.productserieslanguage"
++        module="lp.translations.browser.serieslanguage"
          classes="
              ProductSeriesLanguageNavigation"/>
      <browser:defaultView
@@ -292,7 +291,7 @@
          permission="zope.Public"
          for="lp.translations.interfaces.productserieslanguage.IProductSeriesLanguage"
          template="../templates/serieslanguage-index.pt"
--        class="lp.translations.browser.productserieslanguage.ProductSeriesLanguageView"
++        class="lp.translations.browser.serieslanguage.ProductSeriesLanguageView"
          facet="translations"
          layer="canonical.launchpad.layers.TranslationsLayer"/>
      <facet
 === removed file 'lib/lp/translations/browser/productserieslanguage.py'
 --- lib/lp/translations/browser/productserieslanguage.py	2009-09-17 12:45:52 +0000
 +++ lib/lp/translations/browser/productserieslanguage.py	1970-01-01 00:00:00 +0000
@@ -1,57 +0,0 @@
--# Copyright 2009 Canonical Ltd.  This software is licensed under the
--# GNU Affero General Public License version 3 (see the file LICENSE).
--
--"""Browser code for Product Series Languages."""
--
--__metaclass__ = type
--
--__all__ = [
--    'ProductSeriesLanguageNavigation',
--    'ProductSeriesLanguageView',
--    ]
--
--from canonical.cachedproperty import cachedproperty
--from canonical.launchpad.webapp import LaunchpadView
--from canonical.launchpad.webapp.batching import BatchNavigator
--from canonical.launchpad.webapp.publisher import Navigation
--from lp.translations.interfaces.productserieslanguage import (
--    IProductSeriesLanguage)
--
--
--class ProductSeriesLanguageView(LaunchpadView):
--    """View class to render translation status for an `IProductSeries`."""
--
--    pofiles = None
--    label = "Translatable templates"
--
--    def initialize(self):
--        self.form = self.request.form
--
--        self.batchnav = BatchNavigator(
--            self.context.productseries.getCurrentTranslationTemplates(),
--            self.request)
--
--        self.context.recalculateCounts()
--
--        self.pofiles = self.context.getPOFilesFor(
--            self.batchnav.currentBatch())
--        self.parent = self.context.productseries.product
--
--    @cachedproperty
--    def translation_group(self):
--        return self.context.productseries.product.translationgroup
--
--    @cachedproperty
--    def translation_team(self):
--        """Is there a translation team for this translation."""
--        if self.translation_group is not None:
--            team = self.translation_group.query_translator(
--                self.context.language)
--        else:
--            team = None
--        return team
--
--
--class ProductSeriesLanguageNavigation(Navigation):
--    """Navigation for `IProductSeriesLanguage`."""
--    usedfor = IProductSeriesLanguage
 === renamed file 'lib/lp/translations/browser/distroserieslanguage.py' => 'lib/lp/translations/browser/serieslanguage.py'
 --- lib/lp/translations/browser/distroserieslanguage.py	2009-09-17 11:10:49 +0000
 +++ lib/lp/translations/browser/serieslanguage.py	2009-12-17 16:29:14 +0000
@@ -8,38 +8,64 @@
  __all__ = [
      'DistroSeriesLanguageNavigation',
      'DistroSeriesLanguageView',
++    'ProductSeriesLanguageNavigation',
++    'ProductSeriesLanguageView',
+     ]
++from canonical.cachedproperty import cachedproperty
  from canonical.launchpad.webapp import LaunchpadView
++from canonical.launchpad.webapp.tales import PersonFormatterAPI
  from canonical.launchpad.webapp.batching import BatchNavigator
  from canonical.launchpad.webapp.publisher import Navigation
  from lp.translations.interfaces.distroserieslanguage import (
      IDistroSeriesLanguage)
--
--class DistroSeriesLanguageView(LaunchpadView):
--    """View class to render translation status for an `IDistroSeries`."""
++from lp.translations.interfaces.translationsperson import (
++    ITranslationsPerson)
++from lp.translations.interfaces.translationgroup import (
++    TranslationPermission)
++from lp.translations.interfaces.productserieslanguage import (
++    IProductSeriesLanguage)
++
++
++class BaseSeriesLanguageView(LaunchpadView):
++    """View base class to render translation status for an
++    `IDistroSeries` and `IProductSeries`
++
++    This class should not be directly instantiated.
++    """
      pofiles = None
      label = "Translatable templates"
++    series = None
++    parent = None
++    translationgroup = None
--    def initialize(self):
++    def initialize(self, series, translationgroup):
++        self.series = series
++        self.translationgroup = translationgroup
          self.form = self.request.form
          self.batchnav = BatchNavigator(
--            self.context.distroseries.getCurrentTranslationTemplates(),
++            self.series.getCurrentTranslationTemplates(),
              self.request)
          self.pofiles = self.context.getPOFilesFor(
              self.batchnav.currentBatch())
--        self.parent = self.context.distroseries.distribution
      @property
      def translation_group(self):
--        return self.context.distroseries.distribution.translationgroup
--
--    @property
++        """Return the translation group for these translations.
++
++        Return None if there's no translation group for them.
++        """
++        return self.translationgroup
++
++    @cachedproperty
      def translation_team(self):
--        """Is there a translation team for this translation."""
++        """Return the translation team for these translations.
++
++        Return None if there's no translation team for them.
++        """
          if self.translation_group is not None:
              team = self.translation_group.query_translator(
                  self.context.language)
@@ -47,7 +73,95 @@
              team = None
          return team
++    @property
++    def access_level_description(self):
++        """Must not be called when there's no translation group."""
++
++        if self.user is None:
++            return ("You are not logged in. Please log in to work "
++                    "on translations.")
++
++        translations_person = ITranslationsPerson(self.user)
++        translations_contact_link = None
++
++        if self.translation_team:
++            translations_contact_link = PersonFormatterAPI(
++                self.translation_team.translator).link(None)
++        elif self.translation_group:
++            translations_contact_link = PersonFormatterAPI(
++                self.translation_group.owner).link(None)
++        else:
++            assert self.translation_group is not None, (
++                "Must not be called when there's no translation group.")
++
++        if not translations_person.translations_relicensing_agreement:
++            translation_license_url = PersonFormatterAPI(
++                self.user).url(
++                    view_name='+licensing',
++                    rootsite='translations')
++            return ("To make translations in Launchpad you need to "
++                    "agree with the "
++                    "<a href='%s'>Translations licensing</a>.") % (
++                        translation_license_url)
++
++        sample_pofile = self.pofiles[0]
++        if sample_pofile is not None:
++            if sample_pofile.canEditTranslations(self.user):
++                return "You can add and review translations."
++
++            if sample_pofile.canAddSuggestions(self.user):
++                return ("Your suggestions will be held for review by "
++                        "%s. If you need help, or your translations are "
++                        "not being reviewed, please get in touch with "
++                        "%s.") % (
++                            translations_contact_link,
++                            translations_contact_link)
++
++            permission = sample_pofile.translationpermission
++            if permission == TranslationPermission.CLOSED:
++                return ("These templates can be translated only by "
++                        "their managers.")
++
++        if self.translation_team is None:
++            return ("Since there is nobody to manage translation "
++                    "approvals into this language, your cannot add "
++                    "new suggestions. If you are interested in making "
++                    "translations, please contact %s.") % (
++                        translations_contact_link)
++
++        raise AssertionError(
++            "BUG! Couldn't identify the user's access level for these "
++            "translations.")
++
++
++class DistroSeriesLanguageView(BaseSeriesLanguageView):
++    """View class to render translation status for an `IDistroSeries`."""
++
++    def initialize(self):
++        series = self.context.distroseries
++        super(DistroSeriesLanguageView, self).initialize(
++            series=series,
++            translationgroup=series.distribution.translationgroup)
++        self.parent = self.series.distribution
++
++
++class ProductSeriesLanguageView(BaseSeriesLanguageView):
++    """View class to render translation status for an `IProductSeries`."""
++
++    def initialize(self):
++        series = self.context.productseries
++        super(ProductSeriesLanguageView, self).initialize(
++            series=series,
++            translationgroup=series.product.translationgroup)
++        self.context.recalculateCounts()
++        self.parent = self.series.product
++
  class DistroSeriesLanguageNavigation(Navigation):
      """Navigation for `IDistroSeriesLanguage`."""
      usedfor = IDistroSeriesLanguage
++
++
++class ProductSeriesLanguageNavigation(Navigation):
++    """Navigation for `IProductSeriesLanguage`."""
++    usedfor = IProductSeriesLanguage
 === modified file 'lib/lp/translations/browser/tests/test_distroserieslanguage_views.py'
 --- lib/lp/translations/browser/tests/test_distroserieslanguage_views.py	2009-09-11 06:57:21 +0000
 +++ lib/lp/translations/browser/tests/test_distroserieslanguage_views.py	2009-12-17 16:29:14 +0000
@@ -8,7 +8,7 @@
  from zope.component import getUtility
--from lp.translations.browser.distroserieslanguage import (
++from lp.translations.browser.serieslanguage import (
      DistroSeriesLanguageView)
  from lp.translations.interfaces.translator import ITranslatorSet
  from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -48,6 +48,9 @@
          group = self.factory.makeTranslationGroup(
              self.distroseries.distribution.owner, url=None)
          self.distroseries.distribution.translationgroup = group
++        self.view = DistroSeriesLanguageView(
++            self.dsl, LaunchpadTestRequest())
++        self.view.initialize()
          self.assertEquals(self.view.translation_group, group)
      def test_translation_team(self):
@@ -66,6 +69,7 @@
          # Recreate the view because we are using a cached property.
          self.view = DistroSeriesLanguageView(
              self.dsl, LaunchpadTestRequest())
++        self.view.initialize()
          self.assertEquals(self.view.translation_team, translator)
  def test_suite():
 === modified file 'lib/lp/translations/browser/tests/test_productserieslanguage_views.py'
 --- lib/lp/translations/browser/tests/test_productserieslanguage_views.py	2009-07-17 00:26:05 +0000
 +++ lib/lp/translations/browser/tests/test_productserieslanguage_views.py	2009-12-17 16:29:14 +0000
@@ -7,7 +7,7 @@
  from zope.component import getUtility
--from lp.translations.browser.productserieslanguage import (
++from lp.translations.browser.serieslanguage import (
      ProductSeriesLanguageView)
  from lp.translations.interfaces.translator import ITranslatorSet
  from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -137,6 +137,7 @@
          group = self.factory.makeTranslationGroup(
              self.productseries.product.owner, url=None)
          self.productseries.product.translationgroup = group
++        self.view.initialize()
          self.assertEquals(self.view.translation_group, group)
      def test_translation_team(self):
@@ -155,6 +156,7 @@
          # Recreate the view because we are using a cached property.
          self.view = ProductSeriesLanguageView(
              self.psl, LaunchpadTestRequest())
++        self.view.initialize()
          self.assertEquals(self.view.translation_team, translator)
  def test_suite():
 === renamed file 'lib/lp/translations/stories/productseries/xx-productserieslanguage.txt' => 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt'
 --- lib/lp/translations/stories/productseries/xx-productserieslanguage.txt	2009-09-12 07:25:21 +0000
 +++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt	2009-12-17 16:29:14 +0000
@@ -1,4 +1,9 @@
--= Product release series language overview =
++Product or distribution release series language overview
++========================================================
++
++These pages are used by translators for accessing all templates in a
++release series and viewing translation statistics for each template for
++a specific language
  The Translations page for a product release series with multiple
  templates links to per-language overviews.
@@ -11,4 +16,187 @@
      >>> print browser.title
      Portuguese (Brazil) (pt_BR) : Translations : Series trunk : Evolution
++Since there is no translation team to manage Portuguese (Brazil) language
++in the Evolution's translation group, all users will be informed about it
++and pointed to the translation group owner.
++
++    >>> print extract_text(find_tag_by_id(
++    ...     browser.contents, 'group-team-info'))
++    There is no team to manage Evolution ... translations to ...
++    To set one up, please get in touch with Carlos Perelló Marín.
++
++Anonymous users are informed that in order to make translations they
++need to login first.
++
++    >>> print extract_text(
++    ...     find_tag_by_id(browser.contents, 'translation-access-level'))
++    You are not logged in. Please log in to work on translations...
++
++Authenticated users will see information about what they can do in
++these translations. Things like review, only add suggestion or no
++changes at all.
++
++If a product or distribution had no translation group, visitors are
++informed about this fact and will be able to add translations without
++requiring a review.
++
++    >>> user_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/es')
++    >>> print extract_text(
++    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
++    There is no translation group to manage Ubuntu translations.
++
++Create a translation group for Ubuntu, together with a translation
++person for managing Ubuntu Spanish translations and set translation
++policy to RESTRICTED.
++This is done to so see what the page will look like when they exist.
++
++    >>> from zope.component import getUtility
++    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
++    >>> from lp.translations.interfaces.translationgroup import (
++    ...     TranslationPermission)
++    >>> login('foo.bar@canonical.com')
++    >>> utc_owner = factory.makePerson(displayname='Some Guy')
++    >>> utc_team = factory.makeTeam(
++    ...     owner=utc_owner, name='utc-team',
++    ...     displayname='Ubuntu Translation Coordinators')
++    >>> utg = factory.makeTranslationGroup(
++    ...     owner=utc_team, name='utg', title='Ubuntu Translation Group')
++    >>> st_coordinator = factory.makePerson(
++    ...     name="ubuntu-l10n-es",
++    ...     displayname='Ubuntu Spanish Translators')
++    >>> dude = factory.makePerson(
++    ...     name="dude", email="dude@ex.com", password="test")
++    >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
++    >>> ubuntu.translationgroup = utg
++    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
++    >>> translators = factory.makeTranslator(
++    ...     'es', group=utg, person=st_coordinator)
++    >>> no_license_translator = factory.makeTranslator(
++    ...     'es', person=dude, license=False)
++    >>> logout()
++
++Spanish has a translation team for managing its translations and all
++Evolution Spanish templates can be accessed from the distribution series
++translation page.
++
++    >>> user_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
++    >>> user_browser.getLink('Spanish').click()
++    >>> print user_browser.url
++    http://translations.launchpad.dev/ubuntu/hoary/+lang/es
++
++    >>> print extract_text(
++    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
++    These Ubuntu translations are managed by Ubuntu Spanish Translators.
++
++Authenticated users can add suggestion but will be held for review by
++the members of Spanish translations team.
++
++    >>> print extract_text(
++    ...     find_tag_by_id(
++    ...         user_browser.contents, 'translation-access-level'))
++    Your suggestions will be held for review by
++    Ubuntu Spanish Translator...
++    please get in touch with Ubuntu Spanish Translators...
++
++Users will see three references to the team managing these translations.
++
++    >>> print user_browser.getLink(
++    ...     'Ubuntu Spanish Translator').url
++    http://launchpad.dev/~ubuntu-l10n-es
++
++Catalan has no translation team for managing translations and since
++there is no one to review the work, authenticated users can not add
++suggestions.
++
++    >>> user_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/')
++    >>> user_browser.getLink('Catalan').click()
++    >>> print user_browser.url
++    http://translations.launchpad.dev/ubuntu/hoary/+lang/ca
++
++    >>> print extract_text(
++    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
++    There is no team to manage ... To set one up, please get in touch
++    with Ubuntu Translation Coordinators.
++
++    >>> print extract_text(find_tag_by_id(
++    ...     user_browser.contents, 'translation-access-level'))
++    Since there is nobody to manage translation ...
++    your cannot add new suggestions. If you are interested in making
++    translations, please contact Ubuntu Translation Coordinators...
++
++    >>> print user_browser.getLink(
++    ...     'Ubuntu Translation Coordinators').url
++    http://launchpad.dev/~utc-team
++
++Members of translation team and translations admins have full access to
++translations. They can add and review translations.
++
++    >>> admin_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
++    >>> print extract_text(find_tag_by_id(
++    ...     admin_browser.contents, 'translation-access-level'))
++    You can add and review translations...
++
++For projects using closed translations policy, a translator that is not
++member of the translation team appointed for that language will not
++be allowed to make any changes.
++
++    >>> login('foo.bar@canonical.com')
++    >>> ubuntu.translationpermission = TranslationPermission.CLOSED
++    >>> logout()
++
++    >>> user_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
++    >>> print extract_text(find_tag_by_id(
++    ...     user_browser.contents, 'translation-access-level'))
++    These templates can be translated only by their managers...
++
++Translation policy is rolled back to not affect other tests.
++
++    >>> login('foo.bar@canonical.com')
++    >>> ubuntu.translationpermission = TranslationPermission.RESTRICTED
++    >>> logout()
++
++Translators that have not agreed with the license can not make
++translations, and will see a link to the license page.
++
++    >>> no_license_browser = setupBrowser(
++    ...     auth='Basic dude@ex.com:test')
++    >>> no_license_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
++    >>> print extract_text(find_tag_by_id(
++    ...     no_license_browser.contents, 'translation-access-level'))
++    To make translations in Launchpad you need to agree with
++    the Translations licensing...
++
++    >>> print no_license_browser.getLink(
++    ...     'Translations licensing').url
++    http://translations.launchpad.dev/~dude/+licensing
++
++For project with no translation group, translators see a note stating
++this fact. No access level information is displayed.
++
++    >>> login('foo.bar@canonical.com')
++    >>> ubuntu.translationgroup = None
++    >>> logout()
++
++    >>> user_browser.open(
++    ...     'http://translations.launchpad.dev/ubuntu/hoary/+lang/ro')
++    >>> print extract_text(
++    ...     find_tag_by_id(user_browser.contents, 'group-team-info'))
++    There is no translation group to manage Ubuntu translations.
++
++    >>> print extract_text(find_tag_by_id(
++    ...     user_browser.contents, 'translation-access-level'))
++    Templates which are more important to translate are listed first.
++
++Translation group configuration is rolled back to not affect other tests.
++
++    >>> login('foo.bar@canonical.com')
++    >>> ubuntu.translationgroup = utg
++    >>> logout()
++
  The details of the page are tested at the view level.
 === modified file 'lib/lp/translations/templates/serieslanguage-index.pt'
 --- lib/lp/translations/templates/serieslanguage-index.pt	2009-09-17 11:10:49 +0000
 +++ lib/lp/translations/templates/serieslanguage-index.pt	2009-12-17 16:29:14 +0000
@@ -16,33 +16,45 @@
    <tal:block condition="view/pofiles">
      <div class="top-portlet">
--      <tal:with_group condition="view/translation_group">
--        <p tal:condition="view/translation_team">
--          <tal:product replace="structure
--            view/parent/fmt:link">Evolution</tal:product>
--          is translated by
--          <tal:team replace="structure
--                             view/translation_team/translator/fmt:link">
--            Serbian translators</tal:team> — if you need help, or your
--          translations are not being reviewed, please get in touch with them.
--        </p>
--
--        <p tal:condition="not:view/translation_team">
--          There is no team to translate
--          <tal:product replace="structure
--            view/parent/fmt:link">Evolution</tal:product>
--          to
--          <a tal:attributes="href context/language/fmt:url"
--             tal:content="context/language/englishname">Serbian</a>.
--          To set one up, please get in touch with
--          <a tal:replace="structure view/translation_group/fmt:link"
--             >Launchpad Translators</a>
--          coordinator.
--        </p>
--      </tal:with_group>
--
--      <p>
--        Choose a template name to begin translating.
++      <div id="group-team-info">
++        <tal:with_group condition="view/translation_group">
++          <p tal:condition="view/translation_team">
++            These
++            <tal:product replace="structure
++              view/parent/fmt:link">Evolution</tal:product>
++            translations are managed by
++            <tal:team replace="structure
++                               view/translation_team/translator/fmt:link">
++              Serbian translators</tal:team>.
++          </p>
++
++          <p tal:condition="not:view/translation_team">
++            There is no team to manage
++            <tal:product replace="structure
++              view/parent/fmt:link">Evolution</tal:product>
++            translations to
++            <a tal:attributes="href context/language/fmt:url"
++               tal:content="context/language/englishname">Serbian</a>.
++            To set one up, please get in touch with
++            <a tal:replace="structure view/translation_group/owner/fmt:link"
++               >Launchpad Translators</a>.
++          </p>
++        </tal:with_group>
++        <tal:without_group condition="not: view/translation_group">
++          <p>
++            There is no translation group to manage
++            <tal:product replace="structure
++              view/parent/fmt:link">Evolution</tal:product>
++            translations.
++          </p>
++        </tal:without_group>
++      </div>
++      <p id="translation-access-level">
++        <span
++          tal:condition="view/translation_group"
++          tal:replace="structure view/access_level_description">
++            You can add suggestions for these translations.
++        </span>
          Templates which are more important to translate are listed first.
        </p>
      </div>

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers