launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01298
[Merge] lp:~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff into lp:launchpad
Michael Nelson has proposed merging lp:~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
= Summary =
This branch exposes IDistroSeriesDifferenceComment objects for bug 649599,
getting ready to add comments via JS.
It follows on from:
https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/36442
== Pre-implementation notes ==
The UI has been discussed as part of the LEP:
https://dev.launchpad.net/LEP/DerivativeDistributions
in particular, the mockup at:
https://dev.launchpad.net/LEP/DerivativeDistributions?action=AttachFile&do=get&target=derived-series-diffs_uiround2.png
(Add comment will be what the next branch adds).
== Implementation details ==
Nothing out of the ordinary.
== Tests ==
bin/test -vvm test_distroseriesdifferencecomment -m
test_distroseriesdifference_webservice
== Demo and Q/A ==
None
= Launchpad lint =
I'm updating the lint in the two files below now.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/tests/test_distroseriesdifferencecomment.py
lib/lp/registry/browser/configure.zcml
lib/lp/registry/templates/distroseries-localdifferences.pt
lib/canonical/launchpad/interfaces/__init__.py
lib/lp/registry/interfaces/distroseriesdifference.py
lib/lp/registry/model/distroseriesdifferencecomment.py
lib/lp/registry/browser/distroseriesdifference.py
lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py
lib/lp/registry/model/distroseriesdifference.py
lib/lp/registry/interfaces/distroseriesdifferencecomment.py
lib/lp/registry/templates/distroseriesdifference-listing-extra.pt
lib/canonical/launchpad/interfaces/_schema_circular_imports.py
lib/lp/registry/browser/tests/test_distroseriesdifference_views.py
./lib/lp/registry/tests/test_distroseriesdifferencecomment.py
70: W391 blank line at end of file
52: Line exceeds 78 characters.
./lib/canonical/launchpad/interfaces/__init__.py
12: 'from canonical.launchpad.interfaces.launchpad import *' used; unable to detect undefined names
...
./lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py
20: 'person_logged_in' imported but unused
11: 'Store' imported but unused
--
https://code.launchpad.net/~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff/+merge/36966
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/649599-ajax-comment-on-dsdiff into lp:launchpad.
=== modified file 'lib/canonical/launchpad/interfaces/__init__.py'
--- lib/canonical/launchpad/interfaces/__init__.py 2010-09-20 14:37:52 +0000
+++ lib/canonical/launchpad/interfaces/__init__.py 2010-09-29 09:52:48 +0000
@@ -56,6 +56,7 @@
from lp.registry.interfaces.distributionmirror import *
from lp.registry.interfaces.distributionsourcepackage import *
from lp.registry.interfaces.distroseriesdifference import *
+from lp.registry.interfaces.distroseriesdifferencecomment import *
from lp.soyuz.interfaces.distributionsourcepackagecache import *
from lp.soyuz.interfaces.distributionsourcepackagerelease import *
from lp.registry.interfaces.series import *
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-09-17 10:50:51 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-09-29 09:52:48 +0000
@@ -77,6 +77,9 @@
IDistributionSourcePackage,
)
from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+ IDistroSeriesDifferenceComment,
+ )
from lp.registry.interfaces.person import (
IPerson,
IPersonPublic,
@@ -370,6 +373,9 @@
IDistroSeries, 'getPackageUploads', IPackageUpload)
patch_reference_property(IDistroSeries, 'parent_series', IDistroSeries)
+# IDistroSeriesDifferenceComment
+IDistroSeriesDifferenceComment['comment_author'].schema = IPerson
+
# IDistroArchSeries
patch_reference_property(IDistroArchSeries, 'main_archive', IArchive)
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2010-09-29 07:09:03 +0000
+++ lib/lp/registry/browser/configure.zcml 2010-09-29 09:52:48 +0000
@@ -180,6 +180,14 @@
<browser:defaultView
for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
name="+listing-distroseries-extra"/>
+ <browser:navigation
+ module="lp.registry.browser.distroseriesdifference"
+ classes="DistroSeriesDifferenceNavigation"/>
+ <browser:url
+ for="lp.registry.interfaces.distroseriesdifferencecomment.IDistroSeriesDifferenceComment"
+ path_expression="string:comments/${id}"
+ attribute_to_parent="distro_series_difference"
+ rootsite="mainsite"/>
<browser:menus
classes="
DistroSeriesFacets
=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py 2010-09-28 07:00:56 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py 2010-09-29 09:52:48 +0000
@@ -9,6 +9,7 @@
]
from zope.app.form.browser.itemswidgets import RadioWidget
+from zope.component import getUtility
from zope.interface import (
implements,
Interface,
@@ -19,16 +20,41 @@
SimpleVocabulary,
)
-from canonical.launchpad.webapp import LaunchpadFormView
+from canonical.launchpad.webapp import (
+ LaunchpadFormView,
+ Navigation,
+ stepthrough,
+ )
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.launchpadform import custom_widget
from lp.registry.enum import DistroSeriesDifferenceStatus
+from lp.registry.interfaces.distroseriesdifference import (
+ IDistroSeriesDifference,
+ )
+from lp.registry.interfaces.distroseriesdifferencecomment import (
+ IDistroSeriesDifferenceCommentSource,
+ )
from lp.services.comments.interfaces.conversation import (
IComment,
IConversation,
)
+class DistroSeriesDifferenceNavigation(Navigation):
+ usedfor = IDistroSeriesDifference
+
+ @stepthrough('comments')
+ def traverse_comment(self, id_str):
+ try:
+ id = int(id_str)
+ except ValueError:
+ return None
+
+ return getUtility(
+ IDistroSeriesDifferenceCommentSource).getForDifference(
+ self.context, id)
+
+
class IDistroSeriesDifferenceForm(Interface):
"""An interface used in the browser only for displaying form elements."""
blacklist_options = Choice(vocabulary=SimpleVocabulary((
@@ -86,7 +112,7 @@
comment in self.context.getComments()]
@property
- def show_blacklist_options(self):
+ def show_edit_options(self):
"""Only show the options if an editor requests via JS."""
return self.request.is_ajax and check_permission(
'launchpad.Edit', self.context)
@@ -103,7 +129,6 @@
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
+ self.comment_author = comment.comment_author
+ self.comment_date = comment.comment_date
+ self.body_text = comment.body_text
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-28 07:11:16 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-29 09:52:48 +0000
@@ -130,7 +130,7 @@
self.assertIs(None, ds_diff.source_pub)
self.assertIs(None, view.binary_summaries)
- def test_show_blacklist_options_non_ajax(self):
+ def test_show_edit_options_non_ajax(self):
# Blacklist options are not shown for non-ajax requests.
ds_diff = self.factory.makeDistroSeriesDifference()
@@ -138,9 +138,9 @@
with person_logged_in(ds_diff.owner):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra')
- self.assertFalse(view.show_blacklist_options)
+ self.assertFalse(view.show_edit_options)
- def test_show_blacklist_options_editor(self):
+ def test_show_edit_options_editor(self):
# Blacklist options are shown if requested by an editor via
# ajax.
ds_diff = self.factory.makeDistroSeriesDifference()
@@ -149,16 +149,16 @@
with person_logged_in(ds_diff.owner):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra', request=request)
- self.assertTrue(view.show_blacklist_options)
+ self.assertTrue(view.show_edit_options)
- def test_show_blacklist_options_non_editor(self):
+ def test_show_edit_options_non_editor(self):
# Even with a JS request, non-editors do not see the options.
ds_diff = self.factory.makeDistroSeriesDifference()
request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra', request=request)
- self.assertFalse(view.show_blacklist_options)
+ self.assertFalse(view.show_edit_options)
class DistroSeriesDifferenceTemplateTestCase(TestCaseWithFactory):
@@ -266,7 +266,7 @@
self.assertEqual(
1, len(soup.findAll('div', {'class': 'blacklist-options'})))
- def test_blacklist_options_initial_values_NONE(self):
+ def test_blacklist_options_initial_values_none(self):
ds_diff = self.factory.makeDistroSeriesDifference()
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
@@ -274,7 +274,7 @@
# as the default value for the field.
self.assertEqual('NONE', view.initial_values.get('blacklist_options'))
- def test_blacklist_options_initial_values_CURRENT(self):
+ def test_blacklist_options_initial_values_current(self):
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
@@ -283,7 +283,7 @@
DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
view.initial_values.get('blacklist_options'))
- def test_blacklist_options_initial_values_ALWAYS(self):
+ def test_blacklist_options_initial_values_always(self):
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS)
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2010-09-22 13:45:16 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2010-09-29 09:52:48 +0000
@@ -8,6 +8,7 @@
import transaction
from lazr.restfulclient.errors import Unauthorized
+from storm.store import Store
from zope.component import getUtility
from canonical.testing import AppServerLayer
@@ -17,6 +18,7 @@
IDistroSeriesDifferenceSource,
)
from lp.testing import (
+ person_logged_in,
TestCaseWithFactory,
ws_object,
)
@@ -84,3 +86,16 @@
self.assertEqual(
DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
ds_diff.status)
+
+ def test_addComment(self):
+ # Comments can be added via the API
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ ws_diff = ws_object(self.factory.makeLaunchpadService(
+ ds_diff.owner), ds_diff)
+
+ result = ws_diff.addComment(comment='Hey there')
+
+ self.assertEqual('Hey there', result['body_text'])
+ self.assertTrue(
+ result['resource_type_link'].endswith(
+ '#distro_series_difference_comment'))
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-23 11:17:45 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2010-09-29 09:52:48 +0000
@@ -14,10 +14,12 @@
]
from lazr.restful.declarations import (
+ call_with,
export_as_webservice_entry,
export_write_operation,
exported,
operation_parameters,
+ REQUEST_USER,
)
from lazr.restful.fields import Reference
from zope.interface import Interface
@@ -25,6 +27,7 @@
Bool,
Choice,
Int,
+ Text,
TextLine,
)
@@ -134,7 +137,11 @@
class IDistroSeriesDifferenceEdit(Interface):
"""Difference attributes requiring launchpad.Edit."""
- def addComment(owner, comment):
+ @call_with(commenter=REQUEST_USER)
+ @operation_parameters(
+ comment=Text(title=_("Comment text"), required=True))
+ @export_write_operation()
+ def addComment(commenter, comment):
"""Add a comment on this difference."""
@operation_parameters(
=== modified file 'lib/lp/registry/interfaces/distroseriesdifferencecomment.py'
--- lib/lp/registry/interfaces/distroseriesdifferencecomment.py 2010-08-31 15:56:29 +0000
+++ lib/lp/registry/interfaces/distroseriesdifferencecomment.py 2010-09-29 09:52:48 +0000
@@ -10,9 +10,14 @@
]
+from lazr.restful.declarations import (
+ export_as_webservice_entry,
+ exported,
+ )
from lazr.restful.fields import Reference
from zope.interface import Interface
from zope.schema import (
+ Datetime,
Int,
Text,
)
@@ -26,6 +31,7 @@
class IDistroSeriesDifferenceComment(Interface):
"""A comment for a distroseries difference record."""
+ export_as_webservice_entry()
id = Int(title=_('ID'), required=True, readonly=True)
@@ -38,9 +44,17 @@
IMessage, title=_("Message"), required=True, readonly=True,
description=_("A comment about this difference."))
- comment = Text(
+ body_text = exported(Text(
title=_("Comment text"), readonly=True, description=_(
- "The comment text for the related distro series difference."))
+ "The comment text for the related distro series difference.")))
+
+ comment_author = exported(Reference(
+ # Really IPerson.
+ Interface, title=_("The author of the comment."),
+ readonly=True))
+
+ comment_date = exported(Datetime(
+ title=_('Comment date.'), readonly=True))
class IDistroSeriesDifferenceCommentSource(Interface):
@@ -55,3 +69,6 @@
:param comment: The comment.
:return: A new `DistroSeriesDifferenceComment` object.
"""
+
+ def getForDifference(distro_series_difference, id):
+ """Return the `IDistroSeriesDifferenceComment` with the given id."""
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2010-09-22 11:03:20 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2010-09-29 09:52:48 +0000
@@ -253,10 +253,10 @@
return updated
- def addComment(self, owner, comment):
+ def addComment(self, commenter, comment):
"""See `IDistroSeriesDifference`."""
return getUtility(IDistroSeriesDifferenceCommentSource).new(
- self, owner, comment)
+ self, commenter, comment)
def getComments(self):
"""See `IDistroSeriesDifference`."""
=== modified file 'lib/lp/registry/model/distroseriesdifferencecomment.py'
--- lib/lp/registry/model/distroseriesdifferencecomment.py 2010-08-31 15:56:29 +0000
+++ lib/lp/registry/model/distroseriesdifferencecomment.py 2010-09-29 09:52:48 +0000
@@ -22,7 +22,10 @@
)
from canonical.launchpad.database.message import Message, MessageChunk
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import (
+ IMasterStore,
+ IStore,
+ )
from lp.registry.interfaces.distroseriesdifferencecomment import (
IDistroSeriesDifferenceComment,
IDistroSeriesDifferenceCommentSource,
@@ -46,10 +49,20 @@
message = Reference(message_id, 'Message.id')
@property
- def comment(self):
+ def comment_author(self):
+ """See `IDistroSeriesDifferenceComment`."""
+ return self.message.owner
+
+ @property
+ def body_text(self):
"""See `IDistroSeriesDifferenceComment`."""
return self.message.text_contents
+ @property
+ def comment_date(self):
+ """See `IDistroSeriesDifferenceComment`."""
+ return self.message.datecreated
+
@staticmethod
def new(distro_series_difference, owner, comment):
"""See `IDistroSeriesDifferenceCommentSource`."""
@@ -65,3 +78,13 @@
dsd_comment.message = message
return store.add(dsd_comment)
+
+ @staticmethod
+ def getForDifference(distro_series_difference, id):
+ """See `IDistroSeriesDifferenceCommentSource`."""
+ store = IStore(DistroSeriesDifferenceComment)
+ DSDComment = DistroSeriesDifferenceComment
+ return store.find(
+ DSDComment,
+ DSDComment.distro_series_difference == distro_series_difference,
+ DSDComment.id == id).one()
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-23 11:17:45 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-29 09:52:48 +0000
@@ -68,11 +68,11 @@
<td>
<tal:comment tal:define="comment python:difference.getComments().first();"
tal:condition="comment">
- <span tal:replace="comment/comment/fmt:shorten/50">I'm on this.</span>
+ <span tal:replace="comment/body_text/fmt:shorten/50">I'm on this.</span>
<br /><span class="greyed-out greylink"><span
- tal:replace="comment/message/datecreated/fmt:approximatedate">2005-09-16</span>
+ tal:replace="comment/comment_date/fmt:approximatedate">2005-09-16</span>
by <a tal:replace="structure
- comment/message/owner/fmt:link">joesmith</a></span>
+ comment/comment_author/fmt:link">joesmith</a></span>
</tal:comment>
</td>
</tr>
=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-27 14:18:38 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-29 09:52:48 +0000
@@ -4,7 +4,7 @@
xmlns:i18n="http://xml.zope.org/namespaces/i18n";
i18n:domain="launchpad">
- <tal:show_options condition="view/show_blacklist_options">
+ <tal:show_options condition="view/show_edit_options">
<div class="blacklist-options" style="float:right">
<dl>
<dt>Blacklisted:</dt>
=== modified file 'lib/lp/registry/tests/test_distroseriesdifferencecomment.py'
--- lib/lp/registry/tests/test_distroseriesdifferencecomment.py 2010-08-31 15:56:29 +0000
+++ lib/lp/registry/tests/test_distroseriesdifferencecomment.py 2010-09-29 09:52:48 +0000
@@ -29,12 +29,12 @@
verifyObject(IDistroSeriesDifferenceComment, dsd_comment)
- def test_comment(self):
+ def test_body_text(self):
# The comment attribute returns the text of the comment.
dsd_comment = self.factory.makeDistroSeriesDifferenceComment(
comment="Wait until version 2.3")
- self.assertEqual("Wait until version 2.3", dsd_comment.comment)
+ self.assertEqual("Wait until version 2.3", dsd_comment.body_text)
def test_subject(self):
# The subject of the message is set from the distro series diff
@@ -44,3 +44,27 @@
self.assertEqual(
dsd_comment.distro_series_difference.title,
dsd_comment.message.subject)
+
+ def test_comment_author(self):
+ # The comment author just proxies the author from the message.
+ dsd_comment = self.factory.makeDistroSeriesDifferenceComment()
+
+ self.assertEqual(dsd_comment.message.owner, dsd_comment.comment_author)
+
+ def test_comment_date(self):
+ # The comment date attribute just proxies from the message.
+ dsd_comment = self.factory.makeDistroSeriesDifferenceComment()
+
+ self.assertEqual(
+ dsd_comment.message.datecreated, dsd_comment.comment_date)
+
+ def test_getForDifference(self):
+ # The utility can get comments by id.
+ dsd_comment = self.factory.makeDistroSeriesDifferenceComment()
+ Store.of(dsd_comment).flush()
+
+ comment_src = getUtility(IDistroSeriesDifferenceCommentSource)
+ self.assertEqual(
+ dsd_comment, comment_src.getForDifference(
+ dsd_comment.distro_series_difference, dsd_comment.id))
+
