Launchpad itself

Merge lp:~danilo/launchpad/bug-553093 into lp:launchpad

  1. bug-553093
  2. Merge into devel
Proposed by Данило Шеган
Status: Merged
Approved by: Данило Шеган
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~danilo/launchpad/bug-553093
Merge into: lp:launchpad
Diff against target: 172 lines (+79/-33)
5 files modified
lib/lp/services/worlddata/doc/language.txt (+33/-1)
lib/lp/services/worlddata/interfaces/language.py (+3/-2)
lib/lp/services/worlddata/tests/test_doc.py (+12/-1)
lib/lp/services/worlddata/tests/test_language.py (+21/-0)
lib/lp/translations/templates/language-index.pt (+10/-29)
To merge this branch: bzr merge lp:~danilo/launchpad/bug-553093
Reviewer Review Type Date Requested Status
Michael Nelson (community) code ui Approve
Review via email: mp+23458@code.launchpad.net

Commit message

Do not include translators list in the snapshot of the object, to avoid OOPSing when administering languages.

Description of the change

= Bug 553093 =

This fixes an OOPS which happens because we are snapshoting a full object with the exported API, and when translators property has more than 1000 entries, a shortlist in the snapshot implementation fails.

Fix is simple: just encapsulate doNotSnapshot().

I am struggling with the test, though. I tried making a unit test, but uncommitted it: it was too slow (creating 1001 persons with some karma), and didn't really work (switching DB users to create karma and persons, and then coming back to the regular test user made it impossible to also construct views). I did do this in my local launchpad-dev DB and tested that the fix works correctly.

Along the way, I've fixed a bunch of formatting problems on the page.

See https://translations.launchpad.dev/+languages/es for example

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) wrote :

=== removed file 'lib/lp/translations/browser/tests/test_language_views.py'
--- lib/lp/translations/browser/tests/test_language_views.py 2010-04-13 11:58:43 +0000
+++ lib/lp/translations/browser/tests/test_language_views.py 1970-01-01 00:00:00 +0000
@@ -1,54 +0,0 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-__metaclass__ = type
-
-import unittest
-
-from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
-
-from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing import LaunchpadZopelessLayer
-from lp.registry.model.karma import KarmaCategory, KarmaCache
-from lp.services.worlddata.interfaces.language import ILanguageSet
-from lp.testing import TestCaseWithFactory
-from lp.translations.browser.language import LanguageAdminView
-
-
-class TestLanguageAdminView(TestCaseWithFactory):
- """Test Language web service API."""
-
- layer = LaunchpadZopelessLayer
-
- def setUpLanguageAdminView(self, language):
- return view
-
- def setUpLanguageTranslators(self, language, total=2):
- people = []
-
- for count in range(total):
- person = self.factory.makePerson()
- person.addLanguage(language)
- people.append(person)
-
- translations_category = KarmaCategory.selectOne(
- KarmaCategory.name=='translations')
- LaunchpadZopelessLayer.switchDbUser('karma')
- for person in people:
- # Fake some translations Karma for these Serbian people.
- karma = KarmaCache(person=person,
- category=translations_category,
- karmavalue=1)
- LaunchpadZopelessLayer.commit()
-
- def test_post(self):
- serbian = getUtility(ILanguageSet).getLanguageByCode('sr')
- self.setUpLanguageTranslators(serbian, 1001)
-
- LaunchpadZopelessLayer.switchDbUser('launchpad')
- form = {}
- view = LanguageAdminView(serbian, LaunchpadTestRequest(form=form))
- view.initialize()
- view.updateContextFromData(form)
-

Revision history for this message
Данило Шеган (danilo) wrote :

Oh, and btw, generic tests have passed for the language page: bin/test -vvt lp.translations.*language

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Hi Danilo,

You mentioned that the test reproducing the actual failure takes way too long, and is not worthwhile.

The only ways I can see to get around this are:

Somehow provide a different adapter via zcml (replacing canonical.launchpad.webapp.snapshot.snapshot_sql_result with a similar version but with a limit of 2, for the duration of the test), or

Simply test the interface, that the translators field provides IDoNotSnapshot - this is not as good, but still ensures that the fix is documented and won't regress.

Regarding the template changes, nice simplification!

Revision history for this message
Данило Шеган (danilo) wrote :
Download full text (3.5 KiB)

I've went with IDoNotSnapshot after all: that's what also test_bugs_webservice is using. Since there was missing documentation for translators property, I've added that too.

=== modified file 'lib/lp/services/worlddata/doc/language.txt'
--- lib/lp/services/worlddata/doc/language.txt 2010-02-17 10:39:16 +0000
+++ lib/lp/services/worlddata/doc/language.txt 2010-04-15 11:52:13 +0000
@@ -260,8 +260,40 @@ English name for a language including op
   Serbian ("Latn" variant)

+translators
+===========
+
+Property `translators` contains the list of `Person`s who are considered
+translators for this language.
+
+ >>> sr = language_set.getLanguageByCode('sr')
+ >>> list(sr.translators)
+ []
+
+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)
+ >>> from canonical.testing import LaunchpadZopelessLayer
+ >>> LaunchpadZopelessLayer.commit()
+
+ # We need to fake some Karma.
+ >>> from lp.registry.model.karma import KarmaCategory, KarmaCache
+ >>> LaunchpadZopelessLayer.switchDbUser('karma')
+ >>> translations_category = KarmaCategory.selectOne(
+ ... KarmaCategory.name=='translations')
+ >>> karma = KarmaCache(person=translator,
+ ... category=translations_category,
+ ... karmavalue=1)
+ >>> LaunchpadZopelessLayer.commit()
+ >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
+ >>> [translator.name for translator in sr.translators]
+ [u'serbian-translator']
+
+
 =========
-countries
+Countries
 =========

 Property holding a list of countries a language is spoken in, and allowing

=== modified file 'lib/lp/services/worlddata/tests/test_doc.py'
--- lib/lp/services/worlddata/tests/test_doc.py 2009-06-30 16:56:07 +0000
+++ lib/lp/services/worlddata/tests/test_doc.py 2010-04-15 11:51:21 +0000
@@ -6,9 +6,20 @@ Run the doctests and pagetests.
 """

 import os
+
+from canonical.launchpad.testing.systemdocs import (
+ LayeredDocFileSuite, setUp, tearDown)
+from canonical.testing import LaunchpadZopelessLayer
+
 from lp.services.testing import build_test_suite

 here = os.path.dirname(os.path.realpath(__file__))
+special = {
+ 'language.txt': LayeredDocFileSuite(
+ '../doc/language.txt',
+ layer=LaunchpadZopelessLayer,
+ setUp=setUp, tearDown=tearDown),
+ }

 def test_suite():
- return build_test_suite(here)
+ return build_test_suite(here, special)

=== added file 'lib/lp/services/worlddata/tests/test_language.py'
--- lib/lp/services/worlddata/tests/test_language.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/worlddata/tests/test_language.py 2010-04-15 11:49:44 +0000
@@ -0,0 +1,21 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from canonical.testing import FunctionalLayer
+from lazr.lifecycle.interfaces import IDoNotSnapshot
+from lp.services.worlddata.interfaces.language import ILanguage
+from lp.testing import TestCaseWithFactory
+
+
+class TestLanguageWebserv...

Read more...

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Thanks Danilo, code=me.

And running your branch compared with trunk shows that the table=>div changes don't modify the layout of the translation teams (other than adding the needed comma, and proper icon spacing for the person/team icon), so ui=me too.

review: Approve (code ui)

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-02-17 10:39:16 +0000
+++ lib/lp/services/worlddata/doc/language.txt 2010-04-15 12:07:41 +0000
@@ -260,8 +260,40 @@
260 Serbian ("Latn" variant)260 Serbian ("Latn" variant)
261261
262262
263translators
264===========
265
266Property `translators` contains the list of `Person`s who are considered
267translators for this language.
268
269 >>> sr = language_set.getLanguageByCode('sr')
270 >>> list(sr.translators)
271 []
272
273To be considered a translator, they must have done some translations and
274have the language among their preferred languages.
275
276 >>> translator = factory.makePerson(name=u'serbian-translator')
277 >>> translator.addLanguage(sr)
278 >>> from canonical.testing import LaunchpadZopelessLayer
279 >>> LaunchpadZopelessLayer.commit()
280
281 # We need to fake some Karma.
282 >>> from lp.registry.model.karma import KarmaCategory, KarmaCache
283 >>> LaunchpadZopelessLayer.switchDbUser('karma')
284 >>> translations_category = KarmaCategory.selectOne(
285 ... KarmaCategory.name=='translations')
286 >>> karma = KarmaCache(person=translator,
287 ... category=translations_category,
288 ... karmavalue=1)
289 >>> LaunchpadZopelessLayer.commit()
290 >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
291 >>> [translator.name for translator in sr.translators]
292 [u'serbian-translator']
293
294
263=========295=========
264countries296Countries
265=========297=========
266298
267Property holding a list of countries a language is spoken in, and allowing299Property holding a list of countries a language is spoken in, and allowing
268300
=== modified file 'lib/lp/services/worlddata/interfaces/language.py'
--- lib/lp/services/worlddata/interfaces/language.py 2010-03-05 14:02:05 +0000
+++ lib/lp/services/worlddata/interfaces/language.py 2010-04-15 12:07:41 +0000
@@ -16,6 +16,7 @@
16from zope.schema import TextLine, Int, Choice, Bool, Field, Set16from zope.schema import TextLine, Int, Choice, Bool, Field, Set
17from zope.interface import Interface, Attribute17from zope.interface import Interface, Attribute
18from lazr.enum import DBEnumeratedType, DBItem18from lazr.enum import DBEnumeratedType, DBItem
19from lazr.lifecycle.snapshot import doNotSnapshot
1920
20from lazr.restful.declarations import (21from lazr.restful.declarations import (
21 collection_default_content, exported, export_as_webservice_collection,22 collection_default_content, exported, export_as_webservice_collection,
@@ -75,9 +76,9 @@
75 required=False),76 required=False),
76 exported_as='plural_expression')77 exported_as='plural_expression')
7778
78 translators = Field(79 translators = doNotSnapshot(Field(
79 title=u'List of Person/Team that translate into this language.',80 title=u'List of Person/Team that translate into this language.',
80 required=True)81 required=True))
8182
82 translators_count = exported(83 translators_count = exported(
83 Int(84 Int(
8485
=== modified file 'lib/lp/services/worlddata/tests/test_doc.py'
--- lib/lp/services/worlddata/tests/test_doc.py 2009-06-30 16:56:07 +0000
+++ lib/lp/services/worlddata/tests/test_doc.py 2010-04-15 12:07:41 +0000
@@ -6,9 +6,20 @@
6"""6"""
77
8import os8import os
9
10from canonical.launchpad.testing.systemdocs import (
11 LayeredDocFileSuite, setUp, tearDown)
12from canonical.testing import LaunchpadZopelessLayer
13
9from lp.services.testing import build_test_suite14from lp.services.testing import build_test_suite
1015
11here = os.path.dirname(os.path.realpath(__file__))16here = os.path.dirname(os.path.realpath(__file__))
17special = {
18 'language.txt': LayeredDocFileSuite(
19 '../doc/language.txt',
20 layer=LaunchpadZopelessLayer,
21 setUp=setUp, tearDown=tearDown),
22 }
1223
13def test_suite():24def test_suite():
14 return build_test_suite(here)25 return build_test_suite(here, special)
1526
=== added file 'lib/lp/services/worlddata/tests/test_language.py'
--- lib/lp/services/worlddata/tests/test_language.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/worlddata/tests/test_language.py 2010-04-15 12:07:41 +0000
@@ -0,0 +1,21 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4__metaclass__ = type
5
6from canonical.testing import FunctionalLayer
7from lazr.lifecycle.interfaces import IDoNotSnapshot
8from lp.services.worlddata.interfaces.language import ILanguage
9from lp.testing import TestCaseWithFactory
10
11
12class TestLanguageWebservice(TestCaseWithFactory):
13 """Test Language web service API."""
14
15 layer = FunctionalLayer
16
17 def test_translators(self):
18 self.failUnless(
19 IDoNotSnapshot.providedBy(ILanguage['translators']),
20 "ILanguage.translators should not be included in snapshots, "
21 "see bug 553093.")
022
=== modified file 'lib/lp/translations/templates/language-index.pt'
--- lib/lp/translations/templates/language-index.pt 2009-11-27 14:18:05 +0000
+++ lib/lp/translations/templates/language-index.pt 2010-04-15 12:07:41 +0000
@@ -66,36 +66,17 @@
66 being experts in66 being experts in
67 <tal:language replace="view/language_name">67 <tal:language replace="view/language_name">
68 Espa&ntilde;ol68 Espa&ntilde;ol
69 </tal:language>69 </tal:language>:
70 :
71 </p>70 </p>
72 <table>71 <div tal:repeat="expert_info view/translation_teams">
73 <tr tal:repeat="expert_info view/translation_teams">72 <a tal:replace="structure expert_info/expert/fmt:link">Person</a>
74 <td>73 (<tal:groups repeat="group expert_info/groups"
75 <img tal:condition="expert_info/expert/isTeam"74 ><a tal:replace="structure group/fmt:link"
76 src="/@@/team" />75 >Translation group title</a
77 <img tal:condition="not:expert_info/expert/isTeam"76 ><tal:comma
78 src="/@@/person" />77 condition="not:repeat/group/end">, </tal:comma
79 </td>78 ></tal:groups>)
80 <td>79 </div>
81 <div>
82 <a
83 tal:attributes="
84 href expert_info/expert/fmt:url;
85 title expert_info/expert/displayname/fmt:shorten/80"
86 tal:content="expert_info/expert/displayname"
87 >Expert name</a>(
88 <tal:groups repeat="group expert_info/groups">
89 <a tal:attributes="
90 href group/fmt:url;
91 title group/title/fmt:shorten/80"
92 tal:content="group/title"
93 >Translation group title</a>
94 </tal:groups>)
95 </div>
96 </td>
97 </tr>
98 </table>
99 <p tal:condition="not:view/translation_teams">80 <p tal:condition="not:view/translation_teams">
100 <tal:language replace="view/language_name">81 <tal:language replace="view/language_name">
101 Espa&ntilde;ol82 Espa&ntilde;ol