Code review comment for lp:~jcsackett/launchpad/convert-sql-627631

Hi JC,

This is a nice branch. I have a few comments below. Remember, you can't go on your honeymoon until you land this branch.

-Edwin

>=== modified file 'lib/lp/answers/model/question.py'
>--- lib/lp/answers/model/question.py 2010-10-03 15:30:06 +0000
>+++ lib/lp/answers/model/question.py 2010-11-03 21:39:12 +0000
>@@ -680,8 +681,8 @@
> LEFT OUTER JOIN Distribution ON (
> Question.distribution = Distribution.id)
> WHERE
>- (Product.official_answers is True
>- OR Distribution.official_answers is TRUE)
>+ (Product.answers_usage = %s

Trailing whitespace.

>+ OR Distribution.answers_usage = %s)
> AND Question.datecreated > (
> current_timestamp -interval '60 days')
> LIMIT 5000
>@@ -689,7 +690,8 @@
> GROUP BY product, distribution
> ORDER BY question_count DESC
> LIMIT %s
>- """ % sqlvalues(limit))
>+ """ % sqlvalues(
>+ ServiceUsage.LAUNCHPAD, ServiceUsage.LAUNCHPAD, limit))
>
> projects = []
> product_set = getUtility(IProductSet)
>
>=== modified file 'lib/lp/registry/model/projectgroup.py'
>--- lib/lp/registry/model/projectgroup.py 2010-11-02 20:10:56 +0000
>+++ lib/lp/registry/model/projectgroup.py 2010-11-03 21:39:12 +0000
>@@ -185,17 +185,12 @@

There is a bunch of trailing whitespace a little further up in this
file.

> def translatables(self):
> """See `IProjectGroup`."""
>- # XXX j.c.sackett 2010-08-30 bug=627631: Once data migration has
>- # happened for the usage enums, this sql needs to be updated to
>- # check for the translations_usage, not official_rosetta. At that
>- # time it should also be converted to a Storm query and the issue with
>- # has_translatables resolved.

Can you fix the problem with has_translatables now?

> return Product.select('''
> Product.project = %s AND
>- Product.official_rosetta = TRUE AND
>+ Product.translations_usage = %s AND
> Product.id = ProductSeries.product AND
> POTemplate.productseries = ProductSeries.id
>- ''' % sqlvalues(self),
>+ ''' % sqlvalues(self, ServiceUsage.LAUNCHPAD),
> clauseTables=['ProductSeries', 'POTemplate'],
> distinct=True)
>
>
>=== modified file 'lib/lp/translations/model/translationsoverview.py'
>--- lib/lp/translations/model/translationsoverview.py 2010-08-31 23:03:45 +0000
>+++ lib/lp/translations/model/translationsoverview.py 2010-11-03 21:39:12 +0000
>@@ -67,14 +65,16 @@
> distribution=distribution.id
> WHERE category=3 AND
> (product IS NOT NULL OR distribution IS NOT NULL) AND
>- (product.official_rosetta OR
>- distribution.official_rosetta)
>+ (product.translations_usage = %s OR
>+ distribution.tranlsations_usage = %s)
> GROUP BY product.displayname, product.id,
> distribution.displayname, distribution.id
> HAVING SUM(karmavalue) > 0
> ORDER BY total_karma DESC
> LIMIT %d) AS something
>- ORDER BY name""" % int(limit)
>+ ORDER BY name""" % (ServiceUsage.LAUNCHPAD,
>+ ServiceUsage.LAUNCHPAD,
>+ int(limit))

sqlvalues() should be used here. It was ok before when it just had
%d, which would make sure only a valid value is used. I'd be surprised
if this hasn't broken a test. Can you make sure this is tested
somewhere?

> cur = cursor()
> cur.execute(query)
>
>
>=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
>--- lib/lp/translations/scripts/translations_to_branch.py 2010-10-02 11:41:43 +0000
>+++ lib/lp/translations/scripts/translations_to_branch.py 2010-11-03 21:39:12 +0000
>@@ -306,14 +306,12 @@
>
> self.store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
>
>- # XXX j.c.sackett 2010-08-30 bug=627631 Once data migration has
>- # happened for the usage enums, this sql needs to be updated to
>- # check for the translations_usage, not official_rosetta.
> product_join = Join(
> ProductSeries, Product, ProductSeries.product == Product.id)
> productseries = self.store.using(product_join).find(
> ProductSeries, SQL(
>- "official_rosetta AND translations_branch IS NOT NULL"))
>+ "translations_usage = %s AND translations_branch IS NOT NULL"
>+ % ServiceUsage.LAUNCHPAD))

There is no reason that SQL() needs to be used here. Please convert
it to storm conditionals.

> # Anything deterministic will do, and even that is only for
> # testing.
>