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

On Nov 3, 2010, at 5:56 PM, Edwin Grubbs wrote:

> Review: Approve code
> 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.

Got both of these. It's odd, make lint didn't return this--is it possible for lint to be overcome by big diffs (the branch this depends on has huge sample data changes)?

>> 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?

Yes; I'm pushing up the fix for that too.

>> === 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?

I'm still waiting on EC2; I threw it to that earlier today, but I'm sure it'll catch this. If not I'll get a test put around it.

>> === 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.

Done.

Thanks, Edwin.