Merge lp://staging/~mbp/launchpad/858618-affecting-me into lp://staging/launchpad

robert suggests getting the losas to run the added query against prod to make sure it is <100ms on a plausible bad case (big project, enthusiastic bug filer).

The screenshot is a nice touch.

1 === modified file 'lib/lp/bugs/browser/bugtask.py'
2 --- lib/lp/bugs/browser/bugtask.py 2011-09-23 13:25:35 +0000
3 +++ lib/lp/bugs/browser/bugtask.py 2011-09-25 03:34:26 +0000
4 @@ -1907,6 +1907,14 @@
5 return get_buglisting_search_filter_url(assignee=self.user.name)
6
7 @property
8 + def my_affecting_bugs_url(self):
9 + """A URL to a list of bugs affecting the user, or None."""
10 + if self.user is None:
11 + return None
12 + return get_buglisting_search_filter_url(affecting_me=True,
13 + orderby='-date_last_updated')

Interesting that you don't specify exactly _when_ you return None: only when the user is not logged in, or when there are no affecting bugs. Is that deliberate elbow room for later refinement?

I was tempted to call for a unit test on this method, but that's a lot of cost for very little code. I'll get back to that later.

Oh, one other thing: we don't line-break argument lists like this. If you're going to break lines among arguments, start with a first line break right after the opening parenthesis:

    return get_buglisting_search_filter_url(
        affecting_me=True, orderby='-date_last_updated')

58 === modified file 'lib/lp/bugs/templates/bugtarget-portlet-bugfilters-content.pt'
59 --- lib/lp/bugs/templates/bugtarget-portlet-bugfilters-content.pt 2011-02-24 15:35:44 +0000
60 +++ lib/lp/bugs/templates/bugtarget-portlet-bugfilters-content.pt 2011-09-25 03:34:26 +0000
61 @@ -94,6 +94,20 @@
62 </a>
63 </td>
64 </tr>
65 + <tr tal:condition="view/user"
66 + tal:define="count view/my_affecting_bugs_count|nothing;
67 + plural string: Bugs affecting me;
68 + singular string: Bug affecting me;">
69 + <td class="bugs-count" tal:content="count" />
70 + <td class="bugs-link">
71 + <a tal:attributes="href string:${view/my_affecting_bugs_url}">
72 + <metal:message use-macro="context/@@+base-layout-macros/plural-message"/>
73 + </a>
74 + </td>
75 + </tr>

Funny how we have a macro to construct plural forms in TAL with relative convenience and yet it doesn't make things any easier to read. Me, I usually end up constructing such messages in the view instead. (We also have a reusable plural function in lp.services somewhere).

If what you have works, then it works. But do consider moving as much of this logic as possible into view code. That makes it a lot easier to test.

In the aggregate there's definitely enough here to be worth at least one happy-path integration test. You could render the view, with affecting bugs registered, in a unit test on the view and verify that you get the expected “<n> Bugs affecting me” message.

Jeroen

thanks for the review

the plural construction is the same as the other controls

can you suggest a good example of this style of integration test?

Try lib/lp/registry/browser/tests/test_distroseries.py; look for “html_content = view()” (which also neatly summarizes how to render a view in a test).

the sql looks like this:

SELECT COUNT(*) FROM BugTask JOIN Bug ON BugTask.bug = Bug.id WHERE Bug.id = BugTask.bug AND BugTask.product = 22 AND ((BugTask.status = 10) OR (BugTask.status = 15) OR (BugTask.status = 20) OR (BugTask.status = 21) OR (BugTask.status = 22) OR (BugTask.status = 25)) AND Bug.duplicateof is NULL AND
            BugTask.id IN (
                SELECT BugTask.id FROM BugTask, BugAffectsPerson
                WHERE BugTask.bug = BugAffectsPerson.bug
                AND BugAffectsPerson.person = 16
                AND BugAffectsPerson.affected = TRUE
            )

and some of the top user ids are in https://bugs.launchpad.net/launchpad/+bug/858618/comments/2 with one easy one being user 2, lifeless.

looking for bugs affecting lifeless in launchpad itself, on qas, explain analyze says about 22-45ms to me on staging. however, other runs on production show it being much longer, at around 200ms, which would be too much. i'll see if changing the subquery to a join will help.

... changing to a join doesn't consistently help.

removing the join to 'bug' (which is only used to remove duplicates) also makes it faster in some cases, but not reliably

i think i will cut out the count which avoids doing any extra work, though does make it unfortunately different from other links. perhaps it can be calculated lazily or approximately or cached, as separate work.

that looks like this: https://bugs.launchpad.net/launchpad/+bug/858618/+attachment/2477988/+files/20110929_001.jpeg

ui review:

<huwshimi> poolie: I would have thought having links to those pages would be super useful (is there currently any other way to view them without doing an advanced search?). I'm not sure I understand how it could make thinks worse.

huw approved; robert's concerns are addressed by not doing any more queries; jtv's concerns are addressed or obsolete by not doing any queries.