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

[Adi Roiban]

Allow reseting the review state for local suggestions of a potmsgset. Landed by henninge.

= Bug 201749 =
Take for example this string:

https://translations.edge.launchpad.net/ubuntu/hardy/+source/system-config-printer/+pots/system-config-printer/it/287/+translate

This has been translated by an approved translator, but it's impossibile
to set it with the "Someone should review this translation". I did try a
moment ago, setting it and the filtering with the "need review" filter
in the "Show" drop-down list.

Another example:

https://translations.edge.launchpad.net/ubuntu/hardy/+source/ubuntu-docs/+pots/desktop-effects/it/+translate?show=all&start=10

I marked with "Someone should..." number 15 and 16 and saved, but they
never show up with the "need review" filter.

== Proposed fix ==
If a reviewer is only ticking „needs review”, or submitting an empty translations together with marking it as „needs review”, reset the review state for the current message so that all suggestions can be viewed.

Submitting a new translation (different than the current one) will add it as a new suggestion and will not reset the reviewed translations.

== Pre-implementation notes ==
The logs are pasted on this bug report
https://bugs.edge.launchpad.net/rosetta/+bug/201749

== Implementation details ==
potmsgset.resetCurrentTranslation method has the pofile and suggestion as argument to simplify writing the tests.

translationmessage._storeTranslations already contains a lot of logic and will make those tests very complicated.

I am not sure what changes should I make on a potmsgset to be listed again as „needs review”.
The current implementation just updated the date_created ... but maybe there is a better way of doing that without faking the date_created.

== Tests ==
./bin/test -t potmsgset -t translationmessage

== Demo and Q/A ==

Login in launchpad translation as an reviewer (or admin).
Go to a translation, ie:
https://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/man/es/1/+translate

Submit a new translation ie „some bla bla”, without ticking the „Someone should review this translation” checkbox.
Press „Save and continue” and you should see that „some bla bla” is the current translation.

Not submit a new translation „other bla bla”, but this time tick „Someone should review this translation”.
The current translation should remain „some bla bla” but you should see „other bla bla” listed as a suggestion.

Tick „Confirm this translation and dismiss all suggestions.”
„some bla bla” should be the current translation, why there will be no suggestions.

Next, tick „Someone should review this translation” but without adding any new translation.
The current translation should be empty, while „some bla bla” and „other bla bla” are listed as suggestions.

Add a new translation „diverged bla bla” without ticking „Someone should review this translation”.
You should see that „diverged bla bla” is the current translation, and for the new suggestion you only have the „Someone should review this translation” option, while „Make this translation specific to Ubuntu hoary” is no longer available.

Reset the translation one again by submitting a new empty translation „” (the empty string) and ticking „Someone should review this translation”.
You should see that current translation is empty, while all previously entered translations are listed as suggestions.

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 12.03.2010 16:42, Adi Roiban schrieb:
Hi Adi,
thanks for tackling this. There seem to have been some misunderstanding
about how this option worked, judging by the comments in the bug.

I am a little concerned about the behaviour. Shouldn't the radio button
in front of the input box need to be activated (see below)? Also, I am
not sure if really _all_ old suggestions should be brought up again. But
that may be OK.

I also have some other suggestions. Please look at them and we can talk
again tomorrow.

 review needs-fixing

Please find my comments below.

Cheers,
Hennning

> === modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
> --- lib/lp/translations/browser/tests/translationmessage-views.txt 2009-09-11 22:24:51 +0000
> +++ lib/lp/translations/browser/tests/translationmessage-views.txt 2010-03-15 13:43:34 +0000
> @@ -689,3 +689,108 @@
> False
> >>> subview.shared_translationmessage == translationmessage
> True
> +
> +
> +Reseting current translation

Resetting ...

> +-----------------------------
> +
> +The current translation can be resetted by submiting an empty translation or
> +a translation similar to the current one, while forcing the suggestions.

The current translation can be reset by submitting an empty translation
or a translation similar to the current one while forcing suggestions.

> +
> + >>> # We need to force timestamp update, since suggestions are submitted
> + >>> # to fast

Please don't put comments in doc tests. Whatever goes into the comment
can go into the normal text. It's "too fast", btw. ;)

> + >>> year_tick = 2100
> + >>> def submit_translation(translation, force_suggestion=False):
> + ... global year_tick
> + ... translationmessage = TranslationMessage.get(1)

I find this very obscure. I think it would be clearer to pass
translationmessage as a parameter into the function. If you want "any"
TranslationMessage object, use the factory outside and pass the object
into the function.

> + ... pofile = translationmessage.pofile
> + ... potmsgset = translationmessage.potmsgset
> + ... server_url = '/'.join(
> + ... [canonical_url(translationmessage), '+translate'])
> + ... now = (str(year_tick) +
> + ... datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
> + ... year_tick += 1
> + ... form = {
> + ... 'lock_timestamp': now,
> + ... 'alt': None,
> + ... 'msgset_1': None,
> + ... 'msgset_1_es_translation_0_radiobutton':
> + ... 'msgset_1_es_translation_0_new',
> + ... 'msgset_1_es_translation_0_new': translation,
> + ... 'submit_translations': 'Save & Continue'}
> + ... if force_suggestion:
> + ... form['msgset_1_es_needsreview'] = 'force_suggestion'
> + ... tm_view = create_view(
> + ... translationmessage, "+translate", form=form,
> + ... layer=TranslationsLayer, server_url=server_url)
> + ... tm_view.request.method = 'POST'
> + ... tm_view.initialize()
> +
> + >>> def print_current_trans...

review: Needs Fixing

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

Henning said:

> So, this brings up all old translations? Is that really what we want?

I suggested Adi work in phases: this _might_ be the first step, but the natural next step would be to set the newly set empty message as "date_reviewed = previous_current.date_created - timedelta(0,1)" where we wouldn't really show all suggestions, but only those created after the previous current. Not perfect, but at least we won't have to forge the date_created on the previous_current so it shows up in the list of suggestions.

That's the other option, without making this too complex, but I leave that to the two of you to decide which is better.

[Adi Roiban (adiroiban)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 12.03.2010 16:42, Adi Roiban schrieb:
> Hi Adi,
> thanks for tackling this. There seem to have been some misunderstanding
> about how this option worked, judging by the comments in the bug.
>
> I am a little concerned about the behaviour. Shouldn't the radio button
> in front of the input box need to be activated (see below)? Also, I am
> not sure if really _all_ old suggestions should be brought up again. But
> that may be OK.
This implementation should reset the current translation as there is no other workaround for the following use case:
https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/14

In an ideal world, we would have a "is_reviewed" field for each translation message, a dedicated "reset translation" checkbox and a way to see all previously entered suggestions.

I was thinking there is no need to force a user to add an empty text and mark it as a suggestion and it should be enough to just tick the "needs review" checkbox.

> I also have some other suggestions. Please look at them and we can talk
> again tomorrow.
Many thanks for your review.

Please see my inline comments.

[snip]
> > +Reseting current translation
>
> Resetting ...
Fixed.

> > +-----------------------------
> > +
> > +The current translation can be resetted by submiting an empty translation
> or
> > +a translation similar to the current one, while forcing the suggestions.
>
> The current translation can be reset by submitting an empty translation
> or a translation similar to the current one while forcing suggestions.

Fixed.

> > +
> > + >>> # We need to force timestamp update, since suggestions are
> submitted
> > + >>> # to fast
>
> Please don't put comments in doc tests. Whatever goes into the comment
> can go into the normal text. It's "too fast", btw. ;)

Done.

> > + >>> year_tick = 2100
> > + >>> def submit_translation(translation, force_suggestion=False):
> > + ... global year_tick
> > + ... translationmessage = TranslationMessage.get(1)
>
> I find this very obscure. I think it would be clearer to pass
> translationmessage as a parameter into the function. If you want "any"
> TranslationMessage object, use the factory outside and pass the object
> into the function.
I was getting the translation message with id 1 to simplify the form submission, since
the form names depend on the id, and also since translation permission were already there.

I have rewritten the code to take a translation message as parameter and only use generated data.
[snip]
> > +
> > +Same happens if we force the submission of a suggestion similar with the
>
> ... similar to the ...

Done.

[snip]
> > + if (force_suggestion and
> > + self.user_is_official_translator and
> > + (self._areSuggestionsEmpty(translations) or
> > + (translationmessage is not None and
> > + self._areSuggestionsIdenticalToTranslations(translations,
> > + translationmessage.translations)))):
>
> This is not very easy to read, right? I suggest you create a few
> intermediate boolean values so that they all fit on one li...

[Henning Eggers (henninge)]

Thanks Adi,
you have done a good job restructering the test, I did not intend to put that much extra work on you. Thanks.

Two suggestions on the patch:
 * msgset_stem is not clear to me. Is that a Hungarian word there? ;-) Just msgset_id would be clearer, I gues.
 * Maybe _isNoTranslationConflict is not really a boolean function? The way you use it now, i.e. outside an "if" statement looks much better. I'd suggest doing the same thing at the other call site and rename it to "_maybeRaiseTranslationConflict" or "_checkTranslationConflict". I prefer the first because it is clear that this may raise an exception.

Ok, about the remaining problem.
> I was thinking there is no need to force a user to add an empty
> text and mark it as a suggestion and it should be enough to just
> tick the "needs review" checkbox.

Yes, we agree here. But still the radio button should move to the empty input field, I think. This way it is clear that the current translation will change - to nothing.
There is this big misunderstanding that you and others had about the meaning of "Someone should review..." because it was always only intended for newly entered translations. The use that you added now is a new feature, not a fix of a buggy feature.
That is why I think moving the radio button down to the input field will keep the original intention that the checkbox relates to the input box, not the current translation, while adding the extra feature of resetting the current translatoin.

I know this requires some javascript. I don't know how comfortable you are with writing that. If you want to you can file an extra bug for it which should be fixed this cycle. You can land this branch as it is (see above suggestions) but it would be great if that extra javascript would be in it. ;-)

Cheers,
Henning

review: Approve (code)

[Adi Roiban (adiroiban)]

> Thanks Adi,
> you have done a good job restructering the test, I did not intend to put that
> much extra work on you. Thanks.
>
> Two suggestions on the patch:
> * msgset_stem is not clear to me. Is that a Hungarian word there? ;-) Just
> msgset_id would be clearer, I gues.

I have renamed it to msgset_id.
I was using „stem” as root, (linguistics) the form of a word after all affixes are removed; "thematic vowels are part of the stem"

> * Maybe _isNoTranslationConflict is not really a boolean function? The way
> you use it now, i.e. outside an "if" statement looks much better. I'd suggest
> doing the same thing at the other call site and rename it to
> "_maybeRaiseTranslationConflict" or "_checkTranslationConflict". I prefer the
> first because it is clear that this may raise an exception.

Renamed to _maybeRaiseTranslationConflict and moved it outside of if-statement.

> Ok, about the remaining problem.
> > I was thinking there is no need to force a user to add an empty
> > text and mark it as a suggestion and it should be enough to just
> > tick the "needs review" checkbox.
>
> Yes, we agree here. But still the radio button should move to the empty input
> field, I think. This way it is clear that the current translation will change
> - to nothing.
> There is this big misunderstanding that you and others had about the meaning
> of "Someone should review..." because it was always only intended for newly
> entered translations. The use that you added now is a new feature, not a fix
> of a buggy feature.
> That is why I think moving the radio button down to the input field will keep
> the original intention that the checkbox relates to the input box, not the
> current translation, while adding the extra feature of resetting the current
> translatoin.
Yes, this is a new functionality and maybe I should open a new bug for this branch.

While we don't have the „is_reviewed” field in the DB and the +translate page is not redesigned, this can be a workaround for this bug.

> I know this requires some javascript. I don't know how comfortable you are
> with writing that. If you want to you can file an extra bug for it which
> should be fixed this cycle. You can land this branch as it is (see above
> suggestions) but it would be great if that extra javascript would be in it.
> ;-)
I have added the required javascript.

I don't know how to include the windmill test since it depends on this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-540105/+merge/21552.

Maybe I will have to wait for the landing of that branch and then merge the windmill test for this branch and do a bit of refactoring for the initilization of Javascript code on this page.
Here is the latest diff
=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-02-25 12:56:45 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-22 17:17:22 +0000
@@ -321,12 +321,41 @@
                 singular_copy_text);
         }
     }
-}
+};

+var initializeResetBehavior = function (fields) {
+ for (var key = 0; key < fields.length; key++) {
+ var html_parts ...

[Adi Roiban (adiroiban)]

Here are the Windmill tests and the changes in the pofile and translation +message javascript initialization.

=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
--- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-17 17:33:04 +0000
+++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-22 23:02:19 +0000
@@ -50,3 +50,132 @@

         # Check that the associated radio button is selected.
         client.asserts.assertChecked(id=radiobutton_id)
+
+ def _check_reset_translation_select(
+ self, client, checkbox, singular_select, singular_current,
+ plural_select=None):
+ """Checks that the new translation select radio buttons are checked
+ when ticking 'Someone should review this translation' checkbox.
+ """
+
+ client.waits.forElement(
+ id=checkbox, timeout=constants.FOR_ELEMENT)
+ client.waits.forElement(
+ id=singular_select, timeout=constants.FOR_ELEMENT)
+ if plural_select is not None:
+ client.waits.forElement(
+ id=plural_select, timeout=constants.FOR_ELEMENT)
+
+ # Check that initialy the checkbox is not checked and
+ # that the radio buttons are not selected.
+ client.asserts.assertNotChecked(id=checkbox)
+ client.asserts.assertNotChecked(id=singular_select)
+ client.asserts.assertChecked(id=singular_current)
+ if plural_select is not None:
+ client.asserts.assertNotChecked(id=plural_select)
+
+ # Check the checkbox
+ client.click(id=checkbox)
+
+ # Check that the checkbox and the new translation radio buttons are
+ # selected.
+ client.asserts.assertChecked(id=checkbox)
+ client.asserts.assertChecked(id=singular_select)
+ client.asserts.assertNotChecked(id=singular_current)
+ if plural_select is not None:
+ client.asserts.assertChecked(id=plural_select)
+
+ # We select the current translation for the singular form.
+ client.click(id=singular_current)
+
+ # Then then we uncheck the 'Someone needs to review this translation'
+ # checkbox.
+ client.click(id=checkbox)
+
+ # Unchecking the 'Someone needs to review this translation' checkbox
+ # will not change the state of the radio buttons.
+ client.asserts.assertNotChecked(id=checkbox)
+ client.asserts.assertNotChecked(id=singular_select)
+ client.asserts.assertChecked(id=singular_current)
+ if plural_select is not None:
+ client.asserts.assertChecked(id=plural_select)
+
+ def test_pofile_reset_translation_select(self):
+ """Test for automatically selecting new translation when
+ 'Someone needs to review this translations' is checked.
+ """
+ client = self.client
+ user = lpuser.TRANSLATIONS_ADMIN
+
+ # Go to the zoom in page for a translation with plural forms.
+ self.client.open(
+ url='http://translations.launchpad.dev:8085/'
+ 'ubuntu/hoary/+source/evolution/+pots/'
+ 'evolution-2.2/es/15/+transl...

[Henning Eggers (henninge)]

m 23.03.2010 01:31, schrieb Adi Roiban:
> Here are the Windmill tests and the changes in the pofile and translation +message javascript initialization.

Thanks Adi, looks very good to me. I just have one request.

>
> === modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
> --- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-17 17:33:04 +0000
> +++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-22 23:02:19 +0000
> @@ -50,3 +50,132 @@
>
> # Check that the associated radio button is selected.
> client.asserts.assertChecked(id=radiobutton_id)
> +
> + def _check_reset_translation_select(
[...]
> + # Then then we uncheck the 'Someone needs to review this translation'
> + # checkbox.
> + client.click(id=checkbox)
> +
> + # Unchecking the 'Someone needs to review this translation' checkbox
> + # will not change the state of the radio buttons.

I think that this will lead to accidental unsetting of translations. Someone going through translations and clicking on "Someone should ..." but then changes his mind will most likely forget to move the radio button back to preserve the current translation. The radio button should jump back to the current translation if the text field is empty (only then!).

See the code below.

> + client.asserts.assertNotChecked(id=checkbox)
> + client.asserts.assertNotChecked(id=singular_select)
> + client.asserts.assertChecked(id=singular_current)
> + if plural_select is not None:
> + client.asserts.assertChecked(id=plural_select)
> +
> + def test_pofile_reset_translation_select(self):
[...]

> === modified file 'lib/lp/translations/templates/pofile-translate.pt'
> === modified file 'lib/lp/translations/templates/translationmessage-translate.pt'

Thanks for moving the code out of the templates!

> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
[...]

> +
> +var initializeResetBehavior = function (fields) {
> + for (var key = 0; key< fields.length; key++) {
> + var html_parts = fields[key].split('_');
> + var msgset_id = html_parts[0] + '_' + html_parts[1];
> + var node = Y.one('#' + msgset_id + '_force_suggestion');
> + if (node === null) {
> + // If we don't have a force_suggestion checkbox associated with
> + // this field, just continue to the next field.
> + break;
> + }
> + Y.on(
> + 'click',
> + function (e, translation_id) {
> + if (this === null) {
> + // Don't do nothing if we don't have a context object.
> + return;
> + }
> + if (this.get('checked') == true) {
> + var select = Y.one('#' + translation_id + '_select');
> + if (select !== null) {
> + select.set('checked', true);
> + }

I was thinking about how this would be done and came up with this code. Feel free to use it or find your own solution ... ;-)

               } else {
                   var inputf...

review: Approve (code)

[Adi Roiban (adiroiban)]

Hi Henning,

I'm back in business.

Here are the changes after applying your JS code and updating the windmill tests.

I have also merged with devel.

If everything is ok, can you please send this branch to ec2-test?

Thanks!
=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-23 00:22:40 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-11 23:07:06 +0000
@@ -374,10 +374,25 @@
                     return;
                 }
                 if (this.get('checked') == true) {
+ // When checking the "Need review" checkbox the new
+ // translation should be automatically checked.
                     var select = Y.one('#' + translation_id + '_select');
                     if (select !== null) {
                         select.set('checked', true);
                     }
+ } else {
+ // When unchecking the "Need review" checkbox and the new
+ // translation is empty, the current translation should be
+ // automatically selected.
+ var inputfield = Y.one('#' + translation_id);
+ if (inputfield !== null && inputfield.get('value') == '') {
+ var current_radio_id = translation_id.replace(
+ /_new$/, '_radiobutton');
+ var current_radio = Y.one('#' + current_radio_id);
+ if (current_radio !== null) {
+ current_radio.set('checked', true);
+ }
+ }
                 }
             },
             node , node, fields[key]

=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
--- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-23 00:22:40 +0000
+++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-11 23:40:51 +0000
@@ -52,8 +52,8 @@
         client.asserts.assertChecked(id=radiobutton_id)

     def _check_reset_translation_select(
- self, client, checkbox, singular_select, singular_current,
- plural_select=None):
+ self, client, checkbox, singular_new_select, singular_current_select,
+ singular_new_field=None, plural_new_select=None):
         """Checks that the new translation select radio buttons are checked
         when ticking 'Someone should review this translation' checkbox.
         """
@@ -61,44 +61,61 @@
         client.waits.forElement(
             id=checkbox, timeout=constants.FOR_ELEMENT)
         client.waits.forElement(
- id=singular_select, timeout=constants.FOR_ELEMENT)
- if plural_select is not None:
- client.waits.forElement(
- id=plural_select, timeout=constants.FOR_ELEMENT)
+ id=singular_new_select, timeout=constants.FOR_ELEMENT)
+ client.waits.forElement(
+ id=singular_current_select, timeout=constants.FOR_ELEMENT)
+ if plural_new_select is not None:
+ client.waits.forElement(
+ id=plural_new_select, timeou...

[Henning Eggers (henninge)]

Am 12.04.2010 02:39, schrieb Adi Roiban:
> I'm back in business.

Good to hear! ;-)

>
> Here are the changes after applying your JS code and updating the windmill tests.

Thanks for updating those tests. I actually forgot to try my code on plural
forms but I am glad to see that works, too. ;-)

>
> I have also merged with devel.
>
> If everything is ok, can you please send this branch to ec2-test?

I will but I have two little commenting and naming complaints that you should
please take care of first.

>
> Thanks!

You are most welcome! ;-)

Henning

> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-23 00:22:40 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-11 23:07:06 +0000
> @@ -374,10 +374,25 @@
> return;
> }
> if (this.get('checked') == true) {
> + // When checking the "Need review" checkbox the new
> + // translation should be automatically checked.

Please don't use "should" in comments, this is not a test. ;-)

Also, it is not necessary to explain what the code does unless the code is
really complicated or some special condition/work-around/bug etc has to be
known to understand the code.

Generally, the code should speak for itself. If you feel the code does not,
maybe a variable needs a more descriptive name?

> var select = Y.one('#' + translation_id + '_select');
> if (select !== null) {
> select.set('checked', true);
> }
> + } else {
> + // When unchecking the "Need review" checkbox and the new
> + // translation is empty, the current translation should be
> + // automatically selected.

Same here.

> + var inputfield = Y.one('#' + translation_id);
> + if (inputfield !== null&& inputfield.get('value') == '') {
> + var current_radio_id = translation_id.replace(
> + /_new$/, '_radiobutton');
> + var current_radio = Y.one('#' + current_radio_id);
> + if (current_radio !== null) {
> + current_radio.set('checked', true);
> + }
> + }
> }
> },
> node , node, fields[key]
>
> === modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
> --- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-23 00:22:40 +0000
> +++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-11 23:40:51 +0000
> @@ -52,8 +52,8 @@
> client.asserts.assertChecked(id=radiobutton_id)
>
> def _check_reset_translation_select(

Sorry, I did not notice this before. Methods are always camelCase, functions
and variables use_underscores. It's in the style guide ... ;-)

       def _checkResetTranslationSelect(

That's all.

[Adi Roiban (adiroiban)]

I have renamed the method.

I have also added a test for pt_BR, since the use of „_” in language code and as a separator for form fields can generated errors.

I have removed „user.ensure_login(self.client)” and leave it only of the first page loaded in this test.
Do we really need to call it after each page load?

I have renamed the variables and remove the comment. Many thanks for this review!

=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-12 00:39:34 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-12 14:49:09 +0000
@@ -374,23 +374,20 @@
                     return;
                 }
                 if (this.get('checked') == true) {
- // When checking the "Need review" checkbox the new
- // translation should be automatically checked.
- var select = Y.one('#' + translation_id + '_select');
- if (select !== null) {
- select.set('checked', true);
+ var new_translation_select = Y.one(
+ '#' + translation_id + '_select');
+ if (new_translation_select !== null) {
+ new_translation_select.set('checked', true);
                     }
                 } else {
- // When unchecking the "Need review" checkbox and the new
- // translation is empty, the current translation should be
- // automatically selected.
- var inputfield = Y.one('#' + translation_id);
- if (inputfield !== null && inputfield.get('value') == '') {
- var current_radio_id = translation_id.replace(
+ var new_translation_field = Y.one('#' + translation_id);
+ if (new_translation_field !== null &&
+ new_translation_field.get('value') == '') {
+ var current_select_id = translation_id.replace(
                            /_new$/, '_radiobutton');
- var current_radio = Y.one('#' + current_radio_id);
- if (current_radio !== null) {
- current_radio.set('checked', true);
+ var current_select = Y.one('#' + current_select_id);
+ if (current_select !== null) {
+ current_select.set('checked', true);
                        }
                     }
                 }

=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
--- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-12 00:39:34 +0000
+++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-12 14:37:32 +0000
@@ -51,7 +51,7 @@
         # Check that the associated radio button is selected.
         client.asserts.assertChecked(id=radiobutton_id)

- def _check_reset_translation_select(
+ def _checkResetTranslationSelect(
         self, client, checkbox, singular_new_select, singular_current_select,
         singular_new_field=None, p...

[Henning Eggers (henninge)]

Am 12.04.2010 16:57, schrieb Adi Roiban:
> I have renamed the method.

Thanks.

>
> I have also added a test for pt_BR, since the use of „_” in language code and as a separator for form fields can generated errors.

Good thinking! Very good, indeed.

>
> I have removed „user.ensure_login(self.client)” and leave it only of the first page loaded in this test.
> Do we really need to call it after each page load?

I don't think so, the browser stays logged in, doesn't it?

>
> I have renamed the variables and remove the comment. Many thanks for this review!

Thanks a lot for your work, it is much appreicated! ;-)

I will land this now.

Henning

[Adi Roiban (adiroiban)]

I forget to update the previous integration test with the new implementation that actually requires selecting the new empty translation.

I have fixed it.

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt 2010-03-18 20:42:37 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt 2010-04-12 19:31:17 +0000
@@ -75,8 +75,9 @@
     ... 'evolution/+pots/man/es/1/+translate')
     >>> inputradio = admin_browser.getControl(
     ... name='msgset_166_es_translation_0_radiobutton')
- >>> inputradio.value = [ 'msgset_166_es_translation_0_new']
- >>> inputfield = admin_browser.getControl(name='msgset_166_es_translation_0_new')
+ >>> inputradio.value = ['msgset_166_es_translation_0_new']
+ >>> inputfield = admin_browser.getControl(
+ ... name='msgset_166_es_translation_0_new')
     >>> inputfield.value = 'New test translation'
     >>> admin_browser.getControl('Save & Continue').click()
     >>> print extract_text(find_tag_by_id(admin_browser.contents,
@@ -88,11 +89,15 @@
     Located in ...

 'Someone should review this translation' can be used to reset the translation,
-and after clicking 'Save & Continue', all translations entered for this
+when a new emtpy translation is added and marked as needing review.
+After clicking 'Save & Continue', all translations entered for this
 message will be listed as suggestions.

     >>> admin_browser.getControl(
     ... 'Someone should review this translation').selected = True
+ >>> inputradio = admin_browser.getControl(
+ ... name='msgset_166_es_translation_0_radiobutton')
+ >>> inputradio.value = ['msgset_166_es_translation_0_new']
     >>> admin_browser.getControl('Save & Continue').click()
     >>> print extract_text(find_tag_by_id(admin_browser.contents,
     ... 'messages_to_translate'))

[Henning Eggers (henninge)]

I tried to land it but it has another merge conflict. Please merge the current
devel again and I will try to land it again.

[Adi Roiban (adiroiban)]

Hi,

I merged and solved the conflict and push the changes.

Cheers,
Adi

=== 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-13 13:02:29 +0000
@@ -13,7 +13,7 @@
  * Function to disable/enable all suggestions as they are marked/unmarked
  * for dismission.
  */
-self.setupSuggestionDismissal = function(e) {
+var setupSuggestionDismissal = function(e) {
     all_dismiss_boxes = Y.all('.dismiss_action');
     if (all_dismiss_boxes !== null) {
         all_dismiss_boxes.each(function(checkbox) {
@@ -57,7 +57,8 @@
   hide_anim.run();
 };
 
-self.updateNotificationBox = function(e) {
+
+var updateNotificationBox = function(e) {
   var notice = Y.one('.important-notice-container');
   if (notice === null) {
     // We have no notice container on this page, this is why there is
@@ -92,7 +93,7 @@
 };
 
 
-self.setFocus = function(field) {
+var setFocus = function(field) {
     // if there is nofield, do nothing
     if (Y.one('#' + field) !== null) {
         Y.one('#' + field).focus();
@@ -101,7 +102,7 @@
 
 
 var setNextFocus = function(e, field) {
-    self.setFocus(field);
+    setFocus(field);
     // stopPropagation() and preventDefault()
     e.halt();
 };
@@ -111,8 +112,8 @@
 
     // Original singular test is focused first to make sure
     // it is visible when scrolling up
-    self.setFocus(original);
-    self.setFocus(field);
+    setFocus(original);
+    setFocus(field);
     // stopPropagation() and preventDefault()
     e.halt();
 };
@@ -180,7 +181,7 @@
 };
 
 
-var selectTranslation = function(e, widget_id) {
+var selectTranslation = function(e, field) {
     // Don't select when tabbing, navigating and simply pressing
     // enter to submit the form.
     // Looks like this is not needed for Epiphany and Chromium
@@ -190,7 +191,8 @@
         (e.shiftKey && e.altKey && e.keyCode == 75)) {
             return;
     }
-    selectWidgetByID(widget_id);
+    var translation_select_id = field + '_select';
+    selectWidgetByID(translation_select_id);
 };
 
 
@@ -204,11 +206,11 @@
         }
         // Shift+Alt+f - Go to search field
         if (e.shiftKey && e.altKey && e.keyCode == 70) {
-            self.setFocus('search_box');
+            setFocus('search_box');
         }
         // Shift+Alt+b - Go to first translation field
         if (e.shiftKey && e.altKey && e.keyCode == 66) {
-            self.setFocus(fields[0]);
+            setFocus(fields[0]);
         }
         // Shift+Alt+n - Go to next page in batch
         if (e.shiftKey && e.altKey && e.keyCode == 78) {
@@ -278,14 +280,13 @@
         var html_parts = fields[key].split('_');
         var msgset_id = html_parts[0] + '_' + html_parts[1];
         var translation_stem = fields[key].replace(/_translation_(\d)+_new/,"");
-        var translation_select_id = fields[key] + '_select';
 
         Y.on(
             'change', selectTranslation,
-            '#' + fields[key], Y, translation_select_id);
+            '#' + fields[key], Y, fields[key]);
         Y.on(
             'keypress', selectTranslation,
-            '#' + fields[key], Y, translation_select_id);
+            '#' + fields[key], Y, fields[key]);
 
         // Set next field and copy text for all but last field
         // (last is Save & Continue button)
@@ -346,11 +347,7 @@
 };
 
 
-/**
- * Initialize event-key bindings such as moving to the next or previous
- * field, or copying original text
- */
-self.initializeKeyBindings = function(e) {
+var initializeKeyBindings = function(e) {
 
     if (translations_order.length < 1) {
         // If no translations fiels are displayed on the page
@@ -366,5 +363,109 @@
     initializeFieldsKeyBindings(fields); 
 };
 
+
+var initializeResetBehavior = function (fields) {
+    for (var key = 0; key < fields.length; key++) {
+        var html_parts = fields[key].split('_');
+        var msgset_id = html_parts[0] + '_' + html_parts[1];
+        var node = Y.one('#' + msgset_id + '_force_suggestion');
+        if (node === null) {
+            // If we don't have a force_suggestion checkbox associated with
+            // this field, just continue to the next field.
+            break;
+        }
+        Y.on(
+            'click',
+            function (e, translation_id) {
+                if (this === null) {
+                    // Don't do nothing if we don't have a context object.
+                    return;
+                }
+                if (this.get('checked') == true) {
+                    var new_translation_select = Y.one(
+                        '#' + translation_id + '_select');
+                    if (new_translation_select !== null) {
+                        new_translation_select.set('checked', true);
+                    }
+                } else {
+                    var new_translation_field = Y.one('#' + translation_id);
+                    if (new_translation_field !== null &&
+                        new_translation_field.get('value') == '') {
+                       var current_select_id = translation_id.replace(
+                           /_new$/, '_radiobutton');
+                       var current_select = Y.one('#' + current_select_id);
+                       if (current_select !== null) {
+                           current_select.set('checked', true);
+                       }
+                    }
+                }
+            },
+            node , node, fields[key]
+        );
+    }
+};
+
+/**
+ * Initialize common Javascript code for POFile and TranslationMessage
+ * +translate pages.
+ *
+ * This will add event-key bindings such as moving to the next or previous
+ * field, or copying original text.
+ * It will also initializing the reset checkbox behavior and will show the
+ * error notifications.
+ */
+ 
+var initializeBaseTranslate = function () {
+    try {
+      setupSuggestionDismissal();
+    } catch (e) {
+      Y.log(e, "error");
+    }
+
+    try {
+      initializeKeyBindings();
+    } catch (e) {
+      Y.log(e, "error");
+    }
+
+    try {
+      var fields = translations_order.split(' ');
+      initializeResetBehavior(fields);
+    } catch (e) {
+      Y.log(e, "error");
+    }
+
+    try {
+      setFocus(autofocus_field);
+    } catch (e) {
+      Y.log(e, "error");
+    }
+}
+
+/**
+ * Initialize Javascript code for a POFile +translate page.
+ *
+ * This will initialize the base code and will also show the guidelines
+ * if needeed.
+ */
+self.initializePOFile = function(e) {
+    try {
+      updateNotificationBox();
+    } catch (e) {
+      Y.log(e, "error");
+    }
+    initializeBaseTranslate();
+};
+
+/**
+ * Initialize Javascript code for a TranslationMessage +translate page.
+ *
+ * This will initialize the base code.
+ */
+self.initializeTranslationMessage = function(e) {
+    initializeBaseTranslate();
+};
+
+
 }, "0.1", {"requires": ["event", "event-key", "node", "cookie", "anim"]});
 

=== modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
--- lib/lp/translations/browser/tests/translationmessage-views.txt	2009-09-11 22:24:51 +0000
+++ lib/lp/translations/browser/tests/translationmessage-views.txt	2010-04-13 13:02:29 +0000
@@ -689,3 +689,101 @@
     False
     >>> subview.shared_translationmessage == translationmessage
     True
+
+
+Resetting current translation
+-----------------------------
+
+The current translation can be reset by submitting an empty translation
+or a translation similar to the current one while forcing suggestions.
+
+We need to force timestamp update, since suggestions are submitted too fast.
+
+    >>> from lp.translations.interfaces.translationgroup import (
+    ...     TranslationPermission)
+    >>> pofile = factory.makePOFile('es')
+    >>> potemplate = pofile.potemplate
+    >>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
+    >>> translationmessage = factory.makeTranslationMessage(
+    ...     potmsgset=potmsgset, pofile=pofile,
+    ...     translations = [u"A shared translation"])
+    >>> translationmessage.setPOFile(pofile)
+    >>> product = potemplate.productseries.product
+    >>> product.translationpermission = TranslationPermission.CLOSED
+    >>> year_tick = 2100
+    >>> def submit_translation(
+    ...     translationmessage, translation, force_suggestion=False):
+    ...     global year_tick
+    ...     pofile = translationmessage.pofile
+    ...     potmsgset = translationmessage.potmsgset
+    ...     server_url = '/'.join(
+    ...         [canonical_url(translationmessage), '+translate'])
+    ...     now = (str(year_tick) +
+    ...             datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
+    ...     year_tick += 1
+    ...     msgset_id = 'msgset_' + str(potmsgset.id)
+    ...     msgset_id_lang = msgset_id + '_' + pofile.language.code
+    ...     form = {
+    ...         'lock_timestamp': now,
+    ...         'alt': None,
+    ...         msgset_id : None,
+    ...         msgset_id_lang + '_translation_0_radiobutton':
+    ...         msgset_id_lang + '_translation_0_new',
+    ...         msgset_id_lang + '_translation_0_new': translation,
+    ...         'submit_translations': 'Save &amp; Continue'}
+    ...     if force_suggestion:
+    ...         form[msgset_id_lang + '_needsreview'] = 'force_suggestion'
+    ...     tm_view = create_view(
+    ...         translationmessage, "+translate", form=form,
+    ...         layer=TranslationsLayer, server_url=server_url)
+    ...     tm_view.request.method = 'POST'
+    ...     tm_view.initialize()
+
+    >>> def print_current_translation(potmsgset, pofile):
+    ...     current_translation = potmsgset.getCurrentTranslationMessage(
+    ...         pofile.potemplate, pofile.language)
+    ...     if current_translation is not None:
+    ...         print current_translation.translations
+
+    >>> def print_local_suggestions(potmsgset, pofile):
+    ...     import operator
+    ...     local = sorted(
+    ...         potmsgset.getLocalTranslationMessages(
+    ...             pofile.potemplate, pofile.language),
+    ...         key=operator.attrgetter("date_created"),
+    ...         reverse=True)
+    ...     for suggestion in local:
+    ...         print suggestion.translations
+
+We will make an initial translation.
+
+    >>> submit_translation(
+    ...     translationmessage, u'Foo')
+    >>> print_local_suggestions(potmsgset, pofile)
+    >>> print_current_translation(potmsgset, pofile)
+    [u'Foo']
+
+Forcing suggestions while submitting an empty translation will reset the
+translation for this message and all previously entered translations will be
+listed as suggestions.
+
+    >>> submit_translation(translationmessage, u'', force_suggestion=True)
+    >>> print_local_suggestions(potmsgset, pofile)
+    [u'Foo']
+    [u'A shared translation']
+    >>> print_current_translation(potmsgset, pofile)
+
+Only users with edit rights are allowed to reset a translation.
+
+    >>> submit_translation(translationmessage, u'New current translation')
+    >>> print_current_translation(potmsgset, pofile)
+    [u'New current translation']
+    >>> print_local_suggestions(potmsgset, pofile)
+    >>> login('[e-mail]')
+    >>> submit_translation(
+    ...     translationmessage, u'New current translation',
+    ...     force_suggestion=True)
+    >>> login('[e-mail]')
+    >>> print_local_suggestions(potmsgset, pofile)
+    >>> print_current_translation(potmsgset, pofile)
+    [u'New current translation']

=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py	2010-04-07 06:23:30 +0000
+++ lib/lp/translations/browser/translationmessage.py	2010-04-13 13:02:29 +0000
@@ -425,6 +425,16 @@
                 is_imported=False, lock_timestamp=self.lock_timestamp,
                 force_suggestion=force_suggestion,
                 force_diverged=force_diverge)
+
+            # If suggestions were forced and user has the rights to do it,
+            # reset the current translation.
+            empty_suggestions = self._areSuggestionsEmpty(translations)
+            if (force_suggestion and
+                self.user_is_official_translator and
+                empty_suggestions):
+                potmsgset.resetCurrentTranslation(
+                    self.pofile, self.lock_timestamp)
+
         except TranslationConflict:
             return (
                 u'Somebody else changed this translation since you started.'
@@ -439,6 +449,13 @@
             self._observeTranslationUpdate(potmsgset)
             return None
 
+    def _areSuggestionsEmpty(self, suggestions):
+        """Return true if all suggestions are empty strings or None."""
+        for index in suggestions:
+            if (suggestions[index] is not None and suggestions[index] != ""):
+                return False
+        return True
+
     def _prepareView(self, view_class, current_translation_message, error):
         """Collect data and build a TranslationMessageView for display."""
         # XXX: kiko 2006-09-27:

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2010-03-05 15:31:22 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2010-04-13 13:02:29 +0000
@@ -250,6 +250,18 @@
         If a translation conflict is detected, TranslationConflict is raised.
         """
 
+    def resetCurrentTranslation(pofile, lock_timestamp):
+        """Reset the currently used translation.
+
+        This will set the "is_current" attribute to False and if the message
+        is diverge, will try to converge it.
+        :param pofile: a `POFile` to dismiss suggestions from.
+        :param lock_timestamp: the timestamp when we checked the values we
+            want to update.
+
+        If a translation conflict is detected, TranslationConflict is raised.
+        """
+
     def applySanityFixes(unicode_text):
         """Return 'unicode_text' or None after doing some sanitization.
 

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-03-05 15:31:22 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-04-13 13:02:29 +0000
@@ -883,6 +883,24 @@
         matching_message.sync()
         return matching_message
 
+    def _maybeRaiseTranslationConflict(self, message, lock_timestamp):
+        """Checks if there is a translation conflict for the message.
+
+        If a translation conflict is detected, TranslationConflict is raised.
+        """
+        if message.date_reviewed is not None:
+            use_date = message.date_reviewed
+        else:
+            use_date = message.date_created
+        if use_date >= lock_timestamp:
+            raise TranslationConflict(
+                'While you were reviewing these suggestions, somebody '
+                'else changed the actual translation. This is not an '
+                'error but you might want to re-review the strings '
+                'concerned.')
+        else:
+            return
+
     def dismissAllSuggestions(self, pofile, reviewer, lock_timestamp):
         """See `IPOTMsgSet`."""
         assert(lock_timestamp is not None)
@@ -893,19 +911,29 @@
             current = self.updateTranslation(
                 pofile, reviewer, [], False, lock_timestamp)
         else:
-            if current.date_reviewed is not None:
-                use_date = current.date_reviewed
-            else:
-                use_date = current.date_created
-            if use_date >= lock_timestamp:
-                raise TranslationConflict(
-                    'While you were reviewing these suggestions, somebody '
-                    'else changed the actual translation. This is not an '
-                    'error but you might want to re-review the strings '
-                    'concerned.')
-            else:
-                current.reviewer = reviewer
-                current.date_reviewed = lock_timestamp
+            # Check for translation conflicts and update review fields.
+            self._maybeRaiseTranslationConflict(current, lock_timestamp)
+            current.reviewer = reviewer
+            current.date_reviewed = lock_timestamp
+
+    def resetCurrentTranslation(self, pofile, lock_timestamp):
+        """See `IPOTMsgSet`."""
+
+        assert(lock_timestamp is not None)
+
+        current = self.getCurrentTranslationMessage(
+            pofile.potemplate, pofile.language)
+
+        if (current is not None):
+            # Check for transltion conflicts and update the required
+            # attributes.
+            self._maybeRaiseTranslationConflict(current, lock_timestamp)
+            current.is_current = False
+            # Converge the current translation only if it is diverged and not
+            # imported.
+            if current.potemplate is not None and not current.is_imported:
+                current.potemplate = None
+            pofile.date_changed = UTC_NOW
 
     def applySanityFixes(self, text):
         """See `IPOTMsgSet`."""

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2009-10-28 13:08:05 +0000
+++ lib/lp/translations/model/translationmessage.py	2010-04-13 13:02:29 +0000
@@ -478,12 +478,12 @@
     implements(ITranslationMessageSet)
 
     def getByID(self, ID):
-        """See `ILanguageSet`."""
+        """See `ITranslationMessageSet`."""
         try:
             return TranslationMessage.get(ID)
         except SQLObjectNotFound:
             return None
 
     def selectDirect(self, where=None, order_by=None):
-        """See `ILanguageSet`."""
+        """See `ITranslationMessageSet`."""
         return TranslationMessage.select(where, orderBy=order_by)

=== modified file 'lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt'
--- lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt	2009-07-01 20:45:39 +0000
+++ lib/lp/translations/stories/standalone/xx-pofile-translate-needs-review-flags-preserved.txt	2010-04-13 13:02:29 +0000
@@ -1,6 +1,10 @@
-= Forcing suggestions =
-
-When translation is restricted to a particular set of users
+Someone should review this translation usage
+============================================
+
+Forcing suggestions
+-------------------
+
+When translating is restricted to a particular set of users
 (like Evolution product is restricted for Spanish), other users
 don't see a checkbox named 'Someone should review this translation'.
 
@@ -53,3 +57,58 @@
     >>> print extract_text(find_tag_by_id(user_browser.contents,
     ...                                   'msgset_130_es_suggestion_701_0'))
     New suggestion
+
+
+Reseting translations
+---------------------
+
+If the 'Someone should review this translation' checkbox is used without
+adding any new suggestions or adding an empty string, the current translation
+will be reset causing all suggestions entered for this message to be listed
+again.
+
+A new translation is entered and checked that it was saved as the current
+translation, while no suggestions are displayed.
+
+    >>> admin_browser.open(
+    ...     'http://translations.launchpad.dev/ubuntu/hoary/+source/'
+    ...     'evolution/+pots/man/es/1/+translate')
+    >>> inputradio = admin_browser.getControl(
+    ...    name='msgset_166_es_translation_0_radiobutton')
+    >>> inputradio.value = ['msgset_166_es_translation_0_new']
+    >>> inputfield = admin_browser.getControl(
+    ...     name='msgset_166_es_translation_0_new')
+    >>> inputfield.value = 'New test translation'
+    >>> admin_browser.getControl('Save & Continue').click()
+    >>> print extract_text(find_tag_by_id(admin_browser.contents,
+    ...                             'messages_to_translate'))
+    1. English: test man page
+    Current Spanish: New test translation Translated and reviewed ...
+    New translation:
+    Someone should review this translation
+    Located in ...
+
+'Someone should review this translation' can be used to reset the translation,
+when a new emtpy translation is added and marked as needing review.
+After clicking 'Save & Continue', all translations entered for this
+message will be listed as suggestions.
+
+    >>> admin_browser.getControl(
+    ...     'Someone should review this translation').selected = True
+    >>> inputradio = admin_browser.getControl(
+    ...    name='msgset_166_es_translation_0_radiobutton')
+    >>> inputradio.value = ['msgset_166_es_translation_0_new']
+    >>> admin_browser.getControl('Save & Continue').click()
+    >>> print extract_text(find_tag_by_id(admin_browser.contents,
+    ...                             'messages_to_translate'))
+    1. English: test man page
+    Current Spanish: (no translation yet)
+    Suggestions:
+    New test translation Suggested by Foo Bar ...
+    blah, blah, blah Suggested by Carlos ...
+    lalalala Suggested by Carlos ...
+    just a translation Suggested by Sample Person ...
+    Dismiss all suggestions above.
+    New translation:
+    Someone should review this translation
+    Located in ...

=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
--- lib/lp/translations/templates/pofile-translate.pt	2010-03-22 17:44:53 +0000
+++ lib/lp/translations/templates/pofile-translate.pt	2010-04-13 13:02:29 +0000
@@ -18,33 +18,7 @@
       registerLaunchpadFunction(insertAllExpansionButtons);
 
       LPS.use('lp.pofile', function(Y) {
-
-          Y.on('domready', function(e) {
-            try {
-              Y.lp.pofile.updateNotificationBox();
-            } catch (e) {
-              Y.log(e, "error");
-            }
-
-            try {
-              Y.lp.pofile.setupSuggestionDismissal();
-            } catch (e) {
-              Y.log(e, "error");
-            }
-
-            try {
-              Y.lp.pofile.initializeKeyBindings();
-            } catch (e) {
-              Y.log(e, "error");
-            }
-
-            try {
-              Y.lp.pofile.setFocus(autofocus_field);
-            } catch (e) {
-              Y.log(e, "error");
-            }
-          });
-
+        Y.on('domready', Y.lp.pofile.initializePOFile);
       });
 
     </script>

=== modified file 'lib/lp/translations/templates/translationmessage-translate.pt'
--- lib/lp/translations/templates/translationmessage-translate.pt	2010-03-22 17:44:53 +0000
+++ lib/lp/translations/templates/translationmessage-translate.pt	2010-04-13 13:02:29 +0000
@@ -15,25 +15,7 @@
     </style>
     <script type="text/javascript">
       LPS.use('node', 'lp.pofile', function(Y) {
-          Y.on('domready', function(e) {
-            try {
-              Y.lp.pofile.setupSuggestionDismissal();
-            } catch (e) {
-              Y.log(e, "error");
-            }
-
-            try {
-              Y.lp.pofile.initializeKeyBindings();
-            } catch (e) {
-              Y.log(e, "error");
-            }
-
-            try {
-              Y.lp.pofile.setFocus(autofocus_field);
-            } catch (e) {
-              Y.log(e, "error");
-            }
-          });
+        Y.on('domready', Y.lp.pofile.initializeTranslationMessage);
       });
     </script>
     </div>

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-04-12 16:13:38 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-04-13 13:02:29 +0000
@@ -908,6 +908,89 @@
                 include_dismissed=False, include_unreviewed=False))
 
 
+class TestPOTMsgSetResetTranslation(TestCaseWithFactory):
+    """Test resetting the current translation."""
+
+    layer = ZopelessDatabaseLayer
+
+    def gen_now(self):
+        now = datetime.now(pytz.UTC)
+        while True:
+            yield now
+            now += timedelta(milliseconds=1)
+
+    def setUp(self):
+        # Create a product with all the boilerplate objects to be able to
+        # create TranslationMessage objects.
+        super(TestPOTMsgSetResetTranslation, self).setUp()
+        self.now = self.gen_now().next
+        self.foo = self.factory.makeProduct()
+        self.foo_main = self.factory.makeProductSeries(
+            name='main', product=self.foo)
+        self.foo.official_rosetta = True
+
+        self.potemplate = self.factory.makePOTemplate(
+            productseries=self.foo_main, name="messages")
+        self.potmsgset = self.factory.makePOTMsgSet(self.potemplate,
+                                                    sequence=1)
+        self.pofile = self.factory.makePOFile('eo', self.potemplate)
+
+    def test_resetCurrentTranslation_shared(self):
+        # Resetting a shared current translation will change iscurrent=False
+        # and there will be no other current translations for this POTMsgSet.
+
+        translation = self.factory.makeTranslationMessage(
+            self.pofile, self.potmsgset, translations=[u'Shared translation'],
+            reviewer=self.factory.makePerson(),
+            is_imported=False, force_diverged=False,
+            date_updated=self.now())
+
+        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
+        current = self.potmsgset.getCurrentTranslationMessage(
+            self.potemplate, self.pofile.language)
+        self.assertTrue(current is None)
+        self.assertFalse(translation.is_current)
+        self.assertFalse(translation.is_imported)
+        self.assertTrue(translation.potemplate is None)
+
+    def test_resetCurrentTranslation_diverged_not_imported(self):
+        # Resettting a diverged current translation that was not imported, will
+        # change is_current to False and will make it shared.
+
+        translation = self.factory.makeTranslationMessage(
+            self.pofile, self.potmsgset, translations=[u'Diverged text'],
+            reviewer=self.factory.makePerson(),
+            is_imported=False, force_diverged=True,
+            date_updated=self.now())
+
+        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
+        current = self.potmsgset.getCurrentTranslationMessage(
+            self.potemplate, self.pofile.language)
+        self.assertTrue(current is None)
+        self.assertFalse(translation.is_current)
+        self.assertFalse(translation.is_imported)
+        self.assertTrue(translation.potemplate is None)
+
+    def test_resetCurrentTranslation_diverged_imported(self):
+        # Resetting a diverged current translation that was imported in
+        # Launchpad will change iscurrent to False but the translation
+        # message will be still diverged.
+
+        translation = self.factory.makeTranslationMessage(
+            self.pofile, self.potmsgset, translations=[u'Imported diverged'],
+            reviewer=self.factory.makePerson(),
+            is_imported=True, force_diverged=True,
+            date_updated=self.now())
+
+        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
+        current = self.potmsgset.getCurrentTranslationMessage(
+            self.potemplate, self.pofile.language)
+        self.assertTrue(current is None)
+        self.assertFalse(translation.is_current)
+        self.assertTrue(translation.is_imported)
+        self.assertFalse(translation.potemplate is None)
+
+
 class TestPOTMsgSetCornerCases(TestCaseWithFactory):
     """Test corner cases and constraints."""
 

=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
--- lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-04-11 20:07:25 +0000
+++ lib/lp/translations/windmill/tests/test_pofile_translate.py	2010-04-13 13:02:29 +0000
@@ -21,7 +21,7 @@
     layer = TranslationsWindmillLayer
     suite_name = 'POFile Translate'
 
-    def _check_translation_autoselect(
+    def _checkTranslationAutoselect(
         self, url, new_translation_id, new_translation_select_id):
         """Checks that the select radio button is checked when typing a new
         translation.
@@ -60,7 +60,7 @@
                         'evolution/trunk/+pots/evolution-2.2/es/+translate')
         new_translation_id = u'msgset_1_es_translation_0_new'
         new_translation_select_id = u'msgset_1_es_translation_0_new_select'
-        self._check_translation_autoselect(
+        self._checkTranslationAutoselect(
             start_url, new_translation_id, new_translation_select_id)
 
         # Test the zoom in view for Evolution trunk Brazilian (pt_BR).
@@ -69,7 +69,7 @@
                         'pt_BR/1/+translate')
         new_translation_id = u'msgset_1_pt_BR_translation_0_new'
         new_translation_select_id = u'msgset_1_pt_BR_translation_0_new_select'
-        self._check_translation_autoselect(
+        self._checkTranslationAutoselect(
             start_url, new_translation_id, new_translation_select_id)
 
         # Test the zoom out view for Ubuntu Hoary Brazilian (pt_BR).
@@ -79,5 +79,173 @@
         new_translation_id = u'msgset_152_pt_BR_translation_0_new'
         new_translation_select_id = (u'msgset_152_pt_BR'
                                        '_translation_0_new_select')
-        self._check_translation_autoselect(
+        self._checkTranslationAutoselect(
             start_url, new_translation_id, new_translation_select_id)
+
+    def _checkResetTranslationSelect(
+        self, client, checkbox, singular_new_select, singular_current_select,
+        singular_new_field=None, plural_new_select=None):
+        """Checks that the new translation select radio buttons are checked
+        when ticking 'Someone should review this translation' checkbox.
+        """
+
+        client.waits.forElement(
+            id=checkbox, timeout=constants.FOR_ELEMENT)
+        client.waits.forElement(
+            id=singular_new_select, timeout=constants.FOR_ELEMENT)
+        client.waits.forElement(
+            id=singular_current_select, timeout=constants.FOR_ELEMENT)
+        if plural_new_select is not None:
+            client.waits.forElement(
+                id=plural_new_select, timeout=constants.FOR_ELEMENT)
+        if singular_new_field is not None:
+            client.waits.forElement(
+                id=singular_new_field, timeout=constants.FOR_ELEMENT)
+
+        # Check that initialy the checkbox is not checked and
+        # that the radio buttons are not selected.
+        client.asserts.assertNotChecked(id=checkbox)
+        client.asserts.assertNotChecked(id=singular_new_select)
+        client.asserts.assertChecked(id=singular_current_select)
+        if plural_new_select is not None:
+            client.asserts.assertNotChecked(id=plural_new_select)
+
+        # Check the checkbox
+        client.click(id=checkbox)
+
+        # Check that the checkbox and the new translation radio buttons are
+        # selected.
+        client.asserts.assertChecked(id=checkbox)
+        client.asserts.assertChecked(id=singular_new_select)
+        client.asserts.assertNotChecked(id=singular_current_select)
+        if plural_new_select is not None:
+            client.asserts.assertChecked(id=plural_new_select)
+
+        # Then then we uncheck the 'Someone needs to review this translation'
+        # checkbox.
+        client.click(id=checkbox)
+
+        # Unchecking the 'Someone needs to review this translation' checkbox
+        # when the 'New translation' field is empty, will select the current
+        # translation.
+        client.asserts.assertNotChecked(id=checkbox)
+        client.asserts.assertNotChecked(id=singular_new_select)
+        client.asserts.assertChecked(id=singular_current_select)
+        if plural_new_select is not None:
+            client.asserts.assertNotChecked(id=plural_new_select)
+
+        if singular_new_field is not None:
+            # Checking again the 'Someone need to review this translation'
+            # checkbox, type some text and unchecking it should keep the new
+            # translation fields selected
+            client.click(id=checkbox)
+            client.type(text=u'some test', id=singular_new_field)
+            client.click(id=checkbox)
+
+            client.asserts.assertNotChecked(id=checkbox)
+            client.asserts.assertChecked(id=singular_new_select)
+            client.asserts.assertNotChecked(id=singular_current_select)
+            if plural_new_select is not None:
+                client.asserts.assertNotChecked(id=plural_new_select)
+
+    def test_pofile_reset_translation_select(self):
+        """Test for automatically selecting new translation when
+        'Someone needs to review this translations' is checked.
+        """
+        client = self.client
+        user = lpuser.TRANSLATIONS_ADMIN
+
+        # Go to the zoom in page for a translation with plural forms.
+        self.client.open(
+            url='http://translations.launchpad.dev:8085/'
+                'ubuntu/hoary/+source/evolution/+pots/'
+                'evolution-2.2/es/15/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        user.ensure_login(self.client)
+
+        checkbox = u'msgset_144_force_suggestion'
+        singular_new_select = u'msgset_144_es_translation_0_new_select'
+        singular_new_field = u'msgset_144_es_translation_0_new'
+        singular_current_select = u'msgset_144_es_translation_0_radiobutton'
+        plural_new_select = u'msgset_144_es_translation_1_new_select'
+        self._checkResetTranslationSelect(
+            client,
+            checkbox=checkbox,
+            singular_new_select=singular_new_select,
+            singular_new_field=singular_new_field,
+            singular_current_select=singular_current_select,
+            plural_new_select=plural_new_select)
+
+        # Go to the zoom in page for a pt_BR translation with plural forms.
+        # pt_BR is a language code using the same delimiter as HTTP form
+        # fields and are prone to errors.
+        self.client.open(
+            url='http://translations.launchpad.dev:8085/'
+                'ubuntu/hoary/+source/evolution/+pots/'
+                'evolution-2.2/pt_BR/15/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+
+        checkbox = u'msgset_144_force_suggestion'
+        singular_new_select = u'msgset_144_pt_BR_translation_0_new_select'
+        singular_new_field = u'msgset_144_pt_BR_translation_0_new'
+        singular_current_select = u'msgset_144_pt_BR_translation_0_radiobutton'
+        plural_new_select = u'msgset_144_pt_BR_translation_1_new_select'
+        self._checkResetTranslationSelect(
+            client,
+            checkbox=checkbox,
+            singular_new_select=singular_new_select,
+            singular_new_field=singular_new_field,
+            singular_current_select=singular_current_select,
+            plural_new_select=plural_new_select)
+
+        # Go to the zoom in page for a translation without plural forms.
+        self.client.open(
+            url='http://translations.launchpad.dev:8085/'
+                'ubuntu/hoary/+source/evolution/+pots/'
+                'evolution-2.2/es/19/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+
+        checkbox = u'msgset_148_force_suggestion'
+        singular_new_select = u'msgset_148_es_translation_0_new_select'
+        singular_current_select = u'msgset_148_es_translation_0_radiobutton'
+        self._checkResetTranslationSelect(
+            client,
+            checkbox=checkbox,
+            singular_new_select=singular_new_select,
+            singular_current_select=singular_current_select)
+
+        # Go to the zoom out page for some translations.
+        self.client.open(
+            url='http://translations.launchpad.dev:8085/'
+                'ubuntu/hoary/+source/evolution/+pots/'
+                'evolution-2.2/es/+translate')
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+
+        checkbox = u'msgset_130_force_suggestion'
+        singular_new_select = u'msgset_130_es_translation_0_new_select'
+        singular_current_select = u'msgset_130_es_translation_0_radiobutton'
+        self._checkResetTranslationSelect(
+            client,
+            checkbox=checkbox,
+            singular_new_select=singular_new_select,
+            singular_current_select=singular_current_select)
+
+        # Ensure that the other radio buttons are not changed
+        client.asserts.assertNotChecked(
+            id=u'msgset_131_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_132_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_133_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_134_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_135_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_136_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_137_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_138_es_translation_0_new_select')
+        client.asserts.assertNotChecked(
+            id=u'msgset_139_es_translation_0_new_select')