Merge lp:~michael.nelson/launchpad/635005-difference-details-2 into lp:launchpad/db-devel

[Michael Nelson]

Add extra details of DistroSeriesDifferences via Javascript.

Overview
========

This branch adds comment rendering to the extra-details template for DistroSeriesDifferences and hooks them up to the DistroSeriesDifference list like this:

https://devpad.canonical.com/~michaeln/tmp/distroseriesdifference-extra.ogv

Details
=======
This branch adds and tests the rendering of comments on the distroseriesdifference-listing-extra.pt template, and then adds the non-js link to this page from the distroseries-localdifferences.pt template.

It then adds JS to enhance this by loading the snippet and adding it inline as an expanded table row.

Note: A large part of the diff below is adding a default view and templates for the lp.services.comments system. This involved updating IComment with all the common comment attributes, and updating the code views so that their display comment model also provides IComment.

To test
=======
bin/test -vv -m test_distroseriesdifference_views -m test_series_views -m test_codereviewcomment

Windmill:
bin/test -vvm test_distroseriesdifference_expander

To demo
=======
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs

[Henning Eggers (henninge)]

Thank you for that great screencast. I just installed "recordmydesktop", too. ;-)

As you mentioned and we discussed in IRC you should use the same style for comments as we have elsewhere on Launchpad. But feel free to propose to change our comments to contain (smaller) logos/head shots of uses. ;-)

Also discussed: The items in the list of available diffs should be preceded by a download icon and the list should be indented. I am also wondering (not discussed) if this section could not be reworded like this:

Differences from last common version:

 * Derived version: 1.15-2ubuntu1derilucid2 (1.2kB)
 * Base version: 1.17-1 (0.5kB)

I think this is clearer and has less noise. The word "package" here was noise, too, because we are already in a table row that is about this package. I am just wondering if the whole line should be the link or just the version number. In the latter case the download icon would end up in the middle of the line, which might not be desirable. Maybe you can play around with that a bit to see what looks best.

I am not approving this yet because I'd like to see the outcome of this and also the inclusion of the standard LP comments first.

review: Needs Fixing (ui*)

[Michael Nelson (michael.nelson)]

On Thu, Sep 16, 2010 at 1:06 PM, Henning Eggers
<email address hidden> wrote:
> Review: Needs Fixing ui*
> Thank you for that great screencast. I just installed "recordmydesktop", too. ;-)
>
> As you mentioned and we discussed in IRC you should use the same style for comments as we have elsewhere on Launchpad. But feel free to propose to change our comments to contain (smaller) logos/head shots of uses. ;-)

Yep. Also, I've just found lp.services.comments too, so I'll probably
refactor a bit to use that.

>
> Also discussed: The items in the list of available diffs should be preceded by a download icon and the list should be indented.

Yes for the icon, but when the actual diffs are displayed (perhaps not
when they need to be requested first). And +1 for the indentation too.

> I am also wondering (not discussed) if this section could not be reworded like this:
>
> Differences from last common version:
>
>  * Derived version: 1.15-2ubuntu1derilucid2 (1.2kB)
>  * Base version: 1.17-1 (0.5kB)
>
> I think this is clearer and has less noise. The word "package" here was noise, too, because we are already in a table row that is about this package. I am just wondering if the whole line should be the link or just the version number. In the latter case the download icon would end up in the middle of the line, which might not be desirable. Maybe you can play around with that a bit to see what looks best.

Sounds good. I'll hopefully have something tomorrow morning :)

[Henning Eggers (henninge)]

The comments section looks good now and the wording for the differences is perfect. Thank you. You fixed the "binary descriptions" sections, too, which I had forgotten to mention. Good job! I guess I was wrong about the idention but this is fine, too. I'd still like the download icon but will not hold the review on this. Let's hear Paul's take on this ... ;-)

Thank you very much again!

Paul, I think I should be in "Launchpad UI Reviewers", shouldn't I?

review: Approve (ui*)

[Michael Nelson (michael.nelson)]

On Fri, Sep 17, 2010 at 11:04 AM, Henning Eggers
<email address hidden> wrote:
> Review: Approve ui*
> The comments section looks good now and the wording for the differences is perfect. Thank you. You fixed the "binary descriptions" sections, too, which I had forgotten to mention. Good job!  I guess I was wrong about the idention but this is fine, too. I'd still like the download icon but will not hold the review on this. Let's hear Paul's take on this ... ;-)

Thanks Henning. Just to be clear, I also want the download icon - but
only when there is something to download. The current UI is
representing the state where we *could* generate that PackageDiff if
the user requests it, but I need to add the JS to do that. Once the
diff has been generated I'm all for it being displayed with the icon
(and updating the similar link on PPA packages to do the same).

@Paul: I'm actually updating the lp.services.comments stuff, adding
some generic templates/views. I'll hopefully have pushed this by the
time you check, so in addition to the UI review, would you mind
glancing over the lp.services.comments changes I'm adding to make sure
it's inline with what the code team envisaged. Thanks.

[Abel Deuring (adeuring)]

nice work!

review: Approve (code)

[Curtis Hovey (sinzui)]

This is excellent. I have no remarks; well done Henning.

review: Approve (ui)

[Michael Nelson (michael.nelson)]

I had to make the following changes to ensure code's use of comments renders with their own templates, rather than the default ones for IComment:

http://pastebin.ubuntu.com/496936/

As well as ensuring the zcml links up the correct template, it also ensures that the code comment classes (CodeReviewDisplayComment and CodeReviewNewRevisions) do implement the new IComment iface.

[Henning Eggers (henninge)]

The incremental diff looks good to me. ;-) Placing the marker interface in the browser code seems to be common practice.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-08-27 11:19:54 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-09-21 11:03:45 +0000
@@ -89,6 +89,7 @@
     IStructuralSubscription,
     IStructuralSubscriptionTarget,
     )
+from lp.services.comments.interfaces.conversation import IComment
 from lp.soyuz.enums import (
     PackagePublishingStatus,
     PackageUploadCustomFormat,
@@ -321,6 +322,9 @@
 IBuildFarmJob['status'].vocabulary = BuildStatus
 IBuildFarmJob['buildqueue_record'].schema = IBuildQueue
 
+# IComment
+IComment['comment_author'].schema = IPerson
+
 # IDistribution
 IDistribution['series'].value_type.schema = IDistroSeries
 patch_reference_property(

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-08-20 13:33:51 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-09-21 11:03:45 +0000
@@ -183,6 +183,8 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/bugs/bugtracker_overlay.js"></script>
     <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/registry/distroseriesdifferences_details.js"></script>
+    <script type="text/javascript"
             tal:attributes="src string:${lp_js}/registry/milestoneoverlay.js"></script>
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/registry/milestonetable.js"></script>

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2010-08-24 10:45:57 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2010-09-21 11:03:45 +0000
@@ -547,7 +547,7 @@
         return diff_text.count('\n') >= config.diff.max_format_lines
 
 
-class ICodeReviewNewRevisions(Interface):
+class ICodeReviewNewRevisions(IComment):
     """Marker interface used to register views for CodeReviewNewRevisions."""
 
 
@@ -557,7 +557,7 @@
     Each object instance represents a number of revisions scanned at a
     particular time.
     """
-    implements(IComment, ICodeReviewNewRevisions)
+    implements(ICodeReviewNewRevisions)
 
     def __init__(self, revisions, date, branch):
         self.revisions = revisions
@@ -567,6 +567,13 @@
         # The date attribute is used to sort the comments in the conversation.
         self.date = date
 
+        # Other standard IComment attributes are not used.
+        self.extra_css_class = None
+        self.comment_author = None
+        self.body_text = None
+        self.comment_date = None
+        self.display_attachments = False
+
 
 class CodeReviewNewRevisionsView(LaunchpadView):
     """The view for rendering the new revisions."""

=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2010-08-24 10:45:57 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2010-09-21 11:03:45 +0000
@@ -43,6 +43,10 @@
 from lp.services.propertycache import cachedproperty
 
 
+class ICodeReviewDisplayComment(IComment, ICodeReviewComment):
+    """Marker interface for displaying code review comments."""
+
+
 class CodeReviewDisplayComment:
     """A code review comment or activity or both.
 
@@ -51,7 +55,7 @@
     only code in the model itself.
     """
 
-    implements(IComment)
+    implements(ICodeReviewDisplayComment)
 
     delegates(ICodeReviewComment, 'comment')
 
@@ -70,6 +74,40 @@
         else:
             return ''
 
+    @cachedproperty
+    def comment_author(self):
+        """The author of the comment."""
+        return self.comment.message.owner
+
+    @cachedproperty
+    def has_body(self):
+        """Is there body text?"""
+        return bool(self.body_text)
+
+    @cachedproperty
+    def body_text(self):
+        """Get the body text for the message."""
+        return self.comment.message_body
+
+    @cachedproperty
+    def comment_date(self):
+        """The date of the comment."""
+        return self.comment.message.datecreated
+
+    @cachedproperty
+    def all_attachments(self):
+        return self.comment.getAttachments()
+
+    @cachedproperty
+    def display_attachments(self):
+        # Attachments to show.
+        return [DiffAttachment(alias) for alias in self.all_attachments[0]]
+
+    @cachedproperty
+    def other_attachments(self):
+        # Attachments to not show.
+        return self.all_attachments[1]
+
 
 class CodeReviewCommentPrimaryContext:
     """The primary context is the comment is that of the source branch."""
@@ -132,45 +170,11 @@
         """The decorated code review comment."""
         return CodeReviewDisplayComment(self.context)
 
-    @cachedproperty
-    def comment_author(self):
-        """The author of the comment."""
-        return self.context.message.owner
-
-    @cachedproperty
-    def has_body(self):
-        """Is there body text?"""
-        return bool(self.body_text)
-
-    @cachedproperty
-    def body_text(self):
-        """Get the body text for the message."""
-        return self.context.message_body
-
-    @cachedproperty
-    def comment_date(self):
-        """The date of the comment."""
-        return self.context.message.datecreated
-
     # Should the comment be shown in full?
     full_comment = True
     # Show comment expanders?
     show_expanders = False
 
-    @cachedproperty
-    def all_attachments(self):
-        return self.context.getAttachments()
-
-    @cachedproperty
-    def display_attachments(self):
-        # Attachments to show.
-        return [DiffAttachment(alias) for alias in self.all_attachments[0]]
-
-    @cachedproperty
-    def other_attachments(self):
-        # Attachments to not show.
-        return self.all_attachments[1]
-
 
 class CodeReviewCommentSummary(CodeReviewCommentView):
     """Summary view of a CodeReviewComment"""

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2010-09-20 00:20:42 +0000
+++ lib/lp/code/browser/configure.zcml	2010-09-21 11:03:45 +0000
@@ -691,6 +691,16 @@
             name="+index"
             template="../templates/codereviewcomment-index.pt"/>
         <browser:page
+            name="+fragment"
+            template="../templates/codereviewcomment-fragment.pt"/>
+    </browser:pages>
+    <browser:pages
+        facet="branches"
+        for="lp.code.browser.codereviewcomment.ICodeReviewDisplayComment"
+        layer="lp.code.publisher.CodeLayer"
+        class="lp.code.browser.codereviewcomment.CodeReviewCommentView"
+        permission="zope.Public">
+        <browser:page
             name="+comment-header"
             template="../templates/codereviewcomment-header.pt"/>
         <browser:page
@@ -699,9 +709,6 @@
         <browser:page
             name="+comment-footer"
             template="../templates/codereviewcomment-footer.pt"/>
-        <browser:page
-            name="+fragment"
-            template="../templates/codereviewcomment-fragment.pt"/>
     </browser:pages>
     <browser:pages
         facet="branches"

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-08-22 21:34:16 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-09-21 11:03:45 +0000
@@ -24,6 +24,7 @@
 from canonical.launchpad.database.message import MessageSet
 from canonical.launchpad.webapp.interfaces import IPrimaryContext
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -36,8 +37,10 @@
     BranchMergeProposalMergedView,
     BranchMergeProposalVoteView,
     DecoratedCodeReviewVoteReference,
+    ICodeReviewNewRevisions,
     latest_proposals_for_each_branch,
     )
+from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
 from lp.code.enums import (
     BranchMergeProposalStatus,
     CodeReviewVote,
@@ -634,6 +637,21 @@
             [revisions[2], revisions[3]]]
         self.assertRevisionGroups(bmp, expected_groups)
 
+    def test_CodeReviewNewRevisions_implements_ICodeReviewNewRevisions(self):
+        # The browser helper class implements its interface.
+        review_date = datetime(2009, 9, 10, tzinfo=pytz.UTC)
+        revision_date = review_date + timedelta(days=1)
+        bmp = self.factory.makeBranchMergeProposal(
+            date_created=review_date)
+        revision = add_revision_to_branch(
+            self.factory, bmp.source_branch, revision_date)
+
+        view = create_initialized_view(bmp, '+index')
+        groups = view._getRevisionsSinceReviewStart()
+        new_revisions = groups[0]
+
+        self.assertTrue(verifyObject(ICodeReviewNewRevisions, new_revisions))
+
     def test_include_superseded_comments(self):
         for x, time in zip(range(3), time_counter()):
             if x != 0:
@@ -767,7 +785,8 @@
             body='testing',
             attachments=[('test.diff', 'text/plain', attachment_body)])
         message = MessageSet().fromEmail(msg.as_string())
-        return bmp.createCommentFromMessage(message, None, None, msg)
+        return CodeReviewDisplayComment(
+            bmp.createCommentFromMessage(message, None, None, msg))
 
     def test_nonascii_in_attachment_renders(self):
         # The view should render without errors.
@@ -783,7 +802,7 @@
         # Need to commit in order to read the diff out of the librarian.
         transaction.commit()
         view = create_initialized_view(comment, '+comment-body')
-        [diff_attachment] = view.display_attachments
+        [diff_attachment] = view.comment.display_attachments
         self.assertEqual(u'\u2615', diff_attachment.diff_text)
 
 

=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
--- lib/lp/code/browser/tests/test_codereviewcomment.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/browser/tests/test_codereviewcomment.py	2010-09-21 11:03:45 +0000
@@ -3,35 +3,52 @@
 
 """Unit tests for CodeReviewComments."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 import unittest
 
 from canonical.launchpad.webapp.interfaces import IPrimaryContext
+from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import DatabaseFunctionalLayer
+from lp.code.browser.codereviewcomment import (
+    CodeReviewDisplayComment,
+    ICodeReviewDisplayComment,
+    )
 from lp.testing import (
-    login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 
 
-class TestCodeReviewCommentPrimaryContext(TestCaseWithFactory):
-    # Tests the adaptation of a code review comment into a primary context.
+class TestCodeReviewComments(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
     def testPrimaryContext(self):
+        # Tests the adaptation of a code review comment into a primary
+        # context.
         # We need a person to make a comment.
-        commenter = self.factory.makePerson()
-        login_person(commenter)
-        # The primary context of a code review comment is the same as the
-        # primary context for the branch merge proposal that the comment is
-        # for.
-        comment = self.factory.makeCodeReviewComment()
+        with person_logged_in(self.factory.makePerson()):
+            # The primary context of a code review comment is the same
+            # as the primary context for the branch merge proposal that
+            # the comment is for.
+            comment = self.factory.makeCodeReviewComment()
+
         self.assertEqual(
             IPrimaryContext(comment).context,
             IPrimaryContext(comment.branch_merge_proposal).context)
 
+    def test_display_comment_provides_icodereviewdisplaycomment(self):
+        # The CodeReviewDisplayComment class provides IComment.
+        with person_logged_in(self.factory.makePerson()):
+            comment = self.factory.makeCodeReviewComment()
+
+        display_comment = CodeReviewDisplayComment(comment)
+
+        verifyObject(ICodeReviewDisplayComment, display_comment)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/templates/codereviewcomment-body.pt'
--- lib/lp/code/templates/codereviewcomment-body.pt	2010-04-16 04:20:43 +0000
+++ lib/lp/code/templates/codereviewcomment-body.pt	2010-09-21 11:03:45 +0000
@@ -3,9 +3,9 @@
    xmlns:metal="http://xml.zope.org/namespaces/metal"
    omit-tag="">
 
-  <tal:message replace="structure view/body_text/fmt:obfuscate-email/fmt:nice_pre" />
+  <tal:message replace="structure view/comment/body_text/fmt:obfuscate-email/fmt:nice_pre" />
 
-  <tal:good-attachments repeat="attachment view/display_attachments">
+  <tal:good-attachments repeat="attachment view/comment/display_attachments">
     <div class="boardComment attachment">
       <div class="boardCommentDetails filename"><a tal:content="attachment/filename" tal:attributes="href attachment/getURL"/></div>
       <div class="boardCommentBody attachmentBody" tal:content="structure attachment/diff_text/fmt:diff"/>

=== modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
--- lib/lp/code/templates/codereviewcomment-header.pt	2010-02-18 16:40:09 +0000
+++ lib/lp/code/templates/codereviewcomment-header.pt	2010-09-21 11:03:45 +0000
@@ -3,9 +3,9 @@
    xmlns:metal="http://xml.zope.org/namespaces/metal"
    omit-tag="">
 
-  <tal:author replace="structure view/comment_author/fmt:link:mainsite"/>
-  <tal:has-body condition="view/has_body">wrote</tal:has-body>
-  <tal:date replace="view/comment_date/fmt:displaydate" />
+  <tal:author replace="structure context/comment_author/fmt:link:mainsite"/>
+  <tal:has-body condition="context/has_body">wrote</tal:has-body>
+  <tal:date replace="context/comment_date/fmt:displaydate" />
   <span tal:condition="context/from_superseded" class="sprite warning-icon"
         style="float: right">Posted in <a
           tal:attributes="href context/branch_merge_proposal/fmt:url">a

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-09-13 10:04:20 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-09-21 11:03:45 +0000
@@ -151,7 +151,7 @@
         permission="zope.Public"/>
     <browser:url
         for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
-        path_expression="string:+difference/${difference/source_package_name}"
+        path_expression="string:+difference/${source_package_name/name}"
         rootsite="mainsite"
         attribute_to_parent="derived_series"/>
     <browser:page
@@ -160,6 +160,9 @@
         class="lp.registry.browser.distroseriesdifference.DistroSeriesDifferenceView"
         template="../templates/distroseriesdifference-listing-extra.pt"
         permission="zope.Public"/>
+    <browser:defaultView
+        for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
+        name="+listing-distroseries-extra"/>
     <browser:menus
         classes="
             DistroSeriesFacets

=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py	2010-09-13 15:09:48 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py	2010-09-21 11:03:45 +0000
@@ -8,14 +8,22 @@
     'DistroSeriesDifferenceView',
     ]
 
+from zope.interface import implements
+
 from canonical.launchpad.webapp.publisher import LaunchpadView
+from lp.services.comments.interfaces.conversation import (
+    IComment,
+    IConversation,
+    )
 
 
 class DistroSeriesDifferenceView(LaunchpadView):
 
+    implements(IConversation)
+
     @property
-    def summary(self):
-        """Return the summary of the related source package."""
+    def binary_summaries(self):
+        """Return the summary of the related binary packages."""
         source_pub = None
         if self.context.source_pub is not None:
             source_pub = self.context.source_pub
@@ -23,7 +31,33 @@
             source_pub = self.context.parent_source_pub
 
         if source_pub is not None:
-            return source_pub.meta_sourcepackage.summary
-        else:
-            return None
-
+            summary = source_pub.meta_sourcepackage.summary
+            if summary:
+                return summary.split('\n')
+
+        return None
+
+    @property
+    def comments(self):
+        """See `IConversation`."""
+        # Could use a generator here?
+        return [
+            DistroSeriesDifferenceDisplayComment(comment) for
+                comment in self.context.getComments()]
+
+
+class DistroSeriesDifferenceDisplayComment:
+    """Used simply to provide `IComment` for rendering."""
+    implements(IComment)
+
+    has_body = True
+    has_footer = False
+    display_attachments = False
+    extra_css_class = ''
+
+    def __init__(self, comment):
+        """Setup the attributes required by `IComment`."""
+        self.message_body = comment.comment
+        self.comment_author = comment.message.owner
+        self.comment_date = comment.message.datecreated
+        self.body_text = comment.comment

=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2010-09-14 13:37:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py	2010-09-21 11:03:45 +0000
@@ -10,12 +10,20 @@
 from BeautifulSoup import BeautifulSoup
 from zope.component import getUtility
 
+from canonical.launchpad.webapp.testing import verifyObject
 from canonical.testing import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.registry.browser.distroseriesdifference import (
+    DistroSeriesDifferenceDisplayComment,
+    )
 from lp.registry.enum import DistroSeriesDifferenceType
 from lp.registry.interfaces.distroseriesdifference import IDistroSeriesDifferenceSource
+from lp.services.comments.interfaces.conversation import (
+    IComment,
+    IConversation,
+    )
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
@@ -30,10 +38,28 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_provides_conversation(self):
+        # The DSDView provides a conversation implementation.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+
+        view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+        self.assertTrue(verifyObject(IConversation, view))
+
+    def test_comment_for_display_provides_icomment(self):
+        # The DSDDisplayComment browser object provides IComment.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        owner = ds_diff.derived_series.owner
+        with person_logged_in(owner):
+            comment = ds_diff.addComment(owner, "I'm working on this.")
+        comment_for_display = DistroSeriesDifferenceDisplayComment(comment)
+
+        self.assertTrue(verifyObject(IComment, comment_for_display))
+
     def addSummaryToDifference(self, distro_series_difference):
         """Helper that adds binaries with summary info to the source pubs."""
         distro_series = distro_series_difference.derived_series
-        source_package_name_str = distro_series_difference.source_package_name.name
+        source_package_name_str = (
+            distro_series_difference.source_package_name.name)
         stp = SoyuzTestPublisher()
 
         if distro_series_difference.difference_type == (
@@ -52,7 +78,7 @@
             distro_series, source_package_name_str)
         return ds_diff
 
-    def test_summary_for_source_pub(self):
+    def test_binary_summaries_for_source_pub(self):
         # For packages unique to the derived series (or different
         # versions) the summary is based on the derived source pub.
         ds_diff = self.factory.makeDistroSeriesDifference()
@@ -60,11 +86,13 @@
 
         view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
 
-        self.assertIsNot(None, view.summary)
-        self.assertEqual(
-            ds_diff.source_pub.meta_sourcepackage.summary, view.summary)
+        self.assertIsNot(None, view.binary_summaries)
+        self.assertEqual([
+            u'flubber-bin: summary for flubber-bin',
+            u'flubber-lib: summary for flubber-lib',
+            ], view.binary_summaries)
 
-    def test_summary_for_missing_difference(self):
+    def test_binary_summaries_for_missing_difference(self):
         # For packages only in the parent series, the summary is based
         # on the parent publication.
         ds_diff = self.factory.makeDistroSeriesDifference(
@@ -74,12 +102,13 @@
 
         view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
 
-        self.assertIsNot(None, view.summary)
-        self.assertEqual(
-            ds_diff.parent_source_pub.meta_sourcepackage.summary,
-            view.summary)
+        self.assertIsNot(None, view.binary_summaries)
+        self.assertEqual([
+            u'flubber-bin: summary for flubber-bin',
+            u'flubber-lib: summary for flubber-lib',
+            ], view.binary_summaries)
 
-    def test_summary_no_pubs(self):
+    def test_binary_summaries_no_pubs(self):
         # If the difference has been resolved by removing packages then
         # there will not be a summary.
         ds_diff = self.factory.makeDistroSeriesDifference(
@@ -93,7 +122,7 @@
 
         self.assertIs(None, ds_diff.parent_source_pub)
         self.assertIs(None, ds_diff.source_pub)
-        self.assertIs(None, view.summary)
+        self.assertIs(None, view.binary_summaries)
 
 
 class DistroSeriesDifferenceTemplateTestCase(TestCaseWithFactory):
@@ -103,7 +132,7 @@
     def number_of_request_diff_texts(self, html):
         """Check that the html doesn't include the request diff text."""
         soup = BeautifulSoup(html)
-        return len(soup.findAll('dd', 'request-derived-diff'))
+        return len(soup.findAll('li', 'request-derived-diff'))
 
     def contains_one_link_to_diff(self, html, package_diff):
         """Return whether the html contains a link to the diff content."""
@@ -170,3 +199,19 @@
 
         view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
         self.assertEqual(1, self.number_of_request_diff_texts(view()))
+
+    def test_comments_rendered(self):
+        # If there are comments on the difference, they are rendered.
+        ds_diff = self.factory.makeDistroSeriesDifference()
+        owner = ds_diff.derived_series.owner
+        with person_logged_in(owner):
+            ds_diff.addComment(owner, "I'm working on this.")
+            ds_diff.addComment(owner, "Here's another comment.")
+
+        view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+        soup = BeautifulSoup(view())
+
+        self.assertEqual(
+            1, len(soup.findAll('pre', text="I'm working on this.")))
+        self.assertEqual(
+            1, len(soup.findAll('pre', text="Here's another comment.")))

=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2010-09-08 07:53:06 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2010-09-21 11:03:45 +0000
@@ -181,6 +181,25 @@
         self.assertIn("Latest comment", unicode(rows[0]))
         self.assertNotIn("Earlier comment", unicode(rows[0]))
 
+    def test_diff_row_links_to_extra_details(self):
+        # The source package name links to the difference details.
+        derived_series = self.makeDerivedSeries(
+            parent_name='lucid', derived_name='derilucid')
+        difference = self.factory.makeDistroSeriesDifference(
+            derived_series=derived_series)
+
+        self.setDerivedSeriesUIFeatureFlag()
+        view = create_initialized_view(
+            derived_series, '+localpackagediffs')
+        soup = BeautifulSoup(view())
+        diff_table = soup.find('table', {'class': 'listing'})
+        row = diff_table.tbody.findAll('tr')[0]
+
+        href = canonical_url(difference).replace('http://launchpad.dev', '')
+        links = row.findAll('a', href=href)
+        self.assertEqual(1, len(links))
+        self.assertEqual(difference.source_package_name.name, links[0].string)
+
 
 class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
     """Test the series.milestone_batch_navigator attribute."""

=== added file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2010-09-21 11:03:45 +0000
@@ -0,0 +1,86 @@
+/* Copyright 2010 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Enhancements for the distroseries differences page.
+ *
+ * @module registry
+ * @submodule distroseriesdifferences_details
+ * @requires  io-base, lp.soyuz.base
+ */
+YUI.add('lp.registry.distroseriesdifferences_details', function(Y) {
+
+var namespace = Y.namespace('lp.registry.distroseriesdifferences_details');
+
+/*
+ * Setup the expandable rows for each difference.
+ *
+ * @method setup_expandable_rows
+ */
+namespace.setup_expandable_rows = function() {
+    var start_update = function(uri, container) {
+
+        var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+            'Fetching difference details ...')
+        container.insert(in_progress_message, 'replace');
+
+        var config = {
+            on: {
+                'success': function(transaction_id, response, args) {
+                    args.container.set(
+                        'innerHTML', response.responseText);
+                        // Change to insert(,'replace)
+                    },
+                'failure': function(transaction_id, response, args){
+                       var retry_handler = function(e) {
+                           e.preventDefault();
+                           start_update(args.uri, args.container);
+                           };
+                       var failure_message = Y.lp.soyuz.base.makeFailureNode(
+                           'Failed to fetch difference details.',
+                           retry_handler);
+                       args.container.insert(failure_message, 'replace');
+
+                       var anim = Y.lazr.anim.red_flash({
+                            node: args.container
+                            });
+                       anim.run();
+                }
+            },
+            arguments: {
+                'container': container,
+                'uri': uri
+            }
+        };
+        Y.io(uri, config);
+
+    };
+
+    var expander_handler = function(e) {
+        e.preventDefault();
+        var toggle = e.currentTarget;
+        var row = toggle.ancestor('tr');
+        toggle.toggleClass('treeCollapsed').toggleClass('treeExpanded');
+
+        // Only insert if there isn't already a container row there.
+        next_row = row.next();
+        if (next_row == null || !next_row.hasClass('diff-extra')) {
+            details_row = Y.Node.create(
+                '<table><tr class="diff-extra unseen"><td colspan="5"></td></tr></table>').one('tr');
+            row.insert(details_row, 'after');
+            var uri = toggle.get('href');
+            start_update(uri, details_row.one('td'));
+        } else {
+            details_row = next_row
+        }
+
+        details_row.toggleClass('unseen');
+
+    };
+    Y.all('table.listing a.toggle-extra').each(function(toggle){
+        toggle.addClass('treeCollapsed').addClass('sprite');
+        toggle.on("click", expander_handler);
+    })
+
+};
+
+}, "0.1", {"requires": ["io-base", "lp.soyuz.base"]});

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2010-09-08 09:18:11 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2010-09-21 11:03:45 +0000
@@ -45,8 +45,8 @@
             <tr tal:define="parent_source_pub difference/parent_source_pub;
                             source_pub difference/source_pub;
                             signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;">
-              <td><span
-                      tal:replace="parent_source_pub/source_package_name">Foo</span>
+              <td><a tal:attributes="href difference/fmt:url" class="toggle-extra"
+                     tal:content="parent_source_pub/source_package_name">Foo</a>
               </td>
               <td><a tal:attributes="href parent_source_pub/sourcepackagerelease/fmt:url">
                     <tal:replace
@@ -79,6 +79,13 @@
           </tbody>
         </table>
       </div>
+<script type="text/javascript">
+LPS.use('lp.registry.distroseriesdifferences_details', function(Y) {
+  diff_module = Y.lp.registry.distroseriesdifferences_details
+
+  Y.on('domready', diff_module.setup_expandable_rows);
+});
+</script>
 
     </div>
 

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-09-14 13:37:17 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2010-09-21 11:03:45 +0000
@@ -4,31 +4,43 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   i18n:domain="launchpad">
   <dl>
-    <dt>Description:</dt>
-    <dd><tal:description replace="view/summary" /></dd>
+    <dt>Binary descriptions:</dt>
+      <dd><ul>
+        <li tal:repeat="summary view/binary_summaries">
+          <tal:description replace="summary" /></li>
+      </ul></dd>
     <dt>Last common version:</dt>
     <dd>Not implemented.</dd>
-    <dt>Package differences:</dt>
-    <tal:source-diff-option condition="context/source_pub">
-    <dd tal:condition="context/package_diff"
-        tal:content="structure context/package_diff/fmt:link">
-      <a>link to a diff</a></dd>
-    <dd tal:condition="not: context/package_diff" class="request-derived-diff">
-      Base version to derived <span
+    <dt>Differences from last common version:</dt>
+    <dd>
+      <ul>
+        <tal:source-diff-option condition="context/source_pub">
+        <li tal:condition="context/package_diff"
+            tal:content="structure context/package_diff/fmt:link">
+          <a>link to a diff</a></li>
+        <li tal:condition="not: context/package_diff" class="request-derived-diff">
+          <span tal:replace="context/derived_series/displayname">
+            Derilucid</span> version: <span
           tal:replace="context/source_version">1.2.3</span>
-    </dd>
-    </tal:source-diff-option>
+        </li>
+        </tal:source-diff-option>
 
-    <tal:parent-diff-option condition="context/parent_source_pub">
-    <dd tal:condition="context/parent_package_diff"
-        tal:content="structure context/parent_package_diff/fmt:link">
-      <a>link to a diff</a></dd>
-    <dd tal:condition="not: context/parent_package_diff" class="request-derived-diff">
-      Base version to parent <span
-          tal:replace="context/parent_source_version">1.2.3</span>
+        <tal:parent-diff-option condition="context/parent_source_pub">
+        <li tal:condition="context/parent_package_diff"
+            tal:content="structure context/parent_package_diff/fmt:link">
+          <a>link to a diff</a></li>
+        <li tal:condition="not: context/parent_package_diff" class="request-derived-diff">
+          <span
+              tal:replace="context/derived_series/parent_series/displayname">
+              Lucid</span> version: <span
+              tal:replace="context/parent_source_version">1.2.3</span>
+        </li>
+        </tal:parent-diff-option>
+      </ul>
     </dd>
-    </tal:parent-diff-option>
   </dl>
+
   <h2>Comments:</h2>
+  <tal:conversation replace="structure view/@@+render"/>
 
 </tal:root>

=== added file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py	2010-09-21 11:03:45 +0000
@@ -0,0 +1,42 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import transaction
+
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.windmill.testing import constants
+from lp.registry.windmill.testing import RegistryWindmillLayer
+from lp.services.features.model import FeatureFlag, getFeatureStore
+from lp.testing import WindmillTestCase
+
+
+class TestDistroSeriesDifferenceExtraJS(WindmillTestCase):
+    """Each listed source package can be expanded for extra information."""
+
+    layer = RegistryWindmillLayer
+
+    def setUp(self):
+        """Enable the feature and populate with data."""
+        super(TestDistroSeriesDifferenceExtraJS, self).setUp()
+        # First just ensure that the feature is enabled.
+        getFeatureStore().add(FeatureFlag(
+            scope=u'default', flag=u'soyuz.derived-series-ui.enabled',
+            value=u'on', priority=1))
+
+        # Setup the difference record.
+        self.diff = self.factory.makeDistroSeriesDifference(
+            source_package_name_str="foo", versions=dict(
+                derived='1.15-2ubuntu1derilucid2', parent='1.17-1'))
+        transaction.commit()
+
+        self.package_diffs_url = (
+            canonical_url(self.diff.derived_series) + '/+localpackagediffs')
+
+    def test_diff_extra_details_available(self):
+        """A successful request for the extra info updates the display."""
+        self.client.open(url=self.package_diffs_url)
+        self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
+        self.client.click(link=u'foo')
+        self.client.waits.forElement(
+            classname=u'diff-extra', timeout=constants.FOR_ELEMENT)
+

=== modified file 'lib/lp/services/comments/browser/configure.zcml'
--- lib/lp/services/comments/browser/configure.zcml	2009-07-17 02:25:09 +0000
+++ lib/lp/services/comments/browser/configure.zcml	2010-09-21 11:03:45 +0000
@@ -15,10 +15,18 @@
       permission="zope.Public"
       template="../templates/conversation.pt"/>
 
-  <browser:page
-      name="+render"
+  <browser:pages
       for="lp.services.comments.interfaces.conversation.IComment"
-      permission="zope.Public"
-      template="../templates/comment.pt"/>
+      permission="zope.Public">
+      <browser:page
+          name="+render"
+          template="../templates/comment.pt"/>
+      <browser:page
+          name="+comment-header"
+          template="../templates/comment-header.pt"/>
+      <browser:page
+          name="+comment-body"
+          template="../templates/comment-body.pt"/>
+  </browser:pages>
 
 </configure>

=== modified file 'lib/lp/services/comments/interfaces/conversation.py'
--- lib/lp/services/comments/interfaces/conversation.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/comments/interfaces/conversation.py	2010-09-21 11:03:45 +0000
@@ -17,6 +17,8 @@
 from zope.interface import Interface
 from zope.schema import (
     Bool,
+    Datetime,
+    Text,
     TextLine,
     )
 
@@ -37,6 +39,22 @@
         description=_("Does the comment have a footer?"),
         readonly=True)
 
+    body_text = Text(
+        description=_("The body text of the comment."),
+        readonly=True)
+
+    comment_author = Reference(
+        # Really IPerson.
+        Interface, title=_("The author of the comment."),
+        readonly=True)
+
+    comment_date = Datetime(
+        title=_('Comment date.'), readonly=True)
+
+    display_attachments = Bool(
+        description=_("Should attachments be displayed for this comment."),
+        readonly=True)
+
 
 class IConversation(Interface):
     """A conversation has a number of comments."""

=== added file 'lib/lp/services/comments/templates/comment-body.pt'
--- lib/lp/services/comments/templates/comment-body.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/services/comments/templates/comment-body.pt	2010-09-21 11:03:45 +0000
@@ -0,0 +1,8 @@
+<tal:root
+   xmlns:tal="http://xml.zope.org/namespaces/tal"
+   xmlns:metal="http://xml.zope.org/namespaces/metal"
+   omit-tag="">
+
+  <tal:message replace="structure context/body_text/fmt:obfuscate-email/fmt:nice_pre" />
+
+</tal:root>

=== added file 'lib/lp/services/comments/templates/comment-header.pt'
--- lib/lp/services/comments/templates/comment-header.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/services/comments/templates/comment-header.pt	2010-09-21 11:03:45 +0000
@@ -0,0 +1,9 @@
+<tal:root
+   xmlns:tal="http://xml.zope.org/namespaces/tal"
+   xmlns:metal="http://xml.zope.org/namespaces/metal"
+   omit-tag="">
+
+  <tal:author replace="structure context/comment_author/fmt:link:mainsite"/>
+  <tal:has-body condition="context/has_body">wrote</tal:has-body>
+  <tal:date replace="context/comment_date/fmt:displaydate" />
+</tal:root>