Merge lp:~ursinha/launchpad/bug-418345-language-index-migration into lp:launchpad

[Guilherme Salgado]

[Ursula Junque (ursinha)]

= Summary =

This branch migrates the template of language-index page to the new format.
The bug that's tracking this change is bug 418345.
It also has a fix for test_translationlinksaggregator, that was relying on the database data order and started to fail.
I've added a 3-line fix for bug 404898, because it wasn't worthy creating a separated branch for it. Besides, the fix is related to the main change here, that's bug 418345.

== Pre-implementation notes ==

It was discussed with rockstar about how to deal with the context menu now, and he said we should now use NavigationMenu instead of ContextMenu. So, considering nothing else in translations was using LanguageContextMenu, I've changed this to inherit NavigationMenu, and renamed it to LanguageNavigationMenu.

== Tests ==

This should test all changes, including a fix for test_translationlinksaggregator:
 bin/test -vvt lp.translations.*

To test specifically the template migration changes:
 bin/test -vvt xx-language

== Demo and Q/A ==

To check the changed pages, go to:
 - https://translations.launchpad.dev/+languages/us
 As a regular user you should see the changed layout.
 As an admin user, you should also be able to see the 'Administer' link as well, in the new format.

= 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/configure.zcml
 lib/lp/translations/browser/tests/language-views.txt
 lib/lp/translations/browser/language.py
 lib/lp/translations/browser/tests/test_translationlinksaggregator.py
 lib/lp/translations/templates/language-index.pt
 lib/lp/translations/stories/standalone/xx-language.txt
 lib/lp/translations/templates/language-portlet-top-contributors.pt

[Guilherme Salgado (salgado)]

Hi Ursula,

This looks good, but there are a few stylistic issues that need fixing,
so I'm just giving you some quick feedback, so that you can do these
changes with any other changes that come up during the UI review of
this.

 review needs-fixing

On Fri, 2009-08-28 at 13:15 +0000, Ursula Junque wrote:
> = Summary =
>
> This branch migrates the template of language-index page to the new
> format.
> The bug that's tracking this change is bug 418345.
> It also has a fix for test_translationlinksaggregator, that was
> relying on the database data order and started to fail.
> I've added a 3-line fix for bug 404898, because it wasn't worthy
> creating a separated branch for it. Besides, the fix is related to the
> main change here, that's bug 418345.
>
>
> == Pre-implementation notes ==
>
> It was discussed with rockstar about how to deal with the context menu
> now, and he said we should now use NavigationMenu instead of
> ContextMenu. So, considering nothing else in translations was using
> LanguageContextMenu, I've changed this to inherit NavigationMenu, and
> renamed it to LanguageNavigationMenu.
>
>
> == Tests ==
>
> This should test all changes, including a fix for
> test_translationlinksaggregator:
> bin/test -vvt lp.translations.*
>
> To test specifically the template migration changes:
> bin/test -vvt xx-language
>
>
> == Demo and Q/A ==
>
> To check the changed pages, go to:
> - https://translations.launchpad.dev/+languages/us
> As a regular user you should see the changed layout.
> As an admin user, you should also be able to see the 'Administer'
> link as well, in the new format.

> === modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
> --- lib/lp/translations/stories/standalone/xx-language.txt 2009-07-01 20:45:39 +0000
> +++ lib/lp/translations/stories/standalone/xx-language.txt 2009-08-28 09:10:18 +0000
> @@ -89,15 +89,16 @@
> >>> print browser.url
> http://translations.launchpad.dev/+languages/es
>
> - >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural Forms')
> + >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural forms')
> >>> print pluralforms_portlet
> - <...
> - The amount of plural forms for Spanish is: 2
> + <div ...
> ...
> - And its default plural form expression is:
> + There are 2
> + plural forms for Spanish.

Our test suite runs with the NORMALIZE_WHITESPACE [1] option turned on,
so you can join the two lines above and trim it to make the test a bit
more readable.

[1] http://www.python.org/doc/2.5.2/lib/doctest-options.html

> ...
> <pre>n != 1</pre>
> ...
> + </div>

Why do you want to show that this is inside a <div> now?

>
> >>> translationteams_portlet = find_portlet(
> ... browser.contents, 'Translation teams')
>
> === modified file 'lib/lp/translations/templates/language-index.pt'
> --- lib/lp/translations/templates/language-index.pt 2009-07-17 17:59:07 +0000
> +++ lib/lp/translations/templates/language-index.pt 2009-08-28 10:29:18 +0000
> @@ -3,99 +3,75 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
...

review: Needs Fixing

[Barry Warsaw (barry)]

The language 'us' gives me a NotFound but 'en' works just fine and the ui looks great. You'll need a second mentat ui review. ui=me*

review: Approve (ui)

[Ursula Junque (ursinha)]

> = Summary =
>
> This branch migrates the template of language-index page to the new format.
> The bug that's tracking this change is bug 418345.
> It also has a fix for test_translationlinksaggregator, that was relying on the
> database data order and started to fail.
> I've added a 3-line fix for bug 404898, because it wasn't worthy creating a
> separated branch for it. Besides, the fix is related to the main change here,
> that's bug 418345.
>
>
> == Pre-implementation notes ==
>
> It was discussed with rockstar about how to deal with the context menu now,
> and he said we should now use NavigationMenu instead of ContextMenu. So,
> considering nothing else in translations was using LanguageContextMenu, I've
> changed this to inherit NavigationMenu, and renamed it to
> LanguageNavigationMenu.
>
>
> == Tests ==
>
> This should test all changes, including a fix for
> test_translationlinksaggregator:
> bin/test -vvt lp.translations.*
>
> To test specifically the template migration changes:
> bin/test -vvt xx-language
>
>
> == Demo and Q/A ==
>
> To check the changed pages, go to:
> - https://translations.launchpad.dev/+languages/us

I'm sorry, the correct language is 'en'. This makes the correct link:
 - https://translations.launchpad.dev/+languages/en

> As a regular user you should see the changed layout.
> As an admin user, you should also be able to see the 'Administer' link as
> well, in the new format.
>
>
> = 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/configure.zcml
> lib/lp/translations/browser/tests/language-views.txt
> lib/lp/translations/browser/language.py
> lib/lp/translations/browser/tests/test_translationlinksaggregator.py
> lib/lp/translations/templates/language-index.pt
> lib/lp/translations/stories/standalone/xx-language.txt
> lib/lp/translations/templates/language-portlet-top-contributors.pt

[Ursula Junque (ursinha)]

> Hi Ursula,
>
> This looks good, but there are a few stylistic issues that need fixing,
> so I'm just giving you some quick feedback, so that you can do these
> changes with any other changes that come up during the UI review of
> this.
>
> review needs-fixing
>
> On Fri, 2009-08-28 at 13:15 +0000, Ursula Junque wrote:
> > = Summary =
> >
> > This branch migrates the template of language-index page to the new
> > format.
> > The bug that's tracking this change is bug 418345.
> > It also has a fix for test_translationlinksaggregator, that was
> > relying on the database data order and started to fail.
> > I've added a 3-line fix for bug 404898, because it wasn't worthy
> > creating a separated branch for it. Besides, the fix is related to the
> > main change here, that's bug 418345.
> >
> >
> > == Pre-implementation notes ==
> >
> > It was discussed with rockstar about how to deal with the context menu
> > now, and he said we should now use NavigationMenu instead of
> > ContextMenu. So, considering nothing else in translations was using
> > LanguageContextMenu, I've changed this to inherit NavigationMenu, and
> > renamed it to LanguageNavigationMenu.
> >
> >
> > == Tests ==
> >
> > This should test all changes, including a fix for
> > test_translationlinksaggregator:
> > bin/test -vvt lp.translations.*
> >
> > To test specifically the template migration changes:
> > bin/test -vvt xx-language
> >
> >
> > == Demo and Q/A ==
> >
> > To check the changed pages, go to:
> > - https://translations.launchpad.dev/+languages/us
> > As a regular user you should see the changed layout.
> > As an admin user, you should also be able to see the 'Administer'
> > link as well, in the new format.
>
> > === modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
> > --- lib/lp/translations/stories/standalone/xx-language.txt 2009-07-01
> 20:45:39 +0000
> > +++ lib/lp/translations/stories/standalone/xx-language.txt 2009-08-28
> 09:10:18 +0000
> > @@ -89,15 +89,16 @@
> > >>> print browser.url
> > http://translations.launchpad.dev/+languages/es
> >
> > - >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural
> Forms')
> > + >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural
> forms')
> > >>> print pluralforms_portlet
> > - <...
> > - The amount of plural forms for Spanish is: 2
> > + <div ...
> > ...
> > - And its default plural form expression is:
> > + There are 2
> > + plural forms for Spanish.
>
> Our test suite runs with the NORMALIZE_WHITESPACE [1] option turned on,
> so you can join the two lines above and trim it to make the test a bit
> more readable.
>
> [1] http://www.python.org/doc/2.5.2/lib/doctest-options.html

Thanks for the information. Fixed.

>
> > ...
> > <pre>n != 1</pre>
> > ...
> > + </div>
>
> Why do you want to show that this is inside a <div> now?

I didn't, fixed.

>
> >
> > >>> translationteams_portlet = find_portlet(
> > ... browser.contents, 'Translation teams')
> >
> > === modified file 'lib/lp/translations/templates/language-index.pt'
> > --- lib/lp/transla...

review: Needs Resubmitting

[Edwin Grubbs (edwin-grubbs)]

ui=me

review: Approve (ui)

[Guilherme Salgado (salgado)]

with the few changes we discussed on IRC, r=me

review: Approve

=== modified file 'lib/lp/translations/browser/configure.zcml'
--- lib/lp/translations/browser/configure.zcml	2009-08-25 16:52:31 +0000
+++ lib/lp/translations/browser/configure.zcml	2009-08-28 01:59:32 +0000
@@ -295,7 +295,7 @@
             module="lp.translations.browser.language"
             classes="
                 LanguageSetContextMenu
-                LanguageContextMenu"/>
+                LanguageNavigationMenu"/>
         <browser:url
             for="canonical.launchpad.interfaces.ILanguage"
             path_expression="code"

=== modified file 'lib/lp/translations/browser/language.py'
--- lib/lp/translations/browser/language.py	2009-08-24 13:50:42 +0000
+++ lib/lp/translations/browser/language.py	2009-08-28 09:27:25 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 __all__ = [
     'LanguageAddView',
-    'LanguageContextMenu',
     'LanguageAdminView',
     'LanguageSetContextMenu',
     'LanguageSetNavigation',
@@ -25,7 +24,7 @@
 from lp.translations.browser.translations import TranslationsMixin
 from canonical.launchpad.webapp import (
     GetitemNavigation, LaunchpadView, LaunchpadFormView,
-    LaunchpadEditFormView, action, canonical_url, ContextMenu,
+    LaunchpadEditFormView, action, canonical_url, ContextMenu, NavigationMenu,
     enabled_with_permission, Link, custom_widget)
 
 from canonical.widgets import LabeledMultiCheckBoxWidget
@@ -44,8 +43,9 @@
         return Link('+add', text, icon='add')
 
 
-class LanguageContextMenu(ContextMenu):
+class LanguageNavigationMenu(NavigationMenu):
     usedfor = ILanguage
+    facet = 'translations'
     links = ['administer']
 
     @enabled_with_permission('launchpad.Admin')

=== modified file 'lib/lp/translations/browser/tests/language-views.txt'
--- lib/lp/translations/browser/tests/language-views.txt	2009-08-24 13:49:14 +0000
+++ lib/lp/translations/browser/tests/language-views.txt	2009-08-28 06:48:47 +0000
@@ -10,25 +10,25 @@
 
     >>> language_set = getUtility(ILanguageSet)
     >>> language = language_set.getLanguageByCode('pt_BR')
-    >>> language_view = create_view(
+    >>> language_admin_view = create_view(
     ...     language, '+admin', layer=TranslationsLayer)
-    >>> language_view.initialize()
+    >>> language_admin_view.initialize()
 
 The language +admin view provides a label and a page_title for the page.
 
-    >>> print language_view.label
+    >>> print language_admin_view.label
     Edit Portuguese (Brazil) in Launchpad
 
-    >>> print language_view.page_title
+    >>> print language_admin_view.page_title
     Edit Portuguese (Brazil) (pt_BR)
 
 The view provides also a cancel_url and a next_url, but they're the same because
 the previous and next steps of editing a language are returning to its index.
 
-    >>> print language_view.cancel_url
+    >>> print language_admin_view.cancel_url
     http://translations.launchpad.dev/+languages/pt_BR
 
-    >>> print language_view.next_url
+    >>> print language_admin_view.next_url
     http://translations.launchpad.dev/+languages/pt_BR
 
 
@@ -73,4 +73,3 @@
     >>> print language_add_view.next_url
     http://translations.launchpad.dev/+languages/lp_US
 
-

=== modified file 'lib/lp/translations/browser/tests/test_translationlinksaggregator.py'
--- lib/lp/translations/browser/tests/test_translationlinksaggregator.py	2009-08-25 10:35:29 +0000
+++ lib/lp/translations/browser/tests/test_translationlinksaggregator.py	2009-08-28 08:01:51 +0000
@@ -224,7 +224,7 @@
             (product, canonical_url(product_pofile), [product_pofile]),
             (package, canonical_url(package_pofile), [package_pofile]),
             ]
-        self.assertEqual(expected, descriptions)
+        self.assertContentEqual(expected, descriptions)
 
     def test_aggregate_bundles_productseries(self):
         # _aggregateTranslationTargets describes POFiles for the same

=== modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
--- lib/lp/translations/stories/standalone/xx-language.txt	2009-07-01 20:45:39 +0000
+++ lib/lp/translations/stories/standalone/xx-language.txt	2009-08-28 15:43:29 +0000
@@ -89,12 +89,11 @@
     >>> print browser.url
     http://translations.launchpad.dev/+languages/es
 
-    >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural Forms')
+    >>> pluralforms_portlet = find_portlet(browser.contents, 'Plural forms')
     >>> print pluralforms_portlet
-    <...
-    The amount of plural forms for Spanish is: 2
+    <div ...
     ...
-    And its default plural form expression is:
+                        There are 2 plural forms for Spanish.
     ...
     <pre>n != 1</pre>
     ...

=== modified file 'lib/lp/translations/templates/language-index.pt'
--- lib/lp/translations/templates/language-index.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/translations/templates/language-index.pt	2009-08-28 15:42:38 +0000
@@ -3,39 +3,12 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="context/@@main_template/master"
   i18n:domain="launchpad"
+  metal:use-macro="view/macro:page/main_side"
 >
-  <body>
-  <metal:leftportlets fill-slot="portlets_one">
-    <div tal:replace="structure context/@@+portlet-details" />
-  </metal:leftportlets>
-
-  <div metal:fill-slot="help">
-    <p>
-      Here you see the information we have available about this language.
-    </p>
-
-    <p>
-      If you see anything wrong with this information, please see whether it's
-      already been brought up in the 
-      <a tal:attributes="href view/answers_url">Answers</a> application.
-       If not, please file a question there.
-    </p>
-
-    <ul class="info">
-      <li>
-        <a href="https://help.launchpad.net/RosettaFAQ"
-        >Frequently asked questions</a>
-        </li>
-      </ul>
-  </div>
-
-  <div metal:fill-slot="main">
-    <h1>Overview of
+<body>
+  <h1 metal:fill-slot="heading">
+    Overview of 
     <tal:has_nativename condition="context/nativename">
       <tal:nativename replace="context/nativename">
         Espa&ntilde;ol
@@ -43,47 +16,41 @@
     </tal:has_nativename>
     <tal:englishname replace="context/englishname">Spain</tal:englishname>
     <tal:has_nativename condition="context/nativename">)</tal:has_nativename>
-    in Launchpad.
-    </h1>
-
-    <tal:not-visible condition="not:context/visible">
-      This language is not usually displayed in lists of languages. You will
-      have to select it specially in your <a href="/+editmylanguages">language
-      preferences</a> in order to see it.
-    </tal:not-visible>
-
-    <div class="left">
-      <div class="section">
+    in Launchpad
+  </h1>
+
+  <tal:side metal:fill-slot="side">
+    <div tal:replace="structure context/@@+global-actions" />
+    <div tal:replace="structure context/@@+portlet-details" />
+  </tal:side>
+
+
+  <tal:main metal:fill-slot="main">
+    <div class="yui-g">
+      <div class="yui-u first">
         <div class="portlet">
-          <h2>Plural Forms</h2>
-          <p>
-            When translating into this language, you need to be able to
-            express plural forms effectively. The plural form expression tells
-            the translation system when to use each plural form. For example,
-            some languages express the idea of "zero objects" differently to
-            "one object" or "more than ten objects". In these cases, the
-            plural form expression captures the information needed to know
-            which form to use based on the number in question.
-          </p>
+          <h2>Plural forms</h2>
           <tal:has_pluralforms condition="context/pluralexpression">
             <p>
-              The amount of plural forms for
-              <tal:language replace="view/language_name">
-                Espa&ntilde;ol
-              </tal:language>
-              is:
-              <tal:pluralforms replace="context/pluralforms">
-                2
-              </tal:pluralforms>
-            </p>
-            <p>
-              And its default plural form expression is:
-            </p>
-            <p>
+              There are <tal:pluralforms replace="context/pluralforms">2</tal:pluralforms> 
+              plural forms for <tal:language replace="view/language_name">Espa&ntilde;ol</tal:language>.
+            </p>
+            <p>
+              The default plural form expression for <tal:language replace="view/language_name">Espa&ntilde;ol</tal:language> is:
               <pre tal:content="context/pluralexpression">
                 n != 1
               </pre>
             </p>
+            <p></p>
+            <p>
+              When translating into this language, you need to be able to
+              express plural forms effectively. The plural form expression tells
+              the translation system when to use each plural form. For example,
+              some languages express the idea of "zero objects" differently to
+              "one object" or "more than ten objects". In these cases, the
+              plural form expression captures the information needed to know
+              which form to use based on the number in question.
+            </p>
           </tal:has_pluralforms>
           <tal:has_not_pluralforms condition="not:context/pluralexpression">
             <p class="helpwanted">
@@ -95,7 +62,14 @@
           </tal:has_not_pluralforms>
         </div>
       </div>
-      <div class="section">
+
+      <div class="yui-u">
+        <div tal:content="structure context/@@+portlet-top-contributors" />  
+      </div>
+    </div>
+
+    <div class="yui-g">
+      <div class="first yui-u">
         <div class="portlet">
           <h2>Translation teams</h2>
           <p tal:condition="view/translation_teams">
@@ -110,25 +84,25 @@
             <tr tal:repeat="expert_info view/translation_teams">
               <td>
                 <img tal:condition="expert_info/expert/isTeam"
-                     src="/@@/team" />
+                   src="/@@/team" />
                 <img tal:condition="not:expert_info/expert/isTeam"
-                     src="/@@/person" />
+                   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>)
+                  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>
@@ -141,9 +115,8 @@
           </p>
         </div>
       </div>
-    </div>
-    <div class="right">
-      <div class="section">
+
+      <div class="yui-u">
         <div class="portlet">
           <h2>Countries</h2>
           <tal:has_countries condition="context/countries">
@@ -151,29 +124,31 @@
               <tal:language replace="view/language_name">
                 Espa&ntilde;ol
               </tal:language>
-                is registered as being spoken in the following countries:
+              is registered as being spoken in the following countries:
             </p>
             <ul>
               <li tal:repeat="country context/countries"
-                  tal:content="country/name">Spain</li>
+                tal:content="country/name">Spain</li>
             </ul>
           </tal:has_countries>
           <tal:has_not_countries condition="not:context/countries">
-          <p class="helpwanted">
-            <tal:language replace="view/language_name">
-              Espa&ntilde;ol
-            </tal:language>
-            is not registered as being spoken in any country. If you know
-            about a country that officially speaks this language, please
-            open a <a href="/rosetta/+addticket">ticket</a> with that
-            information, so we can add it to Launchpad.
-          </p>
+            <p class="helpwanted">
+              <tal:language replace="view/language_name">
+                Espa&ntilde;ol
+              </tal:language>
+              is not registered as being spoken in any country. If you know
+              about a country that officially speaks this language, please
+              open a <a href="/rosetta/+addticket">ticket</a> with that
+              information, so we can add it to Launchpad.
+            </p>
           </tal:has_not_countries>
         </div>
       </div>
-      <div class="section"
-           tal:content="structure context/@@+portlet-top-contributors" />
+
     </div>
-  </div>
-  </body>
+
+  </tal:main>
+
+
+</body>
 </html>

=== modified file 'lib/lp/translations/templates/language-portlet-top-contributors.pt'
--- lib/lp/translations/templates/language-portlet-top-contributors.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/translations/templates/language-portlet-top-contributors.pt	2009-08-28 15:40:27 +0000
@@ -13,10 +13,9 @@
         Espa&ntilde;ol
       </tal:language_name>:
     </p>
-    <ul class="person">
+    <ul>
       <li tal:repeat="contributor view/getTopContributors">
-        <a tal:content="contributor/displayname"
-           tal:attributes="href contributor/fmt:url">Guilherme Salgado</a>
+        <a tal:replace="structure contributor/fmt:link">Guilherme Salgado</a>
       </li>
     </ul>
   </div>