Merge lp:~jtv/launchpad/recife-kill-updatetranslation-1 into lp:~launchpad/launchpad/recife

[Jeroen T. Vermeulen]

Replace some updateTranslation calls.

= Kill updateTranslation, Part 1 =

In our Recife feature branch we're finally getting rid of the convoluted monstrosity that is POTMsgSet.updateTranslation. In a way it's tragic: this monster is a helpful one, and that is why we can't afford its upkeep. It just does too much.

In this branch I remove a few uses of updateTranslation from non-test code, though not yet all. Here's a quick walk-through of the diff:

TestResetTranslations replaces a stretch of doctest. It's more setup than test, but even so I think it's an improvement. Going from "story" style to "unit" style also takes away the need to produce a sequence of controlled timestamps: all we need to do is make sure we have a translation older than "now(UTC)" to start out with.

Then, you'll notice my stub implementation of getCurrentTranslation. Danilo currently has another branch in review that implements this in a more permanent way, and mine is marked as temporary. It's just good enough to pass tests in the current state, until we move more methods over to the new model. (This is a feature branch, so there's no production environment to worry about).

What follows is mostly replacing calls to old-style methods with ones to new-style methods.

The tests at the end may confuse you. We renamed some old TranslationMessage attributes to is_current_ubuntu and is_current_upstream respectively, but the automated replacements didn't always leave us with the right one of the two—especially for upstream-side translations, which is what the LaunchpadObjectFactory tends to give us by default. For those we often end up working with is_current_ubuntu when is_current_upstream is really the appropriate flag.

To test this, it's really not good enough to run anything less than:
{{{
./bin/test -vvc lp.translations.tests
}}}

The Q/A will happen along with the rest of the feature branch. I did some lint checks, but at the moment (after commit & push) "make lint" says it doesn't detect any changes so I can't produce a final listing.

Jeroen

[Graham Binns (gmb)]

From IRC:

<gmb> jtv: Please add some comments or docstrings to the start of your unit test functions. Other than that, r=me.

review: Approve (code)

=== modified file 'lib/lp/translations/browser/tests/test_translationmessage_view.py'
--- lib/lp/translations/browser/tests/test_translationmessage_view.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/browser/tests/test_translationmessage_view.py	2010-11-16 14:27:09 +0000
@@ -11,20 +11,28 @@
     )
 
 import pytz
+from zope.security.proxy import removeSecurityProxy
 
+from canonical.launchpad.webapp.publisher import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import ZopelessDatabaseLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.app.errors import UnexpectedFormData
 from lp.testing import (
     anonymous_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.views import create_view
+from lp.translations.enums import TranslationPermission
 from lp.translations.browser.translationmessage import (
     CurrentTranslationMessagePageView,
     CurrentTranslationMessageView,
     )
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
+from lp.translations.publisher import TranslationsLayer
 
 
 class TestCurrentTranslationMessage_can_dismiss(TestCaseWithFactory):
@@ -175,6 +183,97 @@
         self._assertConfirmEmptyPluralPackaged(True, False, False, False)
 
 
+class TestResetTranslations(TestCaseWithFactory):
+    """Test resetting of the current translation.
+
+    A reviewer can reset a current translation by submitting an empty
+    translation and forcing it to be a suggestion.
+
+    :ivar pofile: A `POFile` for an Ubuntu source package.
+    :ivar current_translation: A current `TranslationMessage` in `POFile`,
+        submitted and reviewed sometime in the past.
+    """
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestResetTranslations, self).setUp()
+        package = self.factory.makeSourcePackage()
+        template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+        self.pofile = self.factory.makePOFile(potemplate=template)
+        self.current_translation = self.factory.makeCurrentTranslationMessage(
+            pofile=self.pofile)
+        self.current_translation.setPOFile(self.pofile)
+
+        naked_tm = removeSecurityProxy(self.current_translation)
+        naked_tm.date_created -= timedelta(1)
+        naked_tm.date_reviewed = naked_tm.date_created
+
+    def closeTranslations(self):
+        """Disallow editing of `self.pofile` translations by regular users."""
+        policy = removeSecurityProxy(
+            self.pofile.potemplate.getTranslationPolicy())
+        policy.translationpermission = TranslationPermission.CLOSED
+
+    def getLocalSuggestions(self):
+        """Get local suggestions for `self.current_translation`."""
+        return list(
+            self.current_translation.potmsgset.getLocalTranslationMessages(
+                self.pofile.potemplate, self.pofile.language))
+
+    def submitForcedEmptySuggestion(self):
+        """Submit an empty suggestion for `self.current_translation`."""
+        empty_translation = u''
+
+        msgset_id = 'msgset_' + str(self.current_translation.potmsgset.id)
+        msgset_id_lang = msgset_id + '_' + self.pofile.language.code
+        widget_id_base = msgset_id_lang + '_translation_0_'
+
+        form = {
+            'lock_timestamp': datetime.now(pytz.utc).isoformat(),
+            'alt': None,
+            msgset_id: None,
+            widget_id_base + 'radiobutton': widget_id_base + 'new',
+            widget_id_base + 'new': empty_translation,
+            'submit_translations': 'Save & Continue',
+            msgset_id_lang + '_needsreview': 'force_suggestion',
+        }
+
+        url = canonical_url(self.current_translation) + '/+translate'
+        view = create_view(
+            self.current_translation, '+translate', form=form,
+            layer=TranslationsLayer, server_url=url)
+        view.request.method = 'POST'
+        view.initialize()
+
+    def test_disables_current_translation(self):
+        # Resetting the current translation clears its "current" flag.
+        self.assertTrue(self.current_translation.is_current_ubuntu)
+        with person_logged_in(self.factory.makePerson()):
+            self.submitForcedEmptySuggestion()
+        self.assertFalse(self.current_translation.is_current_ubuntu)
+
+    def test_turns_current_translation_into_suggestion(self):
+        # Resetting the current translation demotes it back to the
+        # status of a suggestion.
+        self.assertEqual([], self.getLocalSuggestions())
+        with person_logged_in(self.factory.makePerson()):
+            self.submitForcedEmptySuggestion()
+        self.assertEqual(
+            [self.current_translation], self.getLocalSuggestions())
+
+    def test_unprivileged_user_cannot_reset(self):
+        # When a user without editing privileges on the translation
+        # submits an empty suggestion, that does not clear the
+        # translation.
+        self.closeTranslations()
+        self.assertTrue(self.current_translation.is_current_ubuntu)
+        with person_logged_in(self.factory.makePerson()):
+            self.submitForcedEmptySuggestion()
+        self.assertTrue(self.current_translation.is_current_ubuntu)
+
+
 class TestCurrentTranslationMessagePageView(TestCaseWithFactory):
     """Test `CurrentTranslationMessagePageView` and its base class."""
 

=== modified file 'lib/lp/translations/browser/tests/translationmessage-views.txt'
--- lib/lp/translations/browser/tests/translationmessage-views.txt	2010-11-09 06:55:07 +0000
+++ lib/lp/translations/browser/tests/translationmessage-views.txt	2010-11-16 14:27:09 +0000
@@ -648,100 +648,3 @@
     False
     >>> subview.shared_translationmessage == translationmessage
     True
-
-
-Resetting current translation
------------------------------
-
-The current translation can be reset by submitting an empty translation
-or a translation similar to the current one while forcing suggestions.
-
-We need to force timestamp update, since suggestions are submitted too fast.
-
-    >>> from lp.translations.enums import TranslationPermission
-    >>> pofile = factory.makePOFile('es')
-    >>> potemplate = pofile.potemplate
-    >>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
-    >>> translationmessage = factory.makeTranslationMessage(
-    ...     potmsgset=potmsgset, pofile=pofile,
-    ...     translations = [u"A shared translation"])
-    >>> translationmessage.setPOFile(pofile)
-    >>> product = potemplate.productseries.product
-    >>> product.translationpermission = TranslationPermission.CLOSED
-    >>> year_tick = 2100
-    >>> def submit_translation(
-    ...     translationmessage, translation, force_suggestion=False):
-    ...     global year_tick
-    ...     potmsgset = translationmessage.potmsgset
-    ...     server_url = '/'.join(
-    ...         [canonical_url(translationmessage), '+translate'])
-    ...     now = (str(year_tick) +
-    ...             datetime.now(UTC).strftime('-%m-%dT%H:%M:%S+00:00'))
-    ...     year_tick += 1
-    ...     msgset_id = 'msgset_' + str(potmsgset.id)
-    ...     language_code = translationmessage.language.code
-    ...     msgset_id_lang = msgset_id + '_' + language_code
-    ...     form = {
-    ...         'lock_timestamp': now,
-    ...         'alt': None,
-    ...         msgset_id : None,
-    ...         msgset_id_lang + '_translation_0_radiobutton':
-    ...         msgset_id_lang + '_translation_0_new',
-    ...         msgset_id_lang + '_translation_0_new': translation,
-    ...         'submit_translations': 'Save & Continue'}
-    ...     if force_suggestion:
-    ...         form[msgset_id_lang + '_needsreview'] = 'force_suggestion'
-    ...     tm_view = create_view(
-    ...         translationmessage, "+translate", form=form,
-    ...         layer=TranslationsLayer, server_url=server_url)
-    ...     tm_view.request.method = 'POST'
-    ...     tm_view.initialize()
-
-    >>> def print_current_translation(potmsgset, pofile):
-    ...     current_translation = potmsgset.getCurrentTranslationMessage(
-    ...         pofile.potemplate, pofile.language)
-    ...     if current_translation is not None:
-    ...         print current_translation.translations
-
-    >>> def print_local_suggestions(potmsgset, pofile):
-    ...     import operator
-    ...     local = sorted(
-    ...         potmsgset.getLocalTranslationMessages(
-    ...             pofile.potemplate, pofile.language),
-    ...         key=operator.attrgetter("date_created"),
-    ...         reverse=True)
-    ...     for suggestion in local:
-    ...         print suggestion.translations
-
-We will make an initial translation.
-
-    >>> submit_translation(
-    ...     translationmessage, u'Foo')
-    >>> print_local_suggestions(potmsgset, pofile)
-    >>> print_current_translation(potmsgset, pofile)
-    [u'Foo']
-
-Forcing suggestions while submitting an empty translation will reset the
-translation for this message and all previously entered translations will be
-listed as suggestions.
-
-    >>> submit_translation(translationmessage, u'', force_suggestion=True)
-    >>> print_local_suggestions(potmsgset, pofile)
-    [u'Foo']
-    [u'A shared translation']
-    >>> print_current_translation(potmsgset, pofile)
-
-Only users with edit rights are allowed to reset a translation.
-
-    >>> submit_translation(translationmessage, u'New current translation')
-    >>> print_current_translation(potmsgset, pofile)
-    [u'New current translation']
-    >>> print_local_suggestions(potmsgset, pofile)
-    >>> login('[e-mail]')
-    >>> submit_translation(
-    ...     translationmessage, u'New current translation',
-    ...     force_suggestion=True)
-    >>> login('[e-mail]')
-    >>> print_local_suggestions(potmsgset, pofile)
-    >>> print_current_translation(potmsgset, pofile)
-    [u'New current translation']

=== modified file 'lib/lp/translations/interfaces/potmsgset.py'
--- lib/lp/translations/interfaces/potmsgset.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/interfaces/potmsgset.py	2010-11-16 14:27:09 +0000
@@ -156,6 +156,12 @@
         Diverged messages are preferred.
         """
 
+    def getCurrentTranslation(potemplate, language):
+        # XXX JeroenVermeulen 2010-11-16: Stub.  To be replaced with
+        # Danilo's simultaneous work.  If you see both definitions, this
+        # is the one you should drop.
+        """Retrieve a current `TranslationMessage`."""
+
     def getSharedTranslationMessage(language):
         """Returns a shared TranslationMessage."""
 

=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-11-15 16:25:05 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-11-16 14:27:09 +0000
@@ -322,6 +322,15 @@
         return self._getUsedTranslationMessage(
             potemplate, language, current=False)
 
+    def getCurrentTranslation(self, potemplate, language):
+        # XXX JeroenVermeulen 2010-11-16: Stub.  To be replaced with
+        # Danilo's simultaneous work.  If you see both definitions, this
+        # is the one you should drop.
+        """See `IPOTMsgSet`."""
+        traits = getUtility(ITranslationSideTraitsSet).getTraits(
+            potemplate.translation_side)
+        return traits.getCurrentMessage(self, potemplate, language)
+
     def getSharedTranslationMessage(self, language):
         """See `IPOTMsgSet`."""
         return self._getUsedTranslationMessage(
@@ -341,7 +350,7 @@
             "msgstr%(form)d IS NOT NULL", "OR")
         query += " AND (%s)" % msgstr_clause
         if include_dismissed != include_unreviewed:
-            current = self.getCurrentTranslationMessage(potemplate, language)
+            current = self.getCurrentTranslation(potemplate, language)
             if current is not None:
                 if current.date_reviewed is None:
                     comparing_date = current.date_created
@@ -1024,17 +1033,18 @@
 
     def dismissAllSuggestions(self, pofile, reviewer, lock_timestamp):
         """See `IPOTMsgSet`."""
-        assert(lock_timestamp is not None)
-        current = self.getCurrentTranslationMessage(
-            self.potemplate, pofile.language)
+        current = self.getCurrentTranslation(
+            pofile.potemplate, pofile.language)
         if current is None:
-            # Create an empty translation message.
-            current = self.updateTranslation(
-                pofile, reviewer, [], False, lock_timestamp)
+            # Create or activate an empty translation message.
+            current = self.setCurrentTranslation(
+                pofile, reviewer, {}, RosettaTranslationOrigin.ROSETTAWEB,
+                lock_timestamp=lock_timestamp)
         else:
             # Check for translation conflicts and update review fields.
             self._maybeRaiseTranslationConflict(current, lock_timestamp)
-            current.markReviewed(reviewer, lock_timestamp)
+        current.markReviewed(reviewer, lock_timestamp)
+        assert self.getCurrentTranslation(pofile.potemplate, pofile.language)
 
     def _nameMessageStatus(self, message, translation_side_traits):
         """Figure out the decision-matrix status of a message.

=== modified file 'lib/lp/translations/model/translatablemessage.py'
--- lib/lp/translations/model/translatablemessage.py	2010-08-23 08:35:29 +0000
+++ lib/lp/translations/model/translatablemessage.py	2010-11-16 14:27:09 +0000
@@ -44,9 +44,8 @@
         self.potemplate = pofile.potemplate
         self.language = pofile.language
 
-        self._current_translation = (
-            self.potmsgset.getCurrentTranslationMessage(
-                self.potemplate, self.language))
+        self._current_translation = self.potmsgset.getCurrentTranslation(
+                self.potemplate, self.language)
 
     @property
     def is_obsolete(self):

=== modified file 'lib/lp/translations/tests/test_potmsgset.py'
--- lib/lp/translations/tests/test_potmsgset.py	2010-11-04 09:32:28 +0000
+++ lib/lp/translations/tests/test_potmsgset.py	2010-11-16 14:27:09 +0000
@@ -24,7 +24,6 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.testing import TestCaseWithFactory
-from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.potmsgset import (
     POTMsgSetInIncompatibleTemplatesError,
@@ -305,8 +304,7 @@
 
         # Setting one of the suggestions as current will leave
         # them both 'reviewed' and thus hidden.
-        current_translation = self.factory.makeSharedTranslationMessage(
-            pofile=sr_pofile, potmsgset=self.potmsgset)
+        shared_suggestion.approve(sr_pofile, self.factory.makePerson())
         self.assertContentEqual(
             [],
             self.potmsgset.getLocalTranslationMessages(
@@ -870,7 +868,7 @@
         self.potmsgset.dismissAllSuggestions(
             self.pofile, self.factory.makePerson(), self.now())
         transaction.commit()
-        current = self.potmsgset.getCurrentTranslationMessage(
+        current = self.potmsgset.getCurrentTranslation(
             self.potemplate, self.pofile.language)
         self.assertNotEqual(None, current)
         self.assertEqual([None], current.translations)

=== modified file 'lib/lp/translations/tests/test_translatablemessage.py'
--- lib/lp/translations/tests/test_translatablemessage.py	2010-11-16 06:15:13 +0000
+++ lib/lp/translations/tests/test_translatablemessage.py	2010-11-16 14:27:09 +0000
@@ -13,6 +13,7 @@
 import pytz
 import transaction
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.app.enums import ServiceUsage
@@ -46,21 +47,29 @@
     def _createTranslation(self, translation=None, is_current_ubuntu=False,
                            is_current_upstream=False, is_diverged=False,
                            date_updated=None):
-        is_suggestion = not (
-            is_current_ubuntu or is_current_upstream or is_diverged)
         if translation is not None:
             translation = [translation]
-        if is_suggestion:
-            return self.factory.makeSuggestion(
+
+        if is_current_upstream:
+            message = self.factory.makeCurrentTranslationMessage(
+                pofile=self.pofile, potmsgset=self.potmsgset,
+                translations=translation,
+                current_other=is_current_ubuntu,
+                diverged=is_diverged)
+            if date_updated is not None:
+                naked_message = removeSecurityProxy(message)
+                naked_message.date_created = date_updated
+                naked_message.date_reviewed = date_updated
+        else:
+            message = self.factory.makeSuggestion(
                 pofile=self.pofile, potmsgset=self.potmsgset,
                 translations=translation, date_created=date_updated)
-        else:
-            return self.factory.makeTranslationMessage(
-                pofile=self.pofile, potmsgset=self.potmsgset,
-                translations=translation,
-                is_current_upstream=is_current_upstream,
-                force_diverged=is_diverged,
-                date_updated=date_updated)
+            message.is_current_ubuntu = is_current_ubuntu
+            self.assertFalse(
+                is_diverged,
+                "Diverging message to a template it's not current in.")
+
+        return message
 
 
 class TestTranslatableMessage(TestTranslatableMessageBase,
@@ -88,8 +97,8 @@
         self.assertTrue(message.is_untranslated)
 
     def test_is_current_diverged(self):
-        translation = self._createTranslation(is_current_ubuntu=True,
-                                              is_diverged=True)
+        translation = self._createTranslation(
+            is_current_upstream=True, is_diverged=True)
         message = TranslatableMessage(self.potmsgset, self.pofile)
         self.assertTrue(message.is_current_diverged)
 
@@ -118,7 +127,7 @@
         self.assertEqual(3, message.number_of_plural_forms)
 
     def test_getCurrentTranslation(self):
-        translation = self._createTranslation(is_current_ubuntu=True)
+        translation = self._createTranslation(is_current_upstream=True)
         message = TranslatableMessage(self.potmsgset, self.pofile)
         current = message.getCurrentTranslation()
         self.assertEqual(translation, current)
@@ -193,7 +202,7 @@
         self.now = self.gen_now().next
         self.suggestion1 = self._createTranslation(date_updated=self.now())
         self.current = self._createTranslation(
-            is_current_ubuntu=True, date_updated=self.now())
+            is_current_upstream=True, date_updated=self.now())
         self.suggestion2 = self._createTranslation(date_updated=self.now())
         self.message = TranslatableMessage(self.potmsgset, self.pofile)