launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00471
[Merge] lp:~jtv/launchpad/bug-613821 into lp:launchpad/devel
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-613821 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#613821 Approver violates unique constraint
https://bugs.launchpad.net/bugs/613821
= Bug 613821 =
This fixes a complicated bug where sometimes two template uploads on the translation import queue can clash in a way that breaks auto-approval. You'll find the scenario described in the test. A unique constraint gets violated.
I did some small cleanups along the way. I separated the big approval loop from the loop body which tries to approve an entry, which makes it a bit easier to test approval of a specific entry. I also extracted the part that looks up a format and its importer, just so that it'd stop distracting me while reading the surrounding code.
The check as it is now is a bit over-careful for the current form of the database constraint: it checks both when setting an entry's pofile and when setting an entry's potemplate. That's not actually necessary for the pofile field, since it's not part of the constraint. If there's a clash there, it occurs during upload. But checking in both cases makes the higher-level code simpler and leaves us freer to change the constraint later if we like. It does add one query to POFile approval, but not to the most performance-sensitive path (which is where we try to approve a file but fail).
I also sanitized the ordering of the approval method a bit. Depending on the details of the ORM and/or database language binding, and how result-set batching may interact with transaction boundaries, it may be possible to query all needs-review entries but actually get some that are already approved. Instead of checking at the very end whether an entry might already have been approved, I now do that check up-front where it doesn't look so crazy.
Test:
{{{
./bin/test -vvc -m lp.translations.tests.test_autoapproval
}}}
To Q/A, run the updated approver on staging and see that it doesn't barf. The production glitch that this fixes has been around long enough that it should be in the staging database prior to release.
Jeroen
--
https://code.launchpad.net/~jtv/launchpad/bug-613821/+merge/31957
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-613821 into lp:launchpad/devel.
=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py 2010-08-02 02:13:52 +0000
+++ lib/lp/translations/model/translationimportqueue.py 2010-08-06 13:37:51 +0000
@@ -863,35 +863,42 @@
return entries[0]
- def addOrUpdateEntry(self, path, content, is_published, importer,
- sourcepackagename=None, distroseries=None, productseries=None,
- potemplate=None, pofile=None, format=None):
- """See ITranslationImportQueue."""
- if ((sourcepackagename is not None or distroseries is not None) and
- productseries is not None):
- raise AssertionError(
- 'The productseries argument cannot be not None if'
- ' sourcepackagename or distroseries is also not None.')
- if (sourcepackagename is None and distroseries is None and
- productseries is None):
- raise AssertionError('Any of sourcepackagename, distroseries or'
- ' productseries must be not None.')
-
- if content is None or content == '':
- raise AssertionError('The content cannot be empty')
-
- if path is None or path == '':
- raise AssertionError('The path cannot be empty')
-
- filename = os.path.basename(path)
- root, ext = os.path.splitext(filename)
- translation_importer = getUtility(ITranslationImporter)
+ def _getFormatAndImporter(self, filename, content, format=None):
+ """Get the appropriate format and importer for this upload.
+
+ :param filename: Name of the uploaded file.
+ :param content: Contents of the uploaded file.
+ :param format: Optional hard choice of format. If none is
+ given, a format will be divined from the file's name and
+ contents.
+ :return: a tuple of the selected format and its importer.
+ """
if format is None:
- # Get it based on the file extension and file content.
+ root, ext = os.path.splitext(filename)
+ translation_importer = getUtility(ITranslationImporter)
format = translation_importer.getTranslationFileFormat(
ext, content)
- format_importer = translation_importer.getTranslationFormatImporter(
- format)
+
+ translation_importer = getUtility(ITranslationImporter)
+ return (
+ format, translation_importer.getTranslationFormatImporter(format))
+
+ def addOrUpdateEntry(self, path, content, is_published, importer,
+ sourcepackagename=None, distroseries=None,
+ productseries=None, potemplate=None, pofile=None,
+ format=None):
+ """See `ITranslationImportQueue`."""
+ assert (distroseries is None) != (productseries is None), (
+ "An upload must be for a productseries or a distroseries. "
+ "This one has either neither or both.")
+ assert productseries is None or sourcepackagename is None, (
+ "Can't upload to a sourcepackagename in a productseries.")
+ assert content is not None and content != '', "Upload has no content."
+ assert path is not None and path != '', "Upload has no path."
+
+ filename = os.path.basename(path)
+ format, format_importer = self._getFormatAndImporter(
+ filename, content, format=format)
# Upload the file into librarian.
size = len(content)
@@ -906,6 +913,7 @@
pofile, sourcepackagename, distroseries, productseries)
except TranslationImportQueueConflictError:
return None
+
if entry is None:
# It's a new row.
entry = TranslationImportQueueEntry(path=path, content=alias,
@@ -1157,52 +1165,65 @@
return distroseriess + products
+ def _attemptToSet(self, entry, potemplate=None, pofile=None):
+ """Set potemplate or pofile on a `TranslationImportQueueEntry`.
+
+ This will do nothing if setting potemplate or pofile would clash
+ with another entry.
+ """
+ if potemplate == entry.potemplate and pofile == entry.pofile:
+ # Nothing to do here.
+ return False
+
+ existing_entry = self._getMatchingEntry(
+ entry.path, entry.importer, potemplate, pofile,
+ entry.sourcepackagename, entry.distroseries, entry.productseries)
+
+ if existing_entry is None:
+ entry.potemplate = potemplate
+ entry.pofile = pofile
+
+ def _attemptToApprove(self, entry):
+ """Attempt to approve one queue entry."""
+ if entry.status != RosettaImportStatus.NEEDS_REVIEW:
+ return
+
+ if entry.import_into is None:
+ # We don't have a place to import this entry. Try to guess it.
+ importer = getUtility(ITranslationImporter)
+ if importer.isTranslationName(entry.path):
+ potemplate = entry.potemplate
+ pofile = entry.getGuessedPOFile()
+ else:
+ # It's a template.
+ # Check if we can guess where it should be imported.
+ potemplate = entry.guessed_potemplate
+ pofile = entry.pofile
+
+ self._attemptToSet(entry, potemplate=potemplate, pofile=pofile)
+
+ if entry.import_into is None:
+ # Still no dice.
+ return False
+
+ # Yay! We have a POTemplate or POFile to import this entry
+ # into. Approve.
+ entry.setStatus(RosettaImportStatus.APPROVED,
+ getUtility(ILaunchpadCelebrities).rosetta_experts)
+
+ return True
+
def executeOptimisticApprovals(self, txn=None):
- """See ITranslationImportQueue."""
- there_are_entries_approved = False
- importer = getUtility(ITranslationImporter)
+ """See `ITranslationImportQueue`."""
+ approved_entries = False
for entry in self._iterNeedsReview():
- if entry.import_into is None:
- # We don't have a place to import this entry. Try to guess it.
- if importer.isTranslationName(entry.path):
- # Check if we can guess where it should be imported.
- guess = entry.getGuessedPOFile()
- if guess is None:
- # We were not able to guess a place to import it,
- # leave the status of this entry as
- # RosettaImportStatus.NEEDS_REVIEW and wait for an
- # admin to manually review it.
- continue
- # Set the place where it should be imported.
- entry.pofile = guess
-
- else:
- # It's a template.
- # Check if we can guess where it should be imported.
- guess = entry.guessed_potemplate
- if guess is None:
- # We were not able to guess a place to import it,
- # leave the status of this entry as
- # RosettaImportStatus.NEEDS_REVIEW and wait for an
- # admin to manually review it.
- continue
- # Set the place where it should be imported.
- entry.potemplate = guess
-
- assert not entry.import_into is None
-
- if entry.status != RosettaImportStatus.APPROVED:
- there_are_entries_approved = True
-
- # Already know where it should be imported. The entry is approved
- # automatically.
- entry.setStatus(RosettaImportStatus.APPROVED,
- getUtility(ILaunchpadCelebrities).rosetta_experts)
-
+ success = self._attemptToApprove(entry)
+ if success:
+ approved_entries = True
if txn is not None:
txn.commit()
- return there_are_entries_approved
+ return approved_entries
def _getSlaveStore(self):
"""Return the slave store for the import queue.
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py 2010-07-25 23:29:15 +0000
+++ lib/lp/translations/tests/test_autoapproval.py 2010-08-06 13:37:51 +0000
@@ -587,6 +587,37 @@
self.assertEqual(template, entry.guessed_potemplate)
+ def test_avoid_clash_with_existing_entry(self):
+ # When trying to approve a template upload that didn't have its
+ # potemplate field set during upload or an earlier approval run,
+ # the approver will fill out the field if it can. But if by
+ # then there's already another entry from the same person and
+ # for the same target that does have the field set, then filling
+ # out the field would make the two entries clash.
+ queue = TranslationImportQueue()
+ template = self.factory.makePOTemplate()
+ old_entry = queue.addOrUpdateEntry(
+ template.path, '# Content here', False, template.owner,
+ productseries=template.productseries)
+ new_entry = queue.addOrUpdateEntry(
+ template.path, '# Content here', False, template.owner,
+ productseries=template.productseries, potemplate=template)
+
+ # Before approval, the two entries differ in that the new one
+ # has a potemplate.
+ self.assertNotEqual(old_entry, new_entry)
+ self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, old_entry.status)
+ self.assertIs(None, old_entry.potemplate)
+ self.assertEqual(template, new_entry.potemplate)
+ IMasterStore(old_entry).flush()
+
+ # The approver deals with the problem by skipping the entry.
+ queue._attemptToApprove(old_entry)
+
+ # So nothing changes.
+ self.assertIs(None, old_entry.potemplate)
+ self.assertEqual(template, new_entry.potemplate)
+
class TestKdePOFileGuess(TestCaseWithFactory, GardenerDbUserMixin):
"""Test auto-approval's `POFile` guessing for KDE uploads.
