Merge lp:~henninge/launchpad/devel-710591-setcurrenttranslation-extension into lp:launchpad

[Henning Eggers]

[r=jtv][ui=none] [r=jtv][bug=710591][incr] Provide TranslationMessage.acceptFromImport and .acceptFromUpstreamImportOnPackage.

= Summary =

Provide a method to import translations in old style. Old style means that
translations coming from an import are marked as "imported" by setting the
"current" flag on the other side. It also follow the precedence rules as
outlined here:
https://wiki.ubuntu.com/Translations/KnowledgeBase/TranslationsPrecedence

== Proposed fix ==

Provide a method "TranslationMessage.acceptAsImported" to be used instead of
"TranslationMessage.approve" during import. Using the "approve" method here
is misleading as translations are not really approved. Hence the new method
does not take a "reviewer" parameter and does not set the "reviewer" and
"date_reviewed" attributes of a TranslationMessage.

== Pre-implementation notes ==

Talked to danilo about the need to fix this as a special case for
setCurrentTranslation.

== Implementation details ==

== Tests ==

bin/test -vvcm lp.translations.tests.test_translationmessage \
    -t AcceptAsImported

== Demo and Q/A ==

None.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/interfaces/translationmessage.py
  lib/lp/translations/tests/test_translationmessage.py
  lib/lp/testing/factory.py
  lib/lp/translations/model/potmsgset.py
  lib/lp/translations/model/translationmessage.py

[Jeroen T. Vermeulen (jtv)]

(Typo in ITranslation.acceptAsImported: "sold_style_import")

Hi Henning,

This looks like a very nice branch overall. Well-tested, well-documented, simple solution to a complex problem. We discussed on IRC, but unfortunately I don't have time to continue doing that today. The branch looks good to me in essence, so I'm approving now but with a few conditions.

First of those: POTMsgSet.acceptAsImported is two methods dressed up as one. There's a "mode switch" that I expect is going to be set explicitly near the call site, and then the two paths inside the method have very little in common. I can see how the method may have grown in that way, but now that it's stabilized it's clear that it needs to be split into two.

By the way, you note that the "old-style import" will be used "only if the upload is by_maintainer and to a source package and no upstream template [exists]." That means that the old-style code probably doesn't need to abstract away the two translation sides. That seems like a very specific behaviour involving both sides, in an asymmetric way. So perhaps the reasoning becomes easier if the sides are kept concrete. But I'll leave that for your consideration.

Second condition: "old-style" isn't going to be very meaningful to someone who started or will start working on Translations since the Recife rollout. That needs a better term. The way you describe it is nice and clear to me, but probably not to someone without the Rosetta background. Future engineers who read that may be afraid to touch the code (because they don't know what the old style does and why) or remove it (because you make it sound like something that's been dropped from the design).

We discussed this on IRC. It's hard to come up with a good name, but how about "upstream_for_package"?

Jeroen

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Oh, another note:

184 + def test_accept_marks_not_reviewed(self):
185 + # Accepting a suggestion does not update its review fields.

The name and the comment describe different behaviours. I believe the comment is the correct one. Could you fix the name?

Jeroen

[Henning Eggers (henninge)]

Thank you for the review. I split the method into two called "acceptFromImport" and "acceptFromUpstreamImportOnPackage", this way it is clear that this is a special case and the "old style" is gone, too. I also applied your suggestion from IRC to use the explicit flag names and added an assert that acceptFromUpstreamImportOnPackage can only be used on a source package.

I fail to produce an incremental diff as I had to merge devel to get some helpful factory additions that Aaron did. He added a "side" parameter to makePOTemplate and makePOFile so that it now much easier to test with source package templates.

[Henning Eggers (henninge)]

Hi Ian,
this was supposed to make the deadline but ec2 failed because of some silly little oversight. This the second-to-last branch in a series to fix bug 710591 which is Critical.

[Ian Booth (wallyworld)]

Hi

One very minor point: the sentence

"This method allow to store translation as upstream translation
even though there is no upstream template"

doesn't quite make sense as written. Perhaps it could be corrected.

review: Approve (release-critical)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-02-03 19:11:38 +0000
+++ lib/lp/testing/factory.py	2011-02-05 15:31:44 +0000
@@ -2716,10 +2716,10 @@
         removeSecurityProxy(potmsgset).sync()
         return potmsgset
 
-    def makePOFileAndPOTMsgSet(self, language_code, msgid=None,
-                               with_plural=False):
+    def makePOFileAndPOTMsgSet(self, language_code=None, msgid=None,
+                               with_plural=False, side=None):
         """Make a `POFile` with a `POTMsgSet`."""
-        pofile = self.makePOFile(language_code)
+        pofile = self.makePOFile(language_code, side=side)
 
         if with_plural:
             if msgid is None:

=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py	2011-01-31 16:02:10 +0000
+++ lib/lp/translations/interfaces/translationmessage.py	2011-02-05 15:31:44 +0000
@@ -267,6 +267,17 @@
     def approveAsDiverged(pofile, reviewer, lock_timestamp=None):
         """Approve this suggestion, as a diverged translation."""
 
+    def acceptFromImport(pofile, share_with_other_side=False,
+                         lock_timestamp=None):
+        """Accept a suggestion coming from a translation import."""
+
+    def acceptFromUpstreamImportOnPackage(pofile, lock_timestamp=None):
+        """Accept a suggestion coming from a translation import.
+
+        This method allows to mark a translation as being current in
+        upstream even though there is no upstream template.
+        """
+
     # XXX CarlosPerelloMarin 20071022: We should move this into browser code.
     def makeHTMLID(description):
         """Unique identifier for self, suitable for use in HTML element ids.

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2011-01-28 01:08:34 +0000
+++ lib/lp/translations/model/potmsgset.py	2011-02-05 15:31:44 +0000
@@ -739,6 +739,79 @@
             template.awardKarma(translator, 'translationsuggestionapproved')
             template.awardKarma(reviewer, 'translationreview')
 
+    def acceptFromImport(self, pofile, suggestion,
+                       share_with_other_side=False, lock_timestamp=None):
+        """Accept a suggestion coming from a translation import.
+
+        When importing translations, these are first added as a suggestion
+        and only after successful validation they are made current. This is
+        slightly different to approving a suggestion because no reviewer is
+        credited.
+
+        :param pofile: The `POFile` that the suggestion is being approved for.
+        :param suggestion: The `TranslationMessage` being approved.
+        :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(
+            template.translation_side)
+        if traits.getFlag(suggestion):
+            # Message is already current.
+            return
+
+        translator = suggestion.submitter
+        potranslations = dictify_translations(suggestion.all_msgstrs)
+        self._setTranslation(
+            pofile, translator, suggestion.origin, potranslations,
+            share_with_other_side=share_with_other_side,
+            identical_message=suggestion, lock_timestamp=lock_timestamp)
+
+    def acceptFromUpstreamImportOnPackage(self, pofile, suggestion,
+                                        lock_timestamp=None):
+        """Accept a suggestion coming from a translation import.
+
+        This method allow to store translation as upstream translation
+        even though there is no upstream template. It is similar to
+        acceptFromImport but will make sure not to overwrite existing
+        translations. Rather, it will mark the translation as being current
+        in upstream.
+
+        :param pofile: The `POFile` that the suggestion is being approved for.
+        :param suggestion: The `TranslationMessage` being approved.
+        :param lock_timestamp: Timestamp of the original translation state
+            that this change is based on.
+        """
+        template = pofile.potemplate
+        assert template.translation_side == TranslationSide.UBUNTU, (
+            "Do not use this method for an upstream project.")
+
+        if suggestion.is_current_ubuntu and suggestion.is_current_upstream:
+            # Message is already current.
+            return
+
+        current = self.getCurrentTranslation(
+            template, pofile.language, template.translation_side)
+        other = self.getOtherTranslation(
+            pofile.language, template.translation_side)
+        if other is not None:
+            other.is_current_upstream = False
+        if current is None or other is None:
+            translator = suggestion.submitter
+            potranslations = dictify_translations(suggestion.all_msgstrs)
+            self._setTranslation(
+                pofile, translator, suggestion.origin, potranslations,
+                share_with_other_side=True,
+                identical_message=suggestion,
+                lock_timestamp=lock_timestamp)
+        else:
+            # Make it only current in upstream.
+            suggestion.is_current_upstream = True
+            if suggestion != other:
+                pofile.markChanged(translator=suggestion.submitter)
+
     def _cloneAndDiverge(self, original_message, pofile):
         """Create a diverged clone of a `TranslationMessage`.
 

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2011-01-31 19:15:50 +0000
+++ lib/lp/translations/model/translationmessage.py	2011-02-05 15:31:44 +0000
@@ -172,6 +172,14 @@
         """See `ITranslationMessage`."""
         raise NotImplementedError()
 
+    def acceptFromImport(self, *args, **kwargs):
+        """See `ITranslationMessage`."""
+        raise NotImplementedError()
+
+    def acceptFromUpstreamImportOnPackage(self, *args, **kwargs):
+        """See `ITranslationMessage`."""
+        raise NotImplementedError()
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         return None
@@ -343,6 +351,19 @@
         return self.potmsgset.approveAsDiverged(
             pofile, self, reviewer, lock_timestamp=lock_timestamp)
 
+    def acceptFromImport(self, pofile, share_with_other_side=False,
+                         lock_timestamp=None):
+        """See `ITranslationMessage`."""
+        self.potmsgset.acceptFromImport(
+            pofile, self, share_with_other_side=share_with_other_side,
+            lock_timestamp=lock_timestamp)
+
+    def acceptFromUpstreamImportOnPackage(self, pofile,
+                                          lock_timestamp=None):
+        """See `ITranslationMessage`."""
+        self.potmsgset.acceptFromUpstreamImportOnPackage(
+            pofile, self, lock_timestamp=lock_timestamp)
+
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         from lp.translations.model.pofile import POFile
@@ -392,12 +413,13 @@
 
     def shareIfPossible(self):
         """See `ITranslationMessage`."""
-        if not self.is_diverged:
+        if self.potemplate is None:
             # Already converged.
             return
 
         # Existing shared direct equivalent to this message, if any.
         shared = self.getSharedEquivalent()
+
         # Existing shared ubuntu translation for this POTMsgSet, if
         # any.
         ubuntu = self.potmsgset.getCurrentTranslation(

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2011-02-03 17:38:15 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2011-02-05 15:31:44 +0000
@@ -77,6 +77,8 @@
 
 
 class TestApprove(TestCaseWithFactory):
+    """Tests for `TranslationMessage.approve`."""
+
     layer = ZopelessDatabaseLayer
 
     def test_approve_activates_message(self):
@@ -294,6 +296,183 @@
         self.assertNotEqual(ubuntu_tm, upstream_tm)
 
 
+class TestAcceptFromImport(TestCaseWithFactory):
+    """Tests for `TranslationMessage.acceptFromImport`.
+
+    This method is a lot like `TranslationMessage.approve`, so this test
+    mainly exercises what it does differently.
+    """
+
+    layer = ZopelessDatabaseLayer
+
+    def test_accept_activates_message(self):
+        # An untranslated message receives an imported translation.
+        pofile = self.factory.makePOFile()
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        reviewer = self.factory.makePerson()
+        self.assertFalse(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+        suggestion.acceptFromImport(pofile)
+
+        # By default the suggestion becomes current only on the side
+        # (upstream or Ubuntu) that it's being approved on.
+        self.assertTrue(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+    def test_accept_can_make_other_side_track(self):
+        # In some situations (see POTMsgSet.setCurrentTranslation for
+        # details) the acceptance can be made to propagate to the other
+        # side, subject to the share_with_other_side parameter.
+        pofile = self.factory.makePOFile()
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        reviewer = self.factory.makePerson()
+
+        self.assertFalse(suggestion.is_current_upstream)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+        suggestion.acceptFromImport(pofile, share_with_other_side=True)
+
+        self.assertTrue(suggestion.is_current_upstream)
+        self.assertTrue(suggestion.is_current_ubuntu)
+
+    def test_accept_does_not_set_review_fields(self):
+        # Accepting a suggestion does not update its review fields.
+        pofile = self.factory.makePOFile()
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        reviewer = self.factory.makePerson()
+
+        self.assertIs(None, suggestion.reviewer)
+        self.assertIs(None, suggestion.date_reviewed)
+
+        suggestion.acceptFromImport(pofile)
+
+        self.assertIs(None, suggestion.reviewer)
+        self.assertIs(None, suggestion.date_reviewed)
+
+    def test_accept_awards_no_karma(self):
+        # The translator receives no karma.
+        translator = self.factory.makePerson()
+        pofile = self.factory.makePOFile()
+        suggestion = self.factory.makeSuggestion(
+            pofile=pofile, translator=translator)
+
+        karmarecorder = self.installKarmaRecorder(person=translator)
+        suggestion.acceptFromImport(pofile)
+
+        self.assertEqual([], karmarecorder.karma_events)
+
+class TestAcceptFromUpstreamImportOnPackage(TestCaseWithFactory):
+    """Tests for `TranslationMessage.acceptFromUpstreamImportOnPackage`.
+
+    This method is a lot like `TranslationMessage.acceptFromImport`, so this
+    test mainly exercises what it does differently.
+    """
+
+    layer = ZopelessDatabaseLayer
+
+    def test_accept_activates_message_if_untranslated(self):
+        # An untranslated message accepts an imported translation.
+        pofile = self.factory.makePOFile(side=TranslationSide.UBUNTU)
+        suggestion = self.factory.makeSuggestion(pofile=pofile)
+        reviewer = self.factory.makePerson()
+        self.assertFalse(suggestion.is_current_ubuntu)
+        self.assertFalse(suggestion.is_current_upstream)
+
+        suggestion.acceptFromUpstreamImportOnPackage(pofile)
+
+        # Messages are always accepted on the other side, too.
+        self.assertTrue(suggestion.is_current_ubuntu)
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_accept_no_previously_imported(self):
+        # If there was already a current translation, but no previously
+        # imported one, it is disabled when a suggestion is accepted.
+        pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet(
+            side=TranslationSide.UBUNTU)
+        suggestion = self.factory.makeSuggestion(
+            pofile=pofile, potmsgset=potmsgset)
+        incumbent_message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset)
+
+        self.assertTrue(incumbent_message.is_current_ubuntu)
+        self.assertFalse(suggestion.is_current_ubuntu)
+
+        suggestion.acceptFromUpstreamImportOnPackage(pofile)
+
+        self.assertFalse(incumbent_message.is_current_ubuntu)
+        self.assertTrue(suggestion.is_current_ubuntu)
+        # Messages are always accepted on the other side, too.
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_accept_previously_imported(self):
+        # If there was already a current translation, and a previously
+        # imported one, the current translation is left untouched.
+        pofile, potmsgset = self.factory.makePOFileAndPOTMsgSet(
+            side=TranslationSide.UBUNTU)
+        imported_message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, current_other=True)
+        imported_message.is_current_ubuntu = False
+
+        suggestion = self.factory.makeSuggestion(
+            pofile=pofile, potmsgset=potmsgset)
+        incumbent_message = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset)
+
+        self.assertTrue(incumbent_message.is_current_ubuntu)
+        self.assertFalse(suggestion.is_current_ubuntu)
+        self.assertTrue(imported_message.is_current_upstream)
+
+        suggestion.acceptFromUpstreamImportOnPackage(pofile)
+
+        self.assertTrue(incumbent_message.is_current_ubuntu)
+        self.assertFalse(suggestion.is_current_ubuntu)
+        # Messages are always accepted on the other side, too.
+        self.assertFalse(imported_message.is_current_upstream)
+        self.assertTrue(suggestion.is_current_upstream)
+
+    def test_accept_current_message(self):
+        # Accepting a message that's already current does nothing on this
+        # side but makes sure the other side's flag is set.
+        pofile = self.factory.makePOFile(side=TranslationSide.UBUNTU)
+        translation = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile)
+        self.assertTrue(translation.is_current_ubuntu)
+        self.assertFalse(translation.is_current_upstream)
+
+        translation.acceptFromUpstreamImportOnPackage(pofile)
+
+        self.assertTrue(translation.is_current_ubuntu)
+        self.assertTrue(translation.is_current_upstream)
+
+    def test_accept_current_and_imported_message(self):
+        # Accepting a message that's already current and was also imported
+        # does nothing.
+        pofile = self.factory.makePOFile(side=TranslationSide.UBUNTU)
+        translation = self.factory.makeCurrentTranslationMessage(
+            pofile=pofile, current_other=True)
+        self.assertTrue(translation.is_current_ubuntu)
+        self.assertTrue(translation.is_current_upstream)
+
+        translation.acceptFromUpstreamImportOnPackage(pofile)
+
+        self.assertTrue(translation.is_current_ubuntu)
+        self.assertTrue(translation.is_current_upstream)
+
+    def test_accept_detects_conflict(self):
+        pofile = self.factory.makePOFile(side=TranslationSide.UBUNTU)
+        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.acceptFromUpstreamImportOnPackage,
+            pofile, lock_timestamp=old)
+
+
 class TestApproveAsDiverged(TestCaseWithFactory):
     """Tests for `TranslationMessage.approveAsDiverged`."""