Code review comment for lp:~adeuring/launchpad/bug-675595-2

Revision history for this message
Māris Fogels (mars) wrote :

Hi Abel,

This code looks good! r=mars, with two comments:

First, on line 72 of the diff, I think you should include a comment that says "If the table prejoin failed, then this will issue two additional SQL queries".

Second, the SQL prejoins in lp.registry.browser.person look odd - it looks like ORM code that should live in the model has been placed in the view. Is there a better place to put these operations?

I recognize that a refactoring of the TaskSearchListingView is not trivial, so please consider the refactoring as optional. If you do want to try the refactoring, it should be in a follow-up branch.

With the above taken into consideration, I think you can land this.

Maris

review: Approve

« Back to merge proposal