Merge lp:~jtv/launchpad/bug-488218 into lp:launchpad
- bug-488218
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Jeroen T. Vermeulen |
Approved revision: | not available |
Merged at revision: | not available |
Proposed branch: | lp:~jtv/launchpad/bug-488218 |
Merge into: | lp:launchpad |
Diff against target: |
325 lines (+105/-61) 7 files modified
lib/lp/translations/browser/person.py (+2/-0) lib/lp/translations/doc/pofile.txt (+39/-12) lib/lp/translations/model/pofile.py (+9/-2) lib/lp/translations/model/potmsgset.py (+14/-11) lib/lp/translations/stories/standalone/xx-translations-to-complete.txt (+25/-0) lib/lp/translations/tests/test_pofile.py (+1/-0) lib/lp/translations/tests/test_potmsgset.py (+15/-36) |
To merge this branch: | bzr merge lp:~jtv/launchpad/bug-488218 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Nelson (community) | code | Approve | |
Review via email: mp+15247@code.launchpad.net |
Commit message
Attribute "credits" translationmessages to rosetta_experts; never list rosetta_experts as contributor.
Description of the change
Jeroen T. Vermeulen (jtv) wrote : | # |
Michael Nelson (michael.nelson) wrote : | # |
Hi Jeroen,
Thanks for all the multiple drive-by's!
r=me, but two thoughts. The first is general - is there not a better way to handle
these translation credits so that they're not translatable messages
at all? I guess not, otherwise you would have done it.
Second thought: you said in your cover letter that "translations are made
by persons, not teams", but translations are still displayed for other
teams - just not the rosetta experts, is that right?
(eg. http://
understanding)
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -215,6 +215,8 @@
> @property
> def person_
> """Is this person active in translations?"""
> + if self.context.
> + return False
> person = ITranslationsPe
> history = person.
> return history is not None
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -1031,8 +1031,17 @@
> The 'contributors' property of a POFile returns all the people who contributed
> translations to it.
>
> - >>> [person.name for person in evolution_
> - [u'carlos', u'mark', u'no-priv']
> + >>> def print_names(
> + ... """Print name for each of `persons`."""
> + ... for person in persons:
> + ... print person.name
> + ... print "--"
> +
> + >>> print_names(
> + carlos
> + mark
> + no-priv
> + --
>
> If you have a distroseries and want to know all the people who contributed
> translations on a given language for that distroseries, you can use
> @@ -1042,9 +1051,16 @@
> >>> from lp.registry.
> >>> hoary = DistroSeries.
> >>> spanish = Language.
> - >>> [p.name for p in hoary.getPOFile
> - [u'jorge-
> - u'name12', u'ubuntu-
> + >>> print_names(
> + jorge-gonzalez-
> + carlos
> + valyag
> + danner
> + name16
> + name12
> + ubuntu-translators
> + tsukimi
> + --
>
> # We can see that there is another translator that doesn't appear in
> # previous list because the template he translated is not current.
> @@ -1052,8 +1068,9 @@
> >>> non_current_
> False
>
> - >>> [p.name for p in non_current_
> - [u'jordi']
> + >>> print_names(
> + jordi
> + --
>
> >>> non_current_
> True
> @@ -1061,6 +1078,17 @@
> >>> non_current_
> True...
Jeroen T. Vermeulen (jtv) wrote : | # |
Thanks for the review!
> r=me, but two thoughts. The first is general - is there not a better way to
> handle
> these translation credits so that they're not translatable messages
> at all? I guess not, otherwise you would have done it.
Right. It's actually an elegant solution in the problem domain that these are in all respects translatable messages. Another way of looking at it is that we, as an implementation, just happen to automate their translation.
> Second thought: you said in your cover letter that "translations are made
> by persons, not teams", but translations are still displayed for other
> teams - just not the rosetta experts, is that right?
> (eg. http://
> understanding)
Yes. One of the tests also showed translations made by another team, though I don't know how it was supposed to arrive at a situation where those can exist. One way I could think of, though, is by changing a person into a team. I believe that's possible, though I haven't checked.
Functionally, there's nothing wrong with crediting team contributions if they should exist. It's really just in the case of rosetta_experts that listing the team as a contributor becomes totally meaningless.
Jeroen
Preview Diff
1 | === modified file 'lib/lp/translations/browser/person.py' |
2 | --- lib/lp/translations/browser/person.py 2009-09-17 20:11:48 +0000 |
3 | +++ lib/lp/translations/browser/person.py 2009-11-25 17:00:32 +0000 |
4 | @@ -215,6 +215,8 @@ |
5 | @property |
6 | def person_is_translator(self): |
7 | """Is this person active in translations?""" |
8 | + if self.context.isTeam(): |
9 | + return False |
10 | person = ITranslationsPerson(self.context) |
11 | history = person.getTranslationHistory(self.history_horizon).any() |
12 | return history is not None |
13 | |
14 | === modified file 'lib/lp/translations/doc/pofile.txt' |
15 | --- lib/lp/translations/doc/pofile.txt 2009-10-29 17:46:00 +0000 |
16 | +++ lib/lp/translations/doc/pofile.txt 2009-11-25 17:00:32 +0000 |
17 | @@ -1031,8 +1031,17 @@ |
18 | The 'contributors' property of a POFile returns all the people who contributed |
19 | translations to it. |
20 | |
21 | - >>> [person.name for person in evolution_es.contributors] |
22 | - [u'carlos', u'mark', u'no-priv'] |
23 | + >>> def print_names(persons): |
24 | + ... """Print name for each of `persons`.""" |
25 | + ... for person in persons: |
26 | + ... print person.name |
27 | + ... print "--" |
28 | + |
29 | + >>> print_names(evolution_es.contributors) |
30 | + carlos |
31 | + mark |
32 | + no-priv |
33 | + -- |
34 | |
35 | If you have a distroseries and want to know all the people who contributed |
36 | translations on a given language for that distroseries, you can use |
37 | @@ -1042,9 +1051,16 @@ |
38 | >>> from lp.registry.model.distroseries import DistroSeries |
39 | >>> hoary = DistroSeries.selectOneBy(name="hoary") |
40 | >>> spanish = Language.selectOneBy(code="es") |
41 | - >>> [p.name for p in hoary.getPOFileContributorsByLanguage(spanish)] |
42 | - [u'jorge-gonzalez-gonzalez', u'carlos', u'valyag', u'danner', u'name16', |
43 | - u'name12', u'ubuntu-translators', u'tsukimi'] |
44 | + >>> print_names(hoary.getPOFileContributorsByLanguage(spanish)) |
45 | + jorge-gonzalez-gonzalez |
46 | + carlos |
47 | + valyag |
48 | + danner |
49 | + name16 |
50 | + name12 |
51 | + ubuntu-translators |
52 | + tsukimi |
53 | + -- |
54 | |
55 | # We can see that there is another translator that doesn't appear in |
56 | # previous list because the template he translated is not current. |
57 | @@ -1052,8 +1068,9 @@ |
58 | >>> non_current_pofile.potemplate.iscurrent |
59 | False |
60 | |
61 | - >>> [p.name for p in non_current_pofile.contributors] |
62 | - [u'jordi'] |
63 | + >>> print_names(non_current_pofile.contributors) |
64 | + jordi |
65 | + -- |
66 | |
67 | >>> non_current_pofile.potemplate.distroseries == hoary |
68 | True |
69 | @@ -1061,6 +1078,17 @@ |
70 | >>> non_current_pofile.language == spanish |
71 | True |
72 | |
73 | +The rosetta_experts team is special: it never shows up in contributors |
74 | +lists. |
75 | + |
76 | + >>> experts_pofile = factory.makePOFile('nl') |
77 | + >>> experts_message = factory.makeTranslationMessage( |
78 | + ... pofile=experts_pofile, translator=rosetta_experts, |
79 | + ... reviewer=rosetta_experts, translations=['hi']) |
80 | + |
81 | + >>> print_names(experts_pofile.contributors) |
82 | + -- |
83 | + |
84 | |
85 | == getPOTMsgSetTranslated == |
86 | |
87 | @@ -1191,7 +1219,6 @@ |
88 | Happy translator |
89 | Launchpad Contributions: |
90 | Carlos ... |
91 | - Rosetta Administrators... |
92 | |
93 | Emails of translators just has the dummy translation. |
94 | |
95 | @@ -1210,7 +1237,7 @@ |
96 | |
97 | >>> print alsa_translation.prepareTranslationCredits( |
98 | ... emails_of_translators) |
99 | - ,,carlos@canonical.com,rosetta@launchpad.net |
100 | + ,,carlos@canonical.com |
101 | |
102 | Names of translators just has the dummy translation. |
103 | |
104 | @@ -1228,7 +1255,7 @@ |
105 | |
106 | >>> print alsa_translation.prepareTranslationCredits( |
107 | ... names_of_translators) |
108 | - ,Launchpad Contributions:,Carlos P... M...,Rosetta Administrators |
109 | + ,Launchpad Contributions:,Carlos P... M... |
110 | |
111 | Lets add a newly supported KDE context message with credit strings |
112 | split into two: context and message, instead of a single message with |
113 | @@ -1241,7 +1268,7 @@ |
114 | prepareTranslationCredits works on such messages as well: |
115 | |
116 | >>> print alsa_translation.prepareTranslationCredits(new_kde_name_credits) |
117 | - ,Launchpad Contributions:,Carlos P... M...,Rosetta Administrators |
118 | + ,Launchpad Contributions:,Carlos P... M... |
119 | |
120 | Similar happens for email credits as with name credits. |
121 | |
122 | @@ -1249,7 +1276,7 @@ |
123 | ... singular_text=u"Your emails", plural_text=None, |
124 | ... context=u"EMAIL OF TRANSLATORS") |
125 | >>> print alsa_translation.prepareTranslationCredits(new_kde_email_credits) |
126 | - ,,carlos@canonical.com,rosetta@launchpad.net |
127 | + ,,carlos@canonical.com |
128 | |
129 | |
130 | == POFileToTranslationFileDataAdapter == |
131 | |
132 | === modified file 'lib/lp/translations/model/pofile.py' |
133 | --- lib/lp/translations/model/pofile.py 2009-11-17 09:50:33 +0000 |
134 | +++ lib/lp/translations/model/pofile.py 2009-11-25 17:00:32 +0000 |
135 | @@ -36,6 +36,7 @@ |
136 | from canonical.launchpad.webapp.publisher import canonical_url |
137 | from canonical.librarian.interfaces import ILibrarianClient |
138 | from lp.registry.interfaces.person import validate_public_person |
139 | +from lp.registry.model.person import Person |
140 | from lp.translations.utilities.rosettastats import RosettaStats |
141 | from lp.translations.interfaces.pofile import IPOFile, IPOFileSet |
142 | from lp.translations.interfaces.potmsgset import ( |
143 | @@ -504,10 +505,15 @@ |
144 | @property |
145 | def contributors(self): |
146 | """See `IPOFile`.""" |
147 | - from lp.registry.model.person import Person |
148 | + # Translation credit messages are "translated" by |
149 | + # rosetta_experts. Shouldn't show up in contributors lists |
150 | + # though. |
151 | + admin_team = getUtility(ILaunchpadCelebrities).rosetta_experts |
152 | + |
153 | contributors = Person.select(""" |
154 | POFileTranslator.person = Person.id AND |
155 | - POFileTranslator.pofile = %s""" % quote(self), |
156 | + POFileTranslator.person <> %s AND |
157 | + POFileTranslator.pofile = %s""" % sqlvalues(admin_team, self), |
158 | clauseTables=["POFileTranslator"], |
159 | distinct=True, |
160 | # XXX: kiko 2006-10-19: |
161 | @@ -516,6 +522,7 @@ |
162 | # function to the column results and then ignore it -- just |
163 | # like selectAlso does, ironically. |
164 | orderBy=["Person.displayname", "Person.name"]) |
165 | + |
166 | return contributors |
167 | |
168 | def prepareTranslationCredits(self, potmsgset): |
169 | |
170 | === modified file 'lib/lp/translations/model/potmsgset.py' |
171 | --- lib/lp/translations/model/potmsgset.py 2009-11-11 11:29:09 +0000 |
172 | +++ lib/lp/translations/model/potmsgset.py 2009-11-25 17:00:32 +0000 |
173 | @@ -1053,17 +1053,20 @@ |
174 | |
175 | def setTranslationCreditsToTranslated(self, pofile): |
176 | """See `IPOTMsgSet`.""" |
177 | - if self.is_translation_credit: |
178 | - translation = self.getSharedTranslationMessage(pofile.language) |
179 | - if translation is None: |
180 | - translator = pofile.lasttranslator |
181 | - if translator is None: |
182 | - translator = pofile.owner |
183 | - message = self.updateTranslation( |
184 | - pofile, translator, [credits_message_str], |
185 | - is_imported=False, allow_credits=True, |
186 | - force_shared=True, force_edition_rights=True, |
187 | - lock_timestamp=datetime.datetime.now(pytz.UTC)) |
188 | + if not self.is_translation_credit: |
189 | + return |
190 | + |
191 | + if self.getSharedTranslationMessage(pofile.language) is not None: |
192 | + return |
193 | + |
194 | + # The credits message has a fixed "translator." |
195 | + translator = getUtility(ILaunchpadCelebrities).rosetta_experts |
196 | + |
197 | + message = self.updateTranslation( |
198 | + pofile, translator, [credits_message_str], |
199 | + is_imported=False, allow_credits=True, |
200 | + force_shared=True, force_edition_rights=True, |
201 | + lock_timestamp=datetime.datetime.now(pytz.UTC)) |
202 | |
203 | def setSequence(self, potemplate, sequence): |
204 | """See `IPOTMsgSet`.""" |
205 | |
206 | === modified file 'lib/lp/translations/stories/standalone/xx-translations-to-complete.txt' |
207 | --- lib/lp/translations/stories/standalone/xx-translations-to-complete.txt 2009-08-31 13:55:35 +0000 |
208 | +++ lib/lp/translations/stories/standalone/xx-translations-to-complete.txt 2009-11-25 17:00:32 +0000 |
209 | @@ -62,3 +62,28 @@ |
210 | ... pierre_browser.contents, 'translations-to-complete-table') |
211 | >>> print tag |
212 | None |
213 | + |
214 | + |
215 | +Teams |
216 | +----- |
217 | + |
218 | +The Rosetta administrators, as a special case, can act as a translator |
219 | +for automatically generated messages. Nevertheless, its Translations |
220 | +page does not show any translations to complete--even to a member of the team. |
221 | + |
222 | + >>> login('carlos@canonical.com') |
223 | + >>> from zope.component import getUtility |
224 | + >>> from canonical.launchpad.interfaces.launchpad import ( |
225 | + ... ILaunchpadCelebrities) |
226 | + >>> pofile = factory.makePOFile('ru') |
227 | + >>> rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts |
228 | + >>> message = factory.makeTranslationMessage( |
229 | + ... pofile=pofile, translator=rosetta_experts, translations=['x']) |
230 | + >>> logout() |
231 | + |
232 | + >>> carlos_browser = setupBrowser('Basic carlos@canonical.com:test') |
233 | + >>> experts_home = 'http://translations.launchpad.dev/~rosetta-admins' |
234 | + >>> carlos_browser.open(experts_home) |
235 | + |
236 | + >>> print extract_text(find_tag_by_id( |
237 | + ... jean_browser.contents, 'translations-to-complete-table')) |
238 | |
239 | === modified file 'lib/lp/translations/tests/test_pofile.py' |
240 | --- lib/lp/translations/tests/test_pofile.py 2009-10-30 10:29:25 +0000 |
241 | +++ lib/lp/translations/tests/test_pofile.py 2009-11-25 17:00:32 +0000 |
242 | @@ -911,6 +911,7 @@ |
243 | self.assertNotEqual(None, stable_potemplate.getPOFileByLang('eo')) |
244 | self.assertNotEqual(None, stable_potemplate.getPOFileByLang('de')) |
245 | |
246 | + |
247 | class TestTranslationCredits(TestCaseWithFactory): |
248 | """Test generation of translation credits.""" |
249 | |
250 | |
251 | === modified file 'lib/lp/translations/tests/test_potmsgset.py' |
252 | --- lib/lp/translations/tests/test_potmsgset.py 2009-11-11 11:38:22 +0000 |
253 | +++ lib/lp/translations/tests/test_potmsgset.py 2009-11-25 17:00:32 +0000 |
254 | @@ -15,6 +15,7 @@ |
255 | from zope.security.proxy import isinstance as zope_isinstance |
256 | from zope.security.proxy import removeSecurityProxy |
257 | |
258 | +from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities |
259 | from lp.registry.interfaces.person import IPersonSet |
260 | from lp.registry.interfaces.product import IProductSet |
261 | from lp.services.worlddata.interfaces.language import ILanguageSet |
262 | @@ -23,7 +24,6 @@ |
263 | from lp.translations.interfaces.translationfileformat import ( |
264 | TranslationFileFormat) |
265 | from lp.translations.interfaces.translationmessage import TranslationConflict |
266 | -from lp.translations.interfaces.translationsperson import ITranslationsPerson |
267 | from lp.translations.model.translationmessage import ( |
268 | DummyTranslationMessage) |
269 | |
270 | @@ -675,41 +675,20 @@ |
271 | self.assertNotEqual(None, current_shared) |
272 | self.assertEqual(None, current_shared.potemplate) |
273 | |
274 | - def test_setTranslationCreditsToTranslated_permissions(self): |
275 | - # Even if POFile.owner has not agreed to the Launchpad |
276 | - # translations licensing policy, automated credit |
277 | - # messages are still created. |
278 | - sr_pofile = self.factory.makePOFile('sr', self.devel_potemplate) |
279 | - owner = sr_pofile.owner |
280 | - ITranslationsPerson(owner).translations_relicensing_agreement = False |
281 | - credits_potmsgset = self.factory.makePOTMsgSet( |
282 | - self.devel_potemplate, singular=u'translator-credits') |
283 | - current = credits_potmsgset.getCurrentTranslationMessage( |
284 | - self.devel_potemplate, sr_pofile.language) |
285 | - self.assertEqual(owner, current.submitter) |
286 | - |
287 | - def test_setTranslationCreditsToTranslated_submitter_lasttranslator(self): |
288 | - # Submitter on the automated translation message is set to |
289 | - # POFile.lasttranslator if it's defined. |
290 | - sr_pofile = self.factory.makePOFile('sr', self.devel_potemplate) |
291 | - last_translator = self.factory.makePerson() |
292 | - sr_pofile.lasttranslator = last_translator |
293 | - credits_potmsgset = self.factory.makePOTMsgSet( |
294 | - self.devel_potemplate, singular=u'translator-credits') |
295 | - current = credits_potmsgset.getCurrentTranslationMessage( |
296 | - self.devel_potemplate, sr_pofile.language) |
297 | - self.assertEqual(last_translator, current.submitter) |
298 | - |
299 | - def test_setTranslationCreditsToTranslated_submitter_owner(self): |
300 | - # Submitter on the automated translation message is set to |
301 | - # POFile.owner if POFile.lasttranslator is not defined. |
302 | - sr_pofile = self.factory.makePOFile('sr', self.devel_potemplate) |
303 | - self.assertEqual(None, sr_pofile.lasttranslator) |
304 | - credits_potmsgset = self.factory.makePOTMsgSet( |
305 | - self.devel_potemplate, singular=u'translator-credits') |
306 | - current = credits_potmsgset.getCurrentTranslationMessage( |
307 | - self.devel_potemplate, sr_pofile.language) |
308 | - self.assertEqual(sr_pofile.owner, current.submitter) |
309 | + def test_setTranslationCreditsToTranslated_submitter(self): |
310 | + # Submitter on the automated translation message is always |
311 | + # the rosetta_experts team. |
312 | + sr_pofile = self.factory.makePOFile('sr', self.devel_potemplate) |
313 | + translator = self.factory.makePerson() |
314 | + sr_pofile.lasttranslator = translator |
315 | + sr_pofile.owner = translator |
316 | + credits_potmsgset = self.factory.makePOTMsgSet( |
317 | + self.devel_potemplate, singular=u'translator-credits') |
318 | + current = credits_potmsgset.getCurrentTranslationMessage( |
319 | + self.devel_potemplate, sr_pofile.language) |
320 | + |
321 | + rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts |
322 | + self.assertEqual(rosetta_experts, current.submitter) |
323 | |
324 | |
325 | class TestPOTMsgSetSuggestions(TestCaseWithFactory): |
= Bug 488218 =
Some translatable messages (POTMsgSets) are special. They are not
really translatable; they represent translation credits and their
"translations" are generated on-demand.
In order to stop these messages from showing up as untranslated, we
create special, "fake" translation messages for them.
But every time we create a TranslationMessage, the fact is recorded as a
contribution made by the message's translator. In this case, the last
translator for the POFile was recorded as the translator for the credits
message; or failing that, its owner.
So we had phantom "contributions" based on these automatically generated
messages. This branch fixes that by:
1. Making the Rosetta Experts team the translator for these messages.
2. Never listing Rosetta Experts as a contributor. Translations are
made by persons, not teams, and this team isn't for translation as such
anyway.
The tests for ownership are simplified; a new test shows explicitly that
the Rosetta Experts team is ignored in the contributors list. You'll
note a small drive-by there as well.
To test: to-complete
{{{
./bin/test -vv -t pofile.txt -t test_pofile -t test_potmsgset -t translations-
}}}
To Q/A: import a template with a translation credits message. For
example:
msgid "" Transfer- Encoding: 8bit\n"
msgstr ""
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-
msgid "Text"
msgstr ""
msgid "translation- credits"
msgstr ""
Then visit a (still blank) translation of the template, and log out from
there. This will create (because of a known, low-priority bug) a POFile
for that translation.
Log back in and look at your personal Translations page, under Activity.
There should be no reference to the POFile you just created; without
this branch, there is.
Also look at the translation credits message. Both before and after, it
will show your username. But previously it would show you as the
translator; now it will show Rosetta Administrators as the translator.
Finally, there turned out to be an oops waiting to happen when visiting
the Translations page for rosetta_experts, because the browser code
recognized that the team is now active as a translator, and was trying
to list translations the team could help complete. Teams should not be
considered translators in that context. I added a bit of pagetest.
Jeroen