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 set difference?

> 
> === 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 15:26:22 +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

Can you pop that one back a few lines so the import lines are alphabetical?

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

Given that you've removed all the specialisation from this inherited setUp,
you can just delete it.

>  
>      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,
>              sourcepackagename=sourcepackage.sourcepackagename)
> -        self.assertEquals(None, self.view.primary_translatable)
> +        self.assertEquals(None, view.primary_translatable)
> +
> +    def test_untranslatable_series(self):
> +        # Create a product that uses translations.
> +        product = self.factory.makeProduct()
> +        series_trunk = product.development_focus
> +        product.official_rosetta = True
> +        view = ProductView(product, LaunchpadTestRequest())
> +
> +        # New series are added, one for each type of status
> +        series_experimental = self.factory.makeProductSeries(
> +            product=product, name='evo-experimental')
> +        series_experimental.status = SeriesStatus.EXPERIMENTAL
> +
> +        series_development = self.factory.makeProductSeries(
> +            product=product, name='evo-development')
> +        series_development.status = SeriesStatus.DEVELOPMENT
> +
> +        series_frozen = self.factory.makeProductSeries(
> +            product=product, name='evo-frozen')
> +        series_frozen.status = SeriesStatus.FROZEN
> +
> +        series_current = self.factory.makeProductSeries(
> +            product=product, name='evo-current')
> +        series_current.status = SeriesStatus.CURRENT
> +
> +        series_supported = self.factory.makeProductSeries(
> +            product=product, name='evo-supported')
> +        series_supported.status = SeriesStatus.SUPPORTED
> +
> +        series_obsolete = self.factory.makeProductSeries(
> +            product=product, name='evo-obsolete')
> +        series_obsolete.status = SeriesStatus.OBSOLETE
> +
> +        series_future = self.factory.makeProductSeries(
> +            product=product, name='evo-future')
> +        series_future.status = SeriesStatus.FUTURE
> +

Everything from here to...
> +        # '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])

...here *could* be replaced with the following, if you think it's more readable:

        series_names = [series.name for series in view.untranslatable_series]
        
        # The series are returned in alphabetical order and do not
        # include obsolete series.
        self.assertEqual([
            u'evo-current',
            u'evo-development',
            u'evo-experimental',
            u'evo-frozen',
            u'evo-future',
            u'evo-supported',
            u'trunk'], series_names)

> +
>  
>  def test_suite():
>      return unittest.TestLoader().loadTestsFromName(__name__)
> -
> 
> === modified file 'lib/lp/translations/stories/productseries/xx-productseries-translations.txt'
> --- lib/lp/translations/stories/productseries/xx-productseries-translations.txt	2010-01-05 16:19:13 +0000
> +++ lib/lp/translations/stories/productseries/xx-productseries-translations.txt	2010-01-28 15:26:22 +0000
> @@ -148,7 +148,7 @@
>  
>      # Use the raw DB object to bypass the security proxy.
>      >>> from lp.registry.model.product import Product
> -    >>> Product.byName('evolution').official_rosetta = False
> +    >>> Product.byName('bazaar').official_rosetta = False
>  
>  When the owner now visits the upload page for trunk, there's a notice.
>  
> @@ -268,3 +268,60 @@
>      >>> print extract_text(
>      ...     find_tag_by_id(admin_browser.contents, 'translation-focus'))
>      Launchpad currently recommends translating 1.6.
> +
> +
> +Setting up translations for series
> +------------------------------------
> +
> +When visiting product translations main page, project developers sees
> +status for current series configured for translations.
> +Beside the "All translatable series" section, they will find the
> +"Set up translations for a series" section with links to other series
> +that can be configured for translations.
> +
> +When projects have only one active series, and it is already configured,
> +project admin does not see the link for configuring other branches.
> +
> +    >>> admin_browser.open(
> +    ...     'http://translations.launchpad.dev/evolution')
> +    >>> untranslatable = find_tag_by_id(
> +    ...     admin_browser.contents, 'portlet-untranslatable-branches')
> +    >>> untranslatable is None
> +    True
> +
> +A new series is added.
> +
> +    >>> from lp.registry.model.product import Product
> +    >>> from lp.registry.interfaces.series import SeriesStatus
> +    >>> login('<email address hidden>')
> +    >>> evolution = Product.byName('evolution')
> +    >>> series = factory.makeProductSeries(
> +    ...     product=evolution, name='evo-new')
> +    >>> series.status = SeriesStatus.EXPERIMENTAL
> +    >>> logout()
> +
> +Project administrator will see links to configuring translations for
> +the new series.
> +
> +    >>> admin_browser.open(
> +    ...     'http://translations.launchpad.dev/evolution')
> +    >>> untranslatable = find_tag_by_id(
> +    ...     admin_browser.contents, 'portlet-untranslatable-branches')
> +    >>> print extract_text(untranslatable)
> +    Set up translations for a series...
> +    evo-new series — manual or automatic...
> +
> +For each series there is a link for accesing the series translations
> +page togheter with link for uploading a template from that series
> +(manual) and setting automatic imports.
> +
> +    >>> print admin_browser.getLink('Evolution evo-new series').url
> +    http://translations.launchpad.dev/evolution/evo-new/+translations
> +
> +    >>> print admin_browser.getLink(
> +    ... 'manual', url='/evolution/evo-new/+translations-upload').url
> +    http://translations.launchpad.dev/evolution/evo-new/+translations-upload
> +
> +    >>> print admin_browser.getLink(
> +    ... 'automatic', url='/evolution/evo-new/+translations-settings').url
> +    http://translations.launchpad.dev/evolution/evo-new/+translations-settings
> 

Great, thanks for working on that Adi, and sorry if I've been pedantic :/. I
learned quite a bit going through the code, so it was helpful for me :)