> === modified file 'lib/canonical/launchpad/icing/style.css'
> --- lib/canonical/launchpad/icing/style.css     2010-01-20 21:57:44 +0000
> +++ lib/canonical/launchpad/icing/style.css     2010-01-28 17:24:20 +0000
> @@ -1458,7 +1458,7 @@
>
>  div.popupTitle {
>   background: #ffffdc;
> -  padding: 0 1em;
> +  padding: 0.5em 1em;
>   border: 1px black solid;
>   position: absolute;
>   display: none;
>
> === modified file 'lib/lp/bugs/browser/bugtarget.py'
> --- lib/lp/bugs/browser/bugtarget.py    2010-01-07 05:41:58 +0000
> +++ lib/lp/bugs/browser/bugtarget.py    2010-01-28 17:24:20 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>
>  """IBugTarget-related browser views."""
> @@ -15,12 +15,15 @@
>     "FileBugViewBase",
>     "OfficialBugTagsManageView",
>     "ProjectFileBugGuidedView",
> +    "BugsPatchesView",
>     ]

Let's sort this alphabetically, so that it's easier to locate an export later.

>
>  import cgi
>  from cStringIO import StringIO
> +from datetime import datetime
>  from email import message_from_string
>  from operator import itemgetter
> +from pytz import timezone
>  from simplejson import dumps
>  import tempfile
>  import urllib
> @@ -40,6 +43,7 @@
>  from canonical.config import config
>  from lp.bugs.browser.bugtask import BugTaskSearchListingView
>  from lp.bugs.interfaces.bug import IBug
> +from lp.bugs.interfaces.bugattachment import BugAttachmentType
>  from canonical.launchpad.browser.feeds import (
>     BugFeedLink, BugTargetLatestBugsFeedLink, FeedsMixin,
>     PersonLatestBugsFeedLink)
> @@ -73,6 +77,7 @@
>     LaunchpadEditFormView, LaunchpadFormView, LaunchpadView, action,
>     canonical_url, custom_widget, safe_action)
>  from canonical.launchpad.webapp.authorization import check_permission
> +from canonical.launchpad.webapp.batching import BatchNavigator
>  from canonical.launchpad.webapp.tales import BugTrackerFormatterAPI
>  from canonical.launchpad.validators.name import valid_name_pattern
>  from canonical.launchpad.webapp.menu import structured
> @@ -1372,3 +1377,51 @@
>  class BugsVHostBreadcrumb(Breadcrumb):
>     rootsite = 'bugs'
>     text = 'Bugs'
> +
> +
> +class BugsPatchesView(LaunchpadView):
> +    """View list of patch attachments associated with bugs."""
> +
> +    @property
> +    def label(self):
> +        """The display label for the view."""
> +        return 'Patch attachments in %s' % self.context.displayname
> +
> +    def batchedPatchTasks(self):
> +        """Return a BatchNavigator for bug tasks with patch attachments."""
> +        any_unresolved_plus_fixreleased = \
> +            UNRESOLVED_BUGTASK_STATUSES + (BugTaskStatus.FIXRELEASED,)

We avoid using line continuation in Launchpad code, because we don't like how
it messes with the interpretation of the text file (the parser consideres
everything after the \ to actually be on the same line). Instead we use
parentheses, like this:

       any_unresolved_plus_fixreleased = (
           UNRESOLVED_BUGTASK_STATUSES + (BugTaskStatus.FIXRELEASED,))

Also, since this value doesn't ever change I think it would be appropriate to
move it to the module's topmost level and use an ALL_CAPITAL identifier.


> +        return BatchNavigator(
> +            self.context.searchTasks(None, user=self.user,
> +                                     order_by=self.request.get("orderby"),

Why do you need to read from the request directly? If there's a zope form
covering this, you can get the value from self.request.form. If there isn't a
form then maybe we should consider having one, since it gives us validation
"for free" (here it seems there's no validation at all).

> +                                     status=any_unresolved_plus_fixreleased,
> +                                     omit_duplicates=True, has_patch=True),
> +            self.request)
> +
> +    def shouldShowTargetName(self):
> +        """Return True if current context can have different bugtargets."""
> +        return (IDistribution.providedBy(self.context)
> +                or IDistroSeries.providedBy(self.context)
> +                or IProject.providedBy(self.context))

I would prefer the combining operator to be on the end of the previous lines,
so that the constituents being combined align nicely to the left. I don't think
this is a requirement, though, so only change this if you agree with me.

> +
> +    def youngest_patch(self, bug):

Following ZOPE, we deviate from PEP-8 by using camelCase for method names. This
one should be called youngestPatch.

> +        """Return the youngest patch attached to a bug, else error."""
> +        youngest = None
> +        # Loop over bugtasks, gathering youngest patch for each's bug.
> +        for a in bug.attachments:

Let's avoid a single-character identifier. 'attachment' takes a bit longer to
write, but is much easier to read.

> +            if a.type == BugAttachmentType.PATCH:

If you want you can use the helper property is_patch, instead of comparing the
patch type.

> +                if youngest is None:
> +                    youngest = a
> +                else:
> +                    if (a.message.datecreated > youngest.message.datecreated):
> +                        youngest = a
> +        if youngest is None:
> +            # This is the patches view, so every bug under
> +            # consideration should have at least one patch attachment.
> +            raise AssertionError("bug %i has no patch attachments" % bug.id)
> +        return youngest

Another way to express this would be: (achtung: untested code)

    patches = sorted(
        [attachment for attachment in bug.attachments if attachment.is_patch],
        operator.itemgetter('message.datecreated'))
    youngest = patches[-1] if len(patches) > 0 or None

Both ways are fine, so you don't need to change this unless you feel like.

> +
> +    def patch_age(self, patch):

camelCase

> +        """Return a timedelta object for the age of a patch attachment."""
> +        now = datetime.now(timezone('UTC'))
> +        return now - patch.message.datecreated
>
> === modified file 'lib/lp/bugs/browser/configure.zcml'
> --- lib/lp/bugs/browser/configure.zcml  2009-12-09 20:48:01 +0000
> +++ lib/lp/bugs/browser/configure.zcml  2010-01-28 17:24:20 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
>      GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>
> @@ -108,6 +108,13 @@
>             facet="bugs"
>             permission="launchpad.Edit"
>             template="../templates/official-bug-target-manage-tags.pt"/>
> +        <browser:page
> +            name="+patches"
> +            for="lp.bugs.interfaces.bugtarget.IHasBugs"
> +            class="lp.bugs.browser.bugtarget.BugsPatchesView"
> +            facet="bugs"
> +            permission="zope.Public"
> +            template="../templates/bugtarget-patches.pt"/>
>     </facet>
>     <browser:page
>         name="+bugtarget-macros-search"
>
> === modified file 'lib/lp/bugs/interfaces/bugtarget.py'
> --- lib/lp/bugs/interfaces/bugtarget.py 2009-08-18 11:12:06 +0000
> +++ lib/lp/bugs/interfaces/bugtarget.py 2010-01-28 17:24:20 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>
>  # pylint: disable-msg=E0211,E0213
>
> === modified file 'lib/lp/bugs/model/bugtarget.py'
> --- lib/lp/bugs/model/bugtarget.py      2010-01-21 16:47:24 +0000
> +++ lib/lp/bugs/model/bugtarget.py      2010-01-28 17:24:20 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>
>  # pylint: disable-msg=E0611,W0212
>
> === added directory 'lib/lp/bugs/stories/patches-view'
> === added file 'lib/lp/bugs/stories/patches-view/patches-view.txt'
> --- lib/lp/bugs/stories/patches-view/patches-view.txt   1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/stories/patches-view/patches-view.txt   2010-01-28 17:24:20 +0000
> @@ -0,0 +1,301 @@
> +Patches View
> +============
> +
> +Patches View by Product
> +-----------------------
> +
> +We have a view listing patches attached to bugs that target a given
> +product.  At first, the product is new and has no bugs.
> +
> +    >>> def make_thing(factory_method, **thing_args):
> +    ...     login('<email address hidden>')
> +    ...     result = factory_method(**thing_args)
> +    ...     transaction.commit()
> +    ...     logout()
> +    ...     return result

That's a nice helper function. Let's extract it out of this test and make it
generally available to all tests. It probably deserves a more descriptive name,
though.

> +
> +    >>> patchy_product = make_thing(
> +    ...     factory.makeProduct, name='patchy-product-1')
> +
> +We don't see any patches when we open the patches view.
> +
> +    >>> def show_patches_view(expected_contents):
> +    ...     for tag in find_tags_by_class(
> +    ...         expected_contents, 'listing'):
> +    ...         print extract_text(tag)
> +
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug Importance Status Patch Age
> +
> +After the product has a bug, it still doesn't show up in the patches
> +view, because that bug has no patch attachments.
> +
> +    >>> from lp.bugs.interfaces.bugtask import (
> +    ...     BugTaskImportance, BugTaskStatus)
> +    >>> def make_bug(
> +    ...     title, product, importance=BugTaskImportance.UNDECIDED,
> +    ...     status=BugTaskStatus.NEW):
> +    ...     bug = factory.makeBug(title=title, product=product)
> +    ...     bug.default_bugtask.transitionToImportance(
> +    ...         importance, product.owner)
> +    ...     bug.default_bugtask.transitionToStatus(
> +    ...         status, product.owner)
> +    ...     return bug
> +
> +    >>> bug_a = make_thing(
> +    ...     make_bug, title="bug_a title", product=patchy_product)

This made me scratch my head. Isn't this just a weird calling convention. Then
I realise make_thing does the extra work of logging you in and out. Hence my
suggestion of a more descriptive name.

> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug Importance Status Patch Age
> +
> +After we add a non-patch attachment to that bug, the patches view
> +still shows no patches.
> +
> +    >>> make_thing(factory.makeBugAttachment, bug=bug_a, is_patch=False)
> +    <BugAttachment at...
> +    >>> user_browser.open('http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug Importance Status Patch Age
> +
> +After we add a patch attachment that's one day old, we see it in the
> +patches view.
> +
> +    >>> patch_submitter = make_thing(
> +    ...    factory.makePerson, name="patchy-person",
> +    ...    displayname="Patchy Person")
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch a",
> +    ...     filename="patch_a.diff", owner=patch_submitter,
> +    ...     description="description of patch a", bug=bug_a, is_patch=True)
> +    <BugAttachment at...
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                                 Importance   Status    Patch Age
> +    Bug #16: bug_a title                  Undecided     New     ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
> +
> +After creating some more bugs, with some non-patch and some patch
> +attachments...
> +
> +    >>> bug_b = make_thing(
> +    ...     make_bug, title="bug_b title", product=patchy_product,
> +    ...     importance=BugTaskImportance.CRITICAL,
> +    ...     status=BugTaskStatus.CONFIRMED)
> +    >>> bug_c = make_thing(
> +    ...     make_bug, title="bug_c title", product=patchy_product,
> +    ...     importance=BugTaskImportance.WISHLIST,
> +    ...     status=BugTaskStatus.FIXCOMMITTED)
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch b",
> +    ...     filename="patch_b.diff", owner=patch_submitter,
> +    ...     description="description of patch b", bug=bug_b, is_patch=True)
> +    <BugAttachment at...
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch c",
> +    ...     filename="patch_c.diff", owner=patch_submitter,
> +    ...     description="description of patch c", bug=bug_b, is_patch=True)
> +    <BugAttachment at...
> +    >>> make_thing(factory.makeBugAttachment, bug=bug_c, is_patch=False)
> +    <BugAttachment at...
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch d",
> +    ...     filename="patch_d.diff", owner=patch_submitter,
> +    ...     description="description of patch d", bug=bug_c, is_patch=True)
> +    <BugAttachment at...
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch e",
> +    ...     filename="patch_e.diff", owner=patch_submitter,
> +    ...     description="description of patch e", bug=bug_c, is_patch=True)
> +    <BugAttachment at...
> +    >>> make_thing(
> +    ...     factory.makeBugAttachment, comment="comment about patch f",
> +    ...     filename="patch_f.diff", owner=patch_submitter,
> +    ...     description="description of patch f", bug=bug_c, is_patch=True)
> +    <BugAttachment at...
> +
> +...the youngest patch on each bug is visible is the patch report.
> +
> +    >>> user_browser.open('http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                                 Importance   Status    Patch Age
> +    Bug #17: bug_b title                  Critical    Confirmed ...second...
> +    From: Patchy Person
> +    Link: patch_c.diff description of patch c
> +    Bug #18: bug_c title                  Wishlist  Fix Committed ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +    Bug #16: bug_a title                  Undecided     New     ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
> +
> +We can sort patches by importance and status.
> +
> +    >>> user_browser.getControl(name="orderby").value = ['-importance']
> +    >>> user_browser.getControl("sort").click()
> +    >>> user_browser.url
> +    'http://bugs.launchpad.dev/patchy-product-1/+patches?orderby=-importance'
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                                 Importance   Status    Patch Age
> +    Bug #17: bug_b title                  Critical    Confirmed ...second...
> +    From: Patchy Person
> +    Link: patch_c.diff description of patch c
> +    Bug #18: bug_c title                  Wishlist  Fix Committed ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +    Bug #16: bug_a title                  Undecided     New     ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
> +
> +    >>> user_browser.getControl(name="orderby").value = ['status']
> +    >>> user_browser.getControl("sort").click()
> +    >>> user_browser.url
> +    'http://bugs.launchpad.dev/patchy-product-1/+patches?orderby=status'
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                                 Importance   Status    Patch Age
> +    Bug #16: bug_a title                  Undecided     New     ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
> +    Bug #17: bug_b title                  Critical    Confirmed ...second...
> +    From: Patchy Person
> +    Link: patch_c.diff description of patch c
> +    Bug #18: bug_c title                  Wishlist  Fix Committed ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +
> +To nominate a bug for a series, we need to be able to make bugtasks.
> +It helps to have a meta-factory for this.  The meta-factory defaults
> +to the 'ubuntu' distro, for all instances where a distro is needed.

I'd avoid including explanations about the testing infrastructure in the test
narrative. You can use a docstring for stuff like that. In fact, in this case
it's quite important that you include some kind of comment after the function's
declaration and before its body, so that it's easier to distinguish between
them.

> +
> +    >>> from zope.component import getUtility
> +    >>> from lp.registry.interfaces.distribution import IDistributionSet
> +    >>> def make_bugtask(
> +    ...     bug, target,
> +    ...     target_is_spkg_name=False,
> +    ...     target_is_distroseries_name=False,
> +    ...     importance=None, status=None):
> +    ...     ubuntu_distro = getUtility(IDistributionSet).getByName('ubuntu')
> +    ...     if target_is_spkg_name:
> +    ...         target = ubuntu_distro.getSourcePackage(target)
> +    ...     if target_is_distroseries_name:
> +    ...         target = ubuntu_distro.getSeries(target)
> +    ...     bugtask = factory.makeBugTask(bug=bug, target=target)
> +    ...     if importance is not None:
> +    ...         bugtask.transitionToImportance(importance, ubuntu_distro.owner)
> +    ...     if status is not None:
> +    ...         bugtask.transitionToStatus(status, ubuntu_distro.owner)
> +

As I mentioned (I think) in voice call we had, it may be best to make the test
runner run the same test over different IHasBugs implementations. But maybe
it's better not to worry about this for now, since this test is adequate.

> +Bugs in a product series show up in the patches view for that series.
> +
> +    >>> patchy_product_series = patchy_product.getSeries('trunk')
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_a,
> +    ...     target=patchy_product_series)
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_c,
> +    ...     target=patchy_product_series)
> +    >>> user_browser.open(
> +    ...     'https://launchpad.dev/patchy-product-1/trunk/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance     Status      Patch Age
> +    Bug #18: bug_c title       Wishlist  Fix Committed   ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff
> +    description of patch f
> +    Bug #16: bug_a title       Undecided      New        ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff
> +    description of patch a
> +
> +Patches View by Distro
> +----------------------
> +
> +The patches view also works for distributions, and it shows the target
> +package when viewed via a distribution.
> +
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_a,
> +    ...     target='evolution', target_is_spkg_name=True,
> +    ...     importance=BugTaskImportance.MEDIUM,
> +    ...     status=BugTaskStatus.FIXRELEASED)
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_c,
> +    ...     target='a52dec', target_is_spkg_name=True,
> +    ...     importance=BugTaskImportance.HIGH,
> +    ...     status=BugTaskStatus.TRIAGED)
> +
> +    >>> user_browser.open('http://bugs.launchpad.dev/ubuntu/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance   Status    Package    Patch Age
> +    Bug #18: bug_c title       High       Triaged    a52dec  ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +    Bug #16: bug_a title       Medium   Fix Released   evolution  ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
> +
> +Patches View by Distro Series
> +-----------------------------
> +
> +The patches view works for distro series.
> +
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_a, target='hoary',
> +    ...     target_is_distroseries_name=True)
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_a, target='warty',
> +    ...     target_is_distroseries_name=True)
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_b, target='warty',
> +    ...     target_is_distroseries_name=True)
> +    >>> make_thing(
> +    ...     make_bugtask, bug=bug_c, target='warty',
> +    ...     target_is_distroseries_name=True,
> +    ...     importance=BugTaskImportance.HIGH,
> +    ...     status=BugTaskStatus.TRIAGED)
> +    >>> user_browser.open('https://launchpad.dev/ubuntu/hoary/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance   Status   Package    Patch Age
> +    Bug #16: bug_a title     Undecided       New    hoary     ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff
> +    description of patch a
> +    >>> user_browser.open('https://launchpad.dev/ubuntu/warty/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance   Status   Package    Patch Age
> +    Bug #18: bug_c title        High       Triaged   warty    ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +    Bug #16: bug_a title      Undecided      New     warty    ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff
> +    description of patch a
> +    Bug #17: bug_b title      Undecided      New     warty    ...second...
> +    From: Patchy Person
> +    Link: patch_c.diff
> +    description of patch c
> +
> +Patches View by Source Package
> +------------------------------
> +
> +The patches view works for source packages too.  The view doesn't show
> +target package column in that case, because the package is implied.
> +
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/ubuntu/+source/a52dec/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance   Status     Patch Age
> +    Bug #18: bug_c title       High       Triaged     ...second...
> +    From: Patchy Person
> +    Link: patch_f.diff description of patch f
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/ubuntu/+source/evolution/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    Bug                       Importance   Status     Patch Age
> +    Bug #16: bug_a title       Medium   Fix Released  ...second...
> +    From: Patchy Person
> +    Link: patch_a.diff description of patch a
>
> === added file 'lib/lp/bugs/templates/bugtarget-patches.pt'
> --- lib/lp/bugs/templates/bugtarget-patches.pt  1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/templates/bugtarget-patches.pt  2010-01-28 17:24:20 +0000
> @@ -0,0 +1,127 @@
> +<html
> +  xmlns="http://www.w3.org/1999/xhtml"
> +  xmlns:tal="http://xml.zope.org/namespaces/tal"
> +  xmlns:metal="http://xml.zope.org/namespaces/metal"
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
> +  xml:lang="en"
> +  lang="en"
> +  dir="ltr"
> +  metal:use-macro="view/macro:page/main_only"
> +  i18n:domain="malone"
> +>
> +
> +  <body>
> +    <div metal:fill-slot="main" class="tab-bugs"
> +         tal:define="batchnav view/batchedPatchTasks;
> +                     batch batchnav/currentBatch">
> +
> +  <form class="lesser" id="sort" method="get"
> +        tal:attributes="action string:${context/fmt:url}/+patches">
> +
> +    <script type="text/javascript">
> +      <!--

The 90s called and they're asking for their HTML hacks back. I don't think
these comments in JS blocks are necessary.

> +      YUI().use('base', 'node', 'event', function(Y) {
> +        Y.on('domready', function(e) {
> +          Y.get('#sort-button').setStyle('display', 'none');
> +          Y.get('#orderby').on('change', function(e) {
> +             Y.get('#sort').submit();
> +           });
> +        });
> +      });
> +      // -->
> +    </script>
> +
> +    <select name="orderby" id="orderby" size="1"
> +            tal:define="orderby request/orderby|string:-importance">
> +      <option
> +        value="-importance"
> +        tal:attributes="selected python:orderby == '-importance'"
> +      >by importance</option>
> +      <option
> +        value="status"
> +        tal:attributes="selected python:orderby == 'status'"
> +      >by status</option>
> +      <option
> +        tal:condition="view/shouldShowTargetName"
> +        tal:attributes="selected python:orderby == 'targetname'"
> +        value="targetname"
> +      >by package</option>
> +      <option
> +        value="datecreated"
> +        tal:attributes="selected python:orderby == 'datecreated'"
> +      >oldest first</option>
> +      <option
> +        value="-datecreated"
> +        tal:attributes="selected python:orderby == '-datecreated'"
> +      >newest first</option>
> +    </select>
> +    <input type="submit" value="sort" id="sort-button"/>
> +  </form>

It would be better to use a real ZOPE widget with an enumeration. I know you've
must have seen this style in legacy code, but it's better avoided when adding
new code. I won't object to landing this, as long as I'm reasonably assured that
we'll fix it still this cycle.

> +
> +    <table class="listing"><thead>
> +             <tr>
> +               <th style="padding-right: 1em;"
> +                   >Bug</th>
> +               <th style="padding-right: 1em;"
> +                   >Importance</th>
> +               <th style="padding-right: 1em;"
> +                   >Status</th>
> +               <th style="padding-right: 1em;"
> +                   tal:condition="view/shouldShowTargetName"
> +                   >Package</th>
> +               <th style="padding-right: 1em;"
> +                   >Patch Age</th>
> +             </tr>

Repeating the same style over and over again kinda' screams CSS class.

> +           </thead>
> +           <tr tal:repeat="patch_task batch">
> +             <td style="padding-right: 1em;"
> +              ><a tal:replace="structure patch_task/bug/fmt:link" /></td>

Are we worried about whitespace around the anchor? If not I'd much preder
breaking after the <td> and indenting the next line. So much easier to read and
modify.

> +             <td style="padding-right: 1em;"
> +                 tal:content="patch_task/importance/title"
> +                 tal:attributes="class
> string:importance${patch_task/importance/name}"></td>
> +             <td style="padding-right: 1em;"
> +                 tal:content="patch_task/status/title"
> +                 tal:attributes="class
> string:status${patch_task/status/name}"></td>
> +             <td style="padding-right: 1em;"
> +                 tal:condition="view/shouldShowTargetName"
> +                 ><a tal:attributes="href patch_task/target/fmt:url"
> +                     tal:content="patch_task/target/name"
> +                  ></a></td>
> +             <td style="padding-right: 1em;"
> +                 tal:define="p python:view.youngest_patch(patch_task.bug);
> +                             age python:view.patch_age(p)"
> +                 tal:attributes="id
> string:patch-cell-${repeat/patch_task/index}"
> +                  ><a tal:attributes="href p/libraryfile/http_url"
> +                      tal:content="age/fmt:approximateduration/use-digits"
> +                  ></a>
> +               <div class="popupTitle"

Is this a new CSS class or an existing one. If it's new, let's change it to
lisp-style-identifier. Otherwise don't worry about it.

> +                    tal:attributes="id
> string:patch-popup-${repeat/patch_task/index};">
> +                <p tal:define="submitter p/message/owner"
> +                   ><strong>From:</strong>
> +                    <a tal:replace="structure submitter/fmt:link"></a><br/>
> +                    <strong>Link:</strong>
> +                    <a tal:attributes="href p/libraryfile/http_url"
> +                       tal:content="p/libraryfile/filename"
> +                       ></a></p>
> +                <p tal:content="string:${p/title}"></p>
> +               </div>
> +               <script type="text/javascript" tal:content="string:
> +                 YUI().use('base', 'node', 'event', function(Y) {
> +                   Y.on('domready', function(e) {
> +                     var cell_id = '#patch-cell-${repeat/patch_task/index}';
> +                     var target_id = '#patch-popup-${repeat/patch_task/index}';
> +                     var elt = Y.get(cell_id);
> +                     elt.on('mouseover', function(e) {
> +                        Y.get(target_id).setStyle('display', 'block');
> +                      });
> +                     elt.on('mouseout', function(e) {
> +                        Y.get(target_id).setStyle('display', 'none');
> +                      });
> +                   });
> +                 });"/>
> +             </td>
> +             </tr></table>
> +    <div tal:replace="structure batchnav/@@+navigation-links-lower" />
> +    </div><!-- main -->
> +  </body>
> +</html>
>
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py   2010-01-21 02:16:15 +0000
> +++ lib/lp/testing/factory.py   2010-01-28 17:24:20 +0000
> @@ -1070,7 +1070,7 @@
>
>     def makeBugAttachment(self, bug=None, owner=None, data=None,
>                           comment=None, filename=None, content_type=None,
> -                          description=None):
> +                          description=None, is_patch=None):

Thankyou! I had the same need last week but was too lazy to make this addition :)

>         """Create and return a new bug attachment.
>
>         :param bug: An `IBug` or a bug ID or name, or None, in which
> @@ -1085,6 +1085,7 @@
>             string will be used.
>         :param content_type: The MIME-type of this file.
>         :param description: The description of the attachment.
> +        :param is_patch: Iff true, this attachment is a patch.
>         :return: An `IBugAttachment`.
>         """
>         if bug is None:
> @@ -1101,9 +1102,16 @@
>             comment = self.getUniqueString()
>         if filename is None:
>             filename = self.getUniqueString()
> +        # If the default value of is_patch when creating a new
> +        # BugAttachment should ever change, we don't want to interfere
> +        # with that.  So, we only override it if our caller explicitly
> +        # passed it.
> +        other_params = {}
> +        if is_patch is not None:
> +            other_params['is_patch'] = is_patch
>         return bug.addAttachment(
>             owner, data, comment, filename, content_type=content_type,
> -            description=description)
> +            description=description, **other_params)
>
>     def makeSignedMessage(self, msgid=None, body=None, subject=None,
>             attachment_contents=None, force_transfer_encoding=False,
>