Merge lp:~jtv/launchpad/recife-check-conflicts into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

Consistent support for conflict checks.

= Check for Translation Conflicts =

For the Recife feature branch. This implements more consistent support for translation conflict checks.

It's something we're already doing in the data model we have in production: when a translation is changed (whether through the web UI or through an import), a timestamp can be given that basically says, "this is when I looked at the state of this translation and decided to change it." The methods that change the translation can then check whether someone else might have set a new translation in the meantime; if they have, the later change is effectively obsolete.

Here I change the higher-level methods that set translations to accept a lock_timestamp, and check.

You'll notice that _checkForConflict builds on the existing _maybeRaiseTranslationConflict in a slightly awkward way. I hope we'll want to merge the two methods in the future to make this easier, but didn't want to pull in more changes for now. The third alternative would have been to check for identical translations _before_ calling _maybeRaiseTranslationConflict, but since this check is the most expensive step (and this is pretty "hot" code) I also wanted it to be the rarest.

To test:
{{{
./bin/test -vvc -m lp.translations.tests -t TestApprove -t CurrentTranslation
}}}

Jeroen

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2010-08-10 17:28:08 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2010-08-24 07:55:55 +0000
@@ -267,7 +267,8 @@
         """
 
     def setCurrentTranslation(pofile, submitter, translations, origin,
-                              share_with_other_side=False):
+                              share_with_other_side=False,
+                              lock_timestamp=None):
         """Set the message's translation in Ubuntu, or upstream, or both.
 
         :param pofile: `POFile` you're setting translations in.  Other
@@ -279,6 +280,8 @@
         :param origin: A `RosettaTranslationOrigin`.
         :param share_with_other_side: When sharing this translation,
             share it with the other `TranslationSide` as well.
+        :param lock_timestamp: Timestamp of the original translation state
+            that this change is based on.
         """
 
     def resetCurrentTranslation(pofile, lock_timestamp):
@@ -294,7 +297,8 @@
         """
 
     def clearCurrentTranslation(pofile, submitter, origin,
-                                share_with_other_side=False):
+                                share_with_other_side=False,
+                                lock_timestamp=None):
         """Set the current message in `pofile` to be untranslated.
 
         If the current message is shared, this will also clear it in
@@ -310,6 +314,8 @@
             current on the other side (i.e. the Ubuntu side if working
             upstream, or vice versa) then should it be cleared there as
             well?
+        :param lock_timestamp: Timestamp of the original translation state
+            that this change is based on.
         """
 
     def applySanityFixes(unicode_text):

=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py	2010-08-20 05:56:43 +0000
+++ lib/lp/translations/interfaces/translationmessage.py	2010-08-24 07:55:55 +0000
@@ -240,7 +240,8 @@
         It must not be referenced by any other object.
         """
 
-    def approve(pofile, reviewer, share_with_other_side=False):
+    def approve(pofile, reviewer, share_with_other_side=False,
+                lock_timestamp=None):
         """Approve this suggestion, making it a current translation."""
 
     # XXX CarlosPerelloMarin 20071022: We should move this into browser code.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-24 07:55:54 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-24 07:55:55 +0000
@@ -37,8 +37,7 @@
     IPOTMsgSet,
     POTMsgSetInIncompatibleTemplatesError,
     TranslationCreditsType)
-from lp.translations.interfaces.side import (
-    ITranslationSideTraitsSet, TranslationSide)
+from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat)
 from lp.translations.interfaces.translationimporter import (
@@ -89,6 +88,22 @@
 incumbent_unknown = object()
 
 
+def dictify_msgstrs(potranslations):
+    """Represent `potranslations` as a normalized dict.
+
+    :param potranslations: a dict or sequence of `POTranslation`s per
+        plural form.
+    :return: a dict mapping each translated plural form to a
+        `POTranslation`.  Untranslated forms are omitted.
+    """
+    if not isinstance(potranslations, dict):
+        potranslations = dict(enumerate(potranslations))
+    return dict(
+        (form, msgstr)
+        for form, msgstr in potranslations.iteritems()
+        if msgstr is not None)
+
+
 class POTMsgSet(SQLBase):
     implements(IPOTMsgSet)
 
@@ -932,6 +947,60 @@
             origin=RosettaTranslationOrigin.ROSETTAWEB, submitter=submitter,
             **forms)
 
+    def _checkForConflict(self, current_message, lock_timestamp,
+                          potranslations=None):
+        """Check `message` for conflicting changes since `lock_timestamp`.
+
+        Call this before changing this message's translations, to ensure
+        that a read-modify-write operation on a message does not
+        accidentally overwrite newer changes based on older information.
+
+        One example of a read-modify-write operation is: user downloads
+        translation file, translates a message, then re-uploads.
+        Another is: user looks at a message in the web UI, decides that
+        neither the current translation nor any of the suggestions are
+        right, and clears the message.
+
+        In these scenarios, it's possible for someone else to come along
+        and change the message's translation between the time we provide
+        the user with a view of the current state and the time we
+        receive a change from the user.  We call this a conflict.
+
+        Raises `TranslationConflict` if a conflict exists.
+
+        :param currentmessage: The `TranslationMessage` that is current
+            now.  This is where we'll see any conflicting changes
+            reflected in the date_reviewed timestamp.
+        :param lock_timestamp: The timestamp of the translation state
+            that the change is based on.
+        :param potranslations: `POTranslation`s dict for the new
+            translation.  If these are given, and identical to those of
+            `current_message`, there is no conflict.
+        """
+        if lock_timestamp is None:
+            # We're not really being asked to check for conflicts.
+            return
+        if current_message is None:
+            # There is no current message to conflict with.
+            return
+        try:
+            self._maybeRaiseTranslationConflict(
+                current_message, lock_timestamp)
+        except TranslationConflict:
+            if potranslations is None:
+                # We don't know what translations are going to be set;
+                # based on the timestamps this is a conflict.
+                raise
+            old_msgstrs = dictify_msgstrs(current_message.all_msgstrs)
+            new_msgstrs = dictify_msgstrs(potranslations)
+            if new_msgstrs != old_msgstrs:
+                # Yup, there really is a difference.  This is a proper
+                # conflict.
+                raise
+            else:
+                # Two identical translations crossed.  Not a conflict.
+                pass
+
     def _maybeRaiseTranslationConflict(self, message, lock_timestamp):
         """Checks if there is a translation conflict for the message.
 
@@ -1008,7 +1077,7 @@
             **translation_args)
 
     def approveSuggestion(self, pofile, suggestion, reviewer,
-                          share_with_other_side=False):
+                          share_with_other_side=False, lock_timestamp=None):
         """Approve a suggestion.
 
         :param pofile: The `POFile` that the suggestion is being approved for.
@@ -1017,6 +1086,8 @@
             suggestion.
         :param share_with_other_side: Policy selector: share this change with
             the other translation side if possible?
+        :param lock_timestamp: Timestamp of the original translation state
+            that this change is based on.
         """
         template = pofile.potemplate
         traits = getUtility(ITranslationSideTraitsSet).getTraits(
@@ -1030,7 +1101,7 @@
         activated_message = self._setTranslation(
             pofile, translator, suggestion.origin, potranslations,
             share_with_other_side=share_with_other_side,
-            identical_message=suggestion)
+            identical_message=suggestion, lock_timestamp=lock_timestamp)
 
         activated_message.markReviewed(reviewer)
         if reviewer != translator:
@@ -1038,7 +1109,8 @@
             template.awardKarma(reviewer, 'translationreview')
 
     def setCurrentTranslation(self, pofile, submitter, translations, origin,
-                              share_with_other_side=False):
+                              share_with_other_side=False,
+                              lock_timestamp=None):
         """See `IPOTMsgSet`."""
         potranslations = self._findPOTranslations(translations)
         identical_message = self._findTranslationMessage(
@@ -1046,10 +1118,12 @@
         return self._setTranslation(
             pofile, submitter, origin, potranslations,
             share_with_other_side=share_with_other_side,
-            identical_message=identical_message)
+            identical_message=identical_message,
+            lock_timestamp=lock_timestamp)
 
     def _setTranslation(self, pofile, submitter, origin, potranslations,
-                        identical_message=None, share_with_other_side=False):
+                        identical_message=None, share_with_other_side=False,
+                        lock_timestamp=None):
         """Set the current translation.
 
         :param pofile: The `POFile` to set the translation in.
@@ -1060,11 +1134,13 @@
         :param identical_message: The already existing message, if any,
             that's either shared or diverged for `pofile.potemplate`,
             whose translations are identical to the ones we're setting.
-        :param share_with_other_side:
-        :return:
+        :param share_with_other_side: Propagate this change to the other
+            translation side if appropriate.
+        :param lock_timestamp: The timestamp of the translation state
+            that the change is based on.
+        :return: The `TranslationMessage` that is current after
+            completion.
         """
-        # XXX JeroenVermeulen 2010-08-16: Update pofile.date_changed.
-
         twin = identical_message
 
         traits = getUtility(ITranslationSideTraitsSet).getTraits(
@@ -1074,6 +1150,9 @@
         incumbent_message = traits.getCurrentMessage(
             self, pofile.potemplate, pofile.language)
 
+        self._checkForConflict(
+            incumbent_message, lock_timestamp, potranslations=potranslations)
+
         # Summary of the matrix:
         #  * If the incumbent message is diverged and we're setting a
         #    translation that's already shared: converge.
@@ -1178,7 +1257,7 @@
                         traits.other_side_traits.getCurrentMessage(
                             self, pofile.potemplate, pofile.language))
                     if other_incumbent is None:
-                        traits.other_side_side.setFlag(message, True)
+                        traits.other_side_traits.setFlag(message, True)
             elif character == '+':
                 if share_with_other_side:
                     traits.other_side_traits.setFlag(message, True)
@@ -1189,7 +1268,9 @@
         if decisions == '':
             message = twin
 
-        traits.setFlag(message, True)
+        if not traits.getFlag(message):
+            traits.setFlag(message, True)
+            pofile.markChanged(translator=submitter)
 
         return message
 
@@ -1214,11 +1295,13 @@
             pofile.date_changed = UTC_NOW
 
     def clearCurrentTranslation(self, pofile, submitter, origin,
-                                share_with_other_side=False):
+                                share_with_other_side=False,
+                                lock_timestamp=None):
         """See `IPOTMsgSet`."""
         message = self.setCurrentTranslation(
             pofile, submitter, [], origin,
-            share_with_other_side=share_with_other_side)
+            share_with_other_side=share_with_other_side,
+            lock_timestamp=lock_timestamp)
         message.markReviewed(submitter)
 
     def applySanityFixes(self, text):

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2010-08-20 05:56:43 +0000
+++ lib/lp/translations/model/translationmessage.py	2010-08-24 07:55:55 +0000
@@ -326,11 +326,13 @@
             date_reviewed = current.date_created
         return date_reviewed > self.date_created
 
-    def approve(self, pofile, reviewer, share_with_other_side=False):
+    def approve(self, pofile, reviewer, share_with_other_side=False,
+                lock_timestamp=None):
         """See `ITranslationMessage`."""
         self.potmsgset.approveSuggestion(
             pofile, self, reviewer,
-            share_with_other_side=share_with_other_side)
+            share_with_other_side=share_with_other_side,
+            lock_timestamp=lock_timestamp)
 
     def getOnePOFile(self):
         """See `ITranslationMessage`."""

=== modified file 'lib/lp/translations/tests/test_clearcurrenttranslation.py'
--- lib/lp/translations/tests/test_clearcurrenttranslation.py	2010-08-20 05:43:19 +0000
+++ lib/lp/translations/tests/test_clearcurrenttranslation.py	2010-08-24 07:55:55 +0000
@@ -21,7 +21,9 @@
 from lp.testing import TestCaseWithFactory
 from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationmessage import (
-    RosettaTranslationOrigin)
+    RosettaTranslationOrigin,
+    TranslationConflict,
+    )
 
 ORIGIN = RosettaTranslationOrigin.SCM
 
@@ -249,6 +251,17 @@
         self.assertEqual(new_reviewer, current.reviewer)
         self.assertSqlAttributeEqualsDate(current, 'date_reviewed', UTC_NOW)
 
+    def test_detects_conflict(self):
+        pofile = self._makePOFile()
+        current_message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile)
+        old = datetime.now(UTC) - timedelta(days=7)
+
+        self.assertRaises(
+            TranslationConflict,
+            current_message.potmsgset.clearCurrentTranslation,
+            pofile, self.factory.makePerson(), ORIGIN, lock_timestamp=old)
+
 
 class TestClearCurrentTranslationUpstream(TestCaseWithFactory,
                                           ScenarioMixin):

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-08-24 07:55:54 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-08-24 07:55:55 +0000
@@ -14,6 +14,8 @@
 from zope.security.proxy import isinstance as zope_isinstance
 from zope.security.proxy import removeSecurityProxy
 
+from storm.locals import Store
+
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
@@ -24,7 +26,6 @@
     TranslationFileFormat)
 from lp.translations.interfaces.translationmessage import (
     RosettaTranslationOrigin, TranslationConflict)
-from lp.translations.interfaces.side import TranslationSide
 from lp.translations.model.translationmessage import (
     DummyTranslationMessage)
 
@@ -945,11 +946,10 @@
             name='main', product=self.foo)
         removeSecurityProxy(self.foo).official_rosetta = True
 
-        self.potemplate = self.factory.makePOTemplate(
+        template = self.potemplate = self.factory.makePOTemplate(
             productseries=self.foo_main, name="messages")
-        self.potmsgset = self.factory.makePOTMsgSet(self.potemplate,
-                                                    sequence=1)
-        self.pofile = self.factory.makePOFile('eo', self.potemplate)
+        self.potmsgset = self.factory.makePOTMsgSet(template, sequence=1)
+        self.pofile = self.factory.makePOFile('eo', template)
 
     def test_resetCurrentTranslation_shared(self):
         # Resetting a shared current translation will change iscurrent=False
@@ -1625,3 +1625,75 @@
 
         self.assertEqual(message, potmsgset.getImportedTranslationMessage(
             pofile.potemplate, pofile.language))
+
+    def test_detects_conflict(self):
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+        translations = self._makeTranslations(potmsgset)
+        origin = RosettaTranslationOrigin.ROSETTAWEB
+
+        # A translator bases a change on a page view from 5 minutes ago.
+        lock_timestamp = datetime.now(pytz.UTC) - timedelta(minutes=5)
+
+        # Meanwhile someone else changes the same message's translation.
+        newer_translation = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset)
+
+        # This raises a translation conflict.
+        self.assertRaises(
+            TranslationConflict,
+            potmsgset.setCurrentTranslation,
+            pofile, pofile.potemplate.owner, translations, origin,
+            lock_timestamp=lock_timestamp)
+
+
+class TestCheckForConflict(TestCaseWithFactory):
+    """Test POTMsgSet._checkForConflict."""
+
+    layer = ZopelessDatabaseLayer
+
+    def test_passes_nonconflict(self):
+        # If there is no conflict, _checkForConflict completes normally.
+        current_tm = self.factory.makeCurrentTranslationMessage()
+        potmsgset = removeSecurityProxy(current_tm.potmsgset)
+        newer = current_tm.date_reviewed + timedelta(days=1)
+
+        potmsgset._checkForConflict(current_tm, newer)
+
+    def test_detects_conflict(self):
+        # If there's been another translation since lock_timestamp,
+        # _checkForConflict raises TranslationConflict.
+        current_tm = self.factory.makeCurrentTranslationMessage()
+        potmsgset = removeSecurityProxy(current_tm.potmsgset)
+        older = current_tm.date_reviewed - timedelta(days=1)
+
+        self.assertRaises(
+            TranslationConflict,
+            potmsgset._checkForConflict,
+            current_tm, older)
+
+    def test_passes_identical_change(self):
+        # When concurrent translations are identical, there is no
+        # conflict.
+        current_tm = self.factory.makeCurrentTranslationMessage()
+        potmsgset = removeSecurityProxy(current_tm.potmsgset)
+        older = current_tm.date_reviewed - timedelta(days=1)
+
+        potmsgset._checkForConflict(
+            current_tm, older, potranslations=current_tm.all_msgstrs)
+
+    def test_quiet_if_no_current_message(self):
+        # If there is no current translation, _checkForConflict accepts
+        # that as conflict-free.
+        potemplate = self.factory.makePOTemplate()
+        potmsgset = self.factory.makePOTMsgSet(potemplate, sequence=1)
+        old = datetime.now(pytz.UTC) - timedelta(days=366)
+
+        removeSecurityProxy(potmsgset)._checkForConflict(None, old)
+
+    def test_quiet_if_no_timestamp(self):
+        # If there is no lock_timestamp, _checkForConflict does not
+        # check for conflicts.
+        current_tm = self.factory.makeCurrentTranslationMessage()
+        potmsgset = removeSecurityProxy(current_tm.potmsgset)
+
+        removeSecurityProxy(potmsgset)._checkForConflict(current_tm, None)

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2010-08-17 10:13:25 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2010-08-24 07:55:55 +0000
@@ -18,7 +18,9 @@
 from lp.translations.model.translationmessage import DummyTranslationMessage
 from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationmessage import (
-    ITranslationMessage)
+    ITranslationMessage,
+    TranslationConflict,
+    )
 from lp.translations.interfaces.translations import TranslationConstants
 from canonical.testing import ZopelessDatabaseLayer
 
@@ -250,6 +252,19 @@
 
         self.assertEqual([], karmarecorder.karma_events)
 
+    def test_approve_detects_conflict(self):
+        pofile = self.factory.makePOFile('bo')
+        current = self.factory.makeCurrentTranslationMessage(pofile=pofile)
+        potmsgset = current.potmsgset
+        suggestion = self.factory.makeSuggestion(
+            pofile=pofile, potmsgset=potmsgset)
+        old = datetime.now(UTC) - timedelta(days=1)
+
+        self.assertRaises(
+            TranslationConflict,
+            suggestion.approve,
+            pofile, self.factory.makePerson(), lock_timestamp=old)
+
 
 class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
     """Tests for `TranslationMessage.findIdenticalMessage`."""