Code review comment for lp:~jtv/launchpad/bug-416434-aggregator

= 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 the TranslationLinksAggregator. There will also
be a link to a full list of just the most relevant ones; same story
there.

If that person is not you, append /+translations-to-review to the URL.
I've got a separate branch (being reviewed as I write this) that makes
it a bit more accessible.

== Lint ==

I seem to have messed up my RocketFuel setup. Now Pylint complains that
it can't import a lot of stuff I didn't touch: "no module named lp" and
such. The rest of the checks in "make lint" are happy with the code.

One other complaint from Pylint only is that my test classes use "super"
to call TestCaseWithFactory.setUp. It says "Use super on an old style
class," which I guess really means "Don't use super on an old style
class." I started noticing this around the time I messed up my RF, so
I can't run tests locally now and am too scared to change it.

Jeroen