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

> 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 = 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);
+ }
+ }
+ },
+ node , node, fields[key]
+ );
+ }
+};

 /**
  * Initialize event-key bindings such as moving to the next or previous
- * field, or copying original text
+ * field, or copying original text.
+ * This function is also initializing the reset checkbox behavior.
  */
 self.initializeKeyBindings = function(e) {

@@ -342,6 +371,7 @@

     initializeGlobalKeyBindings(fields);
     initializeFieldsKeyBindings(fields);
+ initializeResetBehavior(fields);
 };

 }, "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 2010-03-22 15:33:15 +0000
+++ lib/lp/translations/browser/tests/translationmessage-views.txt 2010-03-22 16:34:02 +0000
@@ -721,18 +721,18 @@
     ... now = (str(year_tick) +
     ... datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
     ... year_tick += 1
- ... msgset_stem = 'msgset_' + str(potmsgset.id)
- ... msgset_stem_lang = msgset_stem + '_' + pofile.language.code
+ ... msgset_id = 'msgset_' + str(potmsgset.id)
+ ... msgset_id_lang = msgset_id + '_' + pofile.language.code
     ... form = {
     ... 'lock_timestamp': now,
     ... 'alt': None,
- ... msgset_stem : None,
- ... msgset_stem_lang + '_translation_0_radiobutton':
- ... msgset_stem_lang + '_translation_0_new',
- ... msgset_stem_lang + '_translation_0_new': translation,
+ ... 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_stem_lang + '_needsreview'] = 'force_suggestion'
+ ... form[msgset_id_lang + '_needsreview'] = 'force_suggestion'
     ... tm_view = create_view(
     ... translationmessage, "+translate", form=form,
     ... layer=TranslationsLayer, server_url=server_url)

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py 2010-03-18 20:42:37 +0000
+++ lib/lp/translations/model/potmsgset.py 2010-03-22 16:44:42 +0000
@@ -883,7 +883,7 @@
         matching_message.sync()
         return matching_message

- def _isNoTranslationConflict(self, message, lock_timestamp):
+ 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.
@@ -899,7 +899,7 @@
                 'error but you might want to re-review the strings '
                 'concerned.')
         else:
- return True
+ return

     def dismissAllSuggestions(self, pofile, reviewer, lock_timestamp):
         """See `IPOTMsgSet`."""
@@ -912,7 +912,7 @@
                 pofile, reviewer, [], False, lock_timestamp)
         else:
             # Check for translation conflicts and update review fields.
- self._isNoTranslationConflict(current, lock_timestamp)
+ self._maybeRaiseTranslationConflict(current, lock_timestamp)
             current.reviewer = reviewer
             current.date_reviewed = lock_timestamp

@@ -924,11 +924,10 @@
         current = self.getCurrentTranslationMessage(
             pofile.potemplate, pofile.language)

- # Reset only if we don't have a conflict and
- # if no new suggestions were provided or
- # the new suggestions are empty.
- if (current is not None and
- self._isNoTranslationConflict(current, lock_timestamp)):
+ 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.