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  adiroiban : danilos: i guess there are many SELECT queries that will need to be modified to take into acount the tag 
18:33    danilos : adiroiban, yeah, it'd end up being a pretty big change, with many performance implications
18:34    danilos : adiroiban, as such, I wouldn't tackle this before many other things are cleaned up first
18:35  adiroiban : danilos: but this is a big issues if someone is using Launchpad translations for reviewing or doing QA 
                   related tasks
18:36    danilos : adiroiban, I am not disagreeing, I am just saying that it'll take a lot of work to get this done properly; 
                   or, we can ignore some of the issues and live with some problems we introduce, but we've got to be aware of 
                   them first
18:37    danilos : adiroiban, for example, perhaps it's not a big deal if we've got duplicate identical suggestions in the DB, 
                   so let's not block on that
18:38  adiroiban : danilos: for the duplicate suggestions
18:38    danilos : adiroiban, we'll get some weird behaviour occasionally, but let's say they are of lower priority
18:38  adiroiban : maybe we can have a cron job that will do the cleaning
18:38    danilos : adiroiban, now, there's still some things to worry about

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  adiroiban : danilos: i guess there are many SELECT queries that will need to be modified to take into acount the tag 
18:33    danilos : adiroiban, yeah, it'd end up being a pretty big change, with many performance implications
18:34    danilos : adiroiban, as such, I wouldn't tackle this before many other things are cleaned up first
18:35  adiroiban : danilos: but this is a big issues if someone is using Launchpad translations for reviewing or doing QA 
                   related tasks
18:36    danilos : adiroiban, I am not disagreeing, I am just saying that it'll take a lot of work to get this done properly; 
                   or, we can ignore some of the issues and live with some problems we introduce, but we've got to be aware of 
                   them first
18:37    danilos : adiroiban, for example, perhaps it's not a big deal if we've got duplicate identical suggestions in the DB, 
                   so let's not block on that
18:38  adiroiban : danilos: for the duplicate suggestions
18:38    danilos : adiroiban, we'll get some weird behaviour occasionally, but let's say they are of lower priority
18:38  adiroiban : maybe we can have a cron job that will do the cleaning
18:38    danilos : adiroiban, now, there's still some things to worry about
18:38    danilos : adiroiban, maybe, but that'll be very complex as well
18:39  adiroiban : danilos: what are are the other issues?
18:39    danilos : adiroiban, and it won't stop any weirdness
18:40    danilos : adiroiban, the other issue is that we need to carefully consider what will happen when somebody asks a 
                   diverged translation to be reviewed, and there is an existing shared one
18:41    danilos : adiroiban, i.e. http://paste.ubuntu.com/394130/
18:41    danilos : adiroiban, also, note that you can't unset potemplate if is_imported is true either
18:43  adiroiban : danilos: for lates example, I think result 1 is fine
18:44  adiroiban : and then another reviewer can decide if the shared suggestion is better
18:44  adiroiban : or it will need a new diverged translation
18:45    danilos : adiroiban, right
18:46    danilos : adiroiban, ok, so, then it won't be that complicated
18:46  adiroiban : danilos: what are the consequences of setting potemplate = None for a msgset with is_imported = True ?
18:47    danilos : adiroiban, basically, we'll have to watch if current message is diverged or not; if it is, and it isn't 
                   is_imported, then you unset is_current and .potemplate and how you don't hit a DB constraint :)
18:47    danilos : adiroiban, it won't be diverged is_imported message, and if there's another shared is_imported message, 
                   you'd be in for a ride
18:47    danilos : adiroiban, basically, many places where only one is expected would assert/traceback

18:48    danilos : adiroiban, provided you don't hit a DB constraint first (I am not sure if we have one like that)
18:48  adiroiban : danilos: I see
18:50  adiroiban : danilos: ok. thanks. one more question regarding potmsgset.updateTranslation()
18:50    danilos : adiroiban, sure (though, you still can't touch it: nobody can, functionality needs to be slowly factored 
                   out of it :)
18:50  adiroiban : should I add a new bug for the case when you tick „needs” for a already reviewd translation?
18:51  adiroiban : or we already have a bug for that?
18:51    danilos : adiroiban, what do you mean?
18:51  adiroiban : if you add a new suggestion
18:51    danilos : adiroiban, ok?
18:51  adiroiban : and you tick „needs review”
18:51  adiroiban : it will be listed as a new suggestion
18:51    danilos : adiroiban, right, that's how it should work
18:52  adiroiban : but if in fact it is not a new suggesion
18:52  jtv : salgado-lunch: I've fixed up the weirdness in my branch.  I'm going to reboot now, hoping that that'll fix it.
18:52  adiroiban : and _findTranslationMessage
18:52  adiroiban : finds that this new suggestion
18:52    danilos : adiroiban, ah, right, it's an already discarded suggestion
18:52  adiroiban : is in fact an already existing suggestion
18:52        jtv < [~jtv@125.25.108.216.adsl.dynamic.totbb.net] quits [Quit: Leaving.]
18:52  adiroiban : it will not mark it as needs review
18:52  adiroiban : this is why I touched the updateTranslation code
18:53    danilos : adiroiban, I am pretty sure we have a bug about that as well
18:53  adiroiban : ok
18:54    danilos : adiroiban, if you ever want to fix that, you'll have to provide a new method addASuggestion() or something, 
                   similar to what you did for this bug, and move the functionality out of updateTranslation method and add 
                   new functionality you want :)
18:54    danilos : adiroiban, and again, moved the logic to decide what to do out into browser code
18:54  adiroiban : danilos: yes. I will move that logic.
18:54  danilos : s/moved/move
18:55  adiroiban : that was the initial implimentation
18:55  adiroiban : but then I didn't know where to put the tests
18:55  adiroiban : for that code
18:55    danilos : adiroiban, right, I am talking about the other bug where discared suggestions don't show up as suggestions 
                   afterwards
18:55  adiroiban : danilos: ah. ok... do you have any suggestion for where should I put the integration tests for 
                   resetCurrentTranslation ?
18:56  adiroiban : lib/lp/translations/browser/tests/translationmessage-views.txt ?

:56  adiroiban : I am very puzzled by the way unit tests and integration tests are structured
18:56    danilos : adiroiban, integration test? that would probably be very tricky, I'd look for existing needs review tests
18:57    danilos : adiroiban, good unit tests are most important here; single integration test is good enough
18:57  adiroiban : danilos: lib/lp/translations/tests/potmsgset-update-translation.txt
18:57    danilos : adiroiban, the problem is that all these translationmessage and pofile views are a mess, just like 
                   updateTranslation is: there is no clear separation of anything
18:58  adiroiban : danilos: ok
18:58  adiroiban : thanks!
18:58    danilos : adiroiban, that'd be for documentation on updateTranslation method, do it in potmsgset.txt with a single 
                   example for how to use resetCurrentTranslation (and please make it accept identical parameters as the rest 
                   of the methods: i.e. don't use pofile if other methods are passing potemplate, language and variant)
18:59  adiroiban : I will try to fix the current problems and come back with more questions
18:59    danilos : adiroiban, sure, I am likely to be out, but I can probably find some time over the weekend (email me :)
19:00  adiroiban : danilos: no emails for the weekend... and I don't know if I will have time over the weekend
19:00    danilos : adiroiban, heh, ok :)
19:01    danilos : adiroiban, anyway, to be honest, this should probably be a new checkbox closer to the current translation; 
                   hopefully redesign of +translate views will fix all this