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