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

> +                    "<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('<email address hidden>')
> +    >>> 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>