Merge lp://staging/~adiroiban/launchpad/bug-512307 into lp://staging/launchpad

Proposed by Adi Roiban
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~adiroiban/launchpad/bug-512307
Merge into: lp://staging/launchpad
Diff against target: 246 lines (+111/-19)
5 files modified
lib/lp/translations/browser/language.py (+26/-3)
lib/lp/translations/browser/tests/language-views.txt (+73/-7)
lib/lp/translations/stories/standalone/xx-pofile-details.txt (+4/-3)
lib/lp/translations/templates/language-portlet-top-contributors.pt (+2/-2)
lib/lp/translations/templates/pofile-details.pt (+6/-4)
To merge this branch: bzr merge lp://staging/~adiroiban/launchpad/bug-512307
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+20442@code.staging.launchpad.net

Commit message

Filter merge accounts from overall language contributors.

To post a comment you must log in.
Revision history for this message
Adi Roiban (adiroiban) wrote :

= Bug 512307=
On https://translations.launchpad.net/+languages/st we can see that Leonardo Gregianin appears twice, one with this current account (leogregianin) and an old merged one, (leogregianin-merged.

We should not show merged accounts.

In bug 121520 , we fixed some placed where merged accounts were displayed, but missed language-portlet-top-contributors.pt

== Proposed fix ==
Only show top 20 contributors and if we have a merged account in the top, replace it with the target account.

== Pre-implementation notes ==
My initial proposed fix was to just filter the merge account, but since by doing this we will no longer have a top 20, Jeroen asked to replace merged account with they target.

== Implementation details ==
Top contributors for a language, depends on the full contributors list and this is generated by cronjobs that compute the KarmaCache tables. I talked with Danilo and for writing the tests he suggest to fake the generation of language contributors.

In lib/lp/translations/templates/pofile-details.pt I have done a small cleanup for my fix for bug 121520, since for merged account it generates empty <li> tags.

== Tests ==
lp-test -t language-views -t pofile-details

== Demo and Q/A ==
Since overall language statistics depends on KarmaCache and user preferences it is hard to test it on a local branch.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/translations/browser/language.py
  lib/lp/translations/browser/tests/language-views.txt
  lib/lp/translations/stories/standalone/xx-pofile-details.txt
  lib/lp/translations/templates/language-portlet-top-contributors.pt
  lib/lp/translations/templates/pofile-details.pt

Revision history for this message
Graham Binns (gmb) wrote :
Download full text (9.4 KiB)

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.
> +
> + >>> ...

Read more...

review: Approve (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :

Thanks for the review.

I have fixed and pushed the pagetest comments.

Here is the diff.
=== modified file 'lib/lp/translations/browser/language.py'
--- lib/lp/translations/browser/language.py 2010-03-02 11:02:08 +0000
+++ lib/lp/translations/browser/language.py 2010-03-02 12:03:30 +0000
@@ -216,9 +216,10 @@
     @property
     def top_contributors(self):
         """
- Get top 20 contributors for a language.
+ Get the top 20 contributors for a language.

- Instead of merged account, show the merged target account.
+ 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)):

=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
--- lib/lp/translations/browser/tests/language-views.txt 2010-03-02 10:16:13 +0000
+++ lib/lp/translations/browser/tests/language-views.txt 2010-03-02 12:04:45 +0000
@@ -79,7 +79,7 @@
 View Language
 -------------

-Translators list for each language is computed using KarmaCache table for
+Translators lists for each language are computed using KarmaCache tables for
 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.
@@ -104,7 +104,7 @@
     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
+The top 20 contributors are listed on the language page, and for merged account
 we will see their targed account.

 Create some translators and a merged account.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.