Code review comment for lp:~jtv/launchpad/bug-435699

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 @@
> 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')

So this has not changed right? It's just easier to read. Or is it now
that a separate POTemplateSubset is instantiated with each call now?

OK, we clarified on IRC:

<jtv> noodles775: it's purely a refactoring, yes. POTemplateSubset is stateless.

>
> def test_ByPathAndOrigin_product_duplicate(self):
> # When multiple templates match for a product series,
> @@ -311,11 +314,29 @@
> '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)
> +

So this is the regression test right? Ensuring that the distroseries
is respected. OK, I verified that it fails without the change in potemplate
- great.

> def test_ByPathAndOrigin_preferred_match(self):
> # getPOTemplateByPathAndOrigin prefers from_sourcepackagename
> # matches over sourcepackagename matches.
> self._setUpDistro()
> - match_package = SourcePackageNameSet().new('match-package')
> + match_package = SourcePackageNameSet().new(
> + self.factory.getUniqueString())

Thanks for the explanation in your MP! I would have been left guessing
here otherwise!

> self.distrotemplate1.sourcepackagename = match_package
> self.distrotemplate2.from_sourcepackagename = match_package
>

--
Michael