Merge lp:~jtv/launchpad/son-of-bug-487447 into lp:launchpad

[Jeroen T. Vermeulen]

Fixed regressed permissions problem for branch approver.

[Jeroen T. Vermeulen (jtv)]

= Bug 487447 (regression) =

This bug, a missing database permission, just regressed. It's a bit hard to maintain complete test coverage because the same permissions are to an extent shared by two scripts, each with its own database user. The original fix unified those permissions; before that, they were duplicated. In this case, one of the two scripts had the needed privilege but the other one didn't.

Unfortunately I found no test that covered this from the right angle (database permissions for the branch approver), so I extended the existing unit test to switch database identities; and then I added a test for the part that breaks. It reproduces the bug as expected.

Some private methods in the test violate our naming standards (which mandate dromedaryCase instead of lower_case_with_underscores) but for the sake of brevity I decided not to touch them. If you so desire, dear reviewer, this can still be changed.

To Q/A, create a product with a template in one series; the template should have its base filename as its name and domain. Make some translations to ensure that it has POFiles. Then set up bzr imports for another release series, and commit a template with the same filename as the first one. Within an hour after pushing that branch, the template you pushed there should show up as Approved in the project's import queue.

Test:
{{{
./bin/test -vv -t translationbranchapprover
}}}

No lint.

Jeroen

[Henning Eggers (henninge)]

Thanks for the fix, Jeroen. I shan't be picky about the naming of private methods today.

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2009-12-14 15:30:33 +0000
+++ database/schema/security.cfg	2009-12-22 10:26:15 +0000
@@ -1388,6 +1388,7 @@
 public.teamparticipation                = SELECT
 public.translationgroup                 = SELECT
 public.translationimportqueueentry      = SELECT, UPDATE
+public.translationmessage               = SELECT
 public.translationrelicensingagreement  = SELECT
 public.translationtemplateitem          = SELECT
 public.translator                       = SELECT

=== modified file 'lib/lp/translations/tests/test_translationbranchapprover.py'
--- lib/lp/translations/tests/test_translationbranchapprover.py	2009-11-28 15:31:52 +0000
+++ lib/lp/translations/tests/test_translationbranchapprover.py	2009-12-22 10:26:15 +0000
@@ -5,7 +5,9 @@
 
 __metaclass__ = type
 
+import transaction
 from unittest import TestLoader
+
 from zope.component import getUtility
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
@@ -17,6 +19,13 @@
 from lp.translations.model.translationbranchapprover import (
     TranslationBranchApprover)
 
+
+def become_the_approver(layer):
+    """Switch to the branch-approver's database user identity."""
+    transaction.commit()
+    layer.switchDbUser('translationsbranchscanner')
+
+
 class TestTranslationBranchApprover(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
@@ -32,10 +41,12 @@
             self.factory.getUniqueString(), True, self.series.owner,
             productseries=self.series)
 
-    def _create_template(self, path, domain):
+    def _create_template(self, path, domain, productseries=None):
         # Create a template in the database
+        if productseries is None:
+            productseries = self.series
         return self.factory.makePOTemplate(
-            productseries=self.series,
+            productseries=productseries,
             name=domain.replace('_', '-'),
             translation_domain=domain,
             path=path)
@@ -53,6 +64,8 @@
         entry = self._upload_file(template_path)
         self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, entry.status)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.APPROVED, entry.status)
 
@@ -62,6 +75,8 @@
         template_path = u'messages.pot'
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, entry.status)
 
@@ -70,6 +85,8 @@
         path = u'eo.po'
         entry = self._upload_file(path)
         approver = self._create_approver(path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, entry.status)
 
@@ -80,6 +97,8 @@
         template_path = translation_domain + u'.pot'
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(
             translation_domain, entry.potemplate.translation_domain)
@@ -90,6 +109,8 @@
         template_path = translation_domain + '/en-US.xpi'
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(
             translation_domain, entry.potemplate.translation_domain)
@@ -100,6 +121,8 @@
         template_path = translation_domain + u'.pot'
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertTrue(valid_name(entry.potemplate.name))
         self.assertEqual(u'invalid-name-withillegalcharacters',
@@ -112,6 +135,8 @@
         self._create_template(template_path, translation_domain)
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.APPROVED, entry.status)
 
@@ -123,6 +148,8 @@
         potemplate = self._create_template(template_path, translation_domain)
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(potemplate, entry.potemplate)
 
@@ -135,6 +162,8 @@
         template_path = self.factory.getUniqueString() + u'.pot'
         entry = self._upload_file(template_path)
         approver = self._create_approver(template_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.APPROVED, entry.status)
         self.assertEqual(potemplate, entry.potemplate)
@@ -148,6 +177,8 @@
         potemplate = self._create_template(generic_path, translation_domain)
         entry = self._upload_file(generic_path)
         approver = self._create_approver(generic_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.APPROVED, entry.status)
 
@@ -159,6 +190,8 @@
         potemplate = self._create_template(generic_path, translation_domain)
         entry = self._upload_file(generic_path)
         approver = self._create_approver(generic_path)
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(
             translation_domain, entry.potemplate.translation_domain)
@@ -173,6 +206,8 @@
         new_path = u"%s/%s.pot" % (new_domain, new_domain)
         entry = self._upload_file(new_path)
         approver = self._create_approver((existing_path, new_path))
+
+        become_the_approver(self.layer)
         approver.approve(entry)
         self.assertEqual(RosettaImportStatus.APPROVED, entry.status)
         self.assertEqual(new_domain, entry.potemplate.translation_domain)
@@ -185,6 +220,8 @@
         entry1 = self._upload_file(pot_path1)
         entry2 = self._upload_file(pot_path2)
         approver = self._create_approver((pot_path1, pot_path2))
+
+        become_the_approver(self.layer)
         approver.approve(entry1)
         self.assertEqual(RosettaImportStatus.APPROVED, entry1.status)
         approver.approve(entry2)
@@ -198,6 +235,8 @@
         entry1 = self._upload_file(pot_path1)
         entry2 = self._upload_file(pot_path2)
         approver = self._create_approver((pot_path1, pot_path2))
+
+        become_the_approver(self.layer)
         approver.approve(entry1)
         self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, entry1.status)
         approver.approve(entry2)
@@ -205,7 +244,7 @@
 
     def test_approve_only_if_needs_review(self):
         # If an entry is not in NEEDS_REVIEW state, it must not be approved.
-        pot_path = self.factory.getUniqueString()+".pot"
+        pot_path = self.factory.getUniqueString() + ".pot"
         entry = self._upload_file(pot_path)
         entry.potemplate = self.factory.makePOTemplate()
         not_approve_status = (
@@ -221,6 +260,33 @@
             approver.approve(entry)
             self.assertEqual(status, entry.status)
 
+    def test_approveNewSharingTemplate(self):
+        # When the approver creates a new template, the new template
+        # gets copies of any existing POFiles for templates that it will
+        # share translations with.
+        domain = self.factory.getUniqueString()
+        pot_path = domain + ".pot"
+        trunk = self.series.product.getSeries('trunk')
+        trunk_template = self._create_template(
+            pot_path, domain=domain, productseries=trunk)
+        dutch_pofile = self.factory.makePOFile(
+            'nl', potemplate=trunk_template)
+        entry = self._upload_file(pot_path)
+        approver = self._create_approver(pot_path)
+
+        become_the_approver(self.layer)
+        approver.approve(entry)
+
+        # This really did create a new template.
+        self.assertNotEqual(None, entry.potemplate)
+        self.assertNotEqual(trunk_template, entry.potemplate)
+        self.assertEqual(trunk_template.name, entry.potemplate.name)
+
+        # The new template also has a Dutch translation of its own.
+        new_dutch_pofile = entry.potemplate.getPOFileByLang('nl')
+        self.assertNotEqual(None, new_dutch_pofile)
+        self.assertNotEqual(dutch_pofile, new_dutch_pofile)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)
-