Merge lp:~jtv/launchpad/bug-994410-stop-using-latest_message into lp:launchpad

[Jeroen T. Vermeulen]

Stop using POFileTranslator.latest_message.

This field in POFileTranslator can be costly to compute, and can restrict the way we move records around in scripts, but it's only used in one place and not a critical one at that: the translations-merging script tries to fold POFileTranslator records together when merging translations. An approximation of the correct move is sort of good enough. The POFileTranslator table isn't very exact.

More precision for POFileTranslator would be nice, but if that's what we want, we have much bigger fish to fry. Such as the breakage of the Ubuntu translations-copying script, which we risk exactly because of the need to maintain POFileTranslator, and which in a twist of irony caused us to lose a bunch of records in the Precise opening. We should maintain its integrity asynchronously with a scrubber script later (so that it's still inexact but at least *eventually* consistent) and drop the database trigger and other hoop-jumping that we do to keep this table in shape.

This branch contains no lint (you may notice I removed some), it updates copyright notices, and it passes all translations tests.

Jeroen

[Stuart Bishop (stub)]

Looks fine.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Came through EC2 without problems. Now trying to make it land.

=== modified file 'lib/lp/translations/doc/translationmessage-destroy.txt'
--- lib/lp/translations/doc/translationmessage-destroy.txt	2012-01-20 15:42:44 +0000
+++ lib/lp/translations/doc/translationmessage-destroy.txt	2012-05-08 04:45:29 +0000
@@ -33,8 +33,8 @@
 In two sharing POTemplates with one shared POTMsgSet with one shared
 translation, we get two POFileTranslator records for each of the POFiles.
 
-    >>> # We need to be able to create persons and projects so let's just use
-    >>> # a global 'postgres' permission which allows everything.
+    # We need to be able to create persons and projects so let's just use
+    # a global 'postgres' permission which allows everything.
     >>> switch_dbuser('postgres')
     >>> from lp.services.database.sqlbase import sqlvalues
     >>> from lp.app.enums import ServiceUsage
@@ -63,13 +63,6 @@
     ...     potmsgset=potmsgset,
     ...     translations=[u"blah"])
     >>> print POFileTranslator.select(
-    ...     "latest_message=%s" % sqlvalues(tm)).count()
+    ...     "pofile IN (%s, %s)"
+    ...     % sqlvalues(devel_sr_pofile, stable_sr_pofile)).count()
     2
-
-Removing a translation message triggers POFileTranslator row removal as well.
-
-    >>> tm.destroySelf()
-    >>> print list(POFileTranslator.select(
-    ...     "latest_message=%s" % sqlvalues(tm)))
-    []
-

=== modified file 'lib/lp/translations/interfaces/pofiletranslator.py'
--- lib/lp/translations/interfaces/pofiletranslator.py	2011-12-24 16:54:44 +0000
+++ lib/lp/translations/interfaces/pofiletranslator.py	2012-05-08 04:45:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0213
@@ -21,7 +21,6 @@
 from lp import _
 from lp.registry.interfaces.person import IPerson
 from lp.translations.interfaces.pofile import IPOFile
-from lp.translations.interfaces.translationmessage import ITranslationMessage
 
 
 class IPOFileTranslator(Interface):
@@ -37,10 +36,6 @@
         title=_(u"The `POFile` modified by the translator."), required=True,
         readonly=True, schema=IPOFile)
 
-    latest_message = Object(
-        title=_(u"Latest `TranslationMessage` for this person and file."),
-        required=True, readonly=True, schema=ITranslationMessage)
-
     date_last_touched = Datetime(
         title=_(u"When the latest translation message was added."),
         required=True, readonly=True)
@@ -69,9 +64,10 @@
             person and pofile, or None.
         """
 
-    def getForPOTMsgSet(potmsgset):
-        """Retrieve `POFileTranslator`s for translations of `potmsgset`.
+    def getForTemplate(potemplate):
+        """Retrieve `POFileTranslator` objects associated iwth `POTemplate`.
 
-        :return: a query result of `POFileTranslator`s whose
-            `latest_message` are translations of `potmsgset`.
+        :param potemplate: `POTemplate` to look for.
+        :return: Result set of `POFileTranslator` records associated with
+            `POFile`s that translate `potemplate`.
         """

=== modified file 'lib/lp/translations/model/pofiletranslator.py'
--- lib/lp/translations/model/pofiletranslator.py	2011-12-30 06:14:56 +0000
+++ lib/lp/translations/model/pofiletranslator.py	2012-05-08 04:45:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -23,7 +23,7 @@
     IPOFileTranslator,
     IPOFileTranslatorSet,
     )
-from lp.translations.model.translationmessage import TranslationMessage
+from lp.translations.model.pofile import POFile
 
 
 class POFileTranslator(SQLBase):
@@ -34,9 +34,6 @@
     person = ForeignKey(
         dbName='person', foreignKey='Person',
         storm_validator=validate_public_person, notNull=True)
-    latest_message = ForeignKey(
-        foreignKey='TranslationMessage', dbName='latest_message',
-        notNull=True)
     date_last_touched = UtcDateTimeCol(
         dbName='date_last_touched', notNull=False, default=None)
 
@@ -62,10 +59,6 @@
                 'pofile.potemplate.productseries.product',
                 'pofile.potemplate.distroseries',
                 'pofile.potemplate.sourcepackagename',
-                'latest_message',
-                'latest_message.potmsgset',
-                'latest_message.potmsgset.msgid_singular',
-                'latest_message.msgstr0',
                 ]))
 
     def getForPersonPOFile(self, person, pofile):
@@ -74,10 +67,9 @@
             POFileTranslator.person == person.id,
             POFileTranslator.pofile == pofile.id)).one()
 
-    def getForPOTMsgSet(self, potmsgset):
+    def getForTemplate(self, potemplate):
         """See `IPOFileTranslatorSet`."""
-        store = Store.of(potmsgset)
-        match = And(
-            POFileTranslator.latest_message == TranslationMessage.id,
-            TranslationMessage.potmsgset == potmsgset)
-        return store.find(POFileTranslator, match).config(distinct=True)
+        return Store.of(potemplate).find(
+            POFileTranslator,
+            POFileTranslator.pofileID == POFile.id,
+            POFile.potemplateID == potemplate.id)

=== removed file 'lib/lp/translations/tests/pofiletranslator.txt'
--- lib/lp/translations/tests/pofiletranslator.txt	2011-12-30 06:14:56 +0000
+++ lib/lp/translations/tests/pofiletranslator.txt	1970-01-01 00:00:00 +0000
@@ -1,99 +0,0 @@
-The POFileTranslator table is an eager materialized view, containing
-references to the most recent posubmission made by a particular person
-to a particular pofile.
-
-This table is read only to most users, but for this test we connect
-as a superuser so we can poke at it directly.
-
-    >>> from lp.services.database.sqlbase import connect
-    >>> connection = connect(user='testadmin')
-    >>> cur = connection.cursor()
-    >>> def pofiletranslator(person_id, pofile_id):
-    ...     cur.execute("""
-    ...         SELECT latest_message, date_last_touched
-    ...         FROM POFileTranslator
-    ...         WHERE person = %(person_id)s AND pofile = %(pofile_id)s
-    ...         """, vars())
-    ...     result = cur.fetchone()
-    ...     if result is None:
-    ...         return None
-    ...     return list(result)
-
-    >>> stub_id = 22
-    >>> pofile_id = 1
-    >>> language_id = 387
-    >>> potmsgset_id = 1
-
-Note that our oldest TranslationMessage belongs to pofile #1.
-
-Stub has so far not translated anything in this pofile
-
-    >>> pofiletranslator(stub_id, pofile_id) is None
-    True
-
-If we add a message, the cache is updated
-
-    >>> cur.execute("""
-    ...     INSERT INTO TranslationMessage(
-    ...         potmsgset, msgstr1, origin, submitter, language
-    ...       ) VALUES (
-    ...         %(potmsgset_id)s, 1, 1, %(stub_id)s, %(language_id)s)
-    ...     """, vars())
-    >>> posubmission_id, date_touched = pofiletranslator(stub_id, pofile_id)
-
-
-We now set the last touched timestamp into the past so we can detect that
-it is correctly updated. This is only possible because we are connected to
-the database as the testadmin user.
-
-    >>> cur.execute("""
-    ...     UPDATE POFileTranslator
-    ...     SET date_last_touched = CURRENT_TIMESTAMP - '1 day'::interval
-    ...     WHERE person=%(stub_id)s AND pofile=%(pofile_id)s
-    ...     """, vars())
-
-
-If we add a new submission to that pofile, the cache is updated including
-the last touched timestamp.
-
-    >>> cur.execute("""
-    ...     INSERT INTO TranslationMessage(
-    ...         potmsgset, msgstr1, origin, submitter, language
-    ...       ) VALUES (
-    ...         %(potmsgset_id)s, 2, 1, %(stub_id)s, %(language_id)s)
-    ...     """, vars())
-    >>> new_posubmission_id, new_date_touched = pofiletranslator(
-    ...     stub_id, pofile_id)
-    >>> posubmission_id == new_posubmission_id
-    False
-
-
-We should get the same timestamp as before, despite having updated the
-cache manually, as we are in the same database transaction and
-CURRENT_TIMESTAMP will return a constant value throughout the transaction
-and the triggers will have reset the value in the cache.
-
-    >>> date_touched == new_date_touched
-    True
-
-
-If we update the submissin, the cache is updated
-
-    >>> mark_id = 1
-    >>> cur.execute("""
-    ...     UPDATE TranslationMessage SET submitter=%(mark_id)s
-    ...     WHERE id = %(new_posubmission_id)s
-    ...     """, vars())
-    >>> mark_posubmission_id, date_touched = pofiletranslator(
-    ...     mark_id, pofile_id)
-    >>> stub_posubmission_id, date_touched = pofiletranslator(
-    ...     stub_id, pofile_id)
-    >>> mark_posubmission_id == stub_posubmission_id
-    False
-
-
-The trigger was smart enough to locate the previous submission from stub
-
-    >>> stub_posubmission_id == posubmission_id
-    True
-

=== modified file 'lib/lp/translations/tests/test_pofiletranslator.py'
--- lib/lp/translations/tests/test_pofiletranslator.py	2011-12-28 17:03:06 +0000
+++ lib/lp/translations/tests/test_pofiletranslator.py	2012-05-08 04:45:29 +0000
@@ -1,14 +1,60 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Runs the POFileTranslator test."""
 
 __metaclass__ = type
 
-from lp.testing.layers import DatabaseLayer
-from lp.testing.systemdocs import LayeredDocFileSuite
-
-
-def test_suite():
-    return LayeredDocFileSuite(
-        'pofiletranslator.txt', layer=DatabaseLayer)
+from zope.component import getUtility
+
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import ZopelessDatabaseLayer
+from lp.translations.interfaces.pofiletranslator import IPOFileTranslatorSet
+
+
+class TestPOFileTranslator(TestCaseWithFactory):
+    layer = ZopelessDatabaseLayer
+
+    def test_getForPersonPOFile_returns_None_if_not_found(self):
+        self.assertIsNone(
+            getUtility(IPOFileTranslatorSet).getForPersonPOFile(
+                self.factory.makePerson(), self.factory.makePOFile()))
+
+    def test_getForPersonPOFile_finds_record(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        tm = self.factory.makeCurrentTranslationMessage(
+            potmsgset=potmsgset, language=pofile.language)
+        poft = getUtility(IPOFileTranslatorSet).getForPersonPOFile(
+            tm.submitter, pofile)
+        self.assertEqual(pofile, poft.pofile)
+        self.assertEqual(tm.submitter, poft.person)
+
+    def test_getForPersonPOFile_ignores_other_persons(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        self.factory.makeCurrentTranslationMessage(
+            potmsgset=potmsgset, language=pofile.language)
+        self.assertIsNone(
+            getUtility(IPOFileTranslatorSet).getForPersonPOFile(
+                self.factory.makePerson(), pofile))
+
+    def test_getForPersonPOFile_ignores_other_POFiles(self):
+        pofile = self.factory.makePOFile('nl')
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        tm = self.factory.makeCurrentTranslationMessage(
+            potmsgset=potmsgset, language=pofile.language)
+        other_pofile = self.factory.makePOFile('de', pofile.potemplate)
+        self.assertIsNone(
+            getUtility(IPOFileTranslatorSet).getForPersonPOFile(
+                tm.submitter, other_pofile))
+
+    def test_getForTemplate_finds_all_for_template(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(pofile.potemplate)
+        tm = self.factory.makeCurrentTranslationMessage(
+            potmsgset=potmsgset, language=pofile.language)
+        [poft] = list(
+            getUtility(IPOFileTranslatorSet).getForTemplate(pofile.potemplate))
+        self.assertEqual(pofile.potemplate, poft.pofile.potemplate)
+        self.assertEqual(tm.submitter, poft.person)

=== modified file 'lib/lp/translations/tests/test_translationmerger.py'
--- lib/lp/translations/tests/test_translationmerger.py	2012-04-27 15:02:49 +0000
+++ lib/lp/translations/tests/test_translationmerger.py	2012-05-08 04:45:29 +0000
@@ -3,7 +3,6 @@
 
 __metaclass__ = type
 
-from datetime import timedelta
 import gc
 from logging import ERROR
 
@@ -20,7 +19,6 @@
     )
 from lp.testing.layers import LaunchpadZopelessLayer
 from lp.testing.sampledata import ADMIN_EMAIL
-from lp.translations.interfaces.pofiletranslator import IPOFileTranslatorSet
 from lp.translations.model.pomsgid import POMsgID
 from lp.translations.model.potemplate import POTemplate
 from lp.translations.model.potranslation import POTranslation
@@ -510,59 +508,6 @@
         tms = trunk_message.potmsgset.getAllTranslationMessages()
         self.assertEqual(list(tms), [trunk_message])
 
-    # XXX: GavinPanella 2011-10-28 bug=883274: Spurious failure in buildbot.
-    def disabled_test_clashingPOFileTranslatorEntries(self):
-        # POFileTranslator is maintained by a trigger on
-        # TranslationMessage.  Fiddling with TranslationTemplateItems
-        # directly bypasses it, so the script must make sure that
-        # POFileTranslator respects its unique constraints.
-
-        # In this scenario, "trunk" has a TranslationMessage with a
-        # matching POFileTranslator entry.  This message is happy where
-        # it is; it's not changing in any way during the test.
-        poftset = getUtility(IPOFileTranslatorSet)
-
-        translator = self.trunk_template.owner
-        self.trunk_pofile.owner = translator
-        self.stable_pofile.owner = translator
-
-        contented_potmsgset = self.factory.makePOTMsgSet(
-            self.trunk_template, singular='snut', sequence=2)
-        contented_message = self._makeTranslationMessage(
-            self.trunk_pofile, contented_potmsgset, 'druf', False)
-        self.assertEqual(translator, contented_message.submitter)
-        poft = poftset.getForPersonPOFile(translator, self.trunk_pofile)
-        self.assertEqual(poft.latest_message, contented_message)
-
-        # Then there's the pair of POTMsgSets that are identical between
-        # trunk and stable.  This one is translated only in stable.
-        # Merging will transfer that TranslationMessage from
-        # stable to trunk (where it becomes the shared message) through
-        # direct manipulation of TranslationTemplateItem.
-        stable_message = self._makeTranslationMessage(
-                self.stable_pofile, self.stable_potmsgset, 'fulb', False)
-        self.assertEqual(
-            stable_message.submitter, contented_message.submitter)
-
-        stable_message = removeSecurityProxy(stable_message)
-
-        # As it happens, this message is more recent than the happy one.
-        # This doesn't matter except it makes our test more predictable.
-        stable_message.date_created += timedelta(0, 0, 1)
-        poft = poftset.getForPersonPOFile(translator, self.stable_pofile)
-        self.assertEqual(poft.latest_message, stable_message)
-        removeSecurityProxy(poft).date_last_touched = (
-            stable_message.date_created)
-
-        # Now the migration script runs.  This also carries the
-        # POFileTranslator record for stable_message into trunk_pofile.
-        # The one for contented_message disappears in the process.
-        self.merger.mergePOTMsgSets()
-        self.merger.mergeTranslationMessages()
-
-        poft = poftset.getForPersonPOFile(translator, self.trunk_pofile)
-        self.assertEqual(poft.latest_message, stable_message)
-
 
 class TestRemoveDuplicates(TestCaseWithFactory, TranslatedProductMixin):
     """Test _scrubPOTMsgSetTranslations and friends."""

=== modified file 'lib/lp/translations/translationmerger.py'
--- lib/lp/translations/translationmerger.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/translationmerger.py	2012-05-08 04:45:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -56,7 +56,7 @@
         potmsgset.context)
 
 
-def merge_pofiletranslators(from_potmsgset, to_template):
+def merge_pofiletranslators(from_template, to_template):
     """Merge POFileTranslator entries from one template into another.
     """
     # Import here to avoid circular import.
@@ -64,7 +64,7 @@
         IPOFileTranslatorSet)
 
     pofiletranslatorset = getUtility(IPOFileTranslatorSet)
-    affected_rows = pofiletranslatorset.getForPOTMsgSet(from_potmsgset)
+    affected_rows = pofiletranslatorset.getForTemplate(from_template)
     for pofiletranslator in affected_rows:
         person = pofiletranslator.person
         from_pofile = pofiletranslator.pofile
@@ -112,7 +112,7 @@
             item.potmsgset = representative
             templates.add(item.potemplate)
 
-        merge_pofiletranslators(item.potmsgset, representative_template)
+        merge_pofiletranslators(item.potemplate, representative_template)
 
 
 def filter_clashes(clashing_ubuntu, clashing_upstream, twin):