Merge lp:~jtv/launchpad/bug-416434-aggregator into lp:launchpad

[Jeroen T. Vermeulen]

[Jeroen T. Vermeulen (jtv)]

= Translation Links Aggregator =

This branch is a bit overweight because it lifts code & tests out of a
view class and into a module of its own. It's preparatory work for the
"stuff to translate" addition to the Translations personal dashboard.

A recurring problem is this one: given a bunch of POFiles, what is a
sensible set of links to present them to the user? One extreme answer
would be to link to (and describe) each individually, but we're trying
to build a more sensible UI with less pointless information for the
viewer to plod through.

At the other extreme, we could link to the largest single thing in
Launchpad that represents all of the translations: POFile; POTemplate;
ProductSeries or SourcePackage; Product or Distribution; or even all of
Launchpad Translations. The more extreme it gets, the less sensible it
becomes. We like to avoid "false positives" where lots of the stuff we
link to is not actually in the original list of translations.

The TranslationLinksAggregator, introduced here, tries to find a happy
medium. It breaks your set of translations down by ProductSeries or
SourcePackage, and within each of those, it tries to find a good common
link to represent all the inputs without too many false positives. If
it finds no really good single link, it links to the individual POFiles.

You use this new class by inheriting from it and overloading "describe."
This method receives all relevant information about a translation target
and turns it into a description suitable for use by the UI. The
description can take whatever form the subclass likes, making the
aggregator fairly flexible.

(Description is the very last stage of aggregation and I could have left
it out. I could have made the aggregator produce a sequence of tuples,
each with the arguments that now are passed to the describe method. The
caller could then process each of these in any way desired. But in my
experience passing lists of tuples around soon affects maintainability;
turning them into classes on the other hand leads to obesity).

As a free extra, the aggregator supports not just POFiles but
POTemplates as well. In some ways they are like "POFiles without a
language." Danilo reviewed the branch that first introduced this stuff
and found that it solved a very similar problem to one he faced with
translation exports. One of the main differences was that in his case
there might be templates as well as translations. It was easy enough to
add, and hopefully this means that the class will serve his needs as
well.

The hard part about this branch is that it breaks the unit tests for the
PersonTranslationView into two: a new one for the aggregator, and the
gutted remains of the view test proper. Some detail in the unit-testing
will be lost, and a bit of additional high-level testing makes up for
it.

== Tests, demo & Q/A ==

Test with:
{{{
./bin/test -vv -t persontranslationview
./bin/test -vv -t translationlinksaggregator
}}}

To demo and Q/A, look at the home page for someone who is in a
translation group, under the Translations tab. If that person is you,
you'll see a list of translations suggested for review. That list will
now be aggregated using t...

[Graham Binns (gmb)]

review: Approve (gmb)

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2009-08-19 14:25:32 +0000
+++ lib/lp/translations/browser/person.py	2009-08-25 10:44:02 +0000
@@ -26,6 +26,9 @@
 from canonical.launchpad.webapp.publisher import LaunchpadView
 from canonical.widgets import LaunchpadRadioWidget
 from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.translations.browser.translationlinksaggregator import (
+    TranslationLinksAggregator)
 from lp.translations.interfaces.pofiletranslator import (
     IPOFileTranslatorSet)
 from lp.translations.interfaces.translationrelicensingagreement import (
@@ -33,7 +36,47 @@
     TranslationRelicensingAgreementOptions)
 from lp.translations.interfaces.translationsperson import (
     ITranslationsPerson)
-from lp.translations.model.productserieslanguage import ProductSeriesLanguage
+
+
+class WorkListLinksAggregator(TranslationLinksAggregator):
+    """Aggregate translation links for translation or review.
+
+    Here, all files are actually `POFile`s, never `POTemplate`s.
+    """
+
+    def countStrings(self, pofile):
+        """Count number of strings that need work."""
+        raise NotImplementedError()
+
+    def describe(self, target, link, covered_files):
+        """See `TranslationLinksAggregator.describe`."""
+        strings_count = sum(
+            [self.countStrings(pofile) for pofile in covered_files])
+
+        if strings_count == 1:
+            strings_wording = "%d string"
+        else:
+            strings_wording = "%d strings"
+
+        return {
+            'target': target,
+            'count': strings_count,
+            'count_wording': strings_wording % strings_count,
+            'is_product': not ISourcePackage.providedBy(target),
+            'link': link,
+        }
+
+
+class ReviewLinksAggregator(WorkListLinksAggregator):
+    """A `TranslationLinksAggregator` for translations to review."""
+
+    # Link to unreviewed suggestions.
+    pofile_link_suffix = '/+translate?show=new_suggestions'
+
+    # Strings that need work are ones with unreviewed suggestions.
+    def countStrings(self, pofile):
+        """See `WorkListLinksAggregator.countStrings`."""
+        return pofile.unreviewedCount()
 
 
 class PersonTranslationsMenu(NavigationMenu):
@@ -98,6 +141,20 @@
         """Is this person in a translation group?"""
         return len(self.translation_groups) != 0
 
+    @property
+    def person_is_translator(self):
+        """Is this person active in translations?"""
+        return self.context.hasKarma('translations')
+
+    @property
+    def person_includes_me(self):
+        """Is the current user (a member of) this person?"""
+        user = getUtility(ILaunchBag).user
+        if user is None:
+            return False
+        else:
+            return user.inTeam(self.context)
+
     def should_display_message(self, translationmessage):
         """Should a certain `TranslationMessage` be displayed.
 
@@ -111,167 +168,51 @@
         return not (
             translationmessage.potmsgset.hide_translations_from_anonymous)
 
-    def _composeReviewLinks(self, pofiles):
-        """Compose URLs for reviewing given `POFile`s."""
-        return [
-            canonical_url(pofile) + '/+translate?show=new_suggestions'
-            for pofile in pofiles
-            ]
-
-    def _findBestCommonReviewLinks(self, pofiles):
-        """Find best links to a bunch of related `POFile`s.
-
-        The `POFile`s must either be in the same `Product`, or in the
-        same `DistroSeries` and `SourcePackageName`.
-        
-        This method finds the greatest common denominators between them,
-        and returns a list of links to them: the individual translation
-        if there is only one, the template if multiple translations of
-        one template are involved, and so on.
-        """
-        assert pofiles, "Empty POFiles list in reviewable target."
-        first_pofile = pofiles[0]
-
-        if len(pofiles) == 1:
-            # Simple case: one translation file.  Go straight to
-            # translation page for its unreviewed strings.
-            return self._composeReviewLinks(pofiles)
-
-        templates = set(pofile.potemplate for pofile in pofiles)
-
-        productseries = set(
-            template.productseries
-            for template in templates
-            if template.productseries)
-
-        products = set(series.product for series in productseries)
-
-        sourcepackagenames = set(
-            template.sourcepackagename
-            for template in templates
-            if template.sourcepackagename)
-        
-        distroseries = set(
-            template.distroseries
-            for template in templates
-            if template.distroseries)
-
-        assert len(products) <= 1, "Got more than one product."
-        assert len(sourcepackagenames) <= 1, "Got more than one package."
-        assert len(distroseries) <= 1, "Got more than one distroseries."
-        assert len(products) + len(sourcepackagenames) == 1, (
-            "Didn't get POFiles for exactly one package or one product.")
-        
-        first_template = first_pofile.potemplate
-
-        if len(templates) == 1:
-            # Multiple translations for one template.  Link to the
-            # template.
-            return [canonical_url(first_template)]
-
-        if sourcepackagenames:
-            # Multiple POFiles for a source package.  Show its template
-            # listing.
-            distroseries = first_template.distroseries
-            packagename = first_template.sourcepackagename
-            return [canonical_url(distroseries.getSourcePackage(packagename))]
-
-        if len(productseries) == 1:
-            series = first_template.productseries
-            # All for the same ProductSeries.
-            languages = set(pofile.language for pofile in pofiles)
-            if len(languages) == 1:
-                # All for the same language in the same ProductSeries,
-                # but apparently for different templates.  Link to
-                # ProductSeriesLanguage.
-                productserieslanguage = ProductSeriesLanguage(
-                    series, pofiles[0].language)
-                return [canonical_url(productserieslanguage)]
-            else:
-                # Multiple templates and languages in the same product
-                # series.  Show its templates listing.
-                return [canonical_url(series)]
-
-        # Different release series of the same product.  Link to each of
-        # the individual POFiles.
-        return self._composeReviewLinks(pofiles)
-
-    def _describeReviewableTarget(self, target, link, strings_count):
-        """Produce dict to describe a reviewable target.
-
-        The target may be a `Product` or a tuple of `SourcePackageName`
-        and `DistroSeries`.
-        """
-        if isinstance(target, tuple):
-            (name, distroseries) = target
-            target = distroseries.getSourcePackage(name)
-            is_product = False
-        else:
-            is_product = True
-
-        if strings_count == 1:
-            strings_wording = '%d string'
-        else:
-            strings_wording = '%d strings'
-
-        return {
-            'target': target,
-            'count': strings_count,
-            'link': link,
-            'count_wording': strings_wording % strings_count,
-            'is_product': is_product,
-        }
-
     def _setHistoryHorizon(self):
         """If not already set, set `self.history_horizon`."""
         if self.history_horizon is None:
             now = datetime.now(pytz.timezone('UTC'))
             self.history_horizon = now - timedelta(90, 0, 0)
 
-    def _aggregateTranslationTargets(self, pofiles):
-        """Aggregate list of `POFile`s into sensible targets.
-
-        Returns a list of target descriptions as returned by
-        `_describeReviewableTarget` after going through
-        `_findBestCommonReviewLinks`.
-        """
-        targets = {}
-        for pofile in pofiles:
-            template = pofile.potemplate
-            if template.productseries:
-                target = template.productseries.product
-            else:
-                target = (template.sourcepackagename, template.distroseries)
-
-            if target in targets:
-                (count, target_pofiles) = targets[target]
-            else:
-                count = 0
-                target_pofiles = []
-
-            count += pofile.unreviewedCount()
-            target_pofiles.append(pofile)
-
-            targets[target] = (count, target_pofiles)
-
-        result = []
-        for target, stats in targets.iteritems():
-            (count, target_pofiles) = stats
-            links = self._findBestCommonReviewLinks(target_pofiles)
-            for link in links:
-                result.append(
-                    self._describeReviewableTarget(target, link, count))
-
-        return result
+    def _getTargetsForReview(self, max_fetch=None):
+        """Query and aggregate the top targets for review.
+
+        :param max_fetch: Maximum number of `POFile`s to fetch while
+            looking for these.
+        :return: a list of at most `max_fetch` translation targets.
+            Multiple `POFile`s may be aggregated together into a single
+            target.
+        """
+        self._setHistoryHorizon()
+        person = ITranslationsPerson(self.context)
+        pofiles = person.getReviewableTranslationFiles(
+            no_older_than=self.history_horizon)
+
+        if max_fetch is not None:
+            pofiles = pofiles[:max_fetch]
+
+        return ReviewLinksAggregator().aggregate(pofiles)
+
+    def _suggestTargetsForReview(self, max_fetch):
+        """Find random translation targets for review.
+
+        :param max_fetch: Maximum number of `POFile`s to fetch while
+            looking for these.
+        :return: a list of at most `max_fetch` translation targets.
+            Multiple `POFile`s may be aggregated together into a single
+            target.
+        """
+        self._setHistoryHorizon()
+        person = ITranslationsPerson(self.context)
+        pofiles = person.suggestReviewableTranslationFiles(
+            no_older_than=self.history_horizon)[:max_fetch]
+
+        return ReviewLinksAggregator().aggregate(pofiles)
 
     @cachedproperty
     def all_projects_and_packages_to_review(self):
         """Top projects and packages for this person to review."""
-        self._setHistoryHorizon()
-        person = ITranslationsPerson(self.context)
-        return self._aggregateTranslationTargets(
-            person.getReviewableTranslationFiles(
-                no_older_than=self.history_horizon))
+        return self._getTargetsForReview()
 
     @property
     def top_projects_and_packages_to_review(self):
@@ -280,42 +221,30 @@
 
         # Maximum number of projects/packages to list that this person
         # has recently worked on.
-        max_old_targets = 9
+        max_known_targets = 9
         # Length of overall list to display.
         list_length = 10
 
         # Start out with the translations that the person has recently
         # worked on.  Aggregation may reduce the number we get, so ask
         # the database for a few extra.
-        fetch = 5 * max_old_targets
-        recent = self.all_projects_and_packages_to_review[:fetch]
+        fetch = 5 * max_known_targets
+        recent = self._getTargetsForReview(fetch)[:max_known_targets]
 
         # Fill out the list with other translations that the person
         # could also be reviewing.
-        empty_slots = list_length - min(len(recent), max_old_targets)
+        empty_slots = list_length - len(recent)
         fetch = 5 * empty_slots
-
-        person = ITranslationsPerson(self.context)
-        random_suggestions = self._aggregateTranslationTargets(
-            person.suggestReviewableTranslationFiles(
-                no_older_than=self.history_horizon)[:fetch])
-
-        return recent[:max_old_targets] + random_suggestions[:empty_slots]
+        random_suggestions = self._suggestTargetsForReview(fetch)
+        random_suggestions = random_suggestions[:empty_slots]
+
+        return recent + random_suggestions
 
     @cachedproperty
     def num_projects_and_packages_to_review(self):
         """How many translations do we suggest for reviewing?"""
         return len(self.all_projects_and_packages_to_review)
 
-    @property
-    def person_includes_me(self):
-        """Is the current user (a member of) this person?"""
-        user = getUtility(ILaunchBag).user
-        if user is None:
-            return False
-        else:
-            return user.inTeam(self.context)
-
 
 class PersonTranslationRelicensingView(LaunchpadFormView):
     """View for Person's translation relicensing page."""

=== modified file 'lib/lp/translations/browser/tests/test_persontranslationview.py'
--- lib/lp/translations/browser/tests/test_persontranslationview.py	2009-08-19 15:20:40 +0000
+++ lib/lp/translations/browser/tests/test_persontranslationview.py	2009-08-25 10:26:28 +0000
@@ -14,7 +14,6 @@
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from lp.services.worlddata.model.language import LanguageSet
 from lp.translations.browser.person import PersonTranslationView
-from lp.translations.model.productserieslanguage import ProductSeriesLanguage
 from lp.translations.model.translator import TranslatorSet
 
 
@@ -78,251 +77,6 @@
         self._makeReviewer()
         self.assertTrue(self.view.person_is_reviewer)
 
-    def test_findBestCommonReviewLinks_single_pofile(self):
-        # If passed a single POFile, _findBestCommonReviewLinks returns
-        # a list of just that POFile.
-        pofile = self.factory.makePOFile(language_code='lua')
-        links = self.view._findBestCommonReviewLinks([pofile])
-        self.assertEqual(self.view._composeReviewLinks([pofile]), links)
-
-    def test_findBestCommonReviewLinks_product_wild_mix(self):
-        # A combination of wildly different POFiles in the same product
-        # yields links to the individual POFiles.
-        pofile1 = self.factory.makePOFile(language_code='sux')
-        product = pofile1.potemplate.productseries.product
-        series2 = self.factory.makeProductSeries(product)
-        template2 = self.factory.makePOTemplate(productseries=series2)
-        pofile2 = self.factory.makePOFile(
-            potemplate=template2, language_code='la')
-
-        links = self.view._findBestCommonReviewLinks([pofile1, pofile2])
-
-        expected_links = self.view._composeReviewLinks([pofile1, pofile2])
-        self.assertEqual(expected_links, links)
-
-    def test_findBestCommonReviewLinks_different_templates(self):
-        # A combination of POFiles in the same language but different
-        # templates of the same productseries is represented as a link
-        # to the ProductSeriesLanguage.
-        pofile1 = self.factory.makePOFile(language_code='nl')
-        series = pofile1.potemplate.productseries
-        template2 = self.factory.makePOTemplate(productseries=series)
-        pofile2 = self.factory.makePOFile(
-            potemplate=template2, language_code='nl')
-
-        links = self.view._findBestCommonReviewLinks([pofile1, pofile2])
-
-        productserieslanguage = ProductSeriesLanguage(
-            series, pofile1.language)
-        self.assertEqual([canonical_url(productserieslanguage)], links)
-
-    def test_findBestCommonReviewLinks_different_languages(self):
-        # If the POFiles differ only in language, we get a link to the
-        # overview for the template.
-        pofile1 = self.factory.makePOFile(language_code='nl')
-        template = pofile1.potemplate
-        pofile2 = self.factory.makePOFile(
-            potemplate=template, language_code='lo')
-
-        links = self.view._findBestCommonReviewLinks([pofile1, pofile2])
-
-        self.assertEqual([canonical_url(template)], links)
-
-    def test_findBestCommonReviewLinks_sharing_pofiles(self):
-        # In a Product, two POFiles may share their translations.  For
-        # now, we link to each individually.  We may want to make this
-        # more clever in the future.
-        pofile1 = self.factory.makePOFile(language_code='nl')
-        template1 = pofile1.potemplate
-        series1 = template1.productseries
-        series2 = self.factory.makeProductSeries(product=series1.product)
-        template2 = self.factory.makePOTemplate(
-            productseries=series2, name=template1.name,
-            translation_domain=template1.translation_domain)
-        pofile2 = template2.getPOFileByLang('nl')
-
-        pofiles = [pofile1, pofile2]
-        links = self.view._findBestCommonReviewLinks(pofiles)
-
-        self.assertEqual(self.view._composeReviewLinks(pofiles), links)
-
-    def test_findBestCommonReviewLinks_package_different_languages(self):
-        # For package POFiles in the same template but different
-        # languages, we link to the template.
-        package = self.factory.makeSourcePackage()
-        package.distroseries.distribution.official_rosetta = True
-        template = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        pofile1 = self.factory.makePOFile(
-            potemplate=template, language_code='nl')
-        pofile2 = self.factory.makePOFile(
-            potemplate=template, language_code='ka')
-
-        links = self.view._findBestCommonReviewLinks([pofile1, pofile2])
-        self.assertEqual([canonical_url(template)], links)
-
-    def test_findBestCommonReviewLinks_package_different_templates(self):
-        # For package POFiles in different templates, we to the
-        # package's template list.  There is no "source package series
-        # language" page.
-        package = self.factory.makeSourcePackage()
-        package.distroseries.distribution.official_rosetta = True
-        template1 = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        template2 = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        pofile1 = self.factory.makePOFile(
-            potemplate=template1, language_code='nl')
-        pofile2 = self.factory.makePOFile(
-            potemplate=template2, language_code='nl')
-
-        links = self.view._findBestCommonReviewLinks([pofile1, pofile2])
-
-        self.assertEqual([canonical_url(package)], links)
-
-    def test_describeReviewableTarget_string_count(self):
-        # _describeReviewableTarget puts out a human-readable
-        # description of how many strings need review.
-        product = self.factory.makeProduct()
-
-        description = self.view._describeReviewableTarget(
-            product, canonical_url(product), 0)
-        self.assertEqual(description['count'], 0)
-        self.assertEqual(description['count_wording'], '0 strings')
-
-        # Singular applies for exactly 1 string.
-        description = self.view._describeReviewableTarget(
-            product, canonical_url(product), 1)
-        self.assertEqual(description['count'], 1)
-        self.assertEqual(description['count_wording'], '1 string')
-
-
-        description = self.view._describeReviewableTarget(
-            product, canonical_url(product), 2)
-        self.assertEqual(description['count'], 2)
-        self.assertEqual(description['count_wording'], '2 strings')
-
-    def test_describeReviewableTarget_product(self):
-        # _describeReviewableTarget describes a Product with reviewable
-        # translations.
-        product = self.factory.makeProduct()
-        link = canonical_url(product)
-
-        description = self.view._describeReviewableTarget(product, link, 99)
-
-        expected_description = {
-            'target': product,
-            'count': 99,
-            'count_wording': "99 strings",
-            'is_product': True,
-            'link': link,
-        }
-        self.assertEqual(expected_description, description)
-
-    def test_describeReviewableTarget_package(self):
-        # _describeReviewableTarget describes a package with reviewable
-        # translations.
-        package = self.factory.makeSourcePackage()
-        package.distroseries.distribution.official_rosetta = True
-        target = (package.sourcepackagename, package.distroseries)
-        link = canonical_url(package)
-
-        description = self.view._describeReviewableTarget(target, link, 42)
-
-        expected_description = {
-            'target': package,
-            'count': 42,
-            'count_wording': "42 strings",
-            'is_product': False,
-            'link': link,
-        }
-        self.assertEqual(expected_description, description)
-
-    def test_aggregateTranslationTargets(self):
-        # _aggregateTranslationTargets represents a series of POFiles as
-        # a series of target descriptions, aggregating where possible.
-
-        # Trivial case: no POFiles means no targets.
-        self.assertEqual([], self.view._aggregateTranslationTargets([]))
-
-        # Basic case: one POFile yields its product or package.
-        pofile = self.factory.makePOFile(language_code='ca')
-
-        description = self.view._aggregateTranslationTargets([pofile])
-
-        expected_links = self.view._composeReviewLinks([pofile])
-        expected_description = [{
-            'target': pofile.potemplate.productseries.product,
-            'count': 0,
-            'count_wording': "0 strings",
-            'is_product': True,
-            'link': expected_links[0],
-        }]
-        self.assertEqual(expected_description, description)
-
-    def test_aggregateTranslationTargets_product_and_package(self):
-        # _aggregateTranslationTargets keeps a product and a package
-        # separate.
-        product_pofile = self.factory.makePOFile(language_code='th')
-        removeSecurityProxy(product_pofile).unreviewed_count = 1
-
-        package = self.factory.makeSourcePackage()
-        package.distroseries.distribution.official_rosetta = True
-        package_template = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        package_pofile = self.factory.makePOFile(
-            potemplate=package_template, language_code='th')
-        removeSecurityProxy(package_pofile).unreviewed_count = 2
-
-        descriptions = self.view._aggregateTranslationTargets(
-            [product_pofile, package_pofile])
-        links = set(entry['link'] for entry in descriptions)
-
-        expected_links = set(
-            self.view._composeReviewLinks([product_pofile, package_pofile]))
-        self.assertEqual(expected_links, links)
-
-    def test_aggregateTranslationTargets_bundles_productseries(self):
-        # _aggregateTranslationTargets describes POFiles for the same
-        # ProductSeries together.
-        pofile1 = self.factory.makePOFile(language_code='es')
-        series = pofile1.potemplate.productseries
-        template2 = self.factory.makePOTemplate(productseries=series)
-        pofile2 = self.factory.makePOFile(
-            language_code='br', potemplate=template2)
-        
-        description = self.view._aggregateTranslationTargets(
-            [pofile1, pofile2])
-
-        self.assertEqual(1, len(description))
-        self.assertEqual(canonical_url(series), description[0]['link'])
-
-    def test_aggregateTranslationTargets_bundles_package(self):
-        # _aggregateTranslationTargets describes POFiles for the same
-        # ProductSeries together.
-        package = self.factory.makeSourcePackage()
-        package.distroseries.distribution.official_rosetta = True
-        template1 = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        pofile1 = self.factory.makePOFile(
-            language_code='es', potemplate=template1)
-        template2 = self.factory.makePOTemplate(
-            distroseries=package.distroseries,
-            sourcepackagename=package.sourcepackagename)
-        pofile2 = self.factory.makePOFile(
-            language_code='br', potemplate=template2)
-        
-        description = self.view._aggregateTranslationTargets(
-            [pofile1, pofile2])
-
-        self.assertEqual(1, len(description))
-        self.assertEqual(canonical_url(package), description[0]['link'])
-
     def test_num_projects_and_packages_to_review(self):
         # num_projects_and_packages_to_review counts the number of
         # reviewable targets that the person has worked on.
@@ -332,6 +86,50 @@
 
         self.assertEqual(1, self.view.num_projects_and_packages_to_review)
 
+    def test_all_projects_and_packages_to_review_one(self):
+        # all_projects_and_packages describes the translations available
+        # for review by its person.
+        self._makeReviewer()
+        pofile = self._makePOFiles(1, previously_worked_on=True)[0]
+        product = pofile.potemplate.productseries.product
+
+        descriptions = self.view.all_projects_and_packages_to_review
+
+        self.assertEqual(1, len(descriptions))
+        self.assertEqual(product, descriptions[0]['target'])
+
+    def test_all_projects_and_packages_to_review_none(self):
+        # all_projects_and_packages_to_review works even if there is
+        # nothing to review.  It will find nothing.
+        self._makeReviewer()
+
+        descriptions = self.view.all_projects_and_packages_to_review
+
+        self.assertEqual([], descriptions)
+
+    def test_all_projects_and_packages_to_review_string_singular(self):
+        # A translation description says how many strings need review,
+        # both as a number and as text.
+        self._makeReviewer()
+        pofile = self._makePOFiles(1, previously_worked_on=True)[0]
+        removeSecurityProxy(pofile).unreviewed_count = 1
+
+        description = self.view.all_projects_and_packages_to_review[0]
+
+        self.assertEqual(1, description['count'])
+        self.assertEqual("1 string", description['count_wording'])
+
+    def test_all_projects_and_packages_to_review_string_plural(self):
+        # For multiple strings, count_wording uses the plural.
+        self._makeReviewer()
+        pofile = self._makePOFiles(1, previously_worked_on=True)[0]
+        removeSecurityProxy(pofile).unreviewed_count = 2
+
+        description = self.view.all_projects_and_packages_to_review[0]
+
+        self.assertEqual(2, description['count'])
+        self.assertEqual("2 strings", description['count_wording'])
+
     def test_num_projects_and_packages_to_review_zero(self):
         # num_projects_and_packages does not count new suggestions.
         self._makeReviewer()
@@ -351,8 +149,11 @@
 
         targets = self.view.top_projects_and_packages_to_review
 
-        expected_links = self.view._composeReviewLinks(
-            [pofile_worked_on, pofile_not_worked_on])
+        pofile_suffix = '/+translate?show=new_suggestions'
+        expected_links = [
+            canonical_url(pofile_worked_on) + pofile_suffix,
+            canonical_url(pofile_not_worked_on) + pofile_suffix,
+            ]
         self.assertEqual(
             set(expected_links), set(item['link'] for item in targets))
 

=== added file 'lib/lp/translations/browser/tests/test_translationlinksaggregator.py'
--- lib/lp/translations/browser/tests/test_translationlinksaggregator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/tests/test_translationlinksaggregator.py	2009-08-25 10:35:29 +0000
@@ -0,0 +1,269 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from unittest import TestLoader
+
+from zope.security.proxy import removeSecurityProxy
+
+from lp.testing import TestCaseWithFactory
+from canonical.testing import LaunchpadZopelessLayer
+
+from canonical.launchpad.webapp import canonical_url
+from lp.translations.browser.translationlinksaggregator import (
+    TranslationLinksAggregator)
+from lp.translations.model.productserieslanguage import ProductSeriesLanguage
+
+
+class DumbAggregator(TranslationLinksAggregator):
+    """A very simple `TranslationLinksAggregator`.
+
+    The `describe` method returns a tuple of its arguments.
+    """
+    def describe(self, target, link, covered_sheets):
+        """See `TranslationLinksAggregator`."""
+        return (target, link, covered_sheets)
+
+
+def map_link(link_target, sheets=None, add_to=None):
+    """Map a link the way _circumscribe does.
+
+    :param link_target: The object to link to.  Its URL will be used.
+    :param sheets: A list of POFiles and/or POTemplates.  The link will
+        map to these.  If omitted, the list will consist of
+        `link_target` itself.
+    :param add_to: Optional existing dict to add the new entry to.
+    :return: A dict mapping the URL for link_target to sheets.
+    """
+    if add_to is None:
+        add_to = {}
+    if sheets is None:
+        sheets = [link_target]
+    add_to[canonical_url(link_target)] = sheets
+    return add_to
+
+
+class TestTranslationLinksAggregator(TestCaseWithFactory):
+    """Test `TranslationLinksAggregator`."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestTranslationLinksAggregator, self).setUp()
+        self.aggregator = DumbAggregator()
+
+    def test_circumscribe_single_pofile(self):
+        # If passed a single POFile, _circumscribe returns a list of
+        # just that POFile.
+        pofile = self.factory.makePOFile(language_code='lua')
+
+        links = self.aggregator._circumscribe([pofile])
+
+        self.assertEqual(map_link(pofile), links)
+
+    def test_circumscribe_product_wild_mix(self):
+        # A combination of wildly different POFiles in the same product
+        # yields links to the individual POFiles.
+        pofile1 = self.factory.makePOFile(language_code='sux')
+        product = pofile1.potemplate.productseries.product
+        series2 = self.factory.makeProductSeries(product)
+        template2 = self.factory.makePOTemplate(productseries=series2)
+        pofile2 = self.factory.makePOFile(
+            potemplate=template2, language_code='la')
+
+        links = self.aggregator._circumscribe([pofile1, pofile2])
+
+        expected_links = map_link(pofile1)
+        expected_links = map_link(pofile2, add_to=expected_links)
+        self.assertEqual(expected_links, links)
+
+    def test_circumscribe_different_templates(self):
+        # A combination of POFiles in the same language but different
+        # templates of the same productseries is represented as a link
+        # to the ProductSeriesLanguage.
+        pofile1 = self.factory.makePOFile(language_code='nl')
+        series = pofile1.potemplate.productseries
+        template2 = self.factory.makePOTemplate(productseries=series)
+        pofile2 = self.factory.makePOFile(
+            potemplate=template2, language_code='nl')
+
+        links = self.aggregator._circumscribe([pofile1, pofile2])
+
+        psl = ProductSeriesLanguage(series, pofile1.language)
+        self.assertEqual(map_link(psl, [pofile1, pofile2]), links)
+
+    def test_circumscribe_different_languages(self):
+        # If the POFiles differ only in language, we get a link to the
+        # overview for the template.
+        pofile1 = self.factory.makePOFile(language_code='nl')
+        template = pofile1.potemplate
+        pofile2 = self.factory.makePOFile(
+            potemplate=template, language_code='lo')
+
+        pofiles = [pofile1, pofile2]
+        links = self.aggregator._circumscribe(pofiles)
+
+        self.assertEqual(map_link(template, pofiles), links)
+
+    def test_circumscribe_sharing_pofiles(self):
+        # In a Product, two POFiles may share their translations.  For
+        # now, we link to each individually.  We may want to make this
+        # more clever in the future.
+        pofile1 = self.factory.makePOFile(language_code='nl')
+        template1 = pofile1.potemplate
+        series1 = template1.productseries
+        series2 = self.factory.makeProductSeries(product=series1.product)
+        template2 = self.factory.makePOTemplate(
+            productseries=series2, name=template1.name,
+            translation_domain=template1.translation_domain)
+        pofile2 = template2.getPOFileByLang('nl')
+
+        pofiles = [pofile1, pofile2]
+        links = self.aggregator._circumscribe(pofiles)
+
+        expected_links = map_link(pofile1)
+        expected_links = map_link(pofile2, add_to=expected_links)
+        self.assertEqual(expected_links, links)
+
+    def test_circumscribe_package_different_languages(self):
+        # For package POFiles in the same template but different
+        # languages, we link to the template.
+        package = self.factory.makeSourcePackage()
+        package.distroseries.distribution.official_rosetta = True
+        template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        pofile1 = self.factory.makePOFile(
+            potemplate=template, language_code='nl')
+        pofile2 = self.factory.makePOFile(
+            potemplate=template, language_code='ka')
+
+        pofiles = [pofile1, pofile2]
+        links = self.aggregator._circumscribe(pofiles)
+        self.assertEqual(map_link(template, pofiles), links)
+
+    def test_circumscribe_package_different_templates(self):
+        # For package POFiles in different templates, we to the
+        # package's template list.  There is no "source package series
+        # language" page.
+        package = self.factory.makeSourcePackage()
+        package.distroseries.distribution.official_rosetta = True
+        template1 = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        template2 = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        pofile1 = self.factory.makePOFile(
+            potemplate=template1, language_code='nl')
+        pofile2 = self.factory.makePOFile(
+            potemplate=template2, language_code='nl')
+
+        pofiles = [pofile1, pofile2]
+        links = self.aggregator._circumscribe(pofiles)
+
+        self.assertEqual(map_link(package, pofiles), links)
+
+    def test_circumscribe_pofile_plus_template(self):
+        # A template circumscribes both itself and any of its
+        # translations.
+        pofile = self.factory.makePOFile(language_code='uga')
+        template = pofile.potemplate
+
+        sheets = [pofile, template]
+        links = self.aggregator._circumscribe(sheets)
+
+        self.assertEqual(map_link(template, sheets), links)
+
+    def test_aggregate(self):
+        # The aggregator represents a series of POFiles as a series of
+        # target descriptions, aggregating where possible.
+
+        # Trivial case: no POFiles means no targets.
+        self.assertEqual([], self.aggregator.aggregate([]))
+
+        # Basic case: one POFile yields its product or package.
+        pofile = self.factory.makePOFile(language_code='ca')
+        product = pofile.potemplate.productseries.product
+
+        descriptions = self.aggregator.aggregate([pofile])
+
+        expected = [(product, canonical_url(pofile), [pofile])]
+        self.assertEqual(expected, descriptions)
+
+    def test_aggregate_potemplate(self):
+        # Besides POFiles, you can also feed an aggregator POTemplates.
+        template = self.factory.makePOTemplate()
+        product = template.productseries.product
+
+        descriptions = self.aggregator.aggregate([template])
+
+        expected = [(product, canonical_url(template), [template])]
+        self.assertEqual(expected, descriptions)
+
+    def test_aggregate_product_and_package(self):
+        # The aggregator keeps a product and a package separate.
+        product_pofile = self.factory.makePOFile(language_code='th')
+        product = product_pofile.potemplate.productseries.product
+        removeSecurityProxy(product_pofile).unreviewed_count = 1
+
+        package = self.factory.makeSourcePackage()
+        package.distroseries.distribution.official_rosetta = True
+        package_template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        package_pofile = self.factory.makePOFile(
+            potemplate=package_template, language_code='th')
+        removeSecurityProxy(package_pofile).unreviewed_count = 2
+
+        descriptions = self.aggregator.aggregate(
+            [product_pofile, package_pofile])
+
+        expected = [
+            (product, canonical_url(product_pofile), [product_pofile]),
+            (package, canonical_url(package_pofile), [package_pofile]),
+            ]
+        self.assertEqual(expected, descriptions)
+
+    def test_aggregate_bundles_productseries(self):
+        # _aggregateTranslationTargets describes POFiles for the same
+        # ProductSeries together.
+        pofile1 = self.factory.makePOFile(language_code='es')
+        series = pofile1.potemplate.productseries
+        template2 = self.factory.makePOTemplate(productseries=series)
+        pofile2 = self.factory.makePOFile(
+            language_code='br', potemplate=template2)
+
+        pofiles = [pofile1, pofile2]
+        descriptions = self.aggregator.aggregate(pofiles)
+
+        self.assertEqual(1, len(descriptions))
+        self.assertEqual(
+            [(series.product, canonical_url(series), pofiles)], descriptions)
+
+    def test_aggregate_bundles_package(self):
+        # _aggregateTranslationTargets describes POFiles for the same
+        # ProductSeries together.
+        package = self.factory.makeSourcePackage()
+        package.distroseries.distribution.official_rosetta = True
+        template1 = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        pofile1 = self.factory.makePOFile(
+            language_code='es', potemplate=template1)
+        template2 = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        pofile2 = self.factory.makePOFile(
+            language_code='br', potemplate=template2)
+
+        pofiles = [pofile1, pofile2]
+        descriptions = self.aggregator.aggregate(pofiles)
+
+        expected = [(package, canonical_url(package), pofiles)]
+        self.assertEqual(expected, descriptions)
+
+
+def test_suite():
+    return TestLoader().loadTestsFromName(__name__)

=== added file 'lib/lp/translations/browser/translationlinksaggregator.py'
--- lib/lp/translations/browser/translationlinksaggregator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/translationlinksaggregator.py	2009-08-25 10:26:28 +0000
@@ -0,0 +1,186 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+__all__ = [
+    'TranslationLinksAggregator',
+    ]
+
+from canonical.launchpad.webapp import canonical_url
+from lp.translations.interfaces.pofile import IPOFile
+from lp.translations.model.productserieslanguage import ProductSeriesLanguage
+
+
+class TranslationLinksAggregator:
+    """Aggregate `POFile`s and/or `POTemplate`s into meaningful targets.
+
+    Here, `POFile`s and `POTemplate`s are referred to collectively as
+    "sheets."
+    """
+
+    # Suffix to append to URL when linking to a POFile.
+    pofile_link_suffix = ''
+
+    def describe(self, target, link, covered_sheets):
+        """Overridable: return description of given translations link.
+
+        :param target: `Product` or `SourcePackage`.
+        :param link: URL linking to `covered_sheets` in the UI.
+        :param covered_sheets: `POFile`s and/or `POTemplate`s being
+            linked and described together.
+        :return: Some description that will get added to a list and
+            returned by `aggregate`.
+        """
+        raise NotImplementedError()
+
+    def _bundle(self, sheets):
+        """Bundle `sheets` based on target: `Product` or `SourcePackage`.
+
+        :param sheets: Sequence of `POFile`s and/or `POTemplate`s.
+        :return: Dict mapping each targets to a list representing its
+            `POFile`s and `POTemplate`s as found in `sheets`.
+        """
+        targets = {}
+        for sheet in sheets:
+            if IPOFile.providedBy(sheet):
+                template = sheet.potemplate
+            else:
+                template = sheet
+
+            if template.productseries:
+                target = template.productseries.product
+            else:
+                distroseries = template.distroseries
+                sourcepackagename = template.sourcepackagename
+                target = distroseries.getSourcePackage(sourcepackagename)
+
+            if target not in targets:
+                targets[target] = []
+
+            targets[target].append(sheet)
+
+        return targets
+
+    def _composeLink(self, sheet):
+        """Produce a link to a `POFile` or `POTemplate`."""
+        link = canonical_url(sheet)
+        if IPOFile.providedBy(sheet):
+            link += self.pofile_link_suffix
+
+        return link
+
+    def _getTemplate(self, sheet):
+        """Return `POTemplate` for `sheet`.
+
+        :param sheet: A `POTemplate` or `POFile`.
+        """
+        if IPOFile.providedBy(sheet):
+            return sheet.potemplate
+        else:
+            return sheet
+
+    def _getLanguage(self, sheet):
+        """Return language `sheet` is in, if `sheet` is an `IPOFile`."""
+        if IPOFile.providedBy(sheet):
+            return sheet.language
+        else:
+            return None
+
+    def _countLanguages(self, sheets):
+        """Count languages among `sheets`.
+
+        A template's language is None, which also counts.
+        """
+        return len(set(self._getLanguage(sheet) for sheet in sheets))
+
+    def _circumscribe(self, sheets):
+        """Find the best common UI link to cover all of `sheets`.
+
+        :param sheets: List of `POFile`s and/or `POTemplate`s.
+        :return: Dict containing a set of links and the respective lists
+            of `sheets` they cover.
+        """
+        first_sheet = sheets[0]
+        if len(sheets) == 1:
+            # Simple case: one sheet.
+            return {self._composeLink(first_sheet): sheets}
+
+        templates = set([self._getTemplate(sheet) for sheet in sheets])
+
+        productseries = set(
+            template.productseries
+            for template in templates
+            if template.productseries)
+
+        products = set(series.product for series in productseries)
+
+        sourcepackagenames = set(
+            template.sourcepackagename 
+            for template in templates
+            if template.sourcepackagename)
+
+        distroseries = set(
+            template.distroseries
+            for template in templates
+            if template.sourcepackagename)
+
+        assert len(products) <= 1, "Got more than one product."
+        assert len(sourcepackagenames) <= 1, "Got more than one package."
+        assert len(distroseries) <= 1, "Got more than one distroseries."
+        assert len(products) + len(sourcepackagenames) == 1, (
+            "Didn't get exactly one product or one package.")
+
+        first_template = self._getTemplate(first_sheet)
+
+        if len(templates) == 1:
+            # Multiple inputs, but all for the same template.  Link to
+            # the template.
+            return {self._composeLink(first_template): sheets}
+
+        if sourcepackagenames:
+            # Multiple inputs, but they have to be all for the same
+            # source package.  Show its template listing.
+            distroseries = first_template.distroseries
+            packagename = first_template.sourcepackagename
+            link = canonical_url(distroseries.getSourcePackage(packagename))
+            return {link: sheets}
+
+        if len(productseries) == 1:
+            # All for the same ProductSeries.
+            series = first_template.productseries
+            if self._countLanguages(sheets) == 1:
+                # All for the same language in the same ProductSeries,
+                # though still for different templates.  Link to
+                # ProductSeriesLanguage.
+                productserieslanguage = ProductSeriesLanguage(
+                    series, first_sheet.language)
+                return {canonical_url(productserieslanguage): sheets}
+            else:
+                # Multiple templates and languages in the same product
+                # series, or a mix of templates and at least one
+                # language.  Show the product series' templates listing.
+                return {canonical_url(series): sheets}
+
+        # Different release series of the same product.  Break down into
+        # individual sheets.  We could try recursing here to get a better
+        # set of aggregated links, but may not be worth the trouble.
+        return dict(
+            (self._composeLink(sheet), [sheet]) for sheet in sheets)
+
+    def aggregate(self, sheets):
+        """Aggregate `sheets` into a list of translation target descriptions.
+
+        Targets are aggregated into "sensible" chunks first.c
+        
+        :return: A list of whatever the implementation for `describe`
+            returns for the sensible chunks.
+        """
+        links = []
+        for target, sheets in self._bundle(sheets).iteritems():
+            assert sheets, "Translation target has no POFiles or templates."
+            links_and_sheets = self._circumscribe(sheets)
+            for link, covered_sheets in links_and_sheets.iteritems():
+                links.append(self.describe(target, link, covered_sheets))
+
+        return links