Merge lp:~jtv/launchpad/bug-435699 into lp:launchpad/db-devel

[Jeroen T. Vermeulen]

[Jeroen T. Vermeulen (jtv)]

= Bug 435699 =

To fix crashes in the translations auto-approver last cycle, I rewrote
POTemplateSet.getPOTemplateByPathAndOrigin. Where the old version would
crash if multiple templates matched (which happened rarely, fortunately)
the new version detected the condition and logged it.

Now, post-rollout, I find that it logs far too much. As it turns out,
the new version forgets to match on distroseries, so any template that
exists unchanged between Ubuntu releases will seem to have multiple
matches!

This branch fixes that. A test broke in the process, but that seems to
be from using a fixed name for a package, and then not _passing_ the
distroseries argument to getPOTemplateByPathAndOrigin. To stay faithful
to the known interface, which does not require the distroseries argument
(even if it's probably always given in practice), I fixed it by using a
unique name for the package.

What I test for is not the logging, but the more serious consequence of
the condition that's beeing seen (and logged) where it isn't supposed to
be: multiple matches mean that templates are not found, and so uploads
are not approved.

Test:
{{{
./bin/test test_autoapproval
}}}

No lint.

Jeroen

[Michael Nelson (michael.nelson)]

Hi Jeroen,

r=me

Thanks for the details in the MP!

> = Bug 435699 =
>
> To fix crashes in the translations auto-approver last cycle, I rewrote
> POTemplateSet.getPOTemplateByPathAndOrigin. Where the old version would
> crash if multiple templates matched (which happened rarely, fortunately)
> the new version detected the condition and logged it.
>
> Now, post-rollout, I find that it logs far too much. As it turns out,
> the new version forgets to match on distroseries, so any template that
> exists unchanged between Ubuntu releases will seem to have multiple
> matches!

OK - at first I thought - why's this an rc? But yes, with the info in your
last para - it makes more sense!

>
> This branch fixes that. A test broke in the process, but that seems to
> be from using a fixed name for a package, and then not _passing_ the
> distroseries argument to getPOTemplateByPathAndOrigin. To stay faithful
> to the known interface, which does not require the distroseries argument
> (even if it's probably always given in practice), I fixed it by using a
> unique name for the package.
>
> What I test for is not the logging, but the more serious consequence of
> the condition that's beeing seen (and logged) where it isn't supposed to
> be: multiple matches mean that templates are not found, and so uploads
> are not approved.
>
> Test:
> {{{
> ./bin/test test_autoapproval
> }}}
>
> No lint.
>
>
> Jeroen

> === modified file 'lib/lp/translations/model/potemplate.py'
> --- lib/lp/translations/model/potemplate.py 2009-09-17 04:51:07 +0000
> +++ lib/lp/translations/model/potemplate.py 2009-09-24 09:48:24 +0000
> @@ -1225,6 +1225,10 @@
> POTemplate.from_sourcepackagename == sourcepackagename,
> POTemplate.sourcepackagename == sourcepackagename))
>
> + if distroseries:
> + conditions = And(
> + conditions, POTemplate.distroseries == distroseries)
> +

You probably already know, but just in case, another way to do this
is to use a list of extra expressions and just append to it (rather than
wrapping further conditions in And exprs (for eg., Archive.getBuildCounters()).

> store = IStore(POTemplate)
> matches = helpers.shortlist(store.find(POTemplate, conditions))
>
>
> === modified file 'lib/lp/translations/tests/test_autoapproval.py'
> --- lib/lp/translations/tests/test_autoapproval.py 2009-09-15 05:25:13 +0000
> +++ lib/lp/translations/tests/test_autoapproval.py 2009-09-24 09:48:24 +0000
> @@ -272,6 +272,12 @@
> self.producttemplate2 = product_subset.new(
> 'test2', 'test2', 'test.pot', self.product.owner)
>
> + def _makeTemplateForDistroSeries(self, distroseries, name):
> + """Create a template in the given `DistroSeries`."""
> + distro_subset = POTemplateSubset(
> + distroseries=distroseries, sourcepackagename=self.packagename)
> + return distro_subset.new(name, name, 'test.pot', self.distro.owner)
> +

Great.

> def _setUpDistro(self):
> """Set up a `Distribution` with two templates."""
> self.distro = self.factory.makeDistribution()
> @@ -279,13 +285,10 @@
> distri...

review: Approve (code)

[Brad Crittenden (bac)]

review: Approve (release-critical)

[Brad Crittenden (bac)]

Please add the additional comments as we discussed.

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2009-09-17 04:51:07 +0000
+++ lib/lp/translations/model/potemplate.py	2009-09-24 15:05:26 +0000
@@ -1225,6 +1225,10 @@
                 POTemplate.from_sourcepackagename == sourcepackagename,
                 POTemplate.sourcepackagename == sourcepackagename))
 
+        if distroseries:
+            conditions = And(
+                conditions, POTemplate.distroseries == distroseries)
+
         store = IStore(POTemplate)
         matches = helpers.shortlist(store.find(POTemplate, conditions))
 
 
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2009-09-15 05:25:13 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2009-09-24 15:05:26 +0000
@@ -272,6 +272,12 @@
         self.producttemplate2 = product_subset.new(
             'test2', 'test2', 'test.pot', self.product.owner)
 
+    def _makeTemplateForDistroSeries(self, distroseries, name):
+        """Create a template in the given `DistroSeries`."""
+        distro_subset = POTemplateSubset(
+            distroseries=distroseries, sourcepackagename=self.packagename)
+        return distro_subset.new(name, name, 'test.pot', self.distro.owner)
+
     def _setUpDistro(self):
         """Set up a `Distribution` with two templates."""
         self.distro = self.factory.makeDistribution()
@@ -279,13 +285,10 @@
             distribution=self.distro)
         self.packagename = SourcePackageNameSet().new('package')
         self.from_packagename = SourcePackageNameSet().new('from')
-        distro_subset = POTemplateSubset(
-            distroseries=self.distroseries,
-            sourcepackagename=self.packagename)
-        self.distrotemplate1 = distro_subset.new(
-            'test1', 'test1', 'test.pot', self.distro.owner)
-        self.distrotemplate2 = distro_subset.new(
-            'test2', 'test2', 'test.pot', self.distro.owner)
+        self.distrotemplate1 = self._makeTemplateForDistroSeries(
+            self.distroseries, 'test1')
+        self.distrotemplate2 = self._makeTemplateForDistroSeries(
+            self.distroseries, 'test2')
 
     def test_ByPathAndOrigin_product_duplicate(self):
         # When multiple templates match for a product series,
@@ -311,11 +314,32 @@
             'test.pot', sourcepackagename=self.from_packagename)
         self.assertEqual(None, guessed_template)
 
+    def test_ByPathAndOrigin_similar_between_distroseries(self):
+        # getPOTemplateByPathAndOrigin disregards templates from other
+        # distroseries.
+        self._setUpDistro()
+        other_series = self.factory.makeDistroRelease(
+            distribution=self.distro)
+        other_template = self._makeTemplateForDistroSeries(
+            other_series, 'test1')
+        self.distrotemplate1.iscurrent = False
+        self.distrotemplate2.iscurrent = True
+        self.distrotemplate1.from_sourcepackagename = None
+        self.distrotemplate2.from_sourcepackagename = None
+        guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
+            'test.pot', distroseries=self.distroseries,
+            sourcepackagename=self.packagename)
+        self.assertEqual(self.distrotemplate2, guessed_template)
+
     def test_ByPathAndOrigin_preferred_match(self):
         # getPOTemplateByPathAndOrigin prefers from_sourcepackagename
         # matches over sourcepackagename matches.
         self._setUpDistro()
-        match_package = SourcePackageNameSet().new('match-package')
+        # Use unique name for this package, since the search does not
+        # pass a distroseries and so might pick one of the same name up
+        # from elsewhere.
+        match_package = SourcePackageNameSet().new(
+            self.factory.getUniqueString())
         self.distrotemplate1.sourcepackagename = match_package
         self.distrotemplate2.from_sourcepackagename = match_package