Code review comment for lp:~adiroiban/launchpad/bug-193750

Hi Adi,

a very nice branch! I have only a number of mostly formal nitpicks,
see below.

When you have fixed these small issues, I'll ec2-test the branch and
land it on your behalf.

Abel

>
> = Bug 193750 =
>
> == Proposed fix ==
>
> The fix should replace "ticket" to "question" and update the required answer links for addticket to addquestions.
>
> Also instead on translations.lp.dev/rosetta/addticket the url was replaces to answers.lp.dev/rosetta/addquestion to have a clean namespace
>
> == Implementation details ==
>
> There were no test for checking the cases when those links should appear so I have added the required pagetests.

Thanks for adding them!

>
> The translate.txt pagetest was also structured... but a bit.
>
> == Tests ==
>
> ./bin/test -ct language
>
> == Demo and Q/A ==
>
> Abkhazian is a language that has no information about plural forms and is not registered as being spoken in any country.
>
> We will see a note about missing plural forms and a link to Rosetta
> add question page for informing Rosetta admin about the right plural form.
>
> >>> browser.open('http://translations.launchpad.dev/+languages/ab')
> >>> print extract_text(find_portlet(browser.contents, 'Plural forms'
> ... ).renderContents())
> Plural forms
> Unfortunately, Launchpad doesn't know the plural
> form information for this language...
>
> >>> print browser.getLink(id='plural_question').url
> http://answers.launchpad.dev/rosetta/+addquestion
>
> We will see a note that this language is not registred as being spoken in any
> country and a link to add question page for informating Rosetta admin about the
> countries where this page is officially spoken.
>
> >>> countries_portlet = find_portlet(browser.contents, 'Countries')
> >>> print countries_portlet
> <...
> Abkhazian is not registered as being spoken in any
> country...
>
> >>> print browser.getLink(id='country_question').url
> http://answers.launchpad.dev/rosetta/+addquestion
>
> == 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/language.py
> lib/lp/translations/stories/standalone/xx-language.txt
> lib/lp/translations/templates/language-index.pt
>
>
> === modified file 'lib/lp/translations/browser/language.py'
> --- lib/lp/translations/browser/language.py 2009-10-31 11:06:44 +0000
> +++ lib/lp/translations/browser/language.py 2009-11-26 19:30:28 +0000
> @@ -29,6 +29,7 @@
> enabled_with_permission, GetitemNavigation, LaunchpadEditFormView,
> LaunchpadFormView, LaunchpadView, Link, NavigationMenu)
> from lp.translations.utilities.pluralforms import make_friendly_plural_forms
> +from lp.registry.interfaces.product import IProductSet
>
> from canonical.widgets import LabeledMultiCheckBoxWidget
>
> @@ -202,6 +203,13 @@
>
> return pluralforms_list
>
> + @property
> + def add_question_url(self):
> + rosetta = getUtility(IProductSet).getByName('rosetta')

Here you use the rosetta project not as an ordinary project that can
be replaced by any other project but as something that is simply
expected to exist, and you use it for a special purpose: To
create the URL to contact the people who administer LP translations.
For such purposes it is better to access the Rosetta project via
LaunchpadCelebrities than via IProductSet:

    from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
    rosetta = getUtility(ILaunchpadCelebrities).lp_translations

> + return canonical_url(
> + rosetta,
> + view_name='+addquestion',
> + rootsite='answers')
>
> class LanguageAdminView(LaunchpadEditFormView):
> """Handle an admin form submission."""
>
> === modified file 'lib/lp/translations/stories/standalone/xx-language.txt'
> --- lib/lp/translations/stories/standalone/xx-language.txt 2009-10-31 11:06:44 +0000
> +++ lib/lp/translations/stories/standalone/xx-language.txt 2009-11-26 19:30:28 +0000
> @@ -1,6 +1,9 @@
> += Languages view =
> Here is the tale of languages. We will see how to create, find and edit
> them.
>
> +== Getting there ==
> +

/me knows that it is bit of back and forth with the style we use for
headings... But could you change the formatting to our current style:

Language View
=============

and

Getting There
-------------

Also, could you please make sure that we have two empty lines before
each header (ecxpet at the start of the file) and one empty line after
the header?

> Launchpad Translations has a main page.
>
> >>> admin_browser.open('http://translations.launchpad.dev/')
> @@ -11,7 +14,10 @@
> >>> print admin_browser.url
> http://translations.launchpad.dev/+languages
>
> -Following the link, there is a form to add new languages.
> +== Adding new languages ==
> +
> +Following the link from the translations main page, there is a form to add new
> +languages.

Could you wrap the lines above at 72 characters?

>
> >>> admin_browser.getLink('Add new language').click()
> >>> print admin_browser.url
> @@ -70,6 +76,8 @@
> ...
> Unauthorized:...
>
> +== Searching for a language ==
> +
> From the top languages page, anyone can find languages.
>
> >>> browser.open('http://translations.launchpad.dev/+languages')
> @@ -82,7 +90,9 @@
> >>> print browser.url
> http://translations.launchpad.dev/+languages/+index?find=Spanish
>
> -And following one of the found languages, we can see a brief information
> +== Read language information ==
> +
> +Following one of the found languages, we can see a brief information
> about the selected language.
>
> >>> browser.getLink('Spanish').click()
> @@ -133,6 +143,36 @@
> <...
> ...Carlos Perelló Marín...
>
> +Abkhazian is a language that has no information about plural forms and is not registered as being spoken in any country.

With this statement we might inadvertently step on somebody's toes ;)
What about "Our test sample data does not know about plural forms of
Abkhazian and about countries where this language is spoken."?

> +
> +We will see a note about missing plural forms and a link to Rosetta
> +add question page for informing Rosetta admin about the right plural form.
> +
> + >>> browser.open('http://translations.launchpad.dev/+languages/ab')
> + >>> print extract_text(find_portlet(browser.contents, 'Plural forms'
> + ... ).renderContents())
> + Plural forms
> + Unfortunately, Launchpad doesn't know the plural
> + form information for this language...
> +
> + >>> print browser.getLink(id='plural_question').url
> + http://answers.launchpad.dev/rosetta/+addquestion
> +
> +We will see a note that this language is not registred as being spoken in any

"we will see a note that Launchpad does not know in which countries
this language is spoken" ;)

> +country and a link to add question page for informating Rosetta admin about the

s/informating/informing/

> +countries where this page is officially spoken.

Could you wrap the lines above at <= 72 characters?

> +
> + >>> countries_portlet = find_portlet(browser.contents, 'Countries')
> + >>> print countries_portlet
> + <...
> + Abkhazian is not registered as being spoken in any
> + country...
> +
> + >>> print browser.getLink(id='country_question').url
> + http://answers.launchpad.dev/rosetta/+addquestion
> +
> +== Edit language information ==
> +
> Finally, there is the edit form to change language basic information.
>
> >>> user_browser.open('http://translations.launchpad.dev/+languages/es')
>
> === modified file 'lib/lp/translations/templates/language-index.pt'
> --- lib/lp/translations/templates/language-index.pt 2009-09-17 14:45:59 +0000
> +++ lib/lp/translations/templates/language-index.pt 2009-11-26 19:30:28 +0000
> @@ -43,7 +43,11 @@
> <p class="helpwanted">
> Unfortunately, Launchpad doesn't know the plural form
> information for this language. If you know it, please open a
> - <a href="/rosetta/+addticket">ticket</a> with that information,
> + <a id='plural_question'
> + tal:attributes="href view/add_question_url">
> + question
> + </a>
> + with that information,
> so we can add it to Launchpad.
> </p>
> </tal:has_not_pluralforms>
> @@ -124,8 +128,12 @@
> </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.
> + open a
> + <a id='country_question'

please remove the trailing space in the line above.

> + tal:attributes="href view/add_question_url">
> + question

here too.

> + </a>

and here too.

> + with that information, so we can add it to Launchpad.
> </p>
> </tal:has_not_countries>
> </div>