Comment 8 for bug 201749

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 encapsulates the
                   concrete meaning of the action is the way to go if you want to tackle this
18:45 adiroiban : ok

18:45 adiroiban : danilos: so I will move this logic in a new method
18:46 danilos : adiroiban, then you can worry about all the peculiarities of sharing and divergence for your specific use
                   case in that particular method, because they'll be different
18:47 adiroiban : danilos: unfortunately, I don't know what should I do for divergent and sharing messages
18:47 danilos : adiroiban, right, and then you will have to come up with a good story for divergence and sharing; the
                   biggest complication with all that is that diverged messages might be duplicated so once they are not used
                   anymore, they should be aggregated
18:47 adiroiban : but if those cases have tests
18:47 adiroiban : I guess I can run the tests
18:47 danilos : adiroiban, well, for one, once you unset is_current, you should unset TM.potemplate as well
18:47 adiroiban : and improve the implementation
18:48 danilos : adiroiban, yeah, I'd be happy to help explain as much as possible
18:48 danilos : adiroiban, there are a lot of tests, and they are still not complete
18:48 adiroiban : danilos: translationmessage.potemplate ?
18:48 adiroiban : looking the in DB
18:49 adiroiban : that field is not set for all translations
18:49 danilos : adiroiban, yes, that indicates that that message is a divergence for a particular template (i.e. one coming
                   from jaunty, where the shared version is used in karmic, lucid,...)
18:49 danilos : adiroiban, if potemplate IS NULL, you are looking either at a shared current/imported message, or at a
                   suggestion
18:50 adiroiban : I see
18:50 adiroiban : ok
18:50 adiroiban : alse
18:50 adiroiban : else
18:50 adiroiban : is the ID of the specific potemplate
18:50 danilos : adiroiban, ideally, that would have been pofile, and we'd lose (potemplate, language, variant) on TM, but
                   since we did incremental development, it was impossible to re-use pofile
18:50 danilos : adiroiban, that's right
18:51 adiroiban : ok
18:51 adiroiban : I will take care of the TM.potemplate
18:53 danilos : adiroiban, basically, a TM is unreviewed suggestion if it has a date_created newer than the current
                   translation; it means that the simplistic approach will just show all old (even those reviewed long ago)
                   suggestions, which would be bad
18:54 danilos : adiroiban, because unsetting a currrent translation will remove the date_reviewed info, and then all
                   suggestions would end up being unreviewed
18:54 danilos : adiroiban, so, that's another case to worry about; and when you introduce message sharing/divergence into
                   it, it gets even "nicer" :)
18:58 adiroiban : danilos: why is bad if you see all suggetions ?
18:59 danilos : adiroiban, because some have already been reviewed
18:59 danilos : adiroiban, and rejected
18:59 adiroiban : danilos: yes
18:59 adiroiban : but some were rejected by mistake
18:59 danilos : adiroiban, that's a different bug ;)
18:59 adiroiban : or were rejected based on some translation rules
18:59 adiroiban : that was now changed
18:59 danilos : adiroiban, well, different problem
19:00 danilos : adiroiban, we don't want them all to show up on a regular +translate page
19:00 adiroiban : danilos: but we can not tell which suggestion was already reviewed
19:00 danilos : adiroiban, yes we can
19:00 danilos : adiroiban, as I said, there's currently a clear concept of what a reviewed message is
19:00 danilos : adiroiban, anyway, I've got a call now, ttyl
19:00 adiroiban : ok
19:00 adiroiban : I will move the code in a separate logic
19:01 adiroiban : and will consider the cases we discussed here
19:01 adiroiban : and then we can have a new review