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

Hi Adi,

Nice branch. I've got a couple of comments - just fixes needed in the doctest narrative - other than that this is fine. Please ping me when you've made the necessary changes and I'll EC2 it.

> === modified file 'lib/lp/translations/browser/language.py'
> --- lib/lp/translations/browser/language.py 2009-12-08 10:20:37 +0000
> +++ lib/lp/translations/browser/language.py 2010-03-02 11:04:22 +0000
> @@ -34,7 +34,6 @@
> ITranslationsPerson)
> from lp.translations.browser.translations import TranslationsMixin
> from lp.translations.utilities.pluralforms import make_friendly_plural_forms
> -from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
>
> from canonical.widgets import LabeledMultiCheckBoxWidget
>
> @@ -214,8 +213,31 @@
> })
> return translation_teams
>
> - def getTopContributors(self):
> - return self.context.translators[:20]
> + @property
> + def top_contributors(self):
> + """
> + Get top 20 contributors for a language.

s/Get/Get the

> +
> + Instead of merged account, show the merged target account.

This would read better as: "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)):
> + # Get only the top 20 contributors
> + if (len(translators) >= 20):
> + break
> +
> + # For merged account add the target account
> + if translator.merged != None:
> + translator_target = translator.merged
> + else:
> + translator_target = translator
> +
> + # Add translator only if it was not previouly added as a
> + # merged account
> + if translator_target not in translators:
> + translators.append(translator_target)
> +
> + return translators
>
> @property
> def friendly_plural_forms(self):
>
> === modified file 'lib/lp/translations/browser/tests/language-views.txt'
> --- lib/lp/translations/browser/tests/language-views.txt 2009-12-03 15:14:55 +0000
> +++ lib/lp/translations/browser/tests/language-views.txt 2010-03-02 11:04:22 +0000
> @@ -1,8 +1,8 @@
> Language Views
> -==================
> +==============
>
> Admin Language
> -------------------
> +--------------
>
> >>> from zope.component import getUtility
> >>> from lp.services.worlddata.interfaces.language import ILanguageSet
> @@ -34,7 +34,7 @@
>
>
> Add Language
> -------------------
> +------------
>
> >>> language_add_view = create_view(language_set, '+add',
> ... layer=TranslationsLayer)
> @@ -77,13 +77,22 @@
>
>
> View Language
> -------------------
> -
> +-------------
> +
> +Translators list for each language is computed using KarmaCache table for

s/list/lists ; s/is computed/are computed ; s/table/tables.

> +users which have configured their prefered language. Since KarmaCache tables
> +are generated using nightly builds, we will change Langauge.translators to
> +use the list of translators generated by this test.
> +
> + >>> from lp.services.worlddata.model.language import Language
> + >>> # Delete Language.translators so that we can add our custom
> + >>> # implementation later.
> + >>> del Language.translators
> >>> serbian = language_set.getLanguageByCode('sr')
> >>> language_view = create_initialized_view(serbian, '+index',
> ... layer=TranslationsLayer)
>
> -The friendlypluralforms function shows us a list of plural forms and
> +The 'friendly_plural_forms' function shows us a list of plural forms and
> a set of examples for each one, so one won't need to understand the plural
> formula expression to see how it works.
>
> @@ -95,6 +104,55 @@
> 1 : 2, 3, 4, 22, 23, 24...
> 2 : 5, 6, 7, 8, 9, 10...
>
> +Top 20 contributors are listed on the language page, and for merged account

s/Top/The top/g

> +we will see their targed account.
> +
> +Create some translators and a merged account.
> + >>> 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')
> + >>> translators.append(translator_main)
> + >>> translator_merged = factory.makePerson(
> + ... name='translator-merged',
> + ... displayname='Translator Merged')
> + >>> translators.append(translator_merged)
> + >>> translator_merged.merged = translator_main
> +
> +Create a product, a template with one msgset and a pofile
> +
> + >>> product = factory.makeProduct(official_rosetta=True)
> + >>> template = factory.makePOTemplate(
> + ... productseries=product.getSeries('trunk'))
> + >>> potmsgset = factory.makePOTMsgSet(template)
> + >>> pofile = factory.makePOFile('sr', potemplate=template)
> +
> +Add a translation for each translator and one more for main and merged
> +accounts.
> +
> + >>> for translator in translators:
> + ... translation = factory.makeTranslationMessage(
> + ... pofile=pofile, translator=translator, potmsgset=potmsgset)
> + >>> translation = factory.makeTranslationMessage(
> + ... pofile=pofile, translator=translator_merged, potmsgset=potmsgset)
> + >>> translation = factory.makeTranslationMessage(
> + ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)
> +
> + >>> # Monkey-patch Langauge.translators to avoid fetching KarmaCache
> + >>> serbian.translators = translators
> + >>> language_view = create_initialized_view(serbian, '+index',
> + ... layer=TranslationsLayer)
> + >>> top_contributors = language_view.top_contributors
> + >>> translator_main in top_contributors
> + True
> + >>> translator_merged in top_contributors
> + False
> +
> +
> View LanguageSet
> ------------------
>
> @@ -110,7 +168,7 @@
> <a href=".../en" ...>English</a>,
> <a href=".../es" ...>Spanish</a>
>
> -For a user without any preferred languages, English will be returned.
> +For a user without any preferred languages, English will be returned.
>
> >>> person = factory.makePerson()
> >>> print person.languages
>
> === modified file 'lib/lp/translations/stories/standalone/xx-pofile-details.txt'
> --- lib/lp/translations/stories/standalone/xx-pofile-details.txt 2009-12-15 14:11:39 +0000
> +++ lib/lp/translations/stories/standalone/xx-pofile-details.txt 2010-03-02 11:04:22 +0000
> @@ -151,10 +151,8 @@
> >>> language_code = 'es'
> >>> pofile = factory.makePOFile(language_code, potemplate=template)
> >>> potmsgset = factory.makePOTMsgSet(template)
> - >>> potmsgset.setSequence(template, 1)
> >>> translation = factory.makeTranslationMessage(pofile=pofile,
> ... translator=merged_translator, potmsgset=potmsgset)
> - >>> potmsgset.setSequence(template, 2)
> >>> translation = factory.makeTranslationMessage(pofile=pofile,
> ... translator=translator, potmsgset=potmsgset)
> >>> merged_translator.merged = translator
> @@ -164,10 +162,13 @@
> ... ("http://translations.launchpad.dev/"
> ... "ubuntu/hoary/+source/%s/+pots/%s/%s/+details") % (
> ... package.name, template.name, language_code))
> - >>> print extract_text(find_main_content(browser.contents))
> + >>> main_text = extract_text(find_main_content(browser.contents))
> + >>> print main_text
> Details for ...
> Contributors to this translation
> The following people have made some contribution to this specific
> translation:
> Poly Glot (filter)
>
> + >>> u'Mere Pere' in main_text
> + False
>
> === modified file 'lib/lp/translations/templates/language-portlet-top-contributors.pt'
> --- lib/lp/translations/templates/language-portlet-top-contributors.pt 2009-08-28 15:40:27 +0000
> +++ lib/lp/translations/templates/language-portlet-top-contributors.pt 2010-03-02 11:04:22 +0000
> @@ -14,7 +14,7 @@
> </tal:language_name>:
> </p>
> <ul>
> - <li tal:repeat="contributor view/getTopContributors">
> + <li tal:repeat="contributor view/top_contributors">
> <a tal:replace="structure contributor/fmt:link">Guilherme Salgado</a>
> </li>
> </ul>
>
> === modified file 'lib/lp/translations/templates/pofile-details.pt'
> --- lib/lp/translations/templates/pofile-details.pt 2010-01-20 22:31:50 +0000
> +++ lib/lp/translations/templates/pofile-details.pt 2010-03-02 11:04:22 +0000
> @@ -41,14 +41,16 @@
> </p>
>
> <ul tal:condition="view/contributors">
> - <li tal:repeat="contributor view/contributors">
> - <tal:not-merged condition="not: contributor/merged">
> - <a tal:replace="structure contributor/fmt:link" />
> + <tal:contributors-loop repeat="contributor view/contributors">
> + <li tal:condition="not: contributor/merged">
> + <a tal:replace="structure contributor/fmt:link">
> + Mister Potato
> + </a>
> (<a tal:attributes="
> href string:${context/fmt:url}/+filter?person=${contributor/name}"
> >filter</a>)
> - </tal:not-merged>
> </li>
> + </tal:contributors-loop>
> </ul>
>
> <p
>