Launchpad itself

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

  1. 635005-difference-details-2
  2. Merge into db-devel
Proposed by Michael Nelson
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: 9813
Proposed branch: lp:~michael.nelson/launchpad/635005-difference-details-2
Merge into: lp:launchpad/db-devel
Diff against target: 958 lines (+452/-101)
21 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+4/-0)
lib/lp/app/templates/base-layout-macros.pt (+2/-0)
lib/lp/code/browser/branchmergeproposal.py (+9/-2)
lib/lp/code/browser/codereviewcomment.py (+39/-35)
lib/lp/code/browser/configure.zcml (+10/-3)
lib/lp/code/browser/tests/test_branchmergeproposal.py (+21/-2)
lib/lp/code/browser/tests/test_codereviewcomment.py (+26/-9)
lib/lp/code/templates/codereviewcomment-body.pt (+2/-2)
lib/lp/code/templates/codereviewcomment-header.pt (+3/-3)
lib/lp/registry/browser/configure.zcml (+4/-1)
lib/lp/registry/browser/distroseriesdifference.py (+40/-6)
lib/lp/registry/browser/tests/test_distroseriesdifference_views.py (+58/-13)
lib/lp/registry/browser/tests/test_series_views.py (+19/-0)
lib/lp/registry/javascript/distroseriesdifferences_details.js (+86/-0)
lib/lp/registry/templates/distroseries-localdifferences.pt (+9/-2)
lib/lp/registry/templates/distroseriesdifference-listing-extra.pt (+31/-19)
lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py (+42/-0)
lib/lp/services/comments/browser/configure.zcml (+12/-4)
lib/lp/services/comments/interfaces/conversation.py (+18/-0)
lib/lp/services/comments/templates/comment-body.pt (+8/-0)
lib/lp/services/comments/templates/comment-header.pt (+9/-0)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/635005-difference-details-2
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Curtis Hovey (community) ui Approve
Abel Deuring (community) code Approve
Review via email: mp+35640@code.launchpad.net

Commit message

Add extra details of DistroSeriesDifferences via Javascript.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

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*)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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 :)

Revision history for this message
Henning Eggers (henninge) wrote :

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*)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.

Revision history for this message
Abel Deuring (adeuring) wrote :

nice work!

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

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

review: Approve (ui)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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.

Revision history for this message
Henning Eggers (henninge) wrote :

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

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
2--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-08-27 11:19:54 +0000
3+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-09-21 11:03:45 +0000
4@@ -89,6 +89,7 @@
5 IStructuralSubscription,
6 IStructuralSubscriptionTarget,
7 )
8+from lp.services.comments.interfaces.conversation import IComment
9 from lp.soyuz.enums import (
10 PackagePublishingStatus,
11 PackageUploadCustomFormat,
12@@ -321,6 +322,9 @@
13 IBuildFarmJob['status'].vocabulary = BuildStatus
14 IBuildFarmJob['buildqueue_record'].schema = IBuildQueue
15
16+# IComment
17+IComment['comment_author'].schema = IPerson
18+
19 # IDistribution
20 IDistribution['series'].value_type.schema = IDistroSeries
21 patch_reference_property(
22
23=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
24--- lib/lp/app/templates/base-layout-macros.pt 2010-08-20 13:33:51 +0000
25+++ lib/lp/app/templates/base-layout-macros.pt 2010-09-21 11:03:45 +0000
26@@ -183,6 +183,8 @@
27 <script type="text/javascript"
28 tal:attributes="src string:${lp_js}/bugs/bugtracker_overlay.js"></script>
29 <script type="text/javascript"
30+ tal:attributes="src string:${lp_js}/registry/distroseriesdifferences_details.js"></script>
31+ <script type="text/javascript"
32 tal:attributes="src string:${lp_js}/registry/milestoneoverlay.js"></script>
33 <script type="text/javascript"
34 tal:attributes="src string:${lp_js}/registry/milestonetable.js"></script>
35
36=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
37--- lib/lp/code/browser/branchmergeproposal.py 2010-08-24 10:45:57 +0000
38+++ lib/lp/code/browser/branchmergeproposal.py 2010-09-21 11:03:45 +0000
39@@ -547,7 +547,7 @@
40 return diff_text.count('\n') >= config.diff.max_format_lines
41
42
43-class ICodeReviewNewRevisions(Interface):
44+class ICodeReviewNewRevisions(IComment):
45 """Marker interface used to register views for CodeReviewNewRevisions."""
46
47
48@@ -557,7 +557,7 @@
49 Each object instance represents a number of revisions scanned at a
50 particular time.
51 """
52- implements(IComment, ICodeReviewNewRevisions)
53+ implements(ICodeReviewNewRevisions)
54
55 def __init__(self, revisions, date, branch):
56 self.revisions = revisions
57@@ -567,6 +567,13 @@
58 # The date attribute is used to sort the comments in the conversation.
59 self.date = date
60
61+ # Other standard IComment attributes are not used.
62+ self.extra_css_class = None
63+ self.comment_author = None
64+ self.body_text = None
65+ self.comment_date = None
66+ self.display_attachments = False
67+
68
69 class CodeReviewNewRevisionsView(LaunchpadView):
70 """The view for rendering the new revisions."""
71
72=== modified file 'lib/lp/code/browser/codereviewcomment.py'
73--- lib/lp/code/browser/codereviewcomment.py 2010-08-24 10:45:57 +0000
74+++ lib/lp/code/browser/codereviewcomment.py 2010-09-21 11:03:45 +0000
75@@ -43,6 +43,10 @@
76 from lp.services.propertycache import cachedproperty
77
78
79+class ICodeReviewDisplayComment(IComment, ICodeReviewComment):
80+ """Marker interface for displaying code review comments."""
81+
82+
83 class CodeReviewDisplayComment:
84 """A code review comment or activity or both.
85
86@@ -51,7 +55,7 @@
87 only code in the model itself.
88 """
89
90- implements(IComment)
91+ implements(ICodeReviewDisplayComment)
92
93 delegates(ICodeReviewComment, 'comment')
94
95@@ -70,6 +74,40 @@
96 else:
97 return ''
98
99+ @cachedproperty
100+ def comment_author(self):
101+ """The author of the comment."""
102+ return self.comment.message.owner
103+
104+ @cachedproperty
105+ def has_body(self):
106+ """Is there body text?"""
107+ return bool(self.body_text)
108+
109+ @cachedproperty
110+ def body_text(self):
111+ """Get the body text for the message."""
112+ return self.comment.message_body
113+
114+ @cachedproperty
115+ def comment_date(self):
116+ """The date of the comment."""
117+ return self.comment.message.datecreated
118+
119+ @cachedproperty
120+ def all_attachments(self):
121+ return self.comment.getAttachments()
122+
123+ @cachedproperty
124+ def display_attachments(self):
125+ # Attachments to show.
126+ return [DiffAttachment(alias) for alias in self.all_attachments[0]]
127+
128+ @cachedproperty
129+ def other_attachments(self):
130+ # Attachments to not show.
131+ return self.all_attachments[1]
132+
133
134 class CodeReviewCommentPrimaryContext:
135 """The primary context is the comment is that of the source branch."""
136@@ -132,45 +170,11 @@
137 """The decorated code review comment."""
138 return CodeReviewDisplayComment(self.context)
139
140- @cachedproperty
141- def comment_author(self):
142- """The author of the comment."""
143- return self.context.message.owner
144-
145- @cachedproperty
146- def has_body(self):
147- """Is there body text?"""
148- return bool(self.body_text)
149-
150- @cachedproperty
151- def body_text(self):
152- """Get the body text for the message."""
153- return self.context.message_body
154-
155- @cachedproperty
156- def comment_date(self):
157- """The date of the comment."""
158- return self.context.message.datecreated
159-
160 # Should the comment be shown in full?
161 full_comment = True
162 # Show comment expanders?
163 show_expanders = False
164
165- @cachedproperty
166- def all_attachments(self):
167- return self.context.getAttachments()
168-
169- @cachedproperty
170- def display_attachments(self):
171- # Attachments to show.
172- return [DiffAttachment(alias) for alias in self.all_attachments[0]]
173-
174- @cachedproperty
175- def other_attachments(self):
176- # Attachments to not show.
177- return self.all_attachments[1]
178-
179
180 class CodeReviewCommentSummary(CodeReviewCommentView):
181 """Summary view of a CodeReviewComment"""
182
183=== modified file 'lib/lp/code/browser/configure.zcml'
184--- lib/lp/code/browser/configure.zcml 2010-09-20 00:20:42 +0000
185+++ lib/lp/code/browser/configure.zcml 2010-09-21 11:03:45 +0000
186@@ -691,6 +691,16 @@
187 name="+index"
188 template="../templates/codereviewcomment-index.pt"/>
189 <browser:page
190+ name="+fragment"
191+ template="../templates/codereviewcomment-fragment.pt"/>
192+ </browser:pages>
193+ <browser:pages
194+ facet="branches"
195+ for="lp.code.browser.codereviewcomment.ICodeReviewDisplayComment"
196+ layer="lp.code.publisher.CodeLayer"
197+ class="lp.code.browser.codereviewcomment.CodeReviewCommentView"
198+ permission="zope.Public">
199+ <browser:page
200 name="+comment-header"
201 template="../templates/codereviewcomment-header.pt"/>
202 <browser:page
203@@ -699,9 +709,6 @@
204 <browser:page
205 name="+comment-footer"
206 template="../templates/codereviewcomment-footer.pt"/>
207- <browser:page
208- name="+fragment"
209- template="../templates/codereviewcomment-fragment.pt"/>
210 </browser:pages>
211 <browser:pages
212 facet="branches"
213
214=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
215--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2010-08-22 21:34:16 +0000
216+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2010-09-21 11:03:45 +0000
217@@ -24,6 +24,7 @@
218 from canonical.launchpad.database.message import MessageSet
219 from canonical.launchpad.webapp.interfaces import IPrimaryContext
220 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
221+from canonical.launchpad.webapp.testing import verifyObject
222 from canonical.testing import (
223 DatabaseFunctionalLayer,
224 LaunchpadFunctionalLayer,
225@@ -36,8 +37,10 @@
226 BranchMergeProposalMergedView,
227 BranchMergeProposalVoteView,
228 DecoratedCodeReviewVoteReference,
229+ ICodeReviewNewRevisions,
230 latest_proposals_for_each_branch,
231 )
232+from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
233 from lp.code.enums import (
234 BranchMergeProposalStatus,
235 CodeReviewVote,
236@@ -634,6 +637,21 @@
237 [revisions[2], revisions[3]]]
238 self.assertRevisionGroups(bmp, expected_groups)
239
240+ def test_CodeReviewNewRevisions_implements_ICodeReviewNewRevisions(self):
241+ # The browser helper class implements its interface.
242+ review_date = datetime(2009, 9, 10, tzinfo=pytz.UTC)
243+ revision_date = review_date + timedelta(days=1)
244+ bmp = self.factory.makeBranchMergeProposal(
245+ date_created=review_date)
246+ revision = add_revision_to_branch(
247+ self.factory, bmp.source_branch, revision_date)
248+
249+ view = create_initialized_view(bmp, '+index')
250+ groups = view._getRevisionsSinceReviewStart()
251+ new_revisions = groups[0]
252+
253+ self.assertTrue(verifyObject(ICodeReviewNewRevisions, new_revisions))
254+
255 def test_include_superseded_comments(self):
256 for x, time in zip(range(3), time_counter()):
257 if x != 0:
258@@ -767,7 +785,8 @@
259 body='testing',
260 attachments=[('test.diff', 'text/plain', attachment_body)])
261 message = MessageSet().fromEmail(msg.as_string())
262- return bmp.createCommentFromMessage(message, None, None, msg)
263+ return CodeReviewDisplayComment(
264+ bmp.createCommentFromMessage(message, None, None, msg))
265
266 def test_nonascii_in_attachment_renders(self):
267 # The view should render without errors.
268@@ -783,7 +802,7 @@
269 # Need to commit in order to read the diff out of the librarian.
270 transaction.commit()
271 view = create_initialized_view(comment, '+comment-body')
272- [diff_attachment] = view.display_attachments
273+ [diff_attachment] = view.comment.display_attachments
274 self.assertEqual(u'\u2615', diff_attachment.diff_text)
275
276
277
278=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
279--- lib/lp/code/browser/tests/test_codereviewcomment.py 2010-08-20 20:31:18 +0000
280+++ lib/lp/code/browser/tests/test_codereviewcomment.py 2010-09-21 11:03:45 +0000
281@@ -3,35 +3,52 @@
282
283 """Unit tests for CodeReviewComments."""
284
285+from __future__ import with_statement
286+
287 __metaclass__ = type
288
289 import unittest
290
291 from canonical.launchpad.webapp.interfaces import IPrimaryContext
292+from canonical.launchpad.webapp.testing import verifyObject
293 from canonical.testing import DatabaseFunctionalLayer
294+from lp.code.browser.codereviewcomment import (
295+ CodeReviewDisplayComment,
296+ ICodeReviewDisplayComment,
297+ )
298 from lp.testing import (
299- login_person,
300+ person_logged_in,
301 TestCaseWithFactory,
302 )
303
304
305-class TestCodeReviewCommentPrimaryContext(TestCaseWithFactory):
306- # Tests the adaptation of a code review comment into a primary context.
307+class TestCodeReviewComments(TestCaseWithFactory):
308
309 layer = DatabaseFunctionalLayer
310
311 def testPrimaryContext(self):
312+ # Tests the adaptation of a code review comment into a primary
313+ # context.
314 # We need a person to make a comment.
315- commenter = self.factory.makePerson()
316- login_person(commenter)
317- # The primary context of a code review comment is the same as the
318- # primary context for the branch merge proposal that the comment is
319- # for.
320- comment = self.factory.makeCodeReviewComment()
321+ with person_logged_in(self.factory.makePerson()):
322+ # The primary context of a code review comment is the same
323+ # as the primary context for the branch merge proposal that
324+ # the comment is for.
325+ comment = self.factory.makeCodeReviewComment()
326+
327 self.assertEqual(
328 IPrimaryContext(comment).context,
329 IPrimaryContext(comment.branch_merge_proposal).context)
330
331+ def test_display_comment_provides_icodereviewdisplaycomment(self):
332+ # The CodeReviewDisplayComment class provides IComment.
333+ with person_logged_in(self.factory.makePerson()):
334+ comment = self.factory.makeCodeReviewComment()
335+
336+ display_comment = CodeReviewDisplayComment(comment)
337+
338+ verifyObject(ICodeReviewDisplayComment, display_comment)
339+
340
341 def test_suite():
342 return unittest.TestLoader().loadTestsFromName(__name__)
343
344=== modified file 'lib/lp/code/templates/codereviewcomment-body.pt'
345--- lib/lp/code/templates/codereviewcomment-body.pt 2010-04-16 04:20:43 +0000
346+++ lib/lp/code/templates/codereviewcomment-body.pt 2010-09-21 11:03:45 +0000
347@@ -3,9 +3,9 @@
348 xmlns:metal="http://xml.zope.org/namespaces/metal"
349 omit-tag="">
350
351- <tal:message replace="structure view/body_text/fmt:obfuscate-email/fmt:nice_pre" />
352+ <tal:message replace="structure view/comment/body_text/fmt:obfuscate-email/fmt:nice_pre" />
353
354- <tal:good-attachments repeat="attachment view/display_attachments">
355+ <tal:good-attachments repeat="attachment view/comment/display_attachments">
356 <div class="boardComment attachment">
357 <div class="boardCommentDetails filename"><a tal:content="attachment/filename" tal:attributes="href attachment/getURL"/></div>
358 <div class="boardCommentBody attachmentBody" tal:content="structure attachment/diff_text/fmt:diff"/>
359
360=== modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
361--- lib/lp/code/templates/codereviewcomment-header.pt 2010-02-18 16:40:09 +0000
362+++ lib/lp/code/templates/codereviewcomment-header.pt 2010-09-21 11:03:45 +0000
363@@ -3,9 +3,9 @@
364 xmlns:metal="http://xml.zope.org/namespaces/metal"
365 omit-tag="">
366
367- <tal:author replace="structure view/comment_author/fmt:link:mainsite"/>
368- <tal:has-body condition="view/has_body">wrote</tal:has-body>
369- <tal:date replace="view/comment_date/fmt:displaydate" />
370+ <tal:author replace="structure context/comment_author/fmt:link:mainsite"/>
371+ <tal:has-body condition="context/has_body">wrote</tal:has-body>
372+ <tal:date replace="context/comment_date/fmt:displaydate" />
373 <span tal:condition="context/from_superseded" class="sprite warning-icon"
374 style="float: right">Posted in <a
375 tal:attributes="href context/branch_merge_proposal/fmt:url">a
376
377=== modified file 'lib/lp/registry/browser/configure.zcml'
378--- lib/lp/registry/browser/configure.zcml 2010-09-13 10:04:20 +0000
379+++ lib/lp/registry/browser/configure.zcml 2010-09-21 11:03:45 +0000
380@@ -151,7 +151,7 @@
381 permission="zope.Public"/>
382 <browser:url
383 for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
384- path_expression="string:+difference/${difference/source_package_name}"
385+ path_expression="string:+difference/${source_package_name/name}"
386 rootsite="mainsite"
387 attribute_to_parent="derived_series"/>
388 <browser:page
389@@ -160,6 +160,9 @@
390 class="lp.registry.browser.distroseriesdifference.DistroSeriesDifferenceView"
391 template="../templates/distroseriesdifference-listing-extra.pt"
392 permission="zope.Public"/>
393+ <browser:defaultView
394+ for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
395+ name="+listing-distroseries-extra"/>
396 <browser:menus
397 classes="
398 DistroSeriesFacets
399
400=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
401--- lib/lp/registry/browser/distroseriesdifference.py 2010-09-13 15:09:48 +0000
402+++ lib/lp/registry/browser/distroseriesdifference.py 2010-09-21 11:03:45 +0000
403@@ -8,14 +8,22 @@
404 'DistroSeriesDifferenceView',
405 ]
406
407+from zope.interface import implements
408+
409 from canonical.launchpad.webapp.publisher import LaunchpadView
410+from lp.services.comments.interfaces.conversation import (
411+ IComment,
412+ IConversation,
413+ )
414
415
416 class DistroSeriesDifferenceView(LaunchpadView):
417
418+ implements(IConversation)
419+
420 @property
421- def summary(self):
422- """Return the summary of the related source package."""
423+ def binary_summaries(self):
424+ """Return the summary of the related binary packages."""
425 source_pub = None
426 if self.context.source_pub is not None:
427 source_pub = self.context.source_pub
428@@ -23,7 +31,33 @@
429 source_pub = self.context.parent_source_pub
430
431 if source_pub is not None:
432- return source_pub.meta_sourcepackage.summary
433- else:
434- return None
435-
436+ summary = source_pub.meta_sourcepackage.summary
437+ if summary:
438+ return summary.split('\n')
439+
440+ return None
441+
442+ @property
443+ def comments(self):
444+ """See `IConversation`."""
445+ # Could use a generator here?
446+ return [
447+ DistroSeriesDifferenceDisplayComment(comment) for
448+ comment in self.context.getComments()]
449+
450+
451+class DistroSeriesDifferenceDisplayComment:
452+ """Used simply to provide `IComment` for rendering."""
453+ implements(IComment)
454+
455+ has_body = True
456+ has_footer = False
457+ display_attachments = False
458+ extra_css_class = ''
459+
460+ def __init__(self, comment):
461+ """Setup the attributes required by `IComment`."""
462+ self.message_body = comment.comment
463+ self.comment_author = comment.message.owner
464+ self.comment_date = comment.message.datecreated
465+ self.body_text = comment.comment
466
467=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
468--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-14 13:37:17 +0000
469+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-21 11:03:45 +0000
470@@ -10,12 +10,20 @@
471 from BeautifulSoup import BeautifulSoup
472 from zope.component import getUtility
473
474+from canonical.launchpad.webapp.testing import verifyObject
475 from canonical.testing import (
476 DatabaseFunctionalLayer,
477 LaunchpadFunctionalLayer,
478 )
479+from lp.registry.browser.distroseriesdifference import (
480+ DistroSeriesDifferenceDisplayComment,
481+ )
482 from lp.registry.enum import DistroSeriesDifferenceType
483 from lp.registry.interfaces.distroseriesdifference import IDistroSeriesDifferenceSource
484+from lp.services.comments.interfaces.conversation import (
485+ IComment,
486+ IConversation,
487+ )
488 from lp.soyuz.enums import PackagePublishingStatus
489 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
490 from lp.testing import (
491@@ -30,10 +38,28 @@
492
493 layer = DatabaseFunctionalLayer
494
495+ def test_provides_conversation(self):
496+ # The DSDView provides a conversation implementation.
497+ ds_diff = self.factory.makeDistroSeriesDifference()
498+
499+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
500+ self.assertTrue(verifyObject(IConversation, view))
501+
502+ def test_comment_for_display_provides_icomment(self):
503+ # The DSDDisplayComment browser object provides IComment.
504+ ds_diff = self.factory.makeDistroSeriesDifference()
505+ owner = ds_diff.derived_series.owner
506+ with person_logged_in(owner):
507+ comment = ds_diff.addComment(owner, "I'm working on this.")
508+ comment_for_display = DistroSeriesDifferenceDisplayComment(comment)
509+
510+ self.assertTrue(verifyObject(IComment, comment_for_display))
511+
512 def addSummaryToDifference(self, distro_series_difference):
513 """Helper that adds binaries with summary info to the source pubs."""
514 distro_series = distro_series_difference.derived_series
515- source_package_name_str = distro_series_difference.source_package_name.name
516+ source_package_name_str = (
517+ distro_series_difference.source_package_name.name)
518 stp = SoyuzTestPublisher()
519
520 if distro_series_difference.difference_type == (
521@@ -52,7 +78,7 @@
522 distro_series, source_package_name_str)
523 return ds_diff
524
525- def test_summary_for_source_pub(self):
526+ def test_binary_summaries_for_source_pub(self):
527 # For packages unique to the derived series (or different
528 # versions) the summary is based on the derived source pub.
529 ds_diff = self.factory.makeDistroSeriesDifference()
530@@ -60,11 +86,13 @@
531
532 view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
533
534- self.assertIsNot(None, view.summary)
535- self.assertEqual(
536- ds_diff.source_pub.meta_sourcepackage.summary, view.summary)
537+ self.assertIsNot(None, view.binary_summaries)
538+ self.assertEqual([
539+ u'flubber-bin: summary for flubber-bin',
540+ u'flubber-lib: summary for flubber-lib',
541+ ], view.binary_summaries)
542
543- def test_summary_for_missing_difference(self):
544+ def test_binary_summaries_for_missing_difference(self):
545 # For packages only in the parent series, the summary is based
546 # on the parent publication.
547 ds_diff = self.factory.makeDistroSeriesDifference(
548@@ -74,12 +102,13 @@
549
550 view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
551
552- self.assertIsNot(None, view.summary)
553- self.assertEqual(
554- ds_diff.parent_source_pub.meta_sourcepackage.summary,
555- view.summary)
556+ self.assertIsNot(None, view.binary_summaries)
557+ self.assertEqual([
558+ u'flubber-bin: summary for flubber-bin',
559+ u'flubber-lib: summary for flubber-lib',
560+ ], view.binary_summaries)
561
562- def test_summary_no_pubs(self):
563+ def test_binary_summaries_no_pubs(self):
564 # If the difference has been resolved by removing packages then
565 # there will not be a summary.
566 ds_diff = self.factory.makeDistroSeriesDifference(
567@@ -93,7 +122,7 @@
568
569 self.assertIs(None, ds_diff.parent_source_pub)
570 self.assertIs(None, ds_diff.source_pub)
571- self.assertIs(None, view.summary)
572+ self.assertIs(None, view.binary_summaries)
573
574
575 class DistroSeriesDifferenceTemplateTestCase(TestCaseWithFactory):
576@@ -103,7 +132,7 @@
577 def number_of_request_diff_texts(self, html):
578 """Check that the html doesn't include the request diff text."""
579 soup = BeautifulSoup(html)
580- return len(soup.findAll('dd', 'request-derived-diff'))
581+ return len(soup.findAll('li', 'request-derived-diff'))
582
583 def contains_one_link_to_diff(self, html, package_diff):
584 """Return whether the html contains a link to the diff content."""
585@@ -170,3 +199,19 @@
586
587 view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
588 self.assertEqual(1, self.number_of_request_diff_texts(view()))
589+
590+ def test_comments_rendered(self):
591+ # If there are comments on the difference, they are rendered.
592+ ds_diff = self.factory.makeDistroSeriesDifference()
593+ owner = ds_diff.derived_series.owner
594+ with person_logged_in(owner):
595+ ds_diff.addComment(owner, "I'm working on this.")
596+ ds_diff.addComment(owner, "Here's another comment.")
597+
598+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
599+ soup = BeautifulSoup(view())
600+
601+ self.assertEqual(
602+ 1, len(soup.findAll('pre', text="I'm working on this.")))
603+ self.assertEqual(
604+ 1, len(soup.findAll('pre', text="Here's another comment.")))
605
606=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
607--- lib/lp/registry/browser/tests/test_series_views.py 2010-09-08 07:53:06 +0000
608+++ lib/lp/registry/browser/tests/test_series_views.py 2010-09-21 11:03:45 +0000
609@@ -181,6 +181,25 @@
610 self.assertIn("Latest comment", unicode(rows[0]))
611 self.assertNotIn("Earlier comment", unicode(rows[0]))
612
613+ def test_diff_row_links_to_extra_details(self):
614+ # The source package name links to the difference details.
615+ derived_series = self.makeDerivedSeries(
616+ parent_name='lucid', derived_name='derilucid')
617+ difference = self.factory.makeDistroSeriesDifference(
618+ derived_series=derived_series)
619+
620+ self.setDerivedSeriesUIFeatureFlag()
621+ view = create_initialized_view(
622+ derived_series, '+localpackagediffs')
623+ soup = BeautifulSoup(view())
624+ diff_table = soup.find('table', {'class': 'listing'})
625+ row = diff_table.tbody.findAll('tr')[0]
626+
627+ href = canonical_url(difference).replace('http://launchpad.dev', '')
628+ links = row.findAll('a', href=href)
629+ self.assertEqual(1, len(links))
630+ self.assertEqual(difference.source_package_name.name, links[0].string)
631+
632
633 class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
634 """Test the series.milestone_batch_navigator attribute."""
635
636=== added file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
637--- lib/lp/registry/javascript/distroseriesdifferences_details.js 1970-01-01 00:00:00 +0000
638+++ lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-09-21 11:03:45 +0000
639@@ -0,0 +1,86 @@
640+/* Copyright 2010 Canonical Ltd. This software is licensed under the
641+ * GNU Affero General Public License version 3 (see the file LICENSE).
642+ *
643+ * Enhancements for the distroseries differences page.
644+ *
645+ * @module registry
646+ * @submodule distroseriesdifferences_details
647+ * @requires io-base, lp.soyuz.base
648+ */
649+YUI.add('lp.registry.distroseriesdifferences_details', function(Y) {
650+
651+var namespace = Y.namespace('lp.registry.distroseriesdifferences_details');
652+
653+/*
654+ * Setup the expandable rows for each difference.
655+ *
656+ * @method setup_expandable_rows
657+ */
658+namespace.setup_expandable_rows = function() {
659+ var start_update = function(uri, container) {
660+
661+ var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
662+ 'Fetching difference details ...')
663+ container.insert(in_progress_message, 'replace');
664+
665+ var config = {
666+ on: {
667+ 'success': function(transaction_id, response, args) {
668+ args.container.set(
669+ 'innerHTML', response.responseText);
670+ // Change to insert(,'replace)
671+ },
672+ 'failure': function(transaction_id, response, args){
673+ var retry_handler = function(e) {
674+ e.preventDefault();
675+ start_update(args.uri, args.container);
676+ };
677+ var failure_message = Y.lp.soyuz.base.makeFailureNode(
678+ 'Failed to fetch difference details.',
679+ retry_handler);
680+ args.container.insert(failure_message, 'replace');
681+
682+ var anim = Y.lazr.anim.red_flash({
683+ node: args.container
684+ });
685+ anim.run();
686+ }
687+ },
688+ arguments: {
689+ 'container': container,
690+ 'uri': uri
691+ }
692+ };
693+ Y.io(uri, config);
694+
695+ };
696+
697+ var expander_handler = function(e) {
698+ e.preventDefault();
699+ var toggle = e.currentTarget;
700+ var row = toggle.ancestor('tr');
701+ toggle.toggleClass('treeCollapsed').toggleClass('treeExpanded');
702+
703+ // Only insert if there isn't already a container row there.
704+ next_row = row.next();
705+ if (next_row == null || !next_row.hasClass('diff-extra')) {
706+ details_row = Y.Node.create(
707+ '<table><tr class="diff-extra unseen"><td colspan="5"></td></tr></table>').one('tr');
708+ row.insert(details_row, 'after');
709+ var uri = toggle.get('href');
710+ start_update(uri, details_row.one('td'));
711+ } else {
712+ details_row = next_row
713+ }
714+
715+ details_row.toggleClass('unseen');
716+
717+ };
718+ Y.all('table.listing a.toggle-extra').each(function(toggle){
719+ toggle.addClass('treeCollapsed').addClass('sprite');
720+ toggle.on("click", expander_handler);
721+ })
722+
723+};
724+
725+}, "0.1", {"requires": ["io-base", "lp.soyuz.base"]});
726
727=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
728--- lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-08 09:18:11 +0000
729+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-21 11:03:45 +0000
730@@ -45,8 +45,8 @@
731 <tr tal:define="parent_source_pub difference/parent_source_pub;
732 source_pub difference/source_pub;
733 signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;">
734- <td><span
735- tal:replace="parent_source_pub/source_package_name">Foo</span>
736+ <td><a tal:attributes="href difference/fmt:url" class="toggle-extra"
737+ tal:content="parent_source_pub/source_package_name">Foo</a>
738 </td>
739 <td><a tal:attributes="href parent_source_pub/sourcepackagerelease/fmt:url">
740 <tal:replace
741@@ -79,6 +79,13 @@
742 </tbody>
743 </table>
744 </div>
745+<script type="text/javascript">
746+LPS.use('lp.registry.distroseriesdifferences_details', function(Y) {
747+ diff_module = Y.lp.registry.distroseriesdifferences_details
748+
749+ Y.on('domready', diff_module.setup_expandable_rows);
750+});
751+</script>
752
753 </div>
754
755
756=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
757--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-14 13:37:17 +0000
758+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-21 11:03:45 +0000
759@@ -4,31 +4,43 @@
760 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
761 i18n:domain="launchpad">
762 <dl>
763- <dt>Description:</dt>
764- <dd><tal:description replace="view/summary" /></dd>
765+ <dt>Binary descriptions:</dt>
766+ <dd><ul>
767+ <li tal:repeat="summary view/binary_summaries">
768+ <tal:description replace="summary" /></li>
769+ </ul></dd>
770 <dt>Last common version:</dt>
771 <dd>Not implemented.</dd>
772- <dt>Package differences:</dt>
773- <tal:source-diff-option condition="context/source_pub">
774- <dd tal:condition="context/package_diff"
775- tal:content="structure context/package_diff/fmt:link">
776- <a>link to a diff</a></dd>
777- <dd tal:condition="not: context/package_diff" class="request-derived-diff">
778- Base version to derived <span
779+ <dt>Differences from last common version:</dt>
780+ <dd>
781+ <ul>
782+ <tal:source-diff-option condition="context/source_pub">
783+ <li tal:condition="context/package_diff"
784+ tal:content="structure context/package_diff/fmt:link">
785+ <a>link to a diff</a></li>
786+ <li tal:condition="not: context/package_diff" class="request-derived-diff">
787+ <span tal:replace="context/derived_series/displayname">
788+ Derilucid</span> version: <span
789 tal:replace="context/source_version">1.2.3</span>
790- </dd>
791- </tal:source-diff-option>
792+ </li>
793+ </tal:source-diff-option>
794
795- <tal:parent-diff-option condition="context/parent_source_pub">
796- <dd tal:condition="context/parent_package_diff"
797- tal:content="structure context/parent_package_diff/fmt:link">
798- <a>link to a diff</a></dd>
799- <dd tal:condition="not: context/parent_package_diff" class="request-derived-diff">
800- Base version to parent <span
801- tal:replace="context/parent_source_version">1.2.3</span>
802+ <tal:parent-diff-option condition="context/parent_source_pub">
803+ <li tal:condition="context/parent_package_diff"
804+ tal:content="structure context/parent_package_diff/fmt:link">
805+ <a>link to a diff</a></li>
806+ <li tal:condition="not: context/parent_package_diff" class="request-derived-diff">
807+ <span
808+ tal:replace="context/derived_series/parent_series/displayname">
809+ Lucid</span> version: <span
810+ tal:replace="context/parent_source_version">1.2.3</span>
811+ </li>
812+ </tal:parent-diff-option>
813+ </ul>
814 </dd>
815- </tal:parent-diff-option>
816 </dl>
817+
818 <h2>Comments:</h2>
819+ <tal:conversation replace="structure view/@@+render"/>
820
821 </tal:root>
822
823=== added file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
824--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 1970-01-01 00:00:00 +0000
825+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 2010-09-21 11:03:45 +0000
826@@ -0,0 +1,42 @@
827+# Copyright 2010 Canonical Ltd. This software is licensed under the
828+# GNU Affero General Public License version 3 (see the file LICENSE).
829+
830+import transaction
831+
832+from canonical.launchpad.webapp.publisher import canonical_url
833+from canonical.launchpad.windmill.testing import constants
834+from lp.registry.windmill.testing import RegistryWindmillLayer
835+from lp.services.features.model import FeatureFlag, getFeatureStore
836+from lp.testing import WindmillTestCase
837+
838+
839+class TestDistroSeriesDifferenceExtraJS(WindmillTestCase):
840+ """Each listed source package can be expanded for extra information."""
841+
842+ layer = RegistryWindmillLayer
843+
844+ def setUp(self):
845+ """Enable the feature and populate with data."""
846+ super(TestDistroSeriesDifferenceExtraJS, self).setUp()
847+ # First just ensure that the feature is enabled.
848+ getFeatureStore().add(FeatureFlag(
849+ scope=u'default', flag=u'soyuz.derived-series-ui.enabled',
850+ value=u'on', priority=1))
851+
852+ # Setup the difference record.
853+ self.diff = self.factory.makeDistroSeriesDifference(
854+ source_package_name_str="foo", versions=dict(
855+ derived='1.15-2ubuntu1derilucid2', parent='1.17-1'))
856+ transaction.commit()
857+
858+ self.package_diffs_url = (
859+ canonical_url(self.diff.derived_series) + '/+localpackagediffs')
860+
861+ def test_diff_extra_details_available(self):
862+ """A successful request for the extra info updates the display."""
863+ self.client.open(url=self.package_diffs_url)
864+ self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
865+ self.client.click(link=u'foo')
866+ self.client.waits.forElement(
867+ classname=u'diff-extra', timeout=constants.FOR_ELEMENT)
868+
869
870=== modified file 'lib/lp/services/comments/browser/configure.zcml'
871--- lib/lp/services/comments/browser/configure.zcml 2009-07-17 02:25:09 +0000
872+++ lib/lp/services/comments/browser/configure.zcml 2010-09-21 11:03:45 +0000
873@@ -15,10 +15,18 @@
874 permission="zope.Public"
875 template="../templates/conversation.pt"/>
876
877- <browser:page
878- name="+render"
879+ <browser:pages
880 for="lp.services.comments.interfaces.conversation.IComment"
881- permission="zope.Public"
882- template="../templates/comment.pt"/>
883+ permission="zope.Public">
884+ <browser:page
885+ name="+render"
886+ template="../templates/comment.pt"/>
887+ <browser:page
888+ name="+comment-header"
889+ template="../templates/comment-header.pt"/>
890+ <browser:page
891+ name="+comment-body"
892+ template="../templates/comment-body.pt"/>
893+ </browser:pages>
894
895 </configure>
896
897=== modified file 'lib/lp/services/comments/interfaces/conversation.py'
898--- lib/lp/services/comments/interfaces/conversation.py 2010-08-20 20:31:18 +0000
899+++ lib/lp/services/comments/interfaces/conversation.py 2010-09-21 11:03:45 +0000
900@@ -17,6 +17,8 @@
901 from zope.interface import Interface
902 from zope.schema import (
903 Bool,
904+ Datetime,
905+ Text,
906 TextLine,
907 )
908
909@@ -37,6 +39,22 @@
910 description=_("Does the comment have a footer?"),
911 readonly=True)
912
913+ body_text = Text(
914+ description=_("The body text of the comment."),
915+ readonly=True)
916+
917+ comment_author = Reference(
918+ # Really IPerson.
919+ Interface, title=_("The author of the comment."),
920+ readonly=True)
921+
922+ comment_date = Datetime(
923+ title=_('Comment date.'), readonly=True)
924+
925+ display_attachments = Bool(
926+ description=_("Should attachments be displayed for this comment."),
927+ readonly=True)
928+
929
930 class IConversation(Interface):
931 """A conversation has a number of comments."""
932
933=== added file 'lib/lp/services/comments/templates/comment-body.pt'
934--- lib/lp/services/comments/templates/comment-body.pt 1970-01-01 00:00:00 +0000
935+++ lib/lp/services/comments/templates/comment-body.pt 2010-09-21 11:03:45 +0000
936@@ -0,0 +1,8 @@
937+<tal:root
938+ xmlns:tal="http://xml.zope.org/namespaces/tal"
939+ xmlns:metal="http://xml.zope.org/namespaces/metal"
940+ omit-tag="">
941+
942+ <tal:message replace="structure context/body_text/fmt:obfuscate-email/fmt:nice_pre" />
943+
944+</tal:root>
945
946=== added file 'lib/lp/services/comments/templates/comment-header.pt'
947--- lib/lp/services/comments/templates/comment-header.pt 1970-01-01 00:00:00 +0000
948+++ lib/lp/services/comments/templates/comment-header.pt 2010-09-21 11:03:45 +0000
949@@ -0,0 +1,9 @@
950+<tal:root
951+ xmlns:tal="http://xml.zope.org/namespaces/tal"
952+ xmlns:metal="http://xml.zope.org/namespaces/metal"
953+ omit-tag="">
954+
955+ <tal:author replace="structure context/comment_author/fmt:link:mainsite"/>
956+ <tal:has-body condition="context/has_body">wrote</tal:has-body>
957+ <tal:date replace="context/comment_date/fmt:displaydate" />
958+</tal:root>

Subscribers

People subscribed via source and target branches

to status/vote changes: