Merge lp:~adiroiban/launchpad/bug-525992 into lp:launchpad

[Adi Roiban]

Allow reviewers to switch between translator and reviewer working mode. Landed by henninge.

= Bug 525992 =

Member of translations teams are double agents and this is not an easy job :)

They can act as translators or as reviewers .... but if they try to act as humble translators LP just makes their life miserable as they will always have to check the "Someone should review this translation" checkbox.

Maybe we can add an option for "reviewers" to allow them to tell LP they are in "translators mode" and all "Someone should review this translation" checkboxes should be checked by default.

This can be done on clientside using a cookie and javascript.

The checkbox should be checked by default, but the reviewer could manually uncheck it.

== Proposed fix ==

Add an switch on the page so that the reviewers can enter a "translation mode".

== Pre-implementation notes ==

Danilo suggested putting the "switch" on the personal dashboard, but I think that this can interrupt the workflow of a translator, as he/she will open a translation page just to discover that he/she is not in the right translation mode and then have to go to the dashboard, change the setting and then come back to the translation page.

This is why I put the switch on the bottom of the page.

== Implementation details ==

The mode is stored in a session cookie.

If the user is in translator mode, when he/she will add test to an empty translation field, the "Someone should review this translation" will be automatically selected.

Here are the screenshots for the UI review.
Note that the pointer is a hand, but I don't know why gnome-screenshot was not catching it.
http://launchpadlibrarian.net/44052274/translator.png
http://launchpadlibrarian.net/44052266/reviewer.png

I don't know what is the reason for those lint import errors.

== Tests ==
lp-tt -t distro-restricted-permissions -t translation-access-display -t test_pofile_reviewer_mode

== Demo and Q/A ==
Log in as admin.
Go to a translation page. ie https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/pt_BR/1/+translate

Type something in the new translation field.
The "Someone should review this translation" should not be checked.

Go to the bottom of the page and click "Switch to translator" mode.

Now try to add some more text to the previous translation.

The "Someone should review this translation" should not be checked.

Delete all content from the new translation field and type a new translation.

The "Someone should review this translation" should now be checked.

Click "Save and Continue"

Enter some text in the new translation field from the newly loaded page.

The "Someone should review this translation" should now be checked.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/icing/style-3-0.css.in
  lib/canonical/launchpad/javascript/translations/pofile.js
  lib/lp/translations/stories/standalone/xx-translation-access-display.txt
  lib/lp/translations/stories/translationgroups/45-test-distro-restricted-permissions.txt
  lib/lp/translations/templates/pofile-translate-access.pt
  lib/lp/translations/windmill/tests/test_pofile_translate.py

== JSLint notices ==
jslint: Lint found in '/home/adi/launchpad/lp-branches/bug-525992/lib/canonical/launchpad/javascript/translations/pofile.js':
Line 107 character 2: Missing semicolon.
}

Line 127 character 2: Missing semicolon.
}

Line 147 character 2: Missing semicolon.
}

Line 168 character 2: Missing semicolon.
}

Line 178 character 29: Use '!==' to compare with 'null'.
    if (working_mode_switch != null) {

Line 186 character 2: Missing semicolon.
}

Line 294 character 29: Use '!==' to compare with 'null'.
    if (working_mode_switch != null &&

Line 297 character 31: Use '!==' to compare with 'null'.
        if (translation_field != null &&

Line 298 character 44: Use '===' to compare with ''.
            translation_field.get('value') == '') {

Line 422 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                },

Line 429 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:68+shift+alt', Y, msgset_id);

Line 435 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:48+shift+alt', Y, fields[key]);

jslint: 1 file to lint.

== Pylint notices ==

lib/lp/translations/windmill/tests/test_pofile_translate.py
    9: [F0401] Unable to import 'canonical.launchpad.windmill.testing' (No module named canonical.launchpad.windmill.testing)
    10: [F0401] Unable to import 'lp.translations.windmill.testing' (No module named lp.translations.windmill.testing)
    11: [F0401] Unable to import 'lp.testing' (No module named lp.testing)

[Henning Eggers (henninge)]

Thank you for doing this, the basic idea really helpful.

Looking at the bug, though, and checking with Danilo I see that a pre-implementation call/chat has not taken place. We have some different thought on how to do that and we should finish that discussion first. Also, you will need a UI review.

So, this proposal will not go any further for now, sorry.

review: Abstain

[Adi Roiban (adiroiban)]

I have updated the UI implementation as agreed with Danilo.

This is the default view :
http://bayimg.com/image/nalbhaaco.jpg

After clicking "Enter translator mode" the link will change to : http://bayimg.com/image/nalbfaaco.jpg

I have added a help page and maybe Matthew Revell will need to review it.

[Adi Roiban (adiroiban)]

I have fixed the jslint error.
The only remaining error as not really errors since this is the way YUI works.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/translations/pofile.js

== JSLint notices ==
jslint: Lint found in '/home/adi/launchpad/lp-branches/bug-525992/lib/canonical/launchpad/javascript/translations/pofile.js':
Line 403 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                },

Line 410 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:68+shift+alt', Y, msgset_id);

Line 416 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:48+shift+alt', Y, fields[key]);

jslint: 1 file to lint.

[Matthew Revell (matthew.revell)]

Thanks for writing a really useful help pop-up. There are some typos in there (e.g. "desire" rather than "desired").

Tomorrow morning, I can provide specific suggestions for what I think needs to change in the help pop-up. It doesn't need much but I do think it benefit from some small changes.

review: Needs Fixing (documentation)

[Paul Hummer (rockstar)]

<rockstar> adiroiban, you'll need to fix the jslint issues before you can land this.
<adiroiban> rockstar: ok. Do I a need a documentation review from mrevell, or the help page is ok?
<rockstar> adiroiban, you should have him look at it.
<rockstar> adiroiban, when one enters "translator mode," why is "Someone should review this translation" not checked?
<adiroiban> rockstar: do you mean to check "Someone should ..." for the current new translations ?
<adiroiban> I mean for the previouly entered translations?
<rockstar> adiroiban, if I understand this correctly, when a translator is also a reviewer but just trying to translate, they always check "Someone should review this translation" Correct?
<rockstar> adiroiban, and so this branch makes it so that they don't have to check that when they are in translation mode.
<adiroiban> rockstar: that is correct
<rockstar> adiroiban, so, when someone enters "translator mode" there's a cookie set using javascript, correct?
<adiroiban> yes
<rockstar> adiroiban, so you should also use javascript to check the box next to "Someone should review this translation"
<adiroiban> hmm.. there is a javascript for that
<adiroiban> is it not working?
<adiroiban> I have tested it on Karmic using Chromium and Firefox
<adiroiban> and there is also a Windmill test for that
<rockstar> adiroiban, well, this is based on your screenshots. I'm pulling your branch now.
<adiroiban> rockstar: ah. ok
<adiroiban> yes
<rockstar> adiroiban, I'm not really looking at your code specifically. You'll need to get a code review for that. All I'm looking at is the UI currently.
<adiroiban> rockstar: yes. Henning will do the code review
<rockstar> adiroiban, so, your current screenshots are wrong. noodles will probably pull the branch as well when he mentors my review, but you might want to update those, since they indicate something incorrect.
<adiroiban> rockstar: why are they wrong ? :)
<rockstar> adiroiban, because when you're in "translator mode," then "Someone should review this translation" should be checked.
<adiroiban> rockstar: ah... not always
<adiroiban> you can still manually uncheck them
<adiroiban> is just when you type a new translation, the checkbox will be checked
<rockstar> adiroiban, when you make screenshots, they should indicate the default.

review: Approve (ui*)

[Michael Nelson (michael.nelson)]

> <rockstar> adiroiban, you'll need to fix the jslint issues before you can land
> this.
> <adiroiban> rockstar: ok. Do I a need a documentation review from mrevell, or
> the help page is ok?
> <rockstar> adiroiban, you should have him look at it.
> <rockstar> adiroiban, when one enters "translator mode," why is "Someone
> should review this translation" not checked?
> <adiroiban> rockstar: do you mean to check "Someone should ..." for the
> current new translations ?
> <adiroiban> I mean for the previouly entered translations?
> <rockstar> adiroiban, if I understand this correctly, when a translator is
> also a reviewer but just trying to translate, they always check "Someone
> should review this translation" Correct?
> <rockstar> adiroiban, and so this branch makes it so that they don't have to
> check that when they are in translation mode.
> <adiroiban> rockstar: that is correct
> <rockstar> adiroiban, so, when someone enters "translator mode" there's a
> cookie set using javascript, correct?
> <adiroiban> yes
> <rockstar> adiroiban, so you should also use javascript to check the box next
> to "Someone should review this translation"
> <adiroiban> hmm.. there is a javascript for that
> <adiroiban> is it not working?
> <adiroiban> I have tested it on Karmic using Chromium and Firefox
> <adiroiban> and there is also a Windmill test for that
> <rockstar> adiroiban, well, this is based on your screenshots. I'm pulling
> your branch now.
> <adiroiban> rockstar: ah. ok
> <adiroiban> yes
> <rockstar> adiroiban, I'm not really looking at your code specifically.
> You'll need to get a code review for that. All I'm looking at is the UI
> currently.
> <adiroiban> rockstar: yes. Henning will do the code review
> <rockstar> adiroiban, so, your current screenshots are wrong. noodles will
> probably pull the branch as well when he mentors my review, but you might want
> to update those, since they indicate something incorrect.
> <adiroiban> rockstar: why are they wrong ? :)
> <rockstar> adiroiban, because when you're in "translator mode," then "Someone
> should review this translation" should be checked.
> <adiroiban> rockstar: ah... not always
> <adiroiban> you can still manually uncheck them
> <adiroiban> is just when you type a new translation, the checkbox will be
> checked
> <rockstar> adiroiban, when you make screenshots, they should indicate the
> default.

Thanks Paul and Adi. And thanks Adi for the detailed MP with demo steps :)

Adi, when I click on the help text, it never finishes loading (as in the spinner never disappears). Not sure why, but it certainly disappears when I click on help text in other pages.

The simple functionality works really well! Thanks for ensuring that it works not just on page load, but also when the translation is deleted and re-started.

I've just got one small suggested behaviour change: I noticed that you've added the js-action to the template itself... it may be that this has been done elsewhere too, but if it's not a big change, it would be great instead to progressively enhance the page to this feature... that is:

1) If js is not switched on, then the template itself simply renders the span with the defau...

review: Approve (ui)

[Matthew Revell (matthew.revell)]

Adi, below is a diff with my suggested changes for the help pop-up.

I've brought the description of "reviewer" and "translator" modes nearer to the top of the pop-up and started off with a description of who this applies to. Also, I've changed some of the wording so that it focuses more on these as modes of working, rather than the implementation detail of it automatically selecting the check-box.

The only thing I haven't done, but did consider, was explaining that this relies of JS and setting a cookie. I chose not to because you'd pretty much have to have those enabled to be using LP anyway :)

Diff:

=== modified file 'lib/lp/translations/help/working-modes.html'
--- lib/lp/translations/help/working-modes.html 2010-04-13 16:06:52 +0000
+++ lib/lp/translations/help/working-modes.html 2010-04-15 11:01:05 +0000
@@ -9,42 +9,44 @@
           href="/+icing/yui/current/build/cssbase/base.css" />
   </head>
   <body>
- <h1>Translation working modes</h1>
-
- <p>
- When you have full access to translations, all your new translations
- will be automatically applied as the new current translations. This is
- the desire result when reviewing others translations or working in
- small teams. Adding a new suggestion will require to explicitly check
- the "Someone needs to review this translation" checkbox.
- </p>
-
- <p>
- When working in teams using a peer review process as a measurement
- translations quality assurance, the previously described behaviour
- is not suitable, as you will have to manually check the
- "Someone needs to review this translation" checkbox after adding
- each new translation.
- </p>
-
- <p>
- To deal with these different use cases, the following working modes
- are available to persons with full access to translations:
+ <h1>Switching between "translator" and "reviewer" modes</h1>
+
+ <p>
+ If you're making translations for a project that uses a <em>Structured
+ </em> or <em>Restricted</em> <a href=/+help/permissions-policies.html">
+ permissions policy</a> <strong>and</strong> you're a member of the
+ appropriate <a href=/+help/translation-groups.html">translation team</a>
+ you can work in one of two modes:
     </p>

     <ul>
- <li><strong>Reviewer:</strong> This is the default working mode.
- All changes will be automatically approved as new translations. In order
- to add a suggestion, you will have to manually check the "Someone needs
- to review this translation" checkbox.</li>
- <li><strong>Translator:</strong> When entering new translations, the
- "Someone needs to review this translation" checkbox will be
- automatically checked and the new translations will be added as
- suggestions. In order to apply the new translation, you will have to
- manually uncheck the "Someone needs to review this translation"
- checkbox.</li>
+ <li>
+ <strong>Reviewer:</strong> This is the default working mode. All
+ changes you make are approved automatically. If you want to make a
+ suggestion, rather than a new translation, you need to manually selec...

[Adi Roiban (adiroiban)]

On Wed, 2010-04-14 at 07:38 +0000, Michael Nelson wrote:
[snip]
> Thanks Paul and Adi. And thanks Adi for the detailed MP with demo
> steps :)
>
> Adi, when I click on the help text, it never finishes loading (as in
> the spinner never disappears). Not sure why, but it certainly
> disappears when I click on help text in other pages.

I have no idea why the spinner is still there.
Is there any documentation about how should I use the help popup?

I only added "help" class to the link. Is there anything else I need to
do?

> The simple functionality works really well! Thanks for ensuring that
> it works not just on page load, but also when the translation is
> deleted and re-started.
>
> I've just got one small suggested behaviour change: I noticed that
> you've added the js-action to the template itself... it may be that
> this has been done elsewhere too, but if it's not a big change, it
> would be great instead to progressively enhance the page to this
> feature... that is:
>
> 1) If js is not switched on, then the template itself simply renders
> the span with the default text "You are in blah mode" (and it won't be
> a link etc.)
I was not sure if LP interface is supposed to be used on browsers
without JS.
Done :)

> 2) When the js on the page loads, it will add the "sprite edit
> js-action" classes to the node (which changes the way it is
> displayed), and as it does now, link the on click event, and update
> the content to say "Enter ...". (BTW: is there any reason for the
> widget-hd class you've got there currently? it's usually used for
> widgets with header+body, like the slideout stuff).
Done.

I would like to removed the widget-hd class... but js-action class will
not set the proper style.
I looked in the CSS file and the css selector for js-action was
".widget-hd.js-action".
This is why I added the widget-hd class.

Without the widget-hd class, I will have to add some more code to the
CSS file.

This is the current style.css

/* The js-action class is also used for non-links, for example, with
 * expand/collapse sections. */
.widget-hd.js-action {color: #093; cursor:pointer}
.widget-hd.js-action:hover {text-decoration: underline;}

> This will ensure that this aspect of the page will still make sense
> when JS is not enabled (everything else on the page works in FF with
> JS off as far as I can see).
>
> Marking as approved, but please consider the above suggestion.
As a general rule, should we desing LP interface so that it can be used
in browsers without CSS and Javascript?

Here is the latest diff:
=== modified file
'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-13
17:54:56 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-15
16:05:26 +0000
@@ -92,6 +92,7 @@
 };

 var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
+var WORKING_MODE_SWITCH_HELP_ID =
"#translation-switch-working-mode-help";
 var WORKING_MODE_COOKIE = "translation-working-mode";
 var WORKING_MODE_REVIEWER = "reviewer";
 var WORKING_MODE_TRANSLATOR = "translator";
@@ -156,6 +157,15 @@
     var working_mode_switch = Y.one(WORKING_MODE_SWI...

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

"You are in revier mode" should probably be "reviewer". As for the general question, yes we should whenever it doesn't complicate things too much. Since this feature is only activatable through JS, it's probably good to not make it a link if JS is not enabled.

[Michael Nelson (michael.nelson)]

On Thu, Apr 15, 2010 at 6:09 PM, Adi Roiban <email address hidden> wrote:
> On Wed, 2010-04-14 at 07:38 +0000, Michael Nelson wrote:
> [snip]
>> Thanks Paul and Adi. And thanks Adi for the detailed MP with demo
>> steps :)
>>
>> Adi, when I click on the help text, it never finishes loading (as in
>> the spinner never disappears). Not sure why, but it certainly
>> disappears when I click on help text in other pages.
>
> I have no idea why the spinner is still there.
> Is there any documentation about how should I use the help popup?
>
> I only added "help" class to the link. Is there anything else I need to
> do?

I'm not certain, but it could be because of the css links you've got
in your help htmt... they seem slighly different from the other pages
I've checked? (The two templates I checked both used:
href="/+icing/yui/cssreset/reset.css" rather than
"/+icing/yui/current/build/cssreset/reset.css" in your template?).

>
>> The simple functionality works really well! Thanks for ensuring that
>> it works not just on page load, but also when the translation is
>> deleted and re-started.
>>
>> I've just got one small suggested behaviour change: I noticed that
>> you've added the js-action to the template itself... it may be that
>> this has been done elsewhere too, but if it's not a big change, it
>> would be great instead to progressively enhance the page to this
>> feature... that is:
>>
>> 1) If js is not switched on, then the template itself simply renders
>> the span with the default text "You are in blah mode" (and it won't be
>> a link etc.)
> I was not sure if LP interface is supposed to be used on browsers
> without JS.
> Done :)

Great, thanks. We certainly try to use progressive enhancement
wherever possible, so that the UI makes sense and functions with or
without JS.

>
>> 2) When the js on the page loads, it will add the "sprite edit
>> js-action" classes to the node (which changes the way it is
>> displayed), and as it does now, link the on click event, and update
>> the content to say "Enter ...". (BTW: is there any reason for the
>> widget-hd class you've got there currently? it's usually used for
>> widgets with header+body, like the slideout stuff).
> Done.

Excellent.

>
> I would like to removed the widget-hd class... but js-action class will
> not set the proper style.
> I looked in the CSS file and the css selector for js-action was
> ".widget-hd.js-action".
> This is why I added the widget-hd class.
>
> Without the widget-hd class, I will have to add some more code to the
> CSS file.
>
> This is the current style.css
>
> /* The js-action class is also used for non-links, for example, with
>  * expand/collapse sections. */
> .widget-hd.js-action {color: #093; cursor:pointer}
> .widget-hd.js-action:hover {text-decoration: underline;}

OK, at first I thought the CSS should be changed to generalise these
rules, but then noticed that the styling is defined for a.js-action
(and the pointer is default of course for a link), which made me think
a bit more. I think the best solution would be to ensure that you
replace the WORKING_MODE_SWITCH_ID node with an anchor (semantically,
that's correct as it's clickable), and then adding the .js-a...

[Henning Eggers (henninge)]

I wonder if we should show the notice at all if JS is not enabled since the user cannot do anything about it without JS. I think it'll only confuse in that case.

[Michael Nelson (michael.nelson)]

On Fri, Apr 16, 2010 at 9:28 AM, Henning Eggers
<email address hidden> wrote:
> I wonder if we should show the notice at all if JS is not enabled since the user cannot do anything about it without JS. I think it'll only confuse in that case.

Yep, that would be even better.

[Adi Roiban (adiroiban)]

On Fri, 2010-04-16 at 06:33 +0000, Michael Nelson wrote:
> On Thu, Apr 15, 2010 at 6:09 PM, Adi Roiban <email address hidden> wrote:
> > On Wed, 2010-04-14 at 07:38 +0000, Michael Nelson wrote:
> > [snip]
[snip]
> I'm not certain, but it could be because of the css links you've got
> in your help htmt... they seem slighly different from the other pages
> I've checked? (The two templates I checked both used:
> href="/+icing/yui/cssreset/reset.css" rather than
> "/+icing/yui/current/build/cssreset/reset.css" in your template?).
There are some other html files in translations/help using build/cssreset/reset.css.

I have switched to only /+icing/yui/cssreset/reset.css but the spinner
is still there after pageload.

I have also looked at other help pages, and for example in this page the
first help (updated translations) page is ok, while the second still has
the spinner.

https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/+upload

Not sure why the spinner does not disappear... I need to to more
investigation :)
[snip]
> > I would like to removed the widget-hd class... but js-action class will
> > not set the proper style.
> > I looked in the CSS file and the css selector for js-action was
> > ".widget-hd.js-action".
> > This is why I added the widget-hd class.
> >
> > Without the widget-hd class, I will have to add some more code to the
> > CSS file.
> >
> > This is the current style.css
> >
> > /* The js-action class is also used for non-links, for example, with
> > * expand/collapse sections. */
> > .widget-hd.js-action {color: #093; cursor:pointer}
> > .widget-hd.js-action:hover {text-decoration: underline;}
>
> OK, at first I thought the CSS should be changed to generalise these
> rules, but then noticed that the styling is defined for a.js-action
> (and the pointer is default of course for a link), which made me think
> a bit more. I think the best solution would be to ensure that you
> replace the WORKING_MODE_SWITCH_ID node with an anchor (semantically,
> that's correct as it's clickable), and then adding the .js-action
> class on its own will style it correctly?
>
> Give that a try, if it works I think it's the best solution for the
> moment (although it may be also that we should generalise the css
> rules, but we need to do that carefully).
Done.

I have also fixed the typo and when JS is disable there is no text about
the working mode.

Here is the latest diff:

=== modified file
'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-15
16:05:52 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-16
08:47:58 +0000
@@ -157,7 +157,7 @@
     var working_mode_switch = Y.one(WORKING_MODE_SWITCH_ID);
     // Initialize only if user is a reviewer and the switch is on the
page.
     if (working_mode_switch !== null) {
- working_mode_switch.addClass("sprite edit js-action
widget-hd");
+ working_mode_switch.addClass("sprite edit js-action");

         var working_mode_switch_help =
Y.one(WORKING_MODE_SWITCH_HELP_ID);
         if (working_mode_switch_help !== null) {

=== modified file 'lib/lp/translations/help/wor...

[Matthew Revell (matthew.revell)]

review: Approve (documentation)

[Henning Eggers (henninge)]

Hi Adi,
thanks for doing this. It's a great help for reviewers (that want to translate)!

First off one UI comment: I think the naming text is wrong. It should not read "Enter ... mode" but simply display the current mode. This is consistent with the way the little yellow edit buttons work: You display a current setting and add an edit icon to change it. So if Danilo and Michael concur, I ask you to just display the current mode, e.g. "Reviewer mode (What's this?)".

When trying out the feature, the lack of hover interaction was the first thing that struck me as odd. No underlyning, no changing mouse cursor. At least that's what happens (not) on Firefox. It's easily fixed, though. See below.

The other two main issues are the structure of the JS code and the missing CSS on the help pop-up. Please see my comments about the JS code. The missing CSS should be easily fixed.

I am marking it as "Needs fixing" for me to see what JS code fixes you come up with. Please don't hesitate to talk to me about it as I am not the Great Javascript Expert and might be missing something.

Thank you for your work!

Henning

> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-26 23:26:18 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-16 09:09:12 +0000
> @@ -91,6 +91,90 @@
> });
> };
>
> +var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
> +var WORKING_MODE_SWITCH_HELP_ID = "#translation-switch-working-mode-help";
> +var WORKING_MODE_COOKIE = "translation-working-mode";
> +var WORKING_MODE_REVIEWER = "reviewer";
> +var WORKING_MODE_TRANSLATOR = "translator";

Add a blank line here, please.

> +var getWorkingMode = function () {
> + var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> + if (current_mode == WORKING_MODE_TRANSLATOR) {
> + return WORKING_MODE_TRANSLATOR;
> + } else {
> + return WORKING_MODE_REVIEWER;
> + }
> +};

I do not see the magic that this function does. Why not simply this:

var getWorkingMode = function () {
    retrurn Y.Cookie.get(WORKING_MODE_COOKIE);
}

> +
> +var setTranslatorModeText = function () {
> + switch_mode_text = Y.one(WORKING_MODE_SWITCH_ID);
> + if (switch_mode_text === null) {
> + Y.log("Could not find " + WORKING_MODE_SWITCH_ID);
> + return;
> + }
> + switch_mode_text.set(
> + 'innerHTML',
> + 'Enter reviewer mode');
> +};

I link to avoid "innerHTML" where possible. There is a "text" attribute, so
I think this should work.

    switch_mode_text.text = 'Enter reviewer mode';

> +
> +var setReviewerModeText = function () {
> + switch_mode_text = Y.one(WORKING_MODE_SWITCH_ID);
> + if (switch_mode_text === null) {
> + Y.log("Could not find " + WORKING_MODE_SWITCH_ID);
> + return;
> + }
> + switch_mode_text.set(
> + 'innerHTML',
> + 'Enter translator mode');
> +};

Likewise. And a lot of duplicate code, too. Could that not be parameterized
somehow?

> +
> +
> +var switchWorkingMode = function () {
> + if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> + setRev...

review: Needs Fixing (code)

[Adi Roiban (adiroiban)]

> Hi Adi,
> thanks for doing this. It's a great help for reviewers (that want to
> translate)!
>
> First off one UI comment: I think the naming text is wrong. It should not read
> "Enter ... mode" but simply display the current mode. This is consistent with
> the way the little yellow edit buttons work: You display a current setting and
> add an edit icon to change it. So if Danilo and Michael concur, I ask you to
> just display the current mode, e.g. "Reviewer mode (What's this?)".

Done.

There is another UI changes. I have set the text to "Reviewer&nbsp;mode&nbsp;(What's&nbsp;this?)", to make sure the help link is next to the switch...

> When trying out the feature, the lack of hover interaction was the first thing
> that struck me as odd. No underlyning, no changing mouse cursor. At least
> that's what happens (not) on Firefox. It's easily fixed, though. See below.

Yes. There is a problem with the CSS as I could not find an appropriate class for this element... right now I have added hd-widget as it was the closest match but maybe I should merge devel and see if the changes made to Curtis to the CSS can help.

> The other two main issues are the structure of the JS code and the missing CSS
> on the help pop-up. Please see my comments about the JS code. The missing CSS
> should be easily fixed.

CSS fixed.

JS is on it's way (please see my comments).

> I am marking it as "Needs fixing" for me to see what JS code fixes you come up
> with. Please don't hesitate to talk to me about it as I am not the Great
> Javascript Expert and might be missing something.
>
> Thank you for your work!
>
> Henning

[snip]

> > +var WORKING_MODE_REVIEWER = "reviewer";
> > +var WORKING_MODE_TRANSLATOR = "translator";
>
> Add a blank line here, please.

Done.

> > +var getWorkingMode = function () {
> > + var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> > + if (current_mode == WORKING_MODE_TRANSLATOR) {
> > + return WORKING_MODE_TRANSLATOR;
> > + } else {
> > + return WORKING_MODE_REVIEWER;
> > + }
> > +};
>
> I do not see the magic that this function does. Why not simply this:
>
> var getWorkingMode = function () {
> retrurn Y.Cookie.get(WORKING_MODE_COOKIE);
> }

:) Ok. Maybe I was a bit paranoid there.
The idea was to sanitize the WORKING_MODE_COOKIE. In case it contains any unknown values we play it safe and return WORKING_MODE_REVIEWER.

Am I to paranoid and should remove it ? :)

> > +
> > +var setTranslatorModeText = function () {
> > + switch_mode_text = Y.one(WORKING_MODE_SWITCH_ID);
> > + if (switch_mode_text === null) {
> > + Y.log("Could not find " + WORKING_MODE_SWITCH_ID);
> > + return;
> > + }
> > + switch_mode_text.set(
> > + 'innerHTML',
> > + 'Enter reviewer mode');
> > +};
>
> I link to avoid "innerHTML" where possible. There is a "text" attribute, so
> I think this should work.
>
> switch_mode_text.text = 'Enter reviewer mode';

That was also my first implementation but the text attribute is not working in this case.

I tries this in the browser js console:
>>> a = document.getElementById('translation-switch-working-mode')
>>> a.text
Reviewer mode
>>>...

[Henning Eggers (henninge)]

Thanks for improving this! I was wrong in some instances and you explained well. Good. ;)

There are merge conflicts in your branch. I based this review on a diff against a branch where I resolved those as thought it would be right. Please merge devel and resolve those, too. I'll leave this as "needs fixing" just because of that. All other changes are good.

Please see my final comments below and incorporate them.

Thanks,
Henning

> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-13 12:58:36 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-28 10:07:46 +0000
> @@ -92,6 +92,52 @@
> });
> };
>
> +var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
> +var WORKING_MODE_CONTAINER_ID = "#translation-switch-working-mode-container";
> +var WORKING_MODE_COOKIE = "translation-working-mode";
> +var WORKING_MODE_REVIEWER = "reviewer";
> +var WORKING_MODE_TRANSLATOR = "translator";
> +
> +var getWorkingMode = function () {

Sanatizing the cookie seems like a good idea but please add a short comment
about it.

> + var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> + if (current_mode == WORKING_MODE_TRANSLATOR) {
> + return WORKING_MODE_TRANSLATOR;
> + } else {
> + return WORKING_MODE_REVIEWER;
> + }
> +};
> +
> +var setWorkingMode = function (mode, text) {
> + Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
> + Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
> +};

I like the way you refactored this, very nice. ;) Let me just add one more
twist to it, if you don't mind.

I think 'text' does not need to be passed in but can be selected inside this
function. This way the actual strings are only defined here and not in two
places like you have it now.

var setWorkingMode = function (mode) {
    if(mode == WORKING_MODE_TRANSLATOR) {
        text = 'Translator&nbsp;mode';
    } else {
        text = 'Reviewer&nbsp;mode';
    }
    Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
    Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
};

> +

One new line too many.

> +
> +var switchWorkingMode = function () {
> + if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> + setWorkingMode(WORKING_MODE_REVIEWER, 'Reviewer&nbsp;mode');
> + } else {
> + setWorkingMode(WORKING_MODE_TRANSLATOR, 'Translator&nbsp;mode');
> + }
> +};

So, this becomes

var switchWorkingMode = function () {
    if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
        setWorkingMode(WORKING_MODE_REVIEWER);
    } else {
        setWorkingMode(WORKING_MODE_TRANSLATOR);
    }
};

> +
> +/**
> + * Initialize the current working mode and attach the node event for
> + * switching between modes.
> + */
> +var initializeWorkingMode = function () {
> +
> + Y.one(WORKING_MODE_CONTAINER_ID).removeClass('unseen');

This container is still conditional in TAL, don't you need to check for
its presence here? I think I was wrong to ask you why you keep checking
for it. This looks like a place where it should be done, I guess.

> + if (getWorkingMode() == WORKING_MODE_TRANSLAT...

[Adi Roiban (adiroiban)]

Hi,

Please see my inline comments.

On Wed, 2010-04-28 at 10:46 +0000, Henning Eggers wrote:
> Thanks for improving this! I was wrong in some instances and you explained well. Good. ;)
>
> There are merge conflicts in your branch. I based this review on a diff against a branch where I resolved those as thought it would be right. Please merge devel and resolve those, too. I'll leave this as "needs fixing" just because of that. All other changes are good.
>
> Please see my final comments below and incorporate them.
>
> Thanks,
> Henning
>
> > === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> > --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-13 12:58:36 +0000
> > +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-28 10:07:46 +0000
> > @@ -92,6 +92,52 @@
> > });
> > };
> >
> > +var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
> > +var WORKING_MODE_CONTAINER_ID = "#translation-switch-working-mode-container";
> > +var WORKING_MODE_COOKIE = "translation-working-mode";
> > +var WORKING_MODE_REVIEWER = "reviewer";
> > +var WORKING_MODE_TRANSLATOR = "translator";
> > +
> > +var getWorkingMode = function () {
>
> Sanatizing the cookie seems like a good idea but please add a short comment
> about it.

Done.

> > + var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
> > + if (current_mode == WORKING_MODE_TRANSLATOR) {
> > + return WORKING_MODE_TRANSLATOR;
> > + } else {
> > + return WORKING_MODE_REVIEWER;
> > + }
> > +};
> > +
> > +var setWorkingMode = function (mode, text) {
> > + Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
> > + Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
> > +};
>
> I like the way you refactored this, very nice. ;) Let me just add one more
> twist to it, if you don't mind.
>
> I think 'text' does not need to be passed in but can be selected inside this
> function. This way the actual strings are only defined here and not in two
> places like you have it now.
>
> var setWorkingMode = function (mode) {
> if(mode == WORKING_MODE_TRANSLATOR) {
> text = 'Translator&nbsp;mode';
> } else {
> text = 'Reviewer&nbsp;mode';
> }
> Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
> Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
> };
Done.
> > +
>
> One new line too many.
>
> > +
> > +var switchWorkingMode = function () {
> > + if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> > + setWorkingMode(WORKING_MODE_REVIEWER, 'Reviewer&nbsp;mode');
> > + } else {
> > + setWorkingMode(WORKING_MODE_TRANSLATOR, 'Translator&nbsp;mode');
> > + }
> > +};
>
> So, this becomes
>
> var switchWorkingMode = function () {
> if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
> setWorkingMode(WORKING_MODE_REVIEWER);
> } else {
> setWorkingMode(WORKING_MODE_TRANSLATOR);
> }
> };
Done.
>
> > +
> > +/**
> > + * Initialize the current working mode and attach the node event for
> > + * switching between modes.
> > + */
> > +var initializeWorkingMode = function () {
> > +
> > + Y.one(WORKING_MODE_CONTAINER_ID).remov...

[Henning Eggers (henninge)]

Great stuff Adi! This code is good to land. ;-)

You missed one suggestion of mine, though and fixed the code a few lines further down instead (in a good way, btw).

>> > + need_to_switch_mode = False
>> > + if reviewer and not current_is_reviewer:
>> > + need_to_switch_mode = True
>> > + if translator and current_is_reviewer:
>> > + need_to_switch_mode = True
>>
>> need_to_switch_mode = (
>> reviewer and not current_is_reviewer or
>> translator and current_is_reviewer)
>>
>> Using the intermediate variable is a very good decision, though!
>
> Fixed... in another way.

Err, sorry, you did not ... ;-)

Cheers,
Henning

review: Approve (code)

[Henning Eggers (henninge)]

Oh, and you forgot to merge devel and resolve the conflicts! Please do that.

[Adi Roiban (adiroiban)]

Missing fix should be now fixed.
Conflict solved and devel merged.

Cheers,
Adi

[Adi Roiban (adiroiban)]

Hi,

Here is the latest diff that fix the translation pagetests.

I don't know why the windmill tests failed during this run. I was not able to reproduce them on my computer. Maybe it was just a transient error.

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate.txt 2010-02-25 12:56:45 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate.txt 2010-04-29 11:46:29 +0000
@@ -42,7 +42,8 @@

 As an anynoymous user you will have access to the download and details
 pages for the pofile this message belongs to. The link to upload page
-should not be in that list.
+should not be in that list and so does the link for switching between
+translator and reviewer working mode.

     >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
     >>> print extract_text(nav)
@@ -84,11 +85,14 @@
   ... 'evolution/+pots/evolution-2.2/es/+translate')

 As a translation admin you will have access to the download, upload
-and details pages for the pofile this message belongs to.
+and details pages for the pofile this message belongs to. In the same time
+you have access to the link for switching between translator and reviewer
+working mode

   >>> nav = find_tag_by_id(admin_browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
   Download translation Upload translation Translation details
+ Reviewer mode (What's this?)

 All those links should linked the proper pages

=== modified file 'lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt'
--- lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt 2010-04-07 06:23:30 +0000
+++ lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt 2010-04-29 12:17:41 +0000
@@ -28,8 +28,9 @@
   ... raise AssertionError, 'Found input:\n%s' % input

 As an anynoymous user you will have access to the download and details
-pages for the pofile this message belongs to. The link to upload page
-should not be in that list.
+pages for the pofile this message belongs to. The link to upload page and
+the link for switching between translator and reviewer working mode will not
+be displayed in that list.

   >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
@@ -153,11 +154,14 @@
   http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/22/+translate

 As a translation admin you will have access to the download, upload
-and details pages for the pofile this message belongs to.
+and details pages for the pofile this message belongs to. In the same time
+you have access to the link for switching between translator and reviewer
+working mode

   >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
   Download translation Upload translation Translation details
+ Reviewer mode (What's this?)

 All those links should linked the proper pages

=== modified file 'lib/lp/translations/templates/translations-macros.pt'
--- lib/lp/translations/templates/translations-macros.pt 2010-04-28 02:04:57 +0000
+++ lib/lp/tr...

=== modified file 'lib/lp/translations/help/imported-upload.html'
--- lib/lp/translations/help/imported-upload.html	2010-02-19 12:05:10 +0000
+++ lib/lp/translations/help/imported-upload.html	2010-05-21 14:46:28 +0000
@@ -2,11 +2,11 @@
   <head>
     <title>Importing a .po file from upstream</title>
     <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssreset/reset.css" />
-    <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssfonts/fonts.css" />
-    <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssbase/base.css" />
+          href="/+icing/yui/cssreset/reset.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssfonts/fonts.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssbase/base.css" />
   </head>
   <body>
     <h1>Importing a .po file from upstream</h1>

=== modified file 'lib/lp/translations/help/permissions-policies.html'
--- lib/lp/translations/help/permissions-policies.html	2009-07-01 13:12:34 +0000
+++ lib/lp/translations/help/permissions-policies.html	2010-05-21 14:46:28 +0000
@@ -2,11 +2,11 @@
   <head>
     <title>Translation permission policies</title>
     <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssreset/reset.css" />
-    <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssfonts/fonts.css" />
-    <link rel="stylesheet" type="text/css"
-          href="/+icing/yui/current/build/cssbase/base.css" />
+          href="/+icing/yui/cssreset/reset.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssfonts/fonts.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssbase/base.css" />
   </head>
   <body>
     <h1>Translation permission policies</h1>

=== added file 'lib/lp/translations/help/working-modes.html'
--- lib/lp/translations/help/working-modes.html	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/help/working-modes.html	2010-05-21 14:46:28 +0000
@@ -0,0 +1,56 @@
+<html>
+  <head>
+    <title>Translation working modes</title>
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssreset/reset.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssfonts/fonts.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssbase/base.css" />
+  </head>
+  <body>
+    <h1>Switching between "translator" and "reviewer" modes</h1>
+
+    <p>
+      If you're making translations for a project that uses a <em>Structured
+      </em> or <em>Restricted</em> <a href=/+help/permissions-policies.html">
+      permissions policy</a> <strong>and</strong> you're a member of the
+      appropriate <a href=/+help/translation-groups.html">translation team</a>
+      you can work in one of two modes:
+    </p>
+
+    <ul>
+      <li>
+        <strong>Reviewer:</strong> This is the default working mode. All
+        changes you make are approved automatically. If you want to make a
+        suggestion, rather than a new translation, you need to manually select
+        the <em>Someone needs to review this translation</em> check-box.
+      </li>
+      <li>
+        <strong>Translator:</strong> When entering new translations, they're
+        treated as suggestions that someone else needs to review. You can make
+        a direct translation by deselecting the <em>Someone needs to review
+        this translation</em> check-box.</li>
+    </ul>
+
+    <h2>How this affects team workflows</h2>
+
+    <p>
+      When you're reviewing translations made by other people, or you're
+      working in a small team, you usually want your translations to be applied
+      automatically. This is when you'd choose <em>Reviewer</em> mode.
+    </p>
+
+    <p>
+      However, if you're working in a team that uses a peer review process
+      <em>Translator</em> mode will be more suitable as it holds each of your
+      translations as suggestions that should be reviewed by someone else.
+    </p>
+
+    <p>
+      There's <a href="https://help.launchpad.net/Translations"
+      target="_blank">more help with translations</a> in our help wiki.
+    </p>
+
+  </body>
+</html>

=== modified file 'lib/lp/translations/javascript/pofile.js'
--- lib/lp/translations/javascript/pofile.js	2010-05-18 18:04:00 +0000
+++ lib/lp/translations/javascript/pofile.js	2010-05-21 14:46:28 +0000
@@ -94,6 +94,59 @@
   });
 };
 
+var WORKING_MODE_SWITCH_ID = "#translation-switch-working-mode";
+var WORKING_MODE_CONTAINER_ID = "#translation-switch-working-mode-container";
+var WORKING_MODE_COOKIE = "translation-working-mode";
+var WORKING_MODE_REVIEWER = "reviewer";
+var WORKING_MODE_TRANSLATOR = "translator";
+
+/*
+ * This function is sanitizing the WORKING_MODE_COOKIE and in case it contains
+ * unknow values, we play it safe by returning WORKING_MODE_REVIEWER, which
+ * is the default mode.
+ */
+var getWorkingMode = function () {
+    var current_mode = Y.Cookie.get(WORKING_MODE_COOKIE);
+    if (current_mode == WORKING_MODE_TRANSLATOR) {
+        return WORKING_MODE_TRANSLATOR;
+    } else {
+        return WORKING_MODE_REVIEWER;
+    }
+};
+
+var setWorkingMode = function (mode) {
+    if(mode == WORKING_MODE_TRANSLATOR) {
+        text = 'Translator&nbsp;mode';
+    } else {
+        text = 'Reviewer&nbsp;mode';
+    }
+    Y.one(WORKING_MODE_SWITCH_ID).set('innerHTML', text);
+    Y.Cookie.set(WORKING_MODE_COOKIE, mode, {path: "/"});
+};
+
+var switchWorkingMode = function () {
+    if (getWorkingMode() == WORKING_MODE_TRANSLATOR) {
+        setWorkingMode(WORKING_MODE_REVIEWER);
+    } else {
+        setWorkingMode(WORKING_MODE_TRANSLATOR);
+    }
+};
+
+
+/**
+ * Initialize the current working mode and attach the node event for
+ * switching between modes.
+ */
+var initializeWorkingMode = function () {
+
+    var working_mode = Y.one(WORKING_MODE_CONTAINER_ID);
+    if (working_mode !== null) {
+        working_mode.removeClass('unseen');
+        setWorkingMode(getWorkingMode());
+        Y.on("click", switchWorkingMode, WORKING_MODE_SWITCH_ID);
+    }
+};
+
 
 var setFocus = function(field) {
     // if there is nofield, do nothing
@@ -199,6 +252,23 @@
     //  * msgset_2_pt_BR_translation_0_new_select
     var translation_select_id = field + '_select';
     selectWidgetByID(translation_select_id);
+
+    var working_mode_switch = Y.one(WORKING_MODE_SWITCH_ID);
+    // Autoselect the force suggestion checkbox only if working in translator
+    // mode and the switch is on the page.
+    if (working_mode_switch !== null &&
+        getWorkingMode() == WORKING_MODE_TRANSLATOR) {
+        var translation_field = Y.one('#' + field);
+        if (translation_field !== null &&
+            translation_field.get('value') === '') {
+            var html_parts = field.split('_');
+            var force_suggestion = (
+                html_parts[0] + '_' + html_parts[1] +
+                '_force_suggestion');
+            selectWidgetByID(force_suggestion);
+        }
+    }
+
 };
 
 
@@ -504,6 +574,12 @@
     }
 
     try {
+    initializeWorkingMode();
+    } catch (initialize_working_mode_errors) {
+      Y.log(initialize_working_mode_errors, "error");
+    }
+
+    try {
       setFocus(autofocus_field);
     } catch (e) {
       Y.log(e, "error");
@@ -540,4 +616,4 @@
 };
 
 }, "0.1", {"requires": ["event", "event-key", "node", "cookie", "anim"]});
-
+z

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate.txt	2010-02-25 12:56:45 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate.txt	2010-05-21 14:46:28 +0000
@@ -42,7 +42,8 @@
 
 As an anynoymous user you will have access to the download and details
 pages for the pofile this message belongs to. The link to upload page
-should not be in that list.
+should not be in that list and so does the link for switching between
+translator and reviewer working mode.
 
     >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
     >>> print extract_text(nav)
@@ -84,11 +85,14 @@
   ...     'evolution/+pots/evolution-2.2/es/+translate')
 
 As a translation admin you will have access to the download, upload
-and details pages for the pofile this message belongs to.
+and details pages for the pofile this message belongs to. In the same time
+you have access to the link for switching between translator and reviewer
+working mode
 
   >>> nav = find_tag_by_id(admin_browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
   Download translation Upload translation Translation details
+  Reviewer mode (What's this?)
 
 All those links should linked the proper pages
 

=== modified file 'lib/lp/translations/stories/standalone/xx-translation-access-display.txt'
--- lib/lp/translations/stories/standalone/xx-translation-access-display.txt	2009-07-02 17:16:50 +0000
+++ lib/lp/translations/stories/standalone/xx-translation-access-display.txt	2010-05-21 14:46:28 +0000
@@ -22,6 +22,14 @@
     >>> print_tag(admin_browser.contents, 'translation-access')
     You have full access to this translation.
 
+Users with full access will also have an option to switch between translator
+and reviewer working mode.
+
+    >>> switch_working_mode = find_tag_by_id(
+    ...     admin_browser.contents,
+    ...     'translation-switch-working-mode')
+    >>> switch_working_mode is not None
+    True
 
 == Displaying translation groups and reviewers ==
 
@@ -106,6 +114,15 @@
     Your suggestions will be held for review by the managers of this
     translation.
 
+Users without full access will not have an option to switch between translator
+and reviewer working mode.
+
+    >>> switch_working_mode = find_tag_by_id(
+    ...     user_browser.contents,
+    ...     'translation-switch-working-mode')
+    >>> switch_working_mode is not None
+    False
+
 If Evolution's translation is set to Closed mode, Joe will not be able
 to submit suggestions.
 
@@ -118,6 +135,15 @@
     >>> print_tag(user_browser.contents, 'translation-access')
     This template can be translated only by its managers.
 
+If users are not allowed to submit suggestions, they will also not have an
+option to switch between translator and reviewer working mode.
+
+    >>> switch_working_mode = find_tag_by_id(
+    ...     user_browser.contents,
+    ...     'translation-switch-working-mode')
+    >>> switch_working_mode is not None
+    False
+
 There is a special case where Joe visits a translation into a language
 that isn't covered by the translation group: Joe is told he cannot enter
 translations, and invited to contact the translation group about setting
@@ -138,7 +164,8 @@
 
 Finally, if there is no translation group at all and the permissions do
 not allow Joe to translate, the page shows that the translation is
-closed.
+closed and no option to switch between translator and reviewer working mode
+will be displayed.
 
     >>> evolution.translationpermission = TranslationPermission.CLOSED
     >>> evolution.translationgroup = None
@@ -147,3 +174,9 @@
     ...     'evolution/trunk/+pots/evolution-2.2/es/+translate')
     >>> print_tag(user_browser.contents, 'translation-access')
     This translation is not open for changes.
+
+    >>> switch_working_mode = find_tag_by_id(
+    ...     user_browser.contents,
+    ...     'translation-switch-working-mode')
+    >>> switch_working_mode is not None
+    False

=== modified file 'lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt'
--- lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt	2010-04-07 06:23:30 +0000
+++ lib/lp/translations/stories/standalone/xx-translationmessage-translate.txt	2010-05-21 14:46:28 +0000
@@ -28,8 +28,9 @@
   ...     raise AssertionError, 'Found input:\n%s' % input
 
 As an anynoymous user you will have access to the download and details
-pages for the pofile this message belongs to. The link to upload page
-should not be in that list.
+pages for the pofile this message belongs to. The link to upload page and
+the link for switching between translator and reviewer working mode will not
+be displayed in that list.
 
   >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
@@ -153,11 +154,14 @@
   http://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/evolution-2.2/es/22/+translate
 
 As a translation admin you will have access to the download, upload
-and details pages for the pofile this message belongs to.
+and details pages for the pofile this message belongs to. In the same time
+you have access to the link for switching between translator and reviewer
+working mode
 
   >>> nav = find_tag_by_id(browser.contents, 'nav-pofile-subpages')
   >>> print extract_text(nav)
   Download translation Upload translation Translation details
+  Reviewer mode (What's this?)
 
 All those links should linked the proper pages
 

=== modified file 'lib/lp/translations/templates/translations-macros.pt'
--- lib/lp/translations/templates/translations-macros.pt	2010-05-18 18:04:00 +0000
+++ lib/lp/translations/templates/translations-macros.pt	2010-05-21 14:46:28 +0000
@@ -126,6 +126,17 @@
             tal:define="link navigation_menu/details"
             tal:condition="link/enabled"
             tal:content="structure link/render"></li>
+        <li tal:condition="view/user_is_official_translator"
+            id="translation-switch-working-mode-container"
+            class="unseen">
+          <span>
+          <a id="translation-switch-working-mode"
+             class="sprite edit js-action widget-hd">
+             Reviewer mode</a>&nbsp;<!--
+            -->(<a href="/+help/working-modes.html" target="help"><!--
+                -->What's&nbsp;this?</a>)
+          </span>
+        </li>
       </ul>
 </metal:nav-pofile-subpages>
 

=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
--- lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-04-23 16:09:20 +0000
+++ lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-05-21 14:46:28 +0000
@@ -396,3 +396,92 @@
             id=u'msgset_138_es_translation_0_new_select')
         client.asserts.assertNotChecked(
             id=u'msgset_139_es_translation_0_new_select')
+
+
+class POFileTranslatorAndReviewerWorkingMode(WindmillTestCase):
+    """Tests for page behaviour in reviewer or translator mode."""
+
+    layer = TranslationsWindmillLayer
+    suite_name = 'POFile Translate'
+
+    test_user = lpuser.TRANSLATIONS_ADMIN
+
+    switch_working_mode = u'translation-switch-working-mode'
+    force_suggestion = u'msgset_1_force_suggestion'
+    new_translation = u'msgset_1_pt_BR_translation_0_new'
+    js_code = ("lookupNode({id: '%s'}).innerHTML.search('eviewer') > 0" %
+                switch_working_mode)
+
+    def test_pofile_reviewer_mode(self):
+        """Test for reviewer mode.
+
+        Adding new translations will force them as suggestions.
+        """
+
+        self.client.open(
+            url='http://translations.launchpad.dev:8085/'
+            'evolution/trunk/+pots/evolution-2.2/pt_BR/1/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        self.test_user.ensure_login(self.client)
+
+        self._ensureTranslationMode(reviewer=True)
+
+        self.client.waits.forElement(
+            id=self.force_suggestion, timeout=constants.FOR_ELEMENT)
+        self.client.type(text=u'New translation', id=self.new_translation)
+        self.client.asserts.assertNotChecked(id=self.force_suggestion)
+
+    def test_pofile_translator_mode(self):
+        """Test for translator mode.
+
+        Adding new translations will not force them as suggestions.
+        """
+
+        self.client.open(
+            url='http://translations.launchpad.dev:8085'
+            '/evolution/trunk/+pots/evolution-2.2/pt_BR/1/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        self.test_user.ensure_login(self.client)
+
+        self._ensureTranslationMode(translator=True)
+
+        self.client.waits.forElement(
+            id=self.force_suggestion, timeout=constants.FOR_ELEMENT)
+        self.client.type(text=u'New translation', id=self.new_translation)
+        self.client.asserts.assertChecked(id=self.force_suggestion)
+
+        # The new translation will be forced only if the previous new
+        # translation field is empty. Othewise the force suggestion checkbox
+        # will remain unchecked.
+        self.client.click(id=self.force_suggestion)
+        self.client.keyPress(
+            id=self.new_translation,
+            options='a,true,false,false,false,false')
+        self.client.asserts.assertNotChecked(id=self.force_suggestion)
+
+    def _ensureTranslationMode(self, reviewer=False, translator=False):
+        """Ensure the specified mode is currently selected."""
+
+        if (reviewer is translator):
+            raise AssertionError("You must specify a single working mode.")
+
+        self.client.waits.forElement(
+            id=self.switch_working_mode, timeout=constants.FOR_ELEMENT)
+
+        current_is_reviewer = self.client.execJS(js=self.js_code)['output']
+        need_to_switch_mode = (
+            reviewer and not current_is_reviewer or
+            translator and current_is_reviewer)
+        if need_to_switch_mode:
+            self.client.click(id=self.switch_working_mode)
+        else:
+            return
+
+        # We check that the mode was changed.
+        current_is_reviewer = self.client.execJS(js=self.js_code)['output']
+
+        switch_done = (
+            reviewer and current_is_reviewer or
+            translator and not current_is_reviewer)
+        assert switch_done is True, "Could not switch working mode."
+