Merge lp:~danilo/launchpad/migrate-current-flags into lp:launchpad

[Данило Шеган]

Provide a script to set is_imported flag based on is_current flag for upstream projects.

= Bug 677600 =

Provide a script to set is_imported flag for all TranslationMessages on
upstream projects in Launchpad where is_current is set. This is a
script to aid with transition to the new semantics for the data model
that we are doing in our integration branch
(lp:~launchpad/launchpad/recife).

== Implementation details ==

We are using a traditional tried-and-true migration framework through
DBLoopTuner. It warrants that we won't overload the DB replication.

There are no tests for the full script run simply because this is a
script that's going to be used only for the migration (basically twice:
once before the rollout, once after it).

== Tests ==

bin/test -cvvt getProductsWithTemplates -t getCurrentNonimportedTranslations -t updateTranslationMessages

== Demo and Q/A ==

This will have to be Q/Ad on staging. Local runs are simple and
"update" two TranslationMessages total. Just do

  scripts/rosetta/migrate_current_flag.py

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/scripts/tests/test_migrate_current_flag.py
  lib/lp/translations/scripts/migrate_current_flag.py
  scripts/rosetta/migrate_current_flag.py

./scripts/rosetta/migrate_current_flag.py
       8: '_pythonpath' imported but unused

[Aaron Bentley (abentley)]

There appear to be multiple cases where this could do the wrong thing:

<jtv> The code seems to assume that there can be only one is_imported TM per POTMsgSet.
 In which case, it's perfectly valid to say "I'm doing each TM once, and for each, I'll clear any previous is_imported flags first."
 But what of another TM for the same POTMsgSet and a different language, that's already been processed?
 It's had its flag set in a previous iteration, but now gets it callously cleared again "to make room" for a TM it doesn't even compete with. AFAICT there's a similar situation with divergence (which is the case where TM.potemplate is non-null).

review: Needs Fixing

[Данило Шеган (danilo)]

У пон, 22. 11 2010. у 15:21 +0000, Aaron Bentley пише:
> Review: Needs Fixing
> There appear to be multiple cases where this could do the wrong thing:
>
> <jtv> The code seems to assume that there can be only one is_imported TM per POTMsgSet.
> In which case, it's perfectly valid to say "I'm doing each TM once, and for each, I'll clear any previous is_imported flags first."
> But what of another TM for the same POTMsgSet and a different language, that's already been processed?
> It's had its flag set in a previous iteration, but now gets it callously cleared again "to make room" for a TM it doesn't even compete with. AFAICT there's a similar situation with divergence (which is the case where TM.potemplate is non-null).

Indeed, all well spotted. A few tests for the above and an
implementation that unsets only TMs that really are in the way.

Incremental diff attached.

[Aaron Bentley (abentley)]

Jtv was planning to address these issues.

"Данило Шеган" <email address hidden> wrote:

>У пон, 22. 11 2010. у 15:21 +0000, Aaron Bentley пише:
>> Review: Needs Fixing
>> There appear to be multiple cases where this could do the wrong
>thing:
>>
>> <jtv> The code seems to assume that there can be only one is_imported
>TM per POTMsgSet.
>> In which case, it's perfectly valid to say "I'm doing each TM once,
>and for each, I'll clear any previous is_imported flags first."
>> But what of another TM for the same POTMsgSet and a different
>language, that's already been processed?
>> It's had its flag set in a previous iteration, but now gets it
>callously cleared again "to make room" for a TM it doesn't even compete
>with. AFAICT there's a similar situation with divergence (which is the
>case where TM.potemplate is non-null).
>
>Indeed, all well spotted. A few tests for the above and an
>implementation that unsets only TMs that really are in the way.
>
>Incremental diff attached.
>
>
>--
>https://code.launchpad.net/~danilo/launchpad/migrate-current-flags/+merge/41364
>You are reviewing the proposed merge of
>lp:~danilo/launchpad/migrate-current-flags into lp:launchpad/devel.

--
Sent from my phone. Please excuse my brevity.

[Jeroen T. Vermeulen (jtv)]

I actually started a branch based on this review, though I saved the important bit for when I was fresh in the morning. (Which is _naturally_ also when I lost power, a hard disk, and a router—why do I even bother with an expensive UPS?)

Anyway, my branch was lp:~jtv/launchpad/migrate-current-flags.

[Jeroen T. Vermeulen (jtv)]

Does this mean that Storm translates "X.potemplateID == Y.potemplateID" to "X.potemplate IS NOT DISTINCT FROM Y.potemplate" or something equivalent? I naïvely thought it would translate to "X.potemplate = Y.potemplate."

That's another gotcha with ORMs. Having to worry about where you're getting application-language semantics and where it's SQL semantics. :/

[Данило Шеган (danilo)]

У уто, 23. 11 2010. у 07:57 +0000, Jeroen T. Vermeulen пише:
> Does this mean that Storm translates "X.potemplateID == Y.potemplateID"
> to "X.potemplate IS NOT DISTINCT FROM Y.potemplate" or something
> equivalent? I naïvely thought it would translate to "X.potemplate =
> Y.potemplate."

No, it translates it to what you expect. That's why the code is:

                Or(And(PreviousImported.potemplate == None,
                       CurrentTranslation.potemplate == None),
                   (PreviousImported.potemplateID ==
                    CurrentTranslation.potemplateID)),

Existing test ("unsetting_imported") makes sure it works for the
sharing (None) case, and a new diverged-test makes sure it works for the
mixed diverged-shared case.

A test case for diverged-diverged is missing, though I am pretty sure
that works. A test case for shared-diverged is also missing (i.e. shared
existing imported translation, diverged current translation).

> That's another gotcha with ORMs. Having to worry about where you're
> getting application-language semantics and where it's SQL semantics.
> :/

It may come as a surprise, but I still do know a few things about ORMs
(and you may remember that it was me who found out about DISTINCT FROM
in the first place, exactly because we needed it so often :).

Jeroen, also, before I continued on this, I checked if you assigned the
card or the bug to yourself. You didn't so I fixed the branch in order
to help out (and it was very easy for me).

[Jeroen T. Vermeulen (jtv)]

OK, if it's still yours, please go ahead and land it. (I don't have workable EC2 right now).

[Данило Шеган (danilo)]

It'd be nice if there was a review statement to accompany that :)

[Jeroen T. Vermeulen (jtv)]

Ah, of course. We'll see if we can procure that.

[Aaron Bentley (abentley)]

jtv's branch has some of the changes that jtv made during my on-call review on IRC. Most of these comments would not apply if you had merged it.

"from storm.expr import And, Count, Or, Select" should be formatted as a multi-line import. It's also better to use storm.locals so you don't need multiple import statements:

from storm.expr import (
    And,
    Count,
    Or,
    Select,
    )

Similarly:

from lp.translations.scripts.migrate_current_flag import (
    MigrateCurrentFlagProcess)

Should be:

from lp.translations.scripts.migrate_current_flag import (
    MigrateCurrentFlagProcess,
)

I also asked for a docstring for _updateTranslationMessages. It's hard to see exactly what it does.

Also, the docstring for MigrateTranslationFlags looks inaccurate (copy & paste).

None of these affect actual functionality, so I won't block landing, but I'd appreciate if you could fix them at some point.

review: Approve

=== added file 'lib/lp/translations/scripts/migrate_current_flag.py'
--- lib/lp/translations/scripts/migrate_current_flag.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/migrate_current_flag.py	2010-11-22 23:19:36 +0000
@@ -0,0 +1,161 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Set 'is_imported' flag from 'is_current' for upstream projects."""
+
+__metaclass__ = type
+__all__ = ['MigrateCurrentFlagProcess']
+
+import logging
+
+from zope.component import getUtility
+from zope.interface import implements
+
+from storm.info import ClassAlias
+from storm.expr import And, Count, Or, Select
+
+from canonical.launchpad.interfaces.looptuner import ITunableLoop
+from canonical.launchpad.utilities.looptuner import DBLoopTuner
+from canonical.launchpad.webapp.interfaces import (
+    IStoreSelector,
+    MAIN_STORE,
+    MASTER_FLAVOR,
+    )
+from lp.registry.model.product import Product
+from lp.registry.model.productseries import ProductSeries
+from lp.translations.model.potemplate import POTemplate
+from lp.translations.model.translationmessage import TranslationMessage
+from lp.translations.model.translationtemplateitem import (
+    TranslationTemplateItem,
+    )
+
+
+class TranslationMessageImportedFlagUpdater:
+    implements(ITunableLoop)
+    """Populates is_imported flag from is_current flag on translations."""
+
+    def __init__(self, transaction, logger, tm_ids):
+        self.transaction = transaction
+        self.logger = logger
+        self.start_at = 0
+
+        self.tm_ids = list(tm_ids)
+        self.total = len(self.tm_ids)
+        self.logger.info(
+            "Fixing up a total of %d TranslationMessages." % (self.total))
+        self.store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+
+    def isDone(self):
+        """See `ITunableLoop`."""
+        # When the main loop hits the end of the list of objects,
+        # it sets start_at to None.
+        return self.start_at is None
+
+    def getNextBatch(self, chunk_size):
+        """Return a batch of objects to work with."""
+        end_at = self.start_at + int(chunk_size)
+        self.logger.debug(
+            "Getting translations[%d:%d]..." % (self.start_at, end_at))
+        return self.tm_ids[self.start_at: end_at]
+
+    def _updateTranslationMessages(self, tm_ids):
+        # Unset imported messages that might be in the way.
+        PreviousImported = ClassAlias(
+            TranslationMessage, 'PreviousImported')
+        CurrentTranslation = ClassAlias(
+            TranslationMessage, 'CurrentTranslation')
+        previous_imported_select = Select(
+            PreviousImported.id,
+            tables=[PreviousImported, CurrentTranslation],
+            where=And(
+                PreviousImported.is_imported == True,
+                (PreviousImported.potmsgsetID ==
+                 CurrentTranslation.potmsgsetID),
+                Or(And(PreviousImported.potemplate == None,
+                       CurrentTranslation.potemplate == None),
+                   (PreviousImported.potemplateID ==
+                    CurrentTranslation.potemplateID)),
+                PreviousImported.languageID == CurrentTranslation.languageID,
+                CurrentTranslation.id.is_in(tm_ids)))
+
+        previous_imported = self.store.find(
+            TranslationMessage,
+            TranslationMessage.id.is_in(previous_imported_select))
+        previous_imported.set(is_imported=False)
+        translations = self.store.find(
+            TranslationMessage,
+            TranslationMessage.id.is_in(tm_ids))
+        translations.set(is_imported=True)
+
+    def __call__(self, chunk_size):
+        """See `ITunableLoop`.
+
+        Retrieve a batch of TranslationMessages in ascending id order,
+        and set is_imported flag to True on all of them.
+        """
+        tm_ids = self.getNextBatch(chunk_size)
+
+        if len(tm_ids) == 0:
+            self.start_at = None
+        else:
+            self._updateTranslationMessages(tm_ids)
+            self.transaction.commit()
+            self.transaction.begin()
+
+            self.start_at += len(tm_ids)
+            self.logger.info("Processed %d/%d TranslationMessages." % (
+                self.start_at, self.total))
+
+
+class MigrateCurrentFlagProcess:
+    """Mark all translations as is_imported if they are is_current.
+
+    Processes only translations for upstream projects, since Ubuntu
+    source packages need no migration.
+    """
+
+    def __init__(self, transaction, logger=None):
+        self.transaction = transaction
+        self.logger = logger
+        if logger is None:
+            self.logger = logging.getLogger("migrate-current-flag")
+        self.store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
+
+    def getProductsWithTemplates(self):
+        """Get Product.ids for projects with any translations templates."""
+        return self.store.find(
+            Product,
+            POTemplate.productseriesID == ProductSeries.id,
+            ProductSeries.productID == Product.id,
+            ).group_by(Product).having(Count(POTemplate.id) > 0)
+
+    def getCurrentNonimportedTranslations(self, product):
+        """Get TranslationMessage.ids that need migration for a `product`."""
+        return self.store.find(
+            TranslationMessage.id,
+            TranslationMessage.is_current == True,
+            TranslationMessage.is_imported == False,
+            (TranslationMessage.potmsgsetID ==
+             TranslationTemplateItem.potmsgsetID),
+            TranslationTemplateItem.potemplateID == POTemplate.id,
+            POTemplate.productseriesID == ProductSeries.id,
+            ProductSeries.productID == product.id).config(distinct=True)
+
+    def run(self):
+        products_with_templates = list(self.getProductsWithTemplates())
+        total_products = len(products_with_templates)
+        if total_products == 0:
+            self.logger.info("Nothing to do.")
+        current_product = 0
+        for product in products_with_templates:
+            current_product += 1
+            self.logger.info(
+                "Migrating %s translations (%d of %d)..." % (
+                    product.name, current_product, total_products))
+
+            tm_ids = self.getCurrentNonimportedTranslations(product)
+            tm_loop = TranslationMessageImportedFlagUpdater(
+                self.transaction, self.logger, tm_ids)
+            DBLoopTuner(tm_loop, 5, minimum_chunk_size=100).run()
+
+        self.logger.info("Done.")

=== added file 'lib/lp/translations/scripts/tests/test_migrate_current_flag.py'
--- lib/lp/translations/scripts/tests/test_migrate_current_flag.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_migrate_current_flag.py	2010-11-22 23:19:36 +0000
@@ -0,0 +1,183 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import logging
+
+from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.testing import TestCaseWithFactory
+from lp.translations.scripts.migrate_current_flag import (
+    MigrateCurrentFlagProcess,
+    TranslationMessageImportedFlagUpdater,
+    )
+
+
+class TestMigrateCurrentFlag(TestCaseWithFactory):
+    """Test current-flag migration script."""
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        # This test needs the privileges of rosettaadmin (to update
+        # TranslationMessages) but it also needs to set up test conditions
+        # which requires other privileges.
+        self.layer.switchDbUser('postgres')
+        super(TestMigrateCurrentFlag, self).setUp(user='[e-mail]')
+        self.migrate_process = MigrateCurrentFlagProcess(self.layer.txn)
+
+    def test_getProductsWithTemplates_sampledata(self):
+        # Sample data already has 3 products with templates.
+        sampledata_products = list(
+            self.migrate_process.getProductsWithTemplates())
+        self.assertEquals(3, len(sampledata_products))
+
+    def test_getProductsWithTemplates_noop(self):
+        # Adding a product with no templates doesn't change anything.
+        sampledata_products = list(
+            self.migrate_process.getProductsWithTemplates())
+        self.factory.makeProduct()
+        products = self.migrate_process.getProductsWithTemplates()
+        self.assertContentEqual(sampledata_products, list(products))
+
+    def test_getProductsWithTemplates_new_template(self):
+        # A new product with a template is included.
+        sampledata_products = list(
+            self.migrate_process.getProductsWithTemplates())
+        product = self.factory.makeProduct()
+        self.factory.makePOTemplate(productseries=product.development_focus)
+        products = self.migrate_process.getProductsWithTemplates()
+        self.assertContentEqual(
+            sampledata_products + [product], list(products))
+
+    def test_getCurrentNonimportedTranslations_empty(self):
+        # For a product with no translations no messages are returned.
+        potemplate = self.factory.makePOTemplate()
+        results = list(
+            self.migrate_process.getCurrentNonimportedTranslations(
+                potemplate.productseries.product))
+        self.assertContentEqual([], results)
+
+    def test_getCurrentNonimportedTranslations_noncurrent(self):
+        # For a product with non-current translations no messages
+        # are returned.
+        potemplate = self.factory.makePOTemplate()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=potemplate,
+            sequence=1)
+        pofile = self.factory.makePOFile(potemplate=potemplate)
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, suggestion=True)
+        results = list(
+            self.migrate_process.getCurrentNonimportedTranslations(
+                potemplate.productseries.product))
+        self.assertContentEqual([], results)
+
+    def test_getCurrentNonimportedTranslations_current_imported(self):
+        # For a product with current, imported translations no messages
+        # are returned.
+        potemplate = self.factory.makePOTemplate()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=potemplate,
+            sequence=1)
+        pofile = self.factory.makePOFile(potemplate=potemplate)
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, is_imported=True)
+        results = list(
+            self.migrate_process.getCurrentNonimportedTranslations(
+                potemplate.productseries.product))
+        self.assertContentEqual([], results)
+
+    def test_getCurrentNonimportedTranslations_current_nonimported(self):
+        # For a product with current, non-imported translations,
+        # that translation is returned.
+        potemplate = self.factory.makePOTemplate()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=potemplate,
+            sequence=1)
+        pofile = self.factory.makePOFile(potemplate=potemplate)
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, potmsgset=potmsgset, is_imported=False)
+        results = list(
+            self.migrate_process.getCurrentNonimportedTranslations(
+                potemplate.productseries.product))
+        self.assertContentEqual([translation.id], results)
+
+
+class TestUpdaterLoop(TestCaseWithFactory):
+    """Test updater-loop core functionality."""
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        # This test needs the privileges of rosettaadmin (to update
+        # TranslationMessages) but it also needs to set up test conditions
+        # which requires other privileges.
+        self.layer.switchDbUser('postgres')
+        super(TestUpdaterLoop, self).setUp(user='[e-mail]')
+        self.logger = logging.getLogger("migrate-current-flag")
+        self.migrate_loop = TranslationMessageImportedFlagUpdater(
+            self.layer.txn, self.logger, [])
+
+    def test_updateTranslationMessages_base(self):
+        # Passing in a TranslationMessage.id sets is_imported flag
+        # on that message even if it was not set before.
+        translation = self.factory.makeTranslationMessage()
+        self.assertFalse(translation.is_imported)
+
+        self.migrate_loop._updateTranslationMessages([translation.id])
+        self.assertTrue(translation.is_imported)
+
+    def test_updateTranslationMessages_unsetting_imported(self):
+        # If there was a previous imported message, it is unset
+        # first.
+        pofile = self.factory.makePOFile()
+        imported = self.factory.makeTranslationMessage(
+            pofile=pofile, is_imported=True)
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, potmsgset=imported.potmsgset, is_imported=False)
+        self.assertTrue(imported.is_imported)
+        self.assertFalse(imported.is_current)
+        self.assertFalse(translation.is_imported)
+        self.assertTrue(translation.is_current)
+
+        self.migrate_loop._updateTranslationMessages([translation.id])
+        self.assertFalse(imported.is_imported)
+        self.assertTrue(translation.is_imported)
+        self.assertTrue(translation.is_current)
+
+    def test_updateTranslationMessages_other_language(self):
+        # If there was a previous imported message in another language
+        # it is not unset.
+        pofile = self.factory.makePOFile()
+        pofile_other = self.factory.makePOFile(potemplate=pofile.potemplate)
+        imported = self.factory.makeTranslationMessage(
+            pofile=pofile_other, is_imported=True)
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, potmsgset=imported.potmsgset, is_imported=False)
+        self.assertTrue(imported.is_imported)
+        self.assertTrue(imported.is_current)
+        self.assertFalse(translation.is_imported)
+        self.assertTrue(translation.is_current)
+
+        self.migrate_loop._updateTranslationMessages([translation.id])
+        self.assertTrue(imported.is_imported)
+        self.assertTrue(imported.is_current)
+        self.assertTrue(translation.is_imported)
+        self.assertTrue(translation.is_current)
+
+    def test_updateTranslationMessages_diverged(self):
+        # If there was a previous diverged message, it is not
+        # touched.
+        pofile = self.factory.makePOFile()
+        translation = self.factory.makeTranslationMessage(
+            pofile=pofile, is_imported=False)
+        diverged_imported = self.factory.makeTranslationMessage(
+            pofile=pofile, force_diverged=True, is_imported=True,
+            potmsgset=translation.potmsgset)
+        self.assertEquals(pofile.potemplate, diverged_imported.potemplate)
+        self.assertTrue(diverged_imported.is_imported)
+        self.assertTrue(diverged_imported.is_current)
+
+        self.migrate_loop._updateTranslationMessages([translation.id])
+        self.assertEquals(pofile.potemplate, diverged_imported.potemplate)
+        self.assertTrue(diverged_imported.is_imported)
+        self.assertTrue(diverged_imported.is_current)

=== added file 'scripts/rosetta/migrate_current_flag.py'
--- scripts/rosetta/migrate_current_flag.py	1970-01-01 00:00:00 +0000
+++ scripts/rosetta/migrate_current_flag.py	2010-11-22 23:19:36 +0000
@@ -0,0 +1,30 @@
+#!/usr/bin/python -S
+#
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Migrate current flag to imported flag on project translations."""
+
+import _pythonpath
+
+from lp.services.scripts.base import LaunchpadScript
+from lp.translations.scripts.migrate_current_flag import (
+    MigrateCurrentFlagProcess)
+
+
+class MigrateTranslationFlags(LaunchpadScript):
+    """Go through all POFiles and TranslationMessages and get rid of variants.
+
+    Replaces use of `variant` field with a new language with the code
+    corresponding to the 'previous language'@'variant'.
+    """
+
+    def main(self):
+        fixer = MigrateCurrentFlagProcess(self.txn, self.logger)
+        fixer.run()
+
+
+if __name__ == '__main__':
+    script = MigrateTranslationFlags(
+        name="migratecurrentflag", dbuser='rosettaadmin')
+    script.lock_and_run()