Launchpad itself

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

  1. bug-512307
  2. Merge into devel
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:~adiroiban/launchpad/bug-512307
Merge into: lp: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:~adiroiban/launchpad/bug-512307
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+20442@code.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
=== 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-16 01:06:32 +0000
@@ -34,7 +34,6 @@
34 ITranslationsPerson)34 ITranslationsPerson)
35from lp.translations.browser.translations import TranslationsMixin35from lp.translations.browser.translations import TranslationsMixin
36from lp.translations.utilities.pluralforms import make_friendly_plural_forms36from lp.translations.utilities.pluralforms import make_friendly_plural_forms
37from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
3837
39from canonical.widgets import LabeledMultiCheckBoxWidget38from canonical.widgets import LabeledMultiCheckBoxWidget
4039
@@ -214,8 +213,32 @@
214 })213 })
215 return translation_teams214 return translation_teams
216215
217 def getTopContributors(self):216 @property
218 return self.context.translators[:20]217 def top_contributors(self):
218 """
219 Get the top 20 contributors for a language.
220
221 If an account has been merged, the account into which it was
222 merged will be returned.
223 """
224 translators = []
225 for translator in reversed(list(self.context.translators)):
226 # Get only the top 20 contributors
227 if (len(translators) >= 20):
228 break
229
230 # For merged account add the target account
231 if translator.merged != None:
232 translator_target = translator.merged
233 else:
234 translator_target = translator
235
236 # Add translator only if it was not previouly added as a
237 # merged account
238 if translator_target not in translators:
239 translators.append(translator_target)
240
241 return translators
219242
220 @property243 @property
221 def friendly_plural_forms(self):244 def friendly_plural_forms(self):
222245
=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
--- lib/lp/translations/browser/tests/language-views.txt 2010-03-05 13:50:29 +0000
+++ lib/lp/translations/browser/tests/language-views.txt 2010-03-16 01:06:32 +0000
@@ -1,8 +1,8 @@
1Language Views1Language Views
2==================2==============
33
4Admin Language4Admin Language
5------------------5--------------
66
7 >>> from zope.component import getUtility7 >>> from zope.component import getUtility
8 >>> from lp.services.worlddata.interfaces.language import ILanguageSet8 >>> from lp.services.worlddata.interfaces.language import ILanguageSet
@@ -34,7 +34,7 @@
3434
3535
36Add Language36Add Language
37------------------37------------
3838
39 >>> language_add_view = create_view(language_set, '+add',39 >>> language_add_view = create_view(language_set, '+add',
40 ... layer=TranslationsLayer)40 ... layer=TranslationsLayer)
@@ -77,13 +77,24 @@
7777
7878
79View Language79View Language
80------------------80-------------
8181
82Translators lists for each language are computed using KarmaCache tables for
83users which have configured their prefered language. Since KarmaCache tables
84are generated using nightly builds, we will change Langauge.translators to
85use the list of translators generated by this test.
86
87Language.translators is deleted so that we can add our custom
88implementation later.
89
90 >>> from lp.services.worlddata.model.language import Language
91 >>> translators_method = Language.translators
92 >>> del Language.translators
82 >>> serbian = language_set.getLanguageByCode('sr')93 >>> serbian = language_set.getLanguageByCode('sr')
83 >>> language_view = create_initialized_view(serbian, '+index',94 >>> language_view = create_initialized_view(serbian, '+index',
84 ... layer=TranslationsLayer)95 ... layer=TranslationsLayer)
8596
86The friendlypluralforms function shows us a list of plural forms and97The 'friendly_plural_forms' function shows us a list of plural forms and
87a set of examples for each one, so one won't need to understand the plural98a set of examples for each one, so one won't need to understand the plural
88formula expression to see how it works.99formula expression to see how it works.
89100
@@ -95,6 +106,61 @@
95 1 : 2, 3, 4, 22, 23, 24...106 1 : 2, 3, 4, 22, 23, 24...
96 2 : 0, 5, 6, 7, 8, 9...107 2 : 0, 5, 6, 7, 8, 9...
97108
109The top 20 contributors are listed on the language page, and for merged account
110we will see their targed account.
111
112Create some translators and a merged account.
113
114 >>> from zope.security.proxy import removeSecurityProxy
115 >>> translators = []
116 >>> for translator_nr in range(22):
117 ... translators.append(factory.makePerson(
118 ... name='translator-' + str(translator_nr),
119 ... displayname='Translator No.' + str(translator_nr)))
120 >>> translator_main = factory.makePerson(
121 ... name='translator-main',
122 ... displayname='Translator Main')
123 >>> translators.append(translator_main)
124 >>> translator_merged = removeSecurityProxy(factory.makePerson(
125 ... name='translator-merged',
126 ... displayname='Translator Merged'))
127 >>> translators.append(translator_merged)
128 >>> translator_merged.merged = translator_main
129
130Create a product, a template with one msgset and a pofile
131
132 >>> product = factory.makeProduct(official_rosetta=True)
133 >>> template = factory.makePOTemplate(
134 ... productseries=product.getSeries('trunk'))
135 >>> potmsgset = factory.makePOTMsgSet(template)
136 >>> pofile = factory.makePOFile('sr', potemplate=template)
137
138Add a translation for each translator and one more for main and merged
139accounts.
140
141 >>> for translator in translators:
142 ... translation = factory.makeTranslationMessage(
143 ... pofile=pofile, translator=translator, potmsgset=potmsgset)
144 >>> translation = factory.makeTranslationMessage(
145 ... pofile=pofile, translator=translator_merged, potmsgset=potmsgset)
146 >>> translation = factory.makeTranslationMessage(
147 ... pofile=pofile, translator=translator_main, potmsgset=potmsgset)
148
149Langauge.translators is Monkey-patched to avoid fetching KarmaCache.
150
151 >>> serbian.translators = translators
152 >>> language_view = create_initialized_view(serbian, '+index',
153 ... layer=TranslationsLayer)
154 >>> top_contributors = language_view.top_contributors
155 >>> translator_main in top_contributors
156 True
157 >>> translator_merged in top_contributors
158 False
159
160In the end, the changes done to Language class are reverted.
161
162 >>> Language.translators = translators_method
163
98View LanguageSet164View LanguageSet
99------------------165------------------
100166
@@ -110,7 +176,7 @@
110 <a href=".../en" ...>English</a>,176 <a href=".../en" ...>English</a>,
111 <a href=".../es" ...>Spanish</a>177 <a href=".../es" ...>Spanish</a>
112178
113For a user without any preferred languages, English will be returned. 179For a user without any preferred languages, English will be returned.
114180
115 >>> person = factory.makePerson()181 >>> person = factory.makePerson()
116 >>> print person.languages182 >>> print person.languages
117183
=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-details.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-details.txt 2010-03-11 20:54:36 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-details.txt 2010-03-16 01:06:32 +0000
@@ -151,10 +151,8 @@
151 >>> language_code = 'es'151 >>> language_code = 'es'
152 >>> pofile = factory.makePOFile(language_code, potemplate=template)152 >>> pofile = factory.makePOFile(language_code, potemplate=template)
153 >>> potmsgset = factory.makePOTMsgSet(template)153 >>> potmsgset = factory.makePOTMsgSet(template)
154 >>> potmsgset.setSequence(template, 1)
155 >>> translation = factory.makeTranslationMessage(pofile=pofile,154 >>> translation = factory.makeTranslationMessage(pofile=pofile,
156 ... translator=merged_translator, potmsgset=potmsgset)155 ... translator=merged_translator, potmsgset=potmsgset)
157 >>> potmsgset.setSequence(template, 2)
158 >>> translation = factory.makeTranslationMessage(pofile=pofile,156 >>> translation = factory.makeTranslationMessage(pofile=pofile,
159 ... translator=translator, potmsgset=potmsgset)157 ... translator=translator, potmsgset=potmsgset)
160 >>> from zope.security.proxy import removeSecurityProxy158 >>> from zope.security.proxy import removeSecurityProxy
@@ -165,10 +163,13 @@
165 ... ("http://translations.launchpad.dev/"163 ... ("http://translations.launchpad.dev/"
166 ... "ubuntu/hoary/+source/%s/+pots/%s/%s/+details") % (164 ... "ubuntu/hoary/+source/%s/+pots/%s/%s/+details") % (
167 ... package.name, template.name, language_code))165 ... package.name, template.name, language_code))
168 >>> print extract_text(find_main_content(browser.contents))166 >>> main_text = extract_text(find_main_content(browser.contents))
167 >>> print main_text
169 Details for ...168 Details for ...
170 Contributors to this translation169 Contributors to this translation
171 The following people have made some contribution to this specific170 The following people have made some contribution to this specific
172 translation:171 translation:
173 Poly Glot (filter)172 Poly Glot (filter)
174173
174 >>> u'Mere Pere' in main_text
175 False
175176
=== 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-16 01:06:32 +0000
@@ -3,7 +3,7 @@
3 xmlns:metal="http://xml.zope.org/namespaces/metal"3 xmlns:metal="http://xml.zope.org/namespaces/metal"
4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
5 class="portlet" id="portlet-top-contributors"5 class="portlet" id="portlet-top-contributors"
6 tal:condition="view/getTopContributors">6 tal:condition="view/top_contributors">
7 <h2>Top contributors</h2>7 <h2>Top contributors</h2>
8 <div class="portletBody portletContent">8 <div class="portletBody portletContent">
9 <p>9 <p>
@@ -14,7 +14,7 @@
14 </tal:language_name>:14 </tal:language_name>:
15 </p>15 </p>
16 <ul>16 <ul>
17 <li tal:repeat="contributor view/getTopContributors">17 <li tal:repeat="contributor view/top_contributors">
18 <a tal:replace="structure contributor/fmt:link">Guilherme Salgado</a>18 <a tal:replace="structure contributor/fmt:link">Guilherme Salgado</a>
19 </li>19 </li>
20 </ul>20 </ul>
2121
=== 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-16 01:06:32 +0000
@@ -41,14 +41,16 @@
41 </p>41 </p>
4242
43 <ul tal:condition="view/contributors">43 <ul tal:condition="view/contributors">
44 <li tal:repeat="contributor view/contributors">44 <tal:contributors-loop repeat="contributor view/contributors">
45 <tal:not-merged condition="not: contributor/merged">45 <li tal:condition="not: contributor/merged">
46 <a tal:replace="structure contributor/fmt:link" />46 <a tal:replace="structure contributor/fmt:link">
47 Mister Potato
48 </a>
47 (<a tal:attributes="49 (<a tal:attributes="
48 href string:${context/fmt:url}/+filter?person=${contributor/name}"50 href string:${context/fmt:url}/+filter?person=${contributor/name}"
49 >filter</a>)51 >filter</a>)
50 </tal:not-merged>
51 </li>52 </li>
53 </tal:contributors-loop>
52 </ul>54 </ul>
5355
54 <p56 <p