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

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 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]
> + );
> + }
> +};
[...]

So, with that addition and an updated Windmill test in place, you can land this as soon as PQM opens up. Don't forget to merge the current devel first, though, as it has been merged with db-stable and your branch has gotten fairly old, too ... ;-)

Thanks for your work!

Henning