Launchpad itself

Merge lp://staging/~jtv/launchpad/bug-429811 into lp://staging/launchpad

  1. bug-429811
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Stuart Bishop
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~jtv/launchpad/bug-429811
Merge into: lp://staging/launchpad
Diff against target: None lines
To merge this branch: bzr merge lp://staging/~jtv/launchpad/bug-429811
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+11762@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 429811 =

There's a case in auto-approval of uploads where the approver *hopes* to find
exactly one match for a template. It's probably a bad thing for there to be
multiple matches, but it's not unthinkable and it definitely shouldn't break the
entire approval script. We've seen and fixed that sort of breakage before.

Today it failed, just once. It's not serious, just a little nuisance that
causes unnecessary alarm on the error-reports list.

This branch replaces a NotOneError for that situation with a log message. This
also allowed me to make the unit tests for that code a bit more comprehensive,
since it didn't previously touch the disaster case.

Tests:
{{{
./bin/test -vv -t test_autoapproval
}}}

No lint. For QA, see that auto-approval still works and note that the
traceback doesn't recur over time. Instead, there may be the occasional log
message.

Jeroen

Revision history for this message
Stuart Bishop (stub) wrote :

IMasterStore(POTemplate) is a better spelling than manually using the IStoreSelector.

Limiting the shortlist to 2 elements is wrong, as we know we can get larger numbers and the code handles this case. Bump it up.

review: Approve
Revision history for this message
Stuart Bishop (stub) wrote :

> IMasterStore(POTemplate) is a better spelling than manually using the
> IStoreSelector.

Sorry. IStore(POTemplate) for the default store.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2009-08-05 14:54:11 +0000
+++ lib/lp/translations/model/potemplate.py 2009-09-15 05:25:13 +0000
@@ -24,7 +24,7 @@
24from sqlobject import (24from sqlobject import (
25 BoolCol, ForeignKey, IntCol, SQLMultipleJoin, SQLObjectNotFound,25 BoolCol, ForeignKey, IntCol, SQLMultipleJoin, SQLObjectNotFound,
26 StringCol)26 StringCol)
27from storm.expr import Alias, And, LeftJoin, SQL27from storm.expr import Alias, And, LeftJoin, Or, SQL
28from storm.info import ClassAlias28from storm.info import ClassAlias
29from storm.store import Store29from storm.store import Store
30from zope.component import getAdapter, getUtility30from zope.component import getAdapter, getUtility
@@ -38,6 +38,8 @@
38from canonical.database.sqlbase import (38from canonical.database.sqlbase import (
39 SQLBase, quote, flush_database_updates, sqlvalues)39 SQLBase, quote, flush_database_updates, sqlvalues)
40from canonical.launchpad import helpers40from canonical.launchpad import helpers
41from canonical.launchpad.webapp.interfaces import (
42 IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
41from lp.translations.utilities.rosettastats import RosettaStats43from lp.translations.utilities.rosettastats import RosettaStats
42from lp.services.worlddata.model.language import Language44from lp.services.worlddata.model.language import Language
43from lp.registry.interfaces.person import validate_public_person45from lp.registry.interfaces.person import validate_public_person
@@ -1209,46 +1211,52 @@
1209 def getPOTemplateByPathAndOrigin(self, path, productseries=None,1211 def getPOTemplateByPathAndOrigin(self, path, productseries=None,
1210 distroseries=None, sourcepackagename=None):1212 distroseries=None, sourcepackagename=None):
1211 """See `IPOTemplateSet`."""1213 """See `IPOTemplateSet`."""
1212 if productseries is not None:1214 assert (productseries is None) != (sourcepackagename is None), (
1213 return POTemplate.selectOne('''1215 "Must specify either productseries or sourcepackagename.")
1214 POTemplate.iscurrent IS TRUE AND1216
1215 POTemplate.productseries = %s AND1217 conditions = And(
1216 POTemplate.path = %s''' % sqlvalues(1218 POTemplate.iscurrent == True,
1217 productseries.id,1219 POTemplate.path == path,
1218 path)1220 POTemplate.productseries == productseries,
1219 )1221 Or(
1220 elif sourcepackagename is not None:1222 POTemplate.from_sourcepackagename == sourcepackagename,
1221 # The POTemplate belongs to a distribution and it could come from1223 POTemplate.sourcepackagename == sourcepackagename))
1222 # another package that the one it's linked at the moment so we1224
1223 # first check to find it at IPOTemplate.from_sourcepackagename1225 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
1224 potemplate = POTemplate.selectOne('''1226 matches = helpers.shortlist(store.find(POTemplate, conditions), 2)
1225 POTemplate.iscurrent IS TRUE AND1227
1226 POTemplate.distroseries = %s AND1228 if len(matches) == 0:
1227 POTemplate.from_sourcepackagename = %s AND1229 # Nope. Sorry.
1228 POTemplate.path = %s''' % sqlvalues(1230 return None
1229 distroseries.id,1231 elif len(matches) == 1:
1230 sourcepackagename.id,1232 # Yup. Great.
1231 path)1233 return matches[0]
1232 )1234 elif sourcepackagename is None:
1233 if potemplate is not None:1235 # Multiple matches, and for a product not a package.
1234 # There is no potemplate in that 'path' and1236 logging.warn(
1235 # 'from_sourcepackagename' so we do a search using the usual1237 "Found %d templates with path '%s' for productseries %s" % (
1236 # sourcepackagename.1238 len(matches), path, productseries.title))
1237 return potemplate1239 return None
1238
1239 return POTemplate.selectOne('''
1240 POTemplate.iscurrent IS TRUE AND
1241 POTemplate.distroseries = %s AND
1242 POTemplate.sourcepackagename = %s AND
1243 POTemplate.path = %s''' % sqlvalues(
1244 distroseries.id,
1245 sourcepackagename.id,
1246 path)
1247 )
1248 else:1240 else:
1249 raise AssertionError(1241 # Multiple matches, for a distribution package. Prefer a
1250 'Either productseries or sourcepackagename arguments must be'1242 # match on from_sourcepackagename: the file may have been
1251 ' not None.')1243 # uploaded for another package than the one it is meant to
1244 # be imported into.
1245 preferred_matches = [
1246 match
1247 for match in matches
1248 if match.from_sourcepackagename == sourcepackagename
1249 ]
1250
1251 if len(preferred_matches) == 1:
1252 return preferred_matches[0]
1253 else:
1254 logging.warn(
1255 "Found %d templates with path '%s' for package %s "
1256 "(%d matched on from_sourcepackagename)." % (
1257 len(matches), path, sourcepackagename.name,
1258 len(preferred_matches)))
1259 return None
12521260
1253 @staticmethod1261 @staticmethod
1254 def compareSharingPrecedence(left, right):1262 def compareSharingPrecedence(left, right):
12551263
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py 2009-08-06 13:56:17 +0000
+++ lib/lp/translations/tests/test_autoapproval.py 2009-09-15 05:25:13 +0000
@@ -262,6 +262,8 @@
262 self.factory = LaunchpadObjectFactory()262 self.factory = LaunchpadObjectFactory()
263 self.templateset = POTemplateSet()263 self.templateset = POTemplateSet()
264264
265 def _setUpProduct(self):
266 """Set up a `Product` with release series and two templates."""
265 self.product = self.factory.makeProduct()267 self.product = self.factory.makeProduct()
266 self.productseries = self.factory.makeSeries(product=self.product)268 self.productseries = self.factory.makeSeries(product=self.product)
267 product_subset = POTemplateSubset(productseries=self.productseries)269 product_subset = POTemplateSubset(productseries=self.productseries)
@@ -270,6 +272,8 @@
270 self.producttemplate2 = product_subset.new(272 self.producttemplate2 = product_subset.new(
271 'test2', 'test2', 'test.pot', self.product.owner)273 'test2', 'test2', 'test.pot', self.product.owner)
272274
275 def _setUpDistro(self):
276 """Set up a `Distribution` with two templates."""
273 self.distro = self.factory.makeDistribution()277 self.distro = self.factory.makeDistribution()
274 self.distroseries = self.factory.makeDistroRelease(278 self.distroseries = self.factory.makeDistroRelease(
275 distribution=self.distro)279 distribution=self.distro)
@@ -283,9 +287,46 @@
283 self.distrotemplate2 = distro_subset.new(287 self.distrotemplate2 = distro_subset.new(
284 'test2', 'test2', 'test.pot', self.distro.owner)288 'test2', 'test2', 'test.pot', self.distro.owner)
285289
290 def test_ByPathAndOrigin_product_duplicate(self):
291 # When multiple templates match for a product series,
292 # getPOTemplateByPathAndOrigin returns none.
293 self._setUpProduct()
294 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
295 'test.pot', productseries=self.productseries)
296 self.assertEqual(None, guessed_template)
297
298 def test_ByPathAndOrigin_package_duplicate(self):
299 # When multiple templates match on sourcepackagename,
300 # getPOTemplateByPathAndOrigin returns none.
301 self._setUpDistro()
302 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
303 'test.pot', sourcepackagename=self.packagename)
304 self.assertEqual(None, guessed_template)
305
306 def test_ByPathAndOrigin_from_package_duplicate(self):
307 # When multiple templates match on from_sourcepackagename,
308 # getPOTemplateByPathAndOrigin returns none.
309 self._setUpDistro()
310 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
311 'test.pot', sourcepackagename=self.from_packagename)
312 self.assertEqual(None, guessed_template)
313
314 def test_ByPathAndOrigin_preferred_match(self):
315 # getPOTemplateByPathAndOrigin prefers from_sourcepackagename
316 # matches over sourcepackagename matches.
317 self._setUpDistro()
318 match_package = SourcePackageNameSet().new('match-package')
319 self.distrotemplate1.sourcepackagename = match_package
320 self.distrotemplate2.from_sourcepackagename = match_package
321
322 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
323 'test.pot', sourcepackagename=match_package)
324 self.assertEqual(self.distrotemplate2, guessed_template)
325
286 def test_ByPathAndOriginProductNonCurrentDuplicate(self):326 def test_ByPathAndOriginProductNonCurrentDuplicate(self):
287 # If two templates for the same product series have the same327 # If two templates for the same product series have the same
288 # path, but only one is current, that one is returned.328 # path, but only one is current, that one is returned.
329 self._setUpProduct()
289 self.producttemplate1.iscurrent = False330 self.producttemplate1.iscurrent = False
290 self.producttemplate2.iscurrent = True331 self.producttemplate2.iscurrent = True
291 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(332 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
@@ -294,6 +335,7 @@
294335
295 def test_ByPathAndOriginProductNoCurrentTemplate(self):336 def test_ByPathAndOriginProductNoCurrentTemplate(self):
296 # Non-current templates in product series are ignored.337 # Non-current templates in product series are ignored.
338 self._setUpProduct()
297 self.producttemplate1.iscurrent = False339 self.producttemplate1.iscurrent = False
298 self.producttemplate2.iscurrent = False340 self.producttemplate2.iscurrent = False
299 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(341 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
@@ -304,6 +346,7 @@
304 # If two templates for the same distroseries and source package346 # If two templates for the same distroseries and source package
305 # have the same path, but only one is current, the current one347 # have the same path, but only one is current, the current one
306 # is returned.348 # is returned.
349 self._setUpDistro()
307 self.distrotemplate1.iscurrent = False350 self.distrotemplate1.iscurrent = False
308 self.distrotemplate2.iscurrent = True351 self.distrotemplate2.iscurrent = True
309 self.distrotemplate1.from_sourcepackagename = None352 self.distrotemplate1.from_sourcepackagename = None
@@ -315,6 +358,7 @@
315358
316 def test_ByPathAndOriginDistroNoCurrentTemplate(self):359 def test_ByPathAndOriginDistroNoCurrentTemplate(self):
317 # Non-current templates in distroseries are ignored.360 # Non-current templates in distroseries are ignored.
361 self._setUpDistro()
318 self.distrotemplate1.iscurrent = False362 self.distrotemplate1.iscurrent = False
319 self.distrotemplate2.iscurrent = False363 self.distrotemplate2.iscurrent = False
320 self.distrotemplate1.from_sourcepackagename = None364 self.distrotemplate1.from_sourcepackagename = None
@@ -328,6 +372,7 @@
328 # If two templates for the same distroseries and original source372 # If two templates for the same distroseries and original source
329 # package have the same path, but only one is current, that one is373 # package have the same path, but only one is current, that one is
330 # returned.374 # returned.
375 self._setUpDistro()
331 self.distrotemplate1.iscurrent = False376 self.distrotemplate1.iscurrent = False
332 self.distrotemplate2.iscurrent = True377 self.distrotemplate2.iscurrent = True
333 self.distrotemplate1.from_sourcepackagename = self.from_packagename378 self.distrotemplate1.from_sourcepackagename = self.from_packagename
@@ -340,6 +385,7 @@
340 def test_ByPathAndOriginDistroFromSourcePackageNoCurrentTemplate(self):385 def test_ByPathAndOriginDistroFromSourcePackageNoCurrentTemplate(self):
341 # Non-current templates in distroseries are ignored by the386 # Non-current templates in distroseries are ignored by the
342 # "from_sourcepackagename" match.387 # "from_sourcepackagename" match.
388 self._setUpDistro()
343 self.distrotemplate1.iscurrent = False389 self.distrotemplate1.iscurrent = False
344 self.distrotemplate2.iscurrent = False390 self.distrotemplate2.iscurrent = False
345 self.distrotemplate1.from_sourcepackagename = self.from_packagename391 self.distrotemplate1.from_sourcepackagename = self.from_packagename
@@ -352,12 +398,14 @@
352 def test_ByTranslationDomain(self):398 def test_ByTranslationDomain(self):
353 # getPOTemplateByTranslationDomain looks up a template by399 # getPOTemplateByTranslationDomain looks up a template by
354 # translation domain.400 # translation domain.
401 self._setUpDistro()
355 subset = POTemplateSubset(distroseries=self.distroseries)402 subset = POTemplateSubset(distroseries=self.distroseries)
356 potemplate = subset.getPOTemplateByTranslationDomain('test1')403 potemplate = subset.getPOTemplateByTranslationDomain('test1')
357 self.assertEqual(potemplate, self.distrotemplate1)404 self.assertEqual(potemplate, self.distrotemplate1)
358405
359 def test_ByTranslationDomain_none(self):406 def test_ByTranslationDomain_none(self):
360 # Test getPOTemplateByTranslationDomain for the zero-match case.407 # Test getPOTemplateByTranslationDomain for the zero-match case.
408 self._setUpDistro()
361 subset = POTemplateSubset(distroseries=self.distroseries)409 subset = POTemplateSubset(distroseries=self.distroseries)
362 potemplate = subset.getPOTemplateByTranslationDomain('notesthere')410 potemplate = subset.getPOTemplateByTranslationDomain('notesthere')
363 self.assertEqual(potemplate, None)411 self.assertEqual(potemplate, None)
@@ -365,6 +413,7 @@
365 def test_ByTranslationDomain_duplicate(self):413 def test_ByTranslationDomain_duplicate(self):
366 # getPOTemplateByTranslationDomain returns no template if there414 # getPOTemplateByTranslationDomain returns no template if there
367 # is more than one match.415 # is more than one match.
416 self._setUpDistro()
368 self.distrotemplate1.iscurrent = True417 self.distrotemplate1.iscurrent = True
369 other_package = SourcePackageNameSet().new('other-package')418 other_package = SourcePackageNameSet().new('other-package')
370 other_subset = POTemplateSubset(419 other_subset = POTemplateSubset(
@@ -385,6 +434,7 @@
385 # To tickle this condition, the user first has to upload a file434 # To tickle this condition, the user first has to upload a file
386 # that's not attached to a template; then upload another one435 # that's not attached to a template; then upload another one
387 # that is, before the first one goes into auto-approval.436 # that is, before the first one goes into auto-approval.
437 self._setUpProduct()
388 queue = TranslationImportQueue()438 queue = TranslationImportQueue()
389 template = self.producttemplate1439 template = self.producttemplate1
390440