Merge into devel : bug-564852 : Code : Launchpad itself

Status:

Merged

Approved by:

Brad Crittenden on 2010-04-21

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp://staging/~adiroiban/launchpad/bug-564852

Merge into:

lp://staging/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://staging/~adiroiban/launchpad/bug-564852

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2010-04-17	Approve on 2010-04-21
Review via email: mp+23612@code.staging.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)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-20:

#

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

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

review: Needs Fixing (code)

Revision history for this message

Adi Roiban (adiroiban) wrote on 2010-04-21:

#

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

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 top contributors are listed on the language page, and for merged account
+The top contributors are listed on the language page, and for a merged account
 we will see their targed account.
 
 Create some translators and a merged account.
@@ -169,6 +169,7 @@
 
     >>> Language.translators = translators_method
 
+
 View LanguageSet
 ------------------

Revision history for this message

Brad Crittenden (bac) wrote on 2010-04-21:

#

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)

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers