Merge lp:~henninge/launchpad/devel-bugjamming-2 into lp:launchpad

[Henning Eggers]

[r=bac,jcsackett][ui=sinzui][bug=440406,560701] Help with alternate language usage. List languages on a persons translation dashboard.

= Summary =

Fixes two bugs:

Bug 560701: "Not showing suggestions from selected alternative
language" warning should link to edit languages page

Bug 440406: Translations overview should state language code

== Proposed fix ==

Add a link to the user's +editlanguages page to the message.

List languages in person translation tables. The languages can be
extracted from the list of pofiles that is covered by the given
statistic.

== Pre-implementation notes ==

Trivial, no pre-imp.

== Implementation details ==

Pretty straightforward, not much to explain.

The _makePOFiles helper method in test_persontranslationview had to
be adapted to create files for multiple languages but in the same
POTemplate, so they can get aggretated into one.

In that file I also removed the references to the hard-coded language
(Dutch) and used generic languages, which is possible nowadays.

== Tests ==

bin/test -vvcm lp.translations \
  -t xx-pofile-translate-alternative-language \
  -t test_getTargetsForTranslation \
  -t xx-translations-to-review

== Demo and Q/A ==

For bug 560701:
Go to the page listed in the bug and verify that the message
contains a link to +editlanguages.

For bug 440406:
Got to https://translations.launchpad.net/people/+me and verify that
the languages are listed on each line on the table.

= Launchpad lint =

I removed some lint. Yeah!

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/browser/tests/test_persontranslationview.py
  lib/lp/translations/stories/standalone/xx-pofile-translate-alternative-language.txt
  lib/lp/translations/templates/person-translations-to-complete-table.pt
  lib/lp/translations/templates/person-translations-to-review.pt
  lib/lp/translations/browser/translationmessage.py
  lib/lp/translations/templates/person-translations-to-review-table.pt
  lib/lp/translations/browser/person.py

./lib/lp/translations/browser/translationmessage.py
     337: E301 expected 1 blank line, found 2
     365: E301 expected 1 blank line, found 2
     509: E301 expected 1 blank line, found 2
     744: E301 expected 1 blank line, found 2
     827: E301 expected 1 blank line, found 2
    1357: E301 expected 1 blank line, found 2
./lib/lp/translations/browser/person.py
     363: E202 whitespace before ']'

[Henning Eggers (henninge)]

For the UI review:

Bug 560701:
http://people.canonical.com/~henninge/screenshots/alternative-language-editlanguages.png

Bug 440406:
http://people.canonical.com/~henninge/screenshots/translations-to-complete-languages.png

[Curtis Hovey (sinzui)]

Thanks.

review: Approve (ui)

[j.c.sackett (jcsackett)]

Henninge--

This all looks good to me.

I do think the language shown to the user (which existed prior to this branch) is a little clunky. Maybe get Revell to take a look?

review: Approve (code*)

[Brad Crittenden (bac)]

Looks good Henning.

When sorting a set you do not need to call list() first.

Otherwise it looks good.

review: Approve (code)

[Henning Eggers (henninge)]

Thanks for the reviews. I agree that the page could possibly look and "sound" better but I am too tired to do anything about it now...
I removed the superfluous list().

=== modified file 'lib/lp/translations/browser/person.py'
--- lib/lp/translations/browser/person.py	2010-11-23 23:22:27 +0000
+++ lib/lp/translations/browser/person.py	2010-12-24 13:33:43 +0000
@@ -67,6 +67,9 @@
         """See `TranslationLinksAggregator.describe`."""
         strings_count = sum(
             [self.countStrings(pofile) for pofile in covered_files])
+        languages = set(
+            [pofile.language.englishname for pofile in covered_files])
+        languages_list = ", ".join(sorted(languages))
 
         if strings_count == 1:
             strings_wording = "%d string"
@@ -79,6 +82,7 @@
             'count_wording': strings_wording % strings_count,
             'is_product': not ISourcePackage.providedBy(target),
             'link': link,
+            'languages': languages_list,
         }
 
 

=== modified file 'lib/lp/translations/browser/tests/test_persontranslationview.py'
--- lib/lp/translations/browser/tests/test_persontranslationview.py	2010-10-04 19:50:45 +0000
+++ lib/lp/translations/browser/tests/test_persontranslationview.py	2010-12-24 13:33:43 +0000
@@ -10,7 +10,6 @@
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import LaunchpadZopelessLayer
-from lp.services.worlddata.model.language import LanguageSet
 from lp.testing import TestCaseWithFactory
 from lp.translations.browser.person import PersonTranslationView
 from lp.translations.model.translator import TranslatorSet
@@ -26,38 +25,52 @@
         person = removeSecurityProxy(self.factory.makePerson())
         self.view = PersonTranslationView(person, LaunchpadTestRequest())
         self.translationgroup = None
-        self.dutch = LanguageSet().getLanguageByCode('nl')
-        self.view.context.addLanguage(self.dutch)
+        self.language = self.factory.makeLanguage()
+        self.view.context.addLanguage(self.language)
 
     def _makeReviewer(self):
-        """Set up the person we're looking at as a Dutch reviewer."""
+        """Set up the person we're looking at as a reviewer."""
         owner = self.factory.makePerson()
         self.translationgroup = self.factory.makeTranslationGroup(owner=owner)
         TranslatorSet().new(
-            translationgroup=self.translationgroup, language=self.dutch,
+            translationgroup=self.translationgroup, language=self.language,
             translator=self.view.context)
 
-    def _makePOFiles(self, count, previously_worked_on):
+    def _makePOFiles(self, count, previously_worked_on, languages=None):
         """Create `count` `POFile`s that the view's person can review.
 
         :param count: Number of POFiles to create.
         :param previously_worked_on: Whether these should be POFiles
             that the person has already worked on.
+        :param languages: List of languages for each pofile. The length of
+            the list must be the same as count. If None, all files will be
+            created with self.language. Also, if languages is not None,
+            all files will be created for the same template.
         """
         pofiles = []
+        if languages is not None:
+            potemplate = self.factory.makePOTemplate()
         for counter in xrange(count):
-            pofile = self.factory.makePOFile(language_code='nl')
+            if languages is None:
+                pofile = self.factory.makePOFile(language=self.language)
+            else:
+                pofile = self.factory.makePOFile(
+                    potemplate=potemplate, language=languages[counter])
 
             if self.translationgroup:
                 product = pofile.potemplate.productseries.product
                 product.translationgroup = self.translationgroup
 
             if previously_worked_on:
+                if languages is not None:
+                    sequence = counter+1
+                else:
+                    sequence = 1
                 potmsgset = self.factory.makePOTMsgSet(
-                    potemplate=pofile.potemplate, singular='x', sequence=1)
+                    potemplate=pofile.potemplate, sequence=sequence)
                 self.factory.makeTranslationMessage(
                     potmsgset=potmsgset, pofile=pofile,
-                    translator=self.view.context, translations=['y'])
+                    translator=self.view.context)
 
             removeSecurityProxy(pofile).unreviewed_count = 1
             pofiles.append(pofile)
@@ -210,11 +223,12 @@
 
         self.assertTrue(self.view.person_is_translator)
 
-    def test_getTargetsForTranslation(self):
+    def test_getTargetsForTranslation_nothing(self):
         # If there's nothing to translate, _getTargetsForTranslation
         # returns nothing.
         self.assertEqual([], self.view._getTargetsForTranslation())
 
+    def test_getTargetsForTranslation(self):
         # If there's a translation that this person has worked on and
         # is not a reviewer for, and it has untranslated strings, it
         # shows up in _getTargetsForTranslation.
@@ -228,6 +242,26 @@
         description = descriptions[0]
         self.assertEqual(product, description['target'])
         self.assertTrue(description['link'].startswith(canonical_url(pofile)))
+        self.assertEqual(
+            pofile.language.englishname, description['languages'])
+
+    def test_getTargetsForTranslation_multiple_languages(self):
+        # Translations in different languages are aggregated to one target
+        # but the language names are listed.
+        other_language = self.factory.makeLanguage()
+        self.view.context.addLanguage(other_language)
+        pofiles = self._makePOFiles(
+            2, previously_worked_on=True,
+            languages=[self.language, other_language])
+        for pofile in pofiles:
+            self._addUntranslatedMessages(pofile, 1)
+
+        descriptions = self.view._getTargetsForTranslation()
+        self.assertEqual(1, len(descriptions))
+        description = descriptions[0]
+        expected_languages = ', '.join(sorted([
+            self.language.englishname, other_language.englishname]))
+        self.assertContentEqual(expected_languages, description['languages'])
 
     def test_getTargetsForTranslation_max_fetch(self):
         # The max_fetch parameter limits how many POFiles are considered
@@ -360,7 +394,7 @@
 
         # But the answer is True if the person has no preferred
         # languages.
-        self.view.context.removeLanguage(self.dutch)
+        self.view.context.removeLanguage(self.language)
         self.assertTrue(self.view.requires_preferred_languages)
 
 

=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py	2010-12-01 11:26:57 +0000
+++ lib/lp/translations/browser/translationmessage.py	2010-12-24 13:33:43 +0000
@@ -551,12 +551,18 @@
                 translations_person = ITranslationsPerson(user)
                 choices = set(translations_person.translatable_languages)
                 if choices and alternative_language not in choices:
-                    self.request.response.addInfoNotification(
+                    editlanguages_url = canonical_url(
+                        self.user, view_name="+editlanguages")
+                    self.request.response.addInfoNotification(structured(
                         u"Not showing suggestions from selected alternative "
-                        "language %s.  If you wish to see suggestions from "
-                        "this language, add it to your preferred languages "
-                        "first."
-                        % alternative_language.displayname)
+                        "language %(alternative)s.  If you wish to see "
+                        "suggestions from this language, "
+                        '<a href="%(editlanguages_url)s">'
+                        "add it to your preferred languages</a> first."
+                        % dict(
+                            alternative=alternative_language.displayname,
+                            editlanguages_url=editlanguages_url,
+                            )))
                     alternative_language = None
                     second_lang_code = None
 

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-alternative-language.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-alternative-language.txt	2010-12-01 11:26:57 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-alternative-language.txt	2010-12-24 13:33:43 +0000
@@ -1,4 +1,5 @@
-= PO file alternative language selection =
+PO file alternative language selection
+======================================
 
 The translation forms allow the user to select an alternative language.  The
 form will suggest translations from this alternative language as well as those
@@ -70,7 +71,8 @@
     ['']
 
 
-=== Suggestions ===
+Suggestions
+-----------
 
 Carlos also visits the Catalan equivalent of the same translation page, using
 Spanish as an alternative language.
@@ -104,7 +106,9 @@
 Japanese (ja).
 
     >>> browser.open(translate_page +
-    ...     "?field.alternative_language=ja&field.alternative_language-empty-marker=1&select_alternate_language=Change")
+    ...     "?field.alternative_language=ja"
+    ...     "&field.alternative_language-empty-marker=1"
+    ...     "&select_alternate_language=Change")
 
 This leads to a subtle technical problem as the alternative-language dropdown
 would have to be initialized to a language that wasn't in its list of items.
@@ -113,11 +117,16 @@
 first.
 
     >>> for message in get_feedback_messages(browser.contents):
-    ...     print message
+    ...     print extract_text(message)
     Not showing suggestions from selected alternative language Japanese (ja).
     If you wish to see suggestions from this language, add it to your
     preferred languages first.
 
+It even presents a link to where the user can set the preferred languages.
+
+    >>> print browser.getLink("add it to your preferred languages").url
+    http...~carlos/+editlanguages
+
 This distinction between alternative languages from the user's preferred set
 and other alternative languages does not exist, of course, if no preferred
 languages are defined.  Suggestions just work for anonymous users.
@@ -140,7 +149,8 @@
     ...
 
 
-== Filtering & Navigation ==
+Filtering & Navigation
+----------------------
 
 The translate page also allows the user to filter the translatable strings to
 show only the strings he is interested in.
@@ -176,7 +186,8 @@
     11 ... 15  of 15 results ...
 
 
-== Language variants and alternative language ==
+Language variants and alternative language
+------------------------------------------
 
 The language for the suggestions will be selected automatically when it
 is not submitted by the user. If the language variant is not available
@@ -215,7 +226,8 @@
     ['(no value)']
 
 
-== There is only one alternative language at a time ==
+There is only one alternative language at a time
+------------------------------------------------
 
 If a user specifies more than one alternative language in the URL, he
 gets an UnexpectedFormData exception:
@@ -229,7 +241,8 @@
     UnexpectedFormData: You specified...
 
 
-== Requests for a non-translatable alternative language ==
+Requests for a non-translatable alternative language
+----------------------------------------------------
 
 There are older URLs that contain non-translatable languages such as
 English, or a non-visible language. Two example requests in OOPSes

=== modified file 'lib/lp/translations/stories/standalone/xx-translations-to-review.txt'
--- lib/lp/translations/stories/standalone/xx-translations-to-review.txt	2009-08-31 13:04:32 +0000
+++ lib/lp/translations/stories/standalone/xx-translations-to-review.txt	2010-12-24 13:33:43 +0000
@@ -90,8 +90,8 @@
 
     >>> browser.open(homepage)
     >>> list_reviewables(browser)
-    <a ...>...</a> needs <a ...> 1 string reviewed </a>
-    <a ...>...</a> needs <a ...> 1 string reviewed </a>
+    <a ...>...</a> needs <a ...> 1 string reviewed </a> in Khmer
+    <a ...>...</a> needs <a ...> 1 string reviewed </a> in Khmer
     Listing contains 2 translation(s).
 
 These are translations that xowxz has not worked on.  They are not ones

=== modified file 'lib/lp/translations/templates/person-translations-to-complete-table.pt'
--- lib/lp/translations/templates/person-translations-to-complete-table.pt	2009-09-09 09:51:14 +0000
+++ lib/lp/translations/templates/person-translations-to-complete-table.pt	2010-12-24 13:33:43 +0000
@@ -26,8 +26,10 @@
 	<tal:stringcount replace="target_info/count_wording">
 	  1 string
 	</tal:stringcount>
-	translated
-      </a>
+	translated </a> in
+    <tal:languages replace="target_info/languages">
+      Spanish
+    </tal:languages>
     </td>
   </tr>
 </table>

=== modified file 'lib/lp/translations/templates/person-translations-to-review-table.pt'
--- lib/lp/translations/templates/person-translations-to-review-table.pt	2009-09-09 09:51:14 +0000
+++ lib/lp/translations/templates/person-translations-to-review-table.pt	2010-12-24 13:33:43 +0000
@@ -26,8 +26,10 @@
 	<tal:stringcount replace="target_info/count_wording">
 	  1 string
 	</tal:stringcount>
-	reviewed
-      </a>
+	reviewed </a> in
+    <tal:languages replace="target_info/languages">
+      Spanish
+    </tal:languages>
     </td>
   </tr>
 </table>

=== modified file 'lib/lp/translations/templates/person-translations-to-review.pt'
--- lib/lp/translations/templates/person-translations-to-review.pt	2009-09-17 20:11:48 +0000
+++ lib/lp/translations/templates/person-translations-to-review.pt	2010-12-24 13:33:43 +0000
@@ -39,7 +39,10 @@
                   1 string
                 </tal:stringcount>
                 reviewed
-              </a>
+              </a> in
+              <tal:languages replace="target_info/languages">
+                Spanish
+              </tal:languages>
             </td>
           </tr>
         </table>