launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-867411 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #867411 in Launchpad itself: "Evolution translations are not being imported into Launchpad"
  https://bugs.launchpad.net/launchpad/+bug/867411

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-867411/+merge/78270

= Summary =

The translations import auto-approver cannot approve imports if there
are multiple templates with the same directory portion of the path.
However, templates that are marked as obsolete should not be
considered.

== Proposed fix ==

Only consider templates marked iscurrent=True.

== Pre-implementation notes ==

Talks with Danilo and David Planella.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.translations -t TestTranslationsImportApproval

== Demo and Q/A ==

QA may be tricky.  Will talk to Danilo to put together a plan *before*
landing.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/model/translationimportqueue.py
  lib/lp/translations/scripts/tests/test_translations_approval.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-867411/+merge/78270
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-867411 into lp:launchpad.

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2011-09-15 11:35:28 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-10-05 15:32:28 +0000
@@ -270,7 +270,8 @@
         subset = potemplateset.getSubset(
             distroseries=self.distroseries,
             sourcepackagename=self.sourcepackagename,
-            productseries=self.productseries)
+            productseries=self.productseries,
+            iscurrent=True)
         entry_dirname = os.path.dirname(self.path)
         guessed_potemplate = None
         for potemplate in subset:
@@ -295,7 +296,7 @@
             return None
 
         # We have a winner, but to be 100% sure, we should not have
-        # a template file pending of being imported in our queue.
+        # a template file pending or being imported in our queue.
         if self.productseries is None:
             target = self.sourcepackage
         else:

=== added file 'lib/lp/translations/scripts/tests/test_translations_approval.py'
--- lib/lp/translations/scripts/tests/test_translations_approval.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_translations_approval.py	2011-10-05 15:32:28 +0000
@@ -0,0 +1,72 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import logging
+import transaction
+
+from canonical.testing.layers import LaunchpadScriptLayer
+from lp.testing import TestCaseWithFactory
+from lp.translations.enums import RosettaImportStatus
+from lp.translations.model.translationimportqueue import (
+    TranslationImportQueue,
+    )
+from lp.translations.scripts.import_queue_gardener import ImportQueueGardener
+
+
+class TestTranslationsImportApproval(TestCaseWithFactory):
+
+    layer = LaunchpadScriptLayer
+
+    def setUp(self):
+        super(TestTranslationsImportApproval, self).setUp()
+        self.queue = TranslationImportQueue()
+        self.script = ImportQueueGardener(
+            'translations-import-queue-gardener',
+            dbuser='translations_import_queue_gardener',
+            test_args=[])
+        self.script.logger.setLevel(logging.FATAL)
+        self.owner = self.factory.makePerson()
+        self.productseries = self.factory.makeProductSeries()
+
+    def test_templates_with_unique_directories_are_approved(self):
+        # If there are multiple templates with unique directories then the
+        # approval is ok.
+
+        # Make two valid templates with different directories.
+        self.factory.makePOTemplate(
+            path='po/evolution-3.2.pot',
+            productseries=self.productseries)
+        self.factory.makePOTemplate(
+            path='other-po/evolution-3.0.pot',
+            productseries=self.productseries)
+        tiqe = self.factory.makeTranslationImportQueueEntry(
+            path='po/fr.po', productseries=self.productseries)
+        transaction.commit()
+        self.assertIsNone(tiqe.import_into)
+        self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, tiqe.status)
+        self.script.main()
+        self.assertIsNotNone(tiqe.import_into)
+        self.assertEqual(RosettaImportStatus.APPROVED, tiqe.status)
+
+    def test_inactive_templates_do_not_block_approval(self):
+        # If all but one of the templates with the same path directory are
+        # marked as obsolete, the approval proceeds.
+        # See bug 867411 for more details.
+
+        # Make a valid template.
+        self.factory.makePOTemplate(
+            path='po/evolution-3.2.pot',
+            productseries=self.productseries)
+        # Make a obsolete template with the same directory.
+        self.factory.makePOTemplate(
+            path='po/evolution-3.0.pot',
+            productseries=self.productseries,
+            iscurrent=False)
+        tiqe = self.factory.makeTranslationImportQueueEntry(
+            path='po/fr.po', productseries=self.productseries)
+        transaction.commit()
+        self.assertIsNone(tiqe.import_into)
+        self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, tiqe.status)
+        self.script.main()
+        self.assertIsNotNone(tiqe.import_into)
+        self.assertEqual(RosettaImportStatus.APPROVED, tiqe.status)