Merge lp:~jtv/launchpad/recife-new-resetcurrenttranslation into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

resetCurrentTranslation

= resetCurrentTranslation =

For the Recife feature branch, upon discussion with Danilo. This re-implements POTMsgSet.resetCurrentTranslation based on the new data model.

However that change breaks a view test. The breakage would have been hard to fix (without getting very dirty and making it very brittle) because the one call to resetCurrentTranslation, from a view, is right next to POTMsgSet.updateTranslation. That's the huge, complex, convoluted, unmaintainable and inscrutable method that we're trying to get rid of with the old model. No matter what we do, a view method that combines the old-model updateTranslation and the new-model resetCurrentTranslation will be wrong.

So I renamed the old resetCurrentTranslation to old_resetCurrentTranslation and kept its tests (those that still make sense) for a fresh, totally revised, new-world version of resetCurrentTranslation. Once we rip out the updateTranslation-based view code and replace it with proper new-model code, that code will use the new resetCurrentTranslation.

You will see a few things in this branch that may shock you, dear reviewer:

1. old_resetCurrentTranslation is _not_ an acceptable method name.

2. There are no tests for old_resetCurrentTranslation.

How do I look myself in the mirror? Well, I feel it all makes sense if you'll give me a chance to explain.

First, we want old_resetCurrentTranslation to die. Quickly and painlessly, but die nonetheless. The more it stands out as a Thing That Should Not Be™, the less likely it is that we'll be forgetting to get rid of it, or uncertain about whether it can be cleaned out, or using it in more places, or tolerating its presence. So in this case, a bad name is a good one.

Second, we want old_resetCurrentTranslation to die. It has but a single callsite, and that needs to be completely rewritten. The only reason to keep the old method around is to satisfy existing tests until we can get the new model in place. In a shameless reversal of the normal relationships between working code and tests, the acceptance tests are what we really want to preserve here and old_resetCurrentTranslation is there only to make sure that we can. If future changes required this method to stand on its head and recite Shakespeare in order for the view tests to pass, I would happily make it do that—without a single unit test—and still sleep well at night.

With that I conclude my case. I hope I managed to convey the fact that we want old_resetCurrentTranslation to die. And if anything, we are the victims here. Trust us.

To test:
{{{
./bin/test -vvc -m lp.translations.tests.test_potmsgset
./bin/test -vvc -m lp.translations -t pofile-translate-needs-review-flags-preserved
}}}

No lint left, apart from a few pre-existing complaints about comments surrounded by blank lines. In all of the cases I didn't fix, I felt the code had a legitimate need for creative license. Plus, I don't want to avoid _too_ many conflicts with other people's ongoing work!

Jeroen

[Michael Nelson (michael.nelson)]

Heh... I enjoyed reading your MP description :)

And yes, I've been in that position before too... the one case (a transition to new infrastructure) where a bad name is a good one.

I've just got one question below about "the other side". But r=me. And great tests :)

> === modified file 'lib/lp/translations/browser/translationmessage.py'
> --- lib/lp/translations/browser/translationmessage.py 2010-08-23 08:35:29 +0000
> +++ lib/lp/translations/browser/translationmessage.py 2010-08-26 10:24:07 +0000
> @@ -300,16 +299,16 @@
>
> if self.request.method == 'POST':
> if self.user is None:
> - raise UnexpectedFormData, (
> - 'Anonymous users or users who are not accepting our '
> - 'licensing terms cannot do POST submissions.')
> + raise UnexpectedFormData(
> + "Anonymous users or users who are not accepting our "
> + "licensing terms cannot do POST submissions.")

Out of interest, is switching to double-quotes by default a personal
preference or a new LP coding recommendation?

> === modified file 'lib/lp/translations/model/potmsgset.py'
> --- lib/lp/translations/model/potmsgset.py 2010-08-24 11:39:06 +0000
> +++ lib/lp/translations/model/potmsgset.py 2010-08-26 10:24:07 +0000
> @@ -1295,25 +1295,47 @@
>
> return message
>
> - def resetCurrentTranslation(self, pofile, lock_timestamp):
> - """See `IPOTMsgSet`."""
> -
> - assert(lock_timestamp is not None)
> -
> + def old_resetCurrentTranslation(self, pofile, lock_timestamp):
> + """See `POTMsgSet`.
> +
> + This message is OBSOLETE in the Recife feature branch. It's
> + still here only until we replace its one call with the new
> + method.
> + """
> + assert lock_timestamp is not None, "No lock timestamp given."
> current = self.getCurrentTranslationMessage(
> pofile.potemplate, pofile.language)
> -
> - if (current is not None):
> - # Check for translation conflicts and update the required
> - # attributes.
> - self._maybeRaiseTranslationConflict(current, lock_timestamp)
> - current.is_current_ubuntu = False
> - # Converge the current translation only if it is diverged and not
> - # current upstream.
> - is_diverged = current.potemplate is not None
> - if is_diverged and not current.is_current_upstream:
> - current.potemplate = None
> - pofile.date_changed = UTC_NOW
> + if current is None:
> + return

Nice... I prefer guard code like this too :)

> +
> + # Check for translation conflicts and update the required
> + # attributes.
> + self._maybeRaiseTranslationConflict(current, lock_timestamp)
> + current.is_current_ubuntu = False
> + # Converge the current translation only if it is diverged and
> + # not current upstream.
> + if current.is_diverged and not current.is_current_upstream:
> + current.potemplate = None
> + pofile.markChanged()
> +
> + def resetCurrentTranslation(...

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks for the review, and glad you enjoyed the MP. :)

> > === modified file 'lib/lp/translations/browser/translationmessage.py'
> > --- lib/lp/translations/browser/translationmessage.py 2010-08-23 08:35:29
> +0000
> > +++ lib/lp/translations/browser/translationmessage.py 2010-08-26 10:24:07
> +0000
> > @@ -300,16 +299,16 @@
> >
> > if self.request.method == 'POST':
> > if self.user is None:
> > - raise UnexpectedFormData, (
> > - 'Anonymous users or users who are not accepting our '
> > - 'licensing terms cannot do POST submissions.')
> > + raise UnexpectedFormData(
> > + "Anonymous users or users who are not accepting our "
> > + "licensing terms cannot do POST submissions.")
>
> Out of interest, is switching to double-quotes by default a personal
> preference or a new LP coding recommendation?

Personal preference. But I do think it's sensible for free-form text where an apostrophe would be perfectly appropriate. IIRC one of these strings even went so far as to backslash-escape an apostrophe. For God's sake, why!?

Personally I use single quotes (to minimize "ink on the page") for identifiers and such, where an apostrophe would not be appropriate, or for empty strings and things close to it.

> > === modified file 'lib/lp/translations/model/potmsgset.py'
> > --- lib/lp/translations/model/potmsgset.py 2010-08-24 11:39:06 +0000
> > +++ lib/lp/translations/model/potmsgset.py 2010-08-26 10:24:07 +0000

> > +
> > + # Check for translation conflicts and update the required
> > + # attributes.
> > + self._maybeRaiseTranslationConflict(current, lock_timestamp)
> > + current.is_current_ubuntu = False
> > + # Converge the current translation only if it is diverged and
> > + # not current upstream.
> > + if current.is_diverged and not current.is_current_upstream:
> > + current.potemplate = None
> > + pofile.markChanged()
> > +
> > + def resetCurrentTranslation(self, pofile, lock_timestamp=None,
> > + share_with_other_side=False):
>
> OK, I forgot to mention this on the IFace declaration, but I have no idea what
> the other side is (maybe the interface description could say a bit more to
> cater for non-translation folk?). From your tests, it looks like it is
> upstream/downstream?

Pretty much, yes: "project" and "Ubuntu." A TranslationMessage is selected as current on the "project" side if it has the is_current_upstream flag set, and on the Ubuntu side if it has the is_current_ubuntu flag set. See lp.translations.interfaces.side.

> > + """See `IPOTMsgSet`."""
> > + traits = getUtility(ITranslationSideTraitsSet).getTraits(
> > + pofile.potemplate.translation_side)
> > + current_message = traits.getCurrentMessage(
> > + self, pofile.potemplate, pofile.language)
> > +
> > + if current_message is None:
> > + # Nothing to do here.
>
> Is this an error? If a reset is called on a translation that doesn't yet have
> a translation? I'm sure you've though...

=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py	2010-08-23 08:35:29 +0000
+++ lib/lp/translations/browser/translationmessage.py	2010-08-26 09:18:44 +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=W0404
@@ -25,7 +25,6 @@
 import re
 import urllib
 
-import gettextpo
 import pytz
 from z3c.ptcompat import ViewPageTemplateFile
 from zope import datetime as zope_datetime
@@ -300,16 +299,16 @@
 
         if self.request.method == 'POST':
             if self.user is None:
-                raise UnexpectedFormData, (
-                    'Anonymous users or users who are not accepting our '
-                    'licensing terms cannot do POST submissions.')
+                raise UnexpectedFormData(
+                    "Anonymous users or users who are not accepting our "
+                    "licensing terms cannot do POST submissions.")
             translations_person = ITranslationsPerson(self.user)
             if (translations_person.translations_relicensing_agreement
                     is not None and
                 not translations_person.translations_relicensing_agreement):
-                raise UnexpectedFormData, (
-                    'Users who do not agree to licensing terms '
-                    'cannot do POST submissions.')
+                raise UnexpectedFormData(
+                    "Users who do not agree to licensing terms "
+                    "cannot do POST submissions.")
             try:
                 # Try to get the timestamp when the submitted form was
                 # created. We use it to detect whether someone else updated
@@ -320,9 +319,9 @@
             except zope_datetime.DateTimeError:
                 # invalid format. Either we don't have the timestamp in the
                 # submitted form or it has the wrong format.
-                raise UnexpectedFormData, (
-                    'We didn\'t find the timestamp that tells us when was'
-                    ' generated the submitted form.')
+                raise UnexpectedFormData(
+                    "We didn't find the timestamp that tells us when was"
+                    " generated the submitted form.")
 
             # Check if this is really the form we are listening for..
             if self.request.form.get("submit_translations"):
@@ -436,13 +435,14 @@
                 force_suggestion=force_suggestion,
                 force_diverged=force_diverge)
 
-            # If suggestions were forced and user has the rights to do it,
-            # reset the current translation.
             empty_suggestions = self._areSuggestionsEmpty(translations)
             if (force_suggestion and
                 self.user_is_official_translator and
                 empty_suggestions):
-                potmsgset.resetCurrentTranslation(
+                # The user requested that the message be reviewed,
+                # without suggesting a new translation.  Reset the
+                # current translation so that it can be reviewed again.
+                potmsgset.old_resetCurrentTranslation(
                     self.pofile, self.lock_timestamp)
 
         except TranslationConflict:
@@ -1481,7 +1481,6 @@
         return "%s_dismissable_button" % self.html_id
 
 
-
 class CurrentTranslationMessageZoomedView(CurrentTranslationMessageView):
     """A view that displays a `TranslationMessage`, but zoomed in.
 
@@ -1515,7 +1514,6 @@
 # Pseudo-content class
 #
 
-
 class TranslationMessageSuggestions:
     """See `ITranslationMessageSuggestions`."""
 
@@ -1563,6 +1561,7 @@
 class Submission:
     """A submission generated from a TranslationMessage"""
 
+
 def convert_translationmessage_to_submission(
     message, current_message, plural_form, pofile, legal_warning_needed,
     is_empty=False, packaged=False):

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2010-08-24 11:39:06 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2010-08-26 09:18:44 +0000
@@ -298,16 +298,29 @@
             that this change is based on.
         """
 
-    def resetCurrentTranslation(pofile, lock_timestamp):
-        """Reset the currently used translation.
-
-        This will set the "is_current_ubuntu" attribute to False and if the
-        message is diverged, will try to converge it.
-        :param pofile: a `POFile` to dismiss suggestions from.
-        :param lock_timestamp: the timestamp when we checked the values we
-            want to update.
-
-        If a translation conflict is detected, TranslationConflict is raised.
+    def old_resetCurrentTranslation(pofile, lock_timestamp):
+        """Reset a translation.
+
+        OBSOLETE in Recife.  In the new model, use the new
+        `resetCurrentTranslation` implementation instead.
+        """
+
+    def resetCurrentTranslation(pofile, lock_timestamp=None,
+                                share_with_other_side=False):
+        """Turn the current translation back into a suggestion.
+
+        This deactivates the message's current translation.  The message
+        becomes untranslated or, if it was diverged, reverts to its
+        shared translation.
+
+        The previously current translation becomes visible as a new
+        suggestion again, as do all suggestions that came after it.
+
+        :param pofile: The `POFile` to make the change in.
+        :param lock_timestamp: Timestamp of the original translation state
+            that this change is based on.
+        :param share_with_other_side: Make the same change on the other
+            translation side.
         """
 
     def clearCurrentTranslation(pofile, submitter, origin,

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-24 11:39:06 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-26 09:18:44 +0000
@@ -1295,25 +1295,47 @@
 
         return message
 
-    def resetCurrentTranslation(self, pofile, lock_timestamp):
-        """See `IPOTMsgSet`."""
-
-        assert(lock_timestamp is not None)
-
+    def old_resetCurrentTranslation(self, pofile, lock_timestamp):
+        """See `POTMsgSet`.
+
+        This message is OBSOLETE in the Recife feature branch.  It's
+        still here only until we replace its one call with the new
+        method.
+        """
+        assert lock_timestamp is not None, "No lock timestamp given."
         current = self.getCurrentTranslationMessage(
             pofile.potemplate, pofile.language)
-
-        if (current is not None):
-            # Check for translation conflicts and update the required
-            # attributes.
-            self._maybeRaiseTranslationConflict(current, lock_timestamp)
-            current.is_current_ubuntu = False
-            # Converge the current translation only if it is diverged and not
-            # current upstream.
-            is_diverged = current.potemplate is not None
-            if is_diverged and not current.is_current_upstream:
-                current.potemplate = None
-            pofile.date_changed = UTC_NOW
+        if current is None:
+            return
+
+        # Check for translation conflicts and update the required
+        # attributes.
+        self._maybeRaiseTranslationConflict(current, lock_timestamp)
+        current.is_current_ubuntu = False
+        # Converge the current translation only if it is diverged and
+        # not current upstream.
+        if current.is_diverged and not current.is_current_upstream:
+            current.potemplate = None
+        pofile.markChanged()
+
+    def resetCurrentTranslation(self, pofile, lock_timestamp=None,
+                                share_with_other_side=False):
+        """See `IPOTMsgSet`."""
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            pofile.potemplate.translation_side)
+        current_message = traits.getCurrentMessage(
+            self, pofile.potemplate, pofile.language)
+
+        if current_message is None:
+            # Nothing to do here.
+            return
+
+        self._checkForConflict(current_message, lock_timestamp)
+        traits.setFlag(current_message, False)
+        if share_with_other_side:
+            traits.other_side_traits.setFlag(current_message, False)
+        current_message.shareIfPossible()
+        pofile.markChanged()
 
     def clearCurrentTranslation(self, pofile, submitter, origin,
                                 share_with_other_side=False,

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-08-24 11:39:06 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-08-26 09:18:44 +0000
@@ -31,6 +31,7 @@
     POTMsgSetInIncompatibleTemplatesError,
     TranslationCreditsType,
     )
+from lp.translations.interfaces.side import ITranslationSideTraitsSet
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat,
     )
@@ -943,6 +944,12 @@
             yield now
             now += timedelta(milliseconds=1)
 
+    def _getCurrentMessage(self):
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            self.potemplate.translation_side)
+        return traits.getCurrentMessage(
+            self.potmsgset, self.potemplate, self.pofile.language)
+
     def setUp(self):
         # Create a product with all the boilerplate objects to be able to
         # create TranslationMessage objects.
@@ -960,59 +967,117 @@
         self.pofile = self.factory.makePOFile('eo', template)
 
     def test_resetCurrentTranslation_shared(self):
-        # Resetting a shared current translation will change iscurrent=False
-        # and there will be no other current translations for this POTMsgSet.
-
-        translation = self.factory.makeTranslationMessage(
-            self.pofile, self.potmsgset, translations=[u'Shared translation'],
-            reviewer=self.factory.makePerson(),
-            is_current_upstream=False, force_diverged=False,
-            date_updated=self.now())
-
-        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
-        current = self.potmsgset.getCurrentTranslationMessage(
-            self.potemplate, self.pofile.language)
+        # Resetting a shared current translation deactivates it, and
+        # leaves no other current translation in its place.
+        translation = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+
+        self.potmsgset.resetCurrentTranslation(self.pofile)
+
+        current = self._getCurrentMessage()
         self.assertTrue(current is None)
         self.assertFalse(translation.is_current_ubuntu)
         self.assertFalse(translation.is_current_upstream)
-        self.assertTrue(translation.potemplate is None)
+        self.assertFalse(translation.is_diverged)
 
     def test_resetCurrentTranslation_diverged_not_imported(self):
-        # Resetting a diverged current translation that was not
-        # imported, will change is_current_ubuntu to False and will make
-        # it shared.
-        translation = self.factory.makeTranslationMessage(
-            self.pofile, self.potmsgset, translations=[u'Diverged text'],
-            reviewer=self.factory.makePerson(),
-            is_current_upstream=False, force_diverged=True,
-            date_updated=self.now())
-
-        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
-        current = self.potmsgset.getCurrentTranslationMessage(
-            self.potemplate, self.pofile.language)
+        # Resetting a diverged current translation disables it and makes
+        # it shared.  In other words, it becomes a suggestion.
+        translation = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+
+        self.potmsgset.resetCurrentTranslation(self.pofile)
+
+        current = self._getCurrentMessage()
         self.assertTrue(current is None)
         self.assertFalse(translation.is_current_ubuntu)
         self.assertFalse(translation.is_current_upstream)
-        self.assertTrue(translation.potemplate is None)
-
-    def test_resetCurrentTranslation_diverged_imported(self):
-        # Resetting a diverged current translation that was imported in
-        # Launchpad will change iscurrent to False but the translation
-        # message will be still diverged.
-
-        translation = self.factory.makeTranslationMessage(
-            self.pofile, self.potmsgset, translations=[u'Imported diverged'],
-            reviewer=self.factory.makePerson(),
-            is_current_upstream=True, force_diverged=True,
-            date_updated=self.now())
-
-        self.potmsgset.resetCurrentTranslation(self.pofile, self.now())
-        current = self.potmsgset.getCurrentTranslationMessage(
-            self.potemplate, self.pofile.language)
-        self.assertTrue(current is None)
-        self.assertFalse(translation.is_current_ubuntu)
-        self.assertTrue(translation.is_current_upstream)
-        self.assertFalse(translation.potemplate is None)
+        self.assertFalse(translation.is_diverged)
+
+    def test_resetCurrentTranslation_unmasks_shared(self):
+        # Resetting a diverged translation reverts the POTMsgSet to its
+        # current shared translation.
+        shared = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+        diverged = self.factory.makeDivergedTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+
+        self.assertNotEqual(shared, diverged)
+        self.assertTrue(diverged.is_current_upstream)
+        self.assertTrue(shared.is_current_upstream)
+        self.assertEqual(diverged, self._getCurrentMessage())
+
+        self.potmsgset.resetCurrentTranslation(self.pofile)
+
+        self.assertEqual(shared, self._getCurrentMessage())
+
+    def test_resetCurrentTranslation_resets_one_side(self):
+        # By default, resetting a translation works only on one
+        # translation side.
+        current = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset, current_other=True)
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            self.potemplate.translation_side)
+
+        self.assertTrue(traits.getFlag(current))
+        self.assertTrue(traits.other_side_traits.getFlag(current))
+
+        self.potmsgset.resetCurrentTranslation(
+            self.pofile, share_with_other_side=False)
+
+        self.assertFalse(traits.getFlag(current))
+        self.assertTrue(traits.other_side_traits.getFlag(current))
+
+    def test_resetCurrentTranslation_resets_both_sides(self):
+        # The share_with_other_side parameter lets you reset a current
+        # translation on both translation sides.
+        current = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset, current_other=True)
+
+        self.assertTrue(current.is_current_upstream)
+        self.assertTrue(current.is_current_ubuntu)
+
+        self.potmsgset.resetCurrentTranslation(
+            self.pofile, share_with_other_side=True)
+
+        self.assertFalse(current.is_current_upstream)
+        self.assertFalse(current.is_current_ubuntu)
+
+    def test_resetCurrentTranslation_does_not_override_other_message(self):
+        # Resetting a message does not reset the current translation on
+        # the other translation side if it's not the same one as on this
+        # side.
+        self.assertIs(None, self.potemplate.distroseries)
+        other_potemplate = self.factory.makePOTemplate(
+            distroseries=self.factory.makeDistroSeries(),
+            sourcepackagename=self.factory.makeSourcePackageName())
+        other_pofile = self.factory.makePOFile(
+            self.pofile.language.code, potemplate=other_potemplate)
+
+        message_this = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+        message_other = self.factory.makeCurrentTranslationMessage(
+            pofile=other_pofile, potmsgset=self.potmsgset)
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            self.potemplate.translation_side)
+
+        self.assertTrue(traits.other_side_traits.getFlag(message_other))
+
+        self.potmsgset.resetCurrentTranslation(
+            self.pofile, share_with_other_side=True)
+
+        self.assertTrue(traits.other_side_traits.getFlag(message_other))
+
+    def test_resetCurrentTranslation_detects_conflict(self):
+        now = self.now()
+        current = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile, potmsgset=self.potmsgset)
+        current.markReviewed(self.factory.makePerson(), now)
+
+        self.assertRaises(
+            TranslationConflict,
+            self.potmsgset.resetCurrentTranslation,
+            self.pofile, now - timedelta(1))
 
 
 class TestPOTMsgSetCornerCases(TestCaseWithFactory):
@@ -1587,8 +1652,7 @@
     def _makeTranslations(self, potmsgset, forms=1):
         return removeSecurityProxy(potmsgset)._findPOTranslations([
             self.factory.getUniqueString()
-            for counter in xrange(forms)
-            ])
+            for counter in xrange(forms)])
 
     def test_baseline(self):
         # setCurrentTranslation sets the current upstream translation