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

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
1=== modified file 'lib/lp/services/worlddata/doc/language.txt'
2--- lib/lp/services/worlddata/doc/language.txt 2010-02-17 10:39:16 +0000
3+++ lib/lp/services/worlddata/doc/language.txt 2010-04-15 12:07:41 +0000
4@@ -260,8 +260,40 @@
5 Serbian ("Latn" variant)
6
7
8+translators
9+===========
10+
11+Property `translators` contains the list of `Person`s who are considered
12+translators for this language.
13+
14+ >>> sr = language_set.getLanguageByCode('sr')
15+ >>> list(sr.translators)
16+ []
17+
18+To be considered a translator, they must have done some translations and
19+have the language among their preferred languages.
20+
21+ >>> translator = factory.makePerson(name=u'serbian-translator')
22+ >>> translator.addLanguage(sr)
23+ >>> from canonical.testing import LaunchpadZopelessLayer
24+ >>> LaunchpadZopelessLayer.commit()
25+
26+ # We need to fake some Karma.
27+ >>> from lp.registry.model.karma import KarmaCategory, KarmaCache
28+ >>> LaunchpadZopelessLayer.switchDbUser('karma')
29+ >>> translations_category = KarmaCategory.selectOne(
30+ ... KarmaCategory.name=='translations')
31+ >>> karma = KarmaCache(person=translator,
32+ ... category=translations_category,
33+ ... karmavalue=1)
34+ >>> LaunchpadZopelessLayer.commit()
35+ >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
36+ >>> [translator.name for translator in sr.translators]
37+ [u'serbian-translator']
38+
39+
40 =========
41-countries
42+Countries
43 =========
44
45 Property holding a list of countries a language is spoken in, and allowing
46
47=== modified file 'lib/lp/services/worlddata/interfaces/language.py'
48--- lib/lp/services/worlddata/interfaces/language.py 2010-03-05 14:02:05 +0000
49+++ lib/lp/services/worlddata/interfaces/language.py 2010-04-15 12:07:41 +0000
50@@ -16,6 +16,7 @@
51 from zope.schema import TextLine, Int, Choice, Bool, Field, Set
52 from zope.interface import Interface, Attribute
53 from lazr.enum import DBEnumeratedType, DBItem
54+from lazr.lifecycle.snapshot import doNotSnapshot
55
56 from lazr.restful.declarations import (
57 collection_default_content, exported, export_as_webservice_collection,
58@@ -75,9 +76,9 @@
59 required=False),
60 exported_as='plural_expression')
61
62- translators = Field(
63+ translators = doNotSnapshot(Field(
64 title=u'List of Person/Team that translate into this language.',
65- required=True)
66+ required=True))
67
68 translators_count = exported(
69 Int(
70
71=== modified file 'lib/lp/services/worlddata/tests/test_doc.py'
72--- lib/lp/services/worlddata/tests/test_doc.py 2009-06-30 16:56:07 +0000
73+++ lib/lp/services/worlddata/tests/test_doc.py 2010-04-15 12:07:41 +0000
74@@ -6,9 +6,20 @@
75 """
76
77 import os
78+
79+from canonical.launchpad.testing.systemdocs import (
80+ LayeredDocFileSuite, setUp, tearDown)
81+from canonical.testing import LaunchpadZopelessLayer
82+
83 from lp.services.testing import build_test_suite
84
85 here = os.path.dirname(os.path.realpath(__file__))
86+special = {
87+ 'language.txt': LayeredDocFileSuite(
88+ '../doc/language.txt',
89+ layer=LaunchpadZopelessLayer,
90+ setUp=setUp, tearDown=tearDown),
91+ }
92
93 def test_suite():
94- return build_test_suite(here)
95+ return build_test_suite(here, special)
96
97=== added file 'lib/lp/services/worlddata/tests/test_language.py'
98--- lib/lp/services/worlddata/tests/test_language.py 1970-01-01 00:00:00 +0000
99+++ lib/lp/services/worlddata/tests/test_language.py 2010-04-15 12:07:41 +0000
100@@ -0,0 +1,21 @@
101+# Copyright 2010 Canonical Ltd. This software is licensed under the
102+# GNU Affero General Public License version 3 (see the file LICENSE).
103+
104+__metaclass__ = type
105+
106+from canonical.testing import FunctionalLayer
107+from lazr.lifecycle.interfaces import IDoNotSnapshot
108+from lp.services.worlddata.interfaces.language import ILanguage
109+from lp.testing import TestCaseWithFactory
110+
111+
112+class TestLanguageWebservice(TestCaseWithFactory):
113+ """Test Language web service API."""
114+
115+ layer = FunctionalLayer
116+
117+ def test_translators(self):
118+ self.failUnless(
119+ IDoNotSnapshot.providedBy(ILanguage['translators']),
120+ "ILanguage.translators should not be included in snapshots, "
121+ "see bug 553093.")
122
123=== modified file 'lib/lp/translations/templates/language-index.pt'
124--- lib/lp/translations/templates/language-index.pt 2009-11-27 14:18:05 +0000
125+++ lib/lp/translations/templates/language-index.pt 2010-04-15 12:07:41 +0000
126@@ -66,36 +66,17 @@
127 being experts in
128 <tal:language replace="view/language_name">
129 Espa&ntilde;ol
130- </tal:language>
131- :
132+ </tal:language>:
133 </p>
134- <table>
135- <tr tal:repeat="expert_info view/translation_teams">
136- <td>
137- <img tal:condition="expert_info/expert/isTeam"
138- src="/@@/team" />
139- <img tal:condition="not:expert_info/expert/isTeam"
140- src="/@@/person" />
141- </td>
142- <td>
143- <div>
144- <a
145- tal:attributes="
146- href expert_info/expert/fmt:url;
147- title expert_info/expert/displayname/fmt:shorten/80"
148- tal:content="expert_info/expert/displayname"
149- >Expert name</a>(
150- <tal:groups repeat="group expert_info/groups">
151- <a tal:attributes="
152- href group/fmt:url;
153- title group/title/fmt:shorten/80"
154- tal:content="group/title"
155- >Translation group title</a>
156- </tal:groups>)
157- </div>
158- </td>
159- </tr>
160- </table>
161+ <div tal:repeat="expert_info view/translation_teams">
162+ <a tal:replace="structure expert_info/expert/fmt:link">Person</a>
163+ (<tal:groups repeat="group expert_info/groups"
164+ ><a tal:replace="structure group/fmt:link"
165+ >Translation group title</a
166+ ><tal:comma
167+ condition="not:repeat/group/end">, </tal:comma
168+ ></tal:groups>)
169+ </div>
170 <p tal:condition="not:view/translation_teams">
171 <tal:language replace="view/language_name">
172 Espa&ntilde;ol