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