Hi there,

This branch is pretty cool to see, I'm sure it will be very useful!

I have quite a few comments below though, so I'm going to have to vote "Needs Fixing".

Cheers,
mwh

> === modified file 'lib/lp/bugs/browser/bugtarget.py'
> --- lib/lp/bugs/browser/bugtarget.py	2010-01-29 19:00:47 +0000
> +++ lib/lp/bugs/browser/bugtarget.py	2010-02-02 02:15:24 +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."""
> @@ -7,6 +7,7 @@
>
>  __all__ = [
>      "BugsVHostBreadcrumb",
> +    "BugsPatchesView",
>      "BugTargetBugListingView",
>      "BugTargetBugTagsView",
>      "BugTargetBugsView",
> @@ -19,8 +20,10 @@
>
>  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)
> @@ -48,7 +52,8 @@
>      IBugTarget, IOfficialBugTagTargetPublic, IOfficialBugTagTargetRestricted)
>  from lp.bugs.interfaces.bug import IBugSet
>  from lp.bugs.interfaces.bugtask import (
> -    BugTaskStatus, IBugTaskSet, UNRESOLVED_BUGTASK_STATUSES)
> +    BugTaskStatus, IBugTaskSet, UNRESOLVED_BUGTASK_STATUSES,
> +    UNRESOLVED_PLUS_FIXRELEASED_BUGTASK_STATUSES)
>  from canonical.launchpad.interfaces.launchpad import (
>      IHasExternalBugTracker, ILaunchpadUsage)
>  from canonical.launchpad.interfaces.hwdb import IHWSubmissionSet
> @@ -73,6 +78,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
> @@ -1384,3 +1390,59 @@
>  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."""
> +        # XXX: Karl Fogel 2010-02-01 bug=515584: we should be using a
> +        # Zope form instead of validating the values by hand in the
> +        # code.  Doing it the Zope form way would specify rendering
> +        # and validation from the same enum, and thus observe DRY.

Did you try to do this?  It would be much nicer, I think...

> +        orderby = self.request.get("orderby")
> +        if (orderby is not None and
> +            orderby not in ["-importance", "status", "targetname",
> +                            "datecreated", "-datecreated"]):
> +            raise AssertionError("patch task batch navigator ordered by "
> +                                 "invalid value '%s'" % orderby)

We shouldn't raise AssertionError in the face of unexpected user input
-- going to a URL like

    https://bugs.launchpad.dev/patches-view-test/+patches?orderby=xxx

shouldn't oops.  UnexpectedFormData would be better.

> +        return BatchNavigator(
> +            self.context.searchTasks(
> +                None, user=self.user, order_by=orderby,
> +                status=UNRESOLVED_PLUS_FIXRELEASED_BUGTASK_STATUSES,
> +                omit_duplicates=True, has_patch=True),
> +            self.request)
> +        

Trailing whitespace here.

> +    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))
> +
> +    def youngestPatch(self, bug):
> +        """Return the youngest patch attached to a bug, else error."""
> +        youngest = None
> +        # Loop over bugtasks, gathering youngest patch for each's bug.
> +        for attachment in bug.attachments:
> +            if attachment.is_patch:
> +                if youngest is None:
> +                    youngest = attachment
> +                elif (attachment.message.datecreated >
> +                      youngest.message.datecreated):
> +                    youngest = attachment
> +        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
> +
> +    def patchAge(self, patch):
> +        """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/interfaces/bugtarget.py'
> --- lib/lp/bugs/interfaces/bugtarget.py	2009-08-18 11:12:06 +0000
> +++ lib/lp/bugs/interfaces/bugtarget.py	2010-02-02 02:15:24 +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

Seems odd to have this be the only change to the file...

> === modified file 'lib/lp/bugs/interfaces/bugtask.py'
> --- lib/lp/bugs/interfaces/bugtask.py	2010-01-23 21:42:36 +0000
> +++ lib/lp/bugs/interfaces/bugtask.py	2010-02-02 02:15:24 +0000
> @@ -36,6 +36,7 @@
>      'IUpstreamProductBugTaskSearch',
>      'RESOLVED_BUGTASK_STATUSES',
>      'UNRESOLVED_BUGTASK_STATUSES',
> +    'UNRESOLVED_PLUS_FIXRELEASED_BUGTASK_STATUSES',
>      'UserCannotEditBugTaskImportance',
>      'UserCannotEditBugTaskMilestone',
>      'UserCannotEditBugTaskStatus',
> @@ -287,6 +288,9 @@
>      BugTaskStatus.INPROGRESS,
>      BugTaskStatus.FIXCOMMITTED)
>
> +UNRESOLVED_PLUS_FIXRELEASED_BUGTASK_STATUSES = (
> +    UNRESOLVED_BUGTASK_STATUSES + (BugTaskStatus.FIXRELEASED,))
> +
>  RESOLVED_BUGTASK_STATUSES = (
>      BugTaskStatus.FIXRELEASED,
>      BugTaskStatus.INVALID,

> === 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-02-02 02:15:24 +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

Same.

> === 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-02-02 02:15:24 +0000
> @@ -0,0 +1,325 @@
> +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.
> +
> +    >>> patchy_product = factory.doAsUser(
> +    ...     '<email address hidden>', factory.makeProduct,
> +    ...     name='patchy-product-1')
> +
> +We don't see any patches when we open the patches view.
> +
> +    >>> def show_patches_view(expected_contents):

'expected_contents'?  It's more 'actual_contents' isn't it?

> +    ...     for tag in find_tags_by_class(
> +    ...         expected_contents, 'listing'):
> +    ...         print extract_text(tag)
> +    ...     messages = find_tags_by_class(expected_contents, 'informational')
> +    ...     if len(messages) > 0:
> +    ...         print extract_text(messages[0])
> +
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    There are no patches associated with Patchy-product-1 at this time.
> +
> +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 = factory.doAsUser(
> +    ...     '<email address hidden>', make_bug,
> +    ...     title="bug_a title", product=patchy_product)
> +    >>> user_browser.open(
> +    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
> +    >>> show_patches_view(user_browser.contents)
> +    There are no patches associated with Patchy-product-1 at this time.
> +
> +
> +After we add a non-patch attachment to that bug, the patches view
> +still shows no patches.
> +
> +    >>> factory.doAsUser('<email address hidden>', 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)
> +    There are no patches associated with Patchy-product-1 at this time.
> +
> +After we add a patch attachment that's one day old, we see it in the
> +patches view.
> +
> +    >>> patch_submitter = factory.doAsUser(
> +    ...    '<email address hidden>', factory.makePerson,
> +    ...    name="patchy-person", displayname="Patchy Person")
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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 = factory.doAsUser(
> +    ...     '<email address hidden>', make_bug,
> +    ...     title="bug_b title", product=patchy_product,
> +    ...     importance=BugTaskImportance.CRITICAL,
> +    ...     status=BugTaskStatus.CONFIRMED)
> +    >>> bug_c = factory.doAsUser(
> +    ...     '<email address hidden>', make_bug,
> +    ...     title="bug_c title", product=patchy_product,
> +    ...     importance=BugTaskImportance.WISHLIST,
> +    ...     status=BugTaskStatus.FIXCOMMITTED)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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...
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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...
> +    >>> factory.doAsUser(
> +    ...    '<email address hidden>', factory.makeBugAttachment,
> +    ...    bug=bug_c, is_patch=False)
> +    <BugAttachment at...
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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...
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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...
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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
> +
> +Bugs in a product series show up in the patches view for that series.
> +
> +    >>> from zope.component import getUtility
> +    >>> from lp.registry.interfaces.distribution import IDistributionSet
> +    >>> def make_bugtask(
> +    ...     # Meta-factory for making bugtasks.
> +    ...     # 
> +    ...     # In all instances where a distro is needed, defaults to
> +    ...     # 'ubuntu' distro.
> +    ...     # 

Some trailing whitespace here.

> +    ...     # :param bug: The bug with which the task is associated.
> +    ...     # :param target: The target to which to attach this bug.
> +    ...     #     If the target is a string, then it names the target
> +    ...     #     object, and exactly one of following two boolean
> +    ...     #     parameters must be set to indicate the object type.
> +    ...     # :param target_is_spkg_name: If true, target is a string
> +    ...     #     indicating the name of the source package for the task.
> +    ...     # :param target_is_distroseries_name: If true, target is a string
> +    ...     #     indicating the name of the distroseries for the task.
> +    ...     # :param importance: The initial importance of the bugtask;
> +    ...     #     if None, just use the default importance.
> +    ...     # :param status: The initial status of the bugtask;
> +    ...     #     if None, just use the default status.
> +    ...     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)
> +    >>> patchy_product_series = patchy_product.getSeries('trunk')
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', make_bugtask,
> +    ...     bug=bug_a, target=patchy_product_series)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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.
> +
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', make_bugtask, bug=bug_a,
> +    ...     target='evolution', target_is_spkg_name=True,
> +    ...     importance=BugTaskImportance.MEDIUM,
> +    ...     status=BugTaskStatus.FIXRELEASED)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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.
> +
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', make_bugtask, bug=bug_a,
> +    ...     target='hoary', target_is_distroseries_name=True)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', make_bugtask, bug=bug_a,
> +    ...     target='warty', target_is_distroseries_name=True)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', make_bugtask, bug=bug_b,
> +    ...     target='warty', target_is_distroseries_name=True)
> +    >>> factory.doAsUser(
> +    ...     '<email address hidden>', 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
> +

It seems you don't test the patch view on a project group, which is
interesting because the "package" title for the context name column
doesn't really make sense there!

> === 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-02-02 02:15:24 +0000
> @@ -0,0 +1,136 @@
> +<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">
> + <div tal:condition="python: len(batch) > 0">
> +
> +  <form class="lesser" id="sort" method="get"
> +        tal:attributes="action string:${context/fmt:url}/+patches">
> +
> +    <script type="text/javascript">
> +      <!--
> +      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>
> +
> +    Order&nbsp;by:&nbsp;<select
> +                    name="orderby" id="orderby" size="1"
> +                    tal:define="orderby request/orderby|string:-importance">
> +      <option
> +        value="-importance"
> +        tal:attributes="selected python:orderby == '-importance'"
> +      >importance</option>
> +      <option
> +        value="status"
> +        tal:attributes="selected python:orderby == 'status'"
> +      >status</option>
> +      <option
> +        tal:condition="view/shouldShowTargetName"
> +        tal:attributes="selected python:orderby == 'targetname'"
> +        value="targetname"
> +      >package</option>

As above, "package" isn't always the right thing here.

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

As above, "package" isn't always the right thing here.

> +               <th style="padding-right: 1em;"
> +                   >Patch Age</th>
> +             </tr>
> +           </thead>
> +           <tr tal:repeat="patch_task batch">
> +             <td style="padding-right: 1em;"
> +              ><a tal:replace="structure patch_task/fmt:link" /></td>
> +             <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.youngestPatch(patch_task.bug);
> +                             age python:view.patchAge(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"
> +                    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>
> + <div class="informational message"
> +      tal:condition="python: len(batch) == 0">
> +   There are no patches associated with
> +   <tal:target replace="context/displayname" />
> +   at this time.
> + </div>
> +    </div><!-- main -->
> +  </body>
> +</html>

> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2010-01-30 05:27:48 +0000
> +++ lib/lp/testing/factory.py	2010-02-02 02:15:24 +0000
> @@ -130,7 +130,7 @@
>  from lp.soyuz.interfaces.component import IComponentSet
>  from lp.soyuz.interfaces.packageset import IPackagesetSet
>  from lp.soyuz.model.buildqueue import BuildQueue
> -from lp.testing import run_with_login, time_counter
> +from lp.testing import run_with_login, time_counter, login, logout
>
>  SPACE = ' '
>
> @@ -239,6 +239,19 @@
>          %s
>          ''')
>
> +    def doAsUser(self, user, factory_method, **factory_args):
> +        """Perform a factory method while temporarily logged in as a user.

There should be a blank line here.

> +        :param user: The user to log in as, and then to log out from.
> +        :param factory_method: The factory method to invoke while logged in.
> +        :param factory_args: Keyword arguments to pass to factory_method."""

And these quotes should be on their own line.

(These rules may be silly, but they're pretty consistently followed in
the code base...)

> +        login(user)
> +        try:
> +            result = factory_method(**factory_args)
> +            transaction.commit()

Do you really need to commit here?  Zope-level users and database
transactions are basically orthogonal.

> +        finally:
> +            logout()
> +        return result
> +
>      def makeCopyArchiveLocation(self, distribution=None, owner=None,
>          name=None, enabled=True):
>          """Create and return a new arbitrary location for copy packages."""
> @@ -1084,7 +1097,7 @@
>
>      def makeBugAttachment(self, bug=None, owner=None, data=None,
>                            comment=None, filename=None, content_type=None,
> -                          description=None):
> +                          description=None, is_patch=None):
>          """Create and return a new bug attachment.
>
>          :param bug: An `IBug` or a bug ID or name, or None, in which
> @@ -1099,6 +1112,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.

s/Iff/If/ ?

>          :return: An `IBugAttachment`.
>          """
>          if bug is None:
> @@ -1115,9 +1129,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.

There's already a module level _DEFAULT object that might be a better
marker of "no argument passed" than None here.

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