Impossible to mark as needing review strings already translated or with suggestions

Can you please elaborate. At the moment, we have quite a confusing situation with need-review, new-suggestions and fuzzy terminology. I want to fix that, but I just need to know exactly where do you see it as not being marked as need-review when you mark it as such (so I can tell if this is the same problem, or some new problem).

Thanks

Hi Danilo,

Il giorno gio, 20/03/2008 alle 16.59 +0000, Данило Шеган ha scritto:
> Can you please elaborate. At the moment, we have quite a confusing
> situation with need-review, new-suggestions and fuzzy terminology. I
> want to fix that, but I just need to know exactly where do you see it as
> not being marked as need-review when you mark it as such (so I can tell
> if this is the same problem, or some new problem).

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.

Hope this can help...

Bye!

--
Milo Casagrande <email address hidden>

I can confirm this behaviour. However, I plan to change the behaviour of needs-review flag completely, so I'll only link this to the relevant blueprint.

Just to know (I don't want to "pressure" you guys), is it planned for a milestone?

I just encountered the same behavior today and it came to my mind this bug...

Bye! :)

If for so many day the radio box " Someone should review this translation" is not working it would be better to remove it from the UI.

Otherwise it only creates confusion and people thing they have mark a translation. Usualy few people check to see if every action is actualy performed.

Thanks!

This is critical. See the bug demonstration in attachment. The "should review" button is not working at all (!) I have spent a lot of time marking bad translations as "need review" and only after that I found out that nothing is changed (kill kill kill!).

Kamil: it is working, but is for a different purpose. It makes it possible for reviewers (like yourself) to *submit* suggestions (i.e. to have someone else review them).

For reference, here is the chat from #launchpad-reviews

18:34 danilos : adiroiban, first off, all comments should be full sentences that end with punctuation, so don't remove a
                   period in one of those: and doing that when discussing things makes the diff larger and harder to read :)
18:34 gmb : rockstar: https://code.edge.launchpad.net/~rockstar/launchpad/bug-520446/+merge/21025 ?
18:34 rockstar : gmb, aye. Thanks a lot.
18:35 danilos : adiroiban, ok, so what you are trying to do will cause a lot of trouble
18:35 danilos : adiroiban, it won't necessarily have the effect you want it to have
18:36 danilos : adiroiban, for instance, if current translation is diverged, and there is a shared one, this will make the
                   shared one current, and the diverged one might show as a suggestion only accidentally (i.e. if it has a
                   date_created newer than the shared one)
18:36 adiroiban : when I tick the „needs review” box, I do it because I think that the current translation is wrong
18:36 gmb : rockstar: Looks good. r=me.
18:37 rockstar : gmb, ta
18:37 danilos : adiroiban, right, but there is also that 'needs review' side of things that might not really work as desired
18:38 kfogel > [~Karl@173-142-134-98.pools.spcsdns.net] joins #launchpad-reviews
18:38 danilos : adiroiban, I agree unsetting it as current is an improvement over existing behaviour, but you can't do that
                   as simply as unsetting a flag either; if it's diverged and there's an existing identical suggestion, one of
                   those needs to be removed
18:39 danilos : adiroiban, finding all that out is pretty expensive, so I think a proper solution would be to introduce a
                   new column on translationmessage table called something like is_reviewed
18:39 adiroiban : danilos: OK. Right now, I have no idea how diverged translations should be handled
18:40 danilos : adiroiban, it's all very, very complex, and there are some intricate bugs as well; doing it this way would
                   just introduce more intricate bugs
18:41 danilos : adiroiban, in short, there's no simple solution to this
18:41 rockstar : gmb, do you think you could do another quick one? Mostly just mechanical changes for javascript adhere to
                  the style guide.
18:42 gmb : rockstar: Sure.
18:42 danilos : adiroiban, the solution would probably be to create a 'unsetTranslationMessage' on POTMsgSet that takes a
                   template, language, variant as parameters
18:42 adiroiban : danilos: I'm trying to construct a test scenario for the use case described by you above
18:42 danilos : adiroiban, and then we can call that when we figure out that's what a user wants, and it'd be isolated from
                   the rest of the updateTranslation (which is a mess, and should not be extended in any way)
18:44 danilos : adiroiban, look at lib/lp/translations/tests/test_potmsgset.py test_updateTranslation tests and how complex
                   they are
18:45 danilos : adiroiban, making them even more complex is out of the question :) separate method which ...

New chat for reference:

20:03 danilos : adiroiban, I am about to call it a day, is there anything I can do to help out while I am still here? :)
20:03 adiroiban : yes
20:03 adiroiban : can you please explain
20:04 adiroiban : in simple words
20:04 adiroiban : what should „needs review” do
20:04 adiroiban : from the point of view of users
20:04 adiroiban : it should just set the translation to empty
20:04 adiroiban : and mark the current translation as needs reviewn ?
20:04 adiroiban : review?
20:05 adiroiban : danilos: ^ :)
20:05 adiroiban : or it should dismiss the current translation?
20:06 adiroiban : and just leave the message untranslated
20:06 danilos : adiroiban, well
20:06 danilos : adiroiban, I think it should do that and leave all the other unreviewed suggestions as unreviewed
                   suggestions
20:07 danilos : adiroiban, the message should end up untranslated, but with the previous active translation as another
                   suggestion added to the existing set of suggestions
20:07 adiroiban : ok
20:08 adiroiban : and then, listing ALL suggestions should be another bug and should be displayed only in the zoom in view
20:08 danilos : adiroiban, btw, look at dismissAllSuggestions for a few ideas
20:08 danilos : adiroiban, that's right, and there's already that "another" bug filed :)
20:09 adiroiban : danilos: yes. I think I am assigned to that bug
20:09 danilos : adiroiban, bug 339507
20:09 mup : Bug #339507: translation page should show old translations <ui> <Launchpad Translations:Triaged by
                   adiroiban> <https://launchpad.net/bugs/339507>
20:09 danilos : adiroiban, right
20:10 adiroiban : but I was thinking we can force showing all suggestions
20:10 danilos : adiroiban, and do take care of the TranslationConflict in a similar way (when someone changes translations
                   after you have loaded the page)
20:10 adiroiban : by ticking the needs review
20:10 danilos : adiroiban, we shouldn't
20:10 adiroiban : danilos: ok :)
20:10 danilos : adiroiban, we'll get a very confusing behaviour of 'needs review'
20:11 adiroiban : danilos: ok. Many thanks!
20:11 danilos : adiroiban, now, you can perhaps say "it would make the implementation much easier, so I'd do that as a
                   first step/branch", and then I'd agree :)

I have committed a first naive fix.

It sets the latest empty translations iscurrent to False.

In the zoomed out view, only the last 3 translations are displayed... so the UI is not cluttered by this behaviour.

I we would like to see the current translation as a suggestion we would need to fake it's creation time ... and this can also confuse translators as the translation will appear as being created just now.

The code that fakes the creating date is commented in this branch.

Like Danilo suggested, maybe we need to add an „isreviewed” column.

Maybe we can start the procedure of adding isreviewed field in the db, and meanwhile use this solution.
This can be also a workaround for bug 339507 while we finalize the +translate page rework.

---------------------------

It sets the messagetranslation.potemplate field to None, but I still don't know how I should look for diverged messages and what to do with them.

Diverged messages are ones that have their potemplate field set. The diverged messages you're interested in have their potemplate field pointing to the same potemplate whose translation you're showing.

Just to make sure I understand:
By default, all translation messages are shared.
A diverged message is a translation that is not shared and it is only used in the template from „potemplate” field.

-------
Our cornercase is:

Someone could mark a translation as diverged, but without actually changing it's content.
In this case the translation will be duplicated in the database.

When changing such a translation from iscurrent, to needs review, we should look for duplicate messages and rather than marking the diverged message as „needs review”, we should delete it.

Is that correct?

Note: you can't change handling of 'needs review' unconditionally: you have to go through this path only when newly entered suggestions are empty.

Btw, a quick 'review' of your changes on http://bazaar.launchpad.net/~adiroiban/launchpad/bug-201749/revision/10467: browser/translationmessage.py 419-420 are unnecessary. That's what .get(something, False) does: defaults to False if it's not found. _checkTranslationConflict should be '_isTranslationConflict' because it returns a boolean.

And the things you are doing with current.potemplate and empty_translation .potemplate and .is_current flags are not going to do much good. The problem is that if there's a diverged translation, any new translation (including the empty one) is going to remain diverged by default.

So, unsetting is_current on empty_translation is probably a very bad idea. Perhaps first take should be to just set .date_reviewed on it to a current.date_reviewed-timedelta(0,1). But, don't forget to keep the current behaviour for 'needs review' flag when you are just adding a suggestion.

I still think that ticking „needs review” on a translation that is current, should mark that message as not having any current translations.

Here is an usecase.

Step 1:

Original message: „1 + 1 = 2 ”
UserA (not a reviewer) suggestions: „1 + 1 = 2”
UserB (not a reviewer) suggestions: „1 + 1 = ?”

Step 2:
UserC is a reviewer and he want to add the following suggestion „1 + 1 = 3” but he forget to tick the „needs review”.
The „1 + 1 = 3” suggestion is makerd as current and all previous suggestions discarded.

Step3:
UserC realizes he/she forgot to tick the needs review so he/she ticks it now, while „1 + 1 = 3” is the current translation.

Actual result:
„1 + 1 = 3” will be listed as „needs review” , but the original suggestions are not.
If we don't have the „isrevied” db field, „1 + 1 = 3” will be displayed as being created now.

Expected result:
„1 + 1 = 2”, „1 + 1 = ?” and „1 + 1 = 3” should be all listed as suggestions with their correct creating date.
-----------

Please let me know if you consider this is a valid use case for ticking needs review on a current translation.

That is a valid use case, and that's what this bug is about.

However, 'needs review' is currently used like this:

As a reviewer, you go to a message that has:

 Current: "1+1=2"
Suggestion: "1+1=3"

New suggestion: "1+1=2.5"
[x] Needs review

The ending situation should be:

 Current: "1+1=2"
Suggestion: "1+1=3"
Suggestion: "1+1=2.5"

This is the behaviour that we shouldn't break. Figuring out the when to do which is hard.

18:01 danilos : adiroiban, updateTranslations is off limits: no changes allowed there ;)
18:02 danilos : adiroiban, and date_created should probably not be modified
18:02 adiroiban : danilos: a I agree with date_created
18:03 adiroiban : but I don't know how to modify a potmsgset to be listed again as needs review
18:03 danilos : adiroiban, logic to determine which method to call should be in the browser code
18:03 danilos : adiroiban, potmsgset shouldn't be modified at all
18:04 danilos : adiroiban, it's a TM you are modifying, and https://bugs.edge.launchpad.net/rosetta/+bug/201749/comments/13
                   gives you a suggestion
18:04 mup : Bug #201749: Impossible to mark as needing review strings already translated or with suggestions
                   <story-translations-review> <ui> <Launchpad Translations:In Progress by adiroiban>
                   <https://launchpad.net/bugs/201749>
18:04 adiroiban : danilos: ok I will move the logic to browser code
18:04 danilos : adiroiban, and now, on to the resetCurrentTranslation method :)
18:07 danilos : adiroiban, the way you do it won't do: we don't have the luxury of fetching a bunch of messages for every
                   single message on +translate page

18:07 danilos : adiroiban, you need to get only the matching message if it exists
18:08 adiroiban : ok. but the logic is ok ?
18:08 danilos : adiroiban, and the thing you are doing there is not what you should be doing
18:09 danilos : adiroiban, no, sorry
18:09 adiroiban : ok
18:09 adiroiban : I still don't know how diverged messages shoudl be handled
18:10 danilos : adiroiban, basically, your code does this: "current, diverged translation 'blah' needs review, let's find
                   all other diverged translations 'blah' and make them 'shared'
18:12 adiroiban : and what it should do?
18:12 danilos : adiroiban, well, the only thing code should do is find an identical (to current) suggestion (meaning
                   .is_current=False, .potemplate=None) if it exists, and then only discard the older one
18:12 danilos : adiroiban, let me prepare an example
18:15 danilos : adiroiban, http://paste.ubuntu.com/394099/
18:15 danilos : adiroiban, fwiw, removing a message is impossible from web UI anyway, so you really can't fix this bug
                   properly
18:17 danilos : adiroiban, another example to be wary of: http://paste.ubuntu.com/394100/
18:18 adiroiban : how we should proceed with the removal of a message?
18:30 lantash1 > [~lantash@222-179.107-92.cust.bluewin.ch] joins #launchpad-reviews
18:30 danilos : adiroiban, well, the best way would be to somehow tag it an then remove it later
18:31 adiroiban : danilos: and when tag it... we should tag it in a wait it will not be selected by any other existing query
18:31 danilos : adiroiban, what do you mean?
18:32 danilos : adiroiban, perhaps we'll have to allow removal from the web UI as well, though that sounds very, very bad
18:32 danilos : adiroiban, there's also the transfer of all the flags, like is_imported, from the deleted message to the
                   preserved one
18:32 adir...

Fixed in stable r10770 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/10770>

Now I was just thinking .... what if we reset the translation when user enters a new empty translations ? (and no longer require ticking the „someone should review this translation”)