launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-302449 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-302449/+merge/73813

= 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
-- 
https://code.launchpad.net/~danilo/launchpad/bug-302449/+merge/73813
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-302449 into lp:launchpad.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-31 17:40:09 +0000
+++ lib/lp/testing/factory.py	2011-09-02 13:47:00 +0000
@@ -307,6 +307,9 @@
     TranslationFileFormat,
     )
 from lp.translations.interfaces.translationgroup import ITranslationGroupSet
+from lp.translations.interfaces.translationimportqueue import (
+    ITranslationImportQueue,
+    )
 from lp.translations.interfaces.translationmessage import (
     RosettaTranslationOrigin,
     )
@@ -315,9 +318,6 @@
     ITranslationTemplatesBuildJobSource,
     )
 from lp.translations.interfaces.translator import ITranslatorSet
-from lp.translations.model.translationimportqueue import (
-    TranslationImportQueueEntry,
-    )
 from lp.translations.model.translationtemplateitem import (
     TranslationTemplateItem,
     )
@@ -3247,9 +3247,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
@@ -3257,11 +3254,11 @@
         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)
+        return 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)
 
     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-02 13:47:00 +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/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2011-07-26 11:22:15 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2011-09-02 13:47:00 +0000
@@ -835,45 +835,41 @@
         :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.

=== 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-02 13:47:00 +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)