Launchpad itself

Merge lp:~jtv/launchpad/recife-552639 into lp:~launchpad/launchpad/recife

recife-552639
Merge into recife

Proposed by Jeroen T. Vermeulen on 2010-05-07

Status:

Merged

Approved by:

Jeroen T. Vermeulen on 2010-06-30

Approved revision:

no longer in the source branch.

Merged at revision:

9140

Proposed branch:

lp:~jtv/launchpad/recife-552639

Merge into:

lp:~launchpad/launchpad/recife

Diff against target:

752 lines (+509/-30)

8 files modified

database/schema/security.cfg (+1/-1)
lib/lp/translations/interfaces/potmsgset.py (+21/-0)
lib/lp/translations/interfaces/translationmessage.py (+3/-0)
lib/lp/translations/interfaces/translations.py (+12/-3)
lib/lp/translations/model/potmsgset.py (+308/-24)
lib/lp/translations/model/translationmessage.py (+5/-0)
lib/lp/translations/tests/test_potmsgset.py (+122/-1)
lib/lp/translations/tests/test_translationmessage.py (+37/-1)

To merge this branch:

bzr merge lp:~jtv/launchpad/recife-552639

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Henning Eggers (community)	code	2010-05-07	Approve on 2010-06-21
Review via email: mp+24883@code.launchpad.net

Commit message

Implement setCurrentTranslation methods.

Description of the change

= Bug 552639 =

This implements the unconditional setting of a translation for a particular message as current in Ubuntu, or upstream, or both. See the spec: https://dev.launchpad.net/Translations/Specs/UpstreamImportIntoUbuntu/FixingIsImported/setCurrentTranslation#preview

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-05-13:

The "real" tests are being written separately by danilo. I only included some because I couldn't bear not to do it.

The code follows the decision matrix cited in the spec. There's ways to make it prettier, but we'll get to that sometime after we have good tests.

Jeroen

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-06-17:

The tests in danilo's separate branch now pass. I've made a few design changes, fixed one or two mistakes in the code, and changed the expected behavior of a few tests. But for something this big, not bad.

The few relevant tests that are in this branch you can run with:
{{{
./bin/test -vv -t test_potmsgset
}}}

If you're not using the feature branch (lp:~launchpad/launchpad/recife) as a merge base you will see some failures from resetCurrentTranslation tests, which are unrelated to this code. Henning has already fixed those in the main feature branch.

I'm going to put up the test suite for separate review, in order to keep line count in check.

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-17:

Download full text (21.2 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> === modified file 'database/schema/security.cfg'
OK
> === modified file 'lib/lp/translations/interfaces/potmsgset.py'
> --- lib/lp/translations/interfaces/potmsgset.py 2010-06-09 14:17:32 +0000
> +++ lib/lp/translations/interfaces/potmsgset.py 2010-06-17 09:25:53 +0000
> @@ -214,6 +214,10 @@
> force_edition_rights=False, allow_credits=False):
> """Update or create a translation message using `new_translations`.
>
> + This method is Launchpad Translations's sliderule: it does
> + everything, nobody fully understands it all, and we intend to
> + replace it with a range of less flexible tools.
> +
> :param pofile: a `POFile` to add `new_translations` to.
> :param submitter: author of the translations.
> :param new_translations: a dictionary of plural forms, with the
> @@ -261,6 +265,20 @@
> If a translation conflict is detected, TranslationConflict is raised.
> """
>
> + def setCurrentTranslation(pofile, submitter, translations, origin,
> + translation_side, share_with_other_side=False):
> + """Set the message's translation in Ubuntu, or upstream, or both.
> +
> + :param pofile:
> + :param submitter:
> + :param translations:
> + :param origin:
> + :param translation_side: a `TranslationSide` value.
> + :param share_with_other_side:
> + """
> + # XXX: Check signature before completing branch.
> + # XXX: Update docstring.

So, these XXX are supposed to be gone before we land the feature, right?

> +
> def resetCurrentTranslation(pofile, lock_timestamp):
> """Reset the currently used translation.
>
> === modified file 'lib/lp/translations/interfaces/translations.py'
OK

> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py 2010-06-10 11:24:51 +0000
> +++ lib/lp/translations/model/potmsgset.py 2010-06-17 09:25:53 +0000
> @@ -4,11 +4,16 @@
> # pylint: disable-msg=E0611,W0212
>
> __metaclass__ = type
> -__all__ = ['POTMsgSet']
> +__all__ = [
> + 'make_translation_side_message_traits',
> + 'POTMsgSet',
> + ]
> +
>
> import datetime
> import logging
> import pytz
> +import re
>
> from zope.interface import implements
> from zope.component import getUtility
> @@ -41,7 +46,8 @@
> RosettaTranslationOrigin,
> TranslationConflict,
> TranslationValidationStatus)
> -from lp.translations.interfaces.translations import TranslationConstants
> +from lp.translations.interfaces.translations import (
> + TranslationConstants, TranslationSide)
> from lp.translations.model.pomsgid import POMsgID
> from lp.translations.model.potranslation import POTranslation
> from lp.translations.model.translationmessage import (
> @@ -79,6 +85,91 @@
> u'credits are counted as translated.')
>
>
> +class TranslationSideMessageTraits:
> + """Dealing with a `POTMsgSet` on either `TranslationSide`.
> +
> + Encapsulates primitives that depend on translation side: finding the
> + message that ...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> === modified file 'database/schema/security.cfg'
OK
> === modified file 'lib/lp/translations/interfaces/potmsgset.py'
> --- lib/lp/translations/interfaces/potmsgset.py	2010-06-09 14:17:32 +0000
> +++ lib/lp/translations/interfaces/potmsgset.py	2010-06-17 09:25:53 +0000
> @@ -214,6 +214,10 @@
>                            force_edition_rights=False, allow_credits=False):
>          """Update or create a translation message using `new_translations`.
>  
> +        This method is Launchpad Translations's sliderule: it does
> +        everything, nobody fully understands it all, and we intend to
> +        replace it with a range of less flexible tools.
> +
>          :param pofile: a `POFile` to add `new_translations` to.
>          :param submitter: author of the translations.
>          :param new_translations: a dictionary of plural forms, with the
> @@ -261,6 +265,20 @@
>          If a translation conflict is detected, TranslationConflict is raised.
>          """
>  
> +    def setCurrentTranslation(pofile, submitter, translations, origin,
> +                              translation_side, share_with_other_side=False):
> +        """Set the message's translation in Ubuntu, or upstream, or both.
> +
> +        :param pofile:
> +        :param submitter:
> +        :param translations:
> +        :param origin:
> +        :param translation_side: a `TranslationSide` value.
> +        :param share_with_other_side:
> +        """
> +        # XXX: Check signature before completing branch.
> +        # XXX: Update docstring.

So, these XXX are supposed to be gone before we land the feature, right?

> +
>      def resetCurrentTranslation(pofile, lock_timestamp):
>          """Reset the currently used translation.
>  
> === modified file 'lib/lp/translations/interfaces/translations.py'
OK

> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py	2010-06-10 11:24:51 +0000
> +++ lib/lp/translations/model/potmsgset.py	2010-06-17 09:25:53 +0000
> @@ -4,11 +4,16 @@
>  # pylint: disable-msg=E0611,W0212
>  
>  __metaclass__ = type
> -__all__ = ['POTMsgSet']
> +__all__ = [
> +    'make_translation_side_message_traits',
> +    'POTMsgSet',
> +    ]
> +
>  
>  import datetime
>  import logging
>  import pytz
> +import re
>  
>  from zope.interface import implements
>  from zope.component import getUtility
> @@ -41,7 +46,8 @@
>      RosettaTranslationOrigin,
>      TranslationConflict,
>      TranslationValidationStatus)
> -from lp.translations.interfaces.translations import TranslationConstants
> +from lp.translations.interfaces.translations import (
> +    TranslationConstants, TranslationSide)
>  from lp.translations.model.pomsgid import POMsgID
>  from lp.translations.model.potranslation import POTranslation
>  from lp.translations.model.translationmessage import (
> @@ -79,6 +85,91 @@
>                         u'credits are counted as translated.')
>  
>  
> +class TranslationSideMessageTraits:
> +    """Dealing with a `POTMsgSet` on either `TranslationSide`.
> +
> +    Encapsulates primitives that depend on translation side: finding the
> +    message that is current on the given side, checking the flag that
> +    says whether a message is current on this side, setting or clearing
> +    the flag, and providing the same capabilities for the other side.
> +
> +    For an introduction to the Traits pattern, see
> +    http://www.cantrip.org/traits.html
> +    """

Traits! Cool concept. I had seen them in the STL but never knew about the
concept behind them.

> +    def __init__(self, potmsgset, potemplate=None, language=None,
> +                 variant=None):
> +        self.potmsgset = potmsgset
> +        self.potemplate = potemplate
> +        self.language = language
> +        self.variant = variant
> +
> +        self._found_incumbent = False
> +
> +    @property
> +    def incumbent_message(self):
> +        """Message that currently has the flag."""
> +        if not self._found_incumbent:
> +            self._incumbent = self._getIncumbentMessage()
> +            self._found_incumbent = True
> +        return self._incumbent
> +
> +    def getFlag(self, translationmessage):
> +        """Is this message the current one on this side?"""
> +        return getattr(translationmessage, self.flag_name)

It is not clear, where flag_name comes from. Wouldn't it be helpful to have a
"flag_name = None" attribute on the class, maybe with an explaining comment?

> +
> +    def setFlag(self, translationmessage, value):
> +        """Set or clear a message's "current" flag for this side."""
> +        if value == self.getFlag(translationmessage):
> +            return
> +
> +        if value and self.incumbent_message is not None:
> +            self.setFlag(self.incumbent_message, False)
> +
> +        setattr(translationmessage, self.flag_name, value)
> +        self._found_incumbent = False
> +
> +    def _getIncumbentMessage(self):
> +        """Get the message that is current on this side, if any."""
> +        raise NotImplementedError('_getIncumbentMessage')
> +
> +
> +class UpstreamSideTraits(TranslationSideMessageTraits):
> +    """Traits for upstream translations."""
> +
> +    side = TranslationSide.UPSTREAM
> +
> +    flag_name = 'is_current_upstream'
> +
> +    def _getIncumbentMessage(self):
> +        """See `TranslationSideMessageTraits`."""
> +        return self.potmsgset.getImportedTranslationMessage(
> +            self.potemplate, self.language, variant=self.variant)
> +
> +
> +class UbuntuSideTraits(TranslationSideMessageTraits):
> +    """Traits for Ubuntu translations."""
> +
> +    side = TranslationSide.UBUNTU
> +
> +    flag_name = 'is_current_ubuntu'
> +
> +    def _getIncumbentMessage(self):
> +        """See `TranslationSideMessageTraits`."""
> +        return self.potmsgset.getCurrentTranslationMessage(
> +            self.potemplate, self.language, variant=self.variant)
> +
> +
> +def make_translation_side_message_traits(side, potmsgset, potemplate,
> +                                         language, variant=None):
> +    """Create `TranslationSideTraits` object of the appropriate subtype."""
> +    ubuntu = UbuntuSideTraits(potmsgset, potemplate, language, variant)
> +    upstream = UpstreamSideTraits(potmsgset, potemplate, language, variant)
> +    upstream.other_side = ubuntu
> +    ubuntu.other_side = upstream

A completely undocumented attribute AFICT. I think either the base traits
class or the derived classes could use some documentary class attributes. ;-)

> +    mapping = dict((traits.side, traits) for traits in (ubuntu, upstream))
> +    return mapping[side]
> +
> +
>  class POTMsgSet(SQLBase):
>      implements(IPOTMsgSet)
>  
> @@ -232,13 +323,13 @@
>                                     current=True):
>          """Get a translation message which is either used in
>          Launchpad (current=True) or in an import (current=False).
> -        
> +
>          Prefers a diverged message if present.
>          """
> -        # Change the is_current_ubuntu and is_current_upstream conditions
> -        # with extreme care: they need to match condition specified in
> -        # indexes, or postgres may not pick them up (in complicated queries,
> -        # postgres query optimizer sometimes does text-matching of indexes).
> +        # Change 'is_current IS TRUE' and 'is_imported IS TRUE' conditions
Hm, this looks like something is being reverted ...

> +        # carefully: they need to match condition specified in indexes,
> +        # or Postgres may not pick them up (in complicated queries,
> +        # Postgres query optimizer sometimes does text-matching of indexes).
>          if current:
>              used_clause = 'is_current_ubuntu IS TRUE'
>          else:
> @@ -522,13 +613,17 @@
>                  potranslations[pluralform] = None
>          return potranslations
>  
> -    def _findTranslationMessage(self, pofile, potranslations, pluralforms):
> -        """Find a message for this `pofile`.
> -
> -        The returned message matches exactly the given `translations` strings
> -        comparing only `pluralforms` of them.
> +    def _findTranslationMessage(self, pofile, potranslations,
> +                                prefer_shared=True):
> +        """Find a matching message in this `pofile`.
> +
> +        The returned message matches exactly the given `translations`
> +        strings (except plural forms not supported by `pofile`, which
> +        are ignored).
> +
>          :param potranslations: A list of translation strings.
> -        :param pluralforms: The number of pluralforms to compare.
> +        :param prefer_shared: Whether to prefer a shared match over a
> +            diverged one.
>          """
>          clauses = ['potmsgset = %s' % sqlvalues(self),
>                     'language = %s' % sqlvalues(pofile.language),
> @@ -539,7 +634,7 @@
>          else:
>              clauses.append('variant = %s' % sqlvalues(pofile.variant))
>  
> -        for pluralform in range(pluralforms):
> +        for pluralform in range(pofile.plural_forms):
>              if potranslations[pluralform] is None:
>                  clauses.append('msgstr%s IS NULL' % sqlvalues(pluralform))
>              else:
> @@ -547,10 +642,15 @@
>                      sqlvalues(pluralform, potranslations[pluralform])))
>  
>          remaining_plural_forms = range(
> -            pluralforms, TranslationConstants.MAX_PLURAL_FORMS)
> -
> -        # Prefer shared messages over diverged ones.
> -        order = ['potemplate NULLS FIRST']
> +            pofile.plural_forms, TranslationConstants.MAX_PLURAL_FORMS)
> +
> +        # Prefer either shared or diverged messages, depending on
> +        # arguments.
> +        if prefer_shared:
> +            order = ['potemplate NULLS FIRST']
> +        else:
> +            order = ['potemplate NULLS LAST']
> +
>          # Normally at most one message should match.  But if there is
>          # more than one, prefer the one that adds the fewest extraneous
>          # plural forms.
> @@ -564,9 +664,9 @@
>          if len(matches) > 0:
>              if len(matches) > 1:
>                  logging.warn(
> -                    "Translation for POTMsgSet %s into %s "
> -                    "matches %s existing translations." % sqlvalues(
> -                        self, pofile.language.code, len(matches)))
> +                    "Translation for POTMsgSet %s into '%s' "
> +                    "matches %s existing translations.",
> +                        self.id, pofile.language.code, len(matches))
>              return matches[0]
>          else:
>              return None
> @@ -799,7 +899,7 @@
>          # of translations.  None if there is no such message and needs to be
>          # created.
>          matching_message = self._findTranslationMessage(
> -            pofile, potranslations, pofile.plural_forms)
> +            pofile, potranslations)
>  
>          match_is_upstream = (
>              matching_message is not None and
> @@ -897,7 +997,7 @@
>          potranslations = self._findPOTranslations(new_translations)
>  
>          existing_message = self._findTranslationMessage(
> -            pofile, potranslations, pofile.plural_forms)
> +            pofile, potranslations)
>          if existing_message is not None:
>              return existing_message
>  
> @@ -951,10 +1051,188 @@
>              current.reviewer = reviewer
>              current.date_reviewed = lock_timestamp
>  
> -    def setCurrentTranslation(self, pofile, submitter, translations, origin,
> -                              translation_side, share_with_other_side=False):
> -        """See `IPOTMsgSet`."""
> -
> +<<<<<<< TREE
> +    def setCurrentTranslation(self, pofile, submitter, translations, origin,
> +                              translation_side, share_with_other_side=False):
> +        """See `IPOTMsgSet`."""
> +

So, this is easy to resolve. This has to go away, there is a declaration
further down.

> +=======
> +    def _nameMessageStatus(self, message, translation_side_traits):
> +        """Figure out the decision-matrix status of a message.
> +
> +        This is used in navigating the decision matrix in
> +        `setCurrentTranslation`.
> +        """
> +        if message is None:
> +            return 'none'
> +        elif message.potemplate is None:
> +            if translation_side_traits.other_side.getFlag(message):
> +                return 'other_shared'
> +            else:
> +                return 'shared'
> +        else:
> +            assert message.potemplate is not None, "Confused message state."

Your either forgot this in here or it's paranoia. Since the preceding elif
checks the exact same condition, this really is not needed.

> +            return 'diverged'
> +
> +    def _makeTranslationMessage(self, pofile, submitter, translations, origin,
> +                                diverged=False):
> +        # XXX: Document.
> +        """."""
> +        if diverged:
> +            potemplate = pofile.potemplate
> +        else:
> +            potemplate = None
> +
> +        translation_args = dict(
> +            ('msgstr%d' % form, translation)
> +            for form, translation in translations.iteritems()
> +            )
> +
> +        return TranslationMessage(
> +            potmsgset=self,
> +            potemplate=potemplate,
> +            pofile=pofile,
> +            language=pofile.language,
> +            variant=pofile.variant,
> +            origin=origin,
> +            submitter=submitter,
> +            validation_status=TranslationValidationStatus.OK,
> +            **translation_args)
> +
> +    def setCurrentTranslation(self, pofile, submitter, translations, origin,
> +                              translation_side, share_with_other_side=False):
> +        """See `IPOTMsgSet`."""
> +        traits = make_translation_side_message_traits(
> +            translation_side, self, pofile.potemplate, pofile.language,
> +            variant=pofile.variant)
> +
> +        translations = self._findPOTranslations(translations)
> +        incumbent_message = traits.incumbent_message
> +        twin = self._findTranslationMessage(
> +            pofile, translations, prefer_shared=False)

Hm, I am not 100% sure what a twin is. Maybe you could explain that here?

> +
> +        # Summary of the matrix:
> +        #  * If the incumbent message is diverged and we're setting a
> +        #    translation that's already shared: converge.
> +        #  * If the incumbent message is diverged and we're setting a
> +        #    translation that's not already shared: maintain divergence.
> +        #  * If the incumbent message is shared, replace it.
> +        #  * If there is no twin, simply create a new message (shared or
> +        #    diverged depending; see above).
> +        #  * If there is a shared twin, activate it (but also diverge if
> +        #    necessary; see above).
> +        #  * If there is a diverged twin, activate it (and converge it
> +        #    if appropriate; see above).
> +        #  * If there is a twin that's shared on the other side,
> +
> +        # XXX: Steal flag if policy permits and either:

Do we still need to agree on this policy?

> +        #  - there is no shared active message on the other side, or
> +        #  - we're returning a shared message.
> +
> +        decision_matrix = {
> +            'incumbent_none': {
> +                'twin_none': 'Z1*',
> +                'twin_shared': 'Z4*',
> +                'twin_diverged': 'Z7*',
> +                'twin_other_shared': 'Z4',
> +            },
> +            'incumbent_shared': {
> +                'twin_none': 'B1*',
> +                'twin_shared': 'B4*',
> +                'twin_diverged': 'B7*',
> +                'twin_other_shared': 'B4',
> +            },
> +            'incumbent_diverged': {
> +                'twin_none': 'A2',
> +                'twin_shared': 'A5',
> +                'twin_diverged': 'A4',
> +                'twin_other_shared': 'A6',
> +            },
> +            'incumbent_other_shared': {
> +                'twin_none': 'B1+',
> +                'twin_shared': 'B4+',
> +                'twin_diverged': 'B7+',
> +                'twin_other_shared': '',

This last one should be '4' instead of special casing it at the end.

> +            },
> +        }
> +
> +        incumbent_state = "incumbent_%s" % self._nameMessageStatus(
> +            incumbent_message, traits)
> +        twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
> +
> +        decisions = decision_matrix[incumbent_state][twin_state]
> +        file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE
> +        assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
> +            "Bad decision string.")

Paranoia!

> +
> +        for character in decisions:
> +            if character == 'A':
> +                # Deactivate & converge.
> +                # There may be an identical shared message.
> +                traits.setFlag(incumbent_message, False)
> +                incumbent_message.shareIfPossible()
> +            elif character == 'B':
> +                # Deactivate.
> +                traits.setFlag(incumbent_message, False)
> +            elif character == 'Z':
> +                # There is no incumbent message, so do nothing to it.
> +                assert incumbent_message is None, (
> +                    "Incorrect Z in decision matrix.")

Ok, first you hide this check in _nameMessageStatus, now you pull it out in
the open for an assert. Maybe you should trust your matrix more and just put a
"pass" in here?

> +            elif character == '1':
> +                # Create & activate.
> +                message = self._makeTranslationMessage(
> +                    pofile, submitter, translations, origin)
> +            elif character == '2':
> +                # Create, diverge, activate.
> +                message = self._makeTranslationMessage(
> +                    pofile, submitter, translations, origin, diverged=True)
> +            elif character == '4':
> +                # Activate.
> +                message = twin
> +            elif character == '5':
> +                # If other is a suggestion, diverge and activate.
> +                # (If not, it's already active and has been unmasked by
> +                # our deactivating the incumbent).
> +                message = twin
> +                if not traits.getFlag(twin):
> +                    assert not traits.other_side.getFlag(twin), (
> +                        "Decision matrix says '5' for a message that's "
> +                        "active on the other side.")

Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the
error message is quite cryptic to an outsider.

> +                    message.potemplate = pofile.potemplate
> +            elif character == '6':
> +                # If other is not active, fork a diverged message.
> +                if traits.getFlag(twin):
> +                    message = twin
> +                else:
> +                    # The twin is used on the other side, so we can't
> +                    # just reuse it for our diverged message.  Create a
> +                    # new one.
> +                    message = self._makeTranslationMessage(
> +                        pofile, submitter, translations, origin,
> +                        diverged=True)
> +            elif character == '7':
> +                # Converge & activate.
> +                message = twin
> +                message.shareIfPossible()
> +            elif character == '*':
> +                if share_with_other_side:
> +                    if traits.other_side.incumbent_message is None:
> +                        traits.other_side.setFlag(message, True)
> +            elif character == '+':
> +                if share_with_other_side:
> +                    traits.other_side.setFlag(message, True)
> +            else:
> +                raise AssertionError(
> +                    "Bad character in decision string: %s" % character)
> +
> +        if decisions == '':
> +            message = twin

This can go away if you use the '4' as mentioned earlier.

> +
> +        traits.setFlag(message, True)
> +
> +        return message
> +
> +>>>>>>> MERGE-SOURCE
>      def resetCurrentTranslation(self, pofile, lock_timestamp):
>          """See `IPOTMsgSet`."""
>  
> @@ -1145,7 +1423,7 @@
>          # The credits message has a fixed "translator."
>          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
>  
> -        message = self.updateTranslation(
> +        self.updateTranslation(
>              pofile, translator, [credits_message_str],
>              is_current_upstream=False, allow_credits=True,
>              force_shared=True, force_edition_rights=True,
> === modified file 'lib/lp/translations/tests/test_potmsgset.py'

The tests look very good although I am sure there could be even more.... ;-)

Thanks for doing such a good job. I really like the approach to use the matrix
explicitly in the code and also the traits are a great idea. Well done!

I will not approve this yet, obviously, because of the text conflicts but I
have nothing else that needs serious fixing.

review needs-fixing code

Cheers,
Henning

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwaTMsACgkQBT3oW1L17ijACQCdFBkgqS4mFPcJnrYENrq3tqms
2FQAoLBZXCQ5CvV9L4mL21+G133GOe0x
=v9bE
-----END PGP SIGNATURE-----

review: Needs Fixing (code)

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-06-19:

Download full text (17.7 KiB)

> > === modified file 'lib/lp/translations/interfaces/potmsgset.py'
> > --- lib/lp/translations/interfaces/potmsgset.py 2010-06-09 14:17:32
> +0000
> > +++ lib/lp/translations/interfaces/potmsgset.py 2010-06-17 09:25:53
> +0000
> > @@ -214,6 +214,10 @@
> > force_edition_rights=False, allow_credits=False):
> > """Update or create a translation message using `new_translations`.
> >
> > + This method is Launchpad Translations's sliderule: it does
> > + everything, nobody fully understands it all, and we intend to
> > + replace it with a range of less flexible tools.
> > +
> > :param pofile: a `POFile` to add `new_translations` to.
> > :param submitter: author of the translations.
> > :param new_translations: a dictionary of plural forms, with the
> > @@ -261,6 +265,20 @@
> > If a translation conflict is detected, TranslationConflict is
> raised.
> > """
> >
> > + def setCurrentTranslation(pofile, submitter, translations, origin,
> > + translation_side,
> share_with_other_side=False):
> > + """Set the message's translation in Ubuntu, or upstream, or both.
> > +
> > + :param pofile:
> > + :param submitter:
> > + :param translations:
> > + :param origin:
> > + :param translation_side: a `TranslationSide` value.
> > + :param share_with_other_side:
> > + """
> > + # XXX: Check signature before completing branch.
> > + # XXX: Update docstring.
>
> So, these XXX are supposed to be gone before we land the feature, right?

Yes, I've fixed them now.

Once a long time ago I proposed allowing "reviewer notes" in the code. That was turned down, but now I sometimes include these malformed XXX comments to ensure that the reviewer will say something if I let them through.

> > === modified file 'lib/lp/translations/interfaces/potmsgset.py'
> > --- lib/lp/translations/interfaces/potmsgset.py       2010-06-09 14:17:32
> +0000
> > +++ lib/lp/translations/interfaces/potmsgset.py       2010-06-17 09:25:53
> +0000
> > @@ -214,6 +214,10 @@
> >                            force_edition_rights=False, allow_credits=False):
> >          """Update or create a translation message using `new_translations`.
> >
> > +        This method is Launchpad Translations's sliderule: it does
> > +        everything, nobody fully understands it all, and we intend to
> > +        replace it with a range of less flexible tools.
> > +
> >          :param pofile: a `POFile` to add `new_translations` to.
> >          :param submitter: author of the translations.
> >          :param new_translations: a dictionary of plural forms, with the
> > @@ -261,6 +265,20 @@
> >          If a translation conflict is detected, TranslationConflict is
> raised.
> >          """
> >
> > +    def setCurrentTranslation(pofile, submitter, translations, origin,
> > +                              translation_side,
> share_with_other_side=False):
> > +        """Set the message's translation in Ubuntu, or upstream, or both.
> > +
> > +        :param pofile:
> > +        :param submitter:
> > +        :param translations:
> > +        :param origin:
> > +        :param translation_side: a `TranslationSide` value.
> > +        :param share_with_other_side:
> > +        """
> > +        # XXX: Check signature before completing branch.
> > +        # XXX: Update docstring.
> 
> So, these XXX are supposed to be gone before we land the feature, right?

Yes, I've fixed them now.

Once a long time ago I proposed allowing "reviewer notes" in the code.  That was turned down, but now I sometimes include these malformed XXX comments to ensure that the reviewer will say something if I let them through.

> > === modified file 'lib/lp/translations/model/potmsgset.py'
> > --- lib/lp/translations/model/potmsgset.py    2010-06-10 11:24:51 +0000
> > +++ lib/lp/translations/model/potmsgset.py    2010-06-17 09:25:53 +0000
> > @@ -4,11 +4,16 @@
> >  # pylint: disable-msg=E0611,W0212
> >
> >  __metaclass__ = type
> > -__all__ = ['POTMsgSet']
> > +__all__ = [
> > +    'make_translation_side_message_traits',
> > +    'POTMsgSet',
> > +    ]
> > +
> >
> >  import datetime
> >  import logging
> >  import pytz
> > +import re
> >
> >  from zope.interface import implements
> >  from zope.component import getUtility
> > @@ -41,7 +46,8 @@
> >      RosettaTranslationOrigin,
> >      TranslationConflict,
> >      TranslationValidationStatus)
> > -from lp.translations.interfaces.translations import TranslationConstants
> > +from lp.translations.interfaces.translations import (
> > +    TranslationConstants, TranslationSide)
> >  from lp.translations.model.pomsgid import POMsgID
> >  from lp.translations.model.potranslation import POTranslation
> >  from lp.translations.model.translationmessage import (
> > @@ -79,6 +85,91 @@
> >                         u'credits are counted as translated.')
> >
> >
> > +class TranslationSideMessageTraits:
> > +    """Dealing with a `POTMsgSet` on either `TranslationSide`.
> > +
> > +    Encapsulates primitives that depend on translation side: finding the
> > +    message that is current on the given side, checking the flag that
> > +    says whether a message is current on this side, setting or clearing
> > +    the flag, and providing the same capabilities for the other side.
> > +
> > +    For an introduction to the Traits pattern, see
> > +    http://www.cantrip.org/traits.html
> > +    """
> 
> Traits! Cool concept. I had seen them in the STL but never knew about the
> concept behind them.

In this case it's not a completely pure application of the idea, in that they have references to the objects they apply to.  But it was a lot more convenient this way.

> > +    def __init__(self, potmsgset, potemplate=None, language=None,
> > +                 variant=None):
> > +        self.potmsgset = potmsgset
> > +        self.potemplate = potemplate
> > +        self.language = language
> > +        self.variant = variant
> > +
> > +        self._found_incumbent = False
> > +
> > +    @property
> > +    def incumbent_message(self):
> > +        """Message that currently has the flag."""
> > +        if not self._found_incumbent:
> > +            self._incumbent = self._getIncumbentMessage()
> > +            self._found_incumbent = True
> > +        return self._incumbent
> > +
> > +    def getFlag(self, translationmessage):
> > +        """Is this message the current one on this side?"""
> > +        return getattr(translationmessage, self.flag_name)
> 
> It is not clear, where flag_name comes from. Wouldn't it be helpful to have a
> "flag_name = None" attribute on the class, maybe with an explaining comment?

Done.

> > +
> > +    def setFlag(self, translationmessage, value):
> > +        """Set or clear a message's "current" flag for this side."""
> > +        if value == self.getFlag(translationmessage):
> > +            return
> > +
> > +        if value and self.incumbent_message is not None:
> > +            self.setFlag(self.incumbent_message, False)
> > +
> > +        setattr(translationmessage, self.flag_name, value)
> > +        self._found_incumbent = False
> > +
> > +    def _getIncumbentMessage(self):
> > +        """Get the message that is current on this side, if any."""
> > +        raise NotImplementedError('_getIncumbentMessage')
> > +
> > +
> > +class UpstreamSideTraits(TranslationSideMessageTraits):
> > +    """Traits for upstream translations."""
> > +
> > +    side = TranslationSide.UPSTREAM
> > +
> > +    flag_name = 'is_current_upstream'
> > +
> > +    def _getIncumbentMessage(self):
> > +        """See `TranslationSideMessageTraits`."""
> > +        return self.potmsgset.getImportedTranslationMessage(
> > +            self.potemplate, self.language, variant=self.variant)
> > +
> > +
> > +class UbuntuSideTraits(TranslationSideMessageTraits):
> > +    """Traits for Ubuntu translations."""
> > +
> > +    side = TranslationSide.UBUNTU
> > +
> > +    flag_name = 'is_current_ubuntu'
> > +
> > +    def _getIncumbentMessage(self):
> > +        """See `TranslationSideMessageTraits`."""
> > +        return self.potmsgset.getCurrentTranslationMessage(
> > +            self.potemplate, self.language, variant=self.variant)
> > +
> > +
> > +def make_translation_side_message_traits(side, potmsgset, potemplate,
> > +                                         language, variant=None):
> > +    """Create `TranslationSideTraits` object of the appropriate subtype."""
> > +    ubuntu = UbuntuSideTraits(potmsgset, potemplate, language, variant)
> > +    upstream = UpstreamSideTraits(potmsgset, potemplate, language, variant)
> > +    upstream.other_side = ubuntu
> > +    ubuntu.other_side = upstream
> 
> A completely undocumented attribute AFICT. I think either the base traits
> class or the derived classes could use some documentary class attributes. ;-)

Done.

> > +<<<<<<< TREE
> > +    def setCurrentTranslation(self, pofile, submitter, translations,
> origin,
> > +                              translation_side,
> share_with_other_side=False):
> > +        """See `IPOTMsgSet`."""
> > +
> 
> So, this is easy to resolve. This has to go away, there is a declaration
> further down.

It's a bit more complicated than that, but I got it out of the way.

> > +=======
> > +    def _nameMessageStatus(self, message, translation_side_traits):
> > +        """Figure out the decision-matrix status of a message.
> > +
> > +        This is used in navigating the decision matrix in
> > +        `setCurrentTranslation`.
> > +        """
> > +        if message is None:
> > +            return 'none'
> > +        elif message.potemplate is None:
> > +            if translation_side_traits.other_side.getFlag(message):
> > +                return 'other_shared'
> > +            else:
> > +                return 'shared'
> > +        else:
> > +            assert message.potemplate is not None, "Confused message
> state."
> 
> Your either forgot this in here or it's paranoia. Since the preceding elif
> checks the exact same condition, this really is not needed.

It's also partly caused by the "there must be an else" rule.  I resolved this by:

1. Adding an ITranslationMessage.is_diverged to allow for clearer checks.
2. Moving the "else" clause above the "elif" clause.
3. Flattening the "if" inside the "elif" clause to be part of the main if/elif/else.

Looks much nicer now.  We should have added is_diverged earlier.

> > +    def setCurrentTranslation(self, pofile, submitter, translations,
> origin,
> > +                              translation_side,
> share_with_other_side=False):
> > +        """See `IPOTMsgSet`."""
> > +        traits = make_translation_side_message_traits(
> > +            translation_side, self, pofile.potemplate, pofile.language,
> > +            variant=pofile.variant)
> > +
> > +        translations = self._findPOTranslations(translations)
> > +        incumbent_message = traits.incumbent_message
> > +        twin = self._findTranslationMessage(
> > +            pofile, translations, prefer_shared=False)
> 
> Hm, I am not 100% sure what a twin is. Maybe you could explain that here?

I thought the blueprint made that clear, but I've now documented it in the code.

> > +        # Summary of the matrix:
> > +        #  * If the incumbent message is diverged and we're setting a
> > +        #    translation that's already shared: converge.
> > +        #  * If the incumbent message is diverged and we're setting a
> > +        #    translation that's not already shared: maintain divergence.
> > +        #  * If the incumbent message is shared, replace it.
> > +        #  * If there is no twin, simply create a new message (shared or
> > +        #    diverged depending; see above).
> > +        #  * If there is a shared twin, activate it (but also diverge if
> > +        #    necessary; see above).
> > +        #  * If there is a diverged twin, activate it (and converge it
> > +        #    if appropriate; see above).
> > +        #  * If there is a twin that's shared on the other side,
> > +
> > +        # XXX: Steal flag if policy permits and either:
> 
> Do we still need to agree on this policy?

No, this comment was temporary and I removed it.

> > +        #  - there is no shared active message on the other side, or
> > +        #  - we're returning a shared message.
> > +
> > +        decision_matrix = {
> > +            'incumbent_none': {
> > +                'twin_none': 'Z1*',
> > +                'twin_shared': 'Z4*',
> > +                'twin_diverged': 'Z7*',
> > +                'twin_other_shared': 'Z4',
> > +            },
> > +            'incumbent_shared': {
> > +                'twin_none': 'B1*',
> > +                'twin_shared': 'B4*',
> > +                'twin_diverged': 'B7*',
> > +                'twin_other_shared': 'B4',
> > +            },
> > +            'incumbent_diverged': {
> > +                'twin_none': 'A2',
> > +                'twin_shared': 'A5',
> > +                'twin_diverged': 'A4',
> > +                'twin_other_shared': 'A6',
> > +            },
> > +            'incumbent_other_shared': {
> > +                'twin_none': 'B1+',
> > +                'twin_shared': 'B4+',
> > +                'twin_diverged': 'B7+',
> > +                'twin_other_shared': '',
> 
> This last one should be '4' instead of special casing it at the end.

That's true, but it's not time for that yet.  In the design, this case means "the message we want to activate is already active and nothing needs to be done."  It happens that we could implement it in the same way as 4, but that is a detail of my current implementation.  We made a conscious decision to implement the decision matrix that we designed in Recife first, faithfully and directly.  And then later, when we're confident that the tests represent the exact behaviour we want, we can start refactoring the code.

What I've been doing about these possible simplifications is document them in the design as optional steps.  I've documented this field as (B4+) which means we can choose to implement it as B4+ but it won't do anything.

> > +            },
> > +        }
> > +
> > +        incumbent_state = "incumbent_%s" % self._nameMessageStatus(
> > +            incumbent_message, traits)
> > +        twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
> > +
> > +        decisions = decision_matrix[incumbent_state][twin_state]
> > +        file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE
> > +        assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
> > +            "Bad decision string.")
> 
> Paranoia!

I removed the logging code.  I kept the regex assertion because it checks the <letter><digit><+*> structure as well as the actual contents of the string.  We can get rid of that when we leave the matrix structure behind.

> > +
> > +        for character in decisions:
> > +            if character == 'A':
> > +                # Deactivate & converge.
> > +                # There may be an identical shared message.
> > +                traits.setFlag(incumbent_message, False)
> > +                incumbent_message.shareIfPossible()
> > +            elif character == 'B':
> > +                # Deactivate.
> > +                traits.setFlag(incumbent_message, False)
> > +            elif character == 'Z':
> > +                # There is no incumbent message, so do nothing to it.
> > +                assert incumbent_message is None, (
> > +                    "Incorrect Z in decision matrix.")
> 
> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
> the open for an assert. Maybe you should trust your matrix more and just put a
> "pass" in here?

> > +            elif character == '1':
> > +                # Create & activate.
> > +                message = self._makeTranslationMessage(
> > +                    pofile, submitter, translations, origin)
> > +            elif character == '2':
> > +                # Create, diverge, activate.
> > +                message = self._makeTranslationMessage(
> > +                    pofile, submitter, translations, origin, diverged=True)
> > +            elif character == '4':
> > +                # Activate.
> > +                message = twin
> > +            elif character == '5':
> > +                # If other is a suggestion, diverge and activate.
> > +                # (If not, it's already active and has been unmasked by
> > +                # our deactivating the incumbent).
> > +                message = twin
> > +                if not traits.getFlag(twin):
> > +                    assert not traits.other_side.getFlag(twin), (
> > +                        "Decision matrix says '5' for a message that's "
> > +                        "active on the other side.")
> 
> Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the
> error message is quite cryptic to an outsider.
> 
> > +                    message.potemplate = pofile.potemplate
> > +            elif character == '6':
> > +                # If other is not active, fork a diverged message.
> > +                if traits.getFlag(twin):
> > +                    message = twin
> > +                else:
> > +                    # The twin is used on the other side, so we can't
> > +                    # just reuse it for our diverged message.  Create a
> > +                    # new one.
> > +                    message = self._makeTranslationMessage(
> > +                        pofile, submitter, translations, origin,
> > +                        diverged=True)
> > +            elif character == '7':
> > +                # Converge & activate.
> > +                message = twin
> > +                message.shareIfPossible()
> > +            elif character == '*':
> > +                if share_with_other_side:
> > +                    if traits.other_side.incumbent_message is None:
> > +                        traits.other_side.setFlag(message, True)
> > +            elif character == '+':
> > +                if share_with_other_side:
> > +                    traits.other_side.setFlag(message, True)
> > +            else:
> > +                raise AssertionError(
> > +                    "Bad character in decision string: %s" % character)
> > +
> > +        if decisions == '':
> > +            message = twin
> 
> This can go away if you use the '4' as mentioned earlier.
> 
> > +
> > +        traits.setFlag(message, True)
> > +
> > +        return message
> > +
> > +>>>>>>> MERGE-SOURCE
> >      def resetCurrentTranslation(self, pofile, lock_timestamp):
> >          """See `IPOTMsgSet`."""
> >
> > @@ -1145,7 +1423,7 @@
> >          # The credits message has a fixed "translator."
> >          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
> >
> > -        message = self.updateTranslation(
> > +        self.updateTranslation(
> >              pofile, translator, [credits_message_str],
> >              is_current_upstream=False, allow_credits=True,
> >              force_shared=True, force_edition_rights=True,
> > === modified file 'lib/lp/translations/tests/test_potmsgset.py'
> 
> The tests look very good although I am sure there could be even more.... ;-)
> 
> 
> Thanks for doing such a good job. I really like the approach to use the matrix
> explicitly in the code and also the traits are a great idea. Well done!
> 
> I will not approve this yet, obviously, because of the text conflicts but I
> have nothing else that needs serious fixing.
> 
>   review needs-fixing code
> 
> Cheers,
> Henning
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAkwaTMsACgkQBT3oW1L17ijACQCdFBkgqS4mFPcJnrYENrq3tqms
> 2FQAoLBZXCQ5CvV9L4mL21+G133GOe0x
> =v9bE
> -----END PGP SIGNATURE-----

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-06-19:

No idea how I managed to post my unfinished reply by accident just now, but I'll just continue where I left off.

> > + elif character == 'Z':
> > + # There is no incumbent message, so do nothing to it.
> > + assert incumbent_message is None, (
> > + "Incorrect Z in decision matrix.")
>
> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
> the open for an assert. Maybe you should trust your matrix more and just put a
> "pass" in here?

Again rather not in this phase, where we want to be sure we can tweak things without breaking them. Once we start refactoring, the Z probably becomes a no-op anyway.

Alright, I've made the message clearer.

> > +
> > + traits.setFlag(message, True)
> > +
> > + return message
> > +
> > +>>>>>>> MERGE-SOURCE
> > def resetCurrentTranslation(self, pofile, lock_timestamp):
> > """See `IPOTMsgSet`."""
> >
> > @@ -1145,7 +1423,7 @@
> > # The credits message has a fixed "translator."
> > translator = getUtility(ILaunchpadCelebrities).rosetta_experts
> >
> > - message = self.updateTranslation(
> > + self.updateTranslation(
> > pofile, translator, [credits_message_str],
> > is_current_upstream=False, allow_credits=True,
> > force_shared=True, force_edition_rights=True,
> > === modified file 'lib/lp/translations/tests/test_potmsgset.py'
>
> The tests look very good although I am sure there could be even more.... ;-)

I'm not sure what you mean. As you know (and as mentioned several times in the initial comments for this MP) there is a separate branch with exhaustive tests.

Jeroen

No idea how I managed to post my unfinished reply by accident just now, but I'll just continue where I left off.

> > +            elif character == 'Z':
> > +                # There is no incumbent message, so do nothing to it.
> > +                assert incumbent_message is None, (
> > +                    "Incorrect Z in decision matrix.")
> 
> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
> the open for an assert. Maybe you should trust your matrix more and just put a
> "pass" in here?

Again rather not in this phase, where we want to be sure we can tweak things without breaking them.  Once we start refactoring, the Z probably becomes a no-op anyway.

Alright, I've made the message clearer.

> > +
> > +        traits.setFlag(message, True)
> > +
> > +        return message
> > +
> > +>>>>>>> MERGE-SOURCE
> >      def resetCurrentTranslation(self, pofile, lock_timestamp):
> >          """See `IPOTMsgSet`."""
> >
> > @@ -1145,7 +1423,7 @@
> >          # The credits message has a fixed "translator."
> >          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
> >
> > -        message = self.updateTranslation(
> > +        self.updateTranslation(
> >              pofile, translator, [credits_message_str],
> >              is_current_upstream=False, allow_credits=True,
> >              force_shared=True, force_edition_rights=True,
> > === modified file 'lib/lp/translations/tests/test_potmsgset.py'
> 
> The tests look very good although I am sure there could be even more.... ;-)

I'm not sure what you mean.  As you know (and as mentioned several times in the initial comments for this MP) there is a separate branch with exhaustive tests.

Jeroen

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-21:

Download full text (9.6 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 19.06.2010 07:27, schrieb Jeroen T. Vermeulen:
[...]
>>> +=======
>>> + def _nameMessageStatus(self, message, translation_side_traits):
>>> + """Figure out the decision-matrix status of a message.
>>> +
>>> + This is used in navigating the decision matrix in
>>> + `setCurrentTranslation`.
>>> + """
>>> + if message is None:
>>> + return 'none'
>>> + elif message.potemplate is None:
>>> + if translation_side_traits.other_side.getFlag(message):
>>> + return 'other_shared'
>>> + else:
>>> + return 'shared'
>>> + else:
>>> + assert message.potemplate is not None, "Confused message
>> state."
>>
>> Your either forgot this in here or it's paranoia. Since the preceding elif
>> checks the exact same condition, this really is not needed.
>
> It's also partly caused by the "there must be an else" rule. I resolved this by:

Well, the usual way to fulfill that is "comment and pass". But the new 'if'
statement is much better anyway.

>
> 1. Adding an ITranslationMessage.is_diverged to allow for clearer checks.
> 2. Moving the "else" clause above the "elif" clause.
> 3. Flattening the "if" inside the "elif" clause to be part of the main if/elif/else.
>
> Looks much nicer now. We should have added is_diverged earlier.

Yes, definitely! Thanks for finally doing this.

>
>
>>> + def setCurrentTranslation(self, pofile, submitter, translations,
>> origin,
>>> + translation_side,
>> share_with_other_side=False):
>>> + """See `IPOTMsgSet`."""
>>> + traits = make_translation_side_message_traits(
>>> + translation_side, self, pofile.potemplate, pofile.language,
>>> + variant=pofile.variant)
>>> +
>>> + translations = self._findPOTranslations(translations)
>>> + incumbent_message = traits.incumbent_message
>>> + twin = self._findTranslationMessage(
>>> + pofile, translations, prefer_shared=False)
>>
>> Hm, I am not 100% sure what a twin is. Maybe you could explain that here?
>
> I thought the blueprint made that clear, but I've now documented it in the code.

Oh, I guess I was too slow. But "twin" just never appeared before. Thanks for
documenting it now.

>
>
>>> + # Summary of the matrix:
>>> + # * If the incumbent message is diverged and we're setting a
>>> + # translation that's already shared: converge.
>>> + # * If the incumbent message is diverged and we're setting a
>>> + # translation that's not already shared: maintain divergence.
>>> + # * If the incumbent message is shared, replace it.
>>> + # * If there is no twin, simply create a new message (shared or
>>> + # diverged depending; see above).
>>> + # * If there is a shared twin, activate it (but also diverge if
>>> + # necessary; see above).
>>> + # * If there is a diverged twin, activate it (and converge it
>>> + # if appropriate; see above).
>>> + # * If there is a twin that's shared on the other side,
>>> +
>...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 19.06.2010 07:27, schrieb Jeroen T. Vermeulen:
[...]
>>> +=======
>>> +    def _nameMessageStatus(self, message, translation_side_traits):
>>> +        """Figure out the decision-matrix status of a message.
>>> +
>>> +        This is used in navigating the decision matrix in
>>> +        `setCurrentTranslation`.
>>> +        """
>>> +        if message is None:
>>> +            return 'none'
>>> +        elif message.potemplate is None:
>>> +            if translation_side_traits.other_side.getFlag(message):
>>> +                return 'other_shared'
>>> +            else:
>>> +                return 'shared'
>>> +        else:
>>> +            assert message.potemplate is not None, "Confused message
>> state."
>>
>> Your either forgot this in here or it's paranoia. Since the preceding elif
>> checks the exact same condition, this really is not needed.
> 
> It's also partly caused by the "there must be an else" rule.  I resolved this by:

Well, the usual way to fulfill that is "comment and pass". But the new 'if'
statement is much better anyway.

> 
> 1. Adding an ITranslationMessage.is_diverged to allow for clearer checks.
> 2. Moving the "else" clause above the "elif" clause.
> 3. Flattening the "if" inside the "elif" clause to be part of the main if/elif/else.
> 
> Looks much nicer now.  We should have added is_diverged earlier.

Yes, definitely! Thanks for finally doing this.

> 
> 
>>> +    def setCurrentTranslation(self, pofile, submitter, translations,
>> origin,
>>> +                              translation_side,
>> share_with_other_side=False):
>>> +        """See `IPOTMsgSet`."""
>>> +        traits = make_translation_side_message_traits(
>>> +            translation_side, self, pofile.potemplate, pofile.language,
>>> +            variant=pofile.variant)
>>> +
>>> +        translations = self._findPOTranslations(translations)
>>> +        incumbent_message = traits.incumbent_message
>>> +        twin = self._findTranslationMessage(
>>> +            pofile, translations, prefer_shared=False)
>>
>> Hm, I am not 100% sure what a twin is. Maybe you could explain that here?
> 
> I thought the blueprint made that clear, but I've now documented it in the code.

Oh, I guess I was too slow. But "twin" just never appeared before. Thanks for
documenting it now.

> 
> 
>>> +        # Summary of the matrix:
>>> +        #  * If the incumbent message is diverged and we're setting a
>>> +        #    translation that's already shared: converge.
>>> +        #  * If the incumbent message is diverged and we're setting a
>>> +        #    translation that's not already shared: maintain divergence.
>>> +        #  * If the incumbent message is shared, replace it.
>>> +        #  * If there is no twin, simply create a new message (shared or
>>> +        #    diverged depending; see above).
>>> +        #  * If there is a shared twin, activate it (but also diverge if
>>> +        #    necessary; see above).
>>> +        #  * If there is a diverged twin, activate it (and converge it
>>> +        #    if appropriate; see above).
>>> +        #  * If there is a twin that's shared on the other side,
>>> +
>>> +        # XXX: Steal flag if policy permits and either:
>>
>> Do we still need to agree on this policy?
> 
> No, this comment was temporary and I removed it.

Good

> 
> 
>>> +        #  - there is no shared active message on the other side, or
>>> +        #  - we're returning a shared message.
>>> +
>>> +        decision_matrix = {
>>> +            'incumbent_none': {
>>> +                'twin_none': 'Z1*',
>>> +                'twin_shared': 'Z4*',
>>> +                'twin_diverged': 'Z7*',
>>> +                'twin_other_shared': 'Z4',
>>> +            },
>>> +            'incumbent_shared': {
>>> +                'twin_none': 'B1*',
>>> +                'twin_shared': 'B4*',
>>> +                'twin_diverged': 'B7*',
>>> +                'twin_other_shared': 'B4',
>>> +            },
>>> +            'incumbent_diverged': {
>>> +                'twin_none': 'A2',
>>> +                'twin_shared': 'A5',
>>> +                'twin_diverged': 'A4',
>>> +                'twin_other_shared': 'A6',
>>> +            },
>>> +            'incumbent_other_shared': {
>>> +                'twin_none': 'B1+',
>>> +                'twin_shared': 'B4+',
>>> +                'twin_diverged': 'B7+',
>>> +                'twin_other_shared': '',
>>
>> This last one should be '4' instead of special casing it at the end.
> 
> That's true, but it's not time for that yet.  In the design, this case means "the message we want to activate is already active and nothing needs to be done."  It happens that we could implement it in the same way as 4, but that is a detail of my current implementation.  We made a conscious decision to implement the decision matrix that we designed in Recife first, faithfully and directly.  And then later, when we're confident that the tests represent the exact behaviour we want, we can start refactoring the code.

OK, I understand that now.

> 
> What I've been doing about these possible simplifications is document them in the design as optional steps.  I've documented this field as (B4+) which means we can choose to implement it as B4+ but it won't do anything.
> 
> 
>>> +            },
>>> +        }
>>> +
>>> +        incumbent_state = "incumbent_%s" % self._nameMessageStatus(
>>> +            incumbent_message, traits)
>>> +        twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
>>> +
>>> +        decisions = decision_matrix[incumbent_state][twin_state]
>>> +        file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE
>>> +        assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
>>> +            "Bad decision string.")
>>
>> Paranoia!
> 
> I removed the logging code.  I kept the regex assertion because it checks the <letter><digit><+*> structure as well as the actual contents of the string.  We can get rid of that when we leave the matrix structure behind.

As you wish. ;)

> 
> 
>>> +
>>> +        for character in decisions:
>>> +            if character == 'A':
>>> +                # Deactivate & converge.
>>> +                # There may be an identical shared message.
>>> +                traits.setFlag(incumbent_message, False)
>>> +                incumbent_message.shareIfPossible()
>>> +            elif character == 'B':
>>> +                # Deactivate.
>>> +                traits.setFlag(incumbent_message, False)
>>> +            elif character == 'Z':
>>> +                # There is no incumbent message, so do nothing to it.
>>> +                assert incumbent_message is None, (
>>> +                    "Incorrect Z in decision matrix.")
>>
>> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
>> the open for an assert. Maybe you should trust your matrix more and just put a
>> "pass" in here?
>
> Again rather not in this phase, where we want to be sure we can tweak things
without breaking them.  Once we start refactoring, the Z probably becomes a
no-op anyway.

Yes, I understand now that this is a multiple-phase project.

>
>
>>> +            elif character == '1':
>>> +                # Create & activate.
>>> +                message = self._makeTranslationMessage(
>>> +                    pofile, submitter, translations, origin)
>>> +            elif character == '2':
>>> +                # Create, diverge, activate.
>>> +                message = self._makeTranslationMessage(
>>> +                    pofile, submitter, translations, origin, diverged=True)
>>> +            elif character == '4':
>>> +                # Activate.
>>> +                message = twin
>>> +            elif character == '5':
>>> +                # If other is a suggestion, diverge and activate.
>>> +                # (If not, it's already active and has been unmasked by
>>> +                # our deactivating the incumbent).
>>> +                message = twin
>>> +                if not traits.getFlag(twin):
>>> +                    assert not traits.other_side.getFlag(twin), (
>>> +                        "Decision matrix says '5' for a message that's "
>>> +                        "active on the other side.")
>>
>> Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the
>> error message is quite cryptic to an outsider.
>
> Alright, I've made the message clearer.

I don't see that in the current diff.

>
>
>>> +
>>> +        traits.setFlag(message, True)
>>> +
>>> +        return message
>>> +
>>> +>>>>>>> MERGE-SOURCE
>>>      def resetCurrentTranslation(self, pofile, lock_timestamp):
>>>          """See `IPOTMsgSet`."""
>>>
>>> @@ -1145,7 +1423,7 @@
>>>          # The credits message has a fixed "translator."
>>>          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
>>>
>>> -        message = self.updateTranslation(
>>> +        self.updateTranslation(
>>>              pofile, translator, [credits_message_str],
>>>              is_current_upstream=False, allow_credits=True,
>>>              force_shared=True, force_edition_rights=True,
>>> === modified file 'lib/lp/translations/tests/test_potmsgset.py'
>>
>> The tests look very good although I am sure there could be even more.... ;-)
>
> I'm not sure what you mean.  As you know (and as mentioned several times in
the initial comments for this MP) there is a separate branch with exhaustive
tests.

Yes, I do know. ;-) Never mind me trying to be funny ...

review approve code

Cheers,
Henning
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwfQXYACgkQBT3oW1L17ih8wgCgsJG4KDNT99dnZfMtrcMMWn3M
slgAn277jfZvS8SynoxQmJWeCPwl0rkP
=0oh7
-----END PGP SIGNATURE-----

review: Approve (code)

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-21:

Here one comment, I forgot.

> === modified file 'lib/lp/translations/tests/test_translationmessage.py'
> --- lib/lp/translations/tests/test_translationmessage.py 2009-08-25 20:15:38 +0000
> +++ lib/lp/translations/tests/test_translationmessage.py 2010-06-19 03:38:37 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Unit tests for `TranslationMessage`."""
> @@ -10,12 +10,46 @@
> from zope.component import getUtility
> from zope.security.proxy import removeSecurityProxy
>
> +from canonical.launchpad.webapp.testing import verifyObject
> from lp.services.worlddata.interfaces.language import ILanguageSet
> from lp.testing import TestCaseWithFactory
> from lp.translations.model.potranslation import POTranslation
> +from lp.translations.model.translationmessage import DummyTranslationMessage
> +from lp.translations.interfaces.translationmessage import (
> + ITranslationMessage)
> from lp.translations.interfaces.translations import TranslationConstants
> from canonical.testing import ZopelessDatabaseLayer
>
> +
> +class TestTranslationMessage(TestCaseWithFactory):
> + """Unit tests for `TranslationMessage`.
> +
> + There aren't many of these. We didn't do much unit testing back
> + then.
> + """

And thanks a lot for adding these! ;-)

> +
> + layer = ZopelessDatabaseLayer
> +
> + def test_baseline(self):
> + message = self.factory.makeTranslationMessage()
> + verifyObject(ITranslationMessage, message)

I'd have split this test here.

> +
> + pofile = self.factory.makePOFile('nl')
> + potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
> + dummy = DummyTranslationMessage(pofile, potmsgset)
> + verifyObject(ITranslationMessage, dummy)
> +
> + def test_is_diverged(self):
> + # ITranslationMessage.is_diverged is a little helper to let you
> + # say "message.is_diverged" which can be clearer than
> + # "message.potemplate is not None."
> + message = self.factory.makeTranslationMessage(force_diverged=False)
> + self.assertFalse(message.is_diverged)
> +

And this one here.

> + message = self.factory.makeTranslationMessage(force_diverged=True)
> + self.assertTrue(message.is_diverged)

But that is just how I would do that. Makes test failures easier to spot.
Also, unrelated parts of a test will not be run if one fails. Maybe that
second reasoning is even stronger.

> +
> +
> class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
> """Tests for `TranslationMessage.findIdenticalMessage`."""
>

Cheers, Henning

Here one comment, I forgot.

> === modified file 'lib/lp/translations/tests/test_translationmessage.py'
> --- lib/lp/translations/tests/test_translationmessage.py	2009-08-25 20:15:38 +0000
> +++ lib/lp/translations/tests/test_translationmessage.py	2010-06-19 03:38:37 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Unit tests for `TranslationMessage`."""
> @@ -10,12 +10,46 @@
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> +from canonical.launchpad.webapp.testing import verifyObject
>  from lp.services.worlddata.interfaces.language import ILanguageSet
>  from lp.testing import TestCaseWithFactory
>  from lp.translations.model.potranslation import POTranslation
> +from lp.translations.model.translationmessage import DummyTranslationMessage
> +from lp.translations.interfaces.translationmessage import (
> +    ITranslationMessage)
>  from lp.translations.interfaces.translations import TranslationConstants
>  from canonical.testing import ZopelessDatabaseLayer
>  
> +
> +class TestTranslationMessage(TestCaseWithFactory):
> +    """Unit tests for `TranslationMessage`.
> +
> +    There aren't many of these.  We didn't do much unit testing back
> +    then.
> +    """

And thanks a lot for adding these! ;-)

> +
> +    layer = ZopelessDatabaseLayer
> +
> +    def test_baseline(self):
> +        message = self.factory.makeTranslationMessage()
> +        verifyObject(ITranslationMessage, message)

I'd have split this test here.

> +
> +        pofile = self.factory.makePOFile('nl')
> +        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
> +        dummy = DummyTranslationMessage(pofile, potmsgset)
> +        verifyObject(ITranslationMessage, dummy)
> +
> +    def test_is_diverged(self):
> +        # ITranslationMessage.is_diverged is a little helper to let you
> +        # say "message.is_diverged" which can be clearer than
> +        # "message.potemplate is not None."
> +        message = self.factory.makeTranslationMessage(force_diverged=False)
> +        self.assertFalse(message.is_diverged)
> +

And this one here.

> +        message = self.factory.makeTranslationMessage(force_diverged=True)
> +        self.assertTrue(message.is_diverged)

But that is just how I would do that. Makes test failures easier to spot.
Also, unrelated parts of a test will not be run if one fails. Maybe that
second reasoning is even stronger.

> +
> +
>  class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
>      """Tests for `TranslationMessage.findIdenticalMessage`."""
>

Cheers, Henning

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-06-21:

Hi Henning,

Thanks for the review. You were right: my change to that assertion text was lost somehow. Good thing you spotted that. Hope it's fixed this time.

> > === modified file 'lib/lp/translations/tests/test_translationmessage.py'
> > --- lib/lp/translations/tests/test_translationmessage.py 2009-08-25
> 20:15:38 +0000
> > +++ lib/lp/translations/tests/test_translationmessage.py 2010-06-19
> 03:38:37 +0000

> > """Unit tests for `TranslationMessage`."""
> > + def test_baseline(self):
> > + message = self.factory.makeTranslationMessage()
> > + verifyObject(ITranslationMessage, message)
>
> I'd have split this test here.

Good idea. I've been letting them grow longer so they'd waste less time on setup and teardown, sort of like our old doctests, but it's not always a good thing.

> > + def test_is_diverged(self):
> > + # ITranslationMessage.is_diverged is a little helper to let you
> > + # say "message.is_diverged" which can be clearer than
> > + # "message.potemplate is not None."
> > + message = self.factory.makeTranslationMessage(force_diverged=False)
> > + self.assertFalse(message.is_diverged)
> > +
>
> And this one here.

Both split now.

Jeroen

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Jeroen T. Vermeulen

Данило Шеган

 === modified file 'database/schema/security.cfg'
 --- database/schema/security.cfg	2010-05-20 13:59:11 +0000
 +++ database/schema/security.cfg	2010-06-21 16:42:29 +0000
@@ -258,7 +258,7 @@
  public.temporaryblobstorage             = SELECT, INSERT, DELETE
  public.translationgroup                 = SELECT, INSERT, UPDATE
  public.translationimportqueueentry      = SELECT, INSERT, UPDATE, DELETE
--public.translationmessage               = SELECT, INSERT, UPDATE
++public.translationmessage               = SELECT, INSERT, UPDATE, DELETE
  public.translator                       = SELECT, INSERT, UPDATE, DELETE
  public.usertouseremail                  = SELECT, UPDATE
  public.validpersoncache                 = SELECT
 === modified file 'lib/lp/translations/interfaces/potmsgset.py'
 --- lib/lp/translations/interfaces/potmsgset.py	2010-06-09 14:17:32 +0000
 +++ lib/lp/translations/interfaces/potmsgset.py	2010-06-21 16:42:29 +0000
@@ -214,6 +214,10 @@
                            force_edition_rights=False, allow_credits=False):
          """Update or create a translation message using `new_translations`.
++        This method is Launchpad Translations's sliderule: it does
++        everything, nobody fully understands it all, and we intend to
++        replace it with a range of less flexible tools.
++
          :param pofile: a `POFile` to add `new_translations` to.
          :param submitter: author of the translations.
          :param new_translations: a dictionary of plural forms, with the
@@ -261,6 +265,23 @@
          If a translation conflict is detected, TranslationConflict is raised.
          """
++    def setCurrentTranslation(pofile, submitter, translations, origin,
++                              translation_side, share_with_other_side=False):
++        """Set the message's translation in Ubuntu, or upstream, or both.
++
++        :param pofile: `POFile` you're setting translations in.  Other
++            `POFiles` that share translations with this one may also be
++            affected.
++        :param submitter: `Person` who is setting these translations.
++        :param translations: a dict mapping plural-form numbers to the
++            translated string for that form.
++        :param origin: A `RosettaTranslationOrigin`.
++        :param translation_side: The `TranslationSide` that this
++            translation is for (Ubuntu or upstream).
++        :param share_with_other_side: When sharing this translation,
++            share it with the other `TranslationSide` as well.
++        """
++
      def resetCurrentTranslation(pofile, lock_timestamp):
          """Reset the currently used translation.
 === modified file 'lib/lp/translations/interfaces/translationmessage.py'
 --- lib/lp/translations/interfaces/translationmessage.py	2010-03-26 19:49:46 +0000
 +++ lib/lp/translations/interfaces/translationmessage.py	2010-06-21 16:42:29 +0000
@@ -209,6 +209,9 @@
          title=_("Number of plural form translations in this translation."),
          readonly=True, required=True)
++    is_diverged = Bool(
++        title=_("Is this message diverged?"), readonly=True, required=True)
++
      def setPOFile(pofile):
          """Set a POFile for use in views."""
 === modified file 'lib/lp/translations/interfaces/translations.py'
 --- lib/lp/translations/interfaces/translations.py	2009-07-17 00:26:05 +0000
 +++ lib/lp/translations/interfaces/translations.py	2010-06-21 16:42:29 +0000
@@ -1,4 +1,4 @@
--# Copyright 2009 Canonical Ltd.  This software is licensed under the
++# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
  # GNU Affero General Public License version 3 (see the file LICENSE).
  # pylint: disable-msg=E0211,E0213
@@ -7,10 +7,12 @@
  __metaclass__ = type
--__all__ = (
++__all__ = [
      'TranslationConstants',
      'TranslationsBranchImportMode',
--    )
++    'TranslationSide',
++    ]
++
  class TranslationConstants:
      """Set of constants used inside the context of translations."""
@@ -52,3 +54,10 @@
          """)
++class TranslationSide:
++    """The two "sides" of software that can be translated in Launchpad.
++
++    These are "upstream" and "Ubuntu."
++    """
++    UPSTREAM = 1
++    UBUNTU = 2
 === modified file 'lib/lp/translations/model/potmsgset.py'
 --- lib/lp/translations/model/potmsgset.py	2010-06-10 11:24:51 +0000
 +++ lib/lp/translations/model/potmsgset.py	2010-06-21 16:42:29 +0000
@@ -4,11 +4,16 @@
  # pylint: disable-msg=E0611,W0212
  __metaclass__ = type
--__all__ = ['POTMsgSet']
++__all__ = [
++    'make_translation_side_message_traits',
++    'POTMsgSet',
++    ]
++
  import datetime
  import logging
  import pytz
++import re
  from zope.interface import implements
  from zope.component import getUtility
@@ -41,7 +46,8 @@
      RosettaTranslationOrigin,
      TranslationConflict,
      TranslationValidationStatus)
--from lp.translations.interfaces.translations import TranslationConstants
++from lp.translations.interfaces.translations import (
++    TranslationConstants, TranslationSide)
  from lp.translations.model.pomsgid import POMsgID
  from lp.translations.model.potranslation import POTranslation
  from lp.translations.model.translationmessage import (
@@ -79,6 +85,100 @@
                         u'credits are counted as translated.')
++class TranslationSideMessageTraits:
++    """Dealing with a `POTMsgSet` on either `TranslationSide`.
++
++    Encapsulates primitives that depend on translation side: finding the
++    message that is current on the given side, checking the flag that
++    says whether a message is current on this side, setting or clearing
++    the flag, and providing the same capabilities for the other side.
++
++    For an introduction to the Traits pattern, see
++    http://www.cantrip.org/traits.html
++    """
++    # The TranslationSide that these Traits are for.
++    side = None
++
++    # TranslationSideMessageTraits for this message on the "other side."
++    other_side = None
++
++    # Name of this side's flag.
++    flag_name = None
++
++    def __init__(self, potmsgset, potemplate=None, language=None,
++                 variant=None):
++        self.potmsgset = potmsgset
++        self.potemplate = potemplate
++        self.language = language
++        self.variant = variant
++
++        self._found_incumbent = False
++
++    @property
++    def incumbent_message(self):
++        """Message that currently has the flag."""
++        if not self._found_incumbent:
++            self._incumbent = self._getIncumbentMessage()
++            self._found_incumbent = True
++        return self._incumbent
++
++    def getFlag(self, translationmessage):
++        """Is this message the current one on this side?"""
++        return getattr(translationmessage, self.flag_name)
++
++    def setFlag(self, translationmessage, value):
++        """Set or clear a message's "current" flag for this side."""
++        if value == self.getFlag(translationmessage):
++            return
++
++        if value and self.incumbent_message is not None:
++            self.setFlag(self.incumbent_message, False)
++
++        setattr(translationmessage, self.flag_name, value)
++        self._found_incumbent = False
++
++    def _getIncumbentMessage(self):
++        """Get the message that is current on this side, if any."""
++        raise NotImplementedError('_getIncumbentMessage')
++
++
++class UpstreamSideTraits(TranslationSideMessageTraits):
++    """Traits for upstream translations."""
++
++    side = TranslationSide.UPSTREAM
++
++    flag_name = 'is_current_upstream'
++
++    def _getIncumbentMessage(self):
++        """See `TranslationSideMessageTraits`."""
++        return self.potmsgset.getImportedTranslationMessage(
++            self.potemplate, self.language, variant=self.variant)
++
++
++class UbuntuSideTraits(TranslationSideMessageTraits):
++    """Traits for Ubuntu translations."""
++
++    side = TranslationSide.UBUNTU
++
++    flag_name = 'is_current_ubuntu'
++
++    def _getIncumbentMessage(self):
++        """See `TranslationSideMessageTraits`."""
++        return self.potmsgset.getCurrentTranslationMessage(
++            self.potemplate, self.language, variant=self.variant)
++
++
++def make_translation_side_message_traits(side, potmsgset, potemplate,
++                                         language, variant=None):
++    """Create `TranslationSideTraits` object of the appropriate subtype."""
++    ubuntu = UbuntuSideTraits(potmsgset, potemplate, language, variant)
++    upstream = UpstreamSideTraits(potmsgset, potemplate, language, variant)
++    upstream.other_side = ubuntu
++    ubuntu.other_side = upstream
++    mapping = dict((traits.side, traits) for traits in (ubuntu, upstream))
++    return mapping[side]
++
++
  class POTMsgSet(SQLBase):
      implements(IPOTMsgSet)
@@ -232,13 +332,13 @@
                                     current=True):
          """Get a translation message which is either used in
          Launchpad (current=True) or in an import (current=False).
--
++
          Prefers a diverged message if present.
          """
--        # Change the is_current_ubuntu and is_current_upstream conditions
--        # with extreme care: they need to match condition specified in
--        # indexes, or postgres may not pick them up (in complicated queries,
--        # postgres query optimizer sometimes does text-matching of indexes).
++        # Change 'is_current IS TRUE' and 'is_imported IS TRUE' conditions
++        # carefully: they need to match condition specified in indexes,
++        # or Postgres may not pick them up (in complicated queries,
++        # Postgres query optimizer sometimes does text-matching of indexes).
          if current:
              used_clause = 'is_current_ubuntu IS TRUE'
          else:
@@ -522,13 +622,17 @@
                  potranslations[pluralform] = None
          return potranslations
--    def _findTranslationMessage(self, pofile, potranslations, pluralforms):
--        """Find a message for this `pofile`.
--
--        The returned message matches exactly the given `translations` strings
--        comparing only `pluralforms` of them.
++    def _findTranslationMessage(self, pofile, potranslations,
++                                prefer_shared=True):
++        """Find a matching message in this `pofile`.
++
++        The returned message matches exactly the given `translations`
++        strings (except plural forms not supported by `pofile`, which
++        are ignored).
++
          :param potranslations: A list of translation strings.
--        :param pluralforms: The number of pluralforms to compare.
++        :param prefer_shared: Whether to prefer a shared match over a
++            diverged one.
          """
          clauses = ['potmsgset = %s' % sqlvalues(self),
                     'language = %s' % sqlvalues(pofile.language),
@@ -539,7 +643,7 @@
          else:
              clauses.append('variant = %s' % sqlvalues(pofile.variant))
--        for pluralform in range(pluralforms):
++        for pluralform in range(pofile.plural_forms):
              if potranslations[pluralform] is None:
                  clauses.append('msgstr%s IS NULL' % sqlvalues(pluralform))
              else:
@@ -547,10 +651,15 @@
                      sqlvalues(pluralform, potranslations[pluralform])))
          remaining_plural_forms = range(
--            pluralforms, TranslationConstants.MAX_PLURAL_FORMS)
--
--        # Prefer shared messages over diverged ones.
--        order = ['potemplate NULLS FIRST']
++            pofile.plural_forms, TranslationConstants.MAX_PLURAL_FORMS)
++
++        # Prefer either shared or diverged messages, depending on
++        # arguments.
++        if prefer_shared:
++            order = ['potemplate NULLS FIRST']
++        else:
++            order = ['potemplate NULLS LAST']
++
          # Normally at most one message should match.  But if there is
          # more than one, prefer the one that adds the fewest extraneous
          # plural forms.
@@ -564,9 +673,9 @@
          if len(matches) > 0:
              if len(matches) > 1:
                  logging.warn(
--                    "Translation for POTMsgSet %s into %s "
--                    "matches %s existing translations." % sqlvalues(
--                        self, pofile.language.code, len(matches)))
++                    "Translation for POTMsgSet %s into '%s' "
++                    "matches %s existing translations.",
++                        self.id, pofile.language.code, len(matches))
              return matches[0]
          else:
              return None
@@ -799,7 +908,7 @@
          # of translations.  None if there is no such message and needs to be
          # created.
          matching_message = self._findTranslationMessage(
--            pofile, potranslations, pofile.plural_forms)
++            pofile, potranslations)
          match_is_upstream = (
              matching_message is not None and
@@ -897,7 +1006,7 @@
          potranslations = self._findPOTranslations(new_translations)
          existing_message = self._findTranslationMessage(
--            pofile, potranslations, pofile.plural_forms)
++            pofile, potranslations)
          if existing_message is not None:
              return existing_message
@@ -951,9 +1060,184 @@
              current.reviewer = reviewer
              current.date_reviewed = lock_timestamp
++    def _nameMessageStatus(self, message, translation_side_traits):
++        """Figure out the decision-matrix status of a message.
++
++        This is used in navigating the decision matrix in
++        `setCurrentTranslation`.
++        """
++        if message is None:
++            return 'none'
++        elif message.is_diverged:
++            return 'diverged'
++        elif translation_side_traits.other_side.getFlag(message):
++            return 'other_shared'
++        else:
++            return 'shared'
++
++    def _makeTranslationMessage(self, pofile, submitter, translations, origin,
++                                diverged=False):
++        """Create a new `TranslationMessage`.
++
++        The message will not be made current on either side (Ubuntu or
++        upstream), but it can be diverged.  Only messages that are
++        current should be diverged, but it's up to the caller to ensure
++        the right state.
++        """
++        if diverged:
++            potemplate = pofile.potemplate
++        else:
++            potemplate = None
++
++        translation_args = dict(
++            ('msgstr%d' % form, translation)
++            for form, translation in translations.iteritems()
++            )
++
++        return TranslationMessage(
++            potmsgset=self,
++            potemplate=potemplate,
++            pofile=pofile,
++            language=pofile.language,
++            variant=pofile.variant,
++            origin=origin,
++            submitter=submitter,
++            validation_status=TranslationValidationStatus.OK,
++            **translation_args)
++
      def setCurrentTranslation(self, pofile, submitter, translations, origin,
                                translation_side, share_with_other_side=False):
          """See `IPOTMsgSet`."""
++        traits = make_translation_side_message_traits(
++            translation_side, self, pofile.potemplate, pofile.language,
++            variant=pofile.variant)
++
++        translations = self._findPOTranslations(translations)
++
++        # The current message on this translation side, if any.
++        incumbent_message = traits.incumbent_message
++
++        # An already existing message, if any, that's either shared, or
++        # diverged for the template/pofile we're working on, whose
++        # translations are identical to the ones we're setting.
++        twin = self._findTranslationMessage(
++            pofile, translations, prefer_shared=False)
++
++        # Summary of the matrix:
++        #  * If the incumbent message is diverged and we're setting a
++        #    translation that's already shared: converge.
++        #  * If the incumbent message is diverged and we're setting a
++        #    translation that's not already shared: maintain divergence.
++        #  * If the incumbent message is shared, replace it.
++        #  * If there is no twin, simply create a new message (shared or
++        #    diverged depending; see above).
++        #  * If there is a shared twin, activate it (but also diverge if
++        #    necessary; see above).
++        #  * If there is a diverged twin, activate it (and converge it
++        #    if appropriate; see above).
++        #  * If there is a twin that's shared on the other side,
++
++        decision_matrix = {
++            'incumbent_none': {
++                'twin_none': 'Z1*',
++                'twin_shared': 'Z4*',
++                'twin_diverged': 'Z7*',
++                'twin_other_shared': 'Z4',
++            },
++            'incumbent_shared': {
++                'twin_none': 'B1*',
++                'twin_shared': 'B4*',
++                'twin_diverged': 'B7*',
++                'twin_other_shared': 'B4',
++            },
++            'incumbent_diverged': {
++                'twin_none': 'A2',
++                'twin_shared': 'A5',
++                'twin_diverged': 'A4',
++                'twin_other_shared': 'A6',
++            },
++            'incumbent_other_shared': {
++                'twin_none': 'B1+',
++                'twin_shared': 'B4+',
++                'twin_diverged': 'B7+',
++                'twin_other_shared': '',
++            },
++        }
++
++        incumbent_state = "incumbent_%s" % self._nameMessageStatus(
++            incumbent_message, traits)
++        twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
++
++        decisions = decision_matrix[incumbent_state][twin_state]
++        assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
++            "Bad decision string.")
++
++        for character in decisions:
++            if character == 'A':
++                # Deactivate & converge.
++                # There may be an identical shared message.
++                traits.setFlag(incumbent_message, False)
++                incumbent_message.shareIfPossible()
++            elif character == 'B':
++                # Deactivate.
++                traits.setFlag(incumbent_message, False)
++            elif character == 'Z':
++                # There is no incumbent message, so do nothing to it.
++                assert incumbent_message is None, (
++                    "Incorrect Z in decision matrix.")
++            elif character == '1':
++                # Create & activate.
++                message = self._makeTranslationMessage(
++                    pofile, submitter, translations, origin)
++            elif character == '2':
++                # Create, diverge, activate.
++                message = self._makeTranslationMessage(
++                    pofile, submitter, translations, origin, diverged=True)
++            elif character == '4':
++                # Activate.
++                message = twin
++            elif character == '5':
++                # If other is a suggestion, diverge and activate.
++                # (If not, it's already active and has been unmasked by
++                # our deactivating the incumbent).
++                message = twin
++                if not traits.getFlag(twin):
++                    assert not traits.other_side.getFlag(twin), (
++                        "Trying to diverge a message that is current on the "
++                        "other side.")
++                    message.potemplate = pofile.potemplate
++            elif character == '6':
++                # If other is not active, fork a diverged message.
++                if traits.getFlag(twin):
++                    message = twin
++                else:
++                    # The twin is used on the other side, so we can't
++                    # just reuse it for our diverged message.  Create a
++                    # new one.
++                    message = self._makeTranslationMessage(
++                        pofile, submitter, translations, origin,
++                        diverged=True)
++            elif character == '7':
++                # Converge & activate.
++                message = twin
++                message.shareIfPossible()
++            elif character == '*':
++                if share_with_other_side:
++                    if traits.other_side.incumbent_message is None:
++                        traits.other_side.setFlag(message, True)
++            elif character == '+':
++                if share_with_other_side:
++                    traits.other_side.setFlag(message, True)
++            else:
++                raise AssertionError(
++                    "Bad character in decision string: %s" % character)
++
++        if decisions == '':
++            message = twin
++
++        traits.setFlag(message, True)
++
++        return message
      def resetCurrentTranslation(self, pofile, lock_timestamp):
          """See `IPOTMsgSet`."""
@@ -1145,7 +1429,7 @@
          # The credits message has a fixed "translator."
          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
--        message = self.updateTranslation(
++        self.updateTranslation(
              pofile, translator, [credits_message_str],
              is_current_upstream=False, allow_credits=True,
              force_shared=True, force_edition_rights=True,
 === modified file 'lib/lp/translations/model/translationmessage.py'
 --- lib/lp/translations/model/translationmessage.py	2010-05-07 12:29:23 +0000
 +++ lib/lp/translations/model/translationmessage.py	2010-06-21 16:42:29 +0000
@@ -78,6 +78,11 @@
                  forms = 2
              return forms
++    @property
++    def is_diverged(self):
++        """See `ITranslationMessage`."""
++        return self.potemplate is not None
++
      def makeHTMLID(self, suffix=None):
          """See `ITranslationMessage`."""
          elements = [self.language.code]
 === modified file 'lib/lp/translations/tests/test_potmsgset.py'
 --- lib/lp/translations/tests/test_potmsgset.py	2010-06-09 14:17:32 +0000
 +++ lib/lp/translations/tests/test_potmsgset.py	2010-06-21 16:42:29 +0000
@@ -25,6 +25,9 @@
      TranslationFileFormat)
  from lp.translations.interfaces.translationmessage import (
      RosettaTranslationOrigin, TranslationConflict)
++from lp.translations.interfaces.translations import TranslationSide
++from lp.translations.model.potmsgset import (
++    make_translation_side_message_traits)
  from lp.translations.model.translationmessage import (
      DummyTranslationMessage)
@@ -1305,7 +1308,7 @@
      def test_creation_pofile(self):
          # When a new pofile is created, dummy translations are created for
          # all translation credits messages.
--
++
          credits = self.factory.makePOTMsgSet(
              self.potemplate, u'translator-credits', sequence=1)
          eo_pofile = self.factory.makePOFile('eo', potemplate=self.potemplate)
@@ -1572,5 +1575,123 @@
          self.assertEqual([], karma_listener.karma_events)
++class TestSetCurrentTranslation(TestCaseWithFactory):
++    layer = ZopelessDatabaseLayer
++
++    def _makePOFileAndPOTMsgSet(self):
++        pofile = self.factory.makePOFile('nl')
++        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
++        return pofile, potmsgset
++
++    def _makeTranslations(self, potmsgset, forms=1):
++        return removeSecurityProxy(potmsgset)._findPOTranslations([
++            self.factory.getUniqueString()
++            for counter in xrange(forms)
++            ])
++
++    def test_baseline(self):
++        # setCurrentTranslation sets the current upstream translation
++        # for a message.
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        translations = self._makeTranslations(potmsgset)
++        origin = RosettaTranslationOrigin.SCM
++
++        message = potmsgset.setCurrentTranslation(
++            pofile, pofile.potemplate.owner, translations, origin,
++            TranslationSide.UPSTREAM)
++
++        self.assertEqual(message, potmsgset.getImportedTranslationMessage(
++            pofile.potemplate, pofile.language))
++        self.assertEqual(origin, message.origin)
++
++    def test_make_translation_side_message_traits(self):
++        # make_translation_side_message_traits is a factory for traits
++        # objects that help setCurrentTranslations deal with the
++        # dichotomy between upstream and Ubuntu translations.
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        sides = (TranslationSide.UPSTREAM, TranslationSide.UBUNTU)
++        for side in sides:
++            traits = make_translation_side_message_traits(
++                side, potmsgset, pofile.potemplate, pofile.language)
++            self.assertEqual(side, traits.side)
++            self.assertNotEqual(side, traits.other_side.side)
++            self.assertIn(traits.other_side.side, sides)
++            self.assertIs(traits, traits.other_side.other_side)
++
++    def test_UpstreamSideTraits_upstream(self):
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        message = self.factory.makeTranslationMessage(
++            pofile=pofile, potmsgset=potmsgset)
++
++        traits = make_translation_side_message_traits(
++            TranslationSide.UPSTREAM, potmsgset, pofile.potemplate,
++            pofile.language)
++
++        self.assertEqual('is_current_upstream', traits.flag_name)
++
++        self.assertFalse(traits.getFlag(message))
++        self.assertFalse(message.is_current_upstream)
++        self.assertEquals(None, traits.incumbent_message)
++
++        traits.setFlag(message, True)
++
++        self.assertTrue(traits.getFlag(message))
++        self.assertTrue(message.is_current_upstream)
++        self.assertEquals(message, traits.incumbent_message)
++
++    def test_UpstreamSideTraits_ubuntu(self):
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        message = self.factory.makeTranslationMessage(
++            pofile=pofile, potmsgset=potmsgset)
++        message.is_current_ubuntu = False
++
++        traits = make_translation_side_message_traits(
++            TranslationSide.UBUNTU, potmsgset, pofile.potemplate,
++            pofile.language)
++
++        self.assertEqual('is_current_ubuntu', traits.flag_name)
++
++        self.assertFalse(traits.getFlag(message))
++        self.assertFalse(message.is_current_ubuntu)
++        self.assertEquals(None, traits.incumbent_message)
++
++        traits.setFlag(message, True)
++
++        self.assertTrue(traits.getFlag(message))
++        self.assertTrue(message.is_current_ubuntu)
++        self.assertEquals(message, traits.incumbent_message)
++
++    def test_identical(self):
++        # Setting the same message twice leaves the original as-is.
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        translations = self._makeTranslations(potmsgset)
++
++        first_message = potmsgset.setCurrentTranslation(
++            pofile, pofile.potemplate.owner, translations,
++            RosettaTranslationOrigin.ROSETTAWEB, TranslationSide.UPSTREAM)
++        second_message = potmsgset.setCurrentTranslation(
++            pofile, self.factory.makePerson(), translations,
++            RosettaTranslationOrigin.SCM, TranslationSide.UPSTREAM)
++
++        self.assertEqual(first_message, second_message)
++        message = first_message
++        self.assertEqual(pofile.potemplate.owner, message.submitter)
++        self.assertEqual(RosettaTranslationOrigin.ROSETTAWEB, message.origin)
++
++    def test_upstream_also_sets_initial_ubuntu(self):
++        # Setting an upstream translation also initializes the Ubuntu
++        # translation.
++        pofile, potmsgset = self._makePOFileAndPOTMsgSet()
++        translations = self._makeTranslations(potmsgset)
++        origin = RosettaTranslationOrigin.ROSETTAWEB
++
++        message = potmsgset.setCurrentTranslation(
++            pofile, pofile.potemplate.owner, translations, origin,
++            TranslationSide.UPSTREAM)
++
++        self.assertEqual(message, potmsgset.getImportedTranslationMessage(
++            pofile.potemplate, pofile.language))
++
++
  def test_suite():
      return unittest.TestLoader().loadTestsFromName(__name__)
 === modified file 'lib/lp/translations/tests/test_translationmessage.py'
 --- lib/lp/translations/tests/test_translationmessage.py	2009-08-25 20:15:38 +0000
 +++ lib/lp/translations/tests/test_translationmessage.py	2010-06-21 16:42:29 +0000
@@ -1,4 +1,4 @@
--# Copyright 2009 Canonical Ltd.  This software is licensed under the
++# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
  # GNU Affero General Public License version 3 (see the file LICENSE).
  """Unit tests for `TranslationMessage`."""
@@ -10,12 +10,48 @@
  from zope.component import getUtility
  from zope.security.proxy import removeSecurityProxy
++from canonical.launchpad.webapp.testing import verifyObject
  from lp.services.worlddata.interfaces.language import ILanguageSet
  from lp.testing import TestCaseWithFactory
  from lp.translations.model.potranslation import POTranslation
++from lp.translations.model.translationmessage import DummyTranslationMessage
++from lp.translations.interfaces.translationmessage import (
++    ITranslationMessage)
  from lp.translations.interfaces.translations import TranslationConstants
  from canonical.testing import ZopelessDatabaseLayer
++
++class TestTranslationMessage(TestCaseWithFactory):
++    """Unit tests for `TranslationMessage`.
++
++    There aren't many of these.  We didn't do much unit testing back
++    then.
++    """
++
++    layer = ZopelessDatabaseLayer
++
++    def test_baseline(self):
++        message = self.factory.makeTranslationMessage()
++        verifyObject(ITranslationMessage, message)
++
++    def test_dummy_translationmessage(self):
++        pofile = self.factory.makePOFile('nl')
++        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
++        dummy = DummyTranslationMessage(pofile, potmsgset)
++        verifyObject(ITranslationMessage, dummy)
++
++    def test_is_diverged_false(self):
++        # ITranslationMessage.is_diverged is a little helper to let you
++        # say "message.is_diverged" which can be clearer than
++        # "message.potemplate is not None."
++        message = self.factory.makeTranslationMessage(force_diverged=False)
++        self.assertFalse(message.is_diverged)
++
++    def test_is_diverged_true(self):
++        message = self.factory.makeTranslationMessage(force_diverged=True)
++        self.assertTrue(message.is_diverged)
++
++
  class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
      """Tests for `TranslationMessage.findIdenticalMessage`."""