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

-----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...

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.

> -----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...

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

> 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 ...

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...

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...

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...

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.

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...

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

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'))

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.

Hi,

I merged and solved the conflict and push the changes.

Cheers,
Adi