Code review comment for lp:~deryck/launchpad/milestone-portlet-links-385719

On Sep 18, 2009, at 07:30 PM, Deryck Hodge wrote:

>Deryck Hodge has proposed merging lp:~deryck/launchpad/milestone-portlet-links-385719 into lp:launchpad/devel.
>
>Requested reviews:
> Canonical Launchpad Engineering (launchpad)
>
>= Summary =
>
>Bug #385719 notes that it's not possible to get to a milestone bug
>listing from the bugs home page for a project (or distro either). This
>branch adds a milestone-targeted bugs portlet to a bugs home page for a
>distro or project, as well as for series bug pages.

Nice. I often that with LP2.0 it's very difficult to get to a milestone
without url hacking. So making more links to milestones always makes me
happy. This is a nice branch, and thanks for working on it.

I have only minor style issues with the code, but they should be easy to fix,
so merge-conditional, r=me with their consideration.

 review approve code ui*
 status approve

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py 2009-09-10 12:47:31 +0000
+++ lib/lp/bugs/browser/bugtarget.py 2009-09-18 19:01:31 +0000
> @@ -1142,11 +1136,20 @@
> elif IProductSeries(self.context, None):
> serieses = self.context.product.serieses
> else:
> - raise AssertionError, ("series_bug_counts called with "
> - "illegal context")
> -
> + raise AssertionError, ("series_list called with illegal context")

Better to spell that:

            raise AssertionError('series_list called with illegal context')

IOW, better to instantiate exceptions explicitly than to let Python do it
implicitly.

> + return serieses

"serieses" - waauugghh! ;)

> +
> + @property
> + def series_buglistings(self):
> + """Return a buglisting for each series.
> +
> + The list is sorted newest series to oldest.
> +
> + The count only considers bugs that the user would actually be
> + able to see in a listing.
> + """
> series_buglistings = []
> - for series in serieses:
> + for series in self.series_list:
> series_bug_count = series.open_bugtasks.count()
> if series_bug_count > 0:
> series_buglistings.append(
> @@ -1157,6 +1160,21 @@
>
> return series_buglistings
>
> + @property
> + def milestone_buglistings(self):
> + """Return a buglisting for each milestone."""
> + milestone_buglistings = []
> + for series in self.series_list:
> + for milestone in series.milestones:
> + milestone_bug_count = milestone.open_bugtasks.count()
> + if milestone_bug_count > 0:
> + milestone_buglistings.append(
> + dict(
> + title=milestone.name,
> + url=canonical_url(milestone),
> + count=milestone_bug_count))

That last line should be
                            count=milestone_bug_count,
                            ))

=== added file 'lib/lp/bugs/stories/bugs/xx-portlets-bug-milestones.txt'
--- lib/lp/bugs/stories/bugs/xx-portlets-bug-milestones.txt 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/stories/bugs/xx-portlets-bug-milestones.txt 2009-09-18 18:41:39 +0000
> @@ -0,0 +1,76 @@
> +Milestone-targeted bugs portlet
> +===============================
> +
> +The milestone-targeted bugs portlet displays the number of bugs that have
> +been targeted to a specific milestone.
> +
> +This portlet is available from a distribution's bug page.
> +
> + >>> anon_browser.open("http://bugs.launchpad.dev/debian/+bugs")
> + >>> portlet = find_portlet(
> + ... anon_browser.contents, "Milestone-targeted bugs")
> + >>> print extract_text(portlet)
> + Milestone-targeted bugs
> + 3.1 1
> +
> +It is also available from the bug page for a series in the distribution.
> +
> + >>> anon_browser.open("http://bugs.launchpad.dev/debian/sarge/+bugs")
> + >>> portlet = find_portlet(
> + ... anon_browser.contents, "Milestone-targeted bugs")
> + >>> print extract_text(portlet)
> + Milestone-targeted bugs
> + 3.1 1
> +
> +The portlet is also available from a project's bugs home page. To demonstrate
> +this, a project has to first have one of its milestones associated with a bug.
> +If there are no milestones with bugs, then there is no milestone-targeted
> +portlet.
> +
> + >>> anon_browser.open("http://bugs.launchpad.dev/firefox")
> + >>> portlet = find_portlet(
> + ... anon_browser.contents, "Milestone-targeted bugs")
> + >>> portlet is None
> + True

Better to spell this:

    >>> print portlet
    None