Merge lp:~danilo/launchpad/bug-302449 into lp:launchpad

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

[r=bac][bug=302449] Make sure TranslationImportQueue.addOrUpdateEntry doesn't hit a DB constraint on (importer, potemplate, path, target).

= Bug 302449 =

TranslationImportQueue._getMatchingEntry logic does not match the
actual DB constraint, which leads to OOPSes (it is less restrictive than the constraint).

The problem is that it requires a 'pofile' on the TranslationImportQueueEntry table row if pofile is specified, yet the constraint is on the template/path/importer/target.

== Proposed fix ==

Switch it to storm syntax and make pofile clause an Or() with pofile==None.

Drive-by fix for link visibility for POTemplate:+admin page.

It's still possible to hit the same constraint modifying the import queue entry directly, but that's a different problem.

== Tests ==

bin/test -cvvt addOrUpdateEntry -t translationimportqueue.txt

(unfortunately, this dates back to when we did a lot of unit testing in doctests)

== Demo and Q/A ==

Upload a tarball with a translation file on the POTemplate:+upload page, and then upload identical PO file with the same path on the POFile:+upload page. It should not OOPS anymore.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/model/translationimportqueue.py
  lib/lp/testing/factory.py
  lib/lp/translations/tests/test_translationimportqueue.py
  lib/lp/translations/browser/potemplate.py

[Brad Crittenden (bac)]

Looks good Danilo, except for the lint issues you say you've already fixed.

review: Approve (code)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-09-12 07:29:31 +0000
+++ lib/lp/testing/factory.py	2011-09-12 15:16:27 +0000
@@ -306,6 +306,9 @@
     TranslationFileFormat,
     )
 from lp.translations.interfaces.translationgroup import ITranslationGroupSet
+from lp.translations.interfaces.translationimportqueue import (
+    ITranslationImportQueue,
+    )
 from lp.translations.interfaces.translationmessage import (
     RosettaTranslationOrigin,
     )
@@ -314,9 +317,6 @@
     ITranslationTemplatesBuildJobSource,
     )
 from lp.translations.interfaces.translator import ITranslatorSet
-from lp.translations.model.translationimportqueue import (
-    TranslationImportQueueEntry,
-    )
 from lp.translations.model.translationtemplateitem import (
     TranslationTemplateItem,
     )
@@ -3241,9 +3241,6 @@
 
         if content is None:
             content = self.getUniqueString()
-        content_reference = getUtility(ILibraryFileAliasSet).create(
-            name=os.path.basename(path), size=len(content),
-            file=StringIO(content), contentType='text/plain')
 
         if format is None:
             format = TranslationFileFormat.PO
@@ -3251,11 +3248,17 @@
         if status is None:
             status = RosettaImportStatus.NEEDS_REVIEW
 
-        return TranslationImportQueueEntry(
-            path=path, productseries=productseries, distroseries=distroseries,
-            sourcepackagename=sourcepackagename, importer=uploader,
-            content=content_reference, status=status, format=format,
-            by_maintainer=by_maintainer)
+        if type(content) == unicode:
+            content = content.encode('utf-8')
+
+        entry = getUtility(ITranslationImportQueue).addOrUpdateEntry(
+            path=path, content=content, by_maintainer=by_maintainer,
+            importer=uploader, productseries=productseries,
+            distroseries=distroseries, sourcepackagename=sourcepackagename,
+            potemplate=potemplate, pofile=pofile, format=format)
+        entry.setStatus(
+            status, getUtility(ILaunchpadCelebrities).rosetta_experts)
+        return entry
 
     def makeMailingList(self, team, owner):
         """Create a mailing list for the team."""

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2011-08-25 19:48:19 +0000
+++ lib/lp/translations/browser/potemplate.py	2011-09-12 15:16:27 +0000
@@ -76,6 +76,7 @@
 from lp.registry.browser.productseries import ProductSeriesFacets
 from lp.registry.browser.sourcepackage import SourcePackageFacets
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.model.packaging import Packaging
 from lp.registry.model.product import Product
@@ -928,7 +929,9 @@
             self.distroseries = series
         else:
             self.productseries = series
-        self.can_admin = check_permission('launchpad.Admin', series)
+        user = IPersonRoles(self.user, None)
+        self.can_admin = (user is not None and
+                          (user.in_admin or user.in_rosetta_experts))
         self.can_edit = (
             self.can_admin or
             check_permission('launchpad.TranslationsAdmin', series))

=== modified file 'lib/lp/translations/doc/sourcepackagerelease-translations.txt'
--- lib/lp/translations/doc/sourcepackagerelease-translations.txt	2011-05-27 19:53:20 +0000
+++ lib/lp/translations/doc/sourcepackagerelease-translations.txt	2011-09-12 15:16:27 +0000
@@ -65,7 +65,7 @@
 
     >>> spr_test.attachTranslationFiles(restricted_translation, True, katie)
 
-And the queue should have 2 entries, which exactly the same contents.
+And the queue should have 2 entries, with exactly the same contents.
 
     >>> queue_entries = translation_import_queue.getAllEntries(
     ...     target=spr_test.sourcepackage)
@@ -78,7 +78,7 @@
     something/en-US.xpi  katie
     po/es.po             katie
 
-Commit, so the uploaded traslations become available to the scripts.
+Commit, so the uploaded translations become available to the scripts.
 
     >>> transaction.commit()
 

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2011-07-26 11:22:15 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-09-12 15:16:27 +0000
@@ -835,45 +835,43 @@
         :return: The matching entry or None, if no matching entry was found
             at all."""
 
-        # Find possible candidates by querying the database.
-        queries = ['TranslationImportQueueEntry.path = %s' % sqlvalues(path)]
-        queries.append(
-            'TranslationImportQueueEntry.importer = %s' % sqlvalues(importer))
-        # Depending on how specific the new entry is, potemplate and pofile
-        # may be None.
+        # We disallow entries with the identical path, importer, potemplate
+        # and target (eg. productseries or distroseries/sourcepackagename).
+        clauses = [
+            TranslationImportQueueEntry.path == path,
+            TranslationImportQueueEntry.importer == importer,
+            ]
         if potemplate is not None:
-            queries.append(
-                'TranslationImportQueueEntry.potemplate = %s' % sqlvalues(
-                    potemplate))
+            clauses.append(
+                TranslationImportQueueEntry.potemplate == potemplate)
         if pofile is not None:
-            queries.append(
-                'TranslationImportQueueEntry.pofile = %s' % sqlvalues(pofile))
-        if sourcepackagename is not None:
-            # The import is related with a sourcepackage and a distribution.
-            queries.append(
-                'TranslationImportQueueEntry.sourcepackagename = %s' % (
-                    sqlvalues(sourcepackagename)))
-            queries.append(
-                'TranslationImportQueueEntry.distroseries = %s' % sqlvalues(
-                    distroseries))
+            clauses.append(Or(
+                TranslationImportQueueEntry.pofile == pofile,
+                TranslationImportQueueEntry.pofile == None))
+        if productseries is None:
+            assert sourcepackagename is not None and distroseries is not None
+            clauses.extend([
+                (TranslationImportQueueEntry.distroseries_id ==
+                 distroseries.id),
+                (TranslationImportQueueEntry.sourcepackagename_id ==
+                 sourcepackagename.id),
+                ])
         else:
-            # The import is related with a productseries.
-            assert productseries is not None, (
-                'sourcepackagename and productseries cannot be both None at'
-                ' the same time.')
-
-            queries.append(
-                'TranslationImportQueueEntry.productseries = %s' % sqlvalues(
-                    productseries))
-        # Order the results by level of specificity.
-        entries = TranslationImportQueueEntry.select(
-                ' AND '.join(queries),
-                orderBy="potemplate IS NULL DESC, pofile IS NULL DESC")
+            clauses.append(
+                TranslationImportQueueEntry.productseries_id ==
+                productseries.id)
+        store = IMasterStore(TranslationImportQueueEntry)
+        entries = store.find(
+            TranslationImportQueueEntry, *clauses)
+        entries = list(
+            entries.order_by(
+                ['pofile is null desc', 'potemplate is null desc']))
+        count = len(entries)
 
         # Deal with the simple cases.
-        if entries.count() == 0:
+        if count == 0:
             return None
-        if entries.count() == 1:
+        if count == 1:
             return entries[0]
 
         # Check that the top two entries differ in levels of specificity.
@@ -1209,7 +1207,7 @@
             entry.path, entry.importer, potemplate, pofile,
             entry.sourcepackagename, entry.distroseries, entry.productseries)
 
-        if existing_entry is None:
+        if existing_entry is None or existing_entry == entry:
             entry.potemplate = potemplate
             entry.pofile = pofile
 

=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2011-08-03 11:00:11 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2011-09-12 15:16:27 +0000
@@ -16,6 +16,7 @@
 
 from pytz import UTC
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp.testing import verifyObject
@@ -1115,8 +1116,9 @@
 
         target = self._copyTargetFromEntry(same_target_as)
 
-        return self.factory.makeTranslationImportQueueEntry(
-            path=path, status=status, **target)
+        return removeSecurityProxy(
+            self.factory.makeTranslationImportQueueEntry(
+                path=path, status=status, **target))
 
     def _makeTranslationEntry(self, path, status=None, same_target_as=None):
         """Create an import queue entry for a translation file.
@@ -1126,8 +1128,9 @@
         entry that may have to be blocked depending on same_target_as.
         """
         target = self._copyTargetFromEntry(same_target_as)
-        return self.factory.makeTranslationImportQueueEntry(
-            path=path, status=status, **target)
+        return removeSecurityProxy(
+            self.factory.makeTranslationImportQueueEntry(
+                path=path, status=status, **target))
 
     def test_getBlockableDirectories_checks_templates(self):
         old_blocklist = self.queue._getBlockableDirectories()
@@ -1180,7 +1183,6 @@
 
         translations = self._makeTranslationEntry(
             'de.po', same_target_as=blocked_template)
-
         self.assertTrue(self.queue._isBlockable(translations, blocklist))
 
     def test_isBlockable_multiple_blocked(self):

=== modified file 'lib/lp/translations/tests/test_translationimportqueue.py'
--- lib/lp/translations/tests/test_translationimportqueue.py	2011-08-03 11:00:11 +0000
+++ lib/lp/translations/tests/test_translationimportqueue.py	2011-09-12 15:16:27 +0000
@@ -429,3 +429,21 @@
             productseries=self.productseries)
         stripped_path = path.lstrip('/')
         self.assertEqual([stripped_path], self._getQueuePaths())
+
+    def test_addOrUpdateEntry_detects_conflicts(self):
+        pot = self.factory.makePOTemplate(translation_domain='domain')
+        uploader = self.factory.makePerson()
+        pofile = self.factory.makePOFile(potemplate=pot, language_code='fr')
+
+        # Add an import queue entry with a single pofile for a template.
+        tiqe1 = self.factory.makeTranslationImportQueueEntry(
+            path=pofile.path, productseries=pot.productseries,
+            potemplate=pot, uploader=uploader)
+
+        # Add an import queue entry for a the same pofile, but done
+        # directly on the pofile object (i.e. more specific).
+        tiqe2 = self.factory.makeTranslationImportQueueEntry(
+            path=pofile.path, productseries=pot.productseries,
+            potemplate=pot, pofile=pofile, uploader=uploader)
+
+        self.assertEquals(tiqe1, tiqe2)

=== modified file 'lib/lp/translations/utilities/tests/test_translation_importer.py'
--- lib/lp/translations/utilities/tests/test_translation_importer.py	2011-09-07 11:34:06 +0000
+++ lib/lp/translations/utilities/tests/test_translation_importer.py	2011-09-12 15:16:27 +0000
@@ -259,10 +259,8 @@
         """ % potmsgset2.msgid_singular.msgid
 
         entry = self.factory.makeTranslationImportQueueEntry(
-            'foo.po', potemplate=template,
+            'foo.po', potemplate=template, pofile=pofile,
             status=RosettaImportStatus.APPROVED, content=text)
-        entry.pofile = pofile
-        entry.status = RosettaImportStatus.APPROVED
         transaction.commit()
 
         self.assertTrue(existing_translation.is_current_upstream)