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

= 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

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

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

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

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.

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.

Î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:
> > + ...

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

În data de Vi, 11-12-2009 la 20:18 +0000, Guilherme Salgado a scris:
> Review: Approve
> 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
[snip]
> 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.")
> ...
OK. Done.

> This check can go away once the if/elif is converted into an if/else
> with an assertion. The latter is preferred because it will fail
> horribly (instead of silently, like it currently does) when the property
> is used incorrectly (i.e. when translation_group is None).
Yep.

> > +
> > 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')
>
> You might not need rootsite='translations' (as this page is already on
> the translations rootsite), but I might be wrong.
I've checked the implementation for PersonFormatterAPI.url, and as far
I can understand that code, the rootsite is not automatically detected.

rootsite is required... as +licensing is not valid for main.

Also tested without rootsite and it was not created for translations.

> > +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
>
> Is it really important to show that we have 3 links to the team managing
> the translations?
Fixed.

I added new tests for checking when a translator has not agreed the
translation license and when translations are CLOSED.

Merged with devel since I modified the Factory in a branch that was just
commited and those changes also affects these tests.

Cheers.

Here is the diff.

=== modified file 'lib/lp/translations/browser/serieslanguage.py'
--- lib/lp/translations/browser/serieslanguage.py 2009-12-11 20:11:06
+0000
+++ lib/lp/translations/browser/serieslanguage.py 2009-12-12 06:19:24
+0000
@@ -75,6 +75,8 @@

     @property
     def access_level_description(self):
+...

Hi Salgado,

The failing test is /home/adi/launchpad/lp-branches/bug-427319/lib/lp/translations/browser/tests/test_distroserieslanguage_views.py, line 51, in test_translation_group

    def test_translation_group(self):
        group = self.factory.makeTranslationGroup(
            self.distroseries.distribution.owner, url=None)
        self.distroseries.distribution.translationgroup = group
        self.assertEquals(self.view.translation_group, group)

I tried to change it to the following code... but with no luck

    def test_translation_group(self):
        group = self.factory.makeTranslationGroup(
            self.distroseries.distribution.owner, url=None)
        self.distroseries.distribution.translationgroup = group
        self.view = DistroSeriesLanguageView(
            self.dsl, LaunchpadTestRequest())
        self.assertEquals(self.view.translation_group, group)

--------------------
This is a fix for the view, but maybe I have to fix the test.

=== modified file 'lib/lp/translations/browser/serieslanguage.py'
--- lib/lp/translations/browser/serieslanguage.py 2009-12-12 07:16:07 +0000
+++ lib/lp/translations/browser/serieslanguage.py 2009-12-15 02:25:54 +0000
@@ -38,11 +38,9 @@
     label = "Translatable templates"
     series = None
     parent = None
- translationgroup = None

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

         self.batchnav = BatchNavigator(
@@ -58,7 +56,10 @@

         Return None if there's no translation group for them.
         """
- return self.translationgroup
+ if (self.context.distroseries):
+ return self.context.distroseries.distribution.translationgroup
+ else:
+ return self.context.productseries.product.translationgroup

     @cachedproperty
     def translation_team(self):
@@ -134,25 +135,21 @@
             "translations.")

-class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
+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)
+ series=self.context.distroseries)
         self.parent = self.series.distribution

-class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView):
+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)
+ series=self.context.productseries)
         self.context.recalculateCounts()
         self.parent = self.series.product

I start a full test.

Here is the diff for the tests.

=== modified file 'lib/lp/translations/browser/tests/test_distroserieslanguage_views.py'
--- lib/lp/translations/browser/tests/test_distroserieslanguage_views.py 2009-12-11 01:03:25 +0000
+++ lib/lp/translations/browser/tests/test_distroserieslanguage_views.py 2009-12-17 15:05:44 +0000
@@ -48,6 +48,8 @@
         group = self.factory.makeTranslationGroup(
             self.distroseries.distribution.owner, url=None)
         self.distroseries.distribution.translationgroup = group
+ self.view = DistroSeriesLanguageView(
+ self.dsl, LaunchpadTestRequest()).initialize()
         self.assertEquals(self.view.translation_group, group)

     def test_translation_team(self):
@@ -65,7 +67,7 @@
             group, self.language, team)
         # Recreate the view because we are using a cached property.
         self.view = DistroSeriesLanguageView(
- self.dsl, LaunchpadTestRequest())
+ self.dsl, LaunchpadTestRequest()).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-12-11 16:09:04 +0000
+++ lib/lp/translations/browser/tests/test_productserieslanguage_views.py 2009-12-17 15:07:42 +0000
@@ -137,6 +137,8 @@
         group = self.factory.makeTranslationGroup(
             self.productseries.product.owner, url=None)
         self.productseries.product.translationgroup = group
+ self.view = ProductSeriesLanguageView(
+ self.psl, LaunchpadTestRequest()).initialize()
         self.assertEquals(self.view.translation_group, group)

     def test_translation_team(self):
@@ -154,7 +156,7 @@
             group, self.language, team)
         # Recreate the view because we are using a cached property.
         self.view = ProductSeriesLanguageView(
- self.psl, LaunchpadTestRequest())
+ self.psl, LaunchpadTestRequest()).initialize()
         self.assertEquals(self.view.translation_team, translator)

 def test_suite():

I did a full test using the latest branch and everything is OK now. We will see the truth on Monday :)