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

Hi Māris,

thanks for the review!

On 23.11.2010 16:16, Māris Fogels wrote:
> Review: Approve
> 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".

Good idea, added.

>
> 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 am not if it is worth the effort: Basically, we can now pre-join more
or less arbitrary tables in calls of BugTaskSet.searchTasks(). This
method is used quite frequently, and for each callsite we might need to
pre-join other tables. There might even be conflicts: We can at present
join the same table only once, and for one callsite it might be
reasonable to pre-join for exmaple Person in order to retrieve a bug
reporter, while another callsite would benefit from pre-joining a
bugtask assignee.

So I think that the callsites of searchTasks() should decide what to
pre-join -- and these callsite live quite often in browser code. But
perhaps I am too much focused on the way how I implemented the branch
and I'm completely missing some better way to tell searchTasks() what
pre-join...

Abel

>
> 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