Merge lp:~kfogel/launchpad/506018-patch-report into lp:launchpad

[Karl Fogel]

[Karl Fogel (kfogel)]

With Abel, add a "+patches" view based on IHasBugs.

The patch report lists bugtasks -- for bugs that have patch attachments -- on products, series, and packages. The listing can be sorted by importance, status, and bug age. (Sorting on patch age is not yet implemented, and will be done a separate branch. Likewise, the patch report for persons/teams is not yet implemented, and will be a separate branch.)

To review, load new sampledata from http://people.canonical.com/~kfogel/506018/newsampledata-dev.sql.gz . Then visit pages like these:

  https://bugs.launchpad.dev/patches-view-test/+patches
  https://bugs.launchpad.dev/firefox/+patches
  https://launchpad.dev/ubuntu/+patches
  https://bugs.launchpad.dev/ubuntu/+source/cdrkit/+patches

[Martin Albisetti (beuno)]

Overall, this branch is fantastic. I know a lot of people will appreciate a lot your work on this.
A few small tweaks discussed on IRC:

- Padding in the tooltips would be super nice
- I'd add an "Order by: " label to the ordering drop down, and drop the "by" from each option to improve readability
- Bug icons don't respect importance

review: Needs Fixing (ui)

[Eleanor Berger (intellectronica)]

> === 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.FIXR...

review: Needs Fixing

[Abel Deuring (adeuring)]

On 28.01.2010 19:23, Tom Berger wrote:
>> === 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.

A generally available function should have a way to specify the user
that should login: Some objects can only be created by script users.

And this is actually a good candidate for using the "with" statement --
provided that Jeroen does not object to using it for "transaction
wrapping". (see the notes for last reviewers meeting. Has Barry posted
them already?)

Something like

    with factory.database_login('<email address hidden>'):
 factory.makePerson(k1=v1, k2=v2)

would look much nicer than

    make_thing(factory.makePerson, k1=v1, k2=v2...)

Abel

[Eleanor Berger (intellectronica)]

On 28 January 2010 19:51, Abel Deuring <email address hidden> wrote:
> On 28.01.2010 19:23, Tom Berger wrote:
>>> === 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.
>
> A generally available function should have a way to specify the user
> that should login: Some objects can only be created by script users.
>
> And this is actually a good candidate for using the "with" statement --
> provided that Jeroen does not object to using it for "transaction
> wrapping". (see the notes for last reviewers meeting. Has Barry posted
> them already?)
>
> Something like
>
>    with factory.database_login('<email address hidden>'):
>        factory.makePerson(k1=v1, k2=v2)
>
> would look much nicer than
>
>    make_thing(factory.makePerson, k1=v1, k2=v2...)

+1000000

[Karl Fogel (kfogel)]

Well, doing the Python 'with' thing in story test code proved problematic. But otherwise, all comments have been incorporated now, and pushed up. Thanks, guys.

[Karl Fogel (kfogel)]

Note that Michael Hudson effectively reviewed my re-submission when he reviewed

 https://code.edge.launchpad.net/~intellectronica/launchpad/no-patches-message/+merge/18428

because he commented on my changes from here (which had been merged into that branch) as well as Tom's. I'm not sure he found any problems with Tom's, actually :-).

So, before reviewing this, please see Michael's comments; he may have already said what you were going to say.

review: Needs Fixing

[Eleanor Berger (intellectronica)]

You've addressed all of my concerns and the code looks great now.

review: Approve (code)

[Abel Deuring (adeuring)]

Hi Karl,

I looked at the changes between revision 10189 and 10194 -- everything is fine.

review: Approve (code)

[Abel Deuring (adeuring)]

recent screenshots for the UI review:

http://people.canonical.com/~kfogel/patches-view/screenshot-patches-view-distroseries-2010-02-03.png
http://people.canonical.com/~kfogel/patches-view/screenshot-patches-view-no-patches-message-2010-02-03.png
http://people.canonical.com/~kfogel/patches-view/screenshot-patches-view-orderby-dropdown-2010-02-03.png
http://people.canonical.com/~kfogel/patches-view/screenshot-patches-view-padding-around-tooltip-has-orderby-label-bug-icon-respects-importance-2010-02-03.png
http://people.canonical.com/~kfogel/patches-view/screenshot-patches-view-project-group-2010-02-03.png

[Martin Albisetti (beuno)]

A wonderful piece of work. The sooner this lands, the better.

review: Approve (ui)

[Karl Fogel (kfogel)]

LGTM too now, not that I'm a reviewer, and it's mostly my own change.

review: Approve

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2010-01-21 22:07:42 +0000
+++ lib/canonical/launchpad/icing/style.css	2010-02-02 18:46:23 +0000
@@ -1237,6 +1237,10 @@
     margin: 0;
 }
 
+.patches-view-cell {
+  padding-right: 1em;
+}
+
 /* --- Blueprints --- */
 
 body.tab-specifications #actions, body.tab-specifications .results {
@@ -1463,7 +1467,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-29 19:00:47 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2010-02-02 18:46:23 +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,69 @@
 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.
+        orderby = self.request.get("orderby")
+        if (orderby is not None and
+            orderby not in ["-importance", "status", "targetname",
+                            "datecreated", "-datecreated"]):
+            raise UnexpectedFormData(
+                "Unexpected value for field 'orderby': '%s'" % orderby)
+        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)
+
+    def targetName(self):
+        """Return the name of the current context's target type, or None.
+
+        The name is something like "Package" or "Project" (meaning
+        Product); it is intended to be appropriate to use as a column
+        name in a web page, for example.  If no target type is
+        appropriate for the current context, then return None.
+        """
+        if (IDistribution.providedBy(self.context) or
+            IDistroSeries.providedBy(self.context)):
+            return "Package"
+        elif IProject.providedBy(self.context):
+            return "Project"  # meaning Product
+        else:
+            return None
+
+    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/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2010-01-18 21:44:59 +0000
+++ lib/lp/bugs/browser/configure.zcml	2010-02-02 18:46:23 +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-02-02 18:46:23 +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/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2010-02-01 22:47:12 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2010-02-02 18:46:23 +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 18:46:23 +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-02-02 18:46:23 +0000
@@ -0,0 +1,323 @@
+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(
+    ...     '[e-mail]', factory.makeProduct,
+    ...     name='patchy-product-1')
+
+We don't see any patches when we open the patches view.
+
+    >>> def show_patches_view(contents):
+    ...     for tag in find_tags_by_class(contents, 'listing'):
+    ...         print extract_text(tag)
+
+    >>> anon_browser.open(
+    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
+    >>> show_patches_view(anon_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 = factory.doAsUser(
+    ...     '[e-mail]', make_bug,
+    ...     title="bug_a title", product=patchy_product)
+    >>> anon_browser.open(
+    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
+    >>> show_patches_view(anon_browser.contents)
+    Bug Importance Status Patch Age
+
+After we add a non-patch attachment to that bug, the patches view
+still shows no patches.
+
+    >>> factory.doAsUser('[e-mail]', factory.makeBugAttachment,
+    ...            bug=bug_a, is_patch=False)
+    <BugAttachment at...
+    >>> anon_browser.open('http://bugs.launchpad.dev/patchy-product-1/+patches')
+    >>> show_patches_view(anon_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 = factory.doAsUser(
+    ...    '[e-mail]', factory.makePerson,
+    ...    name="patchy-person", displayname="Patchy Person")
+    >>> factory.doAsUser(
+    ...     '[e-mail]', 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...
+    >>> anon_browser.open(
+    ...     'http://bugs.launchpad.dev/patchy-product-1/+patches')
+    >>> show_patches_view(anon_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(
+    ...     '[e-mail]', make_bug,
+    ...     title="bug_b title", product=patchy_product,
+    ...     importance=BugTaskImportance.CRITICAL,
+    ...     status=BugTaskStatus.CONFIRMED)
+    >>> bug_c = factory.doAsUser(
+    ...     '[e-mail]', make_bug,
+    ...     title="bug_c title", product=patchy_product,
+    ...     importance=BugTaskImportance.WISHLIST,
+    ...     status=BugTaskStatus.FIXCOMMITTED)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', 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(
+    ...     '[e-mail]', 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(
+    ...    '[e-mail]', factory.makeBugAttachment,
+    ...    bug=bug_c, is_patch=False)
+    <BugAttachment at...
+    >>> factory.doAsUser(
+    ...     '[e-mail]', 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(
+    ...     '[e-mail]', 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(
+    ...     '[e-mail]', 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.
+
+    >>> anon_browser.open('http://bugs.launchpad.dev/patchy-product-1/+patches')
+    >>> show_patches_view(anon_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.
+
+    >>> anon_browser.getControl(name="orderby").value = ['-importance']
+    >>> anon_browser.getControl("sort").click()
+    >>> anon_browser.url
+    'http://bugs.launchpad.dev/patchy-product-1/+patches?orderby=-importance'
+    >>> show_patches_view(anon_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
+
+    >>> anon_browser.getControl(name="orderby").value = ['status']
+    >>> anon_browser.getControl("sort").click()
+    >>> anon_browser.url
+    'http://bugs.launchpad.dev/patchy-product-1/+patches?orderby=status'
+    >>> show_patches_view(anon_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.
+    ...     #
+    ...     # :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(
+    ...     '[e-mail]', make_bugtask,
+    ...     bug=bug_a, target=patchy_product_series)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', make_bugtask,
+    ...     bug=bug_c, target=patchy_product_series)
+    >>> anon_browser.open(
+    ...     'https://launchpad.dev/patchy-product-1/trunk/+patches')
+    >>> show_patches_view(anon_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(
+    ...     '[e-mail]', make_bugtask, bug=bug_a,
+    ...     target='evolution', target_is_spkg_name=True,
+    ...     importance=BugTaskImportance.MEDIUM,
+    ...     status=BugTaskStatus.FIXRELEASED)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', make_bugtask, bug=bug_c,
+    ...     target='a52dec', target_is_spkg_name=True,
+    ...     importance=BugTaskImportance.HIGH,
+    ...     status=BugTaskStatus.TRIAGED)
+
+    >>> anon_browser.open('http://bugs.launchpad.dev/ubuntu/+patches')
+    >>> show_patches_view(anon_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(
+    ...     '[e-mail]', make_bugtask, bug=bug_a,
+    ...     target='hoary', target_is_distroseries_name=True)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', make_bugtask, bug=bug_a,
+    ...     target='warty', target_is_distroseries_name=True)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', make_bugtask, bug=bug_b,
+    ...     target='warty', target_is_distroseries_name=True)
+    >>> factory.doAsUser(
+    ...     '[e-mail]', make_bugtask, bug=bug_c,
+    ...     target='warty', target_is_distroseries_name=True,
+    ...     importance=BugTaskImportance.HIGH,
+    ...     status=BugTaskStatus.TRIAGED)
+    >>> anon_browser.open('https://launchpad.dev/ubuntu/hoary/+patches')
+    >>> show_patches_view(anon_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
+    >>> anon_browser.open('https://launchpad.dev/ubuntu/warty/+patches')
+    >>> show_patches_view(anon_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.
+
+    >>> anon_browser.open(
+    ...     'http://bugs.launchpad.dev/ubuntu/+source/a52dec/+patches')
+    >>> show_patches_view(anon_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
+    >>> anon_browser.open(
+    ...     'http://bugs.launchpad.dev/ubuntu/+source/evolution/+patches')
+    >>> show_patches_view(anon_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
+
+# XXX Karl Fogel 2010-02-02 bug=516186: We should also test the
+# patches view on a project group, to make sure the "Package" column
+# becomes "Project" in that context.

=== 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 18:46:23 +0000
@@ -0,0 +1,125 @@
+<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">
+      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/targetName"
+        tal:content="view/targetName"
+        tal:attributes="selected python:orderby == 'targetname'"
+        value="targetname"
+      ></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>
+
+    <table class="listing"><thead>
+             <tr>
+               <th class="patches-view-cell"
+                   >Bug</th>
+               <th class="patches-view-cell"
+                   >Importance</th>
+               <th class="patches-view-cell"
+                   >Status</th>
+               <th class="patches-view-cell"
+                   tal:condition="view/targetName"
+                   tal:content="view/targetName"
+                   ></th>
+               <th class="patches-view-cell"
+                   >Patch Age</th>
+             </tr>
+           </thead>
+           <tr tal:repeat="patch_task batch">
+             <td class="patches-view-cell">
+               <a tal:replace="structure patch_task/fmt:link" /></td>
+             <td class="patches-view-cell"
+                 tal:content="patch_task/importance/title"
+                 tal:attributes="class string:importance${patch_task/importance/name}"></td>
+             <td class="patches-view-cell"
+                 tal:content="patch_task/status/title"
+                 tal:attributes="class string:status${patch_task/status/name}"></td>
+             <td class="patches-view-cell"
+                 tal:condition="view/targetName">
+                   <a tal:attributes="href patch_task/target/fmt:url"
+                      tal:content="patch_task/target/name"></a></td>
+             <td class="patches-view-cell"
+                 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><!-- 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 18:46:23 +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,21 @@
         %s
         ''')
 
+    def doAsUser(self, user, factory_method, **factory_args):
+        """Perform a factory method while temporarily logged in as a user.
+
+        :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.
+        """
+        login(user)
+        try:
+            result = factory_method(**factory_args)
+            transaction.commit()
+        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 +1099,7 @@
 
     def makeBugAttachment(self, bug=None, owner=None, data=None,
                           comment=None, filename=None, content_type=None,
-                          description=None):
+                          description=None, is_patch=_DEFAULT):
         """Create and return a new bug attachment.
 
         :param bug: An `IBug` or a bug ID or name, or None, in which
@@ -1099,6 +1114,7 @@
             string will be used.
         :param content_type: The MIME-type of this file.
         :param description: The description of the attachment.
+        :param is_patch: If true, this attachment is a patch.
         :return: An `IBugAttachment`.
         """
         if bug is None:
@@ -1115,9 +1131,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 _DEFAULT:
+            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,