launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

78	+def get_archive_privacy_filter(user):
79	+ return [
80	+ Not(Archive._private),
81	+ Archive.ownerID.is_in(
82	+ Select(
83	+ TeamParticipation.teamID,
84	+ where=(TeamParticipation.person == user)))]

The indentation here is wrong, and it might be better to just say Or() rather than returning a list.

174	+ clauses.extend(
175	+ [BinaryPackageBuild.package_build == PackageBuild.id,
176	+ PackageBuild.build_farm_job == BuildFarmJob.id])

The opening square bracket should probably be on the previous line.

221	- AND SourcepackageName.name LIKE '%%%%' || %s || '%%%%'
222	- ''' % quote_like(name))
223	+ clauses.append(SourcePackageName.name.like(name))

These aren't equivalent. You need to use quote_like and wrap it in % to get a substring match; see Distribution.searchSourcePackageCaches's LIKE stuff for an example of hacking around Storm. There's also another instance of this in the following block.

274	+ if user is None:
275	+ clauses.append(Not(Archive._private))
276	+ elif not IPersonRoles(user).in_admin:
277	+ clauses.append(*get_archive_privacy_filter(user))

Shouldn't this be in get_archive_privacy_filter?

279	+ clauses.append(BuildFarmJob.builder_id == builder_id)

Can you put this in the initial clauses?

288	- queries = []
289	- clauseTables = []
290	+ clauses = []
291	+ origin = [PackageBuild]

Why the new table?
-- 
https://code.launchpad.net/~stevenk/launchpad/stormier-getbuildsforbuilder/+merge/126159
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.