Launchpad itself

Merge lp:~adiroiban/launchpad/bug-564852 into lp:launchpad

  1. bug-564852
  2. Merge into devel
Proposed by Adi Roiban
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~adiroiban/launchpad/bug-564852
Merge into: lp:launchpad
Diff against target: 181 lines (+50/-20)
4 files modified
lib/lp/services/worlddata/doc/language.txt (+26/-7)
lib/lp/translations/browser/language.py (+9/-7)
lib/lp/translations/browser/tests/language-views.txt (+14/-5)
lib/lp/translations/templates/language-portlet-top-contributors.pt (+1/-1)
To merge this branch: bzr merge lp:~adiroiban/launchpad/bug-564852
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+23612@code.launchpad.net

Commit message

Sort the top language contributors according to their karma value. Greatest karma should be listed first.

Description of the change

= Bug 564852 =

Example: https://translations.launchpad.net/+languages/de

The list shows those people with the lowest karma instead of those with the highest. Not really important, but might be easy to fix if it’s just the ordering …

== Proposed fix ==

The list should contain contributors with greatest karma.

== Pre-implementation notes ==
Danilo added a comment on the bug report telling that it is OK if the list will contain less that 20 contributors and in this case we can use slicing to improve performance.

== Implementation details ==

Since we can no longer guarantee that the list will contain 20 items, I have removed all references to „top 20”.

From the SQL result, I'm getting the top 30 items so that in the case some top accounts are merged account of the rest top accounts there will increase the probability of having 20 items in the list.

I don't know how to fix the make lint import errors. Is there a wiki page explaining how to fix those errors?

== Tests ==
lp-test language-views

== Demo and Q/A ==

Since top language contributors depends on karma values that are generated using cronscript it is not easy to demo this feature.

= 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/templates/language-portlet-top-contributors.pt

== Pylint notices ==

lib/lp/translations/browser/language.py
    24: [F0401] Unable to import 'canonical.cachedproperty' (No module named canonical.cachedproperty)
    25: [F0401] Unable to import 'canonical.launchpad.interfaces.launchpad' (No module named canonical.launchpad.interfaces.launchpad)
    26: [F0401] Unable to import 'canonical.launchpad.webapp' (No module named canonical.launchpad.webapp)
    30: [F0401] Unable to import 'canonical.launchpad.webapp.breadcrumb' (No module named canonical.launchpad.webapp.breadcrumb)
    31: [F0401] Unable to import 'canonical.launchpad.webapp.tales' (No module named canonical.launchpad.webapp.tales)
    32: [F0401] Unable to import 'lp.services.worlddata.interfaces.language' (No module named lp.services.worlddata.interfaces.language)
    33: [F0401] Unable to import 'lp.translations.interfaces.translationsperson' (No module named lp.translations.interfaces.translationsperson)
    35: [F0401] Unable to import 'lp.translations.browser.translations' (No module named lp.translations.browser.translations)
    36: [F0401] Unable to import 'lp.translations.utilities.pluralforms' (No module named lp.translations.utilities.pluralforms)
    38: [F0401] Unable to import 'canonical.widgets' (No module named canonical.widgets)

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (4.5 KiB)

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(transla...

Read more...

review: Needs Fixing (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :
Download full text (3.5 KiB)

I am not using a set, since Language.translators is already sorted by karma (done in a single SQL query).
Using a set and then resorting it at the end will require a new SQL query for each person.

KarmaCache can not be updated from the „browser” layer and we need a unit test for services.worlddata.model.language.Language.translators running in LaunchpadZopelessLayer.

I have extended the current Language.translators karma text code to check that translators for a language are sorted according to their karma value.

Here is the latest diff:

=== modified file 'lib/lp/services/worlddata/doc/language.txt'
--- lib/lp/services/worlddata/doc/language.txt 2010-04-15 11:53:11 +0000
+++ lib/lp/services/worlddata/doc/language.txt 2010-04-21 00:06:25 +0000
@@ -273,8 +273,14 @@
 To be considered a translator, they must have done some translations and
 have the language among their preferred languages.

- >>> translator = factory.makePerson(name=u'serbian-translator')
- >>> translator.addLanguage(sr)
+ >>> translator_10 = factory.makePerson(name=u'serbian-translator-karma-10')
+ >>> translator_10.addLanguage(sr)
+ >>> translator_20 = factory.makePerson(name=u'serbian-translator-karma-20')
+ >>> translator_20.addLanguage(sr)
+ >>> translator_30 = factory.makePerson(name=u'serbian-translator-karma-30')
+ >>> translator_30.addLanguage(sr)
+ >>> translator_40 = factory.makePerson(name=u'serbian-translator-karma-40')
+ >>> translator_40.addLanguage(sr)
   >>> from canonical.testing import LaunchpadZopelessLayer
   >>> LaunchpadZopelessLayer.commit()

@@ -283,13 +289,26 @@
   >>> LaunchpadZopelessLayer.switchDbUser('karma')
   >>> translations_category = KarmaCategory.selectOne(
   ... KarmaCategory.name=='translations')
- >>> karma = KarmaCache(person=translator,
- ... category=translations_category,
- ... karmavalue=1)
+ >>> karma = KarmaCache(person=translator_30,
+ ... category=translations_category,
+ ... karmavalue=30)
+ >>> karma = KarmaCache(person=translator_10,
+ ... category=translations_category,
+ ... karmavalue=10)
+ >>> karma = KarmaCache(person=translator_20,
+ ... category=translations_category,
+ ... karmavalue=20)
+ >>> karma = KarmaCache(person=translator_40,
+ ... category=translations_category,
+ ... karmavalue=40)
   >>> LaunchpadZopelessLayer.commit()
   >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
- >>> [translator.name for translator in sr.translators]
- [u'serbian-translator']
+ >>> for translator in sr.translators:
+ ... print translator.name
+ serbian-translator-karma-40
+ serbian-translator-karma-30
+ serbian-translator-karma-20
+ serbian-translator-karma-10

 =========

=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
--- lib/lp/translations/browser/tests/language-views.txt 2010-04-17 17:40:26 +0000
+++ lib/lp/translations/browser/tests/language-views.txt 2010-04-20 23:53:54 +0000
@@ -106,7 +106,7 @@
     1 : 2, 3, 4, 22, 23, 24...
     2 : 0, 5, 6, 7, 8, 9...

-The t...

Read more...

Revision history for this message
Brad Crittenden (bac) wrote :

The diff looks good. Thanks for the nice reply and for discovering the daftness of my suggestion to use a set, which would've destroyed your sort order.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/worlddata/doc/language.txt'
2--- lib/lp/services/worlddata/doc/language.txt 2010-04-15 11:53:11 +0000
3+++ lib/lp/services/worlddata/doc/language.txt 2010-04-21 13:11:27 +0000
4@@ -273,8 +273,14 @@
5 To be considered a translator, they must have done some translations and
6 have the language among their preferred languages.
7
8- >>> translator = factory.makePerson(name=u'serbian-translator')
9- >>> translator.addLanguage(sr)
10+ >>> translator_10 = factory.makePerson(name=u'serbian-translator-karma-10')
11+ >>> translator_10.addLanguage(sr)
12+ >>> translator_20 = factory.makePerson(name=u'serbian-translator-karma-20')
13+ >>> translator_20.addLanguage(sr)
14+ >>> translator_30 = factory.makePerson(name=u'serbian-translator-karma-30')
15+ >>> translator_30.addLanguage(sr)
16+ >>> translator_40 = factory.makePerson(name=u'serbian-translator-karma-40')
17+ >>> translator_40.addLanguage(sr)
18 >>> from canonical.testing import LaunchpadZopelessLayer
19 >>> LaunchpadZopelessLayer.commit()
20
21@@ -283,13 +289,26 @@
22 >>> LaunchpadZopelessLayer.switchDbUser('karma')
23 >>> translations_category = KarmaCategory.selectOne(
24 ... KarmaCategory.name=='translations')
25- >>> karma = KarmaCache(person=translator,
26- ... category=translations_category,
27- ... karmavalue=1)
28+ >>> karma = KarmaCache(person=translator_30,
29+ ... category=translations_category,
30+ ... karmavalue=30)
31+ >>> karma = KarmaCache(person=translator_10,
32+ ... category=translations_category,
33+ ... karmavalue=10)
34+ >>> karma = KarmaCache(person=translator_20,
35+ ... category=translations_category,
36+ ... karmavalue=20)
37+ >>> karma = KarmaCache(person=translator_40,
38+ ... category=translations_category,
39+ ... karmavalue=40)
40 >>> LaunchpadZopelessLayer.commit()
41 >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
42- >>> [translator.name for translator in sr.translators]
43- [u'serbian-translator']
44+ >>> for translator in sr.translators:
45+ ... print translator.name
46+ serbian-translator-karma-40
47+ serbian-translator-karma-30
48+ serbian-translator-karma-20
49+ serbian-translator-karma-10
50
51
52 =========
53
54=== modified file 'lib/lp/translations/browser/language.py'
55--- lib/lp/translations/browser/language.py 2010-04-19 03:32:28 +0000
56+++ lib/lp/translations/browser/language.py 2010-04-21 13:11:27 +0000
57@@ -50,6 +50,7 @@
58
59 class LanguageBreadcrumb(Breadcrumb):
60 """`Breadcrumb` for `ILanguage`."""
61+
62 @property
63 def text(self):
64 return self.context.englishname
65@@ -97,6 +98,7 @@
66 title=u'Name of the language to search for.',
67 required=True)
68
69+
70 class LanguageSetView(LaunchpadFormView):
71 """View class to render main ILanguageSet page."""
72 label = "Languages in Launchpad"
73@@ -217,15 +219,15 @@
74 @property
75 def top_contributors(self):
76 """
77- Get the top 20 contributors for a language.
78+ Get the top contributors for a language.
79
80 If an account has been merged, the account into which it was
81 merged will be returned.
82 """
83- translators = []
84- for translator in reversed(list(self.context.translators)):
85+ top_translators = []
86+ for translator in self.context.translators[:30]:
87 # Get only the top 20 contributors
88- if (len(translators) >= 20):
89+ if (len(top_translators) >= 20):
90 break
91
92 # For merged account add the target account
93@@ -236,10 +238,10 @@
94
95 # Add translator only if it was not previouly added as a
96 # merged account
97- if translator_target not in translators:
98- translators.append(translator_target)
99+ if translator_target not in top_translators:
100+ top_translators.append(translator_target)
101
102- return translators
103+ return top_translators
104
105 @property
106 def friendly_plural_forms(self):
107
108=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
109--- lib/lp/translations/browser/tests/language-views.txt 2010-04-19 03:32:28 +0000
110+++ lib/lp/translations/browser/tests/language-views.txt 2010-04-21 13:11:27 +0000
111@@ -120,17 +120,13 @@
112 1 : 2, 3, 4, 22, 23, 24...
113 2 : 0, 5, 6, 7, 8, 9...
114
115-The top 20 contributors are listed on the language page, and for merged account
116+The top contributors are listed on the language page, and for a merged account
117 we will see their targed account.
118
119 Create some translators and a merged account.
120
121 >>> from zope.security.proxy import removeSecurityProxy
122 >>> translators = []
123- >>> for translator_nr in range(22):
124- ... translators.append(factory.makePerson(
125- ... name='translator-' + str(translator_nr),
126- ... displayname='Translator No.' + str(translator_nr)))
127 >>> translator_main = factory.makePerson(
128 ... name='translator-main',
129 ... displayname='Translator Main')
130@@ -140,6 +136,10 @@
131 ... displayname='Translator Merged'))
132 >>> translators.append(translator_merged)
133 >>> translator_merged.merged = translator_main
134+ >>> for translator_nr in range(22):
135+ ... translators.append(factory.makePerson(
136+ ... name='translator-' + str(translator_nr),
137+ ... displayname='Translator No.' + str(translator_nr)))
138
139 Create a product, a template with one msgset and a pofile
140
141@@ -161,6 +161,8 @@
142 ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)
143
144 Langauge.translators is Monkey-patched to avoid fetching KarmaCache.
145+Language.translator is a list containing all contributors decreasingly sorted
146+according to their karma value.
147
148 >>> serbian.translators = translators
149 >>> language_view = create_initialized_view(serbian, '+index',
150@@ -170,11 +172,18 @@
151 True
152 >>> translator_merged in top_contributors
153 False
154+ >>> for translator in top_contributors:
155+ ... print translator.name
156+ translator-main
157+ translator-0
158+ translator-1
159+ translator-2 ...
160
161 In the end, the changes done to Language class are reverted.
162
163 >>> Language.translators = translators_method
164
165+
166 View LanguageSet
167 ------------------
168
169
170=== modified file 'lib/lp/translations/templates/language-portlet-top-contributors.pt'
171--- lib/lp/translations/templates/language-portlet-top-contributors.pt 2010-03-03 13:04:00 +0000
172+++ lib/lp/translations/templates/language-portlet-top-contributors.pt 2010-04-21 13:11:27 +0000
173@@ -7,7 +7,7 @@
174 <h2>Top contributors</h2>
175 <div class="portletBody portletContent">
176 <p>
177- These are the top 20 translation contributors who have stated they have
178+ These are the top translation contributors who have stated they have
179 a particular interest in
180 <tal:language_name replace="view/language_name">
181 Espa&ntilde;ol