launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

There's a general issue here that your preconditions for applying the restrictions are a little too strict. I'd suggest perhaps maintaining a map of {target type: [potentially relevant subscription targets]}, or just examining all subscription targets unconditionally if that performs reasonably.

I tried doing this in a single ROW ... IN clause as I suggested on the call, but didn't get very far before it tried to strangle me, mostly due to the fact that (distro, spn) tasks must be found by any (distro, NULL) subscription. It'd be possible with a bit of UNIONing, but I'm not sure it's worth it at this stage.

Specific comments inline below.

105	+ Select(
106	+ SS.distributionID,
107	+ tables=[SS],
108	+ where=(SS.sourcepackagenameID == None))))

Given how long the structsub code is, it might be worth dropping the newline in the middle there, and similarly compressing the other blocks. Even cutting just a few lines makes the whole thing seem a lot more manageable.

109	+ ss_clauses.append(In(
110	+ Row(BugTaskFlat.distribution_id,
111	+ BugTaskFlat.sourcepackagename_id),
112	+ Select(
113	+ ((SS.distributionID,
114	+ SS.sourcepackagenameID),),
115	+ tables=[SS])))

I don't think you need the double bracketing in the Select. Removing that also lets you get it onto one line. Same in a couple of other places.

125	+ if params.distroseries is not None:
126	+ distroseries_id = params.distroseries.id
127	+ parent_distro_id = params.distroseries.distributionID
128	+ else:
129	+ distroseries_id = 0
130	+ parent_distro_id = 0

I've never understood this bit. It looks like it will just ignore DSP structsubs for SP tasks unless queried in a distroseries context? 

141	+ if is_person_search or params.product is not None:
142	+ ss_clauses.append(In(
143	+ BugTaskFlat.product_id,
144	+ Select(
145	+ SS.productID,
146	+ tables=[SS])))

What about project groups? They show multiple products, but this will ignore all of the product subscriptions. Also, the entire Select() expression can be one line.

153	+ if is_person_search or params.project is not None:

This will break in one obscure case: if searching in a product, it won't return any bugs if the only structural subscription is through the project group.

163	+ if is_person_search or params.milestone is not None:

This needs to apply in basically all cases. Milestone subscriptions affect all targets.

183	+ # Remove bugtasks from deactivated products.
184	+ # This is needed for searches where people are the context.
185	+ if is_person_search:

This also breaks in the case of project group searches.
-- 
https://code.launchpad.net/~sinzui/launchpad/bug-subselect-timeout/+merge/126566
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.