Code review comment for lp://staging/~mbp/launchpad/858618-affecting-me

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