Merge lp:~jtv/launchpad/bug-590688 into lp:launchpad

[Jeroen T. Vermeulen]

Missing database privilege for translations-export-to-branch script.

= Bug 590688 =

The fix for bug 409686 involves sending an email notification to the owners of any branches that are set up to have translations exported to them, but have never been created in codehosting. (This happens when a user goes through the "register a branch" process in the UI but fails to push a branch to the newly reserved location). For various reasons we have discussed elsewhere, just creating an empty branch is not the right thing to do.

In composing the notification email, the fix for bug 409686 accessed a Branch property that triggers a SELECT on the SeriesSourcePackageBranch table. The script runs under a database user that has no select privileges on that table, and so it fails with a privilege error. This branch applies the missing privilege, and extends the unit test to cover the new code path under a realistic database user identity.

I'm also adding a becomeDbUser method to TestCase, to encourage more realistic testing, and updating an older test to make use of TestCaseWithFactory instead of unittest.TestCase.

To test:
{{{
./bin/test -vv -t translations_to_branch -t test_autoapproval
}}}

To Q/A, see that translations export to branch starts working again on staging. Since none of the branches set up for translations export in the staging database normally have their code hosted on staging (the database being a copy of the production database), all runs of translations-export-to-branch.py on staging fail with this privilege error.

Jeroen

[Henning Eggers (henninge)]

<henninge> jtv: I wonder if the method should not be called "switchDbUser" like the Layer function.
<jtv> henninge: I wanted this to be "attractive" and have a name that doesn't say "you probably don't need to know about this." At the same time, this doesn't just switch the db user but also commits because otherwise the switch doesn't make much sense. So naming it after one part of the task would be misleading.
<henninge> I had a feeling you'd say something like that ... ;)
<jtv> ah :)
<henninge> jtv: I am not sure I go a long with your assesment of "attractive" method names but it's not all that important, I guess.

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Thanks. A better name would be welcome, of course.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-05-27 19:12:43 +0000
+++ database/schema/security.cfg	2010-06-07 10:42:25 +0000
@@ -1394,6 +1394,7 @@
 public.potranslation                    = SELECT
 public.product                          = SELECT
 public.productseries                    = SELECT
+public.seriessourcepackagebranch        = SELECT
 public.translationmessage               = SELECT
 public.translationtemplateitem          = SELECT
 

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-05-28 23:06:25 +0000
+++ lib/lp/testing/__init__.py	2010-06-07 10:42:25 +0000
@@ -220,6 +220,17 @@
 
 class TestCase(testtools.TestCase):
     """Provide Launchpad-specific test facilities."""
+    def becomeDbUser(self, dbuser):
+        """Commit, then log into the database as `dbuser`.
+        
+        For this to work, the test must run in a layer.
+        
+        Try to test every code path at least once under a realistic db
+        user, or you'll hit privilege violations later on.
+        """
+        assert self.layer, "becomeDbUser requires a layer."
+        transaction.commit()
+        self.layer.switchDbUser(dbuser)
 
     def installFixture(self, fixture):
         """Install 'fixture', an object that has a `setUp` and `tearDown`.
@@ -437,6 +448,7 @@
             self._unfoldEmailHeader(expected),
             self._unfoldEmailHeader(observed))
 
+
 class TestCaseWithFactory(TestCase):
 
     def setUp(self, user=ANONYMOUS):

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-05-27 14:20:42 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-06-07 10:42:25 +0000
@@ -139,12 +139,15 @@
     def test_exportToBranches_handles_nonascii_exceptions(self):
         # There's an exception handler in _exportToBranches that must
         # cope well with non-ASCII exception strings.
+        productseries = self.factory.makeProductSeries()
         exporter = ExportTranslationsToBranch(test_args=[])
         exporter.logger = QuietFakeLogger()
         boom = u'\u2639'
         exporter._exportToBranch = FakeMethod(failure=GruesomeException(boom))
 
-        exporter._exportToBranches([self.factory.makeProductSeries()])
+        self.becomeDbUser('translationstobranch')
+
+        exporter._exportToBranches([productseries])
 
         self.assertEqual(1, exporter._exportToBranch.call_count)
 
@@ -162,6 +165,8 @@
         productseries = self.factory.makeProductSeries()
         productseries.translations_branch = self.factory.makeBranch()
 
+        self.becomeDbUser('translationstobranch')
+
         # _handleUnpushedBranch is called if _exportToBranch raises
         # NotBranchError.
         exporter._handleUnpushedBranch = FakeMethod()
@@ -192,6 +197,8 @@
         exporter._exportToBranch = FakeMethod(failure=NotBranchError("Ow"))
         exporter._sendMail = FakeMethod()
 
+        self.becomeDbUser('translationstobranch')
+
         exporter._exportToBranches([productseries])
 
         self.assertEqual(1, exporter._sendMail.call_count)
@@ -208,6 +215,26 @@
         self.assertIn(productseries.translations_branch.bzr_identity, text)
         self.assertIn('bzr push lp://', text)
 
+    def test_handleUnpushedBranch_has_required_privileges(self):
+        # Dealing with an unpushed branch is a special code path that
+        # was not exercised by the full-script test.  Ensure that it has
+        # the database privileges that it requires.
+        exporter = ExportTranslationsToBranch(test_args=[])
+        exporter.logger = QuietFakeLogger()
+        productseries = self.factory.makeProductSeries()
+        email = self.factory.getUniqueEmailAddress()
+        branch_owner = self.factory.makePerson(email=email)
+        productseries.translations_branch = self.factory.makeBranch(
+            owner=branch_owner)
+        exporter._exportToBranch = FakeMethod(failure=NotBranchError("Ow"))
+
+        self.becomeDbUser('translationstobranch')
+
+        exporter._handleUnpushedBranch(productseries)
+
+        # This completes successfully.
+        transaction.commit()
+
 
 class TestExportToStackedBranch(TestCaseWithFactory):
     """Test workaround for bzr bug 375013."""

=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2010-06-04 07:29:20 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2010-06-07 10:42:25 +0000
@@ -13,8 +13,7 @@
 from contextlib import contextmanager
 from datetime import datetime, timedelta
 from pytz import UTC
-import transaction
-import unittest
+from unittest import TestLoader
 
 from zope.component import getUtility
 
@@ -36,7 +35,6 @@
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus, translation_import_queue_entry_age)
 from lp.testing import TestCaseWithFactory
-from lp.testing.factory import LaunchpadObjectFactory
 from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import LaunchpadZopelessLayer
 
@@ -46,31 +44,25 @@
 
     Admittedly, this might be a little over-engineered but it looks good. ;)
     """
-
-    def _become(self, dbuser):
-        """Switch to a different db user."""
-        transaction.commit()
-        self.layer.switchDbUser(dbuser)
-
     def becomeTheGardener(self):
         """One-way method to avoid unnecessary switch back."""
-        self._become('translations_import_queue_gardener')
+        self.becomeDbUser('translations_import_queue_gardener')
 
     @contextmanager
     def beingTheGardener(self):
         """Context manager to restore the launchpad user."""
-        self._become('translations_import_queue_gardener')
+        self.becomeTheGardener()
         yield
-        self._become('launchpad')
-
-
-class TestCustomLanguageCode(unittest.TestCase):
+        self.becomeDbUser('launchpad')
+
+
+class TestCustomLanguageCode(TestCaseWithFactory):
     """Unit tests for `CustomLanguageCode`."""
 
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        self.factory = LaunchpadObjectFactory()
+        super(TestCustomLanguageCode, self).setUp()
         self.product_codes = {}
         self.package_codes = {}
 
@@ -149,8 +141,8 @@
         self.assertEqual(Brazilian_code.language, Language.byCode('pt_BR'))
 
 
-class TestGuessPOFileCustomLanguageCode(
-    unittest.TestCase, GardenerDbUserMixin):
+class TestGuessPOFileCustomLanguageCode(TestCaseWithFactory,
+                                        GardenerDbUserMixin):
     """Test interaction with `TranslationImportQueueEntry.getGuessedPOFile`.
 
     Auto-approval of translation files, i.e. figuring out which existing
@@ -163,7 +155,7 @@
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        self.factory = LaunchpadObjectFactory()
+        super(TestGuessPOFileCustomLanguageCode, self).setUp()
         self.product = self.factory.makeProduct()
         self.series = self.factory.makeSeries(product=self.product)
         self.queue = TranslationImportQueue()
@@ -293,12 +285,12 @@
         self.assertEqual(zh_TW_entry.getGuessedPOFile(), zh_CN_file)
 
 
-class TestTemplateGuess(unittest.TestCase, GardenerDbUserMixin):
+class TestTemplateGuess(TestCaseWithFactory, GardenerDbUserMixin):
     """Test auto-approval's attempts to find the right template."""
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        self.factory = LaunchpadObjectFactory()
+        super(TestTemplateGuess, self).setUp()
         self.templateset = POTemplateSet()
 
     def _setUpProduct(self):
@@ -596,7 +588,7 @@
         self.assertEqual(template, entry.guessed_potemplate)
 
 
-class TestKdePOFileGuess(unittest.TestCase, GardenerDbUserMixin):
+class TestKdePOFileGuess(TestCaseWithFactory, GardenerDbUserMixin):
     """Test auto-approval's `POFile` guessing for KDE uploads.
 
     KDE has an unusual setup that the approver recognizes as a special
@@ -609,10 +601,10 @@
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
-        factory = LaunchpadObjectFactory()
+        super(TestKdePOFileGuess, self).setUp()
         self.queue = TranslationImportQueue()
 
-        self.distroseries = factory.makeDistroRelease()
+        self.distroseries = self.factory.makeDistroRelease()
 
         # For each of KDE3 and KDE4, set up:
         #  a translation package following that KDE's naming pattern,
@@ -621,7 +613,7 @@
         #  a translation file into a language we'll test in.
         self.kde_i18n_ca = SourcePackageNameSet().new('kde-i18n-ca')
         kde3_package = SourcePackageNameSet().new('kde3')
-        ca_template = factory.makePOTemplate(
+        ca_template = self.factory.makePOTemplate(
             distroseries=self.distroseries,
             sourcepackagename=kde3_package, name='kde3',
             translation_domain='kde3')
@@ -629,7 +621,7 @@
 
         self.kde_l10n_nl = SourcePackageNameSet().new('kde-l10n-nl')
         kde4_package = SourcePackageNameSet().new('kde4')
-        nl_template = factory.makePOTemplate(
+        nl_template = self.factory.makePOTemplate(
             distroseries=self.distroseries,
             sourcepackagename=kde4_package, name='kde4',
             translation_domain='kde4')
@@ -925,4 +917,4 @@
 
 
 def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+    return TestLoader().loadTestsFromName(__name__)