Code review comment for lp://staging/~lifeless/launchpad/bug-793848

Revision history for this message
Robert Collins (lifeless) wrote :

Thanks for the review Henninge.
All your suggestions are useful and I'll act on them before landing this branch. The one about chaining however I won't - we end up with 3 lines with poor indenting (its too long).

The implements thing is interesting; it turns out that the security rules are not cross checking the implements() - the interfaces primary job is then to abstract out the method signature. I'll add them though we don't strictly need them - and I bet we're missing other implements within the code base.

The admin team checking is doing visibility checks - same as private bugs. The order_by thing nukes the default order_by - storm sets a query to find_spec.default_order, and we really want no order by to ensure we get the best query plan. (yes, its weird that the default is possibly ordered) - I'm just using belts and bracers to avoid surprises.

« Back to merge proposal