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

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

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.

-----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 ...

> > === 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 `Translatio...

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.

> > + 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.

> > +
> > + 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

-----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,
>>> +
>...

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

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