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
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 :) /code.edge. launchpad. net/~rockstar/ launchpad/ bug-520446/ +merge/ 21025 ?
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) 173-142- 134-98. pools.spcsdns. net] joins #launchpad-reviews
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
new column on translationmessage table called something like is_reviewed
just introduce more intricate bugs
the style guide. onMessage' on POTMsgSet that takes a
template, language, variant as parameters
the rest of the updateTranslation (which is a mess, and should not be extended in any way) translations/ tests/test_ potmsgset. py test_updateTran slation tests and how complex
they are
concrete meaning of the action is the way to go if you want to tackle this
18:34 gmb : rockstar: https:/
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
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@
18:38 danilos : adiroiban, I agree unsetting it as current is an improvement over existing behaviour, but you can't do that
18:39 danilos : adiroiban, finding all that out is pretty expensive, so I think a proper solution would be to introduce a
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
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
18:42 gmb : rockstar: Sure.
18:42 danilos : adiroiban, the solution would probably be to create a 'unsetTranslati
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
18:44 danilos : adiroiban, look at lib/lp/
18:45 danilos : adiroiban, making them even more complex is out of the question :) separate method which encapsulates the
18:45 adiroiban : ok
18:45 adiroiban : danilos: so I will move this logic in a new method
case in that particular method, because they'll be different
biggest complication with all that is that diverged messages might be duplicated so once they are not used
anymore, they should be aggregated age.potemplate ?
from jaunty, where the shared version is used in karmic, lucid,...)
suggestion
since we did incremental development, it was impossible to re-use pofile
translatio n; it means that the simplistic approach will just show all old (even those reviewed long ago)
suggestion s, which would be bad
suggestion s would end up being unreviewed
it, it gets even "nicer" :)
18:46 danilos : adiroiban, then you can worry about all the peculiarities of sharing and divergence for your specific use
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
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: translationmess
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
18:49 danilos : adiroiban, if potemplate IS NULL, you are looking either at a shared current/imported message, or at a
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
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
18:54 danilos : adiroiban, because unsetting a currrent translation will remove the date_reviewed info, and then all
18:54 danilos : adiroiban, so, that's another case to worry about; and when you introduce message sharing/divergence into
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