Merge lp:~jcsackett/launchpad/convert-sql-627631-storm into lp:launchpad/db-devel

Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 10000
Proposed branch: lp:~jcsackett/launchpad/convert-sql-627631-storm
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~jcsackett/launchpad/convert-sql-627631
Diff against target: 233 lines (+62/-25)
7 files modified
lib/canonical/launchpad/database/launchpadstatistic.py (+7/-5)
lib/lp/registry/model/product.py (+5/-1)
lib/lp/registry/model/projectgroup.py (+18/-13)
lib/lp/translations/model/translationsperson.py (+7/-2)
lib/lp/translations/scripts/tests/test_translations_to_branch.py (+2/-1)
lib/lp/translations/scripts/translations_to_branch.py (+6/-3)
lib/lp/translations/stories/standalone/xx-products-with-translations.txt (+17/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/convert-sql-627631-storm
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Benji York (community) code Approve
Review via email: mp+41011@code.launchpad.net

Commit message

[r=benji,edwin-grubbs][ui=none][bug=627631] Update storm queries to use usage enums instead of official booleans.

Description of the change

Summary
=======

There's an ongoing effort to phase out the official_* booleans which indicate how a product/distribution uses the various service on Launchpad. While previous branches cleaned up a lot of model code, they didn't address queries made to the db that used the Boolean columns.

This branch rectifies that by cleaning up those sections of code to reference the EnumCol, so that we can (if we choose to) drop the official_* code completely.

This branch was split out separately from the one it depends on b/c it seemed that storm queries needed special handling, and were related to some other issues in the code base.

Proposed fix
============

Replace locations making Storm queries and update them to use the EnumCol and ServiceUsage DBItem.

Preimplementation talk
======================

Spoke with Curtis Hovey.

Implementation details
======================

As in proposed. Additionally, as low hanging fruit a query was converted to storm to fix a related issue in projectgroups.

Tests
=====

Because of the number of things that end up effected, most modules need testing. Testing registry alone is a reasonable proxy, but not ideal.

bin/test -m lp.registry.tests

OR

bin/test -m lp.registry
bin/test -m lp.translations
bin/test -m lp.answers

Demo & QA
=========

Operation of the various main pages (e.g. +translations, +questions) for a product or distribution shouldn't fail in anyway or appear different; b/c of prior work this change should be invisible.

Lint
====

make lint output:

= Launchpad lint =
  2
  3 Checking for conflicts and issues in changed files.
  4
  5 Linting changed files:
  6 lib/canonical/launchpad/database/launchpadstatistic.py
  7 lib/lp/answers/model/question.py
  8 lib/lp/registry/doc/product.txt
  9 lib/lp/registry/model/product.py
 10 lib/lp/registry/model/projectgroup.py
 11 lib/lp/translations/doc/translationsoverview.txt
 12 lib/lp/translations/model/translationsoverview.py
 13 lib/lp/translations/model/translationsperson.py
 14 lib/lp/translations/scripts/translations_to_branch.py
 15

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

This branch looks good.

Replacing hard-coded SQL with Storm expressions is a nice bonus.

I'm curious why _translations_usage has a leading underscore, but that
wasn't introduced in this branch, so not really relevant.

Most people include the lint output as you do, but personally I prefer
to include the output of lint /before/ I fix the things it notes. That
way the reviewer can know which parts of the diff were lint and which
were integral to the task at hand. Not a big deal, and an opinion that
may not be shared by others; just something to think about.

review: Approve (code)
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi JC,

It's a good thing Benji pointed out the odd leading underscore in "_translations_usage" since this is the cause of a serious bug in your queries, and it is probably something that storm or the sqlobject layer on top of it should warn about.

Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD) actually generates the SQL:
 WHERE FALSE
since Person.translations_usage is a property object and not a storm attribute.

Product.selectBy(_translations_usage=ServiceUsage.LAUNCHPAD) generates the SQL:
 WHERE Product.translations_usage = 20

The need for a property to override the setter can be done in storm with a validator. However, I'm not sure if you will still need to override the getter, which would still require a translations_usage property, and the storm attribute would still need to be renamed _translations_usage.

-Edwin

Revision history for this message
j.c.sackett (jcsackett) wrote :

> I'm curious why _translations_usage has a leading underscore, but that
> wasn't introduced in this branch, so not really relevant.

It's part of the long running effort to migrate to these enums. _translations_usage is the enum column. translations_usage is a property that uses _translations_usage.

At some point there will be a branch that removes the properties, once we're 100% sure we no longer need them.

Revision history for this message
j.c.sackett (jcsackett) wrote :

> Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD) actually generates the SQL:
> WHERE FALSE
> since Person.translations_usage is a property object and not a storm attribute.
>
> Product.selectBy(_translations_usage=ServiceUsage.LAUNCHPAD) generates the SQL:
> WHERE Product.translations_usage = 20

I got this error in another place, didn't realize I hadn't been caught everywhere. Thanks for the note. I'll fix that.

Revision history for this message
Benji York (benji) wrote :

The fact that the bad query made it through suggests that there's a missing test somewhere.

Revision history for this message
j.c.sackett (jcsackett) wrote :

Actually, it suggests I need to pay more attention to which set of test results I'm looking at. :-P

This test result got mixed up with the other branch I had going of a very similar name.

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi JC,

It bothers me a little bit that you will be converting queries over to _translations_usage and then back to translations_usage once the property is removed, but I guess that will be a simple search and replace, so it should be fine to land this branch now.

-Edwin

review: Approve (code)
Revision history for this message
j.c.sackett (jcsackett) wrote :

> It bothers me a little bit that you will be converting queries over to _translations_usage and then back to translations_usage once the property is removed, but I guess that will be a simple search and replace, so it should be fine to land this branch now.

It bothers me a little bit too. Perhaps I misunderstood some of your earlier comments--there's a way to have the query use translations_usage rather than _translations_usage and still have it work? The validator stuff I have seen doesn't seem applicable.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/database/launchpadstatistic.py'
2--- lib/canonical/launchpad/database/launchpadstatistic.py 2010-11-15 16:44:08 +0000
3+++ lib/canonical/launchpad/database/launchpadstatistic.py 2010-11-24 14:50:20 +0000
4@@ -105,8 +105,7 @@
5
6 self.update(
7 'products_using_malone',
8- Product.selectBy(official_malone=True).count()
9- )
10+ Product.selectBy(official_malone=True).count())
11 ztm.commit()
12
13 cur = cursor()
14@@ -164,10 +163,14 @@
15 Product.selectBy(license_reviewed=True, active=True).count())
16
17 def _updateRosettaStatistics(self, ztm):
18+ # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
19+ # this query is using _translations_usage, but there's no cleaner
20+ # way to deal with it. Once the bug above is resolved, this should
21+ # should be fixed to use translations_usage.
22 self.update(
23 'products_using_rosetta',
24- Product.selectBy(translations_usage=ServiceUsage.LAUNCHPAD).count()
25- )
26+ Product.selectBy(
27+ _translations_usage=ServiceUsage.LAUNCHPAD).count())
28 self.update('potemplate_count', POTemplate.select().count())
29 ztm.commit()
30 self.update('pofile_count', POFile.select().count())
31@@ -225,4 +228,3 @@
32 "FROM Question")
33 self.update("projects_with_questions_count", cur.fetchone()[0] or 0)
34 ztm.commit()
35-
36
37=== modified file 'lib/lp/registry/model/product.py'
38--- lib/lp/registry/model/product.py 2010-11-21 09:52:02 +0000
39+++ lib/lp/registry/model/product.py 2010-11-24 14:50:20 +0000
40@@ -1567,12 +1567,16 @@
41
42 def getTranslatables(self):
43 """See `IProductSet`"""
44+ # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
45+ # this query is using _translations_usage, but there's no cleaner
46+ # way to deal with it. Once the bug above is resolved, this should
47+ # should be fixed to use translations_usage.
48 results = IStore(Product).find(
49 (Product, Person),
50 Product.active == True,
51 Product.id == ProductSeries.productID,
52 POTemplate.productseriesID == ProductSeries.id,
53- Product.official_rosetta == True,
54+ Product._translations_usage == ServiceUsage.LAUNCHPAD,
55 Person.id == Product._ownerID).config(
56 distinct=True).order_by(Product.title)
57
58
59=== modified file 'lib/lp/registry/model/projectgroup.py'
60--- lib/lp/registry/model/projectgroup.py 2010-11-15 22:22:38 +0000
61+++ lib/lp/registry/model/projectgroup.py 2010-11-24 14:50:20 +0000
62@@ -21,6 +21,7 @@
63 from storm.expr import (
64 And,
65 SQL,
66+ Join,
67 )
68 from storm.locals import Int
69 from storm.store import Store
70@@ -104,6 +105,7 @@
71 )
72 from lp.services.worlddata.model.language import Language
73 from lp.translations.enums import TranslationPermission
74+from lp.translations.model.potemplate import POTemplate
75
76
77 class ProjectGroup(SQLBase, BugTargetBase, HasSpecificationsMixin,
78@@ -184,22 +186,25 @@
79
80 def translatables(self):
81 """See `IProjectGroup`."""
82- return Product.select('''
83- Product.project = %s AND
84- Product.translations_usage = %s AND
85- Product.id = ProductSeries.product AND
86- POTemplate.productseries = ProductSeries.id
87- ''' % sqlvalues(self, ServiceUsage.LAUNCHPAD),
88- clauseTables=['ProductSeries', 'POTemplate'],
89- distinct=True)
90+ store = Store.of(self)
91+ origin = [
92+ Product,
93+ Join(ProductSeries, Product.id == ProductSeries.productID),
94+ Join(POTemplate, ProductSeries.id == POTemplate.productseriesID),
95+ ]
96+ # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
97+ # this query is using _translations_usage, but there's no cleaner
98+ # way to deal with it. Once the bug above is resolved, this should
99+ # should be fixed to use translations_usage.
100+ return store.using(*origin).find(
101+ Product,
102+ Product.project == self.id,
103+ Product._translations_usage == ServiceUsage.LAUNCHPAD,
104+ ).config(distinct=True)
105
106 def has_translatable(self):
107 """See `IProjectGroup`."""
108- # XXX: BradCrittenden 2010-10-12 bug=659078: The test should be
109- # converted to use is_empty but the implementation in storm's
110- # sqlobject wrapper is broken.
111- # return not self.translatables().is_empty()
112- return self.translatables().count() != 0
113+ return not self.translatables().is_empty()
114
115 def has_branches(self):
116 """ See `IProjectGroup`."""
117
118=== modified file 'lib/lp/translations/model/translationsperson.py'
119--- lib/lp/translations/model/translationsperson.py 2010-11-15 17:38:25 +0000
120+++ lib/lp/translations/model/translationsperson.py 2010-11-24 14:50:20 +0000
121@@ -24,6 +24,7 @@
122
123 from canonical.database.sqlbase import sqlvalues
124 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
125+from lp.app.enums import ServiceUsage
126 from lp.registry.interfaces.person import IPerson
127 from lp.registry.model.distribution import Distribution
128 from lp.registry.model.distroseries import DistroSeries
129@@ -277,16 +278,20 @@
130 # translation focus.
131 distrojoin_conditions = And(
132 Distribution.id == DistroSeries.distributionID,
133- Distribution.official_rosetta == True,
134+ Distribution._translations_usage == ServiceUsage.LAUNCHPAD,
135 Distribution.translation_focusID == DistroSeries.id)
136
137 DistroJoin = LeftJoin(Distribution, distrojoin_conditions)
138
139 ProductSeriesJoin = LeftJoin(
140 ProductSeries, ProductSeries.id == POTemplate.productseriesID)
141+ # XXX j.c.sackett 2010-11-19 bug=677532 It's less than ideal that
142+ # this query is using _translations_usage, but there's no cleaner
143+ # way to deal with it. Once the bug above is resolved, this should
144+ # should be fixed to use translations_usage.
145 ProductJoin = LeftJoin(Product, And(
146 Product.id == ProductSeries.productID,
147- Product.official_rosetta == True))
148+ Product._translations_usage == ServiceUsage.LAUNCHPAD))
149
150 ProjectJoin = LeftJoin(
151 ProjectGroup, ProjectGroup.id == Product.projectID)
152
153=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
154--- lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-11-18 20:39:59 +0000
155+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-11-24 14:50:20 +0000
156@@ -17,6 +17,7 @@
157 from canonical.launchpad.scripts.logger import QuietFakeLogger
158 from canonical.launchpad.scripts.tests import run_script
159 from canonical.testing.layers import ZopelessAppServerLayer
160+from lp.app.enums import ServiceUsage
161 from lp.registry.interfaces.teammembership import (
162 ITeamMembershipSet,
163 TeamMembershipStatus,
164@@ -59,7 +60,7 @@
165 # Set up a translations_branch for the series.
166 db_branch, tree = self.create_branch_and_tree(product=product)
167 removeSecurityProxy(db_branch).last_scanned_id = 'null:'
168- product.official_rosetta = True
169+ product.translations_usage = ServiceUsage.LAUNCHPAD
170 series.translations_branch = db_branch
171
172 # Set up a template & Dutch translation for the series.
173
174=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
175--- lib/lp/translations/scripts/translations_to_branch.py 2010-11-20 15:03:58 +0000
176+++ lib/lp/translations/scripts/translations_to_branch.py 2010-11-24 14:50:20 +0000
177@@ -16,8 +16,8 @@
178 from bzrlib.errors import NotBranchError
179 import pytz
180 from storm.expr import (
181+ And,
182 Join,
183- SQL,
184 )
185 from zope.component import getUtility
186
187@@ -32,6 +32,7 @@
188 MAIN_STORE,
189 SLAVE_FLAVOR,
190 )
191+from lp.app.enums import ServiceUsage
192 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
193 from lp.code.model.directbranchcommit import (
194 ConcurrentUpdateError,
195@@ -316,8 +317,10 @@
196 product_join = Join(
197 ProductSeries, Product, ProductSeries.product == Product.id)
198 productseries = self.store.using(product_join).find(
199- ProductSeries, SQL(
200- "official_rosetta AND translations_branch IS NOT NULL"))
201+ ProductSeries,
202+ And(
203+ Product._translations_usage == ServiceUsage.LAUNCHPAD,
204+ ProductSeries.translations_branch != None))
205
206 # Anything deterministic will do, and even that is only for
207 # testing.
208
209=== modified file 'lib/lp/translations/stories/standalone/xx-products-with-translations.txt'
210--- lib/lp/translations/stories/standalone/xx-products-with-translations.txt 2009-09-11 11:07:49 +0000
211+++ lib/lp/translations/stories/standalone/xx-products-with-translations.txt 2010-11-24 14:50:20 +0000
212@@ -1,5 +1,22 @@
213 = Products with translations =
214
215+We have to do a little set up.
216+
217+ >>> from zope.component import getUtility
218+ >>> from lp.app.enums import ServiceUsage
219+ >>> from lp.registry.interfaces.product import IProductSet
220+ >>> from lp.testing import (
221+ ... login,
222+ ... logout,
223+ ... )
224+ >>> login('admin@canonical.com')
225+ >>> evolution = getUtility(IProductSet).getByName('evolution')
226+ >>> alsa = getUtility(IProductSet).getByName('alsa-utils')
227+ >>> evolution.translations_usage = ServiceUsage.LAUNCHPAD
228+ >>> alsa.translations_usage = ServiceUsage.LAUNCHPAD
229+ >>> transaction.commit()
230+ >>> logout()
231+
232 The +products-with-translations page lists all translatable products in
233 Launchpad.
234

Subscribers

People subscribed via source and target branches

to status/vote changes: