Code review comment for lp:~adiroiban/launchpad/bug-564852

Hi Adi,

Thanks for the branch. I have a few minor corrections and a request for better test coverage.

When this branch is complete I'll be happy to land it for you.

Thanks,

Brad

> === modified file 'lib/lp/translations/browser/language.py'
> --- lib/lp/translations/browser/language.py 2010-03-02 12:04:59 +0000
> @@ -216,15 +218,15 @@
> @property
> def top_contributors(self):
> """
> - Get the top 20 contributors for a language.
> + Get the top contributors for a language.
>
> If an account has been merged, the account into which it was
> merged will be returned.
> """
> - translators = []
> - for translator in reversed(list(self.context.translators)):
> + top_translators = []

If you use a set...

> + for translator in self.context.translators[:30]:
> # Get only the top 20 contributors
> - if (len(translators) >= 20):
> + if (len(top_translators) >= 20):
> break
>
> # For merged account add the target account
> @@ -235,10 +237,10 @@
>
> # Add translator only if it was not previouly added as a
> # merged account
> - if translator_target not in translators:
> - translators.append(translator_target)
> + if translator_target not in top_translators:
> + top_translators.append(translator_target)

You can avoid the test here and just add it.

>
> - return translators
> + return top_translators
>
> @property
> def friendly_plural_forms(self):
> @@ -268,6 +270,7 @@
> view_name='+addquestion',
> rootsite='answers')
>

> === modified file 'lib/lp/translations/browser/tests/language-views.txt'
> --- lib/lp/translations/browser/tests/language-views.txt 2010-03-16 01:04:05 +0000
> +++ lib/lp/translations/browser/tests/language-views.txt 2010-04-17 17:46:16 +0000
> @@ -106,17 +106,13 @@
> 1 : 2, 3, 4, 22, 23, 24...
> 2 : 0, 5, 6, 7, 8, 9...
>
> -The top 20 contributors are listed on the language page, and for merged account
> +The top contributors are listed on the language page, and for
> merged account

typo: and for a merged

> we will see their targed account.
>
> Create some translators and a merged account.
>
> >>> from zope.security.proxy import removeSecurityProxy
> >>> translators = []
> - >>> for translator_nr in range(22):
> - ... translators.append(factory.makePerson(
> - ... name='translator-' + str(translator_nr),
> - ... displayname='Translator No.' + str(translator_nr)))
> >>> translator_main = factory.makePerson(
> ... name='translator-main',
> ... displayname='Translator Main')
> @@ -126,6 +122,10 @@
> ... displayname='Translator Merged'))
> >>> translators.append(translator_merged)
> >>> translator_merged.merged = translator_main
> + >>> for translator_nr in range(22):
> + ... translators.append(factory.makePerson(
> + ... name='translator-' + str(translator_nr),
> + ... displayname='Translator No.' + str(translator_nr)))
>
> Create a product, a template with one msgset and a pofile
>
> @@ -147,6 +147,8 @@
> ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)
>
> Langauge.translators is Monkey-patched to avoid fetching KarmaCache.
> +Language.translator is a list containing all contributors decreasingly sorted
> +according to their karma value.

How about: "a list containing all contributors sorted descending by
karma value."

>
> >>> serbian.translators = translators

I'm concerned about this test monkey patching away the karma, which is
the sort ordering which is the basis for the bug you are fixing. It
would be ok if you could refer to the test of the language model class
where the method is tested. Unfortunately looking at
lp/services/worlddata/doc/languages.txt shows that the sort order is
not tested.

Would you beef up that test by creating multiple persons with varying
amount of karma and demonstrating that the sort order is correct?

> >>> language_view = create_initialized_view(serbian, '+index',
> @@ -156,6 +158,12 @@
> True
> >>> translator_merged in top_contributors
> False
> + >>> for translator in top_contributors:
> + ... print translator.name
> + translator-main
> + translator-0
> + translator-1
> + translator-2 ...
>
> In the end, the changes done to Language class are reverted.
>