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
1=== modified file 'lib/lp/translations/model/potemplate.py'
2--- lib/lp/translations/model/potemplate.py 2009-08-05 14:54:11 +0000
3+++ lib/lp/translations/model/potemplate.py 2009-09-15 05:25:13 +0000
4@@ -24,7 +24,7 @@
5 from sqlobject import (
6 BoolCol, ForeignKey, IntCol, SQLMultipleJoin, SQLObjectNotFound,
7 StringCol)
8-from storm.expr import Alias, And, LeftJoin, SQL
9+from storm.expr import Alias, And, LeftJoin, Or, SQL
10 from storm.info import ClassAlias
11 from storm.store import Store
12 from zope.component import getAdapter, getUtility
13@@ -38,6 +38,8 @@
14 from canonical.database.sqlbase import (
15 SQLBase, quote, flush_database_updates, sqlvalues)
16 from canonical.launchpad import helpers
17+from canonical.launchpad.webapp.interfaces import (
18+ IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
19 from lp.translations.utilities.rosettastats import RosettaStats
20 from lp.services.worlddata.model.language import Language
21 from lp.registry.interfaces.person import validate_public_person
22@@ -1209,46 +1211,52 @@
23 def getPOTemplateByPathAndOrigin(self, path, productseries=None,
24 distroseries=None, sourcepackagename=None):
25 """See `IPOTemplateSet`."""
26- if productseries is not None:
27- return POTemplate.selectOne('''
28- POTemplate.iscurrent IS TRUE AND
29- POTemplate.productseries = %s AND
30- POTemplate.path = %s''' % sqlvalues(
31- productseries.id,
32- path)
33- )
34- elif sourcepackagename is not None:
35- # The POTemplate belongs to a distribution and it could come from
36- # another package that the one it's linked at the moment so we
37- # first check to find it at IPOTemplate.from_sourcepackagename
38- potemplate = POTemplate.selectOne('''
39- POTemplate.iscurrent IS TRUE AND
40- POTemplate.distroseries = %s AND
41- POTemplate.from_sourcepackagename = %s AND
42- POTemplate.path = %s''' % sqlvalues(
43- distroseries.id,
44- sourcepackagename.id,
45- path)
46- )
47- if potemplate is not None:
48- # There is no potemplate in that 'path' and
49- # 'from_sourcepackagename' so we do a search using the usual
50- # sourcepackagename.
51- return potemplate
52-
53- return POTemplate.selectOne('''
54- POTemplate.iscurrent IS TRUE AND
55- POTemplate.distroseries = %s AND
56- POTemplate.sourcepackagename = %s AND
57- POTemplate.path = %s''' % sqlvalues(
58- distroseries.id,
59- sourcepackagename.id,
60- path)
61- )
62+ assert (productseries is None) != (sourcepackagename is None), (
63+ "Must specify either productseries or sourcepackagename.")
64+
65+ conditions = And(
66+ POTemplate.iscurrent == True,
67+ POTemplate.path == path,
68+ POTemplate.productseries == productseries,
69+ Or(
70+ POTemplate.from_sourcepackagename == sourcepackagename,
71+ POTemplate.sourcepackagename == sourcepackagename))
72+
73+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
74+ matches = helpers.shortlist(store.find(POTemplate, conditions), 2)
75+
76+ if len(matches) == 0:
77+ # Nope. Sorry.
78+ return None
79+ elif len(matches) == 1:
80+ # Yup. Great.
81+ return matches[0]
82+ elif sourcepackagename is None:
83+ # Multiple matches, and for a product not a package.
84+ logging.warn(
85+ "Found %d templates with path '%s' for productseries %s" % (
86+ len(matches), path, productseries.title))
87+ return None
88 else:
89- raise AssertionError(
90- 'Either productseries or sourcepackagename arguments must be'
91- ' not None.')
92+ # Multiple matches, for a distribution package. Prefer a
93+ # match on from_sourcepackagename: the file may have been
94+ # uploaded for another package than the one it is meant to
95+ # be imported into.
96+ preferred_matches = [
97+ match
98+ for match in matches
99+ if match.from_sourcepackagename == sourcepackagename
100+ ]
101+
102+ if len(preferred_matches) == 1:
103+ return preferred_matches[0]
104+ else:
105+ logging.warn(
106+ "Found %d templates with path '%s' for package %s "
107+ "(%d matched on from_sourcepackagename)." % (
108+ len(matches), path, sourcepackagename.name,
109+ len(preferred_matches)))
110+ return None
111
112 @staticmethod
113 def compareSharingPrecedence(left, right):
114
115=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
116--- lib/lp/translations/tests/test_autoapproval.py 2009-08-06 13:56:17 +0000
117+++ lib/lp/translations/tests/test_autoapproval.py 2009-09-15 05:25:13 +0000
118@@ -262,6 +262,8 @@
119 self.factory = LaunchpadObjectFactory()
120 self.templateset = POTemplateSet()
121
122+ def _setUpProduct(self):
123+ """Set up a `Product` with release series and two templates."""
124 self.product = self.factory.makeProduct()
125 self.productseries = self.factory.makeSeries(product=self.product)
126 product_subset = POTemplateSubset(productseries=self.productseries)
127@@ -270,6 +272,8 @@
128 self.producttemplate2 = product_subset.new(
129 'test2', 'test2', 'test.pot', self.product.owner)
130
131+ def _setUpDistro(self):
132+ """Set up a `Distribution` with two templates."""
133 self.distro = self.factory.makeDistribution()
134 self.distroseries = self.factory.makeDistroRelease(
135 distribution=self.distro)
136@@ -283,9 +287,46 @@
137 self.distrotemplate2 = distro_subset.new(
138 'test2', 'test2', 'test.pot', self.distro.owner)
139
140+ def test_ByPathAndOrigin_product_duplicate(self):
141+ # When multiple templates match for a product series,
142+ # getPOTemplateByPathAndOrigin returns none.
143+ self._setUpProduct()
144+ guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
145+ 'test.pot', productseries=self.productseries)
146+ self.assertEqual(None, guessed_template)
147+
148+ def test_ByPathAndOrigin_package_duplicate(self):
149+ # When multiple templates match on sourcepackagename,
150+ # getPOTemplateByPathAndOrigin returns none.
151+ self._setUpDistro()
152+ guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
153+ 'test.pot', sourcepackagename=self.packagename)
154+ self.assertEqual(None, guessed_template)
155+
156+ def test_ByPathAndOrigin_from_package_duplicate(self):
157+ # When multiple templates match on from_sourcepackagename,
158+ # getPOTemplateByPathAndOrigin returns none.
159+ self._setUpDistro()
160+ guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
161+ 'test.pot', sourcepackagename=self.from_packagename)
162+ self.assertEqual(None, guessed_template)
163+
164+ def test_ByPathAndOrigin_preferred_match(self):
165+ # getPOTemplateByPathAndOrigin prefers from_sourcepackagename
166+ # matches over sourcepackagename matches.
167+ self._setUpDistro()
168+ match_package = SourcePackageNameSet().new('match-package')
169+ self.distrotemplate1.sourcepackagename = match_package
170+ self.distrotemplate2.from_sourcepackagename = match_package
171+
172+ guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
173+ 'test.pot', sourcepackagename=match_package)
174+ self.assertEqual(self.distrotemplate2, guessed_template)
175+
176 def test_ByPathAndOriginProductNonCurrentDuplicate(self):
177 # If two templates for the same product series have the same
178 # path, but only one is current, that one is returned.
179+ self._setUpProduct()
180 self.producttemplate1.iscurrent = False
181 self.producttemplate2.iscurrent = True
182 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
183@@ -294,6 +335,7 @@
184
185 def test_ByPathAndOriginProductNoCurrentTemplate(self):
186 # Non-current templates in product series are ignored.
187+ self._setUpProduct()
188 self.producttemplate1.iscurrent = False
189 self.producttemplate2.iscurrent = False
190 guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
191@@ -304,6 +346,7 @@
192 # If two templates for the same distroseries and source package
193 # have the same path, but only one is current, the current one
194 # is returned.
195+ self._setUpDistro()
196 self.distrotemplate1.iscurrent = False
197 self.distrotemplate2.iscurrent = True
198 self.distrotemplate1.from_sourcepackagename = None
199@@ -315,6 +358,7 @@
200
201 def test_ByPathAndOriginDistroNoCurrentTemplate(self):
202 # Non-current templates in distroseries are ignored.
203+ self._setUpDistro()
204 self.distrotemplate1.iscurrent = False
205 self.distrotemplate2.iscurrent = False
206 self.distrotemplate1.from_sourcepackagename = None
207@@ -328,6 +372,7 @@
208 # If two templates for the same distroseries and original source
209 # package have the same path, but only one is current, that one is
210 # returned.
211+ self._setUpDistro()
212 self.distrotemplate1.iscurrent = False
213 self.distrotemplate2.iscurrent = True
214 self.distrotemplate1.from_sourcepackagename = self.from_packagename
215@@ -340,6 +385,7 @@
216 def test_ByPathAndOriginDistroFromSourcePackageNoCurrentTemplate(self):
217 # Non-current templates in distroseries are ignored by the
218 # "from_sourcepackagename" match.
219+ self._setUpDistro()
220 self.distrotemplate1.iscurrent = False
221 self.distrotemplate2.iscurrent = False
222 self.distrotemplate1.from_sourcepackagename = self.from_packagename
223@@ -352,12 +398,14 @@
224 def test_ByTranslationDomain(self):
225 # getPOTemplateByTranslationDomain looks up a template by
226 # translation domain.
227+ self._setUpDistro()
228 subset = POTemplateSubset(distroseries=self.distroseries)
229 potemplate = subset.getPOTemplateByTranslationDomain('test1')
230 self.assertEqual(potemplate, self.distrotemplate1)
231
232 def test_ByTranslationDomain_none(self):
233 # Test getPOTemplateByTranslationDomain for the zero-match case.
234+ self._setUpDistro()
235 subset = POTemplateSubset(distroseries=self.distroseries)
236 potemplate = subset.getPOTemplateByTranslationDomain('notesthere')
237 self.assertEqual(potemplate, None)
238@@ -365,6 +413,7 @@
239 def test_ByTranslationDomain_duplicate(self):
240 # getPOTemplateByTranslationDomain returns no template if there
241 # is more than one match.
242+ self._setUpDistro()
243 self.distrotemplate1.iscurrent = True
244 other_package = SourcePackageNameSet().new('other-package')
245 other_subset = POTemplateSubset(
246@@ -385,6 +434,7 @@
247 # To tickle this condition, the user first has to upload a file
248 # that's not attached to a template; then upload another one
249 # that is, before the first one goes into auto-approval.
250+ self._setUpProduct()
251 queue = TranslationImportQueue()
252 template = self.producttemplate1
253