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
=== 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 13:11:27 +0000
@@ -273,8 +273,14 @@
273To be considered a translator, they must have done some translations and273To be considered a translator, they must have done some translations and
274have the language among their preferred languages.274have the language among their preferred languages.
275275
276 >>> translator = factory.makePerson(name=u'serbian-translator')276 >>> translator_10 = factory.makePerson(name=u'serbian-translator-karma-10')
277 >>> translator.addLanguage(sr)277 >>> translator_10.addLanguage(sr)
278 >>> translator_20 = factory.makePerson(name=u'serbian-translator-karma-20')
279 >>> translator_20.addLanguage(sr)
280 >>> translator_30 = factory.makePerson(name=u'serbian-translator-karma-30')
281 >>> translator_30.addLanguage(sr)
282 >>> translator_40 = factory.makePerson(name=u'serbian-translator-karma-40')
283 >>> translator_40.addLanguage(sr)
278 >>> from canonical.testing import LaunchpadZopelessLayer284 >>> from canonical.testing import LaunchpadZopelessLayer
279 >>> LaunchpadZopelessLayer.commit()285 >>> LaunchpadZopelessLayer.commit()
280286
@@ -283,13 +289,26 @@
283 >>> LaunchpadZopelessLayer.switchDbUser('karma')289 >>> LaunchpadZopelessLayer.switchDbUser('karma')
284 >>> translations_category = KarmaCategory.selectOne(290 >>> translations_category = KarmaCategory.selectOne(
285 ... KarmaCategory.name=='translations')291 ... KarmaCategory.name=='translations')
286 >>> karma = KarmaCache(person=translator,292 >>> karma = KarmaCache(person=translator_30,
287 ... category=translations_category,293 ... category=translations_category,
288 ... karmavalue=1)294 ... karmavalue=30)
295 >>> karma = KarmaCache(person=translator_10,
296 ... category=translations_category,
297 ... karmavalue=10)
298 >>> karma = KarmaCache(person=translator_20,
299 ... category=translations_category,
300 ... karmavalue=20)
301 >>> karma = KarmaCache(person=translator_40,
302 ... category=translations_category,
303 ... karmavalue=40)
289 >>> LaunchpadZopelessLayer.commit()304 >>> LaunchpadZopelessLayer.commit()
290 >>> LaunchpadZopelessLayer.switchDbUser('launchpad')305 >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
291 >>> [translator.name for translator in sr.translators]306 >>> for translator in sr.translators:
292 [u'serbian-translator']307 ... print translator.name
308 serbian-translator-karma-40
309 serbian-translator-karma-30
310 serbian-translator-karma-20
311 serbian-translator-karma-10
293312
294313
295=========314=========
296315
=== modified file 'lib/lp/translations/browser/language.py'
--- lib/lp/translations/browser/language.py 2010-04-19 03:32:28 +0000
+++ lib/lp/translations/browser/language.py 2010-04-21 13:11:27 +0000
@@ -50,6 +50,7 @@
5050
51class LanguageBreadcrumb(Breadcrumb):51class LanguageBreadcrumb(Breadcrumb):
52 """`Breadcrumb` for `ILanguage`."""52 """`Breadcrumb` for `ILanguage`."""
53
53 @property54 @property
54 def text(self):55 def text(self):
55 return self.context.englishname56 return self.context.englishname
@@ -97,6 +98,7 @@
97 title=u'Name of the language to search for.',98 title=u'Name of the language to search for.',
98 required=True)99 required=True)
99100
101
100class LanguageSetView(LaunchpadFormView):102class LanguageSetView(LaunchpadFormView):
101 """View class to render main ILanguageSet page."""103 """View class to render main ILanguageSet page."""
102 label = "Languages in Launchpad"104 label = "Languages in Launchpad"
@@ -217,15 +219,15 @@
217 @property219 @property
218 def top_contributors(self):220 def top_contributors(self):
219 """221 """
220 Get the top 20 contributors for a language.222 Get the top contributors for a language.
221223
222 If an account has been merged, the account into which it was224 If an account has been merged, the account into which it was
223 merged will be returned.225 merged will be returned.
224 """226 """
225 translators = []227 top_translators = []
226 for translator in reversed(list(self.context.translators)):228 for translator in self.context.translators[:30]:
227 # Get only the top 20 contributors229 # Get only the top 20 contributors
228 if (len(translators) >= 20):230 if (len(top_translators) >= 20):
229 break231 break
230232
231 # For merged account add the target account233 # For merged account add the target account
@@ -236,10 +238,10 @@
236238
237 # Add translator only if it was not previouly added as a239 # Add translator only if it was not previouly added as a
238 # merged account240 # merged account
239 if translator_target not in translators:241 if translator_target not in top_translators:
240 translators.append(translator_target)242 top_translators.append(translator_target)
241243
242 return translators244 return top_translators
243245
244 @property246 @property
245 def friendly_plural_forms(self):247 def friendly_plural_forms(self):
246248
=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
--- lib/lp/translations/browser/tests/language-views.txt 2010-04-19 03:32:28 +0000
+++ lib/lp/translations/browser/tests/language-views.txt 2010-04-21 13:11:27 +0000
@@ -120,17 +120,13 @@
120 1 : 2, 3, 4, 22, 23, 24...120 1 : 2, 3, 4, 22, 23, 24...
121 2 : 0, 5, 6, 7, 8, 9...121 2 : 0, 5, 6, 7, 8, 9...
122122
123The top 20 contributors are listed on the language page, and for merged account123The top contributors are listed on the language page, and for a merged account
124we will see their targed account.124we will see their targed account.
125125
126Create some translators and a merged account.126Create some translators and a merged account.
127127
128 >>> from zope.security.proxy import removeSecurityProxy128 >>> from zope.security.proxy import removeSecurityProxy
129 >>> translators = []129 >>> translators = []
130 >>> for translator_nr in range(22):
131 ... translators.append(factory.makePerson(
132 ... name='translator-' + str(translator_nr),
133 ... displayname='Translator No.' + str(translator_nr)))
134 >>> translator_main = factory.makePerson(130 >>> translator_main = factory.makePerson(
135 ... name='translator-main',131 ... name='translator-main',
136 ... displayname='Translator Main')132 ... displayname='Translator Main')
@@ -140,6 +136,10 @@
140 ... displayname='Translator Merged'))136 ... displayname='Translator Merged'))
141 >>> translators.append(translator_merged)137 >>> translators.append(translator_merged)
142 >>> translator_merged.merged = translator_main138 >>> translator_merged.merged = translator_main
139 >>> for translator_nr in range(22):
140 ... translators.append(factory.makePerson(
141 ... name='translator-' + str(translator_nr),
142 ... displayname='Translator No.' + str(translator_nr)))
143143
144Create a product, a template with one msgset and a pofile144Create a product, a template with one msgset and a pofile
145145
@@ -161,6 +161,8 @@
161 ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)161 ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)
162162
163Langauge.translators is Monkey-patched to avoid fetching KarmaCache.163Langauge.translators is Monkey-patched to avoid fetching KarmaCache.
164Language.translator is a list containing all contributors decreasingly sorted
165according to their karma value.
164166
165 >>> serbian.translators = translators167 >>> serbian.translators = translators
166 >>> language_view = create_initialized_view(serbian, '+index',168 >>> language_view = create_initialized_view(serbian, '+index',
@@ -170,11 +172,18 @@
170 True172 True
171 >>> translator_merged in top_contributors173 >>> translator_merged in top_contributors
172 False174 False
175 >>> for translator in top_contributors:
176 ... print translator.name
177 translator-main
178 translator-0
179 translator-1
180 translator-2 ...
173181
174In the end, the changes done to Language class are reverted.182In the end, the changes done to Language class are reverted.
175183
176 >>> Language.translators = translators_method184 >>> Language.translators = translators_method
177185
186
178View LanguageSet187View LanguageSet
179------------------188------------------
180189
181190
=== modified file 'lib/lp/translations/templates/language-portlet-top-contributors.pt'
--- lib/lp/translations/templates/language-portlet-top-contributors.pt 2010-03-03 13:04:00 +0000
+++ lib/lp/translations/templates/language-portlet-top-contributors.pt 2010-04-21 13:11:27 +0000
@@ -7,7 +7,7 @@
7 <h2>Top contributors</h2>7 <h2>Top contributors</h2>
8 <div class="portletBody portletContent">8 <div class="portletBody portletContent">
9 <p>9 <p>
10 These are the top 20 translation contributors who have stated they have10 These are the top translation contributors who have stated they have
11 a particular interest in11 a particular interest in
12 <tal:language_name replace="view/language_name">12 <tal:language_name replace="view/language_name">
13 Espa&ntilde;ol13 Espa&ntilde;ol