Merge lp:~danilo/launchpad/get-current-translation into lp:~launchpad/launchpad/recife

[Данило Шеган]

= getCurrentTranslation =

== Background ==

This implements a getCurrentTranslation method for the upstream integration work. We are providing it in parallel to existing getCurrentTranslationMessage/getImportedTranslatioMessage methods so we make the transition easier (if we just try to replace them, a million tests fail and it's basically impossible to move from there).

The work that we are doing is enabling sharing of translations between Ubuntu and upstream projects. The concept that we introduced earlier is "translation sides" — Ubuntu or upstream — which indicate what particular translation we are interested in. So, a single POTMsgSet (basically, an English string container) will provide a translation for both an upstream project and Ubuntu package (and it can even be the same one).

This work extends our existing message sharing infrastructure, where one POTMsgSet can be in more than one POTemplate. The concepts that are relevant to this branch are "shared" and "diverged" translations. A shared translation is one that is not specific to any one POTemplate (so it has TranslationMessage.potemplate == None), and as such is shared between any POTemplates which have no diverged translation. A diverged translation is a TranslationMessage linked to a POTMsgSet which has POTemplate set, and is an overriding translation strictly for that POTemplate.

== Implementation ==

IPOTMsgSet.getCurrentTranslation gets a current translation for POTMsgSet
for a given POTemplate and a given Language. By default, it assumes you are looking for whatever side POTemplate is on (either upstream or Ubuntu)

When POTemplate is not passed in, it is treated as a request solely for the shared message.

TranslationSide traits class is used to generalize some common operations which depend on the side (UPSTREAM/UBUNTU) without having to write code that specifically calls that out. For instance, getFlag call that is used on TranslationMessage is used to construct "TranslationMessage.is_current_ubuntu == True" or "TranslationMessage.is_current_upstream == True" Storm clauses.

However, that gives me a security proxied object so I had to add a removeSecurityProxy around it. I am not sure how I could better solve this (other than moving it into the traits implementation itself, but that's probably even worse). Any suggestions welcome.

Rest of it should be fairly self-explanatory, except perhaps for the order_by clause at the end: it simply sorts diverged translations (those with POTemplate set) before shared ones (so they are preferred ones).

Tests are unified for both "sides" and only basic methods are re-implemented. If test set-up is not clear, I'd be happy to re-write it and provide better commentary.

== Demo & QA ==

This goes into our integration branch, and will be QAd together with it. At the moment, this method is not used and that's coming in follow-up branches: we are taking it step-by-step.

== Tests ==

bin/test -cvvt GetCurrentTranslation

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/interfaces/potmsgset.py
  lib/lp/translations/model/potmsgset.py
  lib/lp/translations/tests/test_potmsgset.py

[Jeroen T. Vermeulen (jtv)]

Doesn't this duplicate TranslationSideTraits.getCurrentMessage and POTMsgSet._getUsedTranslationMessage? If so, you could use this new code to replace those (and conveniently sweep out any non-Storm code along the way). That would require splitting it up a bit further though.

Also, I question the wisdom of intelligently deriving the translation side from a potemplate argument that is optional. We're using the same argument for two very different purposes: divergence and side-sensitivity. It gives rise to two kinds of call: "figure out the side from this template" and "I'll figure out the side; the template may be None." We'd be better off with those as separate methods to represent side-sensitive and side-insensitive "layers" in our code. The latter is basically our existing get{Current,Imported}TranslationMessage and the former is basically a wrapper around that layer.

To do case-sensitivity right we'd need another optional argument, along the lines of ignore_diverged=False. I think that would make the interfaces cleaner and more helpful overall.

Jeroen

[Jeroen T. Vermeulen (jtv)]

(By the way the branch I'm working on has a basic 3-line stub implementation of getCurrentTranslation, with the potemplate non-optional; supporting "ignore_diverged" would only take an extra "if ignore_diverged: potemplate = None" after the TranslationSideTraits have been retrieved.)

[Данило Шеган (danilo)]

Hi Jeroen,

Thanks for taking a look.

У уто, 16. 11 2010. у 07:16 +0000, Jeroen T. Vermeulen пише:
> Doesn't this duplicate TranslationSideTraits.getCurrentMessage and
> POTMsgSet._getUsedTranslationMessage?

Uhm, yes. That's the whole idea: create new methods which support the
new semantics (concept of sides), while keeping old ones intact.

> If so, you could use this new code to replace those (and conveniently
> sweep out any non-Storm code along the way).

Ultimately, yes. The branch's sole purpose is to not remove/replace any
existing code because that's going to cause a lot of test breakage.
I've even tried that long time ago and decided to split it up for that
very reason. I think it was even you who strongly pushed for the
approach, so I am very much surprised by these questions. :)

FWIW, TranslationSideTraits.getCurrentMessage should be switched to use
this method or the other way around. Not in this branch, though.

> That would require splitting it up a bit further though.

This originally started as an internal method to replace _getUsedTM. As
soon as we decide to switch actual callsites (wrapper methods in
IPOTMsgSet) to the new method, I believe it should be made private
again. Or perhaps not, and we should just use this single method.

> Also, I question the wisdom of intelligently deriving the translation
> side from a potemplate argument that is optional. We're using the
> same argument for two very different purposes: divergence and
> side-sensitivity.

Yes, I did consider that too. It turned out it's useful and simplifies
testing. It's easy to split that code out, but as we start using this
method in tests more and more, we are going to hate to have to pass in
potemplate.translation_side in every single time. Perhaps that should
live in the test helpers, and I am fine with that if that's what you'd
agree with.

Still, I feel it's not heavy at all and could very well stay in, but I
am not very attached to the functionality. ;)

> It gives rise to two kinds of call: "figure out the
> side from this template" and "I'll figure out the side; the template
> may be None." We'd be better off with those as separate methods to
> represent side-sensitive and side-insensitive "layers" in our code.
> The latter is basically our existing
> get{Current,Imported}TranslationMessage and the former is basically a
> wrapper around that layer.
>
> To do case-sensitivity right we'd need another optional argument,
> along the lines of ignore_diverged=False. I think that would make the
> interfaces cleaner and more helpful overall.

Uhm, not really (on the "we'd need"). You can get a shared translation
for either side by passing the side explicitely in (while keeping
potemplate=None). As I said, this method could probably be made private
instead.

[Данило Шеган (danilo)]

Ok, Jeroen and I discussed this on IRC in order to agree on a general approach (and clarify what I've been doing). I've done a few minor changes and am now looking for a proper review :)

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2010-11-16 10:51:59 +0000
@@ -279,6 +279,15 @@
         If a translation conflict is detected, TranslationConflict is raised.
         """
 
+    def getCurrentTranslation(potemplate, language, side=None):
+        """Get a current translation message.
+
+        :param potemplate: An `IPOTemplate` to look up a translation for.
+            If it's None, returns a shared translation.
+        :param language: translation should be to this `ILanguage`.
+        :param side: translation side to look at.
+        """
+
     def setCurrentTranslation(pofile, submitter, translations, origin,
                               share_with_other_side=False,
                               lock_timestamp=None):

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-11-15 16:25:05 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-11-16 10:51:59 +0000
@@ -20,7 +20,12 @@
     SQLObjectNotFound,
     StringCol,
     )
-from storm.expr import SQL
+from storm.expr import (
+    Coalesce,
+    Desc,
+    Or,
+    SQL,
+    )
 from storm.store import (
     EmptyResultSet,
     Store,
@@ -327,6 +332,34 @@
         return self._getUsedTranslationMessage(
             None, language, current=True)
 
+    def getCurrentTranslation(self, potemplate, language, side):
+        """See `IPOTMsgSet`."""
+
+        from zope.security.proxy import removeSecurityProxy
+        assert side is not None, "Translation side must be specified."
+
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(side)
+        clauses = [removeSecurityProxy(
+            traits.getFlag(TranslationMessage)) == True]
+
+        if potemplate is None:
+            # Look only for a shared translation.
+            clauses.append(TranslationMessage.potemplate == None)
+        else:
+            clauses.append(
+                Or(TranslationMessage.potemplate == None,
+                   TranslationMessage.potemplateID == potemplate.id))
+        clauses.extend([
+            TranslationMessage.potmsgsetID == self.id,
+            TranslationMessage.languageID == language.id])
+
+        # Return a diverged translation if it exists, and fall back
+        # to the shared one otherwise.
+        result = Store.of(self).find(
+            TranslationMessage, *clauses).order_by(
+              Desc(Coalesce(TranslationMessage.potemplateID, -1))).first()
+        return result
+
     def getLocalTranslationMessages(self, potemplate, language,
                                     include_dismissed=False,
                                     include_unreviewed=True):

=== modified file 'lib/lp/translations/model/side.py'
--- lib/lp/translations/model/side.py	2010-08-23 08:41:03 +0000
+++ lib/lp/translations/model/side.py	2010-11-16 10:51:59 +0000
@@ -33,12 +33,8 @@
 
     def getCurrentMessage(self, potmsgset, potemplate, language):
         """See `ITranslationSideTraits`."""
-        if self.side == TranslationSide.UPSTREAM:
-            return potmsgset.getImportedTranslationMessage(
-                potemplate, language)
-        else:
-            return potmsgset.getCurrentTranslationMessage(
-                potemplate, language)
+        return potmsgset.getCurrentTranslation(
+            potemplate, language, self.side)
 
     def setFlag(self, translationmessage, value):
         """See `ITranslationSideTraits`."""

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-11-16 10:51:59 +0000
@@ -24,7 +24,6 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.testing import TestCaseWithFactory
-from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.potmsgset import (
     POTMsgSetInIncompatibleTemplatesError,
@@ -38,6 +37,7 @@
     RosettaTranslationOrigin,
     TranslationConflict,
     )
+from lp.translations.interfaces.side import TranslationSide
 from lp.translations.model.translationmessage import DummyTranslationMessage
 
 
@@ -1713,6 +1713,203 @@
             lock_timestamp=lock_timestamp)
 
 
+class BaseTestGetCurrentTranslation(object):
+    layer = DatabaseFunctionalLayer
+
+    def test_no_translation(self):
+        # getCurrentTranslation returns None when there's no translation.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+
+        self.assertIs(None,
+                      potmsgset.getCurrentTranslation(
+                          pofile.potemplate, pofile.language, self.this_side))
+
+    def test_basic_get(self):
+        # getCurrentTranslation gets the current translation
+        # for a message.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+        translations = {
+            0: self.factory.getUniqueString('translation'), }
+        origin = RosettaTranslationOrigin.SCM
+        message = potmsgset.setCurrentTranslation(
+            pofile, pofile.potemplate.owner, translations, origin)
+
+        self.assertEqual(message,
+                         potmsgset.getCurrentTranslation(
+                             pofile.potemplate, pofile.language,
+                             self.this_side))
+
+    def test_assertion_on_bad_parameters(self):
+        # When no potemplate is passed in to getCurrentTranslation,
+        # and no explicit side is selected, AssertionError is raised.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+
+        self.assertRaises(AssertionError,
+                          potmsgset.getCurrentTranslation,
+                          None, pofile.language, None)
+
+    def test_other_languages_ignored(self):
+        # getCurrentTranslation never returns a translation for another
+        # language.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+        pofile_other_language = self.factory.makePOFile(
+            potemplate=pofile.potemplate)
+        translations = {
+            0: self.factory.getUniqueString('translation'), }
+        origin = RosettaTranslationOrigin.SCM
+        message = potmsgset.setCurrentTranslation(
+            pofile_other_language, pofile.potemplate.owner,
+            translations, origin)
+
+        self.assertIs(None,
+                      potmsgset.getCurrentTranslation(
+                          pofile.potemplate, pofile.language, self.this_side))
+
+    def test_other_diverged_no_translation(self):
+        # getCurrentTranslation gets the current upstream translation
+        # for a message.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+        pofile_other = self._makeOtherPOFile(pofile, potmsgset)
+
+        # Create a diverged translation in pofile_other.
+        translations = {
+            0: self.factory.getUniqueString('translation'), }
+        suggestion = potmsgset.submitSuggestion(
+            pofile_other, pofile_other.potemplate.owner, translations)
+        suggestion.approveAsDiverged(
+            pofile_other, pofile_other.potemplate.owner)
+
+        self.assertIs(None,
+                      potmsgset.getCurrentTranslation(
+                          pofile.potemplate, pofile.language, self.this_side))
+
+    def test_other_side(self):
+        # getCurrentTranslation gets the current translation
+        # for a message depending on the side that is specified.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+        pofile_other = self._makeOtherPOFile(pofile, potmsgset)
+
+        # Create current translations in 'pofile' and 'pofile_other'.
+        translations_here = {
+            0: self.factory.getUniqueString('here'), }
+        translations_other = {
+            0: self.factory.getUniqueString('other'), }
+        origin = RosettaTranslationOrigin.SCM
+
+        current_translation = potmsgset.setCurrentTranslation(
+            pofile, pofile.potemplate.owner,
+            translations_here, origin)
+        other_translation = potmsgset.setCurrentTranslation(
+            pofile_other, pofile_other.potemplate.owner,
+            translations_other, origin)
+
+        self.assertEquals(current_translation,
+                          potmsgset.getCurrentTranslation(
+                              pofile_other.potemplate, pofile_other.language,
+                              self.this_side))
+        self.assertEquals(other_translation,
+                          potmsgset.getCurrentTranslation(
+                              pofile.potemplate, pofile.language,
+                              self.other_side))
+
+    def test_prefers_diverged(self):
+        # getCurrentTranslation prefers a diverged translation if
+        # it's available for the given potemplate.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+
+        # Create both a shared and a diverged translation in pofile.
+        translations_shared = {
+            0: self.factory.getUniqueString('shared'), }
+        translations_diverged = {
+            0: self.factory.getUniqueString('diverged'), }
+        origin = RosettaTranslationOrigin.SCM
+        shared_message = potmsgset.setCurrentTranslation(
+            pofile, pofile.potemplate.owner, translations_shared, origin)
+        diverged_message = potmsgset.submitSuggestion(
+            pofile, pofile.potemplate.owner, translations_diverged)
+        diverged_message.approveAsDiverged(pofile, pofile.potemplate.owner)
+
+        self.assertEquals(diverged_message,
+                          potmsgset.getCurrentTranslation(
+                              pofile.potemplate, pofile.language,
+                              self.this_side))
+
+    def test_shared_when_requested(self):
+        # getCurrentTranslation returns a shared translation even with
+        # diverged translation present if shared one was asked for.
+        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
+
+        # Create both a shared and a diverged translation in pofile.
+        translations_shared = {
+            0: self.factory.getUniqueString('shared'), }
+        translations_diverged = {
+            0: self.factory.getUniqueString('diverged'), }
+        origin = RosettaTranslationOrigin.SCM
+        shared_message = potmsgset.setCurrentTranslation(
+            pofile, pofile.potemplate.owner, translations_shared, origin)
+        diverged_message = potmsgset.submitSuggestion(
+            pofile, pofile.potemplate.owner, translations_diverged)
+        diverged_message.approveAsDiverged(pofile, pofile.potemplate.owner)
+
+        self.assertEquals(shared_message,
+                          potmsgset.getCurrentTranslation(
+                              None, pofile.language, self.this_side))
+
+
+class TestGetCurrentTranslationForUpstreams(BaseTestGetCurrentTranslation,
+                                            TestCaseWithFactory):
+    """getCurrentTranslation working on an upstream POFile."""
+
+    def setUp(self):
+        super(TestGetCurrentTranslationForUpstreams, self).setUp(
+            '[e-mail]')
+        self.this_side = TranslationSide.UPSTREAM
+        self.other_side = TranslationSide.UBUNTU
+
+    def _makePOFileAndPOTMsgSet(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        return pofile, potmsgset
+
+    def _makeOtherPOFile(self, pofile, potmsgset):
+        sp = self.factory.makeSourcePackage()
+        potemplate = self.factory.makePOTemplate(
+            name=pofile.potemplate.name,
+            distroseries=sp.distroseries,
+            sourcepackagename=sp.sourcepackagename)
+        pofile_other = self.factory.makePOFile(potemplate=potemplate,
+                                               language=pofile.language)
+        potmsgset.setSequence(potemplate, 1)
+        return pofile_other
+
+
+class TestGetCurrentTranslationForUbuntu(BaseTestGetCurrentTranslation,
+                                         TestCaseWithFactory):
+    """getCurrentTranslation working on an Ubuntu POFile."""
+
+    def setUp(self):
+        super(TestGetCurrentTranslationForUbuntu, self).setUp(
+            '[e-mail]')
+        self.this_side = TranslationSide.UBUNTU
+        self.other_side = TranslationSide.UPSTREAM
+
+    def _makePOFileAndPOTMsgSet(self):
+        sp = self.factory.makeSourcePackage()
+        potemplate = self.factory.makePOTemplate(
+            distroseries=sp.distroseries,
+            sourcepackagename=sp.sourcepackagename)
+        pofile = self.factory.makePOFile(potemplate=potemplate)
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        return pofile, potmsgset
+
+    def _makeOtherPOFile(self, pofile, potmsgset):
+        potemplate = self.factory.makePOTemplate(name=pofile.potemplate.name)
+        pofile_other = self.factory.makePOFile(potemplate=potemplate,
+                                               language=pofile.language)
+        potmsgset.setSequence(potemplate, 1)
+        return pofile_other
+
+
 class TestCheckForConflict(TestCaseWithFactory):
     """Test POTMsgSet._checkForConflict."""