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

[Данило Шеган]

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

= 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

[Данило Шеган (danilo)]

=== 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)
-

[Данило Шеган (danilo)]

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

[Michael Nelson (michael.nelson)]

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!

[Данило Шеган (danilo)]

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

[Michael Nelson (michael.nelson)]

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)

=== 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 @@
   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/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 @@
 from zope.schema import TextLine, Int, Choice, Bool, Field, Set
 from zope.interface import Interface, Attribute
 from lazr.enum import DBEnumeratedType, DBItem
+from lazr.lifecycle.snapshot import doNotSnapshot
 
 from lazr.restful.declarations import (
     collection_default_content, exported, export_as_webservice_collection,
@@ -75,9 +76,9 @@
             required=False),
         exported_as='plural_expression')
 
-    translators = Field(
+    translators = doNotSnapshot(Field(
         title=u'List of Person/Team that translate into this language.',
-        required=True)
+        required=True))
 
     translators_count = exported(
         Int(

=== 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 @@
 """
 
 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 12:07:41 +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 TestLanguageWebservice(TestCaseWithFactory):
+    """Test Language web service API."""
+
+    layer = FunctionalLayer
+
+    def test_translators(self):
+        self.failUnless(
+            IDoNotSnapshot.providedBy(ILanguage['translators']),
+            "ILanguage.translators should not be included in snapshots, "
+            "see bug 553093.")

=== 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 @@
             being experts in
             <tal:language replace="view/language_name">
               Espa&ntilde;ol
-            </tal:language>
-            :
+            </tal:language>:
           </p>
-          <table>
-            <tr tal:repeat="expert_info view/translation_teams">
-              <td>
-                <img tal:condition="expert_info/expert/isTeam"
-                   src="/@@/team" />
-                <img tal:condition="not:expert_info/expert/isTeam"
-                   src="/@@/person" />
-              </td>
-              <td>
-                <div>
-                  <a
-                  tal:attributes="
-                    href expert_info/expert/fmt:url;
-                    title expert_info/expert/displayname/fmt:shorten/80"
-                  tal:content="expert_info/expert/displayname"
-                  >Expert name</a>(
-                  <tal:groups repeat="group expert_info/groups">
-                    <a tal:attributes="
-                         href group/fmt:url;
-                         title group/title/fmt:shorten/80"
-                       tal:content="group/title"
-                       >Translation group title</a>
-                  </tal:groups>)
-                </div>
-              </td>
-            </tr>
-          </table>
+          <div tal:repeat="expert_info view/translation_teams">
+            <a tal:replace="structure expert_info/expert/fmt:link">Person</a>
+            (<tal:groups repeat="group expert_info/groups"
+               ><a tal:replace="structure group/fmt:link"
+                   >Translation group title</a
+                ><tal:comma
+                    condition="not:repeat/group/end">, </tal:comma
+                  ></tal:groups>)
+          </div>
           <p tal:condition="not:view/translation_teams">
             <tal:language replace="view/language_name">
               Espa&ntilde;ol