Merge lp:~henninge/launchpad/bug-545354-enable-sharing into lp:~launchpad/launchpad/recife

Seeing how many tests were affected, I'd suggest the following test command which tests all translation tests:

bin/test -vvt lp.translations

Hi Henning,

This looks good. I just have a few comments below.

merge-conditional

-Edwin

>=== modified file 'lib/lp/translations/model/potemplate.py'
>--- lib/lp/translations/model/potemplate.py 2010-01-12 21:29:03 +0000
>+++ lib/lp/translations/model/potemplate.py 2010-06-16 19:25:05 +0000
>@@ -184,6 +184,21 @@
>
> _uses_english_msgids = None
>
>+ @cachedproperty
>+ def _sharing_ids(self):
>+ """Return the IDs of all sharing templates including this one."""
>+ subset = getUtility(IPOTemplateSet).getSharingSubset(
>+ product=self.product,
>+ distribution=self.distribution,
>+ sourcepackagename=self.sourcepackagename)
>+ # Convert to a list for caching.
>+ result = list(subset.getSharingPOTemplateIDs(self.name))
>+ if len(result) == 0:
>+ # Always return at least the template itself.
>+ return [self.id]
>+ else:
>+ return result

Should you also make sure that self.id is in result even if len != 0?

> def __storm_invalidated__(self):
> self.clearPOFileCache()
> self._uses_english_msgids = None
>@@ -1387,45 +1425,102 @@
> package = template.sourcepackagename.name
> return (template.name, package)
>
>- def _iterate_potemplates(self, name_pattern=None):
>- """Yield all templates matching the provided argument.
>-
>- This is much like a `IPOTemplateSubset`, except it operates on
>- `Product`s and `Distribution`s rather than `ProductSeries` and
>- `DistroSeries`.
>- """
>- if self.product:
>- subsets = [
>- self.potemplateset.getSubset(productseries=series)
>- for series in self.product.series
>- ]
>+ def _queryByProduct(self, templatename_clause):
>+ """Build the query that finds POTemplates by their linked product.
>+
>+ Queries the Packaging table to find templates in linked source
>+ packages, too.
>+
>+ :param templatename_clause: A string or a storm expression to
>+ add to the where clause of the query that will select the template
>+ name.
>+ :return: A ResultSet for the query.
>+ """
>+ from lp.registry.model.productseries import ProductSeries
>+
>+ ProductSeries1 = ClassAlias(ProductSeries)
>+ origin = LeftJoin(
>+ LeftJoin(
>+ POTemplate, ProductSeries,
>+ POTemplate.productseriesID == ProductSeries.id),
>+ Join(
>+ Packaging, ProductSeries1,
>+ Packaging.productseriesID == ProductSeries1.id),
>+ And(
>+ Packaging.distroseriesID == POTemplate.distroseriesID,
>+ Packaging.sourcepackagenameID == (
>+ POTemplate.sourcepackagenameID)
>+ )
>+ )
>+ return Store.of(self.product).using(origin).find(
>+ POTemplate,
>+ And(
>+ Or(
>+ ProductSeries.productID == self.product.id,
>+ ProductSeries1.productID == self.product.id
>+ ),
>+ ...

Am 16.06.2010 23:24, schrieb Edwin Grubbs:
> This looks good. I just have a few comments below.

Thanks for the review. I explained one away and fixed two. ;)

>> === modified file 'lib/lp/translations/model/potemplate.py'
>> --- lib/lp/translations/model/potemplate.py 2010-01-12 21:29:03 +0000
>> +++ lib/lp/translations/model/potemplate.py 2010-06-16 19:25:05 +0000
>> @@ -184,6 +184,21 @@
>>
>> _uses_english_msgids = None
>>
>> + @cachedproperty
>> + def _sharing_ids(self):
>> + """Return the IDs of all sharing templates including this one."""
>> + subset = getUtility(IPOTemplateSet).getSharingSubset(
>> + product=self.product,
>> + distribution=self.distribution,
>> + sourcepackagename=self.sourcepackagename)
>> + # Convert to a list for caching.
>> + result = list(subset.getSharingPOTemplateIDs(self.name))
>> + if len(result) == 0:
>> + # Always return at least the template itself.
>> + return [self.id]
>> + else:
>> + return result
>
>
> Should you also make sure that self.id is in result even if len != 0?

No. Actually, the len == 0 case is basically a "no sharing possible case" and
with a single id in the list no sharing happens. If we get any ids back, those
are the sharing ones and we can trust them to include all we need.

[...]

>> def groupEquivalentPOTemplates(self, name_pattern=None): """See IPOTemplateSharingSubset.""" equivalents = {}
>>
>> - for template in self._iterate_potemplates(name_pattern):
>> + if name_pattern is None:
>> + templatename_clause = "1=1"
>> + else:
>> + templatename_clause = "potemplate.name ~ '%s'" % name_pattern
>
>
> I think you should either escape the name_pattern with sqlvalues,
> or you should convert it to a storm conditional.

Yes, thank you. I fixed that by using sqlvalues. Also I replaced "1=1" with
True since we are still in storm land here. I also pulled this out into
another variant of getSharingPOTemplates and added tests for it.

>
>
>> +
>> + for template in self._queryPOTemplates(templatename_clause):
>> key = self._get_potemplate_equivalence_class(template)
>> if key not in equivalents:
>> equivalents[key] = []
>> === modified file 'lib/lp/translations/tests/test_shared_potemplate.py'
>> --- lib/lp/translations/tests/test_shared_potemplate.py 2010-05-18 14:00:10 +0000
>> +++ lib/lp/translations/tests/test_shared_potemplate.py 2010-06-16 19:25:05 +0000
>> @@ -177,5 +176,398 @@
>> self.assertEquals(potmsgset, shared_potmsgset)
>>
>>
>> +class TestMessageSharingProductPackage(TestCaseWithFactory):
>> + """Test message sharing between a product and a package.
>> +
>> + Each test uses assertStatementCount to make sure the number of SQL
>> + queries does not change. This was integrated here to avoid having
>> + a second test case just for statement counts.
>> + The current implementation is good and only needs one statement."""
>
>
> Ending triple-quotes belongs by itself on the following line, since
> this is a multiline docstring.
>

True, thanks.

...

I looked over the incremental diff at Henning's request. A few notes, mostly already discussed on IRC. I think having the separate unit test for the regex matching (which was a bit of a wildcard, ha ha) worked out really well.

These came up on IRC:
 * Backquote identifiers in docstrings: "See `IPOTemplateSharingSubset`."""
 * getSharingPOTemplatesRegex says you're getting a regex, not templates; add "By."
 * No need to create a Product and ProductSeries; create a series and use its product.

The unit tests mostly follow the pattern "create templates with names [...]; create a subset; select from subset by regex; verify output." Most of these would be one-liners if you had a method for "create templates with names [...] (parameter 1); create a subset; select from subset by regex (parameter 2); return list of names of the selected templates." The caller could then assertContentsEqual the result against a list of expected strings. That would also help with:
 * Making the output more obvious (the relevance of "templates[1:]" is less clear).
 * Testing lots of other things that could go wrong; see below.
 * Eliminating implicit state. You wouldn't need any setUp at all.

Other notes:
 * You may want to test that you're really getting a match, not a search: "xyz" should not match "y.*" As long as we're not taking the regex from user input but from existing template names, this is a bigger risk than unescaped quotes. (Not that I disagree with your approach of testing for those right from the start!)
 * "# Quotes in the pattern can be dangerous." To me that looks like you're specifying the appropriate behaviour! How about "quotes do not confuse the regex match"?
 * I'd test single and double quotes separately, not in the same string.
 * When you test for quoting robustness, you might as well set up a template to establish that you don't get any unexpected matches.
 * You don't exercise the case where no templates match.

Apart from those notes, the incremental diff has my unreserved blessing.

Jeroen