Merge lp:~adiroiban/launchpad/bug-462013-try-2 into lp:launchpad

= Bug 462013 =
On a project's main translation page, there is a little box ("Set up translations for other series") that suggests I import templates for any series in my project that isn't yet imported. But it shouldn't bother listing obsolete series.

== Proposed fix ==
Don't include series with status OBSOLETE in the prompt list of series that can be set up for translations

== Pre-implementation notes ==

This was initially branch was submitted together with DistroSeries to Series refactoring (bug 496352).
Danilo took a look and decided to split the branch in refactoring and this new branch.

== Implementation details ==

The test from xx-productseries-translations.txt , line 150 was setting the status for 'evolution' but then the test were done for 'bazaar'.
I've changed the code to set the status bazaar.

The series are listed in random order. This is why I used `u'serie name' in extracted_text`.

== Tests ==
lp-test -t productseries-translations

== Demo and Q/A ==

In a product, create 2 new series, one in developlment and the other obsolete.
Go to product translation page. ie: http://translations.launchpad.dev/evolution

The obsolete series should not appear in the 'Set up translations for a series' list.

= 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/product.py
  lib/lp/translations/stories/productseries/xx-productseries-translations.txt

<noodles775> adiroiban: I can take a look at that branch, or did you want someone specific to look at it?
<adiroiban> noodles775: hi. nope. you can review it
<noodles775> adiroiban: hi, your new test seems to pass even when I revert the change to product.py?
<noodles775> (BTW: thanks for adding extra tests too!)
<adiroiban> noodles775: uh... yes. I forget to test „obsolete is _not_ in extracted_text”
<adiroiban> noodles775: I pushed the missing test
<noodles775> adiroiban: thanks... I'm actually looking at the pagetest, and thinking that most of it is really testing the *view* functionality and should really be in a view/browser test...
<noodles775> I mean, you're really testing the untranslatable_series property... what do you think?
<adiroiban> noodles775: yes. I will move them in browser/tests
<noodles775> adiroiban: So perhaps just add one series in the pagetest and verify that it appears, then yes, the rest can go in the view test.
<adiroiban> noodles775: or 2...one active and one obsolete
<noodles775> adiroiban: also, I (personally) think it would be worth sorting the results of untranslatable_series - random order is not good. What do you think?
<noodles775> adiroiban: well, the fact that the obsolete series won't be included should be in the view test (as it's view code).
<adiroiban> noodles775: ok. alphabetically sorted?
<noodles775> adiroiban: if that make the most sense, yes (another option would be by the status, but you'll know more about which makes most sense :)
<noodles775> adiroiban: So I'll make this as needs-fixing for the moment. Just ping me when you're done and I'll go through the code in detail. Thanks!
<adiroiban> noodles775: ok. thanks

Hi Michael,

I have moved the full test to unit testing layer and listed the series sorted alphabetically.

When you have time, please take a look at the latest diff. No hurry. Thanks!

=== modified file 'lib/lp/translations/browser/product.py'
--- lib/lp/translations/browser/product.py 2010-01-28 09:11:58 +0000
+++ lib/lp/translations/browser/product.py 2010-01-28 14:21:11 +0000
@@ -85,7 +85,7 @@
     @cachedproperty
     def uses_translations(self):
         """Whether this product has translatable templates."""
- return (self.context.official_rosetta and
+ return (self.context.official_rosetta and
                 self.primary_translatable is not None)

     @cachedproperty
@@ -114,13 +114,15 @@
     @cachedproperty
     def untranslatable_series(self):
         """Return series which are not yet set up for translations.
-
- Obsolete series will be excluded from this list.
+
+ The list is sorted in alphabetically order and obsolete series
+ will be excluded.
         """

         all_active_series = set(
             [serie for serie in self.context.series if (
                 serie.status != SeriesStatus.OBSOLETE)])
         translatable = set(self.context.translatable_series)
- return all_active_series - translatable
-
+ return sorted(
+ all_active_series - translatable,
+ key=lambda series: series.name)

=== modified file 'lib/lp/translations/browser/tests/test_product_view.py'
--- lib/lp/translations/browser/tests/test_product_view.py 2010-01-16 08:45:58 +0000
+++ lib/lp/translations/browser/tests/test_product_view.py 2010-01-28 14:20:51 +0000
@@ -9,6 +9,7 @@
 from canonical.testing import LaunchpadZopelessLayer
 from lp.translations.browser.product import ProductView
 from lp.testing import TestCaseWithFactory
+from lp.registry.interfaces.series import SeriesStatus

 class TestProduct(TestCaseWithFactory):
@@ -17,27 +18,82 @@
     layer = LaunchpadZopelessLayer

     def setUp(self):
- # Create a product that uses translations.
         TestCaseWithFactory.setUp(self)
- self.product = self.factory.makeProduct()
- self.series = self.product.development_focus
- self.product.official_rosetta = True
- self.view = ProductView(self.product,
- LaunchpadTestRequest())

     def test_primary_translatable_with_package_link(self):
+ # Create a product that uses translations.
+ product = self.factory.makeProduct()
+ series = product.development_focus
+ product.official_rosetta = True
+ view = ProductView(product, LaunchpadTestRequest())
+
         # If development focus series is linked to
         # a distribution package with translations,
         # we do not try to show translation statistics
         # for the package.
         sourcepackage = self.factory.makeSourcePackage()
- sourcepackage.setPackaging(self.series, None)
+ sourcepackage.setPackaging(series, None)
         sourcepackage.distroseries.distribution.official_rosetta = True
         pot = self.factory.makePOTemplate(
             distroseries=sourcepackage.distroseries,
       ...

Hi Adi,

Thanks for moving those tests to a unit-test, you'll make a lot of people happy :) (We're trying to keep documentation nice and readable, with all non-documentation-worthy tests moved to unit tests).

I think you've got everything in the right place, but I do have a few suggestions below for simplifying the implementation and test a bit. I'll mark this as approved, but I'm keen to see which of the suggestions below you agree or disagree with.

Cheers,
Michael

> === modified file 'lib/lp/translations/browser/product.py'
> --- lib/lp/translations/browser/product.py 2010-01-22 18:10:53 +0000
> +++ lib/lp/translations/browser/product.py 2010-01-28 15:26:22 +0000
> @@ -18,6 +18,7 @@
> from canonical.launchpad.webapp.menu import NavigationMenu
> from lp.registry.interfaces.product import IProduct
> from lp.registry.interfaces.productseries import IProductSeries
> +from lp.registry.interfaces.series import SeriesStatus
> from lp.registry.browser.product import ProductEditView
> from lp.translations.browser.translations import TranslationsMixin
>
> @@ -84,7 +85,7 @@
> @cachedproperty
> def uses_translations(self):
> """Whether this product has translatable templates."""
> - return (self.context.official_rosetta and
> + return (self.context.official_rosetta and

Thanks for the drive-by!

> self.primary_translatable is not None)
>
> @cachedproperty
> @@ -112,7 +113,16 @@
>
> @cachedproperty
> def untranslatable_series(self):
> - """Return series which are not yet set up for translations."""
> - all_series = set(self.context.series)
> + """Return series which are not yet set up for translations.
> +
> + The list is sorted in alphabetically order and obsolete series
> + will be excluded.

s/will be/are ?

> + """
> +
> + all_active_series = set(
> + [serie for serie in self.context.series if (
> + serie.status != SeriesStatus.OBSOLETE)])

It's pretty confusing, but grepping through the code a bit shows that
we seem to have a precedent of:

series for series in self.context.series

Take a look and see what you think.

> translatable = set(self.context.translatable_series)
> - return all_series - translatable
> + return sorted(
> + all_active_series - translatable,
> + key=lambda series: series.name)

We're meant to avoid using lambda (search for it in
https://dev.launchpad.net/PythonStyleGuide). The guide has an alternative
using attrgetter for your specific use-case here.

But that said, I wonder if this could be simpler, given that IProduct.series
already orders by name? I've just tested the following which also works:

    def untranslatable_series(self):
 """..."""
        translatable = self.context.translatable_series
        return [series for series in self.context.series if (
            series.status != SeriesStatus.OBSOLETE and
            series not in translatable)]

Let me know if there was a specific reason for converting to sets - given
that we're iterating anyway for the list comprehension, I'm not sure
there's a benefit in using the...

Hey,

Thanks for the review.
Have no mercy reviewing my branches. It is your project and you should not accept bad code.
Don't worry, I will not take offence.

Here is the latest diff.

=== modified file 'lib/lp/translations/browser/product.py'
--- lib/lp/translations/browser/product.py 2010-01-28 14:23:10 +0000
+++ lib/lp/translations/browser/product.py 2010-01-29 16:08:54 +0000
@@ -116,13 +116,10 @@
         """Return series which are not yet set up for translations.

         The list is sorted in alphabetically order and obsolete series
- will be excluded.
+ are excluded.
         """

- all_active_series = set(
- [serie for serie in self.context.series if (
- serie.status != SeriesStatus.OBSOLETE)])
- translatable = set(self.context.translatable_series)
- return sorted(
- all_active_series - translatable,
- key=lambda series: series.name)
+ translatable = self.context.translatable_series
+ return [series for series in self.context.series if (
+ series.status != SeriesStatus.OBSOLETE and
+ series not in translatable)]

=== modified file 'lib/lp/translations/browser/tests/test_product_view.py'
--- lib/lp/translations/browser/tests/test_product_view.py 2010-01-28 14:23:10 +0000
+++ lib/lp/translations/browser/tests/test_product_view.py 2010-01-29 16:11:36 +0000
@@ -7,9 +7,9 @@

 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing import LaunchpadZopelessLayer
+from lp.registry.interfaces.series import SeriesStatus
 from lp.translations.browser.product import ProductView
 from lp.testing import TestCaseWithFactory
-from lp.registry.interfaces.series import SeriesStatus

 class TestProduct(TestCaseWithFactory):
@@ -17,9 +17,6 @@

     layer = LaunchpadZopelessLayer

- def setUp(self):
- TestCaseWithFactory.setUp(self)
-
     def test_primary_translatable_with_package_link(self):
         # Create a product that uses translations.
         product = self.factory.makeProduct()
@@ -75,24 +72,17 @@
             product=product, name='evo-future')
         series_future.status = SeriesStatus.FUTURE

- # 'untranslatable_series' is a cached property, this is why we
- # check it after adding all series.
- self.assertTrue(series_experimental in view.untranslatable_series)
- self.assertTrue(series_development in view.untranslatable_series)
- self.assertTrue(series_frozen in view.untranslatable_series)
- self.assertTrue(series_current in view.untranslatable_series)
- self.assertTrue(series_supported in view.untranslatable_series)
- self.assertTrue(series_future in view.untranslatable_series)
-
- # Obsolete series are not included
- self.assertFalse(series_obsolete in view.untranslatable_series)
-
- # Series are listed in alphabetical order, 'evo-current'
- # is firsts, while 'trunk' is last.
- self.assertTrue(
- series_current == view.untranslatable_series[0])
- self.assertTrue(
- series_trunk == view.untranslatable_series[-1])
+ # The series are return...

Thanks Adi. Looks good. Still r=me.

Thanks.
Do you have time to land this branch or should I ask someone else?

Cheers,
Adi

On Tue, Feb 2, 2010 at 4:35 PM, Adi Roiban <email address hidden> wrote:
> Thanks.
> Do you have time to land this branch or should I ask someone else?

Sure. It's on ec2 to land now :)