launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-708385 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #708385 "duplicate key value violates unique constraint" when uploading .po
  https://bugs.launchpad.net/bugs/708385

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-708385/+merge/51169

= Summary =

Users were hitting unique violations when trying to replace certain translations.  They were running into unique constraints we have on, in particular, the respective shared current translations for a given potmsgset and language.

The problem wasn't explicit in the code, but the code wasn't explicitly setting a flush order either.  So where the code cleared the "current" flag on one message and then set it on another, the ORM was free to set it on the second message first, and boom: constraint violation.


== Proposed fix ==

Add explicit flush order, so that the "incumbent" message that has its flag cleared gets flushed before the new message has its flag set.


== Pre-implementation notes ==

Mostly discussed with the affected users, but also with Henning.


== Implementation details ==

I added the flush order in two places.  In the order in which you'll see them in the diff:

1. Where we also "steal" the other side's flag from another message.  I also rewrote the comment there because I felt it could be clearer, and removed an "if" whose condition was always True (because it was in an "else" to another "if" on the opposite condition).

2. Just before setting the current flag on the new translation, if there was an incumbent message whose flag we stole.

There is no functional change whatsoever.


== Tests ==

I couldn't think of any meaningful way to test this.  Any remaining failures are likely to be for obscure scenarios, and there's just no way to cover those reliably.

(The translations test suite still passes with the change, of course).


== Demo and Q/A ==

This should make the unique-constraint violation on current-ubuntu and current-upstream TranslationMessages go away.  It's hard to reproduce though.  Try changing the already-translated messages in https://translations.qastaging.launchpad.net/loco-directory/trunk/+pots/loco-directory/nl/+translate?search=mondiale


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/model/potmsgset.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-708385/+merge/51169
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-708385 into lp:launchpad.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2011-02-17 14:18:27 +0000
+++ lib/lp/translations/model/potmsgset.py	2011-02-24 16:59:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -1035,16 +1035,21 @@
                         traits.other_side_traits.getCurrentMessage(
                             self, pofile.potemplate, pofile.language))
                     if other_incumbent is None:
+                        # Untranslated on the other side; use the new
+                        # translation there as well.
                         traits.other_side_traits.setFlag(message, True)
                     elif (incumbent_message is None and
                           traits.side == TranslationSide.UPSTREAM):
-                        # If this is the first upstream translation, we
-                        # we use it as the current Ubuntu translation
-                        # too, overriding a possibly existing current
-                        # Ubuntu translation.
-                        if other_incumbent is not None:
-                            traits.other_side_traits.setFlag(
-                                other_incumbent, False)
+                        # Translating upstream, and the message was
+                        # previously untranslated.  Any translation in
+                        # Ubuntu is probably different, but only because
+                        # no upstream translation was available.  In
+                        # this special case the upstream translation
+                        # overrides the Ubuntu translation.
+                        traits.other_side_traits.setFlag(
+                            other_incumbent, False)
+                        Store.of(message).add_flush_order(
+                            other_incumbent, message)
                         traits.other_side_traits.setFlag(message, True)
             elif character == '+':
                 if share_with_other_side:
@@ -1058,6 +1063,8 @@
             message = twin
 
         if not traits.getFlag(message):
+            if incumbent_message is not None and message != incumbent_message:
+                Store.of(message).add_flush_order(incumbent_message, message)
             traits.setFlag(message, True)
             pofile.markChanged(translator=submitter)