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
> 

> === 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 01:30:49 +0000
> @@ -1,4 +1,9 @@
> -= Product release series language overview =
> +Product of distribution release series language overview

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('<email address hidden>')
> +    >>> utc_member = factory.makePerson(<email address hidden>', 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 address hidden>',
> +    ...                 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 <email address hidden>